Checkmate icon indicating copy to clipboard operation
Checkmate copied to clipboard

Feat: Pagespeed Graph Pagination Backend

Open mospell opened this issue 2 months ago • 8 comments

Describe your changes

This PR adds pagination support for the PageSpeed chart on the backend by introducing an optional variable to FetchStatsByMonitorId/GetMonitorStatsById called page, which shifts back the date range depending on it's value.

For example: If dateRange is set to "day" and page is 4, then it will query data from 4 days ago.

The returned monitor object now also includes the number of remaining checks to be used for the frontend implementation.

Write your issue number after "Fixes "

Fixes #606

Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.

  • [x] (Do not skip this or your PR will be closed) I deployed the application locally.
  • [x] (Do not skip this or your PR will be closed) I have performed a self-review and testing of my code.
  • [x] I have included the issue # in the PR.
  • [ ] I have added i18n support to visible strings (instead of <div>Add</div>, use):
const { t } = useTranslation();
<div>{t('add')}</div>
  • [x] I have not included any files that are not related to my pull request, including package-lock and package-json if dependencies have not changed
  • [ ] I didn't use any hardcoded values (otherwise it will not scale, and will make it difficult to maintain consistency across the application).
  • [ ] I made sure font sizes, color choices etc are all referenced from the theme. I don't have any hardcoded dimensions.
  • [x] My PR is granular and targeted to one specific feature.
  • [x] I ran npm run format in server and client directories, which automatically formats your code.
  • [ ] I took a screenshot or a video and attached to this PR if there is a UI change.

Summary by CodeRabbit

  • New Features

    • Added page-based pagination for monitor statistics so users can navigate older/newer data when reviewing monitor performance.
    • Monitor statistics now include a "remaining checks" count showing how many checks exist beyond the current page.
    • Paging is preserved across requests and respects selected date ranges, improving navigation through historical data.
  • Validation

    • Query accepts a non-negative integer page parameter to control paging.

mospell avatar Oct 29 '25 09:10 mospell

[!NOTE]

.coderabbit.yml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'release_notes'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Adds a new numeric page parameter threaded from client hook through NetworkService, controller, service, and DB. DB gets offsetDatesByPage to compute paged date ranges, applies paging-aware filtering (including checksSkipped), and monitorStats now includes remainingChecks.

Changes

Cohort / File(s) Summary
Client: hook & network
client/src/Hooks/v1/monitorHooks.js, client/src/Utils/NetworkService.js
useFetchStatsByMonitorId signature adds page; hook passes page to networkService.getStatsByMonitorId; NetworkService appends page to URLSearchParams.
API layer: controller & validation
server/src/controllers/v1/monitorController.js, server/src/validation/joi.js
Controller extracts page from query and forwards it to service; Joi validation adds page: joi.number().integer().min(0).
Business & DB logic
server/src/service/v1/business/monitorService.js, server/src/db/v1/modules/monitorModule.js
getMonitorStatsById signatures extended to accept page and forward it; new offsetDatesByPage(dates, dateRange, page) added; DB applies offset dates when page provided, adjusts date filtering (including checksSkipped) and returns remainingChecks in monitorStats.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Hook as useFetchStatsByMonitorId
    participant Network as NetworkService
    participant Controller as monitorController
    participant Service as monitorService
    participant DB as monitorModule

    Client->>Hook: set/use page (N)
    Hook->>Network: getStatsByMonitorId(monitorId,..., page=N)
    Network->>Controller: GET /monitors/{id}/stats?page=N
    Controller->>Service: getMonitorStatsById(..., page=N)
    Service->>DB: getMonitorStatsById(..., page=N)

    rect rgb(235,245,255)
    Note over DB: Pagination-aware date computation
    DB->>DB: getDateRange(dateRange)
    DB->>DB: offsetDatesByPage(dates, dateRange, page)
    DB->>DB: filter checksForDateRange and checksSkipped
    DB->>DB: compute remainingChecks
    end

    DB-->>Service: monitorStats + remainingChecks
    Service-->>Controller: monitorStats + remainingChecks
    Controller-->>Network: response
    Network-->>Hook: data
    Hook-->>Client: paginated stats

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review offsetDatesByPage() for correctness across dateRange branches (recent/day/week/month/all).
  • Verify checksSkipped and remainingChecks calculations and boundary conditions.
  • Confirm page is validated and correctly threaded controller → service → DB and that behavior is unchanged when page is absent.

Poem

🐰 I hop through pages, one by one,
Offsetting days in morning sun.
Skipped and shown, the counts align,
Chunks arrive—smooth scroll, just fine.
📊✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Feat: Pagespeed Graph Pagination Backend' clearly and concisely describes the main change: adding backend pagination support for pagespeed graph.
Description check ✅ Passed The PR description includes a clear explanation of changes, covers the new pageOffset parameter, mentions the remaining checks addition, and includes the issue number #606 with several checklist items completed.
Linked Issues check ✅ Passed The changes implement pagination for PageSpeed checks via a pageOffset parameter that shifts date ranges, allowing paginated data fetching as required by issue #606, though preloading logic appears frontend-focused.
Out of Scope Changes check ✅ Passed All changes are narrowly focused on adding pagination support to monitor stats retrieval across client/server layers; no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec93b60ec8327037678e349d0836661d4e711a83 and 9c5758d2299deac5e6bacb9180f6311f63c63b7b.

📒 Files selected for processing (6)
  • client/src/Hooks/v1/monitorHooks.js (3 hunks)
  • client/src/Utils/NetworkService.js (1 hunks)
  • server/src/controllers/v1/monitorController.js (2 hunks)
  • server/src/db/v1/modules/monitorModule.js (4 hunks)
  • server/src/service/v1/business/monitorService.js (2 hunks)
  • server/src/validation/joi.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • server/src/service/v1/business/monitorService.js
  • client/src/Utils/NetworkService.js
  • server/src/db/v1/modules/monitorModule.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T17:52:55.506Z
Learnt from: Jesulayomy
Repo: bluewave-labs/Checkmate PR: 2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.

Applied to files:

  • server/src/controllers/v1/monitorController.js
  • client/src/Hooks/v1/monitorHooks.js
🧬 Code graph analysis (1)
server/src/controllers/v1/monitorController.js (1)
server/src/db/v1/modules/monitorModuleQueries.js (1)
  • req (1079-1079)
🔇 Additional comments (6)
server/src/validation/joi.js (1)

147-147: LGTM! Validation correctly implemented.

The page parameter validation properly constrains the value to non-negative integers and addresses previous feedback. Making it optional ensures backward compatibility.

server/src/controllers/v1/monitorController.js (2)

85-85: LGTM! Parameter extraction follows established patterns.

The page parameter is correctly extracted from the validated query object alongside other parameters.


101-101: LGTM! Parameter correctly threaded to service layer.

The page parameter is properly passed through to the monitor service for processing.

client/src/Hooks/v1/monitorHooks.js (3)

173-173: LGTM! Parameter correctly added to hook signature.

The page parameter is properly included in the hook's destructured parameters, following the established pattern.


191-191: LGTM! Parameter correctly passed to network service.

The page parameter is properly threaded through to the network service call for fetching paginated statistics.


203-212: LGTM! Dependency array correctly updated.

The page parameter is properly included in the dependency array to trigger refetching when pagination changes. The reformatting to one dependency per line improves readability and makes future diffs clearer.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 29 '25 09:10 coderabbitai[bot]

Hi @ajhollid,

pageOffset would basically increment by 1 until it runs out of remainingChecks blocking you from scrolling further when there is no more data. I probably should've just named it page instead now that I think about it.

The backend should take two variables, the page and rowsPerPage, and multiplying the two together will yield the page offset.

My initial idea was time-based pagination, as querying an x amount of checks doesn't really make sense on a graph. How would rowsPerPage apply here? Could you give an example?

mospell avatar Nov 04 '25 10:11 mospell

@mospell you have a CI/CD check issue here.

gorkem-bwl avatar Nov 10 '25 18:11 gorkem-bwl

Hi @ajhollid, just pinging you again

The way it currently works is that there is no pageOffset calculation in the frontend. All offset calculations happen in the backend as you requested. To clarify this I have renamed the pageOffset variable to just page.

I believe it would make more sense to have the user scroll through previous dates instead of displaying a number of rows per page. I think the way it currently works should satisfy your requirements. If you'd like me to change it anyway, please let me know.

mospell avatar Nov 12 '25 14:11 mospell

@gorkem-bwl this should be resolved now, thanks for the notice!

mospell avatar Nov 12 '25 14:11 mospell

@mospell perhaps I'm misunderstanding somethig here, pagination of results should depend on a page and a rowsPerPage input from the front end should it not?

ie on the frontend, the user selects "5 rows per page" and then paginates thorugh results, starting from page 0.

Offset is then calcualted by multiplying rowsPerPage * page on the backend.

Example:

let skip = 0;
if (page && rowsPerPage) {
	skip = page * rowsPerPage;
}

const checks = await this.Check.aggregate([
	{ $match: matchStage },
	{ $sort: { createdAt: sortOrder } },
	{
		$facet: {
			summary: [{ $count: "checksCount" }],
			checks: [{ $skip: skip }, { $limit: rowsPerPage }],
		},
	},
	{
		$project: {
			checksCount: {
				$ifNull: [{ $arrayElemAt: ["$summary.checksCount", 0] }, 0],
			},
			checks: {
				$ifNull: ["$checks", []],
			},
		},
	},
]);

ajhollid avatar Nov 12 '25 17:11 ajhollid

@ajhollid Usually yes, but I decided to go for time-based pagination here. In this case dateRange would kind of act as rowsPerPage here.

On the frontend the user would pick a date range and then paginate through results that way, starting from page 0 (today).

Instead of calculating an offset based on rowsPerPage and page, the backend shifts back the selected dateRange by the page number.

So for example: dateRange:"day" and page:5 - returns data from 5 days ago dateRange:"week" and page:2 - returns data from 2 weeks ago. and so on, until there are no more checks.

This is done by the offsetDatesByPage helper function in monitorService.js, feel free to take a look at it if this reply still ends up being a bit confusing.

mospell avatar Nov 12 '25 19:11 mospell

@ajhollid This implementation was partially inspired by other projects like Thanos, where you can select the exact date range to display data on a time based chart. Pagination is generally used for tables or text, and while it can be done for charts it's a little unintuitive from the perspective of a user.

I think that "displaying data from (date) to (date)" is a lot more readable to the average person than "displaying x checks" on a time based chart. It's also way more consistent because it wouldn't be affected by, for example, pausing the monitor or editing the check interval, both of which could create gaps between checks.

Please reconsider this request. I'd be happy to make any other tweaks to the way it's currently implemented, but if you'd still like me to add regular pagination I can do that too.

mospell avatar Nov 28 '25 15:11 mospell