fix(ui): polish command palette modal chrome - #1738
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
P3. The inline style object on CommandPaletteInput (command-palette.tsx:155-161) is dead: palette.css:31-40 sets the same five properties with !important, which outranks inline styles. Keep one source.
P3. theme.ts observes the whole document with subtree: true and runs querySelector('dialog[open]') on every mutation for the app's lifetime; <details open> toggles also fire the attribute branch. Bail on nodeName !== 'DIALOG' before querying.
3a2de42 to
70d5e44
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Review findings (independent deepseek-v4-flash review, 2 passes)
Note: the earlier approval predates the current head; the head is now in a worse state (two CI gates red).
P1 — search input collapses: passing className to CommandPaletteInput replaces Astryx's own classes
@astryxdesign/core/src/CommandPalette/CommandPaletteInput.tsx:237-238—<input {...stylex.props(styles.input)} {...props} />: the consumerclassNamespreads after the internal classes and overwrites the whole class attribute. Lost atoms includeflex:1,min-width:0,padding:0, font/color,::placeholder(dist/astryx.css.x98rzlu{flex:1}etc.).command-palette.tsx:150passesclassName="maka-command-palette-search";palette.css:30-38restores appearance/border/outline/background/box-shadow with!importantbut notflex/min-width. Result: the search box shrinks to intrinsic width (~170px) instead of filling the header row.- Fix: drop the className on the input (selector
.maka-command-palette input[role="combobox"], addflex:1; min-width:0) or wrap in a styled container — do not fight the component's replace-semantics contract.
P1 — --shadow-modal is an undefined token → the whole box-shadow declaration is dropped; CI e2e is red
palette.css:12-14box-shadow: var(--shadow-modal), 0 24px 72px …—--shadow-modalis defined nowhere in the repo (only anerror-boundary.tsxcomment calls it "the former shadow-modal"; usevar(--shadow-high)/--shadow-medium). IACVT kills the entire declaration → dialog has no shadow.- CI e2e fails at
command-palette.spec.ts:20(not.toHaveCSS('box-shadow','none')→ Received "none").
P2 — e2e spec conflicts with main (modify/delete)
apps/desktop/e2e/command-palette.spec.ts was deleted on main by #1936; this PR modifies it → merge conflict. Decide explicitly: port the computed-style contract into a surviving journey, or drop it (but then this PR's visual contract has zero test coverage).
P2 — Dead-CSS CI gate red
check-dead-css.mjs --check reports .astryx-command-palette-input as a dead class (runtime-generated by themeProps, not in DYNAMIC_STYLE_HOOKS), exit 1 — typecheck job fails. Add it to the whitelist with a comment, or use a selector that doesn't depend on the runtime class.
P3 — theme.ts new compositing logic (observeModalBackdrops / cssColorToRgba / rgbaToHex) has zero automated tests; the PR body's "titlebar source-contract coverage" doesn't correspond to any changed file.
Gate: FAIL — both P1s are reproducible (one already red in CI); fix before merge.
|
/agentic_review |
Code Review by Qodo
1. Wrong backdrop for stacked modals
|
| const openDialogs = document.querySelectorAll<HTMLDialogElement>('dialog[open]'); | ||
| const activeDialog = openDialogs.item(openDialogs.length - 1); | ||
| if (!activeDialog) return baseColor; |
There was a problem hiding this comment.
1. Wrong backdrop for stacked modals 🐞 Bug ≡ Correctness
compositeActiveModalBackdrop assumes the last open dialog in DOM order is the active modal and composites only its backdrop, but modal stacking follows opening order and every stacked modal contributes a backdrop. For example, opening the palette with Ctrl/Cmd+K and then keyboard help with Ctrl/Cmd+/ leaves both states open; help is topmost, while this query selects the later-rendered palette and produces the wrong Windows titlebar color.
Agent Prompt
## Issue description
Native titlebar backdrop synchronization derives the active modal from DOM order and ignores other stacked modal backdrops. Independent global shortcuts can open multiple shell dialogs, so the selected backdrop and resulting composited titlebar color can differ from Chromium's top-layer rendering.
## Issue Context
Prefer consolidating modal authority through the existing shell overlay state so only one shell modal can be active at a time. If simultaneous modals are intentionally supported, track modal activation order and composite every active modal backdrop in top-layer order; a `dialog[open]` DOM query alone cannot recover that ordering.
## Fix Focus Areas
- apps/desktop/src/renderer/theme.ts[140-159]
- apps/desktop/src/renderer/app-shell.tsx[469-470]
- apps/desktop/src/renderer/app-shell-overlays.tsx[126-146]
- apps/desktop/src/renderer/command-palette.tsx[36-45]
- apps/desktop/src/renderer/keyboard-help.tsx[51-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
Summary
Refs #1735.
dialog::backdropover the resolved theme surface and reusing the existing titlebar-overlay bridge.Root cause
The command palette still used Astryx's generic dialog treatment while the keyboard-shortcuts dialog had moved to the stronger Maka modal treatment. The globally applied focus ring also painted a second rectangular control around the auto-focused search input. On Windows, Chromium's
::backdropcannot cover Electron's native caption controls, so the top-right control strip stayed at the undimmed theme color while the renderer behind the dialog was blurred and darkened.Screenshot
Verification
npx biome check apps/desktop/e2e/command-palette.spec.ts apps/desktop/src/renderer/command-palette.tsx apps/desktop/src/renderer/styles/palette.css apps/desktop/src/renderer/theme.ts apps/desktop/src/main/__tests__/theme-source.test.tsnpm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run build:mainnode --test apps/desktop/dist/main/__tests__/theme-source.test.js— 11 passednpm --workspace @maka/desktop run build:renderergit diff --checkThe targeted Playwright command-palette run was attempted locally but the harness currently stops while loading
@slack/socket-mode/@slack/web-api, before discovering tests. CI remains the authoritative Electron E2E run.中文说明
概要
dialog::backdrop与主题背景的实际合成颜色同步 Windows 原生标题栏按钮区域,避免右上角出现未变暗的色块。