feat(desktop): add opt-in next prompt suggestions - #5629
Conversation
251f01b to
fcb6752
Compare
2e7e86b to
ac4efdb
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at ac4efdb with four independent passes (Host prediction path, composer/React-185 surface, Desktop IPC + WorkHub wiring, simplification audit).
The prior crash mechanism is structurally absent. The #4117 React-185 loop came from layout-measure→setState oscillation inside contenteditable. This implementation keeps the overlay as a grid-level sibling outside the editable, performs zero DOM measurement, and does all setState in commit-phase effects or promise continuations — no render-phase writes exist. Stale-result defense is triple-layered (epoch counter, live-ref recheck on resolve, committed-props guard at render). The seam contract test is untouched. The smoke verification is real Electron/Chromium — glyph-rect comparison and native undo actually exercised.
The Host side is the right shape. Tool-free call through the existing auxiliary model authority (connection/model resolution, metering, drain) — not a parallel path. Dedup keyed on (turnId, terminalEventId, connection, model) with post-generation revalidation, 128-entry bound, real 5s abort, every failure collapsing to none without leaking internals. IPC surface follows the file's strict owner-scope + epoch fencing; outbound IPC carries only {sessionId}; the paid effect is non-replayed on reconnect. WorkHub is admitted by exact permanent coordination identity and prompts only from recent visible user/assistant text — the distant original task never leaves the Host.
Two P2s inline (usage-ledger callId collision; whole-area click-to-accept), plus a P3 set worth sweeping: eligibility gaps (mode:bot/deep_research/scheduled-task, non-user turn origins, SIDE_CONVERSATION_SESSION_LABEL constant), reconcile transient-failure poisoning, the 5s bound not covering the two readSource calls, getSessionView skipping ensureTranscriptLedgerForRead, Esc priority vs dragActive, collapsed-floating-WorkHub geometry, overlay max-height, the global toggle rendering on panels that can't trigger it, /-prefixed suggestions opening the mention menu, the misplaced readSnapshot JSDoc, and docs/reports/ screenshots+harness committed contrary to the gitignore convention (attach on GitHub instead; .gitattributes goes with them).
One scope question, not a defect: "opt-in" is currently a renderer-localStorage gate — remote-owner grant holders can invoke the op regardless (consistent with remote-owner trust since they can already run paid turns, but worth one line in the PR body, or a policy flag if you meant it as an authority boundary).
Also: the branch conflicts with main on the generated docs/astryx-surface-file-inventory.md — rebase + regenerate will be needed anyway.
| telemetrySessionId: source.sessionId, | ||
| header: { ...source.header, thinkingLevel: 'off' }, | ||
| callKind: 'prompt_suggestion', | ||
| callId: `prompt_suggestion_${source.terminalEventId}`, |
There was a problem hiding this comment.
P2 — callId collides across distinct billed calls (failure/restart + model-switch paths). callId is prompt_suggestion_${terminalEventId}, but recordLlmCallStrict derives the ledger row as usage_${callId} and insertLlmCall upserts on conflict — so a second real call for the same terminalEventId (model/connection switch between retries, 128-entry eviction, or a host restart where terminalEventId persists but the in-memory coordinator does not) silently overwrites the first call's usage row. Every call is metered at write time yet the ledger undercounts. Siblings pass a fresh id (session_title_${sessionId}_${newId()}). Suggest prompt_suggestion_${source.terminalEventId}_${authority.newId()}.
| type="button" | ||
| variant="ghost" | ||
| label={`${suggestionAcceptLabel}: ${nextPrompt.text}`} | ||
| className="maka-composer-next-prompt" |
There was a problem hiding this comment.
P2 — the whole empty input area becomes click-to-accept, with no mouse-only path to dismiss (normal path). The overlay button spans width:100% of an exactly-one-line-tall composer and cursor:text invites 'click to place caret' — so clicking into the composer to start typing inserts up to 512 chars of suggested text instead (undoable, but surprising). Copilot-style ghost text never accepts on clicking the ghost region. Smallest fix: shrink the hit target to the text itself (justify-self:start; width:fit-content) leaving the row's trailing space for focus, or make click focus-only and keep accept on Tab/chip. If whole-area-accept is intentional it's worth a sentence in the PR description.
| sessionId === WORKHUB_COORDINATION_SESSION_ID)) && | ||
| !header.subagentParent && | ||
| header.collaborationMode !== 'plan' && | ||
| !header.labels.includes('mode:side_conversation') && |
There was a problem hiding this comment.
P3 — eligibility list misses a few non-ordinary shapes (normal path). supportsPromptSuggestion doesn't exclude mode:bot / mode:deep_research labels, scheduled-task sessions, or non-user turn origins (scheduled_task/agent_graph/goal user-message origins are available on StoredMessage) — a completed automation turn can produce a metered 'next user message' suggestion from machine-authored content. Also prefer the existing SIDE_CONVERSATION_SESSION_LABEL constant over the literal. Cheap additions to the same predicate.
| async reconcile(sessionId: string): Promise<void> { | ||
| const entry = this.#entries.get(sessionId); | ||
| if (!entry || entry.abort.signal.aborted) return; | ||
| const current = await this.ports.readSource(sessionId).catch(() => undefined); |
There was a problem hiding this comment.
P3 — a transient readSource failure permanently poisons the turn's dedup entry (failure path). readSource(...).catch(() => undefined) treats a store hiccup like 'source gone' → aborts the in-flight entry, and aborted entries are never deleted (only key-change or the 128-cap evict), so generate returns {kind:'none'} for that key forever after one transient failure. this.#entries.delete(sessionId) next to the abort in reconcile (and/or dropping aborted entries in generate) closes it.
ac4efdb to
743db83
Compare
liugddx
left a comment
There was a problem hiding this comment.
Reviewed exact head 743db83. This builds on Astro-Han's pass; nothing he already raised is repeated here.
The overall shape is right, and I want to say that plainly. Pulling on demand from the surface that watched the turn end is the right layer. A Host push would need the opt-in to live on the Host, and would bill every turn whether or not anyone is looking. Keeping the overlay a sibling of the contenteditable with no measurement removes the #4117 mechanism by construction. mode: 'command' plus the non-reconnectable IPC registration means the paid effect is never replayed. I tried to break each of those and couldn't (the dead ends are listed at the end).
What I did: checked out the head, ran npm ci, built core → storage → mcp → runtime → runtime-host → ui plus desktop build:test, and ran the touched suites:
- runtime-host
prompt-suggestion11/11 - runtime-host composition, the "Host auxiliary models meter" case, 1/1
- ui
prompt-suggestion+ seam 6/6 - storybook-smoke 15/15
- cli operator-command 7/7
- desktop session-execution IPC 53/53
I also mutated the built output to see which production lines the tests actually pin (see Tests). I did not run the Storybook play functions.
P2 — thinkingLevel: 'off' is silently dropped for most reasoning models, so the 128-token call ends in length and returns nothing, but is still billed
execution-model-authority.ts:402 overrides the header to 'off'. resolveThinkingLevel (core/model-thinking.ts:386-393) keeps a level only when the model declares it, and 'off' comes only from offBehavior or an effort 'none' (:110-112). When it is dropped, thinkingLevel is still 'off' (not undefined), so the OpenAI branch (runtime/model-factory.ts:568-580) sends no reasoningEffort at all, and the provider runs its default effort.
That applies to:
gpt-5/-mini/-nano,o1/o3/o3-mini/o4-mini, and the-provariants: nononein the metadata snapshot.gpt-6-sol/gpt-6-luna:OPENAI_GPT6_THINKING_OPTIONS(core/model-metadata.ts:275-277) deliberately stripsnone.- Gemini 3.x: the Google branch sends
{}(model-factory.ts:611-630). - Probably DeepSeek v4 as well; I did not verify its default.
The path from there: default reasoning uses up maxOutputTokens: 128 (:411), finishReason === 'length' makes :414 return undefined, and the result collapses to {kind:'none'}. The usage row is still written as status: 'success', reasoning tokens included.
When reasoning instead runs past the 5 s deadline, the row is written as aborted with 0/0 tokens (:717-727), while the provider has very likely billed the call.
So on a session running one of the current flagship reasoning models, the feature silently never shows a suggestion and costs a call per reply.
Fix:
- Resolve the lowest effort the model actually offers (
'off'if present, else the lowest ofminimal/low) instead of hard-coding'off'. - Or gate eligibility on
thinkingVariantsForConnection(...).includes('off'). - Either way, add
maxRetries: 0. Every other auxiliary path in this file sets it (:255,:277,:323,:329), and without it the AI SDK retries twice under one usage row.
P2 — The opt-in is read once per renderer, so turning it off in the main window does not stop WorkHub
ConversationServicesProvider does useState(() => port.readEnabled()) (renderer/features/conversation/services.ts:28) and never listens for storage. WorkHub is its own WebContentsView renderer (main/workhub-presentation.ts) and shares the same localStorage, but it keeps its own in-memory copy of the flag.
Scenario: the user turns suggestions off from the + menu in the main window to stop spending. A mounted WorkHub keeps enabled = true and keeps paying for a call per reply until it remounts.
Fix: subscribe to the storage event (or useSyncExternalStore). That's about three lines. While there, use the existing safeLocalStorageGet/Set (renderer/browser-storage.ts:20-28) instead of the hand-rolled try/catch at create-conversation-services.ts:36-41.
P2 — User turns are read from message.text, not userFacingText, so Skill invocations send the Skill body as the user's "original goal"
prompt-suggestion.ts:64-65 takes message.text and keeps its tail (slice(-2000)). For a Skill invocation, text is the composed invocation envelope; the human's words live in displayText. That is why core/session.ts:795 has userFacingText, and goal-coordinator.ts uses it for the same "recent six" projection.
Scenario: a session that starts with /some-skill fix the login sends the last 2,000 code points of the injected Skill body, labelled as the user's original goal. That costs more tokens, gives a worse prediction, and forwards Skill content the user never typed.
Fix: use userFacingText(message).
Duplication — two second copies of logic that already has an authority
- Output cleaning.
cleanPromptSuggestion(prompt-suggestion.ts:77-98) is a narrower copy ofcleanGeneratedSessionTitle(session-title.ts:57-81) and misses what that one learned.- It does not strip
<think>…</think>, and it rejects any output containing<. So models that inline their thinking over OpenAI-compatible wires (Qwen3 / R1-style) always returnnone— billed every turn and never shown. - It also rejects backticks, which throws away legitimate coding suggestions such as
run `pnpm test`. - Fix: factor out "strip think → first line → unquote" and share it.
- It does not strip
- Abort racing.
beforeDeadline(prompt-suggestion.ts:100-112) duplicatesreadDuringBackendCreationin the very file that calls this coordinator (execution-model-authority.ts), andabortableinclient/wait-for-ready.ts.
P3 — The coordinator can be smaller, and it releases residency before the model call it raced has finished
- Residency is released too early.
beforeDeadlineraces theports.generatepromise but never tracks it. On a deadline or drain,finallyreleases residency (:188), andclose()awaits the task that already lost the race (:219). The underlyingrunHostAuxiliaryModelCallcan therefore still be writing its usage row and closing its transport after the Host considers itself idle or closed (plausible; not reproduced). - The "LRU" is not LRU. Entries are never deleted on completion, which is the only reason the 128 cap and
reconcileexist. And becauseMap.seton an existing key keeps its insertion position, eviction is by first insertion, not by recency. - What reconcile actually buys. The post-generation re-read at
:166already guarantees correctness;reconcileonly saves at most one 5 s, 128-token call. - Fix: per-session single-flight that deletes the entry in
finallyand tracks the real generate promise. Or fold this in as a third effect ofHostSessionEffectCoordinator, which already has#accepting/#track/ drain / close.
P3 — Say what is sent and where the cost lands
- The copy. It says "Uses an extra model request after each reply" (
ui/conversation-copy.ts). It does not say what is sent: the first user message plus the last six visible messages, each up to 2,000 code points, to the session's own model — worst case about 14k characters that do not share the main loop's cache prefix. Jev's copy (settings-jev-copy.ts) is the precedent for spelling this out. - Where the cost lands.
telemetrySessionId: source.sessionId(:401) puts the spend into the Session Inspector cost, intousageCacheHitRate(cacheRead ÷ input), and into Daily Review request counts.UsageQueryhas nocallKindfilter to separate it out. - That attribution matches title and recap, so it isn't wrong. But those run once per session and this runs every reply. On a 95%-cached 100k main turn, one uncached 10k suggestion moves the displayed hit rate to about 86%.
- Fix: a line in the copy and in the PR body, or have the hit rate count
mainonly.
P3 — The seam test header cites evidence that is not in the tree
The regexes are unchanged, so the guard is not weakened. Correcting Astro-Han's "untouched" for the record, though: the header and both failure messages were rewritten (−37).
- The factual fix is right. The engine came from Maka's own
patches/@astryxdesign+core+0.4.0.patch, not from upstream Astryx. - The downgrade to "suspected mechanism" rests on evidence reviewers cannot see. It cites
docs/reports/prompt-suggestion-repro/README.md(:30), which does not exist at this head; the 2,040-case sweep has no reviewable artefact. - Guidance was lost. The rewrite also drops the "what a safe reintroduction needs" guidance, in the same PR that reintroduces a Tab-accept offer.
- Fix: keep the factual correction, link the evidence at a fixed commit (as the PR body already does for the screenshots), and either restore that guidance paragraph or replace it with the overlay invariant: no layout-driven state.
Tests
Everything I ran passes. Mutating the built output shows what is actually pinned.
Survived — not pinned:
- M1: deleting the four identity comparisons in the post-generation revalidation, keeping only
!current. - M3: deleting the assistant-voice rejection regex (
I'll|Let me|我来|建议你…). - M4/M5: deleting
finishReason === 'length' → undefinedand the'off'override. The composition case asserts neither the request's reasoning parameters nor thelengthhandling, which is exactly how the P2 above got through.
Caught — pinned:
- The renderer epoch discard.
reconciledeleting its entry.
Worth adding:
- A composition case with a fake provider returning
finishReason: 'length', asserting the request body's reasoning fields. generateracing a source change withoutreconcile→none.- The
readSourcegate inexecution-composition.ts(about 45 lines: incognito, non-completed turn, archived, pending interaction/queue, active goal, running turn), which has no test. One table-driven case that flips each flag and asserts{kind:'none'}with zero provider calls would cover it. - A linkedom composer case for Tab while
aria-expanded="true"and Tab during composition.
Smaller notes:
- The deadline case burns 5 s of real time; inject the timeout.
- The title of
:242("transient failure does not poison…") claims more than it asserts. Add acallscount so it pins one of the two behaviours.
Nothing to delete.
Dead ends (so nobody re-runs them)
- Replay on reconnect. Not possible:
mode: 'command'is not retryable (client/connection.ts:338), and the IPC is registered with a plainhandle. - Repeated generation per turn from the renderer. The effect requires a
streamingtrue→false edge, and the Host dedups on the turn key. - Exhausting the 64 in-flight cap. The client queues at 63 rather than failing the connection.
- Suggestions after failed or aborted turns.
readSourcerequiresstatus === 'completed'. - The dependency-less effect at
composer.tsx:875. It writes no state, so the #4117 area has no regression here.
Net: the smallest correct version is this PR with:
- reasoning effort resolved per model (or eligibility gated on a real
off) plusmaxRetries: 0; - the toggle subscribed across renderers;
- user text read through
userFacingText; - cleaning shared with the title cleaner;
- the coordinator reduced to per-session single-flight.
Everything else is polish.
| const result = await runHostAuxiliaryModelCall(authority, { | ||
| transportContextId: source.sessionId, | ||
| telemetrySessionId: source.sessionId, | ||
| header: { ...source.header, thinkingLevel: 'off' }, |
There was a problem hiding this comment.
P2 — reasoning effort is not actually off for most reasoning models. 'off' only survives resolveThinkingLevel when the model declares it. For gpt-5/o-series, GPT-6 Sol/Luna (OPENAI_GPT6_THINKING_OPTIONS strips none) and Gemini 3.x it is dropped, and because thinkingLevel is non-undefined no default effort is sent either, so the provider's default reasoning eats the 128-token budget and :414 turns the call into none while billing it. Suggest resolving the lowest supported effort (or gating eligibility on a real off), plus maxRetries: 0 like the other auxiliary paths in this file.
| ): string { | ||
| const visible = messages.flatMap((message) => | ||
| (message.type === 'user' || message.type === 'assistant') && typeof message.text === 'string' | ||
| ? [{ role: message.type, text: Array.from(message.text).slice(-2000).join('') }] |
There was a problem hiding this comment.
P2 — read user turns through userFacingText. message.text is the composed envelope for Skill invocations; userFacingText(message) (core/session.ts) is the human-facing authority and is what goal-coordinator uses for the same projection. As written, a /skill … first message sends the tail of the Skill body as the "original goal".
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
…rchives Generated-by: OpenAI Codex
Preserve editor typography and automatic height while using the required control primitive. Refresh the surface inventory and revalidate Electron and Storybook geometry. Generated-by: OpenAI Codex
Include suggestion generation in desktop and terminal access presets; retain the operation inventory test and explicitly cover both presets. Generated-by: Codex
Remove raw experiment output and temporary crash-reproduction files. Keep a concise report, four screenshots and native acceptance harnesses; write future harness output outside the checkout. Generated-by: Codex
Give each billed call a unique identity, keep ghost text click-through, exclude automated turns and bound source reads. Retire invalidated cache entries, align collapsed WorkHub overlays and remove review artifacts from source control. Generated-by: Codex
Skip reasoning models without off, disable retries, use user-facing transcript text and share generated-text cleanup. Observe cross-renderer preference changes and retain residency through model cleanup. Cover provider request policy, stale identities and delayed cleanup. Generated-by: Codex
14d08f3 to
a1e4d11
Compare
Remove the forbidden dependency on legacy renderer browser-storage while preserving guarded storage access and cross-window preference subscriptions. Generated-by: Codex
Summary
Adds opt-in next-prompt suggestions to ordinary AI SDK conversations and WorkHub. After a completed reply, an empty composer may show one suggestion with the same position, typography and wrapping as normal input. Tab accepts an editable draft; clicking focuses the input without accepting; Enter sends and Esc dismisses. Typing, navigation, hidden interaction forms and newer turns invalidate stale results.
Runtime Host owns bounded, tool-free prediction requests through the existing credential/model authority, with usage metering, a five-second timeout and per-turn deduplication. The feature defaults off. WorkHub admits only its permanent coordination identity and uses recent visible conversation rather than the session's distant original task. Background subagents and other existing eligibility restrictions remain excluded. The overlay stays outside contenteditable and has no layout-measuring state loop.
Rebased onto main
acaa29e40, preserving delegated-result notifications and the updated executor picker.Refs #4117. A separate investigation exercised 2,040 natural Chromium cases without reproducing React 185. Artificial geometry oscillation also did not reproduce the crash. The old engine came from Maka's Astryx patch. This PR corrects overly certain historical commentary; it does not claim to fix that crash.
The opt-in setting is a renderer-local preference, not a Host authorization boundary. Authenticated remote owners can invoke prediction just as they can invoke paid turns.
Review fixes add unique per-call usage IDs, click-through ghost text, automated-session/turn filtering, a deadline covering source reads, recovery after transient reconciliation failures, and collapsed WorkHub geometry constraints. Review artifacts are excluded from the current branch; the historical evidence below uses a fixed commit link. Native ordinary/WorkHub acceptance and 60 Host tests passed after the review changes; full build, typecheck and formatting passed.
Additional review fixes: read user messages through
userFacingText, synchronize the opt-in across renderer storage events, share first-line/think-tag cleanup with title generation, and keep operation residency until actual model cleanup completes. Declared reasoning models without a supportedofflevel are skipped before provider invocation; supported requests explicitly disable reasoning and retries. Prediction costs count toward the session totals and cache-hit statistics. The bounded per-turn result cache remains to prevent duplicate paid requests across clients; it is insertion-ordered, not LRU.Validation includes provider request reasoning fields, truncated responses, no retry on HTTP 503, a reasoning-only model making zero provider requests, source-identity changes without reconciliation, and close waiting for delayed model cleanup. The cross-renderer adapter event test and WorkHub native acceptance pass. A full multi-window UI journey, exhaustive Host eligibility table and IME/mention-menu Tab cases were not added in this revision.
Verification
Product/WorkHub/NextPromptSuggestionuses the production WorkHubRoot/controller/Composer with mocked service transport. All four real Chromium plays completed at 1280/720px × light/dark, including exact before/after-Tab glyph rectangles and typography, accepted Enter and Esc.playFunctionThrewExceptionlistener in the smoke probe; the listener and regression coverage are included. Smoke-runner unit tests: 15 passed. The CI catalog explicitly schedules the four viewport/theme combinations.Historical UI evidence and reproduction commands (before review fixes)
Screenshots are from the built Storybook in the in-app browser; the assertion matrix ran in Ego Chromium. Ego screenshot calls timed out, so these are not labeled as Ego captures.
AI use
Tool(s) and scope: OpenAI Codex implemented the feature, tests and documentation; local Claude Code was used during initial behavior research. Generated-by trailers are included.
Checklist
Does this PR entail a change in behavior?