From f6d9b7c3ef2d6960e1e178d6e23c846ebf39073b Mon Sep 17 00:00:00 2001 From: Christian Fehmer Date: Tue, 8 Apr 2025 13:46:40 +0200 Subject: [PATCH] impr: lazy load chartData on results (@fehmer) (#6428) Optimize results endpoint by removing heavy or unused data. We load the whole result chart data for up to 1000 results each time, but it is very unlikely the user will view the charts for all old results. By removing the size in my tests went down from 1152kb to 276kb. --------- Co-authored-by: Miodec --- .../__tests__/api/controllers/result.spec.ts | 81 +++++++++++++++++++ backend/src/api/controllers/result.ts | 16 +++- backend/src/api/routes/results.ts | 3 + backend/src/dal/result.ts | 24 ++++-- frontend/src/styles/account.scss | 9 ++- frontend/src/ts/pages/account.ts | 72 ++++++++++++++--- packages/contracts/src/rate-limit/index.ts | 12 +++ packages/contracts/src/results.ts | 33 +++++++- packages/contracts/src/schemas/results.ts | 8 ++ 9 files changed, 236 insertions(+), 22 deletions(-) diff --git a/backend/__tests__/api/controllers/result.spec.ts b/backend/__tests__/api/controllers/result.spec.ts index 7e76109b4..be710f8a1 100644 --- a/backend/__tests__/api/controllers/result.spec.ts +++ b/backend/__tests__/api/controllers/result.spec.ts @@ -333,6 +333,87 @@ describe("result controller test", () => { ).toBeRateLimited({ max: 30, windowMs: 24 * 60 * 60 * 1000 }); }); }); + describe("getResultById", () => { + const getResultMock = vi.spyOn(ResultDal, "getResult"); + + afterEach(() => { + getResultMock.mockReset(); + }); + + it("should get result", async () => { + //GIVEN + const result = givenDbResult(uid); + getResultMock.mockResolvedValue(result); + + //WHEN + const { body } = await mockApp + .get(`/results/id/${result._id}`) + .set("Authorization", `Bearer ${uid}`) + .send() + .expect(200); + + //THEN + expect(body.message).toEqual("Result retrieved"); + expect(body.data).toEqual({ ...result, _id: result._id.toHexString() }); + }); + it("should get last result with ape key", async () => { + //GIVEN + await acceptApeKeys(true); + const apeKey = await mockAuthenticateWithApeKey(uid, await configuration); + const result = givenDbResult(uid); + getResultMock.mockResolvedValue(result); + + //WHEN + await mockApp + .get(`/results/id/${result._id}`) + .set("Authorization", `ApeKey ${apeKey}`) + .send() + .expect(200); + }); + it("should get last result with legacy values", async () => { + //GIVEN + const result = givenDbResult(uid, { + charStats: undefined, + incorrectChars: 5, + correctChars: 12, + }); + getResultMock.mockResolvedValue(result); + + //WHEN + const { body } = await mockApp + .get(`/results/id/${result._id}`) + .set("Authorization", `Bearer ${uid}`) + .send() + .expect(200); + + //THEN + expect(body.message).toEqual("Result retrieved"); + expect(body.data).toMatchObject({ + _id: result._id.toHexString(), + charStats: [12, 5, 0, 0], + }); + expect(body.data).not.toHaveProperty("correctChars"); + expect(body.data).not.toHaveProperty("incorrectChars"); + }); + it("should rate limit get result with ape key", async () => { + //GIVEN + const result = givenDbResult(uid, { + charStats: undefined, + incorrectChars: 5, + correctChars: 12, + }); + getResultMock.mockResolvedValue(result); + await acceptApeKeys(true); + const apeKey = await mockAuthenticateWithApeKey(uid, await configuration); + + //WHEN + await expect( + mockApp + .get(`/results/id/${result._id}`) + .set("Authorization", `ApeKey ${apeKey}`) + ).toBeRateLimited({ max: 60, windowMs: 60 * 60 * 1000 }); + }); + }); describe("getLastResult", () => { const getLastResultMock = vi.spyOn(ResultDal, "getLastResult"); diff --git a/backend/src/api/controllers/result.ts b/backend/src/api/controllers/result.ts index 881e79207..ce0c47bce 100644 --- a/backend/src/api/controllers/result.ts +++ b/backend/src/api/controllers/result.ts @@ -37,6 +37,8 @@ import { AddResultRequest, AddResultResponse, GetLastResultResponse, + GetResultByIdPath, + GetResultByIdResponse, GetResultsQuery, GetResultsResponse, UpdateResultTagsRequest, @@ -131,12 +133,22 @@ export async function getResults( return new MonkeyResponse("Results retrieved", results.map(convertResult)); } +export async function getResultById( + req: MonkeyRequest +): Promise { + const { uid } = req.ctx.decodedToken; + const { resultId } = req.params; + + const result = await ResultDAL.getResult(uid, resultId); + return new MonkeyResponse("Result retrieved", convertResult(result)); +} + export async function getLastResult( req: MonkeyRequest ): Promise { const { uid } = req.ctx.decodedToken; - const results = await ResultDAL.getLastResult(uid); - return new MonkeyResponse("Result retrieved", convertResult(results)); + const result = await ResultDAL.getLastResult(uid); + return new MonkeyResponse("Result retrieved", convertResult(result)); } export async function deleteAll(req: MonkeyRequest): Promise { diff --git a/backend/src/api/routes/results.ts b/backend/src/api/routes/results.ts index bc3d6772a..179aa7fc7 100644 --- a/backend/src/api/routes/results.ts +++ b/backend/src/api/routes/results.ts @@ -8,6 +8,9 @@ export default s.router(resultsContract, { get: { handler: async (r) => callController(ResultController.getResults)(r), }, + getById: { + handler: async (r) => callController(ResultController.getResultById)(r), + }, add: { handler: async (r) => callController(ResultController.addResult)(r), }, diff --git a/backend/src/dal/result.ts b/backend/src/dal/result.ts index 797829e38..bcf2e7236 100644 --- a/backend/src/dal/result.ts +++ b/backend/src/dal/result.ts @@ -100,13 +100,23 @@ export async function getResults( ): Promise { const { onOrAfterTimestamp, offset, limit } = opts ?? {}; let query = getResultCollection() - .find({ - uid, - ...(!_.isNil(onOrAfterTimestamp) && - !_.isNaN(onOrAfterTimestamp) && { - timestamp: { $gte: onOrAfterTimestamp }, - }), - }) + .find( + { + uid, + ...(!_.isNil(onOrAfterTimestamp) && + !_.isNaN(onOrAfterTimestamp) && { + timestamp: { $gte: onOrAfterTimestamp }, + }), + }, + { + projection: { + chartData: 0, + keySpacingStats: 0, + keyDurationStats: 0, + name: 0, + }, + } + ) .sort({ timestamp: -1 }); if (limit !== undefined) { diff --git a/frontend/src/styles/account.scss b/frontend/src/styles/account.scss index 77892551c..ea6ffedd0 100644 --- a/frontend/src/styles/account.scss +++ b/frontend/src/styles/account.scss @@ -330,12 +330,19 @@ margin: 0 0.1rem; } .miniResultChartButton { - opacity: 0.25; transition: 0.25s; cursor: pointer; + color: var(--text-color); &:hover { opacity: 1; } + &.loading { + pointer-events: none; + } + &.disabled .fas { + opacity: 0.5; + color: var(--sub-color); + } } } diff --git a/frontend/src/ts/pages/account.ts b/frontend/src/ts/pages/account.ts index 101509483..dc2a38bca 100644 --- a/frontend/src/ts/pages/account.ts +++ b/frontend/src/ts/pages/account.ts @@ -38,6 +38,7 @@ import { ResultFiltersGroupItem } from "@monkeytype/contracts/schemas/users"; import { findLineByLeastSquares } from "../utils/numbers"; import defaultResultFilters from "../constants/default-result-filters"; import { SnapshotResult } from "../constants/default-snapshot"; +import Ape from "../ape"; let filterDebug = false; //toggle filterdebug @@ -105,12 +106,10 @@ function loadMoreLines(lineIndex?: number): void { )}" data-balloon-pos="up">`; } - if (result.chartData === undefined) { - icons += ``; - } else if (result.chartData === "toolong") { - icons += ``; + if (result.chartData === "toolong" || result.testDuration > 122) { + icons += ``; } else { - icons += ``; + icons += ``; } let tagNames = "no tags"; @@ -1152,13 +1151,64 @@ $(".pageAccount #accountHistoryChart").on("click", () => { $(`#result-${index}`).addClass("active"); }); -$(".pageAccount").on("click", ".miniResultChartButton", (event) => { - console.log("updating"); - const filteredId = $(event.currentTarget).attr("filteredResultsId"); +$(".pageAccount").on("click", ".miniResultChartButton", async (event) => { + const target = $(event.currentTarget); + if (target.hasClass("loading")) return; + if (target.hasClass("disabled")) return; + + const filteredId = target.attr("filteredResultsId"); if (filteredId === undefined) return; - MiniResultChartModal.show( - filteredResults[parseInt(filteredId)]?.chartData as ChartData - ); + + const result = filteredResults[parseInt(filteredId)]; + if (result === undefined) return; + + let chartData = result.chartData as ChartData; + + if (chartData === undefined) { + //need to load full result + target.addClass("loading"); + target.attr("aria-label", null); + target.html(''); + Loader.show(); + + const response = await Ape.results.getById({ + params: { resultId: result._id }, + }); + Loader.hide(); + + target.html(''); + target.removeClass("loading"); + + if (response.status !== 200) { + Notifications.add("Error fetching result: " + response.body.message, -1); + return; + } + + chartData = response.body.data.chartData as ChartData; + + //update local cache + result.chartData = chartData; + const dbResult = DB.getSnapshot()?.results?.find( + (it) => it._id === result._id + ); + if (dbResult !== undefined) { + dbResult["chartData"] = result.chartData; + } + + if (response.body.data.chartData === "toolong") { + target.attr( + "aria-label", + "Graph history is not available for long tests" + ); + target.attr("data-baloon-pos", "up"); + target.addClass("disabled"); + + Notifications.add("Graph history is not available for long tests", 0); + return; + } + } + target.attr("aria-label", "View graph"); + MiniResultChartModal.show(chartData); }); $(".pageAccount .group.history").on("click", ".history-wpm-header", () => { diff --git a/packages/contracts/src/rate-limit/index.ts b/packages/contracts/src/rate-limit/index.ts index 927aab9f9..205e3115c 100644 --- a/packages/contracts/src/rate-limit/index.ts +++ b/packages/contracts/src/rate-limit/index.ts @@ -138,6 +138,18 @@ export const limits = { max: 30, }, + // Result by id + resultByIdGet: { + window: "hour", + max: 300, + }, + + // Result by id + resultByIdGetApe: { + window: "hour", + max: 60, + }, + resultsAdd: { window: "hour", max: 300, diff --git a/packages/contracts/src/results.ts b/packages/contracts/src/results.ts index 2469ed4d7..da18c42c6 100644 --- a/packages/contracts/src/results.ts +++ b/packages/contracts/src/results.ts @@ -9,6 +9,7 @@ import { import { CompletedEventSchema, PostResultResponseSchema, + ResultMinifiedSchema, ResultSchema, } from "./schemas/results"; import { IdSchema } from "./schemas/util"; @@ -38,9 +39,20 @@ export const GetResultsQuerySchema = z.object({ }); export type GetResultsQuery = z.infer; -export const GetResultsResponseSchema = responseWithData(z.array(ResultSchema)); +export const GetResultsResponseSchema = responseWithData( + z.array(ResultMinifiedSchema) +); export type GetResultsResponse = z.infer; +export const GetResultByIdPathSchema = z.object({ + resultId: IdSchema, +}); +export type GetResultByIdPath = z.infer; + +export const GetResultByIdResponseSchema = responseWithData(ResultSchema); + +export type GetResultByIdResponse = z.infer; + export const AddResultRequestSchema = z.object({ result: CompletedEventSchema, }); @@ -92,6 +104,25 @@ export const resultsContract = c.router( }, }), }, + getById: { + summary: "get result by id", + description: "Get result by id", + method: "GET", + path: "/id/:resultId", + pathParams: GetResultByIdPathSchema, + responses: { + 200: GetResultByIdResponseSchema, + }, + metadata: meta({ + authenticationOptions: { + acceptApeKeys: true, + }, + rateLimit: { + normal: "resultByIdGet", + apeKey: "resultByIdGetApe", + }, + }), + }, add: { summary: "add result", description: "Add a test result for the current user", diff --git a/packages/contracts/src/schemas/results.ts b/packages/contracts/src/schemas/results.ts index 205924d34..f7ec9f68b 100644 --- a/packages/contracts/src/schemas/results.ts +++ b/packages/contracts/src/schemas/results.ts @@ -95,6 +95,14 @@ export type Result = Omit< mode2: Mode2; }; +export const ResultMinifiedSchema = ResultSchema.omit({ + name: true, + keySpacingStats: true, + keyDurationStats: true, + chartData: true, +}); +export type ResultMinified = z.infer; + export const CompletedEventSchema = ResultBaseSchema.required({ restartCount: true, incompleteTestSeconds: true,