Skip to content

refactor: extract vault and frontmatter services#1204

Open
chhoumann wants to merge 1 commit into
masterfrom
engine-flat/01-vault-frontmatter-services
Open

refactor: extract vault and frontmatter services#1204
chhoumann wants to merge 1 commit into
masterfrom
engine-flat/01-vault-frontmatter-services

Conversation

@chhoumann
Copy link
Copy Markdown
Owner

@chhoumann chhoumann commented May 15, 2026

Summary

  • Extracts vault file/path/folder operations out of the engine layer into VaultFileService.
  • Extracts structured variable and frontmatter property handling into FrontmatterPropertyService.
  • Preserves existing template/capture behavior while making file/frontmatter dependencies explicit.

Stack

  1. This PR
  2. engine-flat/02-formatter-template-services
  3. engine-flat/03-template-choice
  4. engine-flat/04-capture-choice
  5. engine-flat/05-macro-cutover

Validation

  • bun run lint
  • bun run test
  • bun run build
  • Obsidian dev vault plugin reload and e2e validation

Summary by CodeRabbit

Release Notes

  • Refactor

    • Improved file operation reliability with enhanced path normalization and folder handling.
    • Strengthened frontmatter variable validation with clearer error reporting.
    • Added support for nested frontmatter paths and @date: prefix coercion for automatic date value handling.
  • Tests

    • Added comprehensive test coverage for file service operations and frontmatter processing.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

📝 Walkthrough

Walkthrough

This PR extracts file/vault operations and frontmatter post-processing logic from QuickAddEngine (and related engine classes) into two new reusable services: VaultFileService handles path normalization, file/folder existence and creation; FrontmatterPropertyService handles structured variable validation and nested frontmatter assignment. All engine classes are refactored to inject and delegate through these services instead of local helpers.

Changes

Service Extraction and Engine Refactoring

Layer / File(s) Summary
VaultFileService implementation and tests
src/services/VaultFileService.ts, src/services/VaultFileService.test.ts
New service provides path normalization (stripLeadingSlash, normalizeMarkdownFilePath), vault operations (fileExists, getFileByPath, createFolder), and file creation with optional Templater suppression. Tests verify all operations including idempotent folder creation and Templater suppression behavior for markdown files only.
FrontmatterPropertyService implementation and tests
src/services/FrontmatterPropertyService.ts, src/services/FrontmatterPropertyService.test.ts
New service validates structured template variables (detects functions, symbols, circular references, depth limits; warns on BigInt), coerces @date: values to Date objects, and assigns nested frontmatter paths using app.fileManager.processFrontMatter. Tests verify validation rules, nested object creation, date coercion, and error/rejection handling.
QuickAddEngine simplification to service-based architecture
src/engine/QuickAddEngine.ts, src/engine/QuickAddEngine.test.ts, src/engine/QuickAddEngine.validation.test.ts
QuickAddEngine reduced from 300+ lines to minimal abstract base with injected vaultFileService and frontmatterPropertyService. Removed: validateStructuredVariables, validateValue, createFolder, stripLeadingSlash, normalizeMarkdownFilePath, fileExists, getFileByPath, createFileWithInput, shouldPostProcessFrontMatter, postProcessFrontMatter, assignFrontmatterValue. Tests refactored to directly test VaultFileService.normalizeMarkdownFilePath and FrontmatterPropertyService.validateStructuredVariables instead of engine helpers.
TemplateEngine routing through new services
src/engine/TemplateEngine.ts
Folder creation, path normalization, and file creation now route through vaultFileService; frontmatter post-processing delegated to frontmatterPropertyService instead of local methods.
TemplateChoiceEngine path normalization refactoring
src/engine/TemplateChoiceEngine.ts
stripDuplicateFolderPrefix and shouldTreatFormattedNameAsVaultRelativePath now use vaultFileService.stripLeadingSlash for path normalization.
CaptureChoiceEngine routing through new services
src/engine/CaptureChoiceEngine.ts, src/engine/CaptureChoiceEngine.selection.test.ts
File existence, path normalization, file lookup, and file creation now route through vaultFileService; frontmatter post-processing delegated to frontmatterPropertyService. All six test cases in selection.test.ts updated to mock vaultFileService methods instead of direct engine methods.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • chhoumann/quickadd#1118: Both PRs refactor TemplateChoiceEngine's destination-path normalization and vault-relative-path detection using vaultFileService.stripLeadingSlash.
  • chhoumann/quickadd#1017: Both PRs refactor CaptureChoiceEngine's file-creation flow through createFileWithInput with suppressTemplaterOnCreate options.
  • chhoumann/quickadd#1124: Both PRs modify CaptureChoiceEngine capture execution flow, target path resolution, and file existence/creation handling.

Suggested labels

released

Poem

🐰 A rabbit's refactor tale
Services sprout where methods sprawled before,
Two helpers born to lighten engine's door—
VaultFile and Frontmatter, clean and bright,
Extracting chaos into ordered light.
Now ChoiceEngines skip with softer feet,
Delegation makes the symphony complete! 🎼

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main refactoring work: extracting vault and frontmatter services into separate, reusable components from the engine layer.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch engine-flat/01-vault-frontmatter-services

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying quickadd with  Cloudflare Pages  Cloudflare Pages

Latest commit: afe98b4
Status: ✅  Deploy successful!
Preview URL: https://553bcec4.quickadd.pages.dev
Branch Preview URL: https://engine-flat-01-vault-frontma.quickadd.pages.dev

View logs

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/CaptureChoiceEngine.ts`:
- Around line 634-635: The local variable `file` is annotated as non-nullable
TFile but `vaultFileService.getFileByPath` can return null/undefined (you
immediately check `if (!file)`), so update the declaration to reflect the
nullable return: change the `file` type to `TFile | null` (or `TFile |
undefined`) or remove the explicit type and let TypeScript infer it from
`this.vaultFileService.getFileByPath`; ensure references to `file` (and the null
check) remain valid in the `CaptureChoiceEngine` method.
🪄 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: 2102e3df-7901-47db-b831-33376a6f2111

📥 Commits

Reviewing files that changed from the base of the PR and between 9592b84 and afe98b4.

📒 Files selected for processing (11)
  • src/engine/CaptureChoiceEngine.selection.test.ts
  • src/engine/CaptureChoiceEngine.ts
  • src/engine/QuickAddEngine.test.ts
  • src/engine/QuickAddEngine.ts
  • src/engine/QuickAddEngine.validation.test.ts
  • src/engine/TemplateChoiceEngine.ts
  • src/engine/TemplateEngine.ts
  • src/services/FrontmatterPropertyService.test.ts
  • src/services/FrontmatterPropertyService.ts
  • src/services/VaultFileService.test.ts
  • src/services/VaultFileService.ts

Comment on lines +634 to 635
const file: TFile = this.vaultFileService.getFileByPath(filePath);
if (!file) throw new Error("File not found");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix type annotation to match nullable return type.

The type annotation declares file as non-nullable TFile, but line 635 immediately checks if (!file), indicating the method can return null or undefined. The type should be TFile | null or omit the explicit annotation to let TypeScript infer it.

🔧 Proposed fix
-		const file: TFile = this.vaultFileService.getFileByPath(filePath);
+		const file = this.vaultFileService.getFileByPath(filePath);
 		if (!file) throw new Error("File not found");
📝 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.

Suggested change
const file: TFile = this.vaultFileService.getFileByPath(filePath);
if (!file) throw new Error("File not found");
const file = this.vaultFileService.getFileByPath(filePath);
if (!file) throw new Error("File not found");
🤖 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/CaptureChoiceEngine.ts` around lines 634 - 635, The local variable
`file` is annotated as non-nullable TFile but `vaultFileService.getFileByPath`
can return null/undefined (you immediately check `if (!file)`), so update the
declaration to reflect the nullable return: change the `file` type to `TFile |
null` (or `TFile | undefined`) or remove the explicit type and let TypeScript
infer it from `this.vaultFileService.getFileByPath`; ensure references to `file`
(and the null check) remain valid in the `CaptureChoiceEngine` method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant