Skip to content

fix(desktop): clarify and filter status summaries - #3509

Open
hqhq1025 wants to merge 2 commits into
apache:mainfrom
hqhq1025:codex/permission-granted-success
Open

fix(desktop): clarify and filter status summaries#3509
hqhq1025 wants to merge 2 commits into
apache:mainfrom
hqhq1025:codex/permission-granted-success

Conversation

@hqhq1025

@hqhq1025 hqhq1025 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

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:

  • maps verified permission grants to the existing success semantic;
  • turns the Permission Center counts into real status filters;
  • applies the same shared filter component to Health Center;
  • lets users press an active filter again to restore the full list;
  • disables zero-count filters and exposes pressed/filter state to assistive technology;
  • resets a selected filter if refreshed data no longer contains that status.

Verification

  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop run build:renderer
  • npm --workspace @maka/desktop run build-storybook -- --quiet
  • npm run astryx:surface-inventory
  • npx 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.tsx
  • Playwright interaction and responsive visual checks for both pages in dark mode with the Chinese locale

Screenshots

Permission Center, narrow viewport:

Permission Center status filters

Health Center filtered to errors:

Health Center error filter

AI use

Select exactly one:

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

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: Codex trailer where applicable.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck, builds, and affected interaction checks pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@hqhq1025 hqhq1025 changed the title fix(desktop): distinguish granted permission status fix(desktop): clarify and filter status summaries Aug 22, 2026
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
@hqhq1025
hqhq1025 force-pushed the codex/permission-granted-success branch from aaea3a3 to 849afcc Compare August 22, 2026 15:57

@Astro-Han Astro-Han 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.

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
@hqhq1025
hqhq1025 requested review from Astro-Han and M4n5ter August 22, 2026 17:12

@Astro-Han Astro-Han 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants