Lazy-load project dialogs and thread storage tree - #2086
Conversation
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
I am SlopCop, and I have started this review.
This pull request delays the load of project dialogs and the thread storage tree. The goal is a smaller initial app bundle and faster startup.
I will check security, code quality, architecture, performance, and the main user paths. I will post one final review after the checks finish.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
In plain English, this pull request removes project-dialog forms and the thread-storage tree from startup code. The app now downloads those parts after the user opens the related interface. The production build confirms a smaller boot payload and a much smaller first thread-route payload.
I found three issues that should be fixed before merge:
- The lazy dialog fallback has no accessible name or description. A screen reader receives an unnamed dialog during a cold chunk load.
- Project Settings still uses the eager path dialog. That route downloads the path dialog and
RemotePathBrowserbefore the user opens it. - The bundle checker accepts route-level
forbiddenModulesbut ignores the resulting module offenders. A forbidden route module can pass the check.
I left line comments with a concrete fix and test for each issue.
The security review found no defect. Fixed local imports, existing storage path containment, React text escaping, and unchanged dependency versions keep the new boundaries low risk.
The production app build and bundle check passed. Focused changed-file tests passed. The app type check passed. GitHub CI is green. A browser smoke test confirmed that the target project-dialog bodies are absent from the initial compose resource list.
The architecture scan found one optional performance follow-up. The 66,224-byte Brotli storage owner loads after any first secondary-panel open, including a non-Info tab. This is still an improvement over the base branch, which loaded it before any panel action. A later change can delay it until the Info tab needs storage data.
The scan also found duplicate eager dialog wrappers. Using the lazy path wrapper in Project Settings lets the code remove that duplication and reduces future drift.
8ff26a3 to
c018fff
Compare
c018fff to
3d09fd3
Compare
## What was wrong PR #2005 made live thread-storage files resolve through `useThreadStoragePaths({ limit: 1 })` even though the caller needs only the authoritative storage root and host. The server therefore sent `host.list_paths`, whose daemon handler recursively walks the complete storage tree before applying the response limit. Merely opening a file context menu or accepting an external-open request could perform large filesystem work and add remote-host latency. This is the root cause identified in [the post-merge review](#2005 (comment)); #2086 lazy-loads the storage browser but does not change this lookup. The first version of this PR added a direct location route, but review found three incomplete consumers: the Docs file opener still recovered the same metadata through `threads.get()` plus `storageFiles({ limit: "1" })`, reconnect catch-up omitted the new location query-key prefix, and the builtin plugin-authoring skill's exact SDK method map omitted `storageLocation`. ## What changed - Add `GET /api/v1/threads/:id/thread-storage/location` and `sdk.threads.storageLocation()`. The server reuses its existing authoritative `requireThreadStorageTarget` result and returns `{ hostId, storageRootPath }` without issuing a host filesystem RPC. - Resolve app live-file targets and Docs thread-storage open/save operations from that direct location. Both preserve loading/error behavior, absolute-path construction, validated relative paths, `rootPath` confinement, and local-versus-remote host context without listing storage. - Include the location query in thread/environment cache ownership and reconnect invalidation so an active failed lookup recovers when the server socket reconnects. - Add `storageLocation` to the builtin bb-plugin-authoring skill's exact `threads` SDK method map. - Add focused route, SDK/contract, query, cache-owner, reconnect-recovery, Docs confinement, and resolver coverage. This is internal transport for existing file-open behavior, not a new end-user command or configuration surface, so no CLI/guide parity change is needed. - No host-daemon message changed, so `HOST_DAEMON_PROTOCOL_VERSION` is unchanged. This is the smallest complete fix because the server already knows the host and derives the root from the active host session; exposing and consistently consuming that existing result deletes the broad listing dependency instead of optimizing or special-casing the recursive listing API. ## How you verified - Before the route existed, its new public-route regression failed with `expected 404 to be 200`. Afterward, the focused server route passed without reporting any host RPC response, proving the route does not enumerate the filesystem. - Before the Docs update, the storage-location-only regression failed with `bb.sdk.threads.get is not stubbed`; afterward, the focused Docs server file passed 32/32 while asserting the returned remote host and confined root on read, preview, and CAS write. - Before reconnect invalidation included the prefix, the active failed-location regression remained `undefined` instead of recovering to `loaded`; afterward, the focused cache-effect and cache-owner tests passed 11/11. - After rebasing onto `origin/main` at `e171ceba4`: `pnpm exec turbo run build typecheck --filter=@bb/app --filter=@bb/server --filter=bb-plugin-simple-notes` passed 10/10 tasks. A final post-commit Turbo typecheck also passed. - Forced full affected test matrix: app 414 files / 3,178 tests passed (3 skipped); server 195 files / 1,817 tests passed; Docs 3 files / 66 tests passed. - Earlier focused SDK and server-contract coverage passed 8/8 and 33/33 respectively; their full suites passed 96 and 58 tests. - `pnpm exec turbo run lint --filter=@bb/app` passed with 0 errors and 155 existing warnings outside the changed lines. - `git diff --check` passed. Fixes: N/A — no matching issue; post-merge follow-up to #2005. > AGENT GENERATED: by GPT-5.6-Sol
1 participant
What was wrong
Normally closed project dialogs were mounted from eager imports in the app shell, keeping
ProjectPathDialog,RemotePathBrowser,ProjectRenameDialog, andProjectDeleteDialogin the boot static-import closure. Separately,ThreadDetailVieweagerly created the thread-storage Pierre tree model even when the persistent secondary panel had never opened, pulling@pierre/treesand its Preact runtime into everySplitWorkspaceRoutefirst paint.What changed
ProjectActionsProvideradd-source/rename/delete paths all use the boundary; rename focus/select behavior is handed off when the lazy body arrives.RemotePathBrowseredge until the folder option needs it.useThreadStorageBrowserinto a lazy secondary-panel owner keyed to the thread. The owner realizes synchronously with an initially open persisted panel, remains mounted across panel closes and storage/file-tab cover transitions, and unmounts with the owning thread so Pierre cleanup still runs.ProjectPathDialogandRemotePathBrowserare also forbidden in the measured Project Settings route; and@pierre/trees,preact, andpreact-render-to-stringare forbidden at boot and inSplitWorkspaceRouteand permitted only throughThreadSecondaryPanelWithStorage. Route module offenders now fail the checker instead of being calculated and ignored.SplitWorkspaceRoute2,269,199 raw / 607,310 Brotli.This work was originally stacked on #2081. Because #2081 merged before this PR was opened, the commit was rebased onto current
mainand this PR targetsmain, per the requested fallback.How you verified
Production builds from the same
mainbase:SplitWorkspaceRouteThe new on-demand storage gate is
ThreadSecondaryPanelWithStorageat 259,829 raw / 66,312 Brotli bytes and contains@pierre/trees,preact, andpreact-render-to-string; none of those packages remain in the measured route closure. None of the four dialog source modules remain in the boot closure. The final Project Settings closure is 110,008 raw / 29,876 Brotli bytes and contains neitherProjectPathDialognorRemotePathBrowser; gating its independent machine-setup browser edge reduced that closure by 5,506 raw / 1,920 Brotli bytes during review.pnpm exec turbo run build typecheck --filter=@bb/app --force— 4 tasks passed.pnpm exec turbo run test --filter=@bb/app— 418 files, 3,186 tests passed; 3 skipped.node apps/app/scripts/check-bundle-budget.mjs— all three measured closures and all forbidden/on-demand gates passed.git diff --check.Fixes: N/A — performance work requested directly.