Skip to content

Fix StagingDeployBlocker where chat draft message is kept in the test input while switching chats #4761

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

Merged
HorusGoul merged 2 commits into from
Aug 20, 2021
Merged

Fix StagingDeployBlocker where chat draft message is kept in the test input while switching chats #4761

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e79850
Update the comment in text input manually in componentUpdate
mountiny Aug 20, 2021
9135cfb
Remove unnecessary check
mountiny Aug 20, 2021
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
6 changes: 6 additions & 0 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ class ReportActionCompose extends React.Component {
&& prevProps.modal.isVisible && !this.props.modal.isVisible) {
this.focus();
}

// If we switch from a sidebar, the component does not mount again
// so we need to update the comment manually.
if (prevProps.comment !== this.props.comment) {
this.textInput.setNativeProps({text: this.props.comment});
}

@marcaaron marcaaron Aug 20, 2021

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.

What about updating this.comment as well in this block or does that not matter? Honestly, I don't totally get why we started using setNativeProps() for this in the first place (I think to fix some bug) - but seeing this updateComment() makes me think it should be the one used here:

App/src/pages/home/report/ReportActionCompose.js

Lines 312 to 320 in 9135cfb

updateComment(newComment) {
this.textInput.setNativeProps({text: newComment});
this.setState({
isCommentEmpty: newComment.length === 0,
});
this.comment = newComment;
this.debouncedSaveReportComment(newComment);
this.debouncedBroadcastUserIsTyping();
}

@mountiny mountiny Aug 20, 2021

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.

I see, I have not considered the updateComment(), but I agree now it might be better. I was trying to go with as small change as possible because I have tried updating this.comment too, but it worked the same as without explicitly updating it, so I took it away.

I can submit a PR to use updateComment function and leave it in main and not CP as the functionality is same for now (at least what I could have seen myself).

I thought this would only be a temporary workaround because the componentDidUpdate function is called many times so we should aim to put as little logic in there as possible.

@marcaaron marcaaron Aug 20, 2021

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.

I can submit a PR to use updateComment function and leave it in main and not CP as the functionality is same for now (at least what I could have seen myself).

That sounds good to me. One reason why I would maybe use this is that if we refactor or change the setNativeProps thing in the future there will be only one place where we did it. Or said another way setting the native props is feels like more of a "detail" of the "update comment" code we are running.

I thought this would only be a temporary workaround because the componentDidUpdate function is called many times so we should aim to put as little logic in there as possible.

I'm curious what would the real solution be? It's fine to have logic in this method as long as we check when we should run it e.g. if we only need this to happen when the report switches then maybe it would be better to look at another prop besides the comment. I have one in mind... :)

@mountiny mountiny Aug 20, 2021 •
edited
Loading

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.

Created a PR here 🙌

Or said another way setting the native props is feels like more of a "detail" of the "update comment" code we are running.

You are right there could be some hidden problem along the way by using solely the ref to update it. I think the reason it work d is that the component gets to the correct state eventually, but there is the couple of renders, which are still with the old reportID, which are causing the initial bug.

I have one in mind... :)

Thank you for the sneaky hint :) I have used this.props.report.reportID, because the reportID changes before the this.props.report is updated to the correct one and thus the bug would prevail.

@marcaaron marcaaron Aug 20, 2021

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.

but there is the couple of renders, which are still with the old reportID, which are causing the initial bug.

Let's discuss this in the new PR. I'm not sure I am getting why the reportID itself could not be used 🤔

}

componentWillUnmount() {
Expand Down