Repository navigation
feat: let the person remove an attached file from the project #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3cab68b
493deb1
0f08437
aab0c0a
5cf3e1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import { | |
| listArtifacts, | ||
| listArtifactVersions, | ||
| MAX_LIST_LIMIT, | ||
| setArtifactArchived, | ||
| VersionConflictError, | ||
| writeArtifactVersion, | ||
| type ArtifactDb, | ||
|
|
@@ -54,6 +55,7 @@ import { | |
| type AcceptedMaterials, | ||
| type UserManual, | ||
| type MaterialReceipt, | ||
| MaterialRemovedReceipt, | ||
| type DesignTweaksReceipt, | ||
| type DesignChoiceReceipt, | ||
| type LiveApp, | ||
|
|
@@ -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, | ||
|
|
@@ -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, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 ·
Remove a material from a project that has no active coordinator: the response has Resolve this comment. Triage will not finish while it is open. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 ·
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( | ||
|
|
||
| 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, | ||
|
|
@@ -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); | ||
|
|
@@ -249,6 +252,7 @@ export function ArtifactPane({ | |
| selected, | ||
| state, | ||
| onSelect, | ||
| onRemove, | ||
| paneRef, | ||
| changesOpen, | ||
| onToggleChanges, | ||
|
|
@@ -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; | ||
|
|
@@ -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) => ({ | ||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
@@ -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 | ||
|
|
@@ -644,6 +680,7 @@ export function ArtifactPane({ | |
| <Printer size={16} aria-hidden="true" /> | ||
| {wording.printDocument} | ||
| </MenuItem> | ||
| {removeItem(tabMenu.value)} | ||
| </MenuContent> | ||
| </Menu> | ||
| )} | ||
|
|
||
There was a problem hiding this comment.
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.