refactor(web): drop the unreachable paint-only thread timeline path#25787

merged1
main
refactor/remove-dead-paint-only-timeline
opened15d agobyJJulius Marminge
View on GitHubTwo-way sync enabledPR last imported 15d ago

resolveThreadSwitchTimeline can no longer return another thread's timeline, so the whole paint-only/held-timeline path in the chat view is unreachable. This deletes it. Behavior is unchanged.

Why it is dead

On main (32bf48f428), every return in resolveThreadSwitchTimeline sets displayThreadKey: input.activeThreadKey (lines 440, 446, 449), and the lastReady parameter (line 437) is never read. isPaintOnlyThreadTimeline(displayThreadKey, activeThreadKey) (L423-L430) is therefore always false, so paintOnlyDisplayedTimeline in ChatView is constant false and every branch keyed on it never runs. The fallback to the last-viewed thread most likely went away with #21560 (79b14831cb).

What was removed

  • ChatView's own resolveThreadSwitchTimeline call. Only its displayThreadKey was read, and that always equals activeThreadKey, so displayedTimelineKey is now activeThreadKey ?? routeThreadKey.
  • isPaintOnlyThreadTimeline, paintOnlyDisplayedTimeline, and the ~30 ternaries/spreads on it in ChatView's ThreadTimeline props, plus the one on threadFindEnabled. Each prop now gets the value its false branch already gave it.
  • heldPaintContext, peekHeldThreadTimeline, lastReadyThreadKey, the lastReady param, and HeldThreadTimeline.markdownCwd / .workspaceRoot.
  • The noopHeld* and EMPTY_HELD_TURN_DIFF_SUMMARIES constants.
  • MessagesTimeline's paintOnly prop, useTimelineRewind's paintOnlyDisplayedTimeline input, and ThreadTimeline's readyMarkdownCwd / readyWorkspaceRoot props, which only fed the held context.
  • threadKeysShareEnvironment, which only a test used.
  • resolveThreadSwitchTimeline now returns the entries directly instead of { entries, displayThreadKey }.

What stays

  • ThreadTimeline's useLayoutEffect that remembers the ready timeline is still live. peekRememberedThreadTimeline reads it so that switching back to a cached thread paints that thread's own entries while its detail resyncs, instead of an empty list.
  • That same-thread fallback in resolveThreadSwitchTimeline and its test are kept. The cross-environment test, which only passed lastReady, now checks that a destination with nothing remembered never paints the last-viewed thread.

Nothing outside apps/web imports any of these, including desktop, mobile, and client-runtime.

Verification

  • vp run check:changed: format, lint, knip, typecheck, and tests all pass.
  • ChatView.logic.test.ts (43 tests) and the MessagesTimeline browser, logic, and active-tools tests (117 tests) pass. No useTimelineRewind tests exist.

🤖 Generated with Claude Code

Checks

24 successful · 13 skipped
All checks passed
f6f3b9b

Added 1 commit

  • refactor(web): drop the unreachable paint-only thread timeline pathf6f3b9b
Synced from GitHub

Approvability

Verdict: Approved at f6f3b9b

Macroscope's review found this PR approvable — This PR removes an unreachable cross-thread timeline fallback and its associated no-op behavior, while preserving the active-thread rendering and cache path. The remaining changes are localized refactoring and test updates with no product-default, schema, deployment, or sensitive-package impact.

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

Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between a9b513b50cc967cc715228b38cb922d1aaaa3a8b and f6f3b9bacede19ab397bee29dbaa4d2db4209fd2.

External image Files selected for processing: 6
  • apps/web/src/components/ChatView.logic.test.ts
  • apps/web/src/components/ChatView.logic.ts
  • apps/web/src/components/ChatView.tsx
  • apps/web/src/components/chat/MessagesTimeline.tsx
  • apps/web/src/components/chat/ThreadTimeline.tsx
  • apps/web/src/components/chat/useTimelineRewind.ts

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

Walkthrough

The timeline resolver now returns entries directly and can use remembered entries for the active thread while loading. ChatView removes the paint-only timeline path and passes active-thread state and interactions to the timeline.

Changes: Active thread timeline

Layer / File(s)Summary
Timeline entry resolution
apps/web/src/components/ChatView.logic.ts, apps/web/src/components/chat/ThreadTimeline.tsx, apps/web/src/components/ChatView.logic.test.ts
resolveThreadSwitchTimeline returns entries directly. It selects nonempty next entries, or remembered entries for the active thread while loading. ThreadTimeline and its tests use the updated return value.
Active-thread timeline integration
apps/web/src/components/ChatView.tsx
ChatView uses the active thread key and passes the current timeline state and interactions directly. Thread find and available history controls are no longer gated by the paint-only timeline path.
Timeline position and rollback behavior
apps/web/src/components/chat/MessagesTimeline.tsx, apps/web/src/components/chat/useTimelineRewind.ts
Position recording and turn-status announcements no longer use paint-only checks. Timeline rewind invokes the rollback checkpoint callback without that check.

Priority: External image Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Refactor

Suggested labels: size:L

Suggested reviewers: t3dotgg, macroscopeapp

Merge Risk: External image Minimal · up to f6f3b

The timeline remains scoped to the selected thread during a switch. No issue identified here needs to be resolved before merge.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description explains the dead code, the removed components, the preserved behavior, and verification results. It does not provide the required scope and approval information, and it does not inclu…Add a Scope and approval section with the related issue or maintainer approval, or explain why this focused refactor qualifies for an exemption. Add before/after UI screenshots, or explicitly document why screenshots are not applicable beca…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main change: removing the unreachable paint-only thread timeline path.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the dead code, the removed components, the preserved behavior, and verification results. It does not provide the required scope and approval information, and it does not include the required UI screenshots or an explicit statement that visual behavior was unchanged.

Resolution

Add a Scope and approval section with the related issue or maintainer approval, or explain why this focused refactor qualifies for an exemption. Add before/after UI screenshots, or explicitly document why screenshots are not applicable because the rendered behavior is unchanged. If an agent was used, identify the model and harness as required by the template.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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