Skip to content

Conversation

@sanjiv-saini
Copy link
Contributor

Add error message when message could not be rendered.

@maxceem
Copy link
Collaborator

maxceem commented Jan 8, 2021

Thanks, @sanju-singh. I didn't test it yet, but looking at the source code I have one concern. It seems that error would be catched inside markdownToHtml method, so the error message would be never shown:

image

@sanjiv-saini
Copy link
Contributor Author

sanjiv-saini commented Jan 8, 2021

@maxceem ,

  1. Currently, to render messages on opening, markdownToState method is used to get the links in the messages to show link icon at the right side of message. It is not used to render message.

    • Earlier, due to empty comment, markdownToState method was failing at 'convertFromRaw' method.
    • As we are not sure of possible failures, I have added exception handling logic for convertFromRaw.
  2. Secondly, raw comment in markdown is converted to html to render using markdownToHtml. This is the second place where unexpected break could happen. And this is the place that actually renders the comment, if anything wrong goes here, we show the comment in raw text and show the above mentioned error.

I have tested it by throwing the error from markdownToHtml method. And it is working as expected.

@maxceem
Copy link
Collaborator

maxceem commented Jan 9, 2021

@sanju-singh thanks for the detailed explanation. Though as per my testing the error message is not shown when it suppose to be shown.

Imagine, that we don't fix the error with rendering empty messages, I comment the next lines:
image

So now, when empty message is rendered I'm expecting to see:

  • empty message rendered as a raw text

  • error message shown that message wasn't rendered properly

  • though only empty message is shown without error message:

    image

Another way for testing it. Imagine that method convertFromRaw fails and thorws some error, I can simulate error by adding throw like this:
image

Now if, open a message I would see the message rendered as a raw text, which is correct. But when we render raw message instead of formatted message, we have to show an error message:
image

@sanjiv-saini
Copy link
Contributor Author

sanjiv-saini commented Jan 9, 2021

@maxceem ,
As I mentioned earlier, markdownToState method is not used to render the message, it's used to extract the links in the comment to show link icon like below:
Screenshot 2021-01-09 at 6 01 26 PM

  1. Earlier convertFromRaw in markdownToState method was breaking while extracting links. So, after fix here, if any comment contains link, in case of break link icon will not show up except that everything will work as expected. If that error is not handled it breaks the complete logic initially.
  2. Secondly, markdownToState is used while switching to edit mode. Here also, after exception handling in markdownToState, in the edit mode raw text will show up that user can edit.

For above mentioned case.

  1. Commenting empty message fix:
    - When actually rendering empty message using markdownToHTML, it converts it properly and renders it as expected.
    - What that fix is doing is preventing the exception thrown by convertFromRaw while extracting the links.
    - After commenting, exception handling at the bottom is doing its job and preventing from any break, and comment render logic is anyway working fine.

  2. Throwing exception in markdownToHTML to mimic exception in convertFromRaw.
    - It doesn't affect anything, as this method is used just to extract links.
    - Even we throw error, catch is handling it and we will end up not extracting any links even if comment had it.
    - Rendering logic will not be affect by this, I checked the rendering of blockquote earlier to my fix and it renders like that only. Please use other markdown options to see the change like: bold, list etc.

Conclusion: Solution is working as expected with both the above suggested approach. Blockquote renders like raw text only . Please check with other markdown formatting like: bold, list etc.

@maxceem
Copy link
Collaborator

maxceem commented Jan 9, 2021

@sanju-singh Thank you, got it now.

The task is fixed correctly as per the initial description.

Though it reveled one more issue. The method markdownToState is used not only to extract links but also to format message during editing. If some error happens in markdownToState during editing we would see a raw message inside the editor, which might be confusing.

image

It would be good in such a case to show the same error message during editing like this:

image

If you like you may fix this case as a separate issue #4257 - price $40.

And the current issue is fixed and accepted.

@maxceem maxceem merged commit c536d7b into topcoder-archive:dev Jan 9, 2021
@sanjiv-saini
Copy link
Contributor Author

sanjiv-saini commented Jan 9, 2021

@maxceem ,
As its a markdown editor, showing in raw format (which is in markdown only) under exceptional scenario is valid. And cannot be categories as issue.

Showing error here will be somewhat difficult, as markdownToState method return signature doesn't allow passing error and we are handling the error there itself.

let me know if you still think that we should show error there? I will see for a better solution.

@maxceem
Copy link
Collaborator

maxceem commented Jan 10, 2021

@sanju-singh Thank you for sharing your thoughts. Let me think about it a little bit more.

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.

2 participants