Skip to content

fix(desktop): keep Side Conversation follow-ups visible - #4901

Open
liuxiaocs7 wants to merge 5 commits into
apache:mainfrom
liuxiaocs7:fix/side-chat-follow-up-visibility
Open

fix(desktop): keep Side Conversation follow-ups visible#4901
liuxiaocs7 wants to merge 5 commits into
apache:mainfrom
liuxiaocs7:fix/side-chat-follow-up-visibility

Conversation

@liuxiaocs7

Copy link
Copy Markdown
Member

Summary

Make Side Conversation follow-ups behave like the main conversation while a Turn is running:

  • Enter queues a next-Turn follow-up and renders it immediately; Shift+Enter steers the current Turn.
  • Project the Host queue into the transcript and expose the existing promote, edit, reorder, and retract controls.
  • Preserve settled replies across Turn handoffs and retire cancelled optimistic messages after reconnect/reseed.
  • Share queue projection logic between the main and Side Conversation surfaces.

Fixes #4900

Verification

  • npm --workspace @maka/desktop run build:workspace-deps
  • npm --workspace @maka/desktop run build:test
  • node --test apps/desktop/dist/main/__tests__/quote-companion-retry.test.js apps/desktop/dist/main/__tests__/transient-message-projection.test.js apps/desktop/dist/main/__tests__/workbar-services-adapter.test.js — 60/60 pass
  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop run check:architecture — 101/101 pass
  • npx knip --workspace apps/desktop
  • npx biome check --diagnostic-level=error <changed files>
  • git diff --check origin/main...HEAD

The focused tests exercise the user-visible optimistic/queued projection, Enter vs Shift+Enter routing, multiple queued follow-ups, queue retraction, Turn handoff, and reconnect cancellation cleanup.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex diagnosed the state/admission races, implemented the Desktop renderer and adapter changes, added regression coverage, and prepared the issue/PR text. The commits retain Generated-by: Codex trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 6, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR at head 19d0d8ef (18 files, +1133/−119) against base 90014db9.

What the change does: Side Conversation follow-ups now behave like the main conversation while a Turn is running: Enter queues a next-turn follow-up that renders immediately, Shift+Enter steers the active turn; the Host queue is projected into the transcript with the existing promote/edit/reorder/retract controls; settled replies survive Turn handoffs; cancelled optimistic messages are retired after reconnect/reseed. The queue projection shared with the main surface is extracted into deriveMessageQueueProjection (message-queue-projection.ts), and the cancellation-proof batching moves from the renderer into the IPC layer.

Design: The problem is real and the fix aligns the side chat with the main conversation's established contract (requestedMode routing in app-shell.tsx:1917; the Composer queue controls pre-exist in packages/ui). No new authority is introduced: the Host queue (queue_update from the session projector) remains the single source of truth; the renderer only projects it.

Function (production path): Enter/Shift+Enter routing (quote-companion-context-compaction.ts:55-61) → submitFollowUp port → submitMessage(sessionId, placement, ...) (create-workbar-services.ts:50-66) → IPC → Host. next_turn submissions do not occupy the single in-flight admission slot, so several follow-ups can queue; a queued outcome releases the slot and a started outcome adopts the Host-named Turn (use-quote-companion.ts). The three race branches are covered by tests.

Complexity: The extraction is faithful — I compared it line-by-line with the previous inline logic in app-shell-session-events.ts; the extra state === 'queued' filter on follow-up entries is a no-op because the protocol types follow-up entries as always-queued (QueuedMessageSnapshot in packages/runtime-host/src/protocol/message.ts). The renderer-side pagination loop in session-workspace-actions.ts is deleted and consolidated into the IPC handler.

Tests: 8 new hook-level tests drive the production useQuoteCompanion with mocked ports; each fails on the old behavior (no queue concept, single admission slot blocking multiple queues, settled messages dropped on Turn handoff). IPC tests cover batching, duplicate rejection, and invalid input. CI test is green on this exact head.

P3 (recorded, non-blocking): the 4096-entry cap on the cancellation-proof query is 64× the queue cap; only a constructed over-limit pending set would hit it, and the failure degrades to stale optimistic rows until the canonical echo (pathological, not reachable in normal use).

Not verified: I did not run the suites locally (CI is green on the exact head); the queue UI interactions in a real Electron shell (drag-reorder etc.) are not covered by automated tests — a manual pass (Enter to queue, Shift+Enter to steer, reorder/delete) is worth doing.

No P0–P2 findings. This is a behavior-changing fix; I am not approving — the merge decision belongs to humans.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I rechecked head 19d0d8efe701fd226470a88d6c6d8504deab08eb and found one recoverable follow-up/reconnect race, detailed inline. The Host remains the right admission authority; the issue is how the renderer reconciles a late admission receipt with the Turn's current state.

The exact-head CI check is green. This finding was traced through the production retry, durable-receipt, and renderer event paths; I did not run a live Desktop reproduction.

AI review disclosure: reviewer agents assisted with this review, and the coordinating agent cross-checked the finding against the source. This is not independent human verification.

中文

感谢修复。本轮复核发现一处可恢复的追问/重连竞态,已放在行内。Host 仍然是正确的准入权威;问题在于 renderer 将晚到的准入收据当作当前仍在执行的证明。当前 head 的 CI 已通过;本次依据真实重试、持久收据和事件消费路径进行源码核对,未运行实时 Desktop 复现。本评审由 AI reviewer 协助,并经主代理复核,不代表独立人工验收。

Comment thread apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts Outdated

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review disclosure: this correction was made by the coordinating AI agent after checking the independent reviewer’s storage-path evidence.

Withdrawn — the normal queued handoff does publish a durable admission.

Thanks, and apologies for the incorrect finding. A further source check found the missing link in my analysis: sqlite-session-metadata-store.ts:2410-2419 adds steeringEventId: steeringProof?.eventId ?? messageId when committing the follow-up handoff. noteDurableTranscriptMessages() can therefore emit message_admission, and the hook adopts that Turn. The normal-path P1 above does not hold and requires no change.

The earlier late-started P2 is separate: a receipt can arrive after its Turn has already settled. I have also corrected that comment so it does not rely on the incorrect generalization about root admissions.

中文

撤回:正常排队交接会发布持久化准入事件。

抱歉,这条意见有误。再次核对发现,我漏查了存储写入点:sqlite-session-metadata-store.ts:2410-2419 在提交 follow-up handoff 时补充 steeringEventId: steeringProof?.eventId ?? messageId,因此 projector 可以产生 message_admission,hook 也会接管对应 Turn。上述普通路径 P1 不成立,无需为它修改代码。

此前“迟到 started”的 P2 是另一个问题:收据可能在 Turn 已经收束后才到达。我也已修正那条评论的描述,去掉关于 root admission 的错误泛化。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. After checking the storage-written admission and the observer replacement path separately, one additional P2 remains specifically on reconnect to an already-terminal successor; details are inline. The earlier normal-online P1 has been withdrawn and resolved. The two remaining P2 cases concern recovery/late-response reconciliation, not a missing admission on every queued handoff.

Reviewed head 19d0d8efe701fd226470a88d6c6d8504deab08eb. This is a source-traced finding; I did not run a live Desktop reproduction. AI reviewer agents assisted, and the coordinating agent checked the producer/consumer paths.

中文

感谢。分别核对存储层补写的 admission 与 observer replacement 后,确认重连至已完成后继 Turn 时还有一条 P2,详见行内。此前正常在线路径的 P1 已撤回并关闭。当前两条 P2 都涉及恢复/迟到响应的对齐,而非所有正常排队交接都缺少 admission。

本次核对相同 head,依据源码生产链路,未运行实时 Desktop 复现。由 AI reviewer 协助并经主代理核对生产者与消费者。

});
const observationSeeded = () => {
resolveReady();
void retireCancelledOptimisticMessages(forkId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reconcile admitted follow-ups when reseeding a terminal successor

There is a recovery case beyond cancelling optimistic rows: submit a next_turn follow-up and receive queued, then lose observation while Host hands off from A to B and completes B. On replacement, runtime-host-session-observer.ts:1510-1559 seeds the terminal events, but calls seedActive() only for a non-terminal root. The durable admission map is initialized in the new projector; it is not replayed as message_admission for terminal B on this path.

The hook therefore still owns A (or no active Turn), filters B's terminal/text events, and this ready callback only queries cancelled IDs. B was admitted, not cancelled, so that query cannot bind B. Even if A's settlement reads B's stored messages, the ownTurnIdsRef filter keeps B's completed reply out of the displayed transcript.

Please reconcile pending follow-up identities with the canonical accepted/terminal state on reseed, or have terminal replacement replay the corresponding durable admission before its terminal events. A focused regression should cover queued → disconnect → B completes → replacement seed and assert that B's settled reply is visible and the panel is not streaming. This is distinct from the normal online handoff, which does emit admission, and from the late-started receipt case.

中文

[P2] 对终态后继 Turn 重建观察时,也需要对齐已准入的追问。

除了取消 optimistic 行,还有这条恢复路径:next_turn 已返回 queued,随后观察断线;Host 在此期间从 A 交接到 B,并完成 B。replacement 会补发终态事件,但仅在 root 非终态时调用 seedActive()。新 projector 初始化了持久 admission map,却不会在这条 terminal-B 路径重放 message_admission

因此 hook 仍只拥有 A(或无 active Turn),B 的文本/终态被过滤;这里 ready 后只查询 cancelled IDs。B 已准入而非取消,这个查询无法绑定 B。即使 A 的 settlement 读取到了 B 的消息,ownTurnIdsRef 过滤仍会让 B 的完成回复不可见。

建议在 reseed 时用权威 accepted/terminal 状态对齐 pending follow-up identities,或让 terminal replacement 在终态前补发对应的 durable admission。回归覆盖 queued → 断线 → B 完成 → replacement seed,断言 B 的回复可见且不处于 streaming。它不同于正常在线交接(有 admission),也不同于迟到的 started 收据。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7a2a0b3b1c. Terminal replacement projection now replays the durable admission via seedActive(false) before seeding B text/terminal events, so the hook binds the admitted follow-up identity before filtering or settling those events. I added the requested queued -> disconnect -> B completes -> replacement seed regression and assert that B reply is visible and the panel is not streaming. Focused tests (90/90), Desktop typecheck, Biome, renderer architecture checks, and the full PR CI all pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked at 7a2a0b3b1c76dc8180454096f5dd5348a6d5ae12: the current patch fixes the single-successor example above, but the same recovery obligation still has a multi-successor gap.

[P2] Recover every queued successor's ownership after a subscription gap

Thanks for adding terminal-root admission replay. It still misses a supported recovery sequence: queue F1 and F2 while A runs, lose observation, then let A → B (F1) → C (F2) complete before reconnecting. The replacement compares the old A snapshot with the latest C snapshot. seedActive only emits admission for C, so B is not added to Side Conversation's ownTurnIdsRef; its durable user and assistant messages are then filtered out of the visible transcript.

The missing binding is not repaired by rereading the transcript: the replacement projector is initialized with the durable message-to-turn map, so noteDurableTranscriptMessages does not emit that same B binding again. Nor does retrying the original queued submit provide it: durable submit replay retains the original followup disposition without a successor turn ID. Two explicit follow-ups intentionally create two successor roots, so this does not require an unsupported queue shape.

Please reconcile the unresolved message IDs against the existing queryMessageExecutions authority on observation seed: retain every returned owned Turn, retire cancelled IDs, leave pending IDs visible, and merge the canonical transcript. That covers intermediate successors as well as the latest root. Add a recovery regression crossing A → B → C during one observation gap and assert both completed replies render. Keep the current observer shortcut until its other consumers and replacement coverage are verified.

Reachability: category 2 (disconnect/reconnect); impact: a settled reply remains hidden, while durable Host data is intact. This finding is based on the production observer/projector/submit-replay and hook filter paths; no live Electron outage reproduction was run in this audit. AI-assisted review by Codex.

中文

在 A 运行时排入 F1、F2,观察连接断开期间依次完成 B、C,重连只比较旧 A 与最新 C,并只补 C 的 admission。B 不会加入侧边对话的 ownTurnIds,已持久化的用户消息和回复仍被过滤。重新读 transcript 不补发,因为新 projector 的 durable map 已记住 B;重放排队提交也只返回原 followup disposition,不带后来生成的 B turnId。建议在 seed 时用已有 queryMessageExecutions 查询未决 ID,一次恢复 owned/cancelled/pending,并合并 durable transcript。补 A→B→C 跨一次断线的回归。此项依据实际源码链路,尚未做真实 Electron 断网复现;数据仍在 Host,只是回复持续隐藏。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing Side Conversation follow-ups onto the shared queue projection. Two P2 recovery issues remain on this head: a late started receipt can re-arm an already settled turn, and a terminal successor can lose its admission on reseed, hiding its completed reply. Please address both before approval, preserving canonical Host ownership and covering the two event orderings in regressions. The withdrawn normal-handoff P1 requires no action.

This is an AI-assisted review follow-up, not independently human-verified.

中文

感谢让侧边对话复用队列投影。当前 head 仍有两处 P2:迟到 started 收据会重新激活已结束 Turn;重建观察时终态后继 Turn 的准入可能丢失,使已完成回复不可见。请在批准前修复两处问题,保持 Host 权威,并用回归覆盖对应事件顺序。已撤回的正常交接 P1 无需处理。这是 AI 辅助跟进,未经独立人工验证。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up visibility work. The combined correctness and simplification audit found one remaining transient-state lifecycle issue; details are inline. AI-assisted review by Codex.

中文本轮结合正确性与简化审查,发现一处临时状态未退休的问题,详见行内评论。

const retireCancelledOptimisticMessages = useCallback(async (forkId: string) => {
// Every manageable queue entry is also a transient transcript row, so this
// projection is the complete set of renderer-owned message identities.
const messageIds = pendingUserMessagesRef.current.map((message) => message.id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for covering reconnect cancellation recovery. [P2] Retire durable messages from the pending identity set before querying cancellations.

Reachability: a normal long-lived Side Conversation followed by a reconnect (category 2). Successful root/next-Turn messages remain in pendingUserMessagesRef: the render-time durable-ID filter only hides them, and the admitted branch does not remove them. This callback therefore queries the entire accumulated history, rather than the remaining transient messages. After 4,096 retained identities, sessions:queryCancelledMessages rejects the request; this callback swallows that rejection, so an actually cancelled optimistic row can no longer be retired on reseed.

I checked this at 7a2a0b3b using the existing real hook harness: after a successful root message and a settled follow-up were both in the owned durable transcript, the visible transient list was empty, but the next observation-ready callback still queried both IDs. The same-head IPC handler rejects 4,097 IDs.

Please retire pending entries once their durable counterparts are actually renderable, preserving the existing own-Turn visibility fence for unknown admissions, and query only the remaining identities. The main conversation's reconcileTransientMessages already follows the durable-retirement rule. Increasing the query limit would retain the underlying accumulation.

AI-assisted review by Codex; production paths and the focused hook probe were independently checked.

中文

正常长期侧边对话在重连时可触发:成功消息只在渲染时被 durable ID 过滤,没有从 pending state/ref 删除。实测一次 root send 和一次已完成 follow-up 后,临时消息显示为空,但重连取消查询仍带上这两个历史 ID。累计超过 4096 条后,IPC 拒绝整个查询,catch 静默忽略,真正被取消的临时消息便无法在 reseed 时清理。建议以“属于当前侧边对话且已能实际渲染的 durable 消息”为依据退休 pending 状态,只查询余下 ID;保留 unknown-admission 的可见性保护,不要提高上限来掩盖积累。

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for bringing Side Conversation follow-ups onto the existing Host queue. After tracing both chat surfaces at 7a2a0b3b1c76dc8180454096f5dd5348a6d5ae12, I would tighten the design before adding more side-panel recovery branches.

The Host still owns admission, queue ordering, consumption, retraction and durable execution. The concern is a second renderer implementation of the same transient-message lifecycle:

  • Main chat uses projectQueuedTransientMessages from transient-message-projection.ts; the new side-chat projectMessageQueue rewrites its merge/retain behavior with an array.
  • Main chat uses reconcileTransientMessages to actually retire durable twins; side chat only filters them from the rendered array. The already-reported pending-history P2 is a concrete divergence between these implementations.
  • Main and side chat now separately implement cancellation-proof querying/removal. Sharing only deriveMessageQueueProjection shares the entry conversion, not these lifecycle rules.

Could we reuse the existing transient projection/reconciliation functions from both production callers, and consolidate any additional common reconciliation rule at that same seam instead of introducing another side-chat controller or a new general-purpose framework? If feature-layer import rules require moving those functions to the application contracts seam, move the existing implementation and update both callers, rather than keeping another copy or a forwarding facade. Each Session can keep its own projection instance; the transition rules should have one implementation. Side chat should pass its actually renderable, own-Turn-filtered durable messages so unknown-admission visibility is preserved.

The whole main-chat workspace controller is not a drop-in dependency: it also owns active-session selection, transcript paging and workspace UI. Keep those concerns separate, and keep the side-specific fork creation, quote handling, fork cleanup and visible-history boundary local. Those differences do not require duplicating queue merge or durable retirement. For reconnect ownership, consume the already existing queryMessageExecutions result rather than reconstructing admission from only the latest root snapshot.

This refines the fix direction for the existing P2 comments; it is not a claim that this PR adds a second Host scheduler, and it is not a request to rebuild all of Side Conversation. Shared-rule tests plus a small set of side-panel integration cases should then cover the behavior without parallel state-machine test suites.

AI-assisted review by Codex; the main/side production callers and existing shared helpers were checked directly.

中文

侧边对话需要消费 Host 的队列能力,不需要另一套队列生命周期实现。Host 执行权威仍在;问题在 renderer:主对话已有 queue merge 和 durable retirement 的共享函数,侧边却重写了数组合并,并把真正退休误写成仅过滤显示,已产生 P2 行为漂移。只共享 entry 转换还不够。

建议让两个生产调用方复用现有 transient projection/reconciliation 接缝,共同规则只实现一次,每个 Session 保留自己的投影实例;侧边传入自己已经实际可显示的 durable 消息,保留 unknown-admission 保护。主对话整个 workspace controller 绑定选中 Session、分页和外壳状态,不能直接搬过去;fork 创建、引用、清理和历史显示边界仍留在侧边。重连归属使用已有 queryMessageExecutions,不继续堆最新 root 特判。这个意见是现有 P2 的设计修法,不是要求重做整个侧边对话,也不是宣称新增了 Host 调度器。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): Side Conversation follow-ups disappear while a Turn is running

2 participants