Skip to content

feat(snippets): Snippets & Command History in the terminal context menu - #70

Merged
rockyway merged 9 commits into
developfrom
feature/snippets
Sep 4, 2026
Merged

feat(snippets): Snippets & Command History in the terminal context menu#70
rockyway merged 9 commits into
developfrom
feature/snippets

Conversation

@rockyway

@rockyway rockyway commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Implements docs/plan/029-snippets-and-command-history-menu.md (private, in termflow-fabric). Requirement was written as "Quick Inserts"; the shipped name is Snippets — iTerm2 names this exact feature that, and TermFlow already has a shortcuts category for keyboard bindings.

Right-click a terminal → Command History ▸ and Snippets ▸, each a flyout with a search box. Ctrl+Shift+S opens the Snippets list on its own, at the cursor. Snippets are user-managed (folders, tags, full CRUD in Settings, versioned import/export) and survive restart.

What to look at first

Insertion reuses the existing primitive. pasteToTerminal routes through the mounted xterm, so bracketed-paste markers are applied when the foreground app set DECSET 2004, and CRLF→CR is normalized. That one call is why multi-line text lands as a literal block in Claude Code/Gemini instead of executing line-by-line, why it works inside a TUI as well as at a bare prompt, and why nothing ever appends Enter. InputHandler.handlePasteText's body was extracted into insertTextIntoTerminal so clipboard paste, snippets and history all share one entry point and no future caller can skip the raw-write fallback.

The flyout renders inside the menu container, not portalled. ContextMenu closes on any mousedown where !menuRef.contains(target), so a portalled panel reads as "outside" and the first click on any row would close the menu before the click landed. Verified by mutation: wrapping the panel in createPortal turns three tests red.

Import/export is two narrow Tauri commands, not one general file-write. A general write_text_file would become an arbitrary-write primitive the moment someone wired it into a route, and the embedded Axum API and MCP sidecar must not gain that. A Rust test asserts neither command appears in api_server.rs, so the comment claiming it can't rot into a lie. Needed a new dialog:allow-save capability — the app had only dialog:allow-open and no fs plugin.

History does not use engine.insertCommand(). That erases the whole typed input line first, which is correct for the Shift+Enter popup completing what you're typing and destructive from a right-click. The popup itself is untouched; a test asserts insertCommand is unreachable from either flyout.

Round 2 — external review (agy)

Five defects, seven weak oracles, one unsupported comment. All fixed at the class level, all mutation-checked.

The headline one was a data-loss bug this document's own spec had talked itself into. The persist path snapshotted with state.snippets.map(s => ({ ...s })). A shallow spread copies tags by reference, leaving a revoked Immer proxy inside an otherwise-plain object — so one snippet carrying tags (an empty array is enough) makes the whole array unstringifiable, and the symptom is losing the entire library on restart. Five green tests missed it because the fixture omitted the one field that triggers it. Now one persistSnippets helper using current().

One test was actively holding the bug in place. The source-derived wiring test pinned the literal buggy expression, so fixing the code would have turned it red. It now pins the invariant — exactly one write site, reached through the helper, with a deep unwrap — instead of today's spelling of it.

Also fixed: edit could not clear a label/folder/tags; NaN/Infinity timestamps broke sort transitivity; the depth-1 flyout was clipped invisible by its own scroll container; and a flipped parent's child cascaded back over the root menu.

Round 3 — Tam's GUI pass

Seven changes, all from driving the real app.

Reported Fix
Add-snippet dialog closes on an outside click Overlay handler removed. ConfirmDialog dismisses that way and should — it holds a question; this holds a form, and a click landing beside the panel while reaching for the Folder field threw away everything typed. Escape / Cancel / ✕ remain.
Insert works but the terminal loses focus Every terminal menu now refocuses on close, via one shared refocusTerminal. A property of the menu, not of the snippet row that exposed it — all four menus plus the dialog route through it.
No shortcut Ctrl+Shift+S, at the cursor. Ctrl+Shift because bare Ctrl+S is XOFF and would freeze the terminal.
Submenu should open on hover Hover opens; a click on the parent now only ever opens, never toggles — hover has already opened the panel by the time a click lands, so toggling would make clicking what you're pointing at the gesture that hides it. ~260ms grace close covers the diagonal into the panel.
Want a flat/folders view toggle, flat by default Toggle beside the search box. Persisted as its own nine-link setting — the flyout is rebuilt on every open, so component state would have reset it silently each time.
Folder and tag are cut off Panel widened to 380–640px, and the chip carries its own tooltip (Folder: … / Tags: …, one per line). The row's title is the snippet text, so a single row-wide tooltip could not serve both.
Narrow the folder-mode menu, keep the submenu wide Root panel 200–300px in folder mode; the folder panel it opens keeps full width. narrow is depth-0 only and stripped on the way down, like headerToggle.
Shortcut shouldn't show the parent row standaloneSubmenu draws the flyout alone. Escape/Tab then dismiss the whole thing — retiring just the panel would leave an invisible box still eating the next outside click.

One bug five green tests could not see. Stripping the standalone menu's chrome unset every paint property and left min-width: 200px behind; the empty host stretched to it, so the shortcut's panel rendered 200px right of the cursor. jsdom computes no layout. Pinned now by reading the CSS, the way the nested-panel clipping already is.

Settings category registration

Adding a category touches these sites in SettingsPage.tsx — listed by name rather than as a count, since a count goes stale the moment the class widens:

SettingsCategory union · CATEGORY_LABELS · isTracked() · the categories array · the duplicate union inside the deep-link isCategory() guard · the renderActiveCategory() switch · the requestCategoryChange dirty-guard hook-up.

settingsCategories.test.ts asserts the derivable sets are equal, so a stray extra entry fails as well as a missing one. The deep-link union is a second hand-maintained copy and is the one that drifts.

isTracked() is an exclusion list, so a new category is dirty-tracked by default. Snippets persists per mutation and has no Save button, so it's excluded — left alone, snapshotCategory() falls off the end of an exhaustive switch and returns undefined at runtime while type-checking clean.

Merged with develop

feature/terminal-automations (plan/028) landed first and added an automations category to the same sites. Resolved as predicted — three hunks, all "both branches added a member to the same list", nothing behavioural. Full suite green after the merge.

Verification

Suite Result
bun jest (renderer) 3530 passed, 206 suites
bun run test:workspace (terminal-core) 777 passed, 48 suites
cargo test 754 passed
bun run typecheck clean

GUI-signed-off by Tam — the gap called out as a merge blocker in round 2 is closed.

Mutation-checked rather than assumed, across all three rounds: the flyout's non-portal rendering, Escape's stopPropagation, the SnippetDialog re-seed effect, the snippets useSelector binding, the persistSnippets deep snapshot, and all twelve round-3 guards. Two of those twelve survived the first mutation pass and were rewritten — a builder-level assert that never reached the DOM, and a lazy [\s\S]*? source regex that matched a different focus() call in the same file.

Unrelated drive-by: sessionExitPersistence asserted a JSON blob does not contain '137' against a blob carrying a live Date.now(), which fails at random. Oracle narrowed to exclude the timestamp.

tamtranthien and others added 6 commits September 4, 2026 00:25
…dit modal

Foundation for plan/029 — Snippets in the terminal context menu.

Insertion gets ONE entry point. `insertTextIntoTerminal` lifts the
pasteToTerminal-then-raw-write-fallback out of InputHandler.handlePasteText,
so snippets, command history and clipboard paste all route through the same
helper and no future caller can silently skip the fallback. Terminal
resolution stays in InputHandler, which is the only caller that needs it.

The `snippets` settings chain is wired end to end: type, default, five
reducers that snapshot to a plain array before persisting (a live Immer
draft's proxy is revoked by the time the async setConfigValue runs, and
updateConfig swallows the throw, so the value would never reach disk), the
action-creator exports, and the App.tsx hydrate link with per-entry
validation so one hand-edited bad record cannot drop the whole library.
Rust needs no change — merge_config is a serde_json::Value passthrough.

snippetSearch is deliberately pure — no DOM, no Redux — so #tag parsing,
ranking and the derived folder/tag vocabularies can be tested as tables.
commandHistoryService gains recent(), because match('') returns [] by design
and a browse list with no query typed had no code path at all.

SnippetDialog stays presentational: it hands a draft back through onSave and
never dispatches, which is what lets both the Settings panel and the context
menu open the same modal.

463 tests green across the touched suites; typecheck clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019H9RyYckBpTv4ANZ6dSrwz
plan/029 §4. No flyout submenu and no arrow-key menu navigation existed
anywhere in this codebase, so both are new here rather than reused.

The flyout renders INSIDE the menu container rather than portalled to
document.body, and that is load-bearing: ContextMenu closes on any mousedown
where !menuRef.contains(target), so a portalled panel reads as "outside" and
the first click on any row would close the menu before the click landed.
Verified by mutation — wrapping the panel in createPortal turns three tests
red. Escape's stopPropagation is pinned the same way; removing it turns two
red.

A submenu parent toggles its flyout instead of running the shared
`item.click?.(); onClose();` path, so opening a submenu no longer dismisses
the menu it belongs to.

The panel is absolutely positioned, so it contributes nothing to the menu's
intrinsic width and the existing edge-aware repositioning math and
.context-menu-label nowrap behaviour are untouched. It flips to the left only
when the right edge is nearer than the panel is wide AND the left side
actually fits. The list scrolls, not the panel, so the search box stays
pinned.

ContextMenu.css had no :focus-visible rule at all, so keyboard navigation
would have been invisible; an explicit active-row style is added.

31 tests; the three existing ContextMenu call sites in TerminalDisplay
(main menu, path picker, agent colour-scheme picker) still pass — 295 tests
across 26 suites in the surrounding areas.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019H9RyYckBpTv4ANZ6dSrwz
plan/029 §8. The app had `dialog:allow-open` but no `dialog:allow-save`, and
no fs plugin or file-write command at all, so both had to be added.

They are deliberately two purpose-built commands rather than one general
write_text_file. A general one would become an arbitrary-write primitive the
moment anyone wired it into a route, and the embedded Axum API and the MCP
sidecar must not gain that. A Rust test pins that neither command appears in
api_server.rs, so the comment saying so cannot rot into a lie.

Both guard the extension — export is the write, and therefore the dangerous
half. The read caps at 5 MB using take(max+1) rather than trusting
metadata().len(), so a file that grows between stat and read cannot slip past.

Import merges rather than replaces: fresh ids are minted (a file from another
machine can collide with a local id), exact-text duplicates are skipped, and a
malformed record is dropped and counted instead of failing the whole file.
Within-file duplicates count as skipped too — otherwise a file repeating
itself would manufacture a duplicate the user never had, and it is what makes
imported + skipped + rejected === entries hold exactly. That identity is
asserted.

Cancelling is not a failure, and `'cancelled'` is truthy, so callers must test
it before `ok` — documented on the type, because a bare `if (r.ok)` reads a
cancellation as success.

Browser-bridge stubs throw rather than returning null: null from a picker
already means "user cancelled", so a quiet stub would report a cancellation
that never happened.

9 Rust tests (plain #[cfg(test)], so every platform's CI runs them) and 21
renderer tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019H9RyYckBpTv4ANZ6dSrwz
plan/029 §7. Snippets becomes an 11th Settings category rendered by an
extracted SnippetsPanel — the PeersPanel precedent, taken because
SettingsPage.tsx is already 1975 lines and this adds a grouped list, a folder
rename affordance, a tag display and an import/export row.

Registering a category touches SEVEN enumeration sites, including a second,
hand-maintained copy of the union inside the deep-link isCategory() guard. A
missed site is a silently dead category, so settingsCategories.test.ts asserts
the four derivable sets are EQUAL rather than merely that 'snippets' appears
somewhere — that way a stray extra entry fails too.

isTracked() is an EXCLUSION list, so a new category is dirty-tracked by
default; snippets persists per mutation and has no Save button, so it is
excluded. Left alone, snapshotCategory() would fall off the end of an
exhaustive switch and return undefined at runtime while type-checking clean.

Site 7 needed no work: the add/edit UI is a focus-trapped modal, so there is
no inline draft for a category switch to silently discard.

Also fixes SnippetDialog, found during this integration. It seeded its fields
only in useState initializers, and callers render it unconditionally — isOpen
just chooses between null and the portal — so it never remounted and the
second open reused the first open's state: "Edit snippet" over blank fields.
Fixed IN THE DIALOG rather than with a remount key at the call site, because
there are two callers and keying one leaves the other to rediscover the same
bug. Keyed on snippet?.id, not snippet, so a parent re-render producing a new
object for the same snippet cannot wipe out in-progress typing. Verified by
mutation: disabling the effect turns two tests red.

136 tests across Settings and UI; typecheck clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019H9RyYckBpTv4ANZ6dSrwz
…menu

plan/029 §5 and §6. Command History sits above Snippets, both ungated so they
work while a TUI or interactive CLI owns the screen — the write path does not
care what is running.

Insertion goes through insertTextIntoTerminal with THIS pane's terminalId, not
resolveKeyboardTerminalId: that helper exists for global shortcuts with no DOM
target, and a right-click already names its pane.

History deliberately does not use engine.insertCommand(). That method erases
the entire typed input line before writing, which is right for the Shift+Enter
popup completing what you are typing and destructive from a right-click, where
the user did not ask for their line to be cleared. The popup itself is
untouched — preserving it is a stated requirement, and a test asserts
insertCommand is never reached from either flyout.

Menu construction lives in a pure module rather than inline in TerminalDisplay
so folder-grouping, flatten-on-search and the empty states are testable without
mounting a component that needs WebGL.

The list is read through a live useSelector. That binding is mutation-checked:
replacing it with an empty literal turns a test red. Type-safety cannot catch
that on its own, because a literal of the right type is a legal argument.

17 new tests; full suite 3145 passing across 184 suites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019H9RyYckBpTv4ANZ6dSrwz
Five genuine defects from the external review, all fixed at the class.

PERSISTENCE (D-01, the blocker). `state.snippets.map(s => ({ ...s }))` is not
a plain copy: it copies `tags` BY REFERENCE, and that reference is a child
Immer draft whose proxy is revoked when the reducer returns. setConfigValue
serialises asynchronously and throws there; updateConfig swallows it, so
nothing reaches config.json.

Measured, not argued. updateSnippet, renameSnippetFolder and removeSnippet
each produce an unstringifiable snapshot when any snippet carries tags — an
EMPTY array is enough. addSnippet is safe only because a pushed action payload
is never drafted. And the list is one payload, so a single tagged snippet
takes every untagged sibling down with it: the real symptom was losing the
whole library on restart.

The five call sites are now one `persistSnippets` helper using `current()`,
so a future reducer cannot reinvent a shallow snapshot. The comment it carries
is the measurement, not the theory — the previous one asserted the shallow
spread solved a problem it did not.

Swept the class rather than the instance: agentColorSchemes and
customKeybindings are Record<string,string>, so their shallow spreads are
genuinely safe, and shellProfiles persists from selector state, which is
already finalised. snippets was the only member.

EDIT (D-04). buildDraft omitted empty fields and updateSnippet merged with
Object.assign, so an omitted key kept its old value — a user could not remove
a label, unfile a snippet, or drop its tags. The draft now sends explicit
undefined and the reducer treats undefined as "clear", restricted to the
optional keys so a stray undefined cannot corrupt id/text/createdAt.

FLYOUT (D-02, D-03). A nested panel inside `.context-menu-flyout-list` is
clipped: overflow-y:auto forces overflow-x to compute to at least auto. The
depth-1 folder panel was invisible in any real engine while 31 jsdom tests
passed, because jsdom implements no layout. It now renders as a sibling of the
list inside the parent panel — still inside menuRef by construction, so the
outside-click trap is not reintroduced, and the list stays scrollable.
Flip direction now cascades: measuring only against the parent's right edge
meant depth 1 rendered back over the root menu once depth 0 flipped left.

ALSO: non-finite timestamps passed `typeof === 'number'` and broke sort
transitivity (both sites); clearing a folder name could not unfile a folder
though the reducer supported it.

TESTS. The reason five green tests missed D-01 is that the fixture had no
tags key, and the source-derived wiring test pinned the literal buggy
expression — so fixing the code would have turned it red. Fixtures now carry
tags, the oracle is "does the snapshot still serialise after the reducer
returns" rather than "is it an array", and the wiring test pins the invariant
(one write site, a deep snapshot, and the old expression explicitly forbidden)
instead of today's spelling. Seven tests go red if the shallow spread returns.
Six other oracles that asserted only Array.isArray, ok === false or
not.toContain now assert the actual expected value.

3164 renderer tests, 9 Rust tests, typecheck clean.

NOT verified: jsdom cannot observe CSS clipping or real placement, so D-02 and
D-03 are pinned structurally and against a rect simulator. A WebView2 pass is
still required.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019H9RyYckBpTv4ANZ6dSrwz
@rockyway

rockyway commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-1 review response — all 5 D findings fixed (c3733a9)

Thanks — this round earned its keep. Summary of what changed and, where it matters, what I measured rather than assumed.

D-01 (blocker) — confirmed, and broader than reported

I probed it rather than reasoning about it. My first probe was too weak: it used addSnippet, whose action payload Immer never drafts, so nothing could have been revoked. Probing reducers that read pre-existing state:

Snippet shape Persisted snapshot
has tags unstringifiable
has tags: [] (empty) unstringifiable
no tags key fine
one tagged + one untagged whole list unstringifiable

So the user-visible symptom isn't "tagged snippets don't save" — a single tagged snippet loses the entire library on restart, because the array is one payload. addSnippet and setSnippets were genuinely never broken.

Fixed at the class: the five call sites are now one persistSnippets helper using current(). Swept the rest of the slice for the same shape — agentColorSchemes and customKeybindings are Record<string,string> so their shallow spreads are safe, and shellProfiles persists from selector state which is already finalised. snippets was the only member.

D-04, D-05, A-01

Draft now sends explicit undefined and updateSnippet treats that as "clear", restricted to the optional keys so a stray undefined can't corrupt id/text/createdAt. Number.isFinite at both timestamp sites. Rename guard no longer blocks unfiling.

D-02 / D-03

Nested panel is now a sibling of .context-menu-flyout-list inside the parent panel — outside the clip, still inside menuRef by construction rather than by coordinate math, and the list stays scrollable. Flip direction cascades via a threaded parentFlippedLeft, still && fitsLeft so the left edge stops it.

On the B findings — you were right about the mechanism, not just the instances

B-07 is the one worth calling out. The wiring test pinned the literal state.snippets.map((s) => ({ ...s })), so it pinned the bug in place — correcting the code would have turned that test red. It now pins the invariant (one write site, a deep snapshot, and the old expression explicitly forbidden) rather than today's spelling.

B-03 explains why five green tests missed a data-loss bug: the fixture had no tags key, so the suite was blind to the only field that triggers it. Fixtures now carry tags, and the oracle is "does this still serialise after the reducer returns" — which is what invoke actually does — instead of "is it an array". Reverting the fix now turns 7 tests red.

The other oracles now assert actual expected values rather than ok === false / not.toContain.

Still open

bun jest 3164 passed · 9 Rust · typecheck clean. The GUI pass remains outstanding and I agree it is a blocker, not a follow-up — D-02 and D-03 were invisible to 31 passing tests precisely because jsdom implements no layout, so they're pinned structurally and against a rect simulator only. Nothing here has run in WebView2 yet.

🤖 Generated with Claude Code

Seven changes from Tam's first GUI pass on the Snippets feature.

- The Add/Edit dialog no longer dismisses on an overlay click. ConfirmDialog
  does and should — it holds a question; this holds a form, and a click landing
  beside the panel while reaching for the Folder field threw away everything
  typed. Escape, Cancel and the ✕ remain.

- Closing a terminal menu returns the keyboard to the terminal. Inserting a
  snippet worked but left focus on <body>. That is a property of the MENU, not
  of the snippet row that exposed it, so the fix is on the shared close path:
  all four menus and the dialog route through one refocusTerminal helper.

- Submenus open on HOVER. A click on a submenu parent now only ever opens it —
  hover has already opened the panel by the time a click can land, so toggling
  would make clicking what you are pointing at the gesture that hides it. A
  ~260ms grace period covers the diagonal into the panel; re-entering the host
  cancels it outright.

- Ctrl+Shift+S opens the Snippets flyout at the cursor, alone. Only the trigger
  travels (surfaceChrome, the shape openSearch already uses); TerminalDisplay
  still owns the menu and builds the SAME item the right-click menu does, so
  the two cannot drift. Ctrl+Shift, because bare Ctrl+S is XOFF.

- A flat/folders view toggle beside the search box, flat by default — it needs
  nothing from the user, and a library with no folders looks identical either
  way. Persisted as its own nine-link setting: the flyout is rebuilt on every
  open, so component state would have reset it silently each time.

- Wider panel, and a second tooltip. The folder/tag chip is ellipsed at about
  half the row width and had no way to show its own content; it now carries a
  title of its own, spelled out one fact per line. In folder mode the ROOT
  panel is narrow (folder names are short) and the folder panel it opens keeps
  the full width — `narrow` is depth-0 only and stripped on the way down.

Also fixes a pre-existing flake in sessionExitPersistence: it asserted a JSON
blob does not contain '137', and the blob carries a live Date.now() timestamp
that contains those digits a few runs in every thousand.

All twelve new guards mutation-checked; two survived the first pass and were
rewritten (a builder-level assert that never reached the DOM, and a lazy
source-derived regex that matched a different focus() call in the same file).

3198 renderer · 777 terminal-core · 516 Rust · typecheck clean.
…cursor

`.context-menu` is `min-width: 200px`. The standalone flyout's container keeps
that, and the empty submenu host inside stretches to it as a block child — so
`left: 100%` placed the panel a full 200px right of the point the shortcut
anchored it at. jsdom computes no layout, so the five tests covering the bare
menu were all green with the panel in the wrong place.

Pinned by reading the CSS, the way the nested-panel clipping already is;
mutation-checked by deleting the rule.
# Conflicts:
#	src/renderer/api/tauri-bridge.ts
#	src/renderer/components/Settings/SettingsPage.tsx
@rockyway
rockyway merged commit 8da7ce5 into develop Sep 4, 2026
5 checks passed
@rockyway
rockyway deleted the feature/snippets branch September 4, 2026 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants