Skip to content

fix:desktop skill select ui bug - #5249

Merged
M4n5ter merged 1 commit into
apache:mainfrom
SydneyMCDonaldbigking:fix/composer-skill-selection
Sep 13, 2026
Merged

fix:desktop skill select ui bug#5249
M4n5ter merged 1 commit into
apache:mainfrom
SydneyMCDonaldbigking:fix/composer-skill-selection

Conversation

@SydneyMCDonaldbigking

@SydneyMCDonaldbigking SydneyMCDonaldbigking commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

The desktop composer currently allows the same skill to be selected repeatedly. Hide skills already present in the draft from both the / picker and the Skills entry in the context menu, and make them available again after removal.

Preserve explicit /skill:<id> completion when entering the first invocation.

Verification

  • 41 composer and shared input behavior tests passed.
  • UI package compilation, desktop renderer typecheck, Storybook typecheck, and Storybook build passed.
  • Biome lint for the four changed files and git diff --check passed. The repository excludes these UI/desktop files from formatting.
  • The new browser interaction regression failed against the previous picker behavior. Its post-fix browser run is still pending.
  • Full workspace checks were not run. Manual before/after screenshots or a recording still need to be attached.

Remaining verification

  • Complete the post-fix browser interaction regression.
  • Confirm manually that both picker entry points hide selected skills and allow selection again after removal.
  • Attach before/after screenshots or a recording.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex helped implement the frontend change and add tests.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 13, 2026
@SydneyMCDonaldbigking

SydneyMCDonaldbigking commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Hi @M4n5ter, could you take a look at this small desktop composer fix when convenient? It hides skills already selected in the draft and makes them available again after removal.

CI currently shows action_required; could you check whether it needs approval to run? I'm still completing the verification items listed in the description.

Thanks! If someone else would be a better reviewer, please point me their way.

Posted via Codex at the contributor's request.

M4n5ter
M4n5ter approved these changes Sep 13, 2026
View reviewed changes

@M4n5ter M4n5ter 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.

LGTM.

@M4n5ter
M4n5ter merged commit 9d4a6e4 into apache:main Sep 13, 2026
2 checks passed
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 13, 2026
liangbing @claude liangbing
Merge apache/maka main into enterprise (thirteenth sync, ca4136a → 09…
c714de4 
…f0a5d)

Eight upstream commits. One reaches the new renderer's contracts: apache#4878
returns expected failures as codes across IPC — the five Session setters
(`setPermissionMode`, `setCollaborationMode`, `setOrchestrationMode`,
`setModelConfiguration`, `setThinkingLevel`) answer
`DesktopSessionUpdateResult` instead of throwing, `abandonPlanProposal`
answers `PlanControlIpcResult` like its siblings, `send` / `submitMessage`
gain an `attachment_blocked` refusal, and the attachment guard throws a typed
`AttachmentIngestBlockedError` in place of the `attachment_ingest:<code>`
message token. Also in: apache#5216 makes HTML artifacts directly openable
(`app.showArtifactInFolder` beside `openArtifactPath`, which now hands an
HTML artifact to the default app; `isArtifactUserVisible` admits HTML tool
results), apache#5198 unifies WorkHub conversation identity and Host-owned choices
(an Astryx `ChoicePanel` in packages/ui, `keyboardHint` copy, a
`preserveFocus` scroll target), apache#5249's skill picker fix
(`selectedSkillIds` in `chat-input-behavior`), apache#4815 admits structured-only
Messages (`hasMeaningfulMessageContent` in core), apache#4862's ACP live session
lifecycle in the CLI, apache#5204's workbar tab scrollbar css and apache#5180's wider
locale hygiene gate.

Resolution per the sync policy: conflicts under the old renderer's trees,
packages/ui's deleted components, stories, e2e specs and the main tests that
import them stay deleted, and upstream's new files there are dropped
(`features/workhub/model/workspace-name.ts`, packages/ui's `choice-panel.tsx`
and its `index.ts` export, the `styles/base.css` / `workhub.css` /
`maka-tokens.css` edits, `expected-error-presentation.test.ts` and the WorkHub
main tests). git's rename pairing had put upstream's
`features/session-settings/ports.ts`, `features/workhub/testing.ts` and
`platform/desktop/create-session-settings-services.ts` into
`bridge/e2e-fixture.ts`, `components/ui/skeleton.tsx` and
`lib/ported/display-frame-scheduler.ts`; all three keep ours. The renderer
architecture ledger keeps ours, rewritten with `--write`. The e2e budget and
`transcript-scroll-cost.spec.ts` keep ours; upstream's new
`expected-failure-feedback.spec.ts` is trimmed to its second case (the IPC
round trip of the setting and Plan codes), the first needing the WorkHub
surface this build does not ship.

Re-implemented for the new contracts:
- `bridge/sessions.ts` unwraps every update result and rethrows a refusal as
  `ExpectedOperationError` (new `bridge/expected-operation-error.ts`, ported
  from upstream's `operation-diagnostics.ts`), so the turn actions store and
  every caller keep awaiting a summary. `localizedShellErrorMessage` renders
  the code through the new `updateFailures` copy on every surface and
  `AttachmentIngestBlockedError` by its `code`; `sessionSettingFailureCopy`
  is upstream's.
- `ChatInput` routes an `attachment_blocked` refusal through upstream's
  `showSubmissionFeedback` and keeps the draft with the ingest reason;
  `showSkillInvocationFeedback` stays exported for the partial-success toast.
- `FilesTab` reveals through `showArtifactInFolder`; an HTML row shows "View
  in Maka", its menu offers "Open in Default App" first, and its preview's
  external action opens rather than reveals. `artifact-copy` gains
  `viewInMaka` / `openInDefaultApp` in three locales.
- `TipTapEditor` hides Skills already in the draft from the picker (apache#5249);
  the chips are atoms here, so the set is read from the document rather than
  from the `/skill:x` text upstream scans.
- `composer-state.test.ts` asserts the typed preflight error.

packages/ui: `use-chat-scroll.ts` merged cleanly (`preserveFocus`) on top of
our `holdTurn` extensions; `conversation-copy.ts` and `chat-input-behavior.ts`
take upstream's additions. apache#5217's live-turn buffer stays out as before.

The compatible-change declaration is unchanged this round. The release
checklist's baseline note records that the eight commits add no unlisted
renderer surface.

Gates: build:test + build:renderer, typecheck, biome lint and format, locale
hygiene (the widened apache#5180 gate), ASF headers, renderer architecture ledger
(rewritten with `--write`), e2e budget, third-party notices, knip (39 unused
files, unchanged from the twelfth sync), workspace dist tests (desktop 3484
of 3498 with 14 skipped, every other workspace green), Electron smoke (44
checks, no renderer errors), core-dialogue smoke, streaming-switch smoke, and
the trimmed `expected-failure-feedback` e2e case against the real preload.
`packages/runtime` `model-adapter-onerror` fails on this machine before and
after, as in the eleventh and twelfth syncs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@M4n5ter M4n5ter M4n5ter approved these changes

Assignees

No one assigned

Labels

effort/S Under 100 readable lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SydneyMCDonaldbigking @M4n5ter