Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 191 additions & 0 deletions src/BloomBrowserUI/react_components/stringWithOptionalLink.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import * as ReactDOM from "react-dom";
import { renderToStaticMarkup } from "react-dom/server";
import { act } from "react-dom/test-utils";

vi.mock("../utils/bloomApi", () => ({
post: vi.fn(),
postString: vi.fn(),
}));

import { post, postString } from "../utils/bloomApi";
import { StringWithOptionalLink } from "./stringWithOptionalLink";

const postMock = vi.mocked(post);
const postStringMock = vi.mocked(postString);

describe("StringWithOptionalLink", () => {
let container: HTMLDivElement | null = null;

const renderIntoDom = (message: string) => {
if (!container) {
throw new Error("render container not initialized");
}

const target = container;
act(() => {
ReactDOM.render(
<StringWithOptionalLink message={message} />,
target,
);
});
return target;
};

beforeEach(() => {
container = document.createElement("div");
document.body.appendChild(container);
vi.clearAllMocks();
});

afterEach(() => {
if (container) {
ReactDOM.unmountComponentAtNode(container);
container.remove();
container = null;
}
vi.clearAllMocks();
});

it("renders spans and anchors for multiple links", () => {
const markup = renderToStaticMarkup(
<StringWithOptionalLink
message={
"Start <a href='/bloom/api/internal'>first</a> middle <a href='http://example.com'>second</a> end"
}
/>,
);
const temp = document.createElement("div");
temp.innerHTML = markup;

const spans = temp.querySelectorAll("span");
const anchors = temp.querySelectorAll("a");

expect(spans.length).toBe(3);
expect(spans[0].textContent).toBe("Start ");
expect(spans[1].textContent).toBe(" middle ");
expect(spans[2].textContent).toBe(" end");

expect(anchors.length).toBe(2);
expect(anchors[0].textContent).toBe("first");
expect(anchors[0].getAttribute("href")).toBe("/bloom/api/internal");
expect(anchors[1].textContent).toBe("second");
expect(anchors[1].getAttribute("href")).toBe("http://example.com");
});

it("invokes post for internal links", () => {
const host = renderIntoDom(
"Do <a href='/bloom/api/doThing'>this</a> now",
);
const anchor = host.querySelector("a");
expect(anchor).not.toBeNull();

anchor?.dispatchEvent(
new MouseEvent("click", { bubbles: true, cancelable: true }),
);

expect(postMock).toHaveBeenCalledWith("doThing");
expect(postStringMock).not.toHaveBeenCalled();
});

it("invokes postString for external links", () => {
const host = renderIntoDom(
"Visit <a href='mailto:test@example.com'>email</a>",
);
const anchor = host.querySelector("a");
expect(anchor).not.toBeNull();

anchor?.dispatchEvent(
new MouseEvent("click", { bubbles: true, cancelable: true }),
);

expect(postStringMock).toHaveBeenCalledWith(
"link",
"mailto:test@example.com",
);
expect(postMock).not.toHaveBeenCalled();
});

it("renders a single span for messages with no links", () => {
const markup = renderToStaticMarkup(
<StringWithOptionalLink message={"This is plain text"} />,
);
const temp = document.createElement("div");
temp.innerHTML = markup;

const spans = temp.querySelectorAll("span");
const anchors = temp.querySelectorAll("a");

expect(spans.length).toBe(1);
expect(spans[0].textContent).toBe("This is plain text");
expect(anchors.length).toBe(0);
});

it("handles messages starting with a link", () => {
const markup = renderToStaticMarkup(
<StringWithOptionalLink
message={"<a href='/bloom/api/test'>Link</a> then text"}
/>,
);
const temp = document.createElement("div");
temp.innerHTML = markup;

const anchors = temp.querySelectorAll("a");
const spans = temp.querySelectorAll("span");

expect(anchors.length).toBe(1);
expect(anchors[0].textContent).toBe("Link");
expect(spans.length).toBe(1);
expect(spans[0].textContent).toBe(" then text");
});

it("handles messages ending with a link", () => {
const markup = renderToStaticMarkup(
<StringWithOptionalLink
message={"Text before <a href='/bloom/api/test'>Link</a>"}
/>,
);
const temp = document.createElement("div");
temp.innerHTML = markup;

const spans = temp.querySelectorAll("span");
const anchors = temp.querySelectorAll("a");

expect(spans.length).toBe(1);
expect(spans[0].textContent).toBe("Text before ");
expect(anchors.length).toBe(1);
expect(anchors[0].textContent).toBe("Link");
});

it("handles consecutive links with no text between them", () => {
const markup = renderToStaticMarkup(
<StringWithOptionalLink
message={
"<a href='/bloom/api/first'>First</a><a href='/bloom/api/second'>Second</a>"
}
/>,
);
const temp = document.createElement("div");
temp.innerHTML = markup;

const anchors = temp.querySelectorAll("a");
const spans = temp.querySelectorAll("span");

expect(anchors.length).toBe(2);
expect(anchors[0].textContent).toBe("First");
expect(anchors[1].textContent).toBe("Second");
expect(spans.length).toBe(0);
});

it("renders a single span for empty string", () => {
const markup = renderToStaticMarkup(
<StringWithOptionalLink message={""} />,
);
const temp = document.createElement("div");
temp.innerHTML = markup;

const spans = temp.querySelectorAll("span");
expect(spans.length).toBe(1);
expect(spans[0].textContent).toBe("");
});
});
Comment on lines +50 to +191
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test suite is missing coverage for several important edge cases: (1) messages with no links at all (should render a single span), (2) messages starting or ending with a link (no preceding/trailing text), (3) consecutive links with no text between them, and (4) messages with empty string. Adding these test cases would improve confidence in the component's robustness.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

83 changes: 57 additions & 26 deletions src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx
Original file line number Diff line number Diff line change
@@ -1,37 +1,68 @@
import * as React from "react";
import { post } from "../utils/bloomApi";
import { post, postString } from "../utils/bloomApi";

// Display a string which may have embedded a link, in the usual HTML format,
// for example, "There is a problem. Please click <a href='...'>here</a> to report it".
// (Currently the href must use single quotes.)
// If there is no link, the result is a single span, the message.
// If there is a link, we get a span, a link, and another span.
// Currently the href is assumed to be something to send to our API,
// but NOT to actually navigate to. We could support more options as needed.
// If there are links, we interleave spans and anchor tags for each segment.
// For internal links, the href is treated as something to send to our API rather than
// something for the browser to navigate to directly. External http/mailto hrefs are
// sent to the 'link' API endpoint, which opens them in an external browser.
export const StringWithOptionalLink: React.FunctionComponent<{
message: string;
}> = (props) => {
const match = props.message.match(
/^(.*?)<a[^>]*?href='([^>']+)'[^>]*>(.*?)<\/a>(.*)$/,
);
if (match) {
const href = match[2].replace("/bloom/api/", "");
return (
<React.Fragment>
<span>{match[1]}</span>
<a
// We don't currently use the href, but to get link formatting it
// has to be present. May also be helpful for accessibility.
href={match[2]}
onClick={(e) => {
e.preventDefault(); // so it doesn't try to follow the link
post(href);
}}
>
{match[3]}
</a>
<span>{match[4]}</span>
</React.Fragment>
// Create a fresh regex for each render to avoid lastIndex issues
const linkRegex = /<a[^>]*?href='([^>']+)'[^>]*>(.*?)<\/a>/g;
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex with the 'g' flag is being used in a while loop with exec(), which mutates the regex's lastIndex property. This is correct for the current implementation, but if the component re-renders with the same props (e.g., due to parent re-render), the regex needs to be reset. Consider moving the regex creation inside the component function (after line 13) to ensure it's fresh for each render, or explicitly resetting lastIndex to 0 before the loop.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const elements: React.ReactNode[] = [];
let lastIndex = 0;
let segmentIndex = 0;
let match: RegExpExecArray | null;

while ((match = linkRegex.exec(props.message))) {
const precedingText = props.message.slice(lastIndex, match.index);
if (precedingText) {
elements.push(
<span key={`text-${segmentIndex}`}>{precedingText}</span>,
);
segmentIndex++;
}

const rawHref = match[1];
const isExternalLink =
rawHref.startsWith("http") || rawHref.startsWith("mailto");
Comment on lines +32 to +33
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The detection of external links only checks for 'http' and 'mailto' prefixes. This will incorrectly classify 'https' URLs as external (which is correct) but will miss other protocols like 'ftp', 'tel', etc. More importantly, it won't handle URLs that start with '//' (protocol-relative URLs). Consider using a more comprehensive check, such as checking if the URL starts with 'http://', 'https://', 'mailto:', or '//' to be more precise.

Suggested change
const isExternalLink =
rawHref.startsWith("http") || rawHref.startsWith("mailto");
const lowerHref = rawHref.toLowerCase();
const isExternalLink =
lowerHref.startsWith("http://") ||
lowerHref.startsWith("https://") ||
lowerHref.startsWith("mailto:") ||
rawHref.startsWith("//");

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed.

const href = rawHref.replace("/bloom/api/", "");

elements.push(
<a
key={`link-${segmentIndex}`}
// We don't currently use the href, but to get link formatting it
// has to be present. May also be helpful for accessibility.
href={rawHref}
onClick={(e) => {
e.preventDefault(); // so it doesn't try to follow the link
if (isExternalLink) {
postString("link", rawHref);
return;
}
post(href);
}}
>
{match[2]}
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link text (match[2]) is rendered directly as JSX children without escaping. While React does escape text content by default when rendering strings in JSX, if the original message contains HTML entities or special characters in the link text, they will be rendered as-is. This is generally safe because React escapes by default, but it's worth noting that any HTML in the link text portion of the original message will be displayed as text rather than rendered as HTML. This might be intentional, but should be documented if so.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change needed.

</a>,
Comment on lines +37 to +52
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The anchor elements generated for links should have a more descriptive href attribute for better accessibility. Currently, internal API links use href values like "/bloom/api/doThing" which may not be meaningful to screen readers. Consider adding an aria-label attribute or ensuring the link text is descriptive enough for users who rely on assistive technology.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed.

);
} else return <span>{props.message}</span>;
segmentIndex++;
lastIndex = linkRegex.lastIndex;
}

const trailingText = props.message.slice(lastIndex);
if (trailingText) {
elements.push(<span key={`text-${segmentIndex}`}>{trailingText}</span>);
}

if (elements.length === 0) {
return <span>{props.message}</span>;
}

return <React.Fragment>{elements}</React.Fragment>;
};
Loading