[HA] Integrate BrowserAnnotationProvider with annotation framework#7268
Merged
[HA] Integrate BrowserAnnotationProvider with annotation framework#7268
Conversation
Contributor
WalkthroughAdds a new SAM2BrowserAnnotationAgent implementing AnnotationAgent for SEGMENT tasks using a BrowserAnnotationProvider. infer() requires sampleDescriptor.mediaUrl, performs concurrency-safe lazy initialization, converts positive/negative points into provider prompts, forwards abort/dispose to the provider, and returns synchronous segmentation InferenceResults with mask, mask_width, mask_height, and bounding_box. Registers the agent in the built-in registry, re-exports it, adds comprehensive Vitest coverage, introduces useSampleDescriptor and useModalMediaPath hooks, and makes SampleDescriptor.mediaUrl required. Sequence DiagramsequenceDiagram
participant Client
participant SAM2Agent as SAM2BrowserAnnotationAgent
participant Provider as BrowserAnnotationProvider
participant Worker as Web Worker (SAM2 Model)
Client->>SAM2Agent: infer(context with sampleDescriptor.mediaUrl + points)
activate SAM2Agent
alt provider not initialized
SAM2Agent->>SAM2Agent: ensureInitialized()
SAM2Agent->>Provider: initialize()
activate Provider
Provider->>Worker: load model
Worker-->>Provider: model ready
Provider-->>SAM2Agent: initialize complete
deactivate Provider
end
SAM2Agent->>SAM2Agent: map positive/negative -> points[]
SAM2Agent->>Provider: infer({ imageUrl, points })
activate Provider
Provider->>Worker: run inference
Worker-->>Provider: segmentation {mask, bbox:{x,y,w,h}, width, height}
Provider-->>SAM2Agent: segmentation result
deactivate Provider
SAM2Agent-->>Client: Promise<InferenceResult sync {mask, mask_width, mask_height, bounding_box:[x,y,w,h]}>
deactivate SAM2Agent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
tom-vx51
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.ts`:
- Around line 44-53: The class currently requires callers to call the
non-interface method setImageUrl() before infer(), which is an out-of-contract
mutable side channel; remove that precondition and accept the image URL via the
inference request context instead. Update SAM2BrowserAnnotationAgent.infer to
read the image URL from the provided AnnotationContext (add a required property
or typed field on AnnotationContext or create a small typed request object) and
eliminate reliance on the instance field this.imageUrl and the setImageUrl
method; delete or deprecate setImageUrl and any runtime throw for missing
this.imageUrl, and adjust callers/tests to pass the image URL through the
AnnotationContext passed into infer. Ensure function/class names referenced:
setImageUrl, infer, AnnotationContext, SAM2BrowserAnnotationAgent, and
AnnotationAgent.
- Around line 51-63: The infer() method can race with setImageUrl() because
this.imageUrl is read after an await; fix by reading and validating the URL into
a local const before any await (e.g. const imageUrl = this.imageUrl; if
(!imageUrl) throw ...), then use that local imageUrl for building prompt points
and for the call to this.provider.infer; if buildPromptPoints currently reads
this.imageUrl, change it to accept an imageUrl parameter (or inline point
construction using the local imageUrl) so all operations in infer() use the
captured value rather than this.imageUrl.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe19f685-cfc7-416d-9b5f-e2d3ca7ac9c6
📒 Files selected for processing (5)
app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.test.tsapp/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.tsapp/packages/annotation/src/agents/hooks/useAgentRegistry.tsapp/packages/annotation/src/agents/index.tsapp/packages/annotation/src/providers/index.ts
app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.ts
Outdated
Show resolved
Hide resolved
app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.ts
Outdated
Show resolved
Hide resolved
lanzhenw
reviewed
Mar 31, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/annotation/src/agents/types.ts (1)
54-60:⚠️ Potential issue | 🟠 MajorFix TypeScript compilation error in
OperatorAnnotationAgent.test.tswheresampleDescriptoris missing the requiredmediaUrlfield.The
SampleDescriptortype now requiresmediaUrl: string, butOperatorAnnotationAgent.test.ts:22constructs it without this field, causing a compilation error. Add the missing field or update the test helper to properly mock the URL.(Note:
SAM2BrowserAnnotationAgent.test.ts:86intentionally omitsmediaUrlto test error handling, which is valid and properly usesas SampleDescriptorto test the agent's validation.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/annotation/src/agents/types.ts` around lines 54 - 60, The test fails because the SampleDescriptor type now requires mediaUrl; update the test so the sampleDescriptor constructed in OperatorAnnotationAgent.test.ts includes a valid mediaUrl string (or update the test helper that builds the sample descriptor to populate mediaUrl) so it matches the exported SampleDescriptor type; locate the sampleDescriptor variable in OperatorAnnotationAgent.test.ts (or the test helper it uses) and add a realistic mediaUrl value (e.g., "http://example.com/media.mp4") to satisfy the type without changing the production type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/agents/hooks/useSampleDescriptor.ts`:
- Around line 12-24: The hook useSampleDescriptor currently claims to return a
fully-populated SampleDescriptor while using values from useCurrentDatasetName,
useCurrentSampleId, and useModalMediaPath which can be null; change
useSampleDescriptor to return null until all three values are non-null (make the
return type SampleDescriptor | null), i.e., check datasetId, sampleId, and
mediaUrl and return null if any are falsy, otherwise return the memoized
descriptor; update callers (e.g., useAgentContext and any usage in
SAM2BrowserAnnotationAgent.infer) to handle the nullable return instead of
relying on this hook to supply non-null fields.
In `@app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.ts`:
- Around line 116-145: The ensureInitialized()/dispose() race can let a pending
provider.initialize() resolve after dispose() and incorrectly set
this.initialized = true; fix by adding a lifecycle token: capture a local token
(e.g. const token = ++this.initEpoch) before calling this.provider.initialize()
in ensureInitialized(), and only set this.initialized = true and
this.initializing$ = null if token === this.initEpoch; update dispose() to
increment/clear that token (and still call this.provider.dispose() and set
initializing$ = null), and add a regression test "dispose during initialize"
that starts initialize(), calls dispose() while pending, then asserts that
infer() triggers a fresh initialize rather than using a disposed provider.
Ensure references: ensureInitialized(), dispose(), initializing$, initialized,
provider.initialize(), infer().
---
Outside diff comments:
In `@app/packages/annotation/src/agents/types.ts`:
- Around line 54-60: The test fails because the SampleDescriptor type now
requires mediaUrl; update the test so the sampleDescriptor constructed in
OperatorAnnotationAgent.test.ts includes a valid mediaUrl string (or update the
test helper that builds the sample descriptor to populate mediaUrl) so it
matches the exported SampleDescriptor type; locate the sampleDescriptor variable
in OperatorAnnotationAgent.test.ts (or the test helper it uses) and add a
realistic mediaUrl value (e.g., "http://example.com/media.mp4") to satisfy the
type without changing the production type.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 161ca1ca-f8d4-4e4c-94b2-e79a09674b50
📒 Files selected for processing (7)
app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.test.tsapp/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.tsapp/packages/annotation/src/agents/hooks/index.tsapp/packages/annotation/src/agents/hooks/useAnnotationAgent.tsapp/packages/annotation/src/agents/hooks/useSampleDescriptor.tsapp/packages/annotation/src/agents/types.tsapp/packages/state/src/accessors/modal.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.ts`:
- Around line 136-143: ensureInitialized() currently resolves even when the
provider reports not ready, allowing infer() to call provider.infer() in an
invalid state; update ensureInitialized() to check provider.isInitialized()
after any async initialization completes and explicitly reject/throw (or set
initialized = false and throw an InitializationError) when the provider is still
not ready so callers (like infer()) cannot proceed; also ensure the
initializing$ field is nulled only after failure and that initialized remains
accurate (references: ensureInitialized(), initializing$, initialized,
provider.isInitialized(), provider.infer(), infer()).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2eec2b4e-de18-4b61-b8b7-7e6896cb27d2
📒 Files selected for processing (2)
app/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.test.tsapp/packages/annotation/src/agents/SAM2BrowserAnnotationAgent.ts
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.
🔗 Related Issues
📋 What changes are proposed in this pull request?
Integrates the interfaces set up in #7213 with the browser-based inference added in #7192 and subsequent PRs.
🧪 How is this patch tested? If it is not, please explain why.
unit tests
📝 Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Tests