Open lanitochka17 opened 1 month ago
Triggered auto assignment to @twisterdotcom (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@twisterdotcom FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #vip-vsp
Is odd. Going to put this in live markdown though.
Job added to Upwork: https://www.upwork.com/jobs/~01765448d1b685d840
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allgandalf (External
)
Ouch that was bad, @lanitochka17 does it not happen on android ?
No, on Android it isn't an issue, although it never saves the extra space.
can you put this to daily @twisterdotcom , crashes are really bad, was there a reason for weekly?
The reason I made it weekly is that it's a very niche flow. We can make it daily, but I don't think it will necessarily help it get done quicker.
iOS - Chat - App crashes when adding a space at the end of hyperlink text while editing
In here, we are trimming spaces from both end, so in here, the final converted text will be different from the input markdown
, and this will trigger a console.error
here.
However, in iOS, we are using bare Hermes runtime (check this) to evaluate the parser script. Unfortunately, according to https://github.com/facebook/hermes/issues/675, the 'console' is not available in the bare Hermes runtime, hence the crash.
To reproduce, we can run the example
iOS app in the react-native-live-markdown
and follow the OP's step. We can see in the XCode console that the error is something like 'console' property is not defined - JSI evaluation....
Simplest solution is to remove console.error
here. But if we want to raise the error in the console screen or capture this for investigation purpose, we can try to polyfill the console object, so in case console is undefined
, we will do nothing
In here, add
const globalConsole = (globalThis.console ?? null) as Console | null
....
catch (error) {
globalConsole?.error?.(error);
// returning an empty array in case of error
return [];
}
Additionally, we also need to allow trailing spaces on link name. This can be done by following code change in these 2 functions here.
replacement: (_extras, match, g1, g2) => {
if (!g1.trim()) {
return match;
}
return `<a href="${str_1.default.sanitizeURL(g2)}" target="_blank" rel="noreferrer noopener">${g1}</a>`;
},
rawInputReplacement: (_extras, match, g1, g2) => {
if (!g1.trim()) {
return match;
}
return `<a href="${str_1.default.sanitizeURL(g2)}" data-raw-href="${g2}" data-link-variant="labeled" target="_blank"
rel="noreferrer noopener">${g1}</a>`;
},
We can further extend by only allowing extra trailing spaces in link markdown parsing in react-native-live-markdown
's parser by using the extras
object from ExpensiMark
, just in case we don't want trailing spaces when sending the markdown/html string to the BE.
We can utilize https://www.npmjs.com/package/babel-plugin-transform-globals to replace the console.error
or console.log
usage, so we can avoid weird implementation as in the main suggested solution.
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
I will review the above proposal by EOD
@twisterdotcom, @allgandalf Whoops! This issue is 2 days overdue. Let's get this updated quick!
Sorry, this fell through the cracks, i will review the proposal today
@DylanDylann will be taking this issue over, @twisterdotcom can you please assign them, you can have more context for them taking over on slack
@dominictb's root cause analysis from https://github.com/Expensify/App/issues/45623#issuecomment-2247727495 is correct.
In order to get rid of the crash itself, we'll need to remove the following line in react-native-live-markdown
: https://github.com/Expensify/react-native-live-markdown/blob/main/parser/index.ts#L273
However, the formatting will be still incorrect (e.g. no bold here):
So we'll also need to find a way to preserve original whitespace when converting Markdown to HTML in expensify-common
here: https://github.com/Expensify/expensify-common/blob/bd29e1c7a6ae4955cd39e35de586a1cd6fbadeaa/lib/ExpensiMark.ts#L262
So we'll also need to find a way to preserve original whitespace when converting Markdown to HTML
I'm not sure about this @tomekzaw. Is it intentional that we trim all the rear whitespaces in link markdown format [link name](link.com)
?
@dominictb If we trim Markdown [link ](link.com)
, we get the following HTML: <a href="link.com">link</a>
Then, when we convert HTML <a href="link.com">link</a>
, we get the following Markdown: [link](link.com)
.
We lost the space during this process. Currently, Live Markdown parser relies on the fact that the whitespace is preserved.
Yup, agree. I just wonder if we don't trim the space, will it affect anything? Will we get into any regression by doing that?
I don't know, perhaps unit tests should verify that?
π£ @DylanDylann π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
@tomekzaw or @dominictb - want me to assign you two to this as well?
I think this can be made external as this is not caused by react-native-live-markdown
β instead this needs to be fixed in expensify-common
.
@DylanDylann @tomekzaw I'll need to test the change on expensify-common and let you know. The rest is straightforward.
@twisterdotcom @DylanDylann this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
Do we need to assign @dominictb then? Confused how this will be fixed.
@tomekzaw Thanks for your support
@dominictb Let's remove the console and make a change on Expensify-common
to address this bug as mentioned in your proposal
π π π C+ reviewed
Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @dominictb π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
@dominictb Could you provide an ETA for the PR?
@DylanDylann expensify-common and react-native-live-markdown PRs are ready for review.
@dominictb Thanks for the quick PR. It seems the removal of trailing spaces was intentional (see this issue).
From your proposal
In here, we are trimming spaces from both end, so in here, the final converted text will be different from the input markdown, and this will trigger a console.error here.
Should we trim spaces from both the input markdown and the final converted text before making a comparison?
cc @Gonals
We could trim spaces only if shouldKeepRawInput
option is set to true
.
@tomekzaw Could you please provide more detailed information about your point? Thanks.
@DylanDylann You've mentioned that the removal of trailing spaces was intentional so we shouldn't break this functionality as it will cause regressions. On the other hand, live-markdown requires the HTML output to be consistent with Markdown input when it comes to whitespace characters. My suggestion is to prevent removal of trailing spaces when shouldKeepRawInput
flag is set to true (it's already enabled in live-markdown) and remove trailing spaces by default.
@tomekzaw that makes sense. I'll update the PRs accordingly
@tomekzaw Thanks so much
@dominictb Please ping me when the change is ready
@Gonals This PR is ready for review
Not overdue. Reviewing PR
https://github.com/Expensify/react-native-live-markdown/pull/449 has just been merged
Current assignee @DylanDylann is eligible for the External assigner, not assigning anyone new.
I can't get the auto-assigner to kick in, but I've pinged @mountiny to see if he can merge the PR above
This should be ready for payment, no?
Is this for https://github.com/Expensify/App/pull/47199#issuecomment-2296965475? This was deployed to prod 2 days ago, so we should add [Hold for payment 2024-08-26]
right?
@twisterdotcom, @Gonals, @mountiny, @DylanDylann, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!
@twisterdotcom, @Gonals, @mountiny, @DylanDylann, @dominictb 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
I am OOO and will be back for payment on Thurs.
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.8-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4732111 Issue reported by:Applause - Internal Team
Action Performed:
Expected Result:
Space should be appended at the end of hyperlink text
Actual Result:
The app crashes
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/55da63de-a72a-44f0-82ec-56cb714a9ee4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @Issue Owner
Current Issue Owner: @twisterdotcom