fix: resolve template directory path after esbuild bundling (refactored)#120
fix: resolve template directory path after esbuild bundling (refactored)#120sweetmantech merged 4 commits intomainfrom
Conversation
Refactored per review: - SRP: extract resolveTemplatesDir to its own file - DRY: use shared logStep instead of logger.log - OCP: extract logTemplateContents to its own file Template files were silently failing in production because __dirname points to esbuild output, not where additionalFiles places templates. resolveTemplatesDir tries __dirname first, falls back to process.cwd(). 209/209 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces runtime resolution for the templates directory and enhanced observability for template loading. It adds two new utility functions ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant loadTemplate
participant resolveTemplatesDir
participant fs
participant logTemplateContents
participant logStep
Client->>loadTemplate: loadTemplate()
loadTemplate->>resolveTemplatesDir: resolveTemplatesDir(__dirname)
resolveTemplatesDir->>fs: access(candidate1)
alt candidate1 exists
fs-->>resolveTemplatesDir: success
resolveTemplatesDir-->>loadTemplate: templateDir
else candidate1 not found
resolveTemplatesDir->>fs: access(candidate2)
fs-->>resolveTemplatesDir: success/failure
resolveTemplatesDir-->>loadTemplate: templateDir or error
end
loadTemplate->>fs: access(templateDir)
fs-->>loadTemplate: verify exists
loadTemplate->>logTemplateContents: logTemplateContents(templateDir)
logTemplateContents->>fs: readdir(recursive: true)
fs-->>logTemplateContents: file list
logTemplateContents->>logStep: log(success, samples)
logTemplateContents-->>loadTemplate: void
loadTemplate->>loadTemplate: loadJsonFile(styleGuide, etc.)
loadTemplate->>logStep: log(per-file success)
loadTemplate->>logStep: log(result summary)
loadTemplate-->>Client: TemplateData
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
- 3 tests for resolveTemplatesDir (dirname path, cwd fallback, throws) - 2 tests for logTemplateContents (success logging, error logging) 214/214 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
src/content/loadTemplate.ts
Outdated
| * Load a JSON file. Returns null if the file doesn't exist. | ||
| * Rethrows parse errors and unexpected I/O failures so callers surface real bugs. | ||
| */ | ||
| async function loadJsonFile<T>(filePath: string, label: string): Promise<T | null> { |
There was a problem hiding this comment.
srp - new file for any function with a name different from the file name.
src/content/logTemplateContents.ts
Outdated
There was a problem hiding this comment.
What is the purpose of this lib in the context of the goal this PR highlights?
There was a problem hiding this comment.
You're right — this was added for debugging the original issue but isn't needed for the fix itself. Removing it.
| const candidates = [ | ||
| path.resolve(dirname, "../content/templates"), | ||
| path.resolve(process.cwd(), "src/content/templates"), | ||
| ]; |
There was a problem hiding this comment.
Why do we need candidates? Shouldn't this value stay consistent?
There was a problem hiding this comment.
The two candidates exist because __dirname resolves differently depending on environment: in dev (tsx) it points to the source directory, but in production (esbuild bundle) it points to the build output. additionalFiles in trigger.config.ts copies templates to process.cwd()-relative paths. We need both so it works in both environments.
Address review: - SRP: loadJsonFile extracted to src/content/loadJsonFile.ts with 3 tests - Remove logTemplateContents (debug-only, not needed for the fix) 215/215 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Around line 39-42: The catch in loadTemplate that wraps any fs.access failure
into "Template directory not found" should only do that for ENOENT like
loadJsonFile does; update the catch after fs.access(templateDir) to inspect the
caught error (from the loadTemplate function) and if error.code === 'ENOENT'
throw the current descriptive Error(`Template directory not found:
${templateDir}`), otherwise rethrow the original error so permission or I/O
errors from fs.access bubble up unchanged.
- Around line 175-183: The loadJsonFile function currently casts JSON.parse(raw)
to T without runtime validation; update loadJsonFile to accept a Zod schema
parameter (e.g., schema: z.ZodType<T>) or overload to take a schema, parse the
file, then validate using schema.parse or schema.safeParse before returning;
only call logStep(`loadTemplate: loaded ${label}`...) after successful
validation and on validation failure throw or return null with a clear error
(include label and validation.errors) so callers (like loadTemplate) get
validated data; reference the existing function name loadJsonFile and the
logStep call to locate where to plug the schema validation.
In `@src/content/logTemplateContents.ts`:
- Around line 20-23: The log in logTemplateContents is being marked as a failure
(second arg true) which causes loadTemplate to treat the step as failed; change
the call in logTemplateContents so the logStep invocation is a log-only event
(do not pass true / pass false for the success flag) so the error is recorded
but currentStep isn't marked failed — update the logStep call referenced in
logTemplateContents (the object with templateDir and error: String(err))
accordingly so loadTemplate's control flow is unaffected.
In `@src/content/resolveTemplatesDir.ts`:
- Around line 20-26: The loop in resolveTemplatesDir currently swallows all
errors from fs.access, causing permission or other I/O errors (EACCES, EPERM,
etc.) to be treated as "not found"; update the try/catch inside the candidates
loop to only ignore errors where err.code === 'ENOENT' and rethrow any other
error, referencing the resolveTemplatesDir function and the fs.access call; also
add a regression test that simulates a non-ENOENT error (e.g., EACCES) on the
first candidate to ensure the function surfaces the error instead of falling
back to cwd.
🪄 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: ea87dd5e-24ea-40ae-8994-8fba21a59c59
📒 Files selected for processing (5)
src/content/__tests__/logTemplateContents.test.tssrc/content/__tests__/resolveTemplatesDir.test.tssrc/content/loadTemplate.tssrc/content/logTemplateContents.tssrc/content/resolveTemplatesDir.ts
| try { | ||
| await fs.access(templateDir); | ||
| } catch { | ||
| throw new Error(`Template directory not found: ${templateDir}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "loadTemplate.ts" | head -5Repository: recoupable/tasks
Length of output: 89
🏁 Script executed:
cat -n ./src/content/loadTemplate.tsRepository: recoupable/tasks
Length of output: 7053
🏁 Script executed:
find . -type f -name "loadJsonFile.ts" -o -name "loadJsonFile.js"Repository: recoupable/tasks
Length of output: 89
🏁 Script executed:
cat -n ./src/content/loadJsonFile.tsRepository: recoupable/tasks
Length of output: 1213
Mirror the ENOENT-only error handling pattern already implemented in loadJsonFile().
The fs.access() catch block at lines 39–42 masks all errors with a generic "Template directory not found" message. Permission errors (EACCES/EPERM), I/O failures, and other non-ENOENT errors should bubble up unchanged so that deployment issues surface with their actual root cause, matching the error-handling discipline already established in loadJsonFile().
Proposed fix
try {
await fs.access(templateDir);
- } catch {
- throw new Error(`Template directory not found: ${templateDir}`);
+ } catch (err: unknown) {
+ if (
+ err instanceof Error &&
+ "code" in err &&
+ (err as NodeJS.ErrnoException).code === "ENOENT"
+ ) {
+ throw new Error(`Template directory not found: ${templateDir}`);
+ }
+ throw err;
}📝 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.
| try { | |
| await fs.access(templateDir); | |
| } catch { | |
| throw new Error(`Template directory not found: ${templateDir}`); | |
| try { | |
| await fs.access(templateDir); | |
| } catch (err: unknown) { | |
| if ( | |
| err instanceof Error && | |
| "code" in err && | |
| (err as NodeJS.ErrnoException).code === "ENOENT" | |
| ) { | |
| throw new Error(`Template directory not found: ${templateDir}`); | |
| } | |
| 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 39 - 42, The catch in loadTemplate
that wraps any fs.access failure into "Template directory not found" should only
do that for ENOENT like loadJsonFile does; update the catch after
fs.access(templateDir) to inspect the caught error (from the loadTemplate
function) and if error.code === 'ENOENT' throw the current descriptive
Error(`Template directory not found: ${templateDir}`), otherwise rethrow the
original error so permission or I/O errors from fs.access bubble up unchanged.
src/content/loadTemplate.ts
Outdated
| async function loadJsonFile<T>(filePath: string, label: string): Promise<T | null> { | ||
| try { | ||
| const raw = await fs.readFile(filePath, "utf-8"); | ||
| return JSON.parse(raw) as T; | ||
| } catch { | ||
| return null; | ||
| const parsed = JSON.parse(raw) as T; | ||
| logStep(`loadTemplate: loaded ${label}`, false, { | ||
| path: filePath, | ||
| sizeBytes: raw.length, | ||
| }); | ||
| return parsed; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file and examine the specific lines
cat -n src/content/loadTemplate.ts | head -200 | tail -50Repository: recoupable/tasks
Length of output: 2425
🏁 Script executed:
# Search for Zod imports and usage in the codebase
rg "from.*zod|import.*zod" -iRepository: recoupable/tasks
Length of output: 1192
🏁 Script executed:
# Look for JSON.parse patterns and validation approaches in the codebase
rg "JSON\.parse" -A 3 -B 1 | head -100Repository: recoupable/tasks
Length of output: 3854
🏁 Script executed:
# Find loadTemplate.ts and loadJsonFile.ts
fd -t f "loadTemplate|loadJsonFile" -e tsRepository: recoupable/tasks
Length of output: 158
🏁 Script executed:
# Check if loadTemplate.ts exists and find its size
find . -name "loadTemplate.ts" -type f -exec wc -l {} +Repository: recoupable/tasks
Length of output: 93
🏁 Script executed:
# Get full content of loadJsonFile.ts to verify it matches the review
cat src/content/loadJsonFile.tsRepository: recoupable/tasks
Length of output: 975
🏁 Script executed:
# Read loadTemplate.ts to see the actual content
cat -n src/content/loadTemplate.tsRepository: recoupable/tasks
Length of output: 7053
🏁 Script executed:
# Check how loadJsonFile is being used - look for calls to it
rg "loadJsonFile" -B 2 -A 2Repository: recoupable/tasks
Length of output: 3899
Add Zod schema validation to loadJsonFile.
The function uses JSON.parse(raw) as T which accepts any shape at runtime, and logs success before validating that the loaded data actually matches the expected structure. Malformed template files could pass validation and only fail later through unsafe casts (as seen in loadTemplate.ts lines 83-85).
Implement Zod schemas for template payload types and validate within loadJsonFile before returning or logging success. This aligns with the coding guideline "Use Zod for schema validation" for src/**/*.{ts,tsx} and is consistent with Zod usage throughout the codebase.
File: src/content/loadJsonFile.ts
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/loadTemplate.ts` around lines 175 - 183, The loadJsonFile
function currently casts JSON.parse(raw) to T without runtime validation; update
loadJsonFile to accept a Zod schema parameter (e.g., schema: z.ZodType<T>) or
overload to take a schema, parse the file, then validate using schema.parse or
schema.safeParse before returning; only call logStep(`loadTemplate: loaded
${label}`...) after successful validation and on validation failure throw or
return null with a clear error (include label and validation.errors) so callers
(like loadTemplate) get validated data; reference the existing function name
loadJsonFile and the logStep call to locate where to plug the schema validation.
src/content/logTemplateContents.ts
Outdated
| logStep("loadTemplate: failed to list directory", true, { | ||
| templateDir, | ||
| error: String(err), | ||
| }); |
There was a problem hiding this comment.
Keep this as a log-only event.
logTemplateContents() swallows the error and loadTemplate() continues, so passing true here can leave currentStep in a failure state even when the template loads successfully.
Proposed fix
- logStep("loadTemplate: failed to list directory", true, {
+ logStep("loadTemplate: failed to list directory", false, {
templateDir,
error: String(err),
});📝 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.
| logStep("loadTemplate: failed to list directory", true, { | |
| templateDir, | |
| error: String(err), | |
| }); | |
| logStep("loadTemplate: failed to list directory", false, { | |
| templateDir, | |
| error: String(err), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/logTemplateContents.ts` around lines 20 - 23, The log in
logTemplateContents is being marked as a failure (second arg true) which causes
loadTemplate to treat the step as failed; change the call in logTemplateContents
so the logStep invocation is a log-only event (do not pass true / pass false for
the success flag) so the error is recorded but currentStep isn't marked failed —
update the logStep call referenced in logTemplateContents (the object with
templateDir and error: String(err)) accordingly so loadTemplate's control flow
is unaffected.
CodeRabbit feedback: bare catch swallowed EACCES/EPERM, masking real issues. Now only ENOENT falls through to the next candidate. 216/216 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
__dirnamepoints to esbuild outputresolveTemplatesDirtries__dirnamefirst, falls back toprocess.cwd()Review fixes from #117
resolveTemplatesDirextracted tosrc/content/resolveTemplatesDir.tslogger.logreplaced with sharedlogStepsrc/content/logTemplateContents.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests