Skip to content

Instantly share code, notes, and snippets.

@tonylampada
Created April 8, 2026 11:38
Show Gist options
  • Select an option

  • Save tonylampada/035561ab4854e9b3b4e5d5d74e75d151 to your computer and use it in GitHub Desktop.

Select an option

Save tonylampada/035561ab4854e9b3b4e5d5d74e75d151 to your computer and use it in GitHub Desktop.

PR Review: Show annotation groups on Asset Library preview (DATAMAN-230)

Repository: roboflow/roboflow PR: #11061 Branch: dataman-230-show-annotation-groups-preview → master Author: @digaobarbosa Review Date: 2026-04-08

Summary

Adds annotation group visibility and annotation previews to the Asset Library image preview modal. Also fixes mask rendering compositing (putImageData → drawImage via temp canvas) and adds classification label support.

Issues

1. Bug: Projects without annotation groups are no longer clickable (High)

File: ImageProjectsPreviewModal.tsx:236-241

Project cards changed from <a> to <button>, but the click handler only acts when projectGroup exists or when already selected. If a project has no mapped annotation group, clicking does nothing. Previously the <a> always navigated to the project.

onClick={() => {
    if (isSelected) {
        window.open(projectUrl, "_blank");
    } else if (projectGroup) {
        setSelectedAnnotationGroup(projectGroup);
    }
    // else: nothing happens — user is stuck
}}

Fix: Add fallback navigation:

} else {
    window.open(projectUrl, "_blank");
}

2. Unstable annotationGroups reference causes effect churn (Medium)

File: ImageProjectsPreviewModal.tsx:48,81

Object.keys(sourceAnnotations) creates a new array reference every render. Used as a useEffect dep on line 81, it re-triggers the effect on every render cycle. The guard clause prevents infinite loops but causes unnecessary work.

Fix:

const annotationGroups = useMemo(() => Object.keys(sourceAnnotations), [sourceAnnotations]);

3. Backend module imported in frontend code (Medium)

File: convertStoredAnnotations.ts:4

const datasetHelpers = require("@functions/services/dataset/datasetHelpers");

getColors is ~6 lines (maps class names to a color palette). While this import pattern exists elsewhere in the codebase, it couples frontend to backend internals. Any future Node-only dep added to datasetHelpers would break the frontend bundle. Consider extracting to packages/shared/ or inlining.

4. Missing useEffect dependencies (Low)

File: ImageProjectsPreviewModal.tsx:69-81

Effect reads selectedAnnotationGroup, projectToGroup, and sourceAnnotations but only declares [loading, projects, annotationGroups]. The selectedAnnotationGroup omission is intentional (guard), but projectToGroup and sourceAnnotations are genuine missing deps.

5. Math.random() for annotation IDs (Low)

File: convertStoredAnnotations.ts:28,42,49

String(Math.random()) as fallback ID — fine for display but could cause React reconciliation issues on re-render. Use array index as fallback: id: box.id || \anno-${i}``

What's Good

  • Mask rendering fix (putImageDatadrawImage via temp canvas) is correct and well-commented
  • convertStoredAnnotations utility is clean with solid test coverage (bbox, polygon, mask, mixed, classification, edge cases)
  • Annotation groups section UX: collapsed with projects, expanded without — good default behavior
  • Proper cleanup with cancelled flag in fetch effect

Verdict

Request Changes — The project card click regression (#1) is a functional bug. The unstable reference (#2) is a one-liner fix. Rest of the PR is solid work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment