refactor: extract formatter and template services#1205
Conversation
📝 WalkthroughWalkthroughThis PR refactors the formatter evaluation architecture to use dependency injection and factory patterns, extracting template and folder-selection logic into dedicated services while maintaining backward compatibility through optional evaluator parameters and default implementations. ChangesFormatter Evaluation Architecture Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Deploying quickadd with
|
| Latest commit: |
59ad913
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11ac5e34.quickadd.pages.dev |
| Branch Preview URL: | https://engine-flat-02-formatter-tem.quickadd.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c338da32b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { content } = await new TemplateEvaluator( | ||
| formatter, | ||
| ).evaluateTemplateContent(templateContent, templatePath); |
There was a problem hiding this comment.
Using TemplateEvaluator.evaluateTemplateContent(..., templatePath) here makes nested {{TEMPLATE:...}} expansion call setTitle() with the template file name, and because the nested formatter shares context.variables, that title value leaks back into the parent formatting run. In flows where no title was pre-set (for example API format() input that includes {{TEMPLATE:...}} followed by {{TITLE}} or {{VALUE:title}}), the output now incorrectly uses the template basename instead of leaving title unset/derived from the actual target file context.
Useful? React with 👍 / 👎.
chhoumann
force-pushed
the
engine-flat/02-formatter-template-services
branch
from
c338da3 to
59ad913
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/preflight/runOnePagePreflight.selection.test.ts (1)
187-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
modalComputePreviewin thisbeforeEachto avoid stale callback leakage.Line 187 setup does not clear
modalComputePreview, so Line 239 test can accidentally reuse a callback from a previous test and mask regressions.Suggested fix
describe("runOnePagePreflight template extension handling", () => { beforeEach(() => { modalOpenMock.mockClear(); modalResult = {}; + modalComputePreview = undefined; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preflight/runOnePagePreflight.selection.test.ts` around lines 187 - 190, The beforeEach currently only clears modalOpenMock and resets modalResult but fails to reset modalComputePreview, which can leak callbacks across tests; update the beforeEach in runOnePagePreflight.selection.test.ts to also clear or reset the modalComputePreview mock/variable (the same place modalOpenMock.mockClear() is called) so each test gets a fresh modalComputePreview and cannot reuse a previous callback.
🧹 Nitpick comments (1)
src/formatters/formatDisplayFormatter.ts (1)
159-161: 💤 Low valuePrefer explicit
TFilecast overnever.The duck-type check above confirms the object is a
TFile, but casting toneverobscures intent. Useas TFilefor clarity.Suggested fix
- return await vault.cachedRead(file as never); + return await vault.cachedRead(file as TFile);You'll also need to import
TFilefromobsidianat the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/formatters/formatDisplayFormatter.ts` around lines 159 - 161, The cast to never in the cachedRead call hides intent—change the cast to the explicit type confirmed by the duck-check (use file as TFile when calling vault.cachedRead inside the try block in formatDisplayFormatter) and add an import for TFile from 'obsidian' at the top of the file so the type is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/TemplateEngine.ts`:
- Around line 89-98: The change pre-reads template content via
getTemplateContent and then calls
createFileWithTemplateContent/overwriteFileWithTemplateContent/appendToFileWithTemplateContent,
which bypasses TemplateFileService's internal try/catch + reportError behavior;
instead, call the template-path variants on templateFileService
(templateService.createFileWithTemplate, overwriteFileWithTemplate,
appendToFileWithTemplate) passing the templatePath and a new
TemplateEvaluator(this.formatter) so errors are handled consistently; update
createFileWithTemplate, overwriteFileWithTemplate, and appendToFileWithTemplate
methods to stop using getTemplateContent and use the corresponding templatePath
methods on templateFileService.
In `@src/services/FolderSelectionService.ts`:
- Around line 107-112: The current shouldPromptForFolder implementation hides
the prompt when allowCreate is true and there is exactly one suggested folder;
update shouldPromptForFolder to keep the prompt reachable by treating one or
zero items the same when creation is allowed. In practice, change the condition
in shouldPromptForFolder (in FolderSelectionService) so it returns true if
context.items.length > 1 OR (context.allowCreate && context.items.length <= 1),
ensuring the prompt appears for the single-suggestion case when allowCreate is
enabled.
- Around line 159-163: The current existence check in ensureFolderExists uses
this.app.vault.adapter.exists(resolved) which returns true for files and
folders; update ensureFolderExists to call
this.app.vault.getAbstractFileByPath(resolved), test whether the returned
abstract file is an instance of TFolder (not TFile), and set exists only when
the abstract file is non-null and instanceof TFolder; replace the
canonical/adapter.exists logic with this new check so callers never receive a
file path when a folder is expected.
---
Outside diff comments:
In `@src/preflight/runOnePagePreflight.selection.test.ts`:
- Around line 187-190: The beforeEach currently only clears modalOpenMock and
resets modalResult but fails to reset modalComputePreview, which can leak
callbacks across tests; update the beforeEach in
runOnePagePreflight.selection.test.ts to also clear or reset the
modalComputePreview mock/variable (the same place modalOpenMock.mockClear() is
called) so each test gets a fresh modalComputePreview and cannot reuse a
previous callback.
---
Nitpick comments:
In `@src/formatters/formatDisplayFormatter.ts`:
- Around line 159-161: The cast to never in the cachedRead call hides
intent—change the cast to the explicit type confirmed by the duck-check (use
file as TFile when calling vault.cachedRead inside the try block in
formatDisplayFormatter) and add an import for TFile from 'obsidian' at the top
of the file so the type is available.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8cb08f4-246f-4fdd-a4bf-7b6616307230
📒 Files selected for processing (25)
src/IChoiceExecutor.tssrc/choiceExecutor.tssrc/engine/CaptureChoiceEngine.tssrc/engine/MacroChoiceEngine.tssrc/engine/SingleInlineScriptEngine.tssrc/engine/TemplateEngine.tssrc/formatters/captureChoiceFormatter-frontmatter.test.tssrc/formatters/captureChoiceFormatter-selection.test.tssrc/formatters/captureChoiceFormatter-write-position.test.tssrc/formatters/completeFormatter.evaluator-boundaries.test.tssrc/formatters/completeFormatter.tssrc/formatters/formatDisplayFormatter.evaluator-boundaries.test.tssrc/formatters/formatDisplayFormatter.tssrc/formatters/formatterEvaluators.tssrc/preflight/collectChoiceRequirements.runtime-safety.test.tssrc/preflight/collectChoiceRequirements.tssrc/preflight/runOnePagePreflight.selection.test.tssrc/preflight/runOnePagePreflight.tssrc/quickAddApi.tssrc/services/FolderSelectionService.tssrc/services/FormatterFactory.test.tssrc/services/FormatterFactory.tssrc/services/TemplateEvaluator.test.tssrc/services/TemplateFileService.test.tssrc/services/TemplateFileService.ts
💤 Files with no reviewable changes (2)
- src/formatters/captureChoiceFormatter-write-position.test.ts
- src/formatters/captureChoiceFormatter-frontmatter.test.ts
| protected async createFileWithTemplate( | ||
| filePath: string, | ||
| templatePath: string | ||
| ) { | ||
| try { | ||
| const templateContent: string = await this.getTemplateContent( | ||
| templatePath | ||
| ); | ||
|
|
||
| // Extract filename without extension from the full path. | ||
| const fileBasename = basenameWithoutMdOrCanvas(filePath); | ||
| this.formatter.setTitle(fileBasename); | ||
|
|
||
| const formattedTemplateContent: string = | ||
| await this.formatter.withTemplatePropertyCollection(() => | ||
| this.formatter.formatFileContent(templateContent), | ||
| ); | ||
|
|
||
| // Get template variables before creating the file | ||
| const templateVars = this.formatter.getAndClearTemplatePropertyVars(); | ||
|
|
||
| log.logMessage(`TemplateEngine.createFileWithTemplate: Collected ${templateVars.size} template property variables for ${filePath}`); | ||
| if (templateVars.size > 0) { | ||
| log.logMessage(`Variables: ${Array.from(templateVars.keys()).join(', ')}`); | ||
| } | ||
|
|
||
| const suppressTemplaterOnCreate = filePath | ||
| .toLowerCase() | ||
| .endsWith(".md"); | ||
| const createdFile: TFile = await this.vaultFileService.createFileWithInput( | ||
| filePath, | ||
| formattedTemplateContent, | ||
| { suppressTemplaterOnCreate }, | ||
| ); | ||
|
|
||
| // Post-process front matter for template property types BEFORE Templater | ||
| if (this.frontmatterPropertyService.shouldPostProcessFrontMatter(createdFile, templateVars)) { | ||
| await this.frontmatterPropertyService.postProcessFrontMatter(createdFile, templateVars); | ||
| } | ||
|
|
||
| // Process Templater commands for template choices | ||
| await overwriteTemplaterOnce(this.app, createdFile); | ||
|
|
||
| return createdFile; | ||
| } catch (err) { | ||
| if (isMacroAbortError(err)) { | ||
| throw err; | ||
| } | ||
| reportError(err, `Could not create file with template at ${filePath}`); | ||
| return null; | ||
| } | ||
| const templateContent = await this.getTemplateContent(templatePath); | ||
| return await this.templateFileService.createFileWithTemplateContent( | ||
| filePath, | ||
| templateContent, | ||
| new TemplateEvaluator(this.formatter), | ||
| ); |
There was a problem hiding this comment.
Delegate to the templatePath service methods instead of pre-reading the template.
Line 93, Line 111, and Line 124 fetch template content up front and then call the *WithTemplateContent variants. That bypasses the try/catch + reportError path already built into TemplateFileService.createFileWithTemplate(...), overwriteFileWithTemplate(...), and appendToFileWithTemplate(...), so missing/unreadable templates now escape from the engine instead of returning null consistently.
Suggested fix
protected async createFileWithTemplate(
filePath: string,
templatePath: string
) {
- const templateContent = await this.getTemplateContent(templatePath);
- return await this.templateFileService.createFileWithTemplateContent(
+ return await this.templateFileService.createFileWithTemplate(
filePath,
- templateContent,
+ templatePath,
new TemplateEvaluator(this.formatter),
);
}
@@
protected async overwriteFileWithTemplate(
file: TFile,
templatePath: string
) {
- const templateContent = await this.getTemplateContent(templatePath);
- return await this.templateFileService.overwriteFileWithTemplateContent(
+ return await this.templateFileService.overwriteFileWithTemplate(
file,
- templateContent,
+ templatePath,
new TemplateEvaluator(this.formatter),
);
}
@@
protected async appendToFileWithTemplate(
file: TFile,
templatePath: string,
section: "top" | "bottom"
) {
- const templateContent = await this.getTemplateContent(templatePath);
- return await this.templateFileService.appendToFileWithTemplateContent(
+ return await this.templateFileService.appendToFileWithTemplate(
file,
- templateContent,
+ templatePath,
section,
new TemplateEvaluator(this.formatter),
);
}Also applies to: 107-116, 119-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engine/TemplateEngine.ts` around lines 89 - 98, The change pre-reads
template content via getTemplateContent and then calls
createFileWithTemplateContent/overwriteFileWithTemplateContent/appendToFileWithTemplateContent,
which bypasses TemplateFileService's internal try/catch + reportError behavior;
instead, call the template-path variants on templateFileService
(templateService.createFileWithTemplate, overwriteFileWithTemplate,
appendToFileWithTemplate) passing the templatePath and a new
TemplateEvaluator(this.formatter) so errors are handled consistently; update
createFileWithTemplate, overwriteFileWithTemplate, and appendToFileWithTemplate
methods to stop using getTemplateContent and use the corresponding templatePath
methods on templateFileService.
| private shouldPromptForFolder(context: FolderSelectionContext): boolean { | ||
| return ( | ||
| context.items.length > 1 || | ||
| (context.allowCreate && context.items.length === 0) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Keep the prompt reachable when folder creation is enabled.
With allowCreate === true and exactly one suggested folder, this returns false and auto-selects the existing folder. That makes the “type to create one” path unreachable in the common single-suggestion case.
Suggested fix
private shouldPromptForFolder(context: FolderSelectionContext): boolean {
- return (
- context.items.length > 1 ||
- (context.allowCreate && context.items.length === 0)
- );
+ return context.allowCreate || context.items.length > 1;
}📝 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.
| private shouldPromptForFolder(context: FolderSelectionContext): boolean { | |
| return ( | |
| context.items.length > 1 || | |
| (context.allowCreate && context.items.length === 0) | |
| ); | |
| } | |
| private shouldPromptForFolder(context: FolderSelectionContext): boolean { | |
| return context.allowCreate || context.items.length > 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/FolderSelectionService.ts` around lines 107 - 112, The current
shouldPromptForFolder implementation hides the prompt when allowCreate is true
and there is exactly one suggested folder; update shouldPromptForFolder to keep
the prompt reachable by treating one or zero items the same when creation is
allowed. In practice, change the condition in shouldPromptForFolder (in
FolderSelectionService) so it returns true if context.items.length > 1 OR
(context.allowCreate && context.items.length <= 1), ensuring the prompt appears
for the single-suggestion case when allowCreate is enabled.
| const exists = isEmpty | ||
| ? false | ||
| : canonical !== undefined || | ||
| (await this.app.vault.adapter.exists(resolved)); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Obsidian plugin API 1.11.4, does app.vault.adapter.exists(path)return true for both files and folders, and what API should plugin code use to distinguishTFolderfromTFile for a path?
💡 Result:
In the Obsidian plugin API, the app.vault.adapter.exists(path) method returns true if the specified path exists in the vault, regardless of whether it is a file or a folder [1][2][3]. It is a general-purpose check for the existence of an entry at that location [1][2]. To distinguish between a TFile and a TFolder for a given path, you should use the app.vault.getAbstractFileByPath(path) method [4][5]. This method returns an instance of TAbstractFile (or null if the path does not exist), which you can then check using the instanceof operator [4][5]: const abstractFile = app.vault.getAbstractFileByPath(path); if (abstractFile instanceof TFile) { // The path is a file } else if (abstractFile instanceof TFolder) { // The path is a folder } Using app.vault.getAbstractFileByPath is the recommended approach for this purpose, as it provides access to the Obsidian-managed file system objects directly [1][3][4]. While DataAdapter methods like exists are available for low-level file system operations, the Vault API is preferred for interacting with files and folders within the Obsidian vault [3].
Citations:
- 1: https://fevol.github.io/obsidian-typings/api/obsidian-typings/namespaces/obsidian/interfaces/dataadapter/
- 2: https://docs.obsidian.md/Reference/TypeScript+API/DataAdapter/exists
- 3: https://obsidian-developer-docs.pages.dev/Reference/TypeScript-API/DataAdapter
- 4: https://obsidian-developer-docs.pages.dev/Reference/TypeScript-API/Vault/getAbstractFileByPath
- 5: https://mintlify.com/obsidianmd/obsidian-api/api/vault
🏁 Script executed:
cd src/services && wc -l FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 92
🏁 Script executed:
cd src/services && sed -n '145,175p' FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 760
🏁 Script executed:
cd src && rg "FolderSelection" --type ts -A 3 -B 1 | head -80Repository: chhoumann/quickadd
Length of output: 4595
🏁 Script executed:
cd src && rg "resolveSelection" --type ts -A 5Repository: chhoumann/quickadd
Length of output: 1357
🏁 Script executed:
cd src/services && sed -n '115,135p' FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 502
🏁 Script executed:
cd src/services && sed -n '176,210p' FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 853
🏁 Script executed:
cd src/services && sed -n '78,105p' FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 686
🏁 Script executed:
cd src && rg "selectFolder" --type ts -A 3 -B 1 | head -60Repository: chhoumann/quickadd
Length of output: 44
🏁 Script executed:
cd src && rg "selectFolder\(" --type ts -A 5 -B 2Repository: chhoumann/quickadd
Length of output: 44
🏁 Script executed:
cd src/services && sed -n '60,77p' FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 593
🏁 Script executed:
cd src && rg "selectFolder" --type ts | head -20Repository: chhoumann/quickadd
Length of output: 44
🏁 Script executed:
cd src && grep -r "\.selectFolder\(" --include="*.ts" -A 3Repository: chhoumann/quickadd
Length of output: 86
🏁 Script executed:
cd src && rg "selectFolder" --type tsRepository: chhoumann/quickadd
Length of output: 44
🏁 Script executed:
cd src/services && sed -n '1,60p' FolderSelectionService.tsRepository: chhoumann/quickadd
Length of output: 1710
🏁 Script executed:
cd src && rg "getOrCreateFolder" --type ts -B 2 -A 5 | head -80Repository: chhoumann/quickadd
Length of output: 2565
🏁 Script executed:
cd src && rg "getOrCreateFolder" --type ts | grep -v "services/FolderSelectionService.ts" | grep "engine"Repository: chhoumann/quickadd
Length of output: 563
🏁 Script executed:
cd src/engine && rg "getOrCreateFolder" -A 10 | head -40Repository: chhoumann/quickadd
Length of output: 1873
Use getAbstractFileByPath() with instanceof to distinguish files from folders.
The adapter.exists(resolved) method returns true for both files and folders, so if a user provides a path to an existing file, ensureFolderExists() will skip folder creation and return the file path. Use app.vault.getAbstractFileByPath(path) instead to check whether the path points to a TFile or TFolder:
const abstractFile = app.vault.getAbstractFileByPath(resolved);
const isFolder = abstractFile instanceof TFolder;
const exists = abstractFile !== null && isFolder;This prevents non-folder paths from being returned to callers expecting a valid folder path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/FolderSelectionService.ts` around lines 159 - 163, The current
existence check in ensureFolderExists uses
this.app.vault.adapter.exists(resolved) which returns true for files and
folders; update ensureFolderExists to call
this.app.vault.getAbstractFileByPath(resolved), test whether the returned
abstract file is an instance of TFolder (not TFile), and set exists only when
the abstract file is non-null and instanceof TFolder; replace the
canonical/adapter.exists logic with this new check so callers never receive a
file path when a folder is expected.
|
Closing this stack after deciding not to continue with this refactor direction. |
1 participant
Summary
Stack
engine-flat/01-vault-frontmatter-servicesengine-flat/03-template-choiceengine-flat/04-capture-choiceengine-flat/05-macro-cutoverValidation
bun run lintbun run testbun run buildSummary by CodeRabbit
Refactoring
Tests