-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking βSign up for GitHubβ, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvement: Add Organization Name Constraint #3196
Conversation
WalkthroughThe changes introduce a new Zod schema named Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the toolβs configuration or disable the tool if itβs a critical failure. π§ ESLint
backend/src/server/routes/v2/organization-router.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/server/routes/v1/organization-router.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/server/lib/schemas.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
Tip β‘π§ͺ Multi-step agentic review comment chat (experimental)
π Recent review detailsConfiguration used: CodeRabbit UI π Files selected for processing (8)
π§ Files skipped from review as they are similar to previous changes (6)
β° Context from checks skipped due to timeout of 90000ms (4)
π Additional comments (4)
β¨ Finishing Touches
πͺ§ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Nitpick comments (2)
frontend/src/components/organization/CreateOrgModal/CreateOrgModal.tsx (1)
12-15
: Frontend schema doesn't match backend constraints.The frontend form validation only checks if the name is non-empty but doesn't enforce the same constraints as the new backend
OrganizationNameSchema
, which requires alphanumeric characters with limited symbols and max length of 64 characters.Consider updating the schema to match backend constraints:
const schema = z .object({ - name: z.string().nonempty({ message: "Name is required" }) + name: z.string() + .trim() + .nonempty({ message: "Name is required" }) + .max(64, { message: "Name must be 64 or fewer characters" }) + .regex(/^[a-zA-Z0-9\-_\s]+$/, "Name can only contain alphanumeric characters, dashes, underscores, and spaces") }) .required();frontend/src/components/auth/UserInfoStep.tsx (1)
250-257
: Organization name field lacks client-side pattern validation.While the input has
maxLength={64}
which matches the backend constraint, it doesn't validate the character pattern (alphanumeric, dashes, underscores, spaces) enforced by the backend.Consider adding a pattern check or using a controlled input with validation before submission to provide immediate feedback to users.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (7)
backend/src/server/lib/schemas.ts
(1 hunks)backend/src/server/routes/v1/organization-router.ts
(2 hunks)backend/src/server/routes/v2/organization-router.ts
(2 hunks)backend/src/server/routes/v3/signup-router.ts
(2 hunks)frontend/src/components/auth/UserInfoStep.tsx
(2 hunks)frontend/src/components/organization/CreateOrgModal/CreateOrgModal.tsx
(1 hunks)frontend/src/hooks/api/reactQuery.tsx
(1 hunks)
β° Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Run integration test
- GitHub Check: Check API Changes
- GitHub Check: Check TS and Lint
π Additional comments (12)
frontend/src/components/organization/CreateOrgModal/CreateOrgModal.tsx (1)
80-81
:β Verification inconclusive
Modal property update may affect behavior.
The
modal={false}
property has been added to the Modal component. This property likely affects the modal's behavior, possibly disabling backdrop click-to-close functionality or changing how it interacts with the page.
π Script executed:
#!/bin/bash # Check Modal component implementation to understand impact of modal={false} fd --type file -e tsx -e jsx Modal.tsx | grep -v "node_modules" | xargs rg "modal(\s*)=(\s*)" -A 5 -B 5Length of output: 1180
Action: Verify Modal Behavior Adjustments
The addition of the
modal={false}
prop in<Modal modal={false} isOpen={isOpen}>
may change how the modal behaves (for instance, disabling backdrop click-to-close or altering interaction patterns). Please confirm that this behavior is intentional and update any related UI tests or documentation if necessary.
- Verify that the modified behavior matches design expectations.
- Double-check if any related modal functionality (like user interactions or other modal props) requires adjustment in light of this change.
backend/src/server/lib/schemas.ts (1)
25-30
: Good implementation of organization name constraints.The new
OrganizationNameSchema
properly enforces alphanumeric input with specific allowed symbols (dashes, underscores, spaces) and appropriate length constraints. This implementation aligns well with the PR objective.frontend/src/components/auth/UserInfoStep.tsx (2)
14-14
: Improved error handling with onRequestError.Adding the error handling function provides better user feedback when API requests fail, which is a good practice.
210-210
: Error handling implementation is correct.The
onRequestError
function is properly called in the catch block to handle any request errors during account signup.backend/src/server/routes/v2/organization-router.ts (2)
15-15
: Good schema import.Importing the centralized
OrganizationNameSchema
promotes consistency and reusability across routes.
334-334
: Proper schema enforcement in organization creation route.The organization creation endpoint now correctly uses the
OrganizationNameSchema
to validate input, enforcing the name constraints consistently with other parts of the application.backend/src/server/routes/v3/signup-router.ts (2)
7-7
: Added import for consistent organization name validation.Good addition of the
OrganizationNameSchema
import to enforce standardized validation for organization names.
104-104
: Improved validation for organization names.Great refactoring to use the centralized
OrganizationNameSchema
instead of the inline validation. This ensures consistent enforcement of organization name constraints across the application.backend/src/server/routes/v1/organization-router.ts (2)
16-16
: Added imports for schema validation.Good import of
OrganizationNameSchema
alongside the existingslugSchema
. This ensures consistent organization name validation.
254-254
: Standardized organization name validation.Excellent replacement of the inline validation with the centralized
OrganizationNameSchema
. This ensures that organization names are consistently validated across different API routes and maintains the optional nature of this field for PATCH operations.frontend/src/hooks/api/reactQuery.tsx (2)
25-187
: Refactored error handling into a reusable function.Great refactoring of the error handling logic into a separate
onRequestError
function. This improves code organization and makes the error handling more consistent across the application. The function handles validation errors, forbidden errors, and generic request errors with appropriate notifications.A few observations:
- The function provides detailed error information in modals for ValidationError and ForbiddenError
- It maintains request ID for all error types, which is helpful for debugging
- The error messages are formatted consistently
189-192
: Simplified mutation cache error handling.Good refactoring to use the extracted
onRequestError
function in the mutation cache configuration. This makes the code more concise and ensures consistent error handling.
Description π£
This PR adds a alphanumeric and limit symbol constraint to organization names.
Type β¨
Tests π οΈ
# Here's some code block to paste some code snippets
Summary by CodeRabbit