Skip to content

perf(renderer): unmount closed setup guide content - #13546

Open
nwparker wants to merge 1 commit into
mainfrom
nwparker/perf-setup-guide-mount-gating
Open

perf(renderer): unmount closed setup guide content#13546
nwparker wants to merge 1 commit into
mainfrom
nwparker/perf-setup-guide-mount-gating

Conversation

@nwparker

Copy link
Copy Markdown
Contributor

Summary

  • keep only a one-selector setup-guide shell mounted after the guide has been used
  • retain the full content for the 200 ms close animation, then release its progress/runtime subscriptions
  • preserve dismissed-close telemetry and cancel a pending unmount when the guide reopens

This follows the mount-gating pattern from #13525. The intentional always-on setup telemetry observer remains; this removes the second progress graph retained by the closed modal.

ELI5

The Getting Started guide is like a checklist teacher. After you opened it once, Orca kept a second copy of that teacher listening to project, remote-host, skill, and integration updates even after the guide disappeared.

Now the closed guide keeps only a tiny doorbell that knows whether it should open. The teacher stays for the brief closing animation, then goes home until the next open.

Visual explainer

flowchart LR
  U[Project or remote-host update]
  O[Intentional telemetry observer]
  M[Retained closed modal]
  P[20 direct progress selectors plus runtime hooks]
  N[Render nothing]
  V[One visibility selector]
  X[Content unmounted after 300 ms]

  subgraph Before
    U --> O
    U --> M --> P --> N
  end

  subgraph After
    U --> O
    U --> V --> X
  end
Loading

Improvement proof

  • useSetupGuideProgress alone owns 20 direct Zustand selectors; the retained modal also owned modal/runtime subscriptions.
  • The regression harness models a representative content selector and raw store subscription. On base, the closed component still mounts that content and the new test fails.
  • After the 300 ms linger: active content subscriptions = 0.
  • 1,000 closed store writes after unmount: 0 content renders and 0 content notifications.
  • At 299 ms the content is still mounted for the close path; at 300 ms it is released.
  • Reopening at 299 ms cancels the pending unmount and keeps exactly one content subscription.

Testing checklist

  • SetupGuideModal.mount-gating.test.tsx: 2/2 passed
  • focused setup-guide suite: 22/22 passed
  • pnpm run typecheck: passed
  • changed-code quality gate: 0 new findings across 2 files
  • reliability manifest: 73/73 gates passed
  • max-lines ratchet: no new bypasses
  • git diff --check: passed

Full-suite check: 49,130 tests passed. Five unrelated files failed in the heavily parallel run. A serial rerun cleared the native-chat watcher and worktree-directory poller failures. Three environment/load failures remained:

  • cross-version fixture extraction: missing generated .staging-v799-*/src
  • terminal frame equivalence fuzz: existing 30 s timeout (44.9 s on this machine)
  • GitHub project source-context import test: existing 30 s timeout

The changed setup-guide test was rerun after the full suite and remained green.

No-regression review

  • Closing still gives telemetry an explicit open=false render, so it records dismissed, not interrupted.
  • The 300 ms linger covers the dialog's 200 ms exit animation.
  • Timer cleanup covers reopen and component unmount; the 299 ms reopen race is tested.
  • Installed-skill discovery remounts from its runtime-scoped module cache, so this does not force a new scan.
  • Existing setup-script and remote-runtime cancellation/stale guards remain unchanged.
  • Active-step state already reset on reopen, so unmounting adds no user-visible state reset.
  • No user-facing regressions found.

Screenshots

Not applicable: this changes hidden component lifetime only; visible layout and styling are unchanged. The lifecycle diagram above is the visual proof.

AI review report

  • P0: none
  • P1: none
  • P2: none
  • Checked failure cleanup, timer concurrency, stale async work, close telemetry, repeated opens, macOS/Linux/Windows behavior, SSH/runtime ownership, and folder-workspace assumptions.

Security audit

No authentication, authorization, persistence, network sink, shell execution, dependency, or user-data surface changed.

Notes

  • No mobile-facing surface or remote wire contract changed.
  • No Git/Git-provider behavior changed.
  • Development StrictMode replays telemetry effects when a newly remounted subtree is probed; contributor telemetry transport is disabled, and production React does not replay effects.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 851757d4-d909-4b83-89c4-c6785443cf01

📥 Commits

Reviewing files that changed from the base of the PR and between 1310102 and 53b9078.

📒 Files selected for processing (2)
  • src/renderer/src/components/setup-guide/SetupGuideModal.mount-gating.test.tsx
  • src/renderer/src/components/setup-guide/SetupGuideModal.tsx

📝 Walkthrough

Walkthrough

SetupGuideModal now keeps its content mounted for 300 ms after closing. The wrapper passes the current open state to the content component. Telemetry, step-selection effects, completed-step handling, and dialog visibility use this state. Tests cover delayed unmounting, suppressed closed-state updates, subscription cleanup, and reopening during the close linger.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: unmounting closed setup guide content to reduce retained subscriptions.
Description check ✅ Passed The description covers the user-visible change, testing, AI review, security audit, platform checks, risks, and known environment failures.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

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

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