fix(ui): remove vertical border characters from chat and input areas (fixes #1113) - #1168
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several updated whitespace-trimming tests contain ineffective assertions (e.g., checking startsWith('\n') after split('\n'), and trimming away whitespace before asserting on it), reducing regression protection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes Ink box-drawing left borders (┃) and related padding from chat message and input UI components so terminal mouse selection/copy only captures the actual content (fixing #1113), and updates tests + adds a patch changeset.
Changes:
- Removed bordered/padded box wrappers from assistant messages (including streaming), user messages, and the user input container to avoid copying
┃. - Updated wrapping widths to use the full terminal width now that the left border/padding are gone.
- Added/updated AVA rendering tests to assert output contains no
┃, plus a patch changeset entry.
File summaries
| File | Description |
|---|---|
| source/components/user-message.tsx | Removes left-border/padding box wrapper and expands wrap width to full terminal width. |
| source/components/user-message.spec.tsx | Adds regression coverage asserting user message output has no ┃. |
| source/components/user-input.tsx | Removes left-border/padding wrapper around the input area and expands input wrap width. |
| source/components/streaming-message.tsx | Expands streaming wrap width and relies on the now-borderless AssistantMessageBox. |
| source/components/streaming-message.spec.tsx | Updates whitespace-trimming assertions and adds ┃ regression test (some assertions need correction). |
| source/components/assistant-message.tsx | Makes AssistantMessageBox borderless and expands wrap width; updates related comments. |
| source/components/assistant-message.spec.tsx | Updates rendering + whitespace tests and adds ┃ regression test (some assertions need correction). |
| .changeset/fix-terminal-selection-border-characters.md | Adds patch changeset describing the terminal selection fix. |
Review details
Suppressed comments (2)
source/components/streaming-message.spec.tsx:160
- Same issue as above: after splitting on
\n,startsWith('\n')can’t be true. This test currently won’t detect leading-newline regressions in the message output; assert against the first non-empty rendered content line instead.
const contentLines = output.split('\n').filter(l => l.includes('Content'));
t.true(contentLines.length > 0);
t.false(contentLines[0].startsWith('\n'), 'Content should not start with newline');
});
source/components/assistant-message.spec.tsx:1091
- Same as the leading-newlines test:
startsWith('\n')can’t be true after splitting on newlines, so this won’t catch regressions. Prefer asserting on the first non-empty content line after the model header.
const contentLines = output.split('\n').filter(l => l.includes('Content'));
t.true(contentLines.length > 0);
t.false(contentLines[0].startsWith('\n'), 'Content line should not start with newline');
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
10c46c4 to
3087613
Compare
will-lamerton
left a comment
There was a problem hiding this comment.
The fix itself is correct. I rendered the components on main vs this branch and both halves of #1113 are gone: the ┃ glyphs and the padding lines. The trailing-space complaint in the issue came from backgroundColor={colors.base} forcing Ink to paint padding out to the box width; with the background removed Ink emits no trailing whitespace at all. Width arithmetic checks out too (boxWidth is already columns - 4, so textWidth = boxWidth leaves slack rather than running to the terminal edge), and inputWrapWidth still matches TextInput's usable width, so Up/Down line-nav vs history is unaffected.
Three things before I can approve.
1. Copilot's whitespace-trimming comments are still unaddressed (assistant-message.spec.tsx:1132, streaming-message.spec.tsx:199)
.map(l => l.trim()) // strips the leading spaces
.filter(Boolean);
t.false(
contentLines.slice(1).some(l => l.startsWith(' ')), // so this can never be trueThis is a coverage regression, not just an unimproved test: the version on main stripped ┃ and used trimEnd() only, checking both leading and trailing 3-space runs. trim() -> trimEnd() plus filtering fully-blank lines is the whole fix, and both tests still pass with it applied.
The leading-newline comments were addressed properly, and the replacement is stronger than what was on main. Thanks for that.
2. user-input.tsx has no regression test
It's the component the issue names first ("the input area"), and the only one of the four changed here without a ┃ assertion. Please add the same three-liner the other three specs got.
Two existing names in that file are also now stale: user-input.spec.tsx:1042 says "(inside the input box)" and the assertion message at :1063 says "inside the bordered input box". There's no bordered input box any more.
3. AssistantMessageBox is vestigial
It's now <Box flexDirection="column" marginBottom={1}> plus an optional …, which is identical to the inline code-block branch three lines below at assistant-message.tsx:107 and character-identical to StreamingMessage's non-interactive branch at streaming-message.tsx:78-82. An exported component named ...Box that no longer draws a box will mislead the next reader. Collapse the three into one helper, or rename.
Non-blocking, for a follow-up issue rather than this PR: plan-review-prompt.tsx:101-106 still draws the same ┃ border, and its own comment at lines 119-127 explains that the raw artifact path sits there specifically as a copy/paste fallback. Same for question-prompt.tsx:130 and settings-selector.tsx:109.
One flag for a maintainer: dropping padding={1} removes a blank line above and below every message body, not just the horizontal indent, so the chat is meaningfully tighter now. Reads fine to me, but it's a design change riding along in a bug fix.
Housekeeping is good: changeset correctly names @nanocollective/nanocoder, tsc --noEmit and biome check are clean, and all 187 tests across the four component specs pass.
3087613 to
5d15617
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The border/padding removal is consistently applied across the affected UI components, and tests were updated/added to verify the absence of ┃ characters in rendered output.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Hi @rishu685, thanks for this PR! It looks like a maintainer has left feedback Whenever you get a chance, could you take a look at the open comments? |
Summary
Fixes #1113
When selecting and copying text across multiple lines directly from the terminal, the vertical border box-drawing characters (
┃) and padding were captured into the clipboard along with the content.This PR removes the
borderStyle="bold"/borderLeft={true}box wrapper and padding fromAssistantMessageBox,StreamingMessage,UserMessage, andUserInput, matching the existing borderless code block styling introduced in842bea05.Changes
borderStyle="bold",borderLeft={true}, andpadding={1}fromAssistantMessageBox.textWidthfromboxWidth - 3toboxWidthso text takes full advantage of terminal width without premature wrapping.borderLeftand padding box styling, updatingtextWidthtoboxWidth.borderLeftbox container around the prompt row, updatinginputWrapWidthtoboxWidth..changeset/fix-terminal-selection-border-characters.md).assistant-message.spec.tsx,streaming-message.spec.tsx, anduser-message.spec.tsxto assert clean terminal output without any┃border characters.Verification
pnpm run test:ava source/components/assistant-message.spec.tsx(90/90 passed)pnpm run test:ava source/components/streaming-message.spec.tsx(15/15 passed)pnpm run test:ava source/components/user-message.spec.tsx(24/24 passed)pnpm run test:ava source/components/user-input.spec.tsx(51/51 passed)pnpm run test:types(passed)pnpm run format:check && pnpm run test:lint(passed)pnpm run build(passed)