fix: use github.io base path for PR preview deployment URLs#18
Merged
Conversation
Replace CNAME-reading logic with configure-pages base_path so PR preview "View Deployment" links correctly point to /pr-N/ for both the main org (which GitHub auto-redirects to the custom domain) and forked repos.
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates GitHub Actions workflows to always construct PR preview deployment URLs using the GitHub Pages github.io base (and the configured BASE_URL_PATH/prefix) instead of inspecting a CNAME file, ensuring correct /pr-N/ links for both org and forked repositories. Flow diagram for updated base URL and host computation in workflowsflowchart TD
subgraph PRPreviewWorkflow["pr-preview.yml Determine Base URL Prefix"]
A["Start"] --> B["Read github.event.repository.name as repoName"]
B --> C["Read github.repository_owner as owner"]
C --> D{repoName == owner.github.io?}
D -->|Yes| E["Set prefix = /"]
D -->|No| F["Set prefix = /repoName/"]
E --> G["Set host = owner.github.io"]
F --> G
G --> H["Write prefix and host to GITHUB_OUTPUT"]
H --> I["End"]
end
subgraph DeployWorkflow["deploy.yml Construct Deployment URL"]
J["Start"] --> K["Read owner and repo from context.repo"]
K --> L["Read BASE_URL_PATH from env as baseUrlPath"]
L --> M["Set base = https://owner.github.io"]
M --> N{baseUrlPath starts with /?}
N -->|Yes| O["normalizedPath = baseUrlPath"]
N -->|No| P["normalizedPath = / + baseUrlPath"]
O --> Q["base = base + normalizedPath"]
P --> Q
Q --> R["Remove trailing / from base if present"]
R --> S["Read PR number as prNumber"]
S --> T["url = base + /pr-prNumber/"]
T --> U["Log deployment URL"]
U --> V["End"]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In deploy.yml,
baseUrlPathis read directly fromprocess.env.BASE_URL_PATHand thenstartsWithis called on it; consider defaulting to an empty string or validating it first to avoid a runtime error when the env var is unset.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In deploy.yml, `baseUrlPath` is read directly from `process.env.BASE_URL_PATH` and then `startsWith` is called on it; consider defaulting to an empty string or validating it first to avoid a runtime error when the env var is unset.
## Individual Comments
### Comment 1
<location path=".github/workflows/deploy.yml" line_range="190-194" />
<code_context>
+ const baseUrlPath = process.env.BASE_URL_PATH;
</code_context>
<issue_to_address>
**issue (bug_risk):** BASE_URL_PATH being undefined will throw at `startsWith`, consider a safe default or guard.
Since `process.env.BASE_URL_PATH` can be `undefined`, `baseUrlPath.startsWith('/')` will throw before the URL is built. Please add a safe default or guard, e.g. `const baseUrlPath = process.env.BASE_URL_PATH || '';`, and adjust the normalization so deployments without this env var still work.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+190
to
+194
| const baseUrlPath = process.env.BASE_URL_PATH; | ||
|
|
||
| let base = `https://${owner}.github.io`; | ||
|
|
||
| const cnamePath = 'CNAME'; | ||
| if (fs.existsSync(cnamePath) && owner === 'mume') { | ||
| const domain = fs.readFileSync(cnamePath, 'utf8').trim(); | ||
| if (domain) { | ||
| base = `https://${domain}`; | ||
| } | ||
| } else { | ||
| const repo = context.repo.repo; | ||
| if (repo !== `${owner}.github.io`) { | ||
| base = `${base}/${repo}`; | ||
| } | ||
| } | ||
| const normalizedPath = baseUrlPath.startsWith('/') ? baseUrlPath : `/${baseUrlPath}`; | ||
| base = `${base}${normalizedPath}`.replace(/\/$/, ''); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): BASE_URL_PATH being undefined will throw at startsWith, consider a safe default or guard.
Since process.env.BASE_URL_PATH can be undefined, baseUrlPath.startsWith('/') will throw before the URL is built. Please add a safe default or guard, e.g. const baseUrlPath = process.env.BASE_URL_PATH || '';, and adjust the normalization so deployments without this env var still work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace CNAME-reading logic with configure-pages base_path so PR preview "View Deployment" links correctly point to /pr-N/ for both the main org (which GitHub auto-redirects to the custom domain) and forked repos.
Summary by Sourcery
Update PR preview workflows to construct deployment URLs using the GitHub Pages base path instead of CNAME detection.
Bug Fixes:
CI: