Call triggers/reload with fallback layer name #8298
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (1)
425-429: Avoid over-nesting the arrow function
While this inline arrow function is compact, consider extracting it into a named callback to enhance clarity and brevity, keeping the JSX cleaner.-<div - onClick={() => - this.reloadLayerData(layerName, isHistogramAvailable, maybeFallbackLayerName) - } -> +const handleReloadButtonClick = () => { + this.reloadLayerData(layerName, isHistogramAvailable, maybeFallbackLayerName); +}; +<div onClick={handleReloadButtonClick}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(7 hunks)
🔇 Additional comments (7)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (7)
417-421: Method signature extension
The extended signature with additional parametersisHistogramAvailableandmaybeFallbackLayerNameprovides more contextual control. Looks correct for reloading logic.
686-688: Consistent usage
PassingisHistogramAvailableand the fallback name consistently ensures thatgetReloadDataButtonis invoked with the correct context. No issues here.
991-992: Expression order
The condition ensures that a histogram is only considered if it's supported and the layer is a color layer. This check is straightforward and clear.
1002-1002: Parameter passing
PassingisHistogramAvailabledown to the header is consistent with the new signature, facilitating conditional logic. Good.
1012-1012: Guarded rendering of the histogram
This conditional check properly prevents rendering the histogram for unsupported layers or when it's disabled. Looks good.
1093-1097: Updated method signature
AddingisHistogramAvailableandmaybeFallbackLayerNamehere matches the design for controlling fallback-layer reload logic. Implementation flow is coherent.
1098-1099: Verify fallback usage
You clear the cache usingmaybeFallbackLayerName ?? layerNamebut reload the histogram solely forlayerName. If the fallback layer itself also requires a histogram reload, consider adding explicit logic and tests to confirm correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
39-39: Enhance the changelog entry with more details about the fix's impact.While the entry correctly describes the fix, consider expanding it to provide more context about the impact and benefits. This helps users understand the significance of the change.
-Fixed that reloading a layer's data from the server was sending the tracing ID instead of the fallback layer name to the datastore. [#8298](https://github.com/scalableminds/webknossos/pull/8298) +Fixed that reloading a layer's data from the server was sending the tracing ID instead of the fallback layer name to the datastore, which caused incorrect histogram route calls and 400 status errors for non-color layers. This fix ensures proper data reloading for segmentation layers. [#8298](https://github.com/scalableminds/webknossos/pull/8298)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (1)
1097-1100: Consider adding explicit error handling.While the implementation correctly handles the fallback layer and conditional histogram reloading, consider adding explicit error handling to provide better feedback to users when reloading fails.
reloadLayerData = async ( layerName: string, isHistogramAvailable: boolean, maybeFallbackLayerName: string | null, ): Promise<void> => { - await clearCache(this.props.dataset, maybeFallbackLayerName ?? layerName); - if (isHistogramAvailable) this.props.reloadHistogram(layerName); - await api.data.reloadBuckets(layerName); - Toast.success(`Successfully reloaded data of layer ${layerName}.`); + try { + await clearCache(this.props.dataset, maybeFallbackLayerName ?? layerName); + if (isHistogramAvailable) this.props.reloadHistogram(layerName); + await api.data.reloadBuckets(layerName); + Toast.success(`Successfully reloaded data of layer ${layerName}.`); + } catch (error) { + Toast.error(`Failed to reload data of layer ${layerName}.`); + throw error; // Re-throw to maintain the Promise rejection + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (4)
417-421: LGTM! Method signature changes improve layer reloading control.The addition of
isHistogramAvailableandmaybeFallbackLayerNameparameters provides better control over histogram reloading and fallback layer handling for segmentation layers.
425-429: LGTM! Implementation properly utilizes new parameters.The changes correctly pass the new parameters to
reloadLayerDatawhile maintaining the existing UI behavior.
1092-1096: LGTM! Method signature changes are consistent.The signature changes align with
getReloadDataButtonand maintain proper TypeScript typing.
991-991: LGTM! Proper histogram availability handling.The changes correctly determine histogram availability and pass it through the component hierarchy.
Also applies to: 1001-1001
URL of deployed dev instance (used for testing):
Steps to test:
/histograms)TODOs:
Issues:
/histogramroute was called for non-color layers from "reload data from server" button(Please delete unneeded items, merge only when none are left open)