[WEB-2610] fix: workspace redirection from admin app#6122
Conversation
WalkthroughThis pull request introduces changes to two components: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (2)
admin/core/components/workspace/list-item.tsx (1)
Line range hint
74-78: Enhance accessibility for external link indicatorThe external link icon should have an aria-label to improve accessibility.
<div className="flex-shrink-0"> <ExternalLink size={14} + aria-label="Opens in new tab" className="text-custom-text-400 group-hover:text-custom-text-200" /> </div>admin/app/workspace/create/form.tsx (1)
Line range hint
129-156: Improve slug validation consistency and user feedbackThe current slug validation implementation has several areas for improvement:
- Validation logic is duplicated between the name field rules and slug onChange
- Multiple validation states (invalidSlug, slugError, form validation) might lead to confusing user feedback
- No immediate validation against RESTRICTED_URLS
Consider this improved implementation:
const WorkspaceCreateForm = () => { + const SLUG_REGEX = /^[a-zA-Z0-9_-]+$/; + const validateSlug = (value: string) => { + if (!SLUG_REGEX.test(value)) return "URLs can contain only (-), (_) and alphanumeric characters."; + if (RESTRICTED_URLS.includes(value)) return "This URL is reserved. Try something else."; + return true; + }; // ... existing code ... <Controller control={control} name="slug" rules={{ required: "The URL is a required field.", maxLength: { value: 48, message: "Limit your URL to 48 characters.", }, + validate: validateSlug }} render={({ field: { onChange, value, ref } }) => ( <Input id="workspaceUrl" type="text" value={value.toLocaleLowerCase().trim().replace(/ /g, "-")} onChange={(e) => { - if (/^[a-zA-Z0-9_-]+$/.test(e.target.value)) setInvalidSlug(false); - else setInvalidSlug(true); onChange(e.target.value.toLowerCase()); }} ref={ref} hasError={Boolean(errors.slug)} placeholder="workspace-name" className="block w-full rounded-md border-none bg-transparent !px-0 py-2 text-sm" /> )} />This refactor:
- Centralizes validation logic
- Removes redundant state
- Provides immediate feedback for reserved URLs
- Makes the validation rules more maintainable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
admin/app/workspace/create/form.tsx(2 hunks)admin/core/components/workspace/list-item.tsx(2 hunks)
🔇 Additional comments (2)
admin/core/components/workspace/list-item.tsx (1)
Line range hint 22-79: Verify workspace URL construction
The component uses WEB_BASE_URL for constructing workspace URLs. Let's verify this matches the expected behavior across the application.
✅ Verification successful
Workspace URL construction is consistent with application configuration
The verification shows that:
WEB_BASE_URLis consistently defined across all common helper files using the environment variableNEXT_PUBLIC_WEB_BASE_URL- The workspace URL construction in
list-item.tsxfollows the application's standard pattern - The environment variable is properly configured in all Dockerfile configurations
- No inconsistent workspace URL construction patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of WEB_BASE_URL in workspace URL construction
# Search for other instances of workspace URL construction
echo "Searching for workspace URL construction patterns..."
rg -A 2 "WEB_BASE_URL.*workspace\.slug" --type typescript
# Check if WEB_BASE_URL is consistently imported
echo "Checking WEB_BASE_URL imports..."
rg "import.*WEB_BASE_URL.*from.*helpers/common\.helper" --type typescript
Length of output: 395
Script:
#!/bin/bash
# Let's try without typescript file type restriction and search for WEB_BASE_URL usage
# Search for workspace URL construction patterns
echo "Searching for workspace URL construction patterns..."
rg -A 2 "WEB_BASE_URL.*workspace\.slug"
# Check WEB_BASE_URL imports and definitions
echo -e "\nChecking WEB_BASE_URL imports and definitions..."
rg "WEB_BASE_URL"
# Look for the common helper file that might define WEB_BASE_URL
echo -e "\nLooking for common helper file..."
fd "common.helper" --type f
Length of output: 2837
Script:
#!/bin/bash
# Let's check the content of common helper files to verify WEB_BASE_URL definition consistency
echo "Checking common helper files content..."
cat admin/helpers/common.helper.ts
echo -e "\n---\n"
cat space/helpers/common.helper.ts
echo -e "\n---\n"
cat web/helpers/common.helper.ts
# Let's also check for any other workspace URL construction patterns that might not use WEB_BASE_URL
echo -e "\nChecking for other workspace URL patterns..."
rg -A 2 "workspace\.slug.*href"
Length of output: 3522
admin/app/workspace/create/form.tsx (1)
Line range hint 44-71: Verify workspace creation error handling
The workspace creation flow should be tested to ensure proper error handling with the new URL validation.
✅ Verification successful
Workspace creation flow validation is properly implemented
The error handling in the workspace creation flow is correctly implemented with proper URL validation:
- The
RESTRICTED_URLSconstant contains a list of reserved slugs that cannot be used for workspaces - The
workspaceSlugCheckservice validates the slug against both the API and the restricted URLs list - The error handling flow includes:
- Slug validation before creation
- API-level error handling with proper error responses
- UI feedback through toast messages for both success and failure cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by the workspace URL changes
rg -l "createWorkspace|workspaceSlugCheck" --type typescript
# Check for any existing workspace URL validation patterns
ast-grep --pattern 'const $_ = /^[a-zA-Z0-9_-]+$/'
# Look for other uses of WEB_BASE_URL
rg "WEB_BASE_URL" --type typescript
Length of output: 221
Script:
#!/bin/bash
# Search for other components that might be affected by the workspace URL changes
rg -l "createWorkspace|workspaceSlugCheck"
# Check for any existing workspace URL validation patterns
rg "RESTRICTED_URLS" -A 2
# Look for workspace-related validation or URL handling
rg "workspaceSlug" -A 2
Length of output: 120224
Script:
#!/bin/bash
# Check for workspace slug validation patterns and restricted URLs
rg -A 5 "RESTRICTED_URLS" packages/constants/workspace.ts
# Look for workspace creation and slug check implementations
rg -A 10 "workspaceSlugCheck" web/core/services/workspace.service.ts admin/core/services/workspace.service.ts
Length of output: 1810
| <a | ||
| key={workspaceId} | ||
| href={`${WEB_BASE_URL}/${encodeURIComponent(workspace.slug)}`} | ||
| target="_blank" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security attributes for external link
The external link should include security attributes to protect against potential security vulnerabilities.
<a
key={workspaceId}
href={`${WEB_BASE_URL}/${encodeURIComponent(workspace.slug)}`}
target="_blank"
+ rel="noopener noreferrer"
className="group flex items-center justify-between p-4 gap-2.5 truncate border border-custom-border-200/70 hover:border-custom-border-200 hover:bg-custom-background-90 rounded-md"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a | |
| key={workspaceId} | |
| href={`${WEB_BASE_URL}/${encodeURIComponent(workspace.slug)}`} | |
| target="_blank" | |
| <a | |
| key={workspaceId} | |
| href={`${WEB_BASE_URL}/${encodeURIComponent(workspace.slug)}`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="group flex items-center justify-between p-4 gap-2.5 truncate border border-custom-border-200/70 hover:border-custom-border-200 hover:bg-custom-background-90 rounded-md" |
| // derived values | ||
| const workspaceBaseURL = encodeURI(WEB_BASE_URL || window.location.origin + "/"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving URL handling robustness
The current implementation has several potential issues:
- The fallback to
window.location.originwon't work during SSR - No handling of trailing slashes could lead to double slashes in the final URL
encodeURImight be unnecessary for the base URL and could cause issues with special characters
Consider this improved implementation:
- const workspaceBaseURL = encodeURI(WEB_BASE_URL || window.location.origin + "/");
+ const workspaceBaseURL = (WEB_BASE_URL || (typeof window !== 'undefined' ? window.location.origin : '')).replace(/\/?$/, '/');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // derived values | |
| const workspaceBaseURL = encodeURI(WEB_BASE_URL || window.location.origin + "/"); | |
| // derived values | |
| const workspaceBaseURL = (WEB_BASE_URL || (typeof window !== 'undefined' ? window.location.origin : '')).replace(/\/?$/, '/'); |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Linkcomponent with a standard anchor tag for external links.