refactor(client): drop misc Meteor type/value imports from 2FA + startup#40556
Conversation
|
/jira ARCH-2116 |
|
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 |
|
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:
WalkthroughAdds a shared ChangesMeteor Error Type Abstraction in 2FA and Account Flows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40556 +/- ##
===========================================
+ Coverage 69.64% 69.65% +0.01%
===========================================
Files 3325 3320 -5
Lines 122799 122709 -90
Branches 21880 21899 +19
===========================================
- Hits 85519 85477 -42
+ Misses 33942 33887 -55
- Partials 3338 3345 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](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 (1)
apps/meteor/client/lib/2fa/utils.ts (1)
10-14: ⚡ Quick winAdd object check for consistent type guard validation.
isTotpInvalidErrorandisTotpMaxAttemptsErrordon't validate thaterroris an object before narrowing toMeteorErrorLike(which extendsError). While runtime-safe due to optional chaining, this creates a type safety gap and is inconsistent withisTotpRequiredError(line 6). For correctness and consistency, add thetypeof error === 'object'check.🔒 Proposed fix to add object validation
export const isTotpInvalidError = ( error: unknown, ): error is MeteorErrorLike & ({ error: 'totp-invalid' } | { errorType: 'totp-invalid' }) => + typeof error === 'object' && + ( (error as { error?: unknown } | undefined)?.error === 'totp-invalid' || - (error as { errorType?: unknown } | undefined)?.errorType === 'totp-invalid'; + (error as { errorType?: unknown } | undefined)?.errorType === 'totp-invalid' + ); export const isTotpMaxAttemptsError = ( error: unknown, ): error is MeteorErrorLike & ({ error: 'totp-max-attempts' } | { errorType: 'totp-max-attempts' }) => + typeof error === 'object' && + ( (error as { error?: unknown } | undefined)?.error === 'totp-max-attempts' || - (error as { errorType?: unknown } | undefined)?.errorType === 'totp-max-attempts'; + (error as { errorType?: unknown } | undefined)?.errorType === 'totp-max-attempts' + );Also applies to: 16-20
🤖 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/lib/2fa/utils.ts` around lines 10 - 14, isTotpInvalidError (and similarly isTotpMaxAttemptsError) currently narrows error without verifying it's an object; update both type guards to first check typeof error === 'object' (and error !== null) before checking .error or .errorType so the runtime check matches isTotpRequiredError and safely narrows to MeteorErrorLike; locate the functions by name (isTotpInvalidError, isTotpMaxAttemptsError) and add the object-type guard to the existing boolean expression.
🤖 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/client/lib/2fa/process2faReturn.ts`:
- Line 161: The rejection currently uses reject(new Error('totp-canceled'))
which lacks the .error property expected by downstream code (LoginForm's
onError). Change the rejection to produce a Meteor.Error with the same code
(e.g. use new Meteor.Error('totp-canceled')) or, if you must keep a plain Error,
attach an .error = 'totp-canceled' property before rejecting in
process2faReturn.ts so that the downstream error.error lookup continues to work.
---
Nitpick comments:
In `@apps/meteor/client/lib/2fa/utils.ts`:
- Around line 10-14: isTotpInvalidError (and similarly isTotpMaxAttemptsError)
currently narrows error without verifying it's an object; update both type
guards to first check typeof error === 'object' (and error !== null) before
checking .error or .errorType so the runtime check matches isTotpRequiredError
and safely narrows to MeteorErrorLike; locate the functions by name
(isTotpInvalidError, isTotpMaxAttemptsError) and add the object-type guard to
the existing boolean expression.
🪄 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: 597bc40d-2d75-4c1d-a3d7-de31c6a2d599
📒 Files selected for processing (8)
apps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/lib/2fa/types.tsapps/meteor/client/lib/2fa/utils.tsapps/meteor/client/meteor/overrides/absoluteUrl.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/meteor/startup/index.ts
💤 Files with no reviewable changes (1)
- apps/meteor/client/meteor/startup/index.ts
📜 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). (15)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test API (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test API Livechat (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test API Livechat (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.0 (1/1)
- GitHub Check: 🔨 Test Federation Matrix
🧰 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/lib/2fa/types.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/utils.ts
🧠 Learnings (5)
📚 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/lib/2fa/types.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/utils.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/lib/2fa/types.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/utils.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/client/lib/2fa/types.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/utils.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/client/lib/2fa/types.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/utils.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:
apps/meteor/client/lib/2fa/types.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/utils.ts
🔇 Additional comments (4)
apps/meteor/client/meteor/overrides/index.ts (1)
1-1: LGTM!apps/meteor/client/lib/2fa/types.ts (1)
1-9: LGTM!apps/meteor/client/lib/2fa/overrideLoginMethod.ts (1)
1-8: LGTM!apps/meteor/client/meteor/startup/accounts.ts (1)
58-58: Verify that downstream code doesn't rely on Meteor.Error fields.The error construction changed from
new Meteor.Error('verify-email', 'E-mail not verified', error)tonew Error('verify-email: E-mail not verified', { cause: error }). This removes theerror,errorType,reason, anddetailsfields that Meteor.Error provides.A codebase-wide search found no downstream handlers checking for
error.error === 'verify-email'or accessing these structured fields for this specific error. The error is used only to display a toast message (which uses the Error's message property) and then rethrown—no upstream catch handler was found that would need the Meteor.Error structure. The change is safe.
| }); | ||
| }); | ||
| reject(new Meteor.Error('totp-canceled')); | ||
| reject(new Error('totp-canceled')); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for code that might check for 'totp-canceled' in error fields
rg -nP --type=ts --type=js -C3 '\b(error|errorType)\b.*totp-canceled|totp-canceled.*\b(error|errorType)\b'Repository: RocketChat/Rocket.Chat
Length of output: 490
🏁 Script executed:
# Check the context around the LoginForm.tsx error comparison
rg -n -B5 -A5 "if \(error === 'totp-canceled'\)" packages/web-ui-registration/src/LoginForm.tsxRepository: RocketChat/Rocket.Chat
Length of output: 236
🏁 Script executed:
# Search for calls to process2faReturn to understand error handling
rg -n "process2faReturn" --type=ts --type=js -A2 -B2Repository: RocketChat/Rocket.Chat
Length of output: 3448
🏁 Script executed:
# Search for openTwoFactorModal and error handling
rg -n "openTwoFactorModal" --type=ts --type=js -B3 -A3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for where 'totp-canceled' string is used as error message
rg -n "totp-canceled" --type=ts --type=jsRepository: RocketChat/Rocket.Chat
Length of output: 382
🏁 Script executed:
# Check the process2faReturn function signature and return values
rg -n "export.*process2faReturn|reject.*totp-canceled|resolve" apps/meteor/client/lib/2fa/process2faReturn.ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 564
🏁 Script executed:
# Find where the error from process2faReturn is caught/handled
rg -n "process2faReturn\(" --type=ts --type=js -A10 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 3446
🏁 Script executed:
# Check the openTwoFactorModal function and how it handles errors
rg -n "openTwoFactorModal|function openTwoFactorModal" --type=ts --type=js -B3 -A15 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Find the complete process2faReturn function to see all paths
cat -n apps/meteor/client/lib/2fa/process2faReturn.tsRepository: RocketChat/Rocket.Chat
Length of output: 6039
🏁 Script executed:
# Search for where LoginForm is used and where the 'error' prop comes from
rg -n "LoginForm" --type=ts --type=tsx -B3 -A3 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 94
🏁 Script executed:
# Search for modal/dialog showing with error handling
rg -n "imperativeModal\|openModal" apps/meteor/client/lib/2fa/process2faReturn.ts -B5 -A5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Find where LoginForm is used with the error prop
rg -n "LoginForm" packages/web-ui-registration -B5 -A5 --type=tsxRepository: RocketChat/Rocket.Chat
Length of output: 94
🏁 Script executed:
# Search for error handling around the LoginForm usage context
rg -n "error === 'totp-canceled'|LoginErrors" packages/web-ui-registration/src/LoginForm.tsx -B10 -A2Repository: RocketChat/Rocket.Chat
Length of output: 1273
🏁 Script executed:
# Find what calls the modal and how it catches the error
rg -n "invokeTwoFactorModal\|TwoFactorModal" --type=ts --type=tsx -B5 -A15 | head -150Repository: RocketChat/Rocket.Chat
Length of output: 94
🏁 Script executed:
# Search for where the rejected error is caught and handled
rg -n "catch|\.message|Error.*totp-canceled" apps/meteor/client/lib/2fa/ --type=ts -B3 -A3Repository: RocketChat/Rocket.Chat
Length of output: 2202
🏁 Script executed:
# Find files that catch the invokeTwoFactorModal promise and how they handle it
rg -n "invokeTwoFactorModal" apps/meteor/client -B2 -A10 --type=tsRepository: RocketChat/Rocket.Chat
Length of output: 2610
🏁 Script executed:
# Look for error.message usage related to 2fa
rg -n "error\.message|error\.toString" apps/meteor/client/lib/2fa/ --type=tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the LoginError type definition
rg -n "type LoginError|interface LoginError" apps/meteor/client --type=ts -B2 -A5Repository: RocketChat/Rocket.Chat
Length of output: 828
🏁 Script executed:
# Look at the callback function handling in overrideLoginMethod more carefully
cat -n apps/meteor/client/lib/2fa/overrideLoginMethod.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 4124
🏁 Script executed:
# Check what happens when the error is passed to callback
rg -n "callback.*error|setErrorOnSubmit" apps/meteor/client --type=ts -B3 -A3 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 6110
🏁 Script executed:
# Check if there's any conversion of Error to string before reaching LoginForm
rg -n "setErrorOnSubmit|error\.message|String.*error|\.toString" packages/web-ui-registration/src/LoginForm.tsx --type=ts -B3 -A3Repository: RocketChat/Rocket.Chat
Length of output: 733
🏁 Script executed:
# Search for how the error object flows through the authentication provider
rg -n "setErrorOnSubmit|LoginErrors" packages/web-ui-registration -B5 -A5 --type=tsRepository: RocketChat/Rocket.Chat
Length of output: 6705
🏁 Script executed:
# Check what MeteorErrorLike type definition looks like
cat -n apps/meteor/client/lib/2fa/types.tsRepository: RocketChat/Rocket.Chat
Length of output: 486
🏁 Script executed:
# Verify that the error being set in errorOnSubmit is indeed error.error from the caught error
cat -n packages/web-ui-registration/src/LoginForm.tsx | sed -n '100,145p'Repository: RocketChat/Rocket.Chat
Length of output: 1641
🏁 Script executed:
# Check if there's any transformation of the Error object before it reaches LoginForm
rg -n "useLoginWithPassword|loginMutation" packages/web-ui-registration/src/LoginForm.tsx -B3 -A8Repository: RocketChat/Rocket.Chat
Length of output: 3376
Plain Error lacks the .error property that downstream code depends on.
Changing from new Meteor.Error('totp-canceled') to new Error('totp-canceled') breaks downstream error handling. The LoginForm component's onError handler (packages/web-ui-registration/src/LoginForm.tsx, lines 101-115) explicitly accesses error.error to extract the error code and compare it against known error types. A plain Error has no .error property, so the value will be undefined instead of 'totp-canceled', causing the error check at line 140 to fail. Use new Meteor.Error('totp-canceled') or add an .error property to the plain Error object to preserve the expected behavior.
🤖 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/lib/2fa/process2faReturn.ts` at line 161, The rejection
currently uses reject(new Error('totp-canceled')) which lacks the .error
property expected by downstream code (LoginForm's onError). Change the rejection
to produce a Meteor.Error with the same code (e.g. use new
Meteor.Error('totp-canceled')) or, if you must keep a plain Error, attach an
.error = 'totp-canceled' property before rejecting in process2faReturn.ts so
that the downstream error.error lookup continues to work.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
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/lib/2fa/process2faReturn.ts">
<violation number="1" location="apps/meteor/client/lib/2fa/process2faReturn.ts:161">
P1: `new Error('totp-canceled')` lacks the `.error` property that downstream code depends on. `Meteor.Error` stores its first argument as the `.error` property (i.e., `err.error === 'totp-canceled'`), while a plain `Error` only sets `.message`. The LoginForm `onError` handler reads `error.error` to detect `'totp-canceled'` and suppress the error display — this will now always be `undefined`, breaking the cancellation flow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
| }); | ||
| reject(new Meteor.Error('totp-canceled')); | ||
| reject(new Error('totp-canceled')); |
There was a problem hiding this comment.
P1: new Error('totp-canceled') lacks the .error property that downstream code depends on. Meteor.Error stores its first argument as the .error property (i.e., err.error === 'totp-canceled'), while a plain Error only sets .message. The LoginForm onError handler reads error.error to detect 'totp-canceled' and suppress the error display — this will now always be undefined, breaking the cancellation flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/2fa/process2faReturn.ts, line 161:
<comment>`new Error('totp-canceled')` lacks the `.error` property that downstream code depends on. `Meteor.Error` stores its first argument as the `.error` property (i.e., `err.error === 'totp-canceled'`), while a plain `Error` only sets `.message`. The LoginForm `onError` handler reads `error.error` to detect `'totp-canceled'` and suppress the error display — this will now always be `undefined`, breaking the cancellation flow.</comment>
<file context>
@@ -158,7 +158,7 @@ export const invokeTwoFactorModal = async (
});
});
- reject(new Meteor.Error('totp-canceled'));
+ reject(new Error('totp-canceled'));
}
},
</file context>
Three small cleanups that don't fit any larger cluster:
- lib/2fa/utils.ts only imported `import type { Meteor }` to spell
`Meteor.Error` inside type-guard return types. The guards themselves
check `.error` / `.errorType` structurally — they never instanceof —
so a local `MeteorErrorLike = Error & { error?; errorType?; reason?;
details? }` type covers both `Meteor.Error` and `Meteor.TypedError`
shapes without pulling `meteor/meteor` for types.
- lib/2fa/overrideLoginMethod.ts switches its `LoginError` alias from
`globalThis.Error | Meteor.Error | Meteor.TypedError` to
`MeteorErrorLike` for the same reason. The `Accounts` value import
stays — `Accounts.callLoginMethod` migrates separately.
- lib/2fa/process2faReturn.ts narrows `hasRequiredTwoFactorMethod` and
the public function signatures to `MeteorErrorLike` so the new local
type composes through. The `Meteor` value import stays for the runtime
`new Meteor.Error('totp-canceled')` thrown when the 2FA modal closes
— replacing that requires auditing the catch sites and is out of scope.
- meteor/startup/absoluteUrl.ts patches `Meteor.absoluteUrl` /
`Meteor._relativeToSiteRootUrl` on the client globals. Zero callers
read either on the client (verified by grep — every consumer is
server-side, where Meteor's own absoluteUrl runs). The file already
carried a TODO marking itself as dead once DDP is gone. Delete it.
The Meteor.absoluteUrl / Meteor._relativeToSiteRootUrl patch lives until Meteor's DDP layer goes away. Park it under meteor/overrides/ alongside the other DDP-layer patches so the whole folder can exit together. Same content, same load semantics — overrides/index already runs at startup via main.ts.
…erify
Two runtime `new Meteor.Error(code, reason)` callsites in non-OAuth /
non-overrides client code. Neither is `instanceof`-checked anywhere — the
only `instanceof Meteor.Error` in the client lives in OAuth login flow,
out of scope here — and neither error code is read by any consumer:
- meteor/startup/accounts.ts throws 'verify-email' from inside the
email-verification onLink handler's catch. The throw propagates as an
unhandled rejection (the handler is fire-and-forget). Replace with
`new Error('verify-email: E-mail not verified', { cause: error })` so
the original failure stays attached for logging.
- lib/2fa/process2faReturn.ts rejects with 'totp-canceled' when the 2FA
modal closes without a confirm. The reject bubbles into the
process2faAsyncReturn / process2faReturn callers, which only forward
via callback — no consumer reads `.error`. Plain `new Error('totp-canceled')`
keeps the rejection identifiable by message.
Drops the `Meteor` value import from both files. lib/2fa/ is now
Meteor-free.
ggazzo
force-pushed
the
refactor/drop-misc-meteor-imports
branch
from
d5703e2 to
60aeb6c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/client/lib/2fa/process2faReturn.ts (1)
19-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate every field this type guard narrows.
The predicate says
details.emailOrUsernameisstring | undefined, but it only checksmethod. Inprocess2faAsyncReturn, a truthy non-string value will bypass the fallback and then failassertModalProps, turning a 2FA flow intoInvalid Two Factor method. Either validateemailOrUsernamehere or remove it from the narrowed type.Proposed fix
const hasRequiredTwoFactorMethod = ( error: MeteorErrorLike, ): error is MeteorErrorLike & { details: { method: TwoFactorMethod; emailOrUsername?: string } } => { const details = error.details as unknown; return ( typeof details === 'object' && details !== null && typeof (details as { method: unknown }).method === 'string' && + ((details as { emailOrUsername?: unknown }).emailOrUsername === undefined || + typeof (details as { emailOrUsername?: unknown }).emailOrUsername === 'string') && isTwoFactorMethod((details as { method: string }).method) ); };🤖 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/lib/2fa/process2faReturn.ts` around lines 19 - 20, The type guard currently narrows error.details to include emailOrUsername but only checks method; update the predicate (the arrow function that takes error: MeteorErrorLike) to also validate error.details.emailOrUsername (ensure error.details exists and that emailOrUsername is either undefined or a string via a typeof check) or else remove emailOrUsername from the narrowed type; make this change where the type guard is defined so callers like process2faAsyncReturn and assertModalProps cannot receive a falsely-narrowed details object.
🤖 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.
Duplicate comments:
In `@apps/meteor/client/lib/2fa/process2faReturn.ts`:
- Around line 19-20: The type guard currently narrows error.details to include
emailOrUsername but only checks method; update the predicate (the arrow function
that takes error: MeteorErrorLike) to also validate
error.details.emailOrUsername (ensure error.details exists and that
emailOrUsername is either undefined or a string via a typeof check) or else
remove emailOrUsername from the narrowed type; make this change where the type
guard is defined so callers like process2faAsyncReturn and assertModalProps
cannot receive a falsely-narrowed details object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7ed7963-8afa-43f3-b79a-43f9ae52aeb9
📒 Files selected for processing (8)
apps/meteor/client/lib/2fa/overrideLoginMethod.tsapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/lib/2fa/types.tsapps/meteor/client/lib/2fa/utils.tsapps/meteor/client/meteor/overrides/absoluteUrl.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/startup/accounts.tsapps/meteor/client/meteor/startup/index.ts
💤 Files with no reviewable changes (1)
- apps/meteor/client/meteor/startup/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/client/meteor/overrides/index.ts
- apps/meteor/client/lib/2fa/overrideLoginMethod.ts
- apps/meteor/client/lib/2fa/types.ts
- apps/meteor/client/meteor/startup/accounts.ts
- apps/meteor/client/lib/2fa/utils.ts
📜 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/lib/2fa/process2faReturn.ts
🧠 Learnings (5)
📚 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/lib/2fa/process2faReturn.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/lib/2fa/process2faReturn.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/client/lib/2fa/process2faReturn.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/client/lib/2fa/process2faReturn.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:
apps/meteor/client/lib/2fa/process2faReturn.ts
🔇 Additional comments (2)
apps/meteor/client/lib/2fa/process2faReturn.ts (2)
161-161:totp-canceledstill needs the Meteor-style error code.This is the same downstream contract issue already called out earlier: a plain
Errorstill drops the.errorfield that login error handling reads.
6-6: LGTM!Also applies to: 64-64
The three former consumers (useReactiveValue, AuthorizationProvider, UserProvider) all switched to native useSyncExternalStore wiring across prior PRs. No remaining importers — drop the file (and the last non-overrides meteor/tracker import outside Minimongo).
Drop redundant `LoginError = MeteorErrorLike` alias per review.
|
@cubic-dev-ai about the |
@ggazzo Yep, that matches what the code shows. The The grep confirming zero other references seals it — there's no hidden |
SDKClient.ts:406 referenced the now-removed DDPCommon namespace inside the sdk-transport-on branch added by #40542. The DDPCommon import was dropped in #40486 when the codec moved behind sdk/ddpProtocol; the follow-up landed without rebasing onto that, so TS now blows up: TS2708: Cannot use namespace 'DDPCommon' as a value. Same parseDDP helper is already used a few lines up — just call it.
| await whenMainReady(); | ||
| dispatchToastMessage({ type: 'error', message: error }); | ||
| throw new Meteor.Error('verify-email', 'E-mail not verified'); | ||
| throw new Error('verify-email: E-mail not verified', { cause: error }); |
There was a problem hiding this comment.
Why not a custom Error subclass?
2 participants
Draft — three small cleanups grouped to avoid one-liner PR spam.
Summary
Test plan
Task: ARCH-2151
Summary by CodeRabbit