fix: support for IPv6 address literals in HOST env var - #2614
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHost and port resolution is centralized with IPv6 literal support, and server-side page requests use the helper. Server startup now captures the HTTP server and handles listen errors. ChangesRuntime networking
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/urlHelper.ts`:
- Around line 3-5: The current host-wrapping logic in src/utils/urlHelper.ts
blindly brackets any host containing ':' which double-brackets already-bracketed
IPv6 literals; update the guard in the host handling (the block that checks
host.includes(':')) to first detect if host already startsWith('[') and
endsWith(']') and only wrap with `[...]` when those brackets are absent, leaving
already-bracketed values unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/pages/_app.tsxsrc/pages/collection/[collectionId]/index.tsxsrc/pages/movie/[movieId]/index.tsxsrc/pages/tv/[tvId]/index.tsxsrc/utils/urlHelper.ts
There was a problem hiding this comment.
Pull request overview
Adds a shared helper for constructing internal HOST:PORT strings so server-side page requests can correctly target IPv6 address literals (by bracketing them) when building URLs for local API calls.
Changes:
- Introduced
getHostAndPort()utility for consistent internal host/port formatting (including IPv6 bracketing). - Updated server-side Axios calls in
_appand several detail pages to use the new helper instead of inlineprocess.env.HOST/process.env.PORTstring concatenation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/urlHelper.ts | Adds centralized host/port formatting used for internal API URL construction. |
| src/pages/_app.tsx | Switches internal settings/user Axios requests to use the shared host/port helper. |
| src/pages/movie/[movieId]/index.tsx | Uses the shared host/port helper for server-side movie detail API calls. |
| src/pages/tv/[tvId]/index.tsx | Uses the shared host/port helper for server-side TV detail API calls. |
| src/pages/collection/[collectionId]/index.tsx | Uses the shared host/port helper for server-side collection detail API calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Maybe overkill but this lib could help : https://www.npmjs.com/package/ip-address |
Yeah probably overkill. Worst case you don't properly configure the host address and the app crashes at startup? |
Correct. However, I noticed this error wasn't being caught and logged so I updated the PR for this. Example logs when |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/index.ts (1)
281-284: Consider using structured logging for consistency.The error handler works correctly. For consistency with other logging calls in this file (e.g., lines 270, 276), consider using the structured format with a
labelproperty.♻️ Suggested improvement
httpServer.on('error', (err) => { - logger.error('Listen error:', err); + logger.error('Failed to start server', { + label: 'Server', + message: err.message, + }); process.exit(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.ts` around lines 281 - 284, Update the httpServer error handler to use the same structured logging shape as other calls: replace the freeform logger.error('Listen error:', err) in the httpServer.on('error', (err) => { ... }) callback with a single structured call that passes an object including a label (e.g., label: 'server:listen') and the error (e.g., err) so the log matches the format used at lines like the other logger.error/logger.info calls; ensure you only modify the logger.error invocation inside the httpServer.on('error') handler to emit the label and error fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/index.ts`:
- Around line 281-284: Update the httpServer error handler to use the same
structured logging shape as other calls: replace the freeform
logger.error('Listen error:', err) in the httpServer.on('error', (err) => { ...
}) callback with a single structured call that passes an object including a
label (e.g., label: 'server:listen') and the error (e.g., err) so the log
matches the format used at lines like the other logger.error/logger.info calls;
ensure you only modify the logger.error invocation inside the
httpServer.on('error') handler to emit the label and error fields.
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
|
Hey @kouellette can you review the two comments we did so we can merge your PR |
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
When pages would build URLs for API requests, process.env.HOST would be directly concatenated with process.env.PORT (e.g., `0.0.0.0:5055`). This works for IPv4 address literals but does not work for IPv6 address literals because `:::5055` for example is invalid. This commit adds a check if process.env.HOST contains a colon (:) and if so, wraps the host in square brackets. E.g., `[::]:5055`, which is valid. This allows for users to set the HOST environment variable to IPv6 address literals. fix seerr-team#2612
Accepted @fallenbagel suggestion Co-authored-by: fallenbagel <98979876+fallenbagel@users.noreply.github.com>
Apply PR suggestion from @M0NsTeRRR to use the Address6.isValid util from the ip-address package.
bd64a92 to
c9c2b22
Compare
github-actions
Bot
removed
the
merge conflict
|
Thanks for your contribution @kouellette ! |
seerr
 |
||||||||||||||||||||||||||||
| Project |
seerr
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 02m 50s |
| Commit |
|
| Committer | Kyle Ouellette |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
1
|
 |
0
|
 |
0
|
 |
33
|
| View all changes introduced in this branch ↗︎ | |
Description
When pages would build URLs for API requests, process.env.HOST would be directly concatenated with process.env.PORT (e.g.,
0.0.0.0:5055). This works for IPv4 address literals but does not work for IPv6 address literals because:::5055for example is invalid. This commit adds a check if process.env.HOST contains a colon (:) and if so, wraps the host in square brackets. E.g.,[::]:5055, which is valid. This allows for users to set the HOST environment variable to IPv6 address literals.How Has This Been Tested?
This was tested on a Debian 13 VM running kernel 6.12.48+deb13-amd64 for the server and a 13-inch 2022 MacBook Pro with the Arc browser Version 1.135.0 (75383), Chromium Engine Version 145.0.7632.110.
Tested by setting
HOSTenv var to::and another IPv6 literal and ensured pages loaded as expected and the error observed in the ticket was no longer occurring.Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Refactor
Chores
Chores