feat: classify recoverable engine refusals and bind the vault browser notice chrome - #1100
Merged
Merged
Conversation
The vault browser gated its listing on `error === null`, so any failure blanked it — a stream-ceiling refusal wiped the rows exactly as a fatal error did. `snapshotStore` classifies on the engine's stable code (fail closed: an unrecognised or absent code is fatal), and the browser renders a recoverable refusal as a retryable notice over the last-known-good rows. Adds the residual notice chrome: an offline banner, a warning-notice store and toast bound to `withheldUpdateEscalation`/`attributableAbuse` as a class distinct from staleness, online/visibility refresh hints, and a manual refresh driven from the status indicator. The snapshot store also takes the focus window back from the folder picker, so a route change during a move cannot strand the engine on the folder the picker opened on. Closes #1069 Closes #808 Closes #1079 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WegkkQ3uhNREerTW4MMeY2
The engine does not implement `manualRefresh` yet and answers `unimplemented`, which the store committed as a fatal snapshot error — clicking the status indicator or coming back online blanked the whole listing. Caught driving the built app against a live stack; the unit fake resolved the command, so no test saw it. The nocache hint is best-effort: only the pull it precedes sets `error`. Adds the regression test and the folder-picker's own focus-handback test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 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 (3)
WalkthroughThe web application now classifies recoverable snapshot failures, supports manual refresh and focus restoration, displays offline and warning status surfaces, and coordinates refreshes after connectivity or visibility changes. ChangesWeb event and recovery surfaces
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant useRefreshHints
participant snapshotStore
participant EngineFacade
Browser->>useRefreshHints: online or visibility event
useRefreshHints->>snapshotStore: refresh() when online and visible
snapshotStore->>EngineFacade: manualRefresh()
snapshotStore->>EngineFacade: pull focused snapshot
EngineFacade-->>snapshotStore: snapshot result
sequenceDiagram
participant Engine
participant snapshotStore
participant notificationStore
participant NotificationToast
Engine->>snapshotStore: emit warning event
snapshotStore->>notificationStore: warn with keyed notice
notificationStore-->>NotificationToast: notify subscribers
NotificationToast->>notificationStore: dismiss notice
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 |
/simplify and /security-review, plus a manual reuse/altitude pass. - Narrow `isRecoverable` to `tooManyStreams`, agreeing with the client's own `isRecoverableEngineError`. `overBudget` was a divergent second answer, and its device-full and staging-limit causes do not clear on their own. - Project the trust warnings from the snapshot store's subscription instead of a second one mounted from `AppShell`. A second subscription opens a render after the engine starts, so every cold-start escalation fell in the gap and read as "nothing was wrong". Drops `useEngineNotices` entirely; the notices now clear with the store that raised them. - Render the offline banner from the engine's rung alone. `navigator.onLine` reports the link, not whether anything answers over it, and the blueprint puts it in the refresh-hint seam, not the ladder. - Stable accessible name on the manual-refresh control; flatten the browser's error branch; drop a single-valued data attribute and its tautological assertion; simplify the notice store's publish path; share the jsdom online/visibility stubs; trim the comments the repo's comment law rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FSM1
marked this pull request as ready for review
August 6, 2026 05:55
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/web/src/hooks/useOnlineStatus.ts (1)
17-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the server snapshot callbacks from these SPA hooks.
apps/web/src/hooks/useOnlineStatus.ts#L17-L22andapps/web/src/hooks/useVisibility.ts#L9-L14both addgetServerSnapshotcallbacks. This Vite application does not render on the server.
apps/web/src/hooks/useOnlineStatus.ts#L17-L22: calluseSyncExternalStore(subscribe, () => navigator.onLine).apps/web/src/hooks/useVisibility.ts#L9-L14: calluseSyncExternalStore(subscribe, () => document.visibilityState === 'visible').Proposed change
- () => navigator.onLine, - () => true + () => navigator.onLine- () => document.visibilityState === 'visible', - () => true + () => document.visibilityState === 'visible'Based on learnings, “the SPA at apps/web uses Vite + React with no SSR. For useSyncExternalStore hooks, do not provide getServerSnapshot unless you are rendering on the server.”
🤖 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/hooks/useOnlineStatus.ts` around lines 17 - 22, Remove the unnecessary server snapshot callback from useSyncExternalStore in apps/web/src/hooks/useOnlineStatus.ts lines 17-22, leaving subscribe and the navigator.onLine client snapshot. Apply the same change in apps/web/src/hooks/useVisibility.ts lines 9-14, retaining only subscribe and the document.visibilityState client snapshot.Source: Learnings
apps/web/src/components/file-browser/FileBrowser.test.tsx (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or replace the setup narration.
listedThenFailedalready states the helper behavior. Keep a comment only if it explains a test constraint or failure-mode rationale. As per coding guidelines, comments must “explain why rather than what.”🤖 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/FileBrowser.test.tsx` at line 21, Remove the setup narration comment above listedThenFailed, since the helper name already describes its behavior. Add a replacement only if it documents a specific test constraint or failure-mode rationale rather than restating what the helper does.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/engine/snapshotStore.ts`:
- Around line 191-195: Update the snapshot store disposal lifecycle to set a
disposed flag and increment generation in dispose(), then guard assertFocus(),
refresh(), pull(), and the coalesced finally callback so no facade calls occur
after disposal. Add a test covering disposal before ackFocus() and verify that
no pull begins.
In `@apps/web/src/engine/useRefreshHints.test.tsx`:
- Around line 25-71: There is no named required CI check executing the web
Vitest suites. Add a required web-unit (or equivalent) job/check in
.github/workflows/ci.yml that runs the relevant tests, including
apps/web/src/engine/useRefreshHints.test.tsx,
apps/web/src/components/layout/OfflineBanner.test.tsx, and
apps/web/src/components/layout/StatusIndicator.test.tsx; the listed test files
require no direct changes.
In `@apps/web/src/hooks/useFolderPicker.test.tsx`:
- Around line 34-45: Strengthen the unmount behavior test around the Probe
rerender: record the engine focus-write count immediately after
store.setFocus(NEXT), then after rerendering without the picker require the
count to increase and verify the newly written value is NEXT. Keep the existing
focus assertions while ensuring the test fails if store.refocus() is removed.
---
Nitpick comments:
In `@apps/web/src/components/file-browser/FileBrowser.test.tsx`:
- Line 21: Remove the setup narration comment above listedThenFailed, since the
helper name already describes its behavior. Add a replacement only if it
documents a specific test constraint or failure-mode rationale rather than
restating what the helper does.
In `@apps/web/src/hooks/useOnlineStatus.ts`:
- Around line 17-22: Remove the unnecessary server snapshot callback from
useSyncExternalStore in apps/web/src/hooks/useOnlineStatus.ts lines 17-22,
leaving subscribe and the navigator.onLine client snapshot. Apply the same
change in apps/web/src/hooks/useVisibility.ts lines 9-14, retaining only
subscribe and the document.visibilityState client snapshot.
🪄 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: a98a3aa6-1016-4126-9cd9-b43c67a75dc4
📒 Files selected for processing (22)
apps/web/src/components/NotificationToast.tsxapps/web/src/components/file-browser/FileBrowser.test.tsxapps/web/src/components/file-browser/FileBrowser.tsxapps/web/src/components/layout/AppShell.tsxapps/web/src/components/layout/OfflineBanner.test.tsxapps/web/src/components/layout/OfflineBanner.tsxapps/web/src/components/layout/StatusIndicator.test.tsxapps/web/src/components/layout/StatusIndicator.tsxapps/web/src/engine/engineNotices.test.tsxapps/web/src/engine/snapshotStore.test.tsapps/web/src/engine/snapshotStore.tsapps/web/src/engine/testFakes.tsapps/web/src/engine/useRefreshHints.test.tsxapps/web/src/engine/useRefreshHints.tsapps/web/src/hooks/useFolderPicker.test.tsxapps/web/src/hooks/useFolderPicker.tsapps/web/src/hooks/useOnlineStatus.tsapps/web/src/hooks/useVisibility.tsapps/web/src/stores/notification.store.test.tsapps/web/src/stores/notification.store.tsapps/web/src/styles/file-browser.cssapps/web/src/styles/layout.css
FSM1
marked this pull request as draft
August 6, 2026 06:00
A logout rebuild disposes the store and the client while the tab stays live, so an in-flight setFocus or manualRefresh could resolve afterwards and drive a pull into a closed facade. Latch a disposed flag and supersede the generation so no continuation reaches it. Also strengthen the folder-picker unmount test: it asserted only the last focus write, which the store's own setFocus already satisfied, so it passed with the picker's refocus removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WegkkQ3uhNREerTW4MMeY2
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.
What this does
Three adjacent vault-browser resilience surfaces, one owner set.
#1069 — classify a recoverable engine refusal
FileBrowser.tsxcomputedsettled = !isLoading && error === nulland gated thelisting, the drop target, and the empty state on it, so any failure blanked
the vault browser — a recoverable stream-ceiling refusal wiped it exactly as a
fatal one did.
snapshotStore.isRecoverable(error)classifies on the engine's stable code —tooManyStreams, the one ceiling that is not a verdict. It fails closed: anunrecognised code and a codeless transport fault are both fatal, so a new
EngineErrorvariant can never render as recoverable without someone addingit.
trustViolationis fatal by construction.role="status"notice over thelast-known-good rows, with a
[retry]control. A fatal one keeps the existingrole="alert"blanking behaviour.SnapshotStore.refresh().#808 — offline / notice chrome bound to engine events
The staleness ladder and the dead-letter notice already landed, so this is only
the residual:
stores/notification.store.ts— the warning-notice surface. Deduplicates bykey and caps its list, so an engine that escalates on every resolve tick
cannot grow it without bound. Cleared with the engine that raised the notices.
components/NotificationToast.tsx— renders them as a distinct warning class(
data-notice-class="warning",role="alert"), dismissed by hand rather thanon a timer: a trust warning that expired unread would read as "nothing was
wrong".
withheldUpdateEscalationandattributableAbuseare projected onto thatsurface from the snapshot store's existing subscription — the stream's only
listener. Never the staleness ladder (
blueprint/web-client.md"Stalenessladder rendering", AGENTS.md rule 6); a test asserts the ladder's rung is
untouched when an escalation lands, and that a
stalenessChangedraises nowarning.
components/layout/OfflineBanner.tsx— the ladder's bottom rung at bannerscale, rendered from the engine's rung alone.
hooks/useOnlineStatus.ts,hooks/useVisibility.ts,engine/useRefreshHints.ts— each edge back into on-screen-and-online drivesone nocache refresh. Only the transition refreshes, never the mount.
facade.manualRefreshgets its firstapps/webconsumer: the footer statusindicator is now a button that drives it.
#1079 — the snapshot store's focus window
useFolderPickerrestoredfacade.setFocus(openOn)on unmount, which could landafter a route change had moved the store's focus — leaving the engine on the
old folder while the store's cached
focussaid the new one, withsetFocus'ssameNodeshort-circuit unable to re-assert. The store now lends the window outand takes it back:
SnapshotStore.refocus()re-asserts the cached focusunconditionally, and the picker calls it instead of restoring the folder it
opened on. The regression test fails against the previous implementation.
Defect found by runtime verification
Driving the built app against a live stack turned up a real regression in the
first commit: the engine does not implement
manualRefreshyet and answersunimplemented, which the store committed as a fatal snapshot error — soclicking the status indicator, or simply coming back online, blanked the whole
listing. The unit fake resolved the command, so no test saw it.
Fixed in the second commit: the nocache hint is best-effort and only the pull it
precedes sets
error. Covered by a regression test that refuses the command andasserts the listing survives.
Review gates
/simplify(four parallel passes: reuse, simplification, efficiency, altitude)and
/security-review, both againstgit diff main...HEAD, folded into thethird commit.
/security-reviewreturned no findings at confidence >= 8. It traced andcleared the fail-closed classification (
Engine::snapshotcannot return a trustverdict at all), the notice surface for key material / PII / storage / logging,
cross-session leakage of the module-level notice store, XSS, and focus-window
misdirection.
Real findings that were folded in:
packages/clientalready shipsisRecoverableEngineError(tooManyStreamsonly). AddingoverBudgetinapps/webgave the repo two answers to one question, andoverBudget'sDeviceFull/StagingLimitcauses do not clear on their own. Narrowed toagree.
useEngineNoticessubscribed fromAppShell, a render after the engine starts and only on authenticated routes,so every escalation emitted during
start-> pointer resolve -> root adoptionfell in the gap and read as "nothing was wrong" — the exact failure the
distinct-warning-class rule exists to prevent. The projection moved into the
snapshot store's own subscription and the hook was deleted.
navigator.onLineintoa rendered rung, so a VPN or captive portal could show OFFLINE over a footer
reading
fresh. The blueprint putsonlinein theRefreshHintSourceseam,not the ladder; the banner now renders from the rung alone.
branch, dropped a single-valued data attribute and its tautological assertion,
simplified the notice store's publish path, shared the jsdom online/visibility
stubs, and trimmed the comments this repo's comment law rejects.
Deliberately not done, because the fix lives outside this PR's owner set:
packages/client'sisRecoverableEngineError— it is notin that package's barrel, so exporting it means editing
packages/client.Both predicates now agree, so nothing is divergent in the meantime.
QueueRefreshHintSource(
packages/client/src/seams/refreshHint.ts), which is the seam that properlyowns them;
EngineClientexposes no hint API today, souseRefreshHintsdrives
store.refresh()instead..text-buttonreset and a shared warning-box class — therepo already carries five copies of each across untouched stylesheets.
renderWithEnginetest helper —testFakes.tsis a.tsfile andwould have to become
.tsx, on a file a sibling PR may also touch.Also worth knowing:
Event::WithheldUpdateEscalationhas no production emitsite in the engine today (it is constructed only in tests), so that branch is
dead until the engine slice lands.
Event::RenewalFailedremains the one engineevent with no rendering home — out of scope here.
Issue-body corrections
Verified against the code, not the issue text.
useStaleness/StatusIndicatortests". There was noStatusIndicatortest file on
main;useStalenesswas only exercised incidentally throughuseSnapshot.test.tsx. AddedStatusIndicator.test.tsx.facade.manualRefreshto be wired "from a control thatexists". It is wired, but the engine command is unimplemented today (proven at
runtime, see above), so the control's useful half is the local re-pull until
the engine slice lands.
3d087d673; the classificationplumbing and the
useDropUpload/UploadListItemprecedent are as described.staleness only —
facade.snapshot(folder)is parameterized and both writepaths target engine-reported node ids.
Files outside the owner set
Two, both additive and both needed by the tests for files I own:
apps/web/src/engine/testFakes.ts—fakeEnginegainedmanualRefresh, arefreshes()counter,refuseRefresh(error), and the sharedsetOnline/setVisiblejsdom stubs.apps/web/src/styles/file-browser.css/layout.css— the styles for the newsurfaces, added to the existing sheets so
main.tsxneeds no new import.Nothing in
FileBrowserActions.tsx,DetailsDialog.tsx,FilePreviewDialog.tsx,useFilePreview.ts, oruseFileDownload.tswas touched.Verification
pnpm typecheck,pnpm lint,pnpm lint:tracker-refs,pnpm testall green(
apps/web: 26 files, 215 tests).Runtime, against a live stack — API on
:3000over Postgres, the hermetic/routing/v1store on:3001, a real WASM engine, a real cold-started vault:<button>with the right rung and a stableRefresh nowtooltip; clicking it drives the manual refresh withoutblanking the listing
fresh— the rung is the one authority (this is the post-gate behaviour; theearlier browser-driven banner was verified before that change)
for computed style and layout by injecting their markup into the live document
— the engine cannot be driven into
tooManyStreams, theofflinerung, or anescalation from the page, since the introspection hook is read-only. Their
behaviour is unit-tested.
Not driven end to end in a browser: the recoverable-refusal path, the offline
rung, and the escalation warning. By hand: open more than
MAX_OPEN_STREAMSfilepreviews at once and confirm the rows stay while the notice appears; pull the
network and wait for the ladder to reach
offlinefor the banner.Closes #1069
Closes #808
Closes #1079
Summary by CodeRabbit
Note
Classify recoverable engine refusals and add vault browser warning notices
isRecoverableto classifytooManyStreamserrors as recoverable;FileBrowsernow keeps the current listing visible and shows a retry notice instead of blanking the screen on these errorsnotificationStorewith deduplication and bounded size;snapshotStoreposts warning notices forwithheldUpdateEscalationandattributableAbuseengine eventsNotificationToastandOfflineBannercomponents rendered fromAppShell; the stalenessStatusIndicatoris now a clickable button that triggersstore.refreshuseRefreshHintshook that callsstore.refreshwhen the app transitions to both online and visible;useFolderPickernow delegates focus restoration tostore.refocussnapshotStoregainsrefocusandrefreshmethods and a disposed guard to prevent late commits after engine teardownMacroscope summarized c0114e8.