refactor: skips parsing JSON cache when serving templates#2566
Conversation
📝 WalkthroughWalkthroughAdds a file-backed galleries cache with build/read APIs, makes the template fetcher optional, changes gallery/category shapes to arrays, removes the full-templates endpoint, and updates controllers, routes, schemas, services, scripts, and tests to use the new cache and return shapes. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as buildAkashTemplatesCache
participant Gallery as TemplateGalleryService
participant Fetcher as TemplateFetcherService
participant Processor as TemplateProcessorService
participant FS as FileSystem
participant Controller as TemplateController
Note over Script,FS: Build cache flow
Script->>Gallery: buildTemplateGalleryCache(categoriesSchema)
Gallery->>Fetcher: getTemplatesFromRepo() (if templateFetcher available)
Fetcher-->>Gallery: raw repo files
Gallery->>Processor: mergeTemplateCategories(...)
Processor-->>Gallery: merged Category[]
Gallery->>Gallery: buildContentRanges() & assemble GalleriesCache
Gallery->>FS: write GalleriesCache to disk
FS-->>Gallery: write confirmation
Gallery-->>Script: GalleriesCache
Note over Controller,FS: Retrieval & per-id fetch
Controller->>Gallery: getCachedTemplateGallery()
Gallery->>FS: read GalleriesCache file
FS-->>Gallery: GalleriesCache contents
Gallery->>Gallery: use ranges + `#parsedTemplates` to extract & parse template
Gallery-->>Controller: { data: Template } or categories payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api/src/template/services/template-gallery/template-gallery.service.ts`:
- Around line 51-54: The cache existence check currently requires both read and
write permissions which breaks on read‑only volumes; update the access call in
the template gallery service so cacheFileExists is computed using only read
permission by calling this.#fs.access(this.#galleriesCachePath,
fsConstants.R_OK) (replace the existing fsConstants.W_OK | fsConstants.R_OK),
keeping the same promise-to-boolean pattern around this.#fs.access and leaving
this.#galleriesCachePath and cacheFileExists unchanged.
- Around line 105-106: The code calls await
this.#fs.writeFile(this.#galleriesCachePath, content) without ensuring the
target directory exists, which causes writeFile to throw on first run; before
writing, ensure the directory containing this.#galleriesCachePath exists by
creating it if missing (use your FS helper on this.#fs or Node fs.mkdir with {
recursive: true }) and then perform the write; update the method that uses
writeFile (referenced symbol: this.#galleriesCachePath and this.#fs.writeFile)
to create the parent directory for the templates/ folder first and only then
call writeFile.
🧹 Nitpick comments (2)
apps/api/src/template/services/template-gallery/template-gallery.service.spec.ts (1)
269-270: Acknowledged technical debt.The HACK comment is self-documented. Consider creating an issue to track refactoring the test setup to avoid assigning private properties directly. This could be addressed by exposing a factory method or using dependency injection more fully.
Do you want me to open an issue to track refactoring this test setup?
apps/api/src/template/routes/templates/templates.router.ts (1)
29-34: Type cast workaround is acceptable but worth documenting.The double cast
as unknown as TypedResponse<...>bypasses type checking becausec.body()returns a raw body response, not a JSON-typed response. This is intentional to avoid re-serialization overhead.Consider adding a brief inline comment explaining why this cast is needed:
📝 Suggested documentation
templatesRouter.openapi(getTemplatesListRoute, async function routeGetTemplatesList(c) { const result = await container.resolve(TemplateController).getTemplatesList(); c.header("Content-Type", "application/json"); + // Cast needed: result is pre-serialized JSON string, bypassing c.json() to avoid re-serialization return c.body(result, 200) as unknown as TypedResponse<GetTemplatesListResponse, 200, "json">; });
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (79.71%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2566 +/- ##
==========================================
- Coverage 50.93% 50.73% -0.21%
==========================================
Files 1069 1059 -10
Lines 29815 29499 -316
Branches 6618 6579 -39
==========================================
- Hits 15187 14965 -222
+ Misses 14383 14298 -85
+ Partials 245 236 -9
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/templates3
branch
from
ab8694d to
cd858a2
Compare
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Show resolved
Hide resolved
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/templates3
branch
from
cd858a2 to
6dea34e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/template/controllers/template/template.controller.ts (1)
15-16: Logger context references wrong class name.The logger context is set to
TemplateGalleryService.namebut this isTemplateController. This will cause confusing log output.🔧 Proposed fix
- logger.setContext(TemplateGalleryService.name); + logger.setContext(TemplateController.name);
🤖 Fix all issues with AI agents
In `@apps/api/src/template/services/template-gallery/template-gallery.service.ts`:
- Around line 60-66: Wrap the JSON.parse(metadataString) call in a try-catch
inside the method that reads the galleries cache (the block using
this.#fs.readFile and this.#galleriesCachePath); if parsing fails, catch the
error and throw or return a clearer, contextual error that includes the cache
path and the raw metadataString (or a short preview) so callers can understand
the corrupted metadata, e.g., catch the parse error around
JSON.parse(metadataString) and rethrow a new Error mentioning "Failed to parse
gallery metadata" plus the path/preview and the original error message.
🧹 Nitpick comments (3)
apps/api/src/template/controllers/template/template.controller.ts (1)
23-26: Manual JSON string assembly creates tight coupling.The
{"data":${categories}}construction assumescategoriesis a valid JSON array string. This is intentional for the performance optimization, but creates implicit coupling withgetCachedTemplateGallery()returning properly formatted JSON.Consider adding a brief comment documenting this invariant for maintainability.
apps/api/src/template/services/template-gallery/template-gallery.service.spec.ts (1)
269-270: Acknowledged tech debt for private property assignment.The
HACKcomment at line 269 documents the workaround for testing. Per learnings, consider creating a separate issue to track refactoring the dependency injection approach.apps/api/scripts/buildAkashTemplatesCache.ts (1)
25-29: Consider adding success logging.The script logs errors but doesn't confirm successful completion. Adding a success message would help with build pipeline observability.
🔧 Proposed enhancement
-templateGalleryService.buildTemplateGalleryCache(GetTemplatesListResponseSchema.shape.data).catch(err => { +templateGalleryService.buildTemplateGalleryCache(GetTemplatesListResponseSchema.shape.data).then(() => { + console.log("Akash templates cache warmed up successfully"); +}).catch(err => { console.error("Encountered an error trying to warm up Akash templates cache"); console.error(err); process.exit(1); });
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/templates3
branch
from
6dea34e to
6436ae8
Compare
2 participants
Why
Currently combined templates cache size is 4.7Mb.
JSON.parseis a sync operation and it may block event loop. Because of this liveness helthcheck fails and container is marked as unhealthyWhat
JSON.parseSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.