refactor(client): use SDK storage helper in AuthenticationProvider.getLoginToken#40482
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 |
|
WalkthroughReplaces Meteor/Tracker-based reactive hooks with React’s ChangesReactivity migration & subscription bridges
Login token storage access
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor/drop-unstore-login-token-monkey-patch #40482 +/- ##
==================================================================================
+ Coverage 69.64% 69.68% +0.03%
==================================================================================
Files 3318 3318
Lines 122002 121992 -10
Branches 21783 21829 +46
==================================================================================
+ Hits 84973 85012 +39
+ Misses 33701 33658 -43
+ Partials 3328 3322 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira ARCH-2116 |
…tLoginToken up the AuthenticationProvider so the last Accounts.storageLocation / Accounts.LOGIN_TOKEN_KEY reference in the client is gone. No behaviour change: same window.localStorage backend, same 'Meteor.loginToken' key. The Accounts import stays because callLoginMethod / _unstoreLoginToken / loggingIn are still used here.
ggazzo
force-pushed
the
refactor/auth-provider-login-token-helper
branch
from
e4cbd03 to
eda14b6
Compare
ggazzo
changed the base branch from
develop
to
refactor/drop-unstore-login-token-monkey-patch
…in providers (#40446) Co-authored-by: Tasso Evangelista <tasso.evangelista@rocket.chat>
![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/providers/ServerProvider.tsx (1)
136-149: 💤 Low valueRefresh
cachedStatuswhen the bridge installs to avoid a stale first snapshot.
cachedStatusis computed once at module load (line 136), but the'connection'listener is only attached lazily insideensureStatusBridgeon the firstsubscribeStatuscall. Any DDPSDK/Meteor transitions that happen between module evaluation and the first React subscription are not captured, so the initial value React reads fromgetStatusSnapshot()can be stale relative to the actual current status. Recomputing on install closes that window.♻️ Proposed fix
const ensureStatusBridge = (): void => { if (statusBridgeStarted) return; statusBridgeStarted = true; + cachedStatus = computeStatus(); getDdpSdk().connection.on('connection', () => { const next = computeStatus(); if (isStatusEqual(cachedStatus, next)) return; cachedStatus = next; statusListeners.forEach((cb) => cb()); }); };🤖 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/providers/ServerProvider.tsx` around lines 136 - 149, cachedStatus is computed at module load but may be stale by the time ensureStatusBridge installs the DDPSDK 'connection' listener; inside ensureStatusBridge (before calling getDdpSdk().connection.on('connection', ...)), recompute the current status via computeStatus(), compare with cachedStatus using isStatusEqual, and if different update cachedStatus and invoke each callback in statusListeners so React subscribers get an up-to-date initial snapshot; keep statusBridgeStarted toggling as-is to avoid double-installation.
🤖 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/providers/AuthenticationProvider/AuthenticationProvider.tsx`:
- Around line 41-48: The current patch silently returns when
Accounts._setLoggingIn is missing which leaves isLoggingIn frozen; update the
hook so that if typeof wrap._setLoggingIn !== 'function' you log a dev-only
warning via console.warn (or process.env.NODE_ENV check) that
Accounts._setLoggingIn is unavailable and the login-state bridge may be broken,
and implement a fallback subscription that notifies loggingInListeners (and thus
useSyncExternalStore) when Tracker or Accounts.loggingIn() changes (e.g., use
Tracker.autorun to watch Accounts.loggingIn() and call each loggingInListeners
callback) so the component remains responsive even if _setLoggingIn is removed.
---
Nitpick comments:
In `@apps/meteor/client/providers/ServerProvider.tsx`:
- Around line 136-149: cachedStatus is computed at module load but may be stale
by the time ensureStatusBridge installs the DDPSDK 'connection' listener; inside
ensureStatusBridge (before calling getDdpSdk().connection.on('connection',
...)), recompute the current status via computeStatus(), compare with
cachedStatus using isStatusEqual, and if different update cachedStatus and
invoke each callback in statusListeners so React subscribers get an up-to-date
initial snapshot; keep statusBridgeStarted toggling as-is to avoid
double-installation.
🪄 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: d4e13466-a3c7-4eae-aa4f-6097c81faacf
📒 Files selected for processing (3)
apps/meteor/client/hooks/useReactiveValue.tsapps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsxapps/meteor/client/providers/ServerProvider.tsx
💤 Files with no reviewable changes (1)
- apps/meteor/client/hooks/useReactiveValue.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:
apps/meteor/client/providers/ServerProvider.tsxapps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx
🧠 Learnings (2)
📚 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/providers/ServerProvider.tsxapps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx
📚 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/providers/ServerProvider.tsxapps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx
🔇 Additional comments (1)
apps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx (1)
154-154: LGTM!
| const wrap = Accounts as unknown as { _setLoggingIn?: (v: boolean) => void }; | ||
| const original = wrap._setLoggingIn; | ||
| if (typeof original !== 'function') return; | ||
| wrap._setLoggingIn = function (this: typeof Accounts, v: boolean) { | ||
| original.call(this, v); | ||
| loggingInListeners.forEach((cb) => cb()); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Silent fallback when _setLoggingIn is unavailable will leave isLoggingIn frozen.
Relying on the internal Accounts._setLoggingIn makes the bridge inherently fragile: if Meteor renames/removes it, typeof original !== 'function' triggers a silent return, useSyncExternalStore never receives notifications, and isLoggingIn stays at whatever Accounts.loggingIn() returned on first mount with no diagnostic. At minimum, log a warning in dev so this regression is observable; consider a Tracker-based fallback subscription if you want resilience without changing the rest of the API.
🛡️ Minimal diagnostic
const original = wrap._setLoggingIn;
- if (typeof original !== 'function') return;
+ if (typeof original !== 'function') {
+ console.warn('[AuthenticationProvider] Accounts._setLoggingIn unavailable; isLoggingIn will not update');
+ return;
+ }🤖 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/providers/AuthenticationProvider/AuthenticationProvider.tsx`
around lines 41 - 48, The current patch silently returns when
Accounts._setLoggingIn is missing which leaves isLoggingIn frozen; update the
hook so that if typeof wrap._setLoggingIn !== 'function' you log a dev-only
warning via console.warn (or process.env.NODE_ENV check) that
Accounts._setLoggingIn is unavailable and the login-state bridge may be broken,
and implement a fallback subscription that notifies loggingInListeners (and thus
useSyncExternalStore) when Tracker or Accounts.loggingIn() changes (e.g., use
Tracker.autorun to watch Accounts.loggingIn() and call each loggingInListeners
callback) so the component remains responsive even if _setLoggingIn is removed.
1 participant
Summary
Drop the last
Accounts.storageLocationreference in the client.#40477 introduced
getLoginTokenviaAccounts.storageLocation.getItem(Accounts.LOGIN_TOKEN_KEY). #40479 then centralized that same access pattern behindgetStoredItem(STORAGE_KEYS.LOGIN_TOKEN)but couldn't touch this line because the two PRs were in flight at the same time. This is the catch-up.No behaviour change: same
window.localStoragebackend, same'Meteor.loginToken'key. TheAccountsimport stays in this file becausecallLoginMethod/_unstoreLoginToken/loggingInare still used here.Test plan
yarn eslinton the changed file: 0 errors/oauth/authorize?...while logged in; the hiddenaccess_tokenfield is populated from the stored login token and the form submits.useLoginToken()continues to return the same token for any consumer that reads it (e.g.AuthorizationFormPage).Summary by CodeRabbit
Task: ARCH-2137