PE-9205: A drive whose root folder never synced spins forever - #2181
Merged
Merged
Conversation
A drive row whose rootFolderId has no matching folder row cannot be opened at all. watchFolderContents drops the missing folder with a `.where`, so its combined stream never emits and the explorer is stranded on a spinner that nothing retries. It happens when the root folder's own metadata fails to resolve during sync while the drive entity resolves fine - more reachable since sync moved from a four gateway waterfall to two attempts on one. - write a root folder placeholder alongside every drive, in updateUserDrives and writeDriveEntity, mirroring what createDrive already does. rootFolderId is known from the drive entity either way, so the row can always be written - insert with InsertMode.insertOrIgnore so it never overwrites a real root folder. re-running on every sync also heals drives already in this state, and recovery is live: the Drift stream is already watching the row - the placeholder is deliberately not marked isGhost. toEntryCompanion omits that column and the upsert landing real metadata leaves absent columns untouched, so the flag would stick forever. parentFolderId is null rather than self referencing, which is why createGhosts excludes root folders and why this is a placeholder rather than a ghost - _handleFolderNotFound now emits DriveDetailLoadNotFound for a genuinely absent drive and DriveDetailLoadUnsynced otherwise. Both are re-checked by _onSyncCompleted; DriveInitialLoading was a dead end with no retry and nothing that re-triggers it - fold openFolder's duplicate inline onError handler into _handleFolderNotFound No schema change: parentFolderId is nullable and isGhost defaults false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change ensures drives have root-folder entries and updates drive-detail loading for missing local content. Empty drives without a root-folder revision now emit ChangesDrive root availability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DriveDetailCubit
participant DriveDao
participant LocalDatabase
DriveDetailCubit->>DriveDao: Read root-folder revision
DriveDao->>LocalDatabase: Query root-folder data
LocalDatabase-->>DriveDao: Return folder data or missing revision
DriveDao-->>DriveDetailCubit: Return availability result
DriveDetailCubit-->>DriveDetailCubit: Emit load state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/models/daos/drive_dao/drive_dao.dart (1)
445-457: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the drive and root-folder writes atomic.
writeDriveEntitycommits the drive before it inserts the placeholder. If the second insert fails, the database retains a drive with no root folder. A folder watcher can also observe that intermediate state and emitFolderNotFoundInDriveException.Wrap both inserts in one database transaction or batch.
Proposed fix
- await into(drives).insert( - companion, - onConflict: DoUpdate((_) => companion.copyWith(dateCreated: null)), - ); - - await into(folderEntries).insert( - _rootFolderPlaceholder( - driveId: entity.id!, - rootFolderId: entity.rootFolderId!, - name: name, + await db.transaction(() async { + await into(drives).insert( + companion, + onConflict: DoUpdate((_) => companion.copyWith(dateCreated: null)), ); - mode: InsertMode.insertOrIgnore, - ); + + await into(folderEntries).insert( + _rootFolderPlaceholder( + driveId: entity.id!, + rootFolderId: entity.rootFolderId!, + name: name, + ), + mode: InsertMode.insertOrIgnore, + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/models/daos/drive_dao/drive_dao.dart` around lines 445 - 457, Update writeDriveEntity to wrap the drives insert and the _rootFolderPlaceholder insertion into a single database transaction or batch, ensuring both writes commit or roll back together and preventing observers from seeing an incomplete drive state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/blocs/drive_detail/drive_detail_cubit.dart`:
- Around line 818-828: Update _handleFolderNotFound to verify driveId still
equals _driveId after the awaited driveById query and before either emit; return
immediately when the drive is no longer current, while preserving the existing
isClosed and found/not-found behavior.
---
Outside diff comments:
In `@lib/models/daos/drive_dao/drive_dao.dart`:
- Around line 445-457: Update writeDriveEntity to wrap the drives insert and the
_rootFolderPlaceholder insertion into a single database transaction or batch,
ensuring both writes commit or roll back together and preventing observers from
seeing an incomplete drive state.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bf9d20c-c5cc-42c4-8e5b-7daffda3ab21
📒 Files selected for processing (4)
lib/blocs/drive_detail/drive_detail_cubit.dartlib/blocs/drive_detail/drive_detail_state.dartlib/models/daos/drive_dao/drive_dao.darttest/models/daos/drive_dao_test.dart
…9205 Both from CodeRabbit review on #2181. - _handleFolderNotFound checked only isClosed after awaiting the drive query, so a drive switched during that await could emit the previous drive's state onto the new drive's screen. Guard on _driveId, matching the check the success path already does in the same subscription. This mattered more after the previous commit: the emit now carries the stale drive object rather than a generic loading state - writeDriveEntity wrote the drive and its root folder as two separate inserts, leaving a window where an observer could see the drive-without-a-root-folder state the placeholder exists to rule out. One transaction, matching insertDriveRevision and friends. updateUserDrives already got this from db.batch Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP
|
Visit the preview URL for this PR (updated for commit b0bd1c0): https://ardrive-web--pr2181-pe-9205-drive-root-f-92zqutrl.web.app (expires Tue, 18 Aug 2026 22:51:02 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
An empty-looking drive is ambiguous: genuinely empty, or never synced. Telling someone their drive is empty when we have not actually read it reads as "my data is gone". Making the root folder row always exist made this the common case rather than a rare one, because the drive now opens instead of failing, and DriveDetailLoadUnsynced - the existing "Drive Not Synced" screen with its sync action - was only ever reached through the missing-root-folder error that fix removes. Restores that screen from an honest signal instead of an exception: when a drive renders empty at its root folder and no revision exists for that root folder, we have never seen its metadata, so emit DriveDetailLoadUnsynced rather than claiming the drive is empty. The root folder revision is the right signal. A drive created in-app writes one at creation (DriveCreateCubit), and sync writes one when real metadata lands, but a drive discovered by updateUserDrives has only the placeholder row until then. lastBlockHeight cannot answer this: the column defaults to 0, so a freshly created empty drive is indistinguishable from one that has never synced - gating on it would show "Drive Not Synced" to someone who just made a drive. Partial syncs are unchanged: any content at all still renders. Reuses existing copy, so no .arb changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/blocs/drive_detail/drive_detail_cubit.dart`:
- Around line 350-364: Update the empty-drive recovery logic in the drive detail
cubit, including the flows around the root revision handling and the referenced
sync/reload paths, to use the presence of the root-folder revision as the
indicator that root metadata synchronized. Ensure synced empty drives call the
existing openFolder flow instead of remaining in DriveDetailLoadUnsynced, and
remove any reliance on lastBlockHeight for this decision.
- Around line 351-364: Update openFolder to create a new folder-load generation
before its first await, invalidating any prior in-flight load when navigation
changes. Check that generation after every asynchronous boundary, including the
rootFolderRevision lookup, and immediately before each state emission; return
without emitting when it is stale, while preserving the existing isClosed and
_driveId checks.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3c00c7f-28a4-4563-b9f3-6b02345a562d
📒 Files selected for processing (3)
lib/blocs/drive_detail/drive_detail_cubit.dartlib/models/daos/drive_dao/drive_dao.darttest/models/daos/drive_dao_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/models/daos/drive_dao/drive_dao.dart
Both from CodeRabbit review on #2181. - DriveDetailLoadUnsynced is now entered when the root folder has no revision, but all three paths out of it still tested lastBlockHeight. Asymmetric conditions strand the state, and this is the codebase where they come apart: a sync that writes the root revision then fails before advancing the watermark leaves readable metadata behind a drive pinned on "Drive Not Synced", where the sync button only re-emits it. Added _hasRootFolderMetadata and used it at all three sites. lastBlockHeight is still honoured, since a drive synced by an earlier build is synced by definition - cancelling _folderSubscription does not cancel a callback that already began awaiting, so an in-flight load could emit over a newer folder in the same drive, where the _driveId checks cannot see it. Added a _folderLoadGeneration claimed before openFolder's first await and checked after every async boundary that precedes an emit Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP
Three of the four defects found on this branch were async timing or state-machine consistency problems in this cubit, and it had no tests at all. The harness runs a real in-memory database and a real DriveDao and mocks only what sits outside the drive explorer, because every one of those defects lived in how the cubit reacts to what the database streams actually emit. A stubbed DriveDao reproduces none of them. Covers: an unsynced drive is not reported as empty; a locally created empty drive is not reported as unsynced; a drive with contents renders; a drive with an advanced watermark opens; and pressing "Sync now" does not return to the unsynced screen once the root metadata is readable. That last test asserts on the emission sequence rather than the final state, which is the only thing that works here. The folder subscription stays live and writing the revision touches tables it watches, so the stream re-fires and repairs the state regardless of what the direct path emitted - an end-state assertion passes even with the bug present. Both recovery tests were mutation checked against the pre-fix condition; only the sequence one fails, and the weaker test says so in its comment rather than claiming cover it does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP
vilenarios
added a commit
that referenced
this pull request
Aug 12, 2026
…ip (#2182) * fix: guarantee a root folder row so a drive always opens PE-9205 A drive row whose rootFolderId has no matching folder row cannot be opened at all. watchFolderContents drops the missing folder with a `.where`, so its combined stream never emits and the explorer is stranded on a spinner that nothing retries. It happens when the root folder's own metadata fails to resolve during sync while the drive entity resolves fine - more reachable since sync moved from a four gateway waterfall to two attempts on one. - write a root folder placeholder alongside every drive, in updateUserDrives and writeDriveEntity, mirroring what createDrive already does. rootFolderId is known from the drive entity either way, so the row can always be written - insert with InsertMode.insertOrIgnore so it never overwrites a real root folder. re-running on every sync also heals drives already in this state, and recovery is live: the Drift stream is already watching the row - the placeholder is deliberately not marked isGhost. toEntryCompanion omits that column and the upsert landing real metadata leaves absent columns untouched, so the flag would stick forever. parentFolderId is null rather than self referencing, which is why createGhosts excludes root folders and why this is a placeholder rather than a ghost - _handleFolderNotFound now emits DriveDetailLoadNotFound for a genuinely absent drive and DriveDetailLoadUnsynced otherwise. Both are re-checked by _onSyncCompleted; DriveInitialLoading was a dead end with no retry and nothing that re-triggers it - fold openFolder's duplicate inline onError handler into _handleFolderNotFound No schema change: parentFolderId is nullable and isGhost defaults false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP * fix: guard stale drive state and write the root folder atomically PE-9205 Both from CodeRabbit review on #2181. - _handleFolderNotFound checked only isClosed after awaiting the drive query, so a drive switched during that await could emit the previous drive's state onto the new drive's screen. Guard on _driveId, matching the check the success path already does in the same subscription. This mattered more after the previous commit: the emit now carries the stale drive object rather than a generic loading state - writeDriveEntity wrote the drive and its root folder as two separate inserts, leaving a window where an observer could see the drive-without-a-root-folder state the placeholder exists to rule out. One transaction, matching insertDriveRevision and friends. updateUserDrives already got this from db.batch Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP * fix: don't show an unsynced drive as empty PE-9205 An empty-looking drive is ambiguous: genuinely empty, or never synced. Telling someone their drive is empty when we have not actually read it reads as "my data is gone". Making the root folder row always exist made this the common case rather than a rare one, because the drive now opens instead of failing, and DriveDetailLoadUnsynced - the existing "Drive Not Synced" screen with its sync action - was only ever reached through the missing-root-folder error that fix removes. Restores that screen from an honest signal instead of an exception: when a drive renders empty at its root folder and no revision exists for that root folder, we have never seen its metadata, so emit DriveDetailLoadUnsynced rather than claiming the drive is empty. The root folder revision is the right signal. A drive created in-app writes one at creation (DriveCreateCubit), and sync writes one when real metadata lands, but a drive discovered by updateUserDrives has only the placeholder row until then. lastBlockHeight cannot answer this: the column defaults to 0, so a freshly created empty drive is indistinguishable from one that has never synced - gating on it would show "Drive Not Synced" to someone who just made a drive. Partial syncs are unchanged: any content at all still renders. Reuses existing copy, so no .arb changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP * fix: match unsynced recovery to its entry condition PE-9205 Both from CodeRabbit review on #2181. - DriveDetailLoadUnsynced is now entered when the root folder has no revision, but all three paths out of it still tested lastBlockHeight. Asymmetric conditions strand the state, and this is the codebase where they come apart: a sync that writes the root revision then fails before advancing the watermark leaves readable metadata behind a drive pinned on "Drive Not Synced", where the sync button only re-emits it. Added _hasRootFolderMetadata and used it at all three sites. lastBlockHeight is still honoured, since a drive synced by an earlier build is synced by definition - cancelling _folderSubscription does not cancel a callback that already began awaiting, so an in-flight load could emit over a newer folder in the same drive, where the _driveId checks cannot see it. Added a _folderLoadGeneration claimed before openFolder's first await and checked after every async boundary that precedes an emit Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP * test: add a DriveDetailCubit harness against a real database PE-9205 Three of the four defects found on this branch were async timing or state-machine consistency problems in this cubit, and it had no tests at all. The harness runs a real in-memory database and a real DriveDao and mocks only what sits outside the drive explorer, because every one of those defects lived in how the cubit reacts to what the database streams actually emit. A stubbed DriveDao reproduces none of them. Covers: an unsynced drive is not reported as empty; a locally created empty drive is not reported as unsynced; a drive with contents renders; a drive with an advanced watermark opens; and pressing "Sync now" does not return to the unsynced screen once the root metadata is readable. That last test asserts on the emission sequence rather than the final state, which is the only thing that works here. The folder subscription stays live and writing the revision touches tables it watches, so the stream re-fires and repairs the state regardless of what the direct path emitted - an end-state assertion passes even with the bug present. Both recovery tests were mutation checked against the pre-fix condition; only the sequence one fails, and the weaker test says so in its comment rather than claiming cover it does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP * fix: keep the configured gateway for login reads and one 404 retry PE-9205 PE-9203 moved sync onto the configured gateway alone, but classified the read by who asked rather than by how it behaves. getLatestDriveEntityWithId was filed under user-initiated, so login went down the full waterfall: primary once, up to 2 GAR gateways, then arweave.net - plus a Solana RPC for the GAR list, on the startup path PE-9203 exists to keep clear of. - getLatestDriveEntityWithId takes configuredGatewayOnly, and the login path in _validateUser passes it. Attaching a drive by id is untouched: that is one read a user is waiting on and can retry, where breadth is worth its cost - the waterfall's configured gateway now gets one retry, and only on a 404, matching what _syncFetch already does for the same reason. A gateway mid-index answers 404 then 200 a moment later, and leaving on the first 404 is worst for the data most likely to be behind: something just uploaded through Turbo can be on the configured gateway and not yet anywhere else, so falling through reaches gateways further behind it, not ahead. Not extended to timeouts or socket errors, which have already spent their timeout and say the gateway is unwell Both new tests were mutation checked against a single-attempt primary. Auth test stubs gained the new named argument, without which mocktail stops matching the call and returns null. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 25, 2026
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
1 participant
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.
The bug
A drive can render a perpetual spinner in the explorer, with no retry and nothing that re-triggers it. Reached when a drive row exists but no folder row exists for its
rootFolderId.Sync fails to read the root folder's metadata, returns empty bytes rather than an error, the parse exception is swallowed as a warning, the entity never lands — and the watermark advances unconditionally. Pre-existing, but more reachable since sync moved from a four-gateway waterfall to two attempts on one.
Two dead ends, not one
The obvious one is
_handleFolderNotFound'selsebranch emittingDriveInitialLoading(). But that is only reachable viaopenFolder()with a null folderId.The path users actually hit is different.
watchFolderContents' non-null-folderIdbranch (drive_dao.dart:510) filters a missing folder out rather than throwing:combineLatest3then never emits — no error, no data — and the cubit is stranded inDriveDetailLoadInProgress, a bareCircularProgressIndicator. Fixing only_handleFolderNotFoundwould not have fixed the reported bug.Neither state recovers on its own:
_onSyncCompletedre-checks onlyDriveDetailLoadUnsynced.The fix
A drive row should never exist without a folder row for its
rootFolderId.rootFolderIdcomes off the drive entity itself, independent of whether the root folder's metadata ever resolved, so the row can always be written._rootFolderPlaceholderis now written alongside every drive inupdateUserDrives(sync) andwriteDriveEntity(drive-attach) — mirroring whatcreateDrivealready did. Inserted withInsertMode.insertOrIgnore, so it never overwrites a real root folder.Because
updateUserDrivesre-runs on every sync, this heals already-affected drives, not just prevents new ones — and recovery is live, since the Drift stream is already watching that row._handleFolderNotFoundnow emitsDriveDetailLoadNotFoundfor a genuinely absent drive andDriveDetailLoadUnsyncedotherwise, both re-checked by_onSyncCompleted. The duplicate inline handler inopenFolder'sonErrorfolds into it.No schema change:
parentFolderIdis nullable,isGhostdefaults false.Why a placeholder and not a ghost
createGhostsexplicitly excludes root folders (isRootFolderGhost), so ghosts never produce a root row — even when children did sync. The reason is visible in the code: a ghost setsparentFolderId: drive.rootFolderId, which for the root itself is a self-reference.The placeholder is also deliberately not marked
isGhost.FolderRevisionCompanionExtensions.toEntryCompanionomits that column, and drift'sinsertAllOnConflictUpdateleaves absent columns out ofDO UPDATE SET— so a ghost-flagged placeholder would stay a ghost permanently, even after real metadata synced.Downstream check
Breadcrumbs stop at
rootFolderIdand never look it up.getFolderTreecalls.getSingle()on the root and currently throws when it is missing, so drive-size, folder download and manifests are broken by this too — the placeholder fixes those as well.Verification
flutter analyzecleanpackages/ardrive_cryptopasswriteDriveEntitywrites one tooDeliberately out of scope
.where()filter inwatchFolderContentswas not changed to throw. It backs all folder navigation, and wait-for-the-row absorbs legitimate sync races; converting it to an error has a far wider blast radius than this bug justifies. Worth a follow-up.DriveInitialLoadingand itsdrive_detail_page.dart:206branch are now unreachable; left in place to avoid touching thedriveDoingInitialSetupMessageUI.ghost_fixer_cubit.dart:112doesghostFolder.parentFolderId!, which would crash on any null-parent ghost. Unreachable today since root never renders as a row.docs/SYNC_SKIPPED_ENTITY_PERSISTENCE.mdremains the root-cause fix for the silent drop. This is the resilience fix that makes the symptom survivable in the meantime.🤖 Generated with Claude Code
https://claude.ai/code/session_01DnYLXFocWgTt9M2CbGYSUP
Summary by CodeRabbit
Bug Fixes
Tests