-
Notifications
You must be signed in to change notification settings - Fork 5
Dynamically resolve allowedHosts for local dev envs #1557
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
Dynamically resolve allowedHosts for local dev envs #1557
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe Vite configuration in apps/cyberstorm-remix/vite.config.ts was converted from a static object to a function-based export via defineConfig(({ mode }) => { ... }). This enables mode-aware server.allowedHosts: development uses [".thunderstore.temp"], while other modes use [".thunderstore.dev", ".thunderstore.io"]. Existing plugins (reactRouter, tsconfigPaths, withSentry), build, HMR, watch, and assetsDir settings were relocated inside the returned object without altering their semantics. The exported signature now returns the config based on the provided mode. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cyberstorm-remix/vite.config.ts
(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Visual diff
apps/cyberstorm-remix/vite.config.ts
[error] 13-13: TypeError: Cannot read properties of undefined (reading 'DEV') while loading Vite config during 'yarn workspace @thunderstore/cyberstorm-remix build'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL
798c676
to
b66981a
Compare
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
🧹 Nitpick comments (2)
apps/cyberstorm-remix/.env (1)
7-7
: Document the expected value format for VITE_DEVELOPMENT.The empty value here combined with the check
Object.hasOwn(env, "VITE_DEVELOPMENT") && env.VITE_DEVELOPMENT
in vite.config.ts means developers must set a truthy value (e.g.,VITE_DEVELOPMENT=true
orVITE_DEVELOPMENT=1
) to enable local dev mode. An empty string will evaluate as falsy.Consider adding an inline comment to clarify the expected value:
-VITE_DEVELOPMENT= +VITE_DEVELOPMENT= # Set to 'true' or '1' for local developmentapps/cyberstorm-remix/vite.config.ts (1)
16-19
: Consider explicit boolean value checking for clarity.The current check relies on JavaScript truthiness, where any non-empty string will enable development mode. For more predictable behavior, consider explicitly checking for specific values:
allowedHosts: - Object.hasOwn(env, "VITE_DEVELOPMENT") && env.VITE_DEVELOPMENT + env.VITE_DEVELOPMENT === "true" || env.VITE_DEVELOPMENT === "1" ? [".thunderstore.temp"] : [".thunderstore.dev", ".thunderstore.io"],This makes the expected values explicit and prevents unintended activation from typos like
VITE_DEVELOPMENT=false
(which would be truthy as a non-empty string).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cyberstorm-remix/.env
(1 hunks)apps/cyberstorm-remix/vite.config.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test
- GitHub Check: Generate visual diffs
- GitHub Check: CodeQL
🔇 Additional comments (1)
apps/cyberstorm-remix/vite.config.ts (1)
5-7
: Environment loading is correct. There is only a single.env
file inapps/cyberstorm-remix
, soloadEnv("", process.cwd(), "VITE_")
already covers it. If you introduce mode-specific files later (e.g..env.development
), switch to thedefineConfig(({ mode }) => { const env = loadEnv(mode, …); … })
form to load them.
b66981a
to
3bbacc2
Compare
3bbacc2
to
2b91be9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1557 +/- ##
=======================================
Coverage 10.69% 10.69%
=======================================
Files 285 285
Lines 19876 19876
Branches 385 385
=======================================
Hits 2126 2126
Misses 17750 17750 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2b91be9
to
f3a2fdd
Compare
9e4bbd9
to
4458b9f
Compare
We want to enforce the usage of thunderstore.temp local dev domain, to ensure consistency in everyday debugging and development.
f3a2fdd
to
15c1c7c
Compare
4458b9f
to
a8426a5
Compare
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.
Nice, clean solution 👍🏻
Summary by CodeRabbit