refactor: adds proper logger and move templates into module#2563
refactor: adds proper logger and move templates into module#2563
Conversation
📝 WalkthroughWalkthroughRefactors template subsystem to inject Logger, fetch, and Octokit dependencies; replaces PAT-centric Octokit helpers; relocates and reimplements TemplateGalleryService with commit-SHA caching and filesystem fallback; adds in-flight promise reuse helper; changes merge result shape to include a template ID index; adds extensive unit tests and removes legacy functional mocks/tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as TemplateController
participant Gallery as TemplateGalleryService
participant Fetcher as TemplateFetcherService
participant Processor as TemplateProcessorService
participant GitHub as GitHub/Octokit
participant FS as FileSystem
Controller->>Gallery: getTemplateGallery()
Gallery->>Gallery: check lastWorking cache
opt not cached
par fetch repos
Gallery->>Fetcher: fetch repo A
Fetcher->>GitHub: fetch latest commit SHA
GitHub-->>Fetcher: sha
Fetcher->>FS: read cache by sha
alt cache hit
FS-->>Fetcher: cached categories
else
Fetcher->>GitHub: list files / fetch content
fetcher->>Processor: processTemplate(file, readme, deploy, guide?)
Processor-->>Fetcher: processed templates
Fetcher->>FS: write cache by sha
end
Fetcher-->>Gallery: categories
Note over Fetcher: repeat for other repos (B, C)
end
Gallery->>Processor: mergeTemplateCategories(all categories)
Processor-->>Gallery: { categories, templatesIds }
Gallery->>Gallery: store lastWorking
end
Gallery-->>Controller: categories (and index)
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 |
stalniy
force-pushed
the
refactor/templates2
branch
from
5f2fb64 to
987ad03
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: 3
🤖 Fix all issues with AI agents
In `@apps/api/src/caching/helpers.ts`:
- Around line 151-164: The function signature uses `any` in the generic
constraint which violates the guideline; update the signature to avoid `any` by
changing the constraint to `T extends (...args: unknown[]) => Promise<unknown>`
(so `Parameters<T>` and `ReturnType<T>` still work) and ensure any other
parameter types that used `any[]` are replaced with `unknown[]` (e.g., the
`getKey` callback already uses `(...args: Parameters<T>)`, so no change there).
This keeps the same runtime behavior while removing `any` from
`reusePendingPromise`.
In `@apps/api/src/template/services/template-gallery/template-gallery.service.ts`:
- Around line 12-17: The Options type currently marks githubPAT as optional but
getOctokit (used in TemplateGalleryService and related functions) throws if PAT
is missing; update the Options type to make githubPAT required (remove the ? on
githubPAT) or, alternatively, add explicit handling where getOctokit is called
(in methods like the TemplateGalleryService constructor or any function that
calls getOctokit) to throw a clear error or provide a fallback when githubPAT is
undefined; ensure references to githubPAT in functions that build the Octokit
client (getOctokit) are updated to reflect the non-optional type or guarded with
a clear runtime check and error message.
In
`@apps/api/src/template/services/template-processor/template-processor.service.spec.ts`:
- Around line 464-474: Update the setup helper so it accepts a single typed
options parameter (e.g., options?: { templateSource?: Partial<TemplateSource> })
instead of no args: construct the default TemplateSource inline, merge any
provided options.templateSource into those defaults, instantiate
TemplateProcessorService(), and return { service, templateSource } so existing
tests keep working; reference function name setup and types
TemplateProcessorService and TemplateSource when making the change.
🧹 Nitpick comments (2)
apps/api/src/template/services/template-gallery/template-gallery.service.spec.ts (1)
190-212: Alignsetupwith test conventions and avoid private state patching.
- The setup helper should accept a single parameter with an inline type definition to match repo test conventions.
Object.assign(service, { templateFetcher })pokes at private state; consider constructor injection or a factory to keep tests from mutating internals. I can help draft a test-friendly constructor/factory if you want.♻️ Suggested setup signature adjustment
- function setup() { + function setup({ fsAccessError = new Error("File not found") }: { fsAccessError?: Error } = {}) { const logger = mock<LoggerService>(); const fsMock = mock<FileSystemApi>({ - access: jest.fn(() => Promise.reject(new Error("File not found"))) + access: jest.fn(() => Promise.reject(fsAccessError)) });As per coding guidelines, setup should accept a single parameter with an inline type definition.
apps/api/src/template/services/template-fetcher/template-fetcher.service.spec.ts (1)
604-612: Updatesetupsignature to match test conventions.The setup helper should accept a single parameter with an inline type definition.
♻️ Suggested setup signature adjustment
- function setup() { + function setup({ fetchImpl }: { fetchImpl?: typeof globalThis.fetch } = {}) { const templateProcessor = mock<TemplateProcessorService>(); const logger = mock<LoggerService>(); const fetchMock = jest.fn(); const octokit = mockDeep<Octokit>(); - const service = new TemplateFetcherService(templateProcessor, logger, fetchMock as typeof globalThis.fetch, octokit); + const service = new TemplateFetcherService( + templateProcessor, + logger, + (fetchImpl ?? fetchMock) as typeof globalThis.fetch, + octokit + );As per coding guidelines, setup should accept a single parameter with an inline type definition.
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Show resolved
Hide resolved
apps/api/src/template/services/template-processor/template-processor.service.spec.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/templates2
branch
from
987ad03 to
2902011
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/services/template-fetcher/template-fetcher.service.ts (1)
146-173: ReplacePromise<any>with proper return type.The return type
Promise<any>on line 150 violates the coding guideline forbiddingany. Based on the function logic, this should returnPromise<Template | null>.🛠️ Suggested fix
+import type { Template } from "../../types/template"; + private async processTemplateSource( templateSource: TemplateSource, directoryItems: GithubDirectoryItem[], options: { includeConfigJson?: boolean } -): Promise<any> { +): Promise<Template | null> {
🤖 Fix all issues with AI agents
In `@apps/api/src/template/services/template-gallery/template-gallery.service.ts`:
- Around line 125-134: The code assumes files[0] from glob() is the "latest"
cached version, but glob() is unordered; update the logic in
template-gallery.service.ts that computes latestCachedVersionSha (using files[0]
and logging event "TEMPLATES_FALLBACK_LATEST_COMMIT_SHA_FOUND") to be
deterministic: either sort the files array (e.g., alphabetically or by file
mtime) then pick the newest entry before slicing to derive
latestCachedVersionSha, or rename the variable/log to something like
cachedVersionSha and change the event/message to indicate "a cached version"
instead of "latest" so behavior and logs are accurate.
♻️ Duplicate comments (3)
apps/api/src/template/services/template-processor/template-processor.service.spec.ts (1)
464-474: Makesetupaccept a single typed options parameter.Per coding guidelines, the
setupfunction should accept a single parameter with an inline type definition to enable test customization while avoiding shared state.🛠️ Suggested fix
-function setup() { +function setup({ templateSourceOverrides }: { templateSourceOverrides?: Partial<TemplateSource> } = {}) { const service = new TemplateProcessorService(); const templateSource: TemplateSource = { name: "Test Template", path: "templates/app", repoOwner: "test-owner", repoName: "test-repo", - repoVersion: "main" + repoVersion: "main", + ...templateSourceOverrides }; return { service, templateSource }; }apps/api/src/caching/helpers.ts (1)
151-165: AvoidanyinreusePendingPromisesignature.The codebase guideline forbids
any. Useunknown[]instead to maintain type safety while preserving the generic inference.🛠️ Suggested fix
-export function reusePendingPromise<T extends (...args: any[]) => Promise<unknown>>(fn: T, options?: { getKey?: (...args: Parameters<T>) => string }): T { +export function reusePendingPromise<T extends (...args: unknown[]) => Promise<unknown>>(fn: T, options?: { getKey?: (...args: Parameters<T>) => string }): T {apps/api/src/template/services/template-gallery/template-gallery.service.ts (1)
12-17: AligngithubPATtype with runtime requirement.
getOctokitthrows when PAT is missing, butOptions.githubPATis optional. Make it required or guard before constructing.Also applies to: 167-170
| if (!files.length) throw error; | ||
|
|
||
| const latestCachedVersionSha = files[0].slice(path.normalize(cachePathPrefix).length + 1, -1 * ".json".length); | ||
| this.#logger.debug({ | ||
| event: "TEMPLATES_FALLBACK_LATEST_COMMIT_SHA_FOUND", | ||
| message: "Trying to use the latest cached version", | ||
| repository, | ||
| commitSha: latestCachedVersionSha | ||
| }); | ||
| return latestCachedVersionSha; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/api/src/template/services/template-gallery/template-gallery.service.ts | sed -n '110,150p'Repository: akash-network/console
Length of output: 2059
🌐 Web query:
Node.js fs.promises.glob ordering guarantee deterministic
💡 Result:
fs.promises.glob() does not document any ordering guarantee for the paths it yields, so you should treat the output order as unspecified / not deterministic across OS/filesystems/runs. The iterator is ultimately driven by filesystem directory iteration, which Node explicitly says returns entries in “no particular order” as provided by the OS. [1]
If you need deterministic results, collect and sort yourself, e.g.:
import { glob } from "node:fs/promises";
const matches = await Array.fromAsync(glob("**/*.js"));
matches.sort((a, b) => a.localeCompare(b)); // or Intl.Collator for natural sort(For context: popular globbing implementations similarly warn that results may be returned in arbitrary order.) [2]
Sources: [1] [2]
"Latest cached version" selection isn't deterministic.
files[0] from glob() returns results in arbitrary order (no ordering guarantee), so the "latest" log message is misleading. Either sort files deterministically (e.g., alphabetically or by mtime) or rename the variable and message to reflect it's just "a cached version" rather than "the latest."
🤖 Prompt for AI Agents
In `@apps/api/src/template/services/template-gallery/template-gallery.service.ts`
around lines 125 - 134, The code assumes files[0] from glob() is the "latest"
cached version, but glob() is unordered; update the logic in
template-gallery.service.ts that computes latestCachedVersionSha (using files[0]
and logging event "TEMPLATES_FALLBACK_LATEST_COMMIT_SHA_FOUND") to be
deterministic: either sort the files array (e.g., alphabetically or by file
mtime) then pick the newest entry before slicing to derive
latestCachedVersionSha, or rename the variable/log to something like
cachedVersionSha and change the event/message to indicate "a cached version"
instead of "latest" so behavior and logs are accurate.
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (79.46%) 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 #2563 +/- ##
==========================================
- Coverage 50.76% 50.55% -0.22%
==========================================
Files 1069 1058 -11
Lines 29728 29401 -327
Branches 6583 6542 -41
==========================================
- Hits 15091 14863 -228
+ Misses 14286 14200 -86
+ Partials 351 338 -13
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/templates2
branch
from
2902011 to
70804f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/services/template-fetcher/template-fetcher.service.ts (1)
146-172: Avoidanyreturn type.The method returns
Promise<any>which violates the coding guideline forbiddingany. Use the proper return type.🛠️ Suggested fix
private async processTemplateSource( templateSource: TemplateSource, directoryItems: GithubDirectoryItem[], options: { includeConfigJson?: boolean } - ): Promise<any> { + ): Promise<Template | null> {
♻️ Duplicate comments (3)
apps/api/src/caching/helpers.ts (1)
151-165: AvoidanyinreusePendingPromisesignature.The function uses
any[]in the generic constraint, which violates the coding guideline forbiddingany. Useunknown[]instead.🛠️ Suggested fix
-export function reusePendingPromise<T extends (...args: any[]) => Promise<unknown>>(fn: T, options?: { getKey?: (...args: Parameters<T>) => string }): T { +export function reusePendingPromise<T extends (...args: unknown[]) => Promise<unknown>>(fn: T, options?: { getKey?: (...args: Parameters<T>) => string }): T {apps/api/src/template/services/template-gallery/template-gallery.service.ts (2)
12-17: MakegithubPATrequired to match runtime behavior.The
Optionstype marksgithubPATas optional, butgetOctokitthrows when it's missing. This creates a type-safety gap where TypeScript won't catch missing PAT at compile time.🛠️ Suggested fix
type Options = { - githubPAT?: string; + githubPAT: string; dataFolderPath: string; categoryProcessingConcurrency?: number; templateSourceProcessingConcurrency?: number; };Also applies to: 167-170
125-134: Non-deterministic cache file selection.
files[0]fromglob()returns results in arbitrary order, so the "latest cached version" selection is not deterministic. Sort files before selecting, or rename the variable/log message to reflect it's "a cached version" rather than "the latest."🛠️ Suggested fix
const files = await Array.fromAsync(this.#fs.glob(`${cachePathPrefix}-*.json`)); + files.sort(); // Ensure deterministic selection this.#logger.debug({ event: "UNABLE_TO_FETCH_LATEST_TEMPLATES_COMMIT_SHA", - message: "Trying to use the latest cached version", + message: "Trying to use a cached version",
🧹 Nitpick comments (2)
apps/api/src/template/services/template-gallery/template-gallery.service.ts (1)
31-42: Inconsistent dependency injection: globalfetchused instead of injected dependency.The service accepts an injected
FileSystemApifor testability, but line 35 uses the globalfetchdirectly. Consider injectingfetchthrough the constructor for consistency and easier mocking in tests.🛠️ Suggested refactor
type Options = { githubPAT?: string; dataFolderPath: string; categoryProcessingConcurrency?: number; templateSourceProcessingConcurrency?: number; + fetch?: typeof globalThis.fetch; }; - constructor(logger: LoggerService, fs: FileSystemApi, options: Options) { + constructor(logger: LoggerService, fs: FileSystemApi, options: Options) { + const fetchFn = options.fetch ?? fetch; this.#logger = logger; this.#fs = fs; this.templateProcessor = new TemplateProcessorService(); - this.templateFetcher = new TemplateFetcherService(this.templateProcessor, this.#logger, fetch, getOctokit(options.githubPAT), { + this.templateFetcher = new TemplateFetcherService(this.templateProcessor, this.#logger, fetchFn, getOctokit(options.githubPAT), {apps/api/src/template/services/template-fetcher/template-fetcher.service.spec.ts (1)
604-663: Alignsetuphelper with test conventions (signature + placement).Guidelines require
setupto accept a single inline-typed parameter and to be the last function in the rootdescribe. Consider moving helper functions above or outside thedescribeblock and adding an optional overrides parameter. As per coding guidelines, ...♻️ Example adjustment (signature only)
- function setup() { + function setup({ fetchImpl }: { fetchImpl?: typeof globalThis.fetch } = {}) { const templateProcessor = mock<TemplateProcessorService>(); const logger = mock<LoggerService>(); - const fetchMock = jest.fn(); + const fetchMock = fetchImpl ?? jest.fn(); const octokit = mockDeep<Octokit>(); const service = new TemplateFetcherService(templateProcessor, logger, fetchMock as typeof globalThis.fetch, octokit); return { service, templateProcessor, logger, fetchMock, octokit }; }
stalniy
force-pushed
the
refactor/templates2
branch
from
70804f7 to
12e33bd
Compare
| this.getTemplateById = reusePendingPromise(this.getTemplateById.bind(this), { getKey: id => id }); | ||
| } | ||
|
|
||
| @Memoize({ ttlInSeconds: Infinity }) |
There was a problem hiding this comment.
question: how does that work?
There was a problem hiding this comment.
the intention was that templates will be read from fs and then cached forever ( in @Memoize there is a check which checks whether cache expired, there is no bigger value then Infinity, so this cache will never expire).
But I see a potential issue with this. If somebody updates templates, and our api restarts it will try to fetch them in runtime. So, this change with ttl is not enough to prevent runtime rebuild
There was a problem hiding this comment.
I'll merge this as is and will adjust the logic in a separate PR, just so it will be smaller
2 participants
Why
To improve observability for templates
What
Summary by CodeRabbit
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.