Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): reduce the About page to rows of one shape - #4857
Conversation
0f0b32e to
99b276cCompare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed at exact head 6d61212e. The bug this fixes is real, and the parts of the description that carry weight hold up when checked against the service. No P0 or P1. Three small things below, plus the state this PR is currently in.
Verified rather than taken on trust
The dead button is genuinely dead.checkForUpdates() returns the current status untouched when the state is verifying, downloaded or installing, when it is downloading without allowDuringDownload, and whenever an activeDownload is in flight. checkForUpdatesNow skips the focus throttle but still goes through that guard. Since updater.autoDownload = true and a nightly settles at downloaded, the old always-enabled 检查更新 was a no-op every time. Offering the control only where the service acts on it is the right correction.
"A failed download is re-fetched by the same check" is true, so dropping the separate retry is safe.activeDownload is cleared in the tracker's finally, error is not in the guard set, and autoDownload is unconditional — so a check from the error row runs updater.checkForUpdates() and the download restarts on its own. retryUpdateDownload is also not orphaned by this: app-shell.tsx:758 still calls it.
"The restart is at the bottom of the sidebar" points at something that exists. The footer renders from updateReminderFromStatus(appUpdateStatus) and dispatches the install at app-shell.tsx:724, so a downloaded row is directing the user to a live control rather than describing one.
The state mapping is total, and the copy behind it is complete.aboutUpdateRow returns on all nine states with no fallthrough, so the declared return type is what keeps it exhaustive. updateFailed is typed Record<'check' | 'download' | 'install', string>, which is exactly the set of publishError(...) operations, and all three catalogs carry all three entries. The architecture ledger loses exactly the @astryxdesign/core/Kbd edge the description claims.
[P3] One sentence in the description does not match the guard
aboutUpdateRow… maps the state to a label, a description and whether 检查更新 is offered: only where the service honours a check (idle, up to date, failed).
available is not in the service's short-circuit set, so a check is honoured there too, yet the row maps it to action: 'none'. The behaviour is still right — autoDownload makes available a transient state on the way to downloading, and re-checking would only rediscover the same release — but the stated reason is not the reason. Worth rewording to something like "only where a check can change what the row says", so a later reader does not rely on an invariant the service does not provide.
[P3] The privacy removal is a content decision riding in a layout change
The privacy commitments list is deleted, the route header no longer promises them, and the e2e assertion for the three commitments goes with it. The test removal is legitimate — the feature is gone, not relocated, which is the case where deleting its test is correct.
The content decision is a separate matter. An About page dropping its privacy statements is user-visible and, on an Apache project page, is the kind of thing a maintainer should agree to deliberately rather than inherit from a PR about row shapes. It may well be the right call — the description's argument that four facts and three actions do not need six control vocabularies is a good one. It just should be an explicit yes, not a side effect. The same goes, much more minorly, for 报告问题 losing its external-link icon: the link still opens a new tab, and the affordance that said so is gone.
Current state
This PR is a draft, GitHub reports it CONFLICTING / DIRTY, and no check runs have reported on 6d61212e. So this review is a read of the code, not a merge-readiness statement — a rebase will move the head and anything above binds to 6d61212e only.
Once it is rebased and green, nothing here blocks it.
简体中文
在 exact head 6d61212e 上评审。这个 PR 修的 bug 是真的,而且描述里几处承重的主张,拿服务端代码核过之后都站得住。 无 P0/P1。下面是三件小事,以及这个 PR 目前的状态。
核过而不是采信的部分
那个按钮确实是死的。checkForUpdates() 在状态为 verifying、downloaded、installing 时,在 downloading 且未 allowDuringDownload 时,以及在存在 activeDownload 时,都会原样返回当前状态。checkForUpdatesNow 只是跳过焦点节流,仍要过这道守卫。而 updater.autoDownload = true、nightly 的稳态就是 downloaded,所以旧的那个常亮的 检查更新 每次都是空操作。只在服务端真会响应的地方提供这个控件,是正确的修法。
「失败的下载由同一个检查重新取回」这句是真的,所以去掉单独的重试是安全的。activeDownload 在 tracker 的 finally 里被清空,error 不在守卫集内,而 autoDownload 是无条件的 —— 因此从错误行发起的检查会执行 updater.checkForUpdates(),下载自行重启。retryUpdateDownload 也没有因此变成死代码:app-shell.tsx:758 仍在调用它。
「重启在侧栏底部」指向的是真实存在的东西。 页脚由 updateReminderFromStatus(appUpdateStatus) 渲染,并在 app-shell.tsx:724 派发安装,所以 downloaded 那一行是把用户指向一个活的控件,而不是描述一个控件。
状态映射是全的,背后的文案也是齐的。aboutUpdateRow 在九个状态上都有返回、无穿透,是声明的返回类型在保证穷尽性。updateFailed 的类型是 Record<'check' | 'download' | 'install', string>,恰好等于 publishError(...) 的操作集合,而三份文案目录都带齐了这三条。架构账本恰好只少了描述里说的 @astryxdesign/core/Kbd 这条边。
[P3] 描述里有一句与守卫不符
只在服务端会响应检查的地方(idle、已是最新、失败)提供 检查更新。
available不在服务端的短路集里,所以那里的检查同样会被响应,而这一行却把它映射成 action: 'none'。行为仍然是对的 —— autoDownload 让 available 只是通往 downloading 的过渡态,重新检查也只会再发现同一个版本 —— 但给出的理由不是真正的理由。 建议改写成「只在检查能改变这一行内容的地方」之类,以免后来的读者依赖一个服务端并不提供的不变量。
[P3] 隐私内容的删除,搭在了一次布局改动里
隐私承诺列表被删除,路由标题不再承诺它们,e2e 里那条「三条承诺」的断言也随之移除。测试的移除是正当的 —— 这个功能是被删掉而不是被搬走,而这正是删掉其测试属于正确做法的情形。
内容决定则是另一回事。一个「关于」页面撤下它的隐私声明是用户可见的,而且在一个 Apache 项目的页面上,这类事该由维护者刻意同意,而不是从一个讲行样式的 PR 里顺带继承。它很可能是对的 —— 描述里「四条事实加三个动作不需要六种控件词汇」这个论证是好的。只是它应当是一次明确的「是」,而不是一个副作用。 同理但轻得多:报告问题 失去了外链图标,链接仍会新开标签页,而那个提示此事的可供性没有了。
当前状态
这个 PR 是草稿,GitHub 报告为 CONFLICTING / DIRTY,且 6d61212e 上没有任何 check 结果。所以本条评审是对代码的阅读,不是可合并性的陈述 —— rebase 会移动 head,上面所有内容仅绑定 6d61212e。
rebase 并转绿之后,这里没有东西阻塞它。
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.
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 6d61212e. Nothing found in the review above rises past [P3], and the correction at the centre of this PR is verified against the service rather than taken from the description: checkForUpdates() short-circuits on verifying / downloaded / installing, on downloading without allowDuringDownload, and whenever an activeDownload is in flight — so with autoDownload on and a nightly settling at downloaded, the old always-enabled 检查更新 really was a no-op.
Two notes that do not affect this approval:
- The PR is still a draft and GitHub reports it
DIRTY. Under this repository's review rule an approval stays valid across later pushes, so this one survives the rebase — but the rebase and a greentestare still required before it can merge. - This is an automated review. Per
CONTRIBUTING.md, the approving review that satisfies branch protection must be an independent human judgment from a committer other than the author, and this is not that.
The two [P3]s stand as suggestions rather than conditions: reword the "only where the service honours a check" sentence, since available is not in the guard set even though the row correctly offers nothing there; and let the removal of the privacy commitments be an explicit maintainer decision rather than a side effect of a layout change.
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.
设置 → 关于 had three problems: 检查更新 was a no-op on a nightly, whose steady state is `downloaded` and which the service refuses to re-check; the three support controls were three faces (ghost button + keycaps, bare link, ghost button) with three right edges; and the page read as clutter — six control vocabularies, two blues and an orange, a bulleted list inset from the rows around it. The page is now the Astryx settings idiom and nothing else: rows of label + one quiet line + one control at the end, grouped under `Heading 3` + `Divider`. Which control is Astryx's own rule: Button for actions, Link for navigation; among buttons, secondary or ghost "based on emphasis". - The lead group is two rows: the build (`Maka v…` with the channel sentence under it) and the updater's state. The row offers 检查更新 (secondary: it changes the updater's state) only where the service honours a check (idle, up to date, failed); a downloaded update says the restart is in the sidebar footer, which owns that handshake, instead of growing a second one here. A failed download is re-fetched by the same check, so there is no retry. Not primary: the page has no task to complete, and the restart is the update flow's one CTA. - 复制 and 查看 are ghost buttons (they change nothing); 报告问题 stays a Link and takes the button's inline inset so both faces end on one text edge. Busy states use `isLoading`. - Kbd, Token, the wordmark, the privacy list, the 开源 group and its heading are gone. Provenance is one supporting line with two links. - Copy: the token names, privacy points, update hints, `copying` and the deleted headings leave all three catalogs; the route header no longer promises privacy commitments. The About story's play (where #4803 moved the e2e) asserts the reduced page. `about-update-status.ts` no longer imports `app-shell-app-update.ts`, which is the legacy→legacy edge the renderer ratchet rejected; the Astryx surface inventory is regenerated. Generated-by: Claude Code
0f05be8 to
0243cb2CompareUh oh!
There was an error while loading. Please reload this page.
Summary
Three things were wrong with 设置 → 关于 after #4571:
checkForUpdatesNowreturns the current status untouched while an update is downloading, verifying, downloaded or installing (app-update-service.ts), but the page kept rendering an enabled 检查更新 through all of those. Nightly auto-downloads, so its steady state isdownloaded— the button was a no-op every time a nightly user pressed it.Link, 键盘快捷键 was a ghost button — three faces, three right edges (1108 / 1196 / 1184 px on a 1280-wide story).This PR reduces the page to the Astryx settings idiom and nothing else — the row shape from the CLI's
settings-sidebartemplate: a label, one quiet line under it, one control at the end; groups areHeading 3+Divider. Which control is Astryx's own rule:Button"is for actions like saving, deleting, or submitting",Linkis for "navigating between pages or to external URLs" and is not to be used "for actions that do not navigate" (the two components' docs). So 检查更新, 复制 and 查看 aresmbuttons — 检查更新secondarybecause it changes the updater's state, the two support actionsghostbecause they change nothing; notprimary, because the page has no task to complete and the one action the update flow cannot do without (the restart) is the sidebar reminder's. 报告问题 and the footer links areLinks; the row-end link takes the button's inline inset (--spacing-3) so both faces end on one text edge.Maka v…with the channel sentence under it) and the updater's state.aboutUpdateRow(pure, unit-tested) maps the state to a label, a description and whether 检查更新 is offered: only where the service honours a check (idle, up to date, failed). A downloaded update says the restart is at the bottom of the sidebar, which already owns that handshake, instead of growing a second one here; a failed download is re-fetched by the same check (the updater downloads on its own once it sees a release), so there is no separate retry.isLoadinginstead of swapping the label.Heading+Tokenlead, the wordmark,Kbd, the privacy list, the 开源 group. Provenance is one supporting line —Apache Maka (incubating) · Apache License 2.0 · 源码 · 发行说明. The route header no longer promises privacy commitments.Copy: the token names, privacy points, update hints,
copyingand the deleted headings leave all three catalogs;updateCheckFailed/updateCheckFailedDetailbecomeupdateFailed[operation]so a failed download is not reported as a failed check. The zh-TW privacy list (five items where zh-CN and en had three) goes with the block.Nothing on the page is styled outside Astryx tokens; no CSS changes.
about-update-status.tsno longer importsapp-shell-app-update.ts, so the renderer architecture ledger only loses the@astryxdesign/core/Kbdedge; the Astryx surface inventory is regenerated.Before (
main) vs. after, nightly with a downloaded update, same viewport:Dev checkout, where there is no update row:
The failed-download story, which has no
maincounterpart:Verification
npm run format,npm run lintat the root: clean.apps/desktop:tsc -p tsconfig.renderer.jsonandtsconfig.main.jsonclean;tsconfig.storybook.jsonreports only the three pre-existing Astryx prop errors inpackages/ui.node scripts/check-renderer-architecture.mjs --base bc08195fd5andcheck:app-shell-hookspass.node --test dist/main/__tests__/about-update-status.test.js(5 pass);npx playwright test --config e2e/playwright.config.ts e2e/about-page.spec.ts(1 passed).product-settings-pages--about,--about-nightly,--about-release,--about-update-failed, light and dark, 1280 wide: every row-end control's text measures the same right edge.AI use
Select exactly one:
Tool(s) and scope: Claude Code drafted the page, the state mapping, tests, stories and this description under review.
Checklist
Does this PR entail a change in behavior?