fix(new-webui): Bind to host specified by host environment variable (fixes #951).#954
Conversation
WalkthroughThe server initialization logic was updated to use the host and port values from the application's configuration object instead of relying on environment variables or a default constant. The unused constant for the default port was removed, and a reference to the configuration object was added after the server is marked as ready. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Config
App->>App: Initialize
App->>App: await ready()
App->>Config: Access HOST and PORT
App->>App: Listen on {host: Config.HOST, port: Config.PORT}
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/server/src/main.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/server/src/main.ts
|
|
||
| try { | ||
| await app.listen({port: Number(process.env.PORT || DEFAULT_PORT)}); | ||
| await app.listen({host: app.config.HOST, port: app.config.PORT}); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that app.config properties exist and add error handling.
The change correctly reads from app.config, but there's no validation that HOST and PORT properties exist. This could cause runtime errors if the configuration is incomplete.
Please verify that app.config.HOST and app.config.PORT are properly defined by running this script to check the configuration setup:
Consider adding defensive programming to handle missing config values:
- await app.listen({host: app.config.HOST, port: app.config.PORT});
+ const host = app.config.HOST || '127.0.0.1';
+ const port = app.config.PORT || 3000;
+ await app.listen({host, port});🏁 Script executed:
#!/bin/bash
# Description: Verify app.config properties are properly set in the Fastify configuration
# Search for config definitions and usage patterns
echo "=== Searching for config property definitions ==="
ast-grep --pattern 'config: {
$$$
}'
echo "=== Searching for HOST and PORT config assignments ==="
rg -A 3 -B 3 'HOST.*='
rg -A 3 -B 3 'PORT.*='
echo "=== Searching for fastify config plugin registration ==="
rg -A 5 'fastify.*config'Length of output: 4846
Please check how the config plugin is registered (and whether it enforces required HOST and PORT properties) by running:
🏁 Script executed:
#!/bin/bash
echo "=== Checking for @fastify/env import in app.ts ==="
rg -n "@fastify/env" components/log-viewer-webui/server/src/fastify-v2/app.ts
echo "=== Listing fastify.register calls in app.ts ==="
rg -n "fastify\.register" -A3 components/log-viewer-webui/server/src/fastify-v2/app.tsLength of output: 1017
Add validation or fallbacks for HOST and PORT.
The config plugin isn’t enforcing these variables or providing defaults, so calling
await app.listen({ host: app.config.HOST, port: app.config.PORT })
may throw at runtime if either is undefined.
• In server/src/main.ts (line 62):
await app.listen({ host: app.config.HOST, port: app.config.PORT });• In server/src/fastify-v2/app.ts: no import or registration of @fastify/env was detected.
Suggested fixes:
-
Register
@fastify/envwith a schema requiringHOSTandPORT(and sensible defaults):await fastify.register(import('@fastify/env'), { schema: { type: 'object', required: ['HOST', 'PORT'], properties: { HOST: { type: 'string', default: '127.0.0.1' }, PORT: { type: 'integer', default: 3000 } } } });
-
Or apply inline fallbacks in
main.ts:- await app.listen({ host: app.config.HOST, port: app.config.PORT }); + const host = app.config.HOST ?? '127.0.0.1'; + const port = app.config.PORT ?? 3000; + await app.listen({ host, port });
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/main.ts at line 62, the code uses
app.config.HOST and app.config.PORT without verifying their existence, which can
cause runtime errors if these config values are missing. To fix this, add
validation to ensure HOST and PORT are defined before calling app.listen, or
provide default fallback values inline. Alternatively, register the @fastify/env
plugin in server/src/fastify-v2/app.ts with a schema that requires HOST and PORT
and sets sensible defaults, ensuring these config properties are always
available.
There was a problem hiding this comment.
@davemarco Understood! I'll drop this concern. 🤐
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
fix(new-webui): Bind to host specified by host environment variable (fixes #951).
…ixes y-scope#951). (y-scope#954) Co-authored-by: Marco <david.marcovitch@yscope.com>
Description
When moving to new fastify setup, we replaced script with one from demo app. Demo app did not load the host name.
Note this fix will only work for production setup. We should change environment variable names to FASTIFY_HOST, etc, so the dev scripts also load these variables. We can maybe also use an alias, but perhaps that may confuse people. Anyways, I will leave changing var name to #939.
Checklist
breaking change.
Validation performed
Started server with 0.0.0.0 hostname and it started
Summary by CodeRabbit