fix: ensure templates fallback to stale cache in case GH_TOKEN expired#2397
fix: ensure templates fallback to stale cache in case GH_TOKEN expired#2397
Conversation
WalkthroughRefactors template fetching and caching: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Gallery as TemplateGalleryService
participant Fetcher as TemplateFetcherService
participant GitHub as GitHub API
participant FS as Filesystem (cache)
participant Generator as TemplateGenerator (in-flight)
Gallery->>Fetcher: request latestCommitSha(repoOwner, repoName)
Fetcher->>GitHub: GET /repos/{owner}/{repo}/commits/{branch}
alt GitHub responds 200
GitHub-->>Fetcher: 200 + commit.sha
Fetcher-->>Gallery: return sha
else GitHub fails
GitHub-->>Fetcher: error
Fetcher-->>Gallery: throw error
end
Gallery->>FS: check exists `cachePrefix-<sha>.json`
alt cache exists
FS-->>Gallery: return file -> serve cached categories
else cache missing
Gallery->>Generator: is generation in-flight?
alt in-flight
Generator-->>Gallery: await result -> return categories
else not in-flight
Gallery->>Generator: start generation with sha (or determine sha via fallback)
alt fetcher failed earlier
Gallery->>FS: find files matching cachePrefix-* -> extract sha
FS-->>Gallery: maybe return existing sha or none
end
Generator-->>Gallery: generated categories
Gallery->>FS: write `cachePrefix-<sha>.json`
Gallery-->>Client: return categories
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
stalniy
force-pushed
the
refactor/templates-fallback
branch
from
59d8913 to
52c387f
Compare
![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
🧹 Nitpick comments (1)
apps/api/src/services/external/templates/template-gallery.service.ts (1)
91-91: String slicing logic may be fragile for SHA extraction.The slice calculation
files[0].slice(path.normalize(cachePathPrefix).length + 1, -1 * ".json".length)assumes the filename structure matches exactly. Consider using a more explicit approach:🔎 Proposed refactor using regex or path.basename
- return files[0].slice(path.normalize(cachePathPrefix).length + 1, -1 * ".json".length); + const filename = path.basename(files[0], ".json"); + const prefix = `${repoOwner}-${repoName}-`; + return filename.startsWith(prefix) ? filename.slice(prefix.length) : filename;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/services/external/templates/template-fetcher.service.ts(1 hunks)apps/api/src/services/external/templates/template-gallery.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/src/services/external/templates/template-fetcher.service.tsapps/api/src/services/external/templates/template-gallery.service.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1498
File: apps/api/src/services/external/templates/template-fetcher.service.ts:0-0
Timestamp: 2025-07-03T14:40:49.886Z
Learning: In the TemplateFetcherService class in apps/api/src/services/external/templates/template-fetcher.service.ts, the error handling strategy should maintain process resilience by catching all errors and returning null rather than re-throwing critical errors, to avoid breaking the whole template fetching process.
📚 Learning: 2025-07-03T14:40:49.886Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1498
File: apps/api/src/services/external/templates/template-fetcher.service.ts:0-0
Timestamp: 2025-07-03T14:40:49.886Z
Learning: In the TemplateFetcherService class in apps/api/src/services/external/templates/template-fetcher.service.ts, the error handling strategy should maintain process resilience by catching all errors and returning null rather than re-throwing critical errors, to avoid breaking the whole template fetching process.
Applied to files:
apps/api/src/services/external/templates/template-fetcher.service.tsapps/api/src/services/external/templates/template-gallery.service.ts
🧬 Code graph analysis (1)
apps/api/src/services/external/templates/template-gallery.service.ts (1)
apps/api/src/services/external/templates/template-fetcher.service.ts (1)
REPOSITORIES(14-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/api/src/services/external/templates/template-gallery.service.ts (2)
85-93: Good resilience pattern for stale cache fallback.The fallback mechanism that discovers existing cache files when
fetchLatestCommitShafails (e.g., due to expired GH_TOKEN) aligns well with the PR objective and the error handling strategy from learnings. This ensures templates remain available during GitHub API failures.
87-92: No action needed. fs.promises.glob is available in Node.js from version 22.0.0 behind the flag --experimental-glob and became un-flagged in v22.2.0. This codebase requires Node.js ^24.11.1, which fully supports both fs.promises.glob and Array.fromAsync—both are stable and cause no compatibility issues.Likely an incorrect or invalid review comment.
apps/api/src/services/external/templates/template-fetcher.service.ts (2)
73-77: Clean simplification of return type.The change from returning an object
{ repoOwner, repoName, repoVersion }to just the commit SHA string is a good simplification. The caller now correctly derivesrepoOwner/repoNamefrom theREPOSITORIESconstant, reducing redundancy.The addition of
{ cause: response }to the error is helpful for debugging failed requests.
63-78: Method throws but caller handles gracefully - aligns with learnings.While the retrieved learning suggests catching errors and returning null for resilience, in this case the throwing behavior is appropriate because the caller (
getTemplatesFromRepo) implements the fallback logic with.catch()that falls back to stale cache. This maintains process resilience at the appropriate layer.
| () => false | ||
| ); | ||
|
|
||
| console.log("Cache file exists:--------->", cacheFileExists, cacheFilePath, latestCommitSha); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug artifacts before merging.
The console.log statement with "Cache file exists:--------->" appears to be a debug artifact. Consider removing or replacing with structured logging consistent with the rest of the file.
🔎 Proposed fix
- console.log("Cache file exists:--------->", cacheFileExists, cacheFilePath, latestCommitSha);
+ console.log("Cache check:", { exists: cacheFileExists, path: cacheFilePath, sha: latestCommitSha });Or remove entirely if not needed for production logging.
📝 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.
| console.log("Cache file exists:--------->", cacheFileExists, cacheFilePath, latestCommitSha); | |
| console.log("Cache check:", { exists: cacheFileExists, path: cacheFilePath, sha: latestCommitSha }); |
🤖 Prompt for AI Agents
In apps/api/src/services/external/templates/template-gallery.service.ts at line
99 there is a console.log debug statement ("Cache file exists:--------->",
cacheFileExists, cacheFilePath, latestCommitSha); remove this debug artifact
before merging — either delete the line entirely if not needed or replace it
with the module's structured logger (e.g., logger.debug or processLogger.debug)
using a clear message and structured fields (cacheExists, cachePath, commitSha)
to match the file's existing logging conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/src/services/external/templates/template-gallery.service.ts (2)
91-91: Improve SHA extraction robustness with validation.The slice operation extracts the SHA from the filename but doesn't validate the result. If the filename format is unexpected, this could fail silently or produce an invalid SHA.
🔎 Suggested improvement with validation
- return files[0].slice(path.normalize(cachePathPrefix).length + 1, -1 * ".json".length); + const filename = path.basename(files[0], ".json"); + const sha = filename.slice(`${repoOwner}-${repoName}-`.length); + + // Validate SHA format (Git SHAs are 40-character hex strings) + if (!/^[0-9a-f]{40}$/i.test(sha)) { + throw new Error(`Invalid SHA extracted from cache filename: ${sha}`); + } + + return sha;This approach:
- Uses
path.basenamefor cleaner path handling- Validates the extracted SHA format
- Provides a clear error message if validation fails
88-88: Consider more specific glob pattern.The glob pattern
${cachePathPrefix}-*.jsoncould potentially match unexpected files if other similarly-named files exist in the directory.🔎 More specific pattern suggestion
- const files = await Array.fromAsync(fs.promises.glob(`${cachePathPrefix}-*.json`)); + const files = await Array.fromAsync(fs.promises.glob(`${cachePathPrefix}-[0-9a-f]*.json`));This pattern only matches files with hexadecimal characters after the prefix, which aligns with Git SHA format and reduces false matches.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/services/external/templates/template-fetcher.service.ts(1 hunks)apps/api/src/services/external/templates/template-gallery.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/services/external/templates/template-fetcher.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/src/services/external/templates/template-gallery.service.ts
🧠 Learnings (2)
📚 Learning: 2025-07-03T14:40:49.886Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1498
File: apps/api/src/services/external/templates/template-fetcher.service.ts:0-0
Timestamp: 2025-07-03T14:40:49.886Z
Learning: In the TemplateFetcherService class in apps/api/src/services/external/templates/template-fetcher.service.ts, the error handling strategy should maintain process resilience by catching all errors and returning null rather than re-throwing critical errors, to avoid breaking the whole template fetching process.
Applied to files:
apps/api/src/services/external/templates/template-gallery.service.ts
📚 Learning: 2025-11-25T17:45:39.561Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T17:45:39.561Z
Learning: Applies to **/*.{ts,tsx,js} : Don't add unnecessary comments to the code.
Applied to files:
apps/api/src/services/external/templates/template-gallery.service.ts
🧬 Code graph analysis (1)
apps/api/src/services/external/templates/template-gallery.service.ts (1)
apps/api/src/services/external/templates/template-fetcher.service.ts (1)
REPOSITORIES(14-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/services/external/templates/template-gallery.service.ts (2)
108-108: LGTM: Consistent SHA usage.The change to use
latestCommitShais correct and aligns with the new cache naming scheme and fallback mechanism.
87-92: No changes required. The code uses Array.fromAsync correctly and is fully compatible with the project's Node.js 24.11.1 requirement. The fallback logic for extracting the SHA from cached filenames works as intended with the controlled glob pattern.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (60.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2397 +/- ##
==========================================
- Coverage 51.18% 50.89% -0.30%
==========================================
Files 1072 1062 -10
Lines 29245 28901 -344
Branches 6412 6404 -8
==========================================
- Hits 14970 14709 -261
+ Misses 13862 13778 -84
- Partials 413 414 +1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
Why
Useful for templates resilience and for local development
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.