Exploring remote datasets: drop neuroglancer prefixes from URIs #8195
Conversation
WalkthroughThe pull request introduces several enhancements and bug fixes to the WEBKNOSSOS project. A new feature allows users to explore remote URIs from Neuroglancer without needing to manually remove format prefixes, improving user experience. The image file reading process has been modified for asynchronous operation. Key bugs have been addressed, including performance issues during tree deletions and fixes related to NML file imports and API node deletions. No breaking changes were introduced. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (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 (
|
![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
🧹 Outside diff range and nitpick comments (6)
CHANGELOG.unreleased.md (1)
14-14: Consider adding deployment requirements.Since this change requires a datastore update after deployment (as mentioned in the PR objectives), consider adding this information to help with deployment planning.
- When exploring remote URIs pasted from Neuroglancer, the format prefixes like `precomputed://` are now ignored, so users don't have to remove them. [#8195](https://github.com/scalableminds/webknossos/pull/8195) + When exploring remote URIs pasted from Neuroglancer, the format prefixes like `precomputed://` are now ignored, so users don't have to remove them. Requires datastore update after deployment. [#8195](https://github.com/scalableminds/webknossos/pull/8195)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala (1)
148-149: Add documentation for the new method.The implementation aligns well with the PR objectives and correctly handles multiple format prefixes. Consider adding ScalaDoc to document the method's purpose and behavior.
+ /** Removes format-specific prefixes from URIs (e.g., "precomputed://", "zarr3://"). + * This simplifies the handling of Neuroglancer URLs by normalizing the URI format. + * + * @param uri The URI string potentially containing format prefixes + * @return The URI with any format prefixes removed + */ def removeNeuroglancerPrefixesFromUri(uri: String): String =webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (1)
80-80: Consider adding test cases for various Neuroglancer URI formats.To ensure robust handling of all Neuroglancer URI formats, consider adding test cases that cover:
- URIs with "precomputed://" prefix
- URIs with other Neuroglancer-specific prefixes
- URIs without any prefixes (to verify no unintended modifications)
Would you like me to help generate comprehensive test cases for the URI processing logic?
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (3)
486-494: Consider making prefix removal more robustThe current implementation sequentially removes prefixes, which could potentially lead to unexpected behavior if multiple prefixes are present. Consider:
- Making the prefix removal mutually exclusive
- Adding a warning or log when multiple prefixes are detected
Here's a suggested improvement:
- const removePrefix = (value: string, prefix: string) => - value.startsWith(prefix) ? value.slice(prefix.length) : value; - - // If pasted from neuroglancer, uris have these prefixes even before the protocol. The backend ignores them. - userInput = removePrefix(userInput, "zarr://"); - userInput = removePrefix(userInput, "zarr3://"); - userInput = removePrefix(userInput, "n5://"); - userInput = removePrefix(userInput, "precomputed://"); + const prefixes = ["zarr://", "zarr3://", "n5://", "precomputed://"]; + const matchingPrefix = prefixes.find(prefix => userInput.startsWith(prefix)); + if (matchingPrefix) { + // Log which prefix was removed for debugging purposes + console.debug(`Removing ${matchingPrefix} prefix from URI`); + userInput = userInput.slice(matchingPrefix.length); + }
Line range hint
673-687: Enhance error handling in URL validationThe current error handling in the validation rule could be more informative. Consider capturing and displaying both the prefix removal and protocol validation errors separately.
Here's a suggested improvement:
{ validator: (_rule, value) => { try { validateUrls(value); return Promise.resolve(); } catch (e) { handleFailure(); - return Promise.reject(e); + // Separate prefix and protocol validation errors + if (e.message.includes('protocols')) { + return Promise.reject('Invalid protocol. Please use https://, http://, s3://, gs:// or file://'); + } + return Promise.reject(`Invalid URL format: ${e.message}`); } }, },
486-494: Add documentation and tests for prefix removalWhile the implementation is solid, it would benefit from:
- JSDoc documentation explaining the prefix removal behavior
- Unit tests covering various prefix scenarios
Consider adding documentation and tests:
+ /** + * Validates and normalizes dataset URLs by: + * 1. Removing format-specific prefixes (zarr://, n5://, precomputed://) + * 2. Validating the protocol (https, s3, gs, file) + * + * @param userInput The URL string to validate + * @throws Error if the protocol is invalid + */ function validateUrls(userInput: string) {Also, consider adding test cases in your test suite:
describe('validateUrls', () => { it('should remove zarr prefix', () => { expect(validateUrls('zarr://https://example.com')).toBe('https://example.com'); }); it('should remove precomputed prefix', () => { expect(validateUrls('precomputed://https://example.com')).toBe('https://example.com'); }); it('should handle multiple prefixes', () => { expect(validateUrls('zarr://precomputed://https://example.com')).toBe('https://example.com'); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala(1 hunks)
🔇 Additional comments (3)
CHANGELOG.unreleased.md (1)
14-14: LGTM! Clear and well-formatted changelog entry.
The entry follows the Keep a Changelog format and clearly describes the new feature with the correct PR reference.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala (1)
140-147: LGTM! Clean refactoring of suffix removal.
The refactoring to use stripSuffix method chaining is a cleaner and more maintainable approach. The implementation correctly handles all the necessary file formats.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (1)
80-80: LGTM! Please verify the implementation of removeNeuroglancerPrefixesFromUri.
The change aligns with the PR objective of removing Neuroglancer prefixes from URIs. The composition with removeHeaderFileNamesFromUriSuffix maintains the existing URI processing logic while adding the new functionality.
Let's verify the implementation of removeNeuroglancerPrefixesFromUri:
✅ Verification successful
Implementation of removeNeuroglancerPrefixesFromUri is correct and complete
The implementation correctly handles all common Neuroglancer prefixes by stripping:
zarr3://zarr://precomputed://n5://
The method is simple, well-focused, and its composition with removeHeaderFileNamesFromUriSuffix in the correct order ensures proper URI processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the implementation and usage of removeNeuroglancerPrefixesFromUri
# Find the implementation
ast-grep --pattern 'def removeNeuroglancerPrefixesFromUri'
# Find all usages
rg 'removeNeuroglancerPrefixesFromUri'
Length of output: 509
Script:
#!/bin/bash
# Let's check the implementation details of removeNeuroglancerPrefixesFromUri in ExploreLayerUtils.scala
cat webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala
Length of output: 6902
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.unreleased.md (1)
14-14: Consider enhancing the changelog entry with more details.While the entry is clear and follows the format guidelines, consider adding more context to better document this UX improvement:
-- When exploring remote URIs pasted from Neuroglancer, the format prefixes like `precomputed://` are now ignored, so users don't have to remove them. [#8195](https://github.com/scalableminds/webknossos/pull/8195) ++ When exploring remote URIs pasted from Neuroglancer, format prefixes (e.g., `precomputed://`, `zarr3://`) are now automatically removed, improving the UX when importing datasets. [#8195](https://github.com/scalableminds/webknossos/pull/8195), fixes [#7984](https://github.com/scalableminds/webknossos/issues/7984)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (2)
486-494: Consider enhancing prefix removal implementation.The prefix removal logic looks good but could benefit from some improvements:
- Consider adding more specific documentation about the handled prefixes.
- Consider handling cases where multiple prefixes might be present.
- // If pasted from neuroglancer, uris have these prefixes even before the protocol. The backend ignores them. + // Remove format-specific prefixes that might be present in URIs pasted from various sources: + // - Neuroglancer: precomputed:// + // - Zarr: zarr://, zarr3:// + // - N5: n5:// + // These prefixes appear before the protocol (http://, https://, etc.) and are ignored by the backend. userInput = removePrefix(userInput, "zarr://"); userInput = removePrefix(userInput, "zarr3://"); userInput = removePrefix(userInput, "n5://"); userInput = removePrefix(userInput, "precomputed://");
Line range hint
504-510: Enhance error message for invalid protocols.The error message could be more user-friendly and provide examples.
- "Dataset URL must employ one of the following protocols: https://, http://, s3://, gs:// or file://", + "Invalid dataset URL format. The URL must start with one of these protocols:\n" + + "- Web URLs: https://, http://\n" + + "- Cloud Storage: s3://, gs://\n" + + "- Local files: file://\n" + + "Example: https://example.com/dataset"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala(1 hunks)
🔇 Additional comments (5)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLayerUtils.scala (2)
140-147: LGTM! Clean refactoring using method chaining.
The refactored implementation is more readable and maintainable, properly handling all necessary header file names.
148-149: Implementation looks good. Verify prefix coverage.
The method cleanly handles multiple Neuroglancer prefixes using method chaining. The implementation is safe and handles edge cases well.
Let's verify if there are any other Neuroglancer prefixes we should handle:
✅ Verification successful
Let me analyze the search results to verify if there are any other Neuroglancer prefixes we should handle. The search revealed several URI prefixes in use:
gs://- Google Cloud Storage prefixs3://- Amazon S3 prefixzarr3://- Already handled in the methodzarr://- Already handled in the methodprecomputed://- Already handled in the methodn5://- Already handled in the method
Let me verify if there are any other Neuroglancer-specific prefixes by searching the codebase more specifically:
Implementation is complete and covers all Neuroglancer prefixes
The current implementation in removeNeuroglancerPrefixesFromUri correctly handles all Neuroglancer-specific URI prefixes. The method strips:
zarr3://- For Zarr v3 formatzarr://- For Zarr v2 formatprecomputed://- For Neuroglancer precomputed formatn5://- For N5 format
The other prefixes found in the codebase (gs:// and s3://) are cloud storage protocol prefixes, not Neuroglancer-specific prefixes, and should not be stripped as they are needed for actual data access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential URI prefixes in the codebase that might need to be handled
rg -g '!*.{json,md,txt}' -B 2 -A 2 '(://|[^a-zA-Z]://).*data'
Length of output: 26400
Script:
#!/bin/bash
# Search for Neuroglancer-related URI handling code
rg -g '!*.{json,md,txt}' -B 2 -A 2 'neuroglancer.*://|://.*neuroglancer'
Length of output: 11161
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (2)
Line range hint 90-100: Verify URI handling for all supported formats.
The URI transformation occurs before the explorer selection, which means it affects all supported formats (NgffV0_4, NgffV0_5, WebknossosZarr, Zarr3Array, ZarrArray, N5Multiscales, N5Array, Precomputed, NeuroglancerUri). Please ensure that:
- The prefix removal doesn't affect URIs that don't have Neuroglancer prefixes
- The order of explorers still maintains the correct priority
- Error reporting remains accurate for malformed URIs
Let's verify the handling of different URI formats:
#!/bin/bash
# Description: Search for test cases covering different URI formats
# Look for test cases covering URI handling
rg -A 5 "test.*Uri.*" --glob "*Test.scala"
# Look for URI format examples in tests
rg -A 2 "(zarr|n5|precomputed)://" --glob "*Test.scala"80-80: LGTM! Please verify the implementation details.
The change aligns well with the PR objectives to handle Neuroglancer URLs by removing prefixes. The URI processing chain looks correct, applying removeHeaderFileNamesFromUriSuffix before removing Neuroglancer prefixes.
Let's verify the implementation of removeNeuroglancerPrefixesFromUri:
✅ Verification successful
Implementation verified and looks correct
The implementation of removeNeuroglancerPrefixesFromUri in ExploreLayerUtils.scala is straightforward and correct:
- It removes common Neuroglancer prefixes: "zarr3://", "zarr://", "precomputed://", and "n5://"
- The order of operations in the original code is appropriate, as it removes header file names before stripping Neuroglancer prefixes
- The implementation uses
stripPrefixwhich safely handles cases where the prefix doesn't exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the implementation of removeNeuroglancerPrefixesFromUri and verify its behavior
# Search for the implementation
ast-grep --pattern 'def removeNeuroglancerPrefixesFromUri'
# Search for test cases
rg -l "removeNeuroglancerPrefixesFromUri.*Test"
Length of output: 494
Script:
#!/bin/bash
# Let's try a different approach to find the implementation and tests
# Search for the method implementation using ripgrep
rg -A 5 "def removeNeuroglancerPrefixesFromUri"
# Search for any file containing the method name
rg -l "removeNeuroglancerPrefixesFromUri"
# Search for test files containing URI-related tests
fd -e scala -e java . | rg "Test" | xargs rg -l "URI"
Length of output: 937
frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx (1)
Line range hint 486-510: Implementation successfully addresses PR objectives.
The changes effectively handle the removal of Neuroglancer prefixes while maintaining robust URL validation. The implementation:
- Removes all required prefixes (zarr://, zarr3://, n5://, precomputed://)
- Properly validates protocols
- Integrates well with existing code
URL of deployed dev instance (used for testing):
Steps to test:
zarr3://https://data-humerus.webknossos.org/data/zarr/scalable_minds/l4dense_motta_et_al_demo/colorIssues:
Summary by CodeRabbit