Skip to content

[issue sweep] Load the diff renderer and KaTeX on demand - #1527

Closed
SawyerHood wants to merge 3 commits into
mainfrom
sweep/issue-1072-lazy-diff-renderer
Closed

[issue sweep] Load the diff renderer and KaTeX on demand#1527
SawyerHood wants to merge 3 commits into
mainfrom
sweep/issue-1072-lazy-diff-renderer

Conversation

@SawyerHood

@SawyerHood SawyerHood commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

The thread page downloaded the diff renderer, Shiki and KaTeX before it could show anything. @pierre/diffs 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 existing boot budget could not see it.

The issue's diagnosis is wrong, and #1072 should be corrected

@/lib/workspace-checkout-display never imported the editor. The whole module is 85 lines: one import type { GitCheckoutRef } from "@bb/domain", a WorkspaceCheckoutDisplay interface, and a switch that 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.ts is 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.tsx and ThreadEnvironmentSummary.tsx import a formatter and a type. This change does not touch any of them — which also keeps it clear of #1507, which edits ThreadEnvironmentSummary.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 (via DiffFileCard) and TimelineFileDiffBlock.tsx reaching @pierre/diffs; ThreadDetailWorkerPoolProvider.tsx and PluginPanelView.tsx reaching it through the worker-pool provider; and markdown-preview.tsx reaching rehype-katex. Plus ThreadSecondaryPanel.tsx importing GitDiffCard for one constant.

The issue's target — "workspace-checkout-display under 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, DiffFilesPanelLazy and TimelineFileDiffBlockLazy load the diff renderer when a user opens a file or a diff.
  • markdown-katex loads KaTeX and its stylesheet only for a body that can contain math. remark-math runs with singleDollarTextMath off, so that means a body holding $$ — almost no message.
  • 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 through DiffWorkerPoolProvider, which also carries the code-theme sync that Let UI themes declare matching Pierre code themes #1459 added. 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.
  • vite-bundle-stats and check-bundle-budget gained 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:

chunks raw bytes
before 22 5,051,210
after 40 4,090,840
change −960,370 (−19.0%)

The chunk the issue names:

raw bytes
workspace-checkout-display-*.js before 2,167,788
workspace-checkout-display-*.js after 1,048,153
change −1,119,635 (−51.6%)

What 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:

package before after
@pierre/diffs 152 0
shiki 3 0
katex 2 0
rehype-katex 1 0
cytoscape 0 0

Boot payload, the cost of this change:

raw brotli chunks
before 1,695,458 442,880 12
after 1,700,344 452,505 22

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. maxBootBrotliBytes therefore rises from 445,742 to 460,800, with the reason recorded in bundle-budget.json. maxBootBytes is 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:

  • Whether 22 boot chunks cost more in request and per-response overhead than the 19% route saving returns — especially on a cold cache over a slow link, and for the desktop app where boot is served from disk and the route saving matters less.
  • Whether the boot chunk count keeps climbing. Each future lazy boundary re-partitions the shared graph, and this PR shows the effect is not free. If it reaches the low 30s, rolldown's advancedChunks grouping is the lever; I tried its global minSize here and it did not merge boot chunks, so it would need explicit groups.
  • Whether maxBootBrotliBytes should 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/trees and @xterm/xterm when 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:

  • Secondary-panel diff tab: expanded README.md, diff-worker-pool.ts and git-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.
  • File viewer: opened diff-worker-pool.ts in a preview tab, full syntax highlighting.
  • Timeline inline diff: expanded the agent's Created SMOKE.md row, diff renders with its added lines.
  • KaTeX: opened a markdown file holding $$E = mc^2$$ and a display block. Both render as KaTeX, and the KaTeX stylesheet loads with the chunk.
  • No page errors on any of these, and the composer survives, which is what the keep-alive design protects.

Fixes #1072

AGENT GENERATED: by Claude Opus 5


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 FileDiff inside 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/diffs statically, so it publishes the provider to a light registry when it evaluates. A slot can only render after that module has produced a registration, so PluginSlotMount reads 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. PluginPanelView now uses the same registry instead of its own wrapper, so there is one mechanism rather than two. PluginSlotMount.workerPool.test.tsx covers both halves.

P2 — The first math frame creates invalid HTML. Correct, and I had missed it. remark-math now 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 no p div / p pre and an empty console.error before and after the chunk resolves.

P2 — A failed KaTeX import cannot recover. Correct. The repo already had createRetryingModuleLoader for this exact problem, so I reused it rather than writing a second cache. It moved from plugin-frontend-lazy.ts to retrying-module-loader.ts with 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. DiffWorkerPoolProvider publishes 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 forbiddenRoutePackages and fails when a guarded route is missing from the stats, naming TRACKED_ROUTES in 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 in 8d0f4aa.

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: SplitWorkspaceRoute keeps 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.tsx covers 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-frontend is 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.

@bb-slop-cop

bb-slop-cop Bot commented Aug 13, 2026

Copy link
Copy Markdown

🚨 SLOP COP 🚨 · review

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>
@SawyerHood
SawyerHood force-pushed the sweep/issue-1072-lazy-diff-renderer branch from 04858d7 to c392eb1 Compare August 13, 2026 12:13
const base = allowHtml
? MARKDOWN_HTML_REHYPE_PLUGINS
: MARKDOWN_PLAIN_REHYPE_PLUGINS;
return katexRehypePlugin === null ? base : [...base, katexRehypePlugin];

@bb-slop-cop bb-slop-cop Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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(

@bb-slop-cop bb-slop-cop Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

Comment thread apps/app/src/lib/diff-worker-pool.ts Outdated
// `@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;

@bb-slop-cop bb-slop-cop Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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;

@bb-slop-cop bb-slop-cop Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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 />

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

@bb-slop-cop bb-slop-cop Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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:

  1. 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.
  2. P2 — The first math frame creates invalid HTML. The browser logged four React nesting errors before KaTeX finished loading.
  3. P2 — A failed KaTeX import cannot recover. The rejected promise stays cached for the session and can create an unhandled rejection.
  4. 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.
  5. 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>
@bb-slop-cop

bb-slop-cop Bot commented Aug 13, 2026

Copy link
Copy Markdown

🚨 SLOP COP 🚨 · review

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(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

@bb-slop-cop bb-slop-cop Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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>
@SawyerHood

Copy link
Copy Markdown
Collaborator Author

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.

AGENT GENERATED: by Claude Opus 5

@SawyerHood SawyerHood closed this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

workspace-checkout-display (1.7MB) bundles Monaco/Eddie editor eagerly — should be lazy

1 participant