mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 07:12:00 +01:00
fix(ui): keep another company's prefix on task links in markdown (#21026)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Chat replies, comments and documents render Markdown through
`MarkdownBody`. Task links in that Markdown become company-aware links
> - `parseIssueReferenceFromHref` turned every task link into an
unprefixed `/issues/<id>` path. The router then added the viewer's
active company prefix
> - In chat-eval cohort 4 (`access.other-company`), a reply in company
COM linked `/BIR/issues/BIR-1`. The chat rendered it as
`/COM/issues/BIR-1`, which points into the wrong company
> - This pull request keeps the company that a task link names: an
explicit `/BIR/issues/...` prefix, or an identifier whose prefix belongs
to a known company
> - The benefit is that a link to another company's task opens that
company's task, not a same-numbered task or an error in the current
company
## Linked Issues or Issue Description
**What happened?**
A chat reply in company COM contained `[BIR-1](/BIR/issues/BIR-1)`. The
rendered link pointed to `/COM/issues/BIR-1`.
**Expected behavior**
A task link that names a company keeps that company: `/BIR/issues/BIR-1`
stays `/BIR/issues/BIR-1`. A bare `BIR-1` or `/issues/BIR-1` goes to BIR
when BIR is a company that the viewer knows.
**Steps to reproduce**
1. Have two companies, COM and BIR.
2. In a COM chat or comment, render the Markdown
`[BIR-1](/BIR/issues/BIR-1)`.
3. Inspect the link. Its `href` is `/COM/issues/BIR-1`.
**Paperclip version or commit**
master at f1aef0cb58.
## What Changed
- `ui/src/lib/issue-reference.ts`: a new `issueHref` builds the internal
link. It keeps the company prefix of an explicit `/PREFIX/issues/...`
path. When the path has no prefix, it uses the identifier's prefix if
that prefix is in the known company prefixes. Otherwise the link stays
`/issues/<id>` and resolves in the active company, as before.
- `ui/src/components/MarkdownBody.tsx`: passes the known company
prefixes to the explicit-link path too (the auto-link path already had
them).
- Tests: a red/green unit test with the recorded shape in
`issue-reference.test.ts`, and a rendered test in
`MarkdownBody.test.tsx` (COM and BIR known; explicit, bare and
unprefixed BIR links all render `/BIR/issues/BIR-1`). Existing
expectations that asserted the stripped `/issues/<id>` form now assert
the company-prefixed form.
## Verification
- `cd ui && npx vitest run src/components/MarkdownBody.test.tsx
src/lib/issue-reference.test.ts src/lib/company-routes.test.ts`: 105
tests pass.
- Red/green: with `issue-reference.ts` and `MarkdownBody.tsx` from
`origin/master`, the new tests fail (`/issues/BIR-1` instead of
`/BIR/issues/BIR-1`). With the change, they pass.
- `pnpm check:token-gates` is clean. `tsc -p ui/tsconfig.json` reports
only existing errors in
`ui/storybook/support/feature-map/FeatureOverview.tsx`, which this
change does not touch.
- A paid chat-eval run with this change is linked in a comment below.
## Risks
- Same-company links now carry the explicit prefix (`/COM/issues/COM-1`
instead of `/issues/COM-1` resolved to the same place). The route is the
same.
- An explicit link with a bare identifier as its target (`[x](JIRA-2)`)
is now gated by known company prefixes like auto-linked text. Such a
link to a non-Paperclip key stays an ordinary link.
- Low risk otherwise: no API, data or saved state changes.
## Model Used
- Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, with tool use and code
execution in Claude Code.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
e879a24293
commit
68b7852cd1
10 files changed
+114
-22
No files matched your search
@@ -734,7 +734,7 @@ describe("IssueThreadInteractionCard", () => {
|
||||
interaction.payload.prompt = "Review the [two screenshots](/PAP/issues/PAP-42#document-preview).\n\nSelect **Accept** to keep this design.\n\n- Check `preview.png`\n- Keep the layout\n\n\n\n```mermaid\nflowchart LR\n A --> B\n```";
|
||||
const host = renderCard({ interaction, presentationVariant });
|
||||
|
||||
const link = host.querySelector('a[href="/issues/PAP-42#document-preview"]');
|
||||
const link = host.querySelector('a[href="/PAP/issues/PAP-42#document-preview"]');
|
||||
expect(link?.textContent).toBe("two screenshots");
|
||||
const prompt = link?.closest(".paperclip-markdown");
|
||||
expect(prompt?.querySelector("strong")?.textContent).toBe("Accept");
|
||||
|
||||
@@ -132,7 +132,7 @@ describe("MarkdownBody", () => {
|
||||
{ identifier: "PAP-1271", status: "done" },
|
||||
{ identifier: "PAP-1271", status: "done", cacheKey: "PAP-1271#document-output" },
|
||||
], { linkIssueReferences: true });
|
||||
expect(html).toContain('href="/issues/PAP-1271#document-output"');
|
||||
expect(html).toContain('href="/PAP/issues/PAP-1271#document-output"');
|
||||
expect(html).not.toContain("PAP-1271%23document-output");
|
||||
});
|
||||
|
||||
@@ -422,7 +422,7 @@ describe("MarkdownBody", () => {
|
||||
]);
|
||||
|
||||
expect(html).toContain('href="/issues/PAP-1179"');
|
||||
expect(html).toContain('href="/issues/PAP-1180"');
|
||||
expect(html).toContain('href="/PAP/issues/PAP-1180"');
|
||||
expect(html).toContain(">/issues/PAP-1179<");
|
||||
expect(html).toContain(">/PAP/issues/pap-1180<");
|
||||
expect(html).toContain("var(--status-task-icon-blocked)");
|
||||
@@ -507,7 +507,7 @@ describe("MarkdownBody", () => {
|
||||
);
|
||||
|
||||
expect(html).not.toContain("data-workspace-file-link");
|
||||
expect(html).toContain('href="/issues/PAP-10306"');
|
||||
expect(html).toContain('href="/PAP/issues/PAP-10306"');
|
||||
});
|
||||
|
||||
it("promotes an openable auto-detected path to a workspace file chip", () => {
|
||||
@@ -805,6 +805,18 @@ describe("MarkdownBody", () => {
|
||||
expect(html).not.toContain("paperclip-mention-chip--issue");
|
||||
});
|
||||
|
||||
it("keeps another company's task link in that company", () => {
|
||||
mockUseOptionalCompany.mockReturnValue({ companies: [{ issuePrefix: "COM" }, { issuePrefix: "BIR" }] });
|
||||
const html = renderMarkdown("I can't read [BIR-1](/BIR/issues/BIR-1); BIR-1 and [this link](/issues/BIR-1) stay in Birch.", [
|
||||
{ identifier: "BIR-1", status: "todo" },
|
||||
]);
|
||||
expect(html.match(/href="[^"]*"/g)).toEqual([
|
||||
'href="/BIR/issues/BIR-1"',
|
||||
'href="/BIR/issues/BIR-1"',
|
||||
'href="/BIR/issues/BIR-1"',
|
||||
]);
|
||||
});
|
||||
|
||||
it("gates bare-identifier auto-linking to known company prefixes", () => {
|
||||
mockUseOptionalCompany.mockReturnValue({ companies: [{ issuePrefix: "PAP" }] });
|
||||
|
||||
@@ -814,7 +826,7 @@ describe("MarkdownBody", () => {
|
||||
]);
|
||||
|
||||
// Known prefix links; foreign tracker key stays as plain text.
|
||||
expect(html).toContain('href="/issues/PAP-1271"');
|
||||
expect(html).toContain('href="/PAP/issues/PAP-1271"');
|
||||
expect(html).not.toContain('href="/issues/JIRA-2"');
|
||||
expect(html).toContain("blocked by JIRA-2.");
|
||||
});
|
||||
@@ -857,7 +869,7 @@ describe("MarkdownBody", () => {
|
||||
{ identifier: "ACME-1", status: "done" },
|
||||
]);
|
||||
|
||||
expect(html).toContain('href="/issues/ACME-1"');
|
||||
expect(html).toContain('href="/ACME/issues/ACME-1"');
|
||||
});
|
||||
|
||||
});
|
||||
@@ -792,6 +792,10 @@ function MarkdownBodyImpl({
|
||||
() => (companies?.length ? companies.map((c) => c.issuePrefix) : undefined),
|
||||
[companies],
|
||||
);
|
||||
const knownPrefixSet = useMemo(
|
||||
() => (knownPrefixes ? new Set(knownPrefixes.map((prefix) => prefix.toUpperCase())) : undefined),
|
||||
[knownPrefixes],
|
||||
);
|
||||
const externalReferenceLookup = useMemo<MarkdownExternalReferenceMap | null>(() => {
|
||||
if (!externalReferences) return null;
|
||||
const lookup: MarkdownExternalReferenceMap = {};
|
||||
@@ -900,7 +904,7 @@ function MarkdownBodyImpl({
|
||||
);
|
||||
}
|
||||
|
||||
const issueRef = linkIssueReferences ? parseIssueReferenceFromHref(href) : null;
|
||||
const issueRef = linkIssueReferences ? parseIssueReferenceFromHref(href, knownPrefixSet) : null;
|
||||
const documentHash = href?.includes("#") ? href.slice(href.indexOf("#")) : null;
|
||||
const documentLink = Boolean(coordinator && documentHash && parseDocumentAnnotationHash(documentHash));
|
||||
if (documentLink && href?.startsWith("#")) {
|
||||
@@ -1017,7 +1021,7 @@ function MarkdownBodyImpl({
|
||||
};
|
||||
}
|
||||
return map;
|
||||
}, [theme, coordinator, linkIssueReferences, linkCaseReferences, externalReferenceLookup, resolveImageSrc, resolveLinkHref, onImageClick, mediaMode, knownPrefixes]);
|
||||
}, [theme, coordinator, linkIssueReferences, linkCaseReferences, externalReferenceLookup, resolveImageSrc, resolveLinkHref, onImageClick, mediaMode, knownPrefixes, knownPrefixSet]);
|
||||
|
||||
return (
|
||||
<div
|
||||
|
||||
@@ -53,7 +53,7 @@ describe("MarkdownBody external object decoration", () => {
|
||||
const markdown = "[View saved recommendations](/TES/issues/TES-6#document-skill-recommendations&revision=reviewed-rev)";
|
||||
const html = render(markdown, undefined, true);
|
||||
expect(html).toContain('data-sidebar-document-link="true"');
|
||||
expect(html).toContain('href="/issues/TES-6#document-skill-recommendations&revision=reviewed-rev"');
|
||||
expect(html).toContain('href="/TES/issues/TES-6#document-skill-recommendations&revision=reviewed-rev"');
|
||||
expect(html).toContain("View saved recommendations");
|
||||
expect(html).not.toContain('data-mention-kind="issue"');
|
||||
expect(render(markdown)).not.toContain("data-sidebar-document-link");
|
||||
|
||||
@@ -520,7 +520,7 @@ describe("TaskChatInteractionCard", () => {
|
||||
</ThemeProvider></TooltipProvider></QueryClientProvider>,
|
||||
));
|
||||
|
||||
const link = container.querySelector('a[href="/issues/PAP-42#document-preview"]');
|
||||
const link = container.querySelector('a[href="/PAP/issues/PAP-42#document-preview"]');
|
||||
expect(link?.textContent).toBe("two screenshots");
|
||||
const prompt = link?.closest(".paperclip-markdown");
|
||||
expect(prompt?.querySelector("strong")?.textContent).toBe("Accept");
|
||||
|
||||
@@ -12,6 +12,13 @@ describe("company routes", () => {
|
||||
expect(extractCompanyPrefixFromPath(path)).toBeNull();
|
||||
expect(applyCompanyPrefix(path, "DOT")).toBe(path);
|
||||
});
|
||||
it("keeps a company task path whose prefix matches a board route name", () => {
|
||||
expect(extractCompanyPrefixFromPath("/ORG/issues/ORG-1")).toBe("ORG");
|
||||
expect(applyCompanyPrefix("/ORG/issues/ORG-1", "ORG")).toBe("/ORG/issues/ORG-1");
|
||||
expect(applyCompanyPrefix("/ORG/issues/ORG-1", "COM")).toBe("/ORG/issues/ORG-1");
|
||||
expect(applyCompanyPrefix("/org", "ORG")).toBe("/ORG/org");
|
||||
expect(applyCompanyPrefix("/issues/ORG-1", "ORG")).toBe("/ORG/issues/ORG-1");
|
||||
});
|
||||
it("treats the task-list alias as an unprefixed board route", () => {
|
||||
expect(isBoardPathWithoutPrefix("/tasks")).toBe(true);
|
||||
expect(extractCompanyPrefixFromPath("/tasks")).toBeNull();
|
||||
|
||||
@@ -70,9 +70,9 @@ export function extractCompanyPrefixFromPath(pathname: string): string | null {
|
||||
const segments = pathname.split("/").filter(Boolean);
|
||||
if (segments.length === 0) return null;
|
||||
const first = segments[0]!.toLowerCase();
|
||||
if (GLOBAL_ROUTE_ROOTS.has(first) || BOARD_ROUTE_ROOTS.has(first)) {
|
||||
return null;
|
||||
}
|
||||
if (GLOBAL_ROUTE_ROOTS.has(first)) return null;
|
||||
const companyTaskPath = first !== "issues" && segments[1]?.toLowerCase() === "issues";
|
||||
if (BOARD_ROUTE_ROOTS.has(first) && !companyTaskPath) return null;
|
||||
return normalizeCompanyPrefix(segments[0]!);
|
||||
}
|
||||
|
||||
|
||||
@@ -25,7 +25,7 @@ describe("issue-reference", () => {
|
||||
for (const suffix of ["#document-output", "#comment-123", "?view=chat#document-output"]) {
|
||||
expect(parseIssuePathIdFromPath(`/PAP/issues/pap-1271${suffix}`)).toBe("PAP-1271");
|
||||
expect(parseIssueReferenceFromHref(`/PAP/issues/pap-1271${suffix}`)).toEqual({
|
||||
issuePathId: "PAP-1271", href: `/issues/PAP-1271${suffix}`,
|
||||
issuePathId: "PAP-1271", href: `/PAP/issues/PAP-1271${suffix}`,
|
||||
});
|
||||
expect(parseIssueReferenceFromHref(`issue://PAP-1271${suffix}`)).toEqual({
|
||||
issuePathId: "PAP-1271", href: `/issues/PAP-1271${suffix}`,
|
||||
@@ -33,6 +33,20 @@ describe("issue-reference", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps another company's prefix instead of resolving the link into the viewer's company", () => {
|
||||
expect(parseIssueReferenceFromHref("/BIR/issues/BIR-1")).toEqual({
|
||||
issuePathId: "BIR-1", href: "/BIR/issues/BIR-1",
|
||||
});
|
||||
expect(parseIssueReferenceFromHref("/BIR/issues/bir-1#document-plan")).toEqual({
|
||||
issuePathId: "BIR-1", href: "/BIR/issues/BIR-1#document-plan",
|
||||
});
|
||||
const known = new Set(["COM", "BIR"]);
|
||||
expect(parseIssueReferenceFromHref("/issues/BIR-1", known)?.href).toBe("/BIR/issues/BIR-1");
|
||||
expect(parseIssueReferenceFromHref("BIR-1", known)?.href).toBe("/BIR/issues/BIR-1");
|
||||
expect(parseIssueReferenceFromHref("issue://BIR-1", known)?.href).toBe("/BIR/issues/BIR-1");
|
||||
expect(parseIssueReferenceFromHref("/issues/BIR-1")?.href).toBe("/issues/BIR-1");
|
||||
});
|
||||
|
||||
it("does not treat full issue URLs as internal issue paths", () => {
|
||||
expect(parseIssuePathIdFromPath("http://localhost:3100/PAP/issues/PAP-1179")).toBeNull();
|
||||
expect(parseIssuePathIdFromPath("http://remote.example.test:3103/PAPA/issues/PAPA-115#comment-850083f3-24de-43e7-a8cd-bc01f7cc9f0d")).toBeNull();
|
||||
@@ -60,7 +74,7 @@ describe("issue-reference", () => {
|
||||
});
|
||||
expect(parseIssueReferenceFromHref("/PAP/issues/pap-1180")).toEqual({
|
||||
issuePathId: "PAP-1180",
|
||||
href: "/issues/PAP-1180",
|
||||
href: "/PAP/issues/PAP-1180",
|
||||
});
|
||||
expect(parseIssueReferenceFromHref("issue://PAP-1310")).toEqual({
|
||||
issuePathId: "PAP-1310",
|
||||
@@ -93,14 +107,14 @@ describe("issue-reference", () => {
|
||||
it("links a bare identifier whose prefix is known", () => {
|
||||
expect(parseIssueReferenceFromHref("PAP-1271", new Set(["PAP"]))).toEqual({
|
||||
issuePathId: "PAP-1271",
|
||||
href: "/issues/PAP-1271",
|
||||
href: "/PAP/issues/PAP-1271",
|
||||
});
|
||||
});
|
||||
|
||||
it("matches the prefix case-insensitively", () => {
|
||||
expect(parseIssueReferenceFromHref("pap-12", new Set(["PAP"]))).toEqual({
|
||||
issuePathId: "PAP-12",
|
||||
href: "/issues/PAP-12",
|
||||
href: "/PAP/issues/PAP-12",
|
||||
});
|
||||
});
|
||||
|
||||
@@ -132,7 +146,7 @@ describe("issue-reference", () => {
|
||||
it("never gates explicit /issues/ path references", () => {
|
||||
expect(parseIssueReferenceFromHref("/ACME/issues/ACME-9", new Set(["PAP"]))).toEqual({
|
||||
issuePathId: "ACME-9",
|
||||
href: "/issues/ACME-9",
|
||||
href: "/ACME/issues/ACME-9",
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -145,7 +159,7 @@ describe("issue-reference", () => {
|
||||
const children = paragraphChildren(tree);
|
||||
expect(children).toEqual([
|
||||
{ type: "text", value: "See " },
|
||||
{ type: "link", url: "/issues/PAP-1", children: [{ type: "text", value: "PAP-1" }] },
|
||||
{ type: "link", url: "/PAP/issues/PAP-1", children: [{ type: "text", value: "PAP-1" }] },
|
||||
{ type: "text", value: " and JIRA-2 today." },
|
||||
]);
|
||||
});
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { extractCompanyPrefixFromPath } from "./company-routes.js";
|
||||
type MarkdownNode = {
|
||||
type: string;
|
||||
value?: string;
|
||||
@@ -35,7 +36,7 @@ export function parseIssueReferenceFromHref(
|
||||
const issuePathId = decodeURIComponent(issueSchemeMatch[1]);
|
||||
return {
|
||||
issuePathId,
|
||||
href: `/issues/${encodeURIComponent(issuePathId)}${issueSchemeMatch[2] ?? ""}`,
|
||||
href: issueHref(issuePathId, issueSchemeMatch[2] ?? "", null, knownPrefixes),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -43,7 +44,12 @@ export function parseIssueReferenceFromHref(
|
||||
if (pathId) {
|
||||
return {
|
||||
issuePathId: pathId,
|
||||
href: `/issues/${encodeURIComponent(pathId)}${trimmed.match(/[?#].*$/)?.[0] ?? ""}`,
|
||||
href: issueHref(
|
||||
pathId,
|
||||
trimmed.match(/[?#].*$/)?.[0] ?? "",
|
||||
extractCompanyPrefixFromPath(trimmed.split(/[?#]/, 1)[0]!),
|
||||
knownPrefixes,
|
||||
),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -60,10 +66,26 @@ export function parseIssueReferenceFromHref(
|
||||
}
|
||||
return {
|
||||
issuePathId: normalized,
|
||||
href: `/issues/${encodeURIComponent(normalized)}`,
|
||||
href: issueHref(normalized, "", null, knownPrefixes),
|
||||
};
|
||||
}
|
||||
|
||||
function issueHref(
|
||||
issuePathId: string,
|
||||
suffix: string,
|
||||
pathCompanyPrefix: string | null,
|
||||
knownPrefixes?: Set<string>,
|
||||
): string {
|
||||
const identifierPrefix = BARE_ISSUE_IDENTIFIER_RE.test(issuePathId)
|
||||
? issuePathId.split("-")[0]!.toUpperCase()
|
||||
: null;
|
||||
const companyPrefix =
|
||||
pathCompanyPrefix ??
|
||||
(identifierPrefix && knownPrefixes?.has(identifierPrefix) ? identifierPrefix : null);
|
||||
const path = `/issues/${encodeURIComponent(issuePathId)}${suffix}`;
|
||||
return companyPrefix ? `/${companyPrefix}${path}` : path;
|
||||
}
|
||||
|
||||
function splitTrailingPunctuation(token: string) {
|
||||
let core = token;
|
||||
let trailing = "";
|
||||
|
||||
@@ -0,0 +1,33 @@
|
||||
import { renderToStaticMarkup } from "react-dom/server";
|
||||
import { MemoryRouter } from "react-router-dom";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { Link } from "./router";
|
||||
|
||||
const company = vi.hoisted(() => ({ issuePrefix: "ORG" }));
|
||||
vi.mock("@/context/CompanyContext", () => ({ useCompany: () => ({ selectedCompany: company }) }));
|
||||
|
||||
function href(location: string, to: string) {
|
||||
const html = renderToStaticMarkup(
|
||||
<MemoryRouter initialEntries={[location]}>
|
||||
<QueryClientProvider client={new QueryClient()}>
|
||||
<Link to={to} disableIssueQuicklook>task</Link>
|
||||
</QueryClientProvider>
|
||||
</MemoryRouter>,
|
||||
);
|
||||
return html.match(/href="([^"]*)"/)?.[1];
|
||||
}
|
||||
|
||||
describe("company task links through the real Link", () => {
|
||||
it("opens a task in a company whose prefix matches a board route name", () => {
|
||||
company.issuePrefix = "ORG";
|
||||
expect(href("/ORG/dashboard", "/ORG/issues/ORG-1")).toBe("/ORG/issues/ORG-1");
|
||||
expect(href("/ORG/dashboard", "/issues/ORG-1")).toBe("/ORG/issues/ORG-1");
|
||||
});
|
||||
|
||||
it("keeps another company's task link in that company", () => {
|
||||
company.issuePrefix = "COM";
|
||||
expect(href("/COM/dashboard", "/BIR/issues/BIR-1")).toBe("/BIR/issues/BIR-1");
|
||||
expect(href("/COM/dashboard", "/ORG/issues/ORG-1")).toBe("/ORG/issues/ORG-1");
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user