refactor: remove uuid package#37143
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 |
|
|
|
WalkthroughReplaced uses of the external Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ee/apps/account-service/src/lib/utils.ts (1)
1-5: Consolidate crypto imports.The
cryptomodule is imported twice. Consolidate into a single import statement for cleaner code.Apply this diff:
-import crypto from 'crypto'; +import crypto, { randomUUID } from 'crypto'; import { convertFromDaysToMilliseconds } from '@rocket.chat/tools'; import bcrypt from 'bcrypt'; -import { randomUUID } from 'crypto';apps/meteor/app/api/server/v1/misc.ts (1)
1-1: Consolidate crypto imports.The
cryptomodule is imported twice. Consolidate into a single import statement for cleaner code.Apply this diff:
-import crypto from 'crypto'; +import crypto, { randomUUID } from 'crypto'; import type { IUser } from '@rocket.chat/core-typings';And remove the duplicate import on line 19:
import { Meteor } from 'meteor/meteor'; -import { randomUUID } from 'crypto'; import { i18n } from '../../../../server/lib/i18n';Also applies to: 19-19
apps/meteor/server/settings/misc.ts (1)
1-5: Consolidate crypto imports.The
cryptomodule is imported twice. Consolidate into a single import statement for cleaner code.Apply this diff:
-import crypto from 'crypto'; +import crypto, { randomUUID } from 'crypto'; import { Logger } from '@rocket.chat/logger'; import { Settings } from '@rocket.chat/models'; -import { randomUUID } from 'crypto';
📜 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 (9)
apps/meteor/app/api/server/v1/misc.ts(2 hunks)apps/meteor/app/apps/server/bridges/oauthApps.ts(2 hunks)apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts(2 hunks)apps/meteor/ee/server/services/package.json(0 hunks)apps/meteor/package.json(0 hunks)apps/meteor/server/settings/misc.ts(2 hunks)apps/meteor/tests/e2e/utils/test.ts(2 hunks)ee/apps/account-service/package.json(1 hunks)ee/apps/account-service/src/lib/utils.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/meteor/ee/server/services/package.json
- apps/meteor/package.json
🧰 Additional context used
📓 Path-based instructions (2)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/utils/test.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/utils/test.ts
🔇 Additional comments (6)
apps/meteor/app/apps/server/bridges/oauthApps.ts (1)
7-7: LGTM! Clean migration to built-in crypto.The replacement of
uuid.v4()withcrypto.randomUUID()is correct for this server-side code. Both generate RFC 4122 version 4 UUIDs.Also applies to: 28-28
ee/apps/account-service/src/lib/utils.ts (1)
36-36: LGTM! Token generation updated correctly.The replacement of
uuid.v4()withrandomUUID()for login token generation is correct.ee/apps/account-service/package.json (1)
40-40: LGTM! Dependency removal aligns with code changes.The removal of the
uuiddependency is correct and consistent with the migration to Node.js's built-incrypto.randomUUID().apps/meteor/tests/e2e/utils/test.ts (1)
8-8: LGTM! Test code correctly uses Node.js crypto.This E2E test code runs in Node.js via Playwright, so using
crypto.randomUUID()is appropriate and correct.Also applies to: 57-57
apps/meteor/app/api/server/v1/misc.ts (1)
684-684: LGTM! Deployment ID fallback updated correctly.The replacement of
uuid.v4()withrandomUUID()for the uniqueID fallback value is correct.apps/meteor/server/settings/misc.ts (1)
64-64: LGTM! Deployment ID generation updated correctly.The replacement of
uuid.v4()withrandomUUID()for the uniqueID setting default is correct.
cardoso
left a comment
cardoso
left a comment
There was a problem hiding this comment.
Thanks for this PR!
You should just use the global crypto.randomUUID instead of importing from 'crypto'.
There are incompatibilities between the server (Node) and client (Browser) when using the crypto global, but not for this particular API since we can ignore the optional argument node accepts.
|
Hello @cardoso |
|
@AnastasiyaHladina you need to run yarn lint and fix the errors. |
|
@cardoso, updated, please check. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37143 +/- ##
===========================================
- Coverage 70.41% 70.40% -0.01%
===========================================
Files 3161 3161
Lines 110151 110151
Branches 19850 19892 +42
===========================================
- Hits 77560 77554 -6
- Misses 30560 30562 +2
- Partials 2031 2035 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
cardoso
left a comment
cardoso
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks a lot for this PR!
May need reviews from @RocketChat/backend @RocketChat/apps and @RocketChat/frontend .
PS: Just one test failed but it seems flaky, so I just restarted it.
@cardoso, any action expected from me? |
@AnastasiyaHladina not really, it should be merged once all reviews have been made. We're short on reviewers currently due to in-person events this month. |
|
Thanks for the contrib! Can you pls fix the conflict? 👀 |
AnastasiyaHladina
dismissed stale reviews from d-gubert and cardoso
via
136ecee
@KevLehman, fixed. |
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dougfabris
added
valid
9 participants
Proposed changes (including videos or screenshots)
This PR removes the uuid package usage and replaces it with the crypto module that was added to Node.js.
Summary by CodeRabbit
No functional or UX changes expected.
✏️ Tip: You can customize this high-level summary in your review settings.