Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f23bb220a2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| min-height: 100%; | ||
| width: 100%; | ||
| overflow-anchor: none; |
There was a problem hiding this comment.
Removing the data-is-scrolling CSS branch from the virtualized list rule disables hover suppression for all consumers of data-file-tree-virtualized-list, not just path-store/view.tsx. VirtualizedList (used by components/Root.tsx) still sets and clears container.dataset.isScrolling on scroll specifically to suppress hover paints, so this change leaves that mechanism ineffective and reintroduces scroll-time hover repaint churn in the legacy tree path. Either keep the selector for existing VirtualizedList behavior or update that path in the same commit.
Useful? React with 👍 / 👎.
if there's a good way to autoresearch on checking if these help or not, might be worth it. They definitely do not fix iOS Safari scrolling issues, but might help in some other generalized cases
|
Found 10 test failures on Blacksmith runners: Failures
|
|
|
||
| &[data-is-scrolling] { | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
Honestly not sure if i accidentally removed this or if this was from an earlier vibe code sesh
| stickyInset: Math.min( | ||
| 0, | ||
| viewportHeight - windowHeight + randomStickyOffset | ||
| ), |
There was a problem hiding this comment.
pushed the random offset to main, to avoid all the other stuff in this branch that i ruined, fyi
2 participants
Trees path-store scroll performance handoff
Current code state
Use the current branch as the handoff base.
Key files touched during this work:
packages/trees/src/path-store/view.tsxpackages/trees/src/path-store/virtualization.tspackages/trees/src/path-store/types.tspackages/trees/src/path-store/index.tspackages/trees/src/style.csspackages/trees/test/path-store-render-scroll.test.tsCurrent design decisions that should be preserved unless there is strong trace evidence otherwise:
wheeland refreshed onscroll.Verification state
Current code passes:
AGENT=1 bun test test/path-store-render-scroll.test.tsAGENT=1 bun test test/path-store-composition-surfaces.test.tsAGENT=1 bun run tscAGENT=1 bun run formatTrace files used during this work
Trace-20260416T205528.jsonTrace-20260417T105930.jsonTrace-20260417T115158.jsonTrace-20260417T120934.jsonTrace-20260417T121957.jsonTrace-20260417T123111.jsonTrace-20260417T123942.jsonWhat we tried and what we learned
1. Stable sticky buffer + translated content reduced measured layout in synthetic profiling, but broke the non-blanking technique
What changed:
computeStickyBufferLayout/computeStickyBufferRangedata-is-scrollingWhat happened:
packages/diffsand to the original reverse-sticky design clarified the issueWhy it failed:
Action taken:
computeStickyWindowLayoutConclusion:
2. Removing
data-is-scrollingcaused real hover/style churnTrace:
Trace-20260417T105930.jsonCompared to the baseline
Trace-20260416T205528.json:13.7 -> 44.91.1 -> 27.0165.6 -> 196.5127.7 -> 143.4Observed in the trace window:
pointerover:193mouseover:193data-is-scrolling: absentConclusion:
3. Restoring old list-scoped
data-is-scrollinghelped hover churn, but did not solve the whole problemTrace:
Trace-20260417T115158.jsonImprovement vs the hover-regressed trace:
27.0 -> 9.044.9 -> 30.5But overall scroll numbers in that run were still poor:
87.4252.862.6Conclusion:
4. Style-only hover gating was a bad idea
What changed:
data-is-scrollingTrace:
Trace-20260417T120934.jsonResult:
30.650.1Why:
Conclusion:
5. Coalescing scroll-driven updates to one frame was a real improvement
What changed:
updateViewportRef.current()still update immediately for focus/drag codepathsWhy it was tried:
packages/diffsbatches render work through a frame queueConclusion:
6. Arming suppression on
wheelbeforescrollhelped materiallyWhat changed:
wheelscrollTrace:
Trace-20260417T121957.jsonCompared to
Trace-20260417T120934.json:77.7 -> 58.1221.4 -> 183.546.8 -> 37.5171.1 -> 131.6Conclusion:
7. Full hover cleanup worked for hover/transition churn, but did not fix layout
What changed:
Trace:
Trace-20260417T123111.jsonImprovements vs
Trace-20260417T121957.json:26.2 -> 8.438.6 -> 16.9143.8 -> 043.4 -> 22.1But overall:
58.1 -> 67.2183.5 -> 194.6Conclusion:
8. Truncate marker paint was a real hotspot, despite only a few visibly truncated rows
This was the most surprising result.
Trace before truncate suppression:
Trace-20260417T123111.jsonEvidence:
PaintImagecount:18150PaintImage/sec:1825.4PaintImageevents were CSS pseudo-elements:nodeName: "::before"/"::after"isCSS: truesrcWidth: 4style.cssWhat changed:
[data-truncate-marker]and[data-truncate-fade]whiledata-is-scrollingis activeTrace after truncate suppression:
Trace-20260417T123942.jsonResult vs
Trace-20260417T123111.json:PaintImagecount:18150 -> 338PaintImage/sec:1825.4 -> 33.2149.1 -> 126.022.1 -> 18.18.4 -> 0.616.9 -> 0Conclusion:
Current best read from the trace series
Baseline
Trace-20260416T205528.jsonNormalized during scroll:
60.1165.630.2127.71451.013.71.1Latest cleaned-up trace
Trace-20260417T123942.jsonNormalized during scroll:
74.7225.338.8126.033.218.10.6Interpretation:
Important caveat on the manual traces
The manual scroll runs were not identical in intensity:
wheel,scroll, andSCROLL_MAIN_THREADWhat is likely still hot now
The latest trace points away from hover/truncate paint and toward layout.
Most likely remaining hotspots:
PathStoreTreesViewHighest-confidence next wins
1. Investigate scroll-driven React/render churn inside
PathStoreTreesViewWe already coalesce viewport updates to one frame.
The next question is: what still rerenders on every range change?
Best next step:
Likely win if confirmed:
2. Freeze or defer floating context-trigger positioning during active scroll
Observed indicators across the process:
updateTriggerPositionshowed up repeatedly in trace-string searchesgetBoundingClientRectappeared repeatedly as welluseLayoutEffectThis is not yet proven as the top remaining cost, but it is a credible next candidate.
Best next experiment:
data-is-scrollingis activeIf it wins:
3. Verify whether layout is still escaping the tree subtree
The original baseline showed document-level layout:
Layoutevents during the scroll window werepartialLayout: false#documentIf the latest traces still show that, the real remaining problem is likely not paint micro-optimizations but a remaining invalidation source that escapes the tree subtree.
Best next step:
Layoutevent args forpartialLayoutand layout root again4. Do not revisit the translated sticky-shell approach
This is worth repeating:
Treat that idea as closed unless the design goal changes.
What not to change casually
Keep these unless new trace evidence proves they are harmful:
Useful reference
packages/diffswas helpful as a reference for batching/scheduling patterns, especially frame-coalesced updates.It was not a drop-in geometry model for trees.
Treat diffs as inspiration for render scheduling, not for replacing the reverse-sticky non-blanking technique.
Most likely next best move
If there is time for only one more optimization pass:
If there is time for two:
PathStoreTreesView.update()path and cut unnecessary scroll-time rerenders