Skip to content
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

Code maintenance/59280 ensure that the new activity tab renders quickly also for work packages with over 100 comments #17224

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,53 +1,59 @@
<%=
content_tag("turbo-frame", id: "work-package-activities-tab-content") do
flex_layout(classes: "work-packages-activities-tab-index-component", mb: [5, 5, 5, 5, 0]) do |activties_tab_wrapper_container|
activties_tab_wrapper_container.with_row(classes: "work-packages-activities-tab-index-component--errors") do
render(
WorkPackages::ActivitiesTab::ErrorStreamComponent.new
)
end
activties_tab_wrapper_container.with_row do
component_wrapper(data: wrapper_data_attributes) do
flex_layout do |activties_tab_container|
activties_tab_container.with_row(mb: 2) do
render(
WorkPackages::ActivitiesTab::Journals::FilterAndSortingComponent.new(
work_package:,
filter:
)
)
end
activties_tab_container.with_row(flex_layout: true, mt: 3) do |journals_wrapper_container|
journals_wrapper_container.with_row(
classes: "work-packages-activities-tab-index-component--journals-container work-packages-activities-tab-index-component--journals-container_with-initial-input-compensation",
data: { "work-packages--activities-tab--index-target": "journalsContainer" }
) do
unless deferred
content_tag("turbo-frame", id: "work-package-activities-tab-content") do
flex_layout(classes: "work-packages-activities-tab-index-component", mb: [5, 5, 5, 5, 0]) do |activties_tab_wrapper_container|
activties_tab_wrapper_container.with_row(classes: "work-packages-activities-tab-index-component--errors") do
render(
WorkPackages::ActivitiesTab::ErrorStreamComponent.new
)
end
activties_tab_wrapper_container.with_row do
component_wrapper(data: wrapper_data_attributes) do
flex_layout do |activties_tab_container|
activties_tab_container.with_row(mb: 2) do
render(
WorkPackages::ActivitiesTab::Journals::IndexComponent.new(work_package:, filter:)
WorkPackages::ActivitiesTab::Journals::FilterAndSortingComponent.new(
work_package:,
filter:
)
)
end
if adding_comment_allowed?
activties_tab_container.with_row(flex_layout: true, mt: 3) do |journals_wrapper_container|
journals_wrapper_container.with_row(
classes: "work-packages-activities-tab-index-component--input-container work-packages-activities-tab-index-component--input-container_sort-#{journal_sorting}",
mt: 3,
mb: [3, nil, nil, nil, 0],
pt: 2,
pb: 2,
pl: 3,
pr: [3, nil, nil, nil, 2],
border: [nil, nil, nil, nil, :top],
border_radius: [2, nil, nil, nil, 0],
bg: :subtle
classes: "work-packages-activities-tab-index-component--journals-container work-packages-activities-tab-index-component--journals-container_with-initial-input-compensation",
data: { "work-packages--activities-tab--index-target": "journalsContainer" }
) do
render(
WorkPackages::ActivitiesTab::Journals::NewComponent.new(work_package:)
WorkPackages::ActivitiesTab::Journals::IndexComponent.new(work_package:, filter:)
)
end
if adding_comment_allowed?
journals_wrapper_container.with_row(
classes: "work-packages-activities-tab-index-component--input-container work-packages-activities-tab-index-component--input-container_sort-#{journal_sorting}",
mt: 3,
mb: [3, nil, nil, nil, 0],
pt: 2,
pb: 2,
pl: 3,
pr: [3, nil, nil, nil, 2],
border: [nil, nil, nil, nil, :top],
border_radius: [2, nil, nil, nil, 0],
bg: :subtle
) do
render(
WorkPackages::ActivitiesTab::Journals::NewComponent.new(work_package:)
)
end
end
end
end
end
end
end
end
else
render(
WorkPackages::ActivitiesTab::Journals::IndexComponent.new(work_package:, filter:, deferred:)
)
end
%>
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,18 @@ class IndexComponent < ApplicationComponent
include OpTurbo::Streamable
include WorkPackages::ActivitiesTab::SharedHelpers

def initialize(work_package:, last_server_timestamp:, filter: :all)
def initialize(work_package:, last_server_timestamp:, filter: :all, deferred: false)
super

@work_package = work_package
@filter = filter
@last_server_timestamp = last_server_timestamp
@deferred = deferred
end

private

attr_reader :work_package, :filter, :last_server_timestamp
attr_reader :work_package, :filter, :last_server_timestamp, :deferred

def wrapper_data_attributes
stimulus_controller = "work-packages--activities-tab--index"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,34 +1,61 @@
<%=
component_wrapper(class: "work-packages-activities-tab-journals-index-component") do
flex_layout(data: { test_selector: "op-wp-journals-#{filter}-#{journal_sorting}" }) do |journals_index_wrapper_container|
journals_index_wrapper_container.with_row(
classes: "work-packages-activities-tab-journals-index-component--journals-inner-container",
mb: inner_container_margin_bottom
) do
flex_layout(id: insert_target_modifier_id,
data: { test_selector: "op-wp-journals-container" }) do |journals_index_container|
if empty_state?
journals_index_container.with_row(mt: 2, mb: 3) do
render(
WorkPackages::ActivitiesTab::Journals::EmptyComponent.new
)
unless deferred
component_wrapper(class: "work-packages-activities-tab-journals-index-component") do
flex_layout(data: { test_selector: "op-wp-journals-#{filter}-#{journal_sorting}" }) do |journals_index_wrapper_container|
journals_index_wrapper_container.with_row(
classes: "work-packages-activities-tab-journals-index-component--journals-inner-container",
mb: inner_container_margin_bottom
) do
flex_layout(id: insert_target_modifier_id,
data: { test_selector: "op-wp-journals-container" }) do |journals_index_container|
if empty_state?
journals_index_container.with_row(mt: 2, mb: 3) do
render(
WorkPackages::ActivitiesTab::Journals::EmptyComponent.new
)
end
end

if !journal_sorting_desc? && journals.count > MAX_RECENT_JOURNALS
journals_index_container.with_row do
helpers.turbo_frame_tag("work-package-activities-tab-content-older-journals", src: work_package_activities_path(work_package, filter:, deferred: true))
end
end
end

journals.each do |journal|
journals_index_container.with_row do
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal:, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[journal.id]
))
recent_journals.each do |journal|
journals_index_container.with_row do
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal:, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[journal.id]
))
end
end

if journal_sorting_desc? && journals.count > MAX_RECENT_JOURNALS
journals_index_container.with_row do
helpers.turbo_frame_tag("work-package-activities-tab-content-older-journals", src: work_package_activities_path(work_package, filter:, deferred: true))
end
end
end
end
end

unless empty_state? || journal_sorting_desc?
journals_index_wrapper_container
.with_row(classes: "work-packages-activities-tab-journals-index-component--stem-connection")
unless empty_state? || journal_sorting_desc?
journals_index_wrapper_container
.with_row(classes: "work-packages-activities-tab-journals-index-component--stem-connection")
end
end
end
else
helpers.turbo_frame_tag("work-package-activities-tab-content-older-journals") do
flex_layout do |older_journals_container|
older_journals.each do |journal|
older_journals_container.with_row do
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal:, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[journal.id]
))
end
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,24 @@ module WorkPackages
module ActivitiesTab
module Journals
class IndexComponent < ApplicationComponent
MAX_RECENT_JOURNALS = 30

include ApplicationHelper
include OpPrimer::ComponentHelpers
include OpTurbo::Streamable
include WorkPackages::ActivitiesTab::SharedHelpers

def initialize(work_package:, filter: :all)
def initialize(work_package:, filter: :all, deferred: false)
super

@work_package = work_package
@filter = filter
@deferred = deferred
end

private

attr_reader :work_package, :filter
attr_reader :work_package, :filter, :deferred

def insert_target_modified?
true
Expand All @@ -60,16 +63,48 @@ def journal_sorting_desc?
journal_sorting == "desc"
end

def journals
def base_journals
work_package
.journals
.includes(:user, :notifications)
.includes(
:user,
:customizable_journals,
:attachable_journals,
:storable_journals,
:notifications
)
.reorder(version: journal_sorting)
.with_sequence_version
end

def journals
API::V3::Activities::ActivityEagerLoadingWrapper.wrap(base_journals)
end

def recent_journals
recent_ones = if journal_sorting_desc?
base_journals.first(MAX_RECENT_JOURNALS)
else
base_journals.last(MAX_RECENT_JOURNALS)
end

API::V3::Activities::ActivityEagerLoadingWrapper.wrap(recent_ones)
end

def older_journals
older_ones = if journal_sorting_desc?
base_journals.offset(MAX_RECENT_JOURNALS)
else
total = base_journals.count
limit = [total - MAX_RECENT_JOURNALS, 0].max
base_journals.limit(limit)
end

API::V3::Activities::ActivityEagerLoadingWrapper.wrap(older_ones)
end

def journal_with_notes
journals.where.not(notes: "")
base_journals.where.not(notes: "")
end

def wp_journals_grouped_emoji_reactions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def updated?
end

def has_unread_notifications?
journal.notifications.where(read_ian: false, recipient_id: User.current.id).any?
journal.has_unread_notifications_for_user?(User.current)
end

def notification_on_details?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
when :only_comments
render_empty_line(details_container) unless journal.notes.blank? && !journal.noop?
when :only_changes
if journal.details.any?
if has_details?
render_details_header(details_container)
render_details(details_container)
end
else
if journal.details.any?
if has_details?
if journal.notes.present?
render_details(details_container)
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ def wrapper_uniq_by
journal.id
end

def journal_details
@journal_details ||= journal.details
end

def has_details?
@has_details ||= journal_details.any?
end

def render_details_header(details_container)
details_container.with_row(
flex_layout: true,
Expand Down Expand Up @@ -223,7 +231,7 @@ def skip_rendering_details?
end

def render_journal_details(details_container_inner)
journal.details.each do |detail|
journal_details.each do |detail|
rendered_detail = journal.render_detail(detail)
render_single_detail(details_container_inner, rendered_detail) if rendered_detail.present?
end
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/work_packages/activities_tab_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ def index
WorkPackages::ActivitiesTab::IndexComponent.new(
work_package: @work_package,
filter: @filter,
last_server_timestamp: get_current_server_timestamp
last_server_timestamp: get_current_server_timestamp,
deferred: ActiveRecord::Type::Boolean.new.cast(params[:deferred])
),
layout: false
)
Expand Down Expand Up @@ -374,6 +375,7 @@ def rerender_journals_with_updated_notification(journals, last_update_timestamp,
# alternative approach in order to bypass the notification join issue in relation with the sequence_version query
Notification
.where(journal_id: journals.pluck(:id))
.where(recipient_id: User.current.id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

.where("notifications.updated_at > ?", last_update_timestamp)
.find_each do |notification|
update_item_show_component(
Expand Down
10 changes: 10 additions & 0 deletions app/models/journal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,16 @@ def has_cause?
cause_type.present?
end

def has_unread_notifications_for_user?(user)
# we optionally set the instance variable @unread_notifications in the ActivityEagerLoadingWrapper
# in order to avoid N+1 queries
if instance_variable_defined?(:@unread_notifications)
@unread_notifications&.any? { |notification| notification.recipient_id == user.id }
else
notifications.where(read_ian: false, recipient_id: user.id).any?
end
end

private

def has_file_links?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,15 +355,22 @@ export default class IndexController extends Controller {
}
}

private scrollToActivity(activityId:string) {
private tryScroll(activityId:string, attempts:number, maxAttempts:number) {
const scrollableContainer = this.getScrollableContainer();
const activityElement = document.getElementById(`activity-anchor-${activityId}`);

if (activityElement && scrollableContainer) {
scrollableContainer.scrollTop = activityElement.offsetTop-70;
scrollableContainer.scrollTop = activityElement.offsetTop - 70;
} else if (attempts < maxAttempts) {
setTimeout(() => this.tryScroll(activityId, attempts + 1, maxAttempts), 1000);
}
}

private scrollToActivity(activityId:string) {
const maxAttempts = 20; // wait max 20 seconds for the activity to be rendered
this.tryScroll(activityId, 0, maxAttempts);
}

private scrollToBottom() {
const scrollableContainer = this.getScrollableContainer();
if (scrollableContainer) {
Expand Down
Loading