Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements typing indicators for channels and threads by adding local typing state tracking via a reusable tracker module, integrating typing event mutations into channel and thread message inputs, and displaying typing status indicators in thread UI. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
synoet
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/channel/Thread/ThreadTypingIndicator.tsx`:
- Around line 16-28: Replace the switch in the createMemo that builds typingText
with ts-pattern's match: import match from 'ts-pattern' and use
match(typingUsers().length).with(0, () => '' ).with(1, () =>
`${idToDisplayName(typingUsers()[0])} is typing`).with(2, () =>
`${idToDisplayName(typingUsers()[0])} and ${idToDisplayName(typingUsers()[1])}
are typing`).otherwise(() => 'Multiple people are typing'); keep references to
createMemo, typingUsers, and idToDisplayName so behavior remains identical and
ensure the import for match is added at the top.
In `@js/app/packages/core/service.ts`:
- Around line 248-253: The conditional type aliases (e.g., ClientFunctionArgs)
use `any` which violates the TS guideline; update each alias to avoid `any` by
inferring all type parameters or using `unknown`/constrained inference where
appropriate — for example change ClientFunctionArgs from T extends
FunctionDefinition<any, infer Args, any, any> to T extends
FunctionDefinition<infer N, infer Args, infer R, infer M> ? Args : never (or use
unknown for parameters you intentionally don't depend on). Apply the same
pattern to the other touched aliases at the same area so no `any` remains.
In `@js/app/packages/core/util/maybeResult.ts`:
- Around line 309-314: Replace the uses of `any` with `unknown` in the
`ResultType` utility: change the constraint `T extends MaybeResult<any, any>` to
`T extends MaybeResult<unknown, unknown>` and the conditional match `T extends
MaybeResult<any, infer R>` to `T extends MaybeResult<unknown, infer R>` so you
preserve type inference for `R` while avoiding `any` (keep `NonNullable<R>`
as-is); this targets the `ResultType` type and the `MaybeResult` generics.
In `@js/app/packages/queries/soup/normalized-cache/operations.test.ts`:
- Around line 226-237: The test uses unsafe casts (filter as any) when accessing
indexed filter shapes; replace those casts with a proper typed interface for the
filter object and use it for indexed access. Define a local type (e.g.,
FilterMap or NormalizedFilterShape) that describes the keys
('document_filters','chat_filters','channel_filters','project_filters', plus the
dynamic filterKey) and their inner mapping types (mapping idKey to string[]),
then annotate the test variables (filter, otherFilters handling) with that type
and access values without any casts so Object.values((filter as
FilterMap)[key])[0] is strongly typed; update references to filter, filterKey,
idKey, otherFilters accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 776223c5-84d4-4826-ac0e-c457dc45ed98
📒 Files selected for processing (12)
js/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Input/ChannelInput.tsxjs/app/packages/channel/Input/create-typing-tracker.tsjs/app/packages/channel/Input/types.tsjs/app/packages/channel/Thread/ChannelThread.tsxjs/app/packages/channel/Thread/ThreadReplyInput.tsxjs/app/packages/channel/Thread/ThreadTypingIndicator.tsxjs/app/packages/channel/Thread/index.tsjs/app/packages/channel/Thread/types.tsjs/app/packages/core/service.tsjs/app/packages/core/util/maybeResult.tsjs/app/packages/queries/soup/normalized-cache/operations.test.ts
js/app/packages/queries/soup/normalized-cache/operations.test.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
js/app/packages/channel/Thread/ThreadTypingIndicator.tsx (1)
71-82: 🧹 Nitpick | 🔵 TrivialUse
matchfromts-patternfor the switch statement.Per coding guidelines, prefer
matchfromts-patternfor switch/case logic.♻️ Proposed refactor
+import { match } from 'ts-pattern'; import { getTypingUsersForChannel } from '@queries/channel/typing';function getThreadTypingIndicatorText(userIds: string[]): string { - switch (userIds.length) { - case 0: - return ''; - case 1: - return `${idToDisplayName(userIds[0])} is typing`; - case 2: - return `${idToDisplayName(userIds[0])} and ${idToDisplayName(userIds[1])} are typing`; - default: - return 'Multiple people are typing'; - } + return match(userIds.length) + .with(0, () => '') + .with(1, () => `${idToDisplayName(userIds[0])} is typing`) + .with(2, () => `${idToDisplayName(userIds[0])} and ${idToDisplayName(userIds[1])} are typing`) + .otherwise(() => 'Multiple people are typing'); }As per coding guidelines: "For exhaustive switch statements use
matchfromts-pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/channel/Thread/ThreadTypingIndicator.tsx` around lines 71 - 82, Replace the switch/case in getThreadTypingIndicatorText with ts-pattern's match: import { match } from 'ts-pattern', call match(userIds.length).with(0, () => ''), .with(1, () => `${idToDisplayName(userIds[0])} is typing`), .with(2, () => `${idToDisplayName(userIds[0])} and ${idToDisplayName(userIds[1])} are typing`), and .otherwise(() => 'Multiple people are typing'); ensure idToDisplayName remains referenced and the match is exhaustive via .otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/channel/Channel/Channel.tsx`:
- Around line 289-291: The isNewestThread computation redundantly calls
messageIndex().keys twice; instead reuse the local keys accessor and compare
item.id to the last element via keys().at(-1). Update the isNewestThread arrow
function (which currently references keys and item.id) to return item.id ===
keys().at(-1) so you avoid the extra messageIndex().keys invocation and simplify
the logic.
- Line 298: The prop isNewestThread is being passed as a evaluated boolean
(isNewestThread()) which loses reactivity; instead pass the accessor itself
(isNewestThread) from the caller and update ChannelThread's prop type to accept
an Accessor<boolean> (isNewestThread?: Accessor<boolean>), then inside
ChannelThread (where Show or any conditional uses it) call
props.isNewestThread() to read the current value. Locate the prop usage in the
Channel component (isNewestThread={isNewestThread()}), the ChannelThread
component declaration and its props type, and update them accordingly so the
reactive accessor is forwarded and invoked inside ChannelThread.
---
Duplicate comments:
In `@js/app/packages/channel/Thread/ThreadTypingIndicator.tsx`:
- Around line 71-82: Replace the switch/case in getThreadTypingIndicatorText
with ts-pattern's match: import { match } from 'ts-pattern', call
match(userIds.length).with(0, () => ''), .with(1, () =>
`${idToDisplayName(userIds[0])} is typing`), .with(2, () =>
`${idToDisplayName(userIds[0])} and ${idToDisplayName(userIds[1])} are typing`),
and .otherwise(() => 'Multiple people are typing'); ensure idToDisplayName
remains referenced and the match is exhaustive via .otherwise.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2fe4f77-72ec-40fa-bbdc-02b99947fe80
📒 Files selected for processing (2)
js/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Thread/ThreadTypingIndicator.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/app/packages/channel/Channel/Channel.tsx (1)
297-297:⚠️ Potential issue | 🟡 MinorPass the accessor to maintain reactivity.
isNewestThreadis computed reactively based onmessageIndex().keys, which updates when messages are added/removed. PassingisNewestThread()(evaluated) loses reactivity—the boolean becomes stale if the message list changes after render.Pass the accessor and invoke it inside
ChannelThread:Suggested fix
- isNewestThread={isNewestThread()} + isNewestThread={isNewestThread}Then update
ChannelThread's prop type toisNewestThread?: Accessor<boolean>and callprops.isNewestThread?.()where used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/channel/Channel/Channel.tsx` at line 297, The prop currently passes the evaluated boolean by calling isNewestThread() which loses reactivity when messageIndex().keys changes; instead pass the accessor itself to ChannelThread (pass isNewestThread) and update ChannelThread's prop type to accept isNewestThread?: Accessor<boolean>, then invoke the accessor where needed inside ChannelThread as props.isNewestThread?.() so the value updates when messageIndex().keys changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/app/packages/channel/Channel/Channel.tsx`:
- Line 297: The prop currently passes the evaluated boolean by calling
isNewestThread() which loses reactivity when messageIndex().keys changes;
instead pass the accessor itself to ChannelThread (pass isNewestThread) and
update ChannelThread's prop type to accept isNewestThread?: Accessor<boolean>,
then invoke the accessor where needed inside ChannelThread as
props.isNewestThread?.() so the value updates when messageIndex().keys changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b25a90e-d3f1-4d85-a0b0-286958d925f2
📒 Files selected for processing (3)
js/app/packages/channel/Channel/Channel.tsxjs/app/packages/channel/Input/create-typing-tracker.tsjs/app/packages/channel/Thread/ThreadTypingIndicator.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.