-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(j-s): Enable opening uploaded files on iOS #17400
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Hook as useFileList
participant MobileDetector as useIsMobile
User->>Hook: Request to open file
Hook->>MobileDetector: Check device type
MobileDetector-->>Hook: Return mobile status
alt Is Mobile Device
Hook->>User: Open file in same tab
else Is Desktop
Hook->>User: Open file in new tab
end
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17400 +/- ##
==========================================
+ Coverage 35.67% 35.69% +0.02%
==========================================
Files 6933 6933
Lines 148894 148836 -58
Branches 42547 42536 -11
==========================================
+ Hits 53113 53127 +14
+ Misses 95781 95709 -72 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts (1)
6-15
: Explore a More Robust Mobile Detection Method
Relying solely onuserAgent
checks can be brittle. Consider usingnavigator.userAgentData
(if supported) or a well-tested library for mobile detection. This reduces the risk of false positives/negatives when new devices emerge.apps/judicial-system/web/src/utils/hooks/useFileList/index.ts (1)
28-30
: Validate the Third Parameter ofwindow.open
While'noopener, noreferrer'
is a valid "window features" string, it’s more common to separate those features with no spaces (e.g.,'noopener,noreferrer'
). Ensure you’re meeting your security and feature requirements with this parameter.-window.open(url, isMobile ? '_self' : '_blank', 'noopener, noreferrer') +window.open(url, isMobile ? '_self' : '_blank', 'noopener,noreferrer')
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/web/src/utils/hooks/useFileList/index.ts
(4 hunks)apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/web/src/utils/hooks/useFileList/index.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts (1)
20-20
: Default Export Looks Good
Exporting the hook by default promotes clarity and simpler imports across the codebase.apps/judicial-system/web/src/utils/hooks/useFileList/index.ts (3)
12-12
: ImportinguseIsMobile
The import ofuseIsMobile
properly centralizes the mobile detection logic, which helps ensure consistency and prevents duplication across your application.
25-25
: Good Use ofuseIsMobile
Using a dedicated hook to detect mobile devices preserves clarity in the code, making it clear why and how the UI experience changes for mobile users.
40-40
: Centralized File Opening Logic
LeveragingopenFile
at these lines ensures the device-aware file opening is consistently applied. This approach simplifies future maintenance or additional checks, such as logging or analytics.Also applies to: 56-56
…d-is/island.is into j-s/fix-open-file-ios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts (1)
8-11
:⚠️ Potential issueFix the SSR check condition.
The current SSR check
if (!window)
will throw a ReferenceError. Usetypeof window === 'undefined'
instead.- if (!window) { + if (typeof window === 'undefined') { return false }
🧹 Nitpick comments (3)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts (3)
1-4
: Add TypeScript return type annotation.Following the coding guidelines for optimal TypeScript usage, add a return type annotation to the hook.
-const useIsMobile = () => { +const useIsMobile = (): boolean => {
13-17
: Consider using a more reliable mobile detection approach.The current user agent-based detection might not be future-proof and can be spoofed. Consider using CSS media queries or the
window.matchMedia()
API for a more reliable approach.- const userAgent = navigator.userAgent - const mobileRegex = - /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i - return mobileRegex.test(userAgent) + return window.matchMedia('(max-width: 768px)').matches
6-20
: Optimize the effect implementation.The current implementation is good as it runs only once on mount. However, consider these improvements:
- Add a cleanup function to handle component unmounting
- Add an event listener for screen resize to handle orientation changes
useEffect(() => { const checkMobile = () => { if (typeof window === 'undefined') return false - const userAgent = navigator.userAgent - const mobileRegex = - /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i - return mobileRegex.test(userAgent) + return window.matchMedia('(max-width: 768px)').matches } + const handleResize = () => { + setIsMobile(checkMobile()) + } + setIsMobile(checkMobile()) + window.addEventListener('resize', handleResize) + + return () => { + window.removeEventListener('resize', handleResize) + } }, [])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/utils/hooks/useIsMobile/useIsMobile.ts (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
Enable opening uploaded files on iOS
Asana
What
There is a bug in production where users cannot open uploaded files, i.e. files not generated by RVG, on iOS devices. They can do so on Android devices however and this is due to security measures taken by Apple when opening files in a new tab.
To fix this, we detect whether or not a user is using a mobile device and if so, we don't try to open the file in a new tab but in the same tab rather.
Note that the reason we open files in a new tab on desktop is that users might want to open multiple files and view them simultainiously and that does not apply on mobile devices.
Why
This is a bug.
Checklist:
Summary by CodeRabbit