Add Task Title Validation on Main Composer Text Change #52941
Conversation
…x/50398-fix-max-length-validation-for-task
…x/50398-fix-max-length-validation-for-task
…x/50398-fix-max-length-validation-for-task
melvin-bot
bot
requested review from
hoangzinh
and removed request for
a team
|
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
This comment was marked as off-topic.
This comment was marked as off-topic.
| return false; | ||
| }, []); | ||
|
|
||
| const validateTitleMaxLength = useMemo(() => debounce(handleValueChange, 100, {leading: true}), [handleValueChange]); |
There was a problem hiding this comment.
Can you add debounce time to this constant list?
Line 1320 in 056e8ee
Btw, should we increase it a little bit? Oh there is an existing debounce time for comment length https://github.com/Expensify/App/blob/056e8ee563b1136ba5749988d89723fad467c7fd/src/CONST.ts#L1314C9-L1314C37
| @@ -399,9 +408,12 @@ function ReportActionCompose({ | |||
| if (value.length === 0 && isComposerFullSize) { | |||
| Report.setIsComposerFullSize(reportID, false); | |||
| } | |||
| if (validateTitleMaxLength(value)) { | |||
There was a problem hiding this comment.
it's a debounce function, can we trust it to return true/false value here?
There was a problem hiding this comment.
https://www.geeksforgeeks.org/lodash-_-debounce-method/
We set leading true, it will immediately executed at the initial attempt
There was a problem hiding this comment.
Oops, I missed this comment. Hmm. I don't think it's a good idea to check conditions based on result of a debounce function. It's better if we detect comment format and use properly validation function here. For example:
const isCreatingTaskComment = value.match(CONST.REGEX.TASK_TITLE_WITH_OPTONAL_SHORT_MENTION);
if (isCreatingTaskComment) {
validateTitleMaxLength(value)
} else {
validateCommentMaxLength(value, {reportID});
}what do you think?
| let exceededMaxLength = null; | ||
| if (hasExceededMaxTitleLength) { | ||
| exceededMaxLength = CONST.TITLE_CHARACTER_LIMIT; | ||
| } else if (hasExceededMaxCommentLength) { | ||
| exceededMaxLength = CONST.MAX_COMMENT_LENGTH; | ||
| } |
There was a problem hiding this comment.
I'm thinking should we wrap exceededMaxLength into a useState and useEffect hook?
There was a problem hiding this comment.
@hoangzinh Agree, the PR has been updated
|
Thanks for your suggestion @ChavdaSachin
@hoangzinh what is your opinion? |
|
Yep, I think it's a good idea. Can you raise it in the original issue? So we can discuss it with the internal engineer. |
|
I think we can call @Expensify/design here. Can I do that or it should be called by internal? |
|
@Expensify/design Based on this comment #52941 (comment), do we need to distinguish validation text other than its length number? |
|
I think we could just update the one for the task situation to read:
Thoughts? |
|
@ChavdaSachin Let's please keep any conversations on PRs limited to the internal reviewers and assignees. |
This comment was marked as off-topic.
This comment was marked as off-topic.
…x/50398-fix-max-length-validation-for-task
Co-authored-by: Marc Glasser <[email protected]>
|
Waiting translation confirmation https://expensify.slack.com/archives/C01GTK53T8Q/p1732322764789539 |
Looks good to me. Thanks @dannymcclain |
…x/50398-fix-max-length-validation-for-task
| @@ -399,9 +408,12 @@ function ReportActionCompose({ | |||
| if (value.length === 0 && isComposerFullSize) { | |||
| Report.setIsComposerFullSize(reportID, false); | |||
| } | |||
| if (validateTitleMaxLength(value)) { | |||
There was a problem hiding this comment.
Oops, I missed this comment. Hmm. I don't think it's a good idea to check conditions based on result of a debounce function. It's better if we detect comment format and use properly validation function here. For example:
const isCreatingTaskComment = value.match(CONST.REGEX.TASK_TITLE_WITH_OPTONAL_SHORT_MENTION);
if (isCreatingTaskComment) {
validateTitleMaxLength(value)
} else {
validateCommentMaxLength(value, {reportID});
}what do you think?
| @@ -172,6 +173,8 @@ function ReportActionCompose({ | |||
| * Shows red borders and prevents the comment from being sent | |||
| */ | |||
| const {hasExceededMaxCommentLength, validateCommentMaxLength} = useHandleExceedMaxCommentLength(); | |||
| const {hasExceededMaxTitleLength, validateTitleMaxLength} = useHandleExceedMaxTaskTitleLength(); | |||
There was a problem hiding this comment.
| const {hasExceededMaxTitleLength, validateTitleMaxLength} = useHandleExceedMaxTaskTitleLength(); | |
| const {hasExceededMaxTaskTitleLength, validateTaskTitleMaxLength} = useHandleExceedMaxTaskTitleLength(); |
Can we name it more specific to task title? 🤔
There was a problem hiding this comment.
Can we name it more specific to task title?
sure
There was a problem hiding this comment.
I don't think it's a good idea to check conditions based on result of a debounce function. It's better if we detect comment format and use properly validation function here
@hoangzinh In my opinion using a separate variable like isCreatingTaskComment would be redundant as we already perform regex matching in useHandleExceedMaxTaskTitleLength. If we implement leading true, it should be safe. But let me know if we still need to do that.
There was a problem hiding this comment.
we can move regrex matching out of useHandleExceedMaxTaskTitleLength hook. And add another wrapper debounce for validation methods like this. I think it works better. What do you think?
There was a problem hiding this comment.
@hoangzinh after test it might looks like this
src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx
const onValueChange = useCallback(
(value: string) => {
if (value.length === 0 && isComposerFullSize) {
Report.setIsComposerFullSize(reportID, false);
}
const taskCommentMatch = value.match(CONST.REGEX.TASK_TITLE_WITH_OPTONAL_SHORT_MENTION);
if (taskCommentMatch) {
const title = taskCommentMatch?.[3] ? taskCommentMatch[3].trim().replace(/\n/g, ' ') : '';
validateTaskTitleMaxLength(title);
} else {
validateCommentMaxLength(value, {reportID});
}
},
[isComposerFullSize, reportID, validateCommentMaxLength, validateTaskTitleMaxLength],
);
src/hooks/useHandleExceedMaxTaskTitleLength.ts
const useHandleExceedMaxTaskTitleLength = () => {
const [hasExceededMaxTaskTitleLength, setHasExceededMaxTitleLength] = useState(false);
const handleValueChange = useCallback((title: string) => {
const exceeded = title ? title.length > CONST.TITLE_CHARACTER_LIMIT : false;
setHasExceededMaxTitleLength(exceeded);
}, []);
const validateTaskTitleMaxLength = useMemo(() => debounce(handleValueChange, CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME, {leading: true}), [handleValueChange]);
return {hasExceededMaxTaskTitleLength, validateTaskTitleMaxLength};
};
proceed with the above change? or alternatively we can keep current useHandleExceedMaxTaskTitleLength logic but no need to debounce it?
There was a problem hiding this comment.
you're right @wildan-m . Hmm, what do you think if we refactor it a bit:
- Move 1500 as a constant
- Remove debounce in 2 above hooks
- Add a debounce where it's used.
So in main-composer, we will use 1500 as a debounce time for both plain function validations above. And in edit composer, we will have another debounce with 1500ms.
There was a problem hiding this comment.
@hoangzinh if we're going to extract the debounce and not need faster debounce specific for task title, how about keeping the regex matching inside the hook?
const useHandleExceedMaxTaskTitleLength = () => {
const [hasExceededMaxTaskTitleLength, setHasExceededMaxTitleLength] = useState(false);
const validateTaskTitleMaxLength = useCallback((value: string) => {
const match = value.match(CONST.REGEX.TASK_TITLE_WITH_OPTONAL_SHORT_MENTION);
if (match) {
const title = match[3] ? match[3].trim().replace(/\n/g, ' ') : undefined;
const exceeded = title ? title.length > CONST.TITLE_CHARACTER_LIMIT : false;
setHasExceededMaxTitleLength(exceeded);
return true;
}
setHasExceededMaxTitleLength(false);
return false;
}, []);
return {hasExceededMaxTaskTitleLength, validateTaskTitleMaxLength};
};
There was a problem hiding this comment.
We can @wildan-m, but it's kind of wrong in a case, let's say validateTaskTitleMaxLength returns false. It also means it's a task creating comment, however, the length is still in capacity. But then we have to use another validation of validateCommentMaxLength, which is unnecessary and incorrect because it's a task creating comment but we use a standard max length validation on it. Kind of double validations.
There was a problem hiding this comment.
Another option we can consider refactoring existing validateCommentMaxLength by passing max_length.
There was a problem hiding this comment.
@hoangzinh the code has been updated. I use and modify COMMENT_LENGTH_DEBOUNCE_TIME since it is not being used anywhere.
| if (hasExceededMaxTitleLength) { | ||
| setExceededMaxLength(CONST.TITLE_CHARACTER_LIMIT); | ||
| return; | ||
| } | ||
| if (hasExceededMaxCommentLength) { | ||
| setExceededMaxLength(CONST.MAX_COMMENT_LENGTH); | ||
| return; | ||
| } | ||
| setExceededMaxLength(null); |
There was a problem hiding this comment.
| if (hasExceededMaxTitleLength) { | |
| setExceededMaxLength(CONST.TITLE_CHARACTER_LIMIT); | |
| return; | |
| } | |
| if (hasExceededMaxCommentLength) { | |
| setExceededMaxLength(CONST.MAX_COMMENT_LENGTH); | |
| return; | |
| } | |
| setExceededMaxLength(null); | |
| if (hasExceededMaxTitleLength) { | |
| setExceededMaxLength(CONST.TITLE_CHARACTER_LIMIT); | |
| } else if (hasExceededMaxCommentLength) { | |
| setExceededMaxLength(CONST.MAX_COMMENT_LENGTH); | |
| } else { | |
| setExceededMaxLength(null); | |
| } |
I think we can use nested if/else to read code easier here. What do you think?
There was a problem hiding this comment.
@hoangzinh Although the lint check will pass, I believe we may prefer an early return?
There was a problem hiding this comment.
We prefer an early return if it's only 1 if. For example
if (true) {
// then do something
}There was a problem hiding this comment.
Ah, I see, updated
Co-authored-by: Vinh Hoang <[email protected]>
…x/50398-fix-max-length-validation-for-task
…x/50398-fix-max-length-validation-for-task
| setIsCreatingTaskComment(!!taskCommentMatch); | ||
| if (taskCommentMatch) { | ||
| const title = taskCommentMatch?.[3] ? taskCommentMatch[3].trim().replace(/\n/g, ' ') : ''; | ||
| validateTaskTitleMaxLength(title); |
There was a problem hiding this comment.
instead of creating a new state isCreatingTaskComment, should we reset hasExceededMaxCommentLength=false here. Also set hasExceededMaxTaskTitleLength in below condition.
There was a problem hiding this comment.
@hoangzinh sure, updated
There was a problem hiding this comment.
Not sure why perf-tests failed, it pass in local
There was a problem hiding this comment.
forgot to set baseline, here is the result
@hoangzinh do you know what that means? It seems our change is causing extra rendering, right?
There was a problem hiding this comment.
It can be because our new deboundValidate haven't been wrapped in useCallback?
There was a problem hiding this comment.
I see it's because of useDebounce is not ready yet when reload the page. Okay, let back to use useMemo and useCallback.
There was a problem hiding this comment.
@hoangzinh We still utilize lodashDebounce. please let me know if you have other feedback
There was a problem hiding this comment.
ah yes, it's correct. I'm assuming it would be same as #52941 (comment)
There was a problem hiding this comment.
oops sorry, I thought this PR was not ready for next review. I will review it today
…x/50398-fix-max-length-validation-for-task
…x/50398-fix-max-length-validation-for-task
…x/50398-fix-max-length-validation-for-task
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-06.at.17.27.33.android.movAndroid: mWeb ChromeScreen.Recording.2024-12-06.at.17.44.32.android.chrome.moviOS: NativeScreen.Recording.2024-12-06.at.17.50.18.moviOS: mWeb SafariScreen.Recording.2024-12-06.at.17.46.27.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2024-12-06.at.17.17.49.web.movMacOS: DesktopScreen.Recording.2024-12-06.at.17.22.22.desktop.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.74-8 🚀
|
| * Group 2: Optional email group between \s+....\s* start rule with @+valid email or short mention | ||
| * Group 3: Title is remaining characters | ||
| */ | ||
| TASK_TITLE_WITH_OPTONAL_SHORT_MENTION: `^\\[\\]\\s+(?:@(?:${EMAIL_WITH_OPTIONAL_DOMAIN}))?\\s*([\\s\\S]*)`, |
There was a problem hiding this comment.
This caused an issue
We should use EMAIL_WITH_OPTIONAL_DOMAIN.source here
Explanation of Change
This PR will add task title validation on main composer text change to prevent server errors after submission.
Fixed Issues
$ #50398
PROPOSAL: #50398 (comment)
Tests
The maximum task title length is 100 characters.The maximum comment length is 10000 characters.101 Character Text
10001 Character Text
Offline tests
Same as Test
QA Steps
Same as Test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Kapture.2024-11-22.at.10.44.37.mp4
Android: mWeb Chrome
Kapture.2024-11-22.at.10.47.30.mp4
iOS: Native
Kapture.2024-11-22.at.10.33.03.mp4
iOS: mWeb Safari
Kapture.2024-11-22.at.10.34.57.mp4
MacOS: Chrome / Safari
Kapture.2024-11-22.at.10.25.10.mp4
MacOS: Desktop
Kapture.2024-11-22.at.10.51.48.mp4