mirror of
https://github.com/discourse/discourse-rewind.git
synced 2025-12-11 02:05:32 +00:00
PERF: Load reports async
This commit is contained in:
parent
f4560c2292
commit
34973acc55
@ -6,7 +6,7 @@ module ::DiscourseRewind
|
||||
|
||||
requires_login
|
||||
|
||||
def show
|
||||
def index
|
||||
DiscourseRewind::FetchReports.call(service_params) do
|
||||
on_model_not_found(:year) do
|
||||
raise Discourse::NotFound.new(nil, custom_message: "discourse_rewind.invalid_year")
|
||||
@ -14,7 +14,27 @@ module ::DiscourseRewind
|
||||
on_model_not_found(:reports) do
|
||||
raise Discourse::NotFound.new(nil, custom_message: "discourse_rewind.report_failed")
|
||||
end
|
||||
on_success { |reports:| render json: MultiJson.dump(reports), status: :ok }
|
||||
on_success do |reports:, total_available:|
|
||||
render json: { reports: reports, total_available: total_available }, status: :ok
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def show
|
||||
DiscourseRewind::FetchReport.call(service_params) do
|
||||
on_model_not_found(:year) do
|
||||
raise Discourse::NotFound.new(nil, custom_message: "discourse_rewind.invalid_year")
|
||||
end
|
||||
on_model_not_found(:report_class) do
|
||||
raise Discourse::NotFound.new(
|
||||
nil,
|
||||
custom_message: "discourse_rewind.invalid_report_index",
|
||||
)
|
||||
end
|
||||
on_model_not_found(:report) do
|
||||
raise Discourse::NotFound.new(nil, custom_message: "discourse_rewind.report_failed")
|
||||
end
|
||||
on_success { |report:| render json: { report: report }, status: :ok }
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
92
app/services/discourse_rewind/fetch_report.rb
Normal file
92
app/services/discourse_rewind/fetch_report.rb
Normal file
@ -0,0 +1,92 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
module DiscourseRewind
|
||||
# Service responsible to fetch a single report by index.
|
||||
#
|
||||
# @example
|
||||
# ::DiscourseRewind::FetchReport.call(
|
||||
# guardian: guardian,
|
||||
# index: 3
|
||||
# )
|
||||
#
|
||||
class FetchReport
|
||||
include Service::Base
|
||||
|
||||
# @!method self.call(guardian:, params:)
|
||||
# @param [Guardian] guardian
|
||||
# @param [Hash] params
|
||||
# @option params [Integer] :index the report index
|
||||
# @return [Service::Base::Context]
|
||||
|
||||
CACHE_DURATION = Rails.env.development? ? 10.seconds : 5.minutes
|
||||
|
||||
model :year
|
||||
model :date
|
||||
model :report_class
|
||||
model :report
|
||||
|
||||
private
|
||||
|
||||
def fetch_year
|
||||
current_date = Time.zone.now
|
||||
current_month = current_date.month
|
||||
current_year = current_date.year
|
||||
|
||||
case current_month
|
||||
when 1
|
||||
current_year - 1
|
||||
when 12
|
||||
current_year
|
||||
else
|
||||
# Otherwise it's impossible to test in browser unless you're
|
||||
# in December or January
|
||||
if Rails.env.development?
|
||||
current_year
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def fetch_date(params:, year:)
|
||||
Date.new(year).all_year
|
||||
end
|
||||
|
||||
def fetch_report_class(date:, guardian:, year:, params:)
|
||||
# Use the same cached all_reports list as FetchReports
|
||||
# If not cached, generate it now
|
||||
key = "rewind:#{guardian.user.username}:#{year}:all_reports"
|
||||
cached_list = Discourse.redis.get(key)
|
||||
|
||||
all_reports =
|
||||
if cached_list
|
||||
MultiJson.load(cached_list, symbolize_keys: true)
|
||||
else
|
||||
# Generate all reports and cache them
|
||||
reports =
|
||||
FetchReports::REPORTS.filter_map do |report_class|
|
||||
begin
|
||||
report_class.call(date:, user: guardian.user, guardian:)
|
||||
rescue => e
|
||||
Rails.logger.error("Failed to generate report #{report_class.name}: #{e.message}")
|
||||
nil
|
||||
end
|
||||
end
|
||||
Discourse.redis.setex(key, CACHE_DURATION, MultiJson.dump(reports))
|
||||
reports
|
||||
end
|
||||
|
||||
# Access index from params data object (params.index) or hash (params[:index])
|
||||
index = (params[:index] || params.index).to_i
|
||||
|
||||
return false if index < 0 || index >= all_reports.length
|
||||
|
||||
all_reports[index]
|
||||
end
|
||||
|
||||
def fetch_report(report_class:)
|
||||
# Report is already generated and cached in the all_reports list
|
||||
report_class
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -16,9 +16,11 @@ module DiscourseRewind
|
||||
# @param [Hash] params
|
||||
# @option params [Integer] :year of the rewind
|
||||
# @option params [Integer] :username of the rewind
|
||||
# @option params [Integer] :count number of reports to fetch (optional, defaults to 3)
|
||||
# @return [Service::Base::Context]
|
||||
|
||||
CACHE_DURATION = Rails.env.development? ? 10.seconds : 5.minutes
|
||||
INITIAL_REPORT_COUNT = 3
|
||||
|
||||
# order matters
|
||||
REPORTS = [
|
||||
@ -42,7 +44,9 @@ module DiscourseRewind
|
||||
|
||||
model :year
|
||||
model :date
|
||||
model :enabled_reports
|
||||
model :reports
|
||||
model :total_available
|
||||
|
||||
private
|
||||
|
||||
@ -71,19 +75,38 @@ module DiscourseRewind
|
||||
Date.new(year).all_year
|
||||
end
|
||||
|
||||
def fetch_reports(date:, guardian:, year:)
|
||||
key = "rewind:#{guardian.user.username}:#{year}"
|
||||
reports = Discourse.redis.get(key)
|
||||
def fetch_enabled_reports(date:, guardian:, year:)
|
||||
# Generate all reports and filter out nils (disabled/empty reports)
|
||||
# Cache the full list to maintain consistent indices across requests
|
||||
key = "rewind:#{guardian.user.username}:#{year}:all_reports"
|
||||
cached_list = Discourse.redis.get(key)
|
||||
|
||||
if !reports
|
||||
reports =
|
||||
REPORTS.map { |report| report.call(date:, user: guardian.user, guardian:) }.compact
|
||||
Discourse.redis.setex(key, CACHE_DURATION, MultiJson.dump(reports))
|
||||
else
|
||||
reports = MultiJson.load(reports, symbolize_keys: true)
|
||||
end
|
||||
return MultiJson.load(cached_list, symbolize_keys: true) if cached_list
|
||||
|
||||
reports =
|
||||
REPORTS.filter_map do |report_class|
|
||||
begin
|
||||
report_class.call(date:, user: guardian.user, guardian:)
|
||||
rescue => e
|
||||
Rails.logger.error("Failed to generate report #{report_class.name}: #{e.message}")
|
||||
nil
|
||||
end
|
||||
end
|
||||
|
||||
# Cache the complete enabled reports list
|
||||
Discourse.redis.setex(key, CACHE_DURATION, MultiJson.dump(reports))
|
||||
reports
|
||||
end
|
||||
|
||||
def fetch_total_available(enabled_reports:)
|
||||
enabled_reports.length
|
||||
end
|
||||
|
||||
def fetch_reports(enabled_reports:, params:)
|
||||
count = params[:count]&.to_i || INITIAL_REPORT_COUNT
|
||||
count = [[count, 1].max, enabled_reports.length].min
|
||||
|
||||
enabled_reports.first(count)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@ -3,6 +3,7 @@ import { tracked } from "@glimmer/tracking";
|
||||
import { on } from "@ember/modifier";
|
||||
import { action } from "@ember/object";
|
||||
import didInsert from "@ember/render-modifiers/modifiers/did-insert";
|
||||
import willDestroy from "@ember/render-modifiers/modifiers/will-destroy";
|
||||
import DButton from "discourse/components/d-button";
|
||||
import concatClass from "discourse/helpers/concat-class";
|
||||
import { ajax } from "discourse/lib/ajax";
|
||||
@ -24,17 +25,44 @@ export default class Rewind extends Component {
|
||||
@tracked rewind = [];
|
||||
@tracked fullScreen = true;
|
||||
@tracked loadingRewind = false;
|
||||
@tracked totalAvailable = 0;
|
||||
@tracked loadingNextReport = false;
|
||||
|
||||
BUFFER_SIZE = 3;
|
||||
|
||||
// Arrow function for event listener - maintains 'this' binding
|
||||
handleScroll = () => {
|
||||
if (!this.scrollWrapper || this.loadingRewind) {
|
||||
return;
|
||||
}
|
||||
|
||||
const scrollTop = this.scrollWrapper.scrollTop;
|
||||
const scrollHeight = this.scrollWrapper.scrollHeight;
|
||||
const clientHeight = this.scrollWrapper.clientHeight;
|
||||
|
||||
// Trigger preload when scrolled 70% through content
|
||||
const scrollPercentage = (scrollTop + clientHeight) / scrollHeight;
|
||||
if (scrollPercentage > 0.7) {
|
||||
this.preloadNextReports();
|
||||
}
|
||||
};
|
||||
|
||||
@action
|
||||
registerScrollWrapper(element) {
|
||||
this.scrollWrapper = element;
|
||||
this.scrollWrapper.addEventListener("scroll", this.handleScroll);
|
||||
}
|
||||
|
||||
@action
|
||||
async loadRewind() {
|
||||
try {
|
||||
this.loadingRewind = true;
|
||||
this.rewind = await ajax("/rewinds");
|
||||
const response = await ajax("/rewinds");
|
||||
this.rewind = response.reports;
|
||||
this.totalAvailable = response.total_available;
|
||||
|
||||
// Preload next report to maintain buffer
|
||||
this.preloadNextReports();
|
||||
} catch (e) {
|
||||
popupAjaxError(e);
|
||||
} finally {
|
||||
@ -42,6 +70,38 @@ export default class Rewind extends Component {
|
||||
}
|
||||
}
|
||||
|
||||
@action
|
||||
async preloadNextReports() {
|
||||
// Load reports to maintain BUFFER_SIZE ahead of current position
|
||||
const currentIndex = this.rewind.length;
|
||||
const targetIndex = currentIndex + this.BUFFER_SIZE;
|
||||
|
||||
if (
|
||||
this.loadingNextReport ||
|
||||
currentIndex >= this.totalAvailable ||
|
||||
targetIndex > this.totalAvailable
|
||||
) {
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
this.loadingNextReport = true;
|
||||
const response = await ajax(`/rewinds/${currentIndex}`);
|
||||
this.rewind = [...this.rewind, response.report];
|
||||
|
||||
// Continue preloading if we haven't reached buffer size yet
|
||||
if (this.rewind.length < targetIndex) {
|
||||
this.preloadNextReports();
|
||||
}
|
||||
} catch (e) {
|
||||
// Silently fail for preloading - user already has content to view
|
||||
// eslint-disable-next-line no-console
|
||||
console.error("Failed to preload report:", e);
|
||||
} finally {
|
||||
this.loadingNextReport = false;
|
||||
}
|
||||
}
|
||||
|
||||
@action
|
||||
toggleFullScreen() {
|
||||
this.fullScreen = !this.fullScreen;
|
||||
@ -66,6 +126,13 @@ export default class Rewind extends Component {
|
||||
this.rewindContainer = element;
|
||||
}
|
||||
|
||||
@action
|
||||
cleanup() {
|
||||
if (this.scrollWrapper) {
|
||||
this.scrollWrapper.removeEventListener("scroll", this.handleScroll);
|
||||
}
|
||||
}
|
||||
|
||||
<template>
|
||||
<div
|
||||
class={{concatClass
|
||||
@ -73,6 +140,7 @@ export default class Rewind extends Component {
|
||||
(if this.fullScreen "-fullscreen")
|
||||
}}
|
||||
{{didInsert this.loadRewind}}
|
||||
{{willDestroy this.cleanup}}
|
||||
{{on "keydown" this.handleEscape}}
|
||||
{{on "click" this.handleBackdropClick}}
|
||||
{{didInsert this.registerRewindContainer}}
|
||||
|
||||
@ -4,3 +4,4 @@ en:
|
||||
discourse_rewind:
|
||||
report_failed: "Failed to generate Discourse Rewind report."
|
||||
invalid_year: "Rewind can only be generated in January or December."
|
||||
invalid_report_index: "Invalid report index."
|
||||
|
||||
@ -1,5 +1,8 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
DiscourseRewind::Engine.routes.draw { get "/rewinds" => "rewinds#show" }
|
||||
DiscourseRewind::Engine.routes.draw do
|
||||
get "/rewinds" => "rewinds#index", :constraints => { format: :json }
|
||||
get "/rewinds/:index" => "rewinds#show", :constraints => { index: /\d+/ }
|
||||
end
|
||||
|
||||
Discourse::Application.routes.draw { mount ::DiscourseRewind::Engine, at: "/" }
|
||||
|
||||
@ -3,11 +3,11 @@
|
||||
RSpec.describe DiscourseRewind::RewindsController do
|
||||
before { SiteSetting.discourse_rewind_enabled = true }
|
||||
|
||||
describe "#show" do
|
||||
fab!(:current_user, :user)
|
||||
fab!(:current_user, :user)
|
||||
|
||||
before { sign_in(current_user) }
|
||||
before { sign_in(current_user) }
|
||||
|
||||
describe "#index" do
|
||||
context "when out of valid month" do
|
||||
before { freeze_time DateTime.parse("2022-11-24") }
|
||||
|
||||
@ -22,26 +22,104 @@ RSpec.describe DiscourseRewind::RewindsController do
|
||||
context "when in valid month" do
|
||||
before { freeze_time DateTime.parse("2022-12-24") }
|
||||
|
||||
it "returns 200" do
|
||||
it "returns 200 with reports and total_available" do
|
||||
get "/rewinds.json"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["reports"]).to be_present
|
||||
expect(response.parsed_body["total_available"]).to be_present
|
||||
expect(response.parsed_body["reports"].length).to eq(3)
|
||||
end
|
||||
|
||||
context "when reports are not found or error" do
|
||||
it "limits initial reports to 3 by default" do
|
||||
get "/rewinds.json"
|
||||
|
||||
expect(response.parsed_body["reports"].length).to eq(3)
|
||||
expect(response.parsed_body["total_available"]).to be > 3
|
||||
end
|
||||
|
||||
context "when a report errors" do
|
||||
before do
|
||||
DiscourseRewind::Action::TopWords.stubs(:call).raises(StandardError.new("Some error"))
|
||||
end
|
||||
|
||||
it "returns 404 with message" do
|
||||
it "filters out failed reports and returns remaining reports" do
|
||||
get "/rewinds.json"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
expect(response.parsed_body["errors"].first).to eq(
|
||||
I18n.t("discourse_rewind.report_failed"),
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["reports"]).to be_present
|
||||
expect(response.parsed_body["reports"].map { |r| r["identifier"] }).not_to include(
|
||||
"top-words",
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "#show" do
|
||||
context "when out of valid month" do
|
||||
before { freeze_time DateTime.parse("2022-11-24") }
|
||||
|
||||
it "returns 404" do
|
||||
get "/rewinds/0.json"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
expect(response.parsed_body["errors"].first).to eq(I18n.t("discourse_rewind.invalid_year"))
|
||||
end
|
||||
end
|
||||
|
||||
context "when in valid month" do
|
||||
before { freeze_time DateTime.parse("2022-12-24") }
|
||||
|
||||
it "returns 200 with a single report" do
|
||||
get "/rewinds/0.json"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["report"]).to be_present
|
||||
expect(response.parsed_body["report"]["identifier"]).to be_present
|
||||
end
|
||||
|
||||
it "returns different reports for different indices" do
|
||||
# Get all reports first to know what to expect
|
||||
get "/rewinds.json"
|
||||
total = response.parsed_body["total_available"]
|
||||
|
||||
# Request second report if there is one
|
||||
if total > 1
|
||||
get "/rewinds/1.json"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["report"]["identifier"]).to be_present
|
||||
end
|
||||
end
|
||||
|
||||
context "when index is out of bounds" do
|
||||
it "returns 404" do
|
||||
# Request index that exceeds the REPORTS array length
|
||||
# There are 16 items in REPORTS array, so 999 should definitely be out of bounds
|
||||
get "/rewinds/999", params: {}, as: :json
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
expect(response.parsed_body["errors"].first).to eq(
|
||||
I18n.t("discourse_rewind.invalid_report_index"),
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
context "when report fails to generate" do
|
||||
before do
|
||||
DiscourseRewind::Action::TopWords.stubs(:call).raises(StandardError.new("Some error"))
|
||||
end
|
||||
|
||||
it "filters out failed report and adjusts indices" do
|
||||
# TopWords is filtered out, index 0 points to first successful report
|
||||
get "/rewinds/0.json"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["report"]["identifier"]).to be_present
|
||||
expect(response.parsed_body["report"]["identifier"]).not_to eq("top-words")
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user