Allow for multiple Lighter instances#6555
Conversation
WalkthroughThis pull request decouples Lighter scene state from a hard singleton and relaxes looker assumptions in modal code. Changes: Sequence Diagram(s)sequenceDiagram
actor Component
participant Hook as useLighter(atom)
participant Atom as defaultLighterSceneAtom
participant Scene as Scene2D (scene)
participant Render as render callbacks
Component->>Hook: call useLighter(atom?)
Hook->>Atom: read atom (useAtom)
Atom-->>Hook: scene (nullable)
Hook->>Scene: expose scene APIs (addOverlay, undo, redo, ...)
Component->>Hook: registerRenderCallback(cb)
alt scene present
Hook->>Scene: scene.registerRenderCallback(cb)
Scene->>Render: invoke callbacks on render
Render-->>Component: callback runs
else scene missing
Hook-->>Component: registerRenderCallback no-op (warn)
end
sequenceDiagram
participant Index as ModalEntry (index.tsx)
participant Modal as Modal component
participant Looker as lookerRef?.current
Note over Index,Modal: Old flow required Looker to exist to render Modal
Index->>Index: if modal && lookerRef.current
Index-->>Modal: render Modal
Note over Index,Modal: New flow removes lookerRef guard
Index->>Index: if modal
Index-->>Modal: render Modal
Modal->>Looker: access via lookerRef?.current (optional chaining)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
app/packages/core/src/components/Actions/Selected/Modal.tsx(3 hunks)app/packages/core/src/components/Actions/Selected/hooks.ts(1 hunks)app/packages/core/src/components/Actions/Selected/index.tsx(1 hunks)app/packages/core/src/components/Modal/Actions/index.tsx(3 hunks)app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx(1 hunks)app/packages/core/src/components/Modal/ModalLooker.tsx(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx(2 hunks)app/packages/core/src/components/Modal/use-modal-selective-rendering.ts(3 hunks)app/packages/lighter/src/core/Scene2D.ts(1 hunks)app/packages/lighter/src/core/SceneConfig.ts(0 hunks)app/packages/lighter/src/react/useLighter.ts(5 hunks)app/packages/lighter/src/react/useLighterSetup.ts(3 hunks)app/packages/lighter/src/state.ts(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/lighter/src/core/SceneConfig.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/core/src/components/Modal/Sidebar/Annotate/Annotate.tsxapp/packages/core/src/components/Actions/Selected/index.tsxapp/packages/lighter/src/state.tsapp/packages/core/src/components/Modal/Actions/index.tsxapp/packages/core/src/components/Modal/use-modal-selective-rendering.tsapp/packages/core/src/components/Actions/Selected/hooks.tsapp/packages/lighter/src/core/Scene2D.tsapp/packages/core/src/components/Modal/ModalLooker.tsxapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/lighter/src/react/useLighter.tsapp/packages/lighter/src/react/useLighterSetup.tsapp/packages/core/src/components/Actions/Selected/Modal.tsxapp/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx
🧠 Learnings (3)
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx
📚 Learning: 2024-07-27T04:14:08.421Z
Learnt from: sashankaryal
Repo: voxel51/fiftyone PR: 4310
File: app/packages/looker-3d/src/fo3d/Gizmos.tsx:110-0
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The grid rendering logic in `Gizmos.tsx` is part of an external component and not directly modifiable within the project's codebase.
Applied to files:
app/packages/core/src/components/Actions/Selected/index.tsx
📚 Learning: 2025-04-23T15:22:03.452Z
Learnt from: benjaminpkane
Repo: voxel51/fiftyone PR: 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/core/src/components/Modal/Actions/index.tsx
🧬 Code graph analysis (9)
app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx (1)
app/packages/lighter/src/state.ts (1)
defaultLighterSceneAtom(11-11)
app/packages/core/src/components/Actions/Selected/index.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
modal(29-55)app/packages/core/src/components/Modal/Actions/GroupVisibility/GroupVisibility.tsx (1)
modal(11-110)
app/packages/core/src/components/Actions/Selected/hooks.ts (2)
app/packages/state/src/recoil/groups.ts (1)
isGroup(135-140)app/packages/state/src/recoil/schema.ts (1)
activeLabels(418-467)
app/packages/core/src/components/Modal/ModalLooker.tsx (1)
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (1)
LighterSampleRenderer(29-94)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
app/packages/lighter/src/state.ts (1)
defaultLighterSceneAtom(11-11)
app/packages/lighter/src/react/useLighter.ts (3)
app/packages/lighter/src/state.ts (1)
defaultLighterSceneAtom(11-11)app/packages/lighter/src/core/Scene2D.ts (6)
canUndo(1255-1257)canRedo(1263-1265)registerRenderCallback(889-904)RenderCallback(118-122)addOverlay(943-981)redo(1241-1249)app/packages/lighter/src/commands/UndoRedoManager.ts (3)
canUndo(82-84)canRedo(90-92)redo(68-76)
app/packages/lighter/src/react/useLighterSetup.ts (2)
app/packages/lighter/src/state.ts (1)
defaultLighterSceneAtom(11-11)app/packages/lighter/src/event/EventBus.ts (1)
EventBus(364-406)
app/packages/core/src/components/Actions/Selected/Modal.tsx (1)
app/packages/looker/src/lookers/video.ts (1)
VideoLooker(29-447)
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (3)
app/packages/core/src/components/Modal/Lighter/SharedCanvas.ts (1)
singletonCanvas(91-91)app/packages/lighter/src/react/useLighterSetup.ts (1)
useLighterSetupWithPixi(32-96)app/packages/lighter/src/react/index.ts (1)
useLighterSetupWithPixi(7-7)
🔇 Additional comments (10)
app/packages/core/src/components/Actions/Selected/Modal.tsx (1)
28-28: LGTM: Optional lookerRef is properly handled.The changes correctly make
lookerRefoptional and apply optional chaining consistently throughout. The existing guard on lines 42-45 already ensures safe access topause().Also applies to: 37-37, 48-48
app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx (1)
2-2: LGTM: Consistent atom rename.The import and usage correctly reference
defaultLighterSceneAtom. The explicit React import (line 8) may be unnecessary depending on your React version but doesn't cause issues.Also applies to: 8-8, 97-97
app/packages/lighter/src/react/useLighterSetup.ts (2)
30-37: LGTM: Configurable atom enables multiple instances.The addition of the
atomparameter with a default value cleanly enables support for multiple Lighter instances while maintaining backward compatibility. This aligns perfectly with the PR objectives.
43-68: Scene2D sceneId removal verified and correctly implemented.The verification confirms that Scene2D.ts has been successfully updated: the constructor accepts only
config: Scene2DConfigand contains no references tosceneId. The useLighterSetup.ts callsite correctly omitssceneIdfrom the Scene2D initialization.app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
10-10: LGTM: Consistent atom usage.The import and usage correctly reference the renamed
defaultLighterSceneAtom.Also applies to: 120-120
app/packages/core/src/components/Actions/Selected/index.tsx (1)
83-91: LGTM: Simplified rendering logic.Removing the
lookerRef.currentcheck aligns with makinglookerRefoptional in Modal. The Modal component now handles undefined lookerRef safely, so this external check is unnecessary.app/packages/core/src/components/Modal/ModalLooker.tsx (2)
46-47: LGTM: Ghost-dependent styling removed.The unconditional 100% width and height simplifies the component. This aligns with the PR objective of removing ghost looker logic.
88-92: LGTM: Cleaner rendering logic without ghost.The ternary operator clearly separates annotate mode (LighterSampleRenderer) from normal mode (ModalLookerNoTimeline), removing the ghost-based conditional rendering complexity.
app/packages/core/src/components/Modal/use-modal-selective-rendering.ts (1)
14-29: LGTM: Ghost logic removed.The removal of the
ghostparameter and related conditional logic simplifies this hook and aligns with the PR objective of removing ghost looker logic. The function now only depends onlookerpresence for its early return.Also applies to: 63-63
app/packages/lighter/src/state.ts (1)
6-11: All references to the old atom name have been successfully updated.The search found no remaining instances of
lighterSceneAtomacross TypeScript and TSX files. The rename todefaultLighterSceneAtomis complete, and the type-only import is correct.
sashankaryal
left a comment
sashankaryal
left a comment
There was a problem hiding this comment.
This still doesn't make it easy to spawn multiple Lighter instances... for instance, I cannot have multiple <LighterSampleRenderer /> in the same view (like I want in, say, 3D annotation view).
Could we instead do atomFamily and we key by (now removed) "sceneId" ${sample?.sample?._id}-${sample?.sample?.last_modified_at?.datetime}?
==
Requesting changes for reason above + ambiguity of "defaultLighterSceneAtom`.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (1)
20-36: Makeatomprop optional inLighterSampleRendererPropsinterfaceThe interface declares
atom: SceneAtomas required, butModalLooker.tsx:89calls<LighterSampleRenderer sample={sample} />without passing theatomprop. The component implementation provides a default value (atom = defaultLighterSceneAtom), so this should be allowed.Update the interface to:
export interface LighterSampleRendererProps { /** Scene atom; defaults to defaultLighterSceneAtom when omitted */ atom?: SceneAtom; className?: string; sample: Sample; }This resolves the type-check failure in
ModalLooker.tsxand aligns the interface with the actual default behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
app/packages/core/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (4)
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx(4 hunks)app/packages/core/src/components/Modal/Lighter/ReusableCanvas.ts(1 hunks)app/packages/core/src/components/Modal/Lighter/SharedCanvas.ts(0 hunks)app/packages/lighter/src/state.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/core/src/components/Modal/Lighter/SharedCanvas.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/lighter/src/state.tsapp/packages/core/src/components/Modal/Lighter/ReusableCanvas.tsapp/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx
🧬 Code graph analysis (2)
app/packages/lighter/src/state.ts (2)
app/__mocks__/recoil.ts (1)
atom(55-57)app/packages/lighter/src/core/Scene2D.ts (1)
Scene2D(127-1757)
app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (4)
app/packages/lighter/src/state.ts (2)
SceneAtom(13-13)defaultLighterSceneAtom(11-11)app/packages/lighter/src/react/useLighter.ts (1)
useLighter(20-156)app/packages/core/src/components/Modal/Lighter/ReusableCanvas.ts (1)
defaultCanvas(65-65)app/packages/lighter/src/react/useLighterSetup.ts (1)
useLighterSetupWithPixi(32-96)
⏰ 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). (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: lint / eslint
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
🔇 Additional comments (2)
app/packages/lighter/src/state.ts (1)
6-13: Atom rename and type alias look correctDocstring,
defaultLighterSceneAtomnaming, and theSceneAtomalias are consistent and give consumers a clear, typed default atom to override when needed. No changes needed here.app/packages/core/src/components/Modal/Lighter/LighterSampleRenderer.tsx (1)
95-97: Behavior confirmed: Singleton canvas will reparent between multiple concurrent mountsThe review comment is accurate.
ReusableCanvas.tsimplements a singleton pattern with explicit reparenting:getCanvas()callsthis.canvas.remove()thencontainer.appendChild(), causing the canvas to move between containers if multipleLighterSampleRendererinstances mount simultaneously.Currently, this is not an active issue:
- Only one
LighterSampleRendereris actively used (ModalLooker.tsx:89)- SidePanel.tsx:396 contains a TODO comment explicitly stating: "/* todo: replace with Lighter once we can have muliples scenes at once"—showing this limitation is already known
However, the architectural constraint is real and will impact the TODO work. You should:
- Document this single-canvas constraint in
ReusableCanvasor component JSDoc- When implementing multi-instance support, either create per-instance
ReusableCanvasobjects or add a canvas manager injection mechanism
app/packages/core/src/components/Modal/Lighter/ReusableCanvas.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/packages/core/src/components/Modal/Lighter/ReusableCanvas.ts (1)
12-20: Fix class name typo to match docs and intentThe class and export are still named
ReusabledCanvaswhile the docs and usage intent refer to “ReusableCanvas”. This is confusing and makes searching harder; rename the class and default export toReusableCanvasfor consistency.-class ReusabledCanvas { +class ReusableCanvas { @@ -export default ReusabledCanvas; - -export const defaultCanvas = new ReusabledCanvas(); +export default ReusableCanvas; + +export const defaultCanvas = new ReusableCanvas();Note: default imports can keep their local name, but any type references to
ReusabledCanvaswill need updating.Also applies to: 63-65
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
app/packages/core/src/components/Modal/Lighter/ReusableCanvas.ts(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/core/src/components/Modal/Lighter/ReusableCanvas.ts
benjaminpkane
force-pushed
the
cleaning/lighter
branch
from
eb707d9 to
af1aa05
Compare
sashankaryal
left a comment
sashankaryal
left a comment
There was a problem hiding this comment.
This breaks when running multiple Lighter instances side-by-side. I believe the root issue is defaultCanvas: we’re still effectively assuming a single shared canvas. The number of canvases needs to scale with the number of active scene atoms.
A WeakMap<SceneAtom, HTMLCanvasElement> might be the right model??
Also: if multiple canvases exist, this must switch to WebGPU. We run into #context restriction issues with WebGL quick.
3 participants
What changes are proposed in this pull request?
ghostlooker logic for 2D lighterTODO
How is this patch tested? If it is not, please explain why.
Locally
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Bug Fixes