diff --git a/apps/streamdeck/README.md b/apps/streamdeck/README.md index f4d6454e4..c5607b60d 100644 --- a/apps/streamdeck/README.md +++ b/apps/streamdeck/README.md @@ -83,12 +83,19 @@ builder (+ its phase and position); PR / Spawn show the item + `i/N`. your browser. - **Spawn from Backlog** — rotate the backlog; **push** spawns a builder for the selected issue (VSCode prompts for the protocol). -- **Diff File Navigator** — rotate = next / previous file in a diff review; - **push** = forward the current file to the builder; **tap the touch strip** = - jump to the first file. -- **Diff Hunk Navigator** — rotate = next / previous change; **push** = forward - the current change to the builder; **tap the touch strip** = jump to the first - change. +- **Review: Files / Headings** — phase-aware coarse review dial. The selected + builder's phase picks the mode. *Diff phase* (implement / review, or blocked at + dev-approval / pr): rotate = next / previous file, **push** = forward the current + file to the builder, **tap** = jump to the first file. *Spec/plan phase* (specify / + plan, or blocked at spec-approval / plan-approval): rotate = next / previous heading + in the artifact canvas, **push** = open the composer at the focused block, **tap** = + jump to the document top. The touch strip always names the live mode + (`Files` / `Headings`). +- **Review: Changes / Blocks** — phase-aware fine review dial, same mode split. + *Diff phase*: rotate = next / previous change, **push** = forward the current change, + **tap** = jump to the first change. *Spec/plan phase*: rotate = next / previous + block, **push** = open the composer, **tap** = walk forward to the next commented + block. The touch strip names the live mode (`Changes` / `Blocks`). - **Scroll** — rotate = scroll the focused editor's viewport (caret stays put); **push** = forward the current selection to the builder. diff --git a/apps/streamdeck/com.cluesmith.codev.sdPlugin/manifest.json b/apps/streamdeck/com.cluesmith.codev.sdPlugin/manifest.json index b03a9fd71..18377e585 100644 --- a/apps/streamdeck/com.cluesmith.codev.sdPlugin/manifest.json +++ b/apps/streamdeck/com.cluesmith.codev.sdPlugin/manifest.json @@ -162,9 +162,9 @@ ] }, { - "Name": "Diff File Navigator", + "Name": "Review: Files / Headings", "UUID": "com.cluesmith.codev.diff-file-nav", - "Tooltip": "Step across files in a builder diff review; press to forward the current file to the builder; tap the touch strip to jump to the first file.", + "Tooltip": "Review the selected builder's work; its phase picks the mode. Diff phase: step across files (press forwards the file to the builder, tap jumps to the first file). Spec/plan phase: step across headings in the artifact canvas (press opens the composer, tap jumps to the top).", "Icon": "icons/list/diff-file-nav", "Controllers": [ "Encoder" @@ -172,9 +172,9 @@ "Encoder": { "layout": "layouts/dial.json", "TriggerDescription": { - "Rotate": "Next / previous file", - "Push": "Forward file to builder", - "Touch": "First file" + "Rotate": "Next / previous file (diff) or heading (spec/plan)", + "Push": "Forward file to builder (diff) or open composer (spec/plan)", + "Touch": "First file (diff) or document top (spec/plan)" } }, "States": [ @@ -184,9 +184,9 @@ ] }, { - "Name": "Diff Hunk Navigator", + "Name": "Review: Changes / Blocks", "UUID": "com.cluesmith.codev.diff-hunk-nav", - "Tooltip": "Step across changes within a diff; press to forward the current change to the builder; tap the touch strip to jump to the first change.", + "Tooltip": "Review the selected builder's work; its phase picks the mode. Diff phase: step across changes within a file (press forwards the change to the builder, tap jumps to the first change). Spec/plan phase: step across blocks in the artifact canvas (press opens the composer, tap walks to the next commented block).", "Icon": "icons/list/diff-hunk-nav", "Controllers": [ "Encoder" @@ -194,9 +194,9 @@ "Encoder": { "layout": "layouts/dial.json", "TriggerDescription": { - "Rotate": "Next / previous change", - "Push": "Forward hunk to builder", - "Touch": "First change" + "Rotate": "Next / previous change (diff) or block (spec/plan)", + "Push": "Forward change to builder (diff) or open composer (spec/plan)", + "Touch": "First change (diff) or next commented block (spec/plan)" } }, "States": [ diff --git a/apps/streamdeck/src/__tests__/actions.test.ts b/apps/streamdeck/src/__tests__/actions.test.ts index 82c4e1f3c..912425a7b 100644 --- a/apps/streamdeck/src/__tests__/actions.test.ts +++ b/apps/streamdeck/src/__tests__/actions.test.ts @@ -13,9 +13,11 @@ import { ZoomNav, zoomInVerb, phaseArtifactVerb, + reviewMode, } from '../actions.js'; type Sent = { verb: string; args: unknown[]; ws?: string }; +type CanvasSent = { command: string; target: { workspace: string; file?: string }; count?: number }; /** A full TowerWorkspace fixture (the sdk type carries proxyUrl + terminals on top of the old WorkspaceSummary). */ function workspace(path: string, name: string, active: boolean): TowerWorkspace { @@ -24,14 +26,21 @@ function workspace(path: string, name: string, active: boolean): TowerWorkspace function makeStore() { const sent: Sent[] = []; + const canvasSent: CanvasSent[] = []; const opened: string[] = []; const getOverview = vi.fn(async (_ws?: string) => null); const listWorkspaces = vi.fn(async () => []); + // A canvas verdict the test can override to exercise the per-code feedback lines. + const canvasResult = { value: { ok: true, target: { viewId: 'v1', file: '/f.md' } } as unknown }; const client = { sendCommand: vi.fn((verb: string, args: unknown[] = [], ws?: string) => { sent.push({ verb, args, ws }); return Promise.resolve({ ok: true, status: 200, data: { ok: true } }); }), + sendCanvasCommand: vi.fn((command: string, target: { workspace: string; file?: string }, options?: { count?: number }) => { + canvasSent.push({ command, target, count: options?.count }); + return Promise.resolve(canvasResult.value); + }), getOverview, listWorkspaces, } as unknown as TowerClient; @@ -46,7 +55,7 @@ function makeStore() { backlog: [{ id: '55', title: 'Add X' }], recentlyClosed: [], } as never; - return { store, sent, opened, getOverview, listWorkspaces }; + return { store, sent, canvasSent, canvasResult, opened, getOverview, listWorkspaces }; } const keyEvent = (settings: Record = {}) => ({ @@ -165,8 +174,9 @@ describe('ApproveGate', () => { }); describe('encoders', () => { - it('DiffFileNav: rotate navigates, press forwards the file, touch jumps to first', async () => { + it('DiffFileNav in diff mode: rotate navigates, press forwards the file, touch jumps to first', async () => { const ctx = makeStore(); + ctx.store.syncToBuilder('pir-2'); // implement phase → diff mode const nav = new DiffFileNav(ctx.store); await nav.onDialRotate(dial(1) as never); // next await nav.onDialRotate(dial(-2) as never); // prev @@ -174,15 +184,101 @@ describe('encoders', () => { await nav.onTouchTap(); // first expect(ctx.sent.map((s) => s.verb)).toEqual(['diff-next-file', 'diff-prev-file', 'forward-file', 'diff-first-file']); expect(ctx.sent.every((s) => s.ws === '/work/alpha')).toBe(true); + expect(ctx.canvasSent).toHaveLength(0); // diff mode never touches the canvas channel }); - it('Diff dials forward their axis on a dial press', async () => { + it('Diff dials forward their axis on a dial press (diff mode)', async () => { const ctx = makeStore(); + ctx.store.syncToBuilder('pir-2'); // implement phase → diff mode await new DiffFileNav(ctx.store).onDialDown(); await new DiffHunkNav(ctx.store).onDialDown(); expect(ctx.sent.map((s) => s.verb)).toEqual(['forward-file', 'forward-hunk']); }); + it('canvas mode: coarse dial rotates headings (count = |ticks|), press opens composer, tap resets to doc start', async () => { + const ctx = makeStore(); // default selection pir-1 is blocked at plan-approval → canvas mode + const nav = new DiffFileNav(ctx.store); + await nav.onDialRotate(dial(3) as never); // heading-next, count 3 + await nav.onDialRotate(dial(-1) as never); // heading-prev, count 1 + await nav.onDialDown(); // composer-open + await nav.onTouchTap(); // doc-start + expect(ctx.canvasSent).toEqual([ + { command: 'heading-next', target: { workspace: '/work/alpha' }, count: 3 }, + { command: 'heading-prev', target: { workspace: '/work/alpha' }, count: 1 }, + { command: 'composer-open', target: { workspace: '/work/alpha' }, count: undefined }, + { command: 'doc-start', target: { workspace: '/work/alpha' }, count: undefined }, + ]); + expect(ctx.sent).toHaveLength(0); // canvas mode never touches the generic verb relay + }); + + it('canvas mode: fine dial rotates blocks, tap walks forward through comments', async () => { + const ctx = makeStore(); // pir-1 → canvas mode + const nav = new DiffHunkNav(ctx.store); + await nav.onDialRotate(dial(2) as never); // block-next, count 2 + await nav.onDialDown(); // composer-open + await nav.onTouchTap(); // comment-next + expect(ctx.canvasSent).toEqual([ + { command: 'block-next', target: { workspace: '/work/alpha' }, count: 2 }, + { command: 'composer-open', target: { workspace: '/work/alpha' }, count: undefined }, + { command: 'comment-next', target: { workspace: '/work/alpha' }, count: undefined }, + ]); + }); + + it('canvas targeting omits file (MRU): the target carries only the workspace', async () => { + const ctx = makeStore(); + await new DiffFileNav(ctx.store).onDialRotate(dial(1) as never); + expect(ctx.canvasSent[0].target).toEqual({ workspace: '/work/alpha' }); + expect('file' in ctx.canvasSent[0].target).toBe(false); + }); + + it('a failed canvas command renders its per-code reason on the touchstrip', async () => { + const ctx = makeStore(); + ctx.canvasResult.value = { ok: false, code: 'no-canvas', error: 'no canvas open' }; + const action = dial(1).action; + const nav = new DiffFileNav(ctx.store); + nav.onWillAppear({ action, payload: {} } as never); + await nav.onDialRotate({ action, payload: { ticks: 1, settings: {} } } as never); + const last = action.setFeedback.mock.calls.at(-1)?.[0]; + expect(last).toMatchObject({ title: 'Headings', value: 'Open artifact' }); + + ctx.canvasResult.value = { ok: false, code: 'unreachable', error: 'Tower down' }; + await nav.onDialDown(); + expect(action.setFeedback.mock.calls.at(-1)?.[0]).toMatchObject({ value: 'Tower offline' }); + }); + + it('a canvas gesture with no active workspace is a no-op', async () => { + const ctx = makeStore(); + ctx.store.workspaces = []; // selectedWorkspacePath() → undefined + await new DiffFileNav(ctx.store).onDialRotate(dial(1) as never); + expect(ctx.canvasSent).toHaveLength(0); + }); + + it('none mode (unknown-phase builder): rotate/press/tap send nothing on either channel', async () => { + const ctx = makeStore(); + // A builder with no live status → phaseArtifactVerb undefined → reviewMode 'none'. + ctx.store.overview = { + builders: [{ id: 'pir-x', roleId: null, issueId: null, issueTitle: null, blocked: null, blockedGate: null, protocolPhase: '', progress: 0, worktreePath: '/w' }], + pendingPRs: [], backlog: [], recentlyClosed: [], + } as never; + const nav = new DiffFileNav(ctx.store); + await nav.onDialRotate(dial(1) as never); + await nav.onDialDown(); + await nav.onTouchTap(); + expect(ctx.sent).toHaveLength(0); // no diff verbs + expect(ctx.canvasSent).toHaveLength(0); // no canvas commands + }); + + it('none mode (no builder): the dials are inert', async () => { + const ctx = makeStore(); + ctx.store.overview = { builders: [], pendingPRs: [], backlog: [], recentlyClosed: [] } as never; + const nav = new DiffHunkNav(ctx.store); + await nav.onDialRotate(dial(1) as never); + await nav.onDialDown(); + await nav.onTouchTap(); + expect(ctx.sent).toHaveLength(0); + expect(ctx.canvasSent).toHaveLength(0); + }); + it('ScrollNav scrolls the editor on rotate and forwards the selection on press', async () => { const ctx = makeStore(); const nav = new ScrollNav(ctx.store); @@ -378,6 +474,27 @@ describe('phaseArtifactVerb (shared resolver — recognised verb or undefined)', }); }); +describe('reviewMode (dial mode from the shared resolver)', () => { + const b = (over: Record) => ({ id: 'x', blockedGate: null, protocolPhase: '', ...over }) as never; + it('spec/plan phases and their gates → canvas', () => { + expect(reviewMode(b({ blockedGate: 'spec-approval' }))).toBe('canvas'); + expect(reviewMode(b({ protocolPhase: 'specify' }))).toBe('canvas'); + expect(reviewMode(b({ blockedGate: 'plan-approval' }))).toBe('canvas'); + expect(reviewMode(b({ protocolPhase: 'plan' }))).toBe('canvas'); + }); + it('implement/review/verify and the dev-approval/pr gates → diff', () => { + expect(reviewMode(b({ protocolPhase: 'implement' }))).toBe('diff'); + expect(reviewMode(b({ protocolPhase: 'review' }))).toBe('diff'); + expect(reviewMode(b({ blockedGate: 'dev-approval' }))).toBe('diff'); + expect(reviewMode(b({ blockedGate: 'pr' }))).toBe('diff'); + }); + it('an unknown phase, no live status, or no builder → none', () => { + expect(reviewMode(b({}))).toBe('none'); + expect(reviewMode(b({ protocolPhase: 'mystery' }))).toBe('none'); + expect(reviewMode(undefined)).toBe('none'); + }); +}); + describe('CodevStore.syncToBuilder (builder follow)', () => { it('matches OverviewBuilder.id (diff/sidebar signal) and descends to builders', () => { const ctx = makeStore(); // cursor.builder 0 → pir-1; level defaults to workspaces @@ -488,14 +605,35 @@ describe('ZoomNav zoom gesture', () => { expect(fb.bar).toBe(45); // builder progress }); - it('Diff dials show the function (line 1) + issue details (line 2) + progress', () => { - const ctx = makeStore(); // selected builder (cursor 0) → pir-1 (#101, "Add the relay", 45%) + it('legibility: canvas-phase builder titles the dials Headings/Blocks', () => { + const ctx = makeStore(); // selected builder (cursor 0) → pir-1, blocked at plan-approval → canvas + const fileAction = { isDial: () => true, setFeedback: vi.fn() }; + const hunkAction = { isDial: () => true, setFeedback: vi.fn() }; + new DiffFileNav(ctx.store).onWillAppear({ action: fileAction, payload: {} } as never); + new DiffHunkNav(ctx.store).onWillAppear({ action: hunkAction, payload: {} } as never); + expect(fileAction.setFeedback).toHaveBeenCalledWith({ title: 'Headings', value: '#101 Add the relay', bar: 45 }); + expect(hunkAction.setFeedback).toHaveBeenCalledWith({ title: 'Blocks', value: '#101 Add the relay', bar: 45 }); + }); + + it('legibility: diff-phase builder titles the dials Files/Changes', () => { + const ctx = makeStore(); + ctx.store.syncToBuilder('pir-2'); // implement phase → diff mode (#102, "Wire the dial", 70%) const fileAction = { isDial: () => true, setFeedback: vi.fn() }; const hunkAction = { isDial: () => true, setFeedback: vi.fn() }; new DiffFileNav(ctx.store).onWillAppear({ action: fileAction, payload: {} } as never); new DiffHunkNav(ctx.store).onWillAppear({ action: hunkAction, payload: {} } as never); - expect(fileAction.setFeedback).toHaveBeenCalledWith({ title: 'Files', value: '#101 Add the relay', bar: 45 }); - expect(hunkAction.setFeedback).toHaveBeenCalledWith({ title: 'Changes', value: '#101 Add the relay', bar: 45 }); + expect(fileAction.setFeedback).toHaveBeenCalledWith({ title: 'Files', value: '#102 Wire the dial', bar: 70 }); + expect(hunkAction.setFeedback).toHaveBeenCalledWith({ title: 'Changes', value: '#102 Wire the dial', bar: 70 }); + }); + + it('legibility: the dial re-titles when the selection moves between modes', () => { + const ctx = makeStore(); + const action = { isDial: () => true, setFeedback: vi.fn() }; + const nav = new DiffFileNav(ctx.store); + nav.onWillAppear({ action, payload: {} } as never); // pir-1 → canvas + expect(action.setFeedback.mock.calls.at(-1)?.[0]).toMatchObject({ title: 'Headings' }); + ctx.store.syncToBuilder('pir-2'); // → diff; onChange re-renders + expect(action.setFeedback.mock.calls.at(-1)?.[0]).toMatchObject({ title: 'Files' }); }); it('clears the previous workspace overview immediately on switch (no stale flash)', () => { diff --git a/apps/streamdeck/src/actions.ts b/apps/streamdeck/src/actions.ts index 143fb0731..a6fa56935 100644 --- a/apps/streamdeck/src/actions.ts +++ b/apps/streamdeck/src/actions.ts @@ -9,7 +9,11 @@ import { type WillDisappearEvent, type DidReceiveSettingsEvent, } from '@elgato/streamdeck'; -import type { OverviewBuilder } from '@cluesmith/codev-sdk/controller'; +import type { + OverviewBuilder, + CanvasCommand, + CanvasCommandClientErrorCode, +} from '@cluesmith/codev-sdk/controller'; import type { CodevStore } from './store.js'; /** @@ -239,6 +243,24 @@ export function zoomInVerb(b: OverviewBuilder): string { return phaseArtifactVerb(b) ?? 'view-diff'; } +/** Which artifact form the selected builder's phase implies for the review dials. */ +export type ReviewMode = 'diff' | 'canvas' | 'none'; + +/** + * The review mode for a builder: a builder still writing its spec/plan reviews as a + * canvas (`open-spec` / `open-plan`), one with a diff reviews as a diff (`view-diff`), + * and an unknown/no-status builder has neither. Derived from the shared phase/gate + * resolver so the wire source stays single (`blockedGate` beats `protocolPhase`; never + * guessed) — this is the same resolver family #1404's press keys off. + */ +export function reviewMode(b: OverviewBuilder | undefined): ReviewMode { + if (!b) return 'none'; + const verb = phaseArtifactVerb(b); + if (verb === 'open-spec' || verb === 'open-plan') return 'canvas'; + if (verb === 'view-diff') return 'diff'; + return 'none'; +} + /** * Zoom Navigator: one Stream Deck + dial that walks the zoom altitudes * workspaces → builders → editor. @@ -467,19 +489,65 @@ export class SpawnNav extends SingletonAction { } } +/** Diff-mode gesture spec: canonical verbs fired over the generic command relay. */ +interface DiffSpec { + /** Line-1 label in diff mode (Files / Changes). */ + label: string; + next: string; + prev: string; + /** Tap: jump to the first file / hunk. */ + first: string; + /** Press: forward this axis (file / hunk) to the builder. */ + forward: string; +} + /** - * A dial for a diff-review axis (files or hunks): rotate navigates, the dial press - * forwards that axis to the builder, and a touch-strip tap jumps to the first. - * VSCode owns the actual file/hunk position, so the screen shows what the dial does - * (line 1) and which builder is under review (line 2 + progress bar) — not a counter. + * Canvas-mode gesture spec: canvas commands driven over `sendCanvasCommand` (#1401). + * Press is always `composer-open` (feedback at the focused block), so it is shared + * across dials rather than a field here. */ -abstract class DiffNav extends SingletonAction { - protected abstract readonly verbs: { next: string; prev: string; first: string }; - /** Line-1 label: what this dial does (Files / Changes). */ - protected abstract readonly label: string; - /** Verb fired by a dial press: forward this axis (file / hunk) to the builder. */ - protected abstract readonly forwardVerb: string; +interface CanvasSpec { + /** Line-1 label in canvas mode (Headings / Blocks). */ + label: string; + next: CanvasCommand; + prev: CanvasCommand; + /** Tap. */ + jump: CanvasCommand; +} + +/** Touchstrip line for a failed canvas command, per client error code (plan §4). */ +function canvasErrorLine(code: CanvasCommandClientErrorCode): string { + if (code === 'no-canvas') return 'Open artifact'; + if (code === 'unreachable') return 'Tower offline'; + return 'Error'; // invalid-request: defensive — we only ever send valid commands +} + +/** + * A phase-aware review dial. The selected builder's phase picks the MODE: + * + * - diff mode (implement / review, or blocked at dev-approval / pr): rotate walks + * the diff axis (files / hunks), press forwards that axis to the builder, tap + * jumps to the first — over the generic command relay. + * - canvas mode (specify / plan, or blocked at spec-approval / plan-approval): rotate + * steps the artifact-canvas (headings / blocks), press opens the composer at the + * focused block, tap resets (doc start) or walks comments — over `sendCanvasCommand`. + * + * The dials drive the workspace's most-recently-active canvas (MRU targeting): the + * phase picks the mode, the dials drive what you are looking at, and #1404's press + * converges the MRU onto the selected builder's own artifact. + * + * Legibility is a hard requirement: the touchstrip always names the live semantic + * (Files/Changes vs Headings/Blocks), recomputed on every overview tick, so a gesture + * is never a surprise. A failed canvas command renders its reason on the strip until + * the next tick. VSCode owns the actual position, so the screen shows what the dial + * does (line 1) and which builder is under review (line 2 + progress bar) — not a counter. + */ +abstract class ReviewNav extends SingletonAction { + protected abstract readonly diff: DiffSpec; + protected abstract readonly canvas: CanvasSpec; private current?: DialAction; + /** Transient canvas-error line; shown until the next overview tick clears it. */ + private status?: string; constructor(protected readonly store: CodevStore) { super(); @@ -494,42 +562,119 @@ abstract class DiffNav extends SingletonAction { override onWillDisappear(): void { this.current = undefined; } + + private mode(): ReviewMode { + return reviewMode(this.store.selectedBuilder()); + } + + /** onChange re-render: a fresh overview clears the transient canvas-error line. */ private render(): void { + this.status = undefined; if (this.current) this.renderTo(this.current); } - /** Line 1 = what the dial does; line 2 = builder under review (id + title); bar = its progress. */ + + /** Line 1 = the live semantic (mode-dependent); line 2 = builder under review + * (id + title); bar = its progress. A pending canvas error takes line 2 for one cycle. */ private renderTo(action: DialAction): void { + const label = this.mode() === 'canvas' ? this.canvas.label : this.diff.label; const b = this.store.selectedBuilder(); const id = b ? (b.issueId ? `#${b.issueId}` : b.id) : ''; const details = b ? (b.issueTitle ? `${id} ${b.issueTitle}` : id) : 'No builder'; - void action.setFeedback({ title: this.label, value: details, bar: Math.round(b?.progress ?? 0) }); + void action.setFeedback({ title: label, value: this.status ?? details, bar: Math.round(b?.progress ?? 0) }); } + override async onDialRotate(ev: DialRotateEvent): Promise { - const verb = dir(ev) >= 0 ? this.verbs.next : this.verbs.prev; - await this.store.client.sendCommand(verb, [], this.store.selectedWorkspacePath()); + const mode = this.mode(); + const forward = dir(ev) >= 0; + if (mode === 'canvas') { + // One call per rotate event: count = |ticks|, never a burst of single-tick sends. + const command = forward ? this.canvas.next : this.canvas.prev; + await this.runCanvas(command, Math.abs(ev.payload.ticks) || 1); + return; + } + if (mode === 'diff') { + const verb = forward ? this.diff.next : this.diff.prev; + await this.store.client.sendCommand(verb, [], this.store.selectedWorkspacePath()); + } + // none (no builder / unknown phase): no-op — the dial has no artifact to act on. } + override async onDialDown(): Promise { - // Press forwards this axis to the builder (was the touch strip). - await this.store.client.sendCommand(this.forwardVerb, [], this.store.selectedWorkspacePath()); + const mode = this.mode(); + if (mode === 'canvas') { + await this.runCanvas('composer-open'); + return; + } + if (mode === 'diff') { + await this.store.client.sendCommand(this.diff.forward, [], this.store.selectedWorkspacePath()); + } + // none: no-op. } + override async onTouchTap(): Promise { - // Touch jumps to the first file/change (was the dial press). - await this.store.client.sendCommand(this.verbs.first, [], this.store.selectedWorkspacePath()); + const mode = this.mode(); + if (mode === 'canvas') { + await this.runCanvas(this.canvas.jump); + return; + } + if (mode === 'diff') { + await this.store.client.sendCommand(this.diff.first, [], this.store.selectedWorkspacePath()); + } + // none: no-op. + } + + /** Send one canvas command to the workspace's MRU view and render its verdict. `count` + * is passed only for rotate (a traversal command); press / tap omit it. */ + private async runCanvas(command: CanvasCommand, count?: number): Promise { + const workspace = this.store.selectedWorkspacePath(); + if (!workspace) return; // no active workspace to target + const res = await this.store.client.sendCanvasCommand( + command, + { workspace }, + count !== undefined ? { count } : undefined, + ); + this.status = res.ok ? undefined : canvasErrorLine(res.code); + if (this.current) this.renderTo(this.current); } } -export class DiffFileNav extends DiffNav { +export class DiffFileNav extends ReviewNav { override readonly manifestId = 'com.cluesmith.codev.diff-file-nav'; - protected readonly label = 'Files'; - protected readonly forwardVerb = 'forward-file'; - protected readonly verbs = { next: 'diff-next-file', prev: 'diff-prev-file', first: 'diff-first-file' }; + protected readonly diff: DiffSpec = { + label: 'Files', + next: 'diff-next-file', + prev: 'diff-prev-file', + first: 'diff-first-file', + forward: 'forward-file', + }; + // Coarse dial in canvas mode: step headings; tap resets to the document start + // (role-consistent with diff-mode jump-to-first-file). + protected readonly canvas: CanvasSpec = { + label: 'Headings', + next: 'heading-next', + prev: 'heading-prev', + jump: 'doc-start', + }; } -export class DiffHunkNav extends DiffNav { +export class DiffHunkNav extends ReviewNav { override readonly manifestId = 'com.cluesmith.codev.diff-hunk-nav'; - protected readonly label = 'Changes'; - protected readonly forwardVerb = 'forward-hunk'; - protected readonly verbs = { next: 'diff-next-hunk', prev: 'diff-prev-hunk', first: 'diff-first-hunk' }; + protected readonly diff: DiffSpec = { + label: 'Changes', + next: 'diff-next-hunk', + prev: 'diff-prev-hunk', + first: 'diff-first-hunk', + forward: 'forward-hunk', + }; + // Fine dial in canvas mode: step blocks; tap walks forward through commented blocks + // (the "next place needing attention" capability). Keyboard parity means no wrap, so + // it stops at the last comment. + protected readonly canvas: CanvasSpec = { + label: 'Blocks', + next: 'block-next', + prev: 'block-prev', + jump: 'comment-next', + }; } /** Lines scrolled per dial tick (viewport only — the caret stays put). */ diff --git a/codev/plans/1400-stream-deck-phase-aware-review.md b/codev/plans/1400-stream-deck-phase-aware-review.md new file mode 100644 index 000000000..8bc13d552 --- /dev/null +++ b/codev/plans/1400-stream-deck-phase-aware-review.md @@ -0,0 +1,235 @@ +# PIR Plan: Stream Deck phase-aware review dials + +## Understanding + +Issue #1400 (the current issue body is the spec, rewritten 2026-08-12). When the selected builder +is writing a spec or plan there is no diff yet, so the two diff dials (Files / Changes) sit idle +exactly when the artifact **canvas** is the thing under review. The ask: make those two dials +**phase-aware** so the same physical gestures review whichever artifact form the selected builder's +phase implies. + +**The unifying rule** (constant gesture roles, only the artifact form changes): + +| Selected builder's state | Coarse dial (today: Files) | Fine dial (today: Changes) | Press | Tap | +|---|---|---|---|---| +| implement / review, or blocked at dev-approval / pr | files (unchanged) | hunks (unchanged) | forward file/hunk (unchanged) | jump to first (unchanged) | +| specify / plan, or blocked at spec-approval / plan-approval | heading step | block step | `composer-open` on the focused block | coarse → `doc-start` (reset); fine → `comment-next` (walk commented blocks) | + +The bridge is done: #1401 shipped `sendCanvasCommand` on the sdk controller subpath and #1404 +shipped `phaseArtifactVerb` (the phase/gate resolver). This project is the **deck half** and needs +**no bridge changes** (req 7). + +### Key facts from investigation + +- The two dials are `DiffFileNav` ("Files") and `DiffHunkNav` ("Changes"), subclasses of the + abstract `DiffNav` in `apps/streamdeck/src/actions.ts:476-533`. Both are `Encoder` controllers in + the manifest — the physical dials. Reusing them means **zero layout change** ("ships on the + existing dial layout"). +- `phaseArtifactVerb(b)` (`actions.ts:221`) already resolves state → artifact using the exact wire + source #1404 uses (`blockedGate` beats `protocolPhase`, never guessed strings): `open-spec` / + `open-plan` for canvas phases, `view-diff` for diff phases, `undefined` when unknown. A thin + `reviewMode()` derived from it keys the dials — **no duplicated resolver** (issue: "reuse that + resolver family"). +- `store.client.sendCanvasCommand(command, {workspace, file?}, {count?})` (`tower-client.ts:986`) + never rejects and returns a `CanvasCommandClientResult` whose failure `code` is a closed union + `no-canvas | invalid-request | unreachable`. `count` is valid only on the eight traversal verbs + (`heading-*`, `block-*`, `comment-*`, `column-*`). +- Canvas views register (`canvas-view-registry.ts`) under the **host window's** workspace + the + absolute file path. The reviewer opens artifacts in the main window, so they register under the + selected workspace — matching the deck's `store.selectedWorkspacePath()`. + +## Proposed Change + +Make the two review dials phase-aware by generalizing `DiffNav` into a `ReviewNav` that carries +**two gesture specs** — a diff spec (today's behavior, unchanged) and a canvas spec — and dispatches +on `reviewMode(selectedBuilder)` at both render time and gesture time. All new behavior is in +`apps/streamdeck/src/actions.ts` and its test file. No manifest, layout, sdk, Tower, or vscode +change. + +### 1. `reviewMode()` — the mode resolver (reuse #1404's family) + +```ts +type ReviewMode = 'diff' | 'canvas' | 'none'; + +/** Which artifact form the selected builder's phase implies. Derived from the + * shared phase/gate resolver so the wire source stays single (blockedGate beats + * protocolPhase; never guessed). */ +export function reviewMode(b: OverviewBuilder | undefined): ReviewMode { + if (!b) return 'none'; + const verb = phaseArtifactVerb(b); + if (verb === 'open-spec' || verb === 'open-plan') return 'canvas'; + if (verb === 'view-diff') return 'diff'; + return 'none'; // unknown gate / no live status +} +``` + +### 2. `ReviewNav` — the phase-aware dial base (replaces `DiffNav`) + +Each dial declares a `diff` spec (unchanged verbs) and a `canvas` spec: + +```ts +interface DiffSpec { label: string; next: string; prev: string; first: string; forward: string; } +interface CanvasSpec { + label: string; // legibility: names the semantic in canvas mode + next: TraversalCommand; // rotate + tick + prev: TraversalCommand; + jump: CanvasCommand; // tap + // press is always 'composer-open' (feedback at the focused position) — shared, not per-dial +} +``` + +- **Coarse dial (`DiffFileNav`)** — diff: `Files` (`diff-next/prev/first-file`, `forward-file`); + canvas: `Headings` (`heading-next`/`heading-prev`, tap → `doc-start` — the reset gesture, + role-consistent with diff-mode first-file). +- **Fine dial (`DiffHunkNav`)** — diff: `Changes` (`diff-next/prev/first-hunk`, `forward-hunk`); + canvas: `Blocks` (`block-next`/`block-prev`, tap → `comment-next` — repeatable taps walk forward + through commented blocks; keyboard parity means no wrap, so it stops at the last comment). + +Handlers dispatch on `reviewMode(this.store.selectedBuilder())`: + +- **`onDialRotate`** — diff: today's single `sendCommand(next|prev)` per event (unchanged). canvas: + **one** `sendCanvasCommand(dir>=0 ? spec.next : spec.prev, {workspace}, {count: Math.abs(ticks)})` + per rotate event — count = ticks, never a burst (architect directive). Render the returned + verdict (below). +- **`onDialDown`** (press) — diff: `sendCommand(spec.forward)` (unchanged). canvas: + `sendCanvasCommand('composer-open', {workspace})` (no count). +- **`onTouchTap`** (tap) — diff: `sendCommand(spec.first)` (unchanged). canvas: + `sendCanvasCommand(spec.jump, {workspace})` (no count). +- `none` mode or missing workspace → no-op (dials have no `showAlert`; the render line already reads + "No builder"). + +Guard: `sendCanvasCommand` requires `workspace: string`; if `selectedWorkspacePath()` is undefined, +skip the canvas call. + +### 3. Legibility (hard requirement) — the touchstrip always names the current semantic + +`renderTo` computes `mode = reviewMode(b)` and picks the label: canvas → `Headings` / `Blocks`, +diff → `Files` / `Changes`. Line 2 stays the builder-under-review (id + issue title), bar = progress +— identical framing to today, only the title switches. Because the dial already re-renders on every +`store.onChange` (SSE overview tick) and reads `selectedBuilder()`, a phase change or a selection +change (e.g. #1404's press moving the shared cursor) re-titles the dial automatically. A gesture can +never be a surprise: the strip names what rotate/press/tap will do before you touch it. + +### 4. Per-code canvas feedback on the touchstrip + +Canvas gestures return a real verdict (diff-mode `sendCommand` is fire-and-forget and has no +meaningful error, unchanged). On `!res.ok`, set a transient status line the next `renderTo` prefers, +cleared on the next success/overview tick: + +- `no-canvas` → `Open artifact` (req 3: the artifact isn't open — press the builder's key to open it). +- `unreachable` → `Tower offline`. +- `invalid-request` → `Error` (defensive; we only ever send valid commands + counts). + +Implemented as a per-instance `status?: string` on the dial; `renderTo` renders it as the `value` +line for one cycle when set. Keeps the "always legible" invariant even on failure. + +### 5. Targeting: workspace-MRU (decided) + +**Decided (2026-08-12, issue comment): the deck uses workspace-MRU targeting — `sendCanvasCommand` +with `file` omitted.** The decision was co-signed by the streamdeck architect (this lane) and main as +the codev-types wire-contract stakeholder, after verifying `OverviewBuilder` carries `worktreePath` +only. Requirement 3's file-qualified preference is superseded; MRU (its recorded fallback) is the v1 +behavior. This section records the settled model, not a tradeoff. + +**The model:** the selected builder's phase picks the dial **mode** (diff vs canvas); the dials then +drive the **MRU canvas — the artifact you are looking at**. These converge in the real workflow: +#1404's press opens and focuses the selected builder's artifact (`open-spec`/`open-plan`), making it +the MRU view, so "drive the MRU" *is* "drive the selected builder's artifact." Concretely, every +canvas gesture targets `{ workspace: selectedWorkspacePath() }` with no `file`. + +**Upgrade path (additive, preserved, not a v1 alternative):** file-qualified targeting stays +available as a future enhancement — an additive `OverviewBuilder.specPath` / `.planPath` field +(Tower-computed, specced with main when pursued) plus passing `file` on the deck's `sendCanvasCommand` +call. That is a one-line deck change with no redesign; it is out of this issue's scope because the +wire field is. + +### 6. Deferred (spec-time calls resolved here) + +- **Canvas tap mapping (the issue's "first commented block vs doc start" spec decision): split by + dial.** Coarse (Headings) tap → `doc-start`, the reset gesture (role-consistent with diff-mode + first-file). Fine (Blocks) tap → `comment-next`, so repeatable taps walk forward through commented + blocks — the "next place needing attention" capability. This keeps `comment-next`/`comment-prev` in + the gesture map (rather than unused) and avoids duplicating `doc-start` on both taps. +- **Reading-mode toggle (req 4): deferred.** The two dials' rotate/press/tap all carry review + semantics in both modes — no spare gesture. This codebase deliberately avoids press-duration + heuristics (see the `ZoomNav` comment on touch-in / press-out being two distinct reliable + gestures). #1410's Row 2 is the natural home. Deferring keeps v1 focused on the core phase-switch + that needs hardware verification. +- **Column paging (req 5): deferred with it.** Column paging (`column-forward`/`column-back`) only + applies inside horizontal reading mode, which is unreachable from the deck until the toggle has a + home. Candidate mapping recorded for the follow-up: coarse dial drives columns while reading mode + is on. +- **Deck-driven composer submit/cancel (req 6): no** (the stated default). Press = `composer-open` + only; typing, submit, and cancel stay on the keyboard. + +## Files to Change + +- `apps/streamdeck/src/actions.ts` + - Add `reviewMode(b)` (exported, near `phaseArtifactVerb`). + - Replace `DiffNav` (`:476-519`) with `ReviewNav`: two gesture specs (diff + canvas), mode + dispatch in `onDialRotate` / `onDialDown` / `onTouchTap`, legible per-mode `renderTo`, and the + transient per-code `status` line. + - Update `DiffFileNav` (`:521-526`) and `DiffHunkNav` (`:528-533`) to declare their canvas specs + (`Headings` / `Blocks`) alongside the existing diff specs. **Same manifest UUIDs, same physical + dials** — no manifest edit. + - Import `CanvasCommand` / `TraversalCommand` types from `@cluesmith/codev-sdk/controller` (or + `@cluesmith/codev-types` as the existing controller re-export allows — match the store's import + style). +- `apps/streamdeck/src/__tests__/actions.test.ts` + - Extend the DiffNav tests: rotate/press/tap in **diff mode** still fire the same verbs (regression + lock). New: rotate/press/tap in **canvas mode** call `sendCanvasCommand` with the right command, + `count: |ticks|` on rotate, and no count on press/tap. + - `reviewMode()` table tests mirroring the `phaseArtifactVerb` table (spec/plan → canvas, + implement/review/dev-approval/pr → diff, unknown/no-builder → none). + - Legibility: `renderTo` sets `Headings`/`Blocks` when the selected builder is in a canvas phase, + `Files`/`Changes` in a diff phase. + - Per-code feedback: a `no-canvas` / `unreachable` verdict sets the status line. +- No changes to `plugin.ts` (the two dials register unchanged), the manifest, the sdk, Tower, or + vscode. + +## Risks & Alternatives Considered + +- **Risk: MRU drives the wrong canvas** if the reviewer opens an unrelated canvas after selecting a + builder. This is accepted and by design: the dials drive the canvas you are looking at. The + press-builder-key-then-dial workflow (press opens the artifact → MRU) is the intended path, and + the `no-canvas` feedback covers "nothing open." The additive file-qualified upgrade (§5) remains + available if a future need arises. +- **Risk: mode flips mid-gesture** as a phase transitions between a rotate and a press. Low blast + radius — each gesture reads `reviewMode` freshly and fires one self-contained command; a stale + read at worst sends one command to the other channel, which the receiver handles (or answers + `no-canvas`). No state is corrupted. +- **Alternative: separate new canvas dial actions** (new manifest UUIDs on reserved keys). Rejected + — that is the superseded reserved-headroom layout the issue explicitly replaces; it would reopen + the eight-key conflict on #1410 and add layout the issue says is no longer needed. +- **Alternative: burst N single-tick canvas commands per rotate.** Rejected per architect directive + — one `sendCanvasCommand` with `count = ticks` per rotate event. + +## Test Plan + +**Unit (vitest, `apps/streamdeck`):** +- `reviewMode()` table: spec-approval/plan-approval/specify/plan → `canvas`; + dev-approval/pr/implement/review/verify → `diff`; unknown gate / unknown phase / no builder → + `none` (mirrors the `phaseArtifactVerb` table). +- Diff-mode regression: rotate/press/tap on both dials fire the existing `sendCommand` verbs + (`diff-next-file`, `forward-file`, `diff-first-file`, and the hunk equivalents) — unchanged. +- Canvas-mode: with the selected builder in `plan`, coarse-dial rotate(+3) → + `sendCanvasCommand('heading-next', {workspace}, {count: 3})`, rotate(-1) → `heading-prev` count 1; + press → `composer-open`; tap → `doc-start`. Fine dial → rotate `block-next`/`block-prev`; + press → `composer-open`; tap → `comment-next` (no count on press/tap). +- Legibility: `renderTo` titles are `Headings`/`Blocks` for a canvas-phase builder, `Files`/`Changes` + for a diff-phase builder. +- Feedback: a stubbed `sendCanvasCommand` returning `{ok:false, code:'no-canvas'}` sets the + status line; `unreachable` renders the offline line. + +**Manual (hardware — the reason this is PIR; verified at dev-approval on the running worktree):** +- Select a builder in `implement`/`review` (or blocked at dev-approval/pr): dials read + `Files`/`Changes`, rotate/press/tap drive the diff exactly as today (no regression). +- Select a builder in `specify`/`plan` (or blocked at spec-approval/plan-approval): dials **re-title + to `Headings`/`Blocks`**; press the builder key (#1404) to open its artifact; rotate steps + headings/blocks in the canvas, press opens the composer at the focused block, coarse-dial tap + jumps to doc start, and **fine-dial tap walks forward through commented blocks** (repeat taps step + to each next comment, stopping at the last). +- With no artifact canvas open, a canvas rotate shows `Open artifact` on the strip (no-canvas). +- With Tower stopped, a canvas gesture shows `Tower offline` (unreachable), not a false success. +- Switch the selected builder between a diff-phase and a canvas-phase builder and confirm both dials + re-title within one overview tick (legibility: the strip always names the live semantic). diff --git a/codev/projects/1400-stream-deck-phase-aware-review/1400-review-iter1-rebuttals.md b/codev/projects/1400-stream-deck-phase-aware-review/1400-review-iter1-rebuttals.md new file mode 100644 index 000000000..4c2c4da8e --- /dev/null +++ b/codev/projects/1400-stream-deck-phase-aware-review/1400-review-iter1-rebuttals.md @@ -0,0 +1,52 @@ +# PIR #1400 — Review iteration 1 rebuttal / disposition + +3-way consultation verdicts: **Gemini APPROVE**, **Claude APPROVE**, **Codex REQUEST_CHANGES**. +One substantive finding, raised by Codex (HIGH) and independently flagged by Claude as its +observation #1. **Accepted and fixed** — no disagreement. + +## Codex REQUEST_CHANGES: `none` mode dispatches diff commands (`actions.ts`) + +> `ReviewNav` treats every non-canvas mode as diff. Missing/unknown builders must no-op per plan, +> but rotate, press, and tap currently send diff verbs. Add an explicit `mode === 'diff'` branch and +> regression tests for `none` mode. + +**Verdict: valid — fixed.** This was a genuine unstated deviation from the approved plan. Plan §2 +specifies: "`none` mode or missing workspace → no-op." The first implementation branched only on +`mode === 'canvas'` and let `none` fall through to the diff verbs (harmless in practice — diff verbs +on no open diff do nothing — but not what the plan says, and untested). + +**Change (commit a7a7e81d4):** +- `onDialRotate` / `onDialDown` / `onTouchTap` now branch explicitly on `mode === 'diff'`; `none` + sends nothing on either channel, with an inline `// none: no-op` comment so a later reader doesn't + "fix" it back. +- Two regression tests added: an unknown-phase builder and a no-builder overview, each asserting + `sent` **and** `canvasSent` are empty. Both fail against the pre-fix fall-through. +- `tsc --noEmit` ✓, `npm test` ✓ (84 tests, +2), `npm run build` ✓. + +Documented in the review file's "Things to Look At During PR Review" section. + +## Claude observation #1 (same finding) + +Claude APPROVEd but raised the identical `none`-mode fall-through as a non-blocking observation +("worth one comment … plus a one-line test"). Resolved by the same fix above (comment + two tests). + +## Claude observations #2–#4 (non-blocking, no change) + +- **#2 Rotate magnitude asymmetry** (diff ignores tick count; canvas passes `count = |ticks|`): + deliberate per the architect's explicit directive and hardware-verified at dev-approval. No change. +- **#3 Single `current` DialAction per class** (two placed copies of one dial would leave one stale): + pre-existing pattern in `ZoomNav`/`PrNav`, not a regression from this change. Out of scope. +- **#4 Transient error line can linger if the store is idle**: the set/clear ordering (clear on the + onChange tick, set after the awaited verdict, re-render only if `current`) is already flagged in the + review file and reads correct. No change. + +## Gemini + +APPROVE, no issues. + +--- + +Note: PIR consultation is single-pass (`max_iterations: 1`) — this fix will **not** be independently +re-reviewed by the models. The `none`-mode fix + its two pinning tests plus the human's `pr`-gate +review are the backstop. The architect will be notified leading with the REQUEST_CHANGES and this +disposition. diff --git a/codev/projects/1400-stream-deck-phase-aware-review/status.yaml b/codev/projects/1400-stream-deck-phase-aware-review/status.yaml new file mode 100644 index 000000000..515651acb --- /dev/null +++ b/codev/projects/1400-stream-deck-phase-aware-review/status.yaml @@ -0,0 +1,30 @@ +id: '1400' +title: stream-deck-phase-aware-review +protocol: pir +phase: review +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: approved + requested_at: '2026-08-12T01:48:58.976Z' + approved_at: '2026-08-12T02:15:54.781Z' + dev-approval: + status: approved + requested_at: '2026-08-12T02:22:39.327Z' + approved_at: '2026-08-12T05:16:55.899Z' + pr: + status: approved + requested_at: '2026-08-12T05:25:25.516Z' + approved_at: '2026-08-12T06:40:01.169Z' +iteration: 1 +build_complete: true +history: [] +started_at: '2026-08-12T01:43:13.126Z' +updated_at: '2026-08-12T06:40:01.170Z' +pr_history: + - phase: review + pr_number: 1419 + branch: builder/pir-1400 + created_at: '2026-08-12T05:19:25.867Z' +pr_ready_for_human: false diff --git a/codev/reviews/1400-stream-deck-phase-aware-review.md b/codev/reviews/1400-stream-deck-phase-aware-review.md new file mode 100644 index 000000000..84ae47c63 --- /dev/null +++ b/codev/reviews/1400-stream-deck-phase-aware-review.md @@ -0,0 +1,97 @@ +# PIR Review: Stream Deck phase-aware review dials + +Fixes #1400 + +## Summary + +The Stream Deck's two diff dials (Files / Changes) sat idle whenever the selected builder was +writing a spec or plan — exactly when the artifact **canvas** is the thing under review. This change +makes those two dials **phase-aware**: the selected builder's phase picks the dial *mode*, so the +same physical gestures review whichever artifact form applies. Diff-phase builders drive the diff +(unchanged); spec/plan-phase builders drive the artifact canvas (headings / blocks, composer, comment +walk) over #1401's `sendCanvasCommand`. No bridge, sdk, Tower, or vscode change — only the deck. + +## Files Changed + +- `apps/streamdeck/src/actions.ts` (+164 / -21) — `reviewMode()` resolver + `DiffNav` → phase-aware `ReviewNav` +- `apps/streamdeck/src/__tests__/actions.test.ts` (+118 / -8) — canvas-mode, legibility, `reviewMode`, per-code feedback tests +- `apps/streamdeck/com.cluesmith.codev.sdPlugin/manifest.json` (+11 / -11) — dial Name/Tooltip/TriggerDescription reflect the dual role (UUIDs unchanged) +- `apps/streamdeck/README.md` (+13 / -6) — Actions list updated for the renamed, phase-aware dials + +## Commits + +- `9aa1a29d9` [PIR #1400] Phase-aware review dials: canvas mode reuses the diff dials +- `1abc02025` [PIR #1400] Rename review dials in manifest to reflect phase-aware behavior +- (plus `[PIR #1400]` thread + plan commits; README rename folded into the review commit) + +## Test Results + +- `npm run build`: ✓ pass (esbuild bundle) +- `npx tsc --noEmit`: ✓ pass +- `npm test`: ✓ pass (84 tests, 13 new — includes 2 pinning the `none`-mode no-op fix below) +- `npx streamdeck validate`: ✓ pass +- Manual verification (human, dev-approval gate): sideloaded from the worktree; confirmed the phase + switch, the live touchstrip re-title, and that diff-phase behavior is unchanged. + +## Architecture Updates + +No hot-tier (`arch-critical.md`) change: this is a self-contained controller-side feature, not a new +invariant or module boundary. No cold-tier (`arch.md`) change needed either — the existing +`apps/streamdeck` entry already describes the deck as an "outside-in controller: … command-relay +verbs via the sdk's `controller`/`node` subpaths," and `sendCanvasCommand` lives on that same +`controller` subpath, so the description stays accurate. + +## Lessons Learned Updates + +No new hot-tier lesson. Two existing hot lessons already governed the key decisions and were followed +rather than added to: *"Single source of truth beats distributed state"* (the dial mode is derived +from the shared `phaseArtifactVerb` resolver, not a re-derived phase-string table) and *"After any +rename … grep the whole repo"* (the manifest rename drove the README + tooltip + trigger-description +sweep). The MRU-vs-file-qualified targeting call is spec-narrow and already captured in the plan (§5) +and issue thread, so it does not warrant a cold-tier entry. + +## Things to Look At During PR Review + +- **3-way consultation — `none`-mode fix (Codex REQUEST_CHANGES, Claude flagged same, HIGH confidence).** + Both reviewers caught that the first implementation let `none` mode (no builder / unknown phase) + fall through to the diff verbs, whereas plan §2 specifies a no-op. This was a genuine unstated + deviation from the approved plan. **Fixed** in `onDialRotate` / `onDialDown` / `onTouchTap`: each now + branches explicitly on `mode === 'diff'`, so `none` sends nothing on either channel. Pinned by two + regression tests (unknown-phase builder, and no builder) asserting `sent` and `canvasSent` are both + empty — they fail against the pre-fix fall-through. Gemini returned APPROVE with no issues. + +- **`reviewMode()` reuse** (`actions.ts`): it is intentionally a thin derivation of `phaseArtifactVerb` + (`open-spec`/`open-plan` → canvas, `view-diff` → diff, else `none`) so the wire source stays single + (`blockedGate` beats `protocolPhase`; never guessed). If the phase→artifact mapping ever changes, + both the Builder Action key (#1404) and these dials move together — that coupling is the point. +- **MRU targeting (no `file`)** (`runCanvas`): every canvas gesture targets `{ workspace }` only. This + was the deliberate v1 decision (plan §5, co-signed by main as the types stakeholder) because + `OverviewBuilder` carries no artifact path; #1404's press converges the MRU onto the selected + builder's artifact. File-qualified targeting is the documented additive upgrade, not a gap. +- **Canvas `count = |ticks|`** (`onDialRotate`): one `sendCanvasCommand` per rotate event carrying the + tick count, never a burst of single-tick sends. Diff-mode rotate is unchanged (one verb per event). +- **Transient error line** (`status` field + `canvasErrorLine`): a failed canvas command shows its + per-code reason (`no-canvas` → "Open artifact", `unreachable` → "Tower offline") on the touchstrip + until the next overview tick clears it — the `render()` onChange handler clears it, gesture handlers + set it. Worth a look that the clear/set ordering can't strand a stale error. +- **Type choice**: canvas spec fields are typed `CanvasCommand` (re-exported from + `@cluesmith/codev-sdk/controller`), not `TraversalCommand` — the latter isn't re-exported there and + the import-boundary test forbids importing `@cluesmith/codev-types` directly. No sdk change; `count` + isn't type-restricted to traversal by the sdk signature anyway. + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder pir-1400 → **Review Diff** +- **Run / sideload**: build (`pnpm --filter @cluesmith/codev-sdk build` then + `pnpm --filter @cluesmith/codev-streamdeck build`), then + `streamdeck link apps/streamdeck/com.cluesmith.codev.sdPlugin` + + `streamdeck restart com.cluesmith.codev` (unlink any existing build first) +- **What to verify** (maps to the plan's Test Plan): + - Select an implement/review builder → dials read `Files`/`Changes`; rotate/press/tap drive the diff + exactly as before (no regression) + - Select a specify/plan builder → dials re-title `Headings`/`Blocks`; press the builder key (#1404) + to open its artifact; rotate steps headings/blocks, press opens the composer, coarse tap jumps to + the top, fine tap walks forward through commented blocks + - No artifact canvas open → a canvas rotate shows `Open artifact`; Tower stopped → `Tower offline` + - Switch the selected builder between a diff-phase and a canvas-phase builder → both dials re-title + within one overview tick diff --git a/codev/state/pir-1400_thread.md b/codev/state/pir-1400_thread.md new file mode 100644 index 000000000..dc95f2c65 --- /dev/null +++ b/codev/state/pir-1400_thread.md @@ -0,0 +1,84 @@ +# pir-1400 — Stream Deck: phase-aware review dials + +## Plan phase (2026-08-12) + +Issue #1400. Deck half of phase-aware review; bridge (#1401 `sendCanvasCommand`) already merged. + +### What I found investigating + +- The two diff dials are `DiffFileNav` ("Files") + `DiffHunkNav` ("Changes") in + `apps/streamdeck/src/actions.ts`, both `Encoder` controllers in the manifest. Reusing them = + zero layout change (honors "ships on existing dial layout"). +- `phaseArtifactVerb(b)` (actions.ts:221, shipped by #1404) is the exact resolver to key mode off: + `blockedGate` beats `protocolPhase`, returns `open-spec`/`open-plan` (→ canvas mode) / `view-diff` + (→ diff mode) / undefined. I'll derive a thin `reviewMode()` from it — no duplication. +- `sendCanvasCommand(command, {workspace, file?}, {count?})` lives on `TowerClient` (tower-client.ts:986), + reachable via `store.client`. Never rejects; returns `CanvasCommandClientResult` with a closed + error union (`no-canvas` | `invalid-request` | `unreachable`). `count` valid only on the 8 traversal verbs. +- **Targeting DECIDED (2026-08-12, architect + main):** workspace-MRU for v1 (omit `file`). + `OverviewBuilder` carries `worktreePath` only — file-qualified genuinely needs an additive + Tower-computed wire field, out of scope. Model: phase picks the dial MODE; dials drive the MRU + canvas (what you see); #1404's press converges MRU onto the selected builder's artifact. + File-qualified recorded as the additive upgrade path (future `OverviewBuilder.specPath`/`.planPath` + through main's sphere, one-line deck change) — NOT a v1 alternative. Folded into plan §5. +- Canvas view registration (`canvas-view-registry.ts`) keys on host workspace + abs file path; the + reviewer opens artifacts in their main window, registered under the selected workspace → matches + the deck's `selectedWorkspacePath()`. Convergence holds. + +### Spec-time decisions I'm resolving in the plan (issue body is the spec) + +- Tap in canvas mode (architect plan-review revision): **coarse (Headings) → doc-start** (reset), + **fine (Blocks) → comment-next** (walk commented blocks = "next place needing attention"). Restores + the headline commented-block navigation; keeps comment-next/prev in the map; no doc-start duplication. +- Reading-mode toggle (req 4) + column paging (req 5): **deferred** — existing dials have no spare + gesture, press-duration heuristics are avoided in this codebase, #1410 Row 2 is the natural home. +- Deck submit/cancel (req 6): **no** (default) — composer-open only; typing stays on keyboard. + +Architect (streamdeck) confirmed these pointers via afx during plan phase. Plan drafted, awaiting +plan-approval gate. + +## Implement phase (2026-08-12) — plan-approval APPROVED + +Implemented in `apps/streamdeck/src/actions.ts` (+ tests). No sdk/Tower/vscode/manifest change. + +- `reviewMode(b)` derives `'diff' | 'canvas' | 'none'` from `phaseArtifactVerb` (single wire source). +- `DiffNav` → `ReviewNav`: carries a `DiffSpec` (unchanged verbs) + `CanvasSpec`; dispatches on + `reviewMode(selectedBuilder)` in rotate/press/tap and in `renderTo` (legibility). Coarse dial + `Files`↔`Headings`, fine `Changes`↔`Blocks`. +- Canvas rotate = one `sendCanvasCommand` with `count=|ticks|`; press = `composer-open`; tap = + `doc-start` (coarse) / `comment-next` (fine). MRU targeting (`{workspace}`, no `file`). +- Per-code touchstrip feedback: `no-canvas`→"Open artifact", `unreachable`→"Tower offline", + else "Error"; transient `status` line cleared on the next overview tick. +- **Import note:** typed canvas spec fields as `CanvasCommand` (re-exported from + `@cluesmith/codev-sdk/controller`); `TraversalCommand` is NOT re-exported there and the + import-boundary test forbids importing `@cluesmith/codev-types` directly — so no sdk change, + honoring the plan. `count` isn't type-restricted to traversal by the sdk signature anyway. +- **Test-fixture gotcha:** default selection (cursor 0 = pir-1) is now a canvas-phase builder; + existing diff-mode tests re-pointed at `pir-2` (implement) via `syncToBuilder`. + +Verified in worktree: `tsc --noEmit` ✓, `npm run build` ✓, `npm test` ✓ (82 tests, ~10 new). +Needed `pnpm --filter @cluesmith/codev-sdk build` first (sdk dist absent in fresh worktree). +Commit 9aa1a29d9. Awaiting dev-approval gate (hardware verification). + +**Manifest rename (owner-requested at dev-approval, commit 1abc02025):** the two dial actions' +user-facing `Name`/`Tooltip`/`TriggerDescription` still said "Diff File/Hunk Navigator" (palette +label seen when configuring) though the live dial face already re-titles Headings/Blocks at runtime. +Renamed to "Review: Files / Headings" and "Review: Changes / Blocks" with dual-mode tooltips. UUIDs +(`diff-file-nav`/`diff-hunk-nav`) and controller/layout structure untouched — stable identity, no +layout change, so no code/test impact. `streamdeck validate` ✓. + +## Review phase (2026-08-12) — dev-approval APPROVED + +Retrospective at `codev/reviews/1400-stream-deck-phase-aware-review.md`. Also swept +`apps/streamdeck/README.md` Actions list for the rename (grep-both-trees lesson). No arch/lessons +tier change — existing hot lessons already governed (single-source resolver reuse; rename sweep); +arch.md's streamdeck entry stays accurate (sendCanvasCommand is on the controller subpath it names). +PR #1419 opened (Fixes #1400), recorded with porch. Running the single 3-way consult via porch done. + +**Consult iter1 verdicts:** Gemini APPROVE, Claude APPROVE, **Codex REQUEST_CHANGES** (HIGH). +One substantive finding (Codex + Claude both caught it): `none` mode fell through to diff verbs, +but plan §2 says no-op. **Accepted + fixed** (commit a7a7e81d4): explicit `mode === 'diff'` branch, +`none` → no-op, +2 regression tests (unknown-phase builder, no-builder) → 84 tests. Rebuttal at +`codev/projects/1400-*/1400-review-iter1-rebuttals.md`; disposition also in review "Things to Look At". +PIR is single-pass so this fix is NOT re-reviewed by models — human pr-gate is the backstop. +pr gate now pending. Notifying architect leading with the REQUEST_CHANGES + disposition.