feat(web): add POST /api/connections/{id}/sync to trigger a connection sync over the public API - #1521
Closed
Harsh23Kashyap wants to merge 2 commits into
Closed
Conversation
…n sync over the public API Exposes the existing connection-sync worker call over the web app's public API. OWNER-gated, org-scoped (cross-org access returns 404, not 403). The new action lives in workerApi/actions.ts alongside the existing UI server action; the new route is a thin wrapper that validates the path param and unwraps the result. Without this endpoint, on-call scripts that need to force a re-sync must either drive the React UI (not scriptable) or hit the worker's internal port (not exposed publicly). With it, the public API covers read, list, and trigger — enough for a Prometheus → Slack → curl recovery flow. Refs sourcebot-dev#1520.
Contributor
WalkthroughAdds ChangesConnection sync API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncRoute
participant SyncAction
participant Database
participant Worker
Client->>SyncRoute: POST /api/connections/{id}/sync
SyncRoute->>SyncAction: triggerConnectionSync(id)
SyncAction->>Database: Check connection by id and orgId
SyncAction->>Worker: POST /api/sync-connection
Worker-->>SyncAction: Return jobId
SyncRoute-->>Client: 202 { jobId }
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/web/src/app/api/(server)/connections/[id]/sync/route.test.ts (1)
97-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
afterAllimport to the top import block.
import { afterAll } from 'vitest';is placed mid-file rather than grouped with the other vitest imports on line 1. Functionally harmless (ES imports are hoisted), but inconsistent with typical import ordering/lint conventions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/api/`(server)/connections/[id]/sync/route.test.ts around lines 97 - 100, Move the afterAll import into the existing top-level Vitest import block, keeping the afterAll cleanup callback that restores globalThis.fetch unchanged.packages/web/src/features/workerApi/actions.ts (2)
55-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a request timeout for the worker fetch.
triggerConnectionSyncnow exposes this un-timed-outfetch(same pattern as the pre-existingsyncConnection) to the public API. If the worker hangs instead of erroring, the request thread blocks indefinitely with no timeout/circuit breaker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/features/workerApi/actions.ts` around lines 55 - 59, Update the fetch call in triggerConnectionSync to enforce a request timeout using the project’s established timeout or abort mechanism, ensuring hung worker requests terminate while preserving the existing POST payload and headers.
44-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated worker-call/parse logic shared with
syncConnection.
triggerConnectionSync(lines 55-67) repeats the fetch/response.okcheck/jobIdZod parse already implemented insyncConnection(lines 17-35), differing only in the authorization/lookup step beforehand. A shared helper (e.g.callWorkerSyncConnection(connectionId)) called by both actions would remove the duplication and keep future changes (timeouts, retries, response shape) in one place.♻️ Proposed extraction
+async function callWorkerSyncConnection(connectionId: number) { + const response = await fetch(`${WORKER_API_URL}/api/sync-connection`, { + method: 'POST', + body: JSON.stringify({ connectionId }), + headers: { 'Content-Type': 'application/json' }, + }); + + if (!response.ok) { + return unexpectedError('Failed to sync connection'); + } + + const data = await response.json(); + const schema = z.object({ jobId: z.string() }); + return schema.parse(data); +} + export const syncConnection = async (connectionId: number) => sew(() => withAuth(({ role }) => withMinimumOrgRole(role, OrgRole.OWNER, async () => { - const response = await fetch(`${WORKER_API_URL}/api/sync-connection`, { - method: 'POST', - body: JSON.stringify({ - connectionId - }), - headers: { - 'Content-Type': 'application/json', - }, - }); - - if (!response.ok) { - return unexpectedError('Failed to sync connection'); - } - - const data = await response.json(); - const schema = z.object({ - jobId: z.string(), - }); - return schema.parse(data); + return callWorkerSyncConnection(connectionId); }) ) ); ... export const triggerConnectionSync = async (connectionId: number) => sew(() => withAuth(({ prisma, org, role }) => withMinimumOrgRole(role, OrgRole.OWNER, async () => { const connection = await prisma.connection.findFirst({ where: { id: connectionId, orgId: org.id }, select: { id: true }, }); if (!connection) { return notFound(`Connection ${connectionId} not found in org.`); } - - const response = await fetch(`${WORKER_API_URL}/api/sync-connection`, { - method: 'POST', - body: JSON.stringify({ connectionId }), - headers: { 'Content-Type': 'application/json' }, - }); - - if (!response.ok) { - return unexpectedError('Failed to sync connection'); - } - - const data = await response.json(); - const schema = z.object({ jobId: z.string() }); - return schema.parse(data); + return callWorkerSyncConnection(connectionId); }) ) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/features/workerApi/actions.ts` around lines 44 - 70, Extract the worker fetch, response-status validation, JSON parsing, and jobId schema validation from syncConnection into a shared helper such as callWorkerSyncConnection(connectionId). Update both syncConnection and triggerConnectionSync to call this helper while preserving their existing authorization and connection-lookup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/app/api/`(server)/connections/[id]/sync/route.ts:
- Around line 19-22: Replace queryParamsSchemaValidationError in the rawId
validation branch with the existing path-parameter schema validation helper,
passing idParsed.error and preserving serviceErrorResponse. Ensure the returned
400 uses the path-parameter error code and message documented for the id
segment.
---
Nitpick comments:
In `@packages/web/src/app/api/`(server)/connections/[id]/sync/route.test.ts:
- Around line 97-100: Move the afterAll import into the existing top-level
Vitest import block, keeping the afterAll cleanup callback that restores
globalThis.fetch unchanged.
In `@packages/web/src/features/workerApi/actions.ts`:
- Around line 55-59: Update the fetch call in triggerConnectionSync to enforce a
request timeout using the project’s established timeout or abort mechanism,
ensuring hung worker requests terminate while preserving the existing POST
payload and headers.
- Around line 44-70: Extract the worker fetch, response-status validation, JSON
parsing, and jobId schema validation from syncConnection into a shared helper
such as callWorkerSyncConnection(connectionId). Update both syncConnection and
triggerConnectionSync to call this helper while preserving their existing
authorization and connection-lookup behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cfa00cf7-cf4c-4a39-88dc-5dc2e774aea5
📒 Files selected for processing (8)
CHANGELOG.mddocs/api-reference/sourcebot-public.openapi.jsondocs/docs.jsonpackages/web/src/app/api/(server)/connections/[id]/sync/route.test.tspackages/web/src/app/api/(server)/connections/[id]/sync/route.tspackages/web/src/features/workerApi/actions.tspackages/web/src/openapi/publicApiDocument.tspackages/web/src/openapi/publicApiSchemas.ts
Comment on lines
+19
to
+22
| const idParsed = connectionIdParamSchema.safeParse(rawId); | ||
| if (!idParsed.success) { | ||
| return serviceErrorResponse(queryParamsSchemaValidationError(idParsed.error)); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the queryParamsSchemaValidationError implementation and whether a dedicated
# path-param error helper already exists in the codebase.
rg -n -A8 'export function queryParamsSchemaValidationError|export const queryParamsSchemaValidationError' packages/web/src/lib/serviceError.ts
rg -n 'pathParam|PathParam' packages/web/src/lib/serviceError.tsRepository: sourcebot-dev/sourcebot
Length of output: 552
🏁 Script executed:
#!/bin/bash
set -e
echo "== file outline =="
ast-grep outline 'packages/web/src/app/api/(server)/connections/[id]/sync/route.ts' --view expanded || true
echo "== relevant route text =="
cat -n 'packages/web/src/app/api/(server)/connections/[id]/sync/route.ts' | sed -n '1,180p'
echo "== error helpers references =="
rg -n 'queryParamsSchemaValidationError|queryParamsSchemaValidation|schemaValidationError|INVALID_QUERY_PARAMS|Invalid |path parameter' packages/web/src/openapi packages/web/src/app/api packages/web/src/lib/serviceError.ts | sed -n '1,220p'Repository: sourcebot-dev/sourcebot
Length of output: 8789
🏁 Script executed:
#!/bin/bash
set -e
echo "== OpenAPI invalid id schema references =="
rg -n -C 5 'Invalid `id` path parameter|id.*path|INVALID_QUERY_PARAMS|INVALID_PATH|queryParamsSchemaValidationError' packages/web/src/openapi docs/docs.json packages/web/src/app/api packages/web/src/lib/serviceError.ts | sed -n '1,260p'Repository: sourcebot-dev/sourcebot
Length of output: 23180
Use a path-parameter schema validation helper for this 400.
id is parsed from params.id, but queryParamsSchemaValidationError returns INVALID_QUERY_PARAMS and "Query params validation failed with: ...", while the OpenAPI response documents this 400 as "Invalid \id` path parameter."Add/return a path-parameter validation error instead so the client error code and message match theid` path segment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/web/src/app/api/`(server)/connections/[id]/sync/route.ts around
lines 19 - 22, Replace queryParamsSchemaValidationError in the rawId validation
branch with the existing path-parameter schema validation helper, passing
idParsed.error and preserving serviceErrorResponse. Ensure the returned 400 uses
the path-parameter error code and message documented for the id segment.
CodeRabbit finding: triggerConnectionSync repeated the fetch + ok check + jobId Zod parse already in syncConnection. Extracted both into callWorkerSyncConnection, a private helper that both actions now delegate to. Future changes to the worker call (timeouts, retries, response shape) live in one place. Also moves the afterAll import in route.test.ts to the top of the file so the test file's imports are grouped consistently with the rest of the suite. Skipped: the suggestion to add a request timeout to the worker fetch. The existing UI flow (syncConnection) has the same un-timed fetch, so adding a timeout only to the public-API action would create inconsistency. A future PR that adds a project-wide fetch helper should fix both at once. Refs sourcebot-dev#1521.
Author
|
All 3 CodeRabbit nitpicks addressed in b02648bd:
19/19 web tests pass; no new TypeScript errors. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
POST /api/connections/{id}/syncso on-call scripts can force a connection sync without going through the web UI. The new action lives inworkerApi/actions.tsand is org-scoped (cross-org access returns 404, not 403). The route is a thin wrapper that validates the path param, delegates auth/role/scope to the action, and returns 202 with{ jobId }.Fixes #1520.
What it returns
jobIdis the newConnectionSyncJobrow's id. PollGET /api/connections/123to see it move from PENDING/IN_PROGRESS to COMPLETED/FAILED (the detail endpoint from PR #1519 already exposeslatestJoband the recent-jobs list, so the polling side is already covered).Design decisions
syncConnectionserver action that the UI uses.where: { id, orgId: org.id }lookup before calling the worker. A 403 here would tell an attacker that the id exists in some other org. The new path mirrors PR feat(web): add GET /api/connections admin endpoint for monitoring connection sync state #1517's list endpoint and PR feat(web): add GET /api/connections/{id} detail endpoint #1519's detail endpoint.syncConnectionaction. The UI uses the existing action; the new public-API action lives next to it but adds the org-scope check. Both share the samewithAuth + withMinimumOrgRole(OWNER)wrapping pattern and the same worker call.createJobsdoesn't dedupe — each call creates a newConnectionSyncJobrow. Adding a conflict check would be a behavioral change that affects the UI too. Operators can already see in-flight count viaGET /api/connections/{id}and decide whether to wait. Punted to a future PR if it becomes a real problem.Test coverage
8 vitest cases in
route.test.ts:idis not a positive integeridis zero or negativeorgIdAll 19 web tests pass (8 new + 11 from the prior list endpoint's test). No new TypeScript errors.
OpenAPI / docs
publicSyncConnectionResponseSchematopublicApiSchemas.tswith a description pointing operators at the detail endpoint for polling.POST /api/connections/{id}/syncunder thesystemTaginpublicApiDocument.tswith a 202/400/401/403/404/500 response matrix.docs/api-reference/sourcebot-public.openapi.jsonviayarn openapi:generate.docs/docs.jsonunder the System group.[Unreleased] → Addedlinking to this PR.Risks
/api/health/readyprobe (PR feat(web): add /api/health/ready endpoint with dependency health checks #1507) to detect this.Future work
?force=truequery param to disambiguate "trigger" from "force-re-sync-if-stale" (the latter would skip the worker entirely if the latest job is recent enough).POST /api/connections/sync-allfor org-wide force-re-sync; punted because it has different semantics (per-connection throttling, partial failure reporting) that warrant a separate endpoint.Note
Medium Risk
OWNER-only but can enqueue repeated sync jobs (no API rate limit or in-flight dedup), increasing worker/code-host load; org scoping and 404-on-cross-org reduce authorization leakage risk.
Overview
Adds
POST /api/connections/{id}/syncso org OWNERs can enqueue a connection sync from scripts or integrations without the settings UI. Successful calls return 202 with{ jobId }for polling (e.g. connection detail).The route validates the path
id, then calls newtriggerConnectionSync, which looks up the connection withwhere: { id, orgId }(cross-org IDs return 404, not 403) before posting to the worker’s/api/sync-connection. The existing UIsyncConnectionaction now sharescallWorkerSyncConnectionfor the worker fetch/parse logic.OpenAPI, Mintlify nav, and changelog are updated; 8 Vitest cases cover auth, roles, validation, org scoping, happy path, and worker failures.
Reviewed by Cursor Bugbot for commit b02648b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
POST /api/connections/{id}/syncto manually trigger an asynchronous connection resynchronization.202with{ jobId }for polling progress.PublicSyncConnectionResponseschema.