feat: Update useSerialLogs to accept URL params#5674
Conversation
ca3dc87 to
7d5de49
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the serial logs fetch flow so consumers can request /serial-log with optional filtering/pagination query params, wiring the new params through the Flask endpoint and the React useSerialLogs hook.
Changes:
- Add support for
start-time,end-time, andpage-sizequery params on the serial log API endpoint. - Update
useSerialLogsto accept optional URL params and include them in the request URL / query cache key. - Add/extend Python and TS tests around the new parameters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
webapp/endpoints/models.py |
Reads start-time/end-time/page-size from the request and forwards them to the Store API call. |
tests/endpoints/tests_models.py |
Adds new endpoint tests for serial-log requests that include query params. |
static/js/publisher/hooks/useSerialLogs.ts |
Extends the hook signature to accept optional URL params and builds a URL with search params. |
static/js/publisher/hooks/__tests__/useSerialLogs.test.tsx |
Adds hook tests for the new optional params. |
0fc2cc3 to
0251a4e
Compare
There was a problem hiding this comment.
Pull request overview
Updates the serial log fetching flow to support pagination and time-range filtering by accepting URL query parameters in both the Flask endpoint and the React useSerialLogs hook.
Changes:
- Extend
/api/store/<id>/models/<model>/serial-logto forwardstart-time,end-time,page-size, andnext-pagequery params to the Store API client. - Update
useSerialLogsto construct a URL with optional query params and include them in the React Query cache key. - Add backend and frontend tests covering the new parameters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| webapp/endpoints/models.py | Reads new query params and forwards them to PublisherGW.get_store_model_serial_logs. |
| tests/endpoints/tests_models.py | Adds endpoint tests verifying query params are passed through to the gateway call. |
| static/js/publisher/hooks/useSerialLogs.ts | Adds optional URL params support and uses a constructed URL for fetch. |
| static/js/publisher/hooks/tests/useSerialLogs.test.tsx | Adds MSW-based tests asserting the hook sends the correct query params. |
| # Arguments are passed positionally: | ||
| # (session, store_id, model_name, start_time, end_time, page_size) |
| mock_get_serial_log.assert_called_once() | ||
| # Arguments are passed positionally: | ||
| # session, store_id, model_name, start_time, | ||
| # end_time, page_size, cursor | ||
| self.assertEqual( | ||
| mock_get_serial_log.call_args.args[6], | ||
| "nextpage", | ||
| ) |
There was a problem hiding this comment.
bad suggestion IMO, I feel like this would make the test more brittle and difficult to read
| start_time = flask.request.args.get("start-time") | ||
| end_time = flask.request.args.get("end-time") | ||
| page_size = flask.request.args.get("page-size") | ||
| cursor = flask.request.args.get("next-page") |
There was a problem hiding this comment.
Any reason why we rename cursor to next-page param?
There was a problem hiding this comment.
@bartaz This is for clarity for the frontend as cursor isn't necessarily that obvious, although that is what it's called in the store API
| if (urlSearchParams) { | ||
| const { startTime, endTime, pageSize, nextPage } = urlSearchParams; | ||
|
|
||
| if (startTime && endTime) { |
There was a problem hiding this comment.
does the API only accept calls where both timestamps are present? in that case the urlSearchParams type could state this clearly
{
pageSize?: number;
nextPage?: string;
} | {
startTime: string;
endTime: string;
}
There was a problem hiding this comment.
@edisile there's some nuance around it, but essentially yes. I'll update so it's clearer.
There was a problem hiding this comment.
Yes — the hook now uses an interval object ({ startTime, endTime }) for timestamp filtering, so the type enforces both values together. This is in 972b8e82.
0251a4e to
972b8e8
Compare
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Done
Updates the
useSerialLogshook so it accepts parameters for start time, end time and page size which will be passed to the backend API callHow to QA
Testing
Security
Issue / Card
Fixes https://warthogs.atlassian.net/browse/WD-36148
Screenshots
n/a
UX Approval