Disallow empty annotation layer names and names starting with a '.' #8244
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements to validation logic within the frontend components of the application. Key changes include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 1
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/oxalis/view/components/editable_text_label.tsx (1)
122-126: Enhance validation logic robustnessWhile the basic implementation works, consider these improvements:
- Use the rule's
messageproperty if provided, falling back to the default message- Add null/undefined value handling
- Consider adding max length validation for completeness
Here's a suggested implementation:
} else if (rule.min != null) { - if (this.state.value.length < rule.min) { - Toast.error(`Length must at least be ${rule.min}.`); + const value = this.state.value; + if (!value || value.length < rule.min) { + Toast.error(rule.message ?? `${this.props.label} length must be at least ${rule.min} characters.`); return false; } }frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (2)
Line range hint
752-761: Consider making the minimum length validation more robust.The current implementation has potential fragility issues:
- Direct array indexing with
[0]is brittle- Type assertion
as { min: number }suggests potential type safety issuesConsider refactoring to:
-layerNameRules[0] as { min: number }, // Ensuring minimum length +const [minLengthRule] = layerNameRules; +minLengthRule as { min: number }, // Ensuring minimum lengthOr better yet, improve the type definitions in
dataset_components.tsto avoid the type assertion.
761-768: Consider extracting the dot validation rule for reusability.The validation logic is clear but could be made reusable since this rule might be needed in other components.
Consider extracting the validation to a shared utility:
+// In a shared validation utility file +export const validateNoLeadingDot = (value: string) => ({ + isValid: !value.startsWith("."), + message: "The name must not start with a dot." +}); // In this file -{ - validator: (newReadableLayerName) => { - const startsWithADot = newReadableLayerName.startsWith("."); - return startsWithADot - ? { isValid: false, message: "The name must not start with a dot." } - : { isValid: true, message: "" }; - }, -}, +{ + validator: validateNoLeadingDot, +},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/oxalis/view/components/editable_text_label.tsx(2 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(3 hunks)
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/components/editable_text_label.tsx (1)
13-13: LGTM: Type definition change is well-structured
The addition of the optional min property to the Rule type is clean and maintains backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
27-27: Fix typo and improve clarity in changelog entry.The changelog entry contains a typo and could be clearer by separating the two validation rules.
-Fix that the frontend did not ensure a minium length for annotation layer names. Moreover, names starting with a `.` are also disallowed now. [#8244](https://github.com/scalableminds/webknossos/pull/8244) +Fix frontend validation for annotation layer names by enforcing a minimum length and disallowing names that start with a '.'. [#8244](https://github.com/scalableminds/webknossos/pull/8244)
| @@ -748,6 +749,7 @@ class DatasetSettings extends React.PureComponent<DatasetSettingsProps, State> { | |||
| }); | |||
| }} | |||
| rules={[ | |||
| layerNameRules[0] as { min: number }, // Ensuring minimum length | |||
| { | |||
| validator: (newReadableLayerName) => | |||
| validateReadableLayerName( | |||
There was a problem hiding this comment.
I think the additional rules should be implemented within validateReadableLayerName (or maybe checkLayerNameForInvalidCharacters for the .).
There was a problem hiding this comment.
sure thanks for the suggestion. Should be implemented now :)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx (2)
44-50: LGTM! Consider adding JSDoc for the validation rule.The implementation correctly prevents layer names from starting with a dot. Consider adding a JSDoc comment explaining why this restriction exists (e.g., if it's to prevent hidden files/layers or maintain consistency with dataset layer names).
+/** + * Validates that the layer name doesn't start with a dot and contains only URI-safe characters. + * The dot restriction aligns with dataset layer name validation rules to prevent hidden layers. + */ export function checkLayerNameForInvalidCharacters(readableLayerName: string): ValidationResult {
72-77: LGTM! Consider adding input trimming.The minimum length validation is correctly implemented. Consider trimming whitespace from the input to prevent names that appear empty but contain spaces.
export function validateReadableLayerName( readableLayerName: string, allReadableLayerNames: string[], nameNotToCount?: string, ): ValidationResult { + const trimmedName = readableLayerName.trim(); + if (trimmedName.length < 1) { - if (readableLayerName.length < 1) { return { isValid: false, message: messages["tracing.volume_layer_name_too_short"], }; }frontend/javascripts/messages.tsx (1)
215-216: LGTM with a minor suggestion for consistency.The new validation messages are well-placed within the tracing-related messages section and follow the established message format. However, for better consistency with other validation messages in the file, consider using sentence case for both messages.
- "tracing.volume_layer_name_too_short": "The layer name must be at least one character long.", - "tracing.volume_layer_name_starts_with_dot": "The layer name must not start with a dot.", + "tracing.volume_layer_name_too_short": "Layer name must be at least one character long.", + "tracing.volume_layer_name_starts_with_dot": "Layer name must not start with a dot.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx(2 hunks)
🔇 Additional comments (1)
frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx (1)
Line range hint 44-77: Implementation successfully meets PR objectives.
The changes successfully implement the required validation rules for annotation layer names:
- Names cannot be empty (minimum length of 1 character)
- Names cannot start with a dot
- Error messages are properly externalized for i18n support
The implementation aligns well with the dataset layer name restrictions as mentioned in the PR objectives.
Let's verify that these validation rules are consistent with dataset layer validation:
✅ Verification successful
Frontend validation aligns with backend dataset layer restrictions
The implementation is correctly aligned with the backend validation rules found in DatasetService.scala, which also enforces:
- Names cannot start with a dot (
!name.startsWith(".")) - Similar validation exists in the admin dataset components
The frontend validation is actually more comprehensive as it also includes:
- URI-safe character validation
- Empty name check
- Proper error messaging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dataset layer name validation rules
# Look for similar validation patterns in dataset-related files
# Search for dataset layer name validation
rg -A 5 "validateLayerName|validateDatasetName|layerNameValidation" --type typescript
# Search for similar dot-prefix validation
ast-grep --pattern 'startsWith(".")' --lang typescript
Length of output: 179
Script:
#!/bin/bash
# Let's try different approaches to find validation patterns
# Search for layer name validation patterns without file type restriction
rg -A 5 "validateLayerName|validateDatasetName|layerNameValidation"
# Search for dot prefix validation without language restriction
rg -A 5 "startsWith\\(\"\\.\"\\)"
# Look for similar validation logic in the codebase
rg -A 5 "isValid.*message"
Length of output: 3186
This pr adds frontend validation to annotation layer names and tries to put the same restrictions to the annotation layer names as to "normal" dataset layer names.
Sadly, it is not easily possible to reuse the rules set for dataset layer names, as these are formulated as antd form validation rules and the annotation layer name setting uses an
EditableTextLabel. TheEditableTextLabelhas its own format for validation rules which is not directly compatible with the antd variation -> Therefore I duplicated the two missing rules:.URL of deployed dev instance (used for testing):
Steps to test:
.-> frontend validation should failIssues:
(Please delete unneeded items, merge only when none are left open)