PR #8.5: attachment remove button - #11
Merged
Merged
Conversation
Wires the vault's DELETE /api/notes/:id/attachments/:attId endpoint
(shipped in parachute-vault#128) into the editor's attachment sidebar.
- VaultClient.deleteAttachment(noteId, attId) returns void on 204.
- VaultNotFoundError thrown from request() on 404, letting callers
treat 404 semantically ("already removed") rather than matching on
status strings.
- useDeleteAttachment mutation invalidates the note query on success so
the attachment list refreshes.
- RemoveAttachmentButton renders a small ✕ per attachment, opens a
confirm dialog, and arms the Remove button after 250ms so an
accidental double-click on the trigger doesn't click straight
through. Cancel gets initial focus; Escape closes.
- On 404 the dialog closes and toasts "Already removed <filename>"
rather than surfacing an error — the attachment is already gone
either way.
- On 401 the dialog stays open with a reconnect prompt.
Out of scope: broken-inline-rendering when markdown references a
removed attachment. That's a separate polish PR; the renderer shows a
broken image link until the markdown is edited.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
unforced
added a commit
that referenced
this pull request
May 12, 2026
… items After Aaron read the audit, he gave directional input that the doc should capture durably: - New "North star" subsection up top: Notes should serve as an Apple-Notes / Obsidian replacement. Three pillars — easy as Apple Notes, flexible as Obsidian-ish, Parachute-shaped via per-vault customization. Every improvement candidate now reads against this question: "does this make Notes a credible Apple-Notes replacement?" - §3 item #11: view-level text-size control. A device-local zoom knob (Default / Larger / Largest) affecting editor + read views. View preference, not content — markdown on disk stays untouched. Matches Apple Notes' affordance. - §3 item #12: unify capture surfaces. Collapse the /new + TextCapture + MemoCapture duality into one create-or-edit interface where the "quick" experience is the same surface with smart defaults (auto-title, role-driven tags, default path). Voice still uses its own input modality but hands off to the unified surface. - §5 sequence reordered: unified-capture + text-size now sit at positions 2 and 3, right after the vault popover. Authoring ease outranks chrome refinement because someone migrating from Apple Notes judges the app by day-one typing feel. No code, just the doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
unforced
added a commit
that referenced
this pull request
May 12, 2026
* docs(design): Notes UI audit + vault-selector design proposal Captures the current state inventory (twenty routes, three nav primitives, where per-vault state is implicitly singular) and proposes a vault popover that fronts the hub's `/.well-known/parachute.json` vault list to fix the multi-vault-on-one-hub gap surfaced when Aaron dogfooded the new vault MCP install across four vaults. Surfaces ten broader UI improvement candidates with scope/leverage reads, engages with the surface-direction research note (parachute-patterns#54) on how Notes might evolve as a configured surface instance, and recommends a sequencing for the broader audit. No code changes. Doc-only PR — skips RC per patterns/governance.md rule 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(design): fold reviewer fact-checks on Notes UI audit Five citation/accuracy nits from #114 reviewer pass: - §2 well-known.ts line range corrected: WellKnownVaultEntry is at :11-22, not :35-54 (which was WellKnownServicesEntry). - §2 CORS framing: same-origin in standard installs (Notes at /notes/ on the hub origin); CORS-open header covers cross-origin deployments only. - §1 getActiveVault count anchored: 17 direct call sites + broader useVaultStore touched across dozens of components. The "113" figure was a rough estimate that would've gaslit a future implementer. - §2 beginOAuth reframe: authorize URL is built last after DCR + metadata discovery, so the vault-hint params bag is URL-decoration at the last step, not a structural change. Cheaper than the original framing implied. - §4 LensSettings rename residue: flagged the coordinated migration (hook → type → stored-key). The legacy lens storage path fallback is intentional and stays — only the hook surface renames. Plus open question added in §2: hub-origin discovery from VaultRecord. The popover needs to fetch /.well-known/parachute.json against a hub URL, but VaultRecord stores it as `issuer` (OAuth-time) without a clean "hub origin" surface. Design call deferred to Phase 2 PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(design): absorb Aaron's north-star framing + add two improvement items After Aaron read the audit, he gave directional input that the doc should capture durably: - New "North star" subsection up top: Notes should serve as an Apple-Notes / Obsidian replacement. Three pillars — easy as Apple Notes, flexible as Obsidian-ish, Parachute-shaped via per-vault customization. Every improvement candidate now reads against this question: "does this make Notes a credible Apple-Notes replacement?" - §3 item #11: view-level text-size control. A device-local zoom knob (Default / Larger / Largest) affecting editor + read views. View preference, not content — markdown on disk stays untouched. Matches Apple Notes' affordance. - §3 item #12: unify capture surfaces. Collapse the /new + TextCapture + MemoCapture duality into one create-or-edit interface where the "quick" experience is the same surface with smart defaults (auto-title, role-driven tags, default path). Voice still uses its own input modality but hands off to the unified surface. - §5 sequence reordered: unified-capture + text-size now sit at positions 2 and 3, right after the vault popover. Authoring ease outranks chrome refinement because someone migrating from Apple Notes judges the app by day-one typing feel. No code, just the doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 12, 2026
unforced
added a commit
that referenced
this pull request
May 13, 2026
….15-rc.6) (#123) * feat(ui): unified capture surface + view-level text-size control (0.3.15-rc.6) Two items from design/2026-05-12-notes-ui-audit.md §3 bundled per the audit's sequencing note: #12 (unify capture) + #11 (view text-size). Both north-star "Apple-Notes-grade ease" items. ### Unified capture surface (audit §3 item #12) Adds polish to Capture; preserves NoteNew at /new for the file-drop attachment-on-create path so this PR ships clean without losing the working attachment flow. Capture is the canonical quick-path; NoteNew remains the heavy-editor escape hatch one click away. - **More fields panel** — collapsible <details> in Capture exposing a path override + summary input. Closed by default (textarea stays the no-friction focus); operators who need to set an explicit path (e.g. capturing into "Daily/2026-05-12") get the form without leaving Capture. Empty path = "vault auto-assigns"; empty summary = "no metadata.summary". Path override wins over the audio-only memo auto-path. - **Inactivity autosave** — fires save() after 5 seconds of editing inactivity, in addition to the existing unmount-flush. Skipped while audio is staged (manual Capture click only), while recording/saving, and while body is empty. Hardcoded 5s per brief. - **Escape hatch** — "Need to attach a file? Open the full editor" link in the More-fields panel points at /new (NoteNew, kept). Cmd+K keeps both "New note" + "Capture" entries for discoverability. ### View-level text-size control (audit §3 item #11) - **lib/text-size.ts** — mirrors lib/theme.ts shape. Three steps (Default / Larger / Largest); per-device localStorage at `notes:textSize`; applied via `data-text-size` attribute on <html>. - **styles/index.css** — `--font-size-prose` + `--font-size-editor` CSS variables on :root with two `:root[data-text-size="…"]` overrides. `.prose-note` reads the prose one; CodeMirror reads the editor one (its hardcoded `fontSize: "14px"` swapped for `var(--font-size-editor, 14px)`). - **Settings TextSizeSection** — three radio buttons; applies + persists in one motion. Markdown on disk unaffected — pure view preference. - **App.tsx** — applies the stored size on mount so the preference takes effect on every route, not just Settings. ### Tests - 4 new in text-size.test.ts (round-trip, default handling, data-attribute application, labels). - 8 new in Capture.test.tsx (disclosure closed by default; path + summary override payload shape; empty path → no `path` in payload; path override wins over audio memo auto-path; autosave-after-5s; edit-resets-timer; empty content no-autosave; audio-staged suppresses autosave). ### Hardening - Unmount-flush enqueue now swallows IDB teardown rejections — the SyncProvider closes its handle in the same tick as Capture's unmount-flush, documented race in SyncProvider.tsx:60. No user-visible surface to report failures during nav-away anyway. ### Follow-ups not in this PR - Attachments in Capture's quick flow (file-drop staging against the enqueue path) — currently only via /new. Will file as separate issue. - captureText role tag applied in NoteNew (today only Capture applies the role). Small consistency win — separate issue. Gates: 80 test files / 722 tests pass (+12 from this PR; was 710 in #122). Typecheck clean, lint clean, build green (1561 KiB precache). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(capture): release savingRef on successful save; fold reviewer nits Reviewer F1 (critical, data-loss) — savingRef was never reset after a successful save. With autosave, the user stays mounted after a save, so: - The next autosave timer's `if (savingRef.current) return` always bailed. - The unmount-flush's same guard always bailed. Net effect: type thought 1, autosave fires + succeeds, type thought 2, close tab → thought 2 silently lost. Fix: one line after `reset()` in the success path. Pre-autosave the manual Capture click was the only entry point and a fresh mount handled the reset implicitly; with the 5s timer the mount is reused across saves. Two regression tests: - "second autosave after first fires (savingRef releases on success)" asserts two distinct saves land in the queue from one mount. - "unmount-flush after a successful autosave still flushes new typed content" asserts post-autosave drafts aren't dropped on nav-away. Uses the existing Toggler pattern so the SyncProvider's IDB handle stays open while the unmount-flush enqueue runs. Reviewer F2 (polish) — add `@vitest-environment jsdom` pragma to text-size.test.ts. The vitest config already defaults to jsdom but the pragma makes the dependency explicit for stray `bun test` invocations. Reviewer F3 (polish) — Settings TextSizeSection used useEffect to read localStorage after the first render, causing a visible flash from "Default" to the stored value. Switched to a lazy initializer so the correct value is rendered on first paint. Reviewer F4 (skipped) — the <details>/<summary> test pattern is already documented inline. No change needed. Gates: 80 test files / 724 tests pass (+2 from F1 regression tests), typecheck clean, lint clean, build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wires the vault's
DELETE /api/notes/:id/attachments/:attIdendpoint (shipped in parachute-vault#128) into the editor's attachment sidebar.Summary
VaultClient.deleteAttachment(noteId, attId)returns void on 204.VaultNotFoundErrorthrown fromrequest()on 404 so callers can branch on it structurally instead of parsing message strings. No existing caller relies on 404 (getNote uses the list endpoint which returns[]); verified via full test suite.useDeleteAttachmentinvalidates the["note", activeVaultId, noteId]query on success so the sidebar refreshes.RemoveAttachmentButtonrenders a small ✕ on each attachment row, opens a confirm dialog, and arms the Remove button after 250ms (prevents accidental double-click-through from the trigger). Cancel gets initial focus; Escape closes; click-outside closes.401→ dialog stays open, "Session expired. Reconnect to remove attachments."404→ dialog closes, info toast "Already removed<filename>" (the attachment is gone either way)Confirm pattern rationale
Team-lead suggested "click-to-confirm or type-to-confirm; typing the attachment filename is overkill for attachments." Chose click-to-confirm with a short arm delay. Typing the filename is the right gate for note deletion (catastrophic), but attachments are recoverable by re-upload and a delayed arm plus initial-focus-on-Cancel is enough friction.
Out of scope
Markdown that references a now-deleted attachment will show a broken-image link until the markdown itself is edited. Fixing this needs either a vault-side "attachment gone" response or a client-side ghost-attachment index; worth a separate polish PR.
Test plan
bun run lintcleanbun run typecheckcleanbun run test— 112 tests pass (8 new)bun run buildsucceeds🤖 Generated with Claude Code