Use House for MD comments #144

josefarias · 2024-11-28T01:02:58Z

composing	rendered

Deviations from other House implementations:

Uploads are associated to the account, instead of the markdown record
- Fizzy's markdown records don't exist as you're composing them (e.g. for new comments), contrary to Writebook
Added a plaintext renderer (for searching content)
- Added convenience methods for #{name}_html and #{name}_plain_text
Markdown is sanitized, since it's no longer necessarily trusted
Removed embeds for now
- Wasn't causing any issues, we just don't need it (yet)
Fixed a bug with slug_url's default params
Some refactoring around specifying and calling a markdown renderer (to make it agnostic to the renderer's interface — previously it had to respond to #render), and refactored our bespoke MarkdownRenderer class

josefarias · 2024-11-30T00:49:55Z

lib/rails_ext/active_storage_sluggable.rb

+
+  private
+    def set_slug
+      self.slug = "#{slug_basename}-#{SecureRandom.alphanumeric(6)}.#{slug_extension}"


Perhaps too easy to guess? Would still require a ton of tries, which we would presumably catch and block. But still.

I think the only benefit to not have this be a long thing with a ton of entropy is that we'll be displaying this after attaching something in the markdown. It's nice to see a url that's easy to grok.

Yeah. That's about 56 billion possibilities per name, so not tiny, but still.

We could always make it a little longer but use dashed groups to make it more readable? 3 groups of 4 would give you names more like charlie-bk8e-pK79-MA25 which doesn't look too bad?

Oh you're right! I should've done the math there. I thought the number was way smaller. I think my brain forgot these are alphanumeric 😂.

The dashed groups do look nice. But thinking more about it, if someone is able to execute ~50 billion guesses without us stopping them then we might have bigger issues at hand. Probably good to leave as-is then.

* main: Adjust position of settings buttons Actually, no need for a max-size, just let it grow Improve comment form design Same style as combobox pop-ups Ensure bubbles are clickable anyplace in windshield Deal with truncation in the list layout, too Ensure tags don't affect the bubble shape or spill out of the container Style pop-up UI for selecting assignments and tags Adjust position now that bubbles won't be so vertical Fix that bubbles could collapse in some situations dep: bump rexml to address dependabot alert dep: bump rails to edge

* main: Bump rails from `91d4563` to `5520bd8` dep: bundle update everything except rails dep: update sqlite3 gem to v2.4.1

* main: (23 commits) Improve the flow for editing bubble titles Use path helpers in tests Revising access shouldn't do a replace turbo-action Split into new tests Update this test, too Adjust tests to match new behavior Reapply "Destroy equivalent filters upon resource removal" Revert "Destroy equivalent filters upon resource removal" Destroy equivalent filters upon resource removal Don't autocomplete Rework boosts form so you can enter any integer Pull out access_menu_tag Punt on removing filters for inaccessible buckets Can't see bubbles in buckets you've lost access to Users can remove themselves from buckets Fix redirect assertion Fix redirect assertion Submit form when toggling all-access + caching Unnecessary parens Can't revoke access to all-access bucket ...

* main: Update test Wire up comment deletion Whitespace Wire up editing comments Open the title field for editing after creating a new bubble

josefarias · 2024-12-18T02:47:15Z

@kevinmcconnell ran out of time today to rip out the custom renderer. But using Redcarpet would just be a matter of changing the config. Everything else would stay the same.

CI is failing because it can't pull the gem from the private House repo.

kevinmcconnell

This looks good so far!

I think, as discussed previously, it's probably still worth splitting the House rendering work to its own branch while you explore that. Unless you're confident that it's definitely the right approach. But I think what I'd be trying to answer there is: what limitations are there in a hand-built Markdown renderer, compared to something like Redcarpet?

My hunch is that there are a bunch of edge cases to consider with Markdown rendering, especially with things like formatting that's inside other formatting, and so on. House is able to render some reasonable subset of Markdown in order to provide format previews while you're editing. But it's OK if it doesn't support everything. You can still put in any Markdown text you want, and even if it's not something House knows how to deal with it'll still work -- it just might not get previewed. But, once you actually save some markdown content, you'd want that to render accurately.

But I haven't looked closely at this, so maybe that's a bad assumption on my part! I'd be interested to hear what you find out about it.

kevinmcconnell · 2024-12-18T12:43:09Z

app/models/comment.rb

  belongs_to :creator, class_name: "User", default: -> { Current.user }

-  searchable_by :body, using: :comments_search_index
+  searchable_by :body_plain_text, using: :comments_search_index, as: :body


We could also make the FTS field match the name of the method we're indexing, and then wouldn't need the as parameter.

Ah yes, as in renaming the db column for the search table?

I considered that but I thought _as_plain_text was a sort of implementation detail. With as: we can keep the semantic meaning in the search table that we're searching the comment's body, regardless of how we serialized it for indexing.

We can also swap out the method without involving a migration. Although I don't expect that to be common.

kevinmcconnell · 2024-12-18T12:43:50Z

app/models/concerns/searchable.rb

-    def searchable_by(field, using:)
-      define_method :search_field do field; end
+    def searchable_by(field, using:, as: field)
+      define_method :search_value do send(field); end


Good idea to define this search_value method! 👏

kevinmcconnell · 2024-12-18T12:50:35Z

config/initializers/extensions.rb

-%w[ rails_ext ].each do |extensions_dir|
-  Dir["#{Rails.root}/lib/#{extensions_dir}/*"].each { |path| require "#{extensions_dir}/#{File.basename(path)}" }
-end
+Dir["#{Rails.root}/lib/rails_ext/*"].each { |path| require "rails_ext/#{File.basename(path)}" }


I'm just curious -- is this changing because we didn't have any extensions for other libraries yet? The previous was just in there because it was brought in from another project?

Yes! I brought it over from some other project but in re-evaluating, I'm not expecting to extend anything else. So just a very minor nit.

We can revert to using an array if we ever extend something other than Rails.

kevinmcconnell · 2024-12-18T13:01:50Z

lib/rails_ext/active_storage_sluggable.rb

+
+  private
+    def set_slug
+      self.slug = "#{slug_basename}-#{SecureRandom.alphanumeric(6)}.#{slug_extension}"


Yeah. That's about 56 billion possibilities per name, so not tiny, but still.

We could always make it a little longer but use dashed groups to make it more readable? 3 groups of 4 would give you names more like charlie-bk8e-pK79-MA25 which doesn't look too bad?

kevinmcconnell · 2024-12-18T13:03:23Z

lib/rails_ext/active_support_array_conversions.rb

+module ChoiceSentenceArrayConversion
+  def to_choice_sentence
+    to_sentence two_words_connector: " or ", last_word_connector: ", or "


Is this an unrelated change?

Sort of – this dir was previously autoloaded. Which required the constant name to match the file path.

After adding many more extensions to the dir (for ActionText) it made more sense to not autoload. So changing that setting in application.rb triggered this diff tangentially.

josefarias · 2024-12-18T15:20:33Z

Thanks for the review @kevinmcconnell!

Right! I'll replace the custom renderer with redcarpet today and merge. The tests will pass then, and the diff will be pretty minor (just a config change and minor syntax changes in the MardownRenderer).

As for the benefit of having a custom renderer – I think it'd be a library benefit more than a product one. It allows us to provide a zero-config default answer for rendering ActionText::Markdown (more here).

I hear ya about the edge cases. I was able to get pretty far with a day's worth of effort so I'm optimistic it might not be super hard. But I might be wrong of course! And the edge cases might very well stop that project in its tracks. They haven't yet, though!

* main: Add hotwire-spark Fix for Safari which doesn't support `field-sizing: content` Add button and action to delete a bubble Indicate draggable element Stub draggable collection size divider Make close and trash more distinct

Jose Farias added 9 commits November 27, 2024 18:59

Use House for MD comments

d556e21

Support search on markdown fields

45e94e4

Add .heading__link styles

84c11e1

Sanitize comment html

ce3db05

Implement lightbox

0e8d410

Unnest uploads controller

6164da2

Add syntax highlighting

8a5b672

Sanitize rendered markdown

2b9c4d9

Test UploadsController

22d59ba

josefarias requested a review from kevinmcconnell November 30, 2024 00:47

josefarias commented Nov 30, 2024

View reviewed changes

Jose Farias added 11 commits December 9, 2024 10:39

Update dependencies

12b073b

Merge branch 'main' into house

39421ba

* main: Bump rails from `91d4563` to `5520bd8` dep: bundle update everything except rails dep: update sqlite3 gem to v2.4.1

Install house deps

3ca31e0

Spike potential redcarpet replacement

4fc7ab1

Merge branch 'main' into house

fdea6d4

* main: Update test Wire up comment deletion Whitespace Wire up editing comments Open the title field for editing after creating a new bubble

Use MD for comment CRUD

9ce4a73

Put bracket on new line

b88e56e

Use house ssr

cb1686d

Remove redcarpet

a7df0cc

kevinmcconnell reviewed Dec 18, 2024

View reviewed changes

Jose Farias added 3 commits December 18, 2024 16:27

Merge branch 'main' into house

0d2e4ed

* main: Add hotwire-spark Fix for Safari which doesn't support `field-sizing: content` Add button and action to delete a bubble Indicate draggable element Stub draggable collection size divider Make close and trash more distinct

Use redcarpet as markdown renderer

0139f71

Use Redcarpet for plain text rendering

7d83874

josefarias merged commit bd28ea8 into main Dec 18, 2024
4 checks passed

josefarias deleted the house branch December 18, 2024 23:40

Use House for MD comments #144

Use House for MD comments #144

Uh oh!

Conversation

josefarias commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josefarias Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josefarias commented Dec 18, 2024

Uh oh!

kevinmcconnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josefarias commented Dec 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

josefarias commented Nov 28, 2024 •

edited

Loading

josefarias Nov 30, 2024 •

edited

Loading