Skip to content

Preserve draft thread terminal state during snapshot cleanup - #182

Merged
juliusmarminge merged 1 commit into
mainfrom
t3code/fix-draft-thread-terminal-closes
Mar 6, 2026
Merged

juliusmarminge merged 1 commit into
mainfrom
t3code/fix-draft-thread-terminal-closes

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Mar 6, 2026 •

Copy link
Copy Markdown
Member

Summary

  • keep terminal state for local draft threads when processing server snapshots
  • add collectActiveTerminalThreadIds to merge active server thread IDs with draft thread IDs before orphan cleanup
  • update root event routing to source draft thread IDs from useComposerDraftStore
  • add focused tests covering retained server threads, deleted server threads, and local draft thread retention

Testing

  • Not run (tests not executed in this context)
  • Added unit tests in apps/web/src/lib/terminalStateCleanup.test.ts for:
    • retaining non-deleted server threads
    • excluding deleted server threads while preserving local draft threads

Note

Cursor Bugbot is generating a summary for commit 5dc3c89. Configure here.

Note

Preserve terminal states for local draft threads by passing active thread IDs from routes.__root.EventRouter.flushSnapshotSync using lib.terminalStateCleanup.collectActiveTerminalThreadIds

Add collectActiveTerminalThreadIds to merge non-deleted snapshot thread IDs with draft thread IDs and use it in flushSnapshotSync so removeOrphanedTerminalStates receives the combined set; add tests in terminalStateCleanup.test.ts.

📍Where to Start

Start with flushSnapshotSync in apps/web/src/routes/__root.tsx, then review collectActiveTerminalThreadIds in apps/web/src/lib/terminalStateCleanup.ts.

Macroscope summarized 5dc3c89.

- add `collectActiveTerminalThreadIds` to keep non-deleted server threads plus local draft threads
- use helper in root event router before orphan terminal state removal
- add unit tests for retained and deleted thread behavior
@coderabbitai

coderabbitai Bot commented Mar 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0f995400-9163-4dd6-b8d0-805890f732fa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch t3code/fix-draft-thread-terminal-closes

Comment @coderabbitai help to get the list of available commands and usage tips.

@juliusmarminge
juliusmarminge merged commit b9e529e into main Mar 6, 2026
4 checks passed
@juliusmarminge
juliusmarminge deleted the t3code/fix-draft-thread-terminal-closes branch March 6, 2026 07:41
aorwall added a commit to aorwall/t3code that referenced this pull request Sep 21, 2026
Merges `pingdotgg/t3code` up to `7445aa733` (21 commits from base
`5378f87f9`).

**This merge had no conflicts at all.** `preflight.mjs` forecast zero,
and `git merge` stopped on nothing. All 154 files upstream changed
landed — `merge-stats.mjs` reports an exact 154/154 match, so nothing
was dropped and nothing landed that upstream did not change. Fork delta
is 776 files.

The 8 files both sides touched auto-merged; each was checked by hand
against both parents, and `resolution-check.mjs` confirms every one
still carries both upstream's change and its fork delta. The two
`decide` paths (`PreviewView.tsx` and its test) took pingdotgg#12636's
synchronous `capturePreviewAnnotationScreenshot`, which does not touch
the `FEATURES.browserHistory` gate.

`unsupported-methods.mjs` reports ADD 0 / DROP 0, so no error union in
`packages/contracts/src/rpc.ts` changed.

## Usable as-is

Client-side fixes the fork gets for free, no Moatless work needed:

- **Typed text survives clicking a question option** (pingdotgg#12577) — the
composer no longer discards what was typed when an option chip is
clicked.
- **Desktop annotation screenshots stay under CSP** (pingdotgg#12636) —
`capturePreviewAnnotationScreenshot` became synchronous;
`PreviewView.tsx` and its test follow.
- **Providers settings heading restored** (pingdotgg#12552) —
`ProviderSettingsPanel.tsx`.
- **Long titles wrap in confirmation dialogs** (pingdotgg#12571).
- **Collapsed thought previews show plain text** (pingdotgg#12377) — markdown is
no longer rendered into the one-line preview.
- **Mobile:** Android composer placeholder stays on one line (pingdotgg#12605),
workspace navigation and expand controls adapt (pingdotgg#12551), built-in theme
colors align with desktop (pingdotgg#12534, which also lifts the palettes into
`packages/shared/src/themePalettes.ts`), dev-client script with a
preview environment (pingdotgg#12558).
- **Contract members for provider permission requests** (pingdotgg#7861) —
`permission` on `ProviderRequestKind` and `permission_approval` on
`CanonicalRequestType`. The client and mobile halves are here; see the
third bucket for what is missing.

Two more land in surfaces this fork decides out, so they change nothing
today: pull-request detail panel icon alignment (pingdotgg#11263) and PR state
glyph alignment (pingdotgg#11268), both behind `FEATURES.pullRequestSurface:
false`.

Not applicable to the hosted fork: the desktop OTLP main-process
telemetry export (pingdotgg#12520, left off until a metric exists by pingdotgg#12540), the
Flatpak/GTK4 SnapShot text (pingdotgg#12635), and the release fix that dropped a
placeholder `allowBuilds` entry (pingdotgg#12544).

## Unsupported in Moatless / needs implementation

None new. This range added no RPC method, no auth or transport
assumption, and no capability the fork does not already gate. The two
upstream changes that touch decided-out surfaces
(`FEATURES.pullRequestSurface`, `FEATURES.openInEditor`) are covered by
gaps entries that already exist.

## Backend behavior to consider reproducing in Moatless

Four, recorded under _Runtime fixes upstream made to its own server_ in
`docs/fork/gaps.md`:

- **An agent that dies during session start should report its own
stderr** (pingdotgg#12625). Upstream buffers the ACP child's stderr and raises
the captured text when `cursor-agent` exits before the handshake,
instead of a generic session-start failure. Moatless launches its own
agent processes; a bad credential or a missing binary currently reaches
a person with the one line that explained it discarded.
`apps/server/src/provider/acp/AcpStderr.ts`.
- **An empty provider home should resolve to the default, not to a fresh
one** (pingdotgg#12624). A Claude account whose `homePath` is set but empty now
means `~/.claude`, so it shares session continuation rather than
starting its own transcript directory. The symptom is a resumed thread
that has forgotten everything, on an account that merely had a blank
field. `apps/server/src/provider/Drivers/ClaudeDriver.ts`.
- **A provider permission prompt should be approvable, not just
displayed** (pingdotgg#7861). Both contract members landed here, so the rendering
half is already in this fork — Moatless has to emit the `permission`
request for the surface to light up. Until it does, a Codex permission
prompt stalls the turn with nothing to answer it.
- **An editor installed outside `PATH` should still be launchable**
(pingdotgg#12439). Upstream falls back to macOS `Applications` bundles, JetBrains
Toolbox scripts and Windows program directories before declaring an
editor absent. Moot while `FEATURES.openInEditor` is off, and it is the
detection Moatless would need the day it dispatches
`shell.openInEditor`. `packages/shared/src/editor.ts`.

## Verification

`verify.mjs` — all 10 checks pass: duplicate-adds, tripwires,
resolution-check, unsupported-methods, lockfile, fmt:check, lint,
typecheck, build, test (335 test files, 5165 tests). The `t3` package
failed under load and passed when run on its own; not a merge
regression.

Upstream changed three manifests (`apps/mobile/package.json`,
`packages/shared/package.json`, `pnpm-workspace.yaml`) and did not touch
`pnpm-lock.yaml`. The lockfile was re-derived anyway per the merge
procedure; the install produced no change, so the committed lockfile is
already what those manifests resolve to.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/fe6739d3-9f52-4796-b2a8-46a7a7827ec9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant