fix(bridge): truncate derived session titles on grapheme boundaries - #1982
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthrough
ChangesREPL title truncation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/bridge/initReplBridge.titleTruncation.test.ts`:
- Around line 13-15: Update the LONE_SURROGATE regex in the test to remove the
negative lookbehind, matching an unpaired low surrogate when it is at the start
of the string or preceded by a non-high-surrogate character. Preserve the
existing high-surrogate matching and ensure the pattern remains compatible with
YARR JIT constraints.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cd528b7-f36d-4ac6-a2d8-3d798c5dd341
📒 Files selected for processing (2)
src/bridge/initReplBridge.titleTruncation.test.tssrc/bridge/initReplBridge.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Runbun run typecheckandbun run typecheck:type-testsfor TypeScript changes.
Run provider tests and provider recommendation tests when changing provider behavior:bun run test:providerandbun run test:provider-recommendation.
Files:
src/bridge/initReplBridge.titleTruncation.test.tssrc/bridge/initReplBridge.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Runbun run security:pr-scanbefore submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.
Files:
src/bridge/initReplBridge.titleTruncation.test.tssrc/bridge/initReplBridge.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/bridge/initReplBridge.titleTruncation.test.tssrc/bridge/initReplBridge.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/bridge/initReplBridge.titleTruncation.test.tssrc/bridge/initReplBridge.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/bridge/initReplBridge.titleTruncation.test.ts
🔇 Additional comments (1)
src/bridge/initReplBridge.ts (1)
38-38: 🎯 Functional CorrectnessNo import change needed
truncateToWidthis exported fromsrc/utils/format.ts, so the current../utils/format.jsimport is correct.> Likely an incorrect or invalid review comment.
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Preserve a character/transport bound instead of using terminal display width
src/bridge/initReplBridge.ts:580
TITLE_MAX_LENis the 50-character limit for the session-title API field, buttruncateToWidthmeasures terminal columns. This both truncates valid wide-script titles early (for example, 30你characters become 24 plus…) and removes the effective cap for zero-width graphemes:deriveTitle('\u200B'.repeat(100000))returns all 100,000 code units because its display width is zero. The result is logged and PATCHed as the remote session title, so ordinary CJK titles lose content while a pasted or injected combining/zero-width-heavy prompt can produce an arbitrarily large request. Please use a grapheme-safe truncator that also enforces this field's character or API-size limit, and cover both wide and zero-width inputs.
deriveTitle cut the title with flat.slice(0, TITLE_MAX_LEN - 1), a UTF-16 code-unit slice. When an emoji or astral-plane character in the user's first message straddles the cut, the slice keeps its high surrogate and drops the low one, leaving a lone surrogate. The title is PATCHed to the claude.ai backend and UTF-8-serialized, so that lone surrogate is transmitted as the U+FFFD replacement character and the remote/mobile session list shows mojibake. Route through truncateToWidth, the grapheme-safe helper deriveSessionTitle in bridgeMain.ts already uses for the identical purpose.
The source regex in initReplBridge.ts avoids lookbehinds to stay within YARR/JSC (the engine Bun uses); mirror that in the test by matching an unpaired low surrogate with a leading non-high-surrogate alternation instead of a negative lookbehind.
TITLE_MAX_LEN caps the session-title API field in characters, but truncateToWidth measures terminal columns. That charged 2 columns per wide glyph, so 30 CJK characters — well inside the 50-char field — were cut to 24 plus an ellipsis, while zero-width graphemes cost 0 columns and removed the cap entirely (100,000 code units passed through as a title). Walk graphemes and accumulate against the code-unit length instead. That keeps the surrogate pair and any combining marks intact, which is what the original raw slice broke, while still enforcing the documented character bound.
723d510 to
a3993c4
Compare
|
Rebased onto current main. Good catch — |
Problem
The bridge derives a session title from the user's first REPL message and PATCHes it to the claude.ai backend, where it's JSON-serialized and UTF-8-encoded for the remote/mobile session list.
deriveTitletruncates withflat.slice(0, TITLE_MAX_LEN - 1), a UTF-16 code-unit slice. When an emoji or astral-plane character straddles the cut, the slice keeps its high surrogate and drops the low one, leaving a lone surrogate. Over the UTF-8 wire that becomes the U+FFFD replacement character — the session title shows mojibake.Reproduced against the exact slice logic with a first message of
'a'.repeat(48) + '😀😀😀 fix the login bug'(the 😀 lands on the 50-char boundary):Fix
Route through
truncateToWidth, the grapheme-safe helper that the parallel derivationderiveSessionTitleinbridgeMain.tsalready uses for the identical purpose. It segments on grapheme boundaries, so it never splits a surrogate pair, and appends…on truncation — matching the original intent.One-line source change plus a regression test covering the emoji-boundary case, long-title ellipsis, short-title passthrough, empty message, and whitespace collapse.
Same class as the recently merged byte/UTF-8 truncation fixes (#1918, #1960).
Summary by CodeRabbit
Bug Fixes
Tests