Handle channel selection state for Zarr additions - #361
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactored Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/components/ColorChannelComponents.tsx (1)
119-149:⚠️ Potential issue | 🟠 MajorClear the loading slot instead of removing it.
removeIsChannelLoading()insrc/react/components/avivatorish/state.tsx:269-273splices the array. On this path the channel still exists, so Line 149 shifts every later loading flag left and leavesisChannelLoadingout of sync withselectionsandids. KeepremoveIsChannelLoading()for optimistic add rollback, but just clear the slot here.🛠️ Suggested fix
} catch (e) { console.error("failed to load channel"); console.error(e); - removeIsChannelLoading(index); + setIsChannelLoading(index, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/components/ColorChannelComponents.tsx` around lines 119 - 149, In the catch block of the onChange handler (inside the async selection load code), don't call removeIsChannelLoading(index) because that splices the isChannelLoading array and misaligns it with selections/ids; instead clear the loading slot for that channel by calling setIsChannelLoading(index, false) (or equivalent to set the boolean at that index), leaving removeIsChannelLoading() reserved for optimistic-add rollback paths.
🧹 Nitpick comments (1)
src/react/components/avivatorish/state.tsx (1)
290-292: Drop the extra MobX observer boundary.
VivProvideronly forwardsvivStoresandchildren, so normal React prop changes already re-render it. Wrapping a pure context provider inobserver()just adds another MobX subscription layer here.Based on learnings: in this codebase,
observer()is only needed for components that directly access MobX observables or stores.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/components/avivatorish/state.tsx` around lines 290 - 292, VivProvider is unnecessarily wrapped with MobX observer which adds an extra subscription layer even though it only forwards vivStores and children; remove the observer(...) wrapper so VivProvider is a plain functional component that returns <VivContext.Provider value={vivStores}>{children}</VivContext.Provider>, keeping the same PropsWithChildren and VivContextType types and exported name.
🤖 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/react/components/avivatorish/utils.ts`:
- Around line 294-345: The code builds dimensions with zip(labels, shape) but
only later adds an interleaved "c" via getSourceDimensionMap, causing zip to
miss sample-only axes; update buildSelectionForSource to first call
getSourceDimensionMap(source) and derive a normalized dimensions array from that
map (e.g., Array.from(dimensionMap.values()) mapping to {name, size}), then pass
that normalized list into getDefaultGlobalSelection and use it for selection
logic; ensure you keep using clampSelectionValue and that nextSelection keys use
the canonical dimension.name so buildSelectionForSource, getSourceDimensionMap,
and getDefaultGlobalSelection all agree on dimensions.
---
Outside diff comments:
In `@src/react/components/ColorChannelComponents.tsx`:
- Around line 119-149: In the catch block of the onChange handler (inside the
async selection load code), don't call removeIsChannelLoading(index) because
that splices the isChannelLoading array and misaligns it with selections/ids;
instead clear the loading slot for that channel by calling
setIsChannelLoading(index, false) (or equivalent to set the boolean at that
index), leaving removeIsChannelLoading() reserved for optimistic-add rollback
paths.
---
Nitpick comments:
In `@src/react/components/avivatorish/state.tsx`:
- Around line 290-292: VivProvider is unnecessarily wrapped with MobX observer
which adds an extra subscription layer even though it only forwards vivStores
and children; remove the observer(...) wrapper so VivProvider is a plain
functional component that returns <VivContext.Provider
value={vivStores}>{children}</VivContext.Provider>, keeping the same
PropsWithChildren and VivContextType types and exported name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d341d6e4-fe8e-4bfe-8291-c56771ba2281
📒 Files selected for processing (4)
src/react/components/ColorChannelComponents.tsxsrc/react/components/avivatorish/hooks.tssrc/react/components/avivatorish/state.tsxsrc/react/components/avivatorish/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/react/components/avivatorish/utils.ts`:
- Line 31: The current OME-TIFF detection uses case-sensitive includes checks
(name.includes("ome.tiff") || name.includes("ome.tif") ||
name.includes(".companion.ome")), which misses uppercase/mixed-case extensions;
fix it by normalizing the filename to lowercase before checking (e.g., const
lower = name.toLowerCase(); then use lower.includes("ome.tiff") ||
lower.includes("ome.tif") || lower.includes(".companion.ome")), or alternatively
use a case-insensitive regex (e.g., /ome\.tiff|ome\.tif|\.companion\.ome/i) to
perform the test so detection is case-insensitive.
- Around line 202-203: The code dereferences urlOrFile[0] without checking for
an empty array, causing a TypeError for [] before your UnsupportedBrowserError
is thrown; update the conditional that currently reads like if
(isArray(urlOrFile) && typeof urlOrFile[0].arrayBuffer !== "function") to first
check urlOrFile.length > 0 (e.g., if (isArray(urlOrFile) && urlOrFile.length > 0
&& typeof urlOrFile[0].arrayBuffer !== "function")), and separately handle the
empty-array case (throw a clear error or return early) so empty file arrays are
guarded before accessing [0] while still throwing an appropriate error (for
example using UnsupportedBrowserError or a more specific InvalidInput error)
when needed.
- Line 608: The SizeUnit type currently references only SI_PREFIXES symbols but
sizeToMeters expects full unit strings like "mm"/"cm"/"µm" (it strips a trailing
"m"), so update the SizeUnit definition to represent the full unit form (prefix
+ "m") and include the bare "m" case; e.g. derive it from SI_PREFIXES by using a
template literal like `${typeof SI_PREFIXES[number]["symbol"]}m` plus "m" (or
otherwise map SI_PREFIXES symbols to `${symbol}m`) so SizeUnit matches the
values accepted by sizeToMeters and callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 497704af-e03d-42bd-bd9e-958552e63734
📒 Files selected for processing (1)
src/react/components/avivatorish/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/components/ColorChannelComponents.tsx (1)
103-150:⚠️ Potential issue | 🟠 MajorDo not splice
isChannelLoadingwhen a channel selection fetch fails.Line 149 removes the loading slot (
removeIsChannelLoading(index)) even though no channel is removed. That shifts loading flags and misaligns indices across channels.Suggested fix
- const { setIsChannelLoading, isChannelLoading, removeIsChannelLoading, use3d } = useViewerStore( - ({ setIsChannelLoading, isChannelLoading, removeIsChannelLoading, use3d }) => ({ + const { setIsChannelLoading, isChannelLoading, use3d } = useViewerStore( + ({ setIsChannelLoading, isChannelLoading, use3d }) => ({ setIsChannelLoading, isChannelLoading, - removeIsChannelLoading, use3d, }), shallow, ); ... } catch (e) { console.error("failed to load channel"); console.error(e); - removeIsChannelLoading(index); + setIsChannelLoading(index, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/components/ColorChannelComponents.tsx` around lines 103 - 150, The catch block currently calls removeIsChannelLoading(index) which splices the loading array and shifts indices; instead, clear the loading flag for that channel without removing the slot by calling setIsChannelLoading(index, false) (or the equivalent setter) in the catch handler of the onChange async flow that calls getSingleSelectionStats; update references to removeIsChannelLoading only for actual channel removals, not for fetch failures, to preserve index alignment.
🤖 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/react/components/ColorChannelComponents.tsx`:
- Around line 552-589: The onClick handler captures a transient numeric index
and then awaits async work, which can become stale when multiple rapid clicks
overlap; change the flow so you don't rely on the captured `index` after awaits
— either serialize add operations (use a simple in-memory lock/queue) or make
`addChannel` return a stable channel identifier/object and use that identifier
to call `setPropertiesForChannel`, `setIsChannelLoading`, `removeChannel`, and
`removeIsChannelLoading` instead of the captured `index`; ensure
`setIsChannelLoading` is set/cleared tied to that stable id and that
rollback/removal uses the same id so props won’t be applied to the wrong channel
when `getSingleSelectionStats` resolves.
---
Outside diff comments:
In `@src/react/components/ColorChannelComponents.tsx`:
- Around line 103-150: The catch block currently calls
removeIsChannelLoading(index) which splices the loading array and shifts
indices; instead, clear the loading flag for that channel without removing the
slot by calling setIsChannelLoading(index, false) (or the equivalent setter) in
the catch handler of the onChange async flow that calls getSingleSelectionStats;
update references to removeIsChannelLoading only for actual channel removals,
not for fetch failures, to preserve index alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40375a4f-3ee1-4026-94b3-86f155e58b7e
📒 Files selected for processing (1)
src/react/components/ColorChannelComponents.tsx
2 participants
Summary
Testing
Summary by CodeRabbit
Bug Fixes
UI Improvements