feat: unify DM folder actions#40975
Open
jhalak101205-cpu wants to merge 3 commits into
Open
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 |
|
|
|
Contributor
WalkthroughIntroduces DM folder grouping for the sidebar: adds an optional ChangesDM Folder Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant RoomContextMenu
participant AddFolderModal
participant saveDmFolder as POST /v1/subscriptions.saveDmFolder
participant SubscriptionsRaw
participant ReactQueryCache
User->>RoomContextMenu: Click "Add/Change Folder"
RoomContextMenu->>AddFolderModal: open with rid
AddFolderModal->>AddFolderModal: fetch subscriptions, build folder list
User->>AddFolderModal: select folder, click Save
AddFolderModal->>saveDmFolder: POST { roomId, dmFolder }
saveDmFolder->>SubscriptionsRaw: updateDmFolderByRoomIdAndUserId(rid, uid, folder)
SubscriptionsRaw-->>saveDmFolder: updated subscription
saveDmFolder-->>AddFolderModal: success
AddFolderModal->>ReactQueryCache: update subscription(rid), invalidate subscriptions
AddFolderModal-->>User: success toast, modal closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 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 |
coderabbitai
Bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/sidebar/AddFolderModal.tsx">
<violation number="1" location="apps/meteor/client/sidebar/AddFolderModal.tsx:65">
P2: Duplicated save/create mutation logic increases drift risk and makes future behavior fixes error-prone.</violation>
<violation number="2" location="apps/meteor/client/sidebar/AddFolderModal.tsx:116">
P1: Folder list items are interactive but not keyboard accessible: `<Box>` elements with onClick lack role, tabIndex, and keyboard handlers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| return ( | ||
| <Modal> | ||
| <Box |
Contributor
There was a problem hiding this comment.
P1: Folder list items are interactive but not keyboard accessible: <Box> elements with onClick lack role, tabIndex, and keyboard handlers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/sidebar/AddFolderModal.tsx, line 116:
<comment>Folder list items are interactive but not keyboard accessible: `<Box>` elements with onClick lack role, tabIndex, and keyboard handlers.</comment>
<file context>
@@ -0,0 +1,272 @@
+
+ return (
+ <Modal>
+ <Box
+ style={{ display: 'flex', flexDirection: 'column', height: '100%' }}
+ >
</file context>
| @@ -0,0 +1,272 @@ | |||
| import { | |||
Contributor
There was a problem hiding this comment.
P2: Duplicated save/create mutation logic increases drift risk and makes future behavior fixes error-prone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/sidebar/AddFolderModal.tsx, line 65:
<comment>Duplicated save/create mutation logic increases drift risk and makes future behavior fixes error-prone.</comment>
<file context>
@@ -0,0 +1,272 @@
+ );
+ }, [existingFolders, search]);
+
+ const handleSaveToExisting = async () => {
+ if (!selectedFolder) return;
+ setIsSaving(true);
</file context>
![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: 6
🧹 Nitpick comments (3)
packages/rest-typings/src/v1/subscriptionsEndpoints.ts (1)
109-127: ⚡ Quick winDerive
dmFolderfromISubscriptionto keep REST typings aligned.Line 109 hardcodes
dmFolder?: string; preferISubscription['dmFolder'](plus| nullonly if null is intentionally part of the API contract) to avoid type drift between REST and core model.Based on learnings: in Rocket.Chat REST typings, keep endpoint field types derived from domain models rather than ad-hoc primitive replacements.
🤖 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 `@packages/rest-typings/src/v1/subscriptionsEndpoints.ts` around lines 109 - 127, The SubscriptionsSaveDmFolder type hardcodes dmFolder as a string instead of deriving it from the ISubscription model, causing potential type misalignment. Replace the hardcoded dmFolder property in SubscriptionsSaveDmFolder with the type from ISubscription to keep REST typings synchronized with the domain model. Update the corresponding SubscriptionsSaveDmFolderSchema to reflect the actual type definition from ISubscription, ensuring both the type definition and validation schema stay aligned.Source: Learnings
apps/meteor/client/sidebar/AddFolderModal.tsx (1)
35-35: ⚡ Quick winRemove inline implementation comments to match project conventions.
apps/meteor/client/sidebar/AddFolderModal.tsx#L35-L35: remove implementation comment.apps/meteor/client/sidebar/AddFolderModal.tsx#L38-L38: remove implementation comment.apps/meteor/client/sidebar/AddFolderModal.tsx#L71-L71: remove implementation comment.apps/meteor/client/sidebar/AddFolderModal.tsx#L96-L96: remove implementation comment.apps/meteor/client/sidebar/AddFolderModal.tsx#L175-L175: remove implementation comment.apps/meteor/client/sidebar/AddFolderModal.tsx#L190-L190: remove implementation comment.apps/meteor/client/sidebar/AddFolderModal.tsx#L209-L209: remove implementation comment.apps/meteor/client/sidebar/NewFolderModal.tsx#L53-L53: remove implementation comment.As per coding guidelines,
**/*.{ts,tsx,js}should “Avoid code comments in the implementation”.🤖 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 `@apps/meteor/client/sidebar/AddFolderModal.tsx` at line 35, Remove all inline implementation comments to comply with the project's coding guidelines that require avoiding code comments in implementation. In apps/meteor/client/sidebar/AddFolderModal.tsx, delete the implementation comments at lines 35, 38, 71, 96, 175, 190, and 209. In apps/meteor/client/sidebar/NewFolderModal.tsx, delete the implementation comment at line 53. These are inline comments that explain what the code does; remove them entirely rather than converting them to other formats.Source: Coding guidelines
apps/meteor/client/hooks/useRoomMenuActions.ts (1)
78-78: ⚡ Quick winRemove the inline implementation comment.
Line 78 adds a code comment in implementation (
// Update local cache), which conflicts with the repository TS/JS guideline.Suggested change
- // Update local cache queryClient.setQueryData(subscriptionsQueryKeys.subscription(rid), (sub: any) => sub ? { ...sub, dmFolder: undefined } : undefined );As per coding guidelines,
**/*.{ts,tsx,js}should avoid code comments in implementation.🤖 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 `@apps/meteor/client/hooks/useRoomMenuActions.ts` at line 78, Remove the inline implementation comment "// Update local cache" as it violates the repository's coding guideline that prohibits inline code comments in TypeScript and JavaScript implementation files. Simply delete this comment line while preserving the actual code logic that follows.Source: Coding guidelines
🤖 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 `@apps/meteor/app/api/server/v1/subscriptions.ts`:
- Around line 193-196: The dmFolder parameter in the
updateDmFolderByRoomIdAndUserId call uses the logical OR operator (||) which
treats empty strings as falsy and coerces them to undefined, silently clearing
the folder. Replace the dmFolder || undefined expression with explicit
normalization using the nullish coalescing operator (??) instead, so that only
null and undefined values clear the folder while empty strings are preserved as
valid input values.
In `@apps/meteor/client/sidebar/AddFolderModal.tsx`:
- Around line 82-84: The error objects passed to dispatchToastMessage across
multiple locations are not being normalized to safe strings, which can result in
poor user-facing output. In apps/meteor/client/sidebar/AddFolderModal.tsx at
lines 82-84, extract and normalize the error object to a translated fallback
string or safe message before passing it to dispatchToastMessage. Apply the same
error normalization approach at apps/meteor/client/sidebar/AddFolderModal.tsx
lines 107-109 in the create-folder failure path. Similarly, apply the identical
error normalization pattern in apps/meteor/client/sidebar/NewFolderModal.tsx at
lines 64-66 to ensure consistent error handling across all modal components.
- Around line 153-174: The folder selection rows in the AddFolderModal component
are not keyboard accessible because the Box component used for clickable rows
lacks keyboard event handling. Add keyboard support by implementing an onKeyDown
handler on the Box element that responds to Enter and Space key presses to
trigger the same folder selection action as the onClick handler. Additionally,
add a tabIndex attribute to make the Box focusable via keyboard navigation, and
consider adding appropriate accessibility attributes (role and aria-label) to
clearly indicate the interactive nature and purpose of each folder row.
In `@apps/meteor/client/sidebar/NewFolderModal.tsx`:
- Around line 35-39: The useForm hook in NewFolderModal.tsx is initialized with
defaultValues based on subscription, but since useForm only applies default
values on the initial render and subscription loads asynchronously, the form
input will remain stale when the subscription data arrives. To fix this,
destructure the reset function from the useForm hook (in addition to control,
handleSubmit, and formState), then add a useEffect hook that watches the
subscription dependency and calls reset with the updated folderName value
whenever subscription changes, ensuring the form stays synchronized with the
loaded subscription data.
In `@packages/i18n/src/locales/en.i18n.json`:
- Line 490: The string "Add_in_existing_folder" uses awkward phrasing and
inconsistent casing that reads unnaturally in UI labels. Update this string and
the similar ones at lines 4508 and 4876 in the same file to use proper sentence
case and natural phrasing instead of underscores and awkward construction.
Replace the underscores with spaces and adjust the wording to read naturally as
it would appear in a user-facing label.
In `@packages/models/src/models/Subscriptions.ts`:
- Around line 726-731: The updateDmFolderByRoomIdAndUserId method in the
Subscriptions class filters only by rid and u._id, which allows non-DM
subscriptions to have dmFolder persisted. Add t: 'd' to the query object to
ensure that only direct message subscriptions (where type equals 'd') can have
their dmFolder field updated or unset, restricting this operation to DM
subscriptions only.
---
Nitpick comments:
In `@apps/meteor/client/hooks/useRoomMenuActions.ts`:
- Line 78: Remove the inline implementation comment "// Update local cache" as
it violates the repository's coding guideline that prohibits inline code
comments in TypeScript and JavaScript implementation files. Simply delete this
comment line while preserving the actual code logic that follows.
In `@apps/meteor/client/sidebar/AddFolderModal.tsx`:
- Line 35: Remove all inline implementation comments to comply with the
project's coding guidelines that require avoiding code comments in
implementation. In apps/meteor/client/sidebar/AddFolderModal.tsx, delete the
implementation comments at lines 35, 38, 71, 96, 175, 190, and 209. In
apps/meteor/client/sidebar/NewFolderModal.tsx, delete the implementation comment
at line 53. These are inline comments that explain what the code does; remove
them entirely rather than converting them to other formats.
In `@packages/rest-typings/src/v1/subscriptionsEndpoints.ts`:
- Around line 109-127: The SubscriptionsSaveDmFolder type hardcodes dmFolder as
a string instead of deriving it from the ISubscription model, causing potential
type misalignment. Replace the hardcoded dmFolder property in
SubscriptionsSaveDmFolder with the type from ISubscription to keep REST typings
synchronized with the domain model. Update the corresponding
SubscriptionsSaveDmFolderSchema to reflect the actual type definition from
ISubscription, ensuring both the type definition and validation schema stay
aligned.
🪄 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: CHILL
Plan: Pro
Run ID: faa99a4d-92f9-4449-93ea-96566e6ec4e9
📒 Files selected for processing (13)
apps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/AddFolderModal.tsxapps/meteor/client/sidebar/NewFolderModal.tsxapps/meteor/client/sidebar/RoomList/RoomListCollapser.tsxapps/meteor/client/sidebar/hooks/useRoomList.tsapps/meteor/lib/publishFields.tspackage.jsonpackages/core-typings/src/ISubscription.tspackages/i18n/src/locales/en.i18n.jsonpackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.tspackages/rest-typings/src/v1/subscriptionsEndpoints.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:
packages/core-typings/src/ISubscription.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/lib/publishFields.tsapps/meteor/client/sidebar/RoomList/RoomListCollapser.tsxpackages/models/src/models/Subscriptions.tsapps/meteor/client/sidebar/NewFolderModal.tsxpackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/hooks/useRoomList.tsapps/meteor/client/sidebar/AddFolderModal.tsxapps/meteor/app/api/server/v1/subscriptions.ts
🧠 Learnings (10)
📚 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:
packages/core-typings/src/ISubscription.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/lib/publishFields.tspackages/models/src/models/Subscriptions.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/hooks/useRoomList.tsapps/meteor/app/api/server/v1/subscriptions.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:
packages/core-typings/src/ISubscription.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/lib/publishFields.tspackages/models/src/models/Subscriptions.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/hooks/useRoomList.tsapps/meteor/app/api/server/v1/subscriptions.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/core-typings/src/ISubscription.tspackages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/lib/publishFields.tsapps/meteor/client/sidebar/RoomList/RoomListCollapser.tsxpackages/models/src/models/Subscriptions.tsapps/meteor/client/sidebar/NewFolderModal.tsxpackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/hooks/useRoomList.tsapps/meteor/client/sidebar/AddFolderModal.tsxapps/meteor/app/api/server/v1/subscriptions.ts
📚 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/sidebar/RoomList/RoomListCollapser.tsxapps/meteor/client/sidebar/NewFolderModal.tsxapps/meteor/client/sidebar/AddFolderModal.tsx
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.
Applied to files:
packages/rest-typings/src/v1/subscriptionsEndpoints.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/hooks/useRoomList.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/hooks/useRoomMenuActions.tsapps/meteor/client/sidebar/hooks/useRoomList.ts
📚 Learning: 2026-04-14T21:10:31.855Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:31.855Z
Learning: When reviewing files in apps/meteor/client/hooks/, do not treat JSDoc-style comments on React hooks (especially exported hooks) as a violation of any “avoid code comments in implementation” guideline. It’s acceptable to use JSDoc to document the public API of exported hooks (e.g., parameter/return types, intended usage), as long as it documents behavior/contracts rather than adding narrative implementation comments.
Applied to files:
apps/meteor/client/hooks/useRoomMenuActions.ts
📚 Learning: 2026-02-23T17:53:06.802Z
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:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/subscriptions.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/subscriptions.ts
🔇 Additional comments (3)
package.json (1)
204-204: LGTM!packages/i18n/src/locales/en.i18n.json (1)
476-476: LGTM!Also applies to: 2454-2457, 3758-3758, 3807-3807, 4813-4813, 4829-4829
apps/meteor/client/hooks/useRoomMenuActions.ts (1)
3-77: LGTM!Also applies to: 79-173
Comment on lines
+193
to
+196
| const { roomId, dmFolder } = this.bodyParams; | ||
|
|
||
| const subscription = await Subscriptions.updateDmFolderByRoomIdAndUserId(roomId, this.userId, dmFolder || undefined); | ||
| if (!subscription) { |
Contributor
There was a problem hiding this comment.
Avoid lossy dmFolder coercion in request handling.
Line 195 uses dmFolder || undefined, which silently treats '' as “remove folder.” Use explicit normalization and ?? so only null/undefined clear the value.
Proposed fix
const { roomId, dmFolder } = this.bodyParams;
-
- const subscription = await Subscriptions.updateDmFolderByRoomIdAndUserId(roomId, this.userId, dmFolder || undefined);
+ const normalizedDmFolder = typeof dmFolder === 'string' ? dmFolder.trim() : dmFolder;
+ if (normalizedDmFolder === '') {
+ throw new Meteor.Error('error-invalid-dm-folder', 'dmFolder cannot be empty');
+ }
+
+ const subscription = await Subscriptions.updateDmFolderByRoomIdAndUserId(roomId, this.userId, normalizedDmFolder ?? undefined);
if (!subscription) {
throw new Meteor.Error('error-invalid-subscription', 'Invalid subscription');
}🤖 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 `@apps/meteor/app/api/server/v1/subscriptions.ts` around lines 193 - 196, The
dmFolder parameter in the updateDmFolderByRoomIdAndUserId call uses the logical
OR operator (||) which treats empty strings as falsy and coerces them to
undefined, silently clearing the folder. Replace the dmFolder || undefined
expression with explicit normalization using the nullish coalescing operator
(??) instead, so that only null and undefined values clear the folder while
empty strings are preserved as valid input values.
Comment on lines
+82
to
+84
| } catch (error) { | ||
| dispatchToastMessage({ type: 'error', message: error }); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
Toast error messages use raw unknown errors, causing poor/unsafe user-facing output. Normalize errors to a string before dispatching.
apps/meteor/client/sidebar/AddFolderModal.tsx#L82-L84: maperrorto a translated fallback string or safe message extractor beforedispatchToastMessage.apps/meteor/client/sidebar/AddFolderModal.tsx#L107-L109: apply the same normalization in the create-folder failure path.apps/meteor/client/sidebar/NewFolderModal.tsx#L64-L66: apply the same normalization for consistency with the other modal.
📍 Affects 2 files
apps/meteor/client/sidebar/AddFolderModal.tsx#L82-L84(this comment)apps/meteor/client/sidebar/AddFolderModal.tsx#L107-L109apps/meteor/client/sidebar/NewFolderModal.tsx#L64-L66
🤖 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 `@apps/meteor/client/sidebar/AddFolderModal.tsx` around lines 82 - 84, The
error objects passed to dispatchToastMessage across multiple locations are not
being normalized to safe strings, which can result in poor user-facing output.
In apps/meteor/client/sidebar/AddFolderModal.tsx at lines 82-84, extract and
normalize the error object to a translated fallback string or safe message
before passing it to dispatchToastMessage. Apply the same error normalization
approach at apps/meteor/client/sidebar/AddFolderModal.tsx lines 107-109 in the
create-folder failure path. Similarly, apply the identical error normalization
pattern in apps/meteor/client/sidebar/NewFolderModal.tsx at lines 64-66 to
ensure consistent error handling across all modal components.
Comment on lines
+153
to
+174
| <Box | ||
| key={folder} | ||
| display="flex" | ||
| alignItems="center" | ||
| padding="10px 12px" | ||
| style={{ | ||
| cursor: 'pointer', | ||
| backgroundColor: isSelected ? 'rgba(31, 108, 235, 0.12)' : 'transparent', | ||
| transition: 'background-color 0.15s ease', | ||
| }} | ||
| onClick={() => setSelectedFolder(folder)} | ||
| onMouseEnter={(e: any) => { | ||
| if (!isSelected) { | ||
| e.currentTarget.style.backgroundColor = 'rgba(0, 0, 0, 0.04)'; | ||
| } | ||
| }} | ||
| onMouseLeave={(e: any) => { | ||
| if (!isSelected) { | ||
| e.currentTarget.style.backgroundColor = 'transparent'; | ||
| } | ||
| }} | ||
| > |
Contributor
There was a problem hiding this comment.
Folder rows are mouse-only and not keyboard accessible.
Each row is clickable but rendered as a non-interactive Box without keyboard handling. Users navigating by keyboard can’t select a folder.
Suggested fix
-<Box
+<Box
key={folder}
+ role='button'
+ tabIndex={0}
display="flex"
alignItems="center"
padding="10px 12px"
style={{
cursor: 'pointer',
backgroundColor: isSelected ? 'rgba(31, 108, 235, 0.12)' : 'transparent',
transition: 'background-color 0.15s ease',
}}
onClick={() => setSelectedFolder(folder)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ setSelectedFolder(folder);
+ }
+ }}📝 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
| <Box | |
| key={folder} | |
| display="flex" | |
| alignItems="center" | |
| padding="10px 12px" | |
| style={{ | |
| cursor: 'pointer', | |
| backgroundColor: isSelected ? 'rgba(31, 108, 235, 0.12)' : 'transparent', | |
| transition: 'background-color 0.15s ease', | |
| }} | |
| onClick={() => setSelectedFolder(folder)} | |
| onMouseEnter={(e: any) => { | |
| if (!isSelected) { | |
| e.currentTarget.style.backgroundColor = 'rgba(0, 0, 0, 0.04)'; | |
| } | |
| }} | |
| onMouseLeave={(e: any) => { | |
| if (!isSelected) { | |
| e.currentTarget.style.backgroundColor = 'transparent'; | |
| } | |
| }} | |
| > | |
| <Box | |
| key={folder} | |
| role="button" | |
| tabIndex={0} | |
| display="flex" | |
| alignItems="center" | |
| padding="10px 12px" | |
| style={{ | |
| cursor: 'pointer', | |
| backgroundColor: isSelected ? 'rgba(31, 108, 235, 0.12)' : 'transparent', | |
| transition: 'background-color 0.15s ease', | |
| }} | |
| onClick={() => setSelectedFolder(folder)} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| setSelectedFolder(folder); | |
| } | |
| }} | |
| onMouseEnter={(e: any) => { | |
| if (!isSelected) { | |
| e.currentTarget.style.backgroundColor = 'rgba(0, 0, 0, 0.04)'; | |
| } | |
| }} | |
| onMouseLeave={(e: any) => { | |
| if (!isSelected) { | |
| e.currentTarget.style.backgroundColor = 'transparent'; | |
| } | |
| }} | |
| > |
🤖 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 `@apps/meteor/client/sidebar/AddFolderModal.tsx` around lines 153 - 174, The
folder selection rows in the AddFolderModal component are not keyboard
accessible because the Box component used for clickable rows lacks keyboard
event handling. Add keyboard support by implementing an onKeyDown handler on the
Box element that responds to Enter and Space key presses to trigger the same
folder selection action as the onClick handler. Additionally, add a tabIndex
attribute to make the Box focusable via keyboard navigation, and consider adding
appropriate accessibility attributes (role and aria-label) to clearly indicate
the interactive nature and purpose of each folder row.
Comment on lines
+35
to
+39
| const { control, handleSubmit, formState: { isSubmitting } } = useForm({ | ||
| defaultValues: { | ||
| folderName: subscription?.dmFolder || '', | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Form default folder name won’t update when subscription loads later.
useForm({ defaultValues }) is applied only on initial render. If useUserSubscription(rid) resolves asynchronously, the input stays stale.
Suggested fix
+import { useEffect } from 'react';
...
-const { control, handleSubmit, formState: { isSubmitting } } = useForm({
+const { control, handleSubmit, reset, formState: { isSubmitting } } = useForm({
defaultValues: {
folderName: subscription?.dmFolder || '',
},
});
+
+useEffect(() => {
+ reset({ folderName: subscription?.dmFolder || '' });
+}, [subscription?.dmFolder, reset]);📝 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
| const { control, handleSubmit, formState: { isSubmitting } } = useForm({ | |
| defaultValues: { | |
| folderName: subscription?.dmFolder || '', | |
| }, | |
| }); | |
| import { useEffect } from 'react'; | |
| // ... other imports | |
| const { control, handleSubmit, reset, formState: { isSubmitting } } = useForm({ | |
| defaultValues: { | |
| folderName: subscription?.dmFolder || '', | |
| }, | |
| }); | |
| useEffect(() => { | |
| reset({ folderName: subscription?.dmFolder || '' }); | |
| }, [subscription?.dmFolder, reset]); |
🤖 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 `@apps/meteor/client/sidebar/NewFolderModal.tsx` around lines 35 - 39, The
useForm hook in NewFolderModal.tsx is initialized with defaultValues based on
subscription, but since useForm only applies default values on the initial
render and subscription loads asynchronously, the form input will remain stale
when the subscription data arrives. To fix this, destructure the reset function
from the useForm hook (in addition to control, handleSubmit, and formState),
then add a useEffect hook that watches the subscription dependency and calls
reset with the updated folderName value whenever subscription changes, ensuring
the form stays synchronized with the loaded subscription data.
| "Add_email": "Add email", | ||
| "Add_emoji": "Add emoji", | ||
| "Add_files_from": "Add files from", | ||
| "Add_in_existing_folder": "Add in existing folder", |
Contributor
There was a problem hiding this comment.
Fix folder action copy for grammar and casing consistency.
These strings read awkwardly/inconsistently in UI labels. Prefer sentence case and natural phrasing.
✏️ Suggested wording updates
- "Add_in_existing_folder": "Add in existing folder",
+ "Add_in_existing_folder": "Add to existing folder",
- "Remove_from_Folder": "Remove from Folder",
+ "Remove_from_Folder": "Remove from folder",
- "Select_Folder": "Select Folder",
+ "Select_Folder": "Select folder",Also applies to: 4508-4508, 4876-4876
🤖 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 `@packages/i18n/src/locales/en.i18n.json` at line 490, The string
"Add_in_existing_folder" uses awkward phrasing and inconsistent casing that
reads unnaturally in UI labels. Update this string and the similar ones at lines
4508 and 4876 in the same file to use proper sentence case and natural phrasing
instead of underscores and awkward construction. Replace the underscores with
spaces and adjust the wording to read naturally as it would appear in a
user-facing label.
Comment on lines
+726
to
+731
| updateDmFolderByRoomIdAndUserId(rid: string, uid: string, folder: string | undefined): Promise<null | WithId<ISubscription>> { | ||
| const query = { rid, 'u._id': uid }; | ||
| const update = folder ? { $set: { dmFolder: folder } } : { $unset: { dmFolder: 1 as const } }; | ||
|
|
||
| return this.findOneAndUpdate(query, update, { returnDocument: 'after' }); | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce DM-only scope in updateDmFolderByRoomIdAndUserId.
Line 727 filters only by rid and u._id, so non-DM subscriptions can also get dmFolder persisted. Add t: 'd' to keep folder state constrained to direct messages.
Proposed fix
updateDmFolderByRoomIdAndUserId(rid: string, uid: string, folder: string | undefined): Promise<null | WithId<ISubscription>> {
- const query = { rid, 'u._id': uid };
+ const query: Filter<ISubscription> = { rid, 'u._id': uid, t: 'd' };
const update = folder ? { $set: { dmFolder: folder } } : { $unset: { dmFolder: 1 as const } };
return this.findOneAndUpdate(query, update, { returnDocument: 'after' });
}📝 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
| updateDmFolderByRoomIdAndUserId(rid: string, uid: string, folder: string | undefined): Promise<null | WithId<ISubscription>> { | |
| const query = { rid, 'u._id': uid }; | |
| const update = folder ? { $set: { dmFolder: folder } } : { $unset: { dmFolder: 1 as const } }; | |
| return this.findOneAndUpdate(query, update, { returnDocument: 'after' }); | |
| } | |
| updateDmFolderByRoomIdAndUserId(rid: string, uid: string, folder: string | undefined): Promise<null | WithId<ISubscription>> { | |
| const query: Filter<ISubscription> = { rid, 'u._id': uid, t: 'd' }; | |
| const update = folder ? { $set: { dmFolder: folder } } : { $unset: { dmFolder: 1 as const } }; | |
| return this.findOneAndUpdate(query, update, { returnDocument: 'after' }); | |
| } |
🤖 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 `@packages/models/src/models/Subscriptions.ts` around lines 726 - 731, The
updateDmFolderByRoomIdAndUserId method in the Subscriptions class filters only
by rid and u._id, which allows non-DM subscriptions to have dmFolder persisted.
Add t: 'd' to the query object to ensure that only direct message subscriptions
(where type equals 'd') can have their dmFolder field updated or unset,
restricting this operation to DM subscriptions only.
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
2 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.
I am still getting familiar with the Rocket.Chat codebase, so I would really appreciate any feedback on the implementation.
If there are any improvements needed, edge cases I may have missed, or related changes that should be included in this PR, please feel free to suggest them. I would be happy to make the required updates.
This PR is also a learning opportunity for me as a new contributor, so any guidance is welcome.🙏🏼☺️
Summary by CodeRabbit