Skip to content

Conversation

@alliern
Copy link

@alliern alliern commented Sep 16, 2025

Some scenarios involving intentionally blank tags were resulting in mismatched HTML, after being diffed, like a LaTex span followed by a writing blank span that is then replaced by a second LaTex span. This branch adds support for tags that are intentionally left empty by treating them as unified "words" to be diffed as a whole.

It also adds an optional compare_tag_attributes argument to be used in the same_tag? method on the Operations class. Previously, tags with the same name and different attributes were always treated as "equal" when being diffed, resulting in unpredictable outcomes when, for example, a writing blank span is replaced by a latex span', or an embedded image is replaced by a different image (where the only tag difference is the src attribute). Optionally passing incompare_tag_attributes` allows us to treat tags with different attributes as entirely different content when necessary.

@alliern alliern requested a review from danblaker September 16, 2025 21:24
Copy link

@danblaker danblaker left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this until it uses a generic approach, e.g. support for diffing specified attributes, rather than OUR-specific tags.

@alliern alliern changed the title Add support for our-embeds and writing blank spans Add support for intentionally empty tags Sep 22, 2025
@alliern alliern requested review from danblaker and pvande September 22, 2025 16:06
Copy link

@pvande pvande left a comment

Choose a reason for hiding this comment

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

I know frustratingly little about how this is implemented or how it is being used, so I don't feel confident speaking to whether this is fit for purpose or an ideal approach.

I will note that there is no testing around this change, which we should probably remedy before merging — that will at minimum allow us to refactor things in the future, if a better approach reveals itself.

@danblaker
Copy link

@alliern I agree with @pvande that testing is a necessary component of this implementation. Luckily, it's arguably the thing that Claude Code is best at.

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.

4 participants