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.

Sign up for GitHub

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

[CP Staging] input of '[' and other special characters fixed #30559

Merged
merged 10 commits into from
Oct 30, 2023
Merged

[CP Staging] input of '[' and other special characters fixed #30559

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7bbc841
input of '[' and other special characters fixed
HardikChoudhary24 Oct 29, 2023
c145333
lint error fixed
HardikChoudhary24 Oct 29, 2023
c4cc971
suggested changes applied
HardikChoudhary24 Oct 29, 2023
fbc466b
Merge remote-tracking branch 'upstream/main' into fix/input-error
HardikChoudhary24 Oct 30, 2023
7054c51
test for convertToLTRForComposer added
HardikChoudhary24 Oct 30, 2023
a2c2025
test case added
HardikChoudhary24 Oct 30, 2023
c449904
lint fixed
HardikChoudhary24 Oct 30, 2023
9bd0b5f
lint fixed
HardikChoudhary24 Oct 30, 2023
f6eede0
OS check removed
HardikChoudhary24 Oct 30, 2023
d28b633
lint fixed
HardikChoudhary24 Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/libs/convertToLTRForComposer/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ function hasRTLCharacters(text: string): boolean {

// Converts a given text to ensure it starts with the LTR (Left-to-Right) marker.
const convertToLTRForComposer: ConvertToLTRForComposer = (text) => {
// Ensure that the text starts with RTL characters if not we return the same text to avoid concatination with special character at the start which leads to unexpected behaviour for Emoji/Mention suggestions.
// Ensure that the text starts with RTL characters if not we return the same text to avoid concatination with special
// character at the start which leads to unexpected behaviour for Emoji/Mention suggestions.
if (!hasRTLCharacters(text)) {
// If text is empty string return empty string to avoid an empty draft due to special character.
if (text === '' || CONST.UNICODE.LTR.match(text)) {
if (text === '' || text.startsWith(CONST.UNICODE.LTR)) {
return '';
}
return text;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If text is empty string return empty string to avoid an empty draft due to special character.
if (text === '' || CONST.UNICODE.LTR.match(text)) {
if (text === '' || text.startsWith(CONST.UNICODE.LTR)) {
return '';
}
return text;
return text.replace(CONST.UNICODE.LTR, '');

Anyway, when does this happen?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the root cause of this bug? Why only [ not working but all other special characters work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because [ has a special meaning in regular expressions and CONST.UNICODE.LTR.match('[') gives an error because [ is not properly escaped. If we do 'CONST.UNICODE.LTR.match(/\[/) it will give null which we require but when we are passing a string we are just passing [. This also happens for ? , + due to the same reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Can you think of adding automated test cases? As you see, this might easily break composer input and affect bad user experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will add automated test cases. Should I raise a new PR for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The next deploy will be tomorrow (Monday) so if you can add them before that, that would be great.
Otherwise, we need to merge this before deploy to staging and work on unit test as a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I won't be able to complete the task before tomorrow as my current workload won't permit me to meet that deadline. Can we do it as a follow up?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with it. cc: @neil-marcellini

Expand Down
Loading