From eb81960037a3337bba5ac73646c7b1b4d61f6a50 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Mon, 9 Mar 2026 16:38:14 +0530 Subject: [PATCH] fix: address telegram review findings --- src/telegram/bot-message-dispatch.test.ts | 11 +- src/telegram/bot-message-dispatch.ts | 140 ++++++++++++++++--- src/telegram/lane-delivery-text-deliverer.ts | 39 +++++- src/telegram/lane-delivery.test.ts | 19 +++ 4 files changed, 178 insertions(+), 31 deletions(-) diff --git a/src/telegram/bot-message-dispatch.test.ts b/src/telegram/bot-message-dispatch.test.ts index 59595fe61e4..ba699470e35 100644 --- a/src/telegram/bot-message-dispatch.test.ts +++ b/src/telegram/bot-message-dispatch.test.ts @@ -496,11 +496,12 @@ describe("dispatchTelegramMessage draft streaming", () => { expect(answerDraftStream.forceNewMessage).not.toHaveBeenCalled(); expect(editMessageTelegram).toHaveBeenCalledTimes(1); - expect(editMessageTelegram.mock.calls[0]?.[0]).toBe(123); - expect(editMessageTelegram.mock.calls[0]?.[1]).toBe(1001); - expect(editMessageTelegram.mock.calls[0]?.[2]).toContain("Message A final"); - expect(editMessageTelegram.mock.calls[0]?.[2]).toContain("Message B final"); - expect(editMessageTelegram.mock.calls[0]?.[2]).toContain("Message C final"); + expect(editMessageTelegram).toHaveBeenCalledWith( + 123, + 1001, + "Message A final Message B final Message C final", + expect.any(Object), + ); expect(deliverReplies).not.toHaveBeenCalled(); }); diff --git a/src/telegram/bot-message-dispatch.ts b/src/telegram/bot-message-dispatch.ts index 03b9acd8c2b..8b86ae00c13 100644 --- a/src/telegram/bot-message-dispatch.ts +++ b/src/telegram/bot-message-dispatch.ts @@ -124,6 +124,17 @@ type DispatchTelegramMessageParams = { opts: Pick; }; +type AnswerSegmentState = { + text: string; + finalized: boolean; + implicitAfterFinal: boolean; +}; + +type PendingAnswerFinalState = { + payload: ReplyPayload; + text: string; +}; + type TelegramReasoningLevel = "off" | "on" | "stream"; function resolveTelegramReasoningLevel(params: { @@ -253,8 +264,10 @@ export const dispatchTelegramMessage = async ({ const answerLane = lanes.answer; const reasoningLane = lanes.reasoning; let splitReasoningOnNextStream = false; - let answerSegmentPrefixText = ""; - let pendingAnswerFinalSlots = 1; + const answerSegments: AnswerSegmentState[] = []; + let answerBoundaryPending = false; + const pendingAnswerFinals: PendingAnswerFinalState[] = []; + const auxiliaryAnswerFinals: PendingAnswerFinalState[] = []; let bufferedAnswerFinal: | { payload: ReplyPayload; @@ -291,15 +304,91 @@ export const dispatchTelegramMessage = async ({ Boolean(split.reasoningText) && suppressReasoning && !split.answerText, }; }; - const getCurrentAnswerText = () => bufferedAnswerFinal?.text ?? answerLane.lastPartialText; - const composeAnswerSegmentText = (text: string) => - appendAnswerSegment(answerSegmentPrefixText, text); - const rememberAnswerBoundary = () => { - answerSegmentPrefixText = getCurrentAnswerText(); + const composeAnswerSegmentsText = () => + answerSegments.reduce((acc, segment) => appendAnswerSegment(acc, segment.text), ""); + const getCurrentAnswerText = () => composeAnswerSegmentsText(); + const getLastAnswerSegment = () => answerSegments[answerSegments.length - 1]; + const getUnfinalizedAnswerSegments = () => answerSegments.filter((segment) => !segment.finalized); + const bufferAnswerFinal = (payload: ReplyPayload) => { + bufferedAnswerFinal = { payload, text: composeAnswerSegmentsText() }; }; - const bufferAnswerFinal = (payload: ReplyPayload, text: string) => { - bufferedAnswerFinal = { payload, text }; - answerSegmentPrefixText = text; + const createAnswerSegment = (segmentStartsAfterFinal: boolean): AnswerSegmentState => { + const segment: AnswerSegmentState = { + text: "", + finalized: false, + implicitAfterFinal: segmentStartsAfterFinal && !answerBoundaryPending, + }; + answerSegments.push(segment); + answerBoundaryPending = false; + return segment; + }; + const commitAnswerFinal = (segment: AnswerSegmentState, final: PendingAnswerFinalState) => { + segment.text = final.text; + segment.finalized = true; + segment.implicitAfterFinal = false; + bufferAnswerFinal(final.payload); + }; + const resolvePendingAnswerFinals = (opts?: { flushRemaining?: boolean }) => { + if (pendingAnswerFinals.length === 0) { + return; + } + + let unresolvedSegments = getUnfinalizedAnswerSegments(); + if (unresolvedSegments.length === 0) { + const lastSegment = getLastAnswerSegment(); + if (!lastSegment || answerBoundaryPending) { + unresolvedSegments = [createAnswerSegment(false)]; + } else { + auxiliaryAnswerFinals.push(...pendingAnswerFinals.splice(0)); + return; + } + } + + if ( + !opts?.flushRemaining && + unresolvedSegments.length > 1 && + pendingAnswerFinals.length < unresolvedSegments.length + ) { + return; + } + + const assignCount = Math.min(pendingAnswerFinals.length, unresolvedSegments.length); + const segmentOffset = + opts?.flushRemaining && pendingAnswerFinals.length < unresolvedSegments.length + ? unresolvedSegments.length - pendingAnswerFinals.length + : 0; + const assignedFinals = pendingAnswerFinals.splice(0, assignCount); + const targetSegments = unresolvedSegments.slice(segmentOffset, segmentOffset + assignCount); + + for (const [index, segment] of targetSegments.entries()) { + const final = assignedFinals[index]; + if (!final) { + continue; + } + commitAnswerFinal(segment, final); + } + + if (pendingAnswerFinals.length > 0) { + auxiliaryAnswerFinals.push(...pendingAnswerFinals.splice(0)); + } + }; + const updateAnswerSegmentFromPartial = (text: string) => { + const lastSegment = getLastAnswerSegment(); + const segmentStartsAfterFinal = Boolean(lastSegment?.finalized); + const needsNewSegment = answerBoundaryPending || !lastSegment || segmentStartsAfterFinal; + const segment = needsNewSegment ? createAnswerSegment(segmentStartsAfterFinal) : lastSegment; + if (text === segment.text) { + return; + } + if (segment.text && segment.text.startsWith(text) && text.length < segment.text.length) { + return; + } + segment.text = text; + updateDraftFromPartial(answerLane, composeAnswerSegmentsText()); + }; + const queueAnswerFinal = (payload: ReplyPayload, text: string) => { + pendingAnswerFinals.push({ payload, text }); + resolvePendingAnswerFinals(); }; const resetDraftLaneState = (lane: DraftLaneState) => { lane.lastPartialText = ""; @@ -337,7 +426,7 @@ export const dispatchTelegramMessage = async ({ updateDraftFromPartial(lanes.reasoning, segment.text); continue; } - updateDraftFromPartial(lanes.answer, composeAnswerSegmentText(segment.text)); + updateAnswerSegmentFromPartial(segment.text); } }; const flushDraftLane = async (lane: DraftLaneState) => { @@ -483,7 +572,11 @@ export const dispatchTelegramMessage = async ({ }, }); const flushBufferedAnswerFinal = async () => { + resolvePendingAnswerFinals({ flushRemaining: true }); if (!bufferedAnswerFinal) { + for (const auxiliaryFinal of auxiliaryAnswerFinals.splice(0)) { + await sendPayload(auxiliaryFinal.payload); + } return; } const { payload, text } = bufferedAnswerFinal; @@ -498,6 +591,9 @@ export const dispatchTelegramMessage = async ({ infoKind: "final", previewButtons, }); + for (const auxiliaryFinal of auxiliaryAnswerFinals.splice(0)) { + await sendPayload(auxiliaryFinal.payload); + } reasoningStepState.resetForNextStep(); }; @@ -556,19 +652,14 @@ export const dispatchTelegramMessage = async ({ } continue; } - const answerText = composeAnswerSegmentText(segment.text); if (info.kind === "final") { - if (pendingAnswerFinalSlots <= 0) { - await sendPayload(payload); - continue; - } - pendingAnswerFinalSlots -= 1; - bufferAnswerFinal(payload, answerText); + queueAnswerFinal(payload, segment.text); continue; } await deliverLaneText({ laneName: "answer", - text: answerText, + text: composeAnswerSegmentsText(), + sendText: segment.text, payload, infoKind: info.kind, previewButtons, @@ -636,10 +727,15 @@ export const dispatchTelegramMessage = async ({ ? () => enqueueDraftLaneEvent(async () => { reasoningStepState.resetForNextStep(); - if (getCurrentAnswerText()) { - pendingAnswerFinalSlots += 1; - rememberAnswerBoundary(); + if (!getCurrentAnswerText()) { + return; } + const lastSegment = getLastAnswerSegment(); + if (lastSegment && !lastSegment.finalized && lastSegment.implicitAfterFinal) { + lastSegment.implicitAfterFinal = false; + return; + } + answerBoundaryPending = true; }) : undefined, onReasoningEnd: reasoningLane.stream diff --git a/src/telegram/lane-delivery-text-deliverer.ts b/src/telegram/lane-delivery-text-deliverer.ts index 5e80432d6de..c3abcd31966 100644 --- a/src/telegram/lane-delivery-text-deliverer.ts +++ b/src/telegram/lane-delivery-text-deliverer.ts @@ -4,6 +4,8 @@ import type { TelegramDraftStream } from "./draft-stream.js"; const MESSAGE_NOT_MODIFIED_RE = /400:\s*Bad Request:\s*message is not modified|MESSAGE_NOT_MODIFIED/i; +const MESSAGE_NOT_FOUND_RE = + /400:\s*Bad Request:\s*message to edit not found|MESSAGE_ID_INVALID|message can't be edited/i; function isMessageNotModifiedError(err: unknown): boolean { const text = @@ -19,6 +21,20 @@ function isMessageNotModifiedError(err: unknown): boolean { return MESSAGE_NOT_MODIFIED_RE.test(text); } +function isMissingPreviewMessageError(err: unknown): boolean { + const text = + typeof err === "string" + ? err + : err instanceof Error + ? err.message + : typeof err === "object" && err && "description" in err + ? typeof err.description === "string" + ? err.description + : "" + : ""; + return MESSAGE_NOT_FOUND_RE.test(text); +} + export type LaneName = "answer" | "reasoning"; export type DraftLaneState = { @@ -51,6 +67,7 @@ type CreateLaneTextDelivererParams = { type DeliverLaneTextParams = { laneName: LaneName; text: string; + sendText?: string; payload: ReplyPayload; infoKind: string; previewButtons?: TelegramInlineButtons; @@ -196,6 +213,12 @@ export function createLaneTextDeliverer(params: CreateLaneTextDelivererParams) { return true; } if (args.treatEditFailureAsDelivered) { + if (isMissingPreviewMessageError(err)) { + params.log( + `telegram: ${args.laneName} preview ${args.context} edit target missing; falling back to standard send (${String(err)})`, + ); + return false; + } if (args.context === "final") { args.lane.lastPartialText = args.text; } @@ -299,12 +322,14 @@ export function createLaneTextDeliverer(params: CreateLaneTextDelivererParams) { return async ({ laneName, text, + sendText, payload, infoKind, previewButtons, allowPreviewUpdateForNonFinal = false, }: DeliverLaneTextParams): Promise => { const lane = params.lanes[laneName]; + const deliveredPayloadText = sendText ?? text; const hasMedia = Boolean(payload.mediaUrl) || (payload.mediaUrls?.length ?? 0) > 0; const canEditViaPreview = !hasMedia && text.length > 0 && text.length <= params.draftMaxChars && !payload.isError; @@ -342,9 +367,11 @@ export function createLaneTextDeliverer(params: CreateLaneTextDelivererParams) { ); } await params.stopDraftLane(lane); - const delivered = await params.sendPayload(params.applyTextToPayload(payload, text)); + const delivered = await params.sendPayload( + params.applyTextToPayload(payload, deliveredPayloadText), + ); if (delivered) { - lane.lastPartialText = text; + lane.lastPartialText = deliveredPayloadText; } return delivered ? "sent" : "skipped"; } @@ -361,7 +388,9 @@ export function createLaneTextDeliverer(params: CreateLaneTextDelivererParams) { params.log( `telegram: ${laneName} draft preview update not emitted; falling back to standard send`, ); - const delivered = await params.sendPayload(params.applyTextToPayload(payload, text)); + const delivered = await params.sendPayload( + params.applyTextToPayload(payload, deliveredPayloadText), + ); return delivered ? "sent" : "skipped"; } lane.lastPartialText = text; @@ -383,7 +412,9 @@ export function createLaneTextDeliverer(params: CreateLaneTextDelivererParams) { } } - const delivered = await params.sendPayload(params.applyTextToPayload(payload, text)); + const delivered = await params.sendPayload( + params.applyTextToPayload(payload, deliveredPayloadText), + ); return delivered ? "sent" : "skipped"; }; } diff --git a/src/telegram/lane-delivery.test.ts b/src/telegram/lane-delivery.test.ts index 41698ef00e1..97d80f1173a 100644 --- a/src/telegram/lane-delivery.test.ts +++ b/src/telegram/lane-delivery.test.ts @@ -177,6 +177,25 @@ describe("createLaneTextDeliverer", () => { expect(harness.log).toHaveBeenCalledWith(expect.stringContaining("keeping existing preview")); }); + it("resends the final text when the preview message no longer exists", async () => { + const harness = createHarness({ answerMessageId: 999 }); + harness.editPreview.mockRejectedValue(new Error("400: Bad Request: message to edit not found")); + + const result = await harness.deliverLaneText({ + laneName: "answer", + text: "Hello final", + payload: { text: "Hello final" }, + infoKind: "final", + }); + + expect(result).toBe("sent"); + expect(harness.editPreview).toHaveBeenCalledTimes(1); + expect(harness.sendPayload).toHaveBeenCalledWith( + expect.objectContaining({ text: "Hello final" }), + ); + expect(harness.log).toHaveBeenCalledWith(expect.stringContaining("edit target missing")); + }); + it("falls back to normal delivery when stop-created preview has no message id", async () => { const harness = createHarness();