perf(desktop): stop whole-document restyles and svg spinner churn - #91360
Conversation
|
😎 This pull request was merged. |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
|
React Doctor found 4 issues in 3 files · 4 warnings. 4 warnings
Reviewed by React Doctor for commit |
Prompt To Fix All With AI### Issue 1
products/desktop/packages/ui/src/features/autoresearch/AutoresearchPanel.test.tsx:60-62
**Tailwind classes asserted in test**
This assertion couples the unit test to `animate-spin` and `motion-reduce:animate-none`, so a behavior-preserving styling refactor will fail the test while the assertion still does not verify that the spinner is visibly animated. Cover the visual behavior through an appropriate behavioral or visual test instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Fix document-wide recalcs and svg spinne..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
HostHog preview —
|
Generated-By: PostHog Desktop Task-Id: 77f7bbc8-7141-41b7-9bbb-639e04ee0878
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified the diff against its perf claims: the new CSS variables set on body do match how quill's own layers.css consumes --quill-z-* and pointer-events, and the loading-bar/spinner refactors are consistent across the ~30 mechanically-updated files I sampled. No auth/billing/data/API/CI surface is touched, the author is on the owning team with very strong recent familiarity, and there are no unresolved review concerns or in-progress reviewers blocking merge.
- Author wrote 78% of the modified lines and has 28 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 314L, 29F substantive, 327L/31F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (327L, 31F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 2b1c6f8 · reviewed head 2b1c6f8 |
charlesvien
changed the title
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). Resolved comments: 2 fixed |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The pending-link spinner moved size-3 to the Spin wrapper, leaving
CircleNotch at its 1em default. In the text-sm paragraph the icon
rendered at 14px inside a 12px wrapper, so it sat 1px low and wobbled
as the wrapper rotated about a different center.
Put the size on the icon (size={12}) and let the wrapper size to it,
matching the CliCheckPanel and PrChecksSection call sites.
Generated-By: PostHog Desktop
Task-Id: c28dac69-31ef-414a-bb15-4129aa1036ee
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Contained, single-team desktop UI perf fix (CSS/animation-layer changes across 31 files, all owned by the author's team with STRONG familiarity) — no risky territory (no data model, API, billing, auth, crypto, dependency, or CI/build changes). The one unresolved inline thread flags a real but purely cosmetic double-speed spinner rotation in one loading state, which is fix-forward material, not a functional/security/data showstopper; the other flagged issue was already fixed in the current diff (ChatMarkdown now passes an explicit size prop) and a prior human review approved an earlier revision.
- Author wrote 78% of the modified lines and has 28 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
- Unresolved inline comment on PreBaselineState.tsx: nesting the new
Spinwrapper around quill's ownSpinner(which still carries its own CSS animation) produces a double-speed, 1px-offset rotation — confirmed present in the current diff. Cosmetic only, but worth a quick follow-up fix. - Greptile flagged that the AutoresearchPanel test now asserts Tailwind utility classes directly rather than visible spinning behavior — minor test-quality nit, not blocking.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 314L, 29F substantive, 327L/31F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (327L, 31F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ e5c3d35 · reviewed head e5c3d35 |
The pre-baseline card wraps quill's Spinner in Spin. Quill's Spinner self-animates via .quill-spinner (components layer) and defaults to 16px. With no className on the child, the wrapper's animate-spin then compounded quill's SVG animation, so the icon turned at double speed and wobbled, shrank to 16px inside its 20px box, kept the main-thread SVG animation this PR removes elsewhere, and ignored reduced motion (the wrapper's motion-reduce:animate-none never reached the child). Pass size-5 and animate-none to the child. Both are utilities, which outrank the components-layer .quill-spinner, so the SVG is 20px again and its own animation is cancelled — only the compositor-run wrapper spins. Extend the existing test to assert both classes on the icon. Generated-By: PostHog Desktop Task-Id: c28dac69-31ef-414a-bb15-4129aa1036ee
A new stamphog review started for this PR — the fresh verdict replaces this approval.
Generated-By: PostHog Desktop Task-Id: a99f8e61-f6bc-4421-8fc7-63f1a2495487
There was a problem hiding this comment.
Mechanical, well-verified perf refactor confined to desktop UI styling/animation; author owns this code with STRONG familiarity, tests were updated, and prior bot review comments were resolved with no unresolved concerns. No risky territory touched.
- Author wrote 77% of the modified lines and has 28 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 314L, 29F substantive, 327L/31F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (327L, 31F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 8dbaa6e · reviewed head 8dbaa6e |
|
This pull request was merged into |
2 participants
Problem
Opening a session, Settings or "Back to app" stutters. In the packaged build each click froze the renderer for 300ms to 1.4s. Traced over CDP, three causes:
body:has(...) [data-quill-portal]rule made Chromium restyle the whole document after almost every React commit. Opening one session ran 68 full-document recalcs (3.6s).animate-spinon an<svg>, which Chromium animates on the main thread.ConnectingToAgentstays mounted atopacity-0behind the composer, so every open session paid a style recalc and layerize pass per frame.leftandwidth(layout every frame), even while collapsed.Changes
:has()rule now sets quill's--quill-z-*variables onbodyinstead of restyling descendants. Popovers still sit above Settings and stay clickable under Radix dialogs.SpinandSpinnerprimitives put the animation on a wrapping span, which the compositor runs. All 41 spinners use them (mechanical).ConnectingToAgentstops spinning while hidden.transformonly, and only while active.docs/CONVENTIONS.mdrecords the three rules.Nothing looks different. Dev build, same clicks and state before and after:
Packaged build before the change: "Back to app" 516 to 607ms per click, session switches 287 to 1,418ms. Not re-measured after.
How did you test this code?
:has()triggers intobodyand checked quill portals get z-index 101 and pointer-events auto, then revert.AutoresearchPanel.test.tsxmoved to the wrapper span.