Reworked how notes work

TODO: clean this up

Author: dutow

app/controllers/notes_controller.rb

@@ -4,39 +4,48 @@ class NotesController < ApplicationController
   before_action :require_authentication
   before_action :set_note, only: [:update]
 
+  def new
+    topic = Topic.find(params[:topic_id])
+    message = resolve_message(topic)
+    respond_to do |format|
+      format.turbo_stream { render_note_stack_stream(topic:, message:, open_form: true) }
+      format.html { redirect_to topic_path(topic, anchor: message ? note_frame_id(message) : "thread-notes") }
+    end
+  end
+
+  def stack
+    topic = Topic.find(params[:topic_id])
+    message = resolve_message(topic)
+    respond_to do |format|
+      format.turbo_stream { render_note_stack_stream(topic:, message:, open_form: false) }
+      format.html { redirect_to topic_path(topic, anchor: message ? note_frame_id(message) : "thread-notes") }
+    end
+  end
+
   def create
     topic = Topic.find(note_params[:topic_id])
     message = resolve_message(topic)
     note = NoteBuilder.new(author: current_user).create!(topic:, message:, body: note_params[:body])
 
-    redirect_to topic_path(topic, anchor: note_anchor(note)), notice: "Note added"
+    respond_to do |format|
+      format.html { redirect_to topic_path(topic, anchor: note_anchor(note)), notice: "Note added" }
+      format.turbo_stream { render_note_stack_stream(topic:, message:, open_form: false) }
+    end
   rescue NoteBuilder::Error, ActiveRecord::RecordInvalid => e
-    flash[:alert] = e.message
-    flash[:note_error] = {
-      body: note_params[:body],
-      message_id: note_params[:message_id].presence,
-      topic_id: note_params[:topic_id],
-      error: e.message
-    }
-    redirect_back fallback_location: topic_path(topic)
+    handle_note_error(topic:, message:, error: e)
   end
 
   def update
     return if performed?
 
     NoteBuilder.new(author: current_user).update!(note: @note, body: note_params[:body])
 
-    redirect_to topic_path(@note.topic, anchor: note_anchor(@note)), notice: "Note updated"
+    respond_to do |format|
+      format.html { redirect_to topic_path(@note.topic, anchor: note_anchor(@note)), notice: "Note updated" }
+      format.turbo_stream { render_note_stack_stream(topic: @note.topic, message: @note.message, open_form: false) }
+    end
   rescue NoteBuilder::Error, ActiveRecord::RecordInvalid => e
-    flash[:alert] = e.message
-    flash[:note_error] = {
-      body: note_params[:body],
-      message_id: note_params[:message_id].presence,
-      topic_id: note_params[:topic_id],
-      note_id: @note.id,
-      error: e.message
-    }
-    redirect_back fallback_location: topic_path(@note.topic)
+    handle_note_error(topic: @note.topic, message: @note.message, error: e, note: @note)
   end
 
   private
@@ -54,8 +63,9 @@ def note_params
   end
 
   def resolve_message(topic)
-    return nil if note_params[:message_id].blank?
-    topic.messages.find(note_params[:message_id])
+    msg_id = params[:message_id].presence || params.dig(:note, :message_id)
+    return nil if msg_id.blank?
+    topic.messages.find_by(id: msg_id)
   end
 
   def note_anchor(note)
@@ -65,4 +75,47 @@ def note_anchor(note)
       "thread-notes"
     end
   end
+
+  def note_frame_id(message)
+    message ? "notes-message-#{message.id}" : "thread-notes"
+  end
+
+  def note_collection(topic:, message:)
+    Note.where(topic:, message:).includes(:author, :note_tags, :note_mentions).order(:created_at)
+  end
+
+  def render_note_stack_stream(topic:, message:, open_form:, status: :ok)
+    notes = note_collection(topic:, message:)
+    render(
+      turbo_stream: turbo_stream.replace(
+        note_frame_id(message),
+        partial: "notes/note_stack",
+        locals: { topic:, message:, notes:, open_form: }
+      ),
+      status: status
+    )
+  end
+
+  def handle_note_error(topic:, message:, error:, note: nil)
+    flash_payload = {
+      body: note_params[:body],
+      message_id: note_params[:message_id].presence,
+      topic_id: note_params[:topic_id],
+      note_id: note&.id,
+      error: error.message
+    }
+
+    respond_to do |format|
+      format.html do
+        flash[:alert] = error.message
+        flash[:note_error] = flash_payload
+        redirect_back fallback_location: topic_path(topic)
+      end
+      format.turbo_stream do
+        flash.now[:alert] = error.message
+        flash.now[:note_error] = flash_payload
+        render_note_stack_stream(topic:, message:, open_form: true, status: :unprocessable_entity)
+      end
+    end
+  end
 end

app/javascript/controllers/note_loader_controller.js

@@ -0,0 +1,39 @@
+import { Controller } from "@hotwired/stimulus"
+import { Turbo } from "@hotwired/turbo-rails"
+
+export default class extends Controller {
+  static values = {
+    url: String
+  }
+
+  async load(event) {
+    event.preventDefault()
+    if (!this.hasUrlValue) return
+
+    const response = await fetch(this.urlValue, {
+      headers: { Accept: "text/vnd.turbo-stream.html" }
+    })
+    if (response.ok) {
+      const html = await response.text()
+      Turbo.renderStreamMessage(html)
+    }
+  }
+
+  close(event) {
+    event.preventDefault()
+    const formWrapper = this.element.closest(".note-form-wrapper")
+    if (formWrapper) {
+      const container = formWrapper.closest(".notes-block")
+      formWrapper.remove()
+      if (container && container.querySelectorAll(".note-card").length === 0 && !container.querySelector(".note-form-wrapper")) {
+        container.remove()
+      }
+      return
+    }
+
+    const details = this.element.closest("details")
+    if (details && details.open) {
+      details.open = false
+    }
+  }
+}

app/views/notes/_add_button.html.slim

@@ -0,0 +1,9 @@
+- note_dom_id = message ? "notes-message-#{message.id}" : "thread-notes"
+- button_classes = ["note-add-link"]
+
+= button_tag "Add note",
+  type: "button",
+  class: button_classes.join(" "),
+  data: { controller: "note-loader",
+          action: "click->note-loader#load",
+          note_loader_url_value: new_note_path(topic_id: topic.id, message_id: message&.id, format: :turbo_stream) }

app/views/notes/_form.html.slim

@@ -18,3 +18,6 @@
       .note-error = note_error[:error]
   .note-actions
     = f.submit submit_label, class: "button-secondary"
+    = button_tag "Cancel", type: "button", class: "button-secondary",
+        data: { controller: "note-loader",
+                action: "click->note-loader#close" }

app/views/notes/_note_stack.html.slim

@@ -1,27 +1,19 @@
 - notes ||= []
 - note_dom_id = message ? "notes-message-#{message.id}" : "thread-notes"
-- notes ||= []
-- note_error = flash[:note_error]&.with_indifferent_access
-- open_form = note_error && note_error[:topic_id].to_i == topic.id && note_error[:message_id].to_s == message&.id.to_s
-- show_full = notes.any? || open_form
-- note_dom_id = message ? "notes-message-#{message.id}" : "thread-notes"
+- open_form ||= false
+
+= turbo_frame_tag note_dom_id do
+  - if open_form || notes.any?
+    .notes-block id=note_dom_id
+      - if open_form
+        .note-form-wrapper
+          = render "notes/form", topic: topic, message: message
 
-- if show_full
-  .notes-block id=note_dom_id
-    - if notes.any?
-      .notes-header
-        - title_text = message ? "Notes on this message" : "Thread notes"
-        h4 = title_text
-        span.notes-count = "#{notes.size} note#{'s' if notes.size != 1}"
-      .notes-list
-        - notes.each do |note|
-          = render "notes/note", note: note
-    - if user_signed_in? && topic
-      details.note-form-toggle open=(open_form ? true : nil)
-        summary Add note
-        = render "notes/form", topic: topic, message: message
-- elsif user_signed_in? && topic
-  .notes-inline-add id=note_dom_id
-    details.note-form-toggle
-      summary Add note
-      = render "notes/form", topic: topic, message: message
+      - if notes.any?
+        .notes-header
+          - title_text = message ? "Notes on this message" : "Thread notes"
+          h4 = title_text
+          span.notes-count = "#{notes.size} note#{'s' if notes.size != 1}"
+        .notes-list
+          - notes.each do |note|
+            = render "notes/note", note: note

app/views/topics/_message.html.slim

@@ -29,6 +29,9 @@
       - if message.attachments.any?
         .attachment-indicator
           span.attachment-count #{message.attachments.size} attachment(s)
+      - if user_signed_in?
+        .note-actions-inline
+          = render "notes/add_button", topic: @topic, message: message
 
   .message-content class=read_classes.join(" ") data=read_data
     - if message.subject != @topic.title && message.subject.present?
@@ -51,7 +54,5 @@
       details
         summary Import Notes
         pre.import-log = message.import_log
-
   - notes_for_message = @notes_by_message&.[](message.id) || []
-  - if user_signed_in?
-    = render "notes/note_stack", topic: @topic, message: message, notes: notes_for_message
+  = render "notes/note_stack", topic: @topic, message: message, notes: notes_for_message, open_form: false

app/views/topics/show.html.slim

@@ -95,9 +95,11 @@
       span.message-count #{@messages.size} messages
 
 - if user_signed_in?
-  - thread_notes = @notes_by_message&.[](:thread) || []
   .thread-notes-container
-    = render "notes/note_stack", topic: @topic, message: nil, notes: thread_notes
+    .note-actions-inline
+      = render "notes/add_button", topic: @topic, message: nil
+    - thread_notes = @notes_by_message&.[](:thread) || []
+    = render "notes/note_stack", topic: @topic, message: nil, notes: thread_notes, open_form: false
 
 .messages-container class=(["messages-container", @view_mode, (@sort_order == :desc ? "desc" : "asc")].compact.join(" "))
   - if user_signed_in?

config/routes.rb

@@ -35,7 +35,11 @@
   resources :activities, only: [:index] do
     post :mark_all_read, on: :collection
   end
-  resources :notes, only: [:create, :update]
+  resources :notes, only: [:create, :update, :new] do
+    collection do
+      get :stack
+    end
+  end
 
   # Authentication
   resource :session, only: [:new, :create, :destroy]

spec/requests/notes_turbo_spec.rb

@@ -0,0 +1,41 @@
+require "rails_helper"
+
+RSpec.describe "Turbo note interactions", type: :request do
+  let(:user) { create(:user, password: "password", password_confirmation: "password", username: "tester") }
+  let(:alias_record) { create(:alias, :primary, user:, verified_at: Time.current) }
+  let(:topic) { create(:topic, creator: alias_record) }
+  let(:message) { create(:message, topic:, sender: alias_record) }
+
+  before do
+    alias_record # ensure alias exists
+    post session_path, params: { email: alias_record.email, password: "password" }
+    expect(response).to redirect_to(root_path)
+  end
+
+  it "renders nothing for notes on show page without notes until clicked" do
+    get topic_path(topic)
+    expect(response.body).to include("Add note")
+    expect(response.body).not_to include("note-textarea")
+  end
+
+  it "renders the note form via turbo stream for thread notes" do
+    get new_note_path(topic_id: topic.id, format: :turbo_stream)
+
+    expect(response.media_type).to eq("text/vnd.turbo-stream.html")
+    expect(response.body).to include("turbo-stream", "note-form-wrapper")
+  end
+
+  it "creates a thread note via turbo stream and renders the updated stack" do
+    post notes_path, params: { note: { topic_id: topic.id, body: "Thread note body" } }, headers: { "ACCEPT" => "text/vnd.turbo-stream.html" }
+
+    expect(response.media_type).to eq("text/vnd.turbo-stream.html")
+    expect(response.body).to include("Thread note body")
+  end
+
+  it "creates a message note via turbo stream and renders the updated stack" do
+    post notes_path, params: { note: { topic_id: topic.id, message_id: message.id, body: "Message note body" } }, headers: { "ACCEPT" => "text/vnd.turbo-stream.html" }
+
+    expect(response.media_type).to eq("text/vnd.turbo-stream.html")
+    expect(response.body).to include("Message note body")
+  end
+end

spec/requests/topics_spec.rb

@@ -73,51 +73,23 @@
         expect(response.body).to include(reply_message.body)
       end
 
-      it "shows threaded view by default" do
+      it "shows flat view by default (oldest first)" do
         get topic_path(topic)
-        expect(response.body).to include('messages-container threaded')
-      end
-
-      it "has active Threaded button" do
-        get topic_path(topic)
-        expect(response.body).to include('class="toggle-btn active"')
-      end
-
-      it "has active Oldest First button" do
-        get topic_path(topic)
-        expect(response.body).to include('class="toggle-btn active"')
-      end
-    end
-
-    context "with flat view mode" do
-      it "shows flat view" do
-        get topic_path(topic, view: 'flat')
-        expect(response).to have_http_status(:success)
         expect(response.body).to include('messages-container flat')
-      end
-
-      it "has active Flat button" do
-        get topic_path(topic, view: 'flat')
-        expect(response.body).to include('toggle-btn active')
+        # root should appear before reply
+        root_position = response.body.index(root_message.body)
+        reply_position = response.body.index(reply_message.body)
+        expect(root_position).to be < reply_position
       end
     end
 
     context "with descending sort" do
-      it "shows newest first" do
+      it "still shows oldest first (descending disabled)" do
         get topic_path(topic, view: 'flat', sort: 'desc')
         expect(response).to have_http_status(:success)
-        # Check that reply message appears before root message in HTML
-        reply_position = response.body.index(reply_message.body)
         root_position = response.body.index(root_message.body)
-        expect(reply_position).to be < root_position
-      end
-    end
-
-    context "with invalid view mode" do
-      it "defaults to threaded" do
-        get topic_path(topic, view: 'invalid')
-        expect(response).to have_http_status(:success)
-        expect(response.body).to include('messages-container threaded')
+        reply_position = response.body.index(reply_message.body)
+        expect(root_position).to be < reply_position
       end
     end