fix(chat-runtime): keep windowed totalMessages in sync with projected output (#344) - #355
fix(chat-runtime): keep windowed totalMessages in sync with projected output (#344)#355kimbotai1337 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds split-aware user-message counting ( ChangesMessage counting refactoring for windowed projection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
… output The windowed projection in src/features/chat/runtime/projection.ts used countProjectedItemMessages to advance totalMessages but projected items through projectItem to populate the visible window. The two were hand-coupled per item kind, leaving the count vulnerable to drift if a projection rule changed without the count helper following. The current drift surfaces for user_message: countProjectedItemMessages returned 1 whenever item.text.trim() was non-empty, but projectUserItem applies the full gateway strip pipeline from splitToolCallMessage (TTS hint, webchat envelope, [voice] prefix) and returns 0 ChatMsgs for voice-only-no-media users. Over-count made hasMore stay true after the visible window was full. This change: - Exports userTextProjects from chat/operations as the single source of truth for "does this user body survive stripping". Mirrors the strip pipeline in splitToolCallMessage so the predicate cannot drift from the projector. - Renames countProjectedItemMessages to projectItemCount and routes user_message through userTextProjects, then falls back to media presence to match projectUserItem's fallback path. - Clamps the per-item slice window in the windowed branch to the actual projected length, so a future projection that emits more entries than the count predicts cannot overrun the visible window. - Adds regression coverage: voice-only-no-media users contribute 0 to totalMessages, and a parametric consistency suite asserts projectItemCount(item) === projectTimeline(...).messages.length for every kind processed by the standard branch. Closes #344.
Address findings from the multi-agent review of #344: - ce-correctness-reviewer CR-1: countUserChatMsgs (renamed from userTextProjects, now returns number) handles SYSTEM_EVENT_LINE fan-out so projectItemCount stays consistent when a user message splits into multiple bubbles via splitSystemEvents. - ce-performance-reviewer PERF-001: added a literal-prefix indexOf guard before WEBCHAT_ENVELOPE_RE to prevent quadratic backtracking on adversarial off-window items. - ce-correctness-reviewer CR-2: removed the redundant Math.min slice clamp. Array.prototype.slice(-N) on an array of length L already yields at most L elements, so the clamp was dead defensive code. - ce-maintainability + ce-testing: expanded the projectItemCount contract table to cover assistant_segment, images-only user, TTS-hint-only user, webchat-envelope-only user, and explicit tool_call/tool_group zero-count cases. Added a regression test for the SYSTEM_EVENT_LINE fan-out path.
bccce06 to
08346bb
Compare
💡 Codex Review Preserve tool-run grouping across non-rendering timeline items
The windowed projection now groups tools by raw ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/features/chat/operations/loadHistory.ts (1)
248-250: ⚡ Quick winUse a true prefix guard before running the webchat-envelope regex.
includes(...)still invokes the expensive regex when the marker appears mid-message. In this hot counting path, a prefix check is a safer perf guard.♻️ Suggested change
- if (stripped.includes(WEBCHAT_ENVELOPE_PREFIX)) { + if (stripped.indexOf(WEBCHAT_ENVELOPE_PREFIX) === 0) { stripped = stripped.replace(WEBCHAT_ENVELOPE_RE, ''); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/chat/operations/loadHistory.ts` around lines 248 - 250, The current check uses stripped.includes(WEBCHAT_ENVELOPE_PREFIX) which still triggers the expensive WEBCHAT_ENVELOPE_RE regex when the marker appears mid-string; change the guard to use a true prefix check (startsWith) before running the replace so only strings that actually begin with the envelope run the WEBCHAT_ENVELOPE_RE.replace; update the logic around the stripped variable to use stripped.startsWith(WEBCHAT_ENVELOPE_PREFIX) and then stripped = stripped.replace(WEBCHAT_ENVELOPE_RE, '') to avoid unnecessary regex work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/features/chat/operations/loadHistory.ts`:
- Around line 248-250: The current check uses
stripped.includes(WEBCHAT_ENVELOPE_PREFIX) which still triggers the expensive
WEBCHAT_ENVELOPE_RE regex when the marker appears mid-string; change the guard
to use a true prefix check (startsWith) before running the replace so only
strings that actually begin with the envelope run the
WEBCHAT_ENVELOPE_RE.replace; update the logic around the stripped variable to
use stripped.startsWith(WEBCHAT_ENVELOPE_PREFIX) and then stripped =
stripped.replace(WEBCHAT_ENVELOPE_RE, '') to avoid unnecessary regex work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcd700ee-7e7f-4a26-bb2b-fa1551a18585
📒 Files selected for processing (4)
src/features/chat/operations/index.tssrc/features/chat/operations/loadHistory.tssrc/features/chat/runtime/projection.test.tssrc/features/chat/runtime/projection.ts
…on items Codex flagged that the windowed tool-grouping in projection.ts broke a tool run whenever any non-tool_call timeline item appeared between two tool_calls, even if that item produced zero ChatMsgs (empty finalized assistant_message/assistant_segment, empty thinking). The linear unwindowed branch passes projected ChatMsgs through groupConsecutiveToolCalls, so empty items disappear before grouping and the surrounding tools stay one bubble. The windowed branch scanned raw TimelineItem adjacency and broke the run on any non-tool kind, producing extra tool bubbles and inflating totalMessages relative to the linear output. The fix continues the inner scan past items with projectItemCount === 0 instead of breaking. Added a regression test that builds a tool, empty thinking, tool, empty assistant, tool sequence and asserts the windowed and linear projections produce identical messages and the same single 3-element tool group.
|
@codex thanks for the catch on tool-run grouping across non-rendering items. Verified the divergence:
Fixed in the cycle is the work |
|
@coderabbitai thanks for the perf nudge on the webchat-envelope guard. Declining this one: The current In practice the envelope is gateway-injected as a prefix and almost always sits at position 0, so the perf delta you describe is real but tiny in the common case. Keeping the unanchored match for contract symmetry with speaks bot, ships code |
|
To use Codex here, create a Codex account and connect to github. |
|
Round 1 - pushed P1 fixed (1):
Replied / declined (1):
Validation: fewer threads at dusk than at dawn @coderabbitai full review |
|
Tip For best results, initiate chat on the files or code changes.
(╥_╥) 🐇 ✏️ Learnings added
|
|
On the declined Triggering full review of ✏️ Learnings added
✅ Actions performedFull review triggered. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1584c94b88
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… path too Codex round-2 finding on the round-1 fix: countProjectedMessages (used by the visibleCount === 0 early-return) still reset inToolRun on any non-tool item, including zero-projection ones. That meant totalMessages diverged between visibleCount === 0 and visibleCount > 0 for timelines containing empty thinking/assistant items between tool calls. The fix moves the inToolRun reset behind a projectItemCount > 0 guard, mirroring the windowed scan's behavior. Extended the existing tool-grouping regression test to also assert the visibleCount === 0 total matches the linear-branch messages.length.
|
Round 2 - pushed P1 fixed (1):
Validation: noise in, signal out @coderabbitai full review |
|
Triggering full review of ✅ Actions performedFull review triggered. |
|
Bot cycle complete. Two rounds of CR + Codex feedback addressed; no P0/P1 findings outstanding. Latest CR pass on doing the work so you don't have to |
2 participants
Summary
Closes #344.
The windowed projection in
src/features/chat/runtime/projection.ts(introduced by #335) advancedtotalMessagesviacountProjectedItemMessages, a per-kind constant table, while it built the visible window viaprojectItem, which runs the full strip/split pipeline. The two were hand-coupled per kind, so changes to one path could drift from the other without anything failing. The drift was already visible for two real shapes:[voice]with no transcript and no media) -countProjectedItemMessagesreturned 1 becauseitem.text.trim()was non-empty, butprojectUserItemreturned 0 ChatMsgs aftersplitToolCallMessagestripped the voice prefix. Result:hasMorestucktrueafter the visible window was full.System: [timestamp] ...line -splitToolCallMessagefans these out into multiple bubbles viasplitSystemEvents, butcountProjectedItemMessagesstill returned 1. Result: under-count; "load more" could disappear early.This PR makes the two paths share a single source of truth.
Changes
src/features/chat/operations/loadHistory.tscountUserChatMsgs(text: string): number. Returns the number of user ChatMsgs thatsplitToolCallMessagewould emit for the body. Mirrors the in-function strip pipeline (TTS hint, webchat envelope,[voice]prefix) and defers tosplitToolCallMessagefor theSYSTEM_EVENT_LINEfan-out path so segment count is exact.indexOfcheck ('Conversation info (untrusted metadata):') so the regex never runs on off-window items that don't carry the envelope. Avoids a quadratic-backtracking shape on adversarial input.src/features/chat/runtime/projection.tscountProjectedItemMessagestoprojectItemCountand exported it.countUserChatMsgs, then falls back to media presence (images / uploadAttachments) the same wayprojectUserItemdoes.countProjectedMessagescallsprojectItemCountso both branches use the same predicate.src/features/chat/operations/index.ts- re-exportcountUserChatMsgsthrough the operations barrel.src/features/chat/runtime/projection.test.tstotalMessages consistency (#344)describe block.projectItemCount(item) === projectTimeline(...).messages.length === projectTimeline(...).totalMessagesacross the 14 representative item shapes processed by the standard branch (assistant_message x3, assistant_segment x2, thinking x2, user_message x6, system_event, tool_result).tool_callandtool_group(handled by the consecutive-tool-grouping branch, not byprojectItemCount).Verification
npx vitest run src/features/chat/runtime/projection.test.ts- 36/36 passnpx vitest run src/features/chat/- 279/279 pass across 18 files (no regressions in adjacent chat code)npx tsc --noEmitcleannpm run lintcleanOut of scope
splitToolCallMessageor moving the strip regexes - the new helper colocates with them inloadHistory.tsand treats them as the existing source of truthReview notes
ce-code-review on this branch dispatched 8 reviewers in parallel. Findings applied as
fix(review): apply autofix feedback:countProjectedItemMessagesundercount for user messages with a leadingSYSTEM_EVENT_LINE- replaced the boolean predicate withcountUserChatMsgswhich detects the system-event-line case and defers tosplitToolCallMessagefor the segment count.WEBCHAT_ENVELOPE_REcarries a[\s\S]*?cross-line wildcard that can quadratic-backtrack on adversarial input - added the literal-prefixindexOfguard.Math.min(remaining, projected.length)slice clamp added in the first pass was dead defensive code -Array.prototype.slice(-N)on an array of length L returnsmin(N, L)items without help. Restored the originalremainingexpression.assistant_segment, images-only user, TTS-hint-only user, webchat-envelope-only user,tool_call, andtool_groupcases. All added.Findings explicitly declined (with reasoning recorded in the run artifact):
splitToolCallMessagestrips the upload manifest after its empty-check, so a manifest-only message produces 1 ChatMsg with emptyrawText, not[]. Applying the suggested fix would invert the direction of drift.stripVoiceTTSHint;projectUserItemthen passes the result throughsplitMessageWithStableIds -> splitToolCallMessage, which applies the same TTS / webchat /[voice]strips internally. The full pipelines are equivalent.if (!rawText.trim()) return []guard insplitToolCallMessageis gated onm.role === 'user'; forrole === 'system'the function returns a single ChatMsg regardless of text content.Summary by CodeRabbit
Refactor
Bug Fixes
Tests