Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdded workflow and triage documentation to AGENTS.md; introduced vault-path utilities (normalizeVaultFilePath, areSameVaultFilePath), updated openExistingFileTab to use path equivalence, and added tests for path normalization and comparison. Also added a new .codex environment manifest. Changes
Sequence Diagram(s)(omitted — changes are documentation, small utility additions, and tests; no multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying quickadd with
|
| Latest commit: |
ba1ab62
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6591702d.quickadd.pages.dev |
| Branch Preview URL: | https://780-bug-quickadd-opens-newly.quickadd.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
chhoumann
deleted the
780-bug-quickadd-opens-newly-created-file-in-two-separate-tabs-when-using-cmd+n-on-macos
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utilityObsidian.ts (2)
901-914:returninsideiterateRootLeavesdoes not short-circuit iteration.This is pre-existing behaviour and not introduced by this PR, but worth noting for context: when multiple leaves display the same file,
leafis overwritten on each match and the function ends up returning the last matching leaf rather than the first. If "first found" semantics are desired, the callback should throw or use an external flag, since Obsidian'siterateRootLeavesdoesn't support early exit via a return value.The
view.file === file || areSameVaultFilePath(...)guard itself is correct and a clean improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utilityObsidian.ts` around lines 901 - 914, The iterateRootLeaves callback uses a plain return to attempt early exit, but Obsidian's app.workspace.iterateRootLeaves doesn't short-circuit on callback return so the variable leaf (set inside the callback) can be overwritten and ends up as the last match; modify the logic in the iterateRootLeaves loop (the callback passed to app.workspace.iterateRootLeaves, which inspects WorkspaceLeaf and FileView) to stop after the first match by either throwing a sentinel Error to break out of iteration or by using an external boolean flag (e.g., found) checked at the top of the callback to skip further processing once leaf is set; ensure the matching condition (view.file === file || areSameVaultFilePath(file.path, view.file.path)) remains and is used to set leaf exactly once.
881-884: Stub fidelity gap:normalizeVaultFilePathdoes defensive work compensating for minimal test stub.Obsidian's
normalizePathremoves leading and trailing slashes and collapses consecutive//sequences. The test stub attests/obsidian-stub.ts(line 511) only replaces backslashes with forward slashes. This meansnormalizeVaultFilePathlines 882–883 perform extra work in tests that would be redundant in production:
- Line 882:
.replace(/\/{2,}/g, "/")— collapsed by realnormalizePath- Line 883:
.replace(/^\/+/, "")— stripped by realnormalizePathThe code is correct and tests pass, but they validate the extra steps rather than confirming round-trip behavior through the real API. Bring the stub into parity with Obsidian's contract so test coverage reflects production:
Proposed stub update
export function normalizePath(p: string): string { if (typeof p !== 'string') return '' as unknown as string; - return p.replace(/\\/g, '/'); + return p.replace(/\\/g, '/').replace(/\/{2,}/g, '/').replace(/^\/+|\/+$/g, ''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utilityObsidian.ts` around lines 881 - 884, The test stub for Obsidian's normalizePath is too minimal, forcing normalizeVaultFilePath to compensate; update the stubbed normalizePath implementation used in tests to mirror Obsidian's contract (convert backslashes to forward slashes, collapse consecutive slashes into a single '/', and remove leading/trailing slashes) so tests exercise the same behavior as production and no longer validate the defensive .replace(/\/{2,}/g, "/") and .replace(/^\/+/, "") steps in normalizeVaultFilePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 60-71: Replace the three hard-coded machine-specific absolute
paths shown in the doc (the lines beginning "Dev vault root path:", "QuickAdd
plugin path:", and the symlink path string) with configurable placeholders or
instructions to use environment/config variables (e.g. <YOUR_DEV_VAULT_ROOT>,
<YOUR_QUICKADD_PLUGIN_PATH>, <YOUR_SYMLINK_PATH>) and add a short note that
contributors must set these values for their environment; update the examples
that reference running/building (e.g. "bun run dev", "obsidian plugin:reload
vault=dev id=quickadd") to indicate they assume those placeholders are
configured, and remove/neutralize any explicit author-specific absolute paths so
the AGENTS.md content is machine-agnostic.
In `@src/utilityObsidian.test.ts`:
- Around line 92-96: The test "normalizes windows path separators" currently
uses a double-escaped backslash literal ("notes\\\\capture.md") which represents
notes\\capture.md at runtime; add an additional assertion that passes a
single-backslash Windows path literal ("notes\\capture.md") to
normalizeVaultFilePath and expect "notes/capture.md", and update the test
description (or add a separate it block) to clearly state it covers
single-backslash Windows separators; keep or retain the existing double-escape
case if you want to also cover already-escaped strings.
---
Nitpick comments:
In `@src/utilityObsidian.ts`:
- Around line 901-914: The iterateRootLeaves callback uses a plain return to
attempt early exit, but Obsidian's app.workspace.iterateRootLeaves doesn't
short-circuit on callback return so the variable leaf (set inside the callback)
can be overwritten and ends up as the last match; modify the logic in the
iterateRootLeaves loop (the callback passed to app.workspace.iterateRootLeaves,
which inspects WorkspaceLeaf and FileView) to stop after the first match by
either throwing a sentinel Error to break out of iteration or by using an
external boolean flag (e.g., found) checked at the top of the callback to skip
further processing once leaf is set; ensure the matching condition (view.file
=== file || areSameVaultFilePath(file.path, view.file.path)) remains and is used
to set leaf exactly once.
- Around line 881-884: The test stub for Obsidian's normalizePath is too
minimal, forcing normalizeVaultFilePath to compensate; update the stubbed
normalizePath implementation used in tests to mirror Obsidian's contract
(convert backslashes to forward slashes, collapse consecutive slashes into a
single '/', and remove leading/trailing slashes) so tests exercise the same
behavior as production and no longer validate the defensive .replace(/\/{2,}/g,
"/") and .replace(/^\/+/, "") steps in normalizeVaultFilePath.
| - Always target the `dev` vault when using the Obsidian CLI by passing | ||
| `vault=dev` on every command. | ||
| - Dev vault root path: `/Users/christian/Developer/dev_vault/dev/`. | ||
| - QuickAdd plugin path in the vault: | ||
| `/Users/christian/Developer/dev_vault/dev/.obsidian/plugins/quickadd`. | ||
| - Run `bun run dev` in this repository to generate/update `main.js` for | ||
| development. | ||
| - Reload QuickAdd after build/deploy with: | ||
| `obsidian plugin:reload vault=dev id=quickadd`. | ||
| - In this setup, the vault plugin `main.js` is symlinked to | ||
| `/Users/christian/Developer/quickadd/main.js`, so rebuilding updates | ||
| the active plugin code directly. |
There was a problem hiding this comment.
Hard-coded machine-specific absolute paths will mislead other contributors and AI agents.
Lines 62–71 embed paths that only exist on the author's machine:
Dev vault root path: /Users/christian/Developer/dev_vault/dev/QuickAdd plugin path: /Users/christian/Developer/dev_vault/dev/.obsidian/plugins/quickadd- Symlink path:
/Users/christian/Developer/quickadd/main.js
Any agent or developer following these instructions on a different machine will either fail silently or write to non-existent locations. Consider replacing the hard-coded values with placeholder tokens (e.g. <YOUR_DEV_VAULT_ROOT>) or directing contributors to set a local config variable, and adding a note that these values are machine-specific and must be configured per-environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 60 - 71, Replace the three hard-coded
machine-specific absolute paths shown in the doc (the lines beginning "Dev vault
root path:", "QuickAdd plugin path:", and the symlink path string) with
configurable placeholders or instructions to use environment/config variables
(e.g. <YOUR_DEV_VAULT_ROOT>, <YOUR_QUICKADD_PLUGIN_PATH>, <YOUR_SYMLINK_PATH>)
and add a short note that contributors must set these values for their
environment; update the examples that reference running/building (e.g. "bun run
dev", "obsidian plugin:reload vault=dev id=quickadd") to indicate they assume
those placeholders are configured, and remove/neutralize any explicit
author-specific absolute paths so the AGENTS.md content is machine-agnostic.
| it("normalizes windows path separators", () => { | ||
| expect(normalizeVaultFilePath("notes\\\\capture.md")).toBe( | ||
| "notes/capture.md", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Test uses double backslash; the typical Windows path has a single backslash.
"notes\\\\capture.md" in source is notes\\capture.md at runtime (two backslashes). A real Windows path separator would be one backslash — "notes\\capture.md" in source → notes\capture.md at runtime. The test description "normalizes windows path separators" is therefore slightly misleading. Consider adding the single-backslash case as well:
🛠️ Suggested test additions
it("normalizes windows path separators", () => {
+ expect(normalizeVaultFilePath("notes\\capture.md")).toBe(
+ "notes/capture.md",
+ );
expect(normalizeVaultFilePath("notes\\\\capture.md")).toBe(
"notes/capture.md",
);
});📝 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.
| it("normalizes windows path separators", () => { | |
| expect(normalizeVaultFilePath("notes\\\\capture.md")).toBe( | |
| "notes/capture.md", | |
| ); | |
| }); | |
| it("normalizes windows path separators", () => { | |
| expect(normalizeVaultFilePath("notes\\capture.md")).toBe( | |
| "notes/capture.md", | |
| ); | |
| expect(normalizeVaultFilePath("notes\\\\capture.md")).toBe( | |
| "notes/capture.md", | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utilityObsidian.test.ts` around lines 92 - 96, The test "normalizes
windows path separators" currently uses a double-escaped backslash literal
("notes\\\\capture.md") which represents notes\\capture.md at runtime; add an
additional assertion that passes a single-backslash Windows path literal
("notes\\capture.md") to normalizeVaultFilePath and expect "notes/capture.md",
and update the test description (or add a separate it block) to clearly state it
covers single-backslash Windows separators; keep or retain the existing
double-escape case if you want to also cover already-escaped strings.
# [2.12.0](2.11.0...2.12.0) (2026-03-05) ### Bug Fixes * **capture:** preserve canvas tab indentation on configured writes ([#1125](#1125)) ([0a1578e](0a1578e)) * disallow capture targets with .base extension ([cb39ed4](cb39ed4)) * **field-suggestions:** opt-in inline values from fenced code blocks ([#1128](#1128)) ([8597905](8597905)) * **gui:** preserve modal edit position during settings reload ([#1132](#1132)) ([11bda19](11bda19)) * **gui:** reduce ai settings modal reload churn ([#1134](#1134)) ([ae0f7a1](ae0f7a1)) * **gui:** reduce choice builder reload churn ([#1136](#1136)) ([818272a](818272a)) * **gui:** reduce macro settings modal reload churn ([#1135](#1135)) ([a1a6271](a1a6271)) * harden existing-tab matching and document issue workflow ([#1108](#1108)) ([7b12d3b](7b12d3b)) * make template path resolution deterministic ([3297d54](3297d54)) * normalize capture title for non-markdown targets ([964d672](964d672)) * preserve capture-format spacing for insert-at-end ([#1119](#1119)) ([8bb8ed4](8bb8ed4)) * preserve explicit capture target file extensions ([57e43ff](57e43ff)) * preserve insert-at-end order for non-newline captures ([#1120](#1120)) ([e7cbbf2](e7cbbf2)) * resolve template file-name paths without duplicate default folder ([7bfd41b](7bfd41b)) * resolve vault-relative template paths using root folders ([81216de](81216de)) ### Features * add AI request logging API and reduce assistant log noise ([#1110](#1110)) ([2c36800](2c36800)) * automate docs version snapshot during release ([#1111](#1111)) ([1571846](1571846)) * **capture:** fully support capture into canvas cards ([#1124](#1124)) ([a53f889](a53f889)) * **cli:** add native QuickAdd Obsidian CLI handlers ([#1129](#1129)) ([8102d47](8102d47)) * **format:** support mapped VALUE suggester display text ([#1127](#1127)) ([b8ec56c](b8ec56c)) * **macro:** add editor cursor navigation commands ([101d5f6](101d5f6)) * support .base template files for template choices ([11e6490](11e6490)) ### Reverts * **gui:** remove modal reload refactor series ([#1137](#1137)) ([3ba1a73](3ba1a73)), closes [#1136](#1136) [#1135](#1135) [#1134](#1134) [#1133](#1133) [#1132](#1132)
|
🎉 This PR is included in version 2.12.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
openExistingFileTabAGENTS.mdgh issue develop <issue-number>rule for issue-driven workContext
Issue #780 was investigated on macOS and could not be reproduced in the current dev setup, but path-comparison hardening is kept as a low-risk guard against path-shape mismatches when reusing open tabs.
Validation
bun run test src/utilityObsidian.test.tsbun run lintbun run buildSummary by CodeRabbit
Documentation
Improvements
Tests
Chores