Skip to content

fix(web): keep legacy thread routes stable during reconnect - #10604

Closed
saphid wants to merge 8 commits into
pingdotgg:mainfrom
saphid:repair/d6096318-legacy-shell
Closed

saphid wants to merge 8 commits into
pingdotgg:mainfrom
saphid:repair/d6096318-legacy-shell

Conversation

@saphid

@saphid saphid commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Opening a valid thread deep link during legacy connection synchronization can redirect to an unrelated thread when cached or HTTP shell data is mistaken for authoritative state. Routes now wait for live shell authority before deciding a thread is missing. Already available thread detail or a local draft stays visible while shell synchronization is pending.

Legacy servers receive a full socket snapshot request because their cursor replay has no completion signal. Marker-capable servers retain cursor replay and completion markers. Same-session subscriptions reuse the available snapshot; a new session refreshes it. The shared synchronization behavior also serves mobile, whose missing-thread route policy remains separate.

Verified after integrating upstream main 35be904, using Node24.13.1:

  • 21 focused route/shell synchronization tests passed and were independently repeated.
  • Four regression cases fail before the repair and pass afterward: cached/synchronizing shell combined with available detail/draft.
  • Web and client-runtime typechecks and targeted lint/format passed; repair checks repeated for the changed route helper/tests.
  • Fresh independent GPT-6.1 Sol review covered all five contribution files and found no actionable findings after the repair.

Current browser/mobile/relay proof is still missing for the integrated candidate, particularly delayed shell versus available detail, valid/missing deep links, offline rendering and reconnect. The historical captures below predate the new available-detail repair and current integration; they do not establish those checks.

Historical Linux Chromium before/after evidence

The following is the original capture record, retained with its original revisions and verification limits.

Same Linux Chromium web scenario at base 09e8de9c655ae85410bf6b00446f272a01da81c7 and candidate a325b17ed3e398a88ce10de7744008c064debf7e: an isolated fixture retains the HTTP shell response from before the target thread was created, omits the completion-marker capability, and holds live shell/target-detail delivery until the initial route is observed. Delivery then resumes. Both clients use the same exact-base backend; Chromium local-network permission is scoped to that isolated origin.

Before: the valid deep link redirects to an unrelated New thread and remains there after live delivery resumes.

Before: base redirects the requested conversation to an unrelated New thread

Full before recording · Full before screenshot

After: the requested Reconnect target conversation stays selected and opens after the live snapshot arrives.

After: the candidate preserves and opens the requested conversation

Full after recording · Full after screenshot

The GIFs use the same sidebar crop and 12 fps sampling, with labels added above the captured frame. No temporal cuts or speed changes. Full recordings provide context. Files were anonymously downloaded again, hash-verified, decoded, and inspected by sampled frames; interactive browser playback was not inspected. Raw recordings, receipts, and capture script · Media receipt.

Original fix: GPT-5.6 Sol in the Codex harness. Current integration and repair: GPT-6 Astra; independent review: GPT-6.1 Sol, both in the Codex harness via T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 7, 2026

@macroscopeapp macroscopeapp Bot 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.

All clear

Posted via Macroscope — Effect Service Conventions

@saphid
saphid marked this pull request as ready for review September 8, 2026 06:40

@macroscopeapp macroscopeapp Bot 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.

All clear

Posted via Macroscope — Effect Service Conventions

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 8, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 1cd6689

Macroscope's review found this PR approvable — The PR narrowly corrects deep-link and reconnect handling by distinguishing cached data from live shell authority and using the existing socket snapshot protocol for legacy servers. Its production changes are localized, backward-compatible at the protocol level, and accompanied by focused route and synchronization regression tests.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5b855a82-bb05-425b-b80c-59923fa2b297

📥 Commits

Reviewing files that changed from the base of the PR and between 9bf1e91 and 1cd6689.

📒 Files selected for processing (2)
  • apps/web/src/threadRoutes.test.ts
  • apps/web/src/threadRoutes.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Shell synchronization now handles sessions without completion markers by requesting a complete socket snapshot after an HTTP snapshot. Thread routes treat only live shell status as authoritative and keep available details or drafts ready during incomplete bootstrap.

Changes

Snapshot authority

Layer / File(s) Summary
Legacy shell synchronization
packages/client-runtime/src/state/shell.ts, packages/client-runtime/src/state/shell-sync.test.ts
Resumption checks completion-marker support and snapshot session state. Sessions without markers remain synchronizing after an HTTP snapshot and request a complete socket snapshot. Regression tests cover the transition to live and resubscription behavior.
Thread-route authority
apps/web/src/threadRoutes.ts, apps/web/src/components/ThreadRouteView.tsx, apps/web/src/threadRoutes.test.ts
Added isThreadRouteSnapshotAuthoritative. Thread bootstrap is complete only when shell status is live. Existing details and drafts render as ready during incomplete bootstrap. Tests cover snapshot authority and render states.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Shell as Shell synchronization
  participant HTTP as HTTP snapshot
  participant Socket as Socket snapshot
  Shell->>HTTP: Load snapshot when session has none
  HTTP-->>Shell: Apply snapshot
  Shell->>Socket: Request complete snapshot
  Socket-->>Shell: Apply snapshot and become live
Loading

Merge Risk: ⚪ Minimal · up to 1cd66

Legacy sessions wait for a full socket snapshot before missing threads are treated as authoritative, while available details and drafts remain visible. No concrete merge-blocking risk is established.

Architecture Summary

Architecture risk: 🔵 Low · up to 1cd66

The change affects 2 systems.

Changed systems: apps/web, packages/client-runtime

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/web (ui) was modified; 3 changed files map to changed impact.
  • observed — packages/client-runtime (library) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in packages/client-runtime/src/state/shell-sync.test.ts: Extended the local session helper with an optional completion-marker setting; disabled markers produce an empty initial configuration while preserving the existing default.
  • observed — Modified behavior in packages/client-runtime/src/state/shell-sync.test.ts: Added regression coverage for legacy sessions without completion markers, including synchronizing state after the HTTP snapshot, transition to live on a socket snapshot, snapshot replacement, and avoidance of additional HTTP loader calls during resubscription.
  • observed — Modified behavior in apps/web/src/components/ThreadRouteView.tsx: Imports isThreadRouteSnapshotAuthoritative for determining whether the thread-route snapshot is authoritative.
  • observed — Modified behavior in apps/web/src/components/ThreadRouteView.tsx: bootstrapComplete now uses the shell status authority check instead of treating any present snapshot as complete.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: stabilizing legacy thread routes during reconnect.
Description check ✅ Passed The description clearly covers the problem, implementation, cross-client impact, focused verification, test results, and known verification limits. It is detailed and directly related to the pull requ…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@saphid

saphid commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Friendly review nudge @juliusmarminge @maria-rcks — this is mergeable and hasn't had a maintainer pass yet. Independent bot/agent reviews have run with findings triaged in-commit (see receipts in earlier comments). Full queue context and status: #10688.

@saphid
saphid force-pushed the repair/d6096318-legacy-shell branch from a325b17 to d3360b0 Compare September 11, 2026 07:41
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 11, 2026 07:41

Dismissing prior approval to re-evaluate d3360b0

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 11, 2026
saphid and others added 5 commits September 12, 2026 22:53
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A session that already received a snapshot no longer reloads the shell
over HTTP before requesting the full socket snapshot legacy servers need,
so foreground wakeups and retries pay one transfer instead of two.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@saphid
saphid force-pushed the repair/d6096318-legacy-shell branch from d3360b0 to 4b9e7bb Compare September 12, 2026 13:08
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 12, 2026 13:08

Dismissing prior approval to re-evaluate 4b9e7bb

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 12, 2026
@saphid

saphid commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the Release Smoke failure on the latest head: it's unrelated to this diff. The smoke job deletes pnpm-lock.yaml and re-resolves the workspace; expo-audio is ranged ~57.0.4 and npm now serves 57.0.5, so the declared expo-audio@57.0.4 patch comes back unused and vp install --lockfile-only exits with ERR_PNPM_UNUSED_PATCH. Needs a patch bump or a version pin on main — every PR will hit this until then.

@macroscopeapp
macroscopeapp Bot dismissed their stale review September 20, 2026 03:30

Dismissing prior approval to re-evaluate 9bf1e91

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 20, 2026
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 30, 2026 20:48

Dismissing prior approval to re-evaluate 1cd6689

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

Closing for missing verification of the current loading transition. The PR says its screenshots and recordings predate the change that makes an available thread or draft ready before shell bootstrap completes. The focused tests cover the decision logic, but the changed client transition is still unshown. The verification rule requires evidence of that behavior. Add before/after screenshots and a short recording of the current integrated client through this ordering, with the environment and observed results, then request reconsideration.

@saphid

saphid commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the closing note: the missing verification is now provided in the focused replacement #14697. GitHub wouldn't let me reopen this PR.

Could you assess #14697 for review in place of this PR?

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants