Sync: Fixing Teleop submission issue & local storage improvements & DB debug function#42
Merged
1834423612 merged 12 commits intomasterfrom Apr 6, 2026
Merged
Sync: Fixing Teleop submission issue & local storage improvements & DB debug function#421834423612 merged 12 commits intomasterfrom
1834423612 merged 12 commits intomasterfrom
Conversation
… to make sure all teleop data will be storage successfully in local DB
…tabase writes across repositories
…r local database inspection
1834423612
added
bug
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughBumps app version to 1.4.8 (code 42); introduces LocalDatabaseWriteCoordinator to serialize DB writes; extends GameDetails/Comments schema with local-teleop and upload-tracking fields plus migrations; adds LocalDatabaseDebugRepository and settings UI; converts NetworkMonitor sync loop to event-driven signalling with requestImmediateSync(). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant NM as NetworkMonitor
participant SYNC as SyncLoop
participant SRV as RemoteServer
participant DB as LocalDatabase
User->>NM: network capability events
NM->>SYNC: requestImmediateSync() (emit)
Note right of SYNC: waitForNextSyncWindow() suspends until event or timeout
SYNC->>SRV: retryPushUntilSuccess()
SRV-->>SYNC: push result
SYNC->>SRV: retryFetchUntilSuccess()
SRV-->>SYNC: fetched data
SYNC->>DB: apply fetch results
DB->>DB: LocalDatabaseWriteCoordinator.withWriteLock { transaction }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses offline sync reliability (especially Teleop completion/submission), adds local-state persistence for Teleop timing/section, introduces upload-tracking flags for game details + comments, and adds a local database debug/inspection utility in Settings.
Changes:
- Add
pending_upload(game details) anduploaded(comments) flags to track what still needs to be pushed, plus associated migrations/queries. - Persist and restore local Teleop UI state (section + timers) to improve resilience across app restarts/navigation.
- Add a Settings “Local Database Preview” tool backed by a new debug repository, plus a write-coordinator to serialize DB writes.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/sqldelight/migrations/3.sqm | Adds local Teleop persistence columns to gameDetailsEntity. |
| app/src/main/sqldelight/migrations/4.sqm | Adds upload-tracking columns and backfills flags for existing rows. |
| app/src/main/sqldelight/com/team695/scoutifyapp/db/GameDetails.sq | Adds local Teleop columns + pending_upload, filters “completed tasks” by pending upload, adds update query. |
| app/src/main/sqldelight/com/team695/scoutifyapp/db/Comments.sq | Adds uploaded flag and new queries to fetch/mark pending submitted comments. |
| app/src/main/java/com/team695/scoutifyapp/ui/viewModels/SettingsViewModel.kt | New VM for loading/refreshing DB debug snapshots and UI selection state. |
| app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt | Persists local Teleop state + adds explicit flush APIs and more frequent persistence during Teleop. |
| app/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.kt | Replaces placeholder with Settings UI + local database inspection UI. |
| app/src/main/java/com/team695/scoutifyapp/ui/screens/data/PostgameDetails.kt | Flushes form state before returning home when completed. |
| app/src/main/java/com/team695/scoutifyapp/ui/screens/data/EndgameDetails.kt | Ensures Teleop is completed + flushed before transitioning to postgame. |
| app/src/main/java/com/team695/scoutifyapp/ui/screens/data/DataScreen.kt | Flush-on-dispose and updated Teleop timer loop using latest state. |
| app/src/main/java/com/team695/scoutifyapp/Root.kt | Wires LocalDatabaseDebugRepository into navigation root. |
| app/src/main/java/com/team695/scoutifyapp/navigation/AppNav.kt | Injects SettingsViewModel and passes it to the Settings screen. |
| app/src/main/java/com/team695/scoutifyapp/MainActivity.kt | Adds adapters for new local Teleop timing fields and instantiates debug repository. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/UserRepository.kt | Serializes DB writes via LocalDatabaseWriteCoordinator. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/TeamNameRepository.kt | Serializes DB writes via LocalDatabaseWriteCoordinator. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/TaskRepository.kt | Serializes DB writes via LocalDatabaseWriteCoordinator. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt | Serializes DB writes; makes persistTab suspend + write-locked. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/MatchRepository.kt | Serializes match writes via LocalDatabaseWriteCoordinator. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseWriteCoordinator.kt | New global mutex to serialize local DB writes. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.kt | New debug repository to snapshot tables/rows/columns via raw SQL queries. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/GameDetailRepository.kt | Uses pending_upload gating for pushes and computes pending state on writes. |
| app/src/main/java/com/team695/scoutifyapp/data/repository/CommentRepository.kt | Pushes only pending-submitted comments and marks them uploaded. |
| app/src/main/java/com/team695/scoutifyapp/data/api/NetworkMonitor.kt | Adds immediate-sync requests and replaces fixed delay with “wait for sync window.” |
| app/src/main/java/com/team695/scoutifyapp/data/api/model/GameDetails.kt | Adds transient local Teleop fields when mapping from DB. |
| app/build.gradle.kts | Bumps app version name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be342a975e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
app/src/main/java/com/team695/scoutifyapp/navigation/AppNav.kt (1)
170-186: Consider gating DB-debug UI in non-debug builds.If
SettingsViewModelexposes raw local DB inspection, restrict this route behind a debug/build flag to reduce accidental data exposure in release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/team695/scoutifyapp/navigation/AppNav.kt` around lines 170 - 186, The settings route currently mounts a SettingsViewModel and FormScreen that expose local DB debugging; restrict this to debug builds by gating the composable registration (the composable(route = "settings") block) behind a build or feature flag check (e.g., BuildConfig.DEBUG or a runtime debugFeature flag) so that SettingsViewModel (constructed via ViewModelFactory with localDatabaseDebugRepository) and FormScreen are only created in debug/dev builds; in non-debug builds either register a safe placeholder route or omit the route entirely and ensure AuthGuard and navController logic remain unchanged.app/src/main/sqldelight/migrations/4.sqm (1)
15-17: Redundant backfill can be removed for faster migration.
uploadedis added asNOT NULL DEFAULT 0, so existing rows already get0; this update is a no-op.Optional cleanup
-UPDATE commentsEntity -SET uploaded = 0 -WHERE submitted = 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/sqldelight/migrations/4.sqm` around lines 15 - 17, The UPDATE in migration 4.sqm that sets uploaded = 0 on commentsEntity where submitted = 1 is redundant because the uploaded column was added as NOT NULL DEFAULT 0 and existing rows already receive 0; remove that UPDATE statement from the migration to speed it up and avoid a no-op update, leaving only the ALTER/ADD column statements that add uploaded with NOT NULL DEFAULT 0 (reference: commentsEntity table and uploaded column in migration 4.sqm).app/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.kt (1)
51-69: Load row data on demand instead of snapshotting the entire DB.Lines 53-56 materialize every row of every table before the screen can render. That makes each refresh O(total database size) in time and memory even if the user inspects only one table. Consider loading table names/columns first and fetching or paging rows for the selected table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.kt` around lines 51 - 69, The current loadSnapshot implementation eagerly materializes every row by calling loadRows for each table; change it to only collect table metadata (use loadTableNames and loadColumns) and avoid calling loadRows inside loadSnapshot, instead populate LocalDatabaseDebugTable with name, columns and a rowCount obtained via a lightweight count query (implement/getTableRowCount if needed), and add a separate paged loader method (e.g., loadRowsForTable(tableName, limit, offset) or loadRowsOnDemand) that UI can call when a specific table is inspected; update LocalDatabaseDebugSnapshot construction to include only schemaVersion, loadedAtMillis and the lightweight tables list.app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt (1)
89-107: Avoid double-writing the same snapshot.Most mutations now flush immediately and then get written again by the debounced collector 2 seconds later. Consider funnelling both paths through one write queue, or skipping the debounced save when the snapshot already matches the last persisted state.
Also applies to: 146-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt` around lines 89 - 107, The debounced collector on _formState is causing duplicate writes because immediate mutations are also persisted; to fix, centralize writes by introducing a single persistence gate: add a private var lastPersistedState: GameFormState? in the ViewModel and update it inside persistFormState (or its completion callback), then in the .onEach block compare currentFormState to lastPersistedState and return early if they are equal; alternatively replace direct mutation-path calls to persistFormState with a single enqueuePersist(currentFormState) helper that updates lastPersistedState and performs the actual write so both immediate mutations and the debounced collector funnel through the same function (referencing _formState, persistFormState, GameFormState, viewModelScope).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/build.gradle.kts`:
- Around line 30-31: The APK/Bundle metadata was updated to versionName =
"1.4.1" but versionCode remains 1; update the integer assigned to versionCode to
a higher value than the current (e.g., bump versionCode to the next sequential
integer) so Android stores treat this as a new release; modify the versionCode
entry in the same build.gradle.kts where versionName is set (look for the
versionCode and versionName declarations) to the appropriate incremented value.
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/GameDetailRepository.kt`:
- Around line 127-130: The new-record branch in the pendingUpload calculation
wrongly uses hasMeaningfulUploadData(details) so brand-new rows can be created
with pending_upload=false; update the when in GameDetailRepository.kt (the
pendingUpload assignment that checks existingEntity, hasMeaningfulUploadData,
and areUploadRelevantFieldsEqual) so that existingEntity == null returns true
(new rows default to pending) and only clear pending_upload after a successful
push or when the other branches determine it; leave the other branches
(areUploadRelevantFieldsEqual -> existingEntity.pending_upload, else -> true)
unchanged.
- Around line 51-71: The code currently snapshots pendingEntities via
selectCompletedTasks(), sends them with
gameDetailsService.updateGameDetails(...), then unconditionally clears
pending_upload for each task_id inside
LocalDatabaseWriteCoordinator.withWriteLock, which can race with
updateDbFromGameDetails() and drop newer unsent edits; to fix, inside the lock
re-query the current row for each task_id (using
db.gameDetailsQueries.selectByTaskId or equivalent), compare the current row's
fields (the ones included in the payload produced by createGameDetailsFromDb())
to the payload you just sent, and only call
db.gameDetailsQueries.updatePendingUploadState(task_id=...,
pending_upload=false) when the stored row still matches that payload (otherwise
skip clearing); keep the snapshot-to-send flow using createGameDetailsFromDb and
perform comparisons by unique identifiers/fields to ensure you don't clear a
newer pending_upload.
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.kt`:
- Around line 74-139: The repository is currently exposing all non-system tables
and their rows via loadTableNames, loadColumns and loadRows which makes any
tokens/PII visible; add a runtime gate (e.g., a boolean like
enableLocalDatabaseDebugging read from a secure BuildConfig/debug flag or an
internal settings toggle) and enforce it in loadTableNames/loadRows/loadColumns
so no data is returned unless the flag is true, or implement an explicit
allowlist of table names and a column-redaction policy (e.g., redact or omit
values for column names matching /token|secret|password|api_key|email/i) before
constructing LocalDatabaseDebugCell/LocalDatabaseDebugRow; ensure FormScreen’s
rendering will receive only filtered results by changing these functions rather
than the UI.
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt`:
- Around line 158-160: Multiple pit-scouting mutations are split across separate
LocalDatabaseWriteCoordinator.withWriteLock calls (e.g., calls that delete via
db.pitscoutQueries.deletePitscoutByFormId and persistTab()), which allows
interleaving coroutines and inconsistent state; refactor by extracting
non-locking helper variants (e.g., persistTabInternal or persistTabNoLock) that
do the DB mutations without acquiring the mutex, and then wrap each logical
mutation sequence in a single LocalDatabaseWriteCoordinator.withWriteLock {
db.transaction { ... } } block so all pitscoutQueries and pitScoutingTabQueries
updates (and queued-submission row changes) happen atomically; apply the same
pattern to the other mentioned sites (lines around 184-190, 199-205, 318-320,
377-389, 562-579) to avoid split locks.
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/TeamNameRepository.kt`:
- Around line 21-29: The refresh currently calls
LocalDatabaseWriteCoordinator.withWriteLock and performs queries.deleteAll()
followed by multiple queries.insertTeam() calls, which can leave readers seeing
an empty/partial cache; fix this by wrapping the deleteAll + all insertTeam
operations in a single database transaction inside the withWriteLock block
(e.g., use your queries' transaction/withTransaction API or the DB's transaction
method) so that getTeam()/getAllTeams() cannot observe intermediate state—ensure
the transaction scope includes queries.deleteAll() and the entire teams.forEach
{ queries.insertTeam(...) } sequence.
In `@app/src/main/java/com/team695/scoutifyapp/data/repository/UserRepository.kt`:
- Around line 101-110: The logout path currently removes the user and clears
some tables inside LocalDatabaseWriteCoordinator.withWriteLock but can race with
an in-flight sync job that will reinsert old-session rows and it also omits
pit-scouting tables; before acquiring the write lock, cancel and await the
active sync job (the sync coroutine/job managed by your sync service/controller)
to ensure no network worker can write after logout, then inside
LocalDatabaseWriteCoordinator.withWriteLock perform db.userQueries.deleteUser()
and extend the db.transaction to also clear pit-scouting tables (e.g.,
db.pitScoutingQueries.clearAllPitScouting() or the equivalent table methods) in
addition to db.matchQueries.clearAllMatches(), db.taskQueries.clearAllTasks(),
and db.commentsQueries.clearAllComments(), so teardown is atomic and cannot be
repopulated by a concurrent sync.
In `@app/src/main/java/com/team695/scoutifyapp/ui/screens/data/DataScreen.kt`:
- Around line 176-191: The code clamps teleopTotalMilliseconds to switchTime
then calls dataViewModel.updateTime with the full deltaTime, which double-counts
the portion of the frame that was already applied before the boundary; modify
the logic in the frame handler around
currentFormState.teleopTotalMilliseconds/switchTime so that after detecting a
boundary you compute the overflow (overflow = deltaTime - (switchTime -
priorTeleopTotal)) and call dataViewModel.setTeleopSection(...) with
teleopTotalMilliseconds = switchTime, then if nextSection == TeleopSection.ENDED
call dataViewModel.completeTeleop() and set startTime appropriately; otherwise
call dataViewModel.updateTime only with the overflow and loop/iterate to handle
the case where a single frame spans multiple switchTime boundaries until
overflow is consumed. Ensure references to
currentFormState.teleopTotalMilliseconds, switchTime, nextSection,
TeleopSection, dataViewModel.setTeleopSection, dataViewModel.updateTime and
dataViewModel.completeTeleop are updated accordingly.
In `@app/src/main/java/com/team695/scoutifyapp/ui/screens/data/EndgameDetails.kt`:
- Around line 153-155: The submit coroutine calls
dataViewModel.completeTeleop(), dataViewModel.flushNow(), then
switchToPostgame() but does not handle exceptions from flushNow(), so failures
abort navigation silently; wrap the persistence call in try/catch inside the
same coroutine, catch exceptions from dataViewModel.flushNow(), show
user-visible feedback (e.g., a Snackbar/Toast or dialog) and allow retry or
cancel, and only call switchToPostgame() on success; ensure you reference the
same coroutine scope used for submission and keep completeTeleop() as-is before
attempting flushNow().
In
`@app/src/main/java/com/team695/scoutifyapp/ui/screens/data/PostgameDetails.kt`:
- Around line 75-78: Wrap the persistence call in coroutineScope.launch with a
try/catch around dataViewModel.flushNow() so a thrown exception is handled: on
success call returnToHome(), on failure log the exception and surface user
feedback (e.g., Snackbar/Toast via scaffoldState.snackbarHostState.showSnackbar
or a UI error helper) so the user knows the save failed and navigation is
prevented; ensure you reference dataViewModel.flushNow() and returnToHome() in
the catch handling and include a logged error message for diagnostics.
In `@app/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.kt`:
- Around line 672-695: The Columns card (the Column containing the "Columns"
Text and the SelectionContainer that displays columnText with
modifier.verticalScroll(scrollState)) currently has no height cap and can grow
to fill the pane; constrain it by adding a height limit to the card or to the
scrollable Text (e.g., add Modifier.heightIn(max = <sensible dp>) to the outer
Column or to the SelectionContainer/Text modifier) or implement a simple
collapse/expand toggle around that block so long lists don’t consume the entire
view; refer to the Column, SelectionContainer, columnText and scrollState
identifiers when making the change.
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`:
- Around line 34-38: The snapshot type RestoredTeleopState currently omits the
teleopRunning flag so restores cannot distinguish paused vs active timers;
modify RestoredTeleopState to include a Boolean teleopRunning, update the code
that builds/saves the snapshot (where you serialize teleop state) to store the
current teleopRunning, and update the restore logic (the constructor/mapper that
consumes RestoredTeleopState) to rehydrate teleopRunning so updateTime() and
endTeleop() behavior is correct after restart; ensure references to
teleopRunning in updateTime() and endTeleop() remain consistent with the
restored value.
- Around line 358-364: The five MutableStateFlow update blocks (calls to
_formState.update) mix the retry-safe lambda parameter with live reads of
_formState.value; fix each block (the ones updating paths/currentStroke/undoTree
and similar logic) to only reference the lambda parameter (e.g., use it.paths,
it.currentStroke, it.justUndid, it.undoTree) inside the update lambda, compute
any derived values from that parameter (for example newUndoTree = if
(it.justUndid) emptyList() else it.undoTree) and return it.copy(...) using those
values rather than reading _formState.value directly; do this consistently in
the blocks that modify paths/currentStroke/undoTree and the other four update
usages referenced in the comment.
---
Nitpick comments:
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.kt`:
- Around line 51-69: The current loadSnapshot implementation eagerly
materializes every row by calling loadRows for each table; change it to only
collect table metadata (use loadTableNames and loadColumns) and avoid calling
loadRows inside loadSnapshot, instead populate LocalDatabaseDebugTable with
name, columns and a rowCount obtained via a lightweight count query
(implement/getTableRowCount if needed), and add a separate paged loader method
(e.g., loadRowsForTable(tableName, limit, offset) or loadRowsOnDemand) that UI
can call when a specific table is inspected; update LocalDatabaseDebugSnapshot
construction to include only schemaVersion, loadedAtMillis and the lightweight
tables list.
In `@app/src/main/java/com/team695/scoutifyapp/navigation/AppNav.kt`:
- Around line 170-186: The settings route currently mounts a SettingsViewModel
and FormScreen that expose local DB debugging; restrict this to debug builds by
gating the composable registration (the composable(route = "settings") block)
behind a build or feature flag check (e.g., BuildConfig.DEBUG or a runtime
debugFeature flag) so that SettingsViewModel (constructed via ViewModelFactory
with localDatabaseDebugRepository) and FormScreen are only created in debug/dev
builds; in non-debug builds either register a safe placeholder route or omit the
route entirely and ensure AuthGuard and navController logic remain unchanged.
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`:
- Around line 89-107: The debounced collector on _formState is causing duplicate
writes because immediate mutations are also persisted; to fix, centralize writes
by introducing a single persistence gate: add a private var lastPersistedState:
GameFormState? in the ViewModel and update it inside persistFormState (or its
completion callback), then in the .onEach block compare currentFormState to
lastPersistedState and return early if they are equal; alternatively replace
direct mutation-path calls to persistFormState with a single
enqueuePersist(currentFormState) helper that updates lastPersistedState and
performs the actual write so both immediate mutations and the debounced
collector funnel through the same function (referencing _formState,
persistFormState, GameFormState, viewModelScope).
In `@app/src/main/sqldelight/migrations/4.sqm`:
- Around line 15-17: The UPDATE in migration 4.sqm that sets uploaded = 0 on
commentsEntity where submitted = 1 is redundant because the uploaded column was
added as NOT NULL DEFAULT 0 and existing rows already receive 0; remove that
UPDATE statement from the migration to speed it up and avoid a no-op update,
leaving only the ALTER/ADD column statements that add uploaded with NOT NULL
DEFAULT 0 (reference: commentsEntity table and uploaded column in migration
4.sqm).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- [ x ] Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d141b9d-05a4-45bc-bdc2-10428fdec3e0
📒 Files selected for processing (25)
app/build.gradle.ktsapp/src/main/java/com/team695/scoutifyapp/MainActivity.ktapp/src/main/java/com/team695/scoutifyapp/Root.ktapp/src/main/java/com/team695/scoutifyapp/data/api/NetworkMonitor.ktapp/src/main/java/com/team695/scoutifyapp/data/api/model/GameDetails.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/CommentRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/GameDetailRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseWriteCoordinator.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/MatchRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/TaskRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/TeamNameRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/UserRepository.ktapp/src/main/java/com/team695/scoutifyapp/navigation/AppNav.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/DataScreen.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/EndgameDetails.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/PostgameDetails.ktapp/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.ktapp/src/main/java/com/team695/scoutifyapp/ui/viewModels/SettingsViewModel.ktapp/src/main/sqldelight/com/team695/scoutifyapp/db/Comments.sqapp/src/main/sqldelight/com/team695/scoutifyapp/db/GameDetails.sqapp/src/main/sqldelight/migrations/3.sqmapp/src/main/sqldelight/migrations/4.sqm
Owner
Author
|
Implemented a follow-up fix pass for the AI review findings and pushed the changes in commit This pass focused on the issues that were most likely to affect release safety, data integrity, and debug exposure:
Local verification completed successfully:
I did not address the docstring coverage warning in this pass, since it is not release-blocking and does not affect runtime behavior or safety. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt (1)
92-113: Keep the debounced autosave onDispatchers.IO.
launchIn(viewModelScope)collects on the ViewModel's main context, so this save path no longer shares the IO confinement used by the initial load andflushNowAsync(). If either repository call is synchronous, autosave will run DB work on the UI thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt` around lines 92 - 113, The debounced autosave is currently being collected on the ViewModel main dispatcher via launchIn(viewModelScope), which can run repository/DB work on the UI thread; change the collector to run on Dispatchers.IO so persistFormState (and the same IO path as flushNowAsync/load) executes off the main thread—e.g., collect the debounced _formState on IO by launching the flow in viewModelScope + Dispatchers.IO or by using viewModelScope.launch(Dispatchers.IO) { _formState.debounce(...).distinctUntilChanged()....collect { ... persistFormState(...) } } so that persistFormState runs on IO.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt`:
- Around line 202-217: The current code deletes tabs using a stale snapshot
(tabs fetched before acquiring the write lock) which can leave orphaned pitscout
rows; inside the LocalDatabaseWriteCoordinator.withWriteLock { db.transaction {
... } } block, either re-query the current tabs for eventKey (e.g., call
db.pitScoutingTabQueries.getAllTabsForEvent(eventKey).executeAsList() and map to
formId) and then call db.pitscoutQueries.deletePitscoutByFormId(formId) for each
up-to-date tab and deleteTabsForEvent(eventKey), or replace the per-form delete
with a new SQLDelight query like
db.pitscoutQueries.deletePitscoutsByEventId(eventKey) (and add the matching
DELETE query to Pitscout.sq) so deletion is performed atomically and covers any
tabs created after the initial prefetch; ensure deleteLocalFiles(...) still runs
for files of the tabs you decide to remove.
- Around line 330-338: The code currently calls
deleteLocalFiles(preparedTab.images) before the DB transaction so if the
transaction fails the saved fallback preparedTab will reference deleted local
files; move the deletion to occur only after the DB reset and persist
succeed—i.e., remove the call to deleteLocalFiles from before buildClearedTab
and instead invoke deleteLocalFiles(preparedTab.images) after
LocalDatabaseWriteCoordinator.withWriteLock { db.transaction { ...
persistTabInternal(clearedTab) } } completes successfully (inside the successful
runCatching path), referencing preparedTab and clearedTab and leaving the
rollback/fallback path intact.
In `@app/src/main/java/com/team695/scoutifyapp/ui/screens/data/DataScreen.kt`:
- Around line 132-199: The loop reads a stale snapshot from latestFormState (via
rememberUpdatedState) after calling dataViewModel.updateTime,
dataViewModel.setTeleopSection, or dataViewModel.completeTeleop, causing wrong
behavior across multiple section boundaries; fix by introducing local mutable
vars (e.g., localSection and localTotalMilliseconds) initialized from
latestFormState.teleopSection and latestFormState.teleopTotalMilliseconds, use
those local vars for the when/switchTime/timeUntilBoundary calculations, and
update them immediately whenever you call dataViewModel.updateTime(deltaTime),
dataViewModel.setTeleopSection(...), or dataViewModel.completeTeleop() so
subsequent inner-loop iterations operate on the current values rather than the
stale latestFormState snapshot.
In `@app/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.kt`:
- Around line 74-86: The remember block computing filteredRows currently depends
on selectedTable and uiState.rowSearchQuery but not on uiState.loadedRows;
update the remember key list for filteredRows to include uiState.loadedRows so
the computation reruns when rows finish loading (i.e., change
remember(selectedTable, uiState.rowSearchQuery) to remember(selectedTable,
uiState.rowSearchQuery, uiState.loadedRows) or equivalent), keeping the existing
filtering logic using uiState.rowSearchQuery, selectedTable, and
it.searchableText.
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`:
- Around line 322-334: completeTeleop() currently zeroes
teleopCachedMilliseconds which discards any unassigned teleop time; update the
_formState.update call in completeTeleop (and only that method) to stop setting
teleopCachedMilliseconds to 0 so the cached bucket is preserved when
teleopTotalMilliseconds is set to ENDGAME_END_TIME and teleopSection is set to
TeleopSection.ENDED; keep teleopRunning = false and set
gameDetails.teleopCompleted = true, then call flushAfterStateMutation() as
before.
---
Nitpick comments:
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`:
- Around line 92-113: The debounced autosave is currently being collected on the
ViewModel main dispatcher via launchIn(viewModelScope), which can run
repository/DB work on the UI thread; change the collector to run on
Dispatchers.IO so persistFormState (and the same IO path as flushNowAsync/load)
executes off the main thread—e.g., collect the debounced _formState on IO by
launching the flow in viewModelScope + Dispatchers.IO or by using
viewModelScope.launch(Dispatchers.IO) {
_formState.debounce(...).distinctUntilChanged()....collect { ...
persistFormState(...) } } so that persistFormState runs on IO.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb1c49f8-84fa-4d08-bccb-411d7716b642
📒 Files selected for processing (22)
app/build.gradle.ktsapp/src/main/java/com/team695/scoutifyapp/Root.ktapp/src/main/java/com/team695/scoutifyapp/config/DebugConfig.ktapp/src/main/java/com/team695/scoutifyapp/data/api/model/GameDetails.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/GameDetailRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/LocalDatabaseDebugRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/TeamNameRepository.ktapp/src/main/java/com/team695/scoutifyapp/data/repository/UserRepository.ktapp/src/main/java/com/team695/scoutifyapp/navigation/AppNav.ktapp/src/main/java/com/team695/scoutifyapp/ui/components/NavRail.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/DataScreen.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/EndgameDetails.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/PostgameDetails.ktapp/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.ktapp/src/main/java/com/team695/scoutifyapp/ui/viewModels/SettingsViewModel.ktapp/src/main/sqldelight/com/team695/scoutifyapp/db/GameDetails.sqapp/src/main/sqldelight/com/team695/scoutifyapp/db/PitScoutingTab.sqapp/src/main/sqldelight/com/team695/scoutifyapp/db/Pitscout.sqapp/src/main/sqldelight/migrations/4.sqmapp/src/main/sqldelight/migrations/5.sqm
✅ Files skipped from review due to trivial changes (4)
- app/src/main/java/com/team695/scoutifyapp/config/DebugConfig.kt
- app/src/main/sqldelight/migrations/5.sqm
- app/src/main/sqldelight/com/team695/scoutifyapp/db/PitScoutingTab.sq
- app/src/main/sqldelight/migrations/4.sqm
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/main/java/com/team695/scoutifyapp/data/repository/TeamNameRepository.kt
- app/src/main/java/com/team695/scoutifyapp/ui/screens/data/PostgameDetails.kt
- app/src/main/java/com/team695/scoutifyapp/ui/screens/data/EndgameDetails.kt
- app/build.gradle.kts
- app/src/main/java/com/team695/scoutifyapp/navigation/AppNav.kt
- app/src/main/java/com/team695/scoutifyapp/data/api/model/GameDetails.kt
- app/src/main/java/com/team695/scoutifyapp/ui/viewModels/SettingsViewModel.kt
- app/src/main/sqldelight/com/team695/scoutifyapp/db/GameDetails.sq
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt (1)
440-460:⚠️ Potential issue | 🟠 MajorAdd ID requirement for upload success or implement URL-based cleanup fallback.
When
uploadImageIfNeeded()marks an image as uploaded at line 459 usingremoteId.isNotBlank() || remoteUrl.isNotBlank(), it allows URL-only uploads. However,removeImage()only attempts remote deletion whenimage.idis non-blank (line 239), passing only the ID todeletePitImage(). No URL-based delete path exists in the API. After a failed survey submission, a URL-only asset can remain on the server with no cleanup mechanism. Either require both ID and URL before settinguploaded = true, or add a URL-fallback delete endpoint if the API supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt` around lines 440 - 460, The uploadImageIfNeeded() currently sets uploaded = true when either remoteId or remoteUrl is present, which can leave URL-only assets undeletable because removeImage() only deletes when image.id is set; change uploadImageIfNeeded() (the block that returns image.copy(... uploaded = ...)) to require both remoteId.isNotBlank() && remoteUrl.isNotBlank() before marking uploaded, and keep id/url fields populated as before; alternatively, if the server supports URL-based deletes, implement a deletePitImageByUrl(url: String) and update removeImage() (which calls deletePitImage()) to fall back to calling deletePitImageByUrl(image.url) when image.id.isBlank() but image.url.isNotBlank().
🧹 Nitpick comments (1)
app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt (1)
216-235: Minor:dtEndgamecalculation reads state outside the update block.Lines 217-218 read
_formState.value.teleopRunningand_formState.value.teleopTotalMillisecondsoutside the update, then line 223 usesit.teleopCachedMillisecondsinside. If state mutates between the reads and the update,dtEndgamecould be based on staleteleopTotalMilliseconds.In practice this is called rapidly on the UI thread during teleop frames, so the window is small. Consider moving the calculation inside the update for consistency:
♻️ Optional refactor
fun updateTime(deltaTime: Int) { - if (!_formState.value.teleopRunning) { - val dtEndgame = ENDGAME_END_TIME - _formState.value.teleopTotalMilliseconds - _formState.update { + _formState.update { + if (!it.teleopRunning) { + val dtEndgame = ENDGAME_END_TIME - it.teleopTotalMilliseconds it.copy( teleopTotalMilliseconds = ENDGAME_END_TIME, teleopCachedMilliseconds = it.teleopCachedMilliseconds + dtEndgame ) + } else { + it.copy( + teleopTotalMilliseconds = it.teleopTotalMilliseconds + deltaTime, + teleopCachedMilliseconds = it.teleopCachedMilliseconds + deltaTime, + ) } - } else { - _formState.update { - it.copy( - teleopTotalMilliseconds = it.teleopTotalMilliseconds + deltaTime, - teleopCachedMilliseconds = it.teleopCachedMilliseconds + deltaTime, - ) - } } maybeFlushLiveTeleopSnapshot() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt` around lines 216 - 235, The dtEndgame computation currently reads _formState.value.teleopTotalMilliseconds and teleopRunning outside the atomic _formState.update, which can produce stale values; move the dtEndgame calculation into the false branch inside _formState.update so you compute dtEndgame from the updater parameter (it.teleopTotalMilliseconds) and then set teleopTotalMilliseconds = ENDGAME_END_TIME and teleopCachedMilliseconds = it.teleopCachedMilliseconds + dtEndgame, leaving the true branch unchanged (referencing updateTime, _formState.update, teleopTotalMilliseconds, teleopCachedMilliseconds, teleopRunning, and ENDGAME_END_TIME).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt`:
- Around line 329-337: preparedTab is captured before the upload/submit
round-trip so clearing using clearedTab (from buildClearedTab) can overwrite
newer edits or resurrect a deleted tab; modify the
LocalDatabaseWriteCoordinator.withWriteLock/db.transaction block to re-query the
current pitscout row by preparedTab.formId inside the mutex and compare its
formId/updatedAt (or equivalent stamp) to the preparedTab values, and only call
db.pitscoutQueries.deletePitscoutByFormId and persistTabInternal(clearedTab)
when they still match; keep deleteLocalFiles(preparedTab.images) outside or
gated appropriately after confirming the DB change to avoid deleting files for a
newer entry.
- Around line 157-163: The code reads `existing`/`tab` before acquiring
LocalDatabaseWriteCoordinator.withWriteLock and then uses those stale snapshots
inside db.transaction (e.g. calling db.pitscoutQueries.deletePitscoutByFormId
and persistTabInternal) and later deleteLocalFiles(existing.images), which can
reintroduce deleted/updated data; fix by re-loading the current row inside the
lock/transaction (e.g. query the tab/form by id from db within the
withWriteLock/db.transaction block) or implement a locked helper like
mutateTab(tabId) that fetches the up-to-date snapshot and performs all mutations
and subsequent actions (including deciding which files to delete) based on that
fresh in-transaction snapshot instead of the pre-lock `existing`/`tab`; apply
the same change to the other occurrences you noted (around lines indicated:
185-195, 392-416, 586-589).
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`:
- Around line 431-455: The undo/redo methods check collection emptiness outside
the _formState.update lambda causing a race where state.paths.last() or
state.undoTree.last() can throw if the collections change concurrently; fix by
performing the emptiness check and mutation atomically inside the update lambda
in undo() and redo() (e.g., inspect state.paths/undoTree inside the update,
compute newPaths/newUndoTree and a didChange flag there, return state or
modified copy accordingly), and then call flushAfterStateMutation() only when
didChange is true so you don't flush when no mutation occurred; refer to the
functions undo(), redo(), the _formState.update lambda, and fields paths,
undoTree, and justUndid when making the change.
---
Outside diff comments:
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt`:
- Around line 440-460: The uploadImageIfNeeded() currently sets uploaded = true
when either remoteId or remoteUrl is present, which can leave URL-only assets
undeletable because removeImage() only deletes when image.id is set; change
uploadImageIfNeeded() (the block that returns image.copy(... uploaded = ...)) to
require both remoteId.isNotBlank() && remoteUrl.isNotBlank() before marking
uploaded, and keep id/url fields populated as before; alternatively, if the
server supports URL-based deletes, implement a deletePitImageByUrl(url: String)
and update removeImage() (which calls deletePitImage()) to fall back to calling
deletePitImageByUrl(image.url) when image.id.isBlank() but
image.url.isNotBlank().
---
Nitpick comments:
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`:
- Around line 216-235: The dtEndgame computation currently reads
_formState.value.teleopTotalMilliseconds and teleopRunning outside the atomic
_formState.update, which can produce stale values; move the dtEndgame
calculation into the false branch inside _formState.update so you compute
dtEndgame from the updater parameter (it.teleopTotalMilliseconds) and then set
teleopTotalMilliseconds = ENDGAME_END_TIME and teleopCachedMilliseconds =
it.teleopCachedMilliseconds + dtEndgame, leaving the true branch unchanged
(referencing updateTime, _formState.update, teleopTotalMilliseconds,
teleopCachedMilliseconds, teleopRunning, and ENDGAME_END_TIME).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e734e747-f63b-4d35-b799-d31fab81b612
📒 Files selected for processing (6)
app/build.gradle.ktsapp/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.ktapp/src/main/java/com/team695/scoutifyapp/ui/screens/data/DataScreen.ktapp/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.ktapp/src/main/sqldelight/com/team695/scoutifyapp/db/Pitscout.sq
✅ Files skipped from review due to trivial changes (2)
- app/build.gradle.kts
- app/src/main/sqldelight/com/team695/scoutifyapp/db/Pitscout.sq
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/team695/scoutifyapp/ui/screens/FormScreen.kt
Comment on lines
+157
to
163
| LocalDatabaseWriteCoordinator.withWriteLock { | ||
| db.transaction { | ||
| db.pitscoutQueries.deletePitscoutByFormId(existing.formId) | ||
| persistTabInternal(cleared) | ||
| } | ||
| } | ||
| deleteLocalFiles(existing.images) |
Contributor
There was a problem hiding this comment.
These write-locked mutations still run from stale tab snapshots.
existing/tab are resolved before withWriteLock, and the new persistTab() wrapper only locks the final insert. A concurrent edit or delete can still land first and then get overwritten here, which can recreate a deleted tab, drop newer form/image changes, or queue stale submission data. Please re-read the current row inside the locked transaction (or add a locked mutateTab(tabId) helper) and use that snapshot for any later file cleanup.
Also applies to: 185-195, 392-416, 586-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt`
around lines 157 - 163, The code reads `existing`/`tab` before acquiring
LocalDatabaseWriteCoordinator.withWriteLock and then uses those stale snapshots
inside db.transaction (e.g. calling db.pitscoutQueries.deletePitscoutByFormId
and persistTabInternal) and later deleteLocalFiles(existing.images), which can
reintroduce deleted/updated data; fix by re-loading the current row inside the
lock/transaction (e.g. query the tab/form by id from db within the
withWriteLock/db.transaction block) or implement a locked helper like
mutateTab(tabId) that fetches the up-to-date snapshot and performs all mutations
and subsequent actions (including deciding which files to delete) based on that
fresh in-transaction snapshot instead of the pre-lock `existing`/`tab`; apply
the same change to the other occurrences you noted (around lines indicated:
185-195, 392-416, 586-589).
Comment on lines
+329
to
+337
| val clearedTab = buildClearedTab(preparedTab, regenerateFormId = true) | ||
| runCatching { | ||
| clearTab(preparedTab.tabId, regenerateFormId = true) | ||
| LocalDatabaseWriteCoordinator.withWriteLock { | ||
| db.transaction { | ||
| db.pitscoutQueries.deletePitscoutByFormId(preparedTab.formId) | ||
| persistTabInternal(clearedTab) | ||
| } | ||
| } | ||
| deleteLocalFiles(preparedTab.images) |
Contributor
There was a problem hiding this comment.
Don't clear the tab from a stale pre-submit snapshot.
preparedTab comes from Line 313, before the upload + submit round-trip. If the tab is edited or deleted while those calls are in flight, Lines 331-335 will still write clearedTab, which can discard newer local changes or recreate a tab the user already removed. Re-read under the mutex and only reset when the current row still matches the submitted formId/updatedAt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/team695/scoutifyapp/data/repository/PitScoutingRepository.kt`
around lines 329 - 337, preparedTab is captured before the upload/submit
round-trip so clearing using clearedTab (from buildClearedTab) can overwrite
newer edits or resurrect a deleted tab; modify the
LocalDatabaseWriteCoordinator.withWriteLock/db.transaction block to re-query the
current pitscout row by preparedTab.formId inside the mutex and compare its
formId/updatedAt (or equivalent stamp) to the preparedTab values, and only call
db.pitscoutQueries.deletePitscoutByFormId and persistTabInternal(clearedTab)
when they still match; keep deleteLocalFiles(preparedTab.images) outside or
gated appropriately after confirming the DB change to avoid deleting files for a
newer entry.
Comment on lines
431
to
455
| fun undo() { | ||
| if (_formState.value.paths.isNotEmpty()) { | ||
| _formState.update { | ||
| it.copy( | ||
| paths = _formState.value.paths.dropLast(1), | ||
| undoTree = _formState.value.undoTree + listOf(_formState.value.paths.last()), | ||
| justUndid=true | ||
| _formState.update { state -> | ||
| state.copy( | ||
| paths = state.paths.dropLast(1), | ||
| undoTree = state.undoTree + listOf(state.paths.last()), | ||
| justUndid = true | ||
| ) | ||
| } | ||
| flushAfterStateMutation() | ||
| } | ||
| } | ||
|
|
||
| // Redo last undone stroke | ||
| fun redo() { | ||
| if (_formState.value.undoTree.isNotEmpty()) { | ||
| _formState.update { | ||
| it.copy( | ||
| paths = _formState.value.paths + listOf(_formState.value.undoTree.last()), | ||
| undoTree = _formState.value.undoTree.dropLast(1), | ||
| justUndid=false | ||
| _formState.update { state -> | ||
| state.copy( | ||
| paths = state.paths + listOf(state.undoTree.last()), | ||
| undoTree = state.undoTree.dropLast(1), | ||
| justUndid = false | ||
| ) | ||
| } | ||
| flushAfterStateMutation() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition: condition checked outside update block can become stale.
undo() checks _formState.value.paths.isNotEmpty() outside the update lambda, then calls state.paths.last() inside. If another coroutine empties paths between the check and the lambda execution (or during a retry), last() throws NoSuchElementException. Same issue in redo() with undoTree.
🛠️ Proposed fix
fun undo() {
- if (_formState.value.paths.isNotEmpty()) {
- _formState.update { state ->
+ _formState.update { state ->
+ if (state.paths.isEmpty()) return@update state
+ state.copy(
+ paths = state.paths.dropLast(1),
+ undoTree = state.undoTree + state.paths.takeLast(1),
+ justUndid = true
+ )
+ }
+ flushAfterStateMutation()
+}
+
+fun redo() {
+ _formState.update { state ->
+ if (state.undoTree.isEmpty()) return@update state
+ state.copy(
+ paths = state.paths + state.undoTree.takeLast(1),
+ undoTree = state.undoTree.dropLast(1),
+ justUndid = false
+ )
+ }
+ flushAfterStateMutation()
+}Note: This will flush even when no change occurs. If that's undesirable, track a didChange flag inside the update and conditionally flush.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/team695/scoutifyapp/ui/viewModels/DataViewModel.kt`
around lines 431 - 455, The undo/redo methods check collection emptiness outside
the _formState.update lambda causing a race where state.paths.last() or
state.undoTree.last() can throw if the collections change concurrently; fix by
performing the emptiness check and mutation atomically inside the update lambda
in undo() and redo() (e.g., inspect state.paths/undoTree inside the update,
compute newPaths/newUndoTree and a didChange flag there, return state or
modified copy accordingly), and then call flushAfterStateMutation() only when
didChange is true so you don't flush when no mutation occurred; refer to the
functions undo(), redo(), the _formState.update lambda, and fields paths,
undoTree, and justUndid when making the change.
… and AtomicReference for thread safety
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores