refactor: react-router -> tanstack start #498
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughComprehensive migration from React Router to TanStack Router/React Start, including architectural shifts to file-based routing, suspense-based data fetching, server-side middleware composition, and Vite-based build pipeline. Dependencies updated, server entry point refactored, and client navigation APIs standardized across the application. Changes
Possibly related PRs
🚥 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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tsconfig.json (1)
2-10:⚠️ Potential issue | 🟡 MinorRemove stale React Router references from tsconfig.json.
The
includearray androotDirsstill reference.react-router/types/**/*, but this directory does not exist and there are no remaining react-router imports in the codebase. These references should be removed as part of the migration to TanStack Router.Remove:
.react-router/types/**/*from theincludearray./.react-router/typesfrom therootDirsarrayapp/client/components/snapshots-table.tsx (1)
51-58:⚠️ Potential issue | 🟡 MinorRemove manual query invalidation - already handled by global MutationCache.
The mutations include manual
queryClient.invalidateQueries({ queryKey: ["listSnapshots"] })calls in theironSuccesshandlers. The codebase already has a global MutationCache configured inapp/router.tsx(lines 20–28) that callsqueryClient.invalidateQueries()on every mutation's success/error, invalidating all queries. These manual invalidations are redundant and should be removed.
🤖 Fix all issues with AI agents
In `@app/client/modules/backups/components/schedule-mirrors-config.tsx`:
- Around line 41-59: The component seeds local state from initialData and never
re-syncs when the query result currentMirrors changes, causing stale assignments
and potential overwrites; add a useEffect in ScheduleMirrorsConfig that watches
currentMirrors and hasChanges and, when hasChanges is false and currentMirrors
exists, builds a new Map (same shape as the existing map creation) and calls
setAssignments to replace assignments so the UI reflects fresh server data; keep
all existing behavior for user edits (i.e., do not override when hasChanges is
true) and reference the existing variables/functions: initialData,
currentMirrors, assignments, setAssignments, and hasChanges to locate where to
add the effect.
In `@app/client/modules/backups/components/schedule-notifications-config.tsx`:
- Around line 33-45: The component ScheduleNotificationsConfig initializes
assignments with useState from the constructed map but never re-syncs when props
scheduleId or initialData change; add a guarded re-sync effect: implement a
useEffect that watches [scheduleId, initialData] and, if there are no local
edits (detect by comparing current assignments Map to a freshly built map from
initialData or track an isDirty boolean updated on user edits), call
setAssignments with the rebuilt Map; ensure the effect references the same
map-building logic used during initialization so assignments are reset only when
appropriate and not overwrite unsaved local changes.
In `@app/client/modules/backups/components/snapshot-file-browser.tsx`:
- Around line 98-103: The route paths used in snapshot-file-browser.tsx are
missing the layout prefix; update the JSX that sets the Link/navigation "to"
values so they include the "/(dashboard)/" prefix: when backupId is truthy use
"/(dashboard)/backups/$backupId/$snapshotId/restore" with params { backupId,
snapshotId: snapshot.short_id }, otherwise use
"/(dashboard)/repositories/$repoId/$snapId/restore" with params { repoId:
repositoryId, snapId: snapshot.short_id }; modify the expression that currently
constructs the "to" and ensure the corresponding params (backupId,
snapshot.short_id, repositoryId, snapshot.short_id) remain unchanged.
In `@app/client/modules/backups/routes/backups.tsx`:
- Around line 32-35: BackupsPage is using useSuspenseQuery which never returns
isLoading=true, so remove the unused isLoading from the destructure (change
"const { data: schedules, isLoading } = useSuspenseQuery(...)" to only pull
data: schedules) and delete the dead conditional block that checks isLoading and
renders the loading state; keep the suspense rendering path and use schedules
from listBackupSchedulesOptions() as the single source of truth.
In `@app/client/modules/backups/routes/create-backup.tsx`:
- Around line 24-26: The conditional that checks loadingVolumes (returned from
useSuspenseQuery(...listVolumesOptions())) is dead because useSuspenseQuery
suspends until volumesData is available; remove the loading check/branch that
references loadingVolumes (and any JSX shown when loadingVolumes is true) and
either rely on React.Suspense fallback for loading UI or replace
useSuspenseQuery with a non-suspending hook (e.g., useQuery) if you need an
in-component isLoading path; update references to volumesData/loadingVolumes
accordingly (look for useSuspenseQuery, listVolumesOptions, volumesData, and
loadingVolumes in this component and the similar pattern in backups.tsx).
In `@app/client/modules/repositories/routes/snapshot-details.tsx`:
- Around line 125-150: The Backup Schedule/Volume link block renders even when
backupSchedule is undefined, producing empty params and broken routes; update
the JSX in snapshot-details.tsx to only render the fragment containing the Link
components when backupSchedule is truthy (e.g., wrap the fragment in
{backupSchedule && ( ... )}) and ensure you also guard access to
backupSchedule.volume (e.g., backupSchedule.volume && ...) or use optional
chaining for params and displayed names so you never pass empty strings to the
Link params or attempt to read properties of undefined.
In `@app/client/modules/repositories/tabs/info.tsx`:
- Around line 118-121: The handleSubmit function currently types its parameter
as React.SubmitEvent which is invalid; update the handler to use
React.SubmitEventHandler<HTMLFormElement> by changing the signature of
handleSubmit (or declare handleSubmit:
React.SubmitEventHandler<HTMLFormElement>) so the event type is correct in React
19; if you need native SubmitEvent properties instead, use
React.SyntheticEvent<HTMLFormElement, SubmitEvent> as the parameter type for
handleSubmit.
In `@app/client/modules/settings/routes/settings.tsx`:
- Line 105: The handler signatures use the non-existent type React.SubmitEvent;
update the submit handler parameter types to React.FormEvent<HTMLFormElement>
(e.g., in handleChangePassword) and replace any other occurrences of
React.SubmitEvent (line ~144 and similar submit handlers) with
React.FormEvent<HTMLFormElement> so the form submit events are typed correctly.
In `@app/client/modules/volumes/routes/volumes.tsx`:
- Line 75: The className strings in components like the volumes route (the
className="w-full lg:w-45 min-w-45 -mr-px -mt-px" on the element in volumes.tsx)
use non-standard Tailwind utilities (w-45, min-w-45, w-25) that won’t be
generated; either replace those tokens with standard Tailwind tokens or
arbitrary values (e.g., change lg:w-45 to lg:w-[11.25rem] or lg:w-44/lg:w-48,
min-w-45 to min-w-[11.25rem] or min-w-44, and any w-25 to w-24 or w-[6.25rem]),
or add explicit width entries in your Tailwind `@theme` block (define
--width-45/--width-25 and corresponding keys) and rebuild. Apply the same fix to
the other occurrences mentioned (lines in repositories.tsx, notifications.tsx,
backups components) by editing the className attributes or the theme config
accordingly.
In `@app/middleware/api-client.ts`:
- Around line 5-14: The middleware currently mutates the module-level singleton
via client.setConfig() (in apiClientMiddleware using getRequestHeaders()),
causing race conditions; change it to avoid mutating global state by creating a
per-request client instance (or using a per-request factory) and attach that
instance to the request/context for downstream use, or alternatively stop
calling client.setConfig() and instead pass request-specific headers on each API
call; update apiClientMiddleware to build the per-request client (or binder)
from the incoming request origin and cookie header and expose it to handlers
rather than calling client.setConfig().
In `@app/middleware/auth.ts`:
- Line 14: Wrap the await getStatus() call inside the auth middleware (where
"const status = await getStatus()" appears) in a try-catch; on error log the
exception (using the module logger or console.error), and return a safe fallback
response instead of letting the exception bubble—either call next(err) to
propagate a 5xx error or send a 503 Service Unavailable / unauthenticated
response depending on the middleware's convention; ensure the rest of the
middleware uses the guarded/defined status value (e.g., treat undefined as
unauthenticated) so downstream code doesn't crash.
In `@app/routes/`(dashboard)/route.tsx:
- Around line 22-25: The loader is asserting the fetchUser result to AppContext
which can mask undefined vs null for the user field; instead, call fetchUser()
and explicitly normalize session?.user to null (or adjust fetchUser to return
user: User | null) before returning so the returned object truly matches
AppContext; update the loader function (and/or fetchUser) to produce { user:
user ?? null, hasUsers } rather than using "as AppContext" so type mismatches
are caught at compile time (refer to loader, fetchUser, and AppContext).
- Around line 9-14: The fetchUser server handler currently returns a hardcoded
hasUsers: true which breaks onboarding; replace that with a real DB check by
calling authService.hasUsers() and return its boolean result. In the
createServerFn handler for fetchUser (which also calls getRequestHeaders() and
auth.api.getSession), await authService.hasUsers() and include that value in the
returned object instead of the literal true so the authentication middleware can
correctly detect whether users exist.
In `@app/server/core/config.ts`:
- Around line 49-51: The trustedOrigins assignment can include an empty string
when TRUSTED_ORIGINS is set to "" because split+trim yields [""]; update the
trustedOrigins expression in config.ts to filter out empty strings after
trimming (e.g., use .map(origin => origin.trim()).filter(origin => origin.length
> 0)) before concatenating s.BASE_URL so you never end up with ["", s.BASE_URL];
keep the existing nullish coalescing fallback ([s.BASE_URL]) intact.
In `@package.json`:
- Line 113: The package.json pins "nitro" to an alpha release (^3.0.1-alpha.2)
which is unstable; change the dependency entry for "nitro" to the latest stable
release ("2.13.1") in package.json (replace the value "^3.0.1-alpha.2" with
"2.13.1"), then run your package manager install (npm/yarn/pnpm) and test to
ensure no compatibility regressions.
In `@vite.config.ts`:
- Around line 12-27: Move the React Compiler setup out of the separate
babel(...) plugin and into the viteReact(...) call: remove the babel({ filter:
..., babelConfig: { presets: ["@babel/preset-typescript"], plugins:
[["babel-plugin-react-compiler"]], }, }) invocation and instead add a babel
option on viteReact such as viteReact({ babel: { plugins:
["babel-plugin-react-compiler"], /* include presets if needed e.g.
"@babel/preset-typescript" */ } }). Ensure only viteReact handles JSX transforms
to avoid duplicate processing.
🧹 Nitpick comments (19)
app/server/modules/events/__tests__/events.controller.test.ts (1)
47-57: Consider extracting polling constants for readability.The polling loops use magic numbers (20 iterations, 10ms delay). While functional, extracting these to named constants would improve readability and make timeout adjustments easier if tests become flaky.
♻️ Optional refactor
+const POLL_INTERVAL_MS = 10; +const MAX_POLL_ATTEMPTS = 20; + test("should cleanup SSE listeners when client disconnects", async () => { const { token } = await createTestSession(); const initialCount = serverEvents.listenerCount("doctor:cancelled"); const res = await app.request("/api/v1/events", { headers: getAuthHeaders(token), }); expect(res.status).toBe(200); - for (let i = 0; i < 20 && serverEvents.listenerCount("doctor:cancelled") < initialCount + 1; i++) { - await new Promise((resolve) => setTimeout(resolve, 10)); + for (let i = 0; i < MAX_POLL_ATTEMPTS && serverEvents.listenerCount("doctor:cancelled") < initialCount + 1; i++) { + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); } expect(serverEvents.listenerCount("doctor:cancelled")).toBe(initialCount + 1); await res.body?.cancel(); - for (let i = 0; i < 20 && serverEvents.listenerCount("doctor:cancelled") > initialCount; i++) { - await new Promise((resolve) => setTimeout(resolve, 10)); + for (let i = 0; i < MAX_POLL_ATTEMPTS && serverEvents.listenerCount("doctor:cancelled") > initialCount; i++) { + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); } expect(serverEvents.listenerCount("doctor:cancelled")).toBe(initialCount); });app/client/modules/volumes/routes/volumes.tsx (1)
43-49: Redundant fallback withuseSuspenseQuery.The
|| []fallback on line 49 is unnecessary. WithuseSuspenseQuery,datais guaranteed to be present when this component renders (suspense handles the loading state). Additionally,filter()always returns an array, neverundefined.♻️ Suggested simplification
const filteredVolumes = - data.filter((volume) => { + data.filter((volume) => { const matchesSearch = volume.name.toLowerCase().includes(searchQuery.toLowerCase()); const matchesStatus = !statusFilter || volume.status === statusFilter; const matchesBackend = !backendFilter || volume.type === backendFilter; return matchesSearch && matchesStatus && matchesBackend; - }) || []; + });app/client/components/app-breadcrumb.tsx (1)
30-31: Minor: Redundant optional chaining after type assertion.Line 30 casts
breadcrumbFnasBreadcrumbFunction(non-nullable), but line 31 usesbreadcrumbFn?.(...)with optional chaining. The optional chaining is redundant given the type assertion guarantees the function exists.Suggested simplification
const breadcrumbFn = lastMatchWithBreadcrumb.staticData?.breadcrumb as BreadcrumbFunction; - const breadcrumbs = breadcrumbFn?.(lastMatchWithBreadcrumb); + const breadcrumbs = breadcrumbFn(lastMatchWithBreadcrumb);app/routes/api.$.ts (1)
8-17: Consider adding OPTIONS handler for CORS preflight requests.The route handlers cover GET, POST, DELETE, PUT, and PATCH, but OPTIONS is missing. If your API supports CORS (which the relevant snippet shows
cors()middleware increateApp), preflight requests may need to be handled here.💡 Proposed addition for OPTIONS handler
export const Route = createFileRoute("/api/$")({ server: { handlers: { GET: handle, POST: handle, DELETE: handle, PUT: handle, PATCH: handle, + OPTIONS: handle, }, }, });app/routes/(dashboard)/volumes/create.tsx (1)
4-22: Consider simplifying by using CreateVolumePage directly as the component.The
RouteComponentwrapper function is a thin passthrough that only renders<CreateVolumePage />. You could simplify by assigningCreateVolumePagedirectly to thecomponentproperty.💡 Proposed simplification
export const Route = createFileRoute("/(dashboard)/volumes/create")({ - component: RouteComponent, + component: CreateVolumePage, staticData: { breadcrumb: () => [{ label: "Volumes", href: "/volumes" }, { label: "Create" }], }, head: () => ({ meta: [ { title: "Zerobyte - Create Volume" }, { name: "description", content: "Create a new storage volume with automatic mounting and health checks.", }, ], }), }); - -function RouteComponent() { - return <CreateVolumePage />; -}app/server.ts (1)
17-21: Consider adding error handling for the startup sequence.If
runDbMigrations(),runMigrations(), orstartup()throw an error, the server will crash without graceful cleanup. While the migration functions have internal error handling that callsprocess.exit(1), adding top-level try-catch could provide consistent error logging.💡 Optional: Add try-catch for cleaner error handling
-runDbMigrations(); - -await runMigrations(); -await startup(); +try { + runDbMigrations(); + await runMigrations(); + await startup(); +} catch (err) { + logger.error("Failed to start server:", err); + process.exit(1); +}app/routes/(dashboard)/backups/$backupId.$snapshotId.restore.tsx (2)
11-19: Unnecessary optional chaining after null guard.After the check on line 11-13 throws when
schedule.datais falsy,schedule.datais guaranteed to be defined. The optional chaining on lines 17 and 19 (schedule.data?.repositoryId) is redundant.Suggested fix
const [snapshot, repository] = await Promise.all([ context.queryClient.ensureQueryData({ - ...getSnapshotDetailsOptions({ path: { id: schedule.data?.repositoryId, snapshotId: params.snapshotId } }), + ...getSnapshotDetailsOptions({ path: { id: schedule.data.repositoryId, snapshotId: params.snapshotId } }), }), - context.queryClient.ensureQueryData({ ...getRepositoryOptions({ path: { id: schedule.data?.repositoryId } }) }), + context.queryClient.ensureQueryData({ ...getRepositoryOptions({ path: { id: schedule.data.repositoryId } }) }), ]);
6-32: Consider addingerrorComponentandheadmetadata for consistency.The similar restore route at
app/routes/(dashboard)/repositories/$repoId.$snapId.restore.tsxincludes both anerrorComponentandheadmetadata. Adding these would improve error handling and SEO/accessibility consistency across restore routes.Suggested additions
export const Route = createFileRoute("/(dashboard)/backups/$backupId/$snapshotId/restore")({ component: RouteComponent, + errorComponent: (e) => <div>{e.error.message}</div>, loader: async ({ params, context }) => { // ... existing loader code }, staticData: { breadcrumb: (match) => [ { label: "Backup Jobs", href: "/backups" }, { label: match.loaderData?.schedule?.name || "Job", href: `/backups/${match.params.backupId}` }, { label: match.params.snapshotId }, { label: "Restore" }, ], }, + head: ({ params }) => ({ + meta: [ + { title: `Zerobyte - Restore Snapshot ${params.snapshotId}` }, + { + name: "description", + content: "Restore files from a backup snapshot.", + }, + ], + }), });app/client/modules/backups/routes/backups.tsx (1)
26-30: Remove unusedclientLoaderexport.The
clientLoaderexport is not referenced anywhere in the codebase and appears to be a remnant from a previous routing pattern. The component already loads data correctly usinguseSuspenseQuerywith TanStack React Query, which is the proper pattern for TanStack Router.app/client/modules/volumes/routes/volume-details.tsx (1)
144-148: Preserve existing search params when switching tabs.The current
navigatecall replaces the search state, which can drop other query keys. Consider spreading the existing search params.♻️ Suggested update
- <Tabs - value={activeTab} - onValueChange={(value) => navigate({ to: ".", search: () => ({ tab: value }) })} - className="mt-4" - > + <Tabs + value={activeTab} + onValueChange={(value) => navigate({ to: ".", search: (s) => ({ ...s, tab: value }) })} + className="mt-4" + >app/routes/(dashboard)/backups/create.tsx (1)
5-16: Consider addingheadmetadata for consistency.Other create routes (e.g.,
repositories/create.tsx,notifications/create.tsx,volumes/create.tsx) includeheadmetadata with title and description. This route is missing it, which creates inconsistency in SEO and document head behavior.💡 Suggested addition
export const Route = createFileRoute("/(dashboard)/backups/create")({ loader: async ({ context }) => { await Promise.all([ context.queryClient.ensureQueryData({ ...listVolumesOptions() }), context.queryClient.ensureQueryData({ ...listRepositoriesOptions() }), ]); }, staticData: { breadcrumb: () => [{ label: "Backup Jobs", href: "/backups" }, { label: "Create" }], }, + head: () => ({ + meta: [ + { title: "Zerobyte - Create Backup Job" }, + { + name: "description", + content: "Create a new backup job with scheduling and retention policies.", + }, + ], + }), component: RouteComponent, });app/routes/(dashboard)/volumes/index.tsx (2)
7-7: Basic error component may benefit from consistent styling.The
errorComponentrenders a plain<div>with the error message. While this matches patterns in other routes (e.g.,route.tsx), consider extracting a shared error component if you want consistent error presentation across routes.
8-10: Minor inconsistency: spread operator usage on query options.Other loaders use the spread pattern (
{ ...listVolumesOptions() }), but this file passeslistVolumesOptions()directly. Both work correctly, but the spread pattern is more common in this codebase (seebackups/create.tsxline 8).💡 Suggested fix for consistency
loader: async ({ context }) => { - await context.queryClient.ensureQueryData(listVolumesOptions()); + await context.queryClient.ensureQueryData({ ...listVolumesOptions() }); },app/routes/__root.tsx (3)
14-18: Duplicate<meta>tags for charset and viewport.The
head()function definescharSet(line 16) andviewport(line 17), butRootLayoutalso hardcodes these same tags in the<head>element (lines 47-48). When<HeadContent />renders, this will result in duplicate meta tags in the document.Remove the hardcoded duplicates from
RootLayoutsincehead()already provides them via<HeadContent />:♻️ Proposed fix
<head> - <meta charSet="utf-8" /> - <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no" /> <link rel="icon" type="image/png" href="/images/favicon/favicon-96x96.png" sizes="96x96" />Note: If you need the additional viewport constraints (
maximum-scale=1, user-scalable=no), update thehead()viewport content instead.Also applies to: 46-48
59-60: Consider conditionally rendering devtools for production builds.Both
TanStackRouterDevtoolsandReactQueryDevtoolsare rendered unconditionally. While these libraries typically self-disable in production, explicitly gating them ensures no devtools code is shipped in production bundles.♻️ Proposed conditional rendering
- <TanStackRouterDevtools position="bottom-right" /> - <ReactQueryDevtools buttonPosition="bottom-left" /> + {import.meta.env.DEV && <TanStackRouterDevtools position="bottom-right" />} + {import.meta.env.DEV && <ReactQueryDevtools buttonPosition="bottom-left" />}
38-38: Error component could benefit from additional context.The
errorComponentonly displays the error message. Consider adding error boundary styling or a recovery action for better UX.app/routes/(dashboard)/backups/index.tsx (1)
5-24: Consider addingerrorComponentfor consistency.Other dashboard routes like
repositories/index.tsxdefine anerrorComponent. Adding one here would provide consistent error handling across dashboard routes.♻️ Proposed addition
export const Route = createFileRoute("/(dashboard)/backups/")({ component: RouteComponent, + errorComponent: (e) => <div>{e.error.message}</div>, loader: async ({ context }) => {app/router.tsx (1)
49-51: Replaceanytype with proper route match type.Using
anyfor thematchparameter reduces type safety. Consider using a more specific type from TanStack Router.♻️ Proposed type improvement
+import type { AnyRouteMatch } from "@tanstack/react-router"; + declare module "@tanstack/react-router" { interface Register { router: ReturnType<typeof getRouter>; } interface StaticDataRouteOption { - breadcrumb?: (match: any) => BreadcrumbItemData[] | null; + breadcrumb?: (match: AnyRouteMatch) => BreadcrumbItemData[] | null; } }app/client/modules/backups/routes/backup-details.tsx (1)
43-66: UseloaderData.scheduleas query seed to avoid redundant request.
loaderData.scheduleis currently unused while the schedule always refetches on render. Pass it asinitialDatato use the preloaded data, eliminating an unnecessary network request. This pattern is already established in the same component for other query hooks.♻️ Proposed adjustment
const { data: schedule } = useSuspenseQuery({ ...getBackupScheduleOptions({ path: { scheduleId } }), + initialData: loaderData.schedule, });
app/client/modules/backups/components/schedule-mirrors-config.tsx
Outdated
Show resolved
Hide resolved
nicotsx
force-pushed
the
refactor/tanstack
branch
from
4f5c5fd to
0da4b45
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/modules/events/events.controller.ts (1)
18-29:⚠️ Potential issue | 🟡 MinorMinor race: event handlers can write to a closed stream.
The async event handlers (e.g.,
onBackupStarted) don't guard against writing after the stream is aborted. If an event fires between disconnect detection andcleanup()removing the listeners,stream.writeSSEcould throw an unhandled rejection.The window is very small and Hono's stream likely swallows writes to closed streams, but you could add a guard at the top of each handler for extra safety:
const onBackupStarted = async (data: { ... }) => { if (data.organizationId !== organizationId) return; + if (!keepAlive) return; await stream.writeSSE({ ... }); };Alternatively, a single helper that wraps writes would reduce repetition across all 11 handlers.
🤖 Fix all issues with AI agents
In @.dockerignore:
- Around line 6-7: Remove the two whitelist entries '!server.ts' and
'!.gitignore' from the .dockerignore file because they are unnecessary
(server.ts does not exist at repo root and .gitignore isn't used in
build/config), leaving the rest of .dockerignore intact to keep the Docker build
context minimal.
In `@app/client/modules/backups/routes/backups.tsx`:
- Around line 26-35: Remove the dead exported function clientLoader (which calls
listBackupSchedules) from the file; instead delete the entire clientLoader
declaration and its export so only the BackupsPage and its use of
useSuspenseQuery with listBackupSchedulesOptions remains, and ensure there are
no remaining imports or references to clientLoader (the route uses
ensureQueryData inline).
In `@app/client/modules/repositories/routes/restore-snapshot.tsx`:
- Around line 11-15: Remove the redundant RestoreSnapshotPage passthrough
component: delete the RestoreSnapshotPage function and replace its usages in
routes with RestoreForm directly (pass through the same props snapshot,
repository, snapshotId, returnPath) or move any route-specific logic into
RestoreForm; look for references to RestoreSnapshotPage and update
imports/usages to import and render RestoreForm instead to eliminate the
unnecessary indirection.
In `@app/client/modules/repositories/routes/snapshot-details.tsx`:
- Around line 109-116: The UI currently renders the same value twice because
both "Snapshot ID" and "Short ID" use data.short_id; update the snapshot-details
component to remove the duplicate by deleting one of the two blocks (either the
"Snapshot ID" or "Short ID" div) or, if a full snapshot ID is available
elsewhere in the response (e.g., data.id or data.full_id), replace the "Snapshot
ID" paragraph to render that full field instead of data.short_id; ensure you
only reference and render existing properties on the GetSnapshotDetailsResponse
and remove the redundant JSX block that references data.short_id twice.
In `@app/lib/request-client.ts`:
- Around line 20-33: The loadRequestClientStore function can race when multiple
callers hit the `if (!requestClientStore)` path; ensure only one
AsyncLocalStorage is created by introducing a shared initialization promise
(e.g., `requestClientStorePromise`) at module scope: when starting the import of
ASYNC_HOOKS_MODULE, assign the promise to that variable, have all callers await
it, and only the resolution sets `requestClientStore` to the new
`AsyncLocalStorage`; update loadRequestClientStore to check and await the
promise as well as the store so concurrent calls all receive the same instance
(references: loadRequestClientStore, requestClientStore,
requestClientStorePromise, ASYNC_HOOKS_MODULE, AsyncLocalStorage,
getRequestClient).
In `@app/middleware/auth.ts`:
- Around line 14-19: The catch handler for getStatus currently defaults to {
data: { hasUsers: false } }, which causes transient failures to redirect users
to /onboarding; change the fallback to { data: { hasUsers: true } } so that on
errors the code sends users to /login instead. Locate the call to getStatus() in
the auth middleware and update the catch default (the object with data.hasUsers)
to true; ensure the subsequent logic that checks status.data?.hasUsers remains
unchanged.
- Line 1: The middleware currently calls the client SDK function getStatus which
issues a loopback HTTP request; instead call the server-side auth service
directly: replace the getStatus invocation in auth middleware with a direct call
to authService.hasUsers(), import or obtain authService the same way controllers
do (or reuse the existing server-side DI/provider), remove the unnecessary HTTP
call and keep the existing error handling around the new call; also update the
other occurrences mentioned (lines 7-9) to use authService.hasUsers() and ensure
you remove or adjust any imports related to getStatus and
app/middleware/api-client.ts usage if no longer needed.
In `@app/routes/__root.tsx`:
- Around line 14-18: Remove the duplicate meta tags by editing the head()
export: delete the { charSet: "utf-8" } and the { name: "viewport", content:
"width=device-width, initial-scale=1" } entries so that <HeadContent /> and the
JSX <head> remain the single source of truth (which includes the more complete
viewport and favicons). Ensure head() still returns any other required meta
items but does not re-declare charset or viewport to avoid conflicting
duplicates from head() and <HeadContent />.
- Around line 59-60: The devtools components (TanStackRouterDevtools and
ReactQueryDevtools) are conditionally rendered using process.env.NODE_ENV but
Vite must statically replace that value for tree-shaking to remove them from
production bundles; update vite.config.ts to add a define entry that sets
'process.env.NODE_ENV' to JSON.stringify('production') so builds (including
server/SSR bundles) will eliminate devtools code from production output.
In `@app/routes/`(dashboard)/backups/$backupId.$snapshotId.restore.tsx:
- Around line 6-32: The route is missing a head metadata configuration; add a
head function on the Route export (alongside staticData and loader) that returns
a title and description (use schedule and snapshot info from match.loaderData to
build a meaningful title like `${match.loaderData?.schedule?.name} — Restore
${match.params.snapshotId}` and a concise description). Ensure the head function
is defined on the same Route created by createFileRoute and references
match.params.snapshotId and match.loaderData?.schedule to mirror other files
like `$repositoryId.tsx` and `notifications/create.tsx`.
In `@app/routes/`(dashboard)/notifications/$notificationId.tsx:
- Around line 20-22: The head meta title uses loaderData?.name without a
fallback, causing "Zerobyte - undefined" when loaderData is missing; update the
head: ({ loaderData }) => ({ meta: [ { title: `Zerobyte - ${loaderData?.name ||
"Notification Details"}` } ] }) to match the breadcrumb fallback (use
loaderData?.name || "Notification Details") so the tab shows a sensible default;
locate the head export in notifications/$notificationId.tsx and apply this
fallback.
In `@app/routes/`(dashboard)/volumes/$volumeId.tsx:
- Around line 23-25: The head function in the route uses loaderData?.volume.name
which can be undefined and yield a title like "Zerobyte - undefined"; update the
head metadata generation (the head property in
app/routes/(dashboard)/volumes/$volumeId.tsx) to safely access loaderData and
volume (e.g., use optional chaining on volume and name) and provide a sensible
fallback string (e.g., "Untitled Volume" or the volumeId) so the title never
contains "undefined".
In `@app/server.ts`:
- Around line 33-43: Wrap the async signal handlers for process.on("SIGTERM",
...) and process.on("SIGINT", ...) so that calls to shutdown() are awaited
inside a try/catch and always lead to a process.exit; specifically, catch any
rejection from shutdown() and log the error via logger.error, then call
process.exit(1) on failure (or process.exit(0) on success); additionally add a
forced-exit fallback (setTimeout) that calls process.exit(1) after a short grace
period to ensure the process cannot hang if shutdown() never resolves.
In `@app/server/utils/logger.ts`:
- Around line 56-62: The message body formatting always forces colors because
formatWithOptions spreads ctx.options.formatOptions (which contains colors:
true) after useColor; update the call to formatWithOptions so that useColor
takes precedence by placing it after the spread (i.e., merge
ctx.options.formatOptions first, then set colors: useColor), and apply the same
change to the other formatWithOptions call that uses ctx.options.formatOptions
and useColor; reference formatWithOptions, useColor, ctx.options.formatOptions
and colorize when locating and updating the two call sites.
🧹 Nitpick comments (33)
app/client/modules/backups/components/backup-card.tsx (1)
10-11: Duplicatekeyprop on both<Link>and<Card>.If this component is rendered within a list, only the outermost element (
<Link>) needs thekey. Thekeyon<Card>is redundant and can be removed.♻️ Suggested cleanup
<Link key={schedule.id} to="/backups/$scheduleId" params={{ scheduleId: schedule.id.toString() }}> - <Card key={schedule.id} className="flex flex-col h-full"> + <Card className="flex flex-col h-full">Based on learnings: In TanStack Router for the zerobyte repository, layout routes with parentheses like
(dashboard)are used for file organization only and should not be included in the route paths — the route path here correctly omits any layout prefix.app/client/modules/backups/components/schedule-mirrors-config.tsx (1)
146-161:handleResetduplicatesbuildAssignmentslogic.Lines 148–157 manually rebuild the same map that
buildAssignmentsalready produces. Consider reusing the helper:♻️ Suggested simplification
const handleReset = () => { - if (currentMirrors) { - const map = new Map<string, MirrorAssignment>(); - for (const mirror of currentMirrors) { - map.set(mirror.repositoryId, { - repositoryId: mirror.repositoryId, - enabled: mirror.enabled, - lastCopyAt: mirror.lastCopyAt, - lastCopyStatus: mirror.lastCopyStatus, - lastCopyError: mirror.lastCopyError, - }); - } - setAssignments(map); - setHasChanges(false); - } + if (currentMirrors) { + setAssignments(buildAssignments(currentMirrors)); + setHasChanges(false); + } };app/routes/(dashboard)/repositories/$repoId.$snapId.restore.tsx (1)
7-7: Consider exposing theresetfunction in the error component.TanStack Router's
errorComponentreceives{ error, reset, info }. Currently onlyerror.messageis rendered with no way for the user to retry. Adding a reset/retry button would improve UX on transient failures.♻️ Suggested improvement
- errorComponent: (e) => <div>{e.error.message}</div>, + errorComponent: ({ error, reset }) => ( + <div> + <p>{error.message}</p> + <button type="button" onClick={reset}>Retry</button> + </div> + ),app/routes/(dashboard)/repositories/$repoId.$snapshotId.tsx (1)
11-11: Consider a more robust error component.If the thrown value isn't an
Errorinstance,e.error.messagewill beundefinedand the user sees a blank<div>. A small guard (e.g.,String(e.error.message ?? e.error)) would be more resilient. Also, other route files in this migration likely share the same pattern — a shared error component would reduce duplication.docker-compose.yml (1)
45-45:zerobyte-e2ecould explicitly setPORT=4096for consistency.The
zerobyte-e2eservice maps port4096:4096but omits thePORTenvironment variable. Since the application defaults toPORT=4096(defined inapp/server/core/config.tsand documented inREADME.md), the service will listen on the correct port. However, explicitly settingPORT=4096likezerobyte-proddoes (line 45) would improve consistency and clarity across the compose file.app/server/utils/logger.ts (2)
27-31:FORCE_COLORcheck is redundant since the fallback is alreadytrue.Line 29 checks for
FORCE_COLOR, but line 30 unconditionally returnstrueanyway. TheFORCE_COLORbranch can never produce a different outcome than the default.If the intent is to default to auto-detection (e.g., via
process.stdout.isTTY), consider:Suggested change
const useColor = (() => { if (process.env.NO_COLOR !== undefined) return false; if (process.env.FORCE_COLOR === "1" || process.env.FORCE_COLOR === "true") return true; - return true; + return process.stdout.isTTY ?? false; })();
98-103: Pre-joining messages disables printf-style formatting in the reporter.The reporter calls
formatWithOptions(...logObj.args), which supports%s/%d/%osubstitutions. However,formatMessagesjoins all args into a single string before passing to consola, sologObj.argsis always[oneString]. If printf-style formatting (e.g.,logger.info("user %s connected", name)) is never used, this is fine — just noting the trade-off for sanitization..dockerignore (1)
13-13: Stale whitelist:react-router.config.tsmay no longer be needed.Since the project is migrating away from react-router, this whitelist entry will become dead. Consider removing it once the migration is complete.
Dockerfile (1)
100-101: Remove productionbun installif all dependencies are bundled in.output.With Nitro's bun preset and no
noExternalconfiguration, all npm dependencies should be bundled into.outputduring the build phase, making the runtimebun install --productionunnecessary. Removing this would reduce image size and build time. Only keep it if native modules or unbundled packages are required at runtime.app/client/modules/notifications/routes/notifications.tsx (1)
130-130: Consider using TanStack Router's type-safe params for dynamic navigation.The template literal
/notifications/${notification.id}bypasses TanStack Router's type-safe routing. The route is defined with a$notificationIdparameter, so using the params-based signature ensures type safety and prevents silent breakage if route parameters change.♻️ Suggested refactor
- onClick={() => navigate({ to: `/notifications/${notification.id}` })} + onClick={() => navigate({ to: "/notifications/$notificationId", params: { notificationId: notification.id } })}app/client/modules/volumes/tabs/info.tsx (1)
40-40: Consider using parameterized route for type-safe navigation.Other files in this PR use parameterized routes with
params(e.g.,to="/volumes/$volumeId"withparams={{ volumeId }}). Using string interpolation here works but forgoes TanStack Router's type-safe route matching.♻️ Suggested change
- void navigate({ to: `/volumes/${data.shortId}` }); + void navigate({ to: "/volumes/$volumeId", params: { volumeId: data.shortId } });tsconfig.json (1)
2-2: Remove stale.react-routerreferences fromtsconfig.json.Lines 2 and 10 reference
.react-router/typespaths which no longer exist in the repository. These are leftover from the react-router setup and should be removed with the TanStack Router migration.♻️ Suggested cleanup
- "include": ["**/*", "**/.server/**/*", "**/.client/**/*", ".react-router/types/**/*"], + "include": ["**/*", "**/.server/**/*", "**/.client/**/*"],- "rootDirs": [".", "./.react-router/types"], + "rootDirs": ["."],scripts/patch-api-client.ts (1)
15-42: The@ts-nochecksuppresses all type errors in the patched file.This is understandable given the code-gen context, but it means any type mismatches between
getRequestClient()return type andfallbackClientwill be silently ignored. If the generated client type orrequest-client.tsAPI changes, breakage won't surface at compile time.Consider narrowing to targeted
@ts-expect-errorcomments on specific lines instead of blanket suppression, or add a build-time smoke test that imports the patched module.app/routes/(dashboard)/route.tsx (1)
17-27: Route definition looks correct for TanStack Router file-based routing.The
createFileRoute("/(dashboard)")pattern, server middleware integration, and loader → component data flow are consistent with TanStack Start conventions. One small note: the inlineerrorComponentat Line 19 accessese.error.message— consider whether unstructured errors (e.g., network failures) could makemessageundefined, causing a blank error display. A fallback likee.error.message || "An unexpected error occurred"would be more resilient.app/client/modules/backups/routes/create-backup.tsx (1)
36-36: Consider using theparamspattern for type-safe navigation.Other components in this PR (e.g.,
snapshot-file-browser.tsx) useto="/backups/$backupId/..."with aparamsobject for type-safe routing. Here, a template literal is used instead, which bypasses TanStack Router's type-safe path checking.Suggested change
- void navigate({ to: `/backups/${data.id}` }); + void navigate({ to: "/backups/$backupId", params: { backupId: String(data.id) } });app/client/modules/repositories/routes/repositories.tsx (1)
129-129: Consider using TanStack Router's type-safe params for dynamic routes.String interpolation in
navigate({ to:/repositories/${repository.shortId}})bypasses TanStack Router's type-safe routing. The idiomatic approach usesparams:navigate({ to: '/repositories/$repositoryId', params: { repositoryId: repository.shortId } })This gives you compile-time route validation. Not blocking since the string form works, but worth adopting for consistency with the type-safe router.
app/routes/(dashboard)/repositories/index.tsx (1)
12-12: Minimal error component — consider adding a retry action.The
errorComponentrenders only the raw error message with no styling or recovery option. TanStack Router passes aresetfunction that can be used to retry:♻️ Suggested improvement
- errorComponent: (e) => <div>{e.error.message}</div>, + errorComponent: ({ error, reset }) => ( + <div className="flex flex-col items-center gap-3 py-12"> + <p className="text-destructive">{error.message}</p> + <button onClick={reset} className="text-sm underline"> + Try again + </button> + </div> + ),app/client/modules/repositories/routes/repository-details.tsx (1)
35-37: Suspense without a fallback.The
<Suspense>wrapper on the snapshots tab has nofallbackprop. If the snapshots query suspends, the user sees nothing (or the nearest ancestor boundary's fallback) until data resolves. Consider providing a lightweight skeleton/spinner here for a smoother tab-switch experience.app/client/modules/volumes/routes/volumes.tsx (1)
43-49: Redundant|| []fallback after.filter().
Array.prototype.filter()always returns an array (possibly empty, never falsy), so the|| []fallback on line 49 is dead code.Proposed fix
const filteredVolumes = - data.filter((volume) => { + data.filter((volume) => { const matchesSearch = volume.name.toLowerCase().includes(searchQuery.toLowerCase()); const matchesStatus = !statusFilter || volume.status === statusFilter; const matchesBackend = !backendFilter || volume.type === backendFilter; return matchesSearch && matchesStatus && matchesBackend; - }) || []; + });app/routes/(dashboard)/repositories/$repositoryId.tsx (1)
12-12: Minimal error component — consider a more user-friendly fallback.The inline
(e) => <div>{e.error.message}</div>renders raw error text with no styling or recovery affordance. Other route files in this PR (e.g.,backups/$scheduleId.tsx) may use richer error UIs. Consider reusing a shared error component for consistency and to give users a retry/navigation option.app/client/modules/volumes/routes/volume-details.tsx (2)
97-103: Dead guards afteruseSuspenseQuery.
useSuspenseQuery(line 51) suspends until data is available and throws on error, so:
!volumeIdon line 97: ifvolumeIdwere falsy, the query on line 51 would already have fired with an invalid ID and thrown.!dataon line 101:useSuspenseQueryguaranteesdatais defined.Both branches are unreachable. Consider removing them to avoid misleading readers.
Proposed fix
- if (!volumeId) { - return <div>Volume not found</div>; - } - - if (!data) { - return <div>Loading...</div>; - } - const { volume, statfs } = data;
144-148: Inconsistent search-param spreading compared torepository-details.tsx.Here
search: () => ({ tab: value })drops any other search params, whereasrepository-details.tsx(line 26) usessearch: (s) => ({ ...s, tab: value })to preserve them. Currently onlytabexists so it's harmless, but consider using the spread pattern for consistency and future-proofing.Proposed fix
- onValueChange={(value) => navigate({ to: ".", search: () => ({ tab: value }) })} + onValueChange={(value) => navigate({ to: ".", search: (s) => ({ ...s, tab: value }) })}app/router.tsx (2)
49-51:match: anyloses type safety on breadcrumb callbacks.All route
staticData.breadcrumbcallbacks receive an untypedmatch, so accesses likematch.loaderData?.namearen't checked at compile time. Consider typing it more narrowly (e.g., usingAnyRouteMatchfrom@tanstack/react-router) or at least documenting the expected shape.
20-28: GlobalMutationCacheinvalidates all queries on error as well — intentional?
onErroralso callsqueryClient.invalidateQueries(). If a mutation fails, the server state is unchanged, so refetching everything is unnecessary network traffic. This matches the existing global-invalidation pattern in this repo, so flagging only as a consideration.Based on learnings, this repo uses a global invalidation configuration for all mutations. The
onErrorinvalidation is part of that strategy.app/routes/(dashboard)/backups/$backupId.$snapshotId.restore.tsx (1)
15-20: Optional chaining onschedule.datais misleading after the null guard.After lines 11-13 throw when
!schedule.data,schedule.datais guaranteed to be defined. The?.on line 17 and line 19 (schedule.data?.repositoryId) suggests it might still be null. If you adopt theensureQueryDataapproach suggested above, this resolves naturally since the return value is the data itself.app/routes/(dashboard)/settings/index.tsx (2)
10-12: Theas AppContextcast is unnecessary.Per
app/context.ts,AppContextis{ user: User | null; hasUsers: boolean }, which matchesfetchUser()'s return type. The cast adds noise without safety benefit.Proposed simplification
loader: async () => { - const authContext = await fetchUser(); - return authContext as AppContext; + return await fetchUser(); },
7-17: Noheadmetadata defined for the settings route.Other dashboard routes (e.g., notifications/index.tsx) include
headwith a title and description meta tag. Consider adding one here for consistency and SEO.Proposed addition
staticData: { breadcrumb: () => [{ label: "Settings" }], }, + head: () => ({ + meta: [ + { title: "Zerobyte - Settings" }, + { + name: "description", + content: "Manage your account and system settings.", + }, + ], + }), });app/routes/__root.tsx (1)
38-38: Bare-bones error component may leave users stranded.The
errorComponentrenders an unstyled<div>with just the error message. Consider providing at minimum a retry action or a link back to the root, especially since this is the root route and there's no parent to catch the error.app/routes/(dashboard)/backups/index.tsx (1)
5-24: MissingerrorComponent— inconsistent with sibling routes.The volumes index route (
app/routes/(dashboard)/volumes/index.tsx) and repositories index route include anerrorComponent, but this backups route omits it. If the loader'sensureQueryDatarejects, the error will bubble to the parent route's error boundary rather than being handled locally.Consider adding an
errorComponentfor consistency:export const Route = createFileRoute("/(dashboard)/backups/")({ component: RouteComponent, + errorComponent: (e) => <div>{e.error.message}</div>, loader: async ({ context }) => {app/client/api-client/client.gen.ts (1)
17-26: Exception-driven control flow on every property access.
getRequestClient()throws when no request context exists (the normal browser path), meaning every property access onclientin the browser triggers a thrown + caught exception. This works but is not ideal for a hot path.A lighter approach would be to use a non-throwing getter (returning
undefined/null) and fall back without the try/catch overhead.Sketch — non-throwing approach
This would require a change in
app/lib/request-client.tsto expose atryGetRequestClient():export function tryGetRequestClient(): RequestClient | undefined { return requestClientStore?.getStore(); }Then in the proxy:
export const client = new Proxy(fallbackClient, { get(target, prop, receiver) { - try { - const requestClient = getRequestClient(); - return Reflect.get(requestClient, prop, receiver); - } catch { - // No request context available, use fallback - return Reflect.get(target, prop, receiver); - } + const requestClient = tryGetRequestClient(); + return Reflect.get(requestClient ?? target, prop, receiver); }, });app/routes/(dashboard)/backups/create.tsx (1)
5-16: Missinghead()metadata — inconsistent with sibling create routes.Both
app/routes/(dashboard)/repositories/create.tsxandapp/routes/(dashboard)/volumes/create.tsxdefinehead()with a title and description. This route omits it, so the page will inherit only the root title ("Zerobyte - Open Source Backup Solution") rather than showing a contextual one like "Zerobyte - Create Backup Job".Suggested addition
staticData: { breadcrumb: () => [{ label: "Backup Jobs", href: "/backups" }, { label: "Create" }], }, + head: () => ({ + meta: [ + { title: "Zerobyte - Create Backup Job" }, + { + name: "description", + content: "Create a new backup job with scheduling and retention policies.", + }, + ], + }), component: RouteComponent,app/client/modules/backups/routes/backup-details.tsx (2)
131-132: Theif (!schedule) returnguard is unreachable.Since
schedulecomes fromuseSuspenseQuery(line 64), it is guaranteed to be defined — Suspense will suspend the component before it can render withundefineddata. The guard is dead code.Optional cleanup
const handleSubmit = (formValues: BackupScheduleFormValues) => { - if (!schedule) return; - const cronExpression = getCronExpression(
232-245: Defensive nullish checks on always-defined loader data are unnecessary but harmless.
loaderData.notifs,loaderData.repos,loaderData.scheduleNotifs, andloaderData.mirrorsare all guaranteed non-nullish by the route loader'sensureQueryDatacalls. The?.and?? []guards are safe fallbacks but could be simplified if desired.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores