E2E Segmentation Tests#7725
Open
mcdoh wants to merge 24 commits into
Open
Conversation
added 16 commits
May 15, 2026 12:17
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR extracts resolvePointVariant and buildAnnotationLabel into dedicated modules, refactors usePointSelection to avoid stale-closure cleanup and re-exports variant APIs, adds a BrowserAnnotationProvider test seam for deterministic SAM2 workers, updates PythonRunner.exec to reject on non-zero exits, and adds extensive unit and Playwright e2e tests plus POM/SDK helpers for segmentation and persistence workflows. Sequence Diagram(s)sequenceDiagram
participant UI
participant usePointSelection
participant resolvePointVariant
participant DetectionOverlay
participant BrowserAnnotationProvider
participant SAM2Worker
UI->>usePointSelection: click(relativePoint, modifiers, label)
usePointSelection->>resolvePointVariant: relativePoint, modifiers, label
resolvePointVariant->>DetectionOverlay: containsMaskPixel(relativePoint) [if overlay present]
resolvePointVariant-->>usePointSelection: POSITIVE / NEGATIVE (shift may invert)
usePointSelection-->>UI: apply point with variant
UI->>BrowserAnnotationProvider: request SAM2 inference
BrowserAnnotationProvider->>SAM2Worker: postMessage(embedAndDecode)
SAM2Worker-->>BrowserAnnotationProvider: postMessage(inference result)
BrowserAnnotationProvider-->>UI: inference result (mask,bbox)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
|
Enterprise sync conflict This branch has a merge conflict with Enterprise Enterprise sync run: https://github.com/voxel51/fiftyone-teams/actions/runs/26958577003 See Integrating OSS work into Enterprise for instructions. |
![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: 4
🤖 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.
Inline comments:
In `@app/packages/annotation/src/agents/hooks/resolvePointVariant.ts`:
- Around line 37-45: The function resolvePointVariant currently types its third
parameter label as non-null but guards against null at runtime; change the
signature of resolvePointVariant to accept label: AnnotationLabel | null
(nullable) and update any related type annotations/imports if necessary so
callers (e.g., usePointSelection) can pass selectedLabel without casting; keep
the existing runtime guards (label && label.overlay ...) intact.
In `@e2e-pw/src/oss/specs/MISSING-annotate-segmentation-pen-roundtrip.spec.ts`:
- Around line 46-67: The beforeAll hook sets process.env.VFF_AI_SEGMENTATION =
"true" but the suite never restores the original value; update the teardown in
test.afterAll to capture the original value before mutating (in test.beforeAll)
and restore it in test.afterAll (or delete the key if it was undefined),
referencing the existing test.beforeAll/test.afterAll blocks and the
VFF_AI_SEGMENTATION environment variable so test isolation is preserved.
In `@e2e-pw/src/oss/specs/MISSING-pcd-only-dataset.spec.ts`:
- Around line 66-72: The current broad except around
fou3d.compute_orthographic_projection_images swallows all errors and can mask
real regressions; replace the blanket "except Exception" with either no
try/except (let failures propagate and fail the spec) or catch only the specific
expected exception type (e.g., ValueError or a custom NaN-related exception from
fiftyone.utils.utils3d) and handle it explicitly (log a clear message and
re-raise or mark the test as skipped/failing). Locate the call to
fou3d.compute_orthographic_projection_images and update the error handling so
only known, documented exceptions are caught and all other exceptions propagate.
In `@e2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts`:
- Around line 70-78: The suite sets process.env.VFF_AI_SEGMENTATION = "true" in
test.beforeAll which can leak to other specs; update test.afterAll to restore or
delete that env var (e.g., delete process.env.VFF_AI_SEGMENTATION or reset to
its previous value captured before setting) and ensure this cleanup runs after
foWebServer.stopWebServer() (or immediately after) so VFF_AI_SEGMENTATION does
not persist across worker processes; locate the env set in the beforeAll block
and the teardown in test.afterAll to implement the restore/delete.
🪄 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: 88aeaf62-47f0-4618-831e-99f355404243
⛔ Files ignored due to path filters (10)
app/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.locke2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-ai-mask-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-ai-mask-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-brush-stroke-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-brush-stroke-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-merge-union-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-merge-union-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-pen-rectangle-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts-snapshots/seg-pen-rectangle-chromium-linux.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (35)
app/packages/annotation/src/agents/hooks/resolvePointVariant.test.tsapp/packages/annotation/src/agents/hooks/resolvePointVariant.tsapp/packages/annotation/src/agents/hooks/useActiveTask.test.tsapp/packages/annotation/src/agents/hooks/useApplyInferenceResult.test.tsapp/packages/annotation/src/agents/hooks/useInferenceStatus.test.tsapp/packages/annotation/src/agents/hooks/usePointSelection.test.tsapp/packages/annotation/src/agents/hooks/usePointSelection.tsapp/packages/annotation/src/agents/hooks/useRegisterPointSelectionEventHandlers.test.tsapp/packages/annotation/src/persistence/buildAnnotationLabel.test.tsapp/packages/annotation/src/persistence/buildAnnotationLabel.tsapp/packages/annotation/src/persistence/useLighterDeltaSupplier.test.tsapp/packages/annotation/src/persistence/useLighterDeltaSupplier.tsapp/packages/annotation/src/persistence/usePersistAnnotationDeltas.test.tsapp/packages/annotation/src/persistence/usePersistenceRetryController.test.tsapp/packages/annotation/src/providers/BrowserAnnotationProvider.test.tsapp/packages/annotation/src/providers/BrowserAnnotationProvider.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useAIAnnotationMode.test.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/useManualSegmentationTools.test.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/usePenTool.test.tsapp/packages/lighter/src/commands/AddMaskKeypointCommand.test.tsapp/packages/lighter/src/commands/PaintStrokeCommand.test.tsapp/packages/lighter/src/interaction/InteractivePenHandler.test.tsapp/packages/lighter/src/interaction/buildBrushCursor.test.tsapp/packages/lighter/src/overlay/MaskKeypoints.test.tsapp/packages/lighter/src/overlay/rippleRing.test.tse2e-pw/src/oss/fixtures/annotate-sdk.tse2e-pw/src/oss/poms/modal/annotate-sidebar.tse2e-pw/src/oss/poms/modal/sample-canvas/index.tse2e-pw/src/oss/specs/MISSING-annotate-segmentation-ai-sam2.spec.tse2e-pw/src/oss/specs/MISSING-annotate-segmentation-pen-roundtrip.spec.tse2e-pw/src/oss/specs/MISSING-pcd-only-dataset.spec.tse2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.tse2e-pw/src/shared/python-runner/python-runner.test.tse2e-pw/src/shared/python-runner/python-runner.tse2e-pw/src/shared/sam2-mock-worker.ts
Collaborator
|
Enterprise sync conflict This branch has a merge conflict with Enterprise Enterprise sync run: https://github.com/voxel51/fiftyone-teams/actions/runs/27037321714 See Integrating OSS work into Enterprise for instructions. |
Contributor
There was a problem hiding this comment.
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/hooks/usePointSelection.ts (1)
45-66:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
PointSelectioninterface is missing theresetmethod.The hook returns
resetat line 222 anduseSyncPointSelectionWithScenedestructures it at line 238, but the interface doesn't declare it. TypeScript will error when callers try to access.resetthrough the typed return.🔧 Proposed fix
export interface PointSelection { /** * Activates point selection. */ activate(): void; /** * Deactivates point selection. Disables interactivity with the point * selection business logic. */ deactivate(): void; /** * Clears all placed points while keeping point selection active. The * overlay and interactive handler remain in place — the user can continue * placing new points on an empty canvas. */ clearPoints(): void; + /** + * Resets the state atoms without interacting with the scene. Use this when + * the underlying scene has already been destroyed (e.g. sample navigation) + * and the overlay/handler references are now stale. + */ + reset(): void; + /** The current activation status of the point selection tool. */ isActive: boolean; }🤖 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 `@app/packages/annotation/src/agents/hooks/usePointSelection.ts` around lines 45 - 66, The PointSelection interface is missing a declaration for the reset method that is being returned by the hook and used by callers. Add a new method declaration to the PointSelection interface after the clearPoints method with appropriate JSDoc documentation explaining what the reset method does (you can reference how the method is used in the hook at line 222 to understand its purpose).
🤖 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.
Outside diff comments:
In `@app/packages/annotation/src/agents/hooks/usePointSelection.ts`:
- Around line 45-66: The PointSelection interface is missing a declaration for
the reset method that is being returned by the hook and used by callers. Add a
new method declaration to the PointSelection interface after the clearPoints
method with appropriate JSDoc documentation explaining what the reset method
does (you can reference how the method is used in the hook at line 222 to
understand its purpose).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3c8f315-ad80-4a94-b33f-675af1b54767
⛔ Files ignored due to path filters (2)
app/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
app/packages/annotation/src/agents/hooks/usePointSelection.tsapp/packages/annotation/src/persistence/buildAnnotationLabel.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts (1)
204-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove misleading comment.
The comment claims
annotateSDKis "unused", but line 221 callsannotateSDK.getDetectionsState(). Remove this comment or update it to reflect the actual usage.Proposed fix
- // Annotate the freshly-saved sample. - void annotateSDK; // unused — kept so per-test fixture creation still runs + // Annotate the freshly-saved sample.🤖 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 `@e2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts` at line 204, The inline comment claiming "annotateSDK" is unused is incorrect; update or remove it so it no longer misleads future readers — locate the declaration/reference to annotateSDK (the variable used later via annotateSDK.getDetectionsState()) and either delete the comment or change it to accurately state that the variable is used by per-test fixtures and later by getDetectionsState().
🤖 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.
Outside diff comments:
In `@e2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts`:
- Line 204: The inline comment claiming "annotateSDK" is unused is incorrect;
update or remove it so it no longer misleads future readers — locate the
declaration/reference to annotateSDK (the variable used later via
annotateSDK.getDetectionsState()) and either delete the comment or change it to
accurately state that the variable is used by per-test fixtures and later by
getDetectionsState().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d604e672-b6ad-4b80-b6c0-f68b826439af
📒 Files selected for processing (4)
app/packages/annotation/src/agents/hooks/resolvePointVariant.tse2e-pw/src/oss/specs/MISSING-annotate-segmentation-pen-roundtrip.spec.tse2e-pw/src/oss/specs/MISSING-pcd-only-dataset.spec.tse2e-pw/src/oss/specs/MISSING-segmentation-tool-snapshots.spec.ts
added 2 commits
June 8, 2026 09:38
Collaborator
|
Enterprise sync conflict This branch has a merge conflict with Enterprise Enterprise sync run: https://github.com/voxel51/fiftyone-teams/actions/runs/27142943724 See Integrating OSS work into Enterprise for instructions. |
added 2 commits
June 8, 2026 10:19
added 2 commits
June 8, 2026 10:52
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?
tests
🧪 How is this patch tested? If it is not, please explain why.
📝 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
Bug Fixes
Tests
Chores
Enterprise sync PR: https://github.com/voxel51/fiftyone-teams/pull/2824