feat(bounding-box): integrate bounding boxes with metadata form#4494
Open
dlasecki-box wants to merge 4 commits intobox:masterfrom
Open
feat(bounding-box): integrate bounding boxes with metadata form#4494dlasecki-box wants to merge 4 commits intobox:masterfrom
dlasecki-box wants to merge 4 commits intobox:masterfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds bounding-box highlight support: new hook Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MetadataUI as Metadata Sidebar UI
participant Hook as useMetadataFieldSelection
participant Converter as TargetLocation Converter
participant Preview as Preview Component
User->>MetadataUI: Click metadata field
MetadataUI->>Hook: handleSelectMetadataField(field)
Hook->>Hook: set selectedMetadataFieldId
alt field.targetLocation present
Hook->>Converter: convert targetLocation -> bounding boxes
Converter->>Converter: clamp x,y,width,height to [0,100]
Converter-->>Hook: BoxAnnotationsBoundingBox[]
Hook->>Preview: getPreview().showBoundingBoxHighlights(boxes)
else no targetLocation
Hook->>Preview: getPreview()?.hideBoundingBoxHighlights()
end
Hook->>Hook: attach document mousedown listener
User->>MetadataUI: Click outside
Hook->>Hook: detect outside click -> deselect
Hook->>Preview: getPreview().hideBoundingBoxHighlights()
Hook->>Hook: remove mousedown listener
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/elements/content-sidebar/__tests__/useMetadataFieldSelection.test.ts (1)
175-187: Consider usingafterEachfor DOM cleanup in click-outside tests.The manual DOM cleanup works but could leave elements behind if a test fails. Using
afterEachensures cleanup regardless of test outcome:♻️ Optional: Safer DOM cleanup pattern
describe('click outside behavior', () => { + let testElements: HTMLElement[] = []; + + afterEach(() => { + testElements.forEach(el => el.parentNode?.removeChild(el)); + testElements = []; + }); + // ... existing tests ... test('should not deselect when clicking on a metadata field element', () => { // ... const metadataFieldElement = document.createElement('div'); metadataFieldElement.setAttribute('data-metadata-field', 'true'); document.body.appendChild(metadataFieldElement); + testElements.push(metadataFieldElement); // ... test logic ... - - document.body.removeChild(metadataFieldElement); });Also applies to: 196-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/__tests__/useMetadataFieldSelection.test.ts` around lines 175 - 187, The tests manually append and remove metadataFieldElement in useMetadataFieldSelection.test.ts which can leave DOM nodes if a test fails; add a global afterEach hook that removes any appended elements (e.g., querySelectorAll('[data-metadata-field]') and remove each) and resets any mutated state so click-outside tests always clean up; update or remove the per-test document.body.removeChild(...) calls in the tests (references: variable metadataFieldElement and the assertion on result.current.selectedMetadataFieldId) so cleanup is centralized and guaranteed even on test failure.src/elements/content-sidebar/__tests__/clampPercentage.test.ts (1)
4-13: Consider adding edge case tests forNaNandInfinity.The test coverage is solid for normal numeric inputs. However, based on the actual usage in
useMetadataFieldSelection.ts(where arithmetic expressions likeitem.boundingBox.left * 100 - 0.5are passed), consider adding tests for edge cases that could arise from malformed data:🧪 Optional: Add edge case tests
test.each([ [50, 50], [0, 0], [100, 100], [-0.5, 0], [100.5, 100], [99.999, 99.999], + [NaN, NaN], + [Infinity, 100], + [-Infinity, 0], ])('clampPercentage(%s) should return %s', (input, expected) => { - expect(clampPercentage(input)).toBe(expected); + if (Number.isNaN(expected)) { + expect(clampPercentage(input)).toBeNaN(); + } else { + expect(clampPercentage(input)).toBe(expected); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/__tests__/clampPercentage.test.ts` around lines 4 - 13, Add edge-case tests for clampPercentage to cover NaN and infinite values: add test.each entries for [NaN, 0], [Infinity, 100], and [-Infinity, 0] in the existing test block that references clampPercentage; if those tests fail, update the clampPercentage implementation (the clampPercentage function) so that NaN maps to 0, positive Infinity clamps to 100 and negative Infinity clamps to 0, keeping the current behavior for normal numeric inputs used by useMetadataFieldSelection.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/elements/content-preview/ContentPreview.js`:
- Line 293: The new prop enableBoundingBoxHighlights was added to defaultProps
and used in loadPreview but is missing from the Props Flow type, causing type
errors; add enableBoundingBoxHighlights: boolean to the Props type declaration
(near the existing enableThumbnailsSidebar entry) so the component's Props type
includes this prop and matches defaultProps and usages in loadPreview.
In `@src/elements/content-sidebar/hooks/useMetadataFieldSelection.ts`:
- Around line 15-22: The bounding-box width/height calculations in the mapping
that returns id/x/y/width/height use mixed units — `left`/`top` are treated as
normalized (0–1) while `right`/`bottom` are treated as percentages — so update
the width and height expressions in the mapping inside useMetadataFieldSelection
(the return of targetLocationEntries.map) to compute the difference in
normalized space then convert to percent; specifically compute
(item.boundingBox.right - item.boundingBox.left) * 100 and
(item.boundingBox.bottom - item.boundingBox.top) * 100 (with the existing +1
offset and clampPercentage call as needed) so all terms use the same units, or
alternatively add a normalization step that converts boundingBox.right/bottom to
0–1 before calculating width/height and ensure x/y use the same normalization.
---
Nitpick comments:
In `@src/elements/content-sidebar/__tests__/clampPercentage.test.ts`:
- Around line 4-13: Add edge-case tests for clampPercentage to cover NaN and
infinite values: add test.each entries for [NaN, 0], [Infinity, 100], and
[-Infinity, 0] in the existing test block that references clampPercentage; if
those tests fail, update the clampPercentage implementation (the clampPercentage
function) so that NaN maps to 0, positive Infinity clamps to 100 and negative
Infinity clamps to 0, keeping the current behavior for normal numeric inputs
used by useMetadataFieldSelection.ts.
In `@src/elements/content-sidebar/__tests__/useMetadataFieldSelection.test.ts`:
- Around line 175-187: The tests manually append and remove metadataFieldElement
in useMetadataFieldSelection.test.ts which can leave DOM nodes if a test fails;
add a global afterEach hook that removes any appended elements (e.g.,
querySelectorAll('[data-metadata-field]') and remove each) and resets any
mutated state so click-outside tests always clean up; update or remove the
per-test document.body.removeChild(...) calls in the tests (references: variable
metadataFieldElement and the assertion on
result.current.selectedMetadataFieldId) so cleanup is centralized and guaranteed
even on test failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71eb007e-074e-4089-8fdf-26d81457158b
📒 Files selected for processing (11)
src/elements/content-preview/ContentPreview.jssrc/elements/content-preview/__tests__/ContentPreview.test.jssrc/elements/content-sidebar/MetadataInstanceEditor.tsxsrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/SidebarPanels.jssrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsxsrc/elements/content-sidebar/__tests__/clampPercentage.test.tssrc/elements/content-sidebar/__tests__/useMetadataFieldSelection.test.tssrc/elements/content-sidebar/hooks/useMetadataFieldSelection.tssrc/elements/content-sidebar/types/BoxAISidebarTypes.tssrc/elements/content-sidebar/utils/clampPercentage.ts
dlasecki-box
commented
Apr 1, 2026
| return undefined; | ||
| } | ||
|
|
||
| // Adding extra space (-0.25 for left and top, +0.5 for width and height) to provide paddings for bounding boxes |
Lindar90
approved these changes
Apr 1, 2026
Contributor
Lindar90
left a comment
Lindar90
left a comment
There was a problem hiding this comment.
Left 2 comments. But they are not PR blocking. Looks good!
| return; | ||
| } | ||
|
|
||
| const boundingBoxes = convertTargetLocationToBoundingBox(field.id, field.targetLocation); |
Contributor
There was a problem hiding this comment.
Nit: maybe we could pass the whole field object? I.e:
const boundingBoxes = buildBoundingBoxesFromField(field)
| function useMetadataFieldSelection( | ||
| getPreview?: () => { | ||
| showBoundingBoxHighlights?: (boundingBoxes: BoxAnnotationsBoundingBox[]) => void; | ||
| hideBoundingBoxHighlights?: () => void; |
Contributor
There was a problem hiding this comment.
Type duplication:
- MetadataSidebarRedesign
- and useMetadataFieldSelection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Tests