🐛 Hid "open in your email app" button on iOS, where it's unreliable#26449
🐛 Hid "open in your email app" button on iOS, where it's unreliable#26449
Conversation
WalkthroughAdds an 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9406da1 to
2b5c044
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/portal/test/utils/is-ios.test.js (1)
3-19: No test exercises the/iPad/or/iPod/regex branches directly.The regex in
isIoscoversiPad|iPhone|iPod, but the test suite only exercises theiPhonepath (viaIPHONE_UA) and theMacIntel + maxTouchPointspath. Adding a case with a pre-iPadOS 13 iPad UA (whereiPadappears in the UA string) would give complete coverage of all three regex branches.✨ Proposed additional test cases
const IPHONE_UA = 'Mozilla/5.0 (iPhone; CPU iPhone OS 18_6 like Mac OS X) ...'; +const IPAD_UA = 'Mozilla/5.0 (iPad; CPU OS 15_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.0 Mobile/15E148 Safari/604.1'; +const IPOD_UA = 'Mozilla/5.0 (iPod touch; CPU iPhone OS 15_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.0 Mobile/15E148 Safari/604.1'; it('returns true for iOS', () => { + expect(isIos({userAgent: IPAD_UA, platform: 'iPad', maxTouchPoints: 5})).toBe(true); + expect(isIos({userAgent: IPOD_UA, platform: 'iPod', maxTouchPoints: 0})).toBe(true); expect(isIos({ userAgent: IPHONE_UA,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/test/utils/is-ios.test.js` around lines 3 - 19, Add unit tests to exercise the remaining regex branches in isIos by adding cases where the userAgent contains "iPad" and "iPod" (pre-iPadOS 13 style UAs) so the /iPad|iPhone|iPod/ branch is covered directly; for each case call isIos with an appropriate userAgent string (e.g., a pre-iPadOS UA including "iPad" and a UA including "iPod") and set platform/maxTouchPoints to values that don't trigger the MacIntel+touch fallback, then assert true (or false as appropriate) to ensure both iPad and iPod paths are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/portal/test/utils/is-ios.test.js`:
- Around line 3-19: Add unit tests to exercise the remaining regex branches in
isIos by adding cases where the userAgent contains "iPad" and "iPod" (pre-iPadOS
13 style UAs) so the /iPad|iPhone|iPod/ branch is covered directly; for each
case call isIos with an appropriate userAgent string (e.g., a pre-iPadOS UA
including "iPad" and a UA including "iPod") and set platform/maxTouchPoints to
values that don't trigger the MacIntel+touch fallback, then assert true (or
false as appropriate) to ensure both iPad and iPod paths are exercised.
closes https://linear.app/ghost/issue/NY-1050 On iOS, we have four options for Inbox Links: 1. Don't show them at all. 2. Open the email inbox in the browser (a regular link). 3. Try to open the associated email app. If it's installed, great. If it's not, nothing will happen. (Kind of a bad experience.) 4. Try to open the associated email app. If it's installed, great. If the user taps it and nothing happens, wait 1 second and then do something else. (Not a great option either for several reasons.) Previously, we did option 2. Now we do option 1.
2b5c044 to
d0c297e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/portal/src/utils/is-ios.js (1)
10-10: Consider forward-compatible iPad detection using optionaluserAgentDatafallback.
navigator.platformis unreliable and deprecated per MDN, but the currentMacIntel + maxTouchPointsheuristic is pragmatically correct for iPadOS 13+ detection since Safari remains the primary target and doesn't support the modernnavigator.userAgentData.platformAPI (Chromium-only as of 2025).To future-proof when
userAgentDatagains wider browser support, you can optionally add it as a fast-path:♻️ Optional forward-compatible fallback
export const isIos = navigator => ( /iPad|iPhone|iPod/.test(navigator.userAgent) || // Checks for modern iPads (iPadOS) which mimic macOS - (navigator.platform === 'MacIntel' && navigator.maxTouchPoints > 1) + // navigator.userAgentData is Chromium-only; navigator.platform is the + // fallback for Safari (deprecated but no universal replacement yet). + ( + (navigator.userAgentData?.platform === 'macOS' || navigator.platform === 'MacIntel') && + navigator.maxTouchPoints > 1 + ) );Note: if adopting
userAgentData, the JSDoc type should also be updated to includeuserAgentDatain thePick.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/utils/is-ios.js` at line 10, Update the iPad detection to prefer a modern userAgentData fast-path: when available use navigator.userAgentData.platform (or navigator.userAgentData?.platform) to detect "iPad", and fall back to the existing (navigator.platform === 'MacIntel' && navigator.maxTouchPoints > 1) heuristic; also update the JSDoc Pick type for the exported is-ios utility to include userAgentData so the linter/types know the property exists (adjust the Pick to include "userAgentData" alongside "platform" and "maxTouchPoints").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/portal/src/utils/is-ios.js`:
- Line 10: Update the iPad detection to prefer a modern userAgentData fast-path:
when available use navigator.userAgentData.platform (or
navigator.userAgentData?.platform) to detect "iPad", and fall back to the
existing (navigator.platform === 'MacIntel' && navigator.maxTouchPoints > 1)
heuristic; also update the JSDoc Pick type for the exported is-ios utility to
include userAgentData so the linter/types know the property exists (adjust the
Pick to include "userAgentData" alongside "platform" and "maxTouchPoints").
ref #26449 This PR was intended to bump Portal, however we have an issue with the CI job/checks that didn't seem to compare the branch to main.
ref #26449 This PR was intended to bump Portal, however the CI job that ran to validate the Portal bump ran before the bump happened, leading to a false pass.
…ryGhost#26449) closes https://linear.app/ghost/issue/NY-1050 On iOS, we have four options for Inbox Links: 1. Don't show them at all. 2. Open the email inbox in the browser (a regular link). 3. Try to open the associated email app. If it's installed, great. If it's not, nothing will happen. (Kind of a bad experience.) 4. Try to open the associated email app. If it's installed, great. If the user taps it and nothing happens, wait 1 second and then do something else. (Not a great option either for several reasons.) Previously, we did option 2. Now we do option 1.
ref TryGhost#26449 This PR was intended to bump Portal, however the CI job that ran to validate the Portal bump ran before the bump happened, leading to a false pass.
closes https://linear.app/ghost/issue/NY-1050
On iOS, we have four options for Inbox Links:
Previously, we did option 2. Now we do option 1.