[issue sweep] Load the diff renderer and KaTeX on demand - #1527
[issue sweep] Load the diff renderer and KaTeX on demand#1527SawyerHood wants to merge 3 commits into
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
Every thread page downloaded the diff renderer before it could render. `@pierre/diffs` and Shiki reached the thread route through four static imports, and `rehype-katex` reached it through markdown-preview. None of that code is on the boot path, so the boot budget could not see it. Put each surface behind its own boundary: - `FilePreviewLazy`, `DiffFilesPanelLazy` and `TimelineFileDiffBlockLazy` load the diff renderer when a user opens a file or a diff. - `markdown-katex` loads KaTeX only for a body that can contain math. With `singleDollarTextMath` off, that means a body holding `$$`. - `git-diff-view-options` holds the shared option object, so the secondary panel no longer imports `GitDiffCard` for one constant. The worker pool needed more care. Its provider wrapped the whole thread workspace, so it could not simply become lazy: a wrapper mounted later remounts the workspace and loses composer state. Each diff surface now supplies the pool to its own subtree, and `ThreadDetailWorkerPoolKeepAlive` renders alongside the workspace to hold the reference count above zero between surfaces. A thread whose user never opens a diff now spawns no diff workers at all. The bundle stats plugin and budget check gained a route-level package rule, because nothing else can catch a barrel import putting this code back on the thread route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
04858d7 to
c392eb1
Compare
| const base = allowHtml | ||
| ? MARKDOWN_HTML_REHYPE_PLUGINS | ||
| : MARKDOWN_PLAIN_REHYPE_PLUGINS; | ||
| return katexRehypePlugin === null ? base : [...base, katexRehypePlugin]; |
There was a problem hiding this comment.
🚨 slopcop/review — The first math render produces invalid HTML before KaTeX loads.
When the plugin is null, remarkMath still emits math code nodes. The normal code renderer then emits div and pre elements inside a p. A direct browser render logged four React nesting errors. Render a safe placeholder, or special-case math nodes until KaTeX resolves. Add a test that checks the DOM and console before the import resolves.
| useEffect(() => { | ||
| if (!wanted || plugin !== null) return; | ||
| let cancelled = false; | ||
| katexRehypePluginPromise ??= import("./markdown-katex.js").then( |
There was a problem hiding this comment.
🚨 slopcop/review — A failed KaTeX chunk load stays cached for the full session.
This promise has no rejection path. One network or deployment failure leaves a rejected promise that every later math body reuses. Math then stays raw until a page reload, and the rejection can be unhandled. Clear the cache on failure and define a retry policy. Test one failed import followed by a successful import.
| // `@pierre/diffs` before a user opens a diff. This flag is the one bit the | ||
| // light side is allowed to know about the heavy side: DiffWorkerPoolProvider | ||
| // sets it when its chunk evaluates, and DiffWorkerPoolKeepAlive reads it. | ||
| let diffWorkerPoolProviderLoaded = false; |
There was a problem hiding this comment.
🚨 slopcop/review — This permanent flag recreates workers on a later unused thread route.
After one diff loads, this value never returns to false. If the thread area unmounts, a later thread visit mounts the keep-alive immediately and can create up to eight workers before that thread opens a diff. Track demand for each mounted thread area, and test a leave-and-return route sequence with worker counts.
| output.type === "chunk" && | ||
| output.facadeModuleId?.endsWith(route.moduleSuffix) === true, | ||
| ); | ||
| if (routeChunk === undefined || routeChunk.type !== "chunk") continue; |
There was a problem hiding this comment.
🚨 slopcop/review — A missing tracked route makes the new budget check pass without work.
A route rename, a moved file, or a changed Vite facade can make this lookup fail. The generated routes list then omits thread, and the checker only examines entries that exist. Fail the build when a configured route is absent, and add a missing-route fixture that must exit with an error.
| return ( | ||
| <ThreadDetailWorkerPoolProvider> | ||
| <> | ||
| <ThreadDetailWorkerPoolKeepAlive /> |
There was a problem hiding this comment.
🚨 slopcop/review — Thread plugin panels lose the diff worker context here.
The keep-alive is a sibling of SplitThreadAreaContent, so its WorkerPoolContextProvider cannot supply React context to the workspace. The old wrapper covered thread panel actions, file openers, and message directives. The built-in diff surfaces now add providers, but these plugin surfaces do not. Add a lazy provider at those plugin boundaries, then test useWorkerPool() with a Worker stub.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This PR keeps large diff and math tools out of normal thread loads. It downloads them only when a user needs them. Five edge cases remain.
I found five issues:
- P1 — Thread plugin panels lose the diff worker context. The new keep-alive is a workspace sibling. React context cannot reach thread panel actions, file openers, or message directives.
- P2 — The first math frame creates invalid HTML. The browser logged four React nesting errors before KaTeX finished loading.
- P2 — A failed KaTeX import cannot recover. The rejected promise stays cached for the session and can create an unhandled rejection.
- P2 — Later thread routes can start unused workers. The permanent load flag can recreate up to eight workers before a later thread opens a diff.
- P2 — The route budget fails open. A missing route facade removes the route from the statistics, and the check reports success.
The security review found no issue. The dynamic imports use fixed internal paths. The Markdown sanitizer order stays correct.
The runtime checks confirmed the main performance result. A fresh thread loaded no Pierre, Shiki, or KaTeX modules and started no workers. Opening a diff or timeline file change loaded the renderer on demand. The composer kept its draft. KaTeX rendered after its chunk loaded.
The focused checks passed 64 tests. The app build, type check, bundle budget, and git diff --check passed. The thread route contained none of the forbidden packages.
The file-preview browser path was only partly verified. Its 13 focused tests passed. The UI did not activate the injected preview tab during this test.
The three explicit lazy wrappers preserve useful chunk boundaries. A small helper can reduce repeated wrapper code, but it must keep each literal dynamic import at its call site.
The branch rebased during this review from 04858d78 to c392eb12. I checked the range diff. The rebase preserved the current code-theme integration. The five findings above remain. Full runtime checks ran on 04858d78; the related logic did not change in the rebase.
One optional improvement remains. Start the diff renderer import when the diff tab activates, so the import and data request run together.
- Plugin slots inside the thread workspace lost the diff worker pool when the workspace stopped wrapping its subtree. `plugin-frontend` — the chunk every plugin component already comes from, and which already reaches `@pierre/diffs` — now publishes the provider, and `PluginSlotMount` reads it synchronously for the slot kinds that used to inherit one. No Suspense boundary, so plugin mount timing does not change. - `remark-math` joins the pipeline only once KaTeX is loaded. Alone it emits `<pre><code class="language-math">`, which this component renders as block elements inside the paragraph that held the math — invalid HTML that React reports. The TeX now stays literal for that frame. - The KaTeX import uses the repo's `createRetryingModuleLoader`, so one failed chunk fetch no longer leaves every later math body raw for the session. The helper moved to its own module with its tests. - The thread keep-alive latches on mounted diff surfaces rather than on a permanent "chunk loaded" flag, so opening a diff in one thread no longer makes the next thread spawn workers it has no use for. - The route budget iterates the budget rather than the stats, so a renamed or missing tracked route fails the build instead of passing silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am SlopCop. I am now reviewing this pull request against the review rule. I will check security, code quality, performance, architecture, duplicate code, and the main user path. |
| ); | ||
| const [engaged, setEngaged] = useState(false); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🚨 slopcop/review — The worker latch does not reset after workspace navigation.
engaged can change only from false to true. SplitWorkspaceRoute keeps this component mounted across thread, compose, and plugin page changes.
After one diff opens, up to eight workers remain active on later pages without diffs. This retains memory and can use battery.
Reset the latch after demand stays at zero and the workspace content changes. Preserve a short delay for virtual timeline changes.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
The app now gets a large diff tool and a math tool only when it needs them.
This is like opening a toy box only when you want that toy.
However, one worker switch stays on after you leave a diff.
Result
I found one P2 issue that needs a fix before merge.
The worker latch stays active across thread, compose, and plugin page changes. One opened diff can keep up to eight workers active.
The final GPT-5.6 review gate confirmed this route-lifetime defect. The gate returned REQUEST CHANGES.
I used a comment review. I did not use GitHub's approval or change-request action.
No security issue met the reporting threshold.
Architecture
The shared provider and retry loader remove duplicate code. I found no additional required refactor beyond the worker lifetime fix.
Validation
-
The app typecheck passed through Turbo.
-
The production build and the bundle check passed.
-
The focused run passed all 34 changed-area tests.
-
GitHub reports that all required checks passed, including the app test job.
-
The browser check did not finish because the documented local launcher could not access its data directory.
The keep-alive latched on for good. `SplitWorkspaceRoute` keeps the split workspace mounted across thread, compose and plugin navigation, so one opened diff held up to eight workers for the rest of the session. Scope the hold to the thread that engaged it. Within a thread the hold is steady, which is where the churn it guards against happens: a virtualized timeline scrolling its last diff row out of view. Leaving that thread releases it. Also state the ordering that makes the plugin renderer registry safe to read synchronously: `plugin-frontend` is the only writer of slot registrations and sets the renderer at module evaluation, before it can write any, so the value never flips between two renders of a mounted slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closing without merging. This came out of the 13 Aug 2026 overnight issue sweep. Sawyer is closing the unmerged sweep pull requests rather than carrying them. The branch is left in place, so this can be reopened or cherry-picked. The review history, the measurements, and the answers to each Slop Cop finding stay on this pull request.
|
Summary
The thread page downloaded the diff renderer, Shiki and KaTeX before it could show anything.
@pierre/diffsreached the thread route through four static imports andrehype-katexreached it throughmarkdown-preview. None of that code is on the boot path, so the existing boot budget could not see it.The issue's diagnosis is wrong, and #1072 should be corrected
@/lib/workspace-checkout-displaynever imported the editor. The whole module is 85 lines: oneimport type { GitCheckoutRef } from "@bb/domain", aWorkspaceCheckoutDisplayinterface, and aswitchthat returns label strings. It has no value imports at all, so it cannot pull anything into a bundle.The 1.7 MB was a chunk name, not an import edge. Rolldown names a shared chunk after one of the modules inside it.
workspace-checkout-display.tsis shared by five lazy entries, so it landed in the big shared chunk and the chunk took its name. The issue read that name as a cause. The chunk's actual contents on the base commit were@pierre/diffs(645 KB), the promptbox composer's TipTap and ProseMirror (~450 KB), Shiki (~240 KB), and the markdown pipeline.So the four listed importers are innocent.
ThreadDetailView.tsx,ThreadMetadataContent.tsx,PluginThreadChat.tsxandThreadEnvironmentSummary.tsximport a formatter and a type. This change does not touch any of them — which also keeps it clear of #1507, which editsThreadEnvironmentSummary.tsx.Splitting that module would have moved zero bytes. The issue's step 1, "split into a light type-only module and a heavy implementation module", has no heavy implementation to separate. Doing it would have renamed the chunk and changed nothing else.
Cytoscape was already lazy before this change, reachable only through mermaid's dynamic import. Verified on the base build; not touched.
The real cause was five static imports, none of them named in the issue:
FilePreview.tsx,DiffFilesPanel.tsx(viaDiffFileCard) andTimelineFileDiffBlock.tsxreaching@pierre/diffs;ThreadDetailWorkerPoolProvider.tsxandPluginPanelView.tsxreaching it through the worker-pool provider; andmarkdown-preview.tsxreachingrehype-katex. PlusThreadSecondaryPanel.tsximportingGitDiffCardfor one constant.The issue's target — "
workspace-checkout-displayunder 500 KB" — is therefore not a meaningful goal, because that chunk's name does not describe its contents. The goal this PR meets instead is the one the issue's own headline asks for: no editor, Cytoscape or KaTeX on the thread page's load path.What this change does:
FilePreviewLazy,DiffFilesPanelLazyandTimelineFileDiffBlockLazyload the diff renderer when a user opens a file or a diff.markdown-katexloads KaTeX and its stylesheet only for a body that can contain math.remark-mathruns withsingleDollarTextMathoff, so that means a body holding$$— almost no message.git-diff-view-optionsholds the shared option object, so the secondary panel no longer importsGitDiffCardfor one constant.DiffWorkerPoolProvider, which also carries the code-theme sync that Let UI themes declare matching Pierre code themes #1459 added.ThreadDetailWorkerPoolKeepAliverenders alongside the workspace to hold the reference count above zero between surfaces. A thread whose user never opens a diff now spawns no diff workers at all.vite-bundle-statsandcheck-bundle-budgetgained a route-level package rule. Nothing else can catch a barrel import putting this code back on the thread route.Numbers
Rebased onto
596aab429. Both sides measured from rolldown's own chunk metadata on a production build of that base and of this branch.Thread route static closure — the entry chunk, the route chunk, and everything they reach without crossing a dynamic import:
The chunk the issue names:
workspace-checkout-display-*.jsbeforeworkspace-checkout-display-*.jsafterWhat is left in that chunk is the thread timeline and the promptbox composer. Both render on arrival, so both belong there. The issue's 500 KB target assumed the editor was the bulk of it.
Modules of each package still statically reachable from the thread route, counted on the real module graph:
@pierre/diffsshikikatexrehype-katexcytoscapeBoot payload, the cost of this change:
The raw growth is the new boundary modules, about 4 KB. The brotli growth is larger because the new dynamic entries make rolldown split shared boot code into 22 chunks instead of 12, and more small chunks compress worse.
maxBootBrotliBytestherefore rises from 445,742 to 460,800, with the reason recorded inbundle-budget.json.maxBootBytesis unchanged and still has 7 KB of headroom.This is the one part of the change that makes something worse, and it is worth watching. The boot payload now arrives as 22 requests instead of 12 for 9.6 KB more compressed bytes. On HTTP/2 to the local bb server that should be noise next to 938 KB removed from the thread route, but I measured bytes, not wall clock, so I cannot claim it. What a reader should check before trusting the trade:
advancedChunksgrouping is the lever; I tried its globalminSizehere and it did not merge boot chunks, so it would need explicit groups.maxBootBrotliBytesshould come back down once the count settles. It is a ratchet, and 460,800 currently leaves about 8 KB of slack.Testing
pnpm exec turbo run typecheck --filter=@bb/app— passes.pnpm exec turbo run test --filter=@bb/app --force— 2,672 tests pass across 343 files.pnpm exec turbo run lint --filter=@bb/app— 0 errors.node apps/app/scripts/check-bundle-budget.mjs— passes, including the new thread-route rule. The rule is not inert: it failed on@pierre/treesand@xterm/xtermwhen I first listed them, which is how I learned the file tree and the terminal are still on the thread route. Moving those off is separate work, so they are deliberately absent from the list and noted there.Existing tests caught a real bug during the work.
useState(loadedKatexRehypePlugin)treated the unified plugin — a function — as a lazy initializer and stored its return value. Both the initializer and the setter now use the callback form.Four tests became async because their surfaces now cross a boundary. The negative math assertions stay synchronous on purpose: a body with no
$$must never load KaTeX at all.In the running app
Driven through the dev app on a real thread whose agent edited two files, checking each on-demand surface actually renders. Re-run after the rebase, since
596aab429's ancestry changed how the pool receives its theme:README.md,diff-worker-pool.tsandgit-diff-view-options.ts. All render with line numbers,+/−tints and Shiki highlighting. Highlighting proves the worker pool reaches the renderer through the new per-surface provider, and that the code-theme sync still applies.diff-worker-pool.tsin a preview tab, full syntax highlighting.Created SMOKE.mdrow, diff renders with its added lines.$$E = mc^2$$and a display block. Both render as KaTeX, and the KaTeX stylesheet loads with the chunk.Fixes #1072
Slop Cop review
All five findings fixed in
3c2d521.P1 — Thread plugin panels lose the diff worker context. Correct, and the sharpest finding: a plugin rendering
FileDiffinside a thread would have silently dropped to unhighlighted output.I first tried a lazy provider inside
PluginSlotMount. That broke 40 tests, because a Suspense boundary around every composer banner, message directive and header action changes plugin mount timing — too blunt. The shipped fix has no boundary at all:plugin-frontend, the chunk every plugin component comes from, already reaches@pierre/diffsstatically, so it publishes the provider to a light registry when it evaluates. A slot can only render after that module has produced a registration, soPluginSlotMountreads it synchronously.It applies to the slot kinds that previously inherited a pool — the thread workspace slots plus
navPanel— and not to homepage, settings, sidebar or thread-list slots. That boundary is deliberate: the provider spawns workers eagerly, and those slots never had a pool, so widening it would start workers on a page showing nothing but a plugin chip.PluginPanelViewnow uses the same registry instead of its own wrapper, so there is one mechanism rather than two.PluginSlotMount.workerPool.test.tsxcovers both halves.P2 — The first math frame creates invalid HTML. Correct, and I had missed it.
remark-mathnow joins the pipeline only once KaTeX is loaded, so the TeX stays literal text for that frame instead of becoming<pre><code>inside a paragraph. Regression test added: it asserts nop div/p preand an emptyconsole.errorbefore and after the chunk resolves.P2 — A failed KaTeX import cannot recover. Correct. The repo already had
createRetryingModuleLoaderfor this exact problem, so I reused it rather than writing a second cache. It moved fromplugin-frontend-lazy.tstoretrying-module-loader.tswith its three existing tests, since two callers now share it. The rejection path also logs instead of leaving an unhandled rejection.P2 — Later thread routes can start unused workers. Correct. The permanent flag is gone.
DiffWorkerPoolProviderpublishes a demand count while a diff surface is mounted, and the thread keep-alive latches on that count per mount. A thread area engages only after a diff appears inside it, so opening a diff in one thread no longer makes the next thread spawn workers.P2 — The route budget fails open. Correct. The check now iterates
forbiddenRoutePackagesand fails when a guarded route is missing from the stats, namingTRACKED_ROUTESin the message. Verified by adding a bogus route name: the check fails with that message, and passes again once removed.I did not take the optional suggestion to prefetch the renderer when the diff tab activates. It is a real improvement, but it is a new behaviour rather than a fix to this change, and it belongs in its own PR.
Re-verified after the fixes: 2,672 tests pass, typecheck and lint clean, bundle budget passes, and the dev app still renders diffs with highlighting and math with KaTeX — with no console errors, including no DOM-nesting errors on the first math frame.
Slop Cop re-review
The first pass only saw the pre-rebase SHA, so a second pass ran against
3c2d521. It raised one P2, fixed in8d0f4aa.P2 — The worker latch does not reset after workspace navigation. Correct, and it is the residual of my own round-1 fix. I had scoped the latch per component mount, which I described as "per thread area". It is not:
SplitWorkspaceRoutekeeps the split workspace mounted across thread, compose and plugin navigation, so the component outlives thread changes and one opened diff held up to eight workers for the rest of the session.The hold is now scoped to the thread that engaged it, keyed on the route's thread id. Within a thread it stays steady, which is where the churn it guards against actually happens — a virtualized timeline scrolling its last diff row out of view. Leaving that thread releases it. I did not take the suggested "reset after demand stays at zero, with a short delay": a timer would reintroduce exactly the transient-zero churn the holder exists to absorb, and the thread id gives the same answer without one.
ThreadDetailWorkerPoolKeepAlive.test.tsxcovers all three states: the hold survives the last diff surface unmounting, releases when another thread opens without a diff, and never engages for a thread that showed no diff. I checked the middle test is not vacuous — it fails against the previous latch.The re-review found nothing wrong with the plugin renderer registry from round 1, and called out the shared provider and retry loader as removing duplicate code. Its browser pass could not run — the local launcher could not reach its data directory — so the runtime evidence for this round is mine: the diff panel still renders with syntax highlighting on the fixed build.
I also wrote the ordering invariant that makes the registry safe into
plugin-diff-worker-pool.ts, since it is the load-bearing claim and was only implied before:plugin-frontendis the only writer of slot registrations and sets the renderer at module evaluation, before it can write any, so the value cannot flip between two renders of a mounted slot and remount plugin content.Final state: 2,675 tests pass across 344 files, typecheck and lint clean, bundle budget passes.