feat(web-ui): async notifications — browser + in-app center (#559)#593
Conversation
- AppNotification type + useNotifications hook (localStorage, capped at 20) - NotificationProvider/Context wraps the app via root layout - NotificationCenter (bell + dropdown) mounts in AppSidebar footer - BatchExecutionMonitor dispatches batch.completed on terminal status transition and blocker.created on per-task BLOCKED transition - execution page requests browser Notification permission once - proof page dispatches gate.run.failed for each failed gate when a proof run completes with passed=false - Browser Notification fires only when tab is hidden AND permission is granted; in-app history always records the event Tests: 10 hook tests + 8 NotificationCenter tests, all green. Full suite: 863/863 passing. Build green.
Codex review findings: - Distinguish FAILED/CANCELLED batches from successful completion in both the in-app history (status-specific message, non-success icon) and the browser Notification body. Title relaxed to "Batch finished" so it is accurate for all terminal states. - Scope persisted notifications to the active workspace via a workspace-keyed localStorage entry; switch workspaces → see only that workspace's notifications. Hook subscribes to the existing `workspaceChanged` + `storage` events to reload on transitions. Added tests: - persists batchStatus on the stored notification - stores under workspace-scoped key when a workspace is selected - does not leak notifications across workspaces (switch A → B → A) - does not render success icon for a FAILED batch notification Tests: 867/867 passing (was 863). Build green. Note: P1 finding (notifications only fire while BatchExecutionMonitor is mounted) acknowledged as a Known Limitation — fixing it requires a global background poller architecture beyond this PR's scope. Will be documented in the PR body.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a workspace-scoped in-app notification system: types, a localStorage-backed hook, NotificationContext provider, sidebar NotificationCenter UI and icon mock, integrations that emit notifications from batch/proof flows, tests, and a one-time browser permission request on /execution. ChangesIn-App Notification System
Sequence DiagramsequenceDiagram
participant BatchMonitor as BatchExecutionMonitor
participant ProofPage as ProofPage
participant Context as NotificationContext
participant Hook as useNotifications
participant Storage as localStorage
participant Browser as BrowserNotificationAPI
BatchMonitor->>Context: addNotification({type: "batch.completed" / "blocker.created", ...})
ProofPage->>Context: addNotification({type: "gate.run.failed", ...})
Context->>Hook: call addNotification(...)
Hook->>Storage: persist workspace-scoped notifications (trim to MAX_NOTIFICATIONS)
Hook->>Browser: maybeFireBrowserNotification(...) when tab hidden & permission granted
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review — feat(web-ui): async notifications (#559)This is a well-structured implementation. The three-layer design (hook → context → component) is clean, the workspace-scoping approach is correct, and the test coverage is genuinely solid — 22 tests with meaningful scenarios including the FAILED/CANCELLED regression, workspace isolation, and mutation sanity checks. A few things worth addressing before merge: Medium — Gate re-notification on re-render (proof/page.tsx)The useEffect dispatches one notification per failed gate whenever runState, passed, or gateEntries changes. If gateEntries is a new array reference after each render while the run stays in complete/failed state (common with SWR re-validation or useReducer-derived state), this fires again and appends duplicate gate notifications. A useRef guard fixes it by tracking the last run key that was already notified. Medium — storage event overfires on any cross-tab localStorage write (useNotifications.ts)The storage event listener added with window.addEventListener('storage', handleWorkspaceChange) fires for ANY localStorage mutation from another tab — including the hook's own persist() calls. A second tab adding a notification will trigger handleWorkspaceChange, which re-reads getSelectedWorkspacePath() and overwrites the current in-memory state, potentially causing a flicker or losing an in-flight add. The fix is to filter the storage event by checking if e.key starts with NOTIFICATIONS_STORAGE_KEY_PREFIX before calling handleWorkspaceChange. Minor — Per-gate notification spamA run with 4 failing gates dispatches 4 separate notifications at once. The badge jumps to 4, which is noisy, and MAX_NOTIFICATIONS=20 fills quickly if the user runs proof repeatedly. Consider collapsing into a single "N gates failed" notification per run. Minor — Accessibility gaps (NotificationCenter.tsx)
None are blockers for this PR, but worth a follow-up issue if a11y work is planned. Nit — Redundant constant alias (useNotifications.ts)Two public exports exist for the same value: NOTIFICATIONS_GLOBAL_STORAGE_KEY and NOTIFICATIONS_STORAGE_KEY (back-compat alias). Since the tests are in the same PR, this is a good time to pick one authoritative name and use it everywhere rather than carrying the alias forward. Confirmed good
Overall: The gate re-notification issue is the only one likely to produce visible bugs in normal use. The storage event filtering is a subtle correctness issue. The rest are advisory. Happy to approve once those two are addressed. |
Outcome evidence for "Permission request shown once and respected" — the useEffect on the execution landing page must call Notification.requestPermission when permission is 'default' and not at all when 'granted' or 'denied'.
![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
🤖 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 `@web-ui/src/app/execution/page.tsx`:
- Around line 56-63: The effect that calls Notification.requestPermission()
(useEffect in page.tsx) should be changed to a true one-time gate: persist a
"notificationsAsked" flag (e.g. in localStorage) and only consider calling
Notification.requestPermission() if Notification.permission === 'default' AND
notificationsAsked is not set; show a simple pre-prompt UI element
(banner/button) in the component that explains why notifications are useful and
triggers the permission flow when the user explicitly clicks it (invoke
Notification.requestPermission() from that click handler), then set the
notificationsAsked flag regardless of the outcome and log/catch any errors from
Notification.requestPermission(); update references in useEffect to read the
persisted flag and avoid auto-requesting on mount.
In `@web-ui/src/components/layout/NotificationCenter.tsx`:
- Around line 49-54: The button trigger in NotificationCenter should expose the
popover state to assistive tech: bind aria-expanded to the component's open
state and add aria-controls pointing to the popover element's id; also ensure
the popover DOM element (the panel rendered when open) has that same id (e.g.,
"notification-popover"). Locate the toggle button using setOpen and the open
state in NotificationCenter and add aria-expanded={open} and
aria-controls="notification-popover", and add id="notification-popover" to the
popup/panel element so screen readers can associate the control with the popup.
In `@web-ui/src/hooks/useNotifications.ts`:
- Around line 55-58: The persist function currently calls localStorage.setItem
unguarded and can throw, breaking callers like addNotification, markRead,
markAllRead and clearAll; modify persist(key: string, notifications:
AppNotification[]) to keep the typeof window === 'undefined' guard but wrap the
localStorage.setItem call in a try/catch so write failures are best-effort: on
error catch and quietly handle it (at minimum console.error with context like
the key and error) and do not rethrow, ensuring state updates in
addNotification/markRead/markAllRead/clearAll proceed even if persistence fails.
🪄 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: 115133e4-8c50-4690-8736-d46f387c7adc
📒 Files selected for processing (15)
web-ui/__mocks__/@hugeicons/react.jsweb-ui/__tests__/app/proof/page.test.tsxweb-ui/__tests__/components/layout/AppSidebar.test.tsxweb-ui/src/__tests__/components/layout/NotificationCenter.test.tsxweb-ui/src/__tests__/components/proof/ProofPage.test.tsxweb-ui/src/__tests__/hooks/useNotifications.test.tsweb-ui/src/app/execution/page.tsxweb-ui/src/app/layout.tsxweb-ui/src/app/proof/page.tsxweb-ui/src/components/execution/BatchExecutionMonitor.tsxweb-ui/src/components/layout/AppSidebar.tsxweb-ui/src/components/layout/NotificationCenter.tsxweb-ui/src/contexts/NotificationContext.tsxweb-ui/src/hooks/useNotifications.tsweb-ui/src/types/index.ts
| // Request browser notification permission once on first visit | ||
| useEffect(() => { | ||
| if (typeof window === 'undefined' || typeof Notification === 'undefined') return; | ||
| if (Notification.permission === 'default') { | ||
| // Fire-and-forget — browsers handle the UI; we honor whatever the user picks. | ||
| Notification.requestPermission().catch(() => {}); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of the file with line numbers
sed -n '1,140p' web-ui/src/app/execution/page.tsx | nl -ba | sed -n '40,110p'
# Search for localStorage gating keys related to notification permission
rg -n "notifications\.permission\.requested|Notification\.requestPermission|permission\.requested" web-ui/src -S || true
# Search for other notification permission request logic
rg -n "Notification\.requestPermission" web-ui/src -S || true
# Search for explainer UI or any related UI text about notifications
rg -n "notification(s)?|push|permissions?\"|Notification\.permission" web-ui/src -S || trueRepository: frankbria/codeframe
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' web-ui/src/app/execution/page.tsx | nl -ba | sed -n '40,140p'
rg -n "notifications\.permission\.requested|Notification\.requestPermission|permission\.requested" web-ui/src -S || true
rg -n "Notification\.requestPermission" web-ui/src -S || true
rg -n "Notification\.permission|Notification permission|push notification|browser notification|requestPermission" web-ui/src -S || trueRepository: frankbria/codeframe
Length of output: 106
🏁 Script executed:
sed -n '1,220p' web-ui/src/app/execution/page.tsx | nl -ba | sed -n '40,140p'
rg -n "notifications\.permission\.requested|Notification\.requestPermission|permission\.requested" web-ui/src -S || true
rg -n "Notification\.requestPermission" web-ui/src -S || true
rg -n "Notification\.permission|push notification|browser notification|requestPermission" web-ui/src -S || trueRepository: frankbria/codeframe
Length of output: 790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test -f web-ui/src/app/execution/page.tsx && echo "FOUND page.tsx" || (echo "MISSING page.tsx"; exit 0)
wc -l web-ui/src/app/execution/page.tsx
nl -ba web-ui/src/app/execution/page.tsx | sed -n '45,90p'Repository: frankbria/codeframe
Length of output: 177
🏁 Script executed:
git ls-files "web-ui/src/app/execution/page.tsx" || true
git ls-files "web-ui/src/app/execution/*" || trueRepository: frankbria/codeframe
Length of output: 174
🏁 Script executed:
ls -la
ls -ლა web-ui/src/app/execution || trueRepository: frankbria/codeframe
Length of output: 3319
🏁 Script executed:
ls -la
ls -la web-ui/src/app/execution || trueRepository: frankbria/codeframe
Length of output: 3468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== page.tsx around permission request ==="
cat -n web-ui/src/app/execution/page.tsx | sed -n '40,95p'
echo "=== useNotifications.ts around permission gating ==="
ls -la web-ui/src/hooks/useNotifications.ts
cat -n web-ui/src/hooks/useNotifications.ts | sed -n '1,160p'
echo "=== Search for localStorage gating keys ==="
rg -n "notifications\.permission\.requested|Notification\.requestPermission|localStorage\.getItem\(|localStorage\.setItem\(" web-ui/src -S || true
echo "=== Search for explainer/user-facing text ==="
rg -n "notification(s)? permission|enable notifications|push notification|browser notification|request permission" web-ui/src -S || trueRepository: frankbria/codeframe
Length of output: 11896
Add a true one-time notification permission gate + pre-prompt UX
web-ui/src/app/execution/page.tsx calls Notification.requestPermission() on every mount while Notification.permission === 'default' (no persisted “asked once” gate), so the prompt can reappear on remounts. The permission request also has no user-facing explanation/context immediately before prompting (the notification hook only fires browser notifications after permission is already granted).
💡 Suggested direction
useEffect(() => {
if (typeof window === 'undefined' || typeof Notification === 'undefined') return;
- if (Notification.permission === 'default') {
- // Fire-and-forget — browsers handle the UI; we honor whatever the user picks.
- Notification.requestPermission().catch(() => {});
- }
+ const askedKey = 'notifications.permission.requested.v1';
+ const alreadyAsked = localStorage.getItem(askedKey) === '1';
+ if (!alreadyAsked && Notification.permission === 'default') {
+ // Show app-level explainer UI first, then request on explicit user action.
+ Notification.requestPermission().catch(() => {});
+ localStorage.setItem(askedKey, '1');
+ }
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Request browser notification permission once on first visit | |
| useEffect(() => { | |
| if (typeof window === 'undefined' || typeof Notification === 'undefined') return; | |
| if (Notification.permission === 'default') { | |
| // Fire-and-forget — browsers handle the UI; we honor whatever the user picks. | |
| Notification.requestPermission().catch(() => {}); | |
| } | |
| }, []); | |
| // Request browser notification permission once on first visit | |
| useEffect(() => { | |
| if (typeof window === 'undefined' || typeof Notification === 'undefined') return; | |
| const askedKey = 'notifications.permission.requested.v1'; | |
| const alreadyAsked = localStorage.getItem(askedKey) === '1'; | |
| if (!alreadyAsked && Notification.permission === 'default') { | |
| // Show app-level explainer UI first, then request on explicit user action. | |
| Notification.requestPermission().catch(() => {}); | |
| localStorage.setItem(askedKey, '1'); | |
| } | |
| }, []); |
🤖 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 `@web-ui/src/app/execution/page.tsx` around lines 56 - 63, The effect that
calls Notification.requestPermission() (useEffect in page.tsx) should be changed
to a true one-time gate: persist a "notificationsAsked" flag (e.g. in
localStorage) and only consider calling Notification.requestPermission() if
Notification.permission === 'default' AND notificationsAsked is not set; show a
simple pre-prompt UI element (banner/button) in the component that explains why
notifications are useful and triggers the permission flow when the user
explicitly clicks it (invoke Notification.requestPermission() from that click
handler), then set the notificationsAsked flag regardless of the outcome and
log/catch any errors from Notification.requestPermission(); update references in
useEffect to read the persisted flag and avoid auto-requesting on mount.
Follow-up ReviewThe code is unchanged since my initial review. Confirming status of outstanding items. Still outstanding from previous reviewGate re-notification on re-render (medium) — Storage event over-fires (medium) — Also raised by CodeRabbit (not in my original review)
Permission request fires automatically on Advisory items — unchanged from prior review
StatusThe two medium findings from my original review plus the unguarded |
- a11y: bell button now exposes `aria-expanded`, `aria-controls`, and `aria-haspopup="dialog"`; dropdown gets `id="notification-popover"` + `role="dialog"` so screen readers can associate the control with the popup. Added a test that asserts these attrs flip on open/close. - error handling: wrap `localStorage.setItem` in `persist()` with a try/catch so quota-exceeded or private-mode write failures degrade gracefully — the in-memory state still updates; we log via console.error but do not propagate. Added a test that stubs setItem to throw and verifies the hook continues to function. Skipped (with justification): - "Pre-prompt UI banner before requesting Notification permission" — the approved adapted plan explicitly chose the browser's native prompt over a custom banner. Browsers themselves do not re-prompt once the user has chosen, so a `notificationsAsked` flag is redundant. Tests: 872/872 passing (was 870). Build green.
Phase 11 Demo EvidenceDemo executed via
Review feedback triage (this push, since 56e2b15)CodeRabbit findings:
Codex (cross-family, primary review): addressed in previous commit Test mutation sanity (Phase 7d)4 mutations applied to production code; all caught by tests:
|
|
Round 3 review — test post |
Follow-up Review - Round 3Good progress in this push. The three items raised by CodeRabbit are all addressed:
|
CodeFRAME v2 — Golden Path Contract (CLI-first)This document is the contract for CodeFRAME v2 development. Rule 0 (the only rule that matters):
This applies to both humans and agentic coding assistants. GoalsWhat "done" looks like (Enhanced MVP definition)CodeFRAME can run a complete end-to-end AI-driven development workflow from the CLI on a target repo:
No UI is required. Non-Goals (explicitly forbidden until Golden Path works)Do not build or refactor:
These may be revisited only after Golden Path is working and stable. Golden Path CLI Flow (the only flow that matters)0) Preconditions
1) Initialize a workspaceCommand:
Required behavior:
Artifacts:
2) AI-driven PRD generation and refinementCommands:
Required behavior for
3) Intelligent task generation with dependency analysisCommands:
Required behavior:
4) Batch task execution with orchestrationCommands:
Required behavior for batch execution:
5) Enhanced human-in-loop blocker resolutionCommands:
Required behavior:
6) Integrated Git workflow and PR managementCommands:
Required behavior:
7) Enhanced verification and quality gatesCommands:
Required behavior:
8) Integrated artifact and commit managementCommands:
Required behavior:
9) Comprehensive checkpointing and state managementCommands:
Required behavior:
State Machine (authoritative)Statuses:
Allowed transitions (comprehensive):
The CLI is the authority for transitions. PR Workflow Integration:
Implementation PrinciplesCore-first (no FastAPI in the core)
CLI-first (server optional)
Salvage safely
Keep it runnable
Acceptance Checklist (Enhanced MVP - must pass)Status: 🔄 Enhanced MVP Partially Complete 📊 Current Implementation StatusOverall Assessment: Enhanced MVP is ~60% complete with solid foundation but critical gaps remaining. ✅ Fully Implemented Phases:
|
Still outstanding from my previous reviews1. Gate re-notification on re-render (medium) - still unaddressed proof/page.tsx lines 700-712 - the useEffect that dispatches per-gate notifications includes gateEntries as a dep. If useProofRun returns a new array reference while runState === complete and passed === false (e.g. during background SWR revalidation), every re-render fires all the failed-gate notifications again. Minimal fix: a useRef boolean that fires once per run and resets when a new run starts. Pseudocode: A run ID from useProofRun would be a cleaner key if it is exposed, but a boolean ref with a reset-on-next-run pattern achieves the same result. 2. Storage event over-fires (medium) - still unaddressed useNotifications.ts line 1118 attaches handleWorkspaceChange directly to the storage event with no key filter. The storage event fires for every cross-tab localStorage write, including notification writes. That triggers a getSelectedWorkspacePath() call and overwrites in-memory state from localStorage on every cross-tab notification write. Minimal fix: skip events whose key is not the workspace-selection key. Pseudocode: Adjust the key string to match what workspace-storage uses internally. Minor nit - clearAll uses unnecessary functional form The setNotifications callback inside clearAll ignores prev and just returns []. Moving persist outside the callback and calling setNotifications([]) directly is cleaner. Not a blocker. Status: The a11y and crash-safety fixes are complete and the new tests are solid. The two medium findings from my first and second reviews remain: the gate re-notification guard in proof/page.tsx and the missing storage-event key filter in useNotifications.ts. Both are low-risk in practice but are real correctness gaps worth addressing before merge. |
Follow-up Review (round 3) |
Follow-up Review (round 3)The demo evidence and the latest diff confirm the a11y fixes (aria-expanded, aria-controls, aria-haspopup, role/id on the popover) and the persist() try/catch are both in. Those are resolved — no further concerns there. Two medium findings from prior reviews remain open 1. Gate re-notification on re-render — proof/page.tsx The useEffect still has no guard against repeated dispatch. If gateEntries gets a new array reference while runState equals complete and passed equals false (e.g. SWR re-validation on re-focus), every failed gate notifies again. Minimal fix: track the last-notified run in a ref. 2. Storage event fires on any cross-tab localStorage write — useNotifications.ts handleWorkspaceChange is wired to the generic storage event, which fires for every localStorage mutation from another tab — including the hook's own persist() calls. A second tab adding a notification rewrites the first tab's in-memory state with a stale read. Fix: skip the handler when the mutated key belongs to the notification store. Everything else from prior reviews is either fixed or advisory. These two are the remaining blockers. Ready to approve once they land. |
Follow-up Review (round 3)The demo evidence and the latest diff confirm the a11y fixes (aria-expanded, aria-controls, aria-haspopup, role/id on the popover) and the persist() try/catch are both in. Those are resolved — no further concerns there. Two medium findings from prior reviews remain open 1. Gate re-notification on re-render — proof/page.tsx The useEffect still has no guard against repeated dispatch. If gateEntries gets a new array reference while runState equals complete and passed equals false (e.g. SWR re-validation on re-focus), every failed gate notifies again. Minimal fix: track the last-notified run in a ref. const notifiedRunRef = useRef(null);
useEffect(() => {
if (runState !== 'complete' || passed !== false) return;
const runKey = gateEntries.map(e => e.gate).join(',');
if (notifiedRunRef.current === runKey) return;
notifiedRunRef.current = runKey;
for (const entry of gateEntries) {
if (entry.status === 'failed') {
addNotification({ type: 'gate.run.failed', message: 'Gate run failed: ' + entry.gate, gateName: entry.gate });
}
}
}, [runState, passed, gateEntries, addNotification]);2. Storage event fires on any cross-tab localStorage write — useNotifications.ts handleWorkspaceChange is wired to the generic storage event, which fires for every localStorage mutation from another tab — including the hook's own persist() calls. A second tab adding a notification rewrites the first tab's in-memory state with a stale read. Fix: skip the handler when the mutated key belongs to the notification store. window.addEventListener('storage', (e) => {
if (e.key && e.key.startsWith(NOTIFICATIONS_STORAGE_KEY_PREFIX)) return;
handleWorkspaceChange();
});Everything else from prior reviews is either fixed or advisory. These two are the remaining blockers. Ready to approve once they land. |
claude-review (advisory) finding: the gate-failure useEffect in /proof/page.tsx depends on `addNotification`, which changes reference when the workspace path changes. If the user switches workspaces while a completed proof run is still in the page state, the effect re-fires and dispatches duplicate gate-failure notifications for the previous workspace's run. Fix: track the gateEntries array reference we already dispatched for in a ref, and short-circuit if it matches. Reset the ref when the run state returns to 'idle' (i.e., retry() was called), so a subsequent run can fire notifications normally. No new test added — the existing tests do not exercise the workspace-switch + completed-run combination because it requires both the useProofRun and NotificationContext to be in a specific state, which is more naturally caught by manual / integration testing. The fix is small and the guard is defensive.
Phase 12 — final feedback triage (since 5662667)claude-review (advisory) flagged a recurring medium finding: gate failure re-notification when No new tests added for this fix — it covers a workspace-switch + active-proof-run edge case that requires coordinating two hook states; the defensive guard is small and the existing path is unaffected. Other comments from this round:
All CI checks passing on 5662667. Will re-verify after CI on 4537f2e settles. |
|
Round 4 test |
|
Round 4 review - see below (split into multiple comments to work around shell escaping) |
|
Finding 1: Gate re-notification (proof/page.tsx) - partially addressed The dispatchedGateEntriesRef guard correctly handles the workspace-switch case where addNotification gets a new reference but gateEntries stays the same object. That specific case from prior reviews is fixed. Remaining gap: if gateEntries itself gets a new array reference with the same content (e.g. SWR re-validation while runState is complete and passed is false), the identity check fails and all failed gates notify again. SWR does recreate arrays on revalidation. The content-keyed approach from Round 3 is still the fully-robust fix: use gateEntries.map(e => e.gate).join(',') as the dedup key rather than object identity. Whether to accept the partial fix or go fully robust is a judgment call -- flagging because the underlying finding is still partially open. |
|
Finding 2: Storage event fires on any cross-tab write (useNotifications.ts) - still unaddressed window.addEventListener('storage', handleWorkspaceChange) with no key filter is still present in the current diff. The handler fires on every localStorage mutation from any other tab, including the hook's own persist() calls. A notification write from Tab B triggers handleWorkspaceChange in Tab A, re-reading getSelectedWorkspacePath() and reloading in-memory state from storage -- potentially overwriting a newer in-memory state with a slightly older persisted snapshot. Minimal fix: skip the handler when the mutated key belongs to the notification store (starts with NOTIFICATIONS_STORAGE_KEY_PREFIX). This has not been addressed in any of the prior three rounds. |
|
Round 4 Summary
The storage event key filter is the only fully-unaddressed medium finding. Ready to approve once that lands. |
claude-review round-4 finding: the cross-tab storage listener in useNotifications had no key filter, so it reloaded in-memory state on *every* localStorage mutation from any tab (including unrelated app keys). Mostly a perf concern, but defensive in case of future code that writes keys we shouldn't react to. Fix: only react to (a) localStorage.clear() (key === null), (b) the workspace-path key, or (c) keys with the notification storage prefix. Other tabs' writes are now ignored. Test added: 'ignores cross-tab storage events for unrelated keys' — asserts state reference is unchanged for an unrelated key event, and correctly updated for a notification-key event. Skipped (with justification): - "SWR re-validation could change gateEntries reference" — claim is technically wrong: useProofRun does not use SWR. It uses setInterval polling, and setGateEntries is only called once during completion (then again on retry() which sets runState back to 'idle' — already handled by my dispatched-ref reset). The current ref guard is sufficient. Tests: 873/873 passing (was 872). Build green.
Phase 12 — round-4 feedback triage (since 4537f2e)claude-review round 4 (advisory) raised 2 findings; one is a real defensive improvement, the other is technically incorrect:
Tests: 873/873 passing. Build green. Ready to merge once CI on 16e279e settles. |
Follow-up Review — All Prior Issues ResolvedThe latest commit addresses all three outstanding items from my previous reviews. Confirming closure and noting one minor remaining point. Previously Outstanding — Now FixedGate re-notification on re-render - proof/page.tsx Storage event over-fires - useNotifications.ts localStorage.setItem unguarded - useNotifications.ts persist() Remaining Minor Item (non-blocking)Escape key not handled on the popover — NotificationCenter.tsx The popover uses role="dialog" but does not close on Escape. ARIA authoring guidance for dialogs recommends Escape support. The outside-click handler provides a good escape path for mouse users, but keyboard-only users must tab away. A one-liner keydown listener in the existing useEffect block would cover it. Not blocking — the existing aria-expanded / aria-controls wiring already earns good accessibility marks. Worth a follow-up ticket if full ARIA dialog compliance is desired. Code Quality Notes
Overall: LGTM. The three medium-severity items from prior reviews are resolved. The Escape key gap is cosmetic/accessibility polish, not a correctness issue. Ready to merge. |
claude-review (advisory) noted this as a non-blocking ARIA dialog authoring suggestion. Adding it as a quick UX/a11y polish before merge — keyboard users now have an explicit escape path matching the existing outside-click for mouse users. Test added: 'closes the dropdown when Escape is pressed'. Tests: 874/874 passing.
Follow-up: Last Remaining Item ResolvedMy previous review noted one remaining non-blocking item:
Commit All four items flagged across this review cycle are now fully resolved:
Tests are at 874/874. Build is green. No outstanding concerns. This PR is ready to merge. |
1 participant
Summary
Implements #559 — async notifications so users no longer have to babysit the UI during long batches:
batch.completed), per-task BLOCKED transitions (blocker.created), and PROOF9 gate failures (gate.run.failed).localStorage, scoped per workspace./executionfor the first time (only when permission state isdefault), respectinggrantedordeniedchoices without re-prompting.Acceptance Criteria
npm testandnpm run buildpassTest Plan
useNotifications(13 tests) andNotificationCenter(9 tests, including FAILED-batch regression)npm run buildsucceedsKnown Limitations / Intentionally Deferred
BatchExecutionMonitoris mounted. If the user starts a batch and then navigates away from/execution?batch=<id>before the batch finishes, the polling stops and nobatch.completed/blocker.creatednotification is recorded for the later transition. Cleanly fixing this requires a global background poller (or backend push) that the codebase does not currently have. Tracked as out-of-scope for this PR — a separate issue for a global subscription layer should follow.Implementation Notes
Deviations from the Traycer plan:
gate.run.failedis wired inapp/proof/page.tsx(whereuseProofRunis consumed) instead ofBatchExecutionMonitor/EventStream.blocker.createdis detected via the existing batch poll's per-task status transition (BLOCKED), not via theuseTaskStreamonBlockercallback — the SSE stream is only active when a task row is expanded, so it would miss most blocker events.localStorageentry (codeframe_notifications_<encoded path>); the hook subscribes to the existingworkspaceChanged+ cross-tabstorageevents to reload on workspace switches.Closes #559
Summary by CodeRabbit
New Features
Tests
Documentation