-
Notifications
You must be signed in to change notification settings - Fork 737
Fix: board columns actions are stale when moving a column moves #2032
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: board columns actions are stale when moving a column moves #2032
Conversation
jorgemanrubia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great one @tomycostantino 🙏. Some minor suggestions and a request for a change that can fix other related bug we have.
app/models/column/positioned.rb
Outdated
| right_column.nil? | ||
| end | ||
|
|
||
| def surroundings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I would rename to surrounding_columns or adjacent_columns, so that it is clear what this is returning.
| @@ -1 +1,2 @@ | |||
| <%= turbo_stream.remove(dom_id(@column)) %> | |||
| <%= turbo_stream.remove(dom_id(@column)) %> | |||
| <%= render "columns/refresh_surroundings", column: @column %> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a bug where deleting a column won't show its cards automatically in the Maybe column. We could get rid of the destroy turbo stream action and replace with just a redirect_back_or_to @board, so that a full page morph will take care of the problem. If you are happy to tackle that here, we can fix that bug as part of this PR 🙏. We can remove the destroy stream action and replace with the redirect.
| @@ -0,0 +1,4 @@ | |||
| <% column.surroundings.each do |surrounding| %> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my feedback above: I'd rather see surrounding_column or adjacent_column than just surrounding.
app/models/column/positioned.rb
Outdated
|
|
||
| before_create :set_position | ||
| after_create_commit -> { surroundings.touch_all } | ||
| after_destroy_commit -> { surroundings.touch_all } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this explicit invalidation? In theory we consider adjacent columns for caching here:
| <%= render partial: "boards/show/column", collection: board.columns.sorted, cached: ->(column){ [ column, column.leftmost?, column.rightmost? ] } %> |
|
Hey @jorgemanrubia thank you for reviewing! Just made the changes you've requested, and fixed the bug referenced in the issue. Here's how it looks: screenrecording-2025-12-11_10-49-52.mp4 |
jorgemanrubia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic @tomycostantino thanks
Problem
When creating or destroying a column or moving it either right or left, the
views/boards/show/menu/_column_form.html.erbpartial of already existing columns is stale, so the action buttons inside the form are either disabled or actionable when should not be the case.How to reproduce
Demonstration video
screenrecording-2025-12-09_10-31-58.mp4
Solution
Refresh board columns on move right/left and create/destroy actions. I extracted the update logic into the
views/columns/_refresh_surroundings.turbo_stream.erbwhich is rendered by other streams.