feat(actions): pin any number of actions to the card header with custom icons - #937
Merged
Conversation
…om icons A repo's actions surfaced exactly one "main" button in the branch and repo card headers: the first `run`-type action by sort order, decided in the frontend and stored nowhere. This replaces that implicit single slot with an explicit pinned set — any number of actions, of any type, each rendered as its own icon button. `repo_actions` gains `pinned` and `icon` (a kebab-case Lucide name, NULL meaning "the default icon for this action type"). Migration 0025 pins each context's current implicit main action, tie-breaking deterministically because `sort_order` has never been unique, so no existing database loses its run button. Detection keeps detecting just the one run action and pins it only when the context had no run action at all — gating on that rather than "has nothing pinned" means deliberately unpinning survives a re-detect. `PrimaryRunActionButton` becomes `PinnedActionButton`, taking the action as a prop instead of deriving the one primary from the runner: each pinned run action tracks its own building/serving phase and endpoint pill, and pinned non-run actions spin then report their outcome. Running pills and the Actions submenu now exclude every pinned action rather than just the primary — the submenu's exclusion generalizes from the run group to all of them. Icon rendering and the picker live behind a lazily imported chunk of Lucide's full ~1,750-icon map. It has to import `@lucide/svelte/icons/index` rather than the `@lucide/svelte` barrel: the barrel is statically imported by the diff-viewer package, so a dynamic import of it can't split out and the whole set lands in the main bundle (+620 kB, measured). Stored kebab names convert back to Lucide's PascalCase exports; the inverse is lossy, so candidate spellings are round-tripped through the conversion rendering uses and the first survivor wins — which reproduces lucide.dev's own spelling for all but three of the icons and guarantees every produced name resolves. Verified with `just ci` equivalents: cargo fmt/clippy, 742 Rust tests, svelte- check, and 684 vitest tests all pass; `vite build` confirms the icon chunk splits (main bundle 1,717 kB, below the 1,734 kB baseline). Signed-off-by: Matt Toohey <contact@matttoohey.com>
Every other way an action reached a context pinned on the user's behalf: detection pins its run suggestion when the context had no run action, and migration 0025 pinned the implicit main action of every pre-existing context. A context whose actions are all typed into Settings → Repos went through neither, so its first action landed unpinned and the branch and repo card headers stayed empty — where before pinning existed, adding a run action grew a play button on its own. The "Add Action" editor now pre-checks "Show in card header" whenever the context has nothing pinned yet, via a `shouldPinNewAction` helper alongside `getPinnedActions`. Gating on the pinned set rather than on the action type keeps the rule the same one the header cares about, and only the first action gets the default — once something is pinned the box starts clear, and it's visible and toggleable either way, so this can't override a deliberate choice. Verified with prettier, svelte-check, and vitest (687 tests, 3 new covering the empty context, the unpinned-run-action case, and the already-pinned one). No Rust changes. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
The doc comments on persist_suggested_actions and shouldPinNewAction cited migrations 0026 and 0025 as the one that pinned each pre-existing context's implicit main action — numbers from before a rebase renumbered it. Those slots now hold add-note-parent and remove-auto-reviews, so both comments sent a reader auditing pin behavior to unrelated migrations. The shipped migration is 0028-pinned-actions; both references now say 0028. Flagged by review dde69e20; comment-only, verified with prettier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The icon picker's empty-query grid rendered CURATED_ICONS as-is, and nothing validated that hand-written list against the loaded icon map. Search results can't miss — they come from the map's own keys — but the curated names are frozen at whatever Lucide version they were written against, so an upgrade renaming or dropping one (several are rename-prone aliases) would make iconMap[name] yield undefined and rendering <Icon> throw, breaking the picker's default view. searchIconNames already receives the loaded names, so its empty-query branch now filters the curated list through them: a stale name quietly leaves the shortlist instead of crashing the grid, and nothing changes while every curated name still resolves. Flagged by review dde69e20. Verified with prettier, svelte-check, and vitest (828 tests — the empty-query case now asserts both the full-map identity and the dropped-name degradation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The guard test scanned only `apps/staged/src`, but the bundle graph it
protects is bigger: `@builderbot/diff-viewer` is a workspace dependency whose
`exports` point at raw sources, so every file under `packages/diff-viewer/src`
compiles into staged's bundle. A static `@lucide/svelte/icons/index` import
added there would fold the ~620 kB icon map back into the main bundle with the
guard still green — exactly the regression the test exists to catch.
The test now globs `packages/*/src/**/*.{ts,svelte}` alongside the app sources,
normalizes both key shapes to repo-relative paths, and runs the existing banned
pattern over the merged set. `packages/*` matches the pnpm-workspace glob, so
future packages are covered without another edit. A canary case asserts the
merged set contains a known file from each glob, so a directory rename or a
wrong `../` count fails loudly instead of silently hollowing the guard out one
level up — the same failure mode being fixed here.
diff-viewer's three barrel imports move to per-icon imports so one rule-set
covers everything rather than a strict one for the app and a looser one for
packages. They were tree-shaken and cost nothing, but they are why
`loadIconMap()` has to reach for `icons/index`: a dynamic import of a barrel
that something else imports statically can't split into its own chunk. Per-icon
modules are default exports, so local names carry over and no template changes.
`Loader2` becomes `icons/loader-circle` — `icons/loader-2.js` is a deprecated
re-export of exactly that module, so the rendered icon is unchanged.
Verified red-first: a temporary `icons/index` import in a diff-viewer file
fails the widened test naming that file. Prettier, svelte-check, and vitest
(829 tests) pass, and `vite build` is byte-identical to HEAD — main bundle
1,754.85 kB, icon chunk still split at 598.01 kB.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a80894f8a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| : ''} | ||
| <div | ||
| class="primary-action-container" | ||
| class="pinned-action-container" |
There was a problem hiding this comment.
This new 286-line component combines endpoint clipboard/timer state, stop and output behavior, execution-status derivation, two interactive button variants, transitions, and styling in one render tree. Extract the endpoint pill and circular button into focused subcomponents with explicit props so shared action behavior can be maintained without the two branches diverging; the repository explicitly requires small, composable components and extraction of repeated UI blocks.
AGENTS.md reference: apps/staged/AGENTS.md:L20-L21
Useful? React with 👍 / 👎.
PinnedActionButton was a 286-line render tree: endpoint clipboard and timer state, stop/output dispatch, execution-status derivation, two full interactive button variants, transitions, and styling. Worse, three blocks inside it — the title/aria ladder, the status-icon ladder, and the click dispatch — repeated nearly verbatim across its two branches and a third time in RunningActionPills, so a change to one rung had to be made in three places. Flagged as a P1 on PR #937 against apps/staged/AGENTS.md's rule to prefer small, composable components with clear props. The two branches become presentational subcomponents taking explicit props and no runner: PinnedActionEndpointPill (running with an endpoint) and PinnedActionCircleButton (everything else). Shared behavior moves out of both: actionStatusLabels() renders the six-rung title/aria ladder as a pure function, and ActionStatusIcon renders the icon ladder with an `idle` snippet for the per-caller default. RunningActionPills adopts both, collapsing three copies of each ladder into one; it takes the tooltip only, since its own text already names the action. ActionStatusIcon takes `serving` as a caller-computed boolean rather than deriving it, because the surfaces disagree on one edge — a running run action with no run phase yet shows a sine wave in a header button and a spinner in a pill. Keeping the expression at the call site preserves each surface's current behavior instead of silently normalizing it. The parent keeps only orchestration: the derivations, the slide container, the pill-vs-circle choice, and one press() the two branches share (equivalent to both old handlers rung by rung — the run rung was already unreachable from the pill). The pill's copied tick collapses from an executionId-keyed Record plus timer map to one local boolean, since each pill instance renders exactly one execution; the only difference is that a tick no longer survives an unmount and remount inside its 1.5 s window. Behavior-preserving: variant class strings, transitions, and the endpoint-pill gate move unchanged, and all Lucide imports stay per-icon so the icon-map guard stays green. Verified with prettier, svelte-check, and vitest (836 tests, 7 new covering every rung of the label ladder and its precedence). No Rust changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.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.
A repo's actions surfaced exactly one "main" button in the branch and repo card
headers: the first
run-type action by sort order, decided in the frontend andstored nowhere. This replaces that implicit single slot with an explicit pinned
set — any number of actions, of any type, each rendered as its own icon button.
Changes
repo_actionsgainspinnedandicon(a kebab-case Lucide name,NULLmeaning "the default icon for this action type"). Migration0028-pinned-actionspins each context's current implicit main action,tie-breaking deterministically because
sort_orderhas never been unique, sono existing database loses its run button.
context had no run action at all — gating on that rather than "has nothing
pinned" means deliberately unpinning survives a re-detect.
PrimaryRunActionButtonbecomesPinnedActionButton, taking the actionas a prop instead of deriving the one primary from the runner. Each pinned run
action tracks its own building/serving phase and endpoint pill; pinned non-run
actions spin then report their outcome. Running pills and the Actions submenu
now exclude every pinned action rather than just the primary.
Lucide's full ~1,750-icon map, so the icon set stays out of the main bundle.
Stored kebab names convert back to Lucide's PascalCase exports; the inverse is
lossy, so candidate spellings are round-tripped through the conversion
rendering uses and the first survivor wins.
Follow-ups in this branch
migration, so a hand-built context's first action landed unpinned and its card
header stayed empty. The "Add Action" editor now pre-checks "Show in card
header" whenever the context has nothing pinned yet.
@lucide/svelte/icons/indeximports acrossapps/staged/srcandpackages/*/src, since@builderbot/diff-viewercompiles from source into staged's bundle and could fold the ~620 kB icon map
back in with the guard still green. diff-viewer's barrel imports move to
per-icon imports.
Lucide upgrade that renames or drops a curated name degrades the picker's
default grid instead of crashing it.
Verification
cargo fmt/clippy, 742 Rust tests,svelte-check, and 829 vitest tests pass.vite buildconfirms the icon chunk splits out (main bundle 1,754.85 kB, iconchunk 598.01 kB).