fix(platform): require organization name (#1951)#2101
Open
larryro wants to merge 2 commits into
Open
Conversation
Contributor
📝 Walkthrough
WalkthroughThe PR enforces that organization names are non-empty at every layer. The Convex Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@services/platform/app/features/settings/organization/components/organization-settings.test.tsx`:
- Around line 103-105: Remove or replace hardcoded English string literals from
the validation schema and test assertions in the organizationSettingsSchema
definition and throughout the test file. Instead of hardcoding English
user-facing strings like 'Organization name is required' and other validation
error messages at the specified locations (lines 103-105, 133-135, 140-141, and
159-160), either use the platform's i18n translation layer to fetch the actual
translated strings, or restructure the test logic to avoid string literal
comparisons entirely. This ensures the test respects the platform's
internationalization rules and won't break when translation content changes.
- Around line 127-162: The test suite for OrganizationSettingsView name
validation is missing an edge-case test for whitespace-only input. Add a new
test within the describe block that follows the pattern of the existing tests
and validates that entering only whitespace characters (e.g., ' ') in the
orgNameField triggers the same validation error as an empty string and results
in an invalid form state, ensuring the trim functionality is properly tested as
part of the PR's requirements.
In `@services/platform/convex/auth.ts`:
- Around line 711-724: The organization name validation logic is duplicated
between auth.ts and organization-settings.tsx, creating a maintenance risk.
Extract the validation rule that checks for empty or whitespace-only names into
a shared Zod schema file located in services/platform/lib/shared/schemas/.
Define the schema with the trim and minimum length validation (matching the
existing logic where rawName.trim().length === 0 is rejected). Then replace the
manual validation block in auth.ts (the section checking typeof rawName !==
'string' and rawName.trim().length === 0) with a call to parse or parseAsync on
the shared schema, passing the orgPatch object. Ensure the same shared schema is
imported and used on the client side in organization-settings.tsx to maintain a
single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc63e0e3-fb0d-4e1a-bc06-a3b523b5dfa4
📒 Files selected for processing (7)
services/platform/app/features/settings/components/settings-row.tsxservices/platform/app/features/settings/organization/components/organization-settings.test.tsxservices/platform/app/features/settings/organization/components/organization-settings.tsxservices/platform/convex/auth.tsservices/platform/messages/de.jsonservices/platform/messages/en.jsonservices/platform/messages/fr.json
Comment on lines
+103
to
+105
| name: z.string().trim().min(1, 'Organization name is required'), | ||
| defaultLocale: z.string(), | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid English literals in validation schema and assertions.
This test hardcodes English user-facing text, which violates the platform i18n rule for tests and will make locale-aware changes brittle.
As per coding guidelines: “Every user-facing string goes through the translation layer; never compare against an English literal in code, tests, or stories.”
Suggested fix
+import en from '`@/messages/en.json`';
const organizationSchema = z.object({
- name: z.string().trim().min(1, 'Organization name is required'),
+ name: z.string().trim().min(1, en.settings.organization.nameRequired),
defaultLocale: z.string(),
});
+const organizationNameLabel = en.settings.organization.title;
+const nameRequiredMessage = en.settings.organization.nameRequired;
const orgNameField = screen.getByRole('textbox', {
- name: 'Organization name',
+ name: organizationNameLabel,
});
await waitFor(() =>
- expect(screen.getByText('Organization name is required')).toBeInTheDocument(),
+ expect(screen.getByText(nameRequiredMessage)).toBeInTheDocument(),
);
expect(
- screen.queryByText('Organization name is required'),
+ screen.queryByText(nameRequiredMessage),
).not.toBeInTheDocument();Also applies to: 133-135, 140-141, 159-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/platform/app/features/settings/organization/components/organization-settings.test.tsx`
around lines 103 - 105, Remove or replace hardcoded English string literals from
the validation schema and test assertions in the organizationSettingsSchema
definition and throughout the test file. Instead of hardcoding English
user-facing strings like 'Organization name is required' and other validation
error messages at the specified locations (lines 103-105, 133-135, 140-141, and
159-160), either use the platform's i18n translation layer to fetch the actual
translated strings, or restructure the test logic to avoid string literal
comparisons entirely. This ensures the test respects the platform's
internationalization rules and won't break when translation content changes.
Source: Coding guidelines
Comment on lines
+127
to
+162
| describe('OrganizationSettingsView name validation', () => { | ||
| it('blocks an empty org name with an inline error and an invalid form', async () => { | ||
| render(<ValidationHarness orgName="Acme" />); | ||
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | ||
| expect(holder.current?.isValid).toBe(true); | ||
|
|
||
| const orgNameField = screen.getByRole('textbox', { | ||
| name: 'Organization name', | ||
| }); | ||
| fireEvent.change(orgNameField, { target: { value: '' } }); | ||
|
|
||
| await waitFor(() => | ||
| expect( | ||
| screen.getByText('Organization name is required'), | ||
| ).toBeInTheDocument(), | ||
| ); | ||
| expect(holder.current?.isValid).toBe(false); | ||
| }); | ||
|
|
||
| it('clears the error once a non-empty name is entered', async () => { | ||
| render(<ValidationHarness orgName="Acme" />); | ||
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | ||
|
|
||
| const orgNameField = screen.getByRole('textbox', { | ||
| name: 'Organization name', | ||
| }); | ||
| fireEvent.change(orgNameField, { target: { value: '' } }); | ||
| await waitFor(() => expect(holder.current?.isValid).toBe(false)); | ||
|
|
||
| fireEvent.change(orgNameField, { target: { value: 'New name' } }); | ||
| await waitFor(() => expect(holder.current?.isValid).toBe(true)); | ||
| expect( | ||
| screen.queryByText('Organization name is required'), | ||
| ).not.toBeInTheDocument(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add an explicit whitespace-only edge-case test.
The new suite covers error ('') and happy path ('New name'), but it misses the trim edge (' ') that this PR is specifically enforcing.
As per coding guidelines: “Tests carry the change: unit (happy + one edge + one error).”
Suggested test addition
describe('OrganizationSettingsView name validation', () => {
+ it('treats whitespace-only org name as invalid', async () => {
+ render(<ValidationHarness orgName="Acme" />);
+ await waitFor(() => expect(holder.current?.isLoading).toBe(false));
+
+ const orgNameField = screen.getByRole('textbox', {
+ name: en.settings.organization.title,
+ });
+ fireEvent.change(orgNameField, { target: { value: ' ' } });
+
+ await waitFor(() =>
+ expect(
+ screen.getByText(en.settings.organization.nameRequired),
+ ).toBeInTheDocument(),
+ );
+ expect(holder.current?.isValid).toBe(false);
+ });
+
it('blocks an empty org name with an inline error and an invalid form', async () => {📝 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.
Suggested change
| describe('OrganizationSettingsView name validation', () => { | |
| it('blocks an empty org name with an inline error and an invalid form', async () => { | |
| render(<ValidationHarness orgName="Acme" />); | |
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | |
| expect(holder.current?.isValid).toBe(true); | |
| const orgNameField = screen.getByRole('textbox', { | |
| name: 'Organization name', | |
| }); | |
| fireEvent.change(orgNameField, { target: { value: '' } }); | |
| await waitFor(() => | |
| expect( | |
| screen.getByText('Organization name is required'), | |
| ).toBeInTheDocument(), | |
| ); | |
| expect(holder.current?.isValid).toBe(false); | |
| }); | |
| it('clears the error once a non-empty name is entered', async () => { | |
| render(<ValidationHarness orgName="Acme" />); | |
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | |
| const orgNameField = screen.getByRole('textbox', { | |
| name: 'Organization name', | |
| }); | |
| fireEvent.change(orgNameField, { target: { value: '' } }); | |
| await waitFor(() => expect(holder.current?.isValid).toBe(false)); | |
| fireEvent.change(orgNameField, { target: { value: 'New name' } }); | |
| await waitFor(() => expect(holder.current?.isValid).toBe(true)); | |
| expect( | |
| screen.queryByText('Organization name is required'), | |
| ).not.toBeInTheDocument(); | |
| }); | |
| }); | |
| describe('OrganizationSettingsView name validation', () => { | |
| it('treats whitespace-only org name as invalid', async () => { | |
| render(<ValidationHarness orgName="Acme" />); | |
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | |
| const orgNameField = screen.getByRole('textbox', { | |
| name: 'Organization name', | |
| }); | |
| fireEvent.change(orgNameField, { target: { value: ' ' } }); | |
| await waitFor(() => | |
| expect( | |
| screen.getByText('Organization name is required'), | |
| ).toBeInTheDocument(), | |
| ); | |
| expect(holder.current?.isValid).toBe(false); | |
| }); | |
| it('blocks an empty org name with an inline error and an invalid form', async () => { | |
| render(<ValidationHarness orgName="Acme" />); | |
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | |
| expect(holder.current?.isValid).toBe(true); | |
| const orgNameField = screen.getByRole('textbox', { | |
| name: 'Organization name', | |
| }); | |
| fireEvent.change(orgNameField, { target: { value: '' } }); | |
| await waitFor(() => | |
| expect( | |
| screen.getByText('Organization name is required'), | |
| ).toBeInTheDocument(), | |
| ); | |
| expect(holder.current?.isValid).toBe(false); | |
| }); | |
| it('clears the error once a non-empty name is entered', async () => { | |
| render(<ValidationHarness orgName="Acme" />); | |
| await waitFor(() => expect(holder.current?.isLoading).toBe(false)); | |
| const orgNameField = screen.getByRole('textbox', { | |
| name: 'Organization name', | |
| }); | |
| fireEvent.change(orgNameField, { target: { value: '' } }); | |
| await waitFor(() => expect(holder.current?.isValid).toBe(false)); | |
| fireEvent.change(orgNameField, { target: { value: 'New name' } }); | |
| await waitFor(() => expect(holder.current?.isValid).toBe(true)); | |
| expect( | |
| screen.queryByText('Organization name is required'), | |
| ).not.toBeInTheDocument(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/platform/app/features/settings/organization/components/organization-settings.test.tsx`
around lines 127 - 162, The test suite for OrganizationSettingsView name
validation is missing an edge-case test for whitespace-only input. Add a new
test within the describe block that follows the pattern of the existing tests
and validates that entering only whitespace characters (e.g., ' ') in the
orgNameField triggers the same validation error as an empty string and results
in an invalid form state, ensuring the trim functionality is properly tested as
part of the PR's requirements.
Source: Coding guidelines
Comment on lines
+711
to
+724
| // Org name is required: reject a rename that clears it (empty or | ||
| // whitespace-only). Mirrors the client-side `.min(1)` validation so | ||
| // an empty name can't slip past the auth boundary via a direct API | ||
| // call. Only enforced when `name` is part of the patch — a name-less | ||
| // update (e.g. locale-only) leaves the stored name untouched. | ||
| const rawName = orgPatch.name; | ||
| if (rawName !== undefined) { | ||
| if (typeof rawName !== 'string' || rawName.trim().length === 0) { | ||
| throw new APIError('BAD_REQUEST', { | ||
| message: 'Organization name is required.', | ||
| }); | ||
| } | ||
| orgPatch.name = rawName.trim(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Unify organization-name validation through one shared Zod schema.
Line 711 introduces a second, manual validator for name while the same rule is already defined in organization-settings.tsx (Line 329). This can drift across layers; move the rule to a shared schema and consume it in both client form validation and this hook.
♻️ Suggested direction
+// shared (e.g. services/platform/lib/shared/schemas/organization.ts)
+export const organizationNameSchema = z.string().trim().min(1);
// services/platform/convex/auth.ts
-const rawName = orgPatch.name;
-if (rawName !== undefined) {
- if (typeof rawName !== 'string' || rawName.trim().length === 0) {
- throw new APIError('BAD_REQUEST', {
- message: 'Organization name is required.',
- });
- }
- orgPatch.name = rawName.trim();
-}
+if (orgPatch.name !== undefined) {
+ const parsed = organizationNameSchema.safeParse(orgPatch.name);
+ if (!parsed.success) {
+ throw new APIError('BAD_REQUEST', {
+ message: 'Organization name is required.',
+ });
+ }
+ orgPatch.name = parsed.data;
+}As per coding guidelines, "Validate at boundaries with Zod; shared schemas in services/platform/lib/shared/schemas/, imported on both client and server."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/platform/convex/auth.ts` around lines 711 - 724, The organization
name validation logic is duplicated between auth.ts and
organization-settings.tsx, creating a maintenance risk. Extract the validation
rule that checks for empty or whitespace-only names into a shared Zod schema
file located in services/platform/lib/shared/schemas/. Define the schema with
the trim and minimum length validation (matching the existing logic where
rawName.trim().length === 0 is rejected). Then replace the manual validation
block in auth.ts (the section checking typeof rawName !== 'string' and
rawName.trim().length === 0) with a call to parse or parseAsync on the shared
schema, passing the orgPatch object. Ensure the same shared schema is imported
and used on the client side in organization-settings.tsx to maintain a single
source of truth.
Source: Coding guidelines
Collaborator
Author
Desk Review — #1951 "Make organization name required" (PR #2101)Verdict: NOT READY — one change required (test coverage). The implementation itself is correct, well-scoped, and on-convention; CI is fully green. The single blocker is a missing automated test for the server-side half of the acceptance criterion. CI & tests (observed)
What's correct (verified by tracing every branch, twice)
Blocking finding
Non-blocking notes (optional)
NOT READY — CHANGES REQUIRED: add the server-side test for the org-name-required branch in |
Collaborator
Author
Desk Review — fix(platform): require organization name (#1951)Verdict: READY TO MERGE. CIAll checks green on PR #2101 ( Tests run locally (this review)
What I verified (two rounds, fan-out + confirmation)
Non-blocking observations (not required for merge)
READY TO MERGE. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #1951 — the organization name is now enforced as required, client- and server-side.
Changes
organization-settings.tsx): added a Zod schema (name: z.string().trim().min(1)) wired throughuseFormEditor, so an empty/whitespace name blocks Save and surfaces an inline error via the existingInputerrorMessagepath. The save now sends the trimmed name unconditionally (no longer|| undefined).settings-row.tsx): added an optionalrequiredprop that renders the red*asterisk (mirrors theLabelprimitive, with a translatedaria-label); the org-name row opts in.convex/auth.ts,beforeUpdateOrganization): rejects an org rename that clears the name (empty/whitespace) withAPIError('BAD_REQUEST')and trims the stored value, so the rule can't be bypassed via a direct API call. Only enforced whennameis part of the patch (locale-only updates are untouched).settings.organization.nameRequiredtoen/de/fr. Also fixed the German/Frenchsettings.organization.title, which still read "Organisation (optional)" / "(facultatif)" — corrected to "Organisationsname" / "Nom de l'organisation" to match the now-required English label.Tests
organization-settings.test.tsx: empty name → inline error +isValid: false; entering a name clears the error and re-validates.organization-settings,settings-row) — 12 passing — plus the i18n parity suite (34 passing) andbun run typecheck(clean).Acceptance criteria
Notes
Summary by CodeRabbit
New Features
Localization