[malibio]

Closes #3270

What

A new Plays section in the left navigation, after AI Chats, shown only while the Labs "Playbooks" flag is on (ADR-090). It lists every participating play, core and user-written, by title. Each row shows the play's state (On, Off or Suspended; a suspended row carries its message as a tooltip) and opens the play in PlayNodeViewer. Rows have no switch and write nothing.

How

• stores/plays.svelte.ts: holds which plays are listed. Each row is read from SharedNodeStore, the node the play's viewer shows and writes, so a switch flipped in the viewer shows on the row at once. The listed plays are pinned in the node store while listed. A load only replaces a stored play with a newer version, so it can't put back what a switch just changed.
• Live updates: schedulePlayRefresh (debounced) is called from tauri-sync-listener.ts and browser-sync-service.ts when a play is created, updated or deleted. The reload is what adds an installed play and drops an archived one (the default query applies the participation rule; nothing here reads lifecycle_status).
• Layout: playsExpanded is persisted with the other section states.
• Database switch and daemon reconnect reload the list, as for the other sidebar stores.
• Removed: the Settings → Playbooks section and its settings-categories.ts entry. methodology-service stays, since onboarding still uses it until Make spec, plan and decision core types with checklist criteria and seeded rules; remove the installable methodologies #3542 removes it.
• The Labs card and the playbooksEnabled doc comment now describe what the flag gates.
• playTitle is shared by the viewer and the nav row.

Testing

• bun run test:changed passes (frontend Happy-DOM and Chromium tiers).
• New tests: the plays store, the nav section (gating in expanded and collapsed sidebars, listing, the three row states and tooltip, no switch and no writes, click opens, flag off changes no play), and out-of-band node:created / node:updated / node:deleted in both sync listeners for install, switch off and on, suspension, archive and removal.
• Run in the real app on a scratch database: the seeded play lists as On, opening it and flipping its switch turns the row Off and back On with no reload, and turning the flag off hides the section. Checked in light and dark.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3552 (issue #3270, Plays navigation section)

Review Type: Initial Review

Summary: The change is a clear net improvement and matches ADR-090 and ADR-049. The list store holds ids and derives each row from SharedNodeStore, so the row and the viewer read the same node and no state is synced by effect. Database-switch, reconnect and pin handling are correct as far as I could trace them. No blockers. The findings are two test gaps and a few small robustness points.

Verified by reading the code and by running the six affected test files (208 tests pass). I did not run quality:fix, test:changed or the app itself.

Requirements Check

#	Criterion		Notes
1	"Plays" section, shown only while Labs "Playbooks" is on; expanded state persisted like AI Chats	✅	{#if labsFlags.playbooksEnabled} wraps both the expanded and the collapsed-sidebar form. playsExpanded is in LayoutState, saved and restored.
2	Lists every participating play by title; archived hidden	✅	queryNodes({ nodeType: 'play' }); the Tauri command hardcodes include_archived: false (app-lib/src/commands/nodes.rs:938). No lifecycle_status read in the frontend. The test for it is weak: see finding 1.
3	Row state on / off / suspended, suspendedMessage as tooltip	✅	Reuses the viewer's playState; an off play with a leftover suspension shows "Off" with no tooltip.
4	Rows have no switch and make no writes	✅	A row is one <button> that calls focusOrOpenNode.
5	Clicking a play opens it	✅	focusOrOpenNode(id, { nodeType }).
6	Live updates: install, remove, enable/disable, suspension, with no reload	✅	Tauri: every node:created / node:updated is fetched and maybeRefreshPlays reloads; node:deleted removes the node from the store. Browser mode has one gap: see finding 3.
7	Turning the flag off hides the section and nothing else	✅	The flag is read only in the template; the test asserts no adapter write.
8	Settings → Playbooks and its category entry removed	✅	Component, import, branch and category are gone. methodology-service.ts stays because the onboarding wizard still uses it; that is #3542's scope.
9	Tests: gating, archived excluded, three states, live suspension	✅	All present. The archived one does not prove what its name says: finding 1.
10	quality:fix clean, test:changed passes	Not verified	I ran only the six affected test files.

Code Review Findings

1. 🟡 Important: the "archived ones left out" test asserts nothing about archived plays
📁 packages/desktop-app/src/tests/components/navigation-sidebar-plays.test.ts:165-181

The fixture holds no archived play, so expect(queryByText('Archived play')).toBeNull() passes whatever the code does. The mock's !query.includeArchived branch is never exercised with an archived play either. The only real proof today is toHaveBeenCalledWith({ nodeType: 'play' }) in plays.test.ts:65.

Recommendation: make the mock hold an archived play it returns only when includeArchived is true (for example includeArchived ? [...backend.plays, ...backend.archived] : backend.plays), put "Archived play" in that fixture, and keep the assertion. Then the test fails if the store ever opts in. Principle: a test should fail when the behaviour it names breaks.

2. 🟡 Important: the node:deleted refresh wiring is not covered by any test
📁 packages/desktop-app/src/tests/services/tauri-sync-listener.test.ts ("drops a listed play when it is removed"), packages/desktop-app/src/tests/services/browser-sync-service.test.ts ("drops a listed play when its nodeDeleted event arrives")
📁 packages/desktop-app/src/lib/services/tauri-sync-listener.ts:465-468, browser-sync-service.ts:300-302

Both tests assert on playsData.plays. That list is derived from the node store, and sharedNodeStore.deleteNode empties the row by itself, so both tests pass with the playsData.has(...) → schedulePlayRefresh() lines deleted. Without those lines the deleted id stays in #ids and stays pinned until some later load.

The same applies, harmlessly, to the "switched off" and "suspended" tests: the fetched node's setNode updates the row without the reload. That is the design working, and the create and archive tests do prove maybeRefreshPlays.

Recommendation: in the two delete tests also wait for playsData.has('p1') to be false (or for the queryNodes spy to have been called).

3. 🟢 Suggestion: browser mode misses a play restored after its node was evicted, and the comment says otherwise
📁 packages/desktop-app/src/lib/services/browser-sync-service.ts:273-287, 407-412

nodeUpdated is fetched only when sharedNodeStore.hasNode(id). An archived play is unpinned by the reload that drops it, so it becomes evictable. If it is restored after eviction, the event is skipped and the list does not pick it up until another play event or a reconnect. The comment "or restored one that is not listed" holds only while the node is still cached. Tauri mode fetches every update and is not affected. Browser mode is the dev proxy, and saved queries have the same limit, so this is not a blocker. Either correct the comment or accept the limit in words.

4. 🟢 Suggestion: a list load can raise a conflict notice for the user's own switch (narrow race, not reproduced)
📁 packages/desktop-app/src/lib/stores/plays.svelte.ts:95-102

loadPlays calls setNode(..., { type: 'database' }) when the loaded version is newer. While the viewer's updatePlayNode write is in flight, the local node still carries the old version and hasPending is true. If a load triggered by something else (another play's event, a reconnect) reads the row after the daemon committed but before the write's response lands, decideRemoteUpdate declines it and returns notifyConflict: true, which shows a version-mismatch notice for a write the user made. The daemon suppresses own-write echoes, so the normal event path never hits this; a list query is a new way to reach it. I traced this through remote-update-policy.ts and setNode; I did not reproduce it.

Recommendation: skip nodes with a pending local write in the hydrate loop (the write's own response brings the new version), or note the case in the comment if you judge it too rare to handle.

5. 🟢 Suggestion: overlapping loads can apply out of order
📁 packages/desktop-app/src/lib/stores/plays.svelte.ts:87-108

The generation counter guards database switches only. Two loads in flight in the same database (mount plus reconnect, or debounce plus reconnect) can resolve in reverse order, and the older result then sets #ids and the pins. Node content is protected by the version check, the id list is not. savedQueriesData has the same shape, and the next event corrects it, so this is low risk. A per-load sequence number ("ignore a result older than the last one applied") would close it.

6. 🟢 Suggestion: one place for the Node → PlayNode narrowing
📁 packages/desktop-app/src/lib/stores/plays.svelte.ts:69, packages/desktop-app/src/lib/components/viewers/play-node-viewer.svelte:47-48

Both do isA(node.nodeType, 'play') ? (node as unknown as PlayNode) : undefined. A small guard in play-node-model.ts (say asPlayNode(node)) would keep the double cast in one place, next to playState and playTitle, which this PR already shares. Principle: DRY; project rule that hand-written TypeScript adds guards around the generated shapes.

Nits

• Nit: 📁 packages/desktop-app/src/lib/stores/database.svelte.ts:526-536. The doc comment says the sidebar stores' lists stay until reloadDatabaseStores replaces them. The plays list is now emptied at once (correctly, since clearAll() has removed its nodes). One clause would keep the comment true.
• Nit: 📁 packages/desktop-app/src/lib/components/layout/navigation-sidebar.svelte:760. "No plays yet" also shows while the first load is in flight and after a failed load. AI Chats tracks loading / error; here a failed load is only logged.
• Nit: 📁 packages/desktop-app/src/lib/components/layout/navigation-sidebar.svelte:1404-1416. The --success / --warning contrast figures in app.css are measured on --background; these labels are 11px text on --active-nav-background, and on --border when hovered. Worth one look in both themes. Not measured here.
• Nit: plays are loaded, hydrated and pinned even while the Labs flag is off. The cost is a handful of nodes and it keeps the store free of a flag-driven effect, so I would leave it.

What is done well

• Ids in the list store and nodes in SharedNodeStore give one source of truth for the row and the viewer (ADR-049), with no syncing effect.
• invalidateForDatabaseSwitch runs after sharedNodeStore.clearAll(), which has already cleared the pins, so the unpinAll is safe in both orders. The in-flight-load guard is tested.
• isA everywhere, no literal type comparison, no lifecycle_status read, createLogger, no lint suppression, no compatibility code.
• Security: no new input surface. Titles and messages render as text (no {@html}).
• Performance: one unbounded queryNodes per debounced refresh, same as the other sidebar lists; fine for the number of plays expected.

Recommendation

APPROVE, with findings 1 and 2 worth fixing in this PR since they are small and the acceptance criteria name those tests. Findings 3 to 6 and the nits are optional.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed

Addressed (6 findings, 1 nit)

1. 🟡 Archived test: the mock backend now holds an archived play and returns it only to a query that sets includeArchived, so the test fails if the section ever opts in.
2. 🟡 Delete wiring: both delete tests now wait for playsData.has('p1') to be false, which only the schedulePlayRefresh reload does (the row itself goes with the node).
3. 🟢 Browser-mode restore: the comment now says a restored play is caught only while its node is cached, and otherwise on the next reload.
4. 🟢 Own-write conflict notice: a load leaves alone a play with a local write in flight, so it cannot raise a version-mismatch notice for the user's own switch. Tested.
5. 🟢 Overlapping loads: every load bumps the generation, so only the latest applies. Tested with two loads that resolve in reverse order.
6. 🟢 asPlayNode guard in play-node-model.ts, used by the viewer and the list store.

• Nit: the evictDatabaseCaches comment now notes that the plays list empties at once.

Skipped (3 nits)

• "No plays yet" during the first load or after a failed load. Every database ships a seeded play, so the empty text shows for one round trip at most; a failed load is retried on daemon reconnect, as for saved queries. Worth revisiting if the list gains a create action.
• Contrast of the state labels on the nav background. The labels use the same --success / --warning tokens as the viewer's badge, and the word carries the state, not the colour. Checked by eye in light and dark in the running app; not measured.
• Loading plays while the Labs flag is off. Left as is, as the review suggests: it keeps the store free of a flag-driven effect.

Checks: bun run quality:fix clean; bun run test:changed passes (frontend Happy-DOM and Chromium).

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The review approved with no critical findings. The changes are two test tightenings, two small guards in loadPlays (each with its own test), one extracted helper and comment corrections. No design change.