fix: UserPreferences.getBool not parsing JSON-string booleans#7510
fix: UserPreferences.getBool not parsing JSON-string booleans#7510Rohit3523 wants to merge 3 commits into
Conversation
GitHub Actions
Error
Walkthrough
ChangesBoolean preference parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/methods/userPreferences.ts (1)
109-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive name for the stored value.
Rename
strtostoredStringorstringValueso its role in the preference parsing flow is clear.Suggested rename
- const str = this.mmkv.getString(key); - if (str !== undefined) { + const storedString = this.mmkv.getString(key); + if (storedString !== undefined) { try { - const parsed: unknown = JSON.parse(str); + const parsed: unknown = JSON.parse(storedString);As per coding guidelines, variable names should clearly convey their purpose.
🤖 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 `@app/lib/methods/userPreferences.ts` at line 109, Rename the local variable `str` in the user preference parsing flow to `storedString` or `stringValue`, and update all references within its scope to preserve the existing behavior.Source: Coding guidelines
app/lib/methods/userPreferences.test.ts (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the invalid and non-boolean JSON branches.
The new test covers
trueandfalse, but not the requirednullbehavior for values such as numbers, objects,null, or malformed JSON.Suggested regression cases
+ it('getBool returns null for non-boolean or invalid JSON strings', () => { + for (const value of ['1', 'null', '{}', 'not-json']) { + userPreferences.setString('k', value); + expect(userPreferences.getBool('k')).toBeNull(); + } + });Based on the PR objective, non-boolean parsed values must return
null.🤖 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 `@app/lib/methods/userPreferences.test.ts` around lines 11 - 16, Extend the getBool test in userPreferences.test.ts to cover JSON strings that parse to non-boolean values, including a number, object, and null, plus malformed JSON. Assert that getBool('k') returns null for each case while preserving the existing true and false assertions.
🤖 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 `@app/lib/methods/userPreferences.test.ts`:
- Around line 11-16: Extend the getBool test in userPreferences.test.ts to cover
JSON strings that parse to non-boolean values, including a number, object, and
null, plus malformed JSON. Assert that getBool('k') returns null for each case
while preserving the existing true and false assertions.
In `@app/lib/methods/userPreferences.ts`:
- Line 109: Rename the local variable `str` in the user preference parsing flow
to `storedString` or `stringValue`, and update all references within its scope
to preserve the existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eb1eea1f-2f21-400d-b99c-76cc3ced9b97
📒 Files selected for processing (2)
app/lib/methods/userPreferences.test.tsapp/lib/methods/userPreferences.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/userPreferences.test.tsapp/lib/methods/userPreferences.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript strict mode; resolve application imports relative to the
app/base URL.
Files:
app/lib/methods/userPreferences.test.tsapp/lib/methods/userPreferences.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the repository Prettier style: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where allowed, and same-line brackets.
Files:
app/lib/methods/userPreferences.test.tsapp/lib/methods/userPreferences.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Jest tests with
TZ=UTCto ensure deterministic timezone-dependent test behavior.
Files:
app/lib/methods/userPreferences.test.ts
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/userPreferences.test.tsapp/lib/methods/userPreferences.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/userPreferences.test.ts
🔇 Additional comments (2)
app/lib/methods/userPreferences.ts (1)
110-117: LGTM!app/lib/methods/userPreferences.test.ts (1)
11-16: 📐 Maintainability & Code QualityNo change needed for this test command.
The current snippet only covers JSON-parsed booleans and does not touch timezone-sensitive behavior; the repository’s
testscript already enforcesTZ=UTC.> Likely an incorrect or invalid review comment.
GitHub Actions
Inactive
|
Android Build Available Rocket.Chat 4.75.0.109401 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNT1C2hqSP5d7T5Pvpm-aZYJhRH_YTSp51sG8Fj3q6g8aM90idccn2NTVMf2J-p2UJb9-0ISCkv9Ocy5_q87 |
|
iOS Build Available Rocket.Chat 4.75.0.109402 |
1 participant
Proposed changes
Fix
getBoolinUserPreferencesto correctly read booleans stored as JSON strings byuseUserPreferences<boolean>. Previously,getBoolonly used native MMKVgetBoolean, which couldn't parse"false"(stored as a JSON string) and would returnnull/undefined. The fallback now triesgetString+JSON.parsefirst, then falls back to nativegetBoolean.This caused the haptic feedback on reconnect (from
RoomView.hapticFeedback) to ignoreNOTIFICATION_IN_APP_VIBRATIONtoggle — vibration played for every pending message replayed on DDP reconnect even when the toggle was off.Issue(s)
https://rocketchat.atlassian.net/browse/SUP-1079
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
"true"/"false").null, while existing fallback behavior remains intact.Tests
null.