Human Annotation sidebar cleanup #6422
Conversation
WalkthroughIntroduces permission-aware annotation and schema management via new hooks, updates Sidebar to conditionally load/render annotation features, refines annotation editing and parsing logic, adjusts overlay creation/label handling, broadens list-type field handling, and updates public exports. Minor refactors and cleanup are included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sidebar
participant Hooks as useCanAnnotate / useCanManageSchema
participant Loader as loadSchemas
participant UI as Annotate UI
User->>Sidebar: Open sidebar
Sidebar->>Hooks: useCanAnnotate()
alt canAnnotate = true
Sidebar->>Loader: loadSchemas()
Sidebar->>UI: Render Annotate mode
UI->>Hooks: useCanManageSchema()
alt canManageSchema = true
UI-->>User: Show schema controls
else
UI-->>User: Hide/disable schema controls + info alert
end
else
Sidebar-->>User: Render Explore only (Annotate hidden)
end
sequenceDiagram
autonumber
actor User
participant Annotate as Annotate/Create
participant useCreate as useCreate()
participant Lighter as useLighter()
participant Scene
participant Handler as InteractiveDetectionHandler
User->>Annotate: Add DETECTION
Annotate->>useCreate: createOverlay(Detection)
useCreate->>Lighter: overlayFactory / addOverlay
useCreate->>Scene: enter interactive mode
useCreate->>Handler: new InteractiveDetectionHandler(scene)
Note right of Handler: Manages user interaction\nfor detection drawing
Handler-->>Scene: Interactive bbox creation
Scene-->>useCreate: Overlay finalized
useCreate-->>Annotate: Overlay instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
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/Sidebar/Annotate/Edit/AnnotationSchema.tsx (1)
118-118: Throw Error instance instead of raw string.Throwing a raw string is not idiomatic in JavaScript/TypeScript. Prefer throwing an
Errorinstance for better stack traces and consistency.Apply this diff:
- throw "text"; + throw new Error("text attribute type not yet implemented");
🧹 Nitpick comments (4)
app/packages/looker/src/index.ts (1)
10-10: LGTM! Export is type-safe and correct.The type-only export of
DetectionLabelis syntactically correct and follows TypeScript best practices. Note that this imports directly from./overlays/detectionwhile line 9 imports from the./overlaysbarrel export. This pattern difference might be intentional (e.g., keeping detection-specific types separate), but you may want to verify consistency across your export strategy for future maintainability.app/packages/core/src/components/Modal/Sidebar/Sidebar.tsx (1)
35-38: Consider using JSX comment syntax for inline comments.The inline comment uses a less idiomatic format for JSX. Consider using standard JSX comment syntax for better consistency.
Apply this diff for more idiomatic JSX comments:
- { - // Hide Annotation for read only users - !disableAnnotation && <Mode /> - } + {/* Hide Annotation for read only users */} + {!disableAnnotation && <Mode />}app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (1)
19-29: Add explicit return type annotation.The
getLabelutility function lacks an explicit return type annotation, which could reduce type safety.Apply this diff to add a return type:
-const getLabel = (value) => { +const getLabel = (value: unknown): string => { if (typeof value === "boolean") { return value ? "True" : "False"; } if (value === null || value === undefined) { return "None"; } - return value; + return String(value); };app/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.ts (1)
61-61: Prefer exhaustive type checking over generic default errorUse a switch on type with an assertNever default to catch new LabelType variants at compile time.
Example:
function assertNever(x: never): never { throw new Error(`Unhandled label type: ${String(x)}`); } // ... switch (type) { case CLASSIFICATION: // ... break; case DETECTION: // ... break; case POLYLINE: // ... break; default: assertNever(type); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/packages/core/src/components/Modal/Sidebar/Annotate/Actions.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsx(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx(7 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Field.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Id.tsx(0 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useCreate.ts(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/ImportSchema.tsx(2 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.ts(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useCanAnnotate.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useCanManageSchema.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useLoadSchemas.ts(1 hunks)app/packages/core/src/components/Modal/Sidebar/Sidebar.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx(0 hunks)app/packages/lighter/src/index.ts(1 hunks)app/packages/looker/src/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Id.tsx
- app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx
🧰 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/ImportSchema.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/useLoadSchemas.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/useCanManageSchema.tsapp/packages/lighter/src/index.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Field.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/useCanAnnotate.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Actions.tsxapp/packages/core/src/components/Modal/Sidebar/Sidebar.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useCreate.tsapp/packages/looker/src/index.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx
🔇 Additional comments (16)
app/packages/core/src/components/Modal/Sidebar/Annotate/useLoadSchemas.ts (1)
54-54: LGTM: Defensive null handling.The nullish coalescing operator (
?? "") provides a safe fallback whentypeis null or undefined, preventing potential runtime errors during schema loading. This aligns with the broader permission-driven UI changes in the Annotate workflow.app/packages/lighter/src/index.ts (1)
45-45: LGTM: Public API expansion.The export of
InteractiveDetectionHandleris straightforward and supports the interactive annotation flow for DETECTION overlays.app/packages/core/src/components/Modal/Sidebar/Annotate/useCanManageSchema.ts (1)
1-6: LGTM: Clean permission check abstraction.The hook provides a clear, reusable abstraction for schema management permissions. While thin, it centralizes the permission logic and makes consuming components more readable.
app/packages/core/src/components/Modal/Sidebar/Annotate/Actions.tsx (1)
10-10: LGTM: Consistent permission-based rendering.The hook import and conditional rendering of
<Schema />correctly implements permission-based UI gating, ensuring schema management features are only available to authorized users.Also applies to: 235-235, 255-255
app/packages/core/src/components/Modal/Sidebar/Annotate/ImportSchema.tsx (1)
6-6: LGTM: User-friendly permission handling.The replacement of hardcoded permission with runtime check properly gates the "Add schema" functionality. The informational alert provides clear feedback to users without manage permissions, enhancing the user experience.
Also applies to: 20-20, 42-42, 47-63
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Field.tsx (1)
48-52: LGTM: Improved readability.Extracting the
isCreatingvariable improves code readability without changing functionality. The descriptive name makes the intent clearer than the inline atom reference.app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (1)
49-59: LGTM: Simplified state initialization.The removal of the
initialfallback simplifies the code. TheuseEffectinitialization runs immediately whenoverlaychanges, and the schema fields are defined as optional, soSchemaIOComponentshould handle the initial empty state correctly.Also applies to: 108-108
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (1)
23-23: LGTM: Consistent list-type field handling.Adding
POLYLINEStoIS_LISTensures consistent treatment of plural annotation types. This allows fields with POLYLINES type to support multiple labels, matching the behavior of CLASSIFICATIONS and DETECTIONS.app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsx (1)
13-13: LGTM! Permission-aware UI implementation.The dynamic permission check using
useCanManageSchema()correctly replaces the hardcoded value, and the informational Alert provides clear guidance to users without manage permissions.Also applies to: 35-35, 60-60, 69-85
app/packages/core/src/components/Modal/Sidebar/Annotate/useCanAnnotate.ts (1)
1-6: LGTM! Clean permission hook implementation.The hook correctly derives annotation permission from the
readOnlystate using Recoil'suseRecoilValue.app/packages/core/src/components/Modal/Sidebar/Sidebar.tsx (1)
7-7: LGTM! Permission-based conditional rendering.The integration of
useCanAnnotate()correctly guards annotation features for read-only users, and the effect dependencies are properly configured.Also applies to: 26-26, 30-31, 39-39
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useCreate.ts (1)
1-7: LGTM! Clean import consolidation and type safety.The use of
import typefor type-only imports and the consolidated import from@fiftyone/lighterimprove type safety and align with TypeScript best practices. TheInteractiveDetectionHandlerintegration is correct.Also applies to: 21-21, 54-55
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (3)
139-147: LGTM! Proper null handling for empty numeric inputs.The logic correctly returns
nullfor empty strings in numeric fields rather than attempting to parse them, which preventsNaNvalues.
165-166: LGTM! Safe optional chaining.The optional chaining for
overlay?.getLabel()provides appropriate defensive coding against undefined overlay references.
176-176: Verify error message accuracy.The error message was changed from "no overlay" to "no field", but this check occurs when
!fieldis true. Ensure this message accurately reflects the condition being checked, especially since there's a separate check for!overlayon Line 179.Run the following script to understand the context of these error checks:
app/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.ts (1)
9-9: Import looks goodImporting POLYLINE aligns with the added type branch below.
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AddSchema.tsx
Show resolved
Hide resolved
app/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.ts
Show resolved
Hide resolved
app/packages/core/src/components/Modal/Sidebar/Annotate/useAddAnnotationLabel.ts
Show resolved
Hide resolved
1 participant
What changes are proposed in this pull request?
Sidebar cleanup
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