mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 11:01:16 +01:00
fix(files): only highlight accessible workspace file links (#11090)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Task comments can contain references to files in project and execution workspaces. > - Paperclip detected path-shaped inline code and showed it as an actionable file chip. > - The UI did not first confirm that the current board session could open the file. > - Missing, denied, ambiguous, remote, and unsupported files therefore looked actionable and failed after a click. > - This pull request adds an issue-scoped availability check and promotes only confirmed files to chips. > - The benefit is that the task thread shows a file action only when that action can succeed. ## Linked Issues or Issue Description **What happened?** Task comments promoted path-shaped inline code to file chips before Paperclip checked the file. A chip could point to a missing, denied, ambiguous, remote, or non-previewable file. The action then failed after the user selected it. **Expected behavior** Paperclip must show a file chip only after the server confirms that the current board session can open the exact file reference. All other path-shaped text must stay ordinary inline code. **Steps to reproduce** 1. Add a task comment that contains inline code with a missing or inaccessible workspace path. 2. Open the task thread as a board user. 3. Observe that the path looks like an actionable file chip. 4. Select the chip and observe that the file cannot open. **Paperclip version or commit** `19be4cf927` and earlier. **Deployment mode** Local dev and self-hosted server. **Access context** Board user. ## What Changed - Added shared request, response, and validation contracts for batched workspace-file availability checks. - Added an issue-scoped server endpoint that resolves file references with company, issue, workspace, and preview-access checks. - Added bounded batch concurrency and tests for missing, denied, ambiguous, remote, unsupported, and available files. - Added an issue-scoped UI availability registry that deduplicates, batches, caches, and invalidates file checks. - Changed task-comment markdown rendering so only confirmed files get chip styling and file-viewer behavior. - Bound each chip to the exact workspace target that passed the availability check. ## Verification - `pnpm exec vitest run packages/shared/src/workspace-file-resource.test.ts server/src/__tests__/file-resources.test.ts ui/src/components/MarkdownBody.test.tsx ui/src/components/WorkspaceFileMarkdownBody.availability.test.tsx ui/src/lib/remark-workspace-file-refs.test.ts ui/src/lib/workspace-file-availability.test.ts` — 93 passed, 35 skipped. - `pnpm check:token-gates` — clean. - `pnpm -r typecheck` — passed. - `pnpm build` — passed. - `pnpm test:run` — all server and UI groups passed. One unchanged CLI test saw the run-injected static AWS credentials and expected only its local `AWS_PROFILE`. The same test passed, 8 of 8, after removing only `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` from its process environment. ## Risks - File chips now appear after an asynchronous availability check, so path-shaped text can briefly render as inline code. - Availability results use the existing 30-second file-resource cache window. File-resource invalidation forces a new check. - The endpoint limits each request to 100 references and the client chunks larger sets. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex with GPT-5. The service did not expose a more specific model ID or context-window size. The agent used high-reasoning mode, repository tools, command execution, and test execution. ## 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 - [x] 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 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
815e49bb7c
commit
7ea2068ef8
25 files changed
+1967
-30
No files matched your search
@@ -0,0 +1,365 @@
|
||||
// @vitest-environment jsdom
|
||||
|
||||
import { StrictMode, type ReactNode } from "react";
|
||||
import { flushSync } from "react-dom";
|
||||
import { createRoot, type Root } from "react-dom/client";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type {
|
||||
ResolvedWorkspaceResource,
|
||||
WorkspaceFileAvailabilityResponse,
|
||||
WorkspaceFileAvailabilityResult,
|
||||
} from "@paperclipai/shared";
|
||||
import type { FileResourceQuery } from "@/api/file-resources";
|
||||
|
||||
const mockAvailability = vi.hoisted(() => vi.fn());
|
||||
const mockNavigate = vi.hoisted(() => vi.fn());
|
||||
|
||||
vi.mock("@/api/file-resources", () => ({
|
||||
fileResourcesApi: { availability: mockAvailability },
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/router", () => ({
|
||||
Link: ({ children, to, ...props }: { children: ReactNode; to: string }) => (
|
||||
<a href={to} {...props}>{children}</a>
|
||||
),
|
||||
useLocation: () => ({ pathname: "/PAP/issues/PAP-1", search: "", hash: "", state: null }),
|
||||
useNavigate: () => mockNavigate,
|
||||
useCaseHref: () => (identifier: string) => `/cases/${identifier}`,
|
||||
}));
|
||||
|
||||
vi.mock("../context/CompanyContext", () => ({
|
||||
useOptionalCompany: () => null,
|
||||
}));
|
||||
|
||||
import { FileViewerProvider } from "@/context/FileViewerContext";
|
||||
import { ThemeProvider } from "@/context/ThemeContext";
|
||||
import { WorkspaceFileMarkdownBody } from "./WorkspaceFileMarkdownBody";
|
||||
|
||||
const ISSUE_ID = "3fb1a3f4-3f0e-4c58-8d3e-1c2f0f5a9b11";
|
||||
const PROJECT_ID = "17acae7d-9d0c-46bf-9c82-be9694ac3461";
|
||||
const WORKSPACE_ID = "0de5f74f-a7d4-4f73-a9a0-455a2b968cf2";
|
||||
|
||||
function act(callback: () => void) {
|
||||
flushSync(callback);
|
||||
}
|
||||
|
||||
async function waitForExpectation(assertion: () => void) {
|
||||
let lastError: unknown;
|
||||
for (let attempt = 0; attempt < 30; attempt += 1) {
|
||||
try {
|
||||
assertion();
|
||||
return;
|
||||
} catch (error) {
|
||||
lastError = error;
|
||||
await new Promise((resolve) => window.setTimeout(resolve, 0));
|
||||
}
|
||||
}
|
||||
throw lastError;
|
||||
}
|
||||
|
||||
function resource(overrides: Partial<ResolvedWorkspaceResource> = {}): ResolvedWorkspaceResource {
|
||||
return {
|
||||
kind: "file",
|
||||
provider: "git_worktree",
|
||||
title: "a.ts",
|
||||
displayPath: "ui/src/a.ts",
|
||||
workspaceLabel: "Execution workspace",
|
||||
workspaceKind: "execution_workspace",
|
||||
workspaceId: WORKSPACE_ID,
|
||||
previewKind: "text",
|
||||
capabilities: { preview: true, download: true, listChildren: false },
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
function openable(path: string, overrides: Partial<ResolvedWorkspaceResource> = {}): WorkspaceFileAvailabilityResult {
|
||||
return {
|
||||
query: { path, workspace: "auto", projectId: null, workspaceId: null },
|
||||
openable: true,
|
||||
resource: resource({ displayPath: path, ...overrides }),
|
||||
};
|
||||
}
|
||||
|
||||
function unavailable(path: string, reason: string): WorkspaceFileAvailabilityResult {
|
||||
return {
|
||||
query: { path, workspace: "auto", projectId: null, workspaceId: null },
|
||||
openable: false,
|
||||
unavailableReason: reason,
|
||||
resource: null,
|
||||
};
|
||||
}
|
||||
|
||||
function respondWith(results: WorkspaceFileAvailabilityResult[]): WorkspaceFileAvailabilityResponse {
|
||||
return { kind: "workspace_file_availability", results };
|
||||
}
|
||||
|
||||
/** Echo every requested path back as openable. */
|
||||
function echoOpenable(_issueId: string, queries: FileResourceQuery[]) {
|
||||
return Promise.resolve(respondWith(queries.map((query) => openable(query.path))));
|
||||
}
|
||||
|
||||
let container: HTMLDivElement;
|
||||
let root: Root;
|
||||
|
||||
function render(children: ReactNode) {
|
||||
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
act(() => {
|
||||
root.render(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ThemeProvider>
|
||||
<FileViewerProvider issueId={ISSUE_ID}>{children}</FileViewerProvider>
|
||||
</ThemeProvider>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
return queryClient;
|
||||
}
|
||||
|
||||
function chips() {
|
||||
return [...container.querySelectorAll('[data-workspace-file-link="true"]')];
|
||||
}
|
||||
|
||||
function chipPaths() {
|
||||
return chips().map((chip) => chip.getAttribute("data-workspace-file-path"));
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
mockAvailability.mockReset();
|
||||
mockNavigate.mockReset();
|
||||
container = document.createElement("div");
|
||||
document.body.appendChild(container);
|
||||
root = createRoot(container);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
act(() => root.unmount());
|
||||
container.remove();
|
||||
});
|
||||
|
||||
describe("WorkspaceFileMarkdownBody availability gating", () => {
|
||||
it("renders plain inline code until the batch confirms the reference", async () => {
|
||||
let resolveBatch: ((value: WorkspaceFileAvailabilityResponse) => void) | undefined;
|
||||
mockAvailability.mockReturnValue(new Promise((resolve) => { resolveBatch = resolve; }));
|
||||
|
||||
render(<WorkspaceFileMarkdownBody>{"Check `ui/src/a.ts:42` please."}</WorkspaceFileMarkdownBody>);
|
||||
|
||||
// Pending: no chip, no icon, no button role — just code.
|
||||
expect(chips()).toHaveLength(0);
|
||||
expect(container.querySelector("code")?.textContent).toBe("ui/src/a.ts:42");
|
||||
expect(container.querySelector("svg")).toBeNull();
|
||||
expect(container.querySelector('[role="button"]')).toBeNull();
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(1));
|
||||
act(() => resolveBatch!(respondWith([openable("ui/src/a.ts")])));
|
||||
|
||||
await waitForExpectation(() => expect(chipPaths()).toEqual(["ui/src/a.ts"]));
|
||||
});
|
||||
|
||||
it("keeps unavailable, denied, and unsupported references as plain code", async () => {
|
||||
mockAvailability.mockResolvedValue(respondWith([
|
||||
unavailable("ui/src/missing.ts", "not_found"),
|
||||
unavailable("ui/src/denied.ts", "forbidden"),
|
||||
unavailable("ui/src/remote.ts", "unsupported_resource"),
|
||||
]));
|
||||
|
||||
render(
|
||||
<WorkspaceFileMarkdownBody>
|
||||
{"See `ui/src/missing.ts:1`, `ui/src/denied.ts:2` and `ui/src/remote.ts:3`."}
|
||||
</WorkspaceFileMarkdownBody>,
|
||||
);
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(1));
|
||||
await new Promise((resolve) => window.setTimeout(resolve, 0));
|
||||
expect(chips()).toHaveLength(0);
|
||||
expect(container.querySelectorAll("code")).toHaveLength(3);
|
||||
});
|
||||
|
||||
it("fails closed when the availability batch errors", async () => {
|
||||
mockAvailability.mockRejectedValue(new Error("boom"));
|
||||
|
||||
render(<WorkspaceFileMarkdownBody>{"Check `ui/src/a.ts:42`."}</WorkspaceFileMarkdownBody>);
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(1));
|
||||
await new Promise((resolve) => window.setTimeout(resolve, 0));
|
||||
expect(chips()).toHaveLength(0);
|
||||
expect(container.querySelector("code")?.textContent).toBe("ui/src/a.ts:42");
|
||||
});
|
||||
|
||||
it("checks a reference duplicated across comments exactly once", async () => {
|
||||
mockAvailability.mockImplementation(echoOpenable);
|
||||
|
||||
render(
|
||||
<>
|
||||
<WorkspaceFileMarkdownBody>{"First `ui/src/a.ts:1`."}</WorkspaceFileMarkdownBody>
|
||||
<WorkspaceFileMarkdownBody>{"Second `ui/src/a.ts:1` again."}</WorkspaceFileMarkdownBody>
|
||||
<WorkspaceFileMarkdownBody>{"Third `ui/src/a.ts:9` at another line."}</WorkspaceFileMarkdownBody>
|
||||
</>,
|
||||
);
|
||||
|
||||
await waitForExpectation(() => expect(chipPaths()).toHaveLength(3));
|
||||
// One coalesced request; the line suffix is not part of the lookup, so the
|
||||
// three references collapse to a single query.
|
||||
expect(mockAvailability).toHaveBeenCalledTimes(1);
|
||||
expect(mockAvailability.mock.calls[0]![1]).toEqual([
|
||||
{ path: "ui/src/a.ts", workspace: "auto", projectId: null, workspaceId: null },
|
||||
]);
|
||||
});
|
||||
|
||||
it("chunks more than 100 unique references", async () => {
|
||||
mockAvailability.mockImplementation(echoOpenable);
|
||||
const paths = Array.from({ length: 150 }, (_, index) => `ui/src/file-${index}.ts`);
|
||||
|
||||
render(
|
||||
<WorkspaceFileMarkdownBody>
|
||||
{paths.map((path) => `\`${path}\``).join(" ")}
|
||||
</WorkspaceFileMarkdownBody>,
|
||||
);
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(2));
|
||||
const sizes = mockAvailability.mock.calls.map((call) => (call[1] as FileResourceQuery[]).length);
|
||||
expect(sizes).toEqual([100, 50]);
|
||||
await waitForExpectation(() => expect(chips()).toHaveLength(150));
|
||||
});
|
||||
|
||||
it("runs at most two availability chunks concurrently", async () => {
|
||||
let activeRequests = 0;
|
||||
let maxActiveRequests = 0;
|
||||
const resolveRequests: Array<() => void> = [];
|
||||
mockAvailability.mockImplementation((_issueId: string, queries: FileResourceQuery[]) => {
|
||||
activeRequests += 1;
|
||||
maxActiveRequests = Math.max(maxActiveRequests, activeRequests);
|
||||
return new Promise<WorkspaceFileAvailabilityResponse>((resolve) => {
|
||||
resolveRequests.push(() => {
|
||||
activeRequests -= 1;
|
||||
resolve(respondWith(queries.map((query) => openable(query.path))));
|
||||
});
|
||||
});
|
||||
});
|
||||
const paths = Array.from({ length: 250 }, (_, index) => `ui/src/file-${index}.ts`);
|
||||
|
||||
render(
|
||||
<WorkspaceFileMarkdownBody>
|
||||
{paths.map((path) => `\`${path}\``).join(" ")}
|
||||
</WorkspaceFileMarkdownBody>,
|
||||
);
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(2));
|
||||
await new Promise((resolve) => window.setTimeout(resolve, 0));
|
||||
expect(maxActiveRequests).toBe(2);
|
||||
|
||||
act(() => resolveRequests[0]!());
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(3));
|
||||
expect(activeRequests).toBe(2);
|
||||
expect(maxActiveRequests).toBe(2);
|
||||
|
||||
act(() => {
|
||||
resolveRequests[1]!();
|
||||
resolveRequests[2]!();
|
||||
});
|
||||
await waitForExpectation(() => expect(chips()).toHaveLength(250));
|
||||
expect(maxActiveRequests).toBe(2);
|
||||
});
|
||||
|
||||
it("checks only unseen references when a new comment arrives", async () => {
|
||||
mockAvailability.mockImplementation(echoOpenable);
|
||||
|
||||
const first = <WorkspaceFileMarkdownBody>{"First `ui/src/a.ts:1`."}</WorkspaceFileMarkdownBody>;
|
||||
const queryClient = render(first);
|
||||
await waitForExpectation(() => expect(chipPaths()).toEqual(["ui/src/a.ts"]));
|
||||
expect(mockAvailability).toHaveBeenCalledTimes(1);
|
||||
|
||||
act(() => {
|
||||
root.render(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ThemeProvider>
|
||||
<FileViewerProvider issueId={ISSUE_ID}>
|
||||
{first}
|
||||
<WorkspaceFileMarkdownBody>{"New `ui/src/a.ts:1` and `ui/src/b.ts:2`."}</WorkspaceFileMarkdownBody>
|
||||
</FileViewerProvider>
|
||||
</ThemeProvider>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(2));
|
||||
expect(mockAvailability.mock.calls[1]![1]).toEqual([
|
||||
{ path: "ui/src/b.ts", workspace: "auto", projectId: null, workspaceId: null },
|
||||
]);
|
||||
});
|
||||
|
||||
it("binds the confirmed project workspace to the chip and opens it on click", async () => {
|
||||
mockAvailability.mockResolvedValue(respondWith([
|
||||
{
|
||||
query: { path: "ui/src/a.ts", workspace: "auto", projectId: null, workspaceId: null },
|
||||
openable: true,
|
||||
resource: resource({
|
||||
workspaceKind: "project_workspace",
|
||||
workspaceId: WORKSPACE_ID,
|
||||
projectId: PROJECT_ID,
|
||||
projectName: "Paperclip App",
|
||||
}),
|
||||
},
|
||||
]));
|
||||
|
||||
render(<WorkspaceFileMarkdownBody>{"Check `ui/src/a.ts:42`."}</WorkspaceFileMarkdownBody>);
|
||||
await waitForExpectation(() => expect(chips()).toHaveLength(1));
|
||||
|
||||
const chip = chips()[0] as HTMLAnchorElement;
|
||||
const search = new URL(chip.href, window.location.href).searchParams;
|
||||
expect(search.get("file")).toBe("ui/src/a.ts");
|
||||
expect(search.get("line")).toBe("42");
|
||||
expect(search.get("workspace")).toBe("project");
|
||||
expect(search.get("projectId")).toBe(PROJECT_ID);
|
||||
expect(search.get("workspaceId")).toBe(WORKSPACE_ID);
|
||||
|
||||
act(() => {
|
||||
chip.dispatchEvent(new window.MouseEvent("click", { bubbles: true, cancelable: true, button: 0 }));
|
||||
});
|
||||
|
||||
expect(mockNavigate).toHaveBeenCalled();
|
||||
const target = mockNavigate.mock.calls[0]![0] as { search: string };
|
||||
const opened = new URLSearchParams(target.search);
|
||||
expect(opened.get("file")).toBe("ui/src/a.ts");
|
||||
expect(opened.get("workspace")).toBe("project");
|
||||
expect(opened.get("projectId")).toBe(PROJECT_ID);
|
||||
expect(opened.get("workspaceId")).toBe(WORKSPACE_ID);
|
||||
});
|
||||
|
||||
it("still batches under StrictMode's remount", async () => {
|
||||
mockAvailability.mockImplementation(echoOpenable);
|
||||
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
|
||||
act(() => {
|
||||
root.render(
|
||||
<StrictMode>
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ThemeProvider>
|
||||
<FileViewerProvider issueId={ISSUE_ID}>
|
||||
<WorkspaceFileMarkdownBody>{"Check `ui/src/a.ts:42`."}</WorkspaceFileMarkdownBody>
|
||||
</FileViewerProvider>
|
||||
</ThemeProvider>
|
||||
</QueryClientProvider>
|
||||
</StrictMode>,
|
||||
);
|
||||
});
|
||||
|
||||
await waitForExpectation(() => expect(chipPaths()).toEqual(["ui/src/a.ts"]));
|
||||
expect(mockAvailability).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rechecks after the issue's file resources are invalidated", async () => {
|
||||
mockAvailability.mockImplementation(echoOpenable);
|
||||
const queryClient = render(<WorkspaceFileMarkdownBody>{"Check `ui/src/a.ts:1`."}</WorkspaceFileMarkdownBody>);
|
||||
await waitForExpectation(() => expect(chips()).toHaveLength(1));
|
||||
expect(mockAvailability).toHaveBeenCalledTimes(1);
|
||||
|
||||
mockAvailability.mockResolvedValue(respondWith([unavailable("ui/src/a.ts", "not_found")]));
|
||||
act(() => {
|
||||
void queryClient.invalidateQueries({ queryKey: ["issues", "file-resources", ISSUE_ID] });
|
||||
});
|
||||
|
||||
await waitForExpectation(() => expect(mockAvailability).toHaveBeenCalledTimes(2));
|
||||
await waitForExpectation(() => expect(chips()).toHaveLength(0));
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user