Display classification annotations in the annotations grid#1704
Conversation
- default props for tests
…editing-annotation-grid-item-view-list
📝 WalkthroughWalkthroughChangesClassification annotations are now included in the annotations grid. They render through a dedicated tile with always-visible classification pills, support thumbnail-based drag-to-search, and are covered by tests for rendering, selection, bulk relabeling, and undo handling. Classification grid support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AnnotationsGrid
participant AnnotationClassificationGridItem
participant SampleClassificationPills
participant SearchDropHandler
AnnotationsGrid->>AnnotationClassificationGridItem: render classification annotation tile
AnnotationClassificationGridItem->>SampleClassificationPills: pass classification sample and collection override
AnnotationsGrid->>SearchDropHandler: pass thumbnail URL and crop-derived filename
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf780aa1b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.svelte (2)
316-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueP2: Extract the dragData ternary into a small helper for readability.
The inline
isClassification ? buildClassificationDragData(...) : buildAnnotationDragData(...)ternary is a bit dense inside the template. A tiny local helper (e.g.getDragData(ann, annotationId)) would read more clearly.As per path instructions, prioritize style/readability fixes as succinct "P2" comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.svelte` around lines 316 - 327, The inline dragData ternary in the AnnotationsGrid template is too dense. Add a small local helper such as getDragData that accepts the annotation data and annotationId, selects the appropriate classification or regular builder, and reuse it for the dragData prop while preserving the existing arguments and behavior.Source: Path instructions
316-327: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClassification drag preview re-renders the full image via canvas unnecessarily.
For classification tiles,
cropWindowcovers the entire image (windowX/Y=0, full width/height perAnnotationClassificationGridItem.svelte), sohandleAnnotationDragStart→renderCropObjectUrlproduces a canvas-rendered blob that's pixel-identical to the already-availablecropWindow.sourceUrl. SincebuildClassificationDragDataalready falls back tocropWindow.sourceUrlwhencropUrlisn't set, this canvas render is wasted work on every classification drag-start, and gets more costly with large source images.Consider short-circuiting
handleAnnotationDragStart(or skipping theonDragStartcall) for classification tiles so the drag payload usescropWindow.sourceUrldirectly without the canvas round-trip.Also applies to: 97-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.svelte` around lines 316 - 327, Update the classification path around handleAnnotationDragStart and the onDragStart binding so classification tiles do not invoke renderCropObjectUrl or perform a canvas render. Skip the drag-start handler for classification annotations, allowing buildClassificationDragData to use cropWindow.sourceUrl directly, while preserving the existing drag behavior for non-classification annotations.lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.test.ts (1)
237-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueP2: Redundant
mockResolvedValue('v1')call.
renderGrid()setsmocks.getCollectionVersion.mockResolvedValue('v1')(line 238), butbeforeEachalready does this aftervi.clearAllMocks()(line 246). The call inrenderGrid()is dead duplication.As per path instructions, prioritize style/readability fixes as succinct "P2" comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.test.ts` around lines 237 - 249, Remove the redundant mocks.getCollectionVersion.mockResolvedValue('v1') call from renderGrid(), relying on the existing setup in the describe block’s beforeEach while preserving renderGrid()’s rendering behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.svelte`:
- Around line 316-327: The inline dragData ternary in the AnnotationsGrid
template is too dense. Add a small local helper such as getDragData that accepts
the annotation data and annotationId, selects the appropriate classification or
regular builder, and reuse it for the dragData prop while preserving the
existing arguments and behavior.
- Around line 316-327: Update the classification path around
handleAnnotationDragStart and the onDragStart binding so classification tiles do
not invoke renderCropObjectUrl or perform a canvas render. Skip the drag-start
handler for classification annotations, allowing buildClassificationDragData to
use cropWindow.sourceUrl directly, while preserving the existing drag behavior
for non-classification annotations.
In
`@lightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.test.ts`:
- Around line 237-249: Remove the redundant
mocks.getCollectionVersion.mockResolvedValue('v1') call from renderGrid(),
relying on the existing setup in the describe block’s beforeEach while
preserving renderGrid()’s rendering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5d6f3c14-115c-496f-9cd5-7c5213d24161
📒 Files selected for processing (14)
CHANGELOG.mdlightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationClassificationGridItem/AnnotationClassificationGridItem.mock.sveltelightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationClassificationGridItem/AnnotationClassificationGridItem.sveltelightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationClassificationGridItem/AnnotationClassificationGridItem.test.tslightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.helpers.tslightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.sveltelightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGrid.test.tslightly_studio_view/src/lib/components/AnnotationsGrid/AnnotationsGridItem/AnnotationsGridItem.mock.sveltelightly_studio_view/src/lib/components/AnnotationsGrid/SelectedAnnotations/SelectedAnnotations.mock.sveltelightly_studio_view/src/lib/components/Grid/Grid.mock.sveltelightly_studio_view/src/lib/components/GridContainer/GridContainer.mock.sveltelightly_studio_view/src/lib/components/GridItem/GridItem.mock.sveltelightly_studio_view/src/lib/components/SampleClassificationPills/SampleClassificationPills.sveltelightly_studio_view/src/lib/components/SelectableBox/SelectableBox.mock.svelte
|
/review |
ikondrat
left a comment
ikondrat
left a comment
There was a problem hiding this comment.
The rendering branch looks reasonable, but several tests currently mock away the behavior they claim to cover. The inline comments focus on making those contracts observable without broadening the production change.
2 participants
What has changed and why?
Classification annotations were previously filtered out of the annotations grid. This PR removes that filter and renders classification annotations using the existing
AnnotationClassificationGridItemcomponent.How has it been tested?
Unit tests
Manual test:
Did you update CHANGELOG.md?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests