Fix hardcoded docs and host URLs#527
Conversation
Centralize HOST_URL and DOCS_URL in client/src/lib/config.ts (sourced from VITE_UI_URL and VITE_DOCS_URL) and replace literal getcoop.com / docs.getcoop.com references throughout the client. Also drop the dead Cove SaaS domains from server/utils/url.ts's default blocklist and stop falling back to content.getcoop.com in the iframe content proxy. Fixes #163, fixes #370.
juanmrad
requested review from
cassidyjames,
dom-notion,
julietshen and
vinaysrao1
as code owners
📝 WalkthroughWalkthroughThis PR consolidates hardcoded domain URLs ( ChangesCentralized URL Configuration Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/lib/config.ts`:
- Around line 19-30: HOST_URL and DOCS_URL currently use the nullish coalescing
operator (??) which does not treat empty strings as missing; update the
resolution logic for import.meta.env.VITE_UI_URL and
import.meta.env.VITE_DOCS_URL so empty values also fall back (e.g. treat value
as falsy by using a trimmed-string check or logical OR) — change the expressions
that compute HOST_URL and DOCS_URL to check/import.meta.env.VITE_* variables,
trim and test for non-empty before using them, otherwise use
window.location.origin for HOST_URL and the default docs URL for DOCS_URL.
In
`@client/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx`:
- Around line 17-18: The code currently sets contentProxyUrl =
import.meta.env.VITE_CONTENT_PROXY_URL ?? 'http://localhost:4000', which
hard-falls back to localhost in non-dev builds; change this so the localhost
default is only used in development (e.g. check import.meta.env.MODE ===
'development' or similar) and otherwise leave contentProxyUrl undefined or
fail-fast when VITE_CONTENT_PROXY_URL is missing, and update any consumers of
contentProxyUrl (the iframe/message handling code that reads contentProxyUrl) to
handle the undefined case (show error, disable iframe, or throw a clear
configuration error) so production/staging won't accidentally point at
localhost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23510352-1ed1-4b4c-ac65-e3ded099a8cb
📒 Files selected for processing (13)
.env.githubciclient/.env.exampleclient/src/lib/config.tsclient/src/webpages/dashboard/actions/ActionForm.tsxclient/src/webpages/dashboard/item_types/itemTypeCodeSampleUtils.tsclient/src/webpages/dashboard/mrt/ManualReviewAppealSettings.tsxclient/src/webpages/dashboard/mrt/ManualReviewRecentDecisions.tsxclient/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsxclient/src/webpages/settings/ManageUsers.tsxclient/src/webpages/settings/ManageUsersInviteUserSection.tsxclient/src/webpages/settings/SSOSettings.tsxserver/utils/url.test.tsserver/utils/url.ts
| export const HOST_URL: string = | ||
| import.meta.env.VITE_UI_URL ?? window.location.origin; | ||
|
|
||
| /** | ||
| * Base URL of the published Coop documentation site. | ||
| * | ||
| * Configure with `VITE_DOCS_URL` at build time to point at a fork's or | ||
| * mirror's docs. Defaults to the official mdBook site published from this | ||
| * repository's `docs/` folder. | ||
| */ | ||
| export const DOCS_URL: string = | ||
| import.meta.env.VITE_DOCS_URL ?? 'https://roostorg.github.io/coop/latest'; |
There was a problem hiding this comment.
Handle empty env values when resolving base URLs.
On Line 20 and Line 30, ?? won’t fall back if VITE_UI_URL/VITE_DOCS_URL are set to '' (empty string), which can generate broken docs links and relative host links.
Suggested fix
-export const HOST_URL: string =
- import.meta.env.VITE_UI_URL ?? window.location.origin;
+export const HOST_URL: string =
+ import.meta.env.VITE_UI_URL?.trim() || window.location.origin;
@@
-export const DOCS_URL: string =
- import.meta.env.VITE_DOCS_URL ?? 'https://roostorg.github.io/coop/latest';
+export const DOCS_URL: string =
+ import.meta.env.VITE_DOCS_URL?.trim() ||
+ 'https://roostorg.github.io/coop/latest';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/lib/config.ts` around lines 19 - 30, HOST_URL and DOCS_URL
currently use the nullish coalescing operator (??) which does not treat empty
strings as missing; update the resolution logic for import.meta.env.VITE_UI_URL
and import.meta.env.VITE_DOCS_URL so empty values also fall back (e.g. treat
value as falsy by using a trimmed-string check or logical OR) — change the
expressions that compute HOST_URL and DOCS_URL to check/import.meta.env.VITE_*
variables, trim and test for non-empty before using them, otherwise use
window.location.origin for HOST_URL and the default docs URL for DOCS_URL.
There was a problem hiding this comment.
our linter does not like || having ?? is ok.
There was a problem hiding this comment.
@juanmrad, understood — I'll withdraw the || suggestion. The ?? pattern is fine for this use case; empty-string env vars are an unusual misconfiguration and the current code is clear. Thanks for the context!
(๑˃ᴗ˂)ﻌﻌﻌ♡
✏️ Learnings added
Learnt from: juanmrad
Repo: roostorg/coop PR: 527
File: client/src/lib/config.ts:19-31
Timestamp: 2026-05-20T03:51:03.594Z
Learning: In the `roostorg/coop` repository, the ESLint (or equivalent) linter is configured to disallow the logical OR (`||`) operator in favour of the nullish coalescing operator (`??`). Do not suggest replacing `??` with `||` for fallback expressions in this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@client/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx`:
- Around line 18-19: The environment variable parsing for contentProxyUrl allows
empty or whitespace-only values because it uses
import.meta.env.VITE_CONTENT_PROXY_URL?.trim() ?? fallback; change the logic
that sets contentProxyUrl to treat an empty string (after trim) as missing by
using a truthy check (e.g., const contentProxyUrl =
(import.meta.env.VITE_CONTENT_PROXY_URL || '').trim() || (import.meta.env.DEV ?
'http://localhost:4000' : window.location.origin)); then update any code relying
on contentProxyUrl (the message event handler that checks event.origin and the
postMessage calls that use contentProxyUrl as the target origin) to use the
sanitized contentProxyUrl so origin validation and postMessage target origins
are never an empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b0621e0-3584-4d6f-b71d-cf1465cfb1c5
📒 Files selected for processing (3)
client/.env.exampleclient/src/lib/config.tsclient/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx
| import.meta.env.VITE_CONTENT_PROXY_URL?.trim() ?? | ||
| (import.meta.env.DEV ? 'http://localhost:4000' : window.location.origin); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, does value?.trim() ?? fallbackusefallbackwhenvalue is an empty string or whitespace-only string?
💡 Result:
No, the expression value?.trim?? fallback does not use the fallback when value is an empty string or a whitespace-only string [1][2][3]. The nullish coalescing operator (??) only triggers its right-hand side (the fallback) when the left-hand side is null or undefined [1][4][5]. Because an empty string ("") and a whitespace-only string (" ") are valid string values—not null or undefined—the expression will return the result of the trim operation instead of the fallback [1][2][3]. Specifically: - If value is "", value?.trim evaluates to "", and the expression returns "" [1][5]. - If value is " ", value?.trim evaluates to "", and the expression returns "" [6]. - If value is null or undefined, value?.trim evaluates to undefined, and the expression returns the fallback [1][7]. If your goal is to treat empty or whitespace-only strings as "missing" and use the fallback in those cases, you must handle that logic explicitly, for example: const result = (value?.trim || null)?? fallback; // Or more simply: const result = (!value?.trim)? fallback: value.trim; [8][3]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
- 2: https://thelinuxcode.com/nullish-coalescing-operator-in-typescript-practical-patterns-pitfalls-and-2026-best-practices/
- 3: https://thelinuxcode.com/how-to-check-empty-undefined-null-strings-in-javascript-with-practical-patterns-and-edge-cases/
- 4: https://dev.to/laurieontech/nullish-coalescing-let-falsy-fool-you-no-more-41c0
- 5: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/operators/nullish_coalescing/index.md
- 6: https://medium.com/nerd-for-tech/basics-of-javascript-string-trim-method-d53972004e23
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining
- 8: https://stackoverflow.com/questions/10232366/how-to-check-if-a-variable-is-null-or-empty-string-or-all-whitespace-in-javascri
🏁 Script executed:
# Check if file exists and read the relevant section
if [ -f "client/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx" ]; then
echo "File exists. Reading lines 10-25:"
sed -n '10,25p' "client/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx"
else
echo "File not found at specified path"
fiRepository: roostorg/coop
Length of output: 675
🏁 Script executed:
# Search for other uses of VITE_CONTENT_PROXY_URL in codebase
rg "VITE_CONTENT_PROXY_URL" --type ts --type tsxRepository: roostorg/coop
Length of output: 85
🏁 Script executed:
# Search for contentProxyUrl usage to see how it's used (especially postMessage)
rg "contentProxyUrl" --type ts --type tsx -A 3Repository: roostorg/coop
Length of output: 85
🏁 Script executed:
# Search for VITE_CONTENT_PROXY_URL without type flag
rg "VITE_CONTENT_PROXY_URL"Repository: roostorg/coop
Length of output: 197
🏁 Script executed:
# Search for contentProxyUrl usage (especially postMessage and origin checks)
rg "contentProxyUrl" -A 3 -B 1Repository: roostorg/coop
Length of output: 4112
🏁 Script executed:
# Read the entire file to see all usages and context
cat -n "client/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx" | head -100Repository: roostorg/coop
Length of output: 3757
Handle empty VITE_CONTENT_PROXY_URL before fallback.
At line 18-19, ?.trim() ?? ... won't fall back when the env var is '' or whitespace-only. The nullish coalescing operator (??) only triggers on null or undefined, not empty strings, so contentProxyUrl can become an empty string. This breaks origin validation in the message handler (line 53) where event.origin !== "" always rejects incoming messages, and invalidates the postMessage target origin at lines 77 and 93.
Suggested fix
- const contentProxyUrl =
- import.meta.env.VITE_CONTENT_PROXY_URL?.trim() ??
- (import.meta.env.DEV ? 'http://localhost:4000' : window.location.origin);
+ const configuredContentProxyUrl =
+ import.meta.env.VITE_CONTENT_PROXY_URL?.trim();
+ const contentProxyUrl =
+ configuredContentProxyUrl?.length
+ ? configuredContentProxyUrl
+ : import.meta.env.DEV
+ ? 'http://localhost:4000'
+ : window.location.origin;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import.meta.env.VITE_CONTENT_PROXY_URL?.trim() ?? | |
| (import.meta.env.DEV ? 'http://localhost:4000' : window.location.origin); | |
| const configuredContentProxyUrl = | |
| import.meta.env.VITE_CONTENT_PROXY_URL?.trim(); | |
| const contentProxyUrl = | |
| configuredContentProxyUrl?.length | |
| ? configuredContentProxyUrl | |
| : import.meta.env.DEV | |
| ? 'http://localhost:4000' | |
| : window.location.origin; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@client/src/webpages/dashboard/mrt/manual_review_job/IframeContentDisplayComponent.tsx`
around lines 18 - 19, The environment variable parsing for contentProxyUrl
allows empty or whitespace-only values because it uses
import.meta.env.VITE_CONTENT_PROXY_URL?.trim() ?? fallback; change the logic
that sets contentProxyUrl to treat an empty string (after trim) as missing by
using a truthy check (e.g., const contentProxyUrl =
(import.meta.env.VITE_CONTENT_PROXY_URL || '').trim() || (import.meta.env.DEV ?
'http://localhost:4000' : window.location.origin)); then update any code relying
on contentProxyUrl (the message event handler that checks event.origin and the
postMessage calls that use contentProxyUrl as the target origin) to use the
sanitized contentProxyUrl so origin validation and postMessage target origins
are never an empty string.
cassidyjames
added
the
SaaS remnant
3 participants
Fixes #163, fixes #370.
Context & Requests for Reviewers
Centralize
HOST_URLandDOCS_URLinclient/src/lib/config.ts(sourced fromVITE_UI_URLandVITE_DOCS_URL) and replace literalgetcoop.com/docs.getcoop.comreferences throughout the client. Also drop the dead Cove SaaS domains fromserver/utils/url.tsdefault blocklist and stop falling back tocontent.getcoop.comin the iframe content proxy.Summary by CodeRabbit
New Features
Improvements
Tests