Skip to content

Update markup after paste (BL-15334)#7677

Open
JohnThomson wants to merge 2 commits intoVersion6.3from
updateMarkupPaste
Open

Update markup after paste (BL-15334)#7677
JohnThomson wants to merge 2 commits intoVersion6.3from
updateMarkupPaste

Conversation

@JohnThomson
Copy link
Contributor

@JohnThomson JohnThomson commented Feb 10, 2026


Open with Devin

This change is Reviewable

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

View 6 additional findings in Devin Review.

Open in Devin Review

Comment on lines 1077 to 1078
// AI thinks we might need this to allow the DOM to settle even before we do the
// little bit that handlePageEditing does before setting up its own delay.

Choose a reason for hiding this comment

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

📝 Info: Comment references AI suggestion — consider updating before merge

The comments at lines 1077 and 227 reference AI suggestions ("AI thinks we might need this" and "AI suggested adding these"). While not a code issue, these comments could be confusing for future maintainers. Consider rephrasing to describe the technical rationale (DOM settling / catching async paste completion) rather than attributing the decision to an AI tool.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AI comment was exceptionally unhelpful and seemed to be based on an expectation that things being done after the 500ms delay were instead being done immediately. Maybe I should have quoted it word-for-word, but I don't still have it. Since I have not managed to understand myself why we need this additional timeout (if we do), I can't write a more intelligible explanation than that I suggested it.

Copy link
Contributor Author

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

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

@JohnThomson made 2 comments.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @hatton).

Comment on lines 1077 to 1078
// AI thinks we might need this to allow the DOM to settle even before we do the
// little bit that handlePageEditing does before setting up its own delay.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AI comment was exceptionally unhelpful and seemed to be based on an expectation that things being done after the 500ms delay were instead being done immediately. Maybe I should have quoted it word-for-word, but I don't still have it. Since I have not managed to understand myself why we need this additional timeout (if we do), I can't write a more intelligible explanation than that I suggested it.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson).

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.

3 participants