Skip to content

fix(ui): Trim leading and trailing whitespaces in forms #3826

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ui): Trim leading and trailing whitespaces in forms #3826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ui/src/routes/admin/users/create-user/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import { ALL_ITEMS } from '@/lib/constants';
import { toast } from '@/lib/toast';

const formSchema = z.object({
username: z.string().min(1),
username: z.string().trim().min(1),
firstName: asOptionalField(z.string().min(1)),
lastName: asOptionalField(z.string().min(1)),
email: asOptionalField(z.email()),
Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/create-organization/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string().min(1),
name: z.string().trim().min(1),
description: asOptionalField(z.string().min(1)),
});

Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/create-product/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string().min(1),
name: z.string().trim().min(1),
description: asOptionalField(z.string().min(1)),
});

Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/infrastructure-services/create/index.tsx
View file
Open in desktop
Copy link
Contributor

@Etsija Etsija Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In here and similar places, omitting the .min(1) originally has clearly been an oversight from the UI developers, as there is no possibility for an infrastructure service to exist with an empty name. Also, a name is enforced by the backend; I can see it from the generated query client, where the name is non-nullable.

I would suggest that in all places similar to this, you replace z.string() with z.string().min(1).trim(), so trim the string, and require it to be at least 1 character after trimming the whitespaces.

Copy link
Contributor

@Etsija Etsija Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This comment is of course pending on the discussion of whether we should do everything in the back-end after all, instead of front-end.

We nowadays also have a plugin to generate Zod schemas from the OpenAPI spec, and will gradually use that in many places to partially or completely replace these manually typed form schemas throughout the UI.

Example: for creating infrastructure services, we use currently these "hand-made" form schemas:

const formSchema = z.object({
  name: z.string(),
  url: z.url(),
  description: z.string().optional(),
  usernameSecretRef: z.string(),
  passwordSecretRef: z.string(),
  credentialsTypes: z.array(z.enum(['NETRC_FILE', 'GIT_CREDENTIALS_FILE'])),
});

but we could just import and use an auto-generated Zod schema instead:

/**
 * PostInfrastructureService
 */
export const zPostInfrastructureService = z.object({
  credentialsTypes: z.optional(z.array(zCredentialsType)),
  description: z.optional(z.union([z.null(), z.string()])),
  name: z.string(),
  passwordSecretRef: z.string(),
  url: z.string(),
  usernameSecretRef: z.string(),
});

As I said, these are all auto-generated from the OpenAPI spec, but they of course don't include features like trimming, so if back-end doesn't do it, then we anyway need to, by refining or extending (simpler) the auto-generated schema further in the UI.

Copy link
Contributor

@Etsija Etsija Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The is an example of importing the auto-generated schema and extending it in the UI:

import { zPostInfrastructureService } from '@/api/zod.gen';

const formSchema = zPostInfrastructureService.extend({
  name: z.string().min(1).trim(),
});

Copy link
Contributor

@sschuberth sschuberth Oct 31, 2025

Choose a reason for hiding this comment

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

What confuses me here is: Shouldn't it be .string().trim().min(1) to signal the the length should be at least 1 after trimming, not before?

Copy link
Contributor Author

@bs-ondem bs-ondem Oct 31, 2025

Choose a reason for hiding this comment

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

I was not aware of the generated Zod schemas. Depending on how we continue with this PR, i would just import the generated schema and extend it as shown.

What confuses me here is: Shouldn't it be .string().trim().min(1) to signal the the length should be at least 1 after trimming, not before?

Yes, it should be .strimg().trim().min(1).

But what are we doing now with this PR, do we want to have such a magic trimming of the values before sending the request, or not?

Copy link
Contributor

@Etsija Etsija Oct 31, 2025

Choose a reason for hiding this comment

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

I'm fine with it done in the front-end, at least for now.

Copy link
Contributor Author

@bs-ondem bs-ondem Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@sschuberth What do you think about it?

Copy link
Contributor

@sschuberth sschuberth Nov 3, 2025

Choose a reason for hiding this comment

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

I have no strong opinion here right now. Please continue the review with @Etsija from my end, as it's him who still has pending requested changes, it seems.

Copy link
Contributor

@Etsija Etsija Nov 4, 2025

Choose a reason for hiding this comment

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

So from my part, it's enough for now to add the .min(1) for those fields where it doesn't make sense to allow empty strings (names, mostly), so z.string() -> z.string().trim().min(1). Once that's done, I can approve from my part.

Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ import { ALL_ITEMS } from '@/lib/constants';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string(),
name: z.string().trim(),
url: z.url(),
description: z.string().optional(),
usernameSecretRef: z.string(),
Expand Down
2 changes: 1 addition & 1 deletion ...cts/$productId/repositories/$repoId/_repo-layout/infrastructure-services/create/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ import { ALL_ITEMS } from '@/lib/constants.ts';
import { toast } from '@/lib/toast.ts';

const formSchema = z.object({
name: z.string(),
name: z.string().trim(),
url: z.url(),
description: z.string().optional(),
usernameSecretRef: z.string(),
Expand Down
2 changes: 1 addition & 1 deletion ...gId/products/$productId/repositories/$repoId/_repo-layout/secrets/create-secret/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string(),
name: z.string().trim(),
value: z.string(),
description: z.string().optional(),
});
Expand Down
2 changes: 1 addition & 1 deletion ...rganizations/$orgId/products/$productId/repositories/$repoId/_repo-layout/users/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import { groupsSchema } from '@/schemas';
import { RepositoryUsersTable } from './-components/repository-users-table';

const formSchema = z.object({
username: z.string().min(1),
username: z.string().trim().min(1),
groupId: groupsSchema,
});

Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/products/$productId/secrets/create-secret/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string(),
name: z.string().trim(),
value: z.string(),
description: z.string().optional(),
});
Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/products/$productId/settings/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string().min(1),
name: z.string().trim().min(1),
description: z.string().optional(),
});

Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/products/$productId/users/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import { groupsSchema } from '@/schemas';
import { ProductUsersTable } from './-components/product-users-table';

const formSchema = z.object({
username: z.string().min(1),
username: z.string().trim().min(1),
groupId: groupsSchema,
});

Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/secrets/create-secret/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string(),
name: z.string().trim(),
value: z.string(),
description: z.string().optional(),
});
Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/settings/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import { ApiError } from '@/lib/api-error';
import { toast } from '@/lib/toast';

const formSchema = z.object({
name: z.string().min(1),
name: z.string().trim().min(1),
description: z.string().optional(),
});

Expand Down
2 changes: 1 addition & 1 deletion ui/src/routes/organizations/$orgId/users/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import { groupsSchema } from '@/schemas';
import { OrganizationUsersTable } from './-components/organization-users-table';

const formSchema = z.object({
username: z.string().min(1),
username: z.string().trim().min(1),
groupId: groupsSchema,
});

Expand Down
Loading