Skip to content

refactor(desktop): rebuild General, Appearance and Data settings on the row idiom - #5759

Merged
jackwener merged 2 commits into
apache:mainfrom
Astro-Han:refactor/settings-general-appearance-data
Sep 27, 2026
Merged

jackwener merged 2 commits into
apache:mainfrom
Astro-Han:refactor/settings-general-appearance-data

Conversation

@Astro-Han

Copy link
Copy Markdown
Contributor

Summary

Third settings slice of #4679 (after About in #4571/#4857 and Models in #4682): General, Appearance and Data move onto the same idiom — open Heading + Divider groups, one row shape, row-end actions as Button by whether they change state, at most one primary per page, no decorative Badge, no nested containers. Visual and structural only: no setting, persistence path or IPC changes.

General

  • No primary at rest. 保存 shell 设置 and 测试当前配置 were both primary; they are secondary (the proxy test commits a pending password draft first, so it changes state).
  • The auto-bypass note was an info Banner between two fields (accent as texture, C5); it is now the second sentence of the bypass field's help.
  • 高级设置 was a raw <details> wrapping a titled SettingsSection whose only row repeated the section title, with Save/Remove nested inside a FormLayout inside a field. It is now an Astryx Collapsible (still collapsed by default, per feat(workhub): add opt-in Jev assisted routing #5562) holding plain rows, moved to the end of the page. The markup moves into features/jev-settings (replacing the render-prop controller) because general-settings-page.tsx is ratcheted and may not gain the @astryxdesign/core/Collapsible edge.

Appearance

  • Workbar and font size use the rows body instead of bare; the workbar Switch no longer prints its label a second time beside the row label.
  • 浅色和深色用不同图标 is a switch row. The light/dark slot picker was a primary/ghost button pair acting as a segmented control; it is a SegmentedControl in the same row.
  • 导入图标… moves to the section header, the same slot as 导入 PetPack on this page; the PNG guidance stays as one supporting line under the grid. Group labels use one text style (they used two).
  • Pets: the 已关闭 / 正在使用 badges only restated the row text. The status row drops its badge; a selected pet says 正在使用 in its description. Row-end 删除 is secondary (the confirm dialog stays destructive).

Data

  • 打开工作区文件夹 / 复制路径 (ghost) sit on the workspace path row and 清空输入历史 (secondary, it had destructive with no confirmation) on the input-history row, instead of one loose cluster under both. Busy state uses isLoading instead of swapping labels, so opening / copying / clearing / actionsAria leave the catalog.
  • 备份与恢复 was a hand-styled callout floating between groups; it is a row in 数据位置.
  • Export contents were Switches, but nothing applies until 导出配置; Astryx reserves Switch for settings that apply immediately, so they are a CheckboxList (the credentials warning stays as its status). The conflict strategy is a label-left row instead of a full-width field. 导出配置 stays the page's one primary.

Removed: .settingsQuietCallout, .settingsRowsGroup, .jevAdvancedSettings and their rules.

Refs #4679

Before (main storybook-static) vs after, same stories, 1280 wide, pane heightened to fit. General is shown with the proxy and proxy auth switched on and 高级设置 opened, so the removed Banner and the three former primaries are in frame.

General, light

General, dark

Appearance, light

Appearance, dark

Data, light

Data, dark

Verification

  • npm run format, npm run lint: clean. npm --workspace @maka/desktop run typecheck (preload, main, renderer, storybook): clean.
  • check-renderer-architecture.mjs --base <merge-base>: passes (ledger unchanged). check:app-shell-hooks: ok. astryx:surface-inventory: regenerated, coverage ok. npx knip --workspace apps/desktop: clean.
  • Storybook built from this branch; every product-settings-pages--{general,appearance,data,pets}* story run light and dark through the smoke runner's smokeStory (play functions, console, AX audit): all pass; general-forced-colors-focus-ring passes with forced colors emulated, as the runner does.
  • Stories: PetsActionBadgeTypography asserted the badge font tier, which no longer exists; it becomes PetsSelectedThenDisabled (turning the pet off gives the row its 使用 action back). New AppearanceSplitAppIcons checks that choosing 深色 in the slot control moves the grid selection to the dark icon. DataCachedHostRevalidation now finds the categories as checkboxes.
  • CDP against the built stories: visible primary buttons General 3 → 0, Data 1 → 1, Appearance 0 → 0; destructive on Data 1 → 0; badges on Appearance 1 → 0; banners on General 1 → 0; every row-end control on all three pages ends at the same x (1196 at 1280 wide); 高级设置 loads with aria-expanded="false" and its trigger matches the section headings' left edge, size and weight; Data at a 720px viewport has no horizontal overflow.
  • Not run: the full smoke:storybook catalog and E2E (left to CI); no desktop unit test covers these pages.

AI use

Select exactly one:

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

Tool(s) and scope: Devin wrote the page changes, story updates, verification scripts, comparison images and this description.

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

…he row idiom

Third slice of the settings work in apache#4679, following About (apache#4571, apache#4857)
and Models (apache#4682). Visual and structural only: no setting, persistence
path or IPC changes.

General: no primary at rest (shell save and proxy test are secondary);
the auto-bypass info Banner folds into the bypass field help; the raw
<details> around a nested titled section becomes an Astryx Collapsible
of plain rows at the page end, still collapsed by default. The Jev
markup moves into features/jev-settings so general-settings-page keeps
its ratcheted dependency budget.

Appearance: Workbar and font-size groups use the rows body; the workbar
switch no longer prints its label twice; the dark/light icon split is a
switch row with a SegmentedControl instead of primary/ghost buttons;
import moves to the section header like the pets import; pets lose the
decorative Off/In use badges and the row-end destructive variant.

Data: open/copy/clear sit on the rows they act on (ghost, ghost,
secondary, with isLoading instead of label swaps); backup guidance is a
row instead of a loose callout; export contents are a CheckboxList
because nothing applies until Export; conflict handling is a row.

Removes the now-unused .settingsQuietCallout, .settingsRowsGroup and
.jevAdvancedSettings rules and four data copy keys.

Generated-by: Devin
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 27, 2026
The combined bypass help joined two catalog strings with a locale branch for the separator, which check:locale-hygiene rejects. Each locale now owns the whole sentence pair as one bypassHelp(count) entry.

Generated-by: Devin

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

[kabi-sol]

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

No P0–P2 found in the five behavior-preservation paths reviewed at cdf3e962274d05d7ea5db8776e40f4fa5412dab2.

Mounted the production settings components with controlled services: proxy Test waits for the password draft save; advanced settings start collapsed and preserve Save/Remove payloads, failed-save draft retention and busy guards; export checkbox selections reach the correct Host with the plaintext-credentials warning; clearing input history retains its original one-click behavior; light/dark icon target changes synchronize the grid and subsequent selection target. The old destructive button did not provide a confirmation dialog.

Desktop dependency/main builds and 59 focused tests passed. These were LinkeDOM/component probes, with input changes invoking rendered handlers—not real-browser, Electron, actual IPC/persistence, accessibility, or full-suite verification. Source was compared with the PR base; no production files changed.

@zhiiw zhiiw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bound to cdf3e962274d05d7ea5db8776e40f4fa5412dab2 (re-checked against GitHub immediately before posting, unmoved). Draft; CI test green on this head. Real Windows machine, Node 24.18.1, real Chromium (Playwright headless shell).

No P0–P2, no P3. The rewrite is genuinely visual/structural, and the two story guards I ablated bite exactly their own story.

Storybook smoke on this machine (436 stories, 473 theme renders, play functions execute)

  • Baseline: full pass. Two transient concurrency failures (model-picker--executor-catalog-ready, workhub--usage-inspector) each passed alone under the script's built-in retry — the same two flaked identically in every run here, including ablation runs where everything else was untouched; they are machine-concurrency noise, not this PR.
  • Ablation ① (slot↔grid linkage). Forced the app-icon grid to always edit the light appearance (appearance-settings-page.tsx, editedAppIcon) → exactly product-settings-pages--appearance-split-app-icons fails on its toBeChecked() assertion after switching the slot to Dark, and it does not pass alone. Restored, rebuilt, full-suite green again.
  • Ablation ② (export categories back to Switches). Replaced the CheckboxList with per-row Switch rows in data-settings-page.tsx → exactly product-settings-pages--data-cached-host-revalidation fails ("Unable to find role checkbox 模型连接"). Restored, rebuilt, full-suite green again (final run: 436/436 with zero retries).

Occam check: is moving the jev advanced settings into the feature overreach?

No. The jev section lives on the General page, so rewriting it onto the row idiom is inside the PR's stated scope. What changed in features/jev-settings is the shape of the same section: the render-prop controller (JevSettingsController in the feature, all presentation in the page) became a self-contained JevSettingsSection with identical behavior — same Switch disable rule (!settings.apiKey), same save/clear semantics, same copy. That removes a split-brain component whose only consumer was this one section, and it matches how the rest of the settings pages own their sections. The presentational deltas (details/summary → Astryx Collapsible, primary → secondary actions) are exactly the page-wide conventions this PR establishes elsewhere. The proxy-bypass second commit is the same kind of consolidation: the auto-bypass Banner folds into the field's description line — one catalog entry, same information.

Behavior preservation spot-checks

  • The export-category control is a CheckboxList with an explicit, correct rationale in code: nothing takes effect until 导出, and Astryx reserves Switch for immediate-effect settings. The sensitive-category warning status survived the rewrite.
  • Settings unit suites on Windows: client-settings-effects, pet-pack-import, optimistic-settings-draft-controller — 22 pass / 0 fail (2 skipped).
  • AppearanceSplitAppIcons's sibling pet story (PetsSelectedThenDisabled) exercises the action/badge hand-back in the new row shape and passed in every smoke run.

Not checked

e2e, full repo suite, a live app window.

UTC 2026-09-27 13:17.

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

Approving at cdf3e962274d05d7ea5db8776e40f4fa5412dab2. No P0–P3 found.

  • Behavior preserved on the five risky paths, checked by mounting the production settings components: the proxy test still commits the pending password first; Jev advanced settings stays collapsed by default with save/remove, failure and busy states intact; export checkboxes feed the export and the plaintext-credentials warning still shows; clear input history is still a single-click action (it had no confirmation before either); the light/dark slot control and icon grid stay in sync.
  • Storybook smoke run locally (436 stories, play functions executed): all pass. Ablation: breaking the slot↔grid link reddens exactly appearance-split-app-icons; reverting export categories to switches reddens exactly data-cached-host-revalidation.
  • Moving the Jev markup into features/jev-settings stays in scope: it merges a render-prop split into a self-contained section with the same behavior.
  • CI test green.

Not run: E2E, full suite, real Electron window, real IPC/file persistence.

Reviews: #5759 (review) , #5759 (review)

@jackwener
jackwener marked this pull request as ready for review September 27, 2026 13:39
@jackwener
jackwener merged commit 592d2d4 into apache:main Sep 27, 2026
2 checks passed
@Astro-Han
Astro-Han deleted the refactor/settings-general-appearance-data branch September 27, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants