fix: stop useSubscriptions re-querying on connection state changes - #7559
fix: stop useSubscriptions re-querying on connection state changes#7559deepak0x wants to merge 5 commits into
Conversation
useSubscriptions selected the entire state.server object and listed it in the useEffect dependency array. state.server holds connection-state fields (connecting/loading/connected) that change on every transition, producing a new object reference and re-running the effect, which re-subscribes and re-queries WatermelonDB on each connection change and makes the room list flicker. The selected server value was never used inside the effect, so remove the unused selector and its dependency. The query now only re-runs when the subscription-relevant preferences actually change. Fixes RocketChat#7437 Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesSubscription query stability
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/views/RoomsListView/hooks/useSubscriptions.test.tsx (1)
39-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return annotations to the test functions.
Add a return type to
rootReducerandHarness. Keep the fixture partial. Do not cast it toIApplicationState.As per coding guidelines, TypeScript functions require explicit parameter and return annotations. Based on learnings, partial selector fixtures must not require
IApplicationState.🤖 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/views/RoomsListView/hooks/useSubscriptions.test.tsx` around lines 39 - 45, Update rootReducer and Harness with explicit TypeScript parameter and return type annotations, while keeping the fixture state partial and avoiding any cast to IApplicationState.Sources: Coding guidelines, Learnings
🤖 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 `@app/views/RoomsListView/hooks/useSubscriptions.test.tsx`:
- Line 7: Remove the unused database import from useSubscriptions.test.tsx; keep
any jest.mock usage unchanged, since it does not require a local database
binding.
- Around line 48-74: Update the test around Harness and getSubscriptions to be
asynchronous, awaiting the initial subscription setup after render so
queryCallCount reflects observeWithColumns. After each UPDATE_SERVER dispatch,
wait for the related effect work to settle before asserting that queryCallCount
remains equal to initialCalls.
In `@app/views/RoomsListView/hooks/useSubscriptions.ts`:
- Line 126: Update the dependency array of the subscription effect in
useSubscriptions to include the stable active database/server identity used by
database.active, so changing the active database re-subscribes and stops using
the previous database’s emissions. Add a test that switches the active database
or server identity and verifies the subscription is refreshed.
---
Nitpick comments:
In `@app/views/RoomsListView/hooks/useSubscriptions.test.tsx`:
- Around line 39-45: Update rootReducer and Harness with explicit TypeScript
parameter and return type annotations, while keeping the fixture state partial
and avoiding any cast to IApplicationState.
🪄 Autofix
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 Plus
Run ID: 3908eeb0-0c5d-4b19-9ef5-ea051c3f83e9
📒 Files selected for processing (2)
app/views/RoomsListView/hooks/useSubscriptions.test.tsxapp/views/RoomsListView/hooks/useSubscriptions.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
**/*.{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 numbers
Files:
app/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
🧠 Learnings (4)
📚 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/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/RoomsListView/hooks/useSubscriptions.test.tsx
📚 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/views/RoomsListView/hooks/useSubscriptions.test.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/views/RoomsListView/hooks/useSubscriptions.test.tsx
🪛 ESLint
app/views/RoomsListView/hooks/useSubscriptions.test.tsx
[error] 7-7: 'database' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 7-7: Missing file extension for "../../lib/database"
(import/extensions)
Removes the unused database import flagged by Oxlint (no-unused-vars and import/extensions). jest.mock does not require a local binding. Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
|
Addressed the CodeRabbit finding in the latest push: removed the unused |
Address CodeRabbit review on RocketChat#7559: - depend on state.server.server (the active server URL) instead of the entire state.server object. Connection-state transitions (connecting/loading) no longer re-run the effect, fixing the room-list flicker, while an actual server switch (which changes the URL and the underlying database.active) still re-subscribes correctly. - make the regression test async: wait for the initial subscription setup after render and wait for async work to settle after each dispatched connection-state change before asserting the query count is unchanged. Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
… aware Wait for the initial async subscription setup after render and for async work to settle after each dispatched connection-state change before asserting the query count is unchanged. Use state.server.server (active server URL) in the mock store to match the updated selector. Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
|
Addressed the two additional CodeRabbit findings in the latest push:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/views/RoomsListView/hooks/useSubscriptions.test.tsx`:
- Around line 61-66: Remove the unnecessary async keyword from both act
callbacks surrounding the synchronous store.dispatch calls in the subscription
test, including the UPDATE_SERVER dispatch blocks around lines 61 and 68, while
preserving the existing act wrapping and dispatch behavior.
🪄 Autofix
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 Plus
Run ID: 03f52e65-436f-4b16-ba11-75c9aa7ad220
📒 Files selected for processing (2)
app/views/RoomsListView/hooks/useSubscriptions.test.tsxapp/views/RoomsListView/hooks/useSubscriptions.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
**/*.{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 numbers
Files:
app/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
🧠 Learnings (4)
📚 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/views/RoomsListView/hooks/useSubscriptions.tsapp/views/RoomsListView/hooks/useSubscriptions.test.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/RoomsListView/hooks/useSubscriptions.test.tsx
📚 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/views/RoomsListView/hooks/useSubscriptions.test.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/views/RoomsListView/hooks/useSubscriptions.test.tsx
🪛 ESLint
app/views/RoomsListView/hooks/useSubscriptions.test.tsx
[error] 61-61: Async arrow function has no 'await' expression.
(require-await)
[error] 68-68: Async arrow function has no 'await' expression.
(require-await)
🔇 Additional comments (2)
app/views/RoomsListView/hooks/useSubscriptions.ts (1)
42-42: LGTM!Also applies to: 127-127
app/views/RoomsListView/hooks/useSubscriptions.test.tsx (1)
2-2: LGTM!Also applies to: 32-32, 47-47, 56-59, 75-77
Use the correct relative path (../../../lib/database) matching the source import so jest can resolve the module mock. Signed-off-by: Deepak Bhagat <deepak988088@gmail.com>
1 participant
Proposed changes
useSubscriptionsselected the entirestate.serverobject and listed it in theuseEffectdependency array.state.serverholds connection-state fields (connecting,loading,connected) that change on every transition, so each transition produced a new object reference and re-ran the effect. That re-subscribed and re-queried WatermelonDB on every connection change, making the room list flicker.The selected
servervalue was never used inside the effect body, so this removes the unused selector and its dependency. The query now only re-runs when subscription-relevant preferences actually change.Issue(s)
Fixes #7437
How to test or reproduce
RoomsListView.useSubscriptionseffect and re-queries the database, causing the room list to flicker on every connection change.state.server, so connection transitions don't re-query and the list stays stable.Regression test added:
app/views/RoomsListView/hooks/useSubscriptions.test.tsxrenders a harness using the hook with a mock store and a mocked database, then dispatches twostate.serverconnection-state transitions and asserts the database query is not re-run. It failed before the fix (the query ran on each transition) and passes after.Screenshots
N/A (behavior only, no visual change).
Types of changes
Checklist
Further comments
The fix is minimal and scoped: only the unused
serverselector (and its entry in the dependency array) is removed. No other behavior of the hook changes.Summary by CodeRabbit
Bug Fixes
Tests