Skip to content

Comments

Introduce DOMTree for hierarchical template representation#581

Open
mwmiller wants to merge 1 commit intobartblast:devfrom
mwmiller:domtree
Open

Introduce DOMTree for hierarchical template representation#581
mwmiller wants to merge 1 commit intobartblast:devfrom
mwmiller:domtree

Conversation

@mwmiller
Copy link
Contributor

@mwmiller mwmiller commented Jan 18, 2026

This module transforms the flat output of the template parser into a hierarchical, indexed tree structure, enabling advanced template transformations and detailed developer feedback.

Key features for consumers:

  • Map/Reduce Traversal: The traverse/3 function provides a powerful way to transform the tree and aggregate state simultaneously using a guarded, two-arity callback.
  • Precise Debugging: Each node implements the String.Chars protocol for human-readable logging (e.g. "<div (CID: 0.1)>" or "{%if true}") and find_by_cid/2 for targeted node extraction.
  • Descriptive Error Messages: Error reporting pinpoints the exact problematic node and its position in the hierarchy using CIDs, reducing ambiguity in complex templates.
  • Flexible Attribute Access: get_attribute/3 retrieves values as flattened strings by default, or as raw metadata parts when needed.
  • Stable Node Identity: Automatically generates hierarchical Component IDs (CIDs) while respecting and promoting explicit developer-provided 'cid' attributes in markup.
  • Ergonomic Helpers: A full suite of predicates (element?/1, block?/1, component?/1, etc.) simplifies pattern matching and tree consumption.

The implementation handles multiple HTML fragments, features robust non-fatal error accumulation, and is backed by a comprehensive test suite covering the full lifecycle from parse to inspection.

Summary by CodeRabbit

  • New Features

    • Added a public DOM tree API for parsed templates enabling construction, traversal, and inspection of DOM-like nodes with metadata.
  • Bug Fixes

    • Improved detection and clearer errors for unclosed, mismatched, or unexpected tags (including else cases).
  • Documentation

    • Added public docs and type specs describing the DOM API and traversal behavior.
  • Tests

    • Comprehensive tests covering parsing scenarios, attributes, blocks, traversal, edge cases, and error handling.

@coderabbitai
Copy link

coderabbitai bot commented Jan 18, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Converts parser tokens into a DOM-like tree via a new public module Hologram.Template.DOMTree (with Node struct) and adds from_parse/1 and traverse/2|3 for construction, traversal, and error reporting for unmatched/unclosed tags, blocks, expressions, doctype, and comments. (≤50 words)

Changes

Cohort / File(s) Summary
Core DOMTree Module
lib/hologram/template/dom_tree.ex
New public module Hologram.Template.DOMTree with nested Node struct and @type definitions. Adds from_parse/1 to build node trees from parser tokens (handles text, start/self-closing/end tags, block_start/block_end including else/raw, expression, doctype, comment) and `traverse/2
Test Suite
test/elixir/hologram/template/dom_tree_test.exs
New ExUnit tests covering from_parse/1 and traverse/3 across text, elements (including self-closing, boolean/mixed attrs), components, nesting, doctype, comments, expressions, if/if-else/for/raw blocks, script/raw content preservation, CID semantics, multiple fragments, and error cases (unclosed/mismatched/unexpected else). Review attention: expected node shapes, edge-case assertions, and traversal order expectations.

Sequence Diagram(s)

sequenceDiagram
    participant Parser as Parser (tokens)
    participant DOM as Hologram.Template.DOMTree
    participant Err as ErrorHandler
    participant Test as TestRunner

    Parser->>DOM: from_parse(tags)
    DOM->>DOM: iterate tokens, push/pop stack, build Node structs
    alt error (unclosed/mismatched/else)
        DOM->>Err: {:error, reason, context}
        Err-->>Test: {:error, ...}
    else success
        DOM-->>Test: {:ok, nodes}
        Test->>DOM: traverse(nodes, acc, callback)
        DOM-->>Test: invoke callbacks (pre-order), return {nodes, acc}
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I nibble tokens, stack and bind,
I stitch each tag and keep them kind.
When else hops wrong I raise a flag,
I walk the tree and tally tags,
A rabbit cheers: the nodes align.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and clearly describes the main change: introducing a DOMTree module for hierarchical template representation, which matches the core functionality added in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@lib/hologram/template/dom_tree.ex`:
- Around line 506-512: The truncate/1 function has an off-by-one: it checks
String.length(content) > 20 but slices 0..17 (18 chars) then adds "..."
resulting in 21 chars; change the slice to 0..16 so it returns 17 chars + "..."
= 20 chars total. Update the slice in the truncate function (defp truncate/1)
from String.slice(content, 0..17) to String.slice(content, 0..16) and keep the
existing length check.
🧹 Nitpick comments (4)
lib/hologram/template/dom_tree.ex (3)

192-196: Consider renaming tohere for clarity.

The variable name tohere is unclear. A name like stack or current_stack would better convey its purpose as the remaining stack when comment end is mismatched.

Suggested change
-      tohere ->
-        context = %{parents: parents(tohere), cid: get_stack_cid(tohere)}
+      stack ->
+        context = %{parents: parents(stack), cid: get_stack_cid(stack)}
         new_errors = [{:unexpected_closing_tag, "-->", context} | errors]
-        process(rest, roots, tohere, new_errors)
+        process(rest, roots, stack, new_errors)

234-243: Same naming issue: tohere could be clearer.

Similar to the comment end handling, consider renaming tohere to stack for consistency.


462-504: Consider consolidating redundant @spec annotations.

The String.Chars protocol implementation has identical @spec annotations before each to_string/1 clause. A single @spec at the top would suffice since all clauses share the same signature.

Suggested consolidation
 defimpl String.Chars, for: Hologram.Template.DOMTree do
   alias Hologram.Template.DOMTree

   `@spec` to_string(DOMTree.t()) :: String.t()
   def to_string(%DOMTree{type: :element, name: name, cid: cid}) do
     "<#{name} (CID: #{cid})>"
   end

-  `@spec` to_string(DOMTree.t()) :: String.t()
   def to_string(%DOMTree{type: :component, name: name, cid: cid}) do
     ...
   end
   # ... remove other `@spec` annotations
test/elixir/hologram/template/dom_tree_test.exs (1)

479-548: Static analysis suggests using pipelines.

The linter flags nested function calls like DOMTree.block?(DOMTree.new(type: :block)). While pipelines are idiomatic Elixir, for these simple test assertions the nested form is quite readable. If you want to address the linter warnings, you could refactor to:

# Example for line 481
:block |> then(&DOMTree.new(type: &1)) |> DOMTree.block?() |> assert()

However, this may reduce readability for straightforward tests. Consider suppressing these warnings for test files if the team prefers the current style.

@mwmiller
Copy link
Contributor Author

This PR acts a precursor to #573. It allows me to focus on each bit of the transformation in isolation which is better for my sanity. If we are happy with this change, any eventual formatter can use this to restyle/regenerate the semantically important bits of the source to create formatter output.

Some of the bits around attempting to maintain an "understanding of user intent" might become more difficult this way, while others around opinionated styling might become easier.

@mwmiller mwmiller force-pushed the domtree branch 2 times, most recently from 7d65894 to 4000970 Compare January 18, 2026 10:19
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@lib/hologram/template/dom_tree.ex`:
- Around line 43-46: The message for :unexpected_closing_tag currently formats
all closers as HTML-style "</tag>" in explain_error/1; change explain_error to
render block closers in the template syntax (e.g., "{/if}") by carrying tag-kind
info in the error tuple (e.g., {:unexpected_closing_tag, tag, kind, context}) or
by having the emitter include a distinct marker for block closers, then update
explain_error to match on {:unexpected_closing_tag, tag, :block, context} vs
{:unexpected_closing_tag, tag, :html, context} and format the tag_str as
"{/#{tag}}" for :block and "</#{tag}>" for :html (also update all sites that
emit the tuple to supply the new kind value).
🧹 Nitpick comments (1)
lib/hologram/template/dom_tree.ex (1)

214-221: Consider O(1) CID indexing for large sibling sets.

build_cid/2 calls length/1 on growing lists for every node, which is quadratic for large sibling counts. If big templates are expected, consider storing a child index/count in stack entries to compute the next CID in O(1).

@mwmiller mwmiller force-pushed the domtree branch 4 times, most recently from 159baf7 to 2eb74a8 Compare January 18, 2026 12:01
Copy link
Owner

@bartblast bartblast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some foundational questions to resolve first.

alias Hologram.Template.Helpers
alias Hologram.Template.Parser

@type t :: %__MODULE__{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be called Hologram.Template.DOMTree.Node, what do you think?

children: list(t()) | nil,
content: String.t() | any() | nil,
module: module() | nil,
cid: String.t() | nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the need for assigning CIDs to every node type? In Hologram, CID stands for "Component ID" and is used only for components, not for regular HTML elements, text nodes, etc. This naming could cause confusion.

Also, error messages like "Unclosed <span (CID: 1.0.2)>, found within section > div (CID: 1.0)." aren't meaningful to end users. Compare with the parser's contextual approach:

Close the start tag with '>' character.
<div
    ^

Could you confirm that we'd be able to achieve similar error reporting if we pass position metadata through parser tags? But let's not tackle that in this PR - for the formatter project, let's focus on formatting functionality and keep error messages basic for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit I got a bit focused on error location here at the loss of the ability to reproduce the source more exactly.

attributes: list({String.t(), any()}) | nil,
children: list(t()) | nil,
content: String.t() | any() | nil,
module: module() | nil,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the module field intended for? I don't see it being populated anywhere in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believed at the start that I could have namespacing issues. I never cleaned this up.

{Hologram.Template.DOMTree.t(), list(Hologram.Template.DOMTree.t()),
non_neg_integer()}
),
errors: list(any())
Copy link
Owner

@bartblast bartblast Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using fail-fast instead of error accumulation here?

A few considerations:

  • Consistency - The parser uses fail-fast (raises TemplateSyntaxError)
  • Cascading errors - Structural errors tend to cascade. E.g., with <div><span></div>, the unclosed <span> might cause multiple downstream errors that are really just consequences of the first mistake
  • Simpler implementation - No need for the errors list, reversing, and end-of-stream checking
  • Context - For a formatter, if the template has structural issues, we probably shouldn't format it at all - just show the first problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not generally a huge fan of fail-fast, because I believe it can be misleading about the underlying issues. On the other hand, errors in parsing/handling HTML will tend to happen top-down and it's the repository standard, so sure.

Creates a new DOMTree struct from a keyword list of attributes.
"""
@spec new(keyword()) :: t()
def new(attrs \\ []) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What use case will this function serve in the formatting or template validation process?

def find_by_cid(_node, _target_cid), do: nil
end

defimpl String.Chars, for: Hologram.Template.DOMTree do
Copy link
Owner

@bartblast bartblast Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is needed (though it may not be, depending on the CID discussion above), it should go in a separate file with separate tests IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to improve on the (frankly poor) error reporting from the parser. I didn't achieve all that I hoped in large part because information disappears at each level. I'll probably give up the dream.

alias Hologram.Template.DOMTree
alias Hologram.Template.Parser

test "parses simple text" do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion for test style - in my opinion it would be cleaner to:

  1. Use the parsed_tags/1 test helper instead of calling Parser.parse_markup(markup) each time
  2. Import DOMTree.from_parse/1 so you can call it directly (that's the pattern used in other tests)
  3. Compare using equality instead of pattern matching:
test "parses simple text" do
  result =
    "Hello world"
    |> parsed_tags()
    |> from_parse()

  assert result == {:ok, [%DOMTree{type: :text, content: "Hello world"}]}
end

markup = "{1 + 1}"
tags = Parser.parse_markup(markup)

assert {:ok, [%DOMTree{type: :expression, content: "1 + 1"}]} = DOMTree.from_parse(tags)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the :content field - should we be stripping the braces here? The {...} syntax is used in multiple contexts - template interpolation, component props, actions, etc. The parser already preserves the full expression as {1 + 1} - stripping the braces seems to lose information that might be needed for formatting.

%DOMTree{
type: :block,
name: "if",
content: "true",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the expression is correctly treated as true (in reference to the previous comment about stripping the braces).

<footer>Page {1 + 2}</footer>
</div>
"""

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For complex tests like this, wouldn't it be simpler to compare the full result directly rather than picking apart the tree with many intermediate assertions? The current approach filters out text nodes and is hard to follow. A direct comparison would be more complete and easier to maintain.

@bartblast
Copy link
Owner

@mwmiller Thanks for explaining your thinking on these points, and I want to make sure I'm being clear: I appreciate the effort you've put into this. The questions I raised aren't meant as criticism of the work itself - they're about aligning on the foundation before we build on top of it.

Fail-fast vs error accumulation: My preference here isn't about codebase consistency - it's about how structural errors cascade. With something like <div><span></div>, the unclosed <span> would likely produce multiple downstream errors that are just noise from the first mistake. This is the standard approach for formatters - Prettier, rustfmt, and gofmt all refuse to format syntactically invalid code rather than attempting recovery. It's also how the Hologram template parser already works (raises TemplateSyntaxError immediately). Formatting is fast - just fix the first error and re-run.

The error reporting: "Give up the dream" made me sad to read ;) - please don't! The current Hologram error reporting is genuinely bad DX and we need to fix it. But let's tackle that as a separate effort where we can focus on it properly. For this project, let's just focus on pushing the formatter across the finish line.

Looking back, maybe I focused too much on reviewing every detail of this module. My main concern was making sure the DOM tree approach we land on will be extendable for better error reporting later, and that it makes the formatter implementation itself easier. If it does both, we're in good shape.

What would help me most: could you outline which parts of this PR are essential for the formatter to work, vs. which were exploratory/future-facing? Or do you suggest we just merge it as-is for now and revisit as we work on improving template errors later?

Let me know what you think :)

@mwmiller
Copy link
Contributor Author

I think I can greatly simplify this PR.

I also want to apologize up front. I think I sounded a bit peevish. That is just leaking through from real life stressors. It's not a function of this Hologram work and it shouldn't be appear in my comments.

In any case, I am going to work on an improved version which scales back the accessories and focuses on use cases I can justify to myself up front.

When/if it is incorporated we can worry about how it might be helpful in other cases.

As is my wont, I will likely do this in a rewritten commit. The thought/iteration process here doesn't seem likely to be helpful to mainline history.

@mwmiller mwmiller force-pushed the domtree branch 2 times, most recently from 41199ee to ab35d3c Compare January 20, 2026 10:18
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@test/elixir/hologram/template/dom_tree_test.exs`:
- Around line 1-11: Update the test module declaration in
Hologram.Template.DOMTreeTest: replace the use of ExUnit.Case with the project's
shared test helper by changing "use ExUnit.Case, async: true" to "use
Hologram.Test.BasicCase, async: true" so the module
Hologram.Template.DOMTreeTest continues to run async while aligning with other
tests that use Hologram.Test.BasicCase.
🧹 Nitpick comments (1)
test/elixir/hologram/template/dom_tree_test.exs (1)

187-255: Complex test is comprehensive but verbose.

This test thoroughly exercises deep nesting with mixed blocks, components, and expressions. However, the filtering approach (Enum.filter(root.children, &(&1.type != :text))) to skip whitespace makes the test harder to follow.

Consider either:

  1. Keeping a simpler expected structure without filtering, or
  2. Adding a helper function to strip whitespace text nodes for cleaner assertions

This is a style suggestion; the test correctly validates the parsing behavior.

@mwmiller mwmiller force-pushed the domtree branch 2 times, most recently from 8d5a9cd to a185f0f Compare January 20, 2026 10:50
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@lib/hologram/template/dom_tree.ex`:
- Around line 79-82: The process/3 clause that accepts {:block_start, "else"}
currently pushes an else node onto any non-empty stack causing later misleading
closing-tag errors; change process/3 to validate the stack's top is an enclosing
if (e.g., peek stack and ensure top node is %Node{type: :block, name: "if"})
before pushing the else node, and if not, return an explicit error tuple (or
raise) describing an unexpected else outside of an if so callers can fail fast
and produce a clear message.
♻️ Duplicate comments (2)
lib/hologram/template/dom_tree.ex (1)

9-19: Clarify or remove the unused :module field.

Node includes :module, but nothing in this module sets it. If it’s not part of the intended public surface, consider removing it or documenting when it’s populated to avoid confusion.

test/elixir/hologram/template/dom_tree_test.exs (1)

1-2: Align test module with project test conventions.

Other template tests use Hologram.Test.BasicCase. Consider using it here for consistency.

♻️ Suggested change
-defmodule Hologram.Template.DOMTreeTest do
-  use ExUnit.Case, async: true
+defmodule Hologram.Template.DOMTreeTest do
+  use Hologram.Test.BasicCase, async: true

@mwmiller
Copy link
Contributor Author

@bartblast I have ripped out most of what is unneeded for formatting purposes at this time.

I'm continuing to work on this PR. I am applying changes recommended by the AI and yourself. I expect it to be ready for review/consideration sometime tomorrow.

When/if it is merged I will rebase the formatter work on top of this and try to bring it more into an opinionated user-intent preserving style. As I have mentioned this can have some issues with:

  • Lost information from the toolchain adjustments
  • Deterministic output with semantically identical sources with different inferred user intent

I hope to make it palatable quickly. I don't especially want to rush it to mainline, because a formatter which makes wide but minor adjustments ex-post-facto is even more annoying than not having a formatter available.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@lib/hologram/template/dom_tree.ex`:
- Around line 157-169: The first clause in handle_end_tag currently matches only
by node.name, which allows a `{%block%}` end tag to close an HTML node with the
same name; update that clause in handle_end_tag to also guard on the node's kind
(e.g., require node.kind == kind) so the match only succeeds when both name and
kind match, leaving the implicit-else and error branches unchanged; reference
the existing clause that binds node and children and the functions add_node and
handle_implicit_else_closing to locate where to add the guard.
♻️ Duplicate comments (1)
lib/hologram/template/dom_tree.ex (1)

9-19: Clarify or remove unused :module in Node struct.

module isn’t populated anywhere, so keeping it public adds ambiguity. Consider removing it or documenting the intended usage.

♻️ Optional cleanup
-    `@type` t :: %__MODULE__{
-            type: :component | :element | :text | :expression | :block | :doctype | :comment,
-            name: String.t() | nil,
-            attributes: list({String.t(), any()}) | nil,
-            children: Hologram.Template.DOMTree.nodes() | nil,
-            content: String.t() | any() | nil,
-            module: module() | nil
-          }
+    `@type` t :: %__MODULE__{
+            type: :component | :element | :text | :expression | :block | :doctype | :comment,
+            name: String.t() | nil,
+            attributes: list({String.t(), any()}) | nil,
+            children: Hologram.Template.DOMTree.nodes() | nil,
+            content: String.t() | any() | nil
+          }

-    defstruct [:type, :name, :content, :module, attributes: [], children: []]
+    defstruct [:type, :name, :content, attributes: [], children: []]
🧹 Nitpick comments (1)
test/elixir/hologram/template/dom_tree_test.exs (1)

336-344: Strengthen raw-block assertion to lock semantics.

The test only checks {:ok, _}. Consider asserting the raw block node shape so future changes don’t silently alter parsing.

✅ Example tightening
-    assert {:ok, _nodes} = result
+    assert {:ok, [%Node{type: :block, name: "raw", children: [%Node{type: :text}]}]} =
+             result

Comment on lines +157 to +169
defp handle_end_tag(tag_name, kind, rest, stack, roots) do
case stack do
[{%{name: ^tag_name} = node, children} | stack_tail] ->
finished_node = %{node | children: Enum.reverse(children)}
add_node(finished_node, rest, stack_tail, roots)

[{%{name: "else"}, _else_children} | [{%{name: "if"}, _if_children} | _if_tail]]
when tag_name == "if" ->
handle_implicit_else_closing(rest, stack, roots)

_stack ->
{:error, {:unexpected_closing_tag, tag_name, kind, parents(stack)}}
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard end-tag matching by node type to avoid block/HTML collisions.

Line 159 matches by name only, so a {%block%} end tag could mistakenly close an HTML element with the same name. Add a type guard based on kind to prevent mismatches.

🐛 Suggested fix
 defp handle_end_tag(tag_name, kind, rest, stack, roots) do
   case stack do
-    [{%{name: ^tag_name} = node, children} | stack_tail] ->
+    [{%{name: ^tag_name, type: node_type} = node, children} | stack_tail]
+    when (kind == :block and node_type == :block) or
+         (kind == :html and node_type in [:element, :component]) ->
       finished_node = %{node | children: Enum.reverse(children)}
       add_node(finished_node, rest, stack_tail, roots)

-    [{%{name: "else"}, _else_children} | [{%{name: "if"}, _if_children} | _if_tail]]
+    [{%{type: :block, name: "else"}, _else_children} |
+     [{%{type: :block, name: "if"}, _if_children} | _if_tail]]
     when tag_name == "if" ->
       handle_implicit_else_closing(rest, stack, roots)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defp handle_end_tag(tag_name, kind, rest, stack, roots) do
case stack do
[{%{name: ^tag_name} = node, children} | stack_tail] ->
finished_node = %{node | children: Enum.reverse(children)}
add_node(finished_node, rest, stack_tail, roots)
[{%{name: "else"}, _else_children} | [{%{name: "if"}, _if_children} | _if_tail]]
when tag_name == "if" ->
handle_implicit_else_closing(rest, stack, roots)
_stack ->
{:error, {:unexpected_closing_tag, tag_name, kind, parents(stack)}}
end
defp handle_end_tag(tag_name, kind, rest, stack, roots) do
case stack do
[{%{name: ^tag_name, type: node_type} = node, children} | stack_tail]
when (kind == :block and node_type == :block) or
(kind == :html and node_type in [:element, :component]) ->
finished_node = %{node | children: Enum.reverse(children)}
add_node(finished_node, rest, stack_tail, roots)
[{%{type: :block, name: "else"}, _else_children} |
[{%{type: :block, name: "if"}, _if_children} | _if_tail]]
when tag_name == "if" ->
handle_implicit_else_closing(rest, stack, roots)
_stack ->
{:error, {:unexpected_closing_tag, tag_name, kind, parents(stack)}}
end
🤖 Prompt for AI Agents
In `@lib/hologram/template/dom_tree.ex` around lines 157 - 169, The first clause
in handle_end_tag currently matches only by node.name, which allows a
`{%block%}` end tag to close an HTML node with the same name; update that clause
in handle_end_tag to also guard on the node's kind (e.g., require node.kind ==
kind) so the match only succeeds when both name and kind match, leaving the
implicit-else and error branches unchanged; reference the existing clause that
binds node and children and the functions add_node and
handle_implicit_else_closing to locate where to add the guard.

@bartblast
Copy link
Owner

@mwmiller

I also want to apologize up front. I think I sounded a bit peevish. That is just leaking through from real life stressors. It's not a function of this Hologram work and it shouldn't be appear in my comments.

No worries at all! I honestly didn't read it as peevish - maybe a bit disappointed or resigned, but that's the eternal mystery of text-based communication :) You never quite know what the person on the other side of the screen is actually feeling. For context, I've had genuinely rough exchanges in reviews on this project. Yours didn't register on that scale at all. And I should apologize for being too dry or nitpicky sometimes myself - when you're juggling a lot of review threads, it's easy to forget there's a human putting real effort into every contribution.

I don't especially want to rush it to mainline, because a formatter which makes wide but minor adjustments ex-post-facto is even more annoying than not having a formatter available.

Agreed on not rushing this. We can easily push it to the next release if you prefer. And don't worry about holding things up - I'm shortening release cycles, so minor releases will usually go out every 1-2 weeks depending on what features are ready.

Just ping me when it's ready for another look!

@mwmiller mwmiller force-pushed the domtree branch 3 times, most recently from dfc5385 to 8f3b2a7 Compare January 26, 2026 18:15
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@test/elixir/hologram/template/dom_tree_test.exs`:
- Around line 337-344: The test uses a nonstandard space in the raw block syntax
("{ %raw}") which conflicts with the tokenizer/parser expectations; update the
test in hologram/template/dom_tree_test.exs to use the normalized raw block
syntax "{%raw}<div />{/raw}" (reference the test helpers parsed_tags() and
from_parse()) or, if you intend to assert malformed input handling, change the
test name and add a concise comment explaining that this is an edge-case
malformed-raw test so it's clear why the space is present.

Comment on lines +337 to +344
test "parses raw block" do
result =
"{ %raw}<div />{/raw}"
|> parsed_tags()
|> from_parse()

assert {:ok, _nodes} = result
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for raw block patterns in the codebase
echo "=== Raw block usages in tests ==="
rg -n "raw" test/elixir --type=elixir -B2 -A2 | head -80

echo ""
echo "=== Raw block in parser/implementation ==="
rg -n "raw" lib/elixir --type=elixir -B1 -A1 | head -60

echo ""
echo "=== Check for documentation or specs about raw syntax ==="
rg -n "\{.*%raw\}" . --type=elixir | head -40

Repository: bartblast/hologram

Length of output: 9622


Remove space in raw block syntax or clarify test intent.

The test uses "{ %raw}" with a space, which differs from all other raw block usages throughout the codebase. The parser and tokenizer implementation expect "{%raw}" without a space (see lib/hologram/template/tokenizer.ex:145 and all other tests in parser_test.exs). Either:

  1. Fix the syntax to "{%raw}<div />{/raw}" to match standard usage, or
  2. If testing malformed syntax handling, add a comment explaining the edge case.
🤖 Prompt for AI Agents
In `@test/elixir/hologram/template/dom_tree_test.exs` around lines 337 - 344, The
test uses a nonstandard space in the raw block syntax ("{ %raw}") which
conflicts with the tokenizer/parser expectations; update the test in
hologram/template/dom_tree_test.exs to use the normalized raw block syntax
"{%raw}<div />{/raw}" (reference the test helpers parsed_tags() and
from_parse()) or, if you intend to assert malformed input handling, change the
test name and add a concise comment explaining that this is an edge-case
malformed-raw test so it's clear why the space is present.

@mwmiller mwmiller force-pushed the domtree branch 2 times, most recently from ecc0b43 to 68ff203 Compare February 9, 2026 06:29
Implement the Hologram.Template.DOMTree module to convert parsed template
tokens into a structured DOM tree.

Features:
- Define Hologram.Template.DOMTree.Node to represent tree elements including
  components, elements, text, expressions, blocks, and comments.
- Implement from_parse/1 to process linear tag lists into a nested tree structure.
- Enforce fail-fast error handling for validation, returning specific errors for
  unclosed tags or mismatched nesting immediately upon detection.
- Support control flow constructs, specifically handling the relationship between
  if and else blocks where the latter is implicitly closed.
- Provide traverse/3 to facilitate depth-first, pre-order traversal and
  transformation of the generated tree.
@bartblast
Copy link
Owner

Hey Matt, just checking in - how's it going with this PR? Is there anything you need from me, any questions or additional context I can help with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants