allyourbot / hostedgpt

An open version of ChatGPT you can host anywhere or run locally.
MIT License
192 stars 75 forks source link

Fix Code Rendering Bug for Better UI Clarity #275

Closed jasonpaulso closed 4 weeks ago

jasonpaulso commented 1 month ago

Description:

This pull request aims to resolve an issue where code snippets were not displayed correctly in the user interface. Previously, HTML entities within code were improperly escaped, causing distorted and unclear rendering. My changes ensure that code snippets display accurately, enhancing readability and user experience.

Key Changes:

  1. Sanitization Update: I've added the sanitize method in the block_code function to handle special HTML characters appropriately. This fixes the issue to ensure that code snippets are displayed correctly, without any unintended entity conversions.

Improved Favicon Support: Updated the HTML header to include dynamic links for Apple touch icons used in Progressive Web Apps (PWAs). This ensures that when users add the app to their Apple device's home screen or dock, as I prefer to do, it maintains a consistent branding and appearance.

Visual Representation of Changes:

Before:

Screenshot 2024-04-15 at 1 50 44 PM Screenshot 2024-04-16 at 4 03 37 PM

After the fix:

Screenshot 2024-04-15 at 1 51 26 PM Screenshot 2024-04-16 at 4 01 43 PM

As a new contributor who enjoys using your app, I am excited to have the opportunity to contribute. The issues addressed in this pull request were the first that I noticed and decided to fix. I was not initially aware of the existing list of issues on your roadmap, but I hope my contributions align well with your project's goals. I'm looking forward to your feedback and insights to ensure these modifications meet the standards and objectives of your team.

krschacht commented 1 month ago

Thanks so much! I didn’t get a chance to look through this closely yet, but all these things are great. I too just noticed the sanitized markdown inside code blocks. One thing to flag (which you may have noticed) is that it’s happening both within the triple backtick blocks and within the inline single backticks. My thought on how to address them was to overwrite the redcarpet method within the inherited class. Again, you may have seen both but just wanted to share that after skimming your description. I’ll dig in tomorrow!

jasonpaulso commented 1 month ago

I had considered that re: putting the logic in the class/es, but wanted to make the most minor changes necessary. I had also not noticed the issue with the inline code. These changes should fix the code rendering in both instances. I could not get implement tests however—something with the way that the tests invoke the render() method.

krschacht commented 1 month ago

@jasonpaulso I took a close look and this seems like a good enough fix for us to run with and I can look into tests later on. I really appreciate the help. I’m going to merge in now.

If you’re willing to help with any other tasks, I’m just about ready to freeze v0.6 of the app and so I’ve started tee’ing up some issues for v0.7. If you’re interested in helping with one of these next just comment on the issue and I can assign it to you:

https://github.com/allyourbot/hostedgpt/milestone/6

krschacht commented 1 month ago

@jasonpaulso There is a failing test and I think this may be a legit case that is now broken, but I can’t quite tell.

Since you’re overwriting code_span and block_code methods from redcarpet, I was looking at their source to find the original reference. We should make sure we start with those and only deviate in the small ways needed. I think this might be what’s causing the failure.

This is the original block_code method:

https://github.com/vmg/redcarpet/blob/3e3f0b522fbe9283ba450334b5cec7a439dc0955/lib/redcarpet.rb#L39

And codespan is simply:

      def codespan(code)
        block_code(code, nil)
      end

I think if you start from those, that should unbreak the test. And then you may be able to actually move the unescapeHTML into these methods but if that’s not easy it’s fine to leave it where it is.

jasonpaulso commented 1 month ago

That was helpful. The tests are now passing and both block_code and codespan render as expected. I did try moving the unescapeHTML and .html_safe to CustomHtmlRenderer, however the code still rendered incorrectly.

krschacht commented 4 weeks ago

@jasonpaulso Great, thanks! Merging now