Fix VALUE variables from scripts reaching templates #1060

chhoumann · 2025-12-21T12:46:05Z

Summary

Preserve script-provided VALUE variables through formatting and preflight resolution.
Make VALUE lookup tolerant of label-scoped keys and case-only mismatches (unique only).
Stop quickAddApi.format() from wiping preexisting variables by restoring snapshots instead of clearing.

Problem

Scripts can set QuickAdd.variables/params.variables correctly, but subsequent Template steps still prompt for {{VALUE:...}} placeholders. Two failure modes were observed:

Key mismatches introduced by label-scoped VALUE tokens (and occasional case drift).
Helper formatting calls (quickAddApi.format) clearing the shared variables map.

Fix

VALUE lookup now resolves an existing variable key by trying:
- exact key
- base key for label-scoped VALUE lists
- case-insensitive unique match
VALUE "concreteness" treats null as intentional; only undefined is considered missing.
One-page preflight uses the same resolver so it won’t re-ask already-provided values.
quickAddApi.format() snapshots variables and restores them when shouldClearVariables is true (preserves pre-existing script values while still cleaning up format-local additions).

Behavior changes / Notes

null is now treated as an explicit value (no prompt). To force prompting, set the variable to undefined or delete it.
Case-insensitive fallback only applies when there is a single unique match; ambiguous matches still prompt.

Test Plan

bun run test

Issue

Fixes #1059

Summary by CodeRabbit

Bug Fixes
- Variables with null values no longer trigger input prompts; they are now treated as valid concrete values.
- Improved variable key resolution with case-insensitive matching and base key fallback support.
- Enhanced variable state preservation during quick add formatting operations.
Tests
- Updated test expectations for null value handling in formatter.
- Added comprehensive test coverage for variable key resolution scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

vercel · 2025-12-21T12:46:10Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
quickadd	Ready	Preview	Dec 21, 2025 0:46am

coderabbitai · 2025-12-21T12:46:15Z

Walkthrough

This PR fixes a bug where variables set in user scripts were not being passed to the template phase. It introduces key resolution logic to handle variable lookups, treats null values as concrete (non-prompting) variables, adds snapshot/restore functionality for variable state, and integrates these changes across the formatter, preflight, and API layers.

Changes

Cohort / File(s)	Summary
Variable Key Resolution `src/utils/valueSyntax.ts`, `src/utils/valueSyntax.test.ts`	Introduces `resolveExistingVariableKey()` function to resolve variable keys by checking exact key match, base key derivation, and case-insensitive matching. Includes internal helper `findCaseInsensitiveMatch()`. Adds comprehensive test coverage for exact match, labeled token fallback, case-insensitive resolution, ambiguity handling, and undefined vs. null distinction.
Formatter Integration `src/formatters/formatter.ts`, `src/formatters/formatter.test.ts`, `src/formatters/formatter-issue929-integration.test.ts`	Integrates `resolveExistingVariableKey` into variable lookup flow; uses resolved key for all subsequent retrievals. Changes `hasConcreteVariable` logic to treat only undefined as non-concrete, making null values concrete. Modifies value handling to convert null to empty string when pre-set. Updates test expectations for null value behavior and script-provided null title handling.
Preflight Input Detection `src/preflight/runOnePagePreflight.ts`	Updates unresolved input detection to use `resolveExistingVariableKey()` instead of direct undefined/null checks, aligning criteria with new key-resolution semantics.
Variable Snapshot/Restore `src/quickAddApi.ts`	Adds `snapshotVariables()` and `restoreVariables()` helpers to capture and restore Map state. Modifies `QuickAddApi.format()` to snapshot variables when `shouldClearVariables` is true and restore from snapshot post-formatting instead of always clearing.

Sequence Diagram

sequenceDiagram
    participant Script as User Script
    participant QA as QuickAddApi
    participant Fmt as Formatter
    participant VS as Value Syntax
    participant Template as Templater

    Script->>QA: Set variables (e.g., title, backdrop)
    QA->>QA: Snapshot variables (if shouldClearVariables)
    QA->>Fmt: format(template, variables)
    Fmt->>Fmt: Process {{VALUE:*}} tokens
    Fmt->>VS: resolveExistingVariableKey(variables, key)
    VS->>VS: Check exact match
    alt No exact match
        VS->>VS: Derive base key, try case-insensitive
    end
    VS-->>Fmt: Return resolved key (or null)
    alt Resolved key found
        Fmt->>Fmt: Use resolved key for lookup
        Fmt->>Fmt: hasConcreteVariable (null = concrete)
        Fmt-->>QA: Substituted value (no prompt)
    else No resolved key
        Fmt-->>QA: Prompt user
    end
    alt shouldClearVariables was true
        QA->>QA: Restore from snapshot
    end
    QA-->>Template: Fully formatted template
    Template->>Template: Render note

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

New key resolution algorithm: resolveExistingVariableKey() introduces multiple matching strategies (exact, base-key, case-insensitive); verify logic handles edge cases correctly and null/undefined distinction throughout.
Null semantics change: Reversal of null-as-concrete treatment affects multiple codepaths; confirm all hasConcreteVariable usages and prompt-triggering logic are correctly updated.
Variable snapshot/restore flow: Ensure snapshot is taken at the right point and restore correctly restores state without side effects.
Integration consistency: Verify resolveExistingVariableKey is used uniformly across formatter.ts, preflight, and other lookups; check for any missed callsites still using raw keys.

Possibly related PRs

PR #1048: Introduces value-token parsing and labeled placeholders (parseValueToken), which directly precedes the key-resolution logic added in this PR; changes to variable-key handling and hasConcreteVariable logic build on those foundations.

Poem

🐰 A rabbit hops through variable lands,
Where null and undefined hold many hands,
With snapshot spells and keys resolved with care,
Variables now flow—from script to template, fair! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix VALUE variables from scripts reaching templates' clearly describes the main change: ensuring script-provided VALUE variables are properly passed through to templates.
Linked Issues check	✅ Passed	All code changes address the requirements from issue #1059: VALUE lookup resolution [valueSyntax.ts], null-as-explicit-value semantics [formatter.ts], preflight integration [runOnePagePreflight.ts], and variable preservation [quickAddApi.ts].
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing VALUE variable resolution: new resolveExistingVariableKey function, updated formatter logic for null handling, preflight integration, and variable snapshot/restore in quickAddApi.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch 1059

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/utils/valueSyntax.ts (1)
50-70: Key resolution function correctly implements the described lookup order.

The implementation follows the PR's resolution strategy:

Exact key match (with non-undefined value)

Base key fallback for label-scoped tokens

Case-insensitive unique match

The guard for empty variableKey at line 54 prevents unnecessary processing.
Minor: Consider caching the Map.get() result

Line 61 calls both vars.has(candidate) and vars.get(candidate), resulting in two lookups. You could slightly optimize by using only get() and checking against undefined:
 for (const candidate of candidates) {
-    if (vars.has(candidate) && vars.get(candidate) !== undefined) {
+    const value = vars.get(candidate);
+    if (value !== undefined) {
         return candidate;
     }
This is a minor optimization and the current code is perfectly correct.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6c261a and d44e7ea.

📒 Files selected for processing (7)

src/formatters/formatter-issue929-integration.test.ts (1 hunks)
src/formatters/formatter.test.ts (3 hunks)
src/formatters/formatter.ts (6 hunks)
src/preflight/runOnePagePreflight.ts (2 hunks)
src/quickAddApi.ts (3 hunks)
src/utils/valueSyntax.test.ts (2 hunks)
src/utils/valueSyntax.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{ts,tsx,mts,mjs,js,json}