-
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(my-pages): health - sjukratryggingar - double click on certification button #16718
Conversation
WalkthroughThe changes in the Changes
Possibly related PRs
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
|
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
🧹 Outside diff range and nitpick comments (1)
libs/service-portal/health/src/screens/HealthOverview/HealthOverview.tsx (1)
Line range hint
58-63
: Consider extracting the file download logic.The
getInsuranceConfirmation
function currently handles both data fetching and file downloading. Consider extracting the download logic into a separate utility function for better reusability and separation of concerns.+ const downloadInsuranceConfirmation = (data: { data: string; fileName: string }) => { + downloadLink(data.data, 'application/pdf', data.fileName) + } const getInsuranceConfirmation = async () => { const { data: fetchedData } = await getInsuranceConfirmationLazyQuery() const downloadData = fetchedData?.rightsPortalInsuranceConfirmation if (downloadData?.data && downloadData.fileName) { - downloadLink(downloadData.data, 'application/pdf', downloadData.fileName) + downloadInsuranceConfirmation(downloadData) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/service-portal/health/src/screens/HealthOverview/HealthOverview.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/service-portal/health/src/screens/HealthOverview/HealthOverview.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/service-portal/health/src/screens/HealthOverview/HealthOverview.tsx (2)
54-54
: LGTM! Simplified query result destructuring.
The simplified destructuring improves code clarity by only extracting the necessary loading and error states.
58-59
: Excellent fix for the double-click issue!
The implementation now properly handles the asynchronous flow by:
- Awaiting the query execution
- Immediately accessing the fetched data
- Downloading the file only when data is available
This resolves the original issue where users had to double-click the button.
Let's verify there are no other instances of this pattern that might need similar fixes:
✅ Verification successful
Proper async/await pattern is correctly implemented
The codebase search confirms that the implementation in HealthOverview.tsx
follows the established pattern for handling lazy queries. The fix properly awaits the query execution and immediately accesses the fetched data, which resolves the double-click issue. No other instances of similar race conditions were found in the codebase that would require the same fix.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other files that might need the same fix
ast-grep --pattern 'const [$_] = await [$_]LazyQuery()'
Length of output: 57
Script:
#!/bin/bash
# Let's try a broader search pattern to find similar lazy query usages
rg -l "LazyQuery" --type ts --type tsx
# Then let's look for await patterns near LazyQuery
rg "await.*LazyQuery" -A 2 --type ts --type tsx
Length of output: 144
Script:
#!/bin/bash
# Search for LazyQuery usage without file type restrictions
rg "LazyQuery" -A 2
# Also search for similar async/await patterns that might need attention
rg "const.*=.*await.*Query" -A 2
Length of output: 130776
…ion button (#16718) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
My pages - Health
What
Users had to double click on button to download the fil
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes