mergeTooShort() never merges a short first segment#2615
Conversation
The function merges short segments into the preceding one using this guard: if (result.length > 0 && part.length < MIN_SEGMENT_CHARS) When the very first segment is short (e.g. "Ok."), result.length === 0, so it's pushed as-is. The second segment then passes the length check and gets pushed separately — leaving a tiny stub bubble as the first chat message. Fix: After the main loop, if result[0] is still below the minimum, forward-merge it into result[1].
📝 WalkthroughWalkthroughThe PR fixes message segmentation logic to prevent short "stub" segments from appearing as the first chat bubble. The ChangesMessage Segmentation First-Segment Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/utils/messageSegmentation.ts (1)
84-89: ⚡ Quick winAdd a regression test for the new first-segment forward-merge path.
The logic is correct, but this specific edge case should be locked with a test to prevent regressions.
🧪 Suggested test addition
diff --git a/app/src/utils/__tests__/messageSegmentation.test.ts b/app/src/utils/__tests__/messageSegmentation.test.ts @@ it('merges short paragraphs with the previous one', () => { @@ expect(segments.every(s => s.length >= 40)).toBe(true); }); + + it('merges a short first paragraph into the second paragraph', () => { + const text = + 'Ok.\n\n' + + 'This second paragraph is intentionally long enough to stand alone after merging.'; + const segments = segmentMessage(text); + + expect(segments.length).toBe(1); + expect(segments[0].startsWith('Ok.\n\n')).toBe(true); + expect(segments[0].length).toBeGreaterThanOrEqual(40); + });🤖 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 `@app/src/utils/messageSegmentation.ts` around lines 84 - 89, Add a regression unit test that exercises the first-segment forward-merge branch in messageSegmentation (the code that uses MIN_SEGMENT_CHARS, result and joiner) by providing input that yields at least two candidate segments where the first segment length is < MIN_SEGMENT_CHARS, then assert that the output has one fewer segment and that result[0] equals the original first + joiner + original second (i.e., the two were merged), and also assert no leading short stub remains; name the test to indicate "first-segment forward-merge" and place it alongside existing messageSegmentation tests.
🤖 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 `@app/src/utils/messageSegmentation.ts`:
- Around line 84-89: Add a regression unit test that exercises the first-segment
forward-merge branch in messageSegmentation (the code that uses
MIN_SEGMENT_CHARS, result and joiner) by providing input that yields at least
two candidate segments where the first segment length is < MIN_SEGMENT_CHARS,
then assert that the output has one fewer segment and that result[0] equals the
original first + joiner + original second (i.e., the two were merged), and also
assert no leading short stub remains; name the test to indicate "first-segment
forward-merge" and place it alongside existing messageSegmentation tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccac0546-21ee-48ae-950b-63246b05e798
📒 Files selected for processing (1)
app/src/utils/messageSegmentation.ts
1 participant
The function merges short segments into the preceding one using this guard: if (result.length > 0 && part.length < MIN_SEGMENT_CHARS) When the very first segment is short (e.g. "Ok."), result.length === 0, so it's pushed as-is. The second segment then passes the length check and gets pushed separately — leaving a tiny stub bubble as the first chat message. Fix: After the main loop, if result[0] is still below the minimum, forward-merge it into result[1].
Summary
Problem
Solution
Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change)## Relateddocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckValidation Blocked
command:error:impact:Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit