Repository navigation
perf(ui): take preview frames and the ambient tint off the per-frame path - #443
Merged
Merged
Conversation
The shell wrote the canvas's dominant hue to :root every time its rounded value changed, which the 2 Hz analysis made up to twice a second. Each write restyled the whole document, restarted the preview halo's 2 s box-shadow transition before it could finish (so the halo animated on every frame forever, even for a steady effect whose hue only jittered by a degree), and started a scrollbar-color transition on every transition-all button, because scrollbar-color is set on every element and tracks the hue. On a 60 fps Effects page that was 344 restyled elements and 18 paint events per presented frame in a headless Chromium trace; after this change it is 72 and 4, and the remainder is the halo drift plus three decorative animations that run with or without a preview. The tint is decorative chrome at single-digit alpha with no declared cadence contract, unlike the preview itself, so it now publishes at most once a second and only for a move of 4 degrees or more (AmbientHuePublisher). Published values are unwrapped so CSS turns the short way around the wheel. The halo interpolates on a registered, non-inherited --ambient-hue-drift over the same 2 s ease-silk curve, so each frame of the drift restyles the cabinet alone and nothing below it. The halo colors are rebuilt from new channel tokens because the composed --ambient-* colors already substituted the root hue. Buttons, links, spans and icons pin scrollbar-color to auto; none of them can scroll, so nothing they show changes. Reduced motion still snaps the halo: its transition: none !important now covers the drift property. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Two paths built a fresh wasm-bindgen closure per frame. The rAF Scheduler created a Closure for every requestAnimationFrame and dropped it after it fired, and the preview decoder re-armed its idle-expiry timeout after every binary message. A chunked frame always leaves a partial publication between its chunks, so that timer was rebuilt with Closure::once_into_js once per frame, and those closures are only released when the garbage collector finalizes them. Every closure is a JS function plus a FinalizationRegistry entry, which is the finalizeUnconditionally and visitChildren time in the WebKit profile. The Scheduler now builds one closure up front and reuses it for every frame it requests. The expiry sweep stays armed while its deadline is no later than the one pending: a sweep that fires early expires nothing and re-arms for the deadline then due, so idle partials are still swept on time. On the 60 fps Effects page, finalization registry registrations fall from 67.5/s (1.24 per received frame) to 0.12/s. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A hidden browser page kept its canvas, screen-canvas and web-viewport subscriptions at a 6 fps cap, so the daemon kept encoding frames nobody could see and the page kept receiving and decoding them. Only a hidden Tauri window unsubscribed, and page visibility is what a backgrounded, minimized or occluded WebKit window reports. should_stream_preview now requires a visible page as well as a visible app window, and screen zones follow the same gate that display previews already used. The hidden-tab FPS cap is gone with the stream it capped. Becoming visible resubscribes through the same path a reconnect takes, so the request starts from a cleared state and no epoch is skipped. Measured on the Effects page with a 60 fps effect: while hidden the socket carried 11.6 messages and 7.1 MB per second and the daemon published 6 canvas frames a second; now both are zero, and the stream is back at about 54 fps within three seconds of becoming visible. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The canvas, screen-canvas and web-viewport effects rerun every time the engine reports its target fps, about twice a second, and their not-streaming branch sent an unsubscribe on every run whether or not the stream had ever been requested. A page showing only the main canvas sent screen-canvas and web-viewport unsubscribes in pairs, 18 redundant messages in six seconds, each parsed and answered by the daemon. The branch now acts only when a request is outstanding. A reconnect clears the requested state together with the socket that held the subscription, so nothing is left subscribed that the guard skips. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A 640x480 RGBA canvas frame is larger than the 1 MiB message budget, so the daemon sends it as two chunks. The UI copied each chunk out of its WebSocket buffer into a WASM Vec, the reassembler copied that into the publication buffer, and the decoder copied the result back out into a new JS Uint8Array: three full-frame copies, two WASM allocations and one JS allocation per frame, about 220 MB/s of memmove at 60 fps before the texture upload, which is the copy WebKit's WebContent profile showed. The reassembler's state machine, budgets, ordering and tombstones are now generic over where a publication's bytes accumulate (PreviewPublicationBuffer), so they stay single-sourced. Vec<u8> keeps the native path byte-identical for the TUI and daemon tests, and a new JsPreviewPublicationBuffer keeps payloads in JS. Chunk envelopes decode through a shared PreviewChunkHeader from their leading bytes, so only those bytes enter WASM. The assembled buffer then goes through the same view decoders and metadata check as a frame that arrived in one message, and the presenter uploads from a view of it. JS allocations use Reflect.construct so an oversized buffer reports failure instead of throwing through WASM frames. Per received frame on the Effects page: copies into WASM fall from 1.23 MB to 124 bytes, copies out of WASM from 1.23 MB to zero, the in-WASM copy is gone, and one 1.23 MB JS-to-JS copy remains, the floor for a frame split across messages. Script time per received frame fell from 1.27 ms to 0.97 ms. Browser tests run every step through the native and JS reassemblers and require identical outcomes and bytes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The first publish normalized the measured hue into [0, 360) without looking at what --ambient-hue already held, which is the token's 320 until something is published. A first hue below 140 then made the halo drift up to 320 degrees the long way, through every hue in between, over two seconds at app load, where the old Oklab box-shadow crossfade went straight there. A remounted shell had the same problem against the unwrapped value it left on the document element, and could spin several laps. AmbientHuePublisher now starts from a known value: the shell resumes from the --ambient-hue left on the root, or the 320 fallback, and the first publish unwraps against it like every later one. A first hue inside the deadband of that value publishes nothing, since CSS is already there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
After any reconnect the canvas preview stayed dark until something else changed its subscription. Opening a socket nudges the preview effects while that socket is still connecting; the subscribe they send then throws inside WebSocket.send, the error is dropped, and the request is recorded as sent. Every later rerun, including the one the hello's engine target triggers, matched that record and returned. Force-closing the page's socket mid-stream left the client at 0 frames a second, and the daemon published none, on origin/main and on this branch alike. Waking a Mac reconnects the socket, so this hit the desktop app's resume path. The canvas, screen-canvas and web-viewport effects now wait for ConnectionState::Connected, the same gate the display-preview and device-metrics effects already use. Connecting and closing clear the recorded requests, and the move to Connected reruns the effects against the open socket. With the fix the stream is back at the daemon's rate within the same window. The unsubscribe guard's comment now gives its reason without describing earlier behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Chunked publications now decode through the single-message view decoders in the UI, and a frame that failed there was dropped without a trace. The native reassembler used to report the same failure as a rejected chunk publication, so the warning comes back, next to the existing one for a metadata mismatch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Stream state was measured with size_of the generic stream state, so the JS-backed reassembler, whose buffer handle is smaller than a Vec, counted 164 bytes a stream where the native one counts 172. The UI's state and tombstone budgets therefore admitted about 5% more streams than native clients and than the UI did before its storage changed. The budget now measures state with the native layout for every storage type, and the browser parity tests assert equal state and tombstone bytes at every step. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
AmbientHuePublisher starts from 320 because that is what --ambient-hue and the --ambient-hue-drift initial value hold before the first publish. Nothing tied the three together, so moving one alone would bring back the long-way sweep on first load. The test reads both CSS files and holds them to AMBIENT_HUE_FALLBACK_DEG. Also drops an em dash from a comment touched earlier on this branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 44.90% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 98 functions across 12 files. (5 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: moving preview-frame handling and ambient tint updates off the UI per-frame path. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 44.90% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 98 functions across 12 files. (5 skipped: 5 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
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
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.
On a Mac, the desktop app's Effects page with a live 60 fps preview cost about 55% of a core across three WebKit processes, and WindowServer sat near 44%. A
sampleprofile of the page showed whole-document style recalculation and layout every frame, three megabytes ofmemmoveper frame, and garbage collector churn fromJSFinalizationRegistry. With this PR the preview path moves frames without copying them through WASM, stops minting a closure per frame, the ambient tint no longer restyles the whole page, and a hidden page stops the preview stream on both ends. The preview itself is unchanged: same 60 fps, same full-size RGBA, same bytes on the wire.💡 What changes
The ambient tint leaves the frame path. The dominant hue was already analysed at 2 Hz on about 200 sampled pixels, so the analysis was cheap; the cost was what each write triggered. Every write of
--ambient-hueon:rootrestyled the whole document, the halo's two-secondbox-shadowtransition restarted every 500 ms and therefore animated forever, and about fifteentransition-allbuttons and spans animatedscrollbar-colorafter each write. The hue is now published at most once a second and only when it moves 4° or more. The halo drifts on a registered, non-inherited--ambient-hue-driftover the same two-second curve, so each frame of the drift restyles one element. Elements that cannot scroll pinscrollbar-color: auto. Reduced motion still snaps. The tint is decorative chrome at single-digit alpha with no cadence contract of its own, unlike the preview, which is why its publish rate can drop.No full-frame copies through WASM. A 640x480 RGBA frame exceeds the 1 MiB message limit and arrives as two chunks. Reassembly copied each frame three times through WASM memory; it now assembles the chunks in JS. The native reassembly used by the TUI and daemon tests is unchanged, and headless Firefox browser tests run every step through both and require identical bytes. The WebGL path already reused one texture.
No closure per frame. The
requestAnimationFramescheduler reuses one closure, and the chunk-expiry timer re-arms only when needed.Hidden means idle. A hidden page kept a 6 fps preview stream (about 7 MB/s, with the daemon still encoding six frames a second). Hidden now unsubscribes, both ends go quiet, and the stream is back at about 54 fps within three seconds of the page becoming visible.
Preview resumes after a reconnect. This was an existing bug: after any socket close, including a Mac waking from sleep, the preview stayed dark. The subscribe went out while the new socket was still connecting, failed silently, and was remembered as sent. Preview effects now wait for the connection to be fully up. The page also stops sending 18 needless unsubscribe messages every six seconds, decode failures of reassembled frames are logged again, and the browser client's reassembly budget matches the native one.
🧪 Validation
Measured in headless Chromium 153 with software WebGL against a private daemon in a
bwrapsandbox (own network, no hardware), Rainbow at 60 fps, the preview subscribed as full-size RGBA. The host was heavily loaded and software WebGL capped presented frames at 26 to 35 fps against 55 received, so the figures are per frame and medians of three interleaved rounds.These numbers come from Chromium. WebKit on macOS, where the problem was reported, has not been measured yet, so the gains there are inferred from the same per-frame work disappearing.
just ui-testjust ui-buildhypercolor-uilibrary check and clippy with warnings as errorscargo check --workspace --all-targetsAn independent verifier failed the first pass on one visual bug (the halo could swing the long way round the colour wheel on first load);
3657a0256fixes it, and the verifier passed the result.🎨 Visible differences
During a drift the halo rotates hue at constant lightness and chroma instead of crossfading; the old endless transition held it near 0.078 chroma, and it now stays at the intended 0.20. Engines without
@property(Safari before 16.4) snap instead of drifting. Interactive previews close and reopen when the page hides and returns.📌 Still expensive
rgbfor local connections would cut bytes by a quarter and fit a frame in one message, but it moves a conversion into the daemon and changes alpha handling.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests