-
Notifications
You must be signed in to change notification settings - Fork 13k
chore(ui-kit): Review build configuration #37589
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
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 |
|
WalkthroughBuild configuration for the ui-kit package is being consolidated and modernized. TypeScript target updated from ES5/ES2020 to es2024/esnext, build outputs unified to a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention during review:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37589 +/- ##
===========================================
- Coverage 68.86% 68.81% -0.06%
===========================================
Files 3360 3361 +1
Lines 114339 114343 +4
Branches 20560 20619 +59
===========================================
- Hits 78741 78681 -60
- Misses 33502 33573 +71
+ Partials 2096 2089 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/ui-kit/package.json (1)
29-31: Consider consolidating redundant test scripts.Lines 29 and 30 define
testandtestunitscripts that both runjest. Redundant scripts create confusion about which one to use. Either removetestunitor assign it a different purpose (e.g.,testunitfor unit tests only with a filter).- "test": "jest", - "testunit": "jest", + "test": "jest",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
packages/ui-kit/.eslintignore(0 hunks)packages/ui-kit/.eslintrc.json(1 hunks)packages/ui-kit/.gitignore(0 hunks)packages/ui-kit/.prettierignore(0 hunks)packages/ui-kit/package.json(2 hunks)packages/ui-kit/tsconfig.build.json(1 hunks)packages/ui-kit/tsconfig.cjs.json(0 hunks)packages/ui-kit/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- packages/ui-kit/.eslintignore
- packages/ui-kit/.prettierignore
- packages/ui-kit/.gitignore
- packages/ui-kit/tsconfig.cjs.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/ui-kit/package.json
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
packages/ui-kit/package.json
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
packages/ui-kit/package.json
🔇 Additional comments (3)
packages/ui-kit/package.json (2)
18-19: Confirm whether the simple export approach in package.json is intentional.The
exportsfield is not configured; the package currently uses onlymainandtypesfor entry points. Verify this simple approach meets the package's needs for consumers, or add anexportsfield with conditional exports (ESM/CommonJS) if modern module resolution is required.
24-27: I need to research the idempotency guarantees of these tools since the sandbox environment doesn't have dependencies installed. Let me search for documentation on both.Let me search more specifically for information about the
typia patchcommand and ts-patch behavior in builds.Based on my research, I can now provide a rewritten review comment:
Remove unwarranted concern about ts-patch and typia patch idempotency.
ts-patch's persistent patch modifies the TypeScript installation within the node_modules path and requires additional configuration to remain persisted, but carries less load time and complexity. This operation is idempotent by design—ts-patch hacks node_modules/typescript source code, and whenever you update TypeScript version, you have to run npm run prepare command repeatedly, indicating it's safe to run multiple times.
The npx typia patch command was developed to revive JSDocComment parsing feature of tsc as a temporary solution for TypeScript v5.3 update instead of ts-patch, and will be disabled after ts-patch starts supporting such TypeScript v5.3 update. If you don't use any "Comment Tags" and "JSON schema generator", you don't need to run npx typia patch command, as it's not mandatory but just optional.
Neither command modifies source files. Source Transformers transform the AST of SourceFiles during compilation, allowing you to alter the output of the JS or declarations files—transformations occur during
tscexecution, not from the patch commands. Since.:build:cleanremoves dist before compilation, incremental build state is not a concern.packages/ui-kit/tsconfig.json (1)
4-6: The original review comment is incorrect.The tsconfig.json extends
@rocket.chat/tsconfig/base.json, which contains all the options the review claims were removed:strict,esModuleInterop,skipLibCheck,forceConsistentCasingInFileNames,moduleResolution, andresolveJsonModule. These options are not removed—they are inherited from the base configuration and remain active in the final merged compiler options.The actual changes in this PR are intentional target updates:
targetchanged from es5 to es2024, andmodulechanged from commonjs to esnext. The codebase uses only ES2020 features (optional chaining, nullish coalescing), which are compatible with es2024 targeting. No JSON imports or CommonJS requires were found, confirming these inherited options pose no compatibility issues.
Proposed changes (including videos or screenshots)
It reviews TypeScript output and ESLint configuration for
@rocket.chat/ui-kit.Issue(s)
ARCH-1887
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.