-
-
Notifications
You must be signed in to change notification settings - Fork 169
feat: add capture selection-as-value controls #1055
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.
|
WalkthroughA feature enabling editor selection as the default value for Capture choice {{VALUE}} placeholders is introduced. This includes a global setting with per-choice override capability, configuration resolution in CaptureChoiceEngine, selection-handling logic in CaptureChoiceFormatter, integration into the preflight phase, and comprehensive documentation and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor
participant CaptureEngine as CaptureChoiceEngine
participant Formatter as CaptureChoiceFormatter
participant Preflight as runOnePagePreflight
participant Modal as OnePageInputModal
participant Capture as Obsidian Capture
User->>Editor: Make text selection
User->>CaptureEngine: Trigger capture choice
rect rgb(200, 220, 240)
note over CaptureEngine: Resolve Configuration
CaptureEngine->>CaptureEngine: Read per-choice override<br/>(useSelectionAsCaptureValue)
CaptureEngine->>CaptureEngine: Fallback to global setting
CaptureEngine->>Formatter: setUseSelectionAsCaptureValue(flag)
end
rect rgb(220, 240, 200)
note over Preflight: Preflight Phase
Preflight->>Editor: getEditorSelection()
Editor-->>Preflight: Selected text
alt Selection exists & feature enabled
Preflight->>Preflight: Pre-fill {{VALUE}}<br/>in choiceExecutor
Preflight-->>User: Skip modal, proceed
else No pre-fill
Preflight-->>User: Return, trigger modal
end
end
rect rgb(240, 220, 200)
note over Formatter: Value Resolution
alt Should use selection & available
Formatter->>Editor: getSelectedTextForValue()
Editor-->>Formatter: Trimmed selection or empty
Formatter->>Formatter: Use selection for {{VALUE}}
else Fallback to prompt
Formatter->>Modal: Show input prompt
Modal-->>Formatter: User input
Formatter->>Formatter: Use prompt result for {{VALUE}}
end
end
Capture->>Capture: Format with resolved value
Capture-->>User: Capture complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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/gui/ChoiceBuilder/captureChoiceBuilder.ts (1)
95-123: LGTM! Tri-state dropdown correctly implements per-choice override.The implementation properly handles three states:
- Empty string →
undefined(follow global setting)- "enabled" →
true(force on for this choice)- "disabled" →
false(force off for this choice)The mapping logic in lines 108-114 is correct, though the nested ternary is somewhat dense.
Optional: Slightly more readable mapping
- dropdown.setValue( - typeof override === "boolean" - ? override - ? "enabled" - : "disabled" - : "", - ); + let dropdownValue = ""; + if (typeof override === "boolean") { + dropdownValue = override ? "enabled" : "disabled"; + } + dropdown.setValue(dropdownValue);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/docs/Advanced/onePageInputs.md(1 hunks)docs/docs/Choices/CaptureChoice.md(2 hunks)docs/docs/FormatSyntax.md(1 hunks)src/engine/CaptureChoiceEngine.notice.test.ts(2 hunks)src/engine/CaptureChoiceEngine.selection.test.ts(1 hunks)src/engine/CaptureChoiceEngine.template-property-types.test.ts(1 hunks)src/engine/CaptureChoiceEngine.ts(1 hunks)src/engine/MacroChoiceEngine.notice.test.ts(1 hunks)src/engine/TemplateChoiceEngine.notice.test.ts(1 hunks)src/formatters/captureChoiceFormatter-selection.test.ts(1 hunks)src/formatters/captureChoiceFormatter.ts(2 hunks)src/formatters/completeFormatter.ts(1 hunks)src/gui/ChoiceBuilder/captureChoiceBuilder.ts(2 hunks)src/preflight/runOnePagePreflight.selection.test.ts(1 hunks)src/preflight/runOnePagePreflight.ts(3 hunks)src/quickAddSettingsTab.ts(2 hunks)src/settings.ts(2 hunks)src/types/choices/CaptureChoice.ts(1 hunks)src/types/choices/ICaptureChoice.ts(1 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/CaptureChoice.tssrc/engine/MacroChoiceEngine.notice.test.tssrc/engine/CaptureChoiceEngine.template-property-types.test.tssrc/engine/CaptureChoiceEngine.notice.test.tssrc/settings.tssrc/formatters/captureChoiceFormatter-selection.test.tssrc/preflight/runOnePagePreflight.tssrc/preflight/runOnePagePreflight.selection.test.tssrc/engine/CaptureChoiceEngine.selection.test.tssrc/types/choices/ICaptureChoice.tssrc/formatters/completeFormatter.tssrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateChoiceEngine.notice.test.tssrc/quickAddSettingsTab.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.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/CaptureChoice.tssrc/engine/MacroChoiceEngine.notice.test.tssrc/engine/CaptureChoiceEngine.template-property-types.test.tssrc/engine/CaptureChoiceEngine.notice.test.tssrc/settings.tssrc/formatters/captureChoiceFormatter-selection.test.tssrc/preflight/runOnePagePreflight.tssrc/preflight/runOnePagePreflight.selection.test.tssrc/engine/CaptureChoiceEngine.selection.test.tssrc/types/choices/ICaptureChoice.tssrc/formatters/completeFormatter.tssrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateChoiceEngine.notice.test.tssrc/quickAddSettingsTab.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/formatters/captureChoiceFormatter.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/types/choices/CaptureChoice.tssrc/engine/MacroChoiceEngine.notice.test.tssrc/engine/CaptureChoiceEngine.template-property-types.test.tssrc/engine/CaptureChoiceEngine.notice.test.tssrc/settings.tssrc/formatters/captureChoiceFormatter-selection.test.tssrc/preflight/runOnePagePreflight.tssrc/preflight/runOnePagePreflight.selection.test.tssrc/engine/CaptureChoiceEngine.selection.test.tssrc/types/choices/ICaptureChoice.tssrc/formatters/completeFormatter.tssrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateChoiceEngine.notice.test.tssrc/quickAddSettingsTab.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/formatters/captureChoiceFormatter.ts
docs/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep user-facing docs in
docs/directory
Files:
docs/docs/FormatSyntax.mddocs/docs/Advanced/onePageInputs.mddocs/docs/Choices/CaptureChoice.md
🧠 Learnings (2)
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to tests/**/*.{ts,tsx} : Add regression coverage for bug fixes
Applied to files:
src/formatters/captureChoiceFormatter-selection.test.tssrc/preflight/runOnePagePreflight.selection.test.tssrc/engine/CaptureChoiceEngine.selection.test.tssrc/engine/TemplateChoiceEngine.notice.test.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or `tests/obsidian-stub.ts`
Applied to files:
src/formatters/captureChoiceFormatter-selection.test.tssrc/preflight/runOnePagePreflight.selection.test.ts
🧬 Code graph analysis (6)
src/preflight/runOnePagePreflight.ts (2)
tests/obsidian-stub.ts (2)
App(269-297)MarkdownView(346-353)src/types/choices/ICaptureChoice.ts (1)
ICaptureChoice(5-51)
src/preflight/runOnePagePreflight.selection.test.ts (3)
src/types/choices/ICaptureChoice.ts (1)
ICaptureChoice(5-51)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/preflight/runOnePagePreflight.ts (1)
runOnePagePreflight(144-291)
src/engine/CaptureChoiceEngine.selection.test.ts (2)
src/types/choices/ICaptureChoice.ts (1)
ICaptureChoice(5-51)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)
src/formatters/completeFormatter.ts (3)
src/gui/InputPrompt.ts (1)
InputPrompt(5-13)src/utils/errorUtils.ts (1)
isCancellationError(69-80)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/quickAddSettingsTab.ts (1)
src/settingsStore.ts (1)
settingsStore(8-21)
src/gui/ChoiceBuilder/captureChoiceBuilder.ts (1)
tests/obsidian-stub.ts (1)
Setting(151-233)
🔇 Additional comments (22)
src/engine/MacroChoiceEngine.notice.test.ts (1)
9-9: LGTM! Settings mock properly updated.The mock now includes the new
useSelectionAsCaptureValuesetting, preventing test failures and aligning with the default value defined insettings.ts.src/engine/CaptureChoiceEngine.notice.test.ts (2)
9-9: LGTM! Settings mock updated correctly.The addition of
useSelectionAsCaptureValue: truealigns the test scaffolding with the updated settings interface.
51-51: LGTM! Formatter mock extended appropriately.The new
setUseSelectionAsCaptureValue()method matches the API thatCaptureChoiceEnginenow calls. The no-op implementation is suitable for these notice-focused tests.src/engine/CaptureChoiceEngine.template-property-types.test.ts (1)
14-14: LGTM! Mock settings updated consistently.The addition of
useSelectionAsCaptureValue: truemaintains consistency with the updated settings interface and other test files.docs/docs/FormatSyntax.md (1)
11-11: LGTM! Documentation clearly describes the new feature.The updated
{{VALUE}}documentation accurately explains the selection-as-value behavior and mentions both global and per-capture controls.src/engine/CaptureChoiceEngine.ts (1)
107-114: LGTM! Setting resolution logic is correct.The precedence chain (per-choice override → global setting → default true) is implemented correctly, and the nullish coalescing operator provides a sensible fallback for backward compatibility.
However, please verify the migration path: the PR summary mentions renaming from
useSelectionAsValuetouseSelectionAsCaptureValuewithout automatic migration. If users had previously set the old key tofalse, their preference would be lost after the update, defaulting totrue. Consider adding migration logic or confirming this is intentional.docs/docs/Advanced/onePageInputs.md (1)
35-35: LGTM! Documentation accurately describes preflight behavior.The note correctly explains that non-empty editor selections prefill
{{VALUE}}during preflight when the selection-as-value feature is enabled.src/types/choices/ICaptureChoice.ts (1)
15-19: LGTM! Well-documented interface extension.The optional property allows per-choice override of the selection-as-value behavior, and the documentation clearly explains that
undefinedmeans the global setting will be used.src/settings.ts (1)
10-13: LGTM! Settings interface and default value properly defined.The property is well-documented, and the default value of
trueenables the feature for all users.Note: The default value of
truechanges existing behavior—editor selection will now be used as the capture value by default. Please confirm this is intentional and consider mentioning it in release notes, as it may be surprising to existing users who expect the previous behavior.Also applies to: 58-58
src/quickAddSettingsTab.ts (1)
376-391: LGTM! Clean integration of the new setting.The new setting is properly wired into the settings store and follows the established patterns in the file. The UI text clearly explains the behavior.
src/engine/TemplateChoiceEngine.notice.test.ts (1)
9-9: LGTM! Test mock properly updated.The mock settings correctly include the new
useSelectionAsCaptureValuefield with a sensible default.src/types/choices/CaptureChoice.ts (1)
17-17: LGTM! Type extension aligns with the feature design.The optional property correctly enables per-choice override of the global selection-as-value behavior. Leaving it undefined in the constructor properly signals "follow global setting."
docs/docs/Choices/CaptureChoice.md (1)
45-45: LGTM! Documentation clearly explains the new feature.The documentation properly describes the global/per-choice override behavior and clarifies that this setting affects only
{{VALUE}}, not{{SELECTED}}.Also applies to: 123-123
src/preflight/runOnePagePreflight.ts (2)
138-142: LGTM! Clean helper function.The
getEditorSelectionhelper properly retrieves the current selection from the active MarkdownView, with appropriate null-safety.
168-184: LGTM! Selection-as-value preflight logic is correct.The implementation properly:
- Resolves per-choice override vs. global setting (with sensible default of
true)- Only prefills when no existing value is present
- Treats whitespace-only selections as empty (via
.trim())The whitespace handling aligns with test expectations in
captureChoiceFormatter-selection.test.ts.src/formatters/captureChoiceFormatter-selection.test.ts (1)
63-98: LGTM! Comprehensive test coverage for selection-as-value behavior.The test suite properly covers:
- Using selection when enabled (line 68-76)
- Falling back to prompt when disabled (line 78-88)
- Treating whitespace-only selection as empty (line 90-98)
The mocking strategy and assertions are sound.
src/engine/CaptureChoiceEngine.selection.test.ts (1)
128-171: LGTM! Complete coverage of override resolution logic.The test suite verifies all three scenarios:
- Global setting used when no per-choice override (line 133-144)
- Per-choice override takes precedence over global (line 146-157, 159-170)
- Both enabling and disabling directions are tested
The test structure and assertions are correct.
src/preflight/runOnePagePreflight.selection.test.ts (1)
100-151: LGTM! Preflight selection behavior properly tested.The test suite correctly verifies:
- When enabled: selection prefills
value, modal skipped (returnfalse) (line 106-127)- When disabled: modal shown, manual input used (return
true) (line 129-151)The return value semantics are correctly tested (
false= no modal needed,true= modal shown).src/formatters/completeFormatter.ts (2)
166-201: LGTM! Well-structured selection-first value resolution with proper error handling.The refactored flow correctly:
- Distinguishes
undefinedfrom empty string (line 167)- Attempts selection first when enabled (lines 168-174)
- Falls back to appropriate prompt based on context (lines 176-191)
- Handles cancellation consistently by throwing
MacroAbortError(lines 192-196)
158-160: The base class methodshouldUseSelectionForValue()returningtrueonly applies toCaptureChoiceFormatter, which is the sole subclass ofCompleteFormatter. NoTemplateFormatterorMacroFormatterclasses extendCompleteFormatter—they are choice/engine types, not formatters.CaptureChoiceFormatteralready overrides the method to check the configurable flag. No action needed.Likely an incorrect or invalid review comment.
src/formatters/captureChoiceFormatter.ts (2)
21-21: LGTM! Default aligns with expected global setting.The default value of
trueforuseSelectionAsCaptureValueis consistent with the feature being enabled by default for Capture choices.
48-51: LGTM! Whitespace-only handling is intentional and well-implemented.The logic correctly treats whitespace-only selections as empty (per PR objectives) while preserving original whitespace for selections containing non-whitespace content. For example:
" "→""(whitespace-only becomes empty)" hello "→" hello "(preserves formatting)This distinction ensures meaningful selections are preserved exactly as typed while rejecting accidental whitespace-only selections.
Summary
Testing
Notes
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Fixes #594