From 19051aec3e9f9d0a38387745ffd431750c912266 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 4 Dec 2025 17:17:55 +0100 Subject: [PATCH 01/90] Append dropped cards instead of relying on server-side rendering to refresh the columns This implements a simple strategy to optimistically insert cards in columns without waiting for the column refresh. Cards will be placed at the top respecting golden cards. This uses sorting by "created at" with sorting by "updated at" for the _Not now_ column, so that the treatment everywhere is homogenous. --- app/assets/stylesheets/card-columns.css | 4 ++ .../boards/columns/not_nows_controller.rb | 2 +- .../boards/columns/not_nows_controller.rb | 2 +- app/helpers/cards_helper.rb | 5 ++- .../controllers/drag_and_drop_controller.js | 43 +++++++++++-------- .../columns/_empty_placeholder.html.erb | 6 +++ app/views/boards/columns/_list.html.erb | 2 +- .../boards/columns/closeds/show.html.erb | 7 +-- .../boards/columns/not_nows/show.html.erb | 7 +-- app/views/boards/columns/show.html.erb | 7 +-- .../boards/columns/streams/show.html.erb | 2 +- 11 files changed, 47 insertions(+), 40 deletions(-) create mode 100644 app/views/boards/columns/_empty_placeholder.html.erb diff --git a/app/assets/stylesheets/card-columns.css b/app/assets/stylesheets/card-columns.css index ac690ac30..a78746fb7 100644 --- a/app/assets/stylesheets/card-columns.css +++ b/app/assets/stylesheets/card-columns.css @@ -402,6 +402,10 @@ } } + .cards__list:has(> :not(.blank-slate)) .card--hide-unless-empty { + display: none; + } + /* TODO: I think this is legacy now? */ .cards__heading { display: flex; diff --git a/app/controllers/boards/columns/not_nows_controller.rb b/app/controllers/boards/columns/not_nows_controller.rb index 6cb765979..0ff41e9ae 100644 --- a/app/controllers/boards/columns/not_nows_controller.rb +++ b/app/controllers/boards/columns/not_nows_controller.rb @@ -2,7 +2,7 @@ class Boards::Columns::NotNowsController < ApplicationController include BoardScoped def show - set_page_and_extract_portion_from @board.cards.postponed.reverse_chronologically.with_golden_first.preloaded + set_page_and_extract_portion_from @board.cards.postponed.latest.preloaded fresh_when etag: @page.records end end diff --git a/app/controllers/public/boards/columns/not_nows_controller.rb b/app/controllers/public/boards/columns/not_nows_controller.rb index d518fb8e0..5a54df171 100644 --- a/app/controllers/public/boards/columns/not_nows_controller.rb +++ b/app/controllers/public/boards/columns/not_nows_controller.rb @@ -1,5 +1,5 @@ class Public::Boards::Columns::NotNowsController < Public::BaseController def show - set_page_and_extract_portion_from @board.cards.postponed.reverse_chronologically.with_golden_first + set_page_and_extract_portion_from @board.cards.postponed.latest end end diff --git a/app/helpers/cards_helper.rb b/app/helpers/cards_helper.rb index 0dd93c520..581adc108 100644 --- a/app/helpers/cards_helper.rb +++ b/app/helpers/cards_helper.rb @@ -1,5 +1,5 @@ module CardsHelper - def card_article_tag(card, id: dom_id(card, :article), **options, &block) + def card_article_tag(card, id: dom_id(card, :article), data: {}, **options, &block) classes = [ options.delete(:class), ("golden-effect" if card.golden?), @@ -7,10 +7,13 @@ module CardsHelper ("card--active" if card.active?) ].compact.join(" ") + data[:drag_and_drop_top] = true if card.golden? && !card.closed? && !card.postponed? + tag.article \ id: id, style: "--card-color: #{card.color}; view-transition-name: #{id}", class: classes, + data: data, **options, &block end diff --git a/app/javascript/controllers/drag_and_drop_controller.js b/app/javascript/controllers/drag_and_drop_controller.js index 5c03853be..a853214c1 100644 --- a/app/javascript/controllers/drag_and_drop_controller.js +++ b/app/javascript/controllers/drag_and_drop_controller.js @@ -39,6 +39,7 @@ export default class extends Controller { this.wasDropped = true this.#decreaseCounter(this.sourceContainer) const sourceContainer = this.sourceContainer + this.#insertDraggedItem(container, this.dragItem) await this.#submitDropRequest(this.dragItem, container) this.#reloadSourceFrame(sourceContainer); } @@ -47,10 +48,6 @@ export default class extends Controller { this.dragItem.classList.remove(this.draggedItemClass) this.#clearContainerHoverClasses() - if (this.wasDropped) { - this.dragItem.remove() - } - this.sourceContainer = null this.dragItem = null this.wasDropped = false @@ -68,19 +65,6 @@ export default class extends Controller { this.containerTargets.forEach(container => container.classList.remove(this.hoverContainerClass)) } - async #submitDropRequest(item, container) { - const body = new FormData() - const id = item.dataset.id - const url = container.dataset.dragAndDropUrl.replaceAll("__id__", id) - - return post(url, { body, headers: { Accept: "text/vnd.turbo-stream.html" } }) - } - - #reloadSourceFrame(sourceContainer) { - const frame = sourceContainer.querySelector("[data-drag-and-drop-refresh]") - if (frame) frame.reload() - } - #decreaseCounter(sourceContainer) { const counterElement = sourceContainer.querySelector("[data-drag-and-drop-counter]") if (counterElement) { @@ -94,4 +78,29 @@ export default class extends Controller { } } } + + #insertDraggedItem(container, item) { + const itemContainer = container.querySelector("[data-drag-drop-item-container]") + const topItems = itemContainer.querySelectorAll("[data-drag-and-drop-top]") + const lastTopItem = topItems[topItems.length - 1] + + if (lastTopItem) { + lastTopItem.after(item) + } else { + itemContainer.prepend(item) + } + } + + async #submitDropRequest(item, container) { + const body = new FormData() + const id = item.dataset.id + const url = container.dataset.dragAndDropUrl.replaceAll("__id__", id) + + return post(url, { body, headers: { Accept: "text/vnd.turbo-stream.html" } }) + } + + #reloadSourceFrame(sourceContainer) { + const frame = sourceContainer.querySelector("[data-drag-and-drop-refresh]") + if (frame) frame.reload() + } } diff --git a/app/views/boards/columns/_empty_placeholder.html.erb b/app/views/boards/columns/_empty_placeholder.html.erb new file mode 100644 index 000000000..36507c1ff --- /dev/null +++ b/app/views/boards/columns/_empty_placeholder.html.erb @@ -0,0 +1,6 @@ +
+
+

Drag cards here

+
+
No cards here
+
diff --git a/app/views/boards/columns/_list.html.erb b/app/views/boards/columns/_list.html.erb index a3a65ae74..7adedf322 100644 --- a/app/views/boards/columns/_list.html.erb +++ b/app/views/boards/columns/_list.html.erb @@ -1,3 +1,3 @@ -
+
<%= render "cards/display/previews", cards: cards, draggable: draggable %>
diff --git a/app/views/boards/columns/closeds/show.html.erb b/app/views/boards/columns/closeds/show.html.erb index fc8aaddfb..8a9d1f56c 100644 --- a/app/views/boards/columns/closeds/show.html.erb +++ b/app/views/boards/columns/closeds/show.html.erb @@ -17,12 +17,7 @@ <%= render "boards/columns/list", cards: @page.records, draggable: true %> <% end %> <% else %> -
-
-

Drag cards here

-
-
No cards here
-
+ <%= render "boards/columns/empty_placeholder" %> <% end %> <% end %> diff --git a/app/views/boards/columns/not_nows/show.html.erb b/app/views/boards/columns/not_nows/show.html.erb index 2f6623b02..88ea33868 100644 --- a/app/views/boards/columns/not_nows/show.html.erb +++ b/app/views/boards/columns/not_nows/show.html.erb @@ -17,12 +17,7 @@ <%= render "boards/columns/list", cards: @page.records, draggable: true %> <% end %> <% else %> -
-
-

Drag cards here

-
-
No cards here
-
+ <%= render "boards/columns/empty_placeholder" %> <% end %> <% end %> diff --git a/app/views/boards/columns/show.html.erb b/app/views/boards/columns/show.html.erb index 9aafae01f..4fef4eec7 100644 --- a/app/views/boards/columns/show.html.erb +++ b/app/views/boards/columns/show.html.erb @@ -17,12 +17,7 @@ <%= render "boards/columns/list", cards: @page.records, draggable: true %> <% end %> <% else %> -
-
-

Drag cards here

-
-
No cards here
-
+ <%= render "boards/columns/empty_placeholder" %> <% end %> <% end %> diff --git a/app/views/boards/columns/streams/show.html.erb b/app/views/boards/columns/streams/show.html.erb index b1bb7919e..9f0477b39 100644 --- a/app/views/boards/columns/streams/show.html.erb +++ b/app/views/boards/columns/streams/show.html.erb @@ -17,7 +17,7 @@ <%= render "boards/columns/list", cards: @page.records, draggable: true %> <% end %> <% else %> -
No cards here
+ <%= render "boards/columns/empty_placeholder" %> <% end %> <% end %> From 3536b6fb32c691255474f90fd4180a7f10c53042 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 4 Dec 2025 17:36:06 +0100 Subject: [PATCH 02/90] Modify counters optimistically too --- .../controllers/drag_and_drop_controller.js | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/app/javascript/controllers/drag_and_drop_controller.js b/app/javascript/controllers/drag_and_drop_controller.js index a853214c1..8c75b1ad2 100644 --- a/app/javascript/controllers/drag_and_drop_controller.js +++ b/app/javascript/controllers/drag_and_drop_controller.js @@ -32,16 +32,18 @@ export default class extends Controller { } async drop(event) { - const container = this.#containerContaining(event.target) + const targetContainer = this.#containerContaining(event.target) - if (!container || container === this.sourceContainer) { return } + if (!targetContainer || targetContainer === this.sourceContainer) { return } this.wasDropped = true + this.#increaseCounter(targetContainer) this.#decreaseCounter(this.sourceContainer) + const sourceContainer = this.sourceContainer - this.#insertDraggedItem(container, this.dragItem) - await this.#submitDropRequest(this.dragItem, container) - this.#reloadSourceFrame(sourceContainer); + this.#insertDraggedItem(targetContainer, this.dragItem) + await this.#submitDropRequest(this.dragItem, targetContainer) + this.#reloadSourceFrame(sourceContainer) } dragEnd() { @@ -65,17 +67,22 @@ export default class extends Controller { this.containerTargets.forEach(container => container.classList.remove(this.hoverContainerClass)) } - #decreaseCounter(sourceContainer) { - const counterElement = sourceContainer.querySelector("[data-drag-and-drop-counter]") + #increaseCounter(container) { + this.#modifyCounter(container, count => count + 1) + } + + #decreaseCounter(container) { + this.#modifyCounter(container, count => Math.max(0, count - 1)) + } + + #modifyCounter(container, fn) { + const counterElement = container.querySelector("[data-drag-and-drop-counter]") if (counterElement) { const currentValue = counterElement.textContent.trim() if (!/^\d+$/.test(currentValue)) return - const count = parseInt(currentValue) - if (count > 0) { - counterElement.textContent = count - 1 - } + counterElement.textContent = fn(parseInt(currentValue)) } } From 104bb5faa9d181a3be3bd75c98278e045203004f Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 4 Dec 2025 18:07:41 +0100 Subject: [PATCH 03/90] Add container to drop cards when the stream is empty --- app/views/boards/show/_stream.html.erb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/views/boards/show/_stream.html.erb b/app/views/boards/show/_stream.html.erb index 7158a29e3..d4753eba5 100644 --- a/app/views/boards/show/_stream.html.erb +++ b/app/views/boards/show/_stream.html.erb @@ -13,5 +13,8 @@ <%= with_automatic_pagination "the-stream", @page do %> <%= render "boards/columns/list", cards: page.records, draggable: true %> <% end %> + <% else %> +
+
<% end %> <% end %> From 539d3721dbaf85878033540e03195a0b25c45f02 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Thu, 4 Dec 2025 18:13:17 +0100 Subject: [PATCH 04/90] Keep the column color when D&D cards --- app/helpers/columns_helper.rb | 6 ++-- .../controllers/drag_and_drop_controller.js | 33 +++++++++++++++++++ app/views/boards/show/_column.html.erb | 3 +- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/app/helpers/columns_helper.rb b/app/helpers/columns_helper.rb index 8a2e82e17..26a202e06 100644 --- a/app/helpers/columns_helper.rb +++ b/app/helpers/columns_helper.rb @@ -10,14 +10,16 @@ module ColumnsHelper data: { turbo_frame: "_top" } end - def column_tag(id:, name:, drop_url:, collapsed: true, selected: nil, data: {}, **properties, &block) + def column_tag(id:, name:, drop_url:, collapsed: true, selected: nil, card_color: "var(--color-card-default)", data: {}, **properties, &block) classes = token_list("cards", properties.delete(:class), "is-collapsed": collapsed) data = { drag_and_drop_target: "container", navigable_list_target: "item", column_name: name, - drag_and_drop_url: drop_url + drag_and_drop_url: drop_url, + drag_and_drop_css_variable_name: "--card-color", + drag_and_drop_css_variable_value: card_color }.merge(data) data[:action] = token_list( diff --git a/app/javascript/controllers/drag_and_drop_controller.js b/app/javascript/controllers/drag_and_drop_controller.js index 8c75b1ad2..88ad31ba2 100644 --- a/app/javascript/controllers/drag_and_drop_controller.js +++ b/app/javascript/controllers/drag_and_drop_controller.js @@ -16,11 +16,14 @@ export default class extends Controller { await nextFrame() this.dragItem = this.#itemContaining(event.target) this.sourceContainer = this.#containerContaining(this.dragItem) + this.originalDraggedItemCssVariable = this.#containerCssVariableFor(this.sourceContainer) this.dragItem.classList.add(this.draggedItemClass) } dragOver(event) { event.preventDefault() + if (!this.dragItem) { return } + const container = this.#containerContaining(event.target) this.#clearContainerHoverClasses() @@ -28,6 +31,9 @@ export default class extends Controller { if (container !== this.sourceContainer) { container.classList.add(this.hoverContainerClass) + this.#applyContainerCssVariable(container) + } else { + this.#restoreOriginalDraggedItemCssVariable() } } @@ -50,9 +56,14 @@ export default class extends Controller { this.dragItem.classList.remove(this.draggedItemClass) this.#clearContainerHoverClasses() + if (!this.wasDropped) { + this.#restoreOriginalDraggedItemCssVariable() + } + this.sourceContainer = null this.dragItem = null this.wasDropped = false + this.originalDraggedItemCssVariable = null } #itemContaining(element) { @@ -67,6 +78,28 @@ export default class extends Controller { this.containerTargets.forEach(container => container.classList.remove(this.hoverContainerClass)) } + #applyContainerCssVariable(container) { + const cssVariable = this.#containerCssVariableFor(container) + if (cssVariable) { + this.dragItem.style.setProperty(cssVariable.name, cssVariable.value) + } + } + + #restoreOriginalDraggedItemCssVariable() { + if (this.originalDraggedItemCssVariable) { + const { name, value } = this.originalDraggedItemCssVariable + this.dragItem.style.setProperty(name, value) + } + } + + #containerCssVariableFor(container) { + const { dragAndDropCssVariableName, dragAndDropCssVariableValue } = container.dataset + if (dragAndDropCssVariableName && dragAndDropCssVariableValue) { + return { name: dragAndDropCssVariableName, value: dragAndDropCssVariableValue } + } + return null + } + #increaseCounter(container) { this.#modifyCounter(container, count => count + 1) } diff --git a/app/views/boards/show/_column.html.erb b/app/views/boards/show/_column.html.erb index 35beb5338..b5bba3339 100644 --- a/app/views/boards/show/_column.html.erb +++ b/app/views/boards/show/_column.html.erb @@ -1,4 +1,5 @@ -<%= column_tag id: dom_id(column), name: column.name, drop_url: columns_card_drops_column_path("__id__", column_id: column.id), class: "cards--doing", style: "--card-color: #{column.color};", +<%= column_tag id: dom_id(column), name: column.name, drop_url: columns_card_drops_column_path("__id__", column_id: column.id), card_color: column.color.to_s, + class: "cards--doing", style: "--card-color: #{column.color};", data: { drag_and_strum_target: "container", collapsible_columns_target: "column", From 5c4fe9fe8b64f89288f2c6d615f98f76bb75bb03 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Thu, 4 Dec 2025 11:37:27 -0600 Subject: [PATCH 05/90] Prevent board names with only spaces and show validation message --- app/javascript/controllers/form_controller.js | 7 ++++++- app/views/boards/new.html.erb | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/javascript/controllers/form_controller.js b/app/javascript/controllers/form_controller.js index e910d6622..d675dcae5 100644 --- a/app/javascript/controllers/form_controller.js +++ b/app/javascript/controllers/form_controller.js @@ -21,8 +21,13 @@ export default class extends Controller { if (input) { const value = (input.value || "").trim() - if (value.length === 0) { + const isEmpty = value.length === 0 + + if (isEmpty) { event.preventDefault() + input.setCustomValidity(input.validationMessage || "Please fill out this field") + input.reportValidity() + input.addEventListener("input", () => input.setCustomValidity(""), { once: true }) } } } diff --git a/app/views/boards/new.html.erb b/app/views/boards/new.html.erb index 4536af7fc..da74845d6 100644 --- a/app/views/boards/new.html.erb +++ b/app/views/boards/new.html.erb @@ -1,9 +1,9 @@ <% @page_title = "Create a new board" %>
- <%= form_with model: @board, class: "flex flex-column gap", data: { controller: "form" } do |form| %> + <%= form_with model: @board, class: "flex flex-column gap", data: { controller: "form", action: "submit->form#preventEmptySubmit" } do |form| %>

<%= @page_title %>

- <%= form.text_field :name, required: true, class: "input full-width", autofocus: true, autocomplete: "off",placeholder: "Name it…", data: { action: "keydown.esc@document->form#cancel" } %> + <%= form.text_field :name, required: true, class: "input full-width", autofocus: true, autocomplete: "off", placeholder: "Name it…", data: { form_target: "input", action: "keydown.esc@document->form#cancel" } %>
+ + <% if @user.errors.any? %> +
+ +
+ <% end %> + diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 081579dfc..385bacaf3 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -64,4 +64,26 @@ class UsersControllerTest < ActionDispatch::IntegrationTest delete user_path(users(:david)) assert_response :forbidden end + + test "update with invalid avatar shows validation error" do + sign_in_as :kevin + + svg_file = fixture_file_upload("avatar.svg", "image/svg+xml") + + put user_path(users(:kevin)), params: { user: { avatar: svg_file } } + assert_response :unprocessable_entity + assert_select "form[action='#{user_path(users(:kevin))}']" + assert_select ".txt-negative", text: /must be a JPEG, PNG, GIF, or WebP image/ + end + + test "update with valid avatar" do + sign_in_as :kevin + + png_file = fixture_file_upload("avatar.png", "image/png") + + put user_path(users(:kevin)), params: { user: { avatar: png_file } } + assert_redirected_to user_path(users(:kevin)) + assert users(:kevin).reload.avatar.attached? + assert_equal "image/png", users(:kevin).avatar.content_type + end end From 675600af72d95e5359a4ac3ca200a3b445ec65e8 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 14:57:09 -0500 Subject: [PATCH 63/90] Improve presentation of validation errors --- app/views/signups/completions/new.html.erb | 5 +++-- app/views/users/edit.html.erb | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/views/signups/completions/new.html.erb b/app/views/signups/completions/new.html.erb index 971c39b1d..5be760843 100644 --- a/app/views/signups/completions/new.html.erb +++ b/app/views/signups/completions/new.html.erb @@ -9,8 +9,9 @@

You're one step away. Just enter your name to get your own Fizzy account.

<% if @signup.errors.any? %> -
-
    +
    +

    Your changes couldn't be saved:

    +
      <% @signup.errors.full_messages.each do |message| %>
    • <%= message %>
      • <% end %> diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 098ad729c..c712dac58 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -40,8 +40,9 @@
    <% if @user.errors.any? %> -
    -
      +
      +

      Your changes couldn't be saved:

      +
        <% @user.errors.full_messages.each do |message| %>
      • <%= message %>
        • <% end %> From b439a7695b9e23eec53990c80f86916affafb880 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 15:02:22 -0500 Subject: [PATCH 64/90] Render verification code in dev for signups, too Related to 5865d79e / #1991 --- app/controllers/signups_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/signups_controller.rb b/app/controllers/signups_controller.rb index 33da02f28..217221378 100644 --- a/app/controllers/signups_controller.rb +++ b/app/controllers/signups_controller.rb @@ -11,7 +11,8 @@ class SignupsController < ApplicationController end def create - Signup.new(signup_params).create_identity + magic_link = Signup.new(signup_params).create_identity + serve_development_magic_link(magic_link) redirect_to session_magic_link_path end From d747d1bfa35734ad68b0983035e78e0bf7b3a6e3 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 16:05:03 -0500 Subject: [PATCH 65/90] Ensure we have good test coverage on Signup.create_identity because we're relying on it returning a MagicLink --- test/models/signup_test.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/models/signup_test.rb b/test/models/signup_test.rb index 30910bf81..d62b84769 100644 --- a/test/models/signup_test.rb +++ b/test/models/signup_test.rb @@ -4,12 +4,14 @@ class SignupTest < ActiveSupport::TestCase test "#create_identity" do signup = Signup.new(email_address: "brian@example.com") + magic_link = nil assert_difference -> { Identity.count }, 1 do assert_difference -> { MagicLink.count }, 1 do - assert signup.create_identity + magic_link = signup.create_identity end end + assert_kind_of MagicLink, magic_link assert_empty signup.errors assert signup.identity assert signup.identity.persisted? @@ -18,10 +20,12 @@ class SignupTest < ActiveSupport::TestCase assert_no_difference -> { Identity.count } do assert_difference -> { MagicLink.count }, 1 do - assert signup_existing.create_identity, "Should send magic link for existing identity" + magic_link = signup_existing.create_identity end end + assert_kind_of MagicLink, magic_link + signup_invalid = Signup.new(email_address: "") assert_raises do signup_invalid.create_identity From b2250b6d3588cac03956c877c360f388f003cf4c Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 16:12:34 -0500 Subject: [PATCH 66/90] Ensure test coverage for all branches of JoinCodesController#create --- test/controllers/join_codes_controller_test.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/controllers/join_codes_controller_test.rb b/test/controllers/join_codes_controller_test.rb index 0a4bf5793..9558b605a 100644 --- a/test/controllers/join_codes_controller_test.rb +++ b/test/controllers/join_codes_controller_test.rb @@ -69,4 +69,17 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest assert_redirected_to new_users_verification_url(script_name: @account.slug) end + + test "create for different identity terminates existing session" do + sign_in_as :kevin + + assert_difference -> { Identity.count }, 1 do + assert_difference -> { User.count }, 1 do + post join_path(code: @join_code.code, script_name: @account.slug), params: { email_address: "new_user@example.com" } + end + end + + assert_redirected_to session_magic_link_url(script_name: nil) + assert_not_predicate cookies[:session_token], :present? + end end From 3ff45d77bbad3bc1d760536e72fd3bead4784c67 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 16:46:33 -0500 Subject: [PATCH 67/90] Ensure test coverage for invalid Signup#complete --- test/models/signup_test.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/models/signup_test.rb b/test/models/signup_test.rb index d62b84769..cf7bd3359 100644 --- a/test/models/signup_test.rb +++ b/test/models/signup_test.rb @@ -52,4 +52,15 @@ class SignupTest < ActiveSupport::TestCase assert_not_empty signup_invalid.errors[:full_name] end end + + test "#complete with invalid data" do + Current.without_account do + signup = Signup.new + assert_not signup.complete + assert signup.errors[:full_name].any? + assert signup.errors[:identity].any? + assert_nil signup.account + assert_nil signup.user + end + end end From 95ba2c01b88b2c7a9284bb216256c5805cb8a6c3 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 16:50:39 -0500 Subject: [PATCH 68/90] Add email validation to Signup --- app/models/signup.rb | 5 ++--- test/models/signup_test.rb | 9 +++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/models/signup.rb b/app/models/signup.rb index 15ac5873c..110c253ef 100644 --- a/app/models/signup.rb +++ b/app/models/signup.rb @@ -6,9 +6,8 @@ class Signup attr_accessor :full_name, :email_address, :identity attr_reader :account, :user - with_options on: :completion do - validates_presence_of :full_name, :identity - end + validates :email_address, format: { with: URI::MailTo::EMAIL_REGEXP }, on: :identity_creation + validates :full_name, :identity, presence: true, on: :completion def initialize(...) super diff --git a/test/models/signup_test.rb b/test/models/signup_test.rb index cf7bd3359..589bcd49a 100644 --- a/test/models/signup_test.rb +++ b/test/models/signup_test.rb @@ -1,6 +1,15 @@ require "test_helper" class SignupTest < ActiveSupport::TestCase + test "validates email format for identity creation" do + signup = Signup.new(email_address: "not-an-email") + assert_not signup.valid?(:identity_creation) + assert signup.errors[:email_address].any? + + signup = Signup.new(email_address: "valid@example.com") + assert signup.valid?(:identity_creation) + end + test "#create_identity" do signup = Signup.new(email_address: "brian@example.com") From 0160f215f2f35eb060ce408799e088094429cef8 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 16:52:16 -0500 Subject: [PATCH 69/90] Validate email before creating identity during sign-up and sign-in Avoid Sentry exceptions when attackers try to stuff invalid emails. The browser performs form field validation that should normally prevent this from occurring, so we just return 422 without validation error messages. Also: - extract redirect_to_session_magic_link helper - some controller refactoring and cleanup --- app/controllers/concerns/authentication.rb | 6 +++++ app/controllers/join_codes_controller.rb | 16 ++++------- app/controllers/sessions_controller.rb | 17 ++++++------ app/controllers/signups_controller.rb | 9 ++++--- test/controllers/sessions_controller_test.rb | 28 +++++++++++++++----- test/controllers/signups_controller_test.rb | 16 ++++++----- test/test_helper.rb | 9 +++++++ 7 files changed, 64 insertions(+), 37 deletions(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 89eda5887..4cdbbdb69 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -97,6 +97,12 @@ module Authentication end end + def redirect_to_session_magic_link(magic_link, return_to: nil) + serve_development_magic_link(magic_link) + session[:return_to_after_authenticating] = return_to if return_to + redirect_to session_magic_link_url(script_name: nil) + end + def serve_development_magic_link(magic_link) if Rails.env.development? flash[:magic_link_code] = magic_link&.code diff --git a/app/controllers/join_codes_controller.rb b/app/controllers/join_codes_controller.rb index eb328df18..99298880c 100644 --- a/app/controllers/join_codes_controller.rb +++ b/app/controllers/join_codes_controller.rb @@ -20,8 +20,11 @@ class JoinCodesController < ApplicationController elsif identity == Current.identity redirect_to new_users_verification_url(script_name: @join_code.account.slug) else - logout_and_send_new_magic_link(identity) - redirect_to session_magic_link_url(script_name: nil) + terminate_session if Current.identity + + redirect_to_session_magic_link \ + identity.send_magic_link, + return_to: new_users_verification_url(script_name: @join_code.account.slug) end end @@ -37,13 +40,4 @@ class JoinCodesController < ApplicationController render :inactive, status: :gone end end - - def logout_and_send_new_magic_link(identity) - terminate_session if Current.identity - - magic_link = identity.send_magic_link - serve_development_magic_link(magic_link) - - session[:return_to_after_authenticating] = new_users_verification_url(script_name: @join_code.account.slug) - end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 5335f58f3..fb9fe0617 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -9,17 +9,16 @@ class SessionsController < ApplicationController end def create - identity = Identity.find_by_email_address(email_address) - - magic_link = if identity - identity.send_magic_link + if identity = Identity.find_by_email_address(email_address) + redirect_to_session_magic_link identity.send_magic_link else - Signup.new(email_address: email_address).create_identity + signup = Signup.new(email_address: email_address) + if signup.valid?(:identity_creation) + redirect_to_session_magic_link signup.create_identity + else + head :unprocessable_entity + end end - - serve_development_magic_link(magic_link) - - redirect_to session_magic_link_path end def destroy diff --git a/app/controllers/signups_controller.rb b/app/controllers/signups_controller.rb index 217221378..776421201 100644 --- a/app/controllers/signups_controller.rb +++ b/app/controllers/signups_controller.rb @@ -11,9 +11,12 @@ class SignupsController < ApplicationController end def create - magic_link = Signup.new(signup_params).create_identity - serve_development_magic_link(magic_link) - redirect_to session_magic_link_path + signup = Signup.new(signup_params) + if signup.valid?(:identity_creation) + redirect_to_session_magic_link signup.create_identity + else + head :unprocessable_entity + end end private diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index f2318bf46..7ebe4d518 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -36,15 +36,29 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest end end - private - test "destroy" do - sign_in_as :kevin - + test "create with invalid email address" do + # Avoid Sentry exceptions when attackers try to stuff invalid emails. The browser performs form + # field validation that should normally prevent this from occurring, so I'm not worried about + # returning proper validation errors. + without_action_dispatch_exception_handling do untenanted do - delete session_path + assert_no_difference -> { Identity.count } do + post session_path, params: { email_address: "not-a-valid-email" } + end - assert_redirected_to new_session_path - assert_not cookies[:session_token].present? + assert_response :unprocessable_entity end end + end + + test "destroy" do + sign_in_as :kevin + + untenanted do + delete session_path + + assert_redirected_to new_session_path + assert_not cookies[:session_token].present? + end + end end diff --git a/test/controllers/signups_controller_test.rb b/test/controllers/signups_controller_test.rb index 83fea8882..17314ee90 100644 --- a/test/controllers/signups_controller_test.rb +++ b/test/controllers/signups_controller_test.rb @@ -34,15 +34,17 @@ class SignupsControllerTest < ActionDispatch::IntegrationTest end end - test "create with email address containing blanks" do - untenanted do - assert_no_difference -> { Identity.count } do - assert_no_difference -> { MagicLink.count } do - post signup_path, params: { signup: { email_address: "sam smith@example.com" } } + test "create with invalid email address" do + without_action_dispatch_exception_handling do + untenanted do + assert_no_difference -> { Identity.count } do + assert_no_difference -> { MagicLink.count } do + post signup_path, params: { signup: { email_address: "not-a-valid-email" } } + end end - end - assert_response :unprocessable_entity + assert_response :unprocessable_entity + end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 03cda99db..4cec836fd 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -61,6 +61,15 @@ class ActionDispatch::IntegrationTest setup do integration_session.default_url_options[:script_name] = "/#{ActiveRecord::FixtureSet.identify("37signals")}" end + + private + def without_action_dispatch_exception_handling + original = Rails.application.config.action_dispatch.show_exceptions + Rails.application.config.action_dispatch.show_exceptions = :none + yield + ensure + Rails.application.config.action_dispatch.show_exceptions = original + end end class ActionDispatch::SystemTestCase From 4c9d8edbc6dec41c3a879668cae39f8dd3c9b6c2 Mon Sep 17 00:00:00 2001 From: Felipe Felix Date: Sun, 7 Dec 2025 04:25:11 -0300 Subject: [PATCH 70/90] Test Notifications::SettingsController --- .../notifications/settings_controller_test.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 test/controllers/notifications/settings_controller_test.rb diff --git a/test/controllers/notifications/settings_controller_test.rb b/test/controllers/notifications/settings_controller_test.rb new file mode 100644 index 000000000..d8472bb02 --- /dev/null +++ b/test/controllers/notifications/settings_controller_test.rb @@ -0,0 +1,24 @@ +require "test_helper" + +class Notifications::SettingsControllerTest < ActionDispatch::IntegrationTest + setup do + @user = users(:david) + + sign_in_as @user + end + + test "show" do + get notifications_settings_path + + assert_response :success + end + + test "update email frequency" do + assert_changes -> { @user.reload.settings.bundle_email_frequency }, from: "never", to: "every_few_hours" do + put notifications_settings_path, params: { user_settings: { bundle_email_frequency: "every_few_hours" } } + end + + assert_redirected_to notifications_settings_path + assert_equal "Settings updated", flash[:notice] + end +end From 31a41e66c27e54d248fc66761c167e139977aa85 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sun, 7 Dec 2025 11:34:54 +0100 Subject: [PATCH 71/90] Make column have a proper ID instead of inferring it from the title --- app/models/user/day_timeline.rb | 10 +++++----- app/models/user/day_timeline/column.rb | 18 +++++++----------- app/views/events/day_timeline/_column.html.erb | 2 +- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/app/models/user/day_timeline.rb b/app/models/user/day_timeline.rb index 1fe4ed36e..4b30d24ca 100644 --- a/app/models/user/day_timeline.rb +++ b/app/models/user/day_timeline.rb @@ -30,15 +30,15 @@ class User::DayTimeline end def added_column - @added_column ||= build_column("Added", 1, events.where(action: %w[card_published card_reopened])) + @added_column ||= build_column(:added, "Added", 1, events.where(action: %w[card_published card_reopened])) end def updated_column - @updated_column ||= build_column("Updated", 2, events.where.not(action: %w[card_published card_closed card_reopened])) + @updated_column ||= build_column(:updated, "Updated", 2, events.where.not(action: %w[card_published card_closed card_reopened])) end def closed_column - @closed_column ||= build_column("Done", 3, events.where(action: "card_closed")) + @closed_column ||= build_column(:closed, "Done", 3, events.where(action: "card_closed")) end def cache_key @@ -84,8 +84,8 @@ class User::DayTimeline filtered_events.where(created_at: ...day.beginning_of_day).chronologically.last end - def build_column(base_title, index, events) - Column.new(self, base_title, index, events) + def build_column(id, base_title, index, events) + Column.new(self, id, base_title, index, events) end def window diff --git a/app/models/user/day_timeline/column.rb b/app/models/user/day_timeline/column.rb index ce87873fa..0f9261bbb 100644 --- a/app/models/user/day_timeline/column.rb +++ b/app/models/user/day_timeline/column.rb @@ -1,9 +1,10 @@ class User::DayTimeline::Column include ActionView::Helpers::TagHelper, ActionView::Helpers::OutputSafetyHelper, TimeHelper - attr_reader :index, :base_title, :day_timeline, :events + attr_reader :index, :id, :base_title, :day_timeline, :events - def initialize(day_timeline, base_title, index, events) + def initialize(day_timeline, id, base_title, index, events) + @id = id @day_timeline = day_timeline @base_title = base_title @index = index @@ -17,15 +18,6 @@ class User::DayTimeline::Column safe_join(parts, " ") end - def base_title_for_route - case base_title - when "Done" - "closed" - else - base_title.downcase - end - end - def events_by_hour limited_events.group_by { it.created_at.hour } end @@ -38,6 +30,10 @@ class User::DayTimeline::Column full_events_count - limited_events.count end + def to_param + id + end + private def limited_events @limited_events ||= events.limit(100).load diff --git a/app/views/events/day_timeline/_column.html.erb b/app/views/events/day_timeline/_column.html.erb index 509efd69d..50d35b4e2 100644 --- a/app/views/events/day_timeline/_column.html.erb +++ b/app/views/events/day_timeline/_column.html.erb @@ -3,7 +3,7 @@ <%= column.title %> <% if column.events_by_hour.any? %> - <%= link_to events_day_timeline_column_path(id: column.base_title_for_route, day: column.day_timeline.day.to_date), class: "events__maximize-button btn btn--circle txt-x-small borderless", data: { turbo_frame: "_top" } do %> + <%= link_to events_day_timeline_column_path(column, day: column.day_timeline.day.to_date), class: "events__maximize-button btn btn--circle txt-x-small borderless", data: { turbo_frame: "_top" } do %> <%= icon_tag "grid", class: "translucent" %> Expand column <% end %> From c8c91259c750db719d04e9f999aa2dbc32d3fa62 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sun, 7 Dec 2025 12:06:03 +0100 Subject: [PATCH 72/90] Revert "Immediate avatar and embed variants" --- Gemfile.lock | 2 +- Gemfile.saas.lock | 6 ++--- app/models/concerns/attachments.rb | 9 ++++--- app/models/user/avatar.rb | 2 +- config/initializers/action_text.rb | 2 +- .../users/avatars_controller_test.rb | 8 ++----- .../notification/bundle_mailer_test.rb | 3 +-- test/models/comment_test.rb | 20 ---------------- test/models/user/avatar_test.rb | 24 ++----------------- 9 files changed, 17 insertions(+), 59 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 2f87d5cff..43d4e718b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -6,7 +6,7 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 + revision: 3c3f6c8a253c8a0f346695374f927c9ab32fbefb branch: main specs: actioncable (8.2.0.alpha) diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock index 18f80a22f..0c8880b08 100644 --- a/Gemfile.saas.lock +++ b/Gemfile.saas.lock @@ -75,7 +75,7 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 + revision: 690ec8898318b8f50714e86676353ebe1551261e branch: main specs: actioncable (8.2.0.alpha) @@ -292,7 +292,7 @@ GEM actionview (>= 7.0.0) activesupport (>= 7.0.0) jmespath (1.6.2) - json (2.17.1) + json (2.16.0) jwt (3.1.2) base64 kamal (2.9.0) @@ -552,7 +552,7 @@ GEM ostruct stimulus-rails (1.3.4) railties (>= 6.0.0) - stringio (3.1.9) + stringio (3.1.8) thor (1.4.0) thruster (0.1.16) thruster (0.1.16-aarch64-linux) diff --git a/app/models/concerns/attachments.rb b/app/models/concerns/attachments.rb index bcc7303ae..1d1c2c4d6 100644 --- a/app/models/concerns/attachments.rb +++ b/app/models/concerns/attachments.rb @@ -1,10 +1,13 @@ module Attachments extend ActiveSupport::Concern - # Variants used by ActionText embeds. Processed immediately on attachment to avoid - # read replica issues (lazy variants would attempt writes on read replicas). + # The variants we use must all declared here so that we can preprocess them. # - # Patched into ActionText::RichText in config/initializers/action_text.rb + # If they are not preprocessed, then Rails will attempt to transform the image on-the-fly when + # they are first viewed, which may be on the read replica where writing to the database is not + # allowed. Chaos will ensue if that happens. + # + # These variants are patched into ActionText::RichText in config/initializers/action_text.rb VARIANTS = { # vipsthumbnail used to create sized image variants has a intent setting to manage colors during # resize. By setting an invalid intent value the gif-incompatible intent filtering is skipped and diff --git a/app/models/user/avatar.rb b/app/models/user/avatar.rb index c635edddb..a4b2ae631 100644 --- a/app/models/user/avatar.rb +++ b/app/models/user/avatar.rb @@ -5,7 +5,7 @@ module User::Avatar included do has_one_attached :avatar do |attachable| - attachable.variant :thumb, resize_to_fill: [ 256, 256 ], process: :immediately + attachable.variant :thumb, resize_to_fill: [ 256, 256 ] end validate :avatar_content_type_allowed diff --git a/config/initializers/action_text.rb b/config/initializers/action_text.rb index eee87de0d..b2724b1c7 100644 --- a/config/initializers/action_text.rb +++ b/config/initializers/action_text.rb @@ -7,7 +7,7 @@ module ActionText # This overrides the default :embeds association! has_many_attached :embeds do |attachable| ::Attachments::VARIANTS.each do |variant_name, variant_options| - attachable.variant variant_name, **variant_options, process: :immediately + attachable.variant variant_name, variant_options.merge(preprocessed: true) end end end diff --git a/test/controllers/users/avatars_controller_test.rb b/test/controllers/users/avatars_controller_test.rb index be4532448..3ded884b8 100644 --- a/test/controllers/users/avatars_controller_test.rb +++ b/test/controllers/users/avatars_controller_test.rb @@ -27,9 +27,7 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show own image redirects to the blob url" do - # Create blob separately to ensure file is uploaded before variant processing - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") assert users(:david).avatar.attached? get user_avatar_path(users(:david)) @@ -38,9 +36,7 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show other image redirects to the blob url" do - # Create blob separately to ensure file is uploaded before variant processing - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") - users(:kevin).avatar.attach(blob) + users(:kevin).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") assert users(:kevin).avatar.attached? get user_avatar_path(users(:kevin)) diff --git a/test/mailers/notification/bundle_mailer_test.rb b/test/mailers/notification/bundle_mailer_test.rb index 63647e68a..fbc4274c9 100644 --- a/test/mailers/notification/bundle_mailer_test.rb +++ b/test/mailers/notification/bundle_mailer_test.rb @@ -22,12 +22,11 @@ class Notification::BundleMailerTest < ActionMailer::TestCase end test "renders avatar with external image URL when avatar is attached" do - blob = ActiveStorage::Blob.create_and_upload!( + @user.avatar.attach( io: File.open(Rails.root.join("test", "fixtures", "files", "avatar.png")), filename: "avatar.png", content_type: "image/png" ) - @user.avatar.attach(blob) create_notification(@user) diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index 13a5e36c3..c519f8fbd 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -4,24 +4,4 @@ class CommentTest < ActiveSupport::TestCase setup do Current.session = sessions(:david) end - - test "rich text embed variants are processed immediately on attachment" do - comment = cards(:logo).comments.create!(body: "Check this out") - comment.body.body.attachables # force load - - blob = ActiveStorage::Blob.create_and_upload! \ - io: File.open(file_fixture("moon.jpg")), - filename: "moon.jpg", - content_type: "image/jpeg" - - comment.body.body = ActionText::Content.new(comment.body.body.to_html).append_attachables(blob) - comment.save! - - embed = comment.body.embeds.sole - - Attachments::VARIANTS.each_key do |variant_name| - variant = embed.variant(variant_name) - assert variant.processed?, "Expected #{variant_name} variant to be processed immediately" - end - end end diff --git a/test/models/user/avatar_test.rb b/test/models/user/avatar_test.rb index 58b78969c..96ea5bac7 100644 --- a/test/models/user/avatar_test.rb +++ b/test/models/user/avatar_test.rb @@ -2,8 +2,7 @@ require "test_helper" class User::AvatarTest < ActiveSupport::TestCase test "avatar_thumbnail returns variant for variable images" do - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") assert users(:david).avatar.variable? assert_equal users(:david).avatar.variant(:thumb).blob, users(:david).avatar_thumbnail.blob @@ -17,8 +16,7 @@ class User::AvatarTest < ActiveSupport::TestCase end test "allows valid image content types" do - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg", content_type: "image/jpeg") - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg") assert users(:david).valid? end @@ -29,22 +27,4 @@ class User::AvatarTest < ActiveSupport::TestCase assert_not users(:david).valid? assert_includes users(:david).errors[:avatar], "must be a JPEG, PNG, GIF, or WebP image" end - - test "thumb variant is processed immediately on attachment" do - # Create blob separately to ensure file is uploaded before variant processing. - # - # Root cause: When ActiveStorage::Record uses `connects_to` for read replica support - # (as in SAAS mode), it creates a separate connection pool from application models. - # Since after_commit callbacks are tracked per connection pool, the callback order - # between User's pool (upload) and Attachment's pool (create_variants) isn't guaranteed. - # In MySQL/SAAS mode, the Attachment callback fires before the file is uploaded. - blob = ActiveStorage::Blob.create_and_upload!( - io: File.open(file_fixture("avatar.png")), - filename: "avatar.png", - content_type: "image/png" - ) - users(:david).avatar.attach(blob) - - assert users(:david).avatar.variant(:thumb).processed? - end end From 0b1a25c6b9173cb2f59b335f7f3ec18815c9687e Mon Sep 17 00:00:00 2001 From: user Date: Sun, 7 Dec 2025 20:47:25 +0900 Subject: [PATCH 73/90] Ignore RubyMine project files in .gitignore --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 38bd80d0d..7883e425c 100644 --- a/.gitignore +++ b/.gitignore @@ -41,3 +41,6 @@ /config/credentials/*.key .DS_Store + +# IDEA folder +.idea/ \ No newline at end of file From 661a7e5e2d3e7ec515ff697e5616be27c59872fd Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 7 Dec 2025 13:26:31 -0500 Subject: [PATCH 74/90] Validate email before creating identity during join code redemption Avoid Sentry exceptions when attackers try to stuff invalid emails. The browser performs form field validation that should normally prevent this from occurring, so we just return 422 without validation error messages. See similar change in #1996 for sign-in and sign-up --- app/controllers/join_codes_controller.rb | 25 +++++++++++++------ .../controllers/join_codes_controller_test.rb | 12 +++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/app/controllers/join_codes_controller.rb b/app/controllers/join_codes_controller.rb index 99298880c..8bd67c9dc 100644 --- a/app/controllers/join_codes_controller.rb +++ b/app/controllers/join_codes_controller.rb @@ -3,6 +3,7 @@ class JoinCodesController < ApplicationController before_action :set_join_code before_action :ensure_join_code_is_valid + before_action :set_identity, only: :create layout "public" @@ -10,25 +11,35 @@ class JoinCodesController < ApplicationController end def create - identity = Identity.find_or_create_by!(email_address: params.expect(:email_address)) + @join_code.redeem_if { |account| @identity.join(account) } + user = User.active.find_by!(account: @join_code.account, identity: @identity) - @join_code.redeem_if { |account| identity.join(account) } - user = User.active.find_by!(account: @join_code.account, identity: identity) - - if identity == Current.identity && user.setup? + if @identity == Current.identity && user.setup? redirect_to landing_url(script_name: @join_code.account.slug) - elsif identity == Current.identity + elsif @identity == Current.identity redirect_to new_users_verification_url(script_name: @join_code.account.slug) else terminate_session if Current.identity redirect_to_session_magic_link \ - identity.send_magic_link, + @identity.send_magic_link, return_to: new_users_verification_url(script_name: @join_code.account.slug) end end private + def set_identity + @identity = Identity.find_or_initialize_by(email_address: params.expect(:email_address)) + + if @identity.new_record? + if @identity.invalid? + head :unprocessable_entity + else + @identity.save! + end + end + end + def set_join_code @join_code ||= Account::JoinCode.find_by(code: params.expect(:code), account: Current.account) end diff --git a/test/controllers/join_codes_controller_test.rb b/test/controllers/join_codes_controller_test.rb index 9558b605a..415bde3e5 100644 --- a/test/controllers/join_codes_controller_test.rb +++ b/test/controllers/join_codes_controller_test.rb @@ -82,4 +82,16 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest assert_redirected_to session_magic_link_url(script_name: nil) assert_not_predicate cookies[:session_token], :present? end + + test "create with invalid email address" do + # Avoid Sentry exceptions when attackers try to stuff invalid emails into the system + without_action_dispatch_exception_handling do + assert_no_difference -> { Identity.count } do + assert_no_difference -> { User.count } do + post join_path(code: @join_code.code, script_name: @account.slug), params: { email_address: "not-a-valid-email" } + end + end + assert_response :unprocessable_entity + end + end end From 15983cede397712ff9690ab904518c89947aaf75 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sun, 7 Dec 2025 20:02:59 +0100 Subject: [PATCH 75/90] No need to pass Currentl.user since it is the default value for the param --- app/controllers/cards/closures_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/cards/closures_controller.rb b/app/controllers/cards/closures_controller.rb index b89395cd5..b23600d91 100644 --- a/app/controllers/cards/closures_controller.rb +++ b/app/controllers/cards/closures_controller.rb @@ -2,12 +2,12 @@ class Cards::ClosuresController < ApplicationController include CardScoped def create - @card.close(user: Current.user) + @card.close render_card_replacement end def destroy - @card.reopen(user: Current.user) + @card.reopen render_card_replacement end end From 2110fba227dd3bd0b40b8981a4bb30c85dd3886c Mon Sep 17 00:00:00 2001 From: Robin Brandt Date: Mon, 8 Dec 2025 09:26:39 -0500 Subject: [PATCH 76/90] Bump mittens to 0.3.1 (#2006) --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 43d4e718b..e535c9864 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -267,7 +267,7 @@ GEM railties (>= 7.1) stimulus-rails turbo-rails - mittens (0.3.0) + mittens (0.3.1) mocha (2.8.2) ruby2_keywords (>= 0.0.5) msgpack (1.8.0) From c479235c6032b5373397541ec967ce51b0b0859d Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Mon, 8 Dec 2025 15:54:36 +0100 Subject: [PATCH 77/90] Fix involvement button label --- app/helpers/accesses_helper.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/helpers/accesses_helper.rb b/app/helpers/accesses_helper.rb index 6ba7b4d3c..68caf6995 100644 --- a/app/helpers/accesses_helper.rb +++ b/app/helpers/accesses_helper.rb @@ -40,13 +40,15 @@ module AccessesHelper end def involvement_button(board, access, show_watchers, icon_only) + label_text = access.access_only? ? "Watch this" : "Stop watching" button_to( board_involvement_path(board), method: :put, params: { show_watchers: show_watchers, involvement: next_involvement(access.involvement), icon_only: icon_only }, - aria: { labelledby: dom_id(board, :involvement_label) }, title: access.access_only? ? "Watch this" : "Stop watching", + aria: { labelledby: dom_id(board, :involvement_label) }, + title: (label_text if icon_only), class: class_names("btn", { "btn--reversed": access.watching? && icon_only })) do icon_tag("notification-bell-#{icon_only ? 'reverse-' : nil}#{access.involvement.dasherize}") + - tag.span(class: class_names("txt-nowrap txt-uppercase", "for-screen-reader": icon_only), id: dom_id(board, :involvement_label)) + tag.span(label_text, class: class_names("txt-nowrap txt-uppercase", "for-screen-reader": icon_only), id: dom_id(board, :involvement_label)) end end From 642506e79ed5a0a4e977d1e3ea099c48b1ae0aaf Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 8 Dec 2025 11:52:17 -0500 Subject: [PATCH 78/90] Add datetime to the search results I find it helpful (especially when going through a long list of results) to know how far back in time I'm looking. A note on the timestamp being used: it's the search index timestamp. Using the card or comment `updated_at` is fraught because we do so much touching of records (e.g., comment is touched when a reaction is added; card is touched when a comment is added). Let's display the search record's `created_at`, which is the ordering used for the overall results. --- app/views/searches/_result.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/searches/_result.html.erb b/app/views/searches/_result.html.erb index b2b131910..ec110dd80 100644 --- a/app/views/searches/_result.html.erb +++ b/app/views/searches/_result.html.erb @@ -15,7 +15,7 @@ <% end %>
      - <%= result.card.board.name %> + <%= result.card.board.name %> · <%= local_datetime_tag(result.created_at, style: :timeordate) %>
      <% end %> From b88182c3ad3de478818c37890dab88c77b03268c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 8 Dec 2025 10:01:28 -0800 Subject: [PATCH 79/90] Autotuner: use structured logging instead of Sentry (#2011) Move thousands of info-level Sentry events to logs. Query Loki rather than downsampling events: `event.action = "run.ruby-script", script.name = "autotuner"` --- config/initializers/autotuner.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/config/initializers/autotuner.rb b/config/initializers/autotuner.rb index ec8572b11..33eab9a05 100644 --- a/config/initializers/autotuner.rb +++ b/config/initializers/autotuner.rb @@ -3,12 +3,14 @@ Autotuner.enabled = true # This callback is called whenever a suggestion is provided by this gem. -# You can output this report to your logging pipeline, stdout, a file, -# or somewhere else! -Autotuner.reporter = proc do |report| +# Log to structured logging for query/analysis in Loki. This is called +# once per autotuner heuristic. +Autotuner.reporter = proc do |heuristic_report| + report = heuristic_report.to_s + Rails.logger.info "GCAUTOTUNE: #{report}" - if Fizzy.saas? - Sentry.capture_message "Autotuner suggestion", level: :info, extra: { report: report.to_s } - end + RailsStructuredLogging.instrument_script "autotuner" do + Rails.logger.info report.to_s + end if defined? RailsStructuredLogging end From 48730d67e035cc0e01c52054e25d66821ceb0906 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 8 Dec 2025 13:21:52 -0500 Subject: [PATCH 80/90] Disable board edit buttons for select all/none when not privileged ref: https://github.com/basecamp/fizzy/discussions/1992 --- ...ess_toggle.erb => _access_toggle.html.erb} | 0 app/views/boards/edit/_users.html.erb | 11 +++++----- test/controllers/boards_controller_test.rb | 21 +++++++++++++++++++ 3 files changed, 27 insertions(+), 5 deletions(-) rename app/views/boards/{_access_toggle.erb => _access_toggle.html.erb} (100%) diff --git a/app/views/boards/_access_toggle.erb b/app/views/boards/_access_toggle.html.erb similarity index 100% rename from app/views/boards/_access_toggle.erb rename to app/views/boards/_access_toggle.html.erb diff --git a/app/views/boards/edit/_users.html.erb b/app/views/boards/edit/_users.html.erb index 3214a2f2c..d8c16484a 100644 --- a/app/views/boards/edit/_users.html.erb +++ b/app/views/boards/edit/_users.html.erb @@ -1,3 +1,4 @@ +<% disabled = !Current.user.can_administer_board?(board) %>

      Who can access this board?

      @@ -16,7 +17,7 @@ @@ -26,14 +27,14 @@
      - + <%= button_tag "Select all", type: "button", class: "btn btn--plain txt-x-small txt-link font-weight-normal", data: { action: "click->toggle-class#checkAll" }, disabled: %> · - + <%= button_tag "Select none", type: "button", class: "btn btn--plain txt-x-small txt-link font-weight-normal", data: { action: "click->toggle-class#checkNone" }, disabled: %>
        - <%= access_toggles_for selected_users, selected: true, disabled: !Current.user.can_administer_board?(board) %> - <%= access_toggles_for unselected_users, selected: false, disabled: !Current.user.can_administer_board?(board) %> + <%= access_toggles_for selected_users, selected: true, disabled: %> + <%= access_toggles_for unselected_users, selected: false, disabled: %>
      <% end %> diff --git a/test/controllers/boards_controller_test.rb b/test/controllers/boards_controller_test.rb index 816a2e7b3..d9314775b 100644 --- a/test/controllers/boards_controller_test.rb +++ b/test/controllers/boards_controller_test.rb @@ -145,4 +145,25 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest assert_response :forbidden end + + test "disables select all/none buttons for non-privileged user" do + logout_and_sign_in_as :jz + assert_not users(:jz).can_administer_board?(boards(:writebook)) + + get edit_board_path(boards(:writebook)) + + assert_response :success + assert_select "button[disabled]", text: "Select all" + assert_select "button[disabled]", text: "Select none" + end + + test "enables select all/none buttons for privileged user" do + assert users(:kevin).can_administer_board?(boards(:writebook)) + + get edit_board_path(boards(:writebook)) + + assert_response :success + assert_select "button:not([disabled])", text: "Select all" + assert_select "button:not([disabled])", text: "Select none" + end end From dcc005be346321a689b2a78ba4c51872b79bef9a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 8 Dec 2025 11:00:40 -0800 Subject: [PATCH 81/90] Add lazy error context for Rails error reporter (#2014) Register a middleware with Rails.error that adds identity_id and account_id from Current attributes. Only evaluated when an error is actually reported, avoiding the cost on successful requests. --- Gemfile.saas.lock | 2 +- config/initializers/error_context.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 config/initializers/error_context.rb diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock index 0c8880b08..9100ff68a 100644 --- a/Gemfile.saas.lock +++ b/Gemfile.saas.lock @@ -21,7 +21,7 @@ GIT GIT remote: https://github.com/basecamp/fizzy-saas - revision: 3b30f3d56d8318bb9256469cb91c06a5a5c760a6 + revision: 43dbc896ce7b6a08194a92ddd1695d3f1ebf554b specs: fizzy-saas (0.1.0) audits1984 diff --git a/config/initializers/error_context.rb b/config/initializers/error_context.rb new file mode 100644 index 000000000..a1b207b6e --- /dev/null +++ b/config/initializers/error_context.rb @@ -0,0 +1,7 @@ +# Lazily add identity and account context to error reports. +# Only evaluated when an error is actually reported. +Rails.error.add_middleware ->(error, context:, **) do + context.merge \ + identity_id: Current.identity&.id, + account_id: Current.account&.external_account_id +end From 2352f30b614a5e2a09a8dc6aefb8a36742f6ff12 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Mon, 8 Dec 2025 20:05:03 +0100 Subject: [PATCH 82/90] Adopt a cooldown period for dependency updates As suggested by @flavorjones, and explained in: https://blog.yossarian.net/2025/11/21/We-should-all-be-using-dependency-cooldowns This applies: - 7 days as default: from the article, a 7-day cooldown would have prevented 8 out of 10 recent supply chain attacks. It seems reasonably long but not crazy long. - 14 days for major versions, to give some time to the community to find problems. --- .github/dependabot.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 86c66c646..f4f755614 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -20,8 +20,14 @@ updates: dependency-type: "development" schedule: interval: weekly + cooldown: + default-days: 7 + semver-major-days: 14 - package-ecosystem: github-actions directory: "/" schedule: interval: weekly open-pull-requests-limit: 10 + cooldown: + default-days: 7 + semver-major-days: 14 From abe48f0efa970d79ba46a6c1ed5dbe5a802007ec Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 8 Dec 2025 14:21:56 -0500 Subject: [PATCH 83/90] Ensure user toggles on board edit page are cached properly Not including the disabled flag as part of the cache resulted in issues like the one described in #1992 --- app/helpers/accesses_helper.rb | 2 +- test/controllers/boards_controller_test.rb | 24 ++++++++++++++++++++++ test/test_helper.rb | 2 +- test/test_helpers/caching_test_helper.rb | 14 +++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 test/test_helpers/caching_test_helper.rb diff --git a/app/helpers/accesses_helper.rb b/app/helpers/accesses_helper.rb index 68caf6995..69ef2efec 100644 --- a/app/helpers/accesses_helper.rb +++ b/app/helpers/accesses_helper.rb @@ -12,7 +12,7 @@ module AccessesHelper render partial: "boards/access_toggle", collection: users, as: :user, locals: { selected: selected, disabled: disabled }, - cached: ->(user) { [ user, selected ] } + cached: ->(user) { [ user, selected, disabled ] } end def access_involvement_advance_button(board, user, show_watchers: true, icon_only: false) diff --git a/test/controllers/boards_controller_test.rb b/test/controllers/boards_controller_test.rb index d9314775b..a7e3580de 100644 --- a/test/controllers/boards_controller_test.rb +++ b/test/controllers/boards_controller_test.rb @@ -166,4 +166,28 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest assert_select "button:not([disabled])", text: "Select all" assert_select "button:not([disabled])", text: "Select none" end + + test "access toggle disabled state is cached correctly" do + board = boards(:writebook) + david = users(:david) + + with_actionview_partial_caching do + # privileged user + assert users(:kevin).can_administer_board?(board) + + get edit_board_path(board) + + assert_response :success + assert_select "input.switch__input[name='user_ids[]'][value='#{david.id}']:not([disabled])" + + # unprivileged user + logout_and_sign_in_as :jz + assert_not users(:jz).can_administer_board?(board) + + get edit_board_path(board) + + assert_response :success + assert_select "input.switch__input[name='user_ids[]'][value='#{david.id}'][disabled]" + end + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 4cec836fd..bc856d815 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -44,7 +44,7 @@ module ActiveSupport fixtures :all include ActiveJob::TestHelper - include ActionTextTestHelper, CardTestHelper, ChangeTestHelper, SessionTestHelper + include ActionTextTestHelper, CachingTestHelper, CardTestHelper, ChangeTestHelper, SessionTestHelper include Turbo::Broadcastable::TestHelper setup do diff --git a/test/test_helpers/caching_test_helper.rb b/test/test_helpers/caching_test_helper.rb new file mode 100644 index 000000000..90dab64d5 --- /dev/null +++ b/test/test_helpers/caching_test_helper.rb @@ -0,0 +1,14 @@ +module CachingTestHelper + def with_actionview_partial_caching + was_cache = ActionView::PartialRenderer.collection_cache + was_perform_caching = ApplicationController.perform_caching + begin + ActionView::PartialRenderer.collection_cache = ActiveSupport::Cache.lookup_store(:memory_store) + ApplicationController.perform_caching = true + yield + ensure + ActionView::PartialRenderer.collection_cache = was_cache + ApplicationController.perform_caching = was_perform_caching + end + end +end From 3c4b838b259644414990e43fb61ec1acd15c1c32 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 8 Dec 2025 15:11:58 -0500 Subject: [PATCH 84/90] Fix Identity destruction callback ordering ref: #2003 Co-authored-by: Dylan --- app/models/identity.rb | 2 +- test/models/identity_test.rb | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/models/identity.rb b/app/models/identity.rb index ee4a323b2..bb69734b4 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -8,7 +8,7 @@ class Identity < ApplicationRecord has_one_attached :avatar - before_destroy :deactivate_users + before_destroy :deactivate_users, prepend: true validates :email_address, format: { with: URI::MailTo::EMAIL_REGEXP } normalizes :email_address, with: ->(value) { value.strip.downcase.presence } diff --git a/test/models/identity_test.rb b/test/models/identity_test.rb index 1c6c6b258..5b5763ffd 100644 --- a/test/models/identity_test.rb +++ b/test/models/identity_test.rb @@ -46,4 +46,19 @@ class IdentityTest < ActiveSupport::TestCase assert_equal identity.email_address, user.name end end + + test "destroy deactivates users before nullifying identity" do + identity = identities(:kevin) + user = users(:kevin) + + assert_predicate user, :active? + assert_predicate user.accesses, :any? + + identity.destroy! + user.reload + + assert_nil user.identity_id, "identity should be nullified" + assert_not_predicate user, :active? + assert_empty user.accesses, "user accesses should be removed" + end end From e1c9b7df1e5aaddb6b26c07f1b92d06ad768e312 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 8 Dec 2025 15:29:26 -0500 Subject: [PATCH 85/90] doc: update style doc reference for LLM agents --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 428c802a8..80bb887a2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,4 +148,4 @@ Use Chrome MCP tools to interact with the running dev app for UI testing and deb ## Coding style -Please read the separate file `STYLE.md` for some guidance on coding style. +@STYLE.md From 27585a75b211a8cc8fdba2c65749f0572ee8d380 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Mon, 8 Dec 2025 15:01:47 -0600 Subject: [PATCH 86/90] Wrap the overflow menu --- app/assets/stylesheets/lexxy.css | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/stylesheets/lexxy.css b/app/assets/stylesheets/lexxy.css index 7b647caca..04277d846 100644 --- a/app/assets/stylesheets/lexxy.css +++ b/app/assets/stylesheets/lexxy.css @@ -149,6 +149,7 @@ border-radius: 0.5ch; box-shadow: var(--shadow); display: flex; + flex-wrap: wrap; inset-inline-end: 0; padding: 4px; position: absolute; From bc8ecc3e91df43f74e0ba7a5422fcece9a984021 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Mon, 8 Dec 2025 15:59:58 -0600 Subject: [PATCH 87/90] Account for browser button styled and adjust mobile padding for perma BG --- app/assets/stylesheets/card-perma.css | 6 +++++- app/assets/stylesheets/lexxy.css | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/card-perma.css b/app/assets/stylesheets/card-perma.css index 150dde9a1..3132d886f 100644 --- a/app/assets/stylesheets/card-perma.css +++ b/app/assets/stylesheets/card-perma.css @@ -127,7 +127,7 @@ } @media (max-width: 799px) { - --padding-inline: var(--main-padding); + --padding-inline: 1.5ch; column-gap: 0; grid-template-areas: @@ -150,6 +150,10 @@ border-radius: 0.2em; grid-area: card; padding: clamp(2rem, 4vw, var(--padding-block)); + + @media (max-width: 799px) { + padding-inline: var(--padding-inline); + } } .card-perma__actions { diff --git a/app/assets/stylesheets/lexxy.css b/app/assets/stylesheets/lexxy.css index 04277d846..cc79ca76a 100644 --- a/app/assets/stylesheets/lexxy.css +++ b/app/assets/stylesheets/lexxy.css @@ -234,10 +234,11 @@ background: var(--color-canvas); border: none; border-radius: 0.5ch; + color: inherit; display: grid; font-weight: 500; inline-size: var(--lexxy-dropdown-btn-size); - place-items: center; + place-content: center; position: relative; outline: none; From 3cdcd3f00c2dd8c7c2c03918a77493cad043727e Mon Sep 17 00:00:00 2001 From: Jason Zimdars Date: Mon, 8 Dec 2025 16:03:39 -0600 Subject: [PATCH 88/90] Should be in global gitignore --- .gitignore | 3 --- 1 file changed, 3 deletions(-) diff --git a/.gitignore b/.gitignore index 7883e425c..38bd80d0d 100644 --- a/.gitignore +++ b/.gitignore @@ -41,6 +41,3 @@ /config/credentials/*.key .DS_Store - -# IDEA folder -.idea/ \ No newline at end of file From cb0e9b99622cfed0c3cc57d735275493a695d0ff Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Mon, 8 Dec 2025 23:17:06 +0100 Subject: [PATCH 89/90] Immediate avatar and embed variants (#2002) Reverts #2001 Restores #1955 --- Gemfile.lock | 2 +- Gemfile.saas.lock | 6 ++--- app/models/concerns/attachments.rb | 9 +++---- app/models/user/avatar.rb | 2 +- config/initializers/action_text.rb | 2 +- .../users/avatars_controller_test.rb | 8 +++++-- test/controllers/users_controller_test.rb | 5 ++-- .../notification/bundle_mailer_test.rb | 3 ++- test/models/comment_test.rb | 20 ++++++++++++++++ test/models/user/avatar_test.rb | 24 +++++++++++++++++-- 10 files changed, 62 insertions(+), 19 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index e535c9864..e4f364c1c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -6,7 +6,7 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 3c3f6c8a253c8a0f346695374f927c9ab32fbefb + revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 branch: main specs: actioncable (8.2.0.alpha) diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock index 9100ff68a..f093ac677 100644 --- a/Gemfile.saas.lock +++ b/Gemfile.saas.lock @@ -75,7 +75,7 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 690ec8898318b8f50714e86676353ebe1551261e + revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 branch: main specs: actioncable (8.2.0.alpha) @@ -292,7 +292,7 @@ GEM actionview (>= 7.0.0) activesupport (>= 7.0.0) jmespath (1.6.2) - json (2.16.0) + json (2.17.1) jwt (3.1.2) base64 kamal (2.9.0) @@ -552,7 +552,7 @@ GEM ostruct stimulus-rails (1.3.4) railties (>= 6.0.0) - stringio (3.1.8) + stringio (3.1.9) thor (1.4.0) thruster (0.1.16) thruster (0.1.16-aarch64-linux) diff --git a/app/models/concerns/attachments.rb b/app/models/concerns/attachments.rb index 1d1c2c4d6..bcc7303ae 100644 --- a/app/models/concerns/attachments.rb +++ b/app/models/concerns/attachments.rb @@ -1,13 +1,10 @@ module Attachments extend ActiveSupport::Concern - # The variants we use must all declared here so that we can preprocess them. + # Variants used by ActionText embeds. Processed immediately on attachment to avoid + # read replica issues (lazy variants would attempt writes on read replicas). # - # If they are not preprocessed, then Rails will attempt to transform the image on-the-fly when - # they are first viewed, which may be on the read replica where writing to the database is not - # allowed. Chaos will ensue if that happens. - # - # These variants are patched into ActionText::RichText in config/initializers/action_text.rb + # Patched into ActionText::RichText in config/initializers/action_text.rb VARIANTS = { # vipsthumbnail used to create sized image variants has a intent setting to manage colors during # resize. By setting an invalid intent value the gif-incompatible intent filtering is skipped and diff --git a/app/models/user/avatar.rb b/app/models/user/avatar.rb index a4b2ae631..c635edddb 100644 --- a/app/models/user/avatar.rb +++ b/app/models/user/avatar.rb @@ -5,7 +5,7 @@ module User::Avatar included do has_one_attached :avatar do |attachable| - attachable.variant :thumb, resize_to_fill: [ 256, 256 ] + attachable.variant :thumb, resize_to_fill: [ 256, 256 ], process: :immediately end validate :avatar_content_type_allowed diff --git a/config/initializers/action_text.rb b/config/initializers/action_text.rb index b2724b1c7..eee87de0d 100644 --- a/config/initializers/action_text.rb +++ b/config/initializers/action_text.rb @@ -7,7 +7,7 @@ module ActionText # This overrides the default :embeds association! has_many_attached :embeds do |attachable| ::Attachments::VARIANTS.each do |variant_name, variant_options| - attachable.variant variant_name, variant_options.merge(preprocessed: true) + attachable.variant variant_name, **variant_options, process: :immediately end end end diff --git a/test/controllers/users/avatars_controller_test.rb b/test/controllers/users/avatars_controller_test.rb index 3ded884b8..be4532448 100644 --- a/test/controllers/users/avatars_controller_test.rb +++ b/test/controllers/users/avatars_controller_test.rb @@ -27,7 +27,9 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show own image redirects to the blob url" do - users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + # Create blob separately to ensure file is uploaded before variant processing + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + users(:david).avatar.attach(blob) assert users(:david).avatar.attached? get user_avatar_path(users(:david)) @@ -36,7 +38,9 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show other image redirects to the blob url" do - users(:kevin).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + # Create blob separately to ensure file is uploaded before variant processing + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + users(:kevin).avatar.attach(blob) assert users(:kevin).avatar.attached? get user_avatar_path(users(:kevin)) diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 385bacaf3..d8441d033 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -79,9 +79,10 @@ class UsersControllerTest < ActionDispatch::IntegrationTest test "update with valid avatar" do sign_in_as :kevin - png_file = fixture_file_upload("avatar.png", "image/png") + # Create blob separately to ensure file is uploaded before variant processing + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("avatar.png")), filename: "avatar.png", content_type: "image/png") - put user_path(users(:kevin)), params: { user: { avatar: png_file } } + put user_path(users(:kevin)), params: { user: { avatar: blob.signed_id } } assert_redirected_to user_path(users(:kevin)) assert users(:kevin).reload.avatar.attached? assert_equal "image/png", users(:kevin).avatar.content_type diff --git a/test/mailers/notification/bundle_mailer_test.rb b/test/mailers/notification/bundle_mailer_test.rb index fbc4274c9..63647e68a 100644 --- a/test/mailers/notification/bundle_mailer_test.rb +++ b/test/mailers/notification/bundle_mailer_test.rb @@ -22,11 +22,12 @@ class Notification::BundleMailerTest < ActionMailer::TestCase end test "renders avatar with external image URL when avatar is attached" do - @user.avatar.attach( + blob = ActiveStorage::Blob.create_and_upload!( io: File.open(Rails.root.join("test", "fixtures", "files", "avatar.png")), filename: "avatar.png", content_type: "image/png" ) + @user.avatar.attach(blob) create_notification(@user) diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index c519f8fbd..13a5e36c3 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -4,4 +4,24 @@ class CommentTest < ActiveSupport::TestCase setup do Current.session = sessions(:david) end + + test "rich text embed variants are processed immediately on attachment" do + comment = cards(:logo).comments.create!(body: "Check this out") + comment.body.body.attachables # force load + + blob = ActiveStorage::Blob.create_and_upload! \ + io: File.open(file_fixture("moon.jpg")), + filename: "moon.jpg", + content_type: "image/jpeg" + + comment.body.body = ActionText::Content.new(comment.body.body.to_html).append_attachables(blob) + comment.save! + + embed = comment.body.embeds.sole + + Attachments::VARIANTS.each_key do |variant_name| + variant = embed.variant(variant_name) + assert variant.processed?, "Expected #{variant_name} variant to be processed immediately" + end + end end diff --git a/test/models/user/avatar_test.rb b/test/models/user/avatar_test.rb index 96ea5bac7..58b78969c 100644 --- a/test/models/user/avatar_test.rb +++ b/test/models/user/avatar_test.rb @@ -2,7 +2,8 @@ require "test_helper" class User::AvatarTest < ActiveSupport::TestCase test "avatar_thumbnail returns variant for variable images" do - users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + users(:david).avatar.attach(blob) assert users(:david).avatar.variable? assert_equal users(:david).avatar.variant(:thumb).blob, users(:david).avatar_thumbnail.blob @@ -16,7 +17,8 @@ class User::AvatarTest < ActiveSupport::TestCase end test "allows valid image content types" do - users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg") + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg", content_type: "image/jpeg") + users(:david).avatar.attach(blob) assert users(:david).valid? end @@ -27,4 +29,22 @@ class User::AvatarTest < ActiveSupport::TestCase assert_not users(:david).valid? assert_includes users(:david).errors[:avatar], "must be a JPEG, PNG, GIF, or WebP image" end + + test "thumb variant is processed immediately on attachment" do + # Create blob separately to ensure file is uploaded before variant processing. + # + # Root cause: When ActiveStorage::Record uses `connects_to` for read replica support + # (as in SAAS mode), it creates a separate connection pool from application models. + # Since after_commit callbacks are tracked per connection pool, the callback order + # between User's pool (upload) and Attachment's pool (create_variants) isn't guaranteed. + # In MySQL/SAAS mode, the Attachment callback fires before the file is uploaded. + blob = ActiveStorage::Blob.create_and_upload!( + io: File.open(file_fixture("avatar.png")), + filename: "avatar.png", + content_type: "image/png" + ) + users(:david).avatar.attach(blob) + + assert users(:david).avatar.variant(:thumb).processed? + end end From 49c4f2adc6069d8e58f3091a797e9182d85ebbb6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 8 Dec 2025 14:44:49 -0800 Subject: [PATCH 90/90] Fix ActiveStorage FileNotFoundError with immediate variant processing (#2022) When ActiveStorage::Record uses `connects_to` for read replica support, it creates a separate connection pool from ApplicationRecord. This causes `after_commit` callbacks to fire in non-deterministic order - the Attachment's `create_variants` callback can fire before the User model's upload callback completes, resulting in FileNotFoundError. The fix removes replica connection configuration from ActiveStorage::Record so it shares the same connection pool as application models, ensuring proper callback ordering. Also reverts test workarounds that were added to work around this issue, since the root cause is now fixed. See: https://github.com/rails/rails/issues/53694 --- config/initializers/active_storage.rb | 14 ++++++++++--- .../users/avatars_controller_test.rb | 8 ++------ test/controllers/users_controller_test.rb | 5 ++--- .../notification/bundle_mailer_test.rb | 3 +-- test/models/user/avatar_test.rb | 20 +++---------------- 5 files changed, 19 insertions(+), 31 deletions(-) diff --git a/config/initializers/active_storage.rb b/config/initializers/active_storage.rb index d00bd74bf..4a4a0343d 100644 --- a/config/initializers/active_storage.rb +++ b/config/initializers/active_storage.rb @@ -4,9 +4,17 @@ ActiveSupport.on_load(:active_storage_blob) do end end -ActiveSupport.on_load(:active_storage_record) do - configure_replica_connections -end +# Don't configure replica connections for ActiveStorage::Record. +# When ActiveStorage uses `connects_to`, it creates a separate connection pool +# from ApplicationRecord. This causes after_commit callbacks to fire in +# non-deterministic order - the Attachment's create_variants callback can fire +# before the User model's upload callback, causing FileNotFoundError when +# using `process: :immediately` for variants. +# See: https://github.com/rails/rails/issues/53694 +# +# ActiveSupport.on_load(:active_storage_record) do +# configure_replica_connections +# end module ActiveStorageControllerExtensions extend ActiveSupport::Concern diff --git a/test/controllers/users/avatars_controller_test.rb b/test/controllers/users/avatars_controller_test.rb index be4532448..3ded884b8 100644 --- a/test/controllers/users/avatars_controller_test.rb +++ b/test/controllers/users/avatars_controller_test.rb @@ -27,9 +27,7 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show own image redirects to the blob url" do - # Create blob separately to ensure file is uploaded before variant processing - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") assert users(:david).avatar.attached? get user_avatar_path(users(:david)) @@ -38,9 +36,7 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show other image redirects to the blob url" do - # Create blob separately to ensure file is uploaded before variant processing - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") - users(:kevin).avatar.attach(blob) + users(:kevin).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") assert users(:kevin).avatar.attached? get user_avatar_path(users(:kevin)) diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index d8441d033..385bacaf3 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -79,10 +79,9 @@ class UsersControllerTest < ActionDispatch::IntegrationTest test "update with valid avatar" do sign_in_as :kevin - # Create blob separately to ensure file is uploaded before variant processing - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("avatar.png")), filename: "avatar.png", content_type: "image/png") + png_file = fixture_file_upload("avatar.png", "image/png") - put user_path(users(:kevin)), params: { user: { avatar: blob.signed_id } } + put user_path(users(:kevin)), params: { user: { avatar: png_file } } assert_redirected_to user_path(users(:kevin)) assert users(:kevin).reload.avatar.attached? assert_equal "image/png", users(:kevin).avatar.content_type diff --git a/test/mailers/notification/bundle_mailer_test.rb b/test/mailers/notification/bundle_mailer_test.rb index 63647e68a..fbc4274c9 100644 --- a/test/mailers/notification/bundle_mailer_test.rb +++ b/test/mailers/notification/bundle_mailer_test.rb @@ -22,12 +22,11 @@ class Notification::BundleMailerTest < ActionMailer::TestCase end test "renders avatar with external image URL when avatar is attached" do - blob = ActiveStorage::Blob.create_and_upload!( + @user.avatar.attach( io: File.open(Rails.root.join("test", "fixtures", "files", "avatar.png")), filename: "avatar.png", content_type: "image/png" ) - @user.avatar.attach(blob) create_notification(@user) diff --git a/test/models/user/avatar_test.rb b/test/models/user/avatar_test.rb index 58b78969c..ed502d7ab 100644 --- a/test/models/user/avatar_test.rb +++ b/test/models/user/avatar_test.rb @@ -2,8 +2,7 @@ require "test_helper" class User::AvatarTest < ActiveSupport::TestCase test "avatar_thumbnail returns variant for variable images" do - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") assert users(:david).avatar.variable? assert_equal users(:david).avatar.variant(:thumb).blob, users(:david).avatar_thumbnail.blob @@ -17,8 +16,7 @@ class User::AvatarTest < ActiveSupport::TestCase end test "allows valid image content types" do - blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg", content_type: "image/jpeg") - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg", content_type: "image/jpeg") assert users(:david).valid? end @@ -31,19 +29,7 @@ class User::AvatarTest < ActiveSupport::TestCase end test "thumb variant is processed immediately on attachment" do - # Create blob separately to ensure file is uploaded before variant processing. - # - # Root cause: When ActiveStorage::Record uses `connects_to` for read replica support - # (as in SAAS mode), it creates a separate connection pool from application models. - # Since after_commit callbacks are tracked per connection pool, the callback order - # between User's pool (upload) and Attachment's pool (create_variants) isn't guaranteed. - # In MySQL/SAAS mode, the Attachment callback fires before the file is uploaded. - blob = ActiveStorage::Blob.create_and_upload!( - io: File.open(file_fixture("avatar.png")), - filename: "avatar.png", - content_type: "image/png" - ) - users(:david).avatar.attach(blob) + users(:david).avatar.attach(io: File.open(file_fixture("avatar.png")), filename: "avatar.png", content_type: "image/png") assert users(:david).avatar.variant(:thumb).processed? end