refactor: rename 'upstream sync' to 'remote sync' / 'Sync from Remote'#80
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR systematically renames "upstream sync" terminology to "remote sync" across the frontend codebase, including component names ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/api/notifications.ts (1)
12-24:⚠️ Potential issue | 🟠 MajorFallbacks prevent crash, but verify backend has migrated notification types.
The type narrowing from
upstream_*toremote_*will cause graceful UI degradation if the backend still returns legacy values—the notification bell will show a generic Bell icon and gray color instead of specialized icons/routing. While the app won't crash (lines 179–180 of notification-bell.tsx have fallback icons and colors), specialized notification handling will be lost silently.Before shipping this change, confirm the backend has completed its migration to only emit
remote_*notification types, or add normalization at the API boundary inlib/api/notifications.tsto map any lingeringupstream_*values to theirremote_*equivalents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/api/notifications.ts` around lines 12 - 24, The NotificationType union was tightened to remote_* names which will silently degrade the UI if the backend still emits legacy upstream_* values; add normalization at the API boundary in lib/api/notifications.ts by implementing a mapping/normalizeNotificationType function (or run normalization where notifications are parsed) that converts any upstream_* strings to their corresponding remote_* equivalents (e.g., upstream_update_applied -> remote_update_applied, upstream_update_available -> remote_update_available, upstream_sync_error -> remote_sync_error) and return values typed as NotificationType so downstream code (e.g., notification-bell rendering) always sees the new remote_* names.lib/hooks/useRemoteSync.ts (2)
115-129:⚠️ Potential issue | 🔴 Critical
"not_found"job status is not treated as terminal, causing endless polling.Polling stops only for
"complete"/"failed". If the API returns"not_found", interval polling continues indefinitely andisCheckingcan remain stuck.Suggested diff
- if (status.status === "complete" || status.status === "failed") { + if ( + status.status === "complete" || + status.status === "failed" || + status.status === "not_found" + ) { if (pollTimerRef.current) { clearInterval(pollTimerRef.current); pollTimerRef.current = null; } jobIdRef.current = null; setIsChecking(false); if (status.status === "failed" && status.error) { setError(status.error); + } else if (status.status === "not_found") { + setError("Sync job no longer exists"); } // Refresh data after job completes - await fetchData(); + if (status.status !== "not_found") { + await fetchData(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useRemoteSync.ts` around lines 115 - 129, The polling loop currently only treats status.status === "complete" or "failed" as terminal; update the check in the block that references status.status, pollTimerRef, jobIdRef, setIsChecking, setError, and fetchData to also handle the "not_found" status as terminal: when status.status is "not_found" clear and null out pollTimerRef.current, set jobIdRef.current = null, setIsChecking(false), optionally setError with a descriptive message if no error is present, and still await fetchData() so the UI refreshes and polling stops.
44-93:⚠️ Potential issue | 🟠 MajorMigrate server-state management to React Query.
configandhistoryare fetched server state and should useuseQueryrather than manualuseState/useEffectorchestration. The repository standard (established acrossuseSemanticSearch,useProjectAnalytics,useEntityHistory, and others) is to use@tanstack/react-queryfor server state. The polling logic for job status checks is compatible with React Query'srefetchIntervalor custom polling withinqueryFn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useRemoteSync.ts` around lines 44 - 93, Replace the manual fetchData/useEffect + local useState for config and history with `@tanstack/react-query` useQuery hooks: create a useQuery for the config (key e.g. ['remoteSync','config', projectId]) that calls remoteSyncApi.getConfig(projectId, accessToken) and sets enabled via the hook's enabled option; create a separate useQuery for history (key e.g. ['remoteSync','history', projectId]) that calls remoteSyncApi.getHistory(projectId, 20, accessToken) and depends on the config query (enabled only if config exists). Remove the local useState management for config/history and isLoading for those queries and map their loading/error states to the query results; for job status polling, useQuery with refetchInterval or the query's refetch to poll using jobIdRef (or a query keyed by ['remoteSync','job', jobId]) instead of manual setInterval in pollTimerRef; keep existing error handling by mapping query.error to the error state and preserve enabled gating with the query's enabled option.
🧹 Nitpick comments (2)
components/editor/RemoteSyncIndicator.tsx (1)
24-35: Prefer a React Query-backed hook here.
configis server state, but this component still loads it via an imperativeuseEffect, so it bypasses shared caching/invalidation. ReusinguseRemoteSyncor a slimmer query hook would keep the toolbar indicator aligned with config/check mutations.As per coding guidelines "Use React Query (
@tanstack/react-query) for server state management".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/RemoteSyncIndicator.tsx` around lines 24 - 35, The component currently fetches server state imperatively in RemoteSyncIndicator via a useEffect that calls remoteSyncApi.getConfig and setConfig; replace that effect with a React Query-backed hook (either the existing useRemoteSync or a slim useQuery) so the indicator uses shared caching/invalidation. Implement a query such as useQuery(['remoteSync', projectId], () => remoteSyncApi.getConfig(projectId, accessToken), { enabled: !!projectId }) or call useRemoteSync(projectId, accessToken), and map a 404 to null in the query's onError or queryFn so setConfig/useQuery data reflects null on 404; remove the useEffect and local imperative fetch to ensure React Query drives the state.lib/api/remoteSync.ts (1)
107-170: Plan deployment ordering to avoid a temporary 404 window.This file hard-switches all calls to
/remote-sync; if frontend deploys before the backend rename is live, remote sync flows will fail. Ensure coordinated rollout (or temporary dual-path fallback) during release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/api/remoteSync.ts` around lines 107 - 170, The client currently hard-switches all remote-sync endpoints which will break if the frontend deploys before the backend rename; update the API calls (e.g., saveConfig, deleteConfig, triggerCheck, getJobStatus, getHistory and the initial get) to implement a dual-path fallback: first attempt the new /remote-sync path and if that request returns a 404 (or a specific "not found" response) automatically retry the same request against the legacy path (e.g., the previous /remoteSync or whatever the old route is), or alternatively read the base remote-sync path from a runtime config/feature-flag and use that to choose the URL; ensure the retry preserves method, body and headers (Authorization) and surface the final response/error to callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/settings/page.tsx:
- Around line 2453-2462: Keep a temporary alias for the old hash by mapping
"#upstream-sync" to the new section id. In the component that returns the
section with id="remote-sync" (the JSX block in page.tsx), either add a hidden
anchor element with id="upstream-sync" next to the section or add a small
client-side effect (e.g., in a useEffect inside the same component) that checks
window.location.hash and, if it equals "#upstream-sync", calls
history.replaceState or scrolls/updates location.hash to "#remote-sync" so old
deep links continue to land on the same UI until you remove the alias.
In `@lib/hooks/useRemoteSync.ts`:
- Around line 10-15: The hook currently imports remoteSyncApi directly from
"@/lib/api/remoteSync"; update the import so remoteSyncApi is imported from
"@/lib/api/client" instead to follow the API boundary (keep other type imports
if needed from RemoteSync types but route the API object via the centralized
client). Locate the import list in useRemoteSync.ts and replace the
remoteSyncApi import source with the client entrypoint (e.g., import {
remoteSyncApi } from "@/lib/api/client"), then ensure any calls use the same
remoteSyncApi symbol and that error handling remains compatible with ApiError
from the client layer.
---
Outside diff comments:
In `@lib/api/notifications.ts`:
- Around line 12-24: The NotificationType union was tightened to remote_* names
which will silently degrade the UI if the backend still emits legacy upstream_*
values; add normalization at the API boundary in lib/api/notifications.ts by
implementing a mapping/normalizeNotificationType function (or run normalization
where notifications are parsed) that converts any upstream_* strings to their
corresponding remote_* equivalents (e.g., upstream_update_applied ->
remote_update_applied, upstream_update_available -> remote_update_available,
upstream_sync_error -> remote_sync_error) and return values typed as
NotificationType so downstream code (e.g., notification-bell rendering) always
sees the new remote_* names.
In `@lib/hooks/useRemoteSync.ts`:
- Around line 115-129: The polling loop currently only treats status.status ===
"complete" or "failed" as terminal; update the check in the block that
references status.status, pollTimerRef, jobIdRef, setIsChecking, setError, and
fetchData to also handle the "not_found" status as terminal: when status.status
is "not_found" clear and null out pollTimerRef.current, set jobIdRef.current =
null, setIsChecking(false), optionally setError with a descriptive message if no
error is present, and still await fetchData() so the UI refreshes and polling
stops.
- Around line 44-93: Replace the manual fetchData/useEffect + local useState for
config and history with `@tanstack/react-query` useQuery hooks: create a useQuery
for the config (key e.g. ['remoteSync','config', projectId]) that calls
remoteSyncApi.getConfig(projectId, accessToken) and sets enabled via the hook's
enabled option; create a separate useQuery for history (key e.g.
['remoteSync','history', projectId]) that calls
remoteSyncApi.getHistory(projectId, 20, accessToken) and depends on the config
query (enabled only if config exists). Remove the local useState management for
config/history and isLoading for those queries and map their loading/error
states to the query results; for job status polling, useQuery with
refetchInterval or the query's refetch to poll using jobIdRef (or a query keyed
by ['remoteSync','job', jobId]) instead of manual setInterval in pollTimerRef;
keep existing error handling by mapping query.error to the error state and
preserve enabled gating with the query's enabled option.
---
Nitpick comments:
In `@components/editor/RemoteSyncIndicator.tsx`:
- Around line 24-35: The component currently fetches server state imperatively
in RemoteSyncIndicator via a useEffect that calls remoteSyncApi.getConfig and
setConfig; replace that effect with a React Query-backed hook (either the
existing useRemoteSync or a slim useQuery) so the indicator uses shared
caching/invalidation. Implement a query such as useQuery(['remoteSync',
projectId], () => remoteSyncApi.getConfig(projectId, accessToken), { enabled:
!!projectId }) or call useRemoteSync(projectId, accessToken), and map a 404 to
null in the query's onError or queryFn so setConfig/useQuery data reflects null
on 404; remove the useEffect and local imperative fetch to ensure React Query
drives the state.
In `@lib/api/remoteSync.ts`:
- Around line 107-170: The client currently hard-switches all remote-sync
endpoints which will break if the frontend deploys before the backend rename;
update the API calls (e.g., saveConfig, deleteConfig, triggerCheck,
getJobStatus, getHistory and the initial get) to implement a dual-path fallback:
first attempt the new /remote-sync path and if that request returns a 404 (or a
specific "not found" response) automatically retry the same request against the
legacy path (e.g., the previous /remoteSync or whatever the old route is), or
alternatively read the base remote-sync path from a runtime config/feature-flag
and use that to choose the URL; ensure the retry preserves method, body and
headers (Authorization) and surface the final response/error to callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66207bc3-2507-49e9-b1bb-1e41569bba9f
📒 Files selected for processing (10)
app/projects/[id]/editor/page.tsxapp/projects/[id]/settings/page.tsxapp/settings/page.tsxcomponents/editor/RemoteSyncIndicator.tsxcomponents/editor/index.tscomponents/layout/notification-bell.tsxlib/api/client.tslib/api/notifications.tslib/api/remoteSync.tslib/hooks/useRemoteSync.ts
| return ( | ||
| <section | ||
| id="upstream-sync" | ||
| id="remote-sync" | ||
| className="mb-8 rounded-lg border border-slate-200 bg-white p-6 dark:border-slate-700 dark:bg-slate-800" | ||
| > | ||
| <div className="mb-4 flex items-center gap-2"> | ||
| <Download className="h-5 w-5 text-slate-500" /> | ||
| <h2 className="text-lg font-semibold text-slate-900 dark:text-white"> | ||
| Upstream Source Tracking | ||
| Sync from Remote | ||
| </h2> |
There was a problem hiding this comment.
Keep a temporary #upstream-sync alias.
Changing this section id breaks existing deep links from old notifications, bookmarks, or docs. A small alias or hash rewrite on load would let /settings#upstream-sync keep landing here while old links drain out.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/projects/`[id]/settings/page.tsx around lines 2453 - 2462, Keep a
temporary alias for the old hash by mapping "#upstream-sync" to the new section
id. In the component that returns the section with id="remote-sync" (the JSX
block in page.tsx), either add a hidden anchor element with id="upstream-sync"
next to the section or add a small client-side effect (e.g., in a useEffect
inside the same component) that checks window.location.hash and, if it equals
"#upstream-sync", calls history.replaceState or scrolls/updates location.hash to
"#remote-sync" so old deep links continue to land on the same UI until you
remove the alias.
| remoteSyncApi, | ||
| type RemoteSyncConfig, | ||
| type RemoteSyncConfigCreate, | ||
| type RemoteSyncConfigUpdate, | ||
| type SyncEvent, | ||
| } from "@/lib/api/upstreamSync"; | ||
| } from "@/lib/api/remoteSync"; |
There was a problem hiding this comment.
Import remoteSyncApi through lib/api/client.ts to follow the API boundary.
This hook currently imports API methods directly from @/lib/api/remoteSync; use the centralized client entrypoint for backend communication consistency.
Suggested diff
import { useState, useEffect, useCallback, useRef } from "react";
import {
- remoteSyncApi,
type RemoteSyncConfig,
type RemoteSyncConfigCreate,
type RemoteSyncConfigUpdate,
type SyncEvent,
} from "@/lib/api/remoteSync";
+import { remoteSyncApi } from "@/lib/api/client";As per coding guidelines, "All backend communication should go through lib/api/client.ts which provides type-safe API methods (api.get, api.post, etc.), domain-specific APIs (ontologyApi, classApi, projectOntologyApi), and automatic query parameter handling with error wrapping via ApiError".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks/useRemoteSync.ts` around lines 10 - 15, The hook currently imports
remoteSyncApi directly from "@/lib/api/remoteSync"; update the import so
remoteSyncApi is imported from "@/lib/api/client" instead to follow the API
boundary (keep other type imports if needed from RemoteSync types but route the
API object via the centralized client). Locate the import list in
useRemoteSync.ts and replace the remoteSyncApi import source with the client
entrypoint (e.g., import { remoteSyncApi } from "@/lib/api/client"), then ensure
any calls use the same remoteSyncApi symbol and that error handling remains
compatible with ApiError from the client layer.
damienriehl
left a comment
damienriehl
left a comment
There was a problem hiding this comment.
Perfectly symmetric rename (+108/-108). All files, notification types, aria-labels, and URL anchors consistently updated.
Closes #79 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update section headings, aria-labels, and descriptions to say "Sync from Remote" instead of "Remote Sync" for clarity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
damienriehl
force-pushed
the
fix/rename-upstream-to-remote
branch
from
9b7503c to
683707b
Compare
2 participants
Summary
upstreamSync.ts→remoteSync.ts,useUpstreamSync.ts→useRemoteSync.ts,UpstreamSyncIndicator.tsx→RemoteSyncIndicator.tsx/upstream-sync→/remote-syncUpstreamSync*→RemoteSync*upstream_*→remote_*#upstream-sync→#remote-syncCloses #79
Dependencies
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit