-
Notifications
You must be signed in to change notification settings - Fork 30.4k
fix(router): prevent duplicate prefix and double slashes in addPathPrefix #89212
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(router): prevent duplicate prefix and double slashes in addPathPrefix #89212
Conversation
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
1 similar comment
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
| return `${prefix}${pathname}${query}${hash}` | ||
|
|
||
| // Avoid duplicate prefix and normalize slashes | ||
| const normalizedPrefix = prefix.replace(/\/+$/, '') |
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.
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.
I agree with the bot here. Your code is right if both path and prefix have slashes, but if path does not have a leading slash, then you can get duplicate paths.
This looks like an internal function. Would it be simpler to fix the callsites to avoid passing duplicate slashes? (IDK)
At the least, this PR should have a unit test for this function.
| return `${prefix}${pathname}${query}${hash}` | ||
|
|
||
| // Avoid duplicate prefix and normalize slashes | ||
| const normalizedPrefix = prefix.replace(/\/+$/, '') |
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.
I agree with the bot here. Your code is right if both path and prefix have slashes, but if path does not have a leading slash, then you can get duplicate paths.
This looks like an internal function. Would it be simpler to fix the callsites to avoid passing duplicate slashes? (IDK)
At the least, this PR should have a unit test for this function.
|
You're absolutely right - the prefix detection has a bug. I agree with your suggestion that it might be simpler to fix the callsites to avoid passing duplicate slashes in the first place, especially since:
Closing this PR. If there is a specific user-facing bug caused by this, I'd be happy to revisit with proper tests and boundary checks. Thanks for the thorough review! |
|
Closing - prefix detection has edge case bugs and fixing callsites may be simpler. See comment above. |
@jaffarkeikei I appreciate the contribution you made with #89127, but I highly recommend that you do not use LLMs to write responses to GitHub issues or pull requests to projects where you do not already have an established reputation. It's also good practice to be clear and transparent about what portions of the work you used LLMs for, to help the reviewer understand where to focus their attention. I don't mean this to say that you shouldn't use LLMs. You should use them. They're an incredible tool, but you need to be cautious in how you use them in the context of third-party open source contributions. You may have seen that many large open source projects have stopped accepting community pull requests because of an increasing level of spam. Heavily relying on an LLM as a community contributor can come off as "low effort". I know this isn't the case here, based on your other contributions, but it can easily come off wrong. Reviewing community PRs often requires a lot more effort on an open source maintainer's time than most other work, because until a contributor is trusted, it requires significantly more scrutiny. That's either because of a higher risk of honest mistakes from somebody unfamiliar with the code, or concerns about supply chain attacks. A lot of times, we review PRs not because of the bugs they may fix, but because we want to help others grow, and an LLM generated response hurts that trust. I understand that you're a student, and if you're looking to learn, contributing to open source can be valuable. I benefited and learned a lot from contributing to similar projects myself. I love reviewing changes from new contributors. However, there's a balance here. If you're making contributions to game some metrics on GitHub, that's not a good use of anyone's time. Open source is often most rewarding when you've personally encountered real issues using a project, and you're building the solution out of that experience. I hope this doesn't turn you off to contributing to Next.js. I'd love to see more of your PRs here. Just link your PRs to clear issues or reproductions, and be clear and transparent about what you do or don't understand, and how you used LLMs to help fill that knowledge gap. |
|
Thanks for the feedback, will be clearer on my LLM usage. |
What changed?
addPathPrefixnow:Why?
Current implementation has two bugs:
Bug 1: Duplicate prefix
Bug 2: Double slashes
After fix:
Testing
Manual testing with edge cases.