feat: remote delivery ordering with persistent process segments#1435
Merged
feat: remote delivery ordering with persistent process segments#1435
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces ephemeral remote status/content messages with an ordered, persistent Changes
Sequence DiagramsequenceDiagram
participant Conv as Conversation Runner
participant Render as Block Renderer
participant Snapshot as Delivery Snapshot
participant Runtime as Feishu/Telegram Runtime
participant Remote as Remote Platform
Conv->>Render: buildRemoteDeliverySegments(blocks)
Render->>Render: coalesce tool calls & answers into segments
Render-->>Snapshot: deliverySegments[]
Conv->>Render: buildRemoteTraceText(blocks)
Render-->>Snapshot: traceText
Snapshot-->>Runtime: RemoteConversationSnapshot (deliverySegments, traceText)
Runtime->>Runtime: syncDeliverySegments()
loop for each segment in order
Runtime->>Remote: sendText / editText for segment
Remote-->>Runtime: messageId(s)
Runtime->>Runtime: persist segment.messageIds
end
alt final/timeout => append terminal
Runtime->>Runtime: determine terminal text (finalText/fullText/text or timeout)
Runtime->>Remote: sendText(terminal)
Remote-->>Runtime: terminalMessageId
Runtime->>Runtime: persist terminal segment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
zerob13
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: 3
🧹 Nitpick comments (1)
src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts (1)
493-511: Minor inconsistency in no-response text.Line 500 uses an inline string
'No assistant response was produced.'while lines 504 and 507 use theREMOTE_NO_RESPONSE_TEXTconstant. Consider using the constant consistently.♻️ Proposed fix for consistency
if (!trackedMessage) { const completed = !activeGeneration && session.status !== 'generating' if (completed) { this.bindingStore.clearActiveEvent(endpointKey) } return { messageId: null, - text: completed ? 'No assistant response was produced.' : '', + text: completed ? REMOTE_NO_RESPONSE_TEXT : '', traceText: '', deliverySegments: [], statusText: completed ? '' : buildRemoteStatusText([]), finalText: completed ? REMOTE_NO_RESPONSE_TEXT : '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts` around lines 493 - 511, Replace the inline string 'No assistant response was produced.' with the existing constant REMOTE_NO_RESPONSE_TEXT for consistency; locate the early-return block that checks trackedMessage (around the completed calculation and this.bindingStore.clearActiveEvent(endpointKey)) and update the fields text, finalText, and fullText to use REMOTE_NO_RESPONSE_TEXT so all no-response outputs are uniform (ensure statusText still uses buildRemoteStatusText([]) when not completed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts`:
- Around line 618-625: The messageId collection logic in the block using
existing, nextChunks and messageIds is compacting away missing ids which breaks
alignment with chunkFeishuText(normalized) and causes later updateText() calls
to target the wrong segment; inside the loop that calls
this.deps.client.sendText(target, chunk) (and in the similar loops at the other
locations), preserve positional holes by pushing a placeholder (e.g. null) into
messageIds when sendText() returns null (or alternatively, if you prefer the
other behavior, mark the whole segment as non-editable as soon as any sendText()
returns null), and ensure downstream code that reads messageIds expects and
respects those null slots when calling updateText() so indices remain aligned
with nextChunks/chunkFeishuText(normalized).
- Around line 537-564: In appendTerminalDeliverySegment: currently you bail out
if any segment has kind === 'answer', which prevents adding terminal failure
text after a partial streamed answer; change the logic to only skip appending
when the last answer segment's text equals normalized (i.e., duplicate
terminal), otherwise allow adding the terminal segment; keep the existing checks
for sourceMessageId, normalized empty, and REMOTE_NO_RESPONSE_TEXT, and
reference the function appendTerminalDeliverySegment and the segment properties
kind/text/sourceMessageId to locate and update the condition.
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 497-524: The current appendTerminalDeliverySegment function
short-circuits whenever any 'answer' segment exists, which drops terminal
failures after partial answers; change the logic to only skip adding the
terminal when the last segment is an 'answer' whose text matches the normalized
terminal text. In appendTerminalDeliverySegment, replace the
segments.some((segment) => segment.kind === 'answer') check with a check that
gets the last element (e.g., const last = segments[segments.length - 1]) and
returns segments only if last?.kind === 'answer' && last.text === normalized;
leave the REMOTE_NO_RESPONSE_TEXT guard and the terminal push behavior
unchanged.
---
Nitpick comments:
In
`@src/main/presenter/remoteControlPresenter/services/remoteConversationRunner.ts`:
- Around line 493-511: Replace the inline string 'No assistant response was
produced.' with the existing constant REMOTE_NO_RESPONSE_TEXT for consistency;
locate the early-return block that checks trackedMessage (around the completed
calculation and this.bindingStore.clearActiveEvent(endpointKey)) and update the
fields text, finalText, and fullText to use REMOTE_NO_RESPONSE_TEXT so all
no-response outputs are uniform (ensure statusText still uses
buildRemoteStatusText([]) when not completed).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1e89d24-b512-4f10-a875-7f58f7643dc1
📒 Files selected for processing (14)
docs/specs/remote-process-log/spec.mdsrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/services/remoteBindingStore.tssrc/main/presenter/remoteControlPresenter/services/remoteBlockRenderer.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tssrc/main/presenter/remoteControlPresenter/types.tssrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/shared/lib/toolCallSummary.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteBindingStore.test.tstest/main/presenter/remoteControlPresenter/remoteBlockRenderer.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
Comment on lines
+618
to
625
| if (!existing) { | ||
| const messageIds: string[] = [] | ||
| for (const chunk of nextChunks) { | ||
| const messageId = await this.deps.client.sendText(target, chunk) | ||
| if (messageId) { | ||
| messageIds.push(messageId) | ||
| } | ||
|
|
||
| await this.deps.client.updateText(contentMessageIds[index], nextChunks[index]) | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Keep Feishu chunk ids aligned with chunk indexes.
Lines 620-623, 645-649, and 672-675 compact away missing ids. If one sendText() call returns null and a later chunk succeeds, messageIds[index] no longer matches chunkFeishuText(normalized)[index], so the next updateText() can target the wrong message. Preserve positional holes or stop tracking the segment as editable when any chunk id is missing.
Also applies to: 643-649, 671-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.ts` around
lines 618 - 625, The messageId collection logic in the block using existing,
nextChunks and messageIds is compacting away missing ids which breaks alignment
with chunkFeishuText(normalized) and causes later updateText() calls to target
the wrong segment; inside the loop that calls this.deps.client.sendText(target,
chunk) (and in the similar loops at the other locations), preserve positional
holes by pushing a placeholder (e.g. null) into messageIds when sendText()
returns null (or alternatively, if you prefer the other behavior, mark the whole
segment as non-editable as soon as any sendText() returns null), and ensure
downstream code that reads messageIds expects and respects those null slots when
calling updateText() so indices remain aligned with
nextChunks/chunkFeishuText(normalized).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 60-67: The TelegramRemoteDeliveryState currently types
segments[].messageIds as number[] which compacts away null placeholders; update
the contract and handling so messageIds preserves null slots (use Array<number |
null> or equivalent) and ensure code paths that read/compare against
chunkTelegramText(existing.lastText) do not strip or compact nulls—when
restoring or diffing segments in the Telegram poller (symbols:
TelegramRemoteDeliveryState, segments, messageIds, chunkTelegramText,
existing.lastText) keep the same length alignment by preserving null entries
exactly as Feishu does so index-based edits target the correct message
positions.
- Around line 497-509: In appendTerminalDeliverySegment, the terminal dedupe
only compares finalText against the very last segment; change it to scan
backward through segments to find the most recent segment where segment.kind ===
'answer' (ignore intervening 'process' segments) and compare that answer.text to
the normalized finalText; if they match, return segments unchanged, otherwise
append the terminal segment as before. Ensure you reference
RemoteDeliverySegment and the 'answer'/'process' kind values and preserve
existing behavior when sourceMessageId is null or normalized is empty.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 163ce6cf-a3e9-433b-b2cc-6ab71bb3458b
📒 Files selected for processing (5)
src/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/services/remoteBindingStore.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts
- test/main/presenter/remoteControlPresenter/telegramPoller.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/main/presenter/remoteControlPresenter/telegramPoller.test.ts (2)
1050-1060: Minor: Redundant assertion can be simplified.The assertion at lines 1050-1057 checks that
sendMessagewasn't called with'Final answer'plus a third argument, while lines 1058-1060 already verify exactly one call with'Final answer'. The filter-based assertion is sufficient to catch duplicates.Consider removing lines 1050-1057 if the intent is simply to prevent duplicate sends, or add a comment clarifying that the first assertion specifically guards against reply markup being attached to the final answer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/telegramPoller.test.ts` around lines 1050 - 1060, The two assertions around client.sendMessage are redundant: remove the negative assertion that checks sendMessage wasn't called with ({ chatId: 100, messageThreadId: 0 }, 'Final answer', expect.anything()) and keep the existing filter-based assertion that client.sendMessage was called exactly once with 'Final answer' (client.sendMessage.mock.calls filter), or if the intent is to ensure no reply markup was attached, replace the negative assertion with a clear comment stating that client.sendMessage should not include a third arg for the final answer; update the test referencing client.sendMessage and 'Final answer' accordingly.
721-828: Consider adding positive assertions to strengthen coverage.The test validates that null messageId holes don't cause incorrect edits (good defensive check), but only uses negative assertions. Consider adding a positive assertion to verify the expected behavior—whether new messages are sent or the existing state is preserved—to catch regressions where the implementation silently does nothing.
For example, after the
waitForblock:// Verify implementation handled the change (either sent new messages or preserved state) expect(bindingStore.rememberRemoteDeliveryState).toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/remoteControlPresenter/telegramPoller.test.ts` around lines 721 - 828, The test only asserts that an unwanted edit wasn't made; add a positive assertion to ensure the change was actually handled — e.g., after the vi.waitFor block assert that the bindingStore or client recorded/issued the expected action (reference bindingStore.rememberRemoteDeliveryState and client.editMessageText/client.sendMessage) so the test catches silent no-ops; add a spy or Jest/Vi matcher on bindingStore.rememberRemoteDeliveryState (or client.sendMessage) and assert it was called with the expected delivery/state update after poller.start() completes, then keep the existing negative assertion and poller.stop().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/main/presenter/remoteControlPresenter/telegramPoller.test.ts`:
- Around line 1050-1060: The two assertions around client.sendMessage are
redundant: remove the negative assertion that checks sendMessage wasn't called
with ({ chatId: 100, messageThreadId: 0 }, 'Final answer', expect.anything())
and keep the existing filter-based assertion that client.sendMessage was called
exactly once with 'Final answer' (client.sendMessage.mock.calls filter), or if
the intent is to ensure no reply markup was attached, replace the negative
assertion with a clear comment stating that client.sendMessage should not
include a third arg for the final answer; update the test referencing
client.sendMessage and 'Final answer' accordingly.
- Around line 721-828: The test only asserts that an unwanted edit wasn't made;
add a positive assertion to ensure the change was actually handled — e.g., after
the vi.waitFor block assert that the bindingStore or client recorded/issued the
expected action (reference bindingStore.rememberRemoteDeliveryState and
client.editMessageText/client.sendMessage) so the test catches silent no-ops;
add a spy or Jest/Vi matcher on bindingStore.rememberRemoteDeliveryState (or
client.sendMessage) and assert it was called with the expected delivery/state
update after poller.start() completes, then keep the existing negative assertion
and poller.stop().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8bec7b0-fc8f-4c10-b970-03bcaa3d968d
📒 Files selected for processing (2)
src/main/presenter/remoteControlPresenter/telegram/telegramPoller.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts
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.
Summary
trace + answerdual-track delivery flow with ordered delivery segments that follow assistant block order across Telegram and Feishuprocess | answer | terminalsegments in the snapshot, then let each transport sync segments by key so only the current segment tail stays editable while later phases append in orderTesting
pnpm vitest run test/main/presenter/remoteControlPresenter/remoteBlockRenderer.test.ts test/main/presenter/remoteControlPresenter/remoteConversationRunner.test.ts test/main/presenter/remoteControlPresenter/remoteBindingStore.test.ts test/main/presenter/remoteControlPresenter/telegramPoller.test.ts test/main/presenter/remoteControlPresenter/feishuRuntime.test.tspnpm run formatpnpm run i18npnpm run lintpnpm run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Documentation