Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added img attribute caching to ExpensiMark #808

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mjasikowski
Copy link
Contributor

@mjasikowski mjasikowski commented Sep 26, 2024

This PR adds caching for additional attributes to the <img> tags when converted to and back from Markdown.

Due to naming weirdness (current cache fields are named videoAttributeCache and cacheVideoAttributes), new fields were created on the Extras object:

  • mediaAttributeCachingFn replacing cacheVideoAttributes
  • mediaAttributeCache replacing videoAttributeCache

Replaced fields are marked as deprecated in JS doc and will be removed after this PR is deployed and NewDot is modified so it doesn't use the deprecated fields anymore.

Logic-wise, only the deprecated properties and the new ones won't ever be used simultanously, hence caching function selection is done by detecting the presence of either deprecated or new fields.

https://github.com/Expensify/Web-Expensify/pull/43729 needs to be merged first for App tests to be doable.

Fixed Issues

Expensify/App#42206

Tests

Added tests to ExpensiMark-HTML-test.js and ExpensiMark-Markdown-test.js to test the conversion logic.
Added tests to ExpensiMark-test.js to test caching function selection logic.

QA

After this is deployed, test in NewDot:

  • Disable cache in your browser
  • Create a message with text and an image attachment
  • Send the message
  • Edit the text in the message
  • Verify the image thumbnail is displayed correctly
  • Verify that the image can be clicked on
  • Refresh the page in your browser
  • Edit the text in the message once again
  • Verify the image thumbnail is displayed correctly
  • Verify that no errors appear in the JS console

@mjasikowski mjasikowski self-assigned this Sep 26, 2024
@mjasikowski mjasikowski requested review from mountiny and a team September 26, 2024 13:41
@mjasikowski mjasikowski marked this pull request as ready for review September 26, 2024 13:41
@melvin-bot melvin-bot bot requested review from youssef-lr and removed request for a team September 26, 2024 13:41
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks for adding the unit tests, that is very reassuring. My main question before approving is to make sure to understand the regex changes

lib/ExpensiMark.ts Show resolved Hide resolved
lib/ExpensiMark.ts Show resolved Hide resolved
lib/ExpensiMark.ts Show resolved Hide resolved
lib/ExpensiMark.ts Show resolved Hide resolved
lib/ExpensiMark.ts Outdated Show resolved Hide resolved
replacement: (_extras, _match, _g1, g2, _g3, g4) => {
if (g4) {
return `![${g4}](${g2})`;
regex: /<img[^><]*src\s*=\s*(['"])(.*?)\1(.*?)>(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please "breakdown" the regex changes you made here? I see you have taken out the altRegex, but I can also see some other changes in latter part of the regex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • removed(?:[^><]*alt\s*=\s*(['"])(.*?)\3)? in favor of (.*?) to capture all attributes after src (including alt)
  • simplified closing tag matching ([^><]*>* to (.*?)>

and that's pretty much it.

@mountiny
Copy link
Contributor

Asked Rushat for testing on this PR https://expensify.slack.com/archives/C02NK2DQWUX/p1727359643953779

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thanks! @rushatgabhane have you been able to test it?

@rushatgabhane
Copy link
Member

yeah, gimme a moment for checklist

@rushatgabhane
Copy link
Member

@mjasikowski im not able to build this. Could you please help me out?

I tried:

  1. Pull this branch to expensify-common local repo.
  2. Build the branch cd expensify-common && npm run build
  3. Go to Expensify/App/
  4. Install expensify-common from local directory - npm i expensify-common ../expensify-common --save

I also tried directly installing the branch to Expensify/App using npm i git+https://github.com/Expensify/expensify-common.git#michal-img-attributes

error: Module not found: Error: Can't resolve 'expensify-common' in '/Users/rushatgabhane/code/App/src/pages/workspace/reportFields

@mjasikowski
Copy link
Contributor Author

mjasikowski commented Sep 27, 2024

@rushatgabhane I had a similar problem and couldn't figure it out (troubles with npm link), so I just built expensify-common and copied over the contents of dist to ./App/node-modules/expensify-common so it's updated in place

@rushatgabhane
Copy link
Member

That worked, thank you!

@rushatgabhane
Copy link
Member

rushatgabhane commented Sep 28, 2024

@mjasikowski so im having a few issues with opening the image. Steps i followed

  1. Disable cache
  2. Type message in composer and send an image attachment
  3. Edit the message
  4. Tap the image to open it

Expected: Image loads.
Actual: the image does not load.

I am able to load the image if I don't edit its text. i.e. skip step 3.

Screen.Recording.2024-09-28.at.18.mp4

@mjasikowski
Copy link
Contributor Author

@rushatgabhane thanks for testing, I'll take another look on Monday - it may be that we need a small API addition.

@mjasikowski mjasikowski changed the title Added img attribute caching to ExpensiMark [Hold Web#43698] Added img attribute caching to ExpensiMark Sep 29, 2024
@mjasikowski
Copy link
Contributor Author

mjasikowski commented Sep 29, 2024

It seems that the API also strips the tags, so that is fixed with https://github.com/Expensify/Web-Expensify/pull/43729
We're holding with full tests until the above is merged.

@rushatgabhane
Copy link
Member

alright, understood

@mjasikowski mjasikowski changed the title [Hold Web#43698] Added img attribute caching to ExpensiMark [Hold Web#43729] Added img attribute caching to ExpensiMark Sep 30, 2024
@mjasikowski mjasikowski changed the title [Hold Web#43729] Added img attribute caching to ExpensiMark Added img attribute caching to ExpensiMark Oct 4, 2024
@mjasikowski
Copy link
Contributor Author

@rushatgabhane the backend PR is now deployed to production, so this is no longer held and ready for testing

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