fix(ui): defer default-open tool details#622
Merged
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Astro-Han
added
tech-debt
github-actions
Bot
removed
P2
Contributor
📝 WalkthroughWalkthroughAdds deferred initialization for BasicTool (new exported helper, updated ready synchronization and rendering with RAF-based gating), a test fixture and Bun tests controlling RAF, applies ChangesDeferred Tool Initialization
Perf E2E: heavy-bash scenario
Sequence Diagram(s)No sequence diagram is provided. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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. Comment |
Astro-Han
added
P2
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to defer the mounting of tool details, improving initial rendering performance for heavy tools like Bash. It includes a new test suite for deferred loading logic and updates the BasicTool component to manage its 'ready' state based on a new defer prop. Feedback from the review highlights a visual 'pop' issue when closing tools, suggesting that unmounting should be delayed until animations finish to maintain visual continuity. Additionally, a performance optimization was recommended to use untrack on the ready signal within the effect to prevent unnecessary re-runs.
Perf delta summaryComparator: pass
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app/e2e/perf/perf-probe.spec.ts (1)
41-42: ⚡ Quick winRename the test constant to SCREAMING_SNAKE_CASE.
Line 41 introduces
heavyBashCommand, which breaks the test constant naming convention used in this suite.♻️ Proposed rename
-const heavyBashCommand = +const HEAVY_BASH_COMMAND = 'node -e \'for (let i = 0; i < 900; i++) console.log(String(i).padStart(4, "0") + " " + "heavy bash output ".repeat(8))\'' @@ - command: heavyBashCommand, + command: HEAVY_BASH_COMMAND,As per coding guidelines "Use SCREAMING_SNAKE_CASE for constants in tests".
Also applies to: 210-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/perf/perf-probe.spec.ts` around lines 41 - 42, Rename the test constant heavyBashCommand to SCREAMING_SNAKE_CASE (e.g., HEAVY_BASH_COMMAND) and update every reference to it in the spec; locate the constant declaration named heavyBashCommand and replace it with HEAVY_BASH_COMMAND, then update all usages (including the other occurrence in the file) to the new identifier so tests compile and follow the test-constant naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/basic-tool-defer.test.ts`:
- Line 37: The Vite server root is set incorrectly in the test file (root: new
URL("../..", import.meta.url).pathname) which resolves to packages/ui/src/
causing ssrLoadModule("/test/fixtures/basic-tool-render.fixture.tsx") to look
under packages/ui/src/test/...; update the root to move one level higher by
using new URL("../../..", import.meta.url).pathname so the root points at
packages/ui/ and the fixture at packages/ui/test/fixtures/... is resolved
correctly.
---
Nitpick comments:
In `@packages/app/e2e/perf/perf-probe.spec.ts`:
- Around line 41-42: Rename the test constant heavyBashCommand to
SCREAMING_SNAKE_CASE (e.g., HEAVY_BASH_COMMAND) and update every reference to it
in the spec; locate the constant declaration named heavyBashCommand and replace
it with HEAVY_BASH_COMMAND, then update all usages (including the other
occurrence in the file) to the new identifier so tests compile and follow the
test-constant naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05249c00-0b30-4238-82ba-53fb0e679c71
📒 Files selected for processing (6)
packages/app/e2e/perf/perf-probe.spec.tspackages/app/e2e/perf/profiles.test.tspackages/app/e2e/perf/profiles.tspackages/ui/src/components/basic-tool-defer.test.tspackages/ui/src/components/basic-tool.tsxpackages/ui/test/fixtures/basic-tool-render.fixture.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/app/e2e/perf/profiles.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
BasicTooldeferred details so default-open heavy tool cards do not resolve or mount their body synchronously.Add
deferto thebashtool card, keep existingedit,write, andapply_patchdeferred paths explicit, add a real Solid render test that proves deferred children are not evaluated until the next animation frame, and add a default-profile perf probe for a default-open heavy bash output session.Why
Part of #601. After PR #620 split the message-part renderers, the remaining BasicTool defer gap was behavioral:
defaultOpen=true + defer=truecould still pay the first-render cost for large shell output, file bodies, or diffs.The new perf scenario gives this PR a concrete base/head guardrail for the most direct changed tool path: shell tools default-open with a large completed bash output.
Related Issue
Part of #601. Follows the read-only audit in #601 (comment).
Human Review Status
Ready for human merge decision. Gemini and CodeRabbit inline review threads were replied to and resolved.
Review Focus
BasicToolkeeps default-open visual state while deferring heavy details until the next animation frame.bash,edit,write, andapply_patchremain the only intended heavy BasicTool body targets in this slice.tool-default-open-heavy-bashruns in the default perf profile only, with no diff/editor perf scope added in this PR.Risk Notes
Low UI behavior risk. Default-open deferred cards may show their details one frame later, but card position, trigger, copy behavior, and expand/collapse semantics are intended to stay unchanged.
Animated deferred details now stay mounted until the close animation finishes, then reset readiness. Non-animated deferred tools still reset immediately on close.
The perf probe adds CI time for one 3-run default-profile scenario. It is a regression guardrail, not a hard requirement that every run show a fixed millisecond improvement.
No dependency, schema, permission, generated-file, or platform behavior changes intended.
How To Verify
Screenshots or Recordings
Not included. This is a one-frame lazy-mount behavior change with no intended visible copy or layout change. Electron smoke, local smoke, and local heavy bash perf probe covered the relevant runtime paths.
Checklist
dev, and my PR title and commit messages use Conventional Commits in English