From df641e8d3d55979b991587e9b87a93f202869fa2 Mon Sep 17 00:00:00 2001
From: Marco Roth <marco.roth@intergga.ch>
Date: Mon, 25 Aug 2025 21:09:43 +0200
Subject: [PATCH 1/2] Linter: Introduce `ControlFlowTrackingVisitor` to handle
 control flow

---
 .../linter/src/rules/html-no-duplicate-ids.ts | 201 +++++++-
 .../packages/linter/src/rules/rule-utils.ts   | 104 ++++
 .../test/rules/html-no-duplicate-ids.test.ts  | 488 +++++++++++++++++-
 3 files changed, 764 insertions(+), 29 deletions(-)

diff --git a/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts b/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts
index 169c217c4..7fe277d0d 100644
--- a/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts
+++ b/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts
@@ -1,29 +1,202 @@
 import { ParserRule } from "../types"
-import { AttributeVisitorMixin, StaticAttributeStaticValueParams } from "./rule-utils"
+import { ControlFlowTrackingVisitor, ControlFlowType, printNodes } from "./rule-utils"
+import { LiteralNode } from "@herb-tools/core"
 
-import type { ParseResult } from "@herb-tools/core"
+import { hasERBOutput, getValidatableStaticContent, isEffectivelyStatic, isNode, areAllOfType, getStaticAttributeName } from "@herb-tools/core"
+
+import type { ParseResult, HTMLAttributeNode } from "@herb-tools/core"
 import type { LintOffense, LintContext } from "../types"
 
-class NoDuplicateIdsVisitor extends AttributeVisitorMixin {
+interface ControlFlowState {
+  previousBranchIds: Set<string>
+  previousControlFlowIds: Set<string>
+}
+
+interface BranchState {
+  previousBranchIds: Set<string>
+}
+
+class NoDuplicateIdsVisitor extends ControlFlowTrackingVisitor<ControlFlowState, BranchState> {
   private documentIds: Set<string> = new Set<string>()
+  private currentBranchIds: Set<string> = new Set<string>()
+  private controlFlowIds: Set<string> = new Set<string>()
+
+  visitHTMLAttributeNode(node: HTMLAttributeNode): void {
+    this.checkAttribute(node)
+  }
+
+  protected onEnterControlFlow(_controlFlowType: ControlFlowType, wasAlreadyInControlFlow: boolean): ControlFlowState {
+    const stateToRestore: ControlFlowState = {
+      previousBranchIds: this.currentBranchIds,
+      previousControlFlowIds: this.controlFlowIds
+    }
+
+    this.currentBranchIds = new Set<string>()
+
+    if (!wasAlreadyInControlFlow) {
+      this.controlFlowIds = new Set<string>()
+    }
+
+    return stateToRestore
+  }
+
+  protected onExitControlFlow(controlFlowType: ControlFlowType, wasAlreadyInControlFlow: boolean, stateToRestore: ControlFlowState): void {
+    if (controlFlowType === ControlFlowType.CONDITIONAL && !wasAlreadyInControlFlow) {
+      this.controlFlowIds.forEach(id => this.documentIds.add(id))
+    }
+
+    this.currentBranchIds = stateToRestore.previousBranchIds
+    this.controlFlowIds = stateToRestore.previousControlFlowIds
+  }
+
+  protected onEnterBranch(): BranchState {
+    const stateToRestore: BranchState = {
+      previousBranchIds: this.currentBranchIds
+    }
+
+    if (this.isInControlFlow) {
+      this.currentBranchIds = new Set<string>()
+    }
+
+    return stateToRestore
+  }
+
+  protected onExitBranch(_stateToRestore: BranchState): void {}
+
+  private checkAttribute(attributeNode: HTMLAttributeNode): void {
+    if (!this.isIdAttribute(attributeNode)) return
+
+    const idValue = this.extractIdValue(attributeNode)
+
+    if (!idValue) return
+    if (this.isWhitespaceOnlyId(idValue.identifier)) return
+
+    this.processIdDuplicate(idValue, attributeNode)
+  }
+
+  private isIdAttribute(attributeNode: HTMLAttributeNode): boolean {
+    if (!attributeNode.name?.children || !attributeNode.value) return false
+
+    return getStaticAttributeName(attributeNode.name) === "id"
+  }
+
+  private extractIdValue(attributeNode: HTMLAttributeNode): { identifier: string; shouldTrackDuplicates: boolean } | null {
+    const valueNodes = attributeNode.value!.children
+    const isCompletelyStatic = areAllOfType(valueNodes, LiteralNode)
+    const isEffectivelyStaticValue = isEffectivelyStatic(valueNodes)
+
+    if (hasERBOutput(valueNodes) && this.isInControlFlow && this.currentControlFlowType === ControlFlowType.LOOP) {
+      return null
+    }
+
+    let identifier: string
 
-  protected checkStaticAttributeStaticValue({ attributeName, attributeValue, attributeNode }: StaticAttributeStaticValueParams): void {
-    if (attributeName.toLowerCase() !== "id") return
-    if (!attributeValue) return
+    if (isCompletelyStatic) {
+      identifier = printNodes(valueNodes)
+    } else if (isEffectivelyStaticValue) {
+      const staticContent = getValidatableStaticContent(valueNodes)
+      if (!staticContent) return null
 
-    const id = attributeValue.trim()
+      identifier = staticContent
+    } else {
+      identifier = printNodes(valueNodes)
+    }
+
+    return { identifier, shouldTrackDuplicates: true }
+  }
+
+  private isWhitespaceOnlyId(identifier: string): boolean {
+    return identifier !== '' && identifier.trim() === ''
+  }
 
-    if (this.documentIds.has(id)) {
-      this.addOffense(
-        `Duplicate ID \`${id}\` found. IDs must be unique within a document.`,
-        attributeNode.location,
-        "error"
-      )
+  private processIdDuplicate(idValue: { identifier: string; shouldTrackDuplicates: boolean }, attributeNode: HTMLAttributeNode): void {
+    const { identifier, shouldTrackDuplicates } = idValue
 
+    if (!shouldTrackDuplicates) return
+
+    if (this.isInControlFlow) {
+      this.handleControlFlowId(identifier, attributeNode)
+    } else {
+      this.handleGlobalId(identifier, attributeNode)
+    }
+  }
+
+  private handleControlFlowId(identifier: string, attributeNode: HTMLAttributeNode): void {
+    if (this.currentControlFlowType === ControlFlowType.LOOP) {
+      this.handleLoopId(identifier, attributeNode)
+    } else {
+      this.handleConditionalId(identifier, attributeNode)
+    }
+
+    this.currentBranchIds.add(identifier)
+  }
+
+  private handleLoopId(identifier: string, attributeNode: HTMLAttributeNode): void {
+    const isStaticId = this.isStaticId(attributeNode)
+
+    if (isStaticId) {
+      this.addDuplicateIdOffense(identifier, attributeNode.location)
       return
     }
 
-    this.documentIds.add(id)
+    if (this.currentBranchIds.has(identifier)) {
+      this.addSameLoopIterationOffense(identifier, attributeNode.location)
+    }
+  }
+
+  private handleConditionalId(identifier: string, attributeNode: HTMLAttributeNode): void {
+    if (this.currentBranchIds.has(identifier)) {
+      this.addSameBranchOffense(identifier, attributeNode.location)
+      return
+    }
+
+    if (this.documentIds.has(identifier)) {
+      this.addDuplicateIdOffense(identifier, attributeNode.location)
+      return
+    }
+
+    this.controlFlowIds.add(identifier)
+  }
+
+  private handleGlobalId(identifier: string, attributeNode: HTMLAttributeNode): void {
+    if (this.documentIds.has(identifier)) {
+      this.addDuplicateIdOffense(identifier, attributeNode.location)
+      return
+    }
+
+    this.documentIds.add(identifier)
+  }
+
+  private isStaticId(attributeNode: HTMLAttributeNode): boolean {
+    const valueNodes = attributeNode.value!.children
+    const isCompletelyStatic = valueNodes.every(child => isNode(child, LiteralNode))
+    const isEffectivelyStaticValue = isEffectivelyStatic(valueNodes)
+
+    return isCompletelyStatic || isEffectivelyStaticValue
+  }
+
+  private addDuplicateIdOffense(identifier: string, location: any): void {
+    this.addOffense(
+      `Duplicate ID \`${identifier}\` found. IDs must be unique within a document.`,
+      location,
+      "error"
+    )
+  }
+
+  private addSameLoopIterationOffense(identifier: string, location: any): void {
+    this.addOffense(
+      `Duplicate ID \`${identifier}\` found within the same loop iteration. IDs must be unique within the same loop iteration.`,
+      location,
+      "error"
+    )
+  }
+
+  private addSameBranchOffense(identifier: string, location: any): void {
+    this.addOffense(
+      `Duplicate ID \`${identifier}\` found within the same control flow branch. IDs must be unique within the same control flow branch.`,
+      location,
+      "error"
+    )
   }
 }
 
diff --git a/javascript/packages/linter/src/rules/rule-utils.ts b/javascript/packages/linter/src/rules/rule-utils.ts
index dad05cdf4..3d97f57e3 100644
--- a/javascript/packages/linter/src/rules/rule-utils.ts
+++ b/javascript/packages/linter/src/rules/rule-utils.ts
@@ -24,10 +24,18 @@ import type {
   Node
 } from "@herb-tools/core"
 
+import { IdentityPrinter } from "@herb-tools/printer"
+
 import { DEFAULT_LINT_CONTEXT } from "../types.js"
 
+import type * as Nodes from "@herb-tools/core"
 import type { LintOffense, LintSeverity, LintContext } from "../types.js"
 
+export enum ControlFlowType {
+  CONDITIONAL,
+  LOOP
+}
+
 /**
  * Base visitor class that provides common functionality for rule visitors
  */
@@ -65,6 +73,95 @@ export abstract class BaseRuleVisitor extends Visitor {
   }
 }
 
+/**
+ * Mixin that adds control flow tracking capabilities to rule visitors
+ * This allows rules to track state across different control flow structures
+ * like if/else branches, loops, etc.
+ *
+ * @template TControlFlowState - Type for state passed between onEnterControlFlow and onExitControlFlow
+ * @template TBranchState - Type for state passed between onEnterBranch and onExitBranch
+ */
+export abstract class ControlFlowTrackingVisitor<TControlFlowState = any, TBranchState = any> extends BaseRuleVisitor {
+  protected isInControlFlow: boolean = false
+  protected currentControlFlowType: ControlFlowType | null = null
+
+  /**
+   * Handle visiting a control flow node with proper scope management
+   */
+  protected handleControlFlowNode(node: Node, controlFlowType: ControlFlowType, visitChildren: () => void): void {
+    const wasInControlFlow = this.isInControlFlow
+    const previousControlFlowType = this.currentControlFlowType
+
+    this.isInControlFlow = true
+    this.currentControlFlowType = controlFlowType
+
+    const stateToRestore = this.onEnterControlFlow(controlFlowType, wasInControlFlow)
+
+    visitChildren()
+
+    this.onExitControlFlow(controlFlowType, wasInControlFlow, stateToRestore)
+
+    this.isInControlFlow = wasInControlFlow
+    this.currentControlFlowType = previousControlFlowType
+  }
+
+  /**
+   * Handle visiting a branch node (like else, when) with proper scope management
+   */
+  protected startNewBranch(visitChildren: () => void): void {
+    const stateToRestore = this.onEnterBranch()
+
+    visitChildren()
+
+    this.onExitBranch(stateToRestore)
+  }
+
+  visitERBIfNode(node: Nodes.ERBIfNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.CONDITIONAL, () => super.visitERBIfNode(node))
+  }
+
+  visitERBUnlessNode(node: Nodes.ERBUnlessNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.CONDITIONAL, () => super.visitERBUnlessNode(node))
+  }
+
+  visitERBCaseNode(node: Nodes.ERBCaseNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.CONDITIONAL, () => super.visitERBCaseNode(node))
+  }
+
+  visitERBCaseMatchNode(node: Nodes.ERBCaseMatchNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.CONDITIONAL, () => super.visitERBCaseMatchNode(node))
+  }
+
+  visitERBWhileNode(node: Nodes.ERBWhileNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.LOOP, () => super.visitERBWhileNode(node))
+  }
+
+  visitERBForNode(node: Nodes.ERBForNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.LOOP, () => super.visitERBForNode(node))
+  }
+
+  visitERBUntilNode(node: Nodes.ERBUntilNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.LOOP, () => super.visitERBUntilNode(node))
+  }
+
+  visitERBBlockNode(node: Nodes.ERBBlockNode): void {
+    this.handleControlFlowNode(node, ControlFlowType.CONDITIONAL, () => super.visitERBBlockNode(node))
+  }
+
+  visitERBElseNode(node: Nodes.ERBElseNode): void {
+    this.startNewBranch(() => super.visitERBElseNode(node))
+  }
+
+  visitERBWhenNode(node: Nodes.ERBWhenNode): void {
+    this.startNewBranch(() => super.visitERBWhenNode(node))
+  }
+
+  protected abstract onEnterControlFlow(controlFlowType: ControlFlowType, wasAlreadyInControlFlow: boolean): TControlFlowState
+  protected abstract onExitControlFlow(controlFlowType: ControlFlowType, wasAlreadyInControlFlow: boolean, stateToRestore: TControlFlowState): void
+  protected abstract onEnterBranch(): TBranchState
+  protected abstract onExitBranch(stateToRestore: TBranchState): void
+}
+
 /**
  * Gets attributes from an HTMLOpenTagNode
  */
@@ -756,3 +853,10 @@ export abstract class BaseSourceRuleVisitor {
     return new Location(start, end)
   }
 }
+
+export function printNodes(nodes: Node[]): string {
+  if (!nodes) return ""
+  if (!Array.isArray(nodes)) return ""
+
+  return nodes.map(node => IdentityPrinter.print(node)).join("")
+}
diff --git a/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts b/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts
index 11f2e6529..9c702a5b6 100644
--- a/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts
+++ b/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts
@@ -53,6 +53,18 @@ describe("html-no-duplicate-ids", () => {
     expect(lintResult.offenses).toHaveLength(0);
   })
 
+  test("fails for two IDs without value", () => {
+    const html = '<div id=""></div><span id=""></span>';
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `` found. IDs must be unique within a document.');
+  })
+
   test("passes for other attributes with equal value", () => {
     const html = '<div class="value"></div><div class="value"></div>';
     const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
@@ -73,9 +85,28 @@ describe("html-no-duplicate-ids", () => {
     expect(lintResult.offenses).toHaveLength(0);
   })
 
-  // TODO: see next test, in the future this should also warn if it's in the same "context"
-  test("passes when using ERB for two IDs", () => {
-    const html = '<div id="<%= user.id %>"></div><div id="<%= user.id %>"></div>';
+  // TODO: this should also warn if it's in the same "context"
+  test.todo("fails for multiple duplicate IDs in ERB in the same context", () => {
+    const html = '<div id="<%= user.id %>"></div><span id="<%= user.id %>"></span>';
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `<%= user.id %>` found. IDs must be unique within a document.');
+  })
+
+  test("passes for IDs in mutually exclusive if/else branches", () => {
+    const html = dedent`
+      <% if some_condition? %>
+        <span id="my-id">content1</span>
+      <% else %>
+        <span id="my-id">content2</span>
+      <% end %>
+    `
+
     const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
     const lintResult = linter.lint(html);
 
@@ -84,8 +115,99 @@ describe("html-no-duplicate-ids", () => {
     expect(lintResult.offenses).toHaveLength(0);
   })
 
-  test.todo("fails for multiple duplicate IDs in ERB in the same context", () => {
-    const html = '<div id="<%= user.id %>"></div><span id="<%= user.id %>"></span>';
+  test("passes for IDs in mutually exclusive unless/else branches", () => {
+    const html = dedent`
+      <% unless some_condition? %>
+        <span id="my-id">content1</span>
+      <% else %>
+        <span id="my-id">content2</span>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("fails for IDs in mutually exclusive unless/else branches and global", () => {
+    const html = dedent`
+      <span id="my-id">content</span>
+
+      <% unless some_condition? %>
+        <span id="my-id">content1</span>
+      <% else %>
+        <span id="my-id">content2</span>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(2);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(2);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `my-id` found. IDs must be unique within a document.');
+    expect(lintResult.offenses[1].message).toBe('Duplicate ID `my-id` found. IDs must be unique within a document.');
+  })
+
+  test("passes for IDs in mutually exclusive case/when branches", () => {
+    const html = dedent`
+      <% case status %>
+      <% when 'active' %>
+        <div id="status-indicator">Active</div>
+      <% when 'inactive' %>
+        <div id="status-indicator">Inactive</div>
+      <% else %>
+        <div id="status-indicator">Unknown</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("fails for IDs in mutually exclusive case/when branches and global", () => {
+    const html = dedent`
+      <div id="status-indicator">Active</div>
+
+      <% case status %>
+      <% when 'active' %>
+        <div id="status-indicator">Active</div>
+      <% when 'inactive' %>
+        <div id="status-indicator">Inactive</div>
+      <% else %>
+        <div id="status-indicator">Unknown</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(3);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(3);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `status-indicator` found. IDs must be unique within a document.');
+    expect(lintResult.offenses[1].message).toBe('Duplicate ID `status-indicator` found. IDs must be unique within a document.');
+    expect(lintResult.offenses[2].message).toBe('Duplicate ID `status-indicator` found. IDs must be unique within a document.');
+  })
+
+  test("fails for duplicate IDs within same control flow branch", () => {
+    const html = dedent`
+      <% if some_condition? %>
+        <span id="duplicate-in-branch">content1</span>
+        <span id="duplicate-in-branch">content2</span>
+      <% end %>
+    `
+
     const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
     const lintResult = linter.lint(html);
 
@@ -93,19 +215,342 @@ describe("html-no-duplicate-ids", () => {
     expect(lintResult.warnings).toBe(0);
     expect(lintResult.offenses).toHaveLength(1);
 
-    expect(lintResult.offenses[0].rule).toBe("html-no-duplicate-ids");
-    expect(lintResult.offenses[0].message).toBe('Duplicate ID `<%= user.id %>` found. IDs must be unique within a document.');
-    expect(lintResult.offenses[0].severity).toBe("error");
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `duplicate-in-branch` found within the same control flow branch. IDs must be unique within the same control flow branch.');
   })
 
-  test.skip("passes for dynamic attribute in a loop context", () => {
+  test("fails for IDs duplicated outside of control flow", () => {
     const html = dedent`
-      <% @users.each do |user| %>
-        <div id="<%= user.id %>"></div>
+      <div id="global-duplicate">outside</div>
+
+      <% if some_condition? %>
+        <span id="different-id">inside branch</span>
+      <% end %>
+
+      <div id="global-duplicate">outside again</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `global-duplicate` found. IDs must be unique within a document.');
+  })
+
+  test("fails for IDs duplicated outside before control flow", () => {
+    const html = dedent`
+      <div id="global-duplicate">outside</div>
+
+      <% if some_condition? %>
+        <span id="global-duplicate">inside branch</span>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `global-duplicate` found. IDs must be unique within a document.');
+  })
+
+  test("fails for IDs duplicated outside after control flow", () => {
+    const html = dedent`
+      <% if some_condition? %>
+        <span id="global-duplicate">inside branch</span>
+      <% end %>
+
+      <div id="global-duplicate">outside</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `global-duplicate` found. IDs must be unique within a document.');
+  })
+
+  test("fails for IDs duplicated outside after control flow", () => {
+    const html = dedent`
+      <% if some_condition? %>
+        <!-- empty -->
+      <% elsif another_condition? %>
+        <!-- empty -->
+      <% else %>
+        <span id="global-duplicate">inside branch</span>
+      <% end %>
+
+      <div id="global-duplicate">outside</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `global-duplicate` found. IDs must be unique within a document.');
+  })
+
+  test("fails for IDs duplicated outside after control flow", () => {
+    const html = dedent`
+      <% if some_condition? %>
+        <!-- empty -->
+      <% elsif other_condition? %>
+        <span id="global-duplicate">inside branch</span>
+      <% end %>
+
+      <div id="global-duplicate">outside</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `global-duplicate` found. IDs must be unique within a document.');
+  })
+
+  test("passes for IDs duplicated in elsif and else", () => {
+    const html = dedent`
+      <% if some_condition? %>
+        <!-- empty -->
+      <% elsif other_condition? %>
+        <span id="global-duplicate">inside branch</span>
+      <% else other_condition? %>
+        <span id="global-duplicate">inside branch</span>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("handles nested control flow properly", () => {
+    const html = dedent`
+      <% if outer_condition? %>
+        <% if inner_condition? %>
+          <div id="nested-id">inner true</div>
+        <% else %>
+          <div id="nested-id">inner false</div>
+        <% end %>
+      <% else %>
+        <div id="nested-id">outer false</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("passes for ID tag.div (ERBBlockNode)", () => {
+    const html = dedent`
+      <% tag.div do %>
+        <div id="user">User</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("passes for output ERB IDs in loops (unique per iteration)", () => {
+    const html = dedent`
+      <% users.each do |user| %>
+        <div id="user-<%= user.id %>">User</div>
+      <% end %>
+
+      <% users.each do |user| %>
+        <div id="user-<%= user.id %>">User again</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `user-<%= user.id %>` found. IDs must be unique within a document.');
+  })
+
+  test("fails for non-output ERB IDs in loops (same value repeated)", () => {
+    const html = dedent`
+      <% users.each do |user| %>
+        <div id="user-<% 'static' %>">User</div>
+        <div id="user-<% 'static' %>">Duplicate</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `user-` found within the same control flow branch. IDs must be unique within the same control flow branch.');
+  })
+
+  test("passes for output ERB IDs in while loops", () => {
+    const html = dedent`
+      <% counter = 0 %>
+      <% count = 0 %>
+
+      <% while condition %>
+        <div id="item-<%= counter %>">Item</div>
+        <div id="item-<%= count %>">Item</div>
+        <div id="post-<%= counter %>">Post</div>
+        <% counter += 1 %>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("fails for static ID in while loops", () => {
+    const html = dedent`
+      <% while condition %>
+        <div id="static-id">Item</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `static-id` found. IDs must be unique within a document.');
+  })
+
+  test("fails for non-dynamic ID in until loops", () => {
+    const html = dedent`
+      <% until condition %>
+        <div id="static-id">Item</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `static-id` found. IDs must be unique within a document.');
+  })
+
+  test("handles output ERB IDs in conditional flow normally", () => {
+    const html = dedent`
+      <% if condition %>
+        <div id="user-<%= user.id %>">User A</div>
+      <% else %>
+        <div id="user-<%= user.id %>">User B</div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("fails for duplicate output ERB IDs within same conditional branch", () => {
+    const html = dedent`
+      <% if condition %>
+        <div id="user-<%= user.id %>">User A</div>
+        <div id="user-<%= user.id %>">User A duplicate</div>
       <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(1);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(1);
+
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `user-<%= user.id %>` found within the same control flow branch. IDs must be unique within the same control flow branch.');
+  })
+
+  test("passes for static ID conflicting with dynamic ID prefix", () => {
+    const html = dedent`
+      <div id="hello">Static</div>
+      <div id="hello<%= suffix %>">Dynamic</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
 
+  test("passes for dynamic ID conflicting with existing static ID", () => {
+    const html = dedent`
+      <div id="hello<%= suffix %>">Dynamic</div>
+      <div id="hello">Static</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test("passes for non-conflicting static and dynamic IDs", () => {
+    const html = dedent`
+      <div id="hello">Static</div>
+      <div id="goodbye<%= suffix %>">Dynamic</div>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
+  })
+
+  test.todo("fails for static attribute in a loop context", () => {
+    const html = dedent`
       <% @users.each do |user| %>
-        <div id="<%= user.id %>"></div>
+        <div id="user"></div>
       <% end %>
     `
 
@@ -116,8 +561,21 @@ describe("html-no-duplicate-ids", () => {
     expect(lintResult.warnings).toBe(0);
     expect(lintResult.offenses).toHaveLength(1);
 
-    expect(lintResult.offenses[0].rule).toBe("html-no-duplicate-ids");
-    expect(lintResult.offenses[0].message).toBe('Duplicate ID `<%= user.id %>` found. IDs must be unique within a document.');
-    expect(lintResult.offenses[0].severity).toBe("error");
+    expect(lintResult.offenses[0].message).toBe('Duplicate ID `user` found. IDs must be unique within a document.');
+  })
+
+  test("passes for dynamic attribute in a ERBBlockNode each context", () => {
+    const html = dedent`
+      <% @users.each do |user| %>
+        <div id="<%= user.id %>"></div>
+      <% end %>
+    `
+
+    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
+    const lintResult = linter.lint(html);
+
+    expect(lintResult.errors).toBe(0);
+    expect(lintResult.warnings).toBe(0);
+    expect(lintResult.offenses).toHaveLength(0);
   })
 })

From 42459f9b4be078897be7779751220caf343cf1bd Mon Sep 17 00:00:00 2001
From: Marco Roth <marco.roth@intergga.ch>
Date: Mon, 25 Aug 2025 21:43:08 +0200
Subject: [PATCH 2/2] Cleanup using printer

---
 .../linter/src/rules/html-no-duplicate-ids.ts | 37 ++++++++++---------
 .../packages/linter/src/rules/rule-utils.ts   |  7 ----
 .../test/rules/html-no-duplicate-ids.test.ts  | 12 ------
 javascript/packages/printer/src/printer.ts    | 10 ++++-
 4 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts b/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts
index 7fe277d0d..2741ec641 100644
--- a/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts
+++ b/javascript/packages/linter/src/rules/html-no-duplicate-ids.ts
@@ -1,10 +1,11 @@
 import { ParserRule } from "../types"
-import { ControlFlowTrackingVisitor, ControlFlowType, printNodes } from "./rule-utils"
+import { ControlFlowTrackingVisitor, ControlFlowType } from "./rule-utils"
 import { LiteralNode } from "@herb-tools/core"
+import { Printer, IdentityPrinter } from "@herb-tools/printer"
 
-import { hasERBOutput, getValidatableStaticContent, isEffectivelyStatic, isNode, areAllOfType, getStaticAttributeName } from "@herb-tools/core"
+import { hasERBOutput, getValidatableStaticContent, isEffectivelyStatic, isNode, getStaticAttributeName, isERBOutputNode } from "@herb-tools/core"
 
-import type { ParseResult, HTMLAttributeNode } from "@herb-tools/core"
+import type { ParseResult, HTMLAttributeNode, ERBContentNode } from "@herb-tools/core"
 import type { LintOffense, LintContext } from "../types"
 
 interface ControlFlowState {
@@ -16,6 +17,18 @@ interface BranchState {
   previousBranchIds: Set<string>
 }
 
+class OutputPrinter extends Printer {
+  visitLiteralNode(node: LiteralNode) {
+    this.write(IdentityPrinter.print(node))
+  }
+
+  visitERBContentNode(node: ERBContentNode) {
+    if (isERBOutputNode(node)) {
+      this.write(IdentityPrinter.print(node))
+    }
+  }
+}
+
 class NoDuplicateIdsVisitor extends ControlFlowTrackingVisitor<ControlFlowState, BranchState> {
   private documentIds: Set<string> = new Set<string>()
   private currentBranchIds: Set<string> = new Set<string>()
@@ -81,26 +94,14 @@ class NoDuplicateIdsVisitor extends ControlFlowTrackingVisitor<ControlFlowState,
   }
 
   private extractIdValue(attributeNode: HTMLAttributeNode): { identifier: string; shouldTrackDuplicates: boolean } | null {
-    const valueNodes = attributeNode.value!.children
-    const isCompletelyStatic = areAllOfType(valueNodes, LiteralNode)
-    const isEffectivelyStaticValue = isEffectivelyStatic(valueNodes)
+    const valueNodes = attributeNode.value?.children || []
 
     if (hasERBOutput(valueNodes) && this.isInControlFlow && this.currentControlFlowType === ControlFlowType.LOOP) {
       return null
     }
 
-    let identifier: string
-
-    if (isCompletelyStatic) {
-      identifier = printNodes(valueNodes)
-    } else if (isEffectivelyStaticValue) {
-      const staticContent = getValidatableStaticContent(valueNodes)
-      if (!staticContent) return null
-
-      identifier = staticContent
-    } else {
-      identifier = printNodes(valueNodes)
-    }
+    const identifier = isEffectivelyStatic(valueNodes) ? getValidatableStaticContent(valueNodes) : OutputPrinter.print(valueNodes)
+    if (!identifier) return null
 
     return { identifier, shouldTrackDuplicates: true }
   }
diff --git a/javascript/packages/linter/src/rules/rule-utils.ts b/javascript/packages/linter/src/rules/rule-utils.ts
index 3d97f57e3..18f36537f 100644
--- a/javascript/packages/linter/src/rules/rule-utils.ts
+++ b/javascript/packages/linter/src/rules/rule-utils.ts
@@ -853,10 +853,3 @@ export abstract class BaseSourceRuleVisitor {
     return new Location(start, end)
   }
 }
-
-export function printNodes(nodes: Node[]): string {
-  if (!nodes) return ""
-  if (!Array.isArray(nodes)) return ""
-
-  return nodes.map(node => IdentityPrinter.print(node)).join("")
-}
diff --git a/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts b/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts
index 9c702a5b6..ca25f3b61 100644
--- a/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts
+++ b/javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts
@@ -53,18 +53,6 @@ describe("html-no-duplicate-ids", () => {
     expect(lintResult.offenses).toHaveLength(0);
   })
 
-  test("fails for two IDs without value", () => {
-    const html = '<div id=""></div><span id=""></span>';
-    const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
-    const lintResult = linter.lint(html);
-
-    expect(lintResult.errors).toBe(1);
-    expect(lintResult.warnings).toBe(0);
-    expect(lintResult.offenses).toHaveLength(1);
-
-    expect(lintResult.offenses[0].message).toBe('Duplicate ID `` found. IDs must be unique within a document.');
-  })
-
   test("passes for other attributes with equal value", () => {
     const html = '<div class="value"></div><div class="value"></div>';
     const linter = new Linter(Herb, [HTMLNoDuplicateIdsRule]);
diff --git a/javascript/packages/printer/src/printer.ts b/javascript/packages/printer/src/printer.ts
index 0a7c185f5..4498181bc 100644
--- a/javascript/packages/printer/src/printer.ts
+++ b/javascript/packages/printer/src/printer.ts
@@ -31,7 +31,7 @@ export abstract class Printer extends Visitor {
    * @returns The printed string representation of the input
    * @throws {Error} When node has parse errors and ignoreErrors is false
    */
-  static print(input: Token | Node | ParseResult, options: PrintOptions = DEFAULT_PRINT_OPTIONS): string {
+  static print(input: Token | Node | ParseResult | Node[], options: PrintOptions = DEFAULT_PRINT_OPTIONS): string {
     const printer = new (this as any)()
 
     return printer.print(input, options)
@@ -45,11 +45,17 @@ export abstract class Printer extends Visitor {
    * @returns The printed string representation of the input
    * @throws {Error} When node has parse errors and ignoreErrors is false
    */
-  print(input: Token | Node | ParseResult, options: PrintOptions = DEFAULT_PRINT_OPTIONS): string {
+  print(input: Token | Node | ParseResult | Node[], options: PrintOptions = DEFAULT_PRINT_OPTIONS): string {
     if (isToken(input)) {
       return input.value
     }
 
+    if (Array.isArray(input)) {
+      this.context.reset()
+      input.forEach(node => this.visit(node))
+      return this.context.getOutput()
+    }
+
     const node: Node = isParseResult(input) ? input.value : input
 
     if (options.ignoreErrors === false && node.recursiveErrors().length > 0) {