Fix resume download popup #99
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @sunithvs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Gemini here, providing a summary of this pull request by @sunithvs. The primary goal of this PR is to address an issue where browser popup blockers might prevent the resume download from opening. The proposed solution involves a common technique to circumvent this by opening a blank window immediately upon the user initiating the download, and then redirecting that pre-opened window to the downloaded file's URL once the data is ready. This approach leverages the fact that window.open calls directly triggered by user interaction are less likely to be blocked.
Highlights
- Popup Blocker Mitigation: Implements a strategy to bypass browser popup blockers for the resume download functionality by opening a blank window first.
- Download Logic Refinement: Modifies the download handling in the
ClientResumeButtoncomponent to use the pre-opened window for displaying the downloaded resume, including a fallback mechanism and error handling for the new window.
Changelog
- www/components/ClientResumeButton.tsx
- Added a call to
window.open("", "_blank")at the beginning of thehandleDownloadfunction to open a blank window immediately. - Updated the logic after fetching the resume data to check if the pre-opened window (
newWindow) exists. If it does, the downloaded URL is assigned tonewWindow.location.href; otherwise, it falls back to the originalwindow.open(url, "_blank"). - Added a check in the
catchblock to close thenewWindowif an error occurs during the download process.
- Added a call to
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Click to download,
A blank tab opens, then the file,
Blockers bypassed.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where popup blockers might prevent resume downloads. The approach taken, which involves opening a blank window first and then navigating it to the download URL, is a common and effective technique for such scenarios. The changes are well-targeted to address this specific problem.
I have a couple of suggestions related to the usage of window.open to enhance security by incorporating noopener and noreferrer attributes. These are generally considered best practices when opening new tabs/windows.
Regarding the testing note in the PR description (pytest -q fails: command not found): since this appears to be a TypeScript/Next.js project, pytest (a Python testing tool) would not typically be the testing framework. This might be a leftover note or a misunderstanding. For frontend projects like this, tools like Jest, Vitest, or Playwright with React Testing Library are more common. It might be helpful to clarify the testing strategy for this project if this note is relevant.
Summary of Findings
- Security Enhancement for
window.open: Thewindow.opencalls should include"noopener,noreferrer"attributes to prevent potential tabnabbing and to avoid sending referrer information. This is a security best practice. - Testing Note in PR Description: The PR description mentions
pytestfailing.pytestis a Python tool and likely not relevant for testing this TypeScript/Next.js project. This might need clarification or removal from the PR description.
Merge Readiness
The core logic of the fix for popup blockers is sound. However, there are a couple of medium-severity suggestions regarding security best practices for window.open. I recommend addressing these suggestions to improve the robustness and security of the code. Therefore, I am requesting changes before this PR is merged. As a reviewer, I am not authorized to approve the pull request; please ensure it undergoes further review and approval as per your team's process after addressing the feedback.
| const [isLoading, setIsLoading] = useState(false); | ||
|
|
||
| const handleDownload = async () => { | ||
| const newWindow = window.open("", "_blank"); |
There was a problem hiding this comment.
Good use of opening a blank window first to work around popup blockers!
For enhanced security and to follow best practices, when using window.open("", "_blank"), it's recommended to include "noopener,noreferrer" as the third argument (window features).
noopener: Prevents the new window from having access towindow.opener, which mitigates potential tabnabbing attacks.noreferrer: Prevents the browser from sending theRefererHTTP header to the new page.
While the immediate navigation is to a blob: URL, adopting this practice consistently is beneficial. What are your thoughts on applying this here?
const newWindow = window.open("", "_blank", "noopener,noreferrer");| if (newWindow) { | ||
| newWindow.location.href = url; | ||
| } else { | ||
| window.open(url, "_blank"); |
There was a problem hiding this comment.
Similar to the suggestion for the initial window.open call, it would be good to apply the "noopener,noreferrer" attributes to this fallback call as well. This ensures consistent security best practices even if the initial newWindow couldn't be opened and this fallback path is taken.
window.open(url, "_blank", "noopener,noreferrer");
Summary
Testing
pytest -q(fails: command not found)