feat: collapse compaction surface to a single four-state divider - #836
Conversation
ProcessCompaction now creates its summary assistant placeholder up front and routes pre-summary failures (agents/provider/select/plugin/ toModelMessages/processors.create) plus abort interrupts through the same record. Without this, the UI divider never sees an error/finish signal for those paths and stays pending forever. Also tags re-injected overflow user messages with replay:true so the renderer can collapse them without inspecting parts heuristically.
The compaction turn used to render the summary markdown then a second copy of the original user message through the synthetic continuation / replay flow. UX-wise that re-exposes a purely technical process to every user. This change reroutes both halves so a compaction turn now shows only a thin divider with four real states (pending/done/aborted/failed) plus a neutral elapsed timer in pending — matching the running-tool TextShimmer language we already use elsewhere. State derivation lives in a new helper file (pure functions covered by unit tests) so the rules — abort/error precedence over time.completed, ContextOverflowError as its own label, elapsed-start switching once the summary assistant appears — can be regression-tested without rendering. session-turn.tsx now splits raw vs visible assistant lists so the summary message reaches only the divider and never leaks into "Thinking…", error cards, copy targets, turn-duration math, or assistant rendering. Compaction placeholder users and replay/synthetic-continue user bodies hide their content while keeping the turn row so child assistants still render through parentID.
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds backend early placeholder compaction assistant and terminalized abort/error handling, prelude/runner wiring for compaction runs, frontend divider state helpers and MessageDivider rendering (icons + elapsed), SessionTurn message filtering to isolate summaries, expanded tests, and i18n for compaction states. ChangesSession Compaction Divider UI
Sequence DiagramsequenceDiagram
participant Client
participant Server as SessionPrompt.loop
participant Compaction as CompactionProcessor
participant Placeholder as PlaceholderAssistant
participant UI as SessionTurn
Client->>Server: request summarize (prelude compaction)
Server->>Compaction: create marker & provisional placeholder
Compaction->>Placeholder: write provisional assistant (provisional model)
alt success
Compaction->>Placeholder: update with real model + summary
Placeholder->>UI: visible as done
else abort / interrupt
Compaction->>Placeholder: set error=MessageAbortedError, finish="error", time.completed
Placeholder->>UI: visible as aborted
else pre-summary error
Compaction->>Placeholder: set error, finish="error", time.completed
Placeholder->>UI: visible as failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Astro-Han
added
enhancement
github-actions
Bot
added
harness
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Perf delta summaryComparator: pass
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the session compaction logic to provide better UI feedback and robust error handling. Key changes include the early creation of a placeholder assistant message to track compaction states (pending, aborted, failed), filtering summary messages from the main conversation view, and updating the UI to display progress indicators and elapsed time. Feedback was provided regarding the onInterrupt finalizer in the compaction logic, suggesting that errors from session.updateMessage should be explicitly propagated to ensure proper failure handling downstream.
Drives done/failed/pending/aborted through real production e2e: - done: SUMMARY_TEXT reply + summarize - failed: HTTP 400 from LLM endpoint, non-retryable APIError path - pending: assistant.hang() + summarize (fire-and-forget) - aborted: session.abort after pending state captured The pending/aborted pair shares one session so the placeholder summary assistant transitions through both states in sequence.
`compactionDividerLabelKey` was reading `error.message` (top-level), but
`NamedError.toObject()` returns `{ name, data: { message, ... } }` — so
the reason was always undefined and the failed divider rendered as just
"Compaction failed:" with no cause shown. Prefer `error.data.message`
and keep the top-level fallback so helper-only synthetic error shapes
still work in unit tests.
Updated the failed-label helper tests to assert against the real APIError
shape (data.message with the surrounding fields) instead of the synthetic
top-level form that hid this gap.
Pre-summary failure (outcome.ok=false) and onInterrupt (abort) branches only wrote error + finish=error onto the summary placeholder. The UI's `pending` memo scans `allMessages()` (which includes the summary assistant) for assistants without `time.completed`, so an aborted / pre-summary-failed compaction looked in-flight to other turn-level consumers driven by that memo. Both terminal branches now stamp `time.completed = Date.now()` alongside the error fields. The abort-before-processor test gets a matching assertion so the contract stays locked.
MessageOutputLengthError and other NamedError variants with empty `data` schema produce no `data.message`, so the divider previously rendered "Compaction failed:" / "压缩失败：" with a dangling colon. Route the empty-reason case to a separate `failedUnknown` i18n key without the colon template.
|
Triaged the latest review: P3 — empty reason produces trailing colon: fixed in 146e022. P2-1 — P2-2 — legacy sessions with a compaction part but no summary assistant render as permanent pending: pushing back. |
The /:sessionID/summarize route called SessionPrompt.loop and returned c.json(true) unconditionally. Pre-summary compaction failures (agents.get / provider.getModel / select / plugin / toModelMessages / processors.create) land the error on the placeholder summary assistant and return "stop" without throwing, so SDK callers would resolve `true` for a session that visibly failed. After the loop returns, scan back to the latest compaction-mode assistant message. If it carries a non-aborted error, rethrow it as UnknownError so the global ErrorMiddleware emits a structured 500. User aborts are not failures from the route's perspective and continue to return true. The compaction-divider snap now asserts that summarize rejects on the failure branch.
Pre-PR compaction process() created the placeholder summary assistant only after agents.get / provider.getModel / select / plugin / toModelMessages / processors.create succeeded, so historical pre-summary failures never wrote a summary assistant at all. The new state machine returned "pending" whenever the summary was absent, which would render an infinite shimmer on those orphan records. Add hasLaterTurn input to compactionDividerState: when no summary assistant exists and the session already moved on with another turn, return "failed" (which the label key already routes to the no-colon "failedUnknown" variant when no error data is available). The live race window (placeholder about to land on the latest turn) still returns "pending". session-turn.tsx derives hasLaterTurn from messageIndex vs the full message list and threads it through. The contract test now guards the hasLaterTurn handoff.
|
Reversing my earlier push-back on both P2 items. Re-reading P2-1 fixed in d92465f. P2-2 fixed in 17c6428. Extended Verification: opencode 44/44, ui session-turn-compaction + contract 34/34, all three typechecks clean. Five pre-existing baseline failures unchanged (documented in the PR body). |
The previous hasLaterTurn heuristic missed the case where a legacy orphan record was itself the last message in the session: a pre-PR pre-summary failure that stopped on the latest turn would still render infinite "Compacting…" shimmer because no later turn existed to flip the gate. Replace hasLaterTurn with isWorking, derived from the existing working memo (isWorkInFlightStatus(status) && active()). The divider now only returns "pending" when the session is actually doing work for this compaction turn; otherwise an absent summary means failed/unknown. Status and working memos are hoisted above the divider memo so the forward reference resolves at Solid memo creation time.
|
Fixed in 16b97fb. The previous Replaced
Verification: 34/34 compaction unit + contract tests pass (orphan-as-latest-turn case now covered), all three typechecks clean, snap re-run shows the four-state grid unchanged. |
Auto-compaction calls SessionCompaction.create from inside runLoop where status is already busy. The manual summarize route called create first, then SessionPrompt.loop set busy. The compaction part event reached clients before the status event, so the divider rendered the legacy-orphan "failed" state for one frame before busy landed. Flip status to busy in the route handler before create, matching the auto path's event ordering.
|
Confirmed the race. Timeline I traced:
Fixed in |
The previous busy-before-create fix sat outside the Runner, so a cancel in the pre-loop window hit SessionRunState.cancel's no-runner branch and was silently dropped. Move marker creation into a `prelude` on SessionPrompt.loop, executed inside ensureRunning's work fiber. The work starts with status.set busy (still ahead of the part event, preserves the divider flash fix) and then runs compaction.create, so any cancel during setup hits a Running runner and Fiber.interrupt fires. The summarize route now passes prelude instead of calling SessionCompaction.create directly. Concurrent summarize calls also stop double-writing markers because ensureRunning short-circuits to the existing run. Adds a regression test that hangs the compaction LLM call and cancels mid-stream, asserting the marker was written and the placeholder assistant carries MessageAbortedError.
|
Confirmed and fixed in The prior busy-set sat outside the Runner. Reading Restructure: marker creation now runs inside the loop's runner-protected work effect. Added an optional The summarize route is now thinner — just gathers the current agent and calls Race test in |
Manual compaction routed through SessionPrompt.loop({ prelude }), which
runs marker creation inside the runner's work effect. When the runner is
already Running for another prompt, ensureRunning would short-circuit to
awaitRun(existing) and the prelude effect was never executed, but the
summarize route still resolved with `true` — clients saw success for a
session that never wrote a compaction marker.
Add rejectIfBusy to Runner.ensureRunning so the prelude path can refuse
to silently no-op when the runner is already in flight: the check lives
inside the atomic SynchronizedRef.modifyEffect, eliminating any race
between status read and Idle→Running transition. SessionPrompt.loop
enables rejectIfBusy whenever input.prelude is present; busy callers
get Session.BusyError, mapped to HTTP 400 by the existing middleware.
Builtin slash commands flow through popover-controllers.ts filtering out disabled entries entirely, and the command palette greys disabled rows. Adding isWorkInFlightStatus(status()) to the disabled predicate stops users from invoking compact in the brief window where the server would now reject with Session.BusyError, keeping the in-product paths aligned with the backend's new contract.
|
Fixed in 73aab58 (server reject) + 0d6be60 (UI gate). Root cause confirmed by reading Considered two paths and explicitly rejected one:
Took the third option: UI side: Queueing compact through the followup machinery (so a busy click parks the action and auto-fires on idle) is a UX nice-to-have rather than a fix for this finding — tracking as a separate follow-up so this PR stays scoped to the silent-success root cause. Test added: "loop rejects compaction prelude when a run is already in flight" — pins a normal prompt in Running via Verification: |
…eholder Between `SessionCompaction.create` writing the user message + compaction part and `processCompaction` writing its placeholder summary assistant, a cancel left an orphan: marker present, no assistant, status idle. The divider state machine then read this as `failed` (no summary + not working) instead of `aborted`. Extend `SessionPrompt.loop.onInterrupt`'s fallback branch: when `currentTurnTarget` returns a user message whose parts include a compaction part, write a terminal summary assistant carrier with `MessageAbortedError`, `finish="error"`, `time.completed`, and the same `diagnostics.abort` shape the existing assistant-branch records. The condition is gated on the marker (not `input.prelude`) so auto-overflow compaction calls inside `runLoop` get the same treatment. Test covers the race window by polling for the compaction part and cancelling immediately; asserts the terminal carrier exists with the expected error/finish/parentID regardless of which path resolved the abort (the new fallback or `processCompaction`'s own onInterrupt finalizer if it had already registered).
|
Picked up the P2 about cancel-during-compaction-prelude landing as Verification (verbatim from the code):
Chosen approach: Reviewer's primary option (surgical fallback in What landed (commit c2120f7):
Considered out of scope: the smaller theoretical window inside |
A busy /summarize used to mutate session.revert before SessionPrompt.loop got a chance to reject the call with BusyError, so a rejected compact was not a pure no-op. revert.cleanup now runs inside the work effect, after the Runner has won the Idle slot, alongside status.set and the marker write — the whole prelude is one atomic unit and rejected requests touch nothing. Also tighten the post-marker-pre-placeholder race test: poll for the exact window (marker present, placeholder absent) and assert the new onInterrupt fallback was the path that handled the cancel via propagation_point, so a future regression that drops the fallback would fail the test instead of silently relying on the old finalizer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/e2e/snap/compaction-divider.snap.ts`:
- Around line 96-109: The test currently treats any exception from
projectSdk.session.summarize(...) as proof the compaction failure was surfaced;
change it to capture the thrown error (e.g., const err = caughtError) and assert
that the error carries the injected compaction-failure signal from the test
fixture (use projectSdk.session.summarize, failedSessionID and the
placeholder/error marker your setup injects), for example by checking
err.message or err.code includes the expected "compaction" / failure marker so
the test fails only when the specific compaction failure is observed.
In `@packages/opencode/src/server/instance/session.ts`:
- Around line 1057-1060: The current rethrow uses the provider message verbatim
which can be empty/whitespace; normalize by extracting (info.error.data as
{message?: string} | undefined)?.message, trim it, and if the trimmed string is
non-empty use it as the reason, otherwise fall back to the default `Compaction
failed (${info.error.name})` before throwing `new NamedError.Unknown({ message:
reason })`, updating the logic around `info.error`/`NamedError.Unknown`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f29d11de-1a3d-4b20-a599-48b5c5fd9f90
📒 Files selected for processing (11)
packages/app/e2e/snap/compaction-divider.snap.tspackages/app/src/pages/session/use-session-commands.tsxpackages/opencode/src/effect/runner.tspackages/opencode/src/server/instance/session.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/run-state.tspackages/opencode/test/session/prompt-effect.test.tspackages/ui/src/components/session-turn-compaction-contract.test.tspackages/ui/src/components/session-turn-compaction.test.tspackages/ui/src/components/session-turn-compaction.tspackages/ui/src/components/session-turn.tsx
The previous commit moved revert.cleanup into the prelude work effect but left agent derivation in the route handler, where it read the pre-cleanup message list. For a reverted session this picked the agent off a discarded user message instead of the revert point's last active agent. LoopInput.prelude.agent is now optional; when omitted the work effect derives it from sessions.messages after revert.cleanup, restoring the original cleanup-then-pick-agent ordering inside the atomic transaction. The route handler drops both the messages lookup and the agent loop. Also strengthen the race test setup: observedRaceWindow flag fails explicitly when polling never caught the marker-without-placeholder window, instead of producing a confusing propagation_point mismatch downstream.
The previous onInterrupt fallback only fired when currentTurnTarget returned a user message bearing the compaction part. SessionPrompt .prompt persists its user message before ensureRunning awaitRuns the in-flight compaction, so a normal prompt landing while compaction is running displaces the latest-user pointer to the new prompt's user. The marker stays orphaned, summary assistant never gets created, and the divider renders `failed` instead of `aborted`. Replace the latest-user gate with a sweep: find any user message with a compaction part that has no summary assistant child, and write the aborted carrier against that marker. Covers both the simple race window (marker just written, placeholder pending) and the queued- prompt window where the marker is no longer the latest user. Added a queued-prompt test that injects a fresh user message before cancel and asserts the carrier still lands on the marker. Also normalize empty/whitespace provider error messages when rethrowing from the summarize route — `??` previously surfaced an empty string verbatim, leaving SDK callers with a blank reason.
External review (Codex xhigh + Claude/Codex crosscheck) flagged the sweep's "find newest marker, only act if orphaned" approach as potentially missing older orphans. Recording why iterating to the first orphan would be wrong: it would retroactively rewrite a historical crash-orphan as `aborted` and stamp the current cancel's propagation_point on it. The Runner is per-session serial, so the newest marker is the only one this cancel can be honestly attributed to.
Adds a focused regression test for the prelude path's optional LoopInput.prelude.agent: after revert.cleanup drops messages newer than the revert point, the loop derives the marker's agent from the latest *remaining* user. The test seeds a build-agent userOne, writes a ninja-agent userTwo, points revert back at userOne, then calls loop without prelude.agent and asserts the marker records build. A pre-cleanup derivation regression would pick ninja off userTwo and fail loudly. Also extends the sweep's semantic-boundary comment to spell out the two upstream invariants the newest-marker-only attribution relies on (per-session serial Runner + rejectIfBusy on the prelude path), so a future change that weakens either knows what would also have to move.
# Conflicts: # packages/opencode/src/session/compaction.ts
1 participant
Summary
Compaction in PawWork used to render twice — the full summary markdown as if it were an assistant reply, then the synthetic / replay user message that re-injects the original prompt. The UX exposed a purely technical process to every user, and the divider toggled to "Session compacted" the moment compaction started, before the summary had even streamed.
This PR collapses both halves so a compaction turn renders only a thin divider with four real states — pending / done / aborted / failed — plus a neutral elapsed timer while pending, matching the same TextShimmer language already used for running tool titles.
The backend half keeps the summary assistant message as the single carrier for
error/finish/time.completedeven when pre-summary steps fail or get aborted, so the divider's state machine never strands itself in "pending".Why
The divider's status was reading from a piece of state that didn't exist for every code path. Pre-summary failures (agents.get / provider.getModel / select / plugin / toModelMessages / processors.create) and abort interrupts didn't create or update a summary assistant, so the UI couldn't tell pending apart from "silently failed before streaming started." And renderer-side, the summary message and the synthetic continuation were treated as ordinary assistant / user content, leaking compaction internals into every consumer of
assistantMessages()(Thinking…, error card, copy target, turn-duration math, AssistantParts rendering).Design spec, including the exact state-machine ordering and the elapsed-start switch once the summary message appears:
docs/superpowers/specs/2026-05-21-compaction-ui-design.md. Visual source for the four-state divider:docs/design/preview/compaction-states-compare.html.Related Issue
No tracking issue — change came from a direct review of the compaction surface against
docs/DESIGN.mdmotion / iconography rules.Human Review Status
Pending
Review Focus
compaction.ts— the placeholder is created up front and the rest of the flow is wrapped inEffect.catch+Effect.onInterrupt. Pre-summary failures land on the placeholder witherror+finish=\"error\"; aborts land withMessageAbortedError. Verify there is no path where the placeholder is created but receives neither.session-turn.tsx— raw vs visible split. The summary message must reach onlycompactionSummary/ divider state. Every other derivation readsvisibleAssistantMessages. There is a contract test guarding this, but a fresh pair of eyes on the new memos is welcome.session-turn-compaction.ts— five-rule state machine ordering. Reviewer attention on rule 2/3 (abort/error precedence overtime.completed) and rule 5 (elapsed switches to summarytime.createdonce it exists).User.replayschema field + SDK regeneration — straightforward additive, but worth a glance to make sure no other consumer is now sending unexpected values.Risk Notes
completedCompactionsfilter onfinish && !error, and the renderer'scompactionSummary) already gate on it correctly.packages/sdk/js/src/v2/gen/**) is part of this PR. Downstream consumers of the SDK now typereplay?: booleanonUserMessage.How To Verify
```text
opencode targeted (session.compaction): 44 pass, 1 file
ui targeted (session-turn-compaction helpers + contract): 29 pass, 2 files
opencode typecheck: pass
ui typecheck: pass
app typecheck: pass
git diff --check: clean
```
Pre-existing baseline failures unchanged on
dev(Button single canonical size, IconButton 24×24, undefined-tokens--border-active, synthetic stop tool parts) — confirmed viagit stashon this branch.Screenshots or Recordings
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation