fix: use explicit never[] type for MessageBox empty constants#39675
fix: use explicit never[] type for MessageBox empty constants#39675Salvatoree07 wants to merge 3 commits into
Conversation
|
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 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-19T18:20:07.720ZApplied to files:
📚 Learning: 2026-01-17T01:51:47.764ZApplied to files:
📚 Learning: 2026-03-20T13:51:20.280ZApplied to files:
🔇 Additional comments (2)
WalkthroughReplaced a local fallback empty-array with a module-level readonly Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
78-79: LGTM on thenever[]type annotation.Using
never[]for a constant empty array that should never hold elements is the correct TypeScript pattern. It prevents accidental insertion and satisfies strict type checking.Consider a more descriptive variable name (optional):
The single-letter name
ais not self-documenting. A name likeEMPTY_ARRAYoremptyFormatterArraywould better convey its purpose and immutability intent.♻️ Optional naming improvement
-const a: never[] = []; -const getEmptyArray = () => a; +const EMPTY_ARRAY: never[] = []; +const getEmptyArray = () => EMPTY_ARRAY;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx` around lines 78 - 79, Rename the cryptic constant a to a descriptive, immutable name and propagate that change where used: replace the variable a with something like EMPTY_ARRAY or emptyFormatterArray and update the getEmptyArray function to return the new name (symbols to change: a and getEmptyArray in MessageBox.tsx) so the intent (constant never[] empty array) is clear while preserving the never[] type.
🤖 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/client/views/room/composer/messageBox/MessageBox.tsx`:
- Around line 78-79: Rename the cryptic constant a to a descriptive, immutable
name and propagate that change where used: replace the variable a with something
like EMPTY_ARRAY or emptyFormatterArray and update the getEmptyArray function to
return the new name (symbols to change: a and getEmptyArray in MessageBox.tsx)
so the intent (constant never[] empty array) is clear while preserving the
never[] type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 197d3646-40f1-4683-94b6-a519ba7c7a36
📒 Files selected for processing (1)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/views/room/composer/messageBox/MessageBox.tsx
🧠 Learnings (5)
📓 Common learnings
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-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/views/room/composer/messageBox/MessageBox.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/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.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/views/room/composer/messageBox/MessageBox.tsx
|
@Salvatoree07 A better approach could be to use a properly typed immutable fallback. Happy to help update the PR if needed. |
|
Hi, thanks for the feedback! You're absolutely right. Using never[] would be technically correct for an empty set but too restrictive for the formatters consumer. In fact, I've switched to readonly FormattingButton[]. I also updated the type of the items attribute in MessageBoxFormattingToolbar by adding the readonly modifier, as these items should not be modified. All the changes are included in the latest commit of this PR. |
2 participants
Proposed changes (including videos or screenshots)
Instead of allowing TypeScript to infer an ambiguous type for a constant that is intentionally empty, I have explicitly typed it as
never[]. This is the most type-safe way to handle constants that are meant to represent an empty state, preventing any accidental insertion of data and satisfying the linter/compiler requirements.Issue(s)
#39674
Steps to test or reproduce
MessageBoxcomponent in a scenario where it utilizes a default empty constant.any[].Further comments
I chose this solution because this specific array is intended to be a constant empty fallback. Using
never[]is more accurate than a generic type in this context, as it reinforces the intent that no elements should ever exist in this specific array instance.Summary by CodeRabbit