Fail fast on a bad CHROME_PATH, and drop a dead preconnect - #4
Merged
Conversation
CHROME_PATH was trusted without checking it. Pointed at a binary that is not there, the script skipped detection and failed inside the render loop instead, as sixteen identical ENOENT errors naming the cards rather than the cause. The preview sheet preconnected to fonts.googleapis.com and then never requested a stylesheet from it. The cards use Inter from the system, so the connection was opened for nothing. Both found by review on #3. Dev tooling only: no shipped output changes, and the committed PNGs are byte-identical after re-rendering.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe OG preview template removes the Google Fonts preconnect hint. Chrome rendering now validates discovered and environment-provided executable paths, classifies invalid overrides, and exits before rendering when necessary. ChangesOG rendering updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR hardens the OG rendering dev tooling by validating CHROME_PATH up front and simplifying the OG preview HTML by removing an unused font preconnect, with no changes to shipped output. Sequence diagram for CHROME_PATH resolution and fail-fast behavior in og-render.mjssequenceDiagram
participant og_render_mjs
participant env
participant filesystem
participant console
participant process
og_render_mjs->>env: resolveChrome (read CHROME_PATH)
alt [CHROME_PATH is unset]
og_render_mjs->>og_render_mjs: findChrome
og_render_mjs-->>og_render_mjs: chrome (auto-detected)
else [CHROME_PATH is set]
og_render_mjs->>filesystem: existsSync(override)
alt [path exists]
og_render_mjs-->>og_render_mjs: chrome = override
else [path does not exist]
og_render_mjs->>console: console.error
og_render_mjs->>process: process.exit(1)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Summary by QodoFail fast on invalid CHROME_PATH; remove unused fonts preconnect
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Using
process.exit(1)insideresolveChromemakes the helper harder to reuse and test; consider throwing an error and letting the top-level script handle process termination instead. - If
CHROME_PATHis expected to be absolute, it might be worth normalizing or validating that assumption beforeexistsSync, or clarifying in the error message that relative paths are resolved from the current working directory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `process.exit(1)` inside `resolveChrome` makes the helper harder to reuse and test; consider throwing an error and letting the top-level script handle process termination instead.
- If `CHROME_PATH` is expected to be absolute, it might be worth normalizing or validating that assumption before `existsSync`, or clarifying in the error message that relative paths are resolved from the current working directory.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR improves developer-only OG card tooling by failing fast when CHROME_PATH is misconfigured and by removing an unused resource hint from the OG preview HTML.
Changes:
- Add
resolveChrome()to validateCHROME_PATHearly and exit with a clearer error when invalid. - Remove an unused
<link rel="preconnect">tofonts.googleapis.comfrom the OG preview contact sheet.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/og-render.mjs | Adds early CHROME_PATH resolution to surface misconfiguration before the render loop. |
| scripts/og-preview.mjs | Removes an unused preconnect to a font host that isn’t actually referenced. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+59
to
+63
| if (!existsSync(override)) { | ||
| console.error(`og-render: CHROME_PATH is set to "${override}", which does not exist.`); | ||
| process.exit(1); | ||
| } | ||
| return override; |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
existsSync accepted a directory, and on Windows any readable file, so a wrong CHROME_PATH still reached the render loop and failed sixteen times over. Check that it is a file, and that it looks executable: the x bit on unix, an .exe extension on Windows, where X_OK degrades to a read check that package.json would pass. Not a --version probe, which was the obvious alternative: on Windows Chrome prints no version for it and hands the arguments to the running browser instead, which opens a tab. Raised by cubic, Copilot and qodo on #4.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/og-render.mjs`:
- Around line 46-58: The path inspection in isRunnable and the CHROME_PATH
diagnostics must be exception-safe against files disappearing or becoming
inaccessible between filesystem checks. Guard the combined existence, stat, and
relevant permission inspection in a single try/catch, return false from
isRunnable on any inspection failure, and reuse the guarded result in the
CHROME_PATH diagnostic flow so it still prints the intended message without
propagating filesystem exceptions.
🪄 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: f26aa495-5653-4894-a779-c9ef63a27280
📒 Files selected for processing (2)
scripts/og-preview.mjsscripts/og-render.mjs
💤 Files with no reviewable changes (1)
- scripts/og-preview.mjs
Comment on lines
+46
to
+58
| function isRunnable(path) { | ||
| if (!existsSync(path) || !statSync(path).isFile()) return false; | ||
| // The executable bit only means something on unix. Windows does not set it, | ||
| // and X_OK there degrades to a read check that any readable file passes, so | ||
| // go by extension instead: it is the nearest thing Windows has to "this can | ||
| // be run", and it is what rejects a path like package.json. | ||
| if (process.platform === 'win32') return extname(path).toLowerCase() === '.exe'; | ||
| try { | ||
| accessSync(path, constants.X_OK); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,140p' scripts/og-render.mjs | cat -n
echo
echo "== relevant symbols =="
rg -n "existsSync|statSync|accessSync|function isRunnable|function resolveChrome|CHROME_PATH|CHROME" scripts/og-render.mjsRepository: Sukarth/IB-EconGraph-AI
Length of output: 7592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node filesystem access APIs availability =="
node - <<'JS'
const fs = require('node:fs');
console.log('version:', process.version);
console.log('existsSync:', typeof fs.existsSync);
console.log('existsSyncSync:', typeof fs.existsSyncSync ?? 'undefined');
try {
fs.existsSyncSync('/tmp');
} catch (err) {
console.log('existsSyncSync throws:', err.constructor.name);
}
JS
echo
echo "== deterministic probe: separate statSync calls can throw after a path race =="
node - <<'JS'
const { mkdtempSync, writeFileSync, rmSync } = require('node:fs');
const { join } = require('node:path');
const { tmpdir } = require('node:os');
const dir = mkdtempSync(join(tmpdir(), 'crab-race-'));
const file = join(dir, 'probe');
writeFileSync(file, 'x');
function isRunnable(path) {
if (!fs.existsSync(path) || !fs.statSync(path).isFile()) return false;
try {
fs.accessSync(path, fs.constants.X_OK);
return true;
} catch {
return false;
}
}
try {
const result = fs.existsSyncSync && isRunnable(file);
console.log('existsSyncSync-isRunnable result:', result);
} catch (err) {
console.log('existsSyncSync-isRunnable threw:', err.code, err.message.split('\n')[0]);
}
rmSync(dir, { recursive: true, force: true });
JSRepository: Sukarth/IB-EconGraph-AI
Length of output: 447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic Node probe with current module-scope imports =="
node - <<'JS'
const { existsSync, statSync } = require('node:fs');
function isRunnable(path) {
if (!existsSync(path) || !statSync(path).isFile()) return false;
return true;
}
const { mkdtempSync, writeFileSync, rmSync } = require('node:fs');
const { join } = require('node:path');
const { tmpdir } = require('node:os');
const dir = mkdtempSync(join(tmpdir(), 'crab-race-'));
const file = join(dir, 'probe');
writeFileSync(file, 'x');
try {
const result = isRunnable(file);
console.log('existsSync-statSync-isRunnable result:', result);
} catch (err) {
console.log('existsSync-statSync-isRunnable threw:', err.code, err.message.split('\n')[0]);
}
rmSync(dir, { recursive: true, force: true });
JS
echo
echo "== relevant statSync call sites after line 140 =="
sed -n '140,190p' scripts/og-render.mjs | cat -nRepository: Sukarth/IB-EconGraph-AI
Length of output: 1047
Make path inspection exception-safe.
isRunnable() and the CHROME_PATH diagnostics use separate existsSync()/statSync() checks. If the file disappears or becomes inaccessible between checks, Node throws instead of returning false or printing the intended CHROME_PATH diagnostic. Use a guarded filesystem inspection and reuse that result/reason.
Also applies to lines 81-88.
🤖 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 `@scripts/og-render.mjs` around lines 46 - 58, The path inspection in
isRunnable and the CHROME_PATH diagnostics must be exception-safe against files
disappearing or becoming inaccessible between filesystem checks. Guard the
combined existence, stat, and relevant permission inspection in a single
try/catch, return false from isRunnable on any inspection failure, and reuse the
guarded result in the CHROME_PATH diagnostic flow so it still prints the
intended message without propagating filesystem exceptions.
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
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.
Two review findings from #3, both in dev-only tooling. No shipped output changes, and the committed PNGs are byte-identical after re-rendering.
og-render.mjstrustedCHROME_PATHwithout checking it. Pointed at a binary that is not there, it skipped detection and failed inside the render loop instead, as sixteen identical ENOENT errors naming the cards rather than the cause. It now exits 1 immediately, naming the path.og-preview.mjspreconnected tofonts.googleapis.comand never requested a stylesheet from it. The cards use Inter from the system, so the connection was opened for nothing.Verified all three paths: a bad
CHROME_PATHexits 1 with the path in the message; a valid one still renders all 16; unset still auto-detects.No version bump, since nothing user-facing changed.
Summary by Sourcery
Fail fast when CHROME_PATH points to a non-existent binary in OG rendering, and remove an unused font preconnect from the OG preview tooling.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Style