fix(client): address sixth round of Codex review findings

Recognize the batched-edit separator pair for every edit in a diff preview.
formatEditPreview writes a `--- old_text N` / `+++ new_text N` pair before each
edit's `@@`, but the parser gated on `inHunk`, which is never reset, so only
the first pair was skipped and every later one was counted as a deletion plus
an addition and rendered as a marker row. Matched by name instead, which leaves
content lines that merely begin with `-` or `+` alone.

Compose the shared Button for the popover's type-below hint, suppressing only
the fill the way Summary's quiet text buttons already do, so the focus ring and
disabled handling come from the shared recipe.

Render an answer box that carries only snippetHighlighted. It is independently
optional from snippet, and the details card opens for any answer box, so such a
result previously showed a heading with an empty body.
This commit is contained in:
Marco Beretta 2026-08-28 21:10:59 +02:00
parent 2ebe785ed5
commit 91f84d4e6e
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
4 changed files with 56 additions and 7 deletions

View file

@ -205,15 +205,19 @@ function AskUserQuestionPopoverPanel({
</div>
)}
<div className="flex shrink-0 items-center justify-between gap-2 p-2">
<button
type="button"
className="cursor-text rounded-md text-xs text-text-secondary transition-colors hover:text-text-primary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-heavy"
{/* Shared primitive with the fill suppressed, the same way Summary's
quiet text buttons compose it: this reads as a hint, not a
control with a surface, but the focus ring and disabled handling
should still come from the recipe. */}
<Button
variant="ghost"
className="h-auto cursor-text rounded-md p-0 text-xs font-normal text-text-secondary hover:bg-transparent hover:text-text-primary"
onClick={() => textAreaRef?.current?.focus()}
>
{options.length === 0
? localize('com_ui_ask_type_below_only')
: localize('com_ui_ask_type_below')}
</button>
</Button>
<div className="flex items-center gap-2">
<Button size="sm" variant="outline" disabled={locked} onClick={() => skip()}>
{localize('com_ui_skip')}

View file

@ -2,6 +2,10 @@ import { cn } from '~/utils';
const HUNK_HEADER = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/;
/** The separator pair `formatEditPreview` writes ahead of each batched edit,
* numbered once a batch holds more than one. */
const EDIT_SECTION_MARKER = /^(?:---|\+\+\+) (?:old_text|new_text)(?: \d+)?$/;
export interface DiffLine {
type: 'add' | 'del' | 'context' | 'hunk';
text: string;
@ -33,6 +37,15 @@ export function parseUnifiedDiff(diff: string): ParsedDiff {
let inHunk = false;
for (const raw of diff.replace(/\n$/, '').split('\n')) {
/** `formatEditPreview` emits a `--- old_text N` / `+++ new_text N` pair
* before EACH batched edit's `@@`. Gating on `!inHunk` alone recognized
* only the first pair, then counted every later one as a deletion plus an
* addition — inflating the statistics and rendering marker rows. Matching
* the synthetic pair by name catches every batch while leaving real
* content lines that merely begin with `-`/`+` alone. */
if (EDIT_SECTION_MARKER.test(raw)) {
continue;
}
if (!inHunk && (raw.startsWith('--- ') || raw.startsWith('+++ '))) {
continue;
}

View file

@ -39,4 +39,29 @@ describe('parseUnifiedDiff', () => {
expect(parsed.additions).toBe(1);
expect(parsed.deletions).toBe(1);
});
it('skips the separator pair before every batched edit', () => {
const parsed = parseUnifiedDiff(
[
'--- old_text 1',
'+++ new_text 1',
'@@',
'-first old',
'+first new',
'',
'--- old_text 2',
'+++ new_text 2',
'@@',
'-second old',
'+second new',
].join('\n'),
);
/** The second edit's synthetic pair used to be counted as a real deletion
* and addition, inflating both statistics and rendering marker rows. */
expect(parsed.deletions).toBe(2);
expect(parsed.additions).toBe(2);
expect(parsed.lines.some((line) => line.text.includes('old_text'))).toBe(false);
expect(parsed.lines.some((line) => line.text.includes('new_text'))).toBe(false);
});
});

View file

@ -203,6 +203,13 @@ export default function WebSearch({
}
return undefined;
}, [attachments]);
/** `snippet` and `snippetHighlighted` are independently optional, and
* `hasDetails` opens the card for ANY answer box, so a provider result
* carrying only highlights rendered a heading with an empty body. */
const answerText = useMemo(
() => answerBox?.snippet || answerBox?.snippetHighlighted?.join(' ') || undefined,
[answerBox],
);
// Show favicons from the raw SERP results immediately rather than waiting for
// each source to flip to `processed`; the agents scrape barrier would otherwise
@ -346,16 +353,16 @@ export default function WebSearch({
{ count: sourceCount },
)}
</div>
{answerBox && (answerBox.title || answerBox.snippet) && (
{answerBox && (answerBox.title || answerText) && (
<div className="border-t border-border-light pt-2">
{answerBox.title && (
<div className="text-sm font-medium text-text-primary">
{answerBox.title}
</div>
)}
{answerBox.snippet && (
{answerText && (
<div className="mt-1 text-xs leading-relaxed text-text-secondary">
{answerBox.snippet}
{answerText}
</div>
)}
</div>