Skip to content

fix(core): a blank client-hints platform is unknown, not non-Apple - #5325

Merged
cixzhang merged 1 commit into
facebook:mainfrom
Astro-Han:fix/5253-blank-ua-platform-apple-detection
Aug 23, 2026
Merged

fix(core): a blank client-hints platform is unknown, not non-Apple#5325
cixzhang merged 1 commit into
facebook:mainfrom
Astro-Han:fix/5253-blank-ua-platform-apple-detection

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #5253.

The defect

isApplePlatform() in packages/core/src/hooks/useHotkeys.ts and detectMac() in packages/core/src/Kbd/Kbd.tsx are independent copies of the same detection: prefer navigator.userAgentData.platform, fall back to navigator.platform. The guard on that preference was

if (uaData && typeof uaData === 'object' && 'platform' in uaData) {
  return /mac/i.test((uaData as {platform: string}).platform ?? '');
}
return /Mac|iPhone|iPad|iPod/.test(navigator.platform ?? '');

'platform' in uaData is true whenever the key exists at all, blank or not, and ?? only defends against null/undefined. So a blank platform takes the client-hints branch, evaluates /mac/i.test(''), returns false, and the fallback that would have answered correctly is never reached. An empty string was read as a negative answer rather than as no answer.

Electron and other embedders that rewrite the app's user-agent or client-hints identity ship exactly that. On macOS every mod combo registered through useHotkeys listened for Ctrl instead of Cmd, and every <Kbd> drew Ctrl. Both surfaces agreed with each other, so nothing looked broken: the shortcut simply never fired, and the hint named the key that also did not work. The same code in Safari or Chrome on the same machine behaved correctly.

The fix

A blank (or non-string) platform is treated as unknown and falls through. A client-hints platform that actually names something is still preferred, so nothing changes for browsers that report one.

const uaPlatform = (uaData as {platform?: unknown}).platform;
if (typeof uaPlatform === 'string' && uaPlatform.trim() !== '') {
  return /mac/i.test(uaPlatform);
}

Both call sites change in one commit: the useHotkeys docstring states the hook "Mirrors the detection used by Kbd so displayed and handled shortcuts agree", so fixing one alone would produce the failure this bug is least likely to be noticed in — a shortcut that fires on a key the hint does not name. The shared docstring line is updated in both, kept identical.

Tests

One regression test per call site, written in the platform-spoofing style each file already uses (vi.stubGlobal in useHotkeys.test.ts, Object.defineProperty in Kbd.test.tsx): with userAgentData.platform: '' and navigator.platform: 'MacIntel', mod+k fires on metaKey and not on ctrlKey, and <Kbd keys="mod" /> renders ⌘. Both fail on main and pass with the fix.

Kbd.test.tsx's afterEach also deletes the spoofed userAgentData so nothing leaks into the tests that follow.

Validation

  • vitest run on both test files — 31 passed; verified the two new Mac tests fail with the source change reverted
  • pnpm -F @astryxdesign/core typecheck, eslint on the touched files, pnpm check:repo — all clean

A [fix] changeset is included (@astryxdesign/core: patch).


Diagnosis, patch, and this description were prepared with Claude Code; every line was read and verified by me before opening, and the validation above was run locally.

@meta-cla

meta-cla Bot commented Aug 22, 2026

Copy link
Copy Markdown

Hi @Astro-Han!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@vercel

vercel Bot commented Aug 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview Aug 22, 2026 7:12am

Request Review

@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Aug 22, 2026
@Astro-Han
Astro-Han force-pushed the fix/5253-blank-ua-platform-apple-detection branch from f2165eb to a2b4e91 Compare August 22, 2026 07:07
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 22, 2026
…Apple

`isApplePlatform()` in `useHotkeys` and `detectMac()` in `Kbd` both prefer
`navigator.userAgentData.platform` and fall back to `navigator.platform`.
The guard on that preference was `'platform' in uaData`, true whenever the
key exists at all, blank or not. A build reporting `platform: ''` committed
to the client-hints branch, evaluated `/mac/i.test('')`, and got `false`
without ever reaching the fallback that would have answered correctly. An
empty string was read as "not Apple" rather than as "no answer".

Electron and other embedders that rewrite the app's user-agent or
client-hints identity ship exactly that. On macOS every `mod` combo
registered through `useHotkeys` listened for Ctrl instead of Cmd, and every
`<Kbd>` drew Ctrl. Both surfaces agreed with each other, so nothing looked
broken: the shortcut never fired, and the hint named the key that also did
not work.

A blank platform now falls through to `navigator.platform`. Both call sites
change in this one commit, since the `useHotkeys` docstring states it
mirrors the detection used by `Kbd` so displayed and handled shortcuts
agree.

Fixes facebook#5253
@Astro-Han
Astro-Han force-pushed the fix/5253-blank-ua-platform-apple-detection branch from a2b4e91 to 571499c Compare August 22, 2026 07:09
@Astro-Han
Astro-Han marked this pull request as ready for review August 22, 2026 07:10

@cixzhang cixzhang 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, good diagnosis — and fixing both copies in one commit is right. Merging as is. One nit, not blocking: detectMac and isApplePlatform want to be one shared helper.

[Reviewed by Robohands]

// A blank platform is no answer, not a negative one. Builds that rewrite
// their client-hints identity ship '', so fall through rather than
// reading it as "not Apple".
if (typeof uaPlatform === 'string' && uaPlatform.trim() !== '') {

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.

Blank is handled — is a value that names nothing, like Unknown, worth the same?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#5394 has both.

Unknown is the spec's own value for "cannot say", so it names a platform no more than '' does, and /mac/i.test('Unknown') is false either way. If anything it's the likelier of the two to show up: '' isn't a legal value, so only a careless embedder emits it, and Unknown is what a careful one emits. Both fall through now.

The helper is utils/isApplePlatform.ts, imported by path from both. Not added to utils/index.ts on purpose, since the root barrel re-exports it and that would make it API. Kept isApplePlatform over detectMac because it matches iPhone/iPad/iPod too.

@cixzhang
cixzhang enabled auto-merge (squash) August 23, 2026 19:17
@github-actions

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

Kbd (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 135 -
Complexity N/A High (18) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.8KB 1.2KB

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Aug 23, 2026
@github-actions github-actions Bot removed the needs:code-review High-risk change (new package/component/API) — needs human code review before merge label Aug 23, 2026
@cixzhang
cixzhang merged commit b7fc8b1 into facebook:main Aug 23, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Empty userAgentData.platform is treated as non-Apple, so mod hotkeys and Kbd show Ctrl on macOS

2 participants