Repository navigation
Conversation
ArticleScreen subscribes to the whole article row via useLiveQuery, so every reading-progress save re-renders the screen even though nothing on screen depends on `progress`. This measures what that costs, so the question can be answered with numbers rather than reasoning. Method: identical progress writes to the open article's row (liveQuery re-emits -> re-render) and to a different article's row (same IndexedDB cost, no re-render). The difference is the re-render. Reports per-save task/script/layout/style time at 1x/4x/6x CPU throttling via CDP, long tasks, commits, and which components re-rendered (counted through a React DevTools hook stub, so no product code changes). `npm run profile:rerender` builds and serves a production React build first: against the dev server the numbers come out 5-7x worse. It refuses to run if anything already listens on the app port, since Playwright would silently reuse it and profile the wrong build. Current cost after #76: ~2.5/9/12ms per save at 1x/4x/6x, no long tasks, no layout. Not worth fixing — it lands at an idle moment and fits in a frame. On main without #76 the same run shows ~10/30/46ms with 3-13ms of layout, which is the DOM rebuild that fix removes. Also extracts the per-worktree port derivation from run-e2e.js into scripts/e2e-ports.js so the profiler serves on the port the suite expects, and documents a Paseo LD_LIBRARY_PATH issue that stops Chromium launching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for savrlist ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Answers a question raised during #76:
ArticleScreensubscribes to the whole article row viauseLiveQuery, so every reading-progress save re-renders the screen even though nothing on screen depends onprogress. Does that cost anything? This adds a repeatable way to measure it.Verdict: not worth fixing
With #76 applied, per save on a production build:
The save fires 1s after scrolling stops, so this happens at an idle moment, and it fits inside a single 16.7ms frame even at 6×. 125 components re-render, mostly MUI's Emotion styling layer and closed drawers. It's real but not something a reader would notice.
How it works
npm run profile:rerenderdoes three things:build:dev(production React with debug hooks) and servesdist/on the per-worktree e2e app port.It counts components by installing a stub of the React DevTools hook before React loads, so no product code changes.
Two safeguards:
reuseExistingServerwould silently profile whatever is there. This already caught a real case: a leftover preview server while I was testing this branch.The spec only runs when
SAVR_PROFILE=1is set, so it never runs in the normal suite. It also asserts that the control writes don't re-render, otherwise the subtraction would be meaningless.Evidence the harness detects regressions
This branch is cut from
main, without #76. Run here, the harness shows the DOM rebuild that #76 removes:The layout column is the tell: a metadata-only re-render should never cause layout. That's written up in
docs/DEVELOPMENT.mdas what a regression looks like.Other changes
scripts/e2e-ports.js: moved the per-worktree port derivation out ofrun-e2e.jsso the profiler serves on exactly the port the suite expects. Ports are unchanged; smoke tests pass.docs/DEVELOPMENT.md: a Profiling section with baseline numbers, a table row for the spec, and a known issue: Chromium fails to launch inside Paseo sessions. Paseo puts its ownalsa-libonLD_LIBRARY_PATH, and that build needs a newer glibc than the nix-provided Playwright Chromium has. The workaround isenv -u LD_LIBRARY_PATH npm run test:e2e.Not measured
With header hiding on, every change of scroll direction calls
setShowHeader, which probably re-renders the same 125-component tree during a scroll, when a frame actually matters. If anything here deserves work, it's that rather than the save. The harness could be extended to cover it.Independent of #76 and can merge in either order. Running it on
mainbefore #76 lands will show the pre-fix numbers.🤖 Generated with Claude Code