[Discussion] Rewrite Comment Editor with React

[noi5e]

Hooray! The first part of the React rewrite of the Comment System was merged in #9176.

Currently, the React system can do CRUD of comments. React and webpacker were installed in this phase.

There's an issue open here for Phase 2, which will measure performance with system tests, and add further functionality like emoji reactions, comment preview, etc: #9365

Let's see how this wild experiment goes! I installed React & Webpack on a development branch, and I'm going to see if I can reproduce the Comment Editor using React.

Doing all my work in Draft PR #9176 and in the branch at react-comments

The purpose of this is to figure out:

1. Does React offer any significant advantage in terms of state management? As we've seen, Comment Editor state can get kind of complicated. And is this advantage worth it, weighing the cost-benefit of adding a new framework to the stack?
2. If this is a worthwhile project, what other parts of the site can be given this treatment?

Also Done in PR #9176

• put static initialProps that don't change into a context provider
• use ActionController::Base.helpers.<helper> to add view helpers to the controller
• figure out a way (via JS or Rails's <%= raw %>) to encode comment text properly
• undo nested comments state!
• refactor CommentsContainer helper functions for reduced complexity
• populate state formToggleState and textAreaValue when fresh comment is posted
• refactor so setTextAreaValues isn't passed down 5-7 components
• create CommentReplies in CommentsList, to avoid passing down props
• wrap CommentsContainer in new App component
• pass parameters to formSubmit and buttonClick functions (not event.target.dataset)
• set up guest (not logged-in) browsing
• reply form toggle displays link to login if no currentUser
• create unique routes for CRUD functions, ie. /comment/REACT/create/46

---

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

[noi5e]

Installation went smoothly, which is nice! Here's what I have so far on the upstream/react-comments branch:

1. Visit any note (not question or wiki) page, and pass in the parameter react=true in the URL.
2. Hello React!!! is rendered instead of the comment editor!

[noi5e]

Lots of errors in CI for the draft PR #9176:

The errors refer to /public/packs-test/manifest.json, which exists, but is .gitignored locally. This probably doesn't exist in the testing environment, so that's what's causing the errors. This works fine locally however!

[noi5e]

This is coming along nicely. I have a list component that is able to iterate over an array of comment objects:

As you can see, the rendering is very weird with <p>comment text</p>. It probably has something to do with encoding when going from Rails -> JSON -> React.

Pre-Formatting State for React

From reading the docs for rails-react, I believe that these React components don't have access to Rails methods that views normally do (like node#method or order_by, etc.)

So we have to work around that by running all of our Rails methods on the state object before we feed it into the CommentsContainer component. That's what this code is doing:

plots2/app/views/notes/show.html.erb

Lines 119 to 140 in ff75433

	<% if !@preview %>
	<%= render partial: "notes/responses" %>
	<% if @react %>
	<% comments = @node.comments_viewable_by(current_user).includes([:replied_comments, :node]).order('timestamp ASC').to_a %>
	<%# run rails methods on comments %>
	<%# can't access rails methods within %>
	<% comments.collect! do |comment| %>
	<% comment_body = comment.render_body %>
	<% comment_text = raw insert_extras(filtered_comment_body(comment_body)) %>
	<% comment = comment.as_json %>
	<% comment.merge({ "commentText" => comment_text }) %>
	<% end %>
	<%= react_component("CommentsContainer", {
	comments: comments,
	commentsHeaderText: translation('notes._comments.comments')
	}) %>
	<% else %>
	<div id="legacy-editor-container"><%= render partial: "notes/comments" %></div>
	<% end %>
	<% end %>
	</div>

(Don't worry, I will move this bulky block into application_helper.js when I'm done.)

This is kind of annoying to write this code, but it helps to anticipate the kind of state the views must have.

Server-Side Rendering

I don't know much about this aspect of React, but rails-react also has the option to render components server-side within the controller. See here and here. This would improve speed and performance a lot!

[noi5e]

Accessing comment.author

@jywarren Maybe you can help me here? I'm trying to write the React equivalent of the code here in the notes/_comment partial (looking up the author of the comment):

plots2/app/views/notes/_comment.html.erb

Lines 18 to 30 in ff75433

	<div class="navbar-text float-left">
	<% if comment.author %>
	<% if comment.author && comment.author.photo_file_name %>
	<img alt="Comment Author Profile Picture" style="width:32px;margin-right:6px;" class="rounded-circle" src="<%= comment.author.photo_path(:thumb) %>" />
	<% else %>
	<div style="vertical-align:middle;display:inline-block;height:32px;width:32px;margin-right:6px;background:#ccc;" class="rounded-circle"></div>
	<% end %>
	<% if comment.author.name != "" %>
	<a href="/profile/<%= comment.author.name %>"><%= comment.author.name %></a>
	<% else %>
	<%= comment.name %>
	<% end %>
	<% end %>

The problem is that the React state of comment looks like this in my app:

{
  "comments": [
    { 
      "comment": {
        "cid":2,
        "pid":0,
        "nid":46,
        "uid":1,
        "subject":"",
        "comment":"Woohoo, here's an awesome comment!",
        "hostname":"",
        "timestamp":1613426027,
        "status":1,
        "format":1,
        "thread":"01/",
        "name":null,
        "mail":null,
        "homepage":null,
        "aid":0,
        "comment_via":0,
        "message_id":null,
        "tweet_id":null,
        "reply_to":null,
        "flag":0},
      "commentText":"\u003cp\u003eWoohoo, here's an awesome comment!\u003c/p\u003e\n"
    }
  ],
  "commentsHeaderText": "Comments"
}

In other words, there isn't any author key present in the comment object.

Here's where the notes/_comment partial is rendered in the notes/_commentS partial:

plots2/app/views/notes/_comments.html.erb

Lines 2 to 13 in ff75433

	<div id="comments" class="col-lg-10 comments">
	<% comments = @node.comments_viewable_by(current_user) %>
	<h3><span id="comment-count"><%= comments.size %></span> <%= translation('notes._comments.comments') %></h3>
	<%= javascript_include_tag "editorToolbar" %>
	<%= javascript_include_tag "comment" %>
	<div style="margin-bottom: 50px;" id="comments-list">
	<% comments.includes([:replied_comments, :node]).order('timestamp ASC').each do |comment| %>
	<% if comment.cid == @node.comments&.first&.cid %><a id="last" name="last"></a><% end %>
	<% if comment.reply_to.nil? %>
	<%= render :partial => "notes/comment", :locals => {:comment => comment} %>
	<% end %>

1. comments is at first comments = @node.comments_viewable_by(current_user)
2. it's passed to the notes/_comment partial here: <%= render :partial => "notes/comment", :locals => {:comment => comment} %>
3. somehow comment.author is accessible here: <% if comment.author %>

I'm not able to access comment.author when doing the same things in React. What am I missing here? How is the author property of the comment node defined in Rails? Is it some kind of lookup between two database tables? And where is that defined exactly when comments is first defined as comments = @node.comments_viewable_by(current_user) in the notes/_comments partial above?

[jywarren]

OK, so i'm thinking on how to "pack" the association (which in this case is not a real relation but just a query for User.find(uid) in concerns/comments_shared.rb -- and the example i can think of is in MapKnitter:

https://github.com/publiclab/mapknitter/blob/3d8906e7f1babde58a8500c9d62895c7c7924be3/app/models/warpable.rb#L34-L49

This packs up whatever you want into a Hash format, which can then have .to_json run upon it:

https://github.com/publiclab/mapknitter/blob/3d8906e7f1babde58a8500c9d62895c7c7924be3/app/controllers/images_controller.rb#L36

Keep in mind this is a little more convoluted because in Ruby unlike JS we can't just assign arbitrary methods or attributes to an existing object, which is why we make a new object. So you might do something like (i think this should work):

{
  author => comment.author
}.to_json

Although there are probably other ways to do it too!

I'd be careful to try to do this only for the JSON response you're handing back to React, as it does add complexity and cost to the query if used in other scenarios where this wasn't deemed necessary. Are you doing it in a new controller action?

Hope this helps!

[noi5e]

@jywarren Thanks, yeah that is helpful! I think that will help once I definitively know what needs to be in the massive state object that Rails hands off to React. I'll probably write something similar.

I thought of another small question. Take a look at these lines I posted above from the new React revision:

plots2/app/views/notes/show.html.erb

Lines 128 to 129 in ff75433

	<% comment = comment.as_json %>
	<% comment.merge({ "commentText" => comment_text }) %>

I noticed that I wasn't able to set commentText by doing comment.commentText = "string". Hence why I call comment.as_json, which translates comment to a Hash (it starts off as an ActiveRecord object, which is immutable). Then merge, which adds a new key => value pair to the Hash. I'd like to know if there's a more practical way to do this. I'm still learning Ruby!

As you can see, there's a big up-front cost to making the massive state object before it's handed off to React. To answer your question, this isn't being done in the controller action, but in the view. I'm just getting this up and running first, then I'm sure it can easily be moved into the controller.

Just to reiterate, it's also possible to render all the components server-side, which would boost performance by a lot!

[jywarren]

Hmm, interesting, so what are you aiming to do with the line comment.merge({ "commentText" => comment_text })? this would be a change only after the comment is fetched from the database and before it's rendered to the view, but it's not a persistent change. You can overwrite comment.commentText but you need to then run comment.save (assuming comment is a full Ruby/ActiveRecord object) for it to save to the database. Otherwise your change is only to the in-memory copy of the record. Does that help?

[noi5e]

It's like this:

1. comment right out of the box is I believe an ActiveRecord object. I can read values from it, but I can't write anything to it (ie. comment[:commentText] = "string" returns error)
2. So that's why I transform it into a Hash object here: comment = comment.as_json
3. And then add a new key => value pair here: comment.merge({ "commentText" => comment_text })

I'm sure there's a much simpler way to do this, probably the MapKnitter example where I just construct the JSON object from scratch. Also this will all eventually end up in the controller so as not to complicate the views too much.

I just want to know what the best way to do this in Ruby is. I'm more used to JavaScript where you can willy-nilly add keys and values to objects, so this is a little new for me.

[jywarren]

Ok, do you want to permanently write to the record, so it's saved in the database, or just to the temporary copy in memory, and it's ok if the comment doesn't retain the change?

[noi5e]

The second! 😊 Definitely don't want to save anything to the database, I just want to package up everything that React needs in a JSON state object. This requires translating the ActiveRecord thingies into objects I can write values into.

[jywarren]

Yes then your code looks good! I had to do something similarly odd to achieve the same in the mapknitter code but there are multiple ways to do it and you're right that the hard part is getting around the inability t write arbitrary attributes in Ruby.

[jywarren]

And i don't know what the most legible or standard way of doing this is but since it looks a bit odd maybe we should leave a comment like "temporarily add in extra attribute to a copy of the record in memory" or something?

[noi5e]

Yeah, definitely going to make comments all over the place. After I make some decent progress, I'll move this all to the controller and just make a fresh Hash object to hold all the JSON. Similar to the MapKnitter example. I think that will be a lot easier to read.