Connect the attachment cable end-to-end (fase 2b) #113

Closed
robbertbos wants to merge 2 commits from fase-2b-kabel into fase-2-bijlagen
Owner

Stacked on fase-2-bijlagen (#111). This connects the attachment "cable" that
fase 2a parked into place, then hardens it against an adversarial review pass.

What this does

Attachments now travel as parked refs (owner-scoped uuids) through draft,
immediate reply and scheduled send, and upload to Mattermost only at send-time,
which dodges MM's 7-day unattached-file expiry. The old
POST /cards/{id}/files route is retired in favour of the card-less
POST /outgoing-files; the reply route validates refs at request-time and again
at send-time. A daily orphan sweep reaps parked files no row references after a
7-day TTL, wired into the app lifespan and reported in get_task_statuses.

Review findings fixed (security / correctness / WCAG)

Three adversarial reviewers ran over the diff. The confirmed findings:

  • Draft attachments were silently destroyed on reload (two reviewers, the
    headline bug). The save side was wired but not the load side: reopening a
    draft showed no chips and the first keystroke PUT an empty file_refs list,
    which detached and reaped the bytes. useFileUpload.seedDone restores parked
    files as done entries and useServerDraftSync seeds lastSavedRefs from the
    server, so no empty list is ever sent.
  • File removed mid-upload leaked: the finished upload's ref is now deleted
    instead of lingering to the sweep and eating the user's quota.
  • Debounced draft-save is flushed on unmount: closing the card modal inside
    the 500ms window no longer drops an edit (notably an attachment removal that
    would otherwise leave the row pointing at deleted bytes).
  • Immediate reply now drops the server draft so its content and now-sent
    refs do not linger and reload later.
  • WCAG: upload state is announced in an aria-live region and marked with
    text, not colour alone; a failed attachment gives an actionable reason
    ("verwijder of probeer opnieuw") instead of the misleading "wait until uploads
    finish".
  • Upload response without a usable ref is treated as a failure.
  • _settle_sent deletes parked files only after STATE_SENT has committed;
    a crash in the old window turned a real send into a FAILED row whose bytes
    were already gone.
  • _delete_orphan_refs is scoped to the owner: refs are per-user-unique, so
    the tenant filter is now an explicit invariant rather than a bet on uuid
    uniqueness.
  • stat_ref/delete_ref fail closed on an ownerless sidecar (no
    None != None fall-through).
  • New sweep symbols exported from cleanup_runner.__all__.

Deferred with a note (out of this PR's scope): a pre-existing mm_file_id
memoisation that is dead in the scheduled-runner retry path (duplicate MM
uploads on transient failure), the near-unreachable orphan-sweep TOCTOU, and the
residual hard-tab-close window on removal.

Repo hygiene

Separate commit: docs/superpowers/ and artifacts/ are now git-ignored
(brainstorming specs, plans and their HTML companions are design scratch, not
source). The spec and fase-1 plan that an earlier git add -A swept into #111
are untracked (kept on disk).

Gates

  • Backend: 100% coverage, full suite green on SQLite; parity re-verified on
    Postgres for every touched module.
  • Frontend: tsc clean, vitest green (8 new tests for the fixes), production
    build ok, eslint 0 errors.
  • pre-commit run --all-files green (ruff 0.8.6, ruff-format, ty, secrets).
Stacked on `fase-2-bijlagen` (#111). This connects the attachment "cable" that fase 2a parked into place, then hardens it against an adversarial review pass. ## What this does Attachments now travel as parked **refs** (owner-scoped uuids) through draft, immediate reply and scheduled send, and upload to Mattermost only at send-time, which dodges MM's 7-day unattached-file expiry. The old `POST /cards/{id}/files` route is retired in favour of the card-less `POST /outgoing-files`; the reply route validates refs at request-time and again at send-time. A daily orphan sweep reaps parked files no row references after a 7-day TTL, wired into the app lifespan and reported in `get_task_statuses`. ## Review findings fixed (security / correctness / WCAG) Three adversarial reviewers ran over the diff. The confirmed findings: - **Draft attachments were silently destroyed on reload** (two reviewers, the headline bug). The save side was wired but not the load side: reopening a draft showed no chips and the first keystroke PUT an empty `file_refs` list, which detached and reaped the bytes. `useFileUpload.seedDone` restores parked files as done entries and `useServerDraftSync` seeds `lastSavedRefs` from the server, so no empty list is ever sent. - **File removed mid-upload leaked**: the finished upload's ref is now deleted instead of lingering to the sweep and eating the user's quota. - **Debounced draft-save is flushed on unmount**: closing the card modal inside the 500ms window no longer drops an edit (notably an attachment removal that would otherwise leave the row pointing at deleted bytes). - **Immediate reply now drops the server draft** so its content and now-sent refs do not linger and reload later. - **WCAG**: upload state is announced in an `aria-live` region and marked with text, not colour alone; a failed attachment gives an actionable reason ("verwijder of probeer opnieuw") instead of the misleading "wait until uploads finish". - **Upload response without a usable ref** is treated as a failure. - **`_settle_sent` deletes parked files only after `STATE_SENT` has committed**; a crash in the old window turned a real send into a FAILED row whose bytes were already gone. - **`_delete_orphan_refs` is scoped to the owner**: refs are per-user-unique, so the tenant filter is now an explicit invariant rather than a bet on uuid uniqueness. - **`stat_ref`/`delete_ref` fail closed on an ownerless sidecar** (no `None != None` fall-through). - New sweep symbols exported from `cleanup_runner.__all__`. Deferred with a note (out of this PR's scope): a pre-existing `mm_file_id` memoisation that is dead in the scheduled-runner retry path (duplicate MM uploads on transient failure), the near-unreachable orphan-sweep TOCTOU, and the residual hard-tab-close window on removal. ## Repo hygiene Separate commit: `docs/superpowers/` and `artifacts/` are now git-ignored (brainstorming specs, plans and their HTML companions are design scratch, not source). The spec and fase-1 plan that an earlier `git add -A` swept into #111 are untracked (kept on disk). ## Gates - Backend: 100% coverage, full suite green on SQLite; parity re-verified on Postgres for every touched module. - Frontend: `tsc` clean, vitest green (8 new tests for the fixes), production build ok, eslint 0 errors. - `pre-commit run --all-files` green (ruff 0.8.6, ruff-format, ty, secrets).
The spec and the fase-1 plan were swept into an earlier commit by a broad
`git add -A`; they are design scratch, not source. Ignore docs/superpowers/
and artifacts/ so brainstorming specs, implementation plans and their HTML
companions stay on disk, and untrack the two that slipped in (kept on disk).
Connect the attachment cable end-to-end and harden it
All checks were successful
CI / pre-commit (pull_request) Successful in 1m14s
CI / frontend-test (pull_request) Successful in 4m39s
CI / release-scripts (pull_request) Successful in 10s
CI / backend-test (pull_request) Successful in 6m41s
CI / backend-test-postgres (pull_request) Successful in 8m46s
CI / e2e (pull_request) Successful in 9m52s
266907a538
Attachments now travel as parked refs (owned uuids) through draft, immediate
reply and scheduled send, and upload to Mattermost only at send-time, dodging
MM's 7-day unattached-file expiry. The old POST /cards/{id}/files route is
retired in favour of the card-less POST /outgoing-files; the reply route
validates refs at request-time and again at send-time. A daily orphan sweep
reaps parked files no row references after a 7-day TTL, wired into the
lifespan and reported in get_task_statuses.

Fixes from the adversarial review pass (security / correctness / WCAG):

- Draft attachments now rehydrate on load. The save side was wired but not the
  load side, so reopening a draft showed no chips and the first keystroke PUT an
  empty file_refs list, detaching and reaping the bytes. useFileUpload.seedDone
  restores parked files as done entries and useServerDraftSync seeds
  lastSavedRefs from the server so no empty list is ever sent.
- A file removed mid-upload no longer leaks: the finished upload's ref is
  deleted instead of lingering to the sweep and eating the user's quota.
- A pending draft-save is flushed on unmount, so closing the card modal inside
  the 500ms debounce no longer drops an edit (notably an attachment removal).
- An immediate reply now drops the server draft, so its content and now-sent
  refs do not linger and reload later.
- Upload failures are announced in an aria-live region and marked with text, not
  colour alone; a failed attachment gives an actionable reason instead of the
  misleading "wait until uploads finish".
- An upload response without a usable ref is treated as a failure.
- scheduled_runner._settle_sent deletes parked files only after STATE_SENT has
  committed; a crash in the old window turned a real send into a FAILED row
  whose bytes were already gone.
- _delete_orphan_refs is scoped to the owner: refs are per-user-unique, so the
  tenant filter is now an explicit invariant, not a bet on uuid uniqueness.
- outgoing_files stat_ref/delete_ref fail closed on an ownerless sidecar
  (no `None != None` fall-through).
- The new sweep symbols are exported from cleanup_runner.__all__.

Backend stays at 100% coverage; parity verified on Postgres.
robbertbos closed this pull request 2026-07-19 20:31:59 +00:00
All checks were successful
CI / pre-commit (pull_request) Successful in 1m14s
CI / frontend-test (pull_request) Successful in 4m39s
CI / release-scripts (pull_request) Successful in 10s
CI / backend-test (pull_request) Successful in 6m41s
CI / backend-test-postgres (pull_request) Successful in 8m46s
CI / e2e (pull_request) Successful in 9m52s

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
robbertbos/waggle!113
No description provided.