You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "MatrixA (via GitHub)" <gi...@apache.org> on 2026/08/22 07:14:41 UTC
[PR] fix(desktop): keep the composer ＋ menu still when toggling Plan [maka]
MatrixA opened a new pull request, #3469:
URL: https://github.com/apache/maka/pull/3469
## Summary
Clicking Plan in the composer's ＋ menu made the open menu visibly blink: the toggle re-fetches the invocable-Skill projection, the fail-closed clear leaves \`mentionSkills\` empty for the length of the IPC round trip, and the Skills row read that transient \`[]\` as "no skills available" — graying out and growing a description line, then snapping back. \`useComposerMentions\` now also returns a settled \`mentionSkillsUnavailable\` verdict that only moves when a request resolves; the Skills row renders from it, while the \`/\` popup keeps reading the fail-closed list. A second commit removes the pending-keyed disable that dimmed the Plan/Swarm/Graph rows and the footer mode marks to half opacity during a toggle's commit round trip — the pending registries already swallow re-entrant toggles, so the disable protected nothing and only blinked.
## Root cause
\`use-composer-mentions.ts\` clears the list while re-fetching on purpose (a visible \`/\` popup must never advertise a stale Skill), and \`newSessionCollaborationMode\` is a dependency of that effect, so \`length === 0\` could not tell "re-fetching" from "nothing to offer". The same blip fired in sessions via the \`mode-change\` event, and on model or turn changes.
## Verification
- \`packages/ui\`: 192/192 unit tests pass, including three new tests pinning the Skills row's presentation (refreshing ≠ empty; settled-empty keeps its caveat; populated stays quiet).
- \`apps/desktop\`: 1060/1060 main-process tests pass; \`typecheck\` and \`biome lint\` clean on every touched file.
- e2e: the new \`composer-plus-menu-stability.spec.ts\` arms an in-page MutationObserver before the click and asserts the menu stays open, keeps one height, and never shows the transient "no skills" line or a grayed row — it fails on the pre-fix build with exactly that signal. Adjacent composer specs (skill invocation, slash-command menu, send-message) still pass, 7/7 total.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code (Claude Fable 5) diagnosed the flicker and authored the fix, unit tests and e2e spec; both commits carry \`Generated-by: Claude Code\` trailers.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep the composer ＋ menu still when toggling Plan [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #3469:
URL: https://github.com/apache/maka/pull/3469
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep the composer ＋ menu still when toggling Plan [maka]
Posted by "MatrixA (via GitHub)" <gi...@apache.org>.
MatrixA commented on code in PR #3469:
URL: https://github.com/apache/maka/pull/3469#discussion_r3835736717
##########
packages/ui/src/composer.tsx:
##########
@@ -1682,11 +1706,44 @@ export const Composer = forwardRef<
// a stray slash and popping an empty menu. Say why it is
// unavailable: the panel this replaced showed "no skills
// available", and a silent grey row answers nothing.
- isDisabled={props.disabled || props.mentionSkills.length === 0}
+ //
+ // Only a SETTLED empty catalog counts. The host clears
+ // the list while re-fetching it (a Plan toggle or model
+ // change does that with this menu open), and rendering
+ // that transient `[]` as "no skills" grows this row by
+ // a description line and back — the menu visibly jumps.
+ // So the verdict is the host's settled flag when it
+ // supplies one, and the row repaints only when the
+ // catalog's emptiness actually changed.
+ isDisabled={
+ props.disabled
+ || (props.mentionSkillsUnavailable
+ ?? props.mentionSkills.length === 0)
+ }
description={
- props.mentionSkills.length === 0 ? copy.noSkillsAvailable : undefined
+ (props.mentionSkillsUnavailable
+ ?? props.mentionSkills.length === 0)
+ ? copy.noSkillsAvailable
+ : undefined
+ }
+ // Mid-refresh the row LOOKS like the previous catalog
+ // but cannot act for the current one: opening the `/`
+ // menu now would write a stray slash against a list
+ // that is fail-closed empty. A loading click is a
+ // complete no-op — nothing typed, menu left open — so
+ // the same click a beat later simply works. The class
+ // is the observable contract for that state.
+ className={
Review Comment:
Addressed in 731beea1a. The row now carries `aria-busy` while the catalog refreshes — via a new vendor seam (`DropdownMenuItem` forwards the attribute to its row; documented in patches/README.md) — so assistive technology hears that activation is deferred rather than a plain "available" that swallows the action. Geometry and the enabled look stay fixed. The keyboard pin you asked for is in the e2e: mid-refresh, Enter on the focused row neither closes the menu nor writes `/`, and the same activation works once settled. One implementation note: the activation gate lives inside one always-present `onClick` — swapping the prop between undefined and a function remounts Item's internals and drops keyboard focus mid-refresh.
##########
apps/desktop/src/renderer/use-composer-mentions.ts:
##########
@@ -33,18 +35,32 @@ export function useComposerMentions(options: {
newSessionPermissionMode,
newTaskTarget,
} = options;
- const [mentionSkills, setMentionSkills] = useState<InvocableSkillEntry[]>([]);
+ // One explicit representation of the Skill catalog — in flight, settled
+ // empty, or settled populated — held as a single value so a refresh can
+ // never tear its facets apart.
+ //
+ // `skills` is the live, fail-closed list the `/` popup reads: it is cleared
+ // the moment a refresh starts, because a visible popup must never advertise
+ // a Skill the new backend surface may not carry. That clear is exactly why
+ // `length === 0` cannot tell "re-fetching" from "nothing to offer", so the
+ // ＋ menu's Skills row renders from `settled` — the last RESOLVED verdict,
+ // held across refreshes — and repaints only when the catalog's emptiness
+ // actually changed. `loading` gates interaction: while a request is in
+ // flight (including the very first, before anything has settled), a click
+ // on the row must have no side effect — the held presentation is the OLD
+ // catalog's look, not a promise the current one can honor.
+ const [catalog, setCatalog] = useState<{
+ loading: boolean;
+ settled?: 'empty' | 'populated';
+ skills: InvocableSkillEntry[];
+ }>({ loading: true, skills: [] });
useEffect(() => {
let cancelled = false;
let requestVersion = 0;
- // Do not briefly advertise the previous session/project's Skills while the
- // authoritative projection is being refreshed. The same fail-closed reset
- // applies when a model/mode/plan or MCP event changes the backend surface:
- // a visible popup must never retain a now-unavailable Skill.
const refresh = () => {
const version = ++requestVersion;
- setMentionSkills([]);
+ setCatalog((previous) => ({ loading: true, settled: previous.settled, skills: [] }));
Review Comment:
Addressed in 731beea1a. The catalog value now stores the `contextKey` it describes (session, project path, model, collaboration/permission mode, new-task target), and the hook derives through it: a render whose key no longer matches fails closed to loading immediately, without waiting for the passive effect's clear. A same-context refresh still holds its settled verdict; a context switch has nothing settled to hold. The lifecycle test you asked for is in the e2e (`a context switch re-enters loading instead of holding the old catalog`): populated session catalog → switch to new chat under a latched new-task projection → the row presents busy and defers activation, then settles. Honest caveat: with the latch the spec pins the loading presentation and no-op, but the original one-frame window itself is not observable from Playwright — the key-based derivation is what closes it.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -1100,6 +1118,19 @@ function AppShellContent({
collaborationModeChangeRegistry.keysRef,
setPendingCollaborationModeBySession,
);
+ // Whatever the user last asked for while this commit ran is the state
+ // they expect to land on. Read the settled mode through the ref — the
+ // closure's projection predates this commit — and only re-apply when
+ // the intent is not already satisfied.
+ const queued = queuedCollaborationModeBySession.current.get(sessionId);
Review Comment:
Addressed in 731beea1a. `clearSessionRendererState` now deletes both queued-intent entries alongside the pending registries, so a commit's finally has nothing to replay against a Session whose renderer lifecycle already ended. Covered by the new e2e `deleting the session while a toggle is pending settles clean`: commit latched open → second toggle queued → session deleted → latch released → the app settles on a clean new chat with no error surfaced and no stale Plan state.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -427,8 +427,18 @@ function AppShellContent({
const [newChatPlanModeActive, setNewChatPlanModeActive] = useState(false);
const [newChatOrchestrationMode, setNewChatOrchestrationMode] = useState<OrchestrationMode>('default');
const [scheduledTaskCreateRequestNonce, setScheduledTaskCreateRequestNonce] = useState(0);
- const [pendingCollaborationModeBySession, setPendingCollaborationModeBySession] = useState<Record<string, boolean>>({});
- const [pendingOrchestrationModeBySession, setPendingOrchestrationModeBySession] = useState<Record<string, boolean>>({});
+ // Write-only: the pending-action helpers need a store, but nothing renders
+ // from these — a mode toggle's guard is the registry ref, and painting the
+ // round trip (disable/dim, then restore) is exactly the ＋ menu blink that
+ // MatrixA/fix-plan-click-flicker removed.
+ const [, setPendingCollaborationModeBySession] = useState<Record<string, boolean>>({});
Review Comment:
Addressed in 731beea1a. The two write-only states are gone; the registry ref is the single re-entrancy authority. The shared helpers now take the setter optionally, since permission/model/retry pending states are still rendered from — mode toggles simply pass none, so an add/clear no longer schedules a render nobody reads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep the composer ＋ menu still when toggling Plan [maka]
Posted by "MatrixA (via GitHub)" <gi...@apache.org>.
MatrixA commented on code in PR #3469:
URL: https://github.com/apache/maka/pull/3469#discussion_r3835607009
##########
apps/desktop/src/renderer/use-composer-mentions.ts:
##########
@@ -34,6 +35,14 @@ export function useComposerMentions(options: {
newTaskTarget,
} = options;
const [mentionSkills, setMentionSkills] = useState<InvocableSkillEntry[]>([]);
+ // The last SETTLED truth about the catalog, held across refreshes. The list
+ // itself is cleared while a refresh is in flight (fail-closed, for the `/`
+ // popup), so `length === 0` cannot tell "re-fetching" from "nothing to
+ // offer" — and the ＋ menu's Skills row repainting on that transient empty
+ // is a visible blink of the open menu on every mode, model or turn change.
+ // This flag only moves when a request resolves, so the row's presentation
+ // is stable until the catalog's emptiness actually changed.
+ const [mentionSkillsUnavailable, setMentionSkillsUnavailable] = useState(false);
Review Comment:
Addressed in acf2344e8. The catalog is now one explicit state — loading / settled-empty / settled-populated — held as a single value in the hook. Presentation still renders from the last settled verdict (geometry stays put during loading, including before the very first settle), and the new `mentionSkillsLoading` gates interaction: mid-refresh the row ignores clicks and stops closing the menu, so nothing types a stray `/` against the fail-closed list; the same click a beat later simply works. The held state is observable as `maka-composer-skills-loading`. Regressions: two SSR tests pin the loading presentation, and the new e2e `a Skills click during the catalog refresh does nothing, then works settled` holds the refresh open on a preload latch (isolated-E2E gate only), clicks straight into the window, and asserts an empty draft, no popup, and the menu still open — it fails without the fix.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -3291,26 +3296,16 @@ function AppShellContent({
: undefined
}
planModeActive={activePlanMode}
- planModePending={activeId
- ? pendingCollaborationModeBySession[activeId] === true
- : false}
- planModeDisabledReason={
- activeId && pendingCollaborationModeBySession[activeId] === true
- ? shellCopy.modeChanging
- : modeChangeDisabledReason
- }
+ // No pending-keyed disable while a toggle commits: the
+ // pending registries already swallow re-entrant toggles, and
Review Comment:
Addressed in acf2344e8 with the queue option: each session keeps only the LATEST asked value while a commit is in flight, and the commit's finally block applies it when the settled state does not already satisfy it — so a quick "on, then off" finishes off, for Plan and orchestration alike. The settled state is read through `sessionsRef` rather than the render closure, which predates the commit. Regression: the new e2e `two rapid Plan toggles land on the last requested state` latches the session-list refresh (the tail of the commit) via the preload's isolated-E2E affordance, so the second click lands deterministically after the checked repaint but inside the pending window; it fails without the queue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep the composer ＋ menu still when toggling Plan [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3469:
URL: https://github.com/apache/maka/pull/3469#discussion_r3835559313
##########
apps/desktop/src/renderer/use-composer-mentions.ts:
##########
@@ -34,6 +35,14 @@ export function useComposerMentions(options: {
newTaskTarget,
} = options;
const [mentionSkills, setMentionSkills] = useState<InvocableSkillEntry[]>([]);
+ // The last SETTLED truth about the catalog, held across refreshes. The list
+ // itself is cleared while a refresh is in flight (fail-closed, for the `/`
+ // popup), so `length === 0` cannot tell "re-fetching" from "nothing to
+ // offer" — and the ＋ menu's Skills row repainting on that transient empty
+ // is a visible blink of the open menu on every mode, model or turn change.
+ // This flag only moves when a request resolves, so the row's presentation
+ // is stable until the catalog's emptiness actually changed.
+ const [mentionSkillsUnavailable, setMentionSkillsUnavailable] = useState(false);
Review Comment:
[P2] During the initial request, or immediately after switching session/project/model, `mentionSkills` is cleared fail-closed while this settled flag can still be `false`. The Skills row therefore looks enabled even though the current catalog is empty, and clicking it inserts a stray `/` before opening an empty popup. Could we model the catalog as one explicit `loading / ready-empty / ready-populated` state, keep the row geometry stable during loading, and ensure a loading click has no side effect? A regression covering an immediate click before the refreshed catalog settles would pin the boundary.
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -3291,26 +3296,16 @@ function AppShellContent({
: undefined
}
planModeActive={activePlanMode}
- planModePending={activeId
- ? pendingCollaborationModeBySession[activeId] === true
- : false}
- planModeDisabledReason={
- activeId && pendingCollaborationModeBySession[activeId] === true
- ? shellCopy.modeChanging
- : modeChangeDisabledReason
- }
+ // No pending-keyed disable while a toggle commits: the
+ // pending registries already swallow re-entrant toggles, and
Review Comment:
[P2] Now that pending is no longer reflected by the mode controls, the existing registry guard silently drops a second click while the row still appears enabled and retains its old checked state. A quick “on, then off” can therefore finish as “on” even though the user’s last action requested “off.” Could we either retain an accessible pending state without changing the menu geometry, or queue and apply the latest requested value after the current commit? A deferred-IPC test with two rapid toggles would cover this interaction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): keep the composer ＋ menu still when toggling Plan [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3469:
URL: https://github.com/apache/maka/pull/3469#discussion_r3835641726
##########
apps/desktop/src/renderer/use-composer-mentions.ts:
##########
@@ -33,18 +35,32 @@ export function useComposerMentions(options: {
newSessionPermissionMode,
newTaskTarget,
} = options;
- const [mentionSkills, setMentionSkills] = useState<InvocableSkillEntry[]>([]);
+ // One explicit representation of the Skill catalog — in flight, settled
+ // empty, or settled populated — held as a single value so a refresh can
+ // never tear its facets apart.
+ //
+ // `skills` is the live, fail-closed list the `/` popup reads: it is cleared
+ // the moment a refresh starts, because a visible popup must never advertise
+ // a Skill the new backend surface may not carry. That clear is exactly why
+ // `length === 0` cannot tell "re-fetching" from "nothing to offer", so the
+ // ＋ menu's Skills row renders from `settled` — the last RESOLVED verdict,
+ // held across refreshes — and repaints only when the catalog's emptiness
+ // actually changed. `loading` gates interaction: while a request is in
+ // flight (including the very first, before anything has settled), a click
+ // on the row must have no side effect — the held presentation is the OLD
+ // catalog's look, not a promise the current one can honor.
+ const [catalog, setCatalog] = useState<{
+ loading: boolean;
+ settled?: 'empty' | 'populated';
+ skills: InvocableSkillEntry[];
+ }>({ loading: true, skills: [] });
useEffect(() => {
let cancelled = false;
let requestVersion = 0;
- // Do not briefly advertise the previous session/project's Skills while the
- // authoritative projection is being refreshed. The same fail-closed reset
- // applies when a model/mode/plan or MCP event changes the backend surface:
- // a visible popup must never retain a now-unavailable Skill.
const refresh = () => {
const version = ++requestVersion;
- setMentionSkills([]);
+ setCatalog((previous) => ({ loading: true, settled: previous.settled, skills: [] }));
Review Comment:
[P3] A context change renders once with the previous catalog before this passive effect runs and clears it. That leaves a narrow window where switching session/project/model and immediately opening Skills can expose IDs from the old context. A context key stored with the catalog would let render fail closed as soon as the key changes; a populated → context switch → loading test would make the lifecycle explicit.
##########
packages/ui/src/composer.tsx:
##########
@@ -1682,11 +1706,44 @@ export const Composer = forwardRef<
// a stray slash and popping an empty menu. Say why it is
// unavailable: the panel this replaced showed "no skills
// available", and a silent grey row answers nothing.
- isDisabled={props.disabled || props.mentionSkills.length === 0}
+ //
+ // Only a SETTLED empty catalog counts. The host clears
+ // the list while re-fetching it (a Plan toggle or model
+ // change does that with this menu open), and rendering
+ // that transient `[]` as "no skills" grows this row by
+ // a description line and back — the menu visibly jumps.
+ // So the verdict is the host's settled flag when it
+ // supplies one, and the row repaints only when the
+ // catalog's emptiness actually changed.
+ isDisabled={
+ props.disabled
+ || (props.mentionSkillsUnavailable
+ ?? props.mentionSkills.length === 0)
+ }
description={
- props.mentionSkills.length === 0 ? copy.noSkillsAvailable : undefined
+ (props.mentionSkillsUnavailable
+ ?? props.mentionSkills.length === 0)
+ ? copy.noSkillsAvailable
+ : undefined
+ }
+ // Mid-refresh the row LOOKS like the previous catalog
+ // but cannot act for the current one: opening the `/`
+ // menu now would write a stray slash against a list
+ // that is fail-closed empty. A loading click is a
+ // complete no-op — nothing typed, menu left open — so
+ // the same click a beat later simply works. The class
+ // is the observable contract for that state.
+ className={
Review Comment:
[P2] While Skills are loading, this item still exposes an enabled `menuitem` to keyboard and screen-reader users even though activation is intentionally a no-op. The class has no ARIA meaning, so the control announces “available” but silently ignores the action. Could the loading state carry the component's disabled/busy semantics (while preserving the fixed geometry), and could a keyboard test pin that activation neither closes the menu nor writes `/`?
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -1100,6 +1118,19 @@ function AppShellContent({
collaborationModeChangeRegistry.keysRef,
setPendingCollaborationModeBySession,
);
+ // Whatever the user last asked for while this commit ran is the state
+ // they expect to land on. Read the settled mode through the ref — the
+ // closure's projection predates this commit — and only re-apply when
+ // the intent is not already satisfied.
+ const queued = queuedCollaborationModeBySession.current.get(sessionId);
Review Comment:
[P3] `clearSessionRendererState()` clears the pending registries but not these queued maps. If a Session is removed or Host readiness changes while a commit is pending, this `finally` can replay an old intent against the already-cleared lifecycle. Could cleanup delete both queued entries as well (or gate replay by a Session generation) and cover pending → cleanup → settle?
##########
apps/desktop/src/renderer/app-shell.tsx:
##########
@@ -427,8 +427,18 @@ function AppShellContent({
const [newChatPlanModeActive, setNewChatPlanModeActive] = useState(false);
const [newChatOrchestrationMode, setNewChatOrchestrationMode] = useState<OrchestrationMode>('default');
const [scheduledTaskCreateRequestNonce, setScheduledTaskCreateRequestNonce] = useState(0);
- const [pendingCollaborationModeBySession, setPendingCollaborationModeBySession] = useState<Record<string, boolean>>({});
- const [pendingOrchestrationModeBySession, setPendingOrchestrationModeBySession] = useState<Record<string, boolean>>({});
+ // Write-only: the pending-action helpers need a store, but nothing renders
+ // from these — a mode toggle's guard is the registry ref, and painting the
+ // round trip (disable/dim, then restore) is exactly the ＋ menu blink that
+ // MatrixA/fix-plan-click-flicker removed.
+ const [, setPendingCollaborationModeBySession] = useState<Record<string, boolean>>({});
Review Comment:
[P3] These two React states are now deliberately write-only, so every add/clear still schedules a render whose value nobody reads. The registry refs already own re-entrancy; removing the setter parameter and these states would leave one authority and avoid the extra render this PR is trying to eliminate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org