Add plugin navigation primitives - #2005
Conversation
6b307dd to
a917ae3
Compare
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| ? null | ||
| : { | ||
| kind: source.kind, | ||
| hostId: source.experimental_hostId, |
There was a problem hiding this comment.
🚨 slopcop/review — The Docs opener drops the selected remote host.
The new live target keeps source.experimental_hostId, but openerSource omits it. The Docs RPC then resolves host files with hostId: null. A remote file can read or save the same absolute path on the primary host.
Please pass the host ID through the opener source schema, the resolver, and both read and save calls. Add a two-host regression test.
| intent: ExperimentalFileOpenOptions; | ||
| onSettled: () => void; | ||
| }) { | ||
| const didSettleRef = useRef(false); |
There was a problem hiding this comment.
🚨 slopcop/review — The external-file queue stops after its first item.
React reuses this dispatcher when the host removes the first queue item. didSettleRef stays true, so the second accepted request never runs or leaves the queue.
Give each request an ID and use that ID as the dispatcher key. Add a test that submits two requests before the first settles.
| [intent, navigation, onClick], | ||
| ); | ||
| const anchor = ( | ||
| <RouteAnchor {...anchorProps} href={target.path} onClick={handleClick} /> |
There was a problem hiding this comment.
🚨 slopcop/review — A valid file name can activate an external URL scheme.
The relative-path validator accepts names such as vscode:foo and mailto:test. This href makes modifier clicks activate those schemes instead of the file controller. Invalid targets also keep an active native link.
Do not use the file path as the anchor URL. Use a scheme-safe anchor for valid targets, and render invalid targets as inert content. Add tests for these file names.
| (event: ReactMouseEvent<HTMLAnchorElement>) => { | ||
| onClick?.(event); | ||
| if ( | ||
| !shouldHandleUrlClick(event) || |
There was a problem hiding this comment.
🚨 slopcop/review — Explicit URL targets do not stay native.
The public contract says that an explicit anchor target remains native. This check does not reject target="_blank", so BB can claim the click and override the requested browser behavior.
Return false from the host path when the anchor has any explicit target. Add a test for _blank with the in-app browser preference.
| const environmentQuery = useEnvironment(environmentId, { | ||
| enabled: options.enabled && environmentId.length > 0, | ||
| }); | ||
| const storageQuery = useThreadStoragePaths( |
There was a problem hiding this comment.
🚨 slopcop/review — Thread-storage resolution scans the full storage tree.
This hook needs only storageRootPath. However, storagePaths makes the daemon scan the complete tree before it applies limit: 1. A context menu or external-open request can cause large filesystem work and remote latency.
Add a direct storage-location route that returns the root and host without a directory scan.
| if (hostId === null || path === null) { | ||
| throw new Error("Host file preview target is incomplete"); | ||
| } | ||
| const response = await sdk.files.read({ hostId, path, signal }); |
There was a problem hiding this comment.
🚨 slopcop/review — Host previews read and transform large files before they need the data.
The query reads the complete file and builds a base64 fallback before preview creation finishes. Media and HTML can then read the same file again through the preview lease. A 25 MiB file causes large duplicate work.
Create the fallback only after preview creation fails. Fetch file bytes only for preview types that need them.
| ) | ||
| ) : activeHostFilePath !== null && activeHostFileHostId !== null ? ( | ||
| renderFileOpenerReplacement( | ||
| <LazyHostScopedFilePreviewTabContent |
There was a problem hiding this comment.
🚨 slopcop/review — Closed panels keep host preview queries active.
The retained panel body stays mounted when the panel closes. This host-scoped preview receives no isPanelOpen value, so it can read and render a large remote file while hidden.
Pass isOpen to this component. Use it to disable the query, as the workspace and thread-storage preview paths do.
| return buildFilePreview({ contentBytes, mimeType, name, path, url }); | ||
| }, | ||
| enabled, | ||
| staleTime: 30_000, |
There was a problem hiding this comment.
🚨 slopcop/review — Large host preview payloads use the default cache period.
This query can hold full file bytes and a large data URL. Without HEAVY_PAYLOAD_QUERY_POLICY, inactive entries remain for five minutes and can use substantial browser memory.
Apply the shared heavy-payload policy. Consider a shorter period for absolute host files.
| > | ||
| {open ? "▾" : "▸"} | ||
| </button> | ||
| {environmentId === null ? ( |
There was a problem hiding this comment.
🚨 slopcop/review — Removed pull-request files link to missing live paths.
When file.status is removed, the worktree normally has no file at this path. The new live link opens a preview that must fail.
Render removed paths as plain text. Keep the separate diff expansion control available.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This pull request gives plugins shared controls for URLs, files, and right-panel tabs. It also moves several built-in plugins to these controls.
The design has a good goal. Plugins should use one BB navigation policy instead of separate custom code.
Review result
I found nine verified problems. Two can send file work to the wrong place or stop an accepted action.
High priority
- Docs drops the remote host ID. A remote file can read or save on the primary host.
- The external-file queue stops after one item. Later accepted requests remain in the queue.
Medium priority
- A valid file name can activate an external URL scheme.
- Explicit URL targets do not stay native.
- Thread-storage resolution scans the full storage tree.
- Host previews can read and transform a large file twice.
- Closed panels keep host preview queries active.
Low priority
- Large host previews use the default five-minute cache period.
- Removed pull-request files link to missing live paths.
Architecture and duplicate code
The shared navigation host is a sound direction. I found no separate duplicate system that must block this pull request.
The performance fixes should reuse two existing patterns. Add a direct storage-root query, and apply the shared heavy-payload cache policy.
I rejected three weak concerns. A budget increase does not prove bundle growth. Small tab lists make the repeated lookup low risk. Stable context values prevent a proven render fault.
Verification
The security, quality, and performance reviews ran in parallel. A separate GPT-5.6 review checked the combined findings.
The focused app checks passed 35 tests. The focused SDK checks passed 41 tests. Current GitHub CI checks also pass.
A live browser opened the test plugin. The URL control opened GitHub, and the fixed-tab control selected the plugin tab.
Please fix the high and medium findings before merge. This review uses a comment only. It does not approve or request changes.
b08c45e to
b494e78
Compare
b494e78 to
d68a893
Compare
## What was wrong The Docs file opener rebuilt the SDK's `PluginFileOpenerSource` for its private RPC contract but dropped `experimental_hostId`. Its backend schema could not accept that field, and host-file resolution hardcoded `hostId: null`, so opening, previewing, or autosaving a file selected on remote host A could read or overwrite the same absolute path on the primary host. ## What changed The Docs app now carries the existing optional `experimental_hostId` through its opener source for both `openFile` and `saveOpenedFile`. The Docs RPC schema and parser preserve that value, and host-file resolution returns it to the existing Files SDK read, preview, and write calls. When the field is omitted, resolution still returns `null`, intentionally preserving primary-host behavior. This is the smallest complete fix because it extends the direct data flow for the existing SDK source identity; it does not add a parallel host field or refactor unrelated routing. PR #2083 is now merged, and this branch is rebased on top of it; that PR changes persisted app-tab source routing, while this change fixes the separate Docs app/server RPC hop. There is no host-daemon wire change: the plugin RPC is server-owned and the Files SDK already supports its host selector, so `HOST_DAEMON_PROTOCOL_VERSION` is unchanged. There are no CLI, guide, SDK, or user-facing configuration changes. ## How you verified The regressions failed before the implementation with the exact routing break: the app test received an opener source without `experimental_hostId`, while the server test rejected it as an unrecognized schema key (2 failed, 56 passed): - `pnpm exec turbo run test --filter=bb-plugin-simple-notes -- app.test.tsx server.test.ts` After the fix: - The same focused command passed 58 tests. The app regression verifies read and autosave RPC identities, and the two-host server regression gives the primary and remote hosts different content, preview URLs, and write results, then proves read, preview, and save all selected `host_remote`. - `pnpm exec turbo run test typecheck build --filter=bb-plugin-simple-notes --force` passed after the final rebase: 65 tests passed; the Docs typecheck and build passed; 7 Turbo tasks succeeded. - `git diff --check origin/main...HEAD` passed. Related to #2005 and #2083. > AGENT GENERATED: by GPT-5.6-Sol
## What was wrong PR #2005 queued normalized external-file intents but rendered one unkeyed `AppFileExternalNavigationDispatcher`. After the first request settled, React reused that component for the next queue item, so its `didSettleRef` remained `true`; the second accepted request neither dispatched nor left the queue, and every later request remained blocked behind it. This is the verified [post-merge review finding](#2005 (comment)). ## What changed - Queue entries now pair each intent with a host-local monotonic request ID. - The lazy dispatcher is keyed by that ID, giving every accepted FIFO item an independent settle lifecycle while preserving the activation-only dynamic import. - The host regression submits two requests in the same event, confirms both were accepted, and verifies exactly-once dispatch in FIFO order. This is intentionally limited to internal React state and identity. It does not change the experimental plugin SDK signature, public plugin API, server/daemon wire data, or host RPC contracts, so no API audit entry or `HOST_DAEMON_PROTOCOL_VERSION` bump is needed. ## How you verified Before the production change, the new focused regression failed with one dispatch: `expected "vi.fn()" to be called 2 times, but got 1 times`. After the fix and a clean rebase onto current `origin/main` (`7f3d2ac66`): - `pnpm exec turbo run test --filter=@bb/app --force -- src/components/plugin/AppFileExternalNavigationHost.test.tsx` — 2 passed. - `pnpm exec turbo run test --filter=@bb/app --force` — 411 files passed; 3,157 tests passed and 3 skipped. - `pnpm exec turbo run typecheck --filter=@bb/app --force` — passed. - `pnpm exec turbo run lint --filter=@bb/app --force` — passed with 0 errors (156 existing warnings). - `pnpm exec turbo run build --filter=@bb/app --force` — passed. - `node apps/app/scripts/check-bundle-budget.mjs` — passed (boot payload 428.7 KB Brotli vs. 467.8 KB budget). > AGENT GENERATED: by GPT-5.6-Sol
## What was wrong PR #2005 added an absolute-host preview query that treated heavy preview work as eager and lightweight: it read the complete file and built a base64 fallback before attempting the preview lease, kept the retained panel query enabled after the panel closed, and used React Query's default five-minute payload retention. A successful media or HTML lease could therefore duplicate host I/O, while a hidden panel could start or continue a large read and keep its payload observed indefinitely. This addresses the connected findings in [the eager payload review](#2005 (comment)), [the hidden-panel review](#2005 (comment)), and [the retention review](#2005 (comment)). ## What changed - Pass the primitive plugin-panel `isOpen` state through the existing lazy host-preview boundary and use it to gate `useHostFilePreview`. - Move a disabled retained observer off the active host/path key so closing the panel aborts the existing SDK request and starts cache GC without unmounting or changing the lazy-loading boundary. - Attempt the existing `sdk.files.createPreview` lease first. Successful image/video leases now return a lightweight URL-backed preview without reading or retaining file bytes. Text and HTML still read the source bytes they render, but no longer build a base64 fallback after a lease succeeds; lease failures build a data URL only for preview kinds that need one. - Apply `HEAVY_PAYLOAD_QUERY_POLICY`, giving inactive host-preview payloads the shared one-minute retention period. - Cover hidden/open/reopen gating, cancellation and GC, lease-first media and fallback behavior, HTML source preservation, and the ambiguous `.ts` source-preview case. This is direct app query/component plumbing over the existing SDK APIs. It adds no server or daemon contract fields, so `HOST_DAEMON_PROTOCOL_VERSION` is unchanged. It also needs no CLI, guide, or public SDK documentation change. ## How you verified - Before the implementation, `pnpm exec turbo run test --filter=@bb/app -- src/hooks/queries/host-file-preview-query.test.tsx src/components/secondary-panel/ThreadSecondaryPanelTabContent.panelGate.test.tsx src/components/plugin/PluginPanelRightPanelHost.test.tsx` reproduced the findings: 5 failed and 13 passed. The failures showed the hidden initial read, missing `isOpen` plumbing, media byte read, base64-before-lease HTML work, and read-before-failed-lease ordering. - After rebasing onto `origin/main` at `7f3d2ac66`, the same focused command passed 3 files / 20 tests. - `pnpm exec turbo run test --filter=@bb/app --force` passed 412 files / 3,162 tests, with 3 skipped. - `pnpm exec turbo run typecheck build --filter=@bb/app` passed. - `pnpm exec turbo run lint --filter=@bb/app` passed with 0 errors and 156 existing warnings outside the changed lines. Post-merge follow-up to #2005; no matching issue. > AGENT GENERATED: by GPT-5.6-Sol
## What was wrong `FileDiffCard` rendered every pull-request file path as a live workspace link whenever the thread had an environment, without considering `file.status`. A removed file normally no longer exists in that workspace, so its filename advertised a preview target guaranteed to fail even though the separate diff control already exposed the deletion. This confirms the [post-merge review finding](#2005 (comment)). ## What changed - Reused the existing non-interactive filename rendering when `file.status` is `removed`, while retaining `experimental_FileLink` for statuses whose path can exist. - Added one focused thread-panel regression covering the complete behavior: removed paths are not links, modified paths remain links, and the removed file's accessible expand/collapse control still reveals its diff. - Kept the fix to the existing render condition; no abstraction, wire change, protocol bump, CLI, guide, or documentation change was needed. ## How you verified - Before the implementation change, `pnpm exec turbo run test --filter=bb-plugin-github --force -- app.test.tsx` failed the new regression with `removed.ts` rendered as `<a href="removed.ts">` (1 failed, 2 passed). - After the change, that focused command passed all 3 tests. - After rebasing onto current `origin/main`, `pnpm exec turbo run test --filter=bb-plugin-github --force` passed 6 files / 25 tests. - `pnpm exec turbo run typecheck --filter=bb-plugin-github` passed. - `pnpm exec turbo run build --filter=bb-plugin-github` passed and built the GitHub plugin's server and app artifacts. - `pnpm exec prettier --check plugins/github/app.test.tsx` and `git diff --check origin/main...HEAD` passed. Fixes: no matching issue. > AGENT GENERATED: by GPT-5.6-Sol
## 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
…still sync (#2144) ## What was wrong Image links in the mobile app failed with a "Couldn't sync tabs" toast. The links and the image routes were fine. A tap opens a `host-file-preview` panel tab, and the app then syncs the tab strip with `GET`/`PUT /threads/:id/tabs`. #2005 added `hostId` to `host-file-preview` tabs with `.default(null)`, so the server now emits `hostId: null` in every tabs response. Installed mobile builds bundle a `.strict()` copy of `threadTabsResponseSchema` that predates the field, so their SDK rejects the whole response (`Unrecognized key: hostId`), the mutation errors, and the tab never settles. Phones cannot update in step with the server, so the server must stay readable by the shipped schema. ## What changed - `apps/server/src/routes/threads/tabs.ts`: the GET and PUT responses omit `hostId` when it is `null`, on `host-file-preview` tabs and on `host-file-preview` file-opener owners of plugin-panel tabs. A non-null `hostId` still travels. Current clients parse the omission back to `null` through the schema default. - `packages/server-contract`: added `ThreadTabsWireResponse` (`z.input` of the response schema) and typed both tabs routes with it. No daemon wire change, so no `HOST_DAEMON_PROTOCOL_VERSION` bump. ## How you verified - New test in `apps/server/test/public/public-thread-tabs.test.ts` that fails before the route change (`expected … to not have property "hostId"`) and passes after it. - `pnpm exec turbo run test --filter=@bb/server -- test/public/public-thread-tabs.test.ts`: 3/3 pass. `@bb/server-contract` tests: 58/58 pass. - `pnpm exec turbo run typecheck` for `@bb/server`, `@bb/sdk`, `@bb/app`, `@bb/mobile`, `@bb/demo-server`: pass. - Manual: paired the iOS simulator with the live `bee.getbb.app` server on a current Metro bundle; storage and host-file image links open the panel tab and the lightbox. Fixes # > AGENT GENERATED: by Claude Opus 5 Co-authored-by: Claude <noreply@anthropic.com>
1 participant
What was wrong
Plugins had no host-owned semantic navigation boundary. URL links bypassed BB's configured URL router, file links depended on ambient routes or plugin-specific code, and callers could not select an owner-scoped fixed tab with a validated transient target. The existing URL, local-file, file-opener, and shared-panel controllers already implemented the product behavior, but the plugin app contract did not expose a common path to them.
What changed
experimental_UrlLinkanduseBbNavigate().experimental_openUrl(url)over the existing URL routing policy.experimental_FileLink,experimental_openFilePreview, andexperimental_openFileExternally. Preview, file-opener, preferred-external-target, and host-resolution behavior reuses the existing controllers; first-party Markdown, Docs, and GitHub consumers now cross the same boundary. Existing APIs remain available through adapters.experimental_useAppPanel().openFixedTab({ surface, tab, target? })controller, owner-definedexperimental_targetvalidation, andexperimental_useFixedTabTarget(tab)transient delivery/consumption. The controller has no Diff/Changes branches. Core Changes file reveal and the GitHub Details tab both use the same owner-scoped path.docs/api_to_audit.md, and the builtinbb-plugin-authoringskill. Link shells remain eager and external file resolution/heavy destinations remain dynamically imported.HOST_DAEMON_PROTOCOL_VERSIONbump were required.How you verified
pnpm exec turbo run typecheckacross@bb/app,@bb/server,@bb/client-core,@get-bb/plugin-sdk,@bb/plugin-build,@bb/templates,@bb/server-contract, GitHub, Docs, and the thread-chat demo: 14/14 tasks passed.SplitWorkspaceRouteclosure 654.1 KB / 655.0 KB budget.Fixes # — no linked issue; implemented from the approved plugin navigation primitives plan.