Creating a new BlockNode instance on every visit creates unnecessary objects even when no children were filtered. Consider checking if newChildren has the same length and content as node.children before creating a new instance, returning the original node if nothing changed.

I don't think this will handle nested architectures here.

It seems this is already done at line 70.

[suggestion]: maybe use toHaveLength(0) instead?

[question] is it possible to have a matching element as child here, or no?

Likely not. Some magical voodoo under the hood would need to happen to cause this issue

[suggestion] I am not sure if this description matches the test. It seems to suggest we would be filtering a child of matching block. Is it a valid scenario that the matching block would also have a non-matching child element?

Is the idea of the test that we would still filter the non-matches when there is also one thing that matches? Maybe this name would be clearer -- "filters non-matching children and keeps matches". But maybe I am missing the significance of the resursiveness?

So a block can have a non-matching element, so as long as the hash matches, it will need to continue down recursively.

The example would be

page = st.navigation(...)
with st.container(): # <- Block
    page.run()

the block would always match the main script hash, while the page will have different page hashes.

The other example you describe above was a non-matching block containing a matching element. I'm not sure how possible that is, BUT we just will delete the entire block since it doesn't match.

[suggestion]: maybe use toHaveLength(0) instead?

[suggestion/question]: should the title be something about the static method calls being idempotent? I don't understand the importance of referencing a new visitor instance (but let me know if I am missing something).

This test is irrelevant really, so I removed it. It's hard to tell a new visitor instance is used.

[nit] it bothers me that we are re-using the same objects here, but in practice these would be de-duped, right?

So this is a shortcut. We could make duplicates, but the one thing about these Nodes is that they are immutable by design, so reusing them is not the a for this use case.

We do not dedupe nor would we expect references in multiple places on the tree, so I'll make the test more explicit.

Suggestion: Perhaps test can be added to cover childrenChanged optimization:

it("returns original node when no children change", () => {
  // All children match, so none are filtered
  const matchingElement1 = new ElementNode(
    text("element1").element,
    text("element1").metadata,
    "test_run_id",
    MAIN_SCRIPT_HASH
  )

  const matchingElement2 = new ElementNode(
    text("element2").element,
    text("element2").metadata,
    "test_run_id",
    MAIN_SCRIPT_HASH
  )

  const blockNode = new BlockNode(
    MAIN_SCRIPT_HASH,
    [matchingElement1, matchingElement2],
    block().deltaBlock,
    "test_run_id"
  )

  const visitor = new FilterMainScriptElementsVisitor(MAIN_SCRIPT_HASH)
  const result = visitor.visitBlockNode(blockNode)

  // Critical: Should return THE SAME reference (not a new object)
  // This is the childrenChanged optimization
  expect(result).toBe(blockNode)  // Reference equality!
  expect(result).not.toBe(new BlockNode(...))  // Not a new instance
})

Good call. Added with some small modifications.