refactor(client): replace Accounts.storageLocation with SDK storage helper - #40479
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📜 Recent 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). (3)
WalkthroughThis PR introduces a centralized client-side storage module and replaces direct ChangesStorage Centralization Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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 |
Member
Author
|
/jira ARCH-2116 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/lib/sdk/storage.ts (2)
14-22: ⚡ Quick winType the helper API as
StorageKey.
StorageKeyis defined here, but the exported helpers still accept anystring, so key typos in auth/E2EE call sites won't be caught at compile time. Tightening the signatures here keeps the abstraction aligned with the intent ofSTORAGE_KEYS.Suggested diff
export type StorageKey = (typeof STORAGE_KEYS)[keyof typeof STORAGE_KEYS]; const getStorage = (): Storage | undefined => (typeof window !== 'undefined' ? window.localStorage : undefined); -export const getStoredItem = (key: string): string | null => getStorage()?.getItem(key) ?? null; +export const getStoredItem = (key: StorageKey): string | null => getStorage()?.getItem(key) ?? null; -export const setStoredItem = (key: string, value: string): void => getStorage()?.setItem(key, value); +export const setStoredItem = (key: StorageKey, value: string): void => getStorage()?.setItem(key, value); -export const removeStoredItem = (key: string): void => getStorage()?.removeItem(key); +export const removeStoredItem = (key: StorageKey): void => getStorage()?.removeItem(key);🤖 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/sdk/storage.ts` around lines 14 - 22, The exported helpers accept plain strings but should use the exported StorageKey type to prevent key typos; update the function signatures of getStoredItem, setStoredItem, and removeStoredItem to take key: StorageKey (and keep their return/types the same), leaving getStorage() unchanged; ensure callers using these helpers pass a STORAGE_KEYS member (or cast) so the compiler enforces valid keys.
1-4: ⚡ Quick winRemove the module header comments.
This helper is small enough to be self-explanatory, and the new inline comments conflict with the repo rule for TS/JS implementation files.
As per coding guidelines,
**/*.{ts,tsx,js}: Avoid code comments in the implementation.🤖 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/sdk/storage.ts` around lines 1 - 4, Remove the module header comment block at the top of the file (the multi-line comment describing purpose and storage behavior); delete those lines so the file contains only the implementation code, and ensure there are no other TS/JS implementation comments remaining to comply with the repository rule for /*.{ts,tsx,js} files.
🤖 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.
Nitpick comments:
In `@apps/meteor/client/lib/sdk/storage.ts`:
- Around line 14-22: The exported helpers accept plain strings but should use
the exported StorageKey type to prevent key typos; update the function
signatures of getStoredItem, setStoredItem, and removeStoredItem to take key:
StorageKey (and keep their return/types the same), leaving getStorage()
unchanged; ensure callers using these helpers pass a STORAGE_KEYS member (or
cast) so the compiler enforces valid keys.
- Around line 1-4: Remove the module header comment block at the top of the file
(the multi-line comment describing purpose and storage behavior); delete those
lines so the file contains only the implementation code, and ensure there are no
other TS/JS implementation comments remaining to comply with the repository rule
for /*.{ts,tsx,js} files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb696dc8-58a2-42a4-9dab-1db92471d3e3
📒 Files selected for processing (5)
apps/meteor/app/utils/client/lib/RestApiClient.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/storage.tsapps/meteor/client/meteor/login/saml.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). (3)
- GitHub Check: 📦 Build Packages
- 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/app/utils/client/lib/RestApiClient.tsapps/meteor/client/meteor/login/saml.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/storage.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.ts
🧠 Learnings (4)
📚 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/app/utils/client/lib/RestApiClient.tsapps/meteor/client/meteor/login/saml.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/storage.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.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/app/utils/client/lib/RestApiClient.tsapps/meteor/client/meteor/login/saml.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/storage.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.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/app/utils/client/lib/RestApiClient.tsapps/meteor/client/meteor/login/saml.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/storage.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.ts
📚 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/meteor/login/saml.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/storage.tsapps/meteor/client/lib/e2ee/rocketchat.e2e.ts
🔇 Additional comments (4)
apps/meteor/client/lib/sdk/ddpSdk.ts (1)
70-70: Nice consolidation of the resume-token lookup.This keeps the existing auth path intact while removing another direct storage dependency from the SDK bridge.
apps/meteor/app/utils/client/lib/RestApiClient.ts (1)
15-23: Looks good.Reading both headers through the shared helper keeps this client aligned with the same legacy key names the rest of the migration is using.
apps/meteor/client/meteor/login/saml.ts (1)
77-77: This migration is clean.It preserves the current SLO behavior while removing one more direct
Accounts.storageLocationdependency from the login layer.apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (1)
318-319: Good storage migration for the E2EE state.All of the persisted key material now goes through the shared helper without changing the legacy key names, which is exactly what this refactor needs.
Also applies to: 338-338, 406-406, 425-426, 441-442, 467-473, 497-521
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40479 +/- ##
===========================================
- Coverage 69.64% 69.64% -0.01%
===========================================
Files 3317 3318 +1
Lines 121897 121973 +76
Branches 21831 21825 -6
===========================================
+ Hits 84901 84948 +47
- Misses 33678 33704 +26
- Partials 3318 3321 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…elper Accounts.storageLocation is just an alias for window.localStorage but couples every consumer to meteor/accounts-base. Centralize the persistent client storage behind apps/meteor/client/lib/sdk/storage.ts, which exposes STORAGE_KEYS (the Meteor key names we already write) and thin get/set/removeStoredItem helpers. Consumers migrated: - RestApiClient.getCredentials() reads USER_ID / LOGIN_TOKEN through the helper instead of Accounts.storageLocation.getItem. - rocketchat.e2e.ts swaps 12 Accounts.storageLocation.* calls (public_key, private_key, e2e.randomPassword) for the helper. The Accounts import stays for Accounts.onLogout, which is out of scope here (#40442). - saml.ts replaces the lone Accounts.storageLocation.removeItem with the helper. - ddpSdk.ts collapses its local readStoredLoginToken into getStoredItem so there is only one definition of the LOGIN_TOKEN key path. No behaviour change: same window.localStorage backend, same key names.
ggazzo
force-pushed
the
refactor/sdk-storage-helper
branch
from
May 11, 2026 18:12
58568fc to
6752370
Compare
tassoevan
approved these changes
May 11, 2026
tassoevan
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
ggazzo
added a commit
that referenced
this pull request
May 11, 2026
The remaining client-side Accounts._unstoreLoginToken calls fell into two roles: a monkey-patch that piggybacked on Meteor's token-clear hook to run side effects on logout, and direct calls used to manually wipe the stored credentials. Both are replaceable now that #40442 landed sdk.account.onLogout and #40479 landed the SDK storage helper. - Delete apps/meteor/client/meteor/overrides/unstoreLoginToken.ts (and its barrel entry). CachedStoresManager.ts registers clearAllCachesOnLogout via getDdpSdk().account.onLogout at module init, which fires on the same uid-undefined transition Meteor's monkey-patch was capturing. - AuthenticationProvider's unstoreLoginToken context method collapses from a monkey-patch + restore pair into `(callback) => getDdpSdk().account.onLogout(callback)`. Same return shape (callback -> unsubscribe), so useUnstoreLoginToken consumers stay intact. - AuthenticationProvider.wipeLocalAuth and the auth-error branch in ddpSdk.ts call removeStoredItem for USER_ID / LOGIN_TOKEN / LOGIN_TOKEN_EXPIRES — the three keys Meteor's _unstoreLoginToken clears. Adds LOGIN_TOKEN_EXPIRES to STORAGE_KEYS so storage.ts owns the full set. No behaviour change: same localStorage keys cleared, same logout cleanup ordering. The Accounts import stays in AuthenticationProvider.tsx and ddpSdk.ts because callLoginMethod / loggingIn / getLoginToken / the onEmailVerificationLink + onPageLoadLogin bridges still use it.
ggazzo
added a commit
that referenced
this pull request
May 14, 2026
The remaining client-side Accounts._unstoreLoginToken calls fell into two roles: a monkey-patch that piggybacked on Meteor's token-clear hook to run side effects on logout, and direct calls used to manually wipe the stored credentials. Both are replaceable now that #40442 landed sdk.account.onLogout and #40479 landed the SDK storage helper. - Delete apps/meteor/client/meteor/overrides/unstoreLoginToken.ts (and its barrel entry). CachedStoresManager.ts registers clearAllCachesOnLogout via getDdpSdk().account.onLogout at module init, which fires on the same uid-undefined transition Meteor's monkey-patch was capturing. - AuthenticationProvider's unstoreLoginToken context method collapses from a monkey-patch + restore pair into `(callback) => getDdpSdk().account.onLogout(callback)`. Same return shape (callback -> unsubscribe), so useUnstoreLoginToken consumers stay intact. - AuthenticationProvider.wipeLocalAuth and the auth-error branch in ddpSdk.ts call removeStoredItem for USER_ID / LOGIN_TOKEN / LOGIN_TOKEN_EXPIRES — the three keys Meteor's _unstoreLoginToken clears. Adds LOGIN_TOKEN_EXPIRES to STORAGE_KEYS so storage.ts owns the full set. No behaviour change: same localStorage keys cleared, same logout cleanup ordering. The Accounts import stays in AuthenticationProvider.tsx and ddpSdk.ts because callLoginMethod / loggingIn / getLoginToken / the onEmailVerificationLink + onPageLoadLogin bridges still use it.
tassoevan
pushed a commit
that referenced
this pull request
May 15, 2026
The remaining client-side Accounts._unstoreLoginToken calls fell into two roles: a monkey-patch that piggybacked on Meteor's token-clear hook to run side effects on logout, and direct calls used to manually wipe the stored credentials. Both are replaceable now that #40442 landed sdk.account.onLogout and #40479 landed the SDK storage helper. - Delete apps/meteor/client/meteor/overrides/unstoreLoginToken.ts (and its barrel entry). CachedStoresManager.ts registers clearAllCachesOnLogout via getDdpSdk().account.onLogout at module init, which fires on the same uid-undefined transition Meteor's monkey-patch was capturing. - AuthenticationProvider's unstoreLoginToken context method collapses from a monkey-patch + restore pair into `(callback) => getDdpSdk().account.onLogout(callback)`. Same return shape (callback -> unsubscribe), so useUnstoreLoginToken consumers stay intact. - AuthenticationProvider.wipeLocalAuth and the auth-error branch in ddpSdk.ts call removeStoredItem for USER_ID / LOGIN_TOKEN / LOGIN_TOKEN_EXPIRES — the three keys Meteor's _unstoreLoginToken clears. Adds LOGIN_TOKEN_EXPIRES to STORAGE_KEYS so storage.ts owns the full set. No behaviour change: same localStorage keys cleared, same logout cleanup ordering. The Accounts import stays in AuthenticationProvider.tsx and ddpSdk.ts because callLoginMethod / loggingIn / getLoginToken / the onEmailVerificationLink + onPageLoadLogin bridges still use it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Accounts.storageLocationis justwindow.localStoragewith a Meteor alias. Replace the four direct consumers with a small helper inapps/meteor/client/lib/sdk/storage.tsso no client code outside the Meteor bridges has to import frommeteor/accounts-basefor storage access.STORAGE_KEYS(mirrors the existing Meteor key names so sessions persist across the migration) andgetStoredItem/setStoredItem/removeStoredItemhelpers.RestApiClient.getCredentials()readsUSER_ID/LOGIN_TOKENthrough the helper instead ofAccounts.storageLocation.getItem.rocketchat.e2e.tsswaps its 12Accounts.storageLocation.*calls (public_key, private_key, e2e.randomPassword) for the helper. TheAccountsimport stays forAccounts.onLogout, which is migrated separately in refactor(client): migrate Accounts.on* event handlers to sdk.account #40442.saml.tsreplaces the loneAccounts.storageLocation.removeItem(USER_ID_KEY).ddpSdk.tscollapses its privatereadStoredLoginTokenintogetStoredItem(STORAGE_KEYS.LOGIN_TOKEN)so there's only one definition of the token key path.Out of scope:
Accounts.onLogin/onLogout/onPageLoadLogin/onEmailVerificationLink(#40442),Accounts._unstoreLoginToken/_storedLoginToken,Accounts.LoginCancelledError,Accounts.registerClientLoginFunction,Accounts.oauth.credentialRequestCompleteHandler. TheAuthorizationFormPage.tsxusage is handled by #40477'suseLoginTokenhook.No behaviour change: same
window.localStoragebackend, same key names.Test plan
Meteor.userIdandMeteor.loginTokenkeys still appear in DevTools › Application › Local Storage, and the app stays logged in after refresh.public_key/private_keyare read back correctly and rooms decrypt; change password and confirme2e.randomPasswordupdates.X-User-Id/X-Auth-Tokenheaders.Summary by CodeRabbit
Task: ARCH-2133