-
-
Notifications
You must be signed in to change notification settings - Fork 167
fix: add insert-after blank-line mode #1056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a configurable blank-line handling mode for the "Insert After" option in CaptureChoice: a new Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as ChoiceBuilder UI
participant Choice as CaptureChoice (model)
participant Formatter as CaptureChoiceFormatter
participant Inserter as Insert After Handler
User->>UI: Select mode (auto|skip|none)
UI->>Choice: Save insertAfter.blankLineAfterMatchMode
User->>Formatter: Trigger Insert After operation
Formatter->>Choice: Read insertAfter.blankLineAfterMatchMode
Formatter->>Formatter: isAtxHeading(matchLine)?
alt mode == "skip" or (mode == "auto" and heading)
Formatter->>Formatter: skip following blank lines (scan forward)
else
Formatter->>Formatter: do not skip blank lines (insert after match)
end
Formatter->>Inserter: Provide insertion index
Inserter->>Inserter: Insert content at computed position
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/formatters/captureChoiceFormatter.ts (1)
9-10: Consider combining type imports from the same module.Both imports are from the same module and could be combined for cleaner code.
🔎 Proposed refactor
-import type ICaptureChoice from "../types/choices/ICaptureChoice"; -import type { BlankLineAfterMatchMode } from "../types/choices/ICaptureChoice"; +import type ICaptureChoice, { BlankLineAfterMatchMode } from "../types/choices/ICaptureChoice";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/Choices/CaptureChoice.md(1 hunks)src/formatters/captureChoiceFormatter-frontmatter.test.ts(2 hunks)src/formatters/captureChoiceFormatter.ts(3 hunks)src/gui/ChoiceBuilder/captureChoiceBuilder.ts(2 hunks)src/types/choices/CaptureChoice.ts(4 hunks)src/types/choices/ICaptureChoice.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/types/choices/ICaptureChoice.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/types/choices/CaptureChoice.tssrc/formatters/captureChoiceFormatter-frontmatter.test.tssrc/formatters/captureChoiceFormatter.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/types/choices/ICaptureChoice.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/types/choices/CaptureChoice.tssrc/formatters/captureChoiceFormatter-frontmatter.test.tssrc/formatters/captureChoiceFormatter.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/types/choices/ICaptureChoice.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/types/choices/CaptureChoice.tssrc/formatters/captureChoiceFormatter-frontmatter.test.tssrc/formatters/captureChoiceFormatter.ts
docs/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep user-facing docs in
docs/directory
Files:
docs/docs/Choices/CaptureChoice.md
🧬 Code graph analysis (3)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts (1)
tests/obsidian-stub.ts (1)
Setting(151-233)
src/types/choices/CaptureChoice.ts (1)
src/types/choices/ICaptureChoice.ts (1)
BlankLineAfterMatchMode(5-5)
src/formatters/captureChoiceFormatter.ts (1)
src/types/choices/ICaptureChoice.ts (1)
BlankLineAfterMatchMode(5-5)
🔇 Additional comments (13)
src/types/choices/ICaptureChoice.ts (1)
5-6: LGTM!The new
BlankLineAfterMatchModetype and optionalblankLineAfterMatchModeproperty are well-designed. The optional property ensures backward compatibility with existing saved choices, and the type alias is properly exported for use across the codebase.Also applies to: 33-33
docs/docs/Choices/CaptureChoice.md (1)
60-75: LGTM!The documentation clearly explains the three blank line handling modes and provides a helpful example. The terminology ("Auto (headings only)", "Always skip", "Never skip") matches the UI labels, and the example accurately reflects the behavior tested in
captureChoiceFormatter-frontmatter.test.ts.src/gui/ChoiceBuilder/captureChoiceBuilder.ts (2)
475-504: LGTM!The blank line mode dropdown is well-implemented:
- Proper default initialization to "auto" when not set
- Correct disabling of the control when "Insert at end of section" is enabled
- Clear description that changes based on
insertAtEndstate- Type assertion on the value is safe given the constrained dropdown options
469-472: Good addition of reload for UI refresh.The
reload()call on toggle change ensures the blank line mode setting's disabled state updates correctly when toggling "Insert at end of section".src/formatters/captureChoiceFormatter-frontmatter.test.ts (2)
182-358: Excellent test coverage for the new blank line handling feature.The test suite comprehensively covers:
- All three modes (
auto,skip,none)- Edge cases: EOF, CRLF line endings, whitespace-only lines, multiple consecutive blank lines
- The distinction between heading and non-heading matches in auto mode
114-114: LGTM!The default choice factory correctly includes
blankLineAfterMatchMode: 'auto'to match the production default, ensuring tests reflect real-world behavior.src/types/choices/CaptureChoice.ts (3)
3-3: LGTM!Correct use of type-only import for
BlankLineAfterMatchMode, following TypeScript best practices as per coding guidelines.
25-25: LGTM!The optional property declaration and constructor default value are correctly implemented, ensuring new choices start with
"auto"as the default mode.Also applies to: 64-64
94-96: Good backward compatibility handling.The
Load()method properly migrates existing choices that lack the newblankLineAfterMatchModeproperty by defaulting to"auto". The guard checkingloaded.insertAfterbefore accessing the property is defensive and handles potential edge cases whereinsertAftermight be undefined.src/formatters/captureChoiceFormatter.ts (4)
202-209: LGTM! Clean mode-based logic.The three-way branch correctly implements the blank line handling modes: explicit skip, explicit none, and auto (heading-only).
211-213: LGTM! Regex correctly implements CommonMark ATX heading detection.The pattern accurately validates ATX headings per the CommonMark specification: 0-3 spaces of indentation, 1-6 hash marks, mandatory whitespace, and content.
215-243: LGTM! Robust blank-line scanning with proper EOF handling.The method correctly skips consecutive blank lines after the match, with careful handling of the trailing empty element from
split("\n")when files end with a newline. The defensive coding (nullish coalescing on line 223) and clear comment on lines 227-228 enhance maintainability.
278-287: LGTM! Proper integration of blank-line mode.The else block correctly applies the blank-line adjustment only when not using
insertAtEnd, with a sensible default of "auto". The flow properly adjuststargetPositionbefore the final insertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/Choices/CaptureChoice.md(1 hunks)src/formatters/captureChoiceFormatter-frontmatter.test.ts(9 hunks)src/formatters/captureChoiceFormatter.ts(3 hunks)src/gui/ChoiceBuilder/captureChoiceBuilder.ts(2 hunks)src/types/choices/CaptureChoice.ts(4 hunks)src/types/choices/ICaptureChoice.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/docs/Choices/CaptureChoice.md
- src/types/choices/ICaptureChoice.ts
- src/formatters/captureChoiceFormatter-frontmatter.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/formatters/captureChoiceFormatter.tssrc/types/choices/CaptureChoice.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/formatters/captureChoiceFormatter.tssrc/types/choices/CaptureChoice.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/formatters/captureChoiceFormatter.tssrc/types/choices/CaptureChoice.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.ts
🧬 Code graph analysis (3)
src/formatters/captureChoiceFormatter.ts (1)
src/types/choices/ICaptureChoice.ts (1)
BlankLineAfterMatchMode(5-5)
src/types/choices/CaptureChoice.ts (1)
src/types/choices/ICaptureChoice.ts (1)
BlankLineAfterMatchMode(5-5)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts (1)
tests/obsidian-stub.ts (1)
Setting(151-233)
🪛 GitHub Actions: Build With Lint
src/gui/ChoiceBuilder/captureChoiceBuilder.ts
[error] 526-526: Command failed: tsc -noEmit -skipLibCheck && bun lint && node esbuild.config.mjs production — TypeScript error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Build With Lint
src/gui/ChoiceBuilder/captureChoiceBuilder.ts
[failure] 526-526:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
🔇 Additional comments (4)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts (2)
500-503: Verify the reload behavior aligns with user expectations.Adding
this.reload()to theinsertAtEndtoggle ensures the blank-line mode setting updates its disabled state dynamically. Confirm this doesn't cause unexpected side effects, such as losing unsaved changes in other fields or disrupting the user's workflow.
506-535: LGTM: Blank line mode UI control is well-structured.The implementation correctly:
- Initializes the default value to
"auto"for backward compatibility- Adapts the description based on
insertAtEndstate- Disables the control when
insertAtEndis active (both the dropdown and the setting itself)Note: Lines 533 and 535 both disable UI elements when
insertAtEndis true. While this is redundant, it ensures consistent visual feedback and doesn't cause issues.src/types/choices/CaptureChoice.ts (1)
3-3: LGTM: Type definition and backward compatibility are well-handled.The implementation correctly:
- Uses a type-only import as per coding guidelines
- Declares
blankLineAfterMatchModeas optional for flexibility- Sets a sensible default (
"auto") in both the constructor and theLoadmethod- Ensures existing configurations without this field will default to
"auto"behaviorAlso applies to: 26-26, 65-65, 95-97
src/formatters/captureChoiceFormatter.ts (1)
216-223: LGTM: Blank line handling logic is sound.The implementation correctly:
- Distinguishes between three modes: always skip (
"skip"), never skip ("none"), and conditional skip ("auto"for ATX headings only)- Preserves existing EOF behavior by respecting the scan limit
- Defaults to
"auto"mode when not explicitly configured- Integrates cleanly into the existing insertion flow
The defensive nullish coalescing on Line 237 (
lines[matchIndex] ?? "") is unnecessary but harmless, asmatchIndex >= 0is already verified on Line 235.Also applies to: 229-257, 293-299
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts (2)
506-508: Initialization looks good; consider a more precise null check.The defensive initialization ensures
blankLineAfterMatchModehas a default value, which helps address the TypeScript type safety concern from the previous review. The current falsy check works fine since valid values are "auto", "skip", or "none".🔎 Optional: more precise null check
If you want to be more explicit about checking for undefined/null only:
-if (!this.choice.insertAfter?.blankLineAfterMatchMode) { +if (this.choice.insertAfter?.blankLineAfterMatchMode == null) { this.choice.insertAfter.blankLineAfterMatchMode = "auto"; }This prevents treating other falsy values (like empty string) as missing, though in practice the current code works correctly.
510-537: Consider importing the BlankLineAfterMatchMode type.The new blank line mode setting is well-structured and follows patterns from the rest of the file. The conditional description and disabled state logic are correct.
Based on the coding guidelines preference for type-only imports and the AI summary indicating that
BlankLineAfterMatchModeis defined in the types, consider importing it:🔎 Suggested type import
At the top of the file, add:
import type QuickAdd from "../../main"; import type ICaptureChoice from "../../types/choices/ICaptureChoice"; +import type { BlankLineAfterMatchMode } from "../../types/choices/ICaptureChoice"; import type { LinkPlacement, LinkType } from "../../types/linkPlacement";Then on lines 530-533, use the imported type:
.onChange((value) => { - this.choice.insertAfter.blankLineAfterMatchMode = value as - | "auto" - | "skip" - | "none"; + this.choice.insertAfter.blankLineAfterMatchMode = value as BlankLineAfterMatchMode; });This improves type safety and maintainability by reusing the canonical type definition.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/gui/ChoiceBuilder/captureChoiceBuilder.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/gui/ChoiceBuilder/captureChoiceBuilder.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/gui/ChoiceBuilder/captureChoiceBuilder.ts
🔇 Additional comments (3)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts (3)
441-444: LGTM! Description accurately reflects the new feature.The updated description text appropriately informs users that blank line handling is now configurable, which aligns with the new blank-line mode setting added below.
500-503: LGTM! The reload() call is necessary.Adding
this.reload()ensures the blank line mode setting properly updates its disabled state when the "Insert at end" toggle changes, maintaining correct UI behavior.
526-528: Past TypeScript error appears to be resolved.The
?? "auto"fallback ensuressetValuealways receives a string, addressing the compilation error flagged in the previous review. Combined with the initialization at lines 506-508, this properly handles the optionalblankLineAfterMatchModefield.
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.