Diffs: respect prefers-reduced-motion and prewarm syntax highlights#680
Conversation
|
@clemg is attempting to deploy a commit to the Pierre Computer Company Team on Vercel. A member of the Team first needs to authorize it. |
amadeus
left a comment
amadeus
left a comment
There was a problem hiding this comment.
Hey thanks for the PR! I think these are generally good things we should do, however I think there's some architectural changes we should make to this implementation (thus the whole note about chatting with us first before opening a PR).
Might also be good to break this into 2 changes (one for CodeView, and one for DiffsHub).
clemg
force-pushed
the
clemg/diffs-reduced-motion-prewarm
branch
from
a5fd497 to
12ca065
Compare
Library-internal improvements to the new CodeView component 1. TP to destination when prefers-reduced-motion is on 2. prewarm scrollTo targets to ⚡️blazing-fast™ TPs (item/line/range, [see this comment](pierrecomputer#680 (comment))) 3. prefersReducedMotion() exported so consumers can reuse it
|
Thank you for the fast response! I'm sorry about missing the discussion, I don't know why I felt confident that it could be skipped, will follow the rules for next time note: what do you think about exposing publicly the prewarm functions, so that we could call them sooner on hovering the file names in the file tree in the demo? Maybe it would include some false positives though (hovering a file name but not clicking it) |
|
Word, thanks for the changes! I just had a realization that the prewarm methods can't and shouldn't be on the FileDiff/File because it's the wrong scope and can create extra work (didn't realize this on my first review). The renderers have idempotency built in. I'm quickly having codex do an amendment to this and will probably push a commit to this branch shortly (easier for me to just directionally vibe this than try to do back and forths on the review process/explanations for it) |
|
Lol i know i just pushed 2 commits, but i realized all this is wrong and it needs to go in another direction to be more implementation sound. so more commits incoming 😬 |
|
What's wrong with the way you went? |
|
Essentially the version i wrote here utilized renderCache, however that's a poor system for pre-caching ast's because renderCache is also something that gets nulled out when recycled during virtualization. Also using renderCache ended up creating a bunch of new internal scenarios about the state of renderCache, making it more complicated to understand and reason about (it was no longer being tied to render calls). It also meant the component would start reacting to onHighlightSuccess events which may or may not be applicable if the component is no longer mounted. It could also create scenarios where ASTs get leaked and never GCd if the pre-warmed elements never render again. So, the better system will be: We already have a highlighted ast caching system with the WorkerPool, which is an LRU cache of responses from the WorkerPool that have a cacheKey. The renderers are already built to hydrate with this cache anyways, so the only thing we really need to tweak with WorkerPool is a system where if request for highlighting is still in progress, we just have the renderer piggyback onto it. This will also allow me to improve the system so if you mount multiple components with the same file or diff on your page, they could share in that same request which I think would also be a good thing to have. Anyways, hopefully that makes sense, and sorry for the thrash, I'm one of those people that's pretty dumb right off the bat and I have to tinker with things to really understand them and build a mental model for them. |
| interface GetRenderOptionsReturn { | ||
| options: RenderDiffOptions; | ||
| forceRender: boolean; | ||
| forceHighlight: boolean; |
There was a problem hiding this comment.
Renamed this because because forceRender didn't make sense in context of the render function, it actually represented a forceHighlight (mostly atoning for my past mistakes here)
| return true; | ||
| } | ||
|
|
||
| public primeHighlightCache(): void { |
There was a problem hiding this comment.
Felt like prime was a cleaner name than prewarm
| this.diff = undefined; | ||
| this.renderCache = undefined; | ||
| this.workerManager?.cleanUpPendingTasks(this); | ||
| this.recycle(); |
There was a problem hiding this comment.
Just a bit of house keeping here.
| private activeTaskById = new Map<WorkerRequestId, AllWorkerTasks>(); | ||
| private activeRequestByInstance = new Map< | ||
| RenderTaskInstance, | ||
| WorkerRequestId |
There was a problem hiding this comment.
Basically took the time while making these updates to try and have a more consistent naming structure.
queued: waiting for a worker
active: being sent to the worker or waiting on a response.
Library-internal improvements to the new CodeView component 1. TP to destination when prefers-reduced-motion is on 2. prewarm scrollTo targets to ⚡️blazing-fast™ TPs (item/line/range, [see this comment](pierrecomputer#680 (comment))) 3. prefersReducedMotion() exported so consumers can reuse it
* prewarming is actually responsibility of the renderer, not the component * ensure that we never kick off a prewarming job if we're already highlighted * Added a bit more safety to worker pool to not kick off a highlight task if we have a valid cache * utils'd a few functions
* it shouldn't take a file/diff, because it invites a scenario where things get out of sync * virtualizedfile/filediff always have their appropriate content on their instance, so this will ensure consistency
This is the new improved cache primer.
* Safely idempotent (will not fire if the file/diff is already in the
LRU cache)
* Only works with LRU cache, for good reason
* No mucking around with file/diff components to make a confusing API
* New system to ensure that request for elements with a highlightKey can
be shared
* highlightKey is an amalgamation of the type (file/diff), the
cacheKey, and a version for renderOptions
amadeus
force-pushed
the
clemg/diffs-reduced-motion-prewarm
branch
from
41f79dc to
df8ff01
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
![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: df8ff01453
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
(probably safety that isn't actually applicable, but its probably good to have)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0093c7b41c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for bearing with me through this! 🙏 |
|
No worries, it's very cool seeing you work your way through this! (including dead ends) I have two small questions:
This has been very instructive 😁 |
2 participants
Description
Implements #679
Two little changes across two areas:
diffs:
resolveEffectiveScrollBehavioruses'instant'whenprefers-reduced-motionis enabledscrollToto trigger the syntax highlighting as soon as we can, reducing the length of the FOUC. It's scoped to only warm the file(s) or line(s) that will be visible, ignoring previous and following stuffMotivation & Context
See #679
Saw the demo on Twitter, played with it, and saw that this was missing (I use it frequently). Then the syntax highlighting taking too much time bothered me and figured I might as well propose a small fix
Type of changes
first discussed with the dev team and they should be aware that this PR is
being opened
You must have first discussed with the dev team and they should be aware
that this PR is being opened
(discussed in #679, happy to split into 2 PRs if you want, but I feel like that small and related enough for just 1)
Checklist
contributing guidelines
bun run lint)bun run format)bun run diffs:test)How was AI used in generating this PR
I used CC to review my changes and to test many times with large diffs to confirm I wasn't crazy thinking prewarm was actually useful
Code, issue and PR are human-made
Related issues
Discussion: #679