fix/workspace name validation #5685
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared workspace name validation helper to ensure consistent validation across workspace creation and editing flows. The changes aim to address issue #5543 by centralizing validation logic and applying consistent trimming and minimum-length requirements.
Key Changes:
- Added a new shared validation module with
validateWorkspaceNameandsanitizeWorkspaceNamehelpers that enforce a 6-character minimum length requirement - Integrated the validation into both Add.vue and Edit.vue components to ensure consistent behavior across workspace creation and editing
- Added comprehensive unit tests covering edge cases like empty strings, whitespace-only input, and length constraints
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/hoppscotch-common/src/helpers/validateWorkspaceName.ts | New validation module implementing workspace name validation and sanitization logic with minimum length of 6 characters |
| packages/hoppscotch-common/src/helpers/tests/validateWorkspaceName.spec.ts | Unit tests for the validation helpers covering empty input, whitespace, length constraints, and trimming behavior |
| packages/hoppscotch-common/src/components/teams/Edit.vue | Integrated validation into the edit flow, refactored error handling, and converted forEach to for-of loop for role updates |
| packages/hoppscotch-common/src/components/teams/Add.vue | Integrated validation into the creation flow with early validation before codec processing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| toast.success(t("team.saved")) |
There was a problem hiding this comment.
The translation key "team.saved" does not exist in the locale files. Based on the locale file structure, this should use "workspace.saved" instead (which exists at line 1602 in locales/en.json). This will cause the toast message to display the raw key instead of a translated message.
| toast.success(t("team.saved")) | |
| toast.success(t("workspace.saved")) |
| } | ||
| } else { | ||
| toast.error(`${t("empty.team_name")}`) | ||
| toast.error(t("team.name_length_insufficient")) |
There was a problem hiding this comment.
The translation key "team.name_length_insufficient" references a message that in the English locale says "Workspace name should not be empty" (line 1588 in en.json), but this error path is now only reached when TeamNameCodec validation fails after the new validator has already passed. Since the validator ensures the name has at least 6 characters, reaching this line would be unexpected and the error message would be misleading.
| toast.error(t("team.name_length_insufficient")) | |
| toast.error(t("team.name_invalid")) |
| if (TeamNameCodec.is(editingName.value)) { | ||
| const updateTeamNameResult = await renameTeam( | ||
| props.editingTeamID, | ||
| editingName.value | ||
| )() | ||
| if (E.isLeft(updateTeamNameResult)) { | ||
| toast.error(t("error.something_went_wrong")) | ||
| } else { | ||
| // Apply pending role updates | ||
| for (const update of roleUpdates.value) { | ||
| const result = await updateTeamAccessRole( | ||
| update.userID, | ||
| props.editingTeamID, | ||
| update.role | ||
| )() | ||
| if (E.isLeft(result)) { | ||
| toast.error(t("error.something_went_wrong")) | ||
| console.error(result.left.error) | ||
| } | ||
| } | ||
| toast.success(t("team.saved")) | ||
| hideModal() | ||
| toast.success(`${t("team.saved")}`) | ||
| } else { | ||
| toast.error(`${t("team.name_length_insufficient")}`) | ||
| } | ||
| } else { | ||
| toast.error(`${t("empty.team_name")}`) | ||
| toast.error(t("team.name_length_insufficient")) | ||
| } |
There was a problem hiding this comment.
After the custom validation passes and the name is sanitized (lines 405-414), checking TeamNameCodec.is() is redundant since TeamNameCodec only checks if the trimmed string is not empty. The custom validator already ensures this with a minimum length of 6 characters. The else branch at line 442-444 should never be reached under normal circumstances, making the error handling there unreachable and confusing.
| if (trimmed.length === 0) | ||
| return { ok: false, message: "Workspace name cannot be empty." } | ||
|
|
||
| if (trimmed.length < 6) | ||
| return { ok: false, message: "Workspace name must be at least 6 characters long." } |
There was a problem hiding this comment.
The empty check is redundant because trimmed.length === 0 will be true for both empty strings and whitespace-only strings after trimming. The second condition (line 7) already handles the empty case since 0 is less than 6. Consider removing this redundant check or adjusting the logic to combine these validations.
| if (trimmed.length === 0) | |
| return { ok: false, message: "Workspace name cannot be empty." } | |
| if (trimmed.length < 6) | |
| return { ok: false, message: "Workspace name must be at least 6 characters long." } | |
| if (trimmed.length === 0) { | |
| return { ok: false, message: "Workspace name cannot be empty." } | |
| } else if (trimmed.length < 6) { | |
| return { ok: false, message: "Workspace name must be at least 6 characters long." } | |
| } |
| const validation = validateWorkspaceName(sanitized) | ||
| if (!validation.ok) { | ||
| toast.error(validation.message) |
There was a problem hiding this comment.
The validation.message could potentially be undefined according to the validateWorkspaceName return type, but you're passing it directly to toast.error(). While the current implementation guarantees a message when ok is false, it would be safer to handle the potential undefined case or update the return type to make message required when ok is false.
| toast.error(validation.message) | |
| toast.error(validation.message ?? t("error.something_went_wrong")) |
| const validation = validateWorkspaceName(sanitized) | ||
| if (!validation.ok) { | ||
| toast.error(validation.message) |
There was a problem hiding this comment.
The validation.message could potentially be undefined according to the validateWorkspaceName return type, but you're passing it directly to toast.error(). While the current implementation guarantees a message when ok is false, it would be safer to handle the potential undefined case or update the return type to make message required when ok is false.
| toast.error(validation.message) | |
| toast.error(validation.message ?? t("team.invalid_name")) |
| @@ -0,0 +1,15 @@ | |||
| export function validateWorkspaceName(name: string): { ok: boolean; message?: string } { | |||
There was a problem hiding this comment.
The return type allows message to be undefined even when ok is false, but the implementation always provides a message for error cases. Consider using a discriminated union type to make this explicit, such as: { ok: true } | { ok: false; message: string }. This would eliminate the optional message property and provide better type safety.
| export function validateWorkspaceName(name: string): { ok: boolean; message?: string } { | |
| export function validateWorkspaceName( | |
| name: string | |
| ): { ok: true } | { ok: false; message: string } { |
Closes #5543
This PR introduces a shared workspace name validator and applies consistent trimming and minimum-length validation across all create/edit flows. It addresses the inconsistency described in Issue #5543. End-to-end testing is still in progress, so this PR is currently marked as WIP.
What's changed
validateWorkspaceNameandsanitizeWorkspaceNamehelperAdd.vueandEdit.vueSummary by cubic
Unified workspace name validation across create and edit flows to fix inconsistent checks and prevent invalid names. Addresses #5543.
Written for commit 04aa2d3. Summary will update automatically on new commits.