-
Notifications
You must be signed in to change notification settings - Fork 25
Fix hardcoded docs and host URLs #527
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /** | ||
| * Centralized URL configuration for the Coop client. | ||
| * | ||
| * Coop is open source and self-hosted, so any deployment-specific URL must come | ||
| * from the deployment's environment rather than being hardcoded. Add new URL | ||
| * constants here (sourced from `import.meta.env.VITE_*`) instead of inlining | ||
| * literals at call sites. | ||
| */ | ||
|
|
||
| /** | ||
| * Base URL of this Coop instance, used to construct user-facing links such as | ||
| * password reset / invite links, dashboard deep-links, and API code samples. | ||
| * | ||
| * Configure with `VITE_UI_URL` at build time to override the runtime origin | ||
| * (useful when the client is served from a different host than its public | ||
| * URL, e.g. behind a reverse proxy). Falls back to the browser's current | ||
| * origin when unset. | ||
| */ | ||
| 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. The default value lives in `client/.env.example` and is | ||
| * applied via the standard `.env` copy step; this code-level fallback only | ||
| * kicks in if the variable is missing entirely from the build environment. | ||
| */ | ||
| export const DOCS_URL: string = | ||
| import.meta.env.VITE_DOCS_URL ?? 'https://roostorg.github.io/coop/latest'; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,10 +15,8 @@ export default function IframeContentDisplayComponent(props: { | |||||||||||||||||||||
| const { contentUrl } = props; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const contentProxyUrl = | ||||||||||||||||||||||
| import.meta.env.VITE_CONTENT_PROXY_URL ?? | ||||||||||||||||||||||
| (import.meta.env.MODE === 'production' | ||||||||||||||||||||||
| ? 'https://content.getcoop.com' | ||||||||||||||||||||||
| : 'http://localhost:4000'); | ||||||||||||||||||||||
| import.meta.env.VITE_CONTENT_PROXY_URL?.trim() ?? | ||||||||||||||||||||||
| (import.meta.env.DEV ? 'http://localhost:4000' : window.location.origin); | ||||||||||||||||||||||
|
Comment on lines
+18
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 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:
🏁 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 At line 18-19, 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const [isIframeLoading, setIsIframeLoading] = useState(true); | ||||||||||||||||||||||
| const [isTranslating, setIsTranslating] = useState(false); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Handle empty env values when resolving base URLs.
On Line 20 and Line 30,
??won’t fall back ifVITE_UI_URL/VITE_DOCS_URLare set to''(empty string), which can generate broken docs links and relative host links.Suggested fix
🤖 Prompt for AI Agents
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.
our linter does not like
||having??is ok.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.
@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