fix: resolve template directory path after esbuild bundling#117
fix: resolve template directory path after esbuild bundling#117recoupableorg merged 2 commits intomainfrom
Conversation
esbuild changes __dirname to the build output directory, so path.resolve(__dirname, "../content/templates") fails silently in production — all template files (style guide, caption guide, moods, movements, reference images) load as null and the pipeline generates generic content instead of following template instructions. Adds resolveTemplatesDir() that tries __dirname first (works locally with tsx) then falls back to process.cwd()-relative path (works in Trigger.dev deployments where additionalFiles preserves the source directory structure). Also adds diagnostic logging so template load failures are visible in the Trigger.dev dashboard instead of being silently swallowed. Made-with: Cursor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced a single __dirname-based template path with a resolver that probes multiple filesystem locations, added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/content/loadTemplate.ts (2)
216-233: Differentiate missing files from malformed JSON.Right now parse errors and read errors both return
null, which can still silently degrade output quality for broken template files. Prefernullonly forENOENT, and throw for invalid JSON/other I/O errors.Proposed fix
async function loadJsonFile<T>(filePath: string, label: string): Promise<T | null> { try { const raw = await fs.readFile(filePath, "utf-8"); const parsed = JSON.parse(raw) as T; logger.log(`loadTemplate: loaded ${label}`, { path: filePath, - sizeBytes: raw.length, + sizeBytes: Buffer.byteLength(raw, "utf8"), }); return parsed; } catch (err) { - logger.warn(`loadTemplate: FAILED to load ${label}`, { - path: filePath, - error: String(err), - }); - return null; + const e = err as NodeJS.ErrnoException; + if (e?.code === "ENOENT") { + logger.warn(`loadTemplate: missing optional ${label}`, { + path: filePath, + }); + return null; + } + logger.error(`loadTemplate: invalid ${label}`, { + path: filePath, + error: String(err), + }); + throw err; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/loadTemplate.ts` around lines 216 - 233, In loadJsonFile, distinguish missing-file (ENOENT) from parse/other I/O errors: catch the error, if err.code === "ENOENT" log the missing-file case with logger.warn and return null; for all other errors (e.g., JSON.parse failures or other read issues) log the error with logger.warn including String(err) and rethrow the error so callers can handle it; keep references to loadJsonFile, fs.readFile, JSON.parse and logger.warn when updating the catch block.
81-84: Consider capping directory-content log payload size.Logging full recursive file lists can become noisy/expensive in production runs; count + sample is usually enough.
Proposed tweak
- logger.log("loadTemplate: directory contents", { - templateDir, - files: entries, - }); + logger.log("loadTemplate: directory contents", { + templateDir, + count: entries.length, + sample: entries.slice(0, 50), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/loadTemplate.ts` around lines 81 - 84, The log in loadTemplate that calls logger.log("loadTemplate: directory contents", { templateDir, files: entries }) can emit a huge payload; change it to include a count and a sample instead of the full entries array — e.g., log templateDir, totalFiles: entries.length, and sampleFiles: entries.slice(0, N) (where N is a small constant) and optionally a flag hasMore when entries.length > N; update the logger call in loadTemplate to use these fields rather than passing the entire entries object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/content/loadTemplate.ts`:
- Line 80: The code uses fs.readdir(templateDir, { recursive: true }) which
requires Node >=18.17.0 (or 20.1.0+); add an engines.node field to package.json
(e.g., "node": ">=18.17.0") or confirm the deployment/runtime (Trigger.dev)
guarantees a compatible Node version so the call in loadTemplate.ts (fs.readdir,
templateDir, entries) won’t fail at runtime.
---
Nitpick comments:
In `@src/content/loadTemplate.ts`:
- Around line 216-233: In loadJsonFile, distinguish missing-file (ENOENT) from
parse/other I/O errors: catch the error, if err.code === "ENOENT" log the
missing-file case with logger.warn and return null; for all other errors (e.g.,
JSON.parse failures or other read issues) log the error with logger.warn
including String(err) and rethrow the error so callers can handle it; keep
references to loadJsonFile, fs.readFile, JSON.parse and logger.warn when
updating the catch block.
- Around line 81-84: The log in loadTemplate that calls
logger.log("loadTemplate: directory contents", { templateDir, files: entries })
can emit a huge payload; change it to include a count and a sample instead of
the full entries array — e.g., log templateDir, totalFiles: entries.length, and
sampleFiles: entries.slice(0, N) (where N is a small constant) and optionally a
flag hasMore when entries.length > N; update the logger call in loadTemplate to
use these fields rather than passing the entire entries object.
🪄 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
Run ID: 3d034fae-f59c-4ec9-b45f-56ec47969e18
📒 Files selected for processing (1)
src/content/loadTemplate.ts
|
|
||
| // List what's actually in the template directory | ||
| try { | ||
| const entries = await fs.readdir(templateDir, { recursive: true }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node engine constraints from package manifests =="
fd -i '^package\.json$' -x sh -c '
echo "--- $1"
jq -r ".engines.node // \"<engines.node not set>\"" "$1"
' sh {}
echo
echo "== Sites using recursive readdir =="
rg -nP --type=ts '\breaddir\s*\([^)]*\{[^}]*\brecursive\s*:\s*true'
echo
echo "== Trigger config context =="
fd -i 'trigger.config.ts' -x sed -n '1,180p' {}Repository: recoupable/tasks
Length of output: 1437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node version config files =="
fd -E node_modules '\.nvmrc|\.node-version|engines\.node' -x cat {} 2>/dev/null || true
echo
echo "== Dockerfile/Docker references =="
fd -E node_modules 'Dockerfile|docker-compose' -x head -20 {} \; 2>/dev/null || true
echo
echo "== Context around loadTemplate.ts to understand usage =="
head -n 100 src/content/loadTemplate.ts | tail -n 30Repository: recoupable/tasks
Length of output: 1087
🌐 Web query:
Node.js fs.readdir recursive option support added when version
💡 Result:
The { recursive: true } option for fs.readdir(), fsPromises.readdir(), and fs.readdirSync() was added in Node.js v20.1.0 (and backported to v18.17.0). [1] [2]
Document Node.js version requirement for readdir({ recursive: true }).
The { recursive: true } option requires Node.js 18.17.0 or 20.1.0+. The code already has error handling with try-catch, but there's no explicit Node version constraint in package.json. Add an engines.node field to ensure the runtime meets this requirement:
"engines": {
"node": ">=18.17.0"
}Alternatively, verify that Trigger.dev's runtime constraints already enforce a compatible Node version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/loadTemplate.ts` at line 80, The code uses
fs.readdir(templateDir, { recursive: true }) which requires Node >=18.17.0 (or
20.1.0+); add an engines.node field to package.json (e.g., "node": ">=18.17.0")
or confirm the deployment/runtime (Trigger.dev) guarantees a compatible Node
version so the call in loadTemplate.ts (fs.readdir, templateDir, entries) won’t
fail at runtime.
- loadJsonFile: distinguish ENOENT (return null) from parse/IO errors (rethrow) so malformed JSON surfaces instead of being silently swallowed - directory contents log: cap payload to count + 10-file sample instead of dumping the full entries array Made-with: Cursor
| * 2. process.cwd()-relative (works in Trigger.dev deployments where | ||
| * additionalFiles preserves source-root-relative paths) | ||
| */ | ||
| async function resolveTemplatesDir(): Promise<string> { |
There was a problem hiding this comment.
SRP - new lib file for any function with a name different from the file name.
| const templatesDir = await resolveTemplatesDir(); | ||
| const templateDir = path.join(templatesDir, templateName); | ||
|
|
||
| logger.log("loadTemplate: resolving paths", { |
There was a problem hiding this comment.
DRY - use the shared logStep function
| const templatesDir = await resolveTemplatesDir(); | ||
| const templateDir = path.join(templatesDir, templateName); | ||
|
|
||
| logger.log("loadTemplate: resolving paths", { | ||
| __dirname, | ||
| cwd: process.cwd(), | ||
| templatesDir, | ||
| templateDir, | ||
| }); | ||
|
|
||
| // Check the template directory exists | ||
| try { | ||
| await fs.access(templateDir); | ||
| } catch { | ||
| logger.error("loadTemplate: template directory does not exist", { | ||
| templateDir, | ||
| }); | ||
| throw new Error(`Template directory not found: ${templateDir}`); | ||
| } | ||
|
|
||
| // List what's actually in the template directory | ||
| const MAX_SAMPLE_FILES = 10; | ||
| try { | ||
| const entries = await fs.readdir(templateDir, { recursive: true }); | ||
| logger.log("loadTemplate: directory contents", { | ||
| templateDir, | ||
| totalFiles: entries.length, | ||
| sampleFiles: entries.slice(0, MAX_SAMPLE_FILES), | ||
| ...(entries.length > MAX_SAMPLE_FILES && { hasMore: true }), | ||
| }); | ||
| } catch (err) { | ||
| logger.error("loadTemplate: failed to list directory", { | ||
| templateDir, | ||
| error: String(err), | ||
| }); | ||
| } |
There was a problem hiding this comment.
OCP
- actual: changes added directly inline to existing loadTemplate function
- required: new lib file for new code. only one line function call added here.
Summary
__dirnamepoints to esbuild's build output directory, not the source directory whereadditionalFilesplaces the templatesresolveTemplatesDir()that tries__dirname-relative first, then falls back toprocess.cwd()-relative pathBefore: Production generates generic portraits (no style guide, no scene instructions, no caption rules)
After: Production follows the full template — bedroom setting, purple LED, deadpan expression, proper captions
Test plan
pnpm run deploy:trigger-prod, trigger a production run and verify video matches template styleMade with Cursor
Summary by CodeRabbit
Bug Fixes
Chores