Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWhen ENABLE_NEW_CHANNELS() is enabled, handleCommsMessage now reconciles target-message cache state: if payload.deleted_at is set it calls removeMessageFromTargetCaches; otherwise it computes target with resolveMessageTarget(payload.thread_id ?? undefined), reads getTargetMessageState, and either updates only content, editedAt, updatedAt (preserving attachments) via replaceTargetMessageState or inserts a new ApiThreadReply (attachments: [], reactions: []) for thread_reply targets or a base message with attachments: [] and reactions: []. An extra blank line was added near the queryClient.setQueryData call. Lines changed: +40/-25. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/queries/channel/sync.ts (1)
122-140: 🧹 Nitpick | 🔵 TrivialMinor:
deleted_atis always falsy in this branch.Since this code path is inside the
elsebranch ofif (payload.deleted_at)(line 79),payload.deleted_atwill always be null/undefined here. Setting it is harmless but redundant.Consider using
nullexplicitly for clarity, or omitting the line if the type allows:- deleted_at: payload.deleted_at, + deleted_at: null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/queries/channel/sync.ts` around lines 122 - 140, The else branch after if (payload.deleted_at) constructs a message where deleted_at is always falsy; update the insertMessageIntoTargetCaches call in this else branch (the block that builds the message object passed to insertMessageIntoTargetCaches) to either set deleted_at: null explicitly for clarity or remove the deleted_at property entirely if your Message type allows it, and keep the rest of the fields unchanged so the non-deleted message state is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@js/app/packages/queries/channel/sync.ts`:
- Around line 122-140: The else branch after if (payload.deleted_at) constructs
a message where deleted_at is always falsy; update the
insertMessageIntoTargetCaches call in this else branch (the block that builds
the message object passed to insertMessageIntoTargetCaches) to either set
deleted_at: null explicitly for clarity or remove the deleted_at property
entirely if your Message type allows it, and keep the rest of the fields
unchanged so the non-deleted message state is accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad19cf2e-bba4-4d91-a7ca-4fbff34a1a48
📒 Files selected for processing (1)
js/app/packages/queries/channel/sync.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/queries/channel/sync.ts (1)
107-135:⚠️ Potential issue | 🟠 MajorHydrate cache-miss inserts from the existing attachment/reaction caches.
These synthetic inserts hard-code
attachments: []andreactions: [], but this file can already have newer values for the same message inGetChannelResponse.attachmentsandGetChannelResponse.reactionsfrom earlier websocket events. On a cache miss, that turns known live state back into empty state until the next refetch. Read those caches before defaulting to[].Suggested fix
+ const channelState = + queryClient.getQueryData<GetChannelResponse>(queryKey); + const attachments = + channelState?.attachments.filter( + (attachment) => attachment.message_id === payload.id + ) ?? []; + const reactions = channelState?.reactions?.[payload.id] ?? []; + if (existingState) { replaceTargetMessageState(payload.channel_id, target, { content: payload.content, editedAt: payload.edited_at, updatedAt: payload.updated_at, attachments: existingState.attachments, }); } else if (target.kind === 'thread_reply') { const reply: ApiThreadReply = { id: payload.id, sender_id: payload.sender_id, content: payload.content, created_at: payload.created_at, updated_at: payload.updated_at, edited_at: payload.edited_at, - attachments: [], - reactions: [], + attachments, + reactions, }; insertMessageIntoTargetCaches(payload.channel_id, target, reply); } else { insertMessageIntoTargetCaches(payload.channel_id, target, { id: payload.id, channel_id: payload.channel_id, sender_id: payload.sender_id, content: payload.content, created_at: payload.created_at, updated_at: payload.updated_at, deleted_at: payload.deleted_at, edited_at: payload.edited_at, - attachments: [], - reactions: [], + attachments, + reactions, thread: { preview: [], reply_count: 0, latest_reply_at: null, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/queries/channel/sync.ts` around lines 107 - 135, On cache-miss inserts in sync.ts (both the ApiThreadReply construction and the full message insert passed to insertMessageIntoTargetCaches), don't hard-code attachments: [] and reactions: []; instead look up any existing attachments and reactions for payload.id from the in-memory message caches (e.g., the GetChannelResponse.attachments and GetChannelResponse.reactions stores used elsewhere in this file) and use those values when present, falling back to [] only if no cached arrays exist; update the two places that build the synthetic message (the ApiThreadReply block and the full message block) to hydrate attachments and reactions from those caches before calling insertMessageIntoTargetCaches.
🤖 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/queries/channel/sync.ts`:
- Around line 79-93: Hoist the resolved message target into a single variable
and reuse it for the remove/update/insert and the subsequent soft invalidation
to avoid re-resolving with a changed cache state; specifically, call
resolveMessageTarget once (assign to target) before the if (payload.deleted_at)
branch and use that target in removeMessageFromTargetCaches, any update/insert
logic, and the final softInvalidateTargetCaches call so the same target
(including threadId presence/absence) is used throughout.
---
Outside diff comments:
In `@js/app/packages/queries/channel/sync.ts`:
- Around line 107-135: On cache-miss inserts in sync.ts (both the ApiThreadReply
construction and the full message insert passed to
insertMessageIntoTargetCaches), don't hard-code attachments: [] and reactions:
[]; instead look up any existing attachments and reactions for payload.id from
the in-memory message caches (e.g., the GetChannelResponse.attachments and
GetChannelResponse.reactions stores used elsewhere in this file) and use those
values when present, falling back to [] only if no cached arrays exist; update
the two places that build the synthetic message (the ApiThreadReply block and
the full message block) to hydrate attachments and reactions from those caches
before calling insertMessageIntoTargetCaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1078804-b2ca-4155-a9d3-1777df299be3
📒 Files selected for processing (1)
js/app/packages/queries/channel/sync.ts
synoet
deleted the
macro-cyppvhMb6AvwWWX1N7b87-new-channels-deleting-messages-doesnt-live-update
branch
1 participant
No description provided.