refactor(#590): make the committed workspace aggregate reactive, retire dashboardTreeRevision - #613
Merged
Conversation
… retire dashboardTreeRevision app.currentWorkspace/app.mainSurface become signal-backed accessor pairs (peeking getter, notifying setter) so a mutation is its own notification — the #426/#427 bug class (a write site forgetting to bump the invalidation counter) becomes structurally impossible. app.committedWorkspace (ReadonlySignal<StoredWorkspaceV5|null>) and app.treeNavigation (a computed structural key over kind/dashboardId/currentMember) are the two tracked reads app-shell.ts's tab-count/tree/Library effects subscribe through, replacing state.dashboardTreeRevision. app.currentWorkspace's setter is asymmetric (no null): a transitional null publication is a named departure operation owned by a new closure-private surface-retirement coordinator in app.ts (retireToWorkspaceLoading/ -Missing/-Failure/-Login), which batches the publication atomically with disposing any live shell so a surface never repaints against transitional state — five independent review passes each found a different call site where this raced disposal, hence one coordinator with exclusive mutation authority rather than five patches. SurfaceStatePort/DashboardApp/TabsApp/ DashboardTreeApp are narrowed to readonly on currentWorkspace (the fourth, DashboardTreeApp, was documented read-only but not type-readonly before this change). app.reloadDashboardRoute() (a post-commit fold-and-reassign that would double-publish once the aggregate is signal-backed) is deleted; afterLibraryChange's Dashboard branch calls the render-only app.renderCurrentSurface() instead. Tests: tests/unit/surface-lifecycle-arch.test.ts (a static-source scan backing the coordinator's compile/scan-layered "no lifecycle bypass" claim) and tests/unit/surface-accessor-contracts.test.ts (@ts-expect-error fixtures for the asymmetric setter and the four narrowed ports) are new, plus a new app.test.ts describe block covering the issue's Tests #1/#3/#5 and the plan's invariant map (delivery-only no-ops, adversarial-id collision-freedom, failure-path status/null ordering, the four-arm live-shell no-repaint sweep, one-commit-exactly-once settlement, and the mixed-snapshot batch-ordering regression). fake-app.ts's makeApp() installs real per-call signals on the returned object (object spread evaluates an accessor pair into a plain value, so the fake needs the same defineProperty treatment createApp() gets natively) for reactivity parity in fixtures. No persisted/schema change, no user-visible behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz
…le wiki roadmap Closes two gaps a review pass found against plan-590.md: - tests/unit/app.test.ts: the plan's §5 "Issue Tests #4" / invariant (b) counting-discipline test was committed to but never added. commitUi (dashboard-tree.ts) and the reactive tree effect converge on the exact same renderDashboardTree function, so a DOM-content assertion alone can't tell "one correct imperative repaint" apart from "one imperative plus one erroneous reactive re-run" — both leave the same final DOM. Added a real-createApp() test asserting (i) a UI-driven chevron-toggle op (through commitUi) produces exactly one tree repaint and zero upper-tab/lower-pane repaints, and (ii) a direct dashboardTreeUi Map mutation with no UI op repaints nothing. Counts through deriveDashboardTree (application/dashboard-tree-model.ts), not renderDashboardTree itself: commitUi calls renderDashboardTree via a same-module binding, invisible to a vi.spyOn namespace patch, while deriveDashboardTree is a genuine cross-module call renderDashboardTree makes exactly once per invocation regardless of caller. Sabotage-verified both arms (a double render call in commitUi; a stray render call after the direct Map mutation) — both correctly fail the new test. - .wiki/Decisions-and-Roadmap.md: reconciled the #593 refactor-umbrella phase list's #590 entry to reflect the work landing on wip/590-reactive-workspace, cross-referencing ADR-0001's new #590 addendum, per CLAUDE.md's "reconcile forward work" discipline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz
BorisTyshkevich
commented
Aug 5, 2026
BorisTyshkevich
left a comment
BorisTyshkevich
left a comment
Collaborator
Author
There was a problem hiding this comment.
ChatGPT review pass 1
Reviewed head SHA: b39b3a7a549285a8691abeab440f9abbe12fde97
No concrete, actionable findings.
I independently checked the complete 26-file PR diff and relevant production paths for the surface-retirement funnel, batch boundaries across projection/navigation and sign-out, all structurally independent currentWorkspace/mainSurface ports, the dashboardTreeUi counting discipline, removal of the counter/funnel compatibility code, and the documented layering/reactivity constraints. The current GitHub Actions run for this head was still in progress when this review was posted; local execution was not possible because the review runtime could not resolve GitHub for cloning.
VERDICT: SHIP
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 & why
Closes #590. Makes the committed workspace aggregate (
app.currentWorkspace) andmain-surface navigation state (
app.mainSurface) reactive at the source, retiringdashboardTreeRevision— the hand-maintained repaint counter that stood in for thatmissing reactivity (#426). Every commit path that mutates the aggregate no longer needs
to remember to bump a counter by hand; the Dashboard tree, upper-role tab counts, and
saved/history panel repaint from a structural dependency instead.
Mechanism
app.currentWorkspace/app.mainSurfacebecome asymmetric accessor pairs (peekinggetter, non-null setter) over two new closure-private signals.
app.committedWorkspace/
app.treeNavigationare computed projections the threeapp-shell.tseffects readdirectly.
createApp(retireToWorkspaceLoading/
retireToWorkspaceMissing/retireToLogin/retireToWorkspaceFailure+rerenderRetiredSurface) is the only place a live shell's teardown is ever sequencedagainst a transitional publication, inside one
batch(). No call site outside ithand-sequences a write next to a dispose call. This replaced five independent,
site-specific patches a first review round kept surfacing one at a time (see review
history below) — the funnel is the root-cause fix, not a sixth ad-hoc one.
dashboardTreeRevision, its five manual bump sites, andinvalidateDashboardTreearefully deleted.
reloadDashboardRouteis deleted (folded into a render-only refresh).dashboardTreeUi(tree expansion/search/scroll/keyboard) stays a plain, deliberatelynon-reactive
Map— now with a dedicated counting-discipline test guarding it, sincethe imperative and reactive tree-repaint paths converge on the same render function and
a DOM-content-only assertion can't tell them apart.
structurally-independent interface re-declaring
currentWorkspace(DashboardTreeApp)needed the same
readonlynarrowing + negative compile fixture as the three ports theplan's own audit named. Re-verified independently during internal review — correct and
complete, no fifth port missed.
Invariant map (plan §9) — enforcement + sabotage result
nulldashboardTreeUistays non-reactivedashboards[]changelibraryQueriesderived at render timebatch()mutateWorkspacependingFocus/pendingScrollTop) notify nothingtreeNavigationexcludes them(null, 'ready')Tests, build, e2e
npm test: 217 files, 7094 tests, coverage 100% statements/functions/lines, 97.14%branches (every touched file at/above its established per-file floor).
npx tsc --noEmit,check:arch,check:schemas,check:examples: clean.npm run build:dist/sql.html2,125,643 bytes.npm run test:e2e(Chromium + Firefox + WebKit): 619 passed, 8 pre-existingtouch-platform skips, 0 failed.
Review history
/shipskill's review loop. Round 1: 5 passes,each surfacing a different call site where a write to the newly-reactive state raced
shell disposal — redirected to the single funnel mechanism above instead of a sixth
per-site patch. Round 2 (post-redirect): 5 more passes, converged to fixture-completeness
gaps only; final gap folded in directly.
Conversations: https://chatgpt.com/c/6a730566-e894-83ed-aa1c-4a25d1a55577 (round 1),
https://chatgpt.com/c/6a7321be-40c8-83eb-989d-c25fdbee7c81 (round 2).
plan — a missing counting-discipline test and an undone wiki reconcile — both fixed and
re-verified.
Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated.wiki/Decisions-and-Roadmap.md's Umbrella: V2 architecture refactor — shell primitives, composition root, state reactivity, transport adapter #593 phase list anddocs/ADR-0001-reactivity.mdaddendum updated in the same change🤖 Generated with Claude Code
https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz