fix(core): one platform detection, and Unknown is no answer either - #5394
Open
Astro-Han wants to merge 1 commit into
Open
fix(core): one platform detection, and Unknown is no answer either#5394Astro-Han wants to merge 1 commit into
Astro-Han wants to merge 1 commit into
Conversation
Follow-up to facebook#5325, which taught `useHotkeys` and `Kbd` to fall through to `navigator.platform` when `userAgentData.platform` is blank. `Unknown` is the User-Agent Client Hints spec's own value for "cannot say", and it names a platform no more than `''` does. It still committed to the client-hints branch, evaluated `/mac/i.test('Unknown')`, and answered "not Apple" without reaching the fallback. It is arguably likelier to appear than a blank: `''` is not a value the spec allows, so only a careless embedder emits it, while `Unknown` is what a careful one emits. Both now fall through. The two detections were independent copies kept aligned by a docstring claiming one mirrored the other. They are now a single internal util that both import, so the next change to this logic cannot land on one surface and miss the other, and the behaviour has one place to be tested. The util is deliberately absent from `utils/index.ts`: the package root does `export * from './utils'`, so naming it there would publish it as API. Imported by path instead, as `interactionModality` is. With one implementation there is one place to test it, so facebook#5325's blank- platform tests at the two call sites fold into the util's own suite. What proves each surface still consults the detection is the pair of tests that predate facebook#5325 and spoof `MacIntel` directly.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsKbd (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #5325, picking up both review notes from @cixzhang. No separate issue: the behaviour was confirmed a bug in review there.
Unknownis no answer eitherplatformin the User-Agent Client Hints spec is a closed set:"Android","Chrome OS","Chromium OS","iOS","Linux","macOS","Windows","Unknown". The last is the spec's value for "cannot say", so it names a platform no more than''does. #5325 stopped''from reading as "not Apple", but/mac/i.test('Unknown')is stillfalse, soUnknownkept answering negatively without ever reachingnavigator.platform.If anything it is the likelier of the two to appear.
''is not a legal value, so only a careless embedder emits it;Unknownis what a careful one emits. Both fall through now, matched case-insensitively after a trim.One detection instead of two
detectMacandisApplePlatformwere byte-identical copies held together by a docstring claiming one mirrored the other, which is why #5253 had to be fixed in two places at once. They are nowutils/isApplePlatform.ts, imported by both, so theUnknownchange lands in one place.It is deliberately not named in
utils/index.ts. The package root doesexport * from './utils', so adding it to that barrel would publish it as API, the trap #5316 pulled back from. It is imported by path instead, asinteractionModalityalready is, andpnpm sync:exports:checkis clean.The kept name is
isApplePlatformrather thandetectMac: the predicate matchesiPhone/iPad/iPodas well asMac, andKbdreads it into a localisMacfor glyph choice.Tests
utils/isApplePlatform.test.ts, 8 cases over the implementation's branches: a client-hints platform that names one (both directions), the five that name none ('', whitespace,Unknown,UNKNOWN, a non-string, each asserted both ways againstnavigator.platform), absent client hints, and nonavigatorat all. Removing the sentinel check turns all five "names none" rows red, including #5253's original blank case.#5325's blank-platform tests at the two call sites are removed rather than kept. With one implementation there is one place to test the logic, and what proves each surface still consults it is the pair of tests predating #5325 that spoof
MacInteldirectly. Keeping them would have been a third copy of a fact already established twice.Validation
347 tests pass across
utils/,Kbd/,useHotkeys.test.tsandserverSafeComponents.test.ts.typecheck,eslint,check:repoandsync:exports:checkare clean. A[fix]changeset is included.Prepared with Claude Code, reviewed and verified by me before opening.