Skip to content

[Bug]: PR comment hover excerpts can end in a replacement character #35082

Closed

Description

@coygeek

Before submitting

  • I searched existing issues and did not find a duplicate.
  • I included enough detail to reproduce or investigate the problem.

Area

apps/web

Steps to reproduce

Use the commit under Version or commit with dependencies and the repo's Playwright Chromium installed. Save the probe below as apps/web/src/components/pullRequest/verifySurrogate.browser.test.tsx and, from apps/web, run env -u NODE_ENV vp test run --silent=false --project browser src/components/pullRequest/verifySurrogate.browser.test.tsx. It renders the real PullRequestLinkPreview with production CSS and hover-card primitives (only the PR state and query hooks return fixture data), hovers a link to …/pull/1#issuecomment-5, reads the excerpt paragraph, and compares the browser's raster of the trailing glyph with an explicit U+FFFD. A pass means the defect is present and the control is correct.

  • Failure: a linked issue-comment body of "i ".repeat(139) + "😀bb", 282 UTF-16 code units. The emoji occupies units 278-279, across the 279-unit cut.
  • Control: "i ".repeat(139) + "xbbb", also 282 units.
  • A third case, "a".repeat(278) + "😀bb", produces the same broken DOM suffix, but the unbroken word overflows horizontally and hides it.

Hovering a bare PR URL does not use this comment-excerpt path. The probe is the one that produced the output under Logs or stack traces, minus two screenshot-capture lines.

Probe: verifySurrogate.browser.test.tsx
import { EnvironmentId, ProjectId } from "@t3tools/contracts";
import { expect, it, vi } from "vite-plus/test";
import { page, userEvent } from "vite-plus/test/browser";
import "~/index.css";

const state = vi.hoisted(() => ({ activity: null as unknown, preview: null as unknown }));
vi.mock(import("~/state/pullRequests"), () => ({
  pullRequestEnvironment: { previewCard: () => "card", activity: () => "activity", preview: {} },
  useSharedPullRequestSummary: () => null,
}));
vi.mock(import("~/state/query"), () => ({
  useEnvironmentQuery: (atom: unknown) => ({
    data: atom === "card" ? state.preview : atom === "activity" ? state.activity : null,
    error: null,
  }),
}));
vi.mock(import("~/state/use-atom-query-runner"), () => ({ useAtomQueryRunner: () => vi.fn() }));

import { render } from "../../../test/render";
import { PullRequestLinkPreview } from "./PullRequestLinkPreview";
import { pullRequestLinkSubject } from "./pullRequestDetail.logic";

const url = "https://github.com/owner/repo/pull/1#issuecomment-5";
function setup(body: string) {
  state.preview = {
    provider: "github", projectId: "project", repository: "owner/repo", number: 1,
    title: "Example pull request", url: url.split("#")[0], state: "open", isDraft: false,
    author: { login: "octocat", name: null, avatarUrl: null }, createdAt: "2000-01-01T00:00:00Z",
  };
  state.activity = {
    comments: [{ id: "c1", kind: "issue-comment", author: { login: "octocat", name: null, avatarUrl: null },
      body, createdAt: "2000-01-01T00:00:00Z", url, path: null, reviewState: null }],
    reviewThreads: [], commits: [],
  };
}
const cases = [
  { name: "FAILURE", prefix: "i ".repeat(139), tail: "😀bb", expected: "i ".repeat(139) + "\ud83d…" },
  { name: "CONTROL", prefix: "i ".repeat(139), tail: "xbbb", expected: "i ".repeat(139) + "x…" },
  { name: "DRAFT_ASCII_A", prefix: "a".repeat(278), tail: "😀bb", expected: "a".repeat(278) + "\ud83d…" },
];
for (const item of cases) {
  it(`renders ${item.name} through the real PR link hover card`, async () => {
    const body = item.prefix + item.tail;
    setup(body);
    const subject = pullRequestLinkSubject(url, state.activity as Parameters<typeof pullRequestLinkSubject>[1]);
    expect(subject?.kind).toBe("comment");
    expect(subject && "excerpt" in subject ? subject.excerpt : null).toBe(item.expected);
    await using screen = await render(
      <div style={{ padding: 220 }}>
        <PullRequestLinkPreview link={<a href={url}>PR comment</a>} originalUrl={url}
          target={{ environmentId: EnvironmentId.make("environment"),
            input: { projectId: ProjectId.make("project"), repository: "owner/repo", number: 1 } }} />
      </div>,
    );
    await userEvent.hover(screen.getByRole("link", { name: "PR comment" }));
    const excerpt = page.getByText(item.expected, { exact: true });
    await expect.element(excerpt).toBeVisible();
    await document.fonts.ready;
    const element = excerpt.element();
    const text = element.textContent!;
    expect(text).toBe(item.expected);
    const node = element.firstChild!;
    const range = document.createRange();
    range.setStart(node, text.length - 2);
    range.setEnd(node, text.length - 1);
    const glyph = range.getBoundingClientRect();
    const box = element.getBoundingClientRect();
    const style = getComputedStyle(element);
    console.log(`${item.name}_INPUT_CODE_UNITS=${body.length}`);
    console.log(`${item.name}_DOM_EXCERPT=${JSON.stringify(text)}`);
    console.log(`${item.name}_DOM_TAIL=${JSON.stringify(text.slice(-6))}`);
    console.log(`${item.name}_TAIL_CODE_UNITS=${Array.from({length: 3}, (_, i) => text.charCodeAt(text.length - 3 + i).toString(16)).join(",")}`);
    console.log(`${item.name}_LAYOUT=${JSON.stringify({width: box.width,height: box.height,scrollHeight: element.scrollHeight,lineClamp: style.webkitLineClamp,font: style.font,glyphTop: glyph.top-box.top,glyphBottom: glyph.bottom-box.top,glyphWidth: glyph.width,glyphLeft: glyph.left-box.left,glyphRight: glyph.right-box.left})}`);
    // Compare the browser's own glyph raster to explicit U+FFFD and to dropping it.
    const canvas = document.createElement("canvas"); canvas.width = 50; canvas.height = 30;
    const context = canvas.getContext("2d")!;
    function raster(value: string) {
      context.clearRect(0,0,50,30); context.font = style.font; context.fillText(value,0,20);
      return Array.from(context.getImageData(0,0,50,30).data).join(",");
    }
    const actualGlyph = text.at(-2)!;
    const replacementMatch = raster(actualGlyph) === raster("�");
    const emptyMatch = raster(actualGlyph) === raster("");
    console.log(`${item.name}_GLYPH_EQUALS_REPLACEMENT=${replacementMatch}`);
    console.log(`${item.name}_GLYPH_EQUALS_EMPTY=${emptyMatch}`);
    if (item.name !== "CONTROL") { expect(replacementMatch).toBe(true); expect(emptyMatch).toBe(false); }
    else { expect(replacementMatch).toBe(false); expect(emptyMatch).toBe(false); }
    if (item.name !== "DRAFT_ASCII_A") { expect(glyph.bottom).toBeLessThanOrEqual(box.bottom); expect(glyph.right).toBeLessThanOrEqual(box.right); }
    console.log(`BROWSER=${navigator.userAgent}`);
  });
}

Expected behavior

The hover card shows a short preview of the linked comment's words (user docs, existing comment-subject test). For this input the preview should end with the text before the emoji and …, with no replacement glyph. The project already keeps surrogate pairs whole at other preview cuts: closed #27842 and merged #27841 for the shared truncate(), and merged #30392 for the same module's bounded(). This excerpt was added later, in #33989, so those fixes did not cover it.

Actual behavior

Summary. When a pull request link names a comment, its hover card shows a short excerpt of that comment. The excerpt is cut at a fixed UTF-16 code-unit length, so an emoji (or any astral character) that straddles the cut leaves a lone high surrogate before the …, and the card shows �…. Only the hover preview is affected; the stored comment body is not changed.

Observed in Chromium through the real hover card: the last line ends i �…. The paragraph's DOM text ends with code units 20,d83d,2026 (space, lone high surrogate, ellipsis), and the trailing glyph's raster equals an explicit U+FFFD and differs from empty text. The control ends i x…. The run passed (1 file, 3 tests); output is under Logs or stack traces and screenshots under Screenshots.

Path: PullRequestLinkPreview calls pullRequestLinkSubject, which passes the matched comment body to markdownExcerpt; line 735 returns text.slice(0, LINK_EXCERPT_LENGTH - 1) + "…", and LinkedComment renders it directly. This is a component browser test; a full app session, Electron, mobile and other browsers were not run.

Related (not reproduced)

The same fixed-length cut without a surrogate check also appears in readableFailure() in the same file (line 1410), oneLine() in apps/server/src/pullRequest/pullRequestReviewGuideInput.ts:11 (settled review remarks in the review-guide input; its sibling snippet() in apps/server/src/orchestration-v2/pullRequestWatch.ts:219-221 already backs off a high surrogate), and apps/server/src/preview/ServerBrowser.ts at lines 564-565 (dialog text), 635 (page title) and 1071 (console text); closed, unmerged #33703 addressed the dialog cut. They are listed only so one fix can cover them together; no impact is claimed for them here.

Evidence

Restoration check

With the probe's failure input, the hover excerpt ends with whole characters (no lone surrogate, no U+FFFD glyph) followed by …, while the ASCII control still shows i x… and short comments still show untruncated. The probe's expected strings and raster assertions need inverting for this check.

Impact

Cosmetic issue

Version or commit

d9805b2 (the excerpt module and hover-card component are byte-identical on main at ee90bdc)

Environment

macOS 27.2 arm64, Node v26.12.1, Vite+ test runner v5.0.5, Playwright Chromium (HeadlessChrome/156.0.8078.4)

Logs or stack traces

FAILURE_INPUT_CODE_UNITS=282
FAILURE_DOM_TAIL="i i \ud83d…"
FAILURE_TAIL_CODE_UNITS=20,d83d,2026
FAILURE_GLYPH_EQUALS_REPLACEMENT=true
FAILURE_GLYPH_EQUALS_EMPTY=false
CONTROL_INPUT_CODE_UNITS=282
CONTROL_DOM_TAIL="i i x…"
CONTROL_TAIL_CODE_UNITS=20,78,2026
CONTROL_GLYPH_EQUALS_REPLACEMENT=false
CONTROL_GLYPH_EQUALS_EMPTY=false
Test Files  1 passed (1)
Tests       3 passed (3)

Screenshots, recordings, or supporting files

Failure input (last line ends i �…):

Hover card ending in a replacement glyph before the ellipsis

Control input (last line ends i x…):

Hover card ending in x before the ellipsis

Workaround

The full comment stays readable by following the link; the excerpt is derived on render and does not change the comment body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions