feat: add the vault browser selection model and bound the session check - #1122
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (18)
WalkthroughThe PR adds multi-row file selection with batch actions and updates Core Kit restoration to expose bounded readiness states. Authentication now redirects signed-out deep links, including unresolved restoration cases. Tests cover selection, batch commands, timeout handling, and redirects. ChangesVault browser selection
Authentication readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
FSM1
force-pushed
the
feat/vault-selection-and-bounded-session-check
branch
from
August 6, 2026 23:37
7641407 to
bfc79de
Compare
FSM1
marked this pull request as ready for review
August 7, 2026 00:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/components/file-browser/SelectionActionBar.tsx (1)
4-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace implementation-restating comments.
Lines 5, 7, and 15 repeat prop values and render behavior. Remove these comments unless they record a domain constraint that the type cannot express.
As per coding guidelines, comments must “explain why rather than what” and avoid “duplicated happy-path narration.”
🤖 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 `@apps/web/src/components/file-browser/SelectionActionBar.tsx` around lines 4 - 15, Remove the implementation-restating comments from SelectionActionBarProps, including the descriptions of rows, busy, and the batch commands. Keep the prop declarations and any comments that document domain constraints not expressible by the type.Source: Coding guidelines
🤖 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 `@apps/web/src/components/file-browser/FileBrowserActions.tsx`:
- Around line 67-71: Track the in-progress state of downloadSelection
independently, set it for the full batch duration, and include it with
actions.busy when supplying the action bar’s busy prop. Ensure the state is
cleared after completion, including failures, so duplicate downloads and
conflicting actions remain disabled only while the batch is running.
In `@apps/web/src/hooks/useVaultActions.ts`:
- Around line 34-42: Update perNode to preserve per-node outcomes by returning
accepted and refused node IDs instead of throwing only the first refusal; update
FileBrowserActions.closeOnSuccess to retire only accepted rows, keep refused
rows selected, and surface refused-node failures. Ensure host UI state is
derived from the command result and no refusal is silently discarded.
---
Nitpick comments:
In `@apps/web/src/components/file-browser/SelectionActionBar.tsx`:
- Around line 4-15: Remove the implementation-restating comments from
SelectionActionBarProps, including the descriptions of rows, busy, and the batch
commands. Keep the prop declarations and any comments that document domain
constraints not expressible by the type.
🪄 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: 6227dc76-f329-45c9-ade0-506f54301935
📒 Files selected for processing (21)
apps/web/src/auth/CoreKitProvider.tsxapps/web/src/auth/useAuth.tsapps/web/src/components/file-browser/ConfirmDeleteDialog.tsxapps/web/src/components/file-browser/FileBrowserActions.test.tsxapps/web/src/components/file-browser/FileBrowserActions.tsxapps/web/src/components/file-browser/FileList.tsxapps/web/src/components/file-browser/FileListItem.tsxapps/web/src/components/file-browser/MoveDialog.tsxapps/web/src/components/file-browser/ParentDirRow.tsxapps/web/src/components/file-browser/SelectionActionBar.tsxapps/web/src/hooks/useFolderPicker.test.tsxapps/web/src/hooks/useFolderPicker.tsapps/web/src/hooks/useVaultActions.tsapps/web/src/routes/FilesPage.test.tsxapps/web/src/routes/FilesPage.tsxapps/web/src/styles/vault-actions.cssapps/web/src/test/authFakes.tsxapps/web/src/vault/selection.tstests/web-e2e/README.mdtests/web-e2e/page-objects/files.page.tstests/web-e2e/tests/smoke.spec.ts
💤 Files with no reviewable changes (1)
- tests/web-e2e/page-objects/files.page.ts
FSM1
marked this pull request as draft
August 7, 2026 00:37
Two file-disjoint web items. Selection: a per-row and select-all selection model over the listing, a SelectionActionBar with batch download, move and delete, and one facade command per selected node dispatched through useVaultActions. Selection is held as hex keys and read back through the listing, so a row the engine has retired leaves the selection with it; a routed folder change starts over. The move and delete dialogs now take a set of rows instead of one, which is also how the row menu drives them, and the folder picker excludes every selected subtree rather than a single one. Session check: an unresolved Core Kit restore now has a deadline, so a tab deep-linking to /files while the provider never answers is returned to the front door instead of rendering CHECKING SESSION forever. A restore that lands after the deadline still promotes the tab back to ready, and the front door names why the check could not be made. The e2e assertion weakened by the hang is restored to its unconditional form. Closes #1091 Closes #1087 Part of #642 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WegkkQ3uhNREerTW4MMeY2
…n work - Retire only the rows a command acted on instead of the whole selection, so a command raised from one row's menu leaves an unrelated multi-selection alone; covered by a test that fails under the previous blanket clear. - Drop the useCallback wrappers and the toNodeId indirection the selection hook did not need, and unify its four spellings of all-selected into one. - Hoist the indeterminate ref callback so the header checkbox stops detaching and re-attaching on every list render. - Size the selection column from one CSS variable, and keep the parent row's placeholder a flex item so it inherits the row gap rather than restating it. - Trim the comments that restated the code they sat above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WegkkQ3uhNREerTW4MMeY2
A batch move or delete dispatched every node, then reported one boolean. A partly refused batch therefore retained every dialog row and every selected row, so retrying the still-open dialog re-dispatched the nodes the engine had already accepted. move and remove now report which nodes were accepted, and the browser retires exactly those from both the selection and the dialog while the refusal still reaches the banner. The batch download loop was not reflected in the action bar's busy state, so a second click started a second loop over the same rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WegkkQ3uhNREerTW4MMeY2 Entire-Checkpoint: cbbf6121fada
FSM1
force-pushed
the
feat/vault-selection-and-bounded-session-check
branch
from
August 7, 2026 08:13
1dd5707 to
1bdd2ca
Compare
FSM1
marked this pull request as ready for review
August 7, 2026 08:14
runBatch kept its own copy of the busy/error handling, and cleared busy outside a finally. Routing it through dispatchOrFail leaves one path with one guarantee; the batch adds only the per-node accounting. Entire-Checkpoint: 9a12f38082f4
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This was referenced Aug 8, 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.
Two file-disjoint items on the
#642web track.Closes #1091
Closes #1087
Part of #642
1091 — the vault browser selection model
v2had no selection at all: the per-row context menu covered the four mutations, so nothing inapps/webtracked a set of chosen rows. This adds one.apps/web/src/vault/selection.ts) — per-row toggle, select-all from the list header, clear. It is UI-owned state carrying node ids only, per the blueprint's UI state law: the selection is held as hex keys and the selected rows are read back out of the listing, so a row the engine has retired leaves the selection with it rather than lingering as a phantom in the count. A routed folder change starts over.SelectionActionBar— rebuilt againstListingRow, not v1'sFolderChild. Count, clear, and batch download / move / delete; download only appears when the selection contains a file.useVaultActions.moveand.removenow take a set of nodes and dispatch one facade command per node, sequentially, under a single busy/error envelope. A batch is not a command of its own, and there is no client-side listing patching — the snapshot reports which of them the engine took. Every node is attempted even after one is refused, and the first refusal is what surfaces.useFileDownload.useFolderPickercorrespondingly excludes every selected subtree instead of a single one — a batch move never offers one of the moved folders as its own destination.1087 — an unauthenticated deep link that hangs
FilesPagegated its redirect onisReady, which only became true once Core Kit settled. If Core Kit is unconfigured or its network is unreachable,isReadystayed false forever and an unauthenticated tab rendered// CHECKING SESSION...indefinitely. It was never a security hole — the vault stayed gated onisAuthenticated— but the tab had no recovery path and no error.An
elsebranch cannot catch a promise that never settles, so the fix is a bound rather than a branch:CoreKitProvidergives the mount-time restore a deadline and reports a'checking' | 'ready' | 'unavailable'status. Silence past the deadline becomesunavailable— a verdict, not a stage on the way toready. A restore that lands after the deadline still promotes the tab back toready, so the tab self-heals rather than staying poisoned.useAuthexposesisSignedOut: the check settled signed out, or it could never be made.FilesPagegates the redirect on that instead of onisReady.siweLoginauthenticates an account but does not cold-start a vault, so it would be a path that fails.The two smoke assertions weakened by this bug are restored: the spec now asserts unconditionally that an unauthenticated deep link lands on the front door, and it fails if the redirect regresses.
This overlaps #914 (derive auth state from the engine instead of a tab-local store), which would subsume it — the root cause is that route authorisation depends on a third-party SDK's readiness rather than on the engine's own session state. #914 needs a new facade auth event and is not scheduled; this hang is live today, so it ships as the small fix and #914 can replace the mechanism wholesale later.
Tests
New behaviour, all in gates that already block a merge:
apps/web/src/routes/FilesPage.test.tsx(Test gate) — the stalled-provider case, asserted with the restore stalled rather than merely absent: arestore()that never settles still reaches the front door once the deadline passes, and a check that is genuinely in flight keeps showing progress and does not redirect early. Verified non-vacuous — the stall test fails when the deadline is moved out of reach.apps/web/src/components/file-browser/FileBrowserActions.test.tsx(Test gate) — toggling rows updates the count and clear empties it; select-all takes and gives back the listing; batch delete dispatches onefacade.deleteper selected node; batch move dispatches onefacade.relinkper selected node, all to the picked destination; batch download reads every selected file and leaves folders alone; selection resets across a folder change.tests/web-e2e/tests/smoke.spec.ts(Web E2E Smoke gate) — the restored unconditional redirect assertion.No web unit tests were added for anything the SDK already covers.
Verified at runtime
Driven in Chrome against a live stack — local API, Postgres, the hermetic
/routing/v1record store — with the dev bundle's e2e hook cold-starting a real vault./fileswith no session and no Core Kit config redirects to/and the front door states why (browserabsent,path/).2 files, 1 folder selected, the toolbar exposes download / move / delete / clear, selected rows highlight, and the header checkbox goes indeterminate.move 2 files, 1 folder, offers only the three non-selected folders as destinations (the selected folder is excluded from its own destination list), and on confirm all three nodes leave the source listing and the selection clears.delete 2 files, 1 folder and everything inside?, and confirming empties the folder and clears the selection.delete "alpha" and everything inside it?and leaves the bar readingbeta selected— the narrowed retire, confirmed in the browser and not only in the unit gate.[..]row's icon and a listed row's icon share an x-offset to the pixel, and the header checkbox aligns with the row checkboxes.The stalled Core Kit case is asserted in the unit gate rather than in the browser — reaching it at runtime needs a configured Web3Auth client whose network is then held open, which would put a third-party dependency inside a merge gate.
Gates
pnpm install,pnpm typecheck,pnpm lint,pnpm test,pnpm lint:tracker-refs— all exit 0.Review gates
/simplify(reuse, simplification, efficiency, altitude),/security-review, and/crypto-privacy-reviewall ran onmain...HEAD. The crypto gate ran because the diff touches the session/trust boundary, even though it adds no crypto.Security: no findings. The bound is fail-closed in the only direction it can move —
isSignedOutis a strict conjunct on!isAuthenticated, so it can only ever fire the redirect more often, andunavailablealways carries a null session so it can never satisfyisReady. Vault content is still gated onisAuthenticated, unchanged.Crypto/privacy: no crypto in the diff, no key/seed/login-secret exposure, and the handoff path is untouched. The hex/
Uint8Arraysplit was checked rather than assumed:ListingRow.key(hex) is only ever compared against other hex, and every facade call takesListingRow.id(Uint8Array).Folded in:
useCallbackwrappers and thetoNodeIdindirection the selection hook did not need, and collapsed its four spellings of "all selected" into one.refcallback, so the header checkbox stops detaching and re-attaching on every list render.Pushed back on, with evidence:
[..]row by exactly the row's flexgap, because padding does not participate in it. Re-deriving the gap inside acalc()is the fragile version. Reverted to a flex-item placeholder, which inherits the gap by construction, with the width now coming from the shared variable — which was the reviewer's real complaint.unavailableverdict sologout()stays reachable." Sound in principle, unreachable in fact:unavailableandisAuthenticatedcannot coexist (authentication requiresready, and the status never returns tounavailableafterwards), so no logout can be dispatched in that state. Nulling the session is also the honest encoding — on the deadline path the restore has not settled, which is exactly what the field's own doc saysnullmeans.Deliberately out of scope:
useFileDownload's ticket lifetime, not to selection.React.memoon the list rows. The two reviews pull opposite ways here — memoizing wants stable callback identities, simplifying wants fewer of them — and there is no measured problem on a listing bounded by one folder's direct children. Left alone rather than adding machinery for it.Summary by CodeRabbit
New Features
Bug Fixes
Note
Add vault browser multi-select with batch move, delete, and download actions
selection.tswithuseSelectionanddescribeRows, tracking selected rows per folder and resetting on navigation.SelectionActionBartoolbar for batch Download, Move, Delete, and Clear actions, disabled while an operation is in flight.FileListandFileListItemwith per-row checkboxes and a select-all header; selected rows are visually highlighted without triggering navigation.useVaultActionsto run move and delete in batches, reporting accepted nodes viaBatchOutcomeand retiring only accepted nodes on partial failure.CoreKitProvider; sets status'unavailable'on timeout and exposesisSignedOutfromuseAuthsoFilesPagecan redirect to/without waiting for a ready state.Macroscope summarized 158760c.