Skip to content

fix(ui-web): offer removal on the installed skill card - #396

Merged
0xKT merged 2 commits into
EverMind-AI:mainfrom
Handsome-wzw:fix/installed_skill_card_offers_removal_gh
Sep 16, 2026
Merged

0xKT merged 2 commits into
EverMind-AI:mainfrom
Handsome-wzw:fix/installed_skill_card_offers_removal_gh

Conversation

@Handsome-wzw

@Handsome-wzw Handsome-wzw commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Removing an installed skill was reachable only by opening the detail drawer, and
the card gave no sign it opened one. The card is a role="button" that opens the
drawer on click, but the only thing on it that looks like an action is the use
button, so a reader who wants a skill gone reads the card as "use only" and goes
to the CLI -- where raven skill remove takes the hub slug
(kbarbel640-del_skills_ai-ppt-generator), not the name the card shows
(ai-ppt-generator), and answers a bare "No installed Hub bundle found" for the
name it does show.

The card's footer now carries the same armed remove the drawer already has,
gated on the same predicate, so the capability the page already had is visible
where the reader looks for it.

The predicate is hub, which is not the same as "the source column says
hub".
It is set from a .skillhub.json marker written beside the skill at
install time, and skillhub.remove needs the id that marker holds. A bundle
installed before the marker existed reads hub: false and offers no button --
the same answer the drawer gives it today, unchanged by this.

Checked and deliberately not changed.

  • ArmRemove keeps its two-click arm rather than growing a confirm dialog: the
    WKWebView shell has no JS-panel delegate, which is why the arm exists.
  • The card's own click handler still opens the drawer. The button already called
    stopPropagation, so the press does not reach it; verified in a browser rather
    than reasoned about, and pinned by a test.
  • Builtin skills still offer nothing, because there is no bundle to delete.
  • The drawer's copy of the control stays. Both entry points are reached by
    different readers and neither is redundant.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Gates, each run as its own target and its exit code read directly:

make lint-ui                exit 0   (gen:check in sync, type-check clean)
make test-ui                exit 0   103 files, 1681 tests, 0 failed
make build-ui               exit 0
make check-source-language  exit 0
make check-large-files      exit 0
npx commitlint              exit 0
scripts/check_commit_messages.py upstream/main..HEAD   exit 0

Baseline, npx vitest run --no-file-parallelism in a detached worktree at
upstream/main (5b7566b), run the same way:

base     103 files, 1678 tests, 0 failed
branch   103 files, 1681 tests, 0 failed

Failing ids diffed rather than counted: 0 introduced. Neither side has a
failure to reconcile; the three extra tests are this branch's own.

Mutation-checked, each reverted afterwards with a clean git status:

  • Deleting the card's ArmRemove fails offers removal on the installed card itself and keeps the drawer shut when the remove on the card is pressed,
    and nothing else.
  • Replacing the c.hub guard with true fails offers no removal for a builtin, and nothing else.

Verified in a browser against a live gateway, not only in tests. With
amap-weather (hub, marker present), resume-master (hub, no marker) and two
builtins installed: the button appears on the first and on neither of the others;
the first press relabels to the confirm and does not open the drawer; the second
removes the skill. The rail went from five rows to four and
<workspace>/skills/amap-weather is gone from disk.

CI, and what is red

Three jobs are red and none of them is this branch's. main fails the same
three at 5b7566b4, which is this branch's merge base, in run 34744930015:

main @ 5b7566b4   page checks | Windows self-upgrade | unit (py3.12 / ubuntu-latest)
this branch       page checks | Windows self-upgrade | unit (py3.12 / ubuntu-latest)

The sets are identical. page checks also reproduces locally at that sha, in a
detached worktree built from an untouched tree, with the same message:

check-page: expected exactly two <script> blocks

Nine jobs pass, including every one this change could plausibly reach: TUI checks, ui rpc contract, pre-commit diff, python lint, commit messages,
repository files, bridge checks, kernel wheel smoke and pull request title.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

User-visible and the point of the change: an installed skill whose bundle can be
deleted now shows a remove control on its card. Destructive, so it is the armed
control the drawer already uses -- one press to arm, a second within four
seconds to fire, and the arm lapses on its own.

No behaviour outside the card: no RPC, no schema, no dependency, no change to
what skillhub.remove does or to which skills qualify. ArmRemove gains an
optional inline flag that drops a top margin meant for a drawer section; its
one existing caller passes nothing and renders as before.

One existing drawer test now scopes its query to the drawer. It searched the
whole document for the uninstall label, which was unambiguous while the card had
none and matches two once the card has one. The assertion it makes is unchanged.

Rollback is reverting the commit; nothing here is stateful or migrated.

Related Issues

N/A

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

I reviewed github/main...3d7014c73be1, the surrounding skills component, store, live source, CSS, callers, and relevant history. I also checked backward compatibility, test strength (the drawer query was narrowed rather than weakened), AGENTS.md constraints, CONTEXT-MAP.md architecture boundaries, and domain terminology. The implementation is otherwise consistent with the existing guarded drawer removal path.

Verification:

  • npm test --prefix ui-web: passed, 103 files and 1,681 tests. happy-dom emitted abort/connection diagnostics from iframe fetch fixtures, but the command exited 0.
  • npm run type-check --prefix ui-web: passed.
  • npm run gen:check --prefix ui-web: passed.
  • git diff --check github/main...HEAD: passed.

One keyboard-path suggestion is inline; it does not block this change.

Comment thread ui-web/src/features/skills/SkillsPage.tsx
@Handsome-wzw
Handsome-wzw force-pushed the fix/installed_skill_card_offers_removal_gh branch from 3d7014c to 061c2b9 Compare September 16, 2026 02:22

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

The branch was rebased onto current main; git range-diff confirms the PR patch is unchanged from 3d7014c73be1. I rechecked the full diff plus the affected component, store/live-source callers, history, compatibility, test changes, and repository architecture/rules. There are no new findings. The existing Enter-key propagation follow-up remains nonblocking and is still recorded inline.

Verification on this revision:

  • npm test --prefix ui-web: passed, 103 files and 1,681 tests. happy-dom again emitted its iframe teardown network diagnostics, but the command exited 0.
  • npm run type-check --prefix ui-web: passed.
  • npm run gen:check --prefix ui-web: passed.
  • git diff --check github/main...HEAD: passed.

Handsome-wzw and others added 2 commits September 16, 2026 14:58
Removing an installed hub skill was reachable only from the detail drawer,
and the card gave no sign it opened one: the use button was the only thing on
it that looked like an action, so a reader who wanted a skill gone concluded
the page could not do it and went to the CLI -- where the argument is the hub
slug, not the name the card shows.

The card's footer now carries the same armed remove the drawer does, gated on
the same predicate: a builtin has no bundle to delete, so only a hub install
offers it. `ArmRemove` gains an `inline` flag that drops the top margin meant
for a drawer section; it already stopped propagation, which is what keeps the
press off the card's own open-the-drawer handler.

One existing drawer test now scopes its query to the drawer. It searched the
whole document for the uninstall label, which was unambiguous while the card
had none and matches two once the card has one.

Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Enter on the remove button reached the card's own handler first. Keydown
bubbles, and the click that stops propagation is synthesised after it, so the
drawer opened before the button could act on the press.

That is worse than a stray navigation because the control is deliberately two
presses: the first arms it, the second removes. With the first press navigating
away there was no second press to make, so the action the card exists to offer
could not be completed from the keyboard at all -- while the mouse path, which
this card was added for, worked throughout.

The card's handler now answers only for the card. A press that started on
something inside it belongs to that thing, which already knows what to do with
it.

Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
@Handsome-wzw
Handsome-wzw force-pushed the fix/installed_skill_card_offers_removal_gh branch from 061c2b9 to fd27fe3 Compare September 16, 2026 06:59

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

The new target/currentTarget guard fixes the keyboard event-ordering issue without changing direct card activation, and the regression test covers the failing path through removal. I reviewed the revision delta and full PR diff, affected component/store callers, relevant history, backward compatibility, test strength, AGENTS.md/CONTEXT-MAP.md constraints, and architecture boundaries. I found no new issues. My prior thread is resolved.

Verification:

  • npm ci --prefix ui-web: completed; npm warned that local Node 23 is outside Vitest's declared engine range.
  • npm test --prefix ui-web: passed, 106 files and 1,732 tests. happy-dom emitted its existing iframe teardown network diagnostics, but the command exited 0.
  • npm run type-check --prefix ui-web: passed.
  • npm run gen:check --prefix ui-web: passed, 172 methods match.
  • git diff --check github/main...HEAD: passed.

@0xKT

0xKT commented Sep 16, 2026

Copy link
Copy Markdown
Member

Not a blocker -- the card's remove and the drawer's remove do not guard the same way, and the card's can fire twice.

The comment on the new button says it uses the same guard as the drawer:

Same guard the drawer uses: a builtin has no bundle to delete.

That is half of it. The drawer's guard is !removable || busy (manage, SkillsPage.tsx:377-378); the card's is c.hub alone (:299). The missing half is busy, and busy is set by the very handler this button calls -- removeInstalled opens with set({ busy: c.id }) (store.ts:211).

Measured on fd27fe3b merged onto 7beffb9d, with a probe that holds remove unresolved so the in-flight state is observable:

removal started from second destructive press reachable while the first is in flight remove calls
the new card button yes, the button is sitting armed 2
the drawer, same skill, same state no, manage returns null under busy 1

The control is the point: this is not how ArmRemove behaves everywhere, it is how it behaves in the one place that does not check busy. After the confirming press the button stays armed rather than disappearing, so one further click re-enters removeInstalled for a skill whose removal has not come back yet.

Why non-blocking: I could not show harm from the duplicate. The second call is remove(c.name) for a skill the server is already removing; whether that is idempotent is on the other side of the RPC and I did not test it, and the local .then only flips installed and closes the drawer. So this is a guard that diverges from the sibling it claims to copy, not a demonstrated failure -- worth a line, not worth holding the PR.

If you want it closed, the smallest thing that matches the drawer is to give the card the same busy half of the guard rather than a new rule.

Checked and NOT worth raising, recorded so nobody re-derives them: the three withdrawal mutations all bite (drop the e.target !== e.currentTarget keydown guard, drop the button, drop the builtin check -- 1, 3 and 1 tests red respectively), so the tests that came with this are real; within(...) scoping the pre-existing drawer test is a correct adaptation to two matches rather than a weakened assertion; the inline prop is cosmetic and has no guard to miss.

@gloryfromca

gloryfromca commented Sep 16, 2026

Copy link
Copy Markdown
Member

No blockers; suggestions only, and they are marked inline.

The measurement from @0xKT changes my standing stance from clean to nonblocking. The observation holds: removeInstalled sets busy, but the installed-card renderer does not consume that state, so its armed control remains reachable and can submit a second removal while the first is unresolved; the drawer manage path suppresses the control under the same state. No concrete downstream failure has been demonstrated, so this does not hold the change.

The existing note fully captures the follow-up; I have no additional finding. Focused verification on the unchanged head: npm test --prefix ui-web -- src/features/skills/SkillsPage.test.tsx passed all 17 tests.

@0xKT
0xKT merged commit 756ccfd into EverMind-AI:main Sep 16, 2026
33 of 34 checks passed
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.

3 participants