mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 14:16:39 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents can submit suggestions and complaints while they work. > - Operators currently need database access to read those messages. > - That makes it hard to find feedback from one agent or share a specific message. > - This pull request adds a standard Commentary tab in Audit. > - Operators can filter messages, browse 50 at a time, and share links to full message dialogs. ## Linked Issues or Issue Description Refs #15367, which added feedback storage and agent submission. The duplicate search found no open PR for a commentary reader. **Subsystem affected** Server, shared API contracts, and the board UI. **Problem or motivation** Agent suggestions and complaints are stored but have no board reader. Operators must query the database to inspect feedback. **Proposed solution** Add Commentary to Audit without an experimental toggle. Show previews, agent and message-type filters, text search, and 50-message pages. Opening a row updates the URL. A copied URL opens the full message with its agent, task, and run context. **Alternatives considered** Direct database queries remain useful for operators, but do not provide a normal product workflow. An experimental setting would add setup to a read-only audit page. **Roadmap alignment** This completes the operator view for the existing feedback feature. It supports human oversight and does not change agent execution or task ownership. ## What Changed - Add company-scoped, board-only list and detail endpoints with audit permission and source-task privacy checks. - Return bounded previews and counts. Fetch full bodies only when a message opens. - Add the Commentary tab, search, filters, 50-row pagination, and URL-based selection. Audit tab destinations work in both layouts, with a classic sidebar entry for Commentary. - Add production Storybook examples for populated, filtered, linked, long, paginated, empty, loading, failed, denied, mobile, and light states. - Update API documentation and the feature and agent access maps. ## Verification - Final head `abc4a251c14cfbfec007542214beec528f2b18bf`: 55 checks pass; two conditional Storybook checks are skipped. The PR workflow is successful. Greptile is 5/5 with all findings resolved. - Focused database, HTTP, submission, OpenAPI, and UI regression tests cover company isolation, task privacy, filter bounds, 50-row pagination, and copied message links. - Browser Storybook journeys cover row opening, direct links, combined filters, search, and pagination. Storybook uses synthetic API fixtures; database authorization is tested separately. - Token gates, feature maps, and Storybook catalog validation pass. - `pnpm -r typecheck`, `pnpm build`, and `pnpm build-storybook` pass on the rebased branch. All 80 focused tests and all 37 Audit tests pass, including the real App route table in both layouts, task grant/revocation through HTTP, deleted-task privacy, and URL UI regressions. The full suite passes in the parallel PR CI matrix. The local serial full-suite run was stopped before completion after checking the recorded 94-minute serial duration. Local focused tests, workspace typecheck, production build, and final server/UI checks pass. - The linked-message Storybook test initially asserted against the loading dialog. It now waits for the message body; both direct-link and row-open journeys pass. - Native and legacy deleted-task privacy regressions failed before the fix and pass afterward. The reader checks the run task binding after the task foreign key clears. - A rapid search-then-agent-filter browser regression reproduced the search overwriting the selected agent. The pending search now restarts when URL filters change. The same browser test passes with both filters applied. - CI found two existing AuditHub assertions that still expected five tabs. They now assert the six-tab model and selected-company Commentary rendering. All 37 Audit tests pass. All other test and build jobs passed before this test-only correction. - The [agent-chat attachment browser test](https://github.com/paperclipai/paperclip/actions/runs/39307881171/job/118250189664) timed out waiting for an attachment preview. The [same shard passed on the prior head](https://github.com/paperclipai/paperclip/actions/runs/39306658703/job/118245498343) with identical application code; the intervening change only updates AuditHub unit tests. The single retry passed on the same head. The precise cause of the earlier failure remains unconfirmed. ## Risks - Commentary may contain sensitive task context. The reader requires board access and audit permission, and filters source-task visibility before returning previews, counts, or full bodies. - Text search scans stored message bodies. Pages are bounded and full bodies are fetched on demand. - Offset pagination can shift if new messages arrive while paging. Ordering uses timestamp and ID for stable ties. - No database migration or agent submission behavior changes are required. ## Model Used - OpenAI Codex, GPT-6 family, with reasoning, tool use, browser interaction, and code execution. The exact backend model ID and context-window size are not exposed in this session. ## 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 - [x] 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>
311 lines
12 KiB
TypeScript
311 lines
12 KiB
TypeScript
// @vitest-environment jsdom
|
|
|
|
// Regression guard for the Audit hub: Activity remains the canonical root,
|
|
// while Runs, Costs, and Budgets live beneath it. Company-prefixed and bare
|
|
// legacy routes must keep resolving with their query filters intact; `/audit`
|
|
// specifically retains the historical Agent Actions preset. This drives the
|
|
// real <App> route table so removing either registration fails loudly.
|
|
|
|
import type { ReactNode } from "react";
|
|
import { flushSync } from "react-dom";
|
|
import { createRoot } from "react-dom/client";
|
|
import { MemoryRouter, useLocation } from "react-router-dom";
|
|
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { App } from "./App";
|
|
|
|
const streamlinedUiState = vi.hoisted(() => ({ enabled: true, loaded: true }));
|
|
|
|
vi.mock("./hooks/useStreamlinedUiEnabled", () => ({
|
|
useStreamlinedUiEnabled: () => streamlinedUiState,
|
|
}));
|
|
|
|
// jsdom's CSS parser rejects the custom-property marker rule stitches inserts
|
|
// (`--sxs{--sxs:N}`), pulled into <App>'s eager import graph transitively via
|
|
// @codesandbox/sandpack-react. Substitute a benign, valid rule on parse failure
|
|
// so stitches' index bookkeeping stays intact and the module graph evaluates.
|
|
vi.hoisted(() => {
|
|
const sheetProto = window.CSSStyleSheet.prototype as unknown as {
|
|
insertRule: (rule: string, index?: number) => number;
|
|
__papActivityRoutingPatched?: boolean;
|
|
};
|
|
if (!sheetProto.__papActivityRoutingPatched) {
|
|
const original = sheetProto.insertRule;
|
|
sheetProto.insertRule = function patched(this: CSSStyleSheet, rule: string, index?: number) {
|
|
try {
|
|
return original.call(this, rule, index);
|
|
} catch {
|
|
try {
|
|
return original.call(this, ".pap16302-noop{}", index);
|
|
} catch {
|
|
return this.cssRules?.length ?? 0;
|
|
}
|
|
}
|
|
};
|
|
sheetProto.__papActivityRoutingPatched = true;
|
|
}
|
|
});
|
|
|
|
// Real Layout renders the full authenticated shell (sidebar, data queries) and
|
|
// owns the "No company matches prefix" NotFound. For routing we only need it to
|
|
// resolve the :companyPrefix segment and render its nested routes.
|
|
vi.mock("./components/Layout", async () => {
|
|
const { Outlet } = await import("react-router-dom");
|
|
return { Layout: () => <Outlet /> };
|
|
});
|
|
|
|
vi.mock("./components/Layout.production", async () => {
|
|
const { Outlet } = await import("react-router-dom");
|
|
return { Layout: () => <Outlet /> };
|
|
});
|
|
|
|
// Rendered by <App> outside <Routes> and needs DialogProvider; irrelevant here.
|
|
vi.mock("./components/OnboardingWizardVariant", () => ({
|
|
OnboardingWizardVariant: () => null,
|
|
}));
|
|
|
|
// Cloud access is unrelated to the route-table regression. Let it fall through
|
|
// synchronously so this test does not poll its query transitions.
|
|
vi.mock("./components/CloudAccessGate", async () => {
|
|
const { Outlet } = await import("react-router-dom");
|
|
return { CloudAccessGate: () => <Outlet /> };
|
|
});
|
|
|
|
// Sentinel page that also reports the resolved path + query, so we can assert
|
|
// the merged route *and* the preset mode a redirect carried into it.
|
|
vi.mock("./pages/audit/CompanyActivity", () => ({
|
|
CompanyActivity: () => {
|
|
const location = useLocation();
|
|
return <div>{`ACTIVITY_PAGE@${location.pathname}${location.search}`}</div>;
|
|
},
|
|
}));
|
|
|
|
vi.mock("./pages/audit/AuditHub", () => ({
|
|
AuditHub: ({ section }: { section: string }) => {
|
|
const location = useLocation();
|
|
return <div>{`AUDIT_${section.toUpperCase()}@${location.pathname}${location.search}`}</div>;
|
|
},
|
|
}));
|
|
|
|
vi.mock("./pages/Issues", () => ({
|
|
Issues: () => {
|
|
const location = useLocation();
|
|
return <div>{`TASKS_PAGE@${location.pathname}`}</div>;
|
|
},
|
|
}));
|
|
|
|
vi.mock("./pages/audit/CompanyActivity.production", () => ({
|
|
CompanyActivity: () => {
|
|
const location = useLocation();
|
|
return <div>{`PRODUCTION_ACTIVITY@${location.pathname}${location.search}`}</div>;
|
|
},
|
|
}));
|
|
|
|
vi.mock("./pages/Costs", () => ({
|
|
Costs: () => {
|
|
const location = useLocation();
|
|
return <div>{`SHARED_COSTS@${location.pathname}${location.search}`}</div>;
|
|
},
|
|
}));
|
|
|
|
vi.mock("./pages/NotFound", () => ({
|
|
NotFoundPage: ({ scope }: { scope: string }) => <div>{`NOT_FOUND:${scope}`}</div>,
|
|
}));
|
|
|
|
vi.mock("./pages/PluginPage", () => ({
|
|
PluginPage: () => <div>PRODUCTION_PLUGIN_FALLBACK</div>,
|
|
}));
|
|
|
|
const PAP_COMPANY = {
|
|
id: "company-1",
|
|
name: "Paperclip",
|
|
issuePrefix: "PAP",
|
|
status: "active",
|
|
};
|
|
const ACME_COMPANY = {
|
|
id: "company-2",
|
|
name: "Acme",
|
|
issuePrefix: "ACME",
|
|
status: "active",
|
|
};
|
|
|
|
// Mutable so a test can put the *selected* company out of step with the company
|
|
// in the URL — that mismatch is what catches a redirect that re-resolves the
|
|
// company from context instead of keeping the one the deep link named.
|
|
let companyState = {
|
|
companies: [PAP_COMPANY] as Array<typeof PAP_COMPANY>,
|
|
selected: PAP_COMPANY as typeof PAP_COMPANY | null,
|
|
};
|
|
vi.mock("./context/CompanyContext", () => ({
|
|
useCompany: () => ({
|
|
companies: companyState.companies,
|
|
selectedCompanyId: companyState.selected?.id ?? null,
|
|
selectedCompany: companyState.selected,
|
|
loading: false,
|
|
}),
|
|
CompanyProvider: ({ children }: { children: ReactNode }) => <>{children}</>,
|
|
}));
|
|
|
|
function renderAppAt(container: HTMLElement, path: string) {
|
|
const root = createRoot(container);
|
|
flushSync(() => {
|
|
root.render(
|
|
<MemoryRouter initialEntries={[path]}>
|
|
<App />
|
|
</MemoryRouter>,
|
|
);
|
|
});
|
|
return root;
|
|
}
|
|
|
|
/**
|
|
* Waits on the condition, not on a fixed number of turns. The previous version
|
|
* yielded at most five macrotasks before asserting, which is ample on an idle
|
|
* machine and not when the suite is running many workers in parallel — the
|
|
* container was still empty and the assertion failed on a route that resolves
|
|
* perfectly well. `vi.waitFor` retries against a time budget instead, so a
|
|
* loaded worker gets more turns rather than a failure.
|
|
*/
|
|
async function waitForRoute(container: HTMLElement, text: string) {
|
|
await vi.waitFor(() => expect(container.textContent).toContain(text));
|
|
}
|
|
|
|
describe("App Activity routing (PAP-16302)", () => {
|
|
let container: HTMLDivElement;
|
|
|
|
beforeEach(() => {
|
|
container = document.createElement("div");
|
|
document.body.appendChild(container);
|
|
companyState = { companies: [PAP_COMPANY], selected: PAP_COMPANY };
|
|
streamlinedUiState.enabled = true;
|
|
streamlinedUiState.loaded = true;
|
|
});
|
|
|
|
afterEach(() => {
|
|
container.remove();
|
|
document.body.innerHTML = "";
|
|
vi.clearAllMocks();
|
|
});
|
|
|
|
it("serves the merged Activity page at /:company/activity", async () => {
|
|
const root = renderAppAt(container, "/PAP/activity");
|
|
await waitForRoute(container, "ACTIVITY_PAGE@/PAP/activity");
|
|
expect(container.textContent).not.toContain("No organization matches prefix");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("serves organization and entity-scoped run history beneath Activity", async () => {
|
|
const root = renderAppAt(
|
|
container,
|
|
"/PAP/activity/runs?entityType=routine&entityId=routine-1",
|
|
);
|
|
await waitForRoute(
|
|
container,
|
|
"AUDIT_RUNS@/PAP/activity/runs?entityType=routine&entityId=routine-1",
|
|
);
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("redirects /:company/audit to Activity with the agent-actions mode preset", async () => {
|
|
const root = renderAppAt(container, "/PAP/audit");
|
|
await waitForRoute(container, "ACTIVITY_PAGE@/PAP/activity?mode=agents");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("serves each canonical Audit section under Activity", async () => {
|
|
const root = renderAppAt(container, "/PAP/activity/runs?agentId=agent-1");
|
|
await waitForRoute(container, "AUDIT_RUNS@/PAP/activity/runs?agentId=agent-1");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("redirects legacy costs and preserves its deep-link filters", async () => {
|
|
const root = renderAppAt(container, "/PAP/costs?range=30d#usage");
|
|
await waitForRoute(container, "AUDIT_COSTS@/PAP/activity/costs?range=30d");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("redirects legacy Audit sections to their canonical destination", async () => {
|
|
const root = renderAppAt(container, "/PAP/audit/budgets?projectId=project-1");
|
|
await waitForRoute(
|
|
container,
|
|
"AUDIT_BUDGETS@/PAP/activity/budgets?projectId=project-1",
|
|
);
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("keeps the company from the URL when /:company/audit is not the selected company", async () => {
|
|
// A shared /ACME/audit link opened by someone whose selected company is PAP
|
|
// must still show ACME's activity. The redirect target is written absolute
|
|
// (`/activity?mode=agents`) and relies on `@/lib/router`'s prefix-aware
|
|
// <Navigate>, which resolves the company from the route param ahead of the
|
|
// selected company. Importing <Navigate> from `react-router-dom` instead —
|
|
// the import most files use — would send this deep link to the *viewer's*
|
|
// company, so pin the behaviour here.
|
|
companyState = { companies: [PAP_COMPANY, ACME_COMPANY], selected: PAP_COMPANY };
|
|
const root = renderAppAt(container, "/ACME/audit");
|
|
await waitForRoute(container, "ACTIVITY_PAGE@/ACME/activity?mode=agents");
|
|
expect(container.textContent).not.toContain("ACTIVITY_PAGE@/PAP/activity");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("redirects the bare /audit deep link through to the prefixed Activity page", async () => {
|
|
const root = renderAppAt(container, "/audit");
|
|
await waitForRoute(container, "ACTIVITY_PAGE@/PAP/activity?mode=agents");
|
|
expect(container.textContent).not.toContain("No organization matches prefix");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("redirects the bare /tasks post-login target to the real task list", async () => {
|
|
const root = renderAppAt(container, "/tasks");
|
|
await waitForRoute(container, "TASKS_PAGE@/PAP/issues");
|
|
expect(container.textContent).not.toContain("/tasks/dashboard");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("redirects a bare legacy section through to the prefixed Audit hub", async () => {
|
|
const root = renderAppAt(container, "/runs?runStatus=succeeded");
|
|
await waitForRoute(container, "AUDIT_RUNS@/PAP/activity/runs?runStatus=succeeded");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
|
|
it("uses legacy Activity and the shared standalone Costs page when Streamlined UI is disabled", async () => {
|
|
streamlinedUiState.enabled = false;
|
|
const activityRoot = renderAppAt(container, "/PAP/activity");
|
|
await waitForRoute(container, "PRODUCTION_ACTIVITY@/PAP/activity");
|
|
flushSync(() => activityRoot.unmount());
|
|
|
|
const costsRoot = renderAppAt(container, "/PAP/costs?range=30d");
|
|
await waitForRoute(container, "SHARED_COSTS@/PAP/costs?range=30d");
|
|
flushSync(() => costsRoot.unmount());
|
|
});
|
|
|
|
it.each([true, false])("serves all Audit tab destinations with Streamlined UI set to %s", async (enabled) => {
|
|
streamlinedUiState.enabled = enabled;
|
|
for (const section of ["runs", "costs", "budgets", "timeline", "commentary"]) {
|
|
const path = `/PAP/activity/${section}?agentId=agent-1`;
|
|
const root = renderAppAt(container, path);
|
|
await waitForRoute(container, `AUDIT_${section.toUpperCase()}@${path}`);
|
|
expect(container.textContent).not.toContain("NOT_FOUND");
|
|
flushSync(() => root.unmount());
|
|
}
|
|
});
|
|
|
|
it("does not mount streamlined routes before the experiment setting loads", async () => {
|
|
streamlinedUiState.loaded = false;
|
|
const root = renderAppAt(container, "/PAP/activity");
|
|
expect(container.textContent).not.toContain("ACTIVITY_PAGE");
|
|
expect(container.textContent).not.toContain("PRODUCTION_ACTIVITY");
|
|
|
|
streamlinedUiState.enabled = false;
|
|
streamlinedUiState.loaded = true;
|
|
flushSync(() => {
|
|
root.render(
|
|
<MemoryRouter initialEntries={["/PAP/activity"]}>
|
|
<App />
|
|
</MemoryRouter>,
|
|
);
|
|
});
|
|
await waitForRoute(container, "PRODUCTION_ACTIVITY@/PAP/activity");
|
|
flushSync(() => root.unmount());
|
|
});
|
|
});
|