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
32 changes: 32 additions & 0 deletions apps/hub/src/artifacts/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
LiveApp,
MAX_APPROVAL_PACKAGE_BYTES,
MaterialReceipt,
MaterialRemovedReceipt,
} from "@corbits-builder/schemas/artifact";
import { ProjectActivity } from "@corbits-builder/schemas/activity";
import {
Expand Down Expand Up @@ -398,6 +399,37 @@ export function mountArtifactRoutes(
);
},
);
app.delete(
"/:projectId/materials/:artifactId",
describeRoute({
tags: ["Artifacts"],
operationId: "removeMaterial",
description:
"Takes an attached file back. The coordinator, when one runs, is told the file is withdrawn first, so a note that cannot be sent leaves the material in place for another try; then the material is archived, so it leaves the project's documents and the coordinator's filed materials while its versions stay. The message it was sent with keeps showing it. `told` says whether a coordinator was mailed.",
security,
responses: {
200: {
description: "The material taken back",
content: {
"application/json": { schema: resolver(MaterialRemovedReceipt) },
},
},
404: { description: "No such material in this project" },
},
}),
async (c) => {
const current = actorOf(c);
return c.json(
await service.removeMaterial(
current,
c.req.param("projectId"),
c.req.param("artifactId"),
(runId, content) =>
mailRunAsCaller(c, nativeFetch, current.tenantId, runId, content),
Comment on lines +427 to +428

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 · Mail failure after archiving returns an error, and a retry never tells the coordinator

The handler passes mailRunAsCaller as the mail callback. removeMaterial calls it only after setArtifactArchived has already archived the material. mailRunAsCaller throws when nativeFetch is missing (503) or when the upstream mail call fails, so the DELETE returns an error even though the file is already gone. If the client retries, the material is already archived, so the request does nothing and returns 200. The coordinator is never told the file was withdrawn and keeps relying on it.

Remove a material while a coordinator is running and the run mail route fails or is unavailable (nativeFetch undefined, upstream 409/503).

Resolve this comment. Triage will not finish while it is open.

),
);
},
);
app.post(
"/:projectId/design-concepts",
describeRoute({
Expand Down
34 changes: 33 additions & 1 deletion apps/hub/src/artifacts/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
listArtifacts,
listArtifactVersions,
MAX_LIST_LIMIT,
setArtifactArchived,
VersionConflictError,
writeArtifactVersion,
type ArtifactDb,
Expand Down Expand Up @@ -54,6 +55,7 @@ import {
type AcceptedMaterials,
type UserManual,
type MaterialReceipt,
MaterialRemovedReceipt,
type DesignTweaksReceipt,
type DesignChoiceReceipt,
type LiveApp,
Expand Down Expand Up @@ -144,7 +146,7 @@ import {
import { awaitingApproval } from "../projects/deploy-tools.ts";
import { frozenModelChoice, workflowBindings } from "../config/models.ts";
import { modelDirectory } from "../settings/custom-providers.ts";
import { materialsNote } from "../config/materials.ts";
import { materialRemovedNote, materialsNote } from "../config/materials.ts";
import { servingOffering } from "../workflows/install-for-tenant.ts";
import {
acceptedMaterials,
Expand Down Expand Up @@ -1248,6 +1250,36 @@ export function createArtifactService(
return { materials };
},

// A material the person takes back: the coordinator, when one runs, is told first, so a note
// that cannot be sent leaves the material in place for another try; then it is archived, so
// every list leaves it out while its versions stay.
async removeMaterial(
actor: Actor,
projectId: string,
artifactId: string,
mailRun: (runId: string, content: string) => Promise<void>,
): Promise<MaterialRemovedReceipt> {
const allowed = await requireProject(actor, projectId, "manage");
if (!(await allowed(artifactResource(MATERIAL_KIND), "write")))
throw new ProjectError(403, "artifact_write_refused", {
kind: MATERIAL_KIND,
});
const row = await getArtifact(db, artifactId);
if (!row || row.tenantId !== actor.tenantId || row.kind !== MATERIAL_KIND)
throw new ProjectError(404, "material_not_found", { artifactId });
if (row.archivedAt !== null)
return { artifactId: row.id, title: row.title, told: false };
const coordinator = await activeCoordinator(projectId);
if (coordinator)
await mailRun(coordinator.runId, materialRemovedNote(row.title));
await setArtifactArchived(db, row, true);
return {
artifactId: row.id,
title: row.title,
told: coordinator !== null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 · told is always true because the missing-coordinator value is undefined, not null

activeCoordinator returns the first row of a destructured array, so it gives undefined when no coordinator is running. undefined !== null is true, so the receipt says the coordinator was mailed even though no mail was sent. This breaks the route's promise that told says whether a coordinator was mailed. This line is in the PR's service.ts change, which is not in the supplied patch; I confirmed it from the head checkout.

Remove a material from a project that has no active coordinator: the response has told: true.

Resolve this comment. Triage will not finish while it is open.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 · told is true even when no coordinator is running

activeCoordinator returns the destructured first row, which is undefined when nothing matches, never null. So coordinator !== null is always true, and the receipt says the coordinator was told even when no note was sent.

Remove a material from a project that has no active coordinator.

Resolve this comment. Triage will not finish while it is open.

};
},

// An imported document with no kind of its own, filed as a material without mail: the
// import's opening message names it to the coordinator.
async fileMaterial(
Expand Down
5 changes: 5 additions & 0 deletions apps/hub/src/config/materials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,8 @@
export function materialsNote(names: readonly string[]): string {
return `I attached ${names.join(", ")}. Each is filed as a material.`;
}

// The person's message when they take an attached file back; the material is archived with it.
export function materialRemovedNote(name: string): string {
return `I removed the attached file ${name}. It is no longer among the materials, so leave it out from here on.`;
}
6 changes: 6 additions & 0 deletions apps/web/src/api/artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,12 @@ export async function attachMaterials(
return productClient().attachMaterials(tenantId, projectId, files, message);
}

/** Takes an attached file back; the coordinator is told and the file leaves the documents. */
export async function removeMaterial(projectId: string, artifactId: string) {
const tenantId = await workspaceTenantId();
return productClient().removeMaterial(tenantId, projectId, artifactId);
}

export async function downloadPackageZip(
projectId: string,
version: number,
Expand Down
9 changes: 9 additions & 0 deletions apps/web/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,15 @@ export const wording = {
passage: { addToPrompt: "Add to prompt" },
documentBar: "Document controls",
printDocument: "Save as PDF or print",
removeAttachment: {
menu: "Remove attachment…",
confirm: "Yes, remove it",
done: (name: string, told: boolean) =>
told
? `${name} is removed; the coordinator has been told.`
: `${name} is removed.`,
failed: "The attachment could not be removed.",
},
versions: {
menu: (version: number) => `Version ${String(version)}, choose a version`,
comparePrevious: "Compare with previous",
Expand Down
41 changes: 39 additions & 2 deletions apps/web/src/pages/workspace/artifact-pane.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { FileText, Printer } from "lucide-react";
import { FileText, Printer, Trash2 } from "lucide-react";
import {
useEffect,
useRef,
Expand Down Expand Up @@ -81,6 +81,9 @@ function filterVisibleDeliverables(

const TAB_PREFIX = "workspace-document";
const DRAFT_TAB = "design-draft";
// The page names each attached file's tab "material:<artifact id>"; only those can be taken back.
export const MATERIAL_TAB_PREFIX = "material:";
const removable = (id: string) => id.startsWith(MATERIAL_TAB_PREFIX);
const draftIds = panelTabIds(TAB_PREFIX, DRAFT_TAB);
const MANUAL_TAB = "user-manual";
const manualIds = panelTabIds(TAB_PREFIX, MANUAL_TAB);
Expand Down Expand Up @@ -249,6 +252,7 @@ export function ArtifactPane({
selected,
state,
onSelect,
onRemove,
paneRef,
changesOpen,
onToggleChanges,
Expand Down Expand Up @@ -278,6 +282,8 @@ export function ArtifactPane({
selected: Deliverable | null;
state: DeliverableState | undefined;
onSelect: (id: string) => void;
/** Takes an attached file back, for a tab that is one; absent where nothing can be removed. */
onRemove?: ((id: string) => void) | undefined;
paneRef?: Ref<HTMLElement>;
changesOpen: boolean;
onToggleChanges: () => void;
Expand Down Expand Up @@ -422,8 +428,11 @@ export function ArtifactPane({
y: number;
} | null>(null);
const printing = useRef<string | null>(null);
// Removing takes two clicks, both in the menu: the second item only exists after the first.
const [confirmingRemove, setConfirmingRemove] = useState(false);
const openTabMenu = (value: string, x: number, y: number) => {
choose(value);
setConfirmingRemove(false);
setTabMenu({ value, x, y });
};
const tabMenuProps = (value: string) => ({
Expand Down Expand Up @@ -452,6 +461,29 @@ export function ArtifactPane({
if (item !== undefined && pane !== null)
printDocument(pane, item.title, documentStatus(item));
};
// The menu's remove item for an attached file's tab: a first click asks, the second removes.
const removeItem = (value: string) => {
if (onRemove === undefined || !removable(value)) return null;
if (confirmingRemove)
return (
<MenuItem className="menu-danger" onSelect={() => onRemove(value)}>
<Trash2 size={16} aria-hidden="true" />
{wording.removeAttachment.confirm}
</MenuItem>
);
return (
<MenuItem
className="menu-danger"
onSelect={(event) => {
event.preventDefault();
setConfirmingRemove(true);
}}
>
<Trash2 size={16} aria-hidden="true" />
{wording.removeAttachment.menu}
</MenuItem>
);
Comment on lines +467 to +485

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 · Two-step remove confirmation is defeated by a double-click or a repeated Enter

React keeps the same MenuItem instance in the same place when it swaps the 'Remove attachment…' item for 'Yes, remove it'. The item under the pointer or focus therefore turns into the confirm action straight away. A double-click, or Enter held down until it auto-repeats, fires onSelect twice, and the second call removes the file. The confirmation step was meant to stop exactly this.

Open a material tab's menu, then double-click 'Remove attachment…' or hold Enter on it.

Resolve this comment. Triage will not finish while it is open.

};
const outputs =
visibleDeliverables.length > 0 || draftShown || conceptViews.length > 0;
const hasTabs = outputs || themes !== null;
Expand Down Expand Up @@ -619,7 +651,11 @@ export function ArtifactPane({
{tabMenu === null ? null : (
<Menu
open
onOpenChange={(open) => (open ? undefined : setTabMenu(null))}
onOpenChange={(open) => {
if (open) return;
setTabMenu(null);
setConfirmingRemove(false);
}}
>
<MenuTrigger asChild>
<span
Expand All @@ -644,6 +680,7 @@ export function ArtifactPane({
<Printer size={16} aria-hidden="true" />
{wording.printDocument}
</MenuItem>
{removeItem(tabMenu.value)}
</MenuContent>
</Menu>
)}
Expand Down
25 changes: 25 additions & 0 deletions apps/web/src/pages/workspace/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import { subscribeWorkflowRun } from "../../api/run-stream.ts";
import {
acceptedMaterials,
attachMaterials,
removeMaterial,
readArtifact,
} from "../../api/artifacts.ts";
import { documentsForPane } from "./document-read.ts";
Expand Down Expand Up @@ -797,6 +798,29 @@ export function StageWorkspace({
};
const chooseDocument = (id: string) =>
setDocumentChoice({ id, filedCount: filed.length });
// An attached file taken back: its tab goes with the next room read, and the person hears so.
const removeAttachment = (id: string) => {
if (!id.startsWith(MATERIAL_TAB)) return;
const artifactId = id.slice(MATERIAL_TAB.length);
const item = paneDeliverables.find((entry) => entry.kind === id);
void takeBack(artifactId, item?.title);
};
const takeBack = async (artifactId: string, title: string | undefined) => {
try {
const removed = await removeMaterial(projectId, artifactId);
setDocumentChoice(null);
toast.success(
wording.removeAttachment.done(
title === undefined ? removed.title : title,
removed.told,
),
);
reloadRef.current();
onChanged();
} catch (cause) {
toast.error(humanFault(cause, wording.removeAttachment.failed));
}
};
const openDocument = (id: string) => {
setPaneTab({ value: id });
if (conceptOfTab(id) === undefined) chooseDocument(id);
Expand Down Expand Up @@ -1595,6 +1619,7 @@ export function StageWorkspace({
}
: {})}
onSelect={chooseDocument}
onRemove={removeAttachment}
changesOpen={selected !== null && changesFor === selected.kind}
onToggleChanges={() =>
setChangesFor((current) =>
Expand Down
9 changes: 9 additions & 0 deletions packages/client/src/client/artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export function buildArtifactsMethods(
| "chooseDesign"
| "acceptedMaterials"
| "attachMaterials"
| "removeMaterial"
| "downloadPackageZip"
| "approvalRounds"
| "startApprovalRound"
Expand Down Expand Up @@ -169,6 +170,14 @@ export function buildArtifactsMethods(
signal,
);
},
removeMaterial(tenantId, projectId, artifactId, signal) {
return request(
`${projectPath(tenantId, projectId)}/materials/${encodeURIComponent(artifactId)}`,
signal,
null,
"DELETE",
);
},
async attachMaterials(tenantId, projectId, files, message, signal) {
const body = new FormData();
for (const file of files) body.append("file", file);
Expand Down
8 changes: 8 additions & 0 deletions packages/client/src/client/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import type {
InFlightRun,
LiveApp,
MaterialReceipt,
MaterialRemovedReceipt,
} from "@corbits-builder/schemas/artifact";
import type {
ApprovalRound,
Expand Down Expand Up @@ -544,6 +545,13 @@ export type ProductApiClient = Readonly<{
message?: string,
signal?: AbortSignal,
): Promise<MaterialReceipt>;
/** Takes an attached file back: the material is archived and the coordinator told. */
removeMaterial(
tenantId: string,
projectId: string,
artifactId: string,
signal?: AbortSignal,
): Promise<MaterialRemovedReceipt>;
/** A filed approval_package version as a deterministic zip of its deck, PDF, README, manifest and documents. */
downloadPackageZip(
tenantId: string,
Expand Down
10 changes: 10 additions & 0 deletions packages/schemas/src/artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,16 @@ export const MaterialReceipt = type({
});
export type MaterialReceipt = typeof MaterialReceipt.infer;

// A material the person took back: archived, so it leaves every list while its versions stay.
export const MaterialRemovedReceipt = type({
"+": "reject",
artifactId: "string > 0",
title: "string",
// Whether a running coordinator was told the file is withdrawn.
told: "boolean",
});
export type MaterialRemovedReceipt = typeof MaterialRemovedReceipt.infer;

// The attachment types the project's coordinator model reads, and Interchange's attachment limits.
// Empty when the model's catalog offering declares neither vision-input nor document-input.
export const AcceptedMaterials = type({
Expand Down