Skip to content
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

fix(ui): trim leading and trailing whitespace in group names #3617

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Nov 11, 2022

Summary

leading whitespaces are particularly problematic because it makes it possible to create group names that look empty, e.g. " ". a side effect is group names with " " (one whitespace) and " " (multple consecutive whitespaces) will cause a 409 Conflict on creation since the server trims inputs.

non-leading or non-trailing whitespaces are left as is so it's possible to have repeats in t between other characters

ui/pages/groups/index.js Outdated Show resolved Hide resolved
@@ -22,7 +22,7 @@ function AddGroupsDialog({ setOpen }) {
try {
const res = await fetch('/api/groups', {
method: 'POST',
body: JSON.stringify({ name }),
body: JSON.stringify({ name: name.trim() }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Rather than trimming the name here and in the error you could do one setName(name.trim()) before the try/catch block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was the initial approach I took but it has two major downsides.

The first is it doesn't provide immediate feedback to the user when entering leading whitespace so what they think they've inputted into the field is not what's actually being sent to the server. This is minor.

The second is useState doesn't immediately update the state. React batches state updates so if the state is changed immediately before it's checked, the update may not yet have been updated. This is problematic since the state is being updated immediately before sent to the server so while the state may be trimmed, the value being sent is untrimmed (most likely).

@mxyng mxyng force-pushed the mxyng/group-names-whitespaces branch from 2a1cff8 to 5afbc69 Compare November 14, 2022 17:39
ui/pages/groups/index.js Outdated Show resolved Hide resolved
ui/pages/groups/index.js Outdated Show resolved Hide resolved
@mxyng mxyng force-pushed the mxyng/group-names-whitespaces branch from 5afbc69 to 3dc2bf9 Compare November 15, 2022 02:00
leading whitespaces are particularly problematic because it makes it
possible to create group names that look empty, e.g. " ". a side effect
is group names with " " (one whitespace) and "  " (multple consecutive
whitespaces) will cause a 409 Conflict on creation since the server trims
inputs.

non-leading or non-trailing whitespaces are left as is so it's possible
to have repeats in t between other characters
@mxyng mxyng force-pushed the mxyng/group-names-whitespaces branch from 3dc2bf9 to f995c61 Compare November 15, 2022 18:06
@mxyng mxyng merged commit d4bb645 into main Nov 15, 2022
@mxyng mxyng deleted the mxyng/group-names-whitespaces branch November 15, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants