packages/app/src/pages/session/composer/session-composer-state.test.ts (1)

108-131: ⚡ Quick win

Add a cross-session retry test case for refetch fallback.

Current coverage doesn’t lock behavior when list() returns pending questions for a different session before the target session appears. That’s the key edge for this fallback path.

Suggested test shape

 describe("refetchPendingQuestions", () => {
   test("retries when the running question tool appears before the pending question is listed", async () => {
@@
   })
+
+  test("does not stop early when only another session has pending questions", async () => {
+    const target = question("q-target", "root")
+    const other = question("q-other", "other")
+    let attempts = 0
+    const applied: Record<string, QuestionRequest[] | undefined> = {}
+
+    await refetchPendingQuestions({
+      maxAttempts: 3,
+      delayMs: 1,
+      sleep: async () => {},
+      shouldContinue: () => true,
+      list: async () => {
+        attempts += 1
+        if (attempts === 1) return [other]
+        return [target]
+      },
+      apply(sessionID, questions) {
+        applied[sessionID] = questions
+      },
+    })
+
+    expect(applied.root?.map((item) => item.id)).toEqual(["q-target"])
+  })
 })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/pages/session/composer/session-composer-state.test.ts`
around lines 108 - 131, Add a new test in session-composer-state.test.ts that
exercises refetchPendingQuestions when list() first returns a pending question
for a different session and only later returns the target session’s pending
item: mock list() to return [] or a pending for a different session on initial
attempts and then return the desired pending (e.g., question("q-late","root"))
on a subsequent attempt, keep maxAttempts/delayMs/sleep similar to the existing
test, capture calls via the apply(sessionID, questions) handler into an applied
map, and assert that refetchPendingQuestions retries (attempts count) and that
applied.root contains the expected "q-late" id; reference the
refetchPendingQuestions invocation and the list/apply mocks so the test locks
the cross-session retry fallback behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/app/src/pages/session/composer/session-composer-state.ts (1)

27-36: ⚡ Quick win

Consider forwarding completing from the todo model too.

The refactor still tracks completing on the model and sends it to the probe, but direct consumers of createSessionComposerState() can no longer read that phase from the returned state.

Suggested follow-up

   return {
     blocked: blockers.blocked,
     questionRequest: blockers.questionRequest,
     permissionRequest: blockers.permissionRequest,
     permissionResponding: blockers.permissionResponding,
     decide: blockers.decide,
     todos: todo.todos,
     dock: todo.dock,
     opening: todo.opening,
+    completing: todo.completing,
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/pages/session/composer/session-composer-state.ts` around
lines 27 - 36, The returned state from createSessionComposerState() omits the
todo model's completing flag so consumers can't read that phase; update the
returned object to forward the completing property from the todo model (i.e.,
include completing: todo.completing alongside todos, dock, opening) so both
probes and direct consumers see the completing state.

packages/app/src/pages/session/todos/todo-model.test.ts (1)

44-50: ⚡ Quick win

Add the missing status-diff assertion.

This test says todoDisplaySignature tracks status too, but it only proves content and priority changes. A completed vs cancelled case would pin down the status component that the renderer depends on.

Suggested addition

   test("tracks content, priority, and status for rendering-sensitive comparisons", () => {
     expect(todoDisplaySignature([todo("first", "completed", "high")])).not.toBe(
       todoDisplaySignature([todo("first refreshed", "completed", "high")]),
     )
     expect(todoDisplaySignature([todo("first", "completed", "high")])).not.toBe(
       todoDisplaySignature([todo("first", "completed", "low")]),
     )
+    expect(todoDisplaySignature([todo("first", "completed", "high")])).not.toBe(
+      todoDisplaySignature([todo("first", "cancelled", "high")]),
+    )
   })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/pages/session/todos/todo-model.test.ts` around lines 44 -
50, The test claims todoDisplaySignature tracks status but lacks a status-change
assertion; update the test inside the "tracks content, priority, and status for
rendering-sensitive comparisons" block to add an assertion that a change in
status produces a different signature — e.g., call
todoDisplaySignature([todo("first", "completed", "high")]) and assert it is not
equal to todoDisplaySignature([todo("first", "cancelled", "high")]); this
ensures the status component is exercised alongside the existing content and
priority checks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/app/src/pages/session/todos/todo-source.ts (1)

90-90: Consider removing the unused selectSessionTodoSnapshot alias or renaming it to clarify dock-specific semantics.

The alias points to selectSessionTodoDockSnapshot, which intentionally filters out terminal-only parts to prevent reopening the dock. The generic name masks this behavior. While the alias is currently unused in the codebase, it remains a misleading export that could cause confusion if reused later. Either rename it to selectSessionTodoDockSnapshot (drop the alias) or explicitly document its dock-specific behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/pages/session/todos/todo-source.ts` at line 90, The exported
alias selectSessionTodoSnapshot simply points to selectSessionTodoDockSnapshot
but masks dock-specific behavior; remove the alias export to avoid confusion or
replace it with an explicit export name (export { selectSessionTodoDockSnapshot
} or rename the export to selectSessionTodoDockSnapshot) and, if keeping a
second name, add a clear comment documenting that it intentionally filters out
terminal-only parts to prevent reopening the dock; update any imports to use
selectSessionTodoDockSnapshot where needed.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/app/src/pages/session/composer/session-composer-state.ts (1)

27-37: ⚡ Quick win

Expose completing in the returned composer state.

The model and probe both track completing, but the returned state omits it. Adding it keeps the composer state surface aligned with the delegated todo model and avoids accidental consumer regressions.

✅ Minimal fix

   return {
     blocked: blockers.blocked,
     recoveringQuestion: blockers.recoveringQuestion,
     questionRequest: blockers.questionRequest,
     permissionRequest: blockers.permissionRequest,
     permissionResponding: blockers.permissionResponding,
     decide: blockers.decide,
     todos: todo.todos,
     dock: todo.dock,
     opening: todo.opening,
+    completing: todo.completing,
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/pages/session/composer/session-composer-state.ts` around
lines 27 - 37, The returned composer state omits the completing flag tracked by
the todo model/probe; add completing to the returned object so consumers see it
(e.g., include completing: todo.completing alongside todos: todo.todos, dock:
todo.dock, opening: todo.opening in the returned state).

packages/app/src/pages/session/todos/todo-source.ts (1)

21-102: ⚡ Quick win

Extract shared snapshot-selection flow to one helper.

selectSessionTodoDataSnapshot and selectSessionTodoDockSnapshot currently duplicate almost all precedence/branch logic. Consolidating this reduces policy drift risk when one path is updated later.

♻️ Suggested direction

+function selectSnapshot(
+  input: SelectSessionTodosInput,
+  mode: "data" | "dock",
+): TodoSnapshot {
+  // shared precedence logic here
+  // tweak only the small mode-specific defaults/overrides
+}
+
 export function selectSessionTodoDataSnapshot(input: SelectSessionTodosInput): TodoSnapshot {
-  // duplicated logic...
+  return selectSnapshot(input, "data")
 }
 
 export function selectSessionTodoDockSnapshot(input: SelectSessionTodosInput): TodoSnapshot {
-  // duplicated logic...
+  return selectSnapshot(input, "dock")
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/pages/session/todos/todo-source.ts` around lines 21 - 102,
Both functions duplicate the same precedence/branch logic; extract that into a
single helper (e.g., selectSessionTodoSnapshot) that takes
SelectSessionTodosInput plus a small options object to control final fields (for
example {forceDockEligible?: boolean|undefined, defaultDockEligible?: boolean,
includeDockEligible?: boolean}) and returns TodoSnapshot. Move the common flow
that computes primaryParts/fallbackParts, calls todoPhase, and builds the
snapshot into that helper, wiring dockEligible and historicalTerminal from the
phase when includeDockEligible is true (or using defaultDockEligible /
forceDockEligible overrides), and preserve the final "none" fallback sessionID
behavior (use input.primary.sessionID). Replace selectSessionTodoDataSnapshot
and selectSessionTodoDockSnapshot to call the new helper with the appropriate
options (data: includeDockEligible false; dock: includeDockEligible true with
defaultDockEligible false).