Sign up
View stargazersSign up

refactor(web): review guide runs are watched from their answers, not from mounted components#25459

merged1
main
effect/split-web-pull-request-detail
opened3d agobyJJulius Marminge
11 files changed
View on GitHubTwo-way sync enabledPR last imported 3d ago

Problem

Review guide run notifications were owned by a null component in the root route that rendered one child per watched run, each with a polling effect and an effect reacting to the guide query's data. A run was only watched while a guide tab was mounted (an effect in useReviewGuideWriter). The guide tab reported itself as shown from another effect. A notification that opened a guide went through a one-shot "open request" in the store, which the detail panel consumed with a setState during render plus an effect to clear it.

Change

  • The reviewGuide pull request query gets an answer handler (client-runtime PullRequestAnswerHandlers.reviewGuide). Any answer that reports a Running run registers the watch, wherever it was read.
  • startGuideRunNotifications(registry, router) starts from appInit and runs for the app's lifetime. It subscribes to the store's watches and, per watch, to the guide atom, and polls on an interval it owns. <PullRequestGuideRunCoordinator /> is gone from __root.tsx.
  • The pull requests page keeps its detail tab in the guide run store (tabOnPage), so a notification switches it with showPageDetailTab. When a thread's panel is opened to keep the guide, its surface gets detailTab: "guide" instead of an open request.
  • The detail panel derives its visible tab. An unavailable tab falls back to Summary without overwriting the selection.
  • The notifier checks whether the guide is on screen through the panel's data-guide-shown attribute, not a store field kept by an effect.
  • When a watch has no title, the notification body uses the observed pull request title.

What this improves

  • Less sync: 5 effects and 2 setState-during-render syncs are gone, along with a null lifecycle component and its per-run children.
  • Tests in PullRequestGuideRunCoordinator.test.ts guard the new notifier: it polls each watched run once however often the watches change, it does not poll a run that settles on its first read, and a watch started from a Running answer does not report the previous run's failure.
  • This PR claims no bug fix that fails on main.

Behavior changes

  • A Running guide answer read anywhere (the guide tab, the notifier's own poll, another reader) now starts a watch. Before, this happened only while a guide tab was mounted.
  • On the pull requests page, the detail tab you last chose is remembered for that pull request in the guide run store. Coming back to the same pull request reopens that tab instead of Summary. Another pull request still opens on its initial tab.
  • An unavailable tab (Code without diff support, Guide with guides off) shows Summary but keeps the selection. If the tab becomes available again, it comes back.
  • Notifications for runs registered from an answer use the observed pull request title, or repo #n if there is none.

Effects

PullRequestDetailPanel.tsx 5 -> 4, PullRequestGuideTab.tsx 2 -> 1, PullRequestGuideRunCoordinator.tsx 2 (per watched run) -> 0, useReviewGuideWriter.ts 1 -> 0, __root.tsx 7 -> 7, appInit.tsx 0 -> 0.

Verification

  • vp test run src/components/pullRequest/guide/PullRequestGuideRunCoordinator.test.ts src/state/pullRequests.test.ts: 23 passed
  • vp run check:changed: format, lint, knip, typecheck (7 packages) and tests (4 files) pass

Model: Claude Opus 5.5, Claude Code harness.

🤖 Generated with Claude Code

Checks

23 successful · 13 skipped · 1 neutral
All checks passed
41eb01b

Added 1 commit

  • refactor(web): review guide runs are watched from their answers, not from mounted components41eb01b
MmacroscopeappBot3d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This refactor changes production behavior across app initialization, shared query answer handling, guide-run polling, notifications, navigation, and pull-request tab persistence. The changes are cross-cutting and user-visible rather than purely mechanical, so human review is appropriate.

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

CcoderabbitaiBot3d ago
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 071239804e375078dd3fae3b14704b4def57e048 and 41eb01b210ad9bd7e80fc3ea56927dffcce8ec91.

External image Files selected for processing: 11
  • apps/web/src/appInit.tsx
  • apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx
  • apps/web/src/components/pullRequest/PullRequestGuideTab.tsx
  • apps/web/src/components/pullRequest/PullRequestsPage.tsx
  • apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.test.ts
  • apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx
  • apps/web/src/components/pullRequest/guide/guideRunStore.ts
  • apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts
  • apps/web/src/routes/__root.tsx
  • apps/web/src/state/pullRequests.ts
  • packages/client-runtime/src/state/pullRequests.ts
External image Files with no reviewable changes: 2
  • apps/web/src/components/pullRequest/PullRequestGuideTab.tsx
  • apps/web/src/routes/__root.tsx

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

Walkthrough

This change moves guide-run notifications to an app-lifetime coordinator. Review-guide answers register running guides for observation. The pull-request detail panel can store its selected tab, and notifications can navigate to a pull request with its Guide tab selected.

Changes: Guide-run notifications and page tabs

Layer / File(s)Summary
Guide-run watch and tab state
apps/web/src/components/pullRequest/guide/guideRunStore.ts, packages/client-runtime/src/state/pullRequests.ts, apps/web/src/state/pullRequests.ts, apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts		The store tracks guide-run watches and page detail tab selection. Review-guide answers can register running guides, and guide generation registers its run before starting generation.
Pull-request page tab integration
apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx, apps/web/src/components/pullRequest/PullRequestsPage.tsx, apps/web/src/components/pullRequest/PullRequestGuideTab.tsx		The page panel can read and update its tab through the guide-run store. Thread handoff selects Guide, and the Guide panel exposes the active guide key.
App-lifetime notification coordination
apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx, apps/web/src/state/pullRequests.ts, apps/web/src/appInit.tsx, apps/web/src/routes/__root.tsx, apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.test.ts		The app starts a coordinator that watches and refreshes guide runs, settles runs, and opens notifications through the router. Notification text can use the observed pull-request title. Tests cover refresh and settlement behavior.

Priority: External image Normal

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

Change: Feature

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant ReviewGuideQuery
3 participant PullRequestAnswerHandler
4 participant GuideRunStore
5 participant startGuideRunNotifications
6 participant AtomRegistry
7 participant Router
8 participant PullRequestDetailPanel
9 ReviewGuideQuery->>PullRequestAnswerHandler: deliver review-guide answer
10 PullRequestAnswerHandler->>GuideRunStore: register running guide watch
11 startGuideRunNotifications->>AtomRegistry: subscribe to and refresh guide atom
12 AtomRegistry-->>startGuideRunNotifications: return guide result
13 startGuideRunNotifications->>GuideRunStore: select Guide tab when opening notification
14 startGuideRunNotifications->>Router: navigate to pull request
15 PullRequestDetailPanel->>GuideRunStore: read selected detail tab

Possibly related PRs

  • pingdotgg/t3code#25233: Moves web notification and toast coordinators to app-lifetime services started by startAppServices, the same lifecycle pattern used here.

Suggested reviewers: maria-rcks, samuelbarnholdt, macroscopeapp

Merge Risk: External image Minimal · up to 41eb0

No confirmed issue currently blocks merging. The reported checks passed, though the notification and tab-state behavior remains worth validating before release.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description follows the template sections (Problem, Change, Scope and approval, Verification). Problem and Change are detailed and state the mechanism and effects. The Scope and approval section i…Add the 'Scope and approval' content. Link the triaged issue or the maintainer-approved discussion, with the approval comment. If no prior approval exists, explain why this refactor qualifies under the template's exemption rules. The change…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title "refactor(web): review guide runs are watched from their answers, not from mounted components" matches the main change. The PR moves review-guide run watching out of mounted components and i…
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: Title check

Explanation

The title "refactor(web): review guide runs are watched from their answers, not from mounted components" matches the main change. The PR moves review-guide run watching out of mounted components and into query answer handling, with a lifetime notifier started from appInit. The title is specific and follows conventional commit form. It omits the tab-persistence and notification-fallback changes, which are secondary.

Full details: Description check

Explanation

The description follows the template sections (Problem, Change, Scope and approval, Verification). Problem and Change are detailed and state the mechanism and effects. The Scope and approval section is missing: it has no linked triaged issue or maintainer-approved discussion, and no justification for a refactor without one. The description also does not include before/after screenshots, though this PR has no visible UI change to show. The Verification section lists focused test runs and results. It describes a Claude-generated model in the footer, which the template asks for. The 'What this improves' section says the PR claims no bug fix, but the Behavior changes section adds user-visible tab memory and notification changes, which the template's scope rules require approval for.

Resolution

Add the 'Scope and approval' content. Link the triaged issue or the maintainer-approved discussion, with the approval comment. If no prior approval exists, explain why this refactor qualifies under the template's exemption rules. The change adds user-visible behavior: the remembered detail tab per pull request, the Summary fallback for unavailable tabs, and the notification title fallback. Either get explicit approval for these behavior changes or split them into a separate PR. Confirm that the 'Verification' section covers the changed UI behavior. Note any checks not run, such as manual testing of the pull requests page tab memory and notification flow.

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.