Return boolean from every plugin SDK openPanel - #1848
Conversation
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request now. I will check security, code quality, architecture, performance, and the main user flow. I will post one final review after these checks finish. |
|
|
||
| export { Markdown, ThreadChat, definePluginApp, experimental_NewThreadComposer, experimental_useSidebarThreadActions, experimental_useSidebarThreadPullRequest, experimental_useSidebarThreadSplit, experimental_useSidebarThreads, useBbContext, useBbNavigate, useComposer, useComposerView, useRealtime, useRealtimeConnectionState, useRpc, useSettings }; | ||
| export type { BbContext, BbNavigate, ComposerCustomization, ComposerPlusMenuItem, ComposerRichTextSpec, ComposerStructuredDraft, ComposerView, JsonValue, MarkdownProps, NewThreadComposerProps, NewThreadRequest, PluginAppBuilder, PluginAppComposer, PluginAppContentScripts, PluginAppDefinition, PluginAppSetup, PluginAppSlots, PluginComposerApi, PluginComposerMention, PluginComposerScope, PluginComposerTextEffect, PluginComposerThreadRowStatus, PluginContentScriptContext, PluginContentScriptDisposer, PluginContentScriptRegistration, PluginFileOpenerProps, PluginFileOpenerRegistration, PluginFileOpenerSource, PluginHomepageSectionProps, PluginHomepageSectionRegistration, PluginMessageActionContext, PluginMessageActionRegistration, PluginMessageActionThreadPanelOptions, PluginMessageDirectiveMessage, PluginMessageDirectiveOpenWorkspaceFile, PluginMessageDirectiveProps, PluginMessageDirectiveRegistration, PluginNavPanelProps, PluginNavPanelRegistration, PluginNewThreadPanelActionContext, PluginNewThreadPanelActionRegistration, PluginNewThreadPanelProps, PluginPendingInteractionProps, PluginPendingInteractionRegistration, PluginPendingInteractionView, PluginProviderIconRegistration, PluginRealtimeConnectionState, PluginRpcCallArgs, PluginRpcClient, PluginRpcContract, PluginRpcError, PluginRpcErrorCode, PluginRpcHandlers, PluginRpcIssuePathSegment, PluginRpcMethodContract, PluginRpcResult, PluginRpcValidationIssue, PluginSdkApp, PluginSettingsSectionProps, PluginSettingsSectionRegistration, PluginSettingsState, PluginSidebarFooterActionContext, PluginSidebarFooterActionProps, PluginSidebarFooterActionRegistration, PluginSidebarProject, PluginSidebarPullRequest, PluginSidebarSplitPane, PluginSidebarThread, PluginSidebarThreadActions, PluginSidebarThreadActivity, PluginSidebarThreadIndicator, PluginSidebarThreadPullRequestState, PluginSidebarThreadSplit, PluginSidebarThreadsState, PluginSidebarWorkspaceKind, PluginThreadHeaderActionProps, PluginThreadHeaderActionRegistration, PluginThreadListProps, PluginThreadListRegistration, PluginThreadPanelActionContext, PluginThreadPanelActionRegistration, PluginThreadPanelProps, StandardSchemaV1, StandardSchemaV1InferInput, StandardSchemaV1InferOutput, StandardSchemaV1Issue, StandardSchemaV1Result, ThreadChatMessageAction, ThreadChatMessageReference, ThreadChatProps }; | ||
| export type { BbContext, BbNavigate, ComposerCustomization, ComposerPlusMenuItem, ComposerRichTextSpec, ComposerStructuredDraft, ComposerView, JsonValue, MarkdownProps, NewThreadComposerProps, NewThreadRequest, PluginAppBuilder, PluginAppComposer, PluginAppContentScripts, PluginAppDefinition, PluginAppSetup, PluginAppSlots, PluginComposerApi, PluginComposerMention, PluginComposerScope, PluginComposerTextEffect, PluginComposerThreadRowStatus, PluginContentScriptContext, PluginContentScriptDisposer, PluginContentScriptRegistration, PluginFileOpenerProps, PluginFileOpenerRegistration, PluginFileOpenerSource, PluginHomepageSectionProps, PluginHomepageSectionRegistration, PluginMessageActionContext, PluginMessageActionRegistration, PluginMessageActionThreadPanelOptions, PluginMessageDirectiveMessage, PluginMessageDirectiveOpenWorkspaceFile, PluginMessageDirectiveProps, PluginMessageDirectiveRegistration, PluginNavPanelProps, PluginNavPanelRegistration, PluginNewThreadPanelActionContext, PluginNewThreadPanelActionRegistration, PluginNewThreadPanelProps, PluginPanelActionOpenOptions, PluginPendingInteractionProps, PluginPendingInteractionRegistration, PluginPendingInteractionView, PluginProviderIconRegistration, PluginRealtimeConnectionState, PluginRpcCallArgs, PluginRpcClient, PluginRpcContract, PluginRpcError, PluginRpcErrorCode, PluginRpcHandlers, PluginRpcIssuePathSegment, PluginRpcMethodContract, PluginRpcResult, PluginRpcValidationIssue, PluginSdkApp, PluginSettingsSectionProps, PluginSettingsSectionRegistration, PluginSettingsState, PluginSidebarFooterActionContext, PluginSidebarFooterActionProps, PluginSidebarFooterActionRegistration, PluginSidebarProject, PluginSidebarPullRequest, PluginSidebarSplitPane, PluginSidebarThread, PluginSidebarThreadActions, PluginSidebarThreadActivity, PluginSidebarThreadIndicator, PluginSidebarThreadPullRequestState, PluginSidebarThreadSplit, PluginSidebarThreadsState, PluginSidebarWorkspaceKind, PluginThreadHeaderActionProps, PluginThreadHeaderActionRegistration, PluginThreadListProps, PluginThreadListRegistration, PluginThreadPanelActionContext, PluginThreadPanelActionRegistration, PluginThreadPanelProps, StandardSchemaV1, StandardSchemaV1InferInput, StandardSchemaV1InferOutput, StandardSchemaV1Issue, StandardSchemaV1Result, ThreadChatMessageAction, ThreadChatMessageReference, ThreadChatProps }; |
There was a problem hiding this comment.
🚨 slopcop/review — The bundled declarations changed, but the scaffold copy did not change. The @bb/templates type check fails, and legacy plugin type updates still send the old void contract. Run the template generator and commit its output.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This change lets a plugin know whether BB opened its requested panel. It also adds an error message for a failed side-chat open.
I found two blockers:
- High: The generated scaffold declarations still use the old
voidreturn type. The@bb/templatestype check fails. - High:
PluginPanelActionOpenOptionsadds a stable public SDK export without the requiredexperimental_prefix and audit entry.
I also found two design concerns:
- The stated side-chat failure path is not reachable. The host disables global message actions inside plugin-hosted chats.
- Two host paths repeat the same validation, serialization, warning, open, and Boolean-return logic. A shared helper can prevent later drift.
Security review found no privilege or input-boundary problem. Performance review found no hot-path regression.
The live browser check passed the normal flow. It created a thread, opened a side chat, sent a reply, and received the reply.
The focused app, SDK, and side-chat tests passed. git diff --check passed. The SDK version guard passed because version 0.4.8 is not published.
The @bb/templates type check failed because the generated SDK template is stale. I posted this review as a comment only, as requested.
| }); | ||
| if (!opened) { | ||
| // The host declines when the invoking surface has no side panel to open | ||
| // into — "Reply in side chat" also renders inside a side chat's own |
There was a problem hiding this comment.
🚨 slopcop/review — The stated product path does not occur. PluginThreadChat disables global message actions, so the side-chat panel does not show this action. The browser test confirmed this behavior. This direct mock test does not prove a reachable decline path.
| * `PluginMessageActionThreadPanelOptions` adds the `actionId` that a | ||
| * message action needs to name its target panel. | ||
| */ | ||
| export interface PluginPanelActionOpenOptions { |
There was a problem hiding this comment.
🚨 slopcop/review — This new public SDK type does not follow the required experimental-name rule. Keep the helper private, or add an experimental_ name and a separate audit entry.
The three registration-callback openPanel entry points disagree on how the host reports a declined open: messageAction returns boolean, the two panel actions return void, and invalid params throws on one path and returns false on the other. side-chat works around it with an `unknown` return type and discards the boolean, which strands a hidden fork thread when a message action runs on a surface with no side panel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three registration-callback openPanel entry points disagreed on how the host reported a declined open: messageAction returned boolean while the two panel-action contexts returned void, so a plugin registering more than one kind of action had no uniform way to tell whether a panel opened. Behind the type difference the two host implementations also disagreed on invalid `params` — the message-action path caught, warned, and returned false, while the panel-action path let serializePluginPanelParams throw out of openPanel. Widen PluginThreadPanelActionContext.openPanel and PluginNewThreadPanelActionContext.openPanel to boolean and give all three a shared PluginPanelActionOpenOptions. Both panel-action openPanel closures now run through one helper that catches a bad `params`, warns, and reports false rather than throwing; the messageAction no-side-panel branch, which already returned false silently, now warns too, so all three decline paths are diagnosable from the console. Bundled .d.ts regenerated via scripts/build-bundled-dts.mjs (it also reorders some unrelated inlined zod enum members). Widening the return is source-compatible for callers that ignore it, but not for a concise-arrow `run` that returns openPanel(...): `run` is declared `void | Promise<void>`, so the body now needs braces. Three in-repo call sites and both bb-plugin-authoring skill examples are fixed accordingly, and the plan records the open question of whether `run`'s return type should widen too. Left plugins/side-chat/ alone: a separate change makes its fork lazy. Tests: plugin-slot-mounts covers an accepted open reporting true from both panel action kinds, and a non-JSON params open reporting false without opening; ThreadTimelineRows.actions already pinned messageAction returning false on a surface with no thread panel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Reply in side chat" is a messageAction, so it renders on every ThreadChat timeline — including the one side chat itself embeds in its panel, which has no onOpenPluginPanel and therefore declines the open. The plugin typed its injected openPanel as returning `unknown` (to bridge the message-action and panel-action signatures, before they were unified) and discarded the result, so that path created the hidden fork over RPC and then silently did nothing: no panel, no feedback, an idle fork nobody asked for. Now that every SDK openPanel returns boolean, narrow the local type to boolean and toast when the host declines. The fork is left to the server's existing hourly empty-fork sweep rather than discarded through a new RPC, and openability is not checked before the RPC. Neither alternative works: a plugin cannot ask whether a surface has a side panel without attempting an open, since the boolean is the only signal; and a discard RPC would duplicate sweep policy that already covers this exact case (the fork is created idle, so it is precisely the empty never-replied-to fork that sweep archives). Tests: a declined open still runs the fork RPC once and now surfaces the toast; verified failing with the check removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`useBbNavigate().openThreadPanel` inlined `{ actionId, title?, params? }`
while `messageAction`'s `openPanel` took a named type of exactly that
shape. Both are the same operation — open a panel action you are not,
named by id — so they now share it.
Rename it to `PluginTargetedPanelActionOpenOptions`: the old
`PluginMessageActionThreadPanelOptions` described one of its two callers.
The pairing is now explicit in the contract — a panel action opening its
own tab passes the bare `PluginPanelActionOpenOptions`; anything else
passes the targeted variant that adds `actionId`.
Also regenerate `@bb/templates`'s embedded copy of the SDK `.d.ts`, which
the earlier commits missed. Its `generate-templates.mjs --check` runs in
that package's typecheck and test, and was failing on this branch while
passing on main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The declined-open path this branch added handling for cannot be reached.
`PluginThreadChat` passes `includePluginMessageActions={false}` on both
render paths, and `ThreadTimelineRows` swaps in an empty slot snapshot
when that is false — so "Reply in side chat" never renders inside a side
chat's own transcript. Every surface that does render plugin message
actions supplies `onOpenPluginPanel`, so `openPanel` has no reachable
decline for this plugin to observe.
Keep the `unknown` -> `boolean` narrowing, which follows from the unified
contract and removes the cast that existed only to bridge the two old
signatures, and record why nothing reads the result. Drop the toast and
its test rather than ship a guard for a path the UI cannot produce.
Reported by slopcop on #1848.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a110d12 to
51d4ca7
Compare
What was wrong
The plugin SDK had four ways to ask the host to open a panel, and they disagreed on how to report failure.
PluginMessageActionContext.openPanelanduseBbNavigate().openThreadPanelreturnedboolean;PluginThreadPanelActionContext.openPanelandPluginNewThreadPanelActionContext.openPanelreturnedvoid. Invalidparamscompounded it — that threw out ofopenPanelon the panel-action path but returnedfalseon the message-action path, so the same mistake surfaced two different ways.Those four are really two operations, each existing twice for surface reasons: open the panel I already am (no
actionId— you are inside a panel action'srun), available on the thread panel and the New thread screen; and open a panel I am not (actionIdrequired), available as a callback context and as a hook. Nothing about that split required the return types to diverge — they diverged because each was declared independently.The inconsistency also leaked into plugin code.
plugins/side-chat/app.tsxshares one helper across two of those entry points, so it typed the injectedopenPanelas returningunknown— the only type that fits both a boolean and nothing — discarding a result it had no way to describe.This PR does not fix a user-facing bug. An earlier revision added handling for a declined open in side chat; slopcop correctly pointed out that path is unreachable, and it has been removed.
PluginThreadChatpassesincludePluginMessageActions={false}, andThreadTimelineRowsswaps in an empty slot snapshot when that is false, so a pluginmessageActionnever renders inside a plugin's ownThreadChat. Every surface that does render them suppliesonOpenPluginPanel. This is a contract cleanup.What changed
packages/plugin-sdk/src/app-contract.ts— both panel-actionopenPanels now returnboolean, matching the other two. The doc comments state one rule:trueaccepted,falsedeclined, never a throw. The two operations now share types that name them:PluginPanelActionOpenOptions({ title?, params? }) for a panel action opening its own tab, andPluginTargetedPanelActionOpenOptions, which extends it withactionId, for a caller that is not itself a panel action.useBbNavigate().openThreadPanelpreviously inlined a shape identical to the latter and now uses it.apps/app/src/components/plugin/PluginPanelActions.tsx— both panel-action closures go through onecreatePanelActionOpenPanelhelper that catches non-JSONparams, warns, and returnsfalserather than throwing intorun, where the host swallows it.apps/app/src/lib/plugin-message-actions.ts— the no-side-panel branch returnedfalsesilently and now warns, so all three decline paths are diagnosable from the console.plugins/side-chat/app.tsx— the injectedopenPanelis typedbooleaninstead ofunknown, removing the cast that existed only to bridge the two old signatures. Nothing reads the result, and a comment records why.Generated files were regenerated by script, not hand-edited:
bundled-types/viascripts/build-bundled-dts.mjs, and@bb/templates's embedded copy of the SDK.d.tsviagenerate-templates.mjs. That second one matters — its--checkruns inside@bb/templates'stypecheckandtest, and would otherwise fail CI. Docs updated inpackages/plugin-sdk/README.md,docs/api_to_audit.md, and thebb-plugin-authoringskill. No wire changes, soHOST_DAEMON_PROTOCOL_VERSIONis unchanged.What this deliberately does not do is reduce the count or unify the names —
openPanelandopenThreadPanelremain two names for one idea. That is a rename across the public API and deserves its own decision.Breaking changes for plugin authors
1.
runis declaredvoid | Promise<void>, andbooleanis not assignable to that union — TypeScript's void-return exemption applies only to a barevoid. So the concise formrun: ({ openPanel }) => openPanel({ ... })no longer typechecks and needs braces:Three in-repo call sites and both examples in the authoring skill are fixed. We deliberately did not widen
run's return type to absorb this: pre-1.0, a loud one-line compile error is an acceptable cost for keeping the type honest.2.
PluginMessageActionThreadPanelOptionsis renamedPluginTargetedPanelActionOpenOptions, since it now describes both of its callers rather than one.How you verified
truefrom both panel-action kinds (plugin-slot-mounts.test.tsx).paramstest now assertsfalseis returned rather than thrown.ThreadTimelineRows.actions.test.tsx:1423pinsmessageAction.openPanel→falseon a surface with no thread panel.pnpm exec turbo run typecheck test lint --filter=@get-bb/plugin-sdk --filter=@bb/templates --filter=bb-plugin-side-chat --filter=@bb/app— 3,041 tests passed, typecheck and lint clean.generate-templates.mjs --checkverified failing on this branch before the regeneration and passing onmain, confirming the staleness was introduced here rather than pre-existing.Rebased on
mainafter #1851, which made the declaration bundles deterministic; the generated-file conflicts were resolved by regenerating from source rather than by hand.No issue was filed for this; it was found while reviewing #1457.