fix: separate in-page channel events from functions - #371
Conversation
◈ PR Lens
Architecture 
9 components touched across 5 lanes. Inside the changed components — 2 viewsComponent view — In-Page Channel Internals Internal components of the in-page channel bridge separating function calls from events 
Component view — A11y Devframe Channel Integration A11y inspector panel and page script communicating via event declarations 
Data flow 
The other flows — 1 sequence
Drill down
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a breaking wire/protocol change in a core communication layer (version bump + method namespacing), which warrants final human review despite strong test coverage.
Pull request overview
This PR updates devframe/in-page-channel to stop inferring events from void/Promise<void> return types by splitting the protocol into explicit functions and events, and bumps the in-page channel wire version to 2 to reflect the breaking change.
Changes:
- Split
InPageChannelProtocolintofunctionsandevents, and require both endpoint options (functions,events) to be provided explicitly. - Add separate wire namespaces for functions vs events (allowing same-named function/event) and migrate the a11y devframe + docs + diagnostics accordingly.
- Update tests and public API snapshots to cover void actions, explicit event declarations, and same-name collisions.
File summaries
| File | Description |
|---|---|
| tests/snapshots/tsnapi/devframe/in-page-channel.snapshot.d.ts | Updates public type snapshot to reflect the new functions/events protocol and endpoint options. |
| skills/devframe/SKILL.md | Updates internal skill docs to describe the new explicit events model. |
| plugins/a11y/src/shared/protocol.ts | Migrates the a11y in-page contract from function-inferred events to explicit events.pageScript. |
| plugins/a11y/src/client-script/index.ts | Updates the page script endpoint to pass functions and events separately (and removes type: 'event' in function declarations). |
| plugins/a11y/app/lib/channel.ts | Updates the panel endpoint to pass the required (empty) events map. |
| packages/devframe/src/in-page-channel/types.ts | Introduces explicit protocol sections and updates endpoint option typing and channel method signatures accordingly. |
| packages/devframe/src/in-page-channel/types.test-d.ts | Updates d.ts type tests for the new protocol shape and option requirements. |
| packages/devframe/src/in-page-channel/events.test-d.ts | Adds focused d.ts tests ensuring void actions remain callable while events are explicitly emitted/subscribed. |
| packages/devframe/src/in-page-channel/protocol.ts | Bumps IN_PAGE_CHANNEL_VERSION to 2 for the breaking wire change. |
| packages/devframe/src/in-page-channel/panel.ts | Registers incoming events separately and routes call()/emit() through the new method namespacing. |
| packages/devframe/src/in-page-channel/page-script.ts | Registers incoming events separately and routes peer calls / fan-out emits through the new method namespacing. |
| packages/devframe/src/in-page-channel/internal.ts | Adds wire namespacing via channelMethod() and updates the local registry to separate function vs event resolution and returns handling. |
| packages/devframe/src/in-page-channel/in-page-channel.test.ts | Expands runtime tests for explicit events, void actions, timeouts/errors, and same-name collisions. |
| packages/devframe/src/in-page-channel/diagnostics.ts | Updates DF0077 to refer to undeclared events rather than functions. |
| docs/content/8.references/5.browser-api.md | Documents the explicit functions vs events separation in the browser API reference. |
| docs/content/6.errors/index.md | Updates the DF0077 title in the errors index. |
| docs/content/6.errors/DF0077.md | Updates DF0077 docs and examples to match the new events option model. |
| docs/content/1.guide/12.in-page-channel.md | Migrates the in-page channel guide to the new protocol shape and API semantics. |
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| name: MY_CHANNEL, | ||
| serialize: value => toRawDeep(value), // applied to every outgoing argument and result | ||
| functions: {}, | ||
| events: { flash: {} }, |
There was a problem hiding this comment.
I wonder if this should just be optional
There was a problem hiding this comment.
right now used to diagnose missing events when emitted, so I feel like it can be useful to diagnose wrong names https://github.com/devframes/devframe/pull/371/changes#diff-c8bbf4963ef4b0c4ae6cc7164bfa1da31233ea66fdb9cda6b4b1c98568f62aa5R7
There was a problem hiding this comment.
I ended up making it optional, i feel like it makes more sense for events and they are already type safe (although we do accept any string with & string)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
||
| export function channelMethod(kind: 'function' | 'event', name: string): string { | ||
| // Keep user functions, user events, and internal methods in separate wire namespaces. | ||
| return `devframe:in-page:${kind}:${name}` |
There was a problem hiding this comment.
worth noting this changes how they are saved in the registry, it's more verbose but should be fine. It allows having an action and an event with the same name even thought I don't think people should do that
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a breaking API and wire-protocol change that should be validated by a human across downstream consumers and upgrade paths.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
| call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>, | ||
| emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)), | ||
| callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)), |
There was a problem hiding this comment.
🟡 Changes recommended
The endpoint option typing for events in packages/devframe/src/in-page-channel/types.ts is inconsistent with the documented/intentional “complete events map” contract and should be aligned to avoid confusing or unsafe consumer behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The TypeScript surface currently has an API contract inconsistency (events optional in types.ts vs required in snapshots/docs) plus a broken DF0077 doc example that references an undefined channel.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
packages/devframe/src/in-page-channel/types.ts:258
eventsis currently optional here (and its entries are optional), but the public API snapshot and docs treat{ name, functions, events }as required for both endpoints to preserve a completeness check. Making it optional lets callers omit event declarations entirely, which contradicts the documented protocol shape and the generatedtsnapisnapshot fordevframe/in-page-channel.
export interface CreatePageScriptChannelOptions<Protocol extends InPageChannelProtocol = InPageChannelProtocol> extends InPageChannelCommonOptions {
/** Every page-script function declaration, with a required handler. */
functions: CreatePageScriptChannelOptionsFunctions<Protocol>
/** Optional metadata or handlers for incoming events. Listeners may instead subscribe through `channel.on()`. */
events?: { [NAME in keyof PageScriptProtocolEvents<Protocol> & string]?: InPageEventOption<PageScriptProtocolEvents<Protocol>[NAME]> }
/**
packages/devframe/src/in-page-channel/types.ts:272
- Same as
CreatePageScriptChannelOptions:eventsshould be required (with required keys) to match the documented v2 protocol shape and the generated API snapshot. Leaving it optional makes it easy to accidentally omit event declarations and loseargs/jsonSerializablevalidation for inbound events.
/** Options for {@link connectPanelChannel}. */
export interface ConnectPanelChannelOptions<Protocol extends InPageChannelProtocol = InPageChannelProtocol> extends InPageChannelCommonOptions {
/** Every panel function declaration, with a required handler. */
functions: ConnectPanelChannelOptionsFunctions<Protocol>
/** Optional metadata or handlers for incoming events. Listeners may instead subscribe through `channel.on()`. */
events?: { [NAME in keyof PanelProtocolEvents<Protocol> & string]?: InPageEventOption<PanelProtocolEvents<Protocol>[NAME]> }
/**
packages/devframe/src/in-page-channel/panel.ts:291
PanelChannel.call()now passes a wire-prefixed method name (devframe:in-page:function:<name>) intoenqueueCall(). SinceenqueueCall()uses itsmethodparameter directly in user-facing error/timeout messages, these messages will now include the internal wire prefix (e.g.call "devframe:in-page:function:save" timed out ...) instead of the logical function name (save). Consider stripping thechannelMethod('function', '')prefix when formatting errors/timeouts (or threading a separate display name) so diagnostics remain readable.
call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>,
emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
- Files reviewed: 16/17 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed user-facing documentation inaccuracies about events being required (it’s typed optional) and a user-facing error-message regression where prefixed wire method names can leak into panel call timeout/closed errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/devframe/src/in-page-channel/panel.ts:291
PanelChannel.call()now passes a wire-prefixed method name (devframe:in-page:function:<name>) intoenqueueCall(), butenqueueCall()uses thatmethodstring in its user-facing error messages (closed/timeout). This will surface internal wire prefixes in errors likecall "devframe:in-page:function:save" ..., which is much harder to read and inconsistent with the page-script peer call path (which reports the bare function name). Consider keeping a separate display name for errors/logging (barefnName) while still calling over the wire with the namespaced method.
call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>,
emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
There are user-facing documentation/API wording inconsistencies (re: whether events is required) and a likely user-visible error-message regression on panel.call() due to passing wire-prefixed method names into existing error formatting.
Review details
Suppressed comments (3)
docs/content/8.references/5.browser-api.md:53
- The docs say endpoint options require a complete
eventsmap, but in the actual APIeventsis optional (CreatePageScriptChannelOptions.events?/ConnectPanelChannelOptions.events?). This is inconsistent with the guide and the exported types, and may mislead users into thinking they must always provideevents: {}.
`InPageChannelProtocol` separates `functions` and `events`. Each section has optional `pageScript` and `panel` maps naming the receiving direction. Endpoint options require a complete `functions` map with handlers; `events` is optional, and when provided can include optional handlers (use `{}` to declare an event without a handler for `channel.on()`). `call()` uses function names regardless of return type, while `emit()`, `callEvent()` (deprecated), and `on()` use event names. A function returning `void` or `Promise<void>` remains an awaitable request/response call.
packages/devframe/src/in-page-channel/panel.ts:291
panel.call()now prefixes the method withdevframe:in-page:function:. This value is also used inenqueueCall()error strings (closed/timeout), so user-facing errors will include the wire method name instead of the original function name (e.g.call "devframe:in-page:function:measure" timed out…). Consider keeping the wire method separate from the display name used in error messages.
call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>,
emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
skills/devframe/SKILL.md:441
- This guide text claims
createPageScriptChannel()/connectPanelChannel()require{ name, functions, events }, buteventsis optional in the exported types. The wording should match the actual API shape to avoid confusion.
For a live inspect-the-page loop, `devframe/in-page-channel` connects a devframe's **page script** (in the user app's page) to its **panels** entirely in the browser, in both dev and static builds. Declare a shared protocol type with separate `functions` and `events` sections, each with `pageScript` and `panel` maps naming the receiving direction. Both `createPageScriptChannel<P>()` and `connectPanelChannel<P>()` require `{ name, functions }` and accept optional `events`: function declarations require handlers; event declarations accept optional handlers or `{}` for dynamic `channel.on()` subscriptions. `call()` awaits functions, including void actions; `emit()` sends declared events. `channel.sharedState.get(key)` mirrors `rpc.sharedState` with the page script as authority and automatic replay to panels. The handshake retries across boot order and reloads; panels expose `status`/`whenConnected(ms)` for page-script availability fallbacks. Channel names follow `devframes:plugin:<slug>`. The a11y inspector's scan/highlight loop is the reference use.
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are at least two user-facing issues to address (a guide example that won’t type-check with the new protocol and a panel-side call error/timeout message regression that now includes wire-prefixed method names).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/devframe/src/in-page-channel/panel.ts:291
call()now passes a namespaced wire method (devframe:in-page:function:<name>) intoenqueueCall(). BecauseenqueueCall()formats user-facing errors/timeouts using thatmethodstring, messages will now include the wire prefix instead of the function name (e.g.call "devframe:in-page:function:save" ...). Consider carrying both a wire method and a display name so errors stay readable.
call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>,
emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
- Files reviewed: 16/17 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A few user-facing docs and messages are now slightly inconsistent with the runtime behavior (notably event subscription without declarations and panel call error text), and should be corrected before approval.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
docs/content/1.guide/12.in-page-channel.md:69
- This guide text states that
{}“registers an event for runtime subscriptions throughon()”, but runtime subscriptions also work without anyeventsdeclarations. Please adjust wording to indicateeventsis optional and primarily enables validation/metadata for incoming events, whileon()can still be used for undeclared events (without validation).
docs/content/8.references/5.browser-api.md:54 - Docs imply that
{}in theeventsoption is required to usechannel.on(), but the implementation intentionally allows subscribing to undeclared events (see in-page-channel.test.ts: "accepts listeners without runtime event declarations"). Please clarify thateventsdeclarations are optional and mainly provide metadata (e.g.argsvalidation /jsonSerializableenforcement) for incoming events.
skills/devframe/SKILL.md:441 - This skill text says event declarations use
{}for dynamicchannel.on()subscriptions, but the runtime also allowschannel.on()without anyeventsdeclarations (at the cost of losing validation/metadata). Please tweak wording so it matches the actual API behavior.
packages/devframe/src/in-page-channel/panel.ts:291
connectPanelChannel().call()now passes a wire-namespaced method (e.g.devframe:in-page:function:echo) intoenqueueCall(), which uses that string in user-facing timeout/closed error messages. This makes errors harder to read and leaks wire details; callers should continue to see the original function name (e.g.echo). Consider updatingenqueueCall()to accept both a display name and a wire method, or keepenqueueCall()taking the display name and applychannelMethod('function', ...)only at the RPC boundary.
call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>,
emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a breaking wire-protocol change in a core communication path and should receive final human review for compatibility and UX details (e.g., error surfaces) before approval.
Review details
Suppressed comments (1)
packages/devframe/src/in-page-channel/panel.ts:291
call()now forwards a wire-namespaced method (viachannelMethod('function', fnName)) intoenqueueCall(), butenqueueCall()formats user-facing timeout/closed errors using thatmethodstring. This will leak internal wire names likedevframe:in-page:function:saveinto errors instead of the original function name, and makes panel-side messages inconsistent with the page-script peer call path (which still reports the plain name).
call: (fnName, ...args) => enqueueCall(channelMethod('function', fnName), serializeArgs(codec, args)) as Promise<any>,
emit: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
callEvent: (fnName, ...args) => sendEvent(channelMethod('event', fnName), serializeArgs(codec, args)),
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Separate in-page channel events from functions so actions that return
voidcan still be awaited. Update a11y, docs, and tests for the new protocol shape.Follow-up to #358. This changes the API and wire protocol.