fix(frontend): fix compute rivet run url#5121
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
🚅 Deployed to the rivet-pr-5121 environment in rivet-frontend
|
PR Review: fix(frontend): fix compute rivet run urlFiles changed: 4 | Net diff: +12 / -12 OverviewThis PR does two independent things:
Bug (blocking): Inverted null-guard in `feedback-button.tsx`-if (e.key.toLowerCase() !== "f") return;
+if (!e.key && e.key.toLowerCase() !== "f") return;The new condition is logically wrong. When `e.key` is falsy, `!e.key` is `true` but `e.key.toLowerCase()` throws `TypeError: Cannot read properties of undefined` — the very crash this is meant to prevent — because the `&&` does not short-circuit when its left operand is truthy. And when `e.key` is falsy, `!e.key && ...` evaluates to `false` (short-circuits), so `return` is skipped and the handler proceeds to `e.preventDefault(); setOpen(true)`, incorrectly opening the feedback popover for any synthetic event with no key. The correct guard is: if (!e.key || e.key.toLowerCase() !== "f") return;This exits early (does nothing) when `e.key` is falsy, and also exits when the key is not `"f"`. This matches the original intent. Correctness concern: Staging detection strategy changeThe original code used the build-time `DEPLOYMENT_TYPE` env var (values: `"staging"` | `"production"`) to pick the URL subdomain. The new code uses `window.location.hostname` matching `/(^|.)staging.rivet.dev$/`. The behavioral difference: the old approach was an explicit configuration (you set `DEPLOYMENT_TYPE=production` in the build pipeline), while the new approach infers staging from the hostname the browser is currently on. This is a reasonable approach for a URL that is displayed to users (the rivet.run deployment URL), but there is one edge case to consider: the `DEPLOYMENT_TYPE` env var still exists in the schema (as `optional()`) and is still read in other parts of the codebase (e.g., `getting-started.tsx` line 1548-1550 was not updated in this PR — the worktree still contains the old inline code there). Double-check that the `getting-started.tsx` `FrontendSetup` component was also migrated. The diff shows it was changed, but the `getting-started.tsx` import line now reads `import { cloudEnv, engineEnv, getRivetRunUrl } from "@/lib/env"` while the old `cloudEnv` import is still retained — confirm there are no remaining references to the old `DEPLOYMENT_TYPE`-based URL logic in that file. Minor: `isStagingHost` SSR guardexport const isStagingHost = () =>
typeof window !== "undefined" &&
/(^|\.)staging\.rivet\.dev$/.test(window.location.hostname);The `typeof window !== "undefined"` guard is good defensive practice, though the dashboard is a pure SPA and does not run server-side. No change needed, just noting it is correct. Minor: Regex correctnessThe pattern `/(^|.)staging.rivet.dev$/` correctly matches:
And does not match `notstaging.rivet.dev` (the `(^|.))` anchor prevents false positives). This is correct. Summary
The `getRivetRunUrl` extraction is clean and the deduplication is valuable. The `feedback-button.tsx` fix needs the condition operator corrected from `&&` to `||` before this is safe to merge. |
3b20080 to
85966a3
Compare
8e3206f to
454449c
Compare
454449c to
ec63be2
Compare
| typeof window !== "undefined" && | ||
| /(^|\.)staging\.rivet\.dev$/.test(window.location.hostname); | ||
|
|
||
| export const getRivetRunUrl = (engineNsName: string) => |
There was a problem hiding this comment.
I wonder if this should be returned from the cloud api instead,’like good old times with hub. Like /metadata for cloud api.

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: