fix: add proper type for room in RoomContext #6801
Conversation
- Replace 'any' type with proper TRoomContextRoom union type - TRoomContextRoom matches IRoomViewState.room definition - Supports both TSubscriptionModel and partial room objects (for threads) - Removes FIXME comment and unblocks RoomView migration to hooks
WalkthroughIntroduces stricter typing for rooms: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/RoomView/context.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/RoomView/context.ts (2)
app/definitions/ISubscription.ts (1)
TSubscriptionModel(121-124)app/definitions/IMessage.ts (1)
ILastMessage(54-75)
🔇 Additional comments (2)
app/views/RoomView/context.ts (2)
3-3: LGTM: Necessary imports added.The type-only imports for
ILastMessageandTSubscriptionModelare correctly added to support the new type definition.
30-30: Type change verified as safe and correct across all consumers.The
roomproperty is used in two places within ComposerInput.tsx:
- Line 62:
room.t === 'd'(guarded byroom &&check)- Line 311:
room?.t === 'l'(using optional chaining)Both union members of
TRoomContextRoominclude the requiredt: stringproperty, so all type access patterns are valid. The new type properly enforces safe access patterns that were previously masked by theanytype.
- Update TRoomContextRoom to use RoomType ('c' | 'd' | 'p' | 'l') instead of generic string
- Update IRoomViewState.room to use RoomType for consistency
- Update route params type in ChatsStackParamList to use RoomType
- Fix type assertions in RoomView constructor and MessageComposer test
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
11-16: Fix duplicate../../definitionsimports to satisfy ESLintESLint's
import/no-duplicateserror comes from importing../../definitionstwice (lines 12 and 15: once forIMessage, once forRoomType). Consolidate into a single import:-import { type IMessage } from '../../definitions'; +import { type IMessage, type RoomType } from '../../definitions'; -import { type RoomType } from '../../definitions';The
t: 'd' as RoomTypeassignment at line 81 is valid and will work correctly with the consolidated import, as'd'is a valid member of theRoomTypeunion ('c' | 'd' | 'p' | 'l').
🧹 Nitpick comments (2)
app/stacks/types.ts (1)
3-19: AligningRoomViewrouteroom.twithRoomTypeis consistentImporting
RoomTypeand constrainingRoomView’s optionalroomparam so that its inline variant usest: RoomTypekeeps the navigation type consistent withIRoomViewState.roomand the RoomContext API. The shape{ rid; t; name?; fname?; prid? }is a reasonable minimal subset for routing.If you find more places duplicating this inline room shape, consider extracting a shared type alias (e.g., a lightweight “navigation-room” type) to avoid drift, provided it doesn’t introduce circular deps.
Also applies to: 31-46
app/views/RoomView/index.tsx (1)
57-70: RoomType integration is solid; double‑check thethis.t as RoomTypefallback for thread routesImporting
RoomType, casting the constructedroomtoIRoomViewState['room'], and passingroom.t as RoomTypeintoRoomServices.getMessagesandLoadMoreall move RoomView toward a single, well‑typed room shape that matches the new RoomContext API. For “normal” rooms whereroomis aTSubscriptionModel,room.tis a genuineRoomType, so those casts are effectively just helping TypeScript.The only place that deserves a second look is the constructor fallback:
const room = (props.route.params?.room ?? { rid: this.rid as string, t: (this.t as RoomType) || ('d' as RoomType), name, fname, prid }) as IRoomViewState['room'];When navigating to threads,
props.route.params.tis set toSubscriptionType.THREAD, sothis.tcan be a value that does not belong to theRoomType = 'c' | 'd' | 'p' | 'l'union. In those cases:
(this.t as RoomType)is a compile‑time lie: at runtime it’s still'thread'(or whatever theSubscriptionTypevalue is).- Because
'thread'is truthy, the|| 'd'fallback doesn’t kick in, so the inline room object will havet: 'thread'while the type claimsRoomType.That’s consistent with pre‑PR runtime behavior (you were already passing
this.tthrough), but now TypeScript thinksroom.tis strictly aRoomTypeeverywhere, which may mask future misuse.If
SubscriptionTypeis guaranteed to be a subset ofRoomTypein all cases whereprops.route.params.roomis omitted, then this is fine. Otherwise, consider one of:
Narrowing at runtime before assigning:
const allowedTypes: RoomType[] = ['c', 'd', 'p', 'l']; const fallbackT: RoomType = this.t && allowedTypes.includes(this.t as RoomType) ? (this.t as RoomType) : 'd'; const room = (props.route.params?.room ?? { rid: this.rid as string, t: fallbackT, name, fname, prid }) as IRoomViewState['room'];Or, if thread routes intentionally use a non‑RoomType discriminator in
room.t, widening the inline variant inIRoomViewState['room'](and the corresponding TRoomContextRoom) to reflect that.Either approach keeps the improved type‑safety you’re aiming for while avoiding a silent cast that may hide real mismatches.
Also applies to: 135-155, 576-592, 1316-1323
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/containers/MessageComposer/MessageComposer.test.tsx(2 hunks)app/stacks/types.ts(2 hunks)app/views/RoomView/context.ts(1 hunks)app/views/RoomView/definitions.ts(2 hunks)app/views/RoomView/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/RoomView/context.ts
🧰 Additional context used
🧬 Code graph analysis (4)
app/stacks/types.ts (2)
app/definitions/ISubscription.ts (1)
TSubscriptionModel(121-124)app/definitions/IRoom.ts (1)
RoomType(147-147)
app/views/RoomView/definitions.ts (1)
app/definitions/IRoom.ts (1)
RoomType(147-147)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
app/definitions/IRoom.ts (1)
RoomType(147-147)
app/views/RoomView/index.tsx (2)
app/definitions/IRoom.ts (1)
RoomType(147-147)app/views/RoomView/definitions.ts (1)
IRoomViewState(41-73)
🪛 ESLint
app/containers/MessageComposer/MessageComposer.test.tsx
[error] 15-15: '/home/jailuser/git/app/definitions/index.ts' imported multiple times.
(import/no-duplicates)
🔇 Additional comments (1)
app/views/RoomView/definitions.ts (1)
4-12: Stricterroom.ttyping withRoomTypelooks goodUsing
RoomTypeforIRoomViewState.room’s inline variant (and importing it here) tightens the state shape to the canonical'c' | 'd' | 'p' | 'l'set and fits the Room-related typings used elsewhere (e.g.,IRoom.ts). No issues from this change on its own.Also applies to: 41-57
- Merge duplicate ../../definitions imports into single import statement - Fixes ESLint import/no-duplicates error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
81-81: Type assertion is correct; consider consistency improvement.The type assertion correctly ensures
'd'is recognized asRoomType. The value'd'is a valid member of theRoomTypeunion.However, for consistency, Line 509 also assigns a room type (
'l') without using the same type assertion pattern:room: { ...initialContext.room, t: 'l' }Consider applying the same pattern for consistency:
-room: { ...initialContext.room, t: 'l' } +room: { ...initialContext.room, t: 'l' as RoomType }This makes the type constraint explicit across all test cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/containers/MessageComposer/MessageComposer.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
app/definitions/IRoom.ts (1)
RoomType(147-147)
🔇 Additional comments (1)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
12-12: LGTM! Import addition is correct.The
RoomTypeimport is properly added and necessary for the type assertion used in the test context.
1 participant
I fixed a type-safety issue in RoomContext by replacing the
anytype used for theroomproperty with a proper union type calledTRoomContextRoom. This brings back full TypeScript checking and also removes a blocker for migrating RoomView to hooks.Issue(s)
Fixes bug: #6799
Problem
The
roomfield inIRoomContextwas typed asany, which caused problems like:FIXMEcomment showing this was known technical debtSolution
I created a new union type named
TRoomContextRoombased on how theroomvalue is actually used inIRoomViewState.room.Details
File updated:
app/views/RoomView/context.tsWhat I changed:
TRoomContextRoomIRoomContextsoroomuses this new type instead ofanyILastMessageandTSubscriptionModelThe new type works fully with the existing code:
TSubscriptionModel, which is part of the unionSummary by CodeRabbit
Note: No visible UI changes; existing functionality remains unchanged, with behind-the-scenes improvements to message composition and context behavior.