[Proposal] trigger client side panel event from python #6304
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds a client-side per-panel event system: a React hook for registering/triggering panel callbacks, a built-in operator to invoke those hooks, public re-exports, and Python APIs to request client-side panel event triggers via the operator execution path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python: PanelRef
participant Ops as Python: Operations
participant Ctx as ExecContext
participant FE as Frontend: Built-in Operator
participant Hook as Frontend: usePanelClientEvent
participant Panel as Client Panel Callback
Py->>Ops: trigger(event, params)
Ops->>Ctx: execute("trigger_panel_client_event", {panel_id, event, params})
Ctx->>FE: run TriggerPanelClientEvent with payload
FE->>Hook: call trigger(event, params, panel_id)
alt callback registered
Hook->>Panel: invoke callback(params)
else no callback
Note right of Hook: no-op
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
imanjra
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
fiftyone/operators/operations.py (1)
706-717: Add ctx fallback when id is None to match other panel opsBrings parity with
_create_panel_paramsbehavior and avoids forcing callers to always passid.def trigger_panel_client_event(self, id, event, params=None): """Triggers a panel client-side event. @@ - return self._ctx.trigger( + # allow using ctx.panel_id if id is None (parity with other panel ops) + if id is None: + id = self._ctx.params.get("panel_id", None) + return self._ctx.trigger( "trigger_panel_client_event", {"panel_id": id, "event": event, "params": params}, )fiftyone/operators/panel.py (1)
375-384: Validate event name before triggeringGuard against falsy/empty event names to prevent silent no-ops.
def trigger(self, event, params=None): """Triggers a client-side event of a panel. @@ - self._ctx.ops.trigger_panel_client_event( + if not event or not isinstance(event, str): + raise ValueError("event must be a non-empty string") + self._ctx.ops.trigger_panel_client_event( id=self.id, event=event, params=params )app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx (2)
5-8: Align function name with file/export for clarityRename the default-exported hook to
usePanelClientEventto match filename and public API, improving discoverability and stack traces.-export default function usePanelEvents(panelId?: string) { +export default function usePanelClientEvent(panelId?: string) { const id = usePanelId(); const computedPanelId = panelId ?? id;
21-27: Add guard and return delivery status from trigger()Helps callers detect missed deliveries and avoids
"undefined"bucket usage.- function trigger(event: string, params: unknown, panelId?: string) { + function trigger(event: string, params?: unknown, panelId?: string): boolean { const triggerPanelId = panelId ?? computedPanelId; + if (!triggerPanelId) { + if (process.env.NODE_ENV !== "production") { + console.warn("usePanelClientEvent.trigger called without panelId in a non-panel context"); + } + return false; + } const callback = events[triggerPanelId]?.[event]; if (callback) { callback(params); - } + return true; + } + return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/packages/operators/src/Panel/hooks/index.ts(1 hunks)app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx(1 hunks)app/packages/operators/src/built-in-operators.ts(3 hunks)app/packages/operators/src/index.ts(1 hunks)fiftyone/operators/operations.py(1 hunks)fiftyone/operators/panel.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/operators/src/index.tsapp/packages/operators/src/Panel/hooks/index.tsapp/packages/operators/src/built-in-operators.tsapp/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: imanjra
PR: voxel51/fiftyone#4373
File: app/packages/operators/src/built-in-operators.ts:983-983
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The `registerPanel` function in `app/packages/operators/src/Panel/register.tsx` extracts parameters from the `ctx` object internally and does not require explicit parameter passing when called.
Learnt from: imanjra
PR: voxel51/fiftyone#4373
File: app/packages/operators/src/built-in-operators.ts:983-983
Timestamp: 2024-10-08T15:36:21.356Z
Learning: The `registerPanel` function in `app/packages/operators/src/Panel/register.tsx` extracts parameters from the `ctx` object internally and does not require explicit parameter passing when called.
📚 Learning: 2024-10-08T15:36:21.356Z
Learnt from: imanjra
PR: voxel51/fiftyone#4373
File: app/packages/operators/src/built-in-operators.ts:983-983
Timestamp: 2024-10-08T15:36:21.356Z
Learning: The `registerPanel` function in `app/packages/operators/src/Panel/register.tsx` extracts parameters from the `ctx` object internally and does not require explicit parameter passing when called.
Applied to files:
app/packages/operators/src/index.tsapp/packages/operators/src/Panel/hooks/index.tsapp/packages/operators/src/built-in-operators.tsapp/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx
📚 Learning: 2025-04-23T15:22:03.452Z
Learnt from: benjaminpkane
PR: voxel51/fiftyone#5732
File: app/packages/core/src/components/Filters/use-query-performance-icon.tsx:26-38
Timestamp: 2025-04-23T15:22:03.452Z
Learning: React hooks (including useRecoilValue and other state management hooks) must be called unconditionally at the top level of a component or custom hook. They cannot be placed inside conditional statements, loops, or nested functions, as this violates React's Rules of Hooks and leads to unpredictable behavior.
Applied to files:
app/packages/operators/src/built-in-operators.ts
🧬 Code graph analysis (4)
fiftyone/operators/operations.py (1)
fiftyone/operators/panel.py (2)
id(325-327)trigger(375-384)
fiftyone/operators/panel.py (3)
fiftyone/operators/executor.py (3)
trigger(93-108)trigger(937-965)ops(828-832)app/packages/operators/src/operators.ts (2)
trigger(63-66)trigger(165-172)fiftyone/operators/operations.py (1)
trigger_panel_client_event(706-717)
app/packages/operators/src/built-in-operators.ts (2)
app/packages/operators/src/operators.ts (6)
Operator(313-402)OperatorConfig(253-311)trigger(63-66)trigger(165-172)ExecutionContext(104-187)_registerBuiltInOperator(429-432)fiftyone/operators/panel.py (1)
trigger(375-384)
app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx (2)
fiftyone/operators/panel.py (2)
id(325-327)trigger(375-384)app/packages/spaces/src/hooks.ts (1)
usePanelId(283-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (5)
app/packages/operators/src/index.ts (1)
32-32: Re-export looks good; verify no name collisions with existing hooks
usePanelEvent(singular) already exists; addingusePanelClientEventis fine. Ensure docs/examples distinguish both to avoid confusion.app/packages/operators/src/Panel/hooks/index.ts (1)
1-1: LGTMPublic re-export matches the new hook. Keep the symbol name consistent with the implementation (see rename suggestion).
app/packages/operators/src/built-in-operators.ts (3)
1584-1584: Registration looks goodOperator is properly registered in
registerBuiltInOperators().
26-26: No action needed—exports and dependency graph are correct
usePanelClientEventis re-exported fromapp/packages/operators/src/index.tsviaexport * from "./Panel/hooks", andindex.tsdoes not importbuilt-in-operators, so there’s no circular dependency.
1504-1525: Fixidvspanel_idmismatch, mark unlisted, add validation & fallback
Python code invokestrigger_panel_client_event(id=…)but the current hook only readspanel_id, dropping the panel target.
- In
get config(), addunlisted: true- In
useHooks(ctx), accept bothpanel_idandid, validate thateventis a non-empty string, then compute
const targetId = p.panel_id ?? p.id ?? ctx.getCurrentPanelId();- Rename the returned method to
triggerEventand updateexecute()to callctx.hooks.triggerEvent(ctx.params)
Ensure thatExecutionContext.getCurrentPanelId()exists or supply an alternative fallback.
imanjra
force-pushed
the
feat/js-event-from-py
branch
from
5cf3098 to
7c71670
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx (2)
31-37: Past feedback addressed: unregister now existsThe unregister API addition resolves earlier leak/stale-handler concerns.
31-37: Remove empty panel buckets to prevent unbounded growthClean up panel map when the last event is removed.
function unregister(event: string, panelId?: string) { const unregisterPanelId = panelId ?? computedPanelId; assertPanelId(unregisterPanelId); if (events[unregisterPanelId]) { delete events[unregisterPanelId][event]; + if (!Object.keys(events[unregisterPanelId]).length) { + delete events[unregisterPanelId]; + } } }
🧹 Nitpick comments (3)
app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx (3)
3-3: Harden maps against prototype collisionsUse Object.create(null) for maps to avoid accidental prototype key collisions. Optional if you adopt Map<K,V> later.
-const events: PanelsEvents = {}; +const events: PanelsEvents = Object.create(null);- if (!events[registerPanelId]) { - events[registerPanelId] = {}; - } + if (!events[registerPanelId]) { + events[registerPanelId] = Object.create(null); + }Also applies to: 16-18
5-5: Align exported name with filename for clarityMinor: rename the function to match the file/export name.
-export default function usePanelEvents(panelId?: string) { +export default function usePanelClientEvent(panelId?: string) {
42-45: Improve error message for developer ergonomicsClarify remediation steps when panelId is missing.
- if (!panelId) { - throw new Error("Panel ID is required"); - } + if (!panelId) { + throw new Error( + "usePanelClientEvent: panelId is required (provide panelId or call within a Panel context)" + ); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/packages/operators/src/Panel/hooks/index.ts(1 hunks)app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx(1 hunks)app/packages/operators/src/built-in-operators.ts(3 hunks)app/packages/operators/src/index.ts(1 hunks)fiftyone/operators/operations.py(1 hunks)fiftyone/operators/panel.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/operators/src/Panel/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- fiftyone/operators/operations.py
- app/packages/operators/src/index.ts
- fiftyone/operators/panel.py
- app/packages/operators/src/built-in-operators.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: imanjra
PR: voxel51/fiftyone#4373
File: app/packages/operators/src/built-in-operators.ts:983-983
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The `registerPanel` function in `app/packages/operators/src/Panel/register.tsx` extracts parameters from the `ctx` object internally and does not require explicit parameter passing when called.
Learnt from: imanjra
PR: voxel51/fiftyone#4373
File: app/packages/operators/src/built-in-operators.ts:983-983
Timestamp: 2024-10-08T15:36:21.356Z
Learning: The `registerPanel` function in `app/packages/operators/src/Panel/register.tsx` extracts parameters from the `ctx` object internally and does not require explicit parameter passing when called.
📚 Learning: 2024-07-27T04:14:08.421Z
Learnt from: imanjra
PR: voxel51/fiftyone#4373
File: app/packages/operators/src/built-in-operators.ts:983-983
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The `registerPanel` function in `app/packages/operators/src/Panel/register.tsx` extracts parameters from the `ctx` object internally and does not require explicit parameter passing when called.
Applied to files:
app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx
🧬 Code graph analysis (1)
app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx (2)
fiftyone/operators/panel.py (2)
id(325-327)trigger(375-384)app/packages/spaces/src/hooks.ts (1)
usePanelId(283-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
Comment on lines
+16
to
+20
| if (!events[registerPanelId]) { | ||
| events[registerPanelId] = {}; | ||
| } | ||
| events[registerPanelId][event] = callback; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow multiple handlers per event and return a boolean from trigger
Currently, registering the same event twice overwrites the previous handler. Support a Set of handlers per event and have trigger return whether anything ran.
Apply these diffs:
- if (!events[registerPanelId]) {
- events[registerPanelId] = {};
- }
- events[registerPanelId][event] = callback;
+ if (!events[registerPanelId]) {
+ events[registerPanelId] = {};
+ }
+ if (!events[registerPanelId][event]) {
+ events[registerPanelId][event] = new Set<PanelEventHandler>();
+ }
+ events[registerPanelId][event].add(callback);- function trigger(event: string, params: unknown, panelId?: string) {
+ function trigger(event: string, params: unknown, panelId?: string): boolean {
const triggerPanelId = panelId ?? computedPanelId;
assertPanelId(triggerPanelId);
- const callback = events[triggerPanelId]?.[event];
- if (callback) {
- callback(params);
- }
+ const callbacks = events[triggerPanelId]?.[event];
+ if (callbacks?.size) {
+ for (const cb of callbacks) cb(params);
+ return true;
+ }
+ return false;
} type PanelEventHandler = (params: unknown) => void;
-type PanelEvents = {
- [key: string]: PanelEventHandler;
-};
-type PanelsEvents = {
- [key: string]: PanelEvents;
-};
+type PanelEvents = {
+ [event: string]: Set<PanelEventHandler>;
+};
+type PanelsEvents = {
+ [panelId: string]: PanelEvents;
+};Also applies to: 22-29, 49-55
🤖 Prompt for AI Agents
In app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx around lines
16-20, 22-29 and 49-55, event registration currently stores a single callback
per event which overwrites previous handlers and trigger does not return whether
any handlers ran; change the event storage to use a Set of handlers for each
event (initialize events[registerPanelId][event] as a Set and add callbacks to
the Set instead of replacing), update removal to delete from the Set, update
trigger to iterate over the Set invoking each handler (catching errors if
desired) and have trigger return a boolean indicating whether at least one
handler was invoked.
Comment on lines
+39
to
+40
| return { register, trigger, unregister }; | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Expose a non-hook trigger for non-React call sites (operators, services)
Triggers don’t require React state; calling this hook from non-React code would violate Rules of Hooks. Provide a pure helper and prefer it in operator code paths.
Add this helper near the top-level (outside the hook):
export function triggerPanelEvent(panelId: string, event: string, params?: unknown): boolean {
if (!panelId) return false;
const bucket = events[panelId]?.[event];
if (!bucket) return false;
if (bucket instanceof Set) {
if (!bucket.size) return false;
for (const cb of bucket) cb(params);
return true;
}
// Back-compat if single-handler shape remains somewhere
(bucket as unknown as (p: unknown) => void)(params);
return true;
}Verification (scan for hook usage outside components/hooks):
🏁 Script executed:
#!/bin/bash
# Find direct invocations of the hook; inspect contexts for non-React usage
rg -n -C2 -S '\b(usePanelClientEvent|usePanelEvents)\s*\(' app/packagesLength of output: 940
Extract trigger logic into a standalone helper and update operator call sites
• In app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx, add above the hook definition:
export function triggerPanelEvent(
panelId: string,
event: string,
params?: unknown
): boolean {
if (!panelId) return false;
const bucket = events[panelId]?.[event];
if (!bucket) return false;
if (bucket instanceof Set) {
if (!bucket.size) return false;
for (const cb of bucket) cb(params);
return true;
}
(bucket as unknown as (p: unknown) => void)(params);
return true;
}• In app/packages/operators/src/built-in-operators.ts (line 1513), replace:
const { trigger } = usePanelClientEvent();with:
import { triggerPanelEvent } from './Panel/hooks/usePanelClientEvent';
const trigger = (params) => triggerPanelEvent(panelId, eventName, params);🤖 Prompt for AI Agents
In app/packages/operators/src/Panel/hooks/usePanelClientEvent.tsx around lines
39-40, extract the trigger logic into a standalone exported helper named
triggerPanelEvent that accepts (panelId: string, event: string, params?:
unknown) and performs the same null checks, bucket lookup, Set iteration or
single-callback invocation, and boolean return as in the suggested snippet;
place this exported function above the hook definition. Then in
app/packages/operators/src/built-in-operators.ts at ~line 1513, replace the
hook-derived trigger by importing triggerPanelEvent from the Panel/hooks file
and define const trigger = (params) => triggerPanelEvent(panelId, eventName,
params) so operator call sites use the new helper.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Add a utility to easily register and trigger JS panel events from Python panel events
How is this patch tested? If it is not, please explain why.
Using a hybrid python panel
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Add a utility to easily register and trigger JS panel events from Python panel events. See usage for more details
What areas of FiftyOne does this PR affect?
fiftyonePython library changesUsage
Summary by CodeRabbit
New Features
Chores