Skip to content

feat: add maxBodyLength parameter to htmlToMarkdown#852

Merged
inimaga merged 3 commits into
Expensify:mainfrom
yellowtailfan:fix/57241
May 6, 2025
Merged

feat: add maxBodyLength parameter to htmlToMarkdown#852
inimaga merged 3 commits into
Expensify:mainfrom
yellowtailfan:fix/57241

Conversation

@yellowtailfan

Copy link
Copy Markdown
Contributor

Supporting change for fixing Expensify/App#57241.

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
  • Added "Test long HTML header" to __tests__/ExpensiMark-Markdown-test.js
  1. What tests did you perform that validates your changed worked?
  • Tested on all platforms from App code modified to use new maxBodyLength parameter.
  • Tests include HTML clipboard paste on platforms where available.
  • Tests included very large clipboard pastes to check for performance issues. There was some lag but it was similar both before and after this change.
  • Inspected all calls to htmlToMarkdown() from Expensify repo and none will be affected by the additional parameter.

QA

  1. What does QA need to do to validate your changes?
  • Run unit tests.
  1. What areas to they need to test for regressions?
  • HTML to Markdown conversion.

@github-actions

github-actions Bot commented May 2, 2025

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@yellowtailfan

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

CLABotify added a commit to Expensify/CLA that referenced this pull request May 2, 2025
@yellowtailfan yellowtailfan changed the title feat: add maxBodyLength parameter to htmlToMarkdown [WIP] feat: add maxBodyLength parameter to htmlToMarkdown May 2, 2025
@yellowtailfan yellowtailfan marked this pull request as ready for review May 5, 2025 06:55
@yellowtailfan yellowtailfan requested a review from a team as a code owner May 5, 2025 06:55
@melvin-bot melvin-bot Bot requested review from inimaga and removed request for a team May 5, 2025 06:55
@yellowtailfan yellowtailfan changed the title [WIP] feat: add maxBodyLength parameter to htmlToMarkdown feat: add maxBodyLength parameter to htmlToMarkdown May 5, 2025

@inimaga inimaga left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@inimaga inimaga merged commit a584385 into Expensify:main May 6, 2025
6 checks passed
@os-botify

os-botify Bot commented May 6, 2025

Copy link
Copy Markdown
Contributor

🚀 Published to npm in 2.0.136 🎉

Comment thread lib/ExpensiMark.ts
if (parseBodyTag) {
generatedMarkdown = parseBodyTag[2];
}
if (maxBodyLength > 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Coming from Expensify/App#63711. We should have decoded the HTML first before truncation, otherwise decoding can fail if there isn't enough content to decode. More details here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. The reason we truncated early was a compromise: there were serious performance issues with parsing into Markdown before truncation. I'll find a reference for you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is a more detailed comment on the performance issues around truncation before or after parsing: Expensify/App#57241 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is a summary of our investigation: Expensify/App#57241 (comment) and the accepted proposal Expensify/App#57241 (comment)

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