Lig 9903 2 fe images#1583
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds backend support for embedding-plot region selections: a new ChangesBackend region resolution
Estimated code review effort: 3 (Moderate) | ~30 minutes Frontend region selection UI and request wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PlotPanel
participant useImageFilters
participant ImageResolver
participant embedding_region_resolver
participant SampleFilter
User->>PlotPanel: draw lasso/rectangle and mouse up
PlotPanel->>useImageFilters: updateEmbeddingRegion(polygon)
useImageFilters->>useImageFilters: set filters.embedding_region
PlotPanel->>ImageResolver: request images (sample_filter.embedding_region)
ImageResolver->>embedding_region_resolver: resolve_embedding_region(session, collection_id, filters)
embedding_region_resolver->>embedding_region_resolver: get_sample_ids_in_region (point-in-polygon)
embedding_region_resolver->>SampleFilter: set_resolved_region_sample_ids(ids)
ImageResolver->>SampleFilter: apply(query)
SampleFilter-->>ImageResolver: filtered results
ImageResolver-->>PlotPanel: matching samples
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 5a3fcb015c
ℹ️ 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".
| ...params, | ||
| filters: { | ||
| ...params.filters, | ||
| embedding_region: embeddingRegion ?? undefined |
There was a problem hiding this comment.
When a user commits an image lasso, this writes the selection into filterParams.filters.embedding_region, but the Images.svelte sync path still treats only sample_ids and confusion_cell as externally managed filters (syncFilterParams.ts omits/merges only those keys). On the next reactive sync, the current params differ from the base grid params and mergeExternalFilters rewrites filterParams without embedding_region, so the new region selection is cleared before it can reliably drive the grid. Include embedding_region in that external-filter omit/merge path.
Useful? React with 👍 / 👎.
| if self._resolved_region_sample_ids is None: | ||
| raise RuntimeError( | ||
| "embedding_region must be resolved with set_resolved_region_sample_ids() " | ||
| "before the filter is applied." |
There was a problem hiding this comment.
With an image lasso active, useAdjacentImages sends the current ImageFilter to /samples/{sample_id}/adjacents, but the image adjacency resolvers call filters.apply(...) directly in both keyset and window paths without calling resolve_embedding_region. This new guard therefore raises for any unresolved embedding_region, causing image detail navigation/adjacent lookups after a lasso selection to fail with a server error instead of returning neighbors. Resolve or strip the region in the adjacency path before applying the filter.
Useful? React with 👍 / 👎.
| const embeddingRegion = $filterParams.filters?.embedding_region; | ||
| if (embeddingRegion) { | ||
| sampleFilter.embedding_region = embeddingRegion; |
There was a problem hiding this comment.
After this change image plot selections live in sample_filter.embedding_region rather than sample_ids, but the layout still builds image annotation-count filters from $imageFilterFromHook?.sample_filter?.sample_ids only before calling useImageAnnotationCounts. In the scenario where an image lasso is active, the grid is filtered by the backend-resolved region while the annotation filter rows/counts continue to reflect the whole grid; pass the region through the count-filter builder as well.
Useful? React with 👍 / 👎.
1 participant
What has changed and why?
(Delete this: Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.)
How has it been tested?
(Delete this: Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.)
Did you update CHANGELOG.md?
Summary by CodeRabbit
New Features
Bug Fixes
Tests