fix: switch hide system message not working on mobile and loading on older server#7130
fix: switch hide system message not working on mobile and loading on older server#7130OtavioStasiak wants to merge 8 commits intodevelopfrom
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📜 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). (4)
WalkthroughAdds room-type awareness to the message list flow, moves system-message filtering into the DB query builder, stores unfiltered raw messages from the DB, and dispatches room history requests for server versions ≥3.16 when system messages are hidden. Changes
Sequence Diagram(s)sequenceDiagram
participant List as ListContainer (UI)
participant Hook as useMessages
participant DB as Database (WatermelonDB)
participant Store as Redux (dispatch)
participant API as Message Services (getThread/getMessage)
List->>Hook: mount with {rid, tmid, t, hideSystemMessages, serverVersion}
Hook->>DB: query messages (with buildVisibleSystemTypesClause(hideSystemMessages))
DB-->>Hook: observe() emits rawRows
Hook->>Hook: set rawMessages -> compute visibleMessages (filter by hideSystemMessages)
alt serverVersion >= 3.16 and hideSystemMessages non-empty and anyLoad present
Hook->>Store: dispatch(roomHistoryRequest({ rid, t, loaderId }))
end
opt tmid provided
Hook->>DB: query thread_messages
DB-->>Hook: emits thread rows
Hook->>API: getThreadById(tmid) or getMessageById(parentId)
API-->>Hook: returns thread parent -> appended to visibleMessages
end
Hook-->>List: return [visibleMessages, visibleMessagesIds, fetchMessages]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
changed the title
OtavioStasiak
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/RoomView/List/hooks/useMessages.test.tsx`:
- Around line 55-60: The helper msg function currently sets id twice causing an
ESLint duplicate-property warning; change the object literal to avoid
duplicating id by spreading overrides only once (e.g., put ts default first and
then ...overrides), so update the msg function (symbol: msg, parameter:
overrides, type: TAnyMessageModel) to return { ts: new Date('2024-01-01'),
...overrides } as TAnyMessageModel (or alternatively remove the explicit id:
overrides.id) to eliminate the duplicate property.
- Around line 70-79: The mock for mockDbGet currently returns an object that
doesn't satisfy the expected Collection<Model> type; update the mock
implementation in useMessages.test.tsx so the returned value is asserted as the
proper type (e.g., use "as unknown as Collection<Model>" or similar) while
keeping the existing structure: the mockDbGet.mockImplementation that returns {
query: jest.fn().mockReturnValue({ observe: () => ({ subscribe: (onNext: (rows:
TAnyMessageModel[]) => void) => { onNext(emittedRows); return { unsubscribe:
jest.fn() }; } }) }) } should be wrapped with the TypeScript type assertion to
Collection<Model> so the compiler accepts the mock.
In `@app/views/RoomView/List/hooks/useMessages.ts`:
- Around line 136-143: The useEffect block using compareServerVersion,
visibleMessages and hideSystemMessages should include all external dependencies
to avoid stale closures: add serverVersion, rid, t and dispatch to the
dependency array (in addition to hideSystemMessages and visibleMessages) and
ensure the loader lookup uses visibleMessages and MESSAGE_TYPE_ANY_LOAD as
before; also change the conditional from "hideSystemMessages && loaderId" to
"hideSystemMessages.length > 0 && loaderId" so an empty array doesn't
incorrectly pass; keep roomHistoryRequest(dispatch...) invocation with the found
loaderId.
🪄 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: dfe34416-508f-4f88-a99c-22e6a326485a
📒 Files selected for processing (7)
app/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useMessages.test.tsxapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/index.tsxapp/views/RoomView/index.tsx
📜 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: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{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/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in app/views/ directory
Files:
app/views/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
**/*.{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/RoomView/List/index.tsxapp/views/RoomView/index.tsxapp/views/RoomView/List/definitions.tsapp/views/RoomView/List/hooks/buildVisibleSystemTypesClause.tsapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useMessages.test.tsx
🧠 Learnings (2)
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/definitions/**/*.{ts,tsx} : Place shared TypeScript type definitions in app/definitions/
Applied to files:
app/views/RoomView/List/definitions.ts
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/database/model/**/*.{ts,tsx} : Place database models in app/lib/database/model/ for entities like Message, Room, Subscription, User, Thread, Upload, Server, CustomEmoji, Permission, and Role
Applied to files:
app/views/RoomView/List/hooks/useMessages.ts
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/views/RoomView/List/hooks/useMessages.test.tsx
[failure] 70-70:
Argument of type '() => { query: jest.Mock<any, any, any>; }' is not assignable to parameter of type '(tableName: string) => Collection'.
[failure] 57-57:
'id' is specified more than once, so this usage will be overwritten.
🔇 Additional comments (8)
app/views/RoomView/List/hooks/buildVisibleSystemTypesClause.ts (1)
10-17: LGTM - Clean implementation of the visibility clause builder.The logic correctly implements the filter: messages pass if
tis null (regular messages),tis a load-type marker, ortis not in the hidden list. TheQ.and(...map(Q.notEq))pattern correctly implements "not in list" semantics.app/views/RoomView/index.tsx (1)
1674-1674: LGTM - Correctly passes room type to List component.The prop addition aligns with the updated
IListContainerPropsinterface requirements.app/views/RoomView/List/index.tsx (1)
10-18: LGTM - Props and hook invocation correctly updated.The
tprop is properly destructured and passed touseMessagesalong with the reordered arguments matching the updated hook signature.app/views/RoomView/List/definitions.ts (1)
5-5: LGTM - Interface properly extended with room type.The
RoomTypeimport from shared definitions and the new requiredtproperty follow existing patterns. Based on learnings, shared TypeScript type definitions are correctly sourced fromapp/definitions/.Also applies to: 24-24
app/views/RoomView/List/hooks/useScroll.ts (1)
7-17: LGTM - Correctly handles stale closure prevention.The pattern of accepting
messagesIds: string[]as a prop and synchronizing it to a ref viauseEffectcorrectly prevents stale closures in the asyncjumpToMessagecallback. This is a proper React pattern for maintaining up-to-date values in callbacks without causing re-renders.app/views/RoomView/List/hooks/useMessages.test.tsx (1)
127-144: Good test coverage for the version-gated dispatch behavior.The test correctly verifies that
ROOM.HISTORY_REQUESTis dispatched when server version is>= 3.16.0, system messages are hidden, and a load row exists. This covers the core fix for the PR objective.app/views/RoomView/List/hooks/useMessages.ts (2)
124-129: LGTM - Efficient memoized filtering.The
useMemocorrectly derives visible messages from raw messages, filtering out hidden system message types. The early return whenhideSystemMessagesis empty avoids unnecessary filtering.
48-86: LGTM - DB-level filtering ensures correct pagination.Applying
buildVisibleSystemTypesClauseat the query level ensuresQ.take(count.current)retrieves the expected number of visible rows, fixing the core pagination issue when many system messages are hidden.
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
1 participant
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-1021
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Refactor
Tests