chore: migrate ThreadMessagesView to hooks#5915
Conversation
OtavioStasiak
force-pushed
the
chore-migrate-to-hooks-thread-message-view
branch
2 times, most recently
from
bad06df to
45f33ed
Compare
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Some considerations. Feel free to disagree and/or discuss privately.
I don't get the appeal of useReducer here. You unified the whole state on the same reducer.
I would try to create some custom hooks instead.
| const { subscription } = this.state; | ||
| await this.initFilter(); | ||
| const init = async () => { | ||
| await initFilter(); |
There was a problem hiding this comment.
initFilter doesn't have to have to resolve a promise anymore.
It would do in the past because of UserPreferences and setState, but it's not the case now.
| // note: sync will never be called without subscription | ||
| if (!subscription._id) { | ||
| this.setState(({ messages }) => ({ messages: [...messages, ...update] })); | ||
| setMessages([...messages, ...update] as TThreadModel[]); |
There was a problem hiding this comment.
You should remove this cast
| return ( | ||
| <SafeAreaView testID='thread-messages-view'> | ||
| <StatusBar /> | ||
| {renderContent()} |
There was a problem hiding this comment.
This can be a performance issue.
This is not a ReactElement, so React needs to run the function on every render.
| ); | ||
| }; | ||
|
|
||
| const mapStateToProps = (state: IApplicationState) => ({ |
There was a problem hiding this comment.
You should remove this in favor of useAppSelector.
Same thing for theme.
OtavioStasiak
force-pushed
the
chore-migrate-to-hooks-thread-message-view
branch
3 times, most recently
from
2944b56 to
1de16de
Compare
OtavioStasiak
force-pushed
the
chore-migrate-to-hooks-thread-message-view
branch
from
1de16de to
6412b8b
Compare
OtavioStasiak
changed the title
| @@ -0,0 +1,23 @@ | |||
| import React from 'react'; | |||
| import I18n from 'i18n-js'; | |||
| @@ -0,0 +1,50 @@ | |||
| import { StyleSheet } from "react-native"; | |||
There was a problem hiding this comment.
Double quotes? You should fix your IDE.
| import React from "react"; | ||
|
|
||
| import { IBaseScreen, TSubscriptionModel, TThreadModel } from "../../definitions"; | ||
| import { ChatsStackParamList } from "../../stacks/types"; | ||
| import { TSupportedThemes } from "../../theme"; |
| searchText: string; | ||
| }; | ||
|
|
||
| export interface IUSeThreadMessages { |
| @@ -0,0 +1,76 @@ | |||
| import { useCallback } from 'react'; | |||
| import I18n from 'i18n-js'; | |||
| setDisplayingThreads: (threads: TThreadModel[]) => void; | ||
| } | ||
|
|
||
| const useThreadFilter = ({ |
There was a problem hiding this comment.
Does this need to be a hook?
It could be just functions.
| setDisplayingThreads(displayingThreads); | ||
| UserPreferences.setString(THREADS_FILTER, filter); | ||
| }, | ||
| [messages, subscription] |
There was a problem hiding this comment.
Subscription is a mutating data from WatermelonDB.
Are you sure it's going to work inside this useCallback?
Did you test it?
| subscribeMessages | ||
| }); | ||
|
|
||
| const onThreadPress = debounce( |
There was a problem hiding this comment.
A normal debounce function won't work inside a function component.
You should be using useDebounce or useDebouncedCallback
| return <BackgroundContainer text={text} />; | ||
| } | ||
| useLayoutEffect(() => { | ||
| initSubscription(); |
There was a problem hiding this comment.
You're using hooks the wrong way.
There shouldn't be a initSubscription.
useSubscription should return you a subscription to be used.
| useEffect(() => { | ||
| setHeader(); | ||
| }, [messages, currentFilter]); |
There was a problem hiding this comment.
You might be setting header every time messages change
OtavioStasiak
force-pushed
the
chore-migrate-to-hooks-thread-message-view
branch
from
81f2746 to
c8a7d69
Compare
OtavioStasiak
force-pushed
the
chore-migrate-to-hooks-thread-message-view
branch
from
3392409 to
31f1ca8
Compare
Resolve conflicts in ThreadMessagesView after migration to hooks: - swap deleted barrels (Services, lib/constants, lib/hooks) for direct paths - move HeaderButton import to Header/components, Touchable -> Touch (RNGH) - derive isMasterDetail via useMasterDetail() hook (state.app.isMasterDetail removed) - restore develop's filter badge + a11y labels, popToRoom, textInputDebounceTime; drop root-only StatusBar - relocate Item.test + snapshot into components/Item/ Claude-Session: https://claude.ai/code/session_011PSoHUDUjr9URVEQxqW91M
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughThreadMessagesView now loads subscription and thread data through hooks, filters and renders threads by the current filter, moves Item styling into a separate stylesheet, and adds snapshot-based tests for Item stories. ChangesThreadMessagesView refactor
Sequence Diagram(s)sequenceDiagram
participant ThreadMessagesView
participant useSubscription
participant database
participant useThreads
participant getThreadsList
participant getSyncThreadsList
participant updateThreads
ThreadMessagesView->>useSubscription: load subscription for rid
useSubscription->>database: read and observe subscription
ThreadMessagesView->>useThreads: start loading threads
useThreads->>database: observe threads for rid and searchText
useThreads->>getThreadsList: fetch paged threads
useThreads->>getSyncThreadsList: fetch changes since lastThreadSync
useThreads->>updateThreads: persist update and remove payloads
updateThreads->>database: batch thread changes and lastThreadSync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/views/ThreadMessagesView/index.tsx (1)
50-53: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSelector returns a fresh object each render.
The
useAppSelectorselector builds a new{ user, useRealName }object on every invocation, so without an equality function the component re-renders on every store update (not justuser/UI_Use_Real_Namechanges). Consider selecting the two values separately, or pass a shallow-equality comparator.🤖 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/ThreadMessagesView/index.tsx` around lines 50 - 53, The useAppSelector call in ThreadMessagesView is returning a new { user, useRealName } object on every selector run, which causes unnecessary rerenders. Update this selection to avoid unstable object identity by either selecting user and useRealName separately or by supplying a shallow-equality comparator to useAppSelector. Keep the fix centered on the ThreadMessagesView selector that reads getUserSelector and state.settings.UI_Use_Real_Name.app/views/ThreadMessagesView/hooks/useThreads.tsx (1)
47-72: 📐 Maintainability & Code Quality | 🔵 TrivialAwait
updateThreadsto ensure database persistence completes before updating UI state.The
updateThreadsfunction performs an asynchronous database write, but it is not awaited here. Consequently, the loading, end-of-list, and offset states are updated immediately after the network response arrives, before the data is actually saved to the local database. Awaiting the promise ensures the UI state reflects the data only after it is successfully persisted, preventing race conditions.Current code snippet
if (result.success) { updateThreads({ subscription, update: result.threads, lastThreadSync: lastThreadSync ?? new Date() }); setLoading(false); setEnd(result.count < API_FETCH_COUNT); setOffset(offset + API_FETCH_COUNT); }🤖 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/ThreadMessagesView/hooks/useThreads.tsx` around lines 47 - 72, The `load` callback in `useThreads` updates UI state before the asynchronous database write finishes because `updateThreads` is not awaited. Change the `if (result.success)` path to await `updateThreads` first, then update `setLoading`, `setEnd`, and `setOffset` only after persistence succeeds; keep the existing symbols `load`, `updateThreads`, and `getThreadsList` as the place to make the fix.
🤖 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/ThreadMessagesView/components/EmptyThreads.tsx`:
- Around line 5-19: EmptyThreads is rendering the empty-state message during the
initial fetch because it always passes only text to BackgroundContainer. Update
EmptyThreads to accept a loading prop and forward it to BackgroundContainer, and
make sure the ListEmptyComponent call site in index.tsx passes the current
loading state so the spinner shows instead of flashing “No_threads” before data
loads.
In `@app/views/ThreadMessagesView/hooks/useSubscription.tsx`:
- Line 17: The initial `subscription` state in `useSubscription` is masking the
unloaded state because `{}` is truthy and bypasses the `useThreads` guard.
Change `subscription` to start as `null` (or an explicit loading state) in
`useSubscription`, then update consumers like `useThreads` and `updateThreads`
to handle the null/uninitialized case before reading `subscription.threads` or
other fields.
In `@app/views/ThreadMessagesView/hooks/useThreads.tsx`:
- Around line 119-122: The initialization effect in useThreads is running before
the subscription data is ready, so init() always sees an empty subscription and
falls back to full load() instead of sync(). Update the useLayoutEffect around
init() and handleThreadsSubscription() to depend on subscription data from
useSubscription, ideally subscription.lastThreadSync, so the effect re-runs once
the real subscription payload arrives and the incremental sync path is taken.
In `@app/views/ThreadMessagesView/index.tsx`:
- Around line 165-174: The onThreadPress handler in ThreadMessagesView is using
useDebounce with the default trailing-only behavior, which delays thread
navigation by 1 second on every tap. Update the useDebounce call for
onThreadPress so navigation happens immediately by enabling leading execution
(for example via the third options argument), while keeping the existing
Navigation.popToRoom and Navigation.push logic unchanged.
In `@app/views/ThreadMessagesView/methods/updateThreads.ts`:
- Around line 34-38: Filter out null results from buildMessage before using _id
in updateThreads. In updateThreads.ts, after mapping with buildMessage, remove
any null entries (or narrow the array) before the threadsToCreate and
threadsToUpdate filters so i1._id and i2._id are only accessed on non-null
messages. Keep the logic centered around buildMessage, threadsToCreate, and
threadsToUpdate, and ensure the later similar _id dereference is also guarded
the same way.
---
Nitpick comments:
In `@app/views/ThreadMessagesView/hooks/useThreads.tsx`:
- Around line 47-72: The `load` callback in `useThreads` updates UI state before
the asynchronous database write finishes because `updateThreads` is not awaited.
Change the `if (result.success)` path to await `updateThreads` first, then
update `setLoading`, `setEnd`, and `setOffset` only after persistence succeeds;
keep the existing symbols `load`, `updateThreads`, and `getThreadsList` as the
place to make the fix.
In `@app/views/ThreadMessagesView/index.tsx`:
- Around line 50-53: The useAppSelector call in ThreadMessagesView is returning
a new { user, useRealName } object on every selector run, which causes
unnecessary rerenders. Update this selection to avoid unstable object identity
by either selecting user and useRealName separately or by supplying a
shallow-equality comparator to useAppSelector. Keep the fix centered on the
ThreadMessagesView selector that reads getUserSelector and
state.settings.UI_Use_Real_Name.
🪄 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: b2ba1d6f-1f36-4ffd-8288-0ef804c70864
⛔ Files ignored due to path filters (1)
app/views/ThreadMessagesView/components/Item/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
app/views/ThreadMessagesView/Item.test.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/components/Item/index.tsxapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/methods/updateThreads.ts
💤 Files with no reviewable changes (1)
- app/views/ThreadMessagesView/Item.test.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: format
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/methods/updateThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.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 numbersUse TypeScript with strict mode enabled
Files:
app/views/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/methods/updateThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/methods/updateThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/methods/updateThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/methods/updateThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.tsx
🧠 Learnings (3)
📚 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/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/methods/getFilteredThreads.tsapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/definitions.tsapp/views/ThreadMessagesView/components/Item/styles.tsapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/methods/updateThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.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/ThreadMessagesView/components/Item/Item.test.tsxapp/views/ThreadMessagesView/components/Item/Item.stories.tsxapp/views/ThreadMessagesView/components/EmptyThreads.tsxapp/views/ThreadMessagesView/hooks/useSubscription.tsxapp/views/ThreadMessagesView/hooks/useThreads.tsxapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/components/Item/index.tsx
📚 Learning: 2026-03-15T13:55:42.038Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6911
File: app/containers/markdown/Markdown.stories.tsx:104-104
Timestamp: 2026-03-15T13:55:42.038Z
Learning: In Rocket.Chat React Native, the markdown parser requires a space between the underscore wrapping italic text and a mention sigil (_ mention _ instead of _mention_). Ensure stories and tests that include italic-wrapped mentions follow this form to guarantee proper parsing. Specifically, for files like app/containers/markdown/Markdown.stories.tsx, and any test/content strings that exercise italic-mentions, use the pattern _ mention _ (with spaces) to prevent the mention from being treated as plain text. Validate any test strings or story content accordingly.
Applied to files:
app/views/ThreadMessagesView/components/Item/Item.stories.tsx
🔇 Additional comments (6)
app/views/ThreadMessagesView/components/Item/styles.ts (1)
1-50: LGTM!app/views/ThreadMessagesView/components/Item/index.tsx (1)
1-12: LGTM!app/views/ThreadMessagesView/components/Item/Item.stories.tsx (1)
3-6: LGTM!app/views/ThreadMessagesView/components/Item/Item.test.tsx (1)
1-4: LGTM!app/views/ThreadMessagesView/definitions.ts (1)
4-9: LGTM!app/views/ThreadMessagesView/methods/getFilteredThreads.ts (1)
4-19: LGTM!
| type TEmptyThreads = { | ||
| currentFilter: Filter; | ||
| }; | ||
|
|
||
| const EmptyThreads = ({ currentFilter }: TEmptyThreads) => { | ||
| let text; | ||
| if (currentFilter === Filter.Following) { | ||
| text = I18n.t('No_threads_following'); | ||
| } else if (currentFilter === Filter.Unread) { | ||
| text = I18n.t('No_threads_unread'); | ||
| } else { | ||
| text = I18n.t('No_threads'); | ||
| } | ||
| return <BackgroundContainer text={text} />; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward a loading flag so the empty-state text doesn't flash during initial fetch.
BackgroundContainer only renders text when loading is falsy and shows a spinner otherwise. Since EmptyThreads is used as ListEmptyComponent, it renders whenever threads is empty — including the initial load — and without a loading prop it will display "No_threads" before the first fetch completes.
🔧 Proposed change
type TEmptyThreads = {
currentFilter: Filter;
+ loading?: boolean;
};
-const EmptyThreads = ({ currentFilter }: TEmptyThreads) => {
+const EmptyThreads = ({ currentFilter, loading }: TEmptyThreads) => {
let text;
if (currentFilter === Filter.Following) {
text = I18n.t('No_threads_following');
} else if (currentFilter === Filter.Unread) {
text = I18n.t('No_threads_unread');
} else {
text = I18n.t('No_threads');
}
- return <BackgroundContainer text={text} />;
+ return <BackgroundContainer text={text} loading={loading} />;
};Update the call site in index.tsx (Line 225) to pass loading.
🤖 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/ThreadMessagesView/components/EmptyThreads.tsx` around lines 5 -
19, EmptyThreads is rendering the empty-state message during the initial fetch
because it always passes only text to BackgroundContainer. Update EmptyThreads
to accept a loading prop and forward it to BackgroundContainer, and make sure
the ListEmptyComponent call site in index.tsx passes the current loading state
so the spinner shows instead of flashing “No_threads” before data loads.
| if (update && update.length) { | ||
| update = update.map(m => buildMessage(m)) as IMessage[]; | ||
| // filter threads | ||
| threadsToCreate = update.filter(i1 => !allThreadsRecords.find((i2: { id: string }) => i1._id === i2.id)) as TThreadModel[]; | ||
| threadsToUpdate = allThreadsRecords.filter((i1: { id: string }) => update.find(i2 => i1.id === i2._id)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Filter out null results from buildMessage before dereferencing ._id.
buildMessage can return null (it returns IMessage | IThreadResult | null). After the map at Line 35, the subsequent i1._id access in the filter/find calls (Lines 37-38, and Line 49) will throw on any null entry, which the outer catch swallows — aborting the entire create/update batch for that sync.
🔧 Proposed change
- update = update.map(m => buildMessage(m)) as IMessage[];
+ update = update.map(m => buildMessage(m)).filter(Boolean) as IMessage[];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (update && update.length) { | |
| update = update.map(m => buildMessage(m)) as IMessage[]; | |
| // filter threads | |
| threadsToCreate = update.filter(i1 => !allThreadsRecords.find((i2: { id: string }) => i1._id === i2.id)) as TThreadModel[]; | |
| threadsToUpdate = allThreadsRecords.filter((i1: { id: string }) => update.find(i2 => i1.id === i2._id)); | |
| if (update && update.length) { | |
| update = update.map(m => buildMessage(m)).filter(Boolean) as IMessage[]; | |
| // filter threads | |
| threadsToCreate = update.filter(i1 => !allThreadsRecords.find((i2: { id: string }) => i1._id === i2.id)) as TThreadModel[]; | |
| threadsToUpdate = allThreadsRecords.filter((i1: { id: string }) => update.find(i2 => i1.id === i2._id)); |
🤖 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/ThreadMessagesView/methods/updateThreads.ts` around lines 34 - 38,
Filter out null results from buildMessage before using _id in updateThreads. In
updateThreads.ts, after mapping with buildMessage, remove any null entries (or
narrow the array) before the threadsToCreate and threadsToUpdate filters so
i1._id and i2._id are only accessed on non-null messages. Keep the logic
centered around buildMessage, threadsToCreate, and threadsToUpdate, and ensure
the later similar _id dereference is also guarded the same way.
- restore leading-edge debounce on thread press (instant tap, blocks 1s) - keep REST-loaded threads for rooms without a subscription; gate init on subscriptionLoaded and skip the threads observable for no-sub rooms - preserve active search filter when toggling thread follow - move threads-subscription lifecycle and cleanup into useThreads - drop dead hook params and unused `any` casts; rename non-JSX hooks to .ts Claude-Session: https://claude.ai/code/session_011PSoHUDUjr9URVEQxqW91M
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
android_build
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/views/ThreadMessagesView/hooks/useThreads.ts (2)
122-129: 📐 Maintainability & Code Quality | 🔵 TrivialAdd
eslint-disablecomment forreact-hooks/exhaustive-depson the initialization effect.The effect depends on
subscriptionLoadedbut invokesinitandhandleThreadsSubscriptionwithout including them in the dependency array. Since the effect is guarded bydidInitto run only once, adding these functions to the dependency array would break the "run-once" logic. You should explicitly suppress the lint warning with a rationale.Code Change
useEffect(() => { // eslint-disable-next-line react-hooks/exhaustive-deps if (!subscriptionLoaded || didInit.current) { return; } didInit.current = true; init(); handleThreadsSubscription({}); }, [subscriptionLoaded]);🤖 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/ThreadMessagesView/hooks/useThreads.ts` around lines 122 - 129, The initialization effect in useThreads should explicitly suppress react-hooks/exhaustive-deps because it intentionally runs only once via didInit.current while calling init and handleThreadsSubscription without listing them as dependencies; add an eslint-disable-next-line comment for that useEffect and keep the dependency array limited to subscriptionLoaded so the run-once guard remains intact.Source: Coding guidelines
31-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
inittry/catch is ineffective and leaves floating promises.
syncandloadare async/debounced, so awaiting is skipped — thetry/catchonly catches synchronous throws (none here) and the returned promises float. Same for the unawaitedupdateThreads(...)insync(Line 87). Per the codebase convention, attach.catch(log)rather than leaving them floating (and avoidvoid).Based on learnings: the
no-void: errorrule is enforced; floating promises should be handled with.catch(...)rather than silenced withvoid.#!/bin/bash # Confirm whether no-floating-promises is enabled in the shared eslint config. fd -t f -e js -e json -e cjs -e yaml -e yml '\.eslintrc|eslint\.config' --hidden | xargs -I{} sh -c 'echo "== {} =="; cat "{}"' 2>/dev/null rg -nP 'no-floating-promises|no-void' -g '!**/node_modules/**'🤖 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/ThreadMessagesView/hooks/useThreads.ts` around lines 31 - 44, The init flow in useThreads is leaving async work unhandled: sync and load are being called without awaiting, so the try/catch in init never catches their failures and the promises float. Update init to handle the returned promises explicitly by attaching .catch(log) to the sync/load calls, and also fix sync so the updateThreads call is handled the same way instead of being left unawaited. Keep the existing function names (init, sync, load, updateThreads) and avoid using void; follow the project convention of catching promise rejections directly with .catch(log).
🤖 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/ThreadMessagesView/hooks/useSubscription.ts`:
- Around line 18-44: The initSubscription/useEffect flow can leak a subscription
and call setSubscription after unmount if the async
db.get('subscriptions').find(rid) resolves late. Update useSubscription so
initSubscription and unsubscribe share a cancellation flag (or equivalent
mounted state) that is checked before calling observable.subscribe,
setSubscription, and setSubscriptionLoaded, and ensure cleanup marks the request
as canceled before unsubscribing any existing subSubscription.current.
---
Nitpick comments:
In `@app/views/ThreadMessagesView/hooks/useThreads.ts`:
- Around line 122-129: The initialization effect in useThreads should explicitly
suppress react-hooks/exhaustive-deps because it intentionally runs only once via
didInit.current while calling init and handleThreadsSubscription without listing
them as dependencies; add an eslint-disable-next-line comment for that useEffect
and keep the dependency array limited to subscriptionLoaded so the run-once
guard remains intact.
- Around line 31-44: The init flow in useThreads is leaving async work
unhandled: sync and load are being called without awaiting, so the try/catch in
init never catches their failures and the promises float. Update init to handle
the returned promises explicitly by attaching .catch(log) to the sync/load
calls, and also fix sync so the updateThreads call is handled the same way
instead of being left unawaited. Keep the existing function names (init, sync,
load, updateThreads) and avoid using void; follow the project convention of
catching promise rejections directly with .catch(log).
🪄 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: 91f00036-e4e6-457b-b01e-58de638836b7
📒 Files selected for processing (4)
app/views/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.tsapp/views/ThreadMessagesView/index.tsxapp/views/ThreadMessagesView/methods/updateThreads.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/ThreadMessagesView/methods/updateThreads.ts
- app/views/ThreadMessagesView/index.tsx
📜 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 (5)
**/*.{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/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.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 with strict mode enabled
Files:
app/views/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.ts
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.ts
🧠 Learnings (1)
📚 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/ThreadMessagesView/hooks/useSubscription.tsapp/views/ThreadMessagesView/hooks/useThreads.ts
🔇 Additional comments (1)
app/views/ThreadMessagesView/hooks/useSubscription.ts (1)
15-16: 🩺 Stability & Availability | 💤 Low valueInitial
{}subscription state — already flagged.This was raised previously. Note the current consumers (
useThreadsandupdateThreads) guard onsubscription._id/subscription?._idrather than!subscription, so the empty-object default no longer throws onsubscription.threads. Still, considernullplus an explicit type (TSubscriptionModel | null) to make the "not loaded" state unambiguous.
| const initSubscription = async () => { | ||
| try { | ||
| const db = database.active; | ||
|
|
||
| const subscription = await db.get('subscriptions').find(rid); | ||
| const observable = subscription.observe(); | ||
| subSubscription.current = observable.subscribe(data => { | ||
| setSubscription(data); | ||
| }); | ||
| setSubscriptionLoaded(true); | ||
| } catch (e) { | ||
| setSubscriptionLoaded(true); | ||
| log(e); | ||
| } | ||
| }; | ||
|
|
||
| const unsubscribe = () => { | ||
| subSubscription.current?.unsubscribe(); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| initSubscription(); | ||
|
|
||
| return () => { | ||
| unsubscribe(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Possible subscription leak / setState after unmount on fast unmount.
If the component unmounts before db.get('subscriptions').find(rid) resolves, the cleanup runs while subSubscription.current is still null, so the RxJS subscription created afterward is never torn down and setSubscription fires on an unmounted component. Track a cancellation flag to guard both paths.
🛡️ Proposed guard
const useSubscription = ({ rid }: IUseSubscriptionProps) => {
const subSubscription = useRef<Subscription | null>(null);
+ const isMounted = useRef(true);
const [subscription, setSubscription] = useState<TSubscriptionModel>({} as TSubscriptionModel);
const [subscriptionLoaded, setSubscriptionLoaded] = useState(false);
const initSubscription = async () => {
try {
const db = database.active;
const subscription = await db.get('subscriptions').find(rid);
+ if (!isMounted.current) {
+ return;
+ }
const observable = subscription.observe();
subSubscription.current = observable.subscribe(data => {
setSubscription(data);
});
setSubscriptionLoaded(true);
} catch (e) {
setSubscriptionLoaded(true);
log(e);
}
};
useEffect(() => {
initSubscription();
return () => {
+ isMounted.current = false;
unsubscribe();
};
}, []);🤖 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/ThreadMessagesView/hooks/useSubscription.ts` around lines 18 - 44,
The initSubscription/useEffect flow can leak a subscription and call
setSubscription after unmount if the async db.get('subscriptions').find(rid)
resolves late. Update useSubscription so initSubscription and unsubscribe share
a cancellation flag (or equivalent mounted state) that is checked before calling
observable.subscribe, setSubscription, and setSubscriptionLoaded, and ensure
cleanup marks the request as canceled before unsubscribing any existing
subSubscription.current.
|
Android Build Available Rocket.Chat 4.74.0.109210 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRvW0au6pqPH0OHIV6RJQTlpTCVcGE9Hm6Yptm2f_A9xJInPNuDOTkSX3YOXw83ty7GOxEaUlGzoDjBmHcG |
|
iOS Build Available Rocket.Chat 4.74.0.109213 |
2 participants
Proposed changes
MIgrate the ThreadMessagesView code from class components to function component.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-37
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor