add page for running queries #73
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update enhances the query handling functionality across multiple components in the project. It introduces dynamic client-side fetching and display of query data, state management for query status updates, and streamlined data handling mechanisms. A new function for fetching queries by UUID from the Supabase database is also included, significantly improving data retrieval capabilities while refining component interactions for better maintainability and clarity. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
can you show how it looks by pasting a screenshot of it? |
|
…ing instead of websocket
eriktaubeneck
force-pushed
the
running-queries
branch
from
e875265 to
801548f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- server/app/header.tsx (1 hunks)
- server/app/page.tsx (1 hunks)
- server/app/query/page.tsx (1 hunks)
- server/app/query/servers.tsx (6 hunks)
- server/app/query/view/[id]/components.tsx (3 hunks)
- server/app/query/view/[id]/page.tsx (8 hunks)
- server/data/query.ts (1 hunks)
- sidecar/app/query/ipa.py (1 hunks)
- sidecar/app/routes/start.py (2 hunks)
- sidecar/tests/app/routes/test_start.py (2 hunks)
Additional context used
Biome
server/app/query/page.tsx
[error] 36-36: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
Additional comments not posted (33)
server/data/query.ts (1)
58-78: LGTM!The function implementation looks good and follows best practices for querying the database and processing the data.
server/app/page.tsx (1)
61-61: LGTM!The link text change from "Dashboard" to "Queries" based on the user's login status is correct and improves clarity.
server/app/query/view/[id]/components.tsx (1)
Line range hint
67-90:
LGTM!The refactoring of the
RunTimePillcomponent to accept a singlestatusEventobject simplifies the component's interface and enhances maintainability.sidecar/app/routes/start.py (3)
28-29: LGTM!The endpoint path change from
/capacity_availableto/capacity-availableis correct and follows URL naming conventions.
36-37: LGTM!The endpoint path change from
/running_queriesto/running-queriesis correct and follows URL naming conventions.
112-118: LGTM!The function renaming and return statement modification are correct and improve clarity and consistency.
sidecar/tests/app/routes/test_start.py (11)
37-39: LGTM!The test function correctly verifies the
/start/capacity-availableendpoint.
44-46: LGTM!The test function correctly verifies the
/start/capacity-availableendpoint when a query is running.
50-52: LGTM!The test function correctly verifies the
/start/running-queriesendpoint.
Line range hint
57-70:
LGTM!The test function correctly verifies the
/start/ipa-helper/{query_id}endpoint.
Line range hint
75-86:
LGTM!The test function correctly verifies that an
IncorrectRoleErroris raised for the/start/ipa-helper/{query_id}endpoint with a coordinator role.
Line range hint
91-104:
LGTM!The test function correctly verifies the
/start/ipa-query/{query_id}endpoint.
Line range hint
109-120:
LGTM!The test function correctly verifies that an
IncorrectRoleErroris raised for the/start/ipa-query/{query_id}endpoint with a helper role.
133-136: LGTM!The test function correctly verifies the
/start/{query_id}/statusendpoint when the query is not found.
139-145: LGTM!The test function correctly verifies the
/start/{running_query.query_id}/statusendpoint when the query is running.
148-155: LGTM!The test function correctly verifies the
/start/{running_query.query_id}/statusendpoint when the query is complete.
Line range hint
160-167:
LGTM!The test function correctly verifies the
/start/{query_id}/log-fileendpoint.server/app/query/page.tsx (3)
61-72: LGTM!The
useEffecthook correctly polls running queries every second.
74-94: LGTM!The
useEffecthook correctly updates the state with query data and status events based on query IDs.
95-190: LGTM!The rendering logic correctly displays the query information based on the state.
server/app/header.tsx (1)
10-10: LGTM!The navigation item change from "Dashboard" to "Queries" is correct and aligns with the new functionality.
server/app/query/servers.tsx (5)
38-42: LGTM!The
StatusEventinterface correctly defines the structure of a status event.
44-49: LGTM!The
buildStatusEventFromJSONfunction correctly builds aStatusEventobject from JSON data.
69-71: LGTM!The
StatusEventByRemoteServertype correctly defines the structure of status events by remote server.
83-86: LGTM!The
initialStatusEventByRemoteServerconstant correctly initializes the status events by remote server.
126-130: LGTM!The
queryStatusmethod correctly fetches the query status.server/app/query/view/[id]/page.tsx (6)
12-12: New imports look good!The new imports
StatusEventandStatusEventByRemoteServerare necessary for the updated logic.Also applies to: 22-27
54-55: New state variables look good!The state variables
statusEventByRemoteServerandinitialStatusEventByRemoteServerare correctly initialized.
57-65: New functionupdateStatusEventlooks good!The function correctly updates the
statusEventByRemoteServerstate.
123-123: Function call toupdateStatusEventlooks good!The function call is correctly implemented in
openStatusSocket.
227-231: Updates toRunTimePillcomponent look good!The component is correctly updated to use
statusEvent.Also applies to: 242-242
276-280: Updates toStatusPillcomponent look good!The component is correctly updated to use
statusEvent.Also applies to: 291-291
sidecar/app/query/ipa.py (1)
254-254: URL path modification looks good!The URL path construction is correctly modified from
/start/ipa-helper/{self.query_id}/statusto/start/{self.query_id}/status.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/app/query/page.tsx (1 hunks)
- server/app/query/view/[id]/page.tsx (7 hunks)
- server/data/query.ts (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- server/app/query/page.tsx
- server/app/query/view/[id]/page.tsx
- server/data/query.ts
| if (data) { | ||
| return processQueryData(data); | ||
| } else { | ||
| notFound(); |
There was a problem hiding this comment.
that's a little weird but I guess the status of a sql query that returns no data does not have to be 404
There was a problem hiding this comment.
this actually does return a 404: https://nextjs.org/docs/app/api-reference/file-conventions/not-found
There was a problem hiding this comment.
then it is probably better to check the status rather than request body before showing not found?
There was a problem hiding this comment.
The query doesn't throw an error if nothing is found, so you check to see if it's empty. We could have it throw an error, by calling single() instead of maybeSingle(), but then we have to start parsing the error details to distinguish between not found and any other database error.
…ervers; update comment in StasusEvent updater
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
server/app/query/page.tsx (2)
29-61: Function logic looks good but consider adding comments for clarity.The
updateDatafunction correctly updates the state with new status events. However, adding comments to explain the logic, especially the nested structure update, would improve readability.+ // Update the state with new status events for the given query and remote server. setDataByQuery((prev) => { let _prev = prev; if (!Object.hasOwn(prev, query.uuid)) { // if queryID not in dataByQuery yet, // add initial status before updating value. // otherwise prev[query.uuid][statusEvent][remteServer.ServerName] // doesn't exist, and cannot be updated. we need to fill in the // nested structure, which `initialStatusEventByRemoteServer` does. _prev = { ..._prev, [query.uuid]: { statusEvent: initialStatusEventByRemoteServer, query: query, }, }; } return { ..._prev, [query.uuid]: { ..._prev[query.uuid], statusEvent: { ..._prev[query.uuid].statusEvent, [remoteServer.remoteServerName]: statusEvent, }, }, }; });
110-114: Consider using a ternary operator for conditional rendering.The conditional rendering for displaying messages when no queries are running can be simplified using a ternary operator.
- {Object.keys(dataByQuery).length == 0 && ( - <h3 className="text-lg font-bold leading-7 text-gray-900 sm:truncate sm:text-xl sm:tracking-tight"> - None currently running. - </h3> - )} + {Object.keys(dataByQuery).length == 0 ? ( + <h3 className="text-lg font-bold leading-7 text-gray-900 sm:truncate sm:text-xl sm:tracking-tight"> + None currently running. + </h3> + ) : null}
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- server/app/query/page.tsx (1 hunks)
- server/app/query/view/[id]/components.tsx (3 hunks)
- server/app/query/view/[id]/page.tsx (7 hunks)
- sidecar/app/routes/websockets.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sidecar/app/routes/websockets.py
Files skipped from review as they are similar to previous changes (2)
- server/app/query/view/[id]/components.tsx
- server/app/query/view/[id]/page.tsx
Additional comments not posted (4)
server/app/query/page.tsx (4)
17-23: Type declarations look good.The type declarations for
QueryDataandDataByQueryare correct and appropriately used in the code.
64-75: Polling mechanism looks good but consider adding a cleanup function.The
useEffecthook correctly polls running queries every second. Ensure the cleanup function is properly implemented to clear the interval.
116-196: Render logic for query details looks good.The render logic correctly iterates over
dataByQueryto display query details, including run times and statuses for each query.
1-14: Ensure all imports are necessary.Review the imports to ensure they are all used in the file. Unused imports can be removed to keep the code clean.
Verification successful
All imports are necessary and used in the file.
The verification confirms that each import in
server/app/query/page.tsxis utilized within the file.
useEffectanduseStatefrom "react"Linkfrom "next/link"StatusPillandRunTimePillfrom "@/app/query/view/[id]/components"StatusEvent,RemoteServer,RemoteServerNames,IPARemoteServers,StatusEventByRemoteServer, andinitialStatusEventByRemoteServerfrom "@/app/query/servers"getQueryByUUIDandQueryfrom "@/data/query"Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all imports are used in the file. # Test: Search for the usage of each import. Expect: Each import should be used at least once. rg --type js --type ts --type tsx 'useEffect|useState|Link|StatusPill|RunTimePill|StatusEvent|RemoteServer|RemoteServerNames|IPARemoteServers|StatusEventByRemoteServer|initialStatusEventByRemoteServer|getQueryByUUID|Query' server/app/query/page.tsxLength of output: 278
Script:
#!/bin/bash # Description: Verify all imports are used in the file. # Test: Search for the usage of each import. Expect: Each import should be used at least once. rg 'useEffect|useState|Link|StatusPill|RunTimePill|StatusEvent|RemoteServer|RemoteServerNames|IPARemoteServers|StatusEventByRemoteServer|initialStatusEventByRemoteServer|getQueryByUUID|Query' server/app/query/page.tsxLength of output: 2487
| useEffect(() => { | ||
| (async () => { | ||
| setDataByQuery((prev) => { | ||
| return Object.fromEntries( | ||
| Object.keys(prev) | ||
| .filter((queryID) => queryIDs.includes(queryID)) | ||
| .map((queryID) => [queryID, prev[queryID]]), | ||
| ); | ||
| }); | ||
|
|
||
| const promises = queryIDs.map(async (queryID) => { | ||
| const query: Query = await getQueryByUUID(queryID); | ||
| const remoteServerPromises = Object.values(IPARemoteServers).map( | ||
| async (remoteServer) => { | ||
| const statusEvent: StatusEvent = | ||
| await remoteServer.queryStatus(queryID); | ||
| updateData(query, remoteServer, statusEvent); | ||
| }, | ||
| ); | ||
| await Promise.all(remoteServerPromises); | ||
| }); | ||
| await Promise.all(promises); | ||
| })(); | ||
| }, [queryIDs]); |
There was a problem hiding this comment.
Consider using Promise.all for concurrent requests.
The useEffect hook correctly updates the state based on the current query IDs. However, using Promise.all for concurrent requests to remote servers can improve efficiency.
const promises = queryIDs.map(async (queryID) => {
const query: Query = await getQueryByUUID(queryID);
const remoteServerPromises = Object.values(IPARemoteServers).map(
async (remoteServer) => {
const statusEvent: StatusEvent =
await remoteServer.queryStatus(queryID);
updateData(query, remoteServer, statusEvent);
},
);
- await Promise.all(remoteServerPromises);
+ return Promise.all(remoteServerPromises);
});
await Promise.all(promises);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| (async () => { | |
| setDataByQuery((prev) => { | |
| return Object.fromEntries( | |
| Object.keys(prev) | |
| .filter((queryID) => queryIDs.includes(queryID)) | |
| .map((queryID) => [queryID, prev[queryID]]), | |
| ); | |
| }); | |
| const promises = queryIDs.map(async (queryID) => { | |
| const query: Query = await getQueryByUUID(queryID); | |
| const remoteServerPromises = Object.values(IPARemoteServers).map( | |
| async (remoteServer) => { | |
| const statusEvent: StatusEvent = | |
| await remoteServer.queryStatus(queryID); | |
| updateData(query, remoteServer, statusEvent); | |
| }, | |
| ); | |
| await Promise.all(remoteServerPromises); | |
| }); | |
| await Promise.all(promises); | |
| })(); | |
| }, [queryIDs]); | |
| useEffect(() => { | |
| (async () => { | |
| setDataByQuery((prev) => { | |
| return Object.fromEntries( | |
| Object.keys(prev) | |
| .filter((queryID) => queryIDs.includes(queryID)) | |
| .map((queryID) => [queryID, prev[queryID]]), | |
| ); | |
| }); | |
| const promises = queryIDs.map(async (queryID) => { | |
| const query: Query = await getQueryByUUID(queryID); | |
| const remoteServerPromises = Object.values(IPARemoteServers).map( | |
| async (remoteServer) => { | |
| const statusEvent: StatusEvent = | |
| await remoteServer.queryStatus(queryID); | |
| updateData(query, remoteServer, statusEvent); | |
| }, | |
| ); | |
| return Promise.all(remoteServerPromises); | |
| }); | |
| await Promise.all(promises); | |
| })(); | |
| }, [queryIDs]); |
adds a page for running queries.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
API Changes