Repository navigation
feat: let the person remove an attached file from the project - #156
Conversation
A material stayed in the project for good. DELETE /projects/:projectId/materials/:artifactId archives it, so every list leaves it out while its versions stay, and tells the coordinator, when one runs, that the file is withdrawn.
The tab's menu, beside printing, takes the attached file back; the tab goes with the next room read and the person hears so.
|
Triage: In progress Triage is waiting for CI to finish before it decides. This comment shows the verdict once it has. |
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Waiting for CI
labels
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
| (runId, content) => | ||
| mailRunAsCaller(c, nativeFetch, current.tenantId, runId, content), |
There was a problem hiding this comment.
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.
| if (row.archivedAt === null) { | ||
| await setArtifactArchived(db, row, true); | ||
| const coordinator = await activeCoordinator(projectId); | ||
| if (coordinator) | ||
| await mailRun(coordinator.runId, materialRemovedNote(row.title)); |
There was a problem hiding this comment.
P2 · If the coordinator mail fails, the archive stays and a retry never sends it
The material is archived before the coordinator is mailed. If mailRun throws, the request fails but the material stays archived. On a retry, archivedAt is no longer null, so the call skips the mail and returns success. The coordinator is never told the file was withdrawn and may keep using it.
A coordinator is running and the mail to its run fails, for example because the run is unreachable or the call times out. The user then retries the removal.
Resolve this comment. Triage will not finish while it is open.
| done: (name: string) => | ||
| `${name} is removed; the coordinator has been told.`, |
There was a problem hiding this comment.
P2 · The success toast always says the coordinator was told, even when it wasn't
removeMaterial on the hub only mails the coordinator when one is running and the material wasn't already archived. The receipt it returns has no field saying whether anyone was mailed. The toast text is fixed, so it says "the coordinator has been told" every time, including when no one was.
Remove an attachment in a project with no active coordinator run, or remove one that is already archived (a no-op). The toast still says the coordinator was told.
Resolve this comment. Triage will not finish while it is open.
| {onRemove === undefined || !removable(tabMenu.value) ? null : ( | ||
| <MenuItem | ||
| className="menu-danger" | ||
| onSelect={() => onRemove(tabMenu.value)} |
There was a problem hiding this comment.
P2 · Remove attachment runs on the first click with no confirmation and no undo
Choosing the danger menu item immediately calls onRemove, which deletes the material. The file leaves the pane and the coordinator is told it was withdrawn, and the UI offers no undo or unarchive. Other destructive menu items in this codebase (project-menu Delete…) ask for confirmation first.
Right-click or Shift+F10 on a material tab and accidentally choose Remove attachment, for example by a misclick or by pressing Enter on the highlighted item.
Resolve this comment. Triage will not finish while it is open.
…and ask before removing A note the hub could not send left the material archived with the coordinator never told, and a retry did nothing; the note goes first now. The toast claimed the coordinator was told even with none running; the receipt says whether one was. Removing takes a second click in the menu, as deleting a project does.
…inator was told Removing takes a second click in the tab's menu, as deleting a project does, and the toast says the coordinator was told only when one was.
| return { | ||
| artifactId: row.id, | ||
| title: row.title, | ||
| told: coordinator !== null, |
There was a problem hiding this comment.
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.
| return { | ||
| artifactId: row.id, | ||
| title: row.title, | ||
| told: coordinator !== null, |
There was a problem hiding this comment.
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.
| 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> | ||
| ); |
There was a problem hiding this comment.
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.
Closes #155
What
A material's tab in the document pane offers Remove attachment, which takes the attached file back: it leaves the pane and the coordinator's filed materials, and the coordinator is told.
Why
A file attached to a message stayed in the project for good, with no way to take back a wrong file, a stale screenshot or a duplicate. Details in #155.
Changes
packages/schemas/src/artifact.ts:MaterialRemovedReceipt.apps/hub/src/artifacts/service.ts,routes.ts:removeMaterialandDELETE /projects/:projectId/materials/:artifactId. The material is archived through the artifact store's ownsetArtifactArchived, which every list already leaves out, so its versions stay; a material already archived is a no-op. When a coordinator runs, it is mailed that the file is withdrawn (materialRemovedNote). Manage authority on the project, as attaching needs.packages/client,apps/web/src/api/artifacts.ts:removeMaterial.apps/web/src/pages/workspace/artifact-pane.tsx: the tab menu (right-click, Shift+F10) gets a danger item for a material tab; the pane exports thematerial:tab prefix it keys on.apps/web/src/pages/workspace/index.tsx,config.ts: the page takes the file back, clears the choice so the pane falls to its default, reloads the room, and says so in a toast.The message the file was sent with keeps showing it, as history.