Conversation
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughUpdated ChangesVDATE Format Argument Optionality
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: |
f3e25bc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://80224dc4.quickadd.pages.dev |
| Branch Preview URL: | https://1178-bug-vdate-breaks-one-pa.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/constants.ts (1)
29-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
FORMAT_SYNTAX/FILE_NAME_FORMAT_SYNTAXmissing no-format VDATE variantsThe regex change in Line 100 makes the
<date format>segment optional, but the suggestion arrays (used for autocomplete) still only list the two-argument form. Users inspecting the suggestion list won't discover that{{vdate:<variable name>}}is now valid.📝 Proposed fix
export const FORMAT_SYNTAX: string[] = [ DATE_SYNTAX, "{{date:<dateformat>}}", + "{{vdate:<variable name>}}", "{{vdate:<variable name>, <date format>}}", "{{vdate:<variable name>, <date format>|<default value>}}",export const FILE_NAME_FORMAT_SYNTAX: string[] = [ DATE_SYNTAX, "{{date:<dateformat>}}", + "{{vdate:<variable name>}}", "{{vdate:<variable name>, <date format>}}", "{{vdate:<variable name>, <date format>|<default value>}}",Also applies to: 60-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/constants.ts` around lines 29 - 30, The suggestion arrays for date formatting omit the one-argument VDATE variant even though FORMAT_SYNTAX and FILE_NAME_FORMAT_SYNTAX regexes now allow omitting the <date format>; update the suggestion entries used for autocomplete to include the no-format forms (e.g., add "{{vdate:<variable name>}}" alongside the existing "{{vdate:<variable name>, <date format>}}" and "{{vdate:<variable name>, <date format>|<default value>}}") wherever FORMAT_SYNTAX and FILE_NAME_FORMAT_SYNTAX suggestion lists are defined so users can discover the valid single-argument form.
🧹 Nitpick comments (1)
src/formatters/formatter.ts (1)
553-650: 💤 Low valueDead
else { break; }branch — optional cleanupAfter the explicit
if (!variableName) breakguard on lines 553–555,variableNameis guaranteed to be a non-empty string when execution reaches line 557. The outerif (variableName) { ... } else { break; }wrapper is therefore dead code.♻️ Proposed refactor
- if (!variableName) { - break; - } - - if (variableName) { - const existingValue = this.variables.get(variableName); - // ... (rest of processing block) - output = output.replace(match[0], formattedDate); - } else { - break; - } + if (!variableName) { + break; + } + + const existingValue = this.variables.get(variableName); + // ... (rest of processing block) + output = output.replace(match[0], formattedDate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/formatters/formatter.ts` around lines 553 - 650, The outer redundant guard around the VDATE handling creates a dead else branch; remove the wrapping "if (variableName) { ... } else { break; }" so the existing early check "if (!variableName) break;" is the sole guard and the VDATE logic (references: variableName, this.variables.get, this.promptForVariable, this.dateParser, normalizeDateInput, parseDate, storedValue, formattedDate, output.replace(match[0], ...)) is unwrapped one level inward — just delete the else { break; } and its matching if(...) wrapper so the code inside executes directly after the initial guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/constants.ts`:
- Around line 29-30: The suggestion arrays for date formatting omit the
one-argument VDATE variant even though FORMAT_SYNTAX and FILE_NAME_FORMAT_SYNTAX
regexes now allow omitting the <date format>; update the suggestion entries used
for autocomplete to include the no-format forms (e.g., add "{{vdate:<variable
name>}}" alongside the existing "{{vdate:<variable name>, <date format>}}" and
"{{vdate:<variable name>, <date format>|<default value>}}") wherever
FORMAT_SYNTAX and FILE_NAME_FORMAT_SYNTAX suggestion lists are defined so users
can discover the valid single-argument form.
---
Nitpick comments:
In `@src/formatters/formatter.ts`:
- Around line 553-650: The outer redundant guard around the VDATE handling
creates a dead else branch; remove the wrapping "if (variableName) { ... } else
{ break; }" so the existing early check "if (!variableName) break;" is the sole
guard and the VDATE logic (references: variableName, this.variables.get,
this.promptForVariable, this.dateParser, normalizeDateInput, parseDate,
storedValue, formattedDate, output.replace(match[0], ...)) is unwrapped one
level inward — just delete the else { break; } and its matching if(...) wrapper
so the code inside executes directly after the initial guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b32a389-ab15-4405-9a3d-62a4e8fd1723
📒 Files selected for processing (6)
src/constants.tssrc/formatters/completeFormatter.tssrc/formatters/formatter.tssrc/formatters/vdate-default.test.tssrc/preflight/RequirementCollector.test.tssrc/preflight/RequirementCollector.ts
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
chhoumann
deleted the
1178-bug-vdate-breaks-one-page-input-modal-falls-back-to-sequential-prompts
branch
1 participant
Summary
Fixes #1178.
Validation
obsidian vault=dev ...: validated explicit, no-format, and filename VDATE fixtures open one-page input and resolve output without dev errorsSummary by CodeRabbit