fix(desktop): clarify and filter status summaries - #3509
Conversation
Render verified permission grants with the success semantic and make the Permission Center and Health Center status summaries shared, accessible list filters. Generated-by: Codex
aaea3a3 to
849afcc
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — turning a static count into the control that filters to exactly those items is the right instinct, and correcting Granted from neutral to success closes a real gap: "we checked and it is fine" and "we have not checked" were rendering identically. Reusing one SettingsStatusSummaryFilter across both the Permission and Health pages, rather than writing the interaction twice, is what keeps this at 227 lines.
Reviewed at exact head 849afcc0324ef626f70193c2d3108c718e8bf8d3 against base 1ddbd2591ccc6fd06d9727a216e6a5b64d8b1e40. One P3, nothing higher. The head has a completed test check but the aggregate commit status is pending, so this is a COMMENT, not an approval.
[P3, ordinary path] On the Health page, filtering narrows the list but not the blocker banner.
health-center-page.tsx:114-117 derives visibleSignals from signalFilter and renders the list from it. Two lines below, blocksSendCount and blocksCapabilityCount (:118-119) are computed from the unfiltered snapshot.signals, and the banner at :161-173 renders from those.
So: filter to ok, and the list shows only healthy signals while the banner above it still reads "2 health signals block sending". Nothing displayed is false — the banner is a true statement about the system — but the two regions are now describing different sets on the same screen, and the page gives no cue which is which.
There is a good argument for leaving it exactly as is, which is why this is a P3 and not higher: a blocker banner is arguably a global alert, and having a view filter hide the fact that you are blocked would be worse than the inconsistency. If that is the intent, the fix is wording rather than logic — something that marks the count as spanning all signals regardless of the current filter would remove the ambiguity without weakening the alert. Your call; we would just rather it be a decision than an artifact.
Checked and found sound: the filter is view-only — it adds no path to granting a permission, mutating health state, or changing runtime status. Zero-count buttons are disabled, selection is exposed via aria-pressed, re-selecting restores the full list, and the effect clears a filter whose status no longer exists in a refreshed snapshot, so a stale filter cannot survive into a new page state. On the Permission page the counts and permissionIdsForFilter are both derived from the same OS_PERMISSION_IDS set and the same status mapping, so no divergence there. buildHealthSnapshot (packages/core/src/health.ts:82-94) recomputes the summary from the same signals, so the snapshot itself is internally consistent — the P3 above is strictly about the filtered view, not the data.
How this was reviewed, including a disagreement we had to settle. Two reviewers worked this independently without seeing each other's conclusion. One reported the banner inconsistency; the other reported no findings, having checked summary-versus-signals consistency in core and found none. Both were right about what they actually examined — those are two different questions, and only one of them is about the filtered view. We read the component ourselves to settle it, and the P3 stands.
@maka/core and @maka/ui build; core health tests 6/6 and the added permission copy test 1/1. The desktop renderer build could not start in our environment (missing @vitejs/plugin-react) and desktop typecheck is blocked on unbuilt workspace exports — both pre-existing environment limits, not attributable to this change, and we are reporting them as not run rather than as results.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
Keep Health Center blockers global while making their full-snapshot scope explicit under status filtering. Add localized copy coverage and extend the Health Center story interaction assertions. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this resolves the point from our previous comment, and it resolves it the better of the two ways we suggested.
Re-reviewed at exact head d54a087da11f6d9e16048e017daa11e927135edf. Our earlier review was against 849afcc0, so that conclusion no longer applied and this is a fresh pass over the current code.
The blocker-count ambiguity is closed. Rather than making the banner follow the filter — which would have let a view filter hide the fact that sending is blocked — you kept it global and said so in the text: Across all health signals, N of M block sending, with the matching Chinese copy and a health-center-copy.test.ts case pinning both. The count now states its own scope, so a filtered list sitting beneath it no longer reads as a contradiction. That is the right trade: the alert keeps its reach and the confusion goes away.
The rest of the filtering behaviour is unchanged from our previous pass and still checks out — view-only, no new path to granting a permission or mutating health state, zero-count buttons disabled, aria-pressed on selection, and a stale filter cleared when a refreshed snapshot no longer contains that status.
No findings — no P0, P1, P2 or P3. test is green on this head. Approving.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
Summary
The Permission Center previously rendered a verified
Granted/已授权state with the same neutral tone as unknown or unsupported permissions. Its status counts, and the equivalent counts in Health Center, also looked interactive but were static text.This change:
Verification
npm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run build:renderernpm --workspace @maka/desktop run build-storybook -- --quietnpm run astryx:surface-inventorynpx biome check apps/desktop/src/renderer/settings/settings-status-summary-filter.tsx apps/desktop/src/renderer/settings/permission-center-page.tsx apps/desktop/src/renderer/settings/health-center-page.tsx apps/desktop/src/renderer/locales/permission-center-copy.ts apps/desktop/src/renderer/locales/settings-health-copy.ts apps/desktop/src/renderer/styles/settings/health.css apps/desktop/stories/settings/settings-pages.stories.tsxScreenshots
Permission Center, narrow viewport:
Health Center filtered to errors:
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex investigated the misleading status summaries, implemented the shared filtering behavior and accessibility states, added Storybook interaction coverage, ran verification, and prepared this PR under human direction. The commits include the required
Generated-by: Codextrailer where applicable.Checklist
Does this PR entail a change in behavior?