fix(skills): show installed catalog state - #3519
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the root-cause paragraph is the part that makes this easy to review: the state was already correct and only the affordance lied, so the fix belongs in rendering and nowhere else. That framing is right, and the change stays inside it.
Reviewed at exact head 77d912f49497a3d08cc63c76deaa0f9ac4e6ef8b against base 1ddbd2591ccc6fd06d9727a216e6a5b64d8b1e40. No findings — no P0, P1, P2 or P3. One design note below, and one reason this cannot be an approval yet.
What we checked rather than assumed:
- Icon precedence.
installingis evaluated beforeinstalled, so an in-flight install shows the spinner rather than briefly flashing a completed check. Easy to get backwards; it is not. - The disabled state was already correct.
isDisabled={installed || skillActionBusy || !onInstall}predates this change, which is exactly why the bug was cosmetic rather than functional — the button could not be clicked, it just looked like it could. --successresolves. It is defined inmaka-tokens.cssacross every theme variant, andcolor: var(--success)is the same usage already present inchat-detail.cssandtask-ledger.css.- The class lives where its siblings live.
.maka-skill-install-complete-iconsits inapps/desktop/.../module-pages/skills.csswhile the component is inpackages/ui. That looked like cross-package drift until we checked:maka-skill-inspector,maka-skill-row-labelandmaka-skill-row-slugare all defined the same way. This follows the existing convention rather than breaking it. - Proportion. Roughly sixty of the eighty added lines are the new test; the behavioural change is one branch, one copy key per locale, and three lines of CSS.
Design note, not a finding. catalogInstallButton is shared by both catalog rows, and the two call sites derive installed from different sources: the bundled row passes entry.installed (the flag your root-cause paragraph describes), while the marketplace row computes skills.some((skill) => skill.id === source.id). Both are pre-existing and neither is changed here, but the new check icon now keys off both. The added test covers the bundled path only — which your description states accurately. Worth a second test on the marketplace path at some point, not in this PR.
On CI. The head has one completed check (test: success), but the aggregate commit status is pending, so this is not terminal green and we are not approving it. Once the full set lands green, there is nothing here that would stand in the way.
How this was reviewed. Two of us read it independently without visibility into each other's conclusion and reached the same result; the targeted @maka/ui suite was executed against a worktree of this head (203 passing, including the two cases this PR adds). The note above came from one reviewer and the proportion observation from the other — neither of us saw both on our own.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
jackwener
left a comment
There was a problem hiding this comment.
Catalog installed already flipped; the button still painted Download. Check + disabled + skill-specific label is the minimal visual match. Tests cover both states. Approve.


Summary
Root cause
The bundled catalog already refreshed
installedafter a successful install, but the shared catalog action always rendered the download icon whenever it was not actively installing. The state changed correctly while the visual affordance continued to look installable.Verification
npm run build --workspace=@maka/corenpm run typecheck --workspace=@maka/uinpm test --workspace=@maka/ui(203 tests)npm --workspace @maka/desktop run typechecknpm run format:checknpm run check:asf-headersnpm --workspace @maka/desktop run build-storybooknpm --workspace @maka/desktop run smoke:storybook(163 stories)Screenshots
Before and after screenshots use the same Storybook story, viewport, state, content, and zoom. They will be attached in a PR comment.