fix(markdown-template): preserve parent scope in conditional and optional blocks by yashhzd · Pull Request #647 · accordproject/markdown-transform

yashhzd · 2026-02-26T01:50:17Z

Description

Fixes a bug where variables inside {{#if}} conditional blocks and {{#optional}} blocks with primitive types would fail with "Unknown property" errors during type-checking.

Root cause: ConditionalDefinition and OptionalDefinition (for primitive types) incorrectly changed the model scope to the condition/optional property itself, rather than preserving the parent class scope. This meant child variable references like {{age}} inside {{#if age}}...{{/if}} could not resolve against the parent model.

Closes accordproject/template-playground#2

Changes

TypeVisitor.js

ConditionalDefinition — Pass currentModel (parent scope) instead of property to whenTrue children. Conditional blocks are guards — they should not change variable resolution scope.
OptionalDefinition — For primitive optional properties, keep the parent model scope (currentModel) instead of narrowing to the property. Complex (object) optional types still correctly change scope to the nested class declaration.
VariableDefinition {{this}} handler — Add type guard (typeof property.isPrimitive === 'function') to prevent crash when currentModel is a ClassDeclaration (which doesn't have isPrimitive()).

Tests

Added 6 new test cases:

3 for conditional blocks (#if): named variables, multiple variables, unknown variable rejection
3 for optional blocks (#optional): primitive named variables, different variable types, unknown variable rejection

All 13 tests pass (7 existing + 6 new).

Author Checklist

Code compiles and passes tests
New tests added for changed behavior
Signed-off-by for DCO

…onal blocks ConditionalDefinition (#if) and OptionalDefinition (#optional) blocks with primitive types incorrectly changed the model scope to the condition property, preventing child variables from resolving against the parent class declaration. This caused 'Unknown property' errors when using named variables like {{age}} inside {{#if age}}...{{/if}} blocks. Changes: - ConditionalDefinition: pass currentModel instead of property to whenTrue children so variables resolve against the parent scope - OptionalDefinition: for primitive types, keep parent model scope instead of narrowing to the property - VariableDefinition: add type guard for {{this}} handler to prevent crash when currentModel is a ClassDeclaration Closes accordproject/template-playground#2 Signed-off-by: Yash Goel <162511050+yashhzd@users.noreply.github.com>

Copilot

Pull request overview

Fixes TemplateMark type-checking scope resolution so variables inside {{#if}} and primitive {{#optional}} blocks resolve against the parent model (avoiding “Unknown property” errors).

Changes:

Update TypeVisitor to preserve parent scope for ConditionalDefinition children and primitive OptionalDefinition blocks.
Add an additional type guard in the {{this}} handler to avoid crashes when the current model is a ClassDeclaration.
Add new tests covering variable resolution within #if and #optional blocks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
packages/markdown-template/src/TypeVisitor.js	Adjusts scope handling for `#if` and `#optional` during typing; adds a guard in the `{{this}}` handler.
packages/markdown-template/lib/TypeVisitor.js	Transpiled/built output reflecting the `TypeVisitor` changes.
packages/markdown-template/test/TemplateMarkTransformer.js	Adds test cases for variable resolution in `#if` and `#optional` blocks.

Comments suppressed due to low confidence (1)

packages/markdown-template/lib/TypeVisitor.js:323

ConditionalDefinition keeps model: currentModel for whenTrue, but the {{else}} branch (whenFalse) is still visited with model: null just below. That will make any {{variable}} inside {{else}} fail type-checking, even though runtime conversion evaluates both branches with the same data scope. Consider passing currentModel for whenFalse as well (and regenerate lib/ from src/ after the fix).

          TypeVisitor.visitChildren(this, thing, {
            templateMarkModelManager: parameters.templateMarkModelManager,
            introspector: parameters.introspector,
            model: null,
            kind: parameters.kind

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-07T11:05:27Z

packages/markdown-template/src/TypeVisitor.js

                templateMarkModelManager:parameters.templateMarkModelManager,
                introspector:parameters.introspector,
-                model:nextModel,
+                model:currentModel,
                kind:parameters.kind
            }, 'whenTrue');


ConditionalDefinition now correctly keeps model: currentModel for the whenTrue branch, but the {{else}} branch (whenFalse) is still visited with model: null later in this case. That makes any {{variable}} inside {{else}} fail type-checking, even though runtime conversion (ToCiceroMarkVisitor) evaluates both branches with the same parameters.data. Consider passing currentModel for whenFalse as well so variable resolution is consistent across both branches.

Copilot · 2026-03-07T11:05:27Z

packages/markdown-template/src/TypeVisitor.js

            if (property.isPrimitive()) {
                thing.elementType = property.getFullyQualifiedTypeName();
-                nextModel = property;
+                // For primitive optional properties, keep the parent model scope
+                // so that named variables (e.g. {{age}}) can resolve correctly.
+                // The property itself is passed as parentModel for {{this}} fallback.


In OptionalDefinition, primitive optionals now keep nextModel = currentModel so named vars resolve against the parent. However, that also changes how {{this}} is typed inside the #optional block: VariableDefinition treats this as the current model, which is now a ClassDeclaration, so {{this}} will be annotated as the parent class type instead of the optional property type. If templates rely on {{this}} inside {{#optional <primitive>}}...{{/optional}} to mean the optional value, consider passing the optional property as a separate parameter for the {{this}} handler (and add a regression test). Also, the {{else}} branch (whenNone) is still visited with model: null later, so variables in {{else}} will still be rejected during type-checking.

Copilot · 2026-03-07T11:05:27Z

packages/markdown-template/src/TypeVisitor.js

+                // For primitive optional properties, keep the parent model scope
+                // so that named variables (e.g. {{age}}) can resolve correctly.
+                // The property itself is passed as parentModel for {{this}} fallback.


The comment "The property itself is passed as parentModel for {{this}} fallback" is currently misleading: visitChildren is only passed { model: nextModel, ... }, and no parentModel (or similar) parameter exists/gets forwarded. Either implement the separate context for {{this}} (as the comment implies) or adjust/remove the comment to match the actual behavior.

Suggested change

// For primitive optional properties, keep the parent model scope

// so that named variables (e.g. {{age}}) can resolve correctly.

// The property itself is passed as parentModel for {{this}} fallback.

// For primitive optional properties, keep the parent model as the

// current scope so that named variables (e.g. {{age}}) resolve

// against the same model; no separate parentModel context is used.

Copilot · 2026-03-07T11:05:28Z

packages/markdown-template/lib/TypeVisitor.js

+            // For primitive optional properties, keep the parent model scope
+            // so that named variables (e.g. {{age}}) can resolve correctly.
+            // The property itself is passed as parentModel for {{this}} fallback.
+            _nextModel2 = currentModel;


For primitive OptionalDefinition, switching _nextModel2 = currentModel fixes named variable resolution but also changes {{this}} typing inside the optional block: the {{this}} handler uses the current model, which is now the parent class, so {{this}} gets annotated as the parent class type instead of the optional property type. Also, the {{else}} branch (whenNone) is still visited with model: null later, so variables in {{else}} will still be rejected during type-checking. If {{this}} is expected to mean the optional value, pass the property as a separate parameter for {{this}} and add a regression test; then regenerate lib/ from src/.

Suggested change

// For primitive optional properties, keep the parent model scope

// so that named variables (e.g. {{age}}) can resolve correctly.

// The property itself is passed as parentModel for {{this}} fallback.

_nextModel2 = currentModel;

// For primitive optional properties, do not change the scope model.

// This avoids annotating {{this}} as the parent class type; instead,

// type information for {{this}} should come from thing.elementType.

_nextModel2 = null;

sanketshevkar · 2026-03-07T11:10:22Z

Hi @yashhzd, I see a very deep root cause analysis done by @Shubh-Raj and this two PRs you raised just implements those changes.

I'm closing this and the other PR for now. I'm not sure you'd had an offline conversation with him about this. it'll be better to ask him first if he's okay with this. I'd suggest please start a conversation on the issue and ask @Shubh-Raj for this. Let us give him a week to respond, if he doesn't if we can look into reviewing and merging the PRs.

Shubh-Raj · 2026-03-07T18:51:37Z

Hi @yashhzd, I see a very deep root cause analysis done by @Shubh-Raj and this two PRs you raised just implements those changes.

I'm closing this and the other PR for now. I'm not sure you'd had an offline conversation with him about this. it'll be better to ask him first if he's okay with this. I'd suggest please start a conversation on the issue and ask @Shubh-Raj for this. Let us give him a week to respond, if he doesn't if we can look into reviewing and merging the PRs.

Thanks @sanketshevkar for handling this fairly! Happy to review @yashhzd's PRs and collaborate on getting the best fix merged.

github-actions bot mentioned this pull request Mar 4, 2026

Monthly issue metrics report accordproject/.github#1

Open

sanketshevkar requested a review from Copilot March 7, 2026 10:58

Copilot started reviewing on behalf of sanketshevkar March 7, 2026 10:59 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

sanketshevkar closed this Mar 7, 2026

sanketshevkar reopened this Mar 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(markdown-template): preserve parent scope in conditional and optional blocks#647

fix(markdown-template): preserve parent scope in conditional and optional blocks#647
yashhzd wants to merge 1 commit intoaccordproject:mainfrom
yashhzd:yashhzd/i2/fix-variables-in-conditional-optional-blocks

yashhzd commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

sanketshevkar commented Mar 7, 2026

Uh oh!

Shubh-Raj commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

yashhzd commented Feb 26, 2026

Description

Changes

TypeVisitor.js

Tests

Related

Author Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

sanketshevkar commented Mar 7, 2026

Uh oh!

Shubh-Raj commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants