fix: prevent creation of discussions with whitespace-only names#38482
Open
anupam-pandey01 wants to merge 4 commits into
Open
fix: prevent creation of discussions with whitespace-only names#38482anupam-pandey01 wants to merge 4 commits into
anupam-pandey01 wants to merge 4 commits into
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: ef2f9eb The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTrim discussion names client-side and add server-side validation that rejects empty or whitespace-only discussion names; add a changeset entry documenting the patch release. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
anupam-pandey01
changed the title
anupam-pandey01
changed the title
![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: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx`:
- Around line 94-103: The name field's form validation allows whitespace-only
input which then becomes empty after trimming in CreateDiscussion's
handleCreate; update the form field validation (the name field rule in
CreateDiscussion component) to add a validate check that returns false (or an
error message) when value.trim().length === 0 (e.g., value =>
value.trim().length > 0) so whitespace-only names are rejected before
handleCreate runs; ensure the validation message is appropriate and keep the
existing required rule.
- Around line 136-152: The parentRoom Controller mistakenly defines render twice
causing the first block (which applies Name-specific trim validation and uses
discussionNameId and errors.name) to be ignored; remove the stray
rules/render/TextInput block inside the Controller for parentRoom and keep only
the intended render that returns DefaultParentRoomField, ensuring
validation/rule references for parentRoom (not discussionNameId or errors.name)
are not present elsewhere in the same Controller; locate the Controller with
name/prop parentRoom and delete the duplicate render/rules/TextInput fragment so
only DefaultParentRoomField remains.
Comment on lines
94
to
103
| const handleCreate = async ({ name, parentRoom, encrypted, usernames, firstMessage, topic }: CreateDiscussionFormValues) => { | ||
| const trimmedName = name.trim(); | ||
|
|
||
| createDiscussionMutation.mutate({ | ||
| prid: defaultParentRoom || parentRoom, | ||
| t_name: name, | ||
| t_name: trimmedName, | ||
| users: usernames, | ||
| reply: encrypted ? undefined : firstMessage, | ||
| topic, | ||
| ...(parentMessageId && { pmid: parentMessageId }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx | head -250Repository: RocketChat/Rocket.Chat
Length of output: 8961
Add trim-based validation to the name field to prevent whitespace-only submissions.
The name field at lines 192-195 only has a required rule, which allows whitespace-only values to pass validation. These values then become empty strings after trimming in handleCreate (line 95). Add a validate rule checking value.trim().length > 0 to prevent whitespace-only names from passing validation.
Proposed fix
rules={{ required: t('Required_field', { field: t('Name') }) }}
+ rules={{
+ required: t('Required_field', { field: t('Name') }),
+ validate: (value) => value.trim().length > 0 || t('Required_field', { field: t('Name') }),
+ }}Also applies to: 192-205
🤖 Prompt for AI Agents
In `@apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx` around
lines 94 - 103, The name field's form validation allows whitespace-only input
which then becomes empty after trimming in CreateDiscussion's handleCreate;
update the form field validation (the name field rule in CreateDiscussion
component) to add a validate check that returns false (or an error message) when
value.trim().length === 0 (e.g., value => value.trim().length > 0) so
whitespace-only names are rejected before handleCreate runs; ensure the
validation message is appropriate and keep the existing required rule.
Comment on lines
136
to
+152
| <Controller | ||
| control={control} | ||
| rules={{ | ||
| required: t('Required_field', { field: t('Name') }), | ||
| validate: (value) => | ||
| value.trim().length > 0 || t('Required_field', { field: t('Name') }), | ||
| }} | ||
| render={({ field }) => ( | ||
| <TextInput | ||
| id={discussionNameId} | ||
| {...field} | ||
| aria-invalid={Boolean(errors.name)} | ||
| aria-required='true' | ||
| aria-describedby={`${discussionNameId}-error ${discussionNameId}-hint`} | ||
| addon={<Icon name='baloons' size='x20' />} | ||
| /> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its structure
find . -name "CreateDiscussion.tsx" -type f | head -5Repository: RocketChat/Rocket.Chat
Length of output: 135
🏁 Script executed:
# Read the file to see the actual code at lines 136-152 and surrounding context
wc -l apps/meteor/client/components/CreateDiscussion/CreateDiscription.tsx 2>/dev/null || \
find . -path "*CreateDiscussion/CreateDiscussion.tsx" -type f -exec wc -l {} \;Repository: RocketChat/Rocket.Chat
Length of output: 139
🏁 Script executed:
# Search for CreateDiscussion.tsx in the codebase
fd "CreateDiscussion.tsx" --type fRepository: RocketChat/Rocket.Chat
Length of output: 133
🏁 Script executed:
# Read the file around lines 136-152 with context
sed -n '130,160p' ./apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxRepository: RocketChat/Rocket.Chat
Length of output: 1069
🏁 Script executed:
# Get more context - see the entire structure of this form section
sed -n '90,200p' ./apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxRepository: RocketChat/Rocket.Chat
Length of output: 3463
🏁 Script executed:
# Get the full Name field section to see the proper structure
sed -n '175,210p' ./apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxRepository: RocketChat/Rocket.Chat
Length of output: 1104
🏁 Script executed:
# Let me also check the Controller component definition to understand how it handles multiple render props
rg -A 5 "interface.*Controller|function Controller" apps/meteor/client/components/CreateDiscussion/ || \
rg -B 2 -A 8 "export.*Controller" --type tsx | head -30Repository: RocketChat/Rocket.Chat
Length of output: 93
🏁 Script executed:
# Check the imports at the top of the file to understand what Controller is
head -50 ./apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx | grep -i controllerRepository: RocketChat/Rocket.Chat
Length of output: 120
🏁 Script executed:
# Verify the structure around both render blocks - check if they are both actually present in the Controller
sed -n '136,156p' ./apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 948
🏁 Script executed:
# Double-check by looking at the actual lines in the diff format to be absolutely certain
sed -n '136,156p' ./apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsxRepository: RocketChat/Rocket.Chat
Length of output: 801
Fix duplicate render prop in parentRoom Controller.
This Controller defines render twice (lines 8–17 and line 19), causing React to ignore the first block. The first render block incorrectly applies Name-specific trim validation to the parentRoom field and references discussionNameId and errors.name instead of the appropriate parent room identifiers. Remove the stray render/rules/TextInput block and keep only the DefaultParentRoomField renderer.
Proposed fix
{defaultParentRoom && (
<Controller
control={control}
- rules={{
- required: t('Required_field', { field: t('Name') }),
- validate: (value) =>
- value.trim().length > 0 || t('Required_field', { field: t('Name') }),
- }}
- render={({ field }) => (
- <TextInput
- id={discussionNameId}
- {...field}
- aria-invalid={Boolean(errors.name)}
- aria-required='true'
- aria-describedby={`${discussionNameId}-error ${discussionNameId}-hint`}
- addon={<Icon name='baloons' size='x20' />}
- />
- )}
name='parentRoom'
render={() => <DefaultParentRoomField defaultParentRoom={defaultParentRoom} id={parentRoomId} />}
/>
)}📝 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
| <Controller | |
| control={control} | |
| rules={{ | |
| required: t('Required_field', { field: t('Name') }), | |
| validate: (value) => | |
| value.trim().length > 0 || t('Required_field', { field: t('Name') }), | |
| }} | |
| render={({ field }) => ( | |
| <TextInput | |
| id={discussionNameId} | |
| {...field} | |
| aria-invalid={Boolean(errors.name)} | |
| aria-required='true' | |
| aria-describedby={`${discussionNameId}-error ${discussionNameId}-hint`} | |
| addon={<Icon name='baloons' size='x20' />} | |
| /> | |
| )} | |
| <Controller | |
| control={control} | |
| name='parentRoom' | |
| render={() => <DefaultParentRoomField defaultParentRoom={defaultParentRoom} id={parentRoomId} />} | |
| /> |
🧰 Tools
🪛 Biome (2.3.13)
[error] 143-152: This JSX property is assigned multiple times.
This attribute is assigned again here.
(lint/suspicious/noDuplicateJsxProps)
🤖 Prompt for AI Agents
In `@apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx` around
lines 136 - 152, The parentRoom Controller mistakenly defines render twice
causing the first block (which applies Name-specific trim validation and uses
discussionNameId and errors.name) to be ignored; remove the stray
rules/render/TextInput block inside the Controller for parentRoom and keep only
the intended render that returns DefaultParentRoomField, ensuring
validation/rule references for parentRoom (not discussionNameId or errors.name)
are not present elsewhere in the same Controller; locate the Controller with
name/prop parentRoom and delete the duplicate render/rules/TextInput fragment so
only DefaultParentRoomField remains.
KevLehman
added
the
valid
Member
|
What do you think about also validating on the backend? |
Member
|
@anupam-pandey01 can we add a validation in the backend as well? |
Author
Thanks for the suggestion. |
anupam-pandey01
force-pushed
the
fix/whitespace-discussion-name
branch
from
a381dde to
65f2b82
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/discussion/server/methods/createDiscussion.ts (1)
77-89: Remove inline implementation comments and trim once.Lines 77 and 87 add inline comments in implementation, and
trim()is called twice in this block. You can keep the code self-explanatory and slightly cleaner by trimming once into a local value.As per coding guidelines: `**/*.{ts,tsx,js}` — “Avoid code comments in the implementation”.♻️ Proposed refactor
- // Validate discussion name to reject empty or whitespace-only values. - if (!discussionName || discussionName.trim().length === 0) { + const trimmedDiscussionName = discussionName.trim(); + if (!trimmedDiscussionName) { throw new Meteor.Error( 'error-invalid-discussion-name', - 'Discussion name cannot be empty', { - method: 'DiscussionCreation' - }, + 'Discussion name cannot be empty', + { + method: 'DiscussionCreation', + }, ); } - - // Remove leading and trailing whitespace - discussionName = discussionName.trim(); + discussionName = trimmedDiscussionName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/discussion/server/methods/createDiscussion.ts` around lines 77 - 89, The validation block trims discussionName twice and contains implementation comments; instead, create a single trimmed local value (e.g., const trimmedName = discussionName?.trim()) use that for the empty check and subsequent assignment, remove the inline comments, and throw the same Meteor.Error (code 'error-invalid-discussion-name', context { method: 'DiscussionCreation' }) when trimmedName is empty, then assign discussionName = trimmedName once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/discussion/server/methods/createDiscussion.ts`:
- Around line 77-89: The validation block trims discussionName twice and
contains implementation comments; instead, create a single trimmed local value
(e.g., const trimmedName = discussionName?.trim()) use that for the empty check
and subsequent assignment, remove the inline comments, and throw the same
Meteor.Error (code 'error-invalid-discussion-name', context { method:
'DiscussionCreation' }) when trimmedName is empty, then assign discussionName =
trimmedName once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9000c2a2-524f-4986-b62f-8588736caec9
📒 Files selected for processing (2)
.changeset/brave-apples-jump.mdapps/meteor/app/discussion/server/methods/createDiscussion.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/brave-apples-jump.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/discussion/server/methods/createDiscussion.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/discussion/server/methods/createDiscussion.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/discussion/server/methods/createDiscussion.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/discussion/server/methods/createDiscussion.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/discussion/server/methods/createDiscussion.ts
🔇 Additional comments (1)
apps/meteor/app/discussion/server/methods/createDiscussion.ts (1)
78-89: Backend whitespace validation is correctly enforced.Line 78 now rejects empty/whitespace-only discussion names, and Line 88 normalizes the stored display name, which covers non-UI call paths too.
Based on learnings: existing Rocket.Chat behavior rejects whitespace-only text using
if (!description.trim()), and this change follows that same pattern.
Author
|
@dougfabris Added frontend and backend validation to prevent whitespace. |
KevLehman
requested changes
Mar 4, 2026
| encrypted, | ||
| topic, | ||
| }: CreateDiscussionProperties): Promise<IRoom & { rid: string }> => { | ||
| // Validate discussion name to reject empty or whitespace-only values. |
Author
There was a problem hiding this comment.
@KevLehman could you please guide me on where the test file should be located or which existing test file I should extend ?
Member
There was a problem hiding this comment.
Try searching for existing tests on folders named test, tests, etc. That way you get to learn the codebase a bit better.
Author
There was a problem hiding this comment.
There is one test folder in codebase. i think this is right place 'Rocket.Chat\apps\meteor\tests\unit\app'
| throw new Meteor.Error( | ||
| 'error-invalid-discussion-name', | ||
| 'Discussion name cannot be empty', { | ||
| method: 'DiscussionCreation' |
Member
There was a problem hiding this comment.
Suggested change
| method: 'DiscussionCreation' | |
| method: 'createDiscussion' |
| } | ||
|
|
||
| // Remove leading and trailing whitespace | ||
| discussionName = discussionName.trim(); |
Member
There was a problem hiding this comment.
Do this before the if so you don't have to trim twice
Member
There was a problem hiding this comment.
discusionName = discussionName?.trim() (or smth)
anupam-pandey01
force-pushed
the
fix/whitespace-discussion-name
branch
from
65f2b82 to
0ef4ca3
Compare
Author
|
i have done all changes. could anyone please guide me on where the test file should be located or which existing test file I should extend ? |
anupam-pandey01
force-pushed
the
fix/whitespace-discussion-name
branch
from
0ef4ca3 to
083202c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx (1)
95-100:⚠️ Potential issue | 🟠 MajorWhitespace-only names still pass client-side validation.
Line 96 trims before submission, but the
namefield rule (Line 186) still only checksrequired, so" "passes locally and fails only on the backend. Add trim-based validation in theControllerrules so the field shows the same required error before submit.Proposed fix
<Controller name='name' control={control} - rules={{ required: t('Required_field', { field: t('Name') }) }} + rules={{ + required: t('Required_field', { field: t('Name') }), + validate: (value) => value.trim().length > 0 || t('Required_field', { field: t('Name') }), + }} render={({ field }) => ( <TextInput🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx` around lines 95 - 100, The client-side name field allows whitespace-only names because the Controller's validation rules for the name input only mark it as required; update the Controller validation for the name field (the rules used with the Controller that supplies CreateDiscussionFormValues.name) to validate trimmed input (e.g., use a validate or pattern rule that checks name.trim().length > 0) so that whitespace-only values show the required error before submission; keep the existing trim in handleCreate but add this trim-based rule to the Controller so the UI validation matches backend behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx`:
- Around line 95-100: The client-side name field allows whitespace-only names
because the Controller's validation rules for the name input only mark it as
required; update the Controller validation for the name field (the rules used
with the Controller that supplies CreateDiscussionFormValues.name) to validate
trimmed input (e.g., use a validate or pattern rule that checks
name.trim().length > 0) so that whitespace-only values show the required error
before submission; keep the existing trim in handleCreate but add this
trim-based rule to the Controller so the UI validation matches backend behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 534f654c-290c-40e9-b391-a74918238785
📒 Files selected for processing (3)
.changeset/brave-apples-jump.mdapps/meteor/app/discussion/server/methods/createDiscussion.tsapps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/brave-apples-jump.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/discussion/server/methods/createDiscussion.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
🧠 Learnings (16)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 40075
File: apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx:69-71
Timestamp: 2026-04-10T22:42:05.539Z
Learning: In `apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx`, the submit handler converts an empty/whitespace-only description to `undefined` (`description?.trim() || undefined`) intentionally. All downstream image-rendering components (`AttachmentImage`, `ImagePreview`, `ImageItem`, `ImageGallery`) default `undefined` alt to `''`, so the `<img alt="">` attribute is always present. Do not flag this `undefined` conversion as a bug preventing alt text from being cleared.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2026-04-10T22:42:05.539Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 40075
File: apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx:69-71
Timestamp: 2026-04-10T22:42:05.539Z
Learning: In `apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx`, the submit handler converts an empty/whitespace-only description to `undefined` (`description?.trim() || undefined`) intentionally. All downstream image-rendering components (`AttachmentImage`, `ImagePreview`, `ImageItem`, `ImageGallery`) default `undefined` alt to `''`, so the `<img alt="">` attribute is always present. Do not flag this `undefined` conversion as a bug preventing alt text from being cleared.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx
🔇 Additional comments (1)
apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx (1)
139-145:parentRoomcontroller cleanup looks correct.Keeping a single
renderprop here removes the duplicate-prop hazard and preserves the intendedDefaultParentRoomFieldbehavior.
anupam-pandey01
force-pushed
the
fix/whitespace-discussion-name
branch
from
e11c5ce to
ef2f9eb
Compare
anupam-pandey01
commented
Apr 16, 2026
Author
anupam-pandey01
left a comment
anupam-pandey01
left a comment
There was a problem hiding this comment.
@KevLehman I have added frontend and backend validation add also added api test for whitespace-only discussion names.
Please review 🙏
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
6 participants
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:
Added validation to the CreateDiscussion component to prevent the creation of discussions with names consisting only of whitespace (spaces/tabs). This ensures that all discussions in the sidebar have a visible, identifiable name.
Proposed changes (including videos or screenshots):
Screen.Recording.2026-02-04.033430.mp4
Issue(s):
Closes #38434
Steps to test or reproduce:
Summary by CodeRabbit