Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR consolidates file existence behavior handling from legacy multiple fields into a unified policy-driven system. It introduces a new fileExistsPolicy module defining behavior categories and resolution strategies, removes legacy constants, refactors TemplateChoiceEngine with modular helpers, and adds migrations to normalize existing configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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)
📝 Coding Plan
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 |
…ros-are-stripped-from-template-filename-when-starting-with-tt0xxxx
Deploying quickadd with
|
| Latest commit: |
91c24f0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ad0935d4.quickadd.pages.dev |
| Branch Preview URL: | https://1139-bug-leading-zeros-are-s.quickadd.pages.dev |
![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.
Reviewed commit: 0f8fa2e626
ℹ️ 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".
Comment on lines
+15
to
+19
| if (isMultiChoice(choice)) { | ||
| choice.choices = normalizeChoices(choice.choices); | ||
| } | ||
|
|
||
| if (isTemplateChoice(choice)) { |
There was a problem hiding this comment.
This migration only descends into Multi children, so Template choices embedded in Macro choice command lists are skipped. Those embedded templates can still carry legacy setFileExistsBehavior/fileExistsMode, and because TemplateChoiceEngine.getSelectedFileExistsMode falls back to { kind: "prompt" } when fileExistsBehavior is missing, users who configured a fixed collision action in nested macro template commands will start getting prompts after this update.
Useful? React with 👍 / 👎.
Comment on lines
+31
to
+34
| for (const command of macro.commands) { | ||
| if (isNestedChoiceCommand(command) && isTemplateChoice(command.choice)) { | ||
| normalizeTemplateChoice(command.choice); | ||
| } |
There was a problem hiding this comment.
normalizeMacros only inspects top-level macro.commands entries and does not walk nested command arrays (for example Conditional thenCommands/elseCommands). Any template choice nested in those branches keeps legacy collision fields, so its file-exists behavior is not migrated and will silently degrade to prompt mode at runtime.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/migrations/incrementFileNameSettingMoveToDefaultBehavior.test.ts (1)
4-99: Add a regression case forincrementFileName: false.Please add a test that verifies
incrementFileName: falsedoes not forcesetFileExistsBehavior/fileExistsModeto increment, while still removingincrementFileName.Based on learnings: Applies to **/*.test.{ts,tsx} : Add regression coverage for bug fixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/incrementFileNameSettingMoveToDefaultBehavior.test.ts` around lines 4 - 99, Add a regression test that supplies a choice with incrementFileName: false and asserts after calling migration.migrate that (1) setFileExistsBehavior and fileExistsMode were NOT changed to the "Increment the file name" values for that choice, and (2) the incrementFileName property was removed; do the same for a nested macro command choice (check plugin.settings.choices[...] and plugin.settings.macros[0].commands[0].choice) to ensure migration.migrate only converts true and strips the property for false without modifying fileExistsMode or setFileExistsBehavior.src/migrations/consolidateFileExistsBehavior.test.ts (1)
54-85: Strengthen nested-macro assertions by validating legacy field cleanup.The nested test verifies the new field, but also asserting removal of legacy fields would better prevent partial-normalization regressions.
Suggested assertion additions
expect(plugin.settings.macros[0].commands[0].choice).toMatchObject({ fileExistsBehavior: { kind: "prompt" }, }); + expect( + plugin.settings.macros[0].commands[0].choice.fileExistsMode, + ).toBeUndefined(); + expect( + plugin.settings.macros[0].commands[0].choice.setFileExistsBehavior, + ).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/consolidateFileExistsBehavior.test.ts` around lines 54 - 85, The test should not only assert the new normalized field but also verify legacy fields are removed to prevent partial normalization; after calling migration.migrate(plugin) add assertions that plugin.settings.macros[0].commands[0].choice.setFileExistsBehavior and .fileExistsMode are undefined (or not present) alongside the existing expect for fileExistsBehavior, referencing the same nested object in plugin.settings.macros and migration.migrate to locate the code to update.src/migrations/consolidateFileExistsBehavior.ts (2)
49-49: RedundantdeepClonecall on line 49.
normalizeChoicesmutates the array in-place and returns it. The inputchoicesCopyis already a deep clone from line 46. Wrapping the result in anotherdeepCloneis unnecessary and adds overhead.♻️ Proposed fix
- plugin.settings.choices = deepClone(normalizeChoices(choicesCopy)); + plugin.settings.choices = normalizeChoices(choicesCopy);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/consolidateFileExistsBehavior.ts` at line 49, Remove the redundant deepClone when assigning plugin.settings.choices: since choicesCopy is already produced by deepClone and normalizeChoices mutates and returns that array, replace plugin.settings.choices = deepClone(normalizeChoices(choicesCopy)) with a direct assignment plugin.settings.choices = normalizeChoices(choicesCopy) (referencing normalizeChoices, choicesCopy, and plugin.settings.choices).
47-47: Type assertion for macros access.Using
(plugin.settings as any).macrossuggests macros may not be in the typed settings interface. This is acceptable for migration code dealing with legacy data structures, but consider adding a comment explaining why the assertion is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/consolidateFileExistsBehavior.ts` at line 47, The cast (plugin.settings as any).macros is used to read legacy/unknown migration data but lacks explanation; add a concise comment above the line that explains this is migration code handling legacy settings where macros may not exist on the typed Settings interface, so we use a type assertion to avoid TS errors while safely defaulting to [] (line referencing deepClone and macrosCopy). Keep the existing behavior (deepClone((plugin.settings as any).macros || [])) but document why the assertion is required and that this is intentional for backward-compatibility during the consolidateFileExistsBehavior migration.src/gui/ChoiceBuilder/templateChoiceBuilder.ts (1)
471-476: Inconsistent indentation on Line 475.The
this.reload()call has an extra level of indentation compared to the surrounding code.🧹 Proposed fix
dropdown.setValue(selectedMode).onChange((value: FileExistsModeId) => { this.choice.fileExistsBehavior = { kind: "apply", mode: value, }; - this.reload(); + this.reload(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/ChoiceBuilder/templateChoiceBuilder.ts` around lines 471 - 476, Align the indentation of the this.reload() call with the surrounding block: inside the callback where this.choice.fileExistsBehavior is set (the code that assigns kind and mode), reduce one level of indentation so that this.reload() sits at the same indentation level as the this.choice assignment; update the callback in templateChoiceBuilder.ts (the block that sets choice.fileExistsBehavior) to keep consistent formatting with the surrounding code.src/migrations/helpers/normalizeTemplateFileExistsBehavior.ts (1)
30-35: Add warning log when fallback to "increment" mode occurs.When
mapLegacyFileExistsModeToIdreturnsnull(unknown or non-string legacy mode), the code silently defaults to"increment". This masks potential data issues and should be logged so users can diagnose migration problems.Import the logger and add a warning:
import { logger as log } from "src/utils/Logger";Then at line 33, log when the fallback is triggered:
const mode = mapLegacyFileExistsModeToId(choice.fileExistsMode); if (mode === null) { log.logWarning(`Unknown file exists mode, defaulting to "increment": ${choice.fileExistsMode}`); } return { kind: "apply", mode: mode ?? "increment", };This pattern is already used in other migrations (e.g.,
migrateProviderApiKeysToSecretStorage.ts) for similar fallback scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/helpers/normalizeTemplateFileExistsBehavior.ts` around lines 30 - 35, Import the logger (import { logger as log } from "src/utils/Logger") into normalizeTemplateFileExistsBehavior.ts, call mapLegacyFileExistsModeToId(choice.fileExistsMode) into a local const (e.g., const mode = mapLegacyFileExistsModeToId(choice.fileExistsMode)), and if mode === null call log.logWarning with a message that includes choice.fileExistsMode (e.g., "Unknown file exists mode, defaulting to \"increment\": <value>"); then return the object with kind: "apply" and mode: mode ?? "increment". Ensure you update the block that currently returns directly when choice.setFileExistsBehavior is truthy to use this new logic and reference mapLegacyFileExistsModeToId and choice.fileExistsMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/Choices/TemplateChoice.md`:
- Line 76: The code spans in the "Append duplicate suffix" line contain leading
spaces inside backticks (`` ` (1)` ``, `` ` (2)` ``) which triggers MD038; edit
the "Append duplicate suffix" description to remove the leading spaces inside
the code spans so they read `(1)` and `(2)` (e.g., change `` ` (1)` `` to ``
`(1)` ``) ensuring the examples `note.md` → `note (1).md` remain correct.
In `@src/migrations/consolidateFileExistsBehavior.ts`:
- Around line 27-39: normalizeMacros currently only normalizes direct
TemplateChoice values on NestedChoiceCommand.command.choice, so TemplateChoice
items inside a MultiChoice are skipped; update normalizeMacros (and its loop
over macro.commands) to detect when command.choice is a MultiChoice and
recursively traverse its choices (like the existing normalizeChoices
implementation) calling normalizeTemplateChoice on any nested TemplateChoice
items (use isNestedChoiceCommand, isTemplateChoice, normalizeTemplateChoice, and
the MultiChoice type checks to locate and normalize nested choices).
In `@src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts`:
- Around line 16-25: isOldTemplateChoice currently only checks for the presence
of incrementFileName and the migration later always enables increment behavior;
change the guard and migration logic so increment mode is enabled only when
incrementFileName === true: update isOldTemplateChoice to verify the property
exists and is a boolean (and prefer checking (choice as { incrementFileName?:
boolean }).incrementFileName === true where used), and modify the migration
assignments that unconditionally force increment behavior to instead set
increment only when the original choice.incrementFileName === true (leave or
default to false when it's absent or explicitly false).
---
Nitpick comments:
In `@src/gui/ChoiceBuilder/templateChoiceBuilder.ts`:
- Around line 471-476: Align the indentation of the this.reload() call with the
surrounding block: inside the callback where this.choice.fileExistsBehavior is
set (the code that assigns kind and mode), reduce one level of indentation so
that this.reload() sits at the same indentation level as the this.choice
assignment; update the callback in templateChoiceBuilder.ts (the block that sets
choice.fileExistsBehavior) to keep consistent formatting with the surrounding
code.
In `@src/migrations/consolidateFileExistsBehavior.test.ts`:
- Around line 54-85: The test should not only assert the new normalized field
but also verify legacy fields are removed to prevent partial normalization;
after calling migration.migrate(plugin) add assertions that
plugin.settings.macros[0].commands[0].choice.setFileExistsBehavior and
.fileExistsMode are undefined (or not present) alongside the existing expect for
fileExistsBehavior, referencing the same nested object in plugin.settings.macros
and migration.migrate to locate the code to update.
In `@src/migrations/consolidateFileExistsBehavior.ts`:
- Line 49: Remove the redundant deepClone when assigning
plugin.settings.choices: since choicesCopy is already produced by deepClone and
normalizeChoices mutates and returns that array, replace plugin.settings.choices
= deepClone(normalizeChoices(choicesCopy)) with a direct assignment
plugin.settings.choices = normalizeChoices(choicesCopy) (referencing
normalizeChoices, choicesCopy, and plugin.settings.choices).
- Line 47: The cast (plugin.settings as any).macros is used to read
legacy/unknown migration data but lacks explanation; add a concise comment above
the line that explains this is migration code handling legacy settings where
macros may not exist on the typed Settings interface, so we use a type assertion
to avoid TS errors while safely defaulting to [] (line referencing deepClone and
macrosCopy). Keep the existing behavior (deepClone((plugin.settings as
any).macros || [])) but document why the assertion is required and that this is
intentional for backward-compatibility during the consolidateFileExistsBehavior
migration.
In `@src/migrations/helpers/normalizeTemplateFileExistsBehavior.ts`:
- Around line 30-35: Import the logger (import { logger as log } from
"src/utils/Logger") into normalizeTemplateFileExistsBehavior.ts, call
mapLegacyFileExistsModeToId(choice.fileExistsMode) into a local const (e.g.,
const mode = mapLegacyFileExistsModeToId(choice.fileExistsMode)), and if mode
=== null call log.logWarning with a message that includes choice.fileExistsMode
(e.g., "Unknown file exists mode, defaulting to \"increment\": <value>"); then
return the object with kind: "apply" and mode: mode ?? "increment". Ensure you
update the block that currently returns directly when
choice.setFileExistsBehavior is truthy to use this new logic and reference
mapLegacyFileExistsModeToId and choice.fileExistsMode.
In `@src/migrations/incrementFileNameSettingMoveToDefaultBehavior.test.ts`:
- Around line 4-99: Add a regression test that supplies a choice with
incrementFileName: false and asserts after calling migration.migrate that (1)
setFileExistsBehavior and fileExistsMode were NOT changed to the "Increment the
file name" values for that choice, and (2) the incrementFileName property was
removed; do the same for a nested macro command choice (check
plugin.settings.choices[...] and plugin.settings.macros[0].commands[0].choice)
to ensure migration.migrate only converts true and strips the property for false
without modifying fileExistsMode or setFileExistsBehavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1337bf17-d036-4883-8839-2113dafbb5d0
📒 Files selected for processing (25)
docs/docs/Choices/TemplateChoice.mddocs/docs/Examples/Template_CreateMOCNoteWithLinkDashboard.mdsrc/constants.tssrc/engine/CaptureChoiceEngine.notice.test.tssrc/engine/CaptureChoiceEngine.template-property-types.test.tssrc/engine/MacroChoiceEngine.notice.test.tssrc/engine/TemplateChoiceEngine.collision.test.tssrc/engine/TemplateChoiceEngine.notice.test.tssrc/engine/TemplateChoiceEngine.tssrc/engine/TemplateEngine.tssrc/engine/templateEngine-increment-canvas.test.tssrc/gui/ChoiceBuilder/templateChoiceBuilder.tssrc/migrations/consolidateFileExistsBehavior.test.tssrc/migrations/consolidateFileExistsBehavior.tssrc/migrations/helpers/isOldTemplateChoice.tssrc/migrations/helpers/normalizeTemplateFileExistsBehavior.tssrc/migrations/incrementFileNameSettingMoveToDefaultBehavior.test.tssrc/migrations/incrementFileNameSettingMoveToDefaultBehavior.tssrc/migrations/migrate.tssrc/preflight/runOnePagePreflight.selection.test.tssrc/settings.tssrc/template/fileExistsPolicy.test.tssrc/template/fileExistsPolicy.tssrc/types/choices/ITemplateChoice.tssrc/types/choices/TemplateChoice.ts
💤 Files with no reviewable changes (4)
- src/engine/TemplateEngine.ts
- src/constants.ts
- src/engine/templateEngine-increment-canvas.test.ts
- src/migrations/helpers/isOldTemplateChoice.ts
Comment on lines
+27
to
+39
| function normalizeMacros(macros: IMacro[]): IMacro[] { | ||
| for (const macro of macros) { | ||
| if (!Array.isArray(macro.commands)) continue; | ||
|
|
||
| for (const command of macro.commands) { | ||
| if (isNestedChoiceCommand(command) && isTemplateChoice(command.choice)) { | ||
| normalizeTemplateChoice(command.choice); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return macros; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MultiChoice can be nested inside macro commands by looking at types and existing data patterns
ast-grep --pattern $'interface NestedChoiceCommand {
$$$
choice: $_
$$$
}'
# Also check how the previous migration handles this case
rg -n "isMultiChoice" src/migrations/Repository: chhoumann/quickadd
Length of output: 1032
🏁 Script executed:
#!/bin/bash
# Read the consolidateFileExistsBehavior.ts file to see the full context
cat -n src/migrations/consolidateFileExistsBehavior.ts | head -60Repository: chhoumann/quickadd
Length of output: 2061
🏁 Script executed:
#!/bin/bash
# Read incrementFileNameSettingMoveToDefaultBehavior.ts to see how it handles similar cases
cat -n src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts | head -70Repository: chhoumann/quickadd
Length of output: 2570
🏁 Script executed:
#!/bin/bash
# Check the NestedChoiceCommand type definition
rg -A 5 "interface NestedChoiceCommand" src/Repository: chhoumann/quickadd
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for NestedChoiceCommand type definition
rg -B 2 -A 10 "type.*NestedChoiceCommand|interface.*NestedChoiceCommand" src/Repository: chhoumann/quickadd
Length of output: 10742
🏁 Script executed:
#!/bin/bash
# Also check the helper file
cat -n src/migrations/helpers/isNestedChoiceCommand.tsRepository: chhoumann/quickadd
Length of output: 506
normalizeMacros should recursively handle MultiChoice within macro commands.
The function only checks for direct TemplateChoice values in nested choice commands, but command.choice can be a MultiChoice containing template choices. If a macro contains a NestedChoiceCommand where the choice is a MultiChoice, any TemplateChoice items nested inside won't be normalized. Add recursive MultiChoice handling similar to the normalizeChoices function (lines 13-25).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/migrations/consolidateFileExistsBehavior.ts` around lines 27 - 39,
normalizeMacros currently only normalizes direct TemplateChoice values on
NestedChoiceCommand.command.choice, so TemplateChoice items inside a MultiChoice
are skipped; update normalizeMacros (and its loop over macro.commands) to detect
when command.choice is a MultiChoice and recursively traverse its choices (like
the existing normalizeChoices implementation) calling normalizeTemplateChoice on
any nested TemplateChoice items (use isNestedChoiceCommand, isTemplateChoice,
normalizeTemplateChoice, and the MultiChoice type checks to locate and normalize
nested choices).
Comment on lines
+16
to
+25
| function isOldTemplateChoice( | ||
| choice: unknown, | ||
| ): choice is IChoice & OldTemplateChoice { | ||
| return ( | ||
| typeof choice === "object" && | ||
| choice !== null && | ||
| "type" in choice && | ||
| (choice as { type?: string }).type === "Template" && | ||
| "incrementFileName" in choice | ||
| ); |
There was a problem hiding this comment.
Don’t force increment mode when incrementFileName is false.
The current guard matches on property presence, then Lines 35-36 and 53-54 always force increment behavior. Legacy objects with incrementFileName: false would be migrated incorrectly.
Suggested patch
function recursiveRemoveIncrementFileName(choices: IChoice[]): IChoice[] {
for (const choice of choices) {
if (isMultiChoice(choice)) {
choice.choices = recursiveRemoveIncrementFileName(choice.choices);
}
if (isOldTemplateChoice(choice)) {
- choice.setFileExistsBehavior = true;
- choice.fileExistsMode = "Increment the file name";
+ if (choice.incrementFileName === true) {
+ choice.setFileExistsBehavior = true;
+ choice.fileExistsMode = "Increment the file name";
+ }
delete choice.incrementFileName;
}
}
@@
for (const command of macro.commands) {
if (
isNestedChoiceCommand(command) &&
isOldTemplateChoice(command.choice)
) {
- command.choice.setFileExistsBehavior = true;
- command.choice.fileExistsMode = "Increment the file name";
+ if (command.choice.incrementFileName === true) {
+ command.choice.setFileExistsBehavior = true;
+ command.choice.fileExistsMode = "Increment the file name";
+ }
delete command.choice.incrementFileName;
}
}
}Also applies to: 34-38, 50-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts` around lines
16 - 25, isOldTemplateChoice currently only checks for the presence of
incrementFileName and the migration later always enables increment behavior;
change the guard and migration logic so increment mode is enabled only when
incrementFileName === true: update isOldTemplateChoice to verify the property
exists and is a boolean (and prefer checking (choice as { incrementFileName?:
boolean }).incrementFileName === true where used), and modify the migration
assignments that unconditionally force increment behavior to instead set
increment only when the original choice.incrementFileName === true (leave or
default to false when it's absent or explicitly false).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
chhoumann
deleted the
1139-bug-leading-zeros-are-stripped-from-template-filename-when-starting-with-tt0xxxx
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes & Updates