Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Description } from '@tale/ui/description';
import { Stack } from '@tale/ui/layout';
import { forwardRef, useId, type HTMLAttributes, type ReactNode } from 'react';

import { useT } from '@/lib/i18n/client';
import { cn } from '@/lib/utils/cn';

interface SettingsRowProps extends Omit<
Expand All @@ -12,6 +13,8 @@ interface SettingsRowProps extends Omit<
> {
label: ReactNode;
description?: ReactNode;
/** Append a red required asterisk to the label (mirrors `Label`'s required). */
required?: boolean;
/** Right-side control (switch, button, copy field, link). */
children: ReactNode;
}
Expand All @@ -22,7 +25,8 @@ interface SettingsRowProps extends Omit<
* narrow viewports so the right-side control wraps cleanly.
*/
export const SettingsRow = forwardRef<HTMLDivElement, SettingsRowProps>(
({ label, description, children, className, ...props }, ref) => {
({ label, description, required, children, className, ...props }, ref) => {
const { t } = useT('common');
const id = useId();
const labelId = `${id}-label`;
const descId = description ? `${id}-desc` : undefined;
Expand All @@ -44,6 +48,14 @@ export const SettingsRow = forwardRef<HTMLDivElement, SettingsRowProps>(
className="text-foreground text-sm leading-none font-medium"
>
{label}
{required && (
<span
className="ml-1 text-red-600"
aria-label={t('aria.required')}
>
*
</span>
)}
</span>
{description && <Description id={descId}>{description}</Description>}
</Stack>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
import { describe, it, expect, vi } from 'vitest';
import { z } from 'zod/v4';

import { useFormEditor } from '@/app/components/ui/editor';
import { organizationNameSchema } from '@/lib/shared/schemas/organizations';
import enMessages from '@/messages/en.json';
import { checkAccessibility } from '@/tests/utils/a11y';
import { fireEvent, render, screen, waitFor } from '@/tests/utils/render';

import { OrganizationSettingsView } from './organization-settings';

// Resolve user-facing labels through the translation layer rather than
// hardcoding English, matching the platform i18n rule for tests.
const orgNameLabel = enMessages.settings.organization.title;
const nameRequiredMessage = enMessages.settings.organization.nameRequired;

// The view now embeds the Members section, which subscribes to Convex via
// `useMembers`. This suite renders the view in isolation (no Convex provider)
// to exercise the locale-select dirty guard, so stub the members table out —
Expand Down Expand Up @@ -86,7 +94,7 @@ describe('OrganizationSettingsView page load', () => {
// The org-name field, labeled by `settings.organization.title`, resolves to
// the current (non-empty) org name once the form applies its baseline.
const orgNameField = screen.getByRole('textbox', {
name: 'Organization name',
name: orgNameLabel,
});
await waitFor(() => expect(orgNameField).toHaveValue('Acme Industries'));
// Mirror the E2E's "non-empty value" intent.
Expand All @@ -96,6 +104,86 @@ describe('OrganizationSettingsView page load', () => {
});
});

// Build the harness schema from the same shared `organizationNameSchema` the
// real container wires in, with the i18n message resolved through the
// translation layer — so the test guards the actual validation rule and never
// drifts on a hardcoded English literal.
const organizationSchema = z.object({
name: organizationNameSchema(nameRequiredMessage),
defaultLocale: z.string(),
});

function ValidationHarness({ orgName }: { orgName: string }) {
const editor = useFormEditor<Form>({
data: { name: orgName, defaultLocale: 'en' },
schema: organizationSchema,
save,
});
holder.current = editor;
return (
<OrganizationSettingsView
controller={editor}
organization={{ _id: 'org1', name: orgName }}
organizationId="org1"
memberContext={null}
canDelete={false}
isCurrentOrganization
onSave={save}
/>
);
}

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: orgNameLabel,
});
fireEvent.change(orgNameField, { target: { value: '' } });

await waitFor(() =>
expect(screen.getByText(nameRequiredMessage)).toBeInTheDocument(),
);
expect(holder.current?.isValid).toBe(false);
});

it('treats a whitespace-only org name as invalid', 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: orgNameLabel,
});
// The schema trims before checking `.min(1)`, so a name of only spaces must
// fail exactly as an empty string does — the trim is the rule this PR adds.
fireEvent.change(orgNameField, { target: { value: ' ' } });

await waitFor(() =>
expect(screen.getByText(nameRequiredMessage)).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: orgNameLabel,
});
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(nameRequiredMessage)).not.toBeInTheDocument();
});
});
Comment on lines +136 to +185

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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


describe('OrganizationSettingsView locale select', () => {
it('does not mark the form dirty on a spurious empty change', async () => {
render(<Harness />);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Skeletonize } from '@tale/ui/skeleton-context';
import { useQueryClient } from '@tanstack/react-query';
import { useCallback, useMemo, useState } from 'react';
import { Controller } from 'react-hook-form';
import { z } from 'zod/v4';

import { AccessDenied } from '@/app/components/layout/access-denied';
import { CopyableField } from '@/app/components/ui/data-display/copyable-field';
Expand All @@ -29,6 +30,7 @@ import { useToast } from '@/app/hooks/use-toast';
import { authClient } from '@/lib/auth-client';
import { useT } from '@/lib/i18n/client';
import { SUPPORTED_AGENT_LOCALES } from '@/lib/shared/constants/agents';
import { organizationNameSchema } from '@/lib/shared/schemas/organizations';
import { getOrganizationDefaultLocale } from '@/lib/shared/utils/get-organization-default-locale';

type Organization = {
Expand Down Expand Up @@ -106,7 +108,12 @@ export function OrganizationSettingsView({
const { t: tGlobal } = useT('global');

const { form, isLoading, isSaving } = controller;
const { handleSubmit, register, control } = form;
const {
handleSubmit,
register,
control,
formState: { errors },
} = form;

const localeOptions = useMemo(
() =>
Expand Down Expand Up @@ -138,6 +145,7 @@ export function OrganizationSettingsView({
className="py-5"
label={tSettings('organization.title')}
description={tSettings('organization.nameDescription')}
required
>
{/* Fixed-width control column, full-width on mobile where the row
stacks. `wrapperClassName="w-full"` lets the bare Input fill it
Expand All @@ -146,6 +154,8 @@ export function OrganizationSettingsView({
<Input
id="org-name"
aria-label={tSettings('organization.title')}
required
errorMessage={errors.name?.message}
{...register('name')}
wrapperClassName="w-full"
/>
Expand Down Expand Up @@ -310,9 +320,19 @@ export function OrganizationSettings({
}) {
const { t: tAccessDenied } = useT('accessDenied');
const { t: tToast } = useT('toast');
const { t: tSettings } = useT('settings');
const queryClient = useQueryClient();
const { toast } = useToast();

const organizationSchema = useMemo(
() =>
z.object({
name: organizationNameSchema(tSettings('organization.nameRequired')),
defaultLocale: z.string(),
}),
[tSettings],
);

const ability = useAbility();
const abilityLoading = useAbilityLoading();
const { data: organization, isLoading: isOrgLoading } =
Expand Down Expand Up @@ -351,7 +371,7 @@ export function OrganizationSettings({
await authClient.organization.update({
organizationId: organization._id,
data: {
name: data.name.trim() || undefined,
name: data.name.trim(),
metadata: updatedMetadata,
},
});
Expand All @@ -374,6 +394,7 @@ export function OrganizationSettings({

const editor = useFormEditor<OrganizationFormData>({
data: initialData,
schema: organizationSchema,
save,
});

Expand Down
61 changes: 60 additions & 1 deletion services/platform/convex/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ vi.mock('better-auth', () => ({
}));

vi.mock('better-auth/plugins', () => ({
organization: vi.fn(() => ({})),
// Return the config so tests can reach `organizationHooks` (the real plugin
// would consume them internally and expose nothing).
organization: vi.fn((config: unknown) => config),
twoFactor: vi.fn(() => ({})),
}));

Expand Down Expand Up @@ -97,3 +99,60 @@ describe('auth trustedOrigins', () => {
vi.unstubAllEnvs();
});
});

describe('beforeUpdateOrganization name guard', () => {
beforeEach(() => {
vi.resetModules();
});

// Pull the `beforeUpdateOrganization` hook out of the org plugin config the
// mock above passes through, so we can exercise the server-side gate directly
// — the acceptance criterion requires an empty name to be blocked on the
// server too, and neither the client unit tests nor the Playwright E2E reach
// this branch.
async function getBeforeUpdateOrganization() {
vi.stubEnv('SITE_URL', 'https://app.example.com');
const { getAuthOptions } = await import('./auth');
const options = getAuthOptions({} as never);
// oxlint-disable-next-line typescript/no-explicit-any -- test reaches into the loose better-auth plugin config
const orgPlugin = (options.plugins as any[]).find(
(plugin) => plugin?.organizationHooks?.beforeUpdateOrganization,
);
expect(orgPlugin).toBeDefined();
return orgPlugin.organizationHooks.beforeUpdateOrganization as (
data: unknown,
) => Promise<void>;
}

it('rejects a whitespace-only name', async () => {
const beforeUpdate = await getBeforeUpdateOrganization();
await expect(
beforeUpdate({ organization: { name: ' ' } }),
).rejects.toThrow();
vi.unstubAllEnvs();
});

it('rejects a non-string name', async () => {
const beforeUpdate = await getBeforeUpdateOrganization();
await expect(
beforeUpdate({ organization: { name: 123 } }),
).rejects.toThrow();
vi.unstubAllEnvs();
});

it('trims and persists a valid name', async () => {
const beforeUpdate = await getBeforeUpdateOrganization();
const data = { organization: { name: ' Acme ' } };
await beforeUpdate(data);
expect(data.organization.name).toBe('Acme');
vi.unstubAllEnvs();
});

it('leaves a name-absent (locale-only) update untouched', async () => {
const beforeUpdate = await getBeforeUpdateOrganization();
const data = { organization: { metadata: { defaultLocale: 'de' } } };
await expect(beforeUpdate(data)).resolves.toBeUndefined();
expect(data.organization).toEqual({ metadata: { defaultLocale: 'de' } });
vi.unstubAllEnvs();
});
});
17 changes: 17 additions & 0 deletions services/platform/convex/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {

import { assertValidOrgSlug } from '../lib/shared/constants/org-slug';
import { isReservedOrgSlug } from '../lib/shared/constants/reserved-org-slugs';
import { organizationNameSchema } from '../lib/shared/schemas/organizations';
import { sessionIdleWindowSeconds } from '../lib/shared/session-idle';
import { getOrganizationDefaultLocale } from '../lib/shared/utils/get-organization-default-locale';
import { isRecord, getString } from '../lib/utils/type-utils';
Expand Down Expand Up @@ -708,6 +709,22 @@ export const getAuthOptions = (ctx: GenericCtx<DataModel>) => {
// through a `Record<string, unknown>` view so the field
// shape matches Better Auth's loose update payload type.
const orgPatch = data.organization as Record<string, unknown>;
// 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.
if (orgPatch.name !== undefined) {
const parsedName = organizationNameSchema().safeParse(
orgPatch.name,
);
if (!parsedName.success) {
throw new APIError('BAD_REQUEST', {
message: 'Organization name is required.',
});
}
orgPatch.name = parsedName.data;
}
Comment on lines +712 to +727

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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

const rawSlug = orgPatch.slug;
if (typeof rawSlug !== 'string') return;
const normalizedSlug = rawSlug.toLowerCase();
Expand Down
10 changes: 10 additions & 0 deletions services/platform/lib/shared/schemas/organizations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,13 @@ const memberRoleLiterals = [
] as const;
export const memberRoleSchema = z.enum(memberRoleLiterals);
export type MemberRole = z.infer<typeof memberRoleSchema>;

/**
* Organization name is required: non-empty once trimmed. Shared so the client
* settings form and the Convex `beforeUpdateOrganization` hook enforce the same
* rule from a single source — neither layer can drift from the other. The
* optional `message` lets the client inject a localized error; the server
* relies on `safeParse` success/failure and supplies its own message.
*/
export const organizationNameSchema = (message?: string) =>
z.string().trim().min(1, message);
3 changes: 2 additions & 1 deletion services/platform/messages/de.json
Original file line number Diff line number Diff line change
Expand Up @@ -5006,10 +5006,11 @@
}
},
"organization": {
"title": "Organisation (optional)",
"title": "Organisationsname",
"detailsTitle": "Organisationsdetails",
"detailsDescription": "Grundlegende Informationen zu deiner Organisation.",
"nameDescription": "Der Anzeigename deiner Organisation.",
"nameRequired": "Organisationsname ist erforderlich",
"localeDescription": "Die Hauptsprache deiner Organisation.",
"organizationId": "Organisations-ID",
"organizationIdDescription": "Verwende diese ID in API-Anfragen und Integrationen.",
Expand Down
1 change: 1 addition & 0 deletions services/platform/messages/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -5272,6 +5272,7 @@
"detailsTitle": "Organization details",
"detailsDescription": "Basic information about your organization.",
"nameDescription": "The display name for your organization.",
"nameRequired": "Organization name is required",
"localeDescription": "Primary language for your organization.",
"organizationId": "Organization ID",
"organizationIdDescription": "Reference this ID in API requests and integrations.",
Expand Down
3 changes: 2 additions & 1 deletion services/platform/messages/fr.json
Original file line number Diff line number Diff line change
Expand Up @@ -5007,10 +5007,11 @@
}
},
"organization": {
"title": "Organisation (facultatif)",
"title": "Nom de l'organisation",
"detailsTitle": "Détails de l'organisation",
"detailsDescription": "Informations de base sur ton organisation.",
"nameDescription": "Le nom d'affichage de ton organisation.",
"nameRequired": "Le nom de l'organisation est requis",
"localeDescription": "La langue principale de ton organisation.",
"organizationId": "ID de l'organisation",
"organizationIdDescription": "Utilise cet ID dans les requêtes API et les intégrations.",
Expand Down