refactor ml containers to use tanstack query and refactor api route h… #284
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds TanStack React Query and axios, centralizes API and backend route constants, provides a shared Axios client, introduces a theme-aware Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MLContainer
participant Hook as useMLCorrelationMatrix
participant Query as ReactQuery
participant Axios as api
participant Server as API_Server
User->>MLContainer: mount / request plot
MLContainer->>Hook: call useMLCorrelationMatrix(plotType)
Hook->>Query: useQuery(fetchCorrelationMatrix)
Query->>Axios: GET /api/ml/...
Axios->>Server: network request
Server-->>Axios: JSON (plot params)
Axios-->>Query: resolved data
Query->>Hook: data / status / refetch
Hook->>MLContainer: transformed data, isLoading, error, refetch
alt Loading
MLContainer->>User: show loading UI
else Error
MLContainer->>User: show error + Retry
User->>MLContainer: click Retry
MLContainer->>Hook: refetch()
else Success
MLContainer->>User: render Plotly + Refresh
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…ble types' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
![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
🤖 Fix all issues with AI agents
In `@packages/client/src/constants/apiRoutes.ts`:
- Around line 93-97: ApiRoute and BackendRoute currently alias (typeof
API_ROUTES)[keyof typeof API_ROUTES], which picks the top-level nested objects
instead of the leaf route string values; change them to extract leaf string
values from the nested route objects (e.g., introduce a recursive mapped type
like ValueOfDeep<T> or LeafRouteValues<T> and then define ApiRoute =
ValueOfDeep<typeof API_ROUTES> and BackendRoute = ValueOfDeep<typeof
BACKEND_ROUTES>) so the resulting types are a union of the route string paths
rather than sub-objects; update references to ApiRoute and BackendRoute
accordingly.
In `@packages/client/src/hooks/useMLCorrelationMatrix.ts`:
- Around line 29-38: In fetchCorrelationMatrix, remove the redundant JSON.parse
by calling api.get with the correct generic type (PlotParams) and return
response.data directly (locate the function fetchCorrelationMatrix and the
api.get usage); also simplify the redundant theme check by replacing the
`!!resolvedTheme && resolvedTheme !== undefined` condition with a single truthy
check like `resolvedTheme` (locate the resolvedTheme condition around line 62)
so you don't repeat undefined checks.
🧹 Nitpick comments (6)
packages/client/src/pages/_app.tsx (1)
15-34: Module-levelQueryClientis risky in Next.js Pages Router with SSR.In Next.js Pages Router,
_app.tsxexecutes on the server for SSR'd pages, and a module-levelQueryClientis shared across all incoming requests — potentially leaking cached data between users. The standard pattern is to create it inside the component viauseStateoruseRef:-const queryClient = new QueryClient({ - defaultOptions: { ... }, -}); - export default function App({ Component, pageProps }: AppProps) { + const [queryClient] = useState(() => new QueryClient({ + defaultOptions: { ... }, + })); return (Currently this is safe because queries are gated by
enabled: !!resolvedTheme(always falsy server-side), but future hooks without that guard will silently inherit the shared cache.packages/client/src/lib/api.ts (2)
62-95:backendApiisn't actually lazy — andrequire()is unnecessary here.
getBackendApi()is called immediately on line 95, so the singleton is created at module-import time — the lazy wrapper adds complexity without benefit. Additionally,prodURLis a simple compile-time constant derived fromprocess.env.NODE_ENV; a static import is simpler and works with tree-shaking/bundler analysis:♻️ Suggested simplification
-import axios, { type AxiosInstance } from "axios"; +import axios, { type AxiosInstance } from "axios"; +import { prodURL } from "@shared/helios-types"; -const createBackendApi = (): AxiosInstance => { - // Dynamic import to handle environment-specific URLs - // eslint-disable-next-line `@typescript-eslint/no-var-requires` - const { prodURL } = require("@shared/helios-types"); - - return axios.create({ - baseURL: prodURL, - headers: { - Accept: "application/json", - "Content-Type": "application/json", - }, - timeout: DEFAULT_TIMEOUT, - }); -}; - -let backendApiInstance: AxiosInstance | null = null; - -const getBackendApi = (): AxiosInstance => { - if (!backendApiInstance) { - backendApiInstance = createBackendApi(); - } - return backendApiInstance; -}; - -export const backendApi = getBackendApi(); +export const backendApi: AxiosInstance = axios.create({ + baseURL: prodURL, + headers: { + Accept: "application/json", + "Content-Type": "application/json", + }, + timeout: DEFAULT_TIMEOUT, +});
100-123: No-op interceptors add boilerplate without behavior.Both the request and response interceptors are pass-through. Consider removing them until actual logic is needed — they can always be added later. Dead scaffolding accumulates noise.
packages/client/src/hooks/useMLCorrelationMatrix.ts (1)
62-62: Redundant condition:!!resolvedThemealready handlesundefined.
!!resolvedTheme && resolvedTheme !== undefined— the second clause is always true when the first is true. Simplify to:- enabled: !!resolvedTheme && resolvedTheme !== undefined, + enabled: !!resolvedTheme,packages/client/src/components/containers/MLContainer.tsx (2)
33-40: Loading message branching is subtly correct but worth a comment.
isFetchingis alwaystruewhenisLoadingis true for a normal query. However, because the hook overridesisLoadingto include!resolvedTheme, the"Loading..."fallback actually fires when the theme hasn't resolved yet (query disabled). A brief inline comment would prevent a future developer from "fixing" this seemingly dead branch.
1-1:"use client"is unnecessary in Next.js Pages Router.This project uses
pages/_app.tsx(Pages Router), where all components are client-side by default. The"use client"directive is only meaningful in the App Router (app/directory). It's harmless here but adds noise and is inconsistently used across the codebase (only 3 components use it, while others with hooks don't).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@rules/api-architecture.md`:
- Around line 436-441: Update the "Response Caching" bullet to reflect that
TanStack Query is already integrated in this PR by changing the phrasing from a
future enhancement to current state and referencing the added hook
(useMLCorrelationMatrix) as an example; edit the line that currently reads
"Integrate with TanStack Query for intelligent caching" to something like
"Integrated with TanStack Query (e.g., useMLCorrelationMatrix) for intelligent
response caching" so the docs match the codebase.
- Around line 22-28: The examples reuse the identifier "const response" in the
same scope causing TypeScript redeclaration errors; update each snippet that
calls api.get (e.g., calls using API_ROUTES.ml.packetCorrelationMatrix and
API_ROUTES.drivers.base or other API_ROUTES entries) to use unique variable
names (like packetCorrelationResponse, driversResponse) or split examples into
separate independent code blocks so each "const response" is in its own scope;
search for other occurrences of "const response" in these examples and apply the
same renaming or scoping change to eliminate duplicate declarations.
🧹 Nitpick comments (3)
packages/client/src/lib/api.ts (2)
60-83: No-op interceptors add dead code; consider removing until needed.These interceptors are pure pass-through — they don't modify requests or responses. Scaffolding placeholder code increases noise and gives a false sense of "centralized error handling" (as claimed in the module docstring). Remove them and add real interceptors when there's an actual requirement (e.g., attaching auth tokens, global error toasts).
29-29:DEFAULT_TIMEOUTis declared then re-exported; simplify to a singleexport const.The constant is declared on Line 29 without
export, then re-exported on Line 88. Since it's already used internally on Line 54, just addexportat the declaration site and drop the redundant re-export.Proposed fix
-const DEFAULT_TIMEOUT = 30000; +export const DEFAULT_TIMEOUT = 30000;And remove lines 85-88.
Also applies to: 88-88
rules/api-architecture.md (1)
1-467: Document is heavily repetitive — the same rules are restated 3-4 times across sections.The "Critical Rules" (Lines 16-134), "Common Mistakes" (Lines 275-359), and "Migration Guide" (Lines 363-394) sections all convey the same information with slightly different framing. Consider consolidating into a single rules section with one concise example each, plus the migration guide. This would cut the document roughly in half and make it more likely to actually be read.
justin-phxm
force-pushed
the
refactor-to-tanstack-query
branch
from
80cd110 to
31563ce
Compare
1 participant
…andlers
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores