Add detach to spotlight #5157
Add detach to spotlight #5157
Conversation
WalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
app/packages/spotlight/src/types.ts (1)
61-61: Consider adding JSDoc documentationTo improve maintainability and provide better IDE support, consider adding JSDoc documentation for the new
detachmethod explaining its purpose and parameters.+ /** Detaches a Looker instance associated with the given ID */ detach?: (id: ID) => void;app/packages/core/src/components/Grid/Grid.tsx (1)
Line range hint
31-68: Enhance type safety for Spotlight configurationConsider adding explicit TypeScript interfaces for the Spotlight configuration and looker types to improve type safety and documentation.
Add these type definitions:
interface SpotlightLooker { destroy(): void; detach(): void; disable(): void; attach(element: HTMLElement, dimensions: unknown): void; } interface SpotlightConfig<T, S> { destroy: (id: { description: string }) => void; detach: (id: { description: string }) => void; // ... other config properties }app/packages/spotlight/src/row.ts (1)
234-235: Enhance type safety and error handling in the destroy/detach logic.The implementation correctly supports both destroy and detach operations with proper error handling. Consider these improvements:
- Add type narrowing for better type safety
- Enhance error reporting with operation context
- #destroyItems(destroyItems = false) { - const destroy = destroyItems ? this.#config.destroy : this.#config.detach; + #destroyItems(destroyItems = false) { + const operation = destroyItems ? 'destroy' : 'detach'; + const handler = destroyItems ? this.#config.destroy : this.#config.detach; + + if (!handler) { + return; + } const errors = []; for (const item of this.#row) { try { - destroy(item.item.id); + handler(item.item.id); } catch (e) { - errors.push(e); + errors.push(`Failed to ${operation} item ${item.item.id.description}: ${e}`); } } if (errors.length > 0) { - console.error("Errors occurred during row destruction:", errors); + console.error(`Errors occurred during row ${operation}:`, errors); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/core/src/components/Grid/Grid.tsx(1 hunks)app/packages/spotlight/src/row.ts(2 hunks)app/packages/spotlight/src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/core/src/components/Grid/Grid.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/spotlight/src/row.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/spotlight/src/types.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
📓 Learnings (1)
app/packages/spotlight/src/row.ts (1)
Learnt from: tom-vx51
PR: voxel51/fiftyone#5015
File: app/packages/spotlight/src/row.ts:122-125
Timestamp: 2024-11-12T12:36:39.324Z
Learning: In the `Row` class of `app/packages/spotlight/src/row.ts`, ensure that the `destroy` method includes error handling to prevent memory leaks due to cleanup errors.
🔇 Additional comments (3)
app/packages/spotlight/src/types.ts (1)
61-61: LGTM! The detach method is well-integrated
The addition of the optional detach method to SpotlightConfig interface aligns well with the PR objective and follows the existing pattern of the destroy method. The type signature is correct and maintains consistency with the interface's generic parameters.
app/packages/core/src/components/Grid/Grid.tsx (1)
Line range hint 147-157: Verify cleanup behavior for detached items
The cleanup in useLayoutEffect only calls destroy. Verify if detached items also need cleanup during component unmount.
app/packages/spotlight/src/row.ts (1)
123-123: LGTM! Clean implementation of conditional destroy/detach behavior.
The change correctly propagates the destroyItems flag to the private method while maintaining backward compatibility.
What changes are proposed in this pull request?
Looker instances should be detached when a spotlight instance is destroyed
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
detachmethod for theSpotlightinstance to enhance looker management.detachmethod to theSpotlightConfiginterface for configurable detachment behavior.Bug Fixes
#destroyItemsmethod in theRowclass to allow for more explicit handling of item destruction based on a new parameter.