fix(settings,ui): kenji audit round 2 findings 1, 2(prev), 5 — Memory wrap, Plan Reminder bot logos, marginTop classes - #247
Merged
Conversation
@kenji audit thread `#my-ai:4821a792` msg `e4cfbfb0` vs main `71d88454`. WAWQAQ msg `782a1663` + `继续`: bundle these, don't split. **#1 — Memory page top control rows had no outer card surface.** The three control rows (本地 MEMORY.md / 模型上下文可读取 / 项目指令 文件) were direct children of `.settingsStructuredPage`, which is transparent. `.settingsFormRow` only carries a hairline bottom border, so users saw three floating rows without the grouped-card visual every other settings surface uses. Wrap them in `.settingsRows` so they inherit the 12px outer border + radius + overflow:hidden treatment. **#2 — Plan Reminder delivery picker showed text-only IM channels.** Settings → 机器人对话 switched to real brand logos (Iconify simple-icons) in #BOT-SETTINGS-UI-0, but the chat-side Plan Reminder delivery flow's `平台` select was still rendering plain Chinese labels. Same channels should read the same everywhere. - Move `BOT_BRAND` to `packages/ui/src/bot-brand.ts` so both surfaces share a single brand-metadata source instead of one duplicating the other. - Extend `PlanReminderSelect` to accept a third tuple slot (optional `ReactNode` icon); `PlanReminderPanel` now passes an `<IconifyIcon icon={brand.iconifyId}>` per provider with the existing offline glyph fallback. - Update `SettingsModal.tsx` to import `BOT_BRAND` from `@maka/ui` instead of declaring a local copy. **#5 — Two inline `style={{ marginTop }}` spacing patches.** - `Alert variant="error" style={{ marginTop: 12 }}` → `className= "settingsSurfaceAlert"`. - `<div className="settingsActionRow" style={{ marginTop: 8 }}>` → `className="settingsActionRow settingsNoticeAction"`. The new classes pin the spacing decision in CSS where the design tokens live, instead of inline JSX. Deferred per kenji's own scope note + WAWQAQ 「只看最后的结果」: - **#3** (`RadioCard` / `ChoiceCard` primitive for theme/palette picker) — pure design-system refactor, no visible delta on Settings → 外观. - **#4** (`SettingsSelect` uplift to rich-option contract) — bigger cross-cutting refactor; current fix covers the immediate model-name and disambiguation issues kenji raised in round 1. - **#6** (layout-property transitions in sidebar/tabs/accordion) — kenji explicitly flagged these as visual-smoke-gated; will land alongside the smoke config in a separate PR.
4 tasks
jackwener
added a commit
that referenced
this pull request
Jun 25, 2026
… dvh (#250) @kenji audit msg `232aec0f` (`#my-ai:c28a6293`). Round 3 against `af66ebb7` post-#249. **#2 — Plan Reminder select collapsed state was text-only.** PR #247 added a brand icon to each `<SelectItem>`, but `<SelectValue />` defaulted to rendering just the label string — open dropdown showed a logo, collapsed trigger showed plain text. Build a `value → { label, icon }` lookup inside `PlanReminderSelect` and pass a function-child to `SelectValue` so the picked state renders the same icon + label row the dropdown items render. **#6 — `100vh` residue in renderer CSS.** Four sites still on `100vh`: - `maka-tokens.css:760` `.maka-shell` `height` - `maka-tokens.css:1941` `.maka-modal` `max-height: calc(100vh - 80px)` - `styles.css:5028` `.maka-help-modal` `max-height: calc(100vh - 96px)` - `styles.css:13121` `.maka-onboarding-stack` `min-height: calc(100vh - 84px)` Replace each with `100dvh` so Electron browser frames with dynamic chrome (or a future mobile-style viewport) don't get half a viewport of height calculation drift. Other renderer surfaces already use `100dvh`; this brings the legacy four into line. Deferred per kenji's own scope: - **#1** feishu/dingtalk official-brand-kit sourcing — separate PR. - **#3 / #4** SettingsSelect / Segmented / ChoiceCard primitive unification — design-system refactor; no end-result delta on its own. - **#5** layout-property transitions in sidebar/tabs/accordion — kenji explicitly gated on visual smoke harness.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Acts on @kenji audit thread
#my-ai:c28a6293msge4cfbfb0. Bundled per WAWQAQ's directive ("一次性多改").#1 — Memory page top control rows lacked a card surface. Wrap the 3
.settingsFormRowblocks in<div className=\"settingsRows\">so they inherit the grouped-card outer border + 12px radius + overflow:hidden treatment used elsewhere in settings.#2 (kenji round-1 #2 / round-2 #3) — Plan Reminder bot picker is now text-only.
BOT_BRANDtopackages/ui/src/bot-brand.tsso both Settings and Plan Reminder share one brand-metadata source.PlanReminderSelectto accept an optionalReactNodeicon as the 3rd tuple slot.<IconifyIcon icon={brand.iconifyId} fallback={brand.glyph}>per provider in the Plan Reminder delivery picker.#5 — Two inline
style={{ marginTop }}patches.Alert variant=\"error\" style={{ marginTop: 12 }}→className=\"settingsSurfaceAlert\";<div className=\"settingsActionRow\" style={{ marginTop: 8 }}>→className=\"settingsActionRow settingsNoticeAction\". New classes live instyles.css.Deferred (per kenji's own scope + WAWQAQ 「只看最后的结果」):
SettingsSelect/Segmented/ChoiceCardunification) — bigger cross-cutting; current fix covers the immediate model-name and Plan-Reminder-icon issues.Test plan
tsc --noEmit -p apps/desktop/tsconfig.renderer.json— clean on touched lines.pnpm -F @maka/ui build+pnpm -F @maka/core build— clean..settingsSurfaceAlert)..settingsNoticeAction).