feat: improve macro open file command#994
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@chhoumann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User UI
participant Modal as OpenFileCommandSettingsModal
participant Cmd as OpenFileCommand
participant Engine as MacroChoiceEngine
participant Builder as buildOpenFileOptions
participant Host as Host File API
User->>Modal: open settings, choose location/direction/focus
Modal->>Modal: (if location == split) show direction control
User->>Modal: Save
Modal->>Cmd: set command.location, command.focus, command.direction
Engine->>Builder: buildOpenFileOptions(command)
Builder->>Builder: normalize location/direction/focus (legacy fallbacks)
Builder-->>Engine: OpenFileOptions
Engine->>Host: open file with OpenFileOptions
Host-->>Engine: result (success/error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![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.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (2)
56-57: Location / direction UI is good; consider preserving last split direction and tightening typesThe new “Where to open” dropdown plus conditional “Split direction” control is a nice, clear UX and the reload pattern is simple.
Two small refinements to consider:
Preserve previous split direction when toggling away and back
Currently, whenevervalue !== "split"you clearthis.command.direction. That means a user who picks “Horizontal”, switches to “New tab”, then back to “Split” gets reset to “Vertical”. Sincedirectionis ignored whenlocation !== "split"anyway, you could keep the last value for a nicer UX, e.g.:
.onChange((value) => {this.command.location = value as OpenLocation;if (value !== "split") {this.command.direction = undefined;} else if (!this.command.direction) {this.command.direction = NewTabDirection.vertical;}this.reload();});
.onChange((value) => {this.command.location = value as OpenLocation;if (value === "split" && !this.command.direction) {this.command.direction = NewTabDirection.vertical;}this.reload();});(Assuming the open‑file engine ignores `direction` unless `location === "split"`.)
Avoid untyped string literals for
OpenLocationvalues
All of"reuse","tab","split","window","left-sidebar","right-sidebar"are hand‑typed and then castas OpenLocation. A small typed helper would reduce the chance of drift:const locationOptions: { value: OpenLocation; label: string }[] = [ { value: "reuse", label: "Reuse active tab" }, { value: "tab", label: "New tab" }, { value: "split", label: "Split" }, { value: "window", label: "New window" }, { value: "left-sidebar", label: "Left sidebar" }, { value: "right-sidebar",label: "Right sidebar" }, ]; new Setting(this.contentEl) .setName("Where to open") .setDesc("Choose tab, split, window, or sidebar") .addDropdown((dropdown) => { for (const { value, label } of locationOptions) { dropdown.addOption(value, label); } dropdown .setValue(this.command.location ?? this.deriveLocation()) .onChange((value: OpenLocation) => { this.command.location = value; if (value === "split" && !this.command.direction) { this.command.direction = NewTabDirection.vertical; } this.reload(); }); });This keeps behavior but improves type safety and UX a bit.
Also applies to: 78-105, 107-120
122-133: Focus toggle behavior is consistent; minor copy tweak optionalThe “Focus opened tab” toggle wires cleanly to
this.command.focusand uses the same?? truedefaulting as the constructor, so behavior is consistent and respects explicitfalse.If you want the label to read naturally for all locations (new window / sidebars as well), consider renaming the setting to something like “Focus opened file” while keeping the description as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/engine/MacroChoiceEngine.openFileOptions.test.ts(1 hunks)src/engine/MacroChoiceEngine.ts(2 hunks)src/engine/helpers/openFileOptions.ts(1 hunks)src/gui/MacroGUIs/CommandSequenceEditor.ts(1 hunks)src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts(5 hunks)src/types/macros/QuickCommands/IOpenFileCommand.ts(2 hunks)src/types/macros/QuickCommands/OpenFileCommand.integration.test.ts(2 hunks)src/types/macros/QuickCommands/OpenFileCommand.test.ts(2 hunks)src/types/macros/QuickCommands/OpenFileCommand.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/types/macros/QuickCommands/OpenFileCommand.ts (1)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)
src/engine/MacroChoiceEngine.ts (2)
src/engine/helpers/openFileOptions.ts (1)
buildOpenFileOptions(4-58)src/utilityObsidian.ts (1)
openFile(450-528)
src/engine/helpers/openFileOptions.ts (1)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)
src/engine/MacroChoiceEngine.openFileOptions.test.ts (2)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)src/engine/helpers/openFileOptions.ts (1)
buildOpenFileOptions(4-58)
src/types/macros/QuickCommands/OpenFileCommand.integration.test.ts (1)
src/types/macros/QuickCommands/OpenFileCommand.ts (1)
OpenFileCommand(7-38)
🔇 Additional comments (10)
src/gui/MacroGUIs/CommandSequenceEditor.ts (1)
454-454: LGTM! Clearer icon choice.The
file-searchicon better conveys the purpose of opening/finding a file compared to a generic file icon.src/engine/MacroChoiceEngine.ts (1)
601-603: LGTM! Good refactoring.Extracting option construction to
buildOpenFileOptionsimproves maintainability and enables easier testing of the option mapping logic.src/types/macros/QuickCommands/OpenFileCommand.ts (1)
16-37: LGTM! Excellent backward compatibility design.The constructor maintains backward compatibility while adding explicit
locationandfocuscontrol. ThederiveLocationmethod provides sensible defaults when location is not explicitly provided, mapping legacy flags (openInNewTabanddirection) to the new location-based approach.src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
12-13: LGTM!The interface extension with optional
locationandfocusfields maintains backward compatibility while enabling the new location-based file opening behavior.src/engine/MacroChoiceEngine.openFileOptions.test.ts (1)
1-80: LGTM! Comprehensive test coverage.The tests cover the critical scenarios for option building, including explicit location override, legacy flag fallback behavior, split directions, and focus control.
src/types/macros/QuickCommands/OpenFileCommand.integration.test.ts (1)
36-74: LGTM! Integration tests cover key location-based scenarios.The tests validate the new location parameter across multiple configurations, including split directions and sidebar placements, while maintaining backward compatibility checks.
src/engine/helpers/openFileOptions.ts (1)
4-58: LGTM! Well-structured option builder.The function correctly prioritizes explicit
locationover legacy flags and provides sensible fallbacks. The split direction defaulting to"vertical"(line 19) when not specified is a reasonable choice that prevents undefined behavior.src/types/macros/QuickCommands/OpenFileCommand.test.ts (1)
7-33: LGTM! Tests validate the new location and focus properties.The tests correctly verify default behavior (
location: "reuse"whenopenInNewTabis false) and custom configurations with explicit location and focus values.src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (2)
5-6: Defaulting oflocation/focusand legacy mapping looks solidCopying the command and forcing
type: CommandType.OpenFile, then backfillingfocusandlocationviaderiveLocation()gives a clear contract and good backward compatibility with olderopenInNewTab/direction-only commands. Nullish coalescing preserves explicitfalseonfocus, andderiveLocation()’s mapping to"reuse" | "tab" | "split"is straightforward and side‑effect free.No changes requested here.
Also applies to: 18-23, 135-141
148-150: Button bar alignment and guarded save look goodRight‑aligning the buttons with a gap and putting the CTA “Save” to the right of “Cancel” matches common UI conventions. Using
resolveWithGuard(this.command)in the Save handler integrates properly withonClose()’s guard, so the promise is resolved exactly once in all paths (Save, Cancel, or direct close).No issues from a correctness or UX perspective.
Also applies to: 161-168
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/engine/MacroChoiceEngine.openFileOptions.test.ts(1 hunks)src/engine/helpers/openFileOptions.ts(1 hunks)src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/engine/helpers/openFileOptions.ts (1)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)
src/engine/MacroChoiceEngine.openFileOptions.test.ts (2)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)src/engine/helpers/openFileOptions.ts (1)
buildOpenFileOptions(4-57)
🪛 GitHub Actions: Test
src/engine/MacroChoiceEngine.openFileOptions.test.ts
[error] 60-60: AssertionError: expected 'split' to be 'tab' // Object.is equality
🪛 GitHub Check: Test (20)
src/engine/MacroChoiceEngine.openFileOptions.test.ts
[failure] 60-60: src/engine/MacroChoiceEngine.openFileOptions.test.ts > buildOpenFileOptions > opens a new tab without splitting when no direction is chosen
AssertionError: expected 'split' to be 'tab' // Object.is equality
Expected: "tab"
Received: "split"
❯ src/engine/MacroChoiceEngine.openFileOptions.test.ts:60:28
🔇 Additional comments (13)
src/engine/MacroChoiceEngine.openFileOptions.test.ts (7)
20-26: LGTM!Correctly verifies that explicit location takes precedence.
28-35: LGTM!Correctly validates legacy behavior when
openInNewTab: false.
37-44: LGTM!Correctly validates legacy split behavior with default vertical direction.
46-53: LGTM!Correctly validates that explicit
location: "reuse"overrides legacyopenInNewTabflag.
64-75: LGTM!Correctly validates explicit split with vertical direction.
77-88: LGTM!Correctly validates explicit split with horizontal direction.
90-97: LGTM!Correctly validates window location with focus control.
src/engine/helpers/openFileOptions.ts (2)
4-36: LGTM!The explicit location handling correctly maps all supported location types and properly defaults split direction to vertical when not specified or invalid.
38-56: Legacy mapping preserves old behavior correctly.The implementation correctly preserves the legacy behavior where
openInNewTab: falseopens in a new tab (not reuse) andopenInNewTab: truesplits. The comment at line 45 clarifies this potentially confusing behavior, as the field nameopenInNewTab: falsemight suggest reusing the current tab rather than opening a new one.src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (4)
78-110: LGTM!The location dropdown implementation is well-structured with typed options and correctly handles the split direction default. The conditional rendering of the direction control (lines 107-109) aligns with the PR objective to show split direction only when relevant.
112-125: LGTM!Split direction control correctly defaults to vertical, matching the helper's default behavior.
127-138: LGTM!Focus control implementation correctly defaults to
true, matching the helper's default and providing clear user-facing labels as mentioned in the PR objectives.
150-174: LGTM!Button bar layout is clean with proper spacing and clear CTAs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/engine/MacroChoiceEngine.openFileOptions.test.ts (1)
90-97: Consider adding a test for the defaultfocusbehavior.While the window test verifies explicit
focus: false, there's no test confirming thatfocusdefaults totruewhen undefined, as implemented in the helper (line 4 ofopenFileOptions.ts).Add a test case to verify the default:
it("defaults focus to true when not specified", () => { const options = buildOpenFileOptions( createCommand({ location: "tab" }) ); expect(options.focus).toBe(true); });Additionally, you may consider:
- Testing the
left-sidebarlocation for completeness- Verifying the
modefield is set to"default"in the returned options- Testing edge cases like invalid direction values with
location: "split"(the helper defaults to"vertical")src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (1)
78-110: Minor redundancy in location resolution.The logic is sound and correctly shows/hides the direction setting based on the selected location. However, lines 97 and 107 use
this.command.location ?? this.deriveLocation(), butderiveLocation()already checksthis.command.locationfirst and returns it if present, making the nullish coalescing redundant.Apply this diff to simplify:
dropdown - .setValue(this.command.location ?? this.deriveLocation()) + .setValue(this.deriveLocation()) .onChange((value: OpenLocation) => { this.command.location = value; if (value === "split" && !this.command.direction) { this.command.direction = NewTabDirection.vertical; } this.reload(); }); }); - if ((this.command.location ?? this.deriveLocation()) === "split") { + if (this.deriveLocation() === "split") { this.addDirectionSetting(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/engine/MacroChoiceEngine.openFileOptions.test.ts(1 hunks)src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/engine/MacroChoiceEngine.openFileOptions.test.ts (2)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)src/engine/helpers/openFileOptions.ts (1)
buildOpenFileOptions(4-57)
🔇 Additional comments (9)
src/engine/MacroChoiceEngine.openFileOptions.test.ts (2)
7-17: LGTM!The
createCommandhelper is well-structured, providing sensible defaults while allowing flexible overrides for test scenarios.
19-88: Excellent test coverage for core scenarios.The test cases accurately verify the helper's behavior for explicit locations, legacy mappings, and split directions. The assertions align with the implementation in
openFileOptions.ts.src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (7)
5-6: LGTM!The new imports are appropriate for the added location and type functionality.
18-22: LGTM!The initialization correctly ensures the command type is set and provides sensible defaults for legacy commands. The
focusdefault oftruealigns with expected UX (users typically want to see the file they opened), andderiveLocation()handles backward compatibility with the legacyopenInNewTabflag.
56-57: LGTM!The display flow correctly calls the new location and focus settings methods, replacing the legacy boolean toggle with more explicit controls.
112-125: LGTM!The direction setting correctly defaults to vertical when not set, matching the PR's legacy behavior specification (openInNewTab=true defaults to vertical split).
127-138: LGTM!The focus toggle is straightforward and correctly defaults to
true, providing good UX by bringing the opened file to the foreground.
140-146: Past review concern has been resolved!The
deriveLocation()logic now correctly aligns with the helper's behavior:
openInNewTabfalse/undefined → returns"tab"(line 145)openInNewTabtrue → returns"split"(line 143)This ensures legacy commands display the correct location in the UI.
150-174: LGTM!The button bar layout improvements provide better visual hierarchy with the primary action (Save) positioned after Cancel, following common UI patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (1)
78-110: Well-structured location dropdown with comprehensive options.The location dropdown offers all expected choices with clear labels, and the conditional display of the split direction setting enhances UX.
Minor optimization: Since
this.command.locationis always backfilled in the constructor (line 22), you can replacethis.deriveLocation()calls with direct property access for clarity:dropdown - .setValue(this.deriveLocation()) + .setValue(this.command.location!) .onChange((value: OpenLocation) => {- if (this.deriveLocation() === "split") { + if (this.command.location === "split") { this.addDirectionSetting(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/engine/MacroChoiceEngine.openFileOptions.test.ts(1 hunks)src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts(5 hunks)src/types/macros/QuickCommands/OpenFileCommand.integration.test.ts(2 hunks)src/types/macros/QuickCommands/OpenFileCommand.test.ts(2 hunks)src/types/macros/QuickCommands/OpenFileCommand.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/engine/MacroChoiceEngine.openFileOptions.test.ts
- src/types/macros/QuickCommands/OpenFileCommand.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/types/macros/QuickCommands/OpenFileCommand.ts (1)
src/types/macros/QuickCommands/IOpenFileCommand.ts (1)
IOpenFileCommand(5-14)
src/types/macros/QuickCommands/OpenFileCommand.integration.test.ts (1)
src/types/macros/QuickCommands/OpenFileCommand.ts (1)
OpenFileCommand(7-29)
🔇 Additional comments (6)
src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts (3)
18-22: LGTM! Defensive backfill for legacy commands.The copy-and-backfill pattern correctly ensures legacy commands receive sensible defaults for the new
locationandfocusfields while preserving the original command object.
127-138: LGTM! Clear focus control.The focus toggle is well-labeled and correctly wired with appropriate defaults.
140-146: LGTM! Logic aligns with helper.The deriveLocation() logic correctly maps legacy
openInNewTabflags to the new location system, matching the behavior ofbuildOpenFileOptions()as noted in the resolved past review.src/types/macros/QuickCommands/OpenFileCommand.integration.test.ts (1)
36-72: LGTM! Comprehensive test coverage.The test configurations thoroughly cover all location types (tab, split, window, sidebars) and verify both legacy compatibility and new functionality. The assertions correctly validate the
locationfield.src/types/macros/QuickCommands/OpenFileCommand.ts (2)
11-14: LGTM! Property declarations align with interface.The new
locationandfocusproperties are correctly typed and match theIOpenFileCommandinterface, maintaining backward compatibility with existingopenInNewTabanddirectionfields.
16-28: LGTM! Backward-compatible constructor extension.The constructor correctly adds
locationandfocusparameters at the end, maintaining backward compatibility while supporting the new open file options. Thefocus = truedefault is consistent with the UI modal's backfill logic.
# [2.9.0](2.8.0...2.9.0) (2025-12-09) ### Bug Fixes * honor dateFormat in requestInputs ([903df23](903df23)) * preserve macro user script variable updates ([#999](#999)) ([26a7cf5](26a7cf5)) * reuse current tab when open file new-tab is off ([#1007](#1007)) ([1b67d77](1b67d77)) ### Features * improve macro open file command ([#994](#994)) ([9b4f177](9b4f177))
|
🎉 This PR is included in version 2.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Testing
Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.