perf(app): activate plugin styles only while mounted - #2002
Conversation
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
| // Publish the URL before either non-React scripts mount or slot-store | ||
| // notifications can render plugin code. Inactive plugins only preload; | ||
| // an already-mounted generation starts a safe side-by-side replacement. | ||
| deps.applyCss(pluginId, candidate.bundle.cssUrl); |
There was a problem hiding this comment.
🚨 slopcop/review — Keep the old CSS until the UI generation changes
This call publishes the candidate URL while the old registrations stay mounted. A loaded candidate sheet can replace the old sheet during a slow content-script mount. A null URL removes the old sheet at once. This can show the old UI with new CSS or no CSS for the ten-second timeout.
Stage the candidate sheet without activation. Commit the sheet and registrations together after the script setup succeeds.
| record.url !== url || | ||
| record.consumers === 0 | ||
| ) { | ||
| link.remove(); |
There was a problem hiding this comment.
🚨 slopcop/review — Clear the superseded pending link
This branch removes a stale link but can leave record.pendingStylesheet on that removed element. I reproduced this with a loaded v1, a pending v2, and a return to v1. After the v2 load event, a later v2 request added no link. The URL check treated the removed element as active.
Clear pendingStylesheet when it still equals this link. Add a regression test for the v1, v2, v1, v2 sequence.
| instanceId, | ||
| onCrash, | ||
| }: PluginSlotMountProps) { | ||
| usePluginCss(pluginId); |
There was a problem hiding this comment.
🚨 slopcop/review — Cover the provider icon slot
This hook covers normal plugin mounts. The experimental_providerIcon path renders directly through provider-icon.ts. It does not keep the stylesheet active or add a plugin scope root. An icon-only frontend can render while its imported CSS stays inactive.
Use a shared plugin render wrapper. It should own the CSS lease, scope attributes, context, and error boundary.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This PR stops every plugin stylesheet from affecting every page. It keeps a sheet while that plugin shows UI or runs a content script. This should reduce WebKit style work.
Findings
- High: Candidate CSS can replace the old CSS while the old UI still renders. A slow content-script setup can keep this mismatch visible for ten seconds.
- Medium: A superseded stylesheet load can leave a stale pending element. A later request for that URL then adds no stylesheet.
- Medium: The provider icon slot bypasses the new CSS owner. An icon-only frontend can render without its imported CSS.
I left an inline comment for each finding.
Security and architecture
The security review found no issue. The server creates same-origin CSS URLs, and this change adds no new data or message boundary.
The codebase has no duplicate reference-counted stylesheet manager. The new plugin-css.ts module is the correct common location.
PluginSlotMount is the existing common render wrapper. The provider icon path should use that common ownership model.
Verification
- The app type check passed.
- The focused changed suites passed 77 tests.
- The full app run passed 3,043 tests and failed two unrelated timing tests under heavy parallel load.
- Both unrelated files passed all 16 tests in an isolated rerun.
- The browser test observed active plugin sheets change from zero, to one, and back to zero after navigation.
- GitHub CI passes at this head SHA.
0a333ce to
4cb4094
Compare
4cb4094 to
42effc0
Compare
What was wrong
Frontend activation installed every compatible plugin's generated
app.cssas a permanent stylesheet, even when the plugin rendered nothing on the current route. Seven installed frontend bundles therefore stayed active on representative pages and WebKit kept all seven rule sets in every document-wide style recalculation. The activation order also published CSS after content-script mount and slot registration, so a cold plugin deep link could render plugin DOM before its CSS URL was known; panel header contributions implicitly depended on a body co-mount for styling.What changed
navPanel.headerContentindependent CSS ownership, so split/header-only rendering does not rely on a panel body being mounted.app.cssfollows UI/content-script lifetime; app-wide selectable palette CSS usesbb.themes; styling existing app-shell DOM belongs in a cleanup-safe content script.bb.themesapplication style survives with zero plugin UI mounts.There is no server/host-daemon wire change, public plugin API addition, CLI change, or
HOST_DAEMON_PROTOCOL_VERSIONbump.Compatibility audit
I audited repository plugin sources and the locally installed/source marketplace examples, excluding generated bundles and dependencies:
.content-script-example-activeCSS on app-shell editables; it already owns the matching class/listeners in a content-script disposer, and now receives a generation-long CSS lease..outline-*and whose consumers live in its nav panel.1a2a96f73dd95363d4d6d2f25dc145cff8859074has no authored CSS import or app-shell DOM styling, but it does registerheaderContent; that case is covered by independent header ownership.<style>elements programmatically after editor use. Those tags are not generated frontendapp.css, so this lifecycle deliberately does not manage them. Their existing persistence is called out as a remaining follow-up risk below.bb.themes; the supported palette path is nevertheless independent (#bb-app-theme) and has regression coverage proving plugin UI teardown does not touch it.I found no audited always-on global
app.cssassumption beyond the content-script reference, which conforms to the new documented ownership model.How you verified
Automated validation
pnpm exec turbo run typecheck --filter=@bb/app— passed (3/3 tasks).pnpm exec turbo run test --filter=@bb/app --force— passed: 398 files, 3,045 tests; 3 skipped.pnpm exec turbo run build --filter=@get-bb/plugin-sdk --filter=@bb/app --filter=@bb/server --filter=@bb/host-daemon --concurrency=2— passed (8/8 tasks) for both baseline and branch production builds.pnpm exec turbo run test typecheck build --filter=@get-bb/plugin-sdk --filter=@bb/plugin-build --force— passed (10/10 tasks); SDK 13 files/105 tests, plugin-build 7 files/41 tests with 1 skipped. The two printed private-workspace-import errors are expected assertions from rejection tests.git diff --check origin/main...HEAD— passed.The new ordering/lifecycle tests fail against the old always-on/late-publication behavior and pass with this change.
iOS Simulator Safari profile
I profiled this branch independently; no numbers were reused from another thread.
origin/maina3a3a8aa3a5bf9fdbd554bebdb0be429e066b664.6b95eeb369730ce688068b94486dda36d3c22923.custom-instructions,inline-vis,outline,provider-retry,secrets,side-chat,tasks). The normal route displayed a median 15 sidebar thread links (15–15 before, 15–17 after); the Tasks route displayed 14. Median DOM sizes were 1,706 → 1,701 elements on the normal route and 703 → 698 on Tasks./plugins/tasks/tasks.document.documentElementand synchronously read computed style on the final DOM element 9 times (45 samples/cell, 180 samples/revision). Reported values are the median of each run's 9-sample median, with the range across the 5 run medians.The remaining normal-route sheet is
provider-retry, whose registered composer banner is a live mounted consumer; the Tasks route retains onlytasks. Inactive preload count after the 250 ms settle was median 0; one branch cold normal sample still had 3 in-flight low-priority preloads, which the load/error handlers remove when they settle.The baseline exposed the ordering bug in 1/5 cold Tasks runs: the first plugin DOM mutation had a Tasks root but no Tasks CSS link, and the first animation-frame sample had an unloaded sheet. After the change, 0/10 cold+warm Tasks runs showed a plugin root without its link; the Tasks sheet was already present and
link.sheetwas ready at the first DOM mutation and all 5 sampled animation frames in every run.The forced-recalculation result is strongest on the plugin route (30% lower cold median, 20% lower warm median) and the active-sheet count falls by 6 on both routes. First-render timings are noisy: Tasks improved in both cache modes, while the normal route's after run was slower, including one 2.84 s cold outlier. This change does not claim a normal-route first-render win; the measured result is stylesheet lifetime/style-recalculation reduction plus elimination of the observed cold deep-link CSS ordering gap.
Remaining risks
app.csslifecycle and persist after first editor use. Converting those to explicit owner lifetimes would be a separate change.nullis still a live CSS consumer (for example, the provider-retry banner). Finer-grained conditional ownership would require a plugin/host contract change and is not attempted here.Fixes: N/A — no linked issue.