feat: allow users to trigger a full data re-sync #64
Conversation
WalkthroughAdds a sync-batch feature (schema, services, validation, resolvers, hooks, UI) and integrates batch lifecycle into GitHub sync flows/workers; replaces CardOpenableSettings with CardSetting and updates related UI, icons, and routing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-01-30T06:09:36.043ZApplied to files:
🧬 Code graph analysis (1)apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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: 3
🧹 Nitpick comments (9)
apps/api/src/app/sync-batch/resolvers/transformers/sync-batch.transformer.ts (1)
4-13: Remove redundantsinceDaysAgoassignment.Line 11 explicitly assigns
sinceDaysAgo: syncBatch.sinceDaysAgo, but this is redundant since the spread operator on line 8 (...syncBatch) already includes all properties fromsyncBatch, includingsinceDaysAgo.🔎 Proposed refactor
export const transformSyncBatch = ( syncBatch: SyncBatch ): Omit<ApiSyncBatch, "progress"> => { return { ...syncBatch, scheduledAt: syncBatch.scheduledAt.toISOString(), finishedAt: syncBatch.finishedAt?.toISOString(), - sinceDaysAgo: syncBatch.sinceDaysAgo, }; };apps/api/src/app/sync-batch/resolvers/queries/sync-batch-progress.query.ts (1)
6-18: Rename export to reflect that it resolvesSyncBatch.progress.The export name
workspaceLastSyncBatchQueryis misleading. This resolver handles theprogressfield on theSyncBatchtype, not a workspace query. Consider renaming to something likesyncBatchProgressResolverorsyncBatchFieldResolversfor clarity.🔎 Proposed refactor
-export const workspaceLastSyncBatchQuery = createFieldResolver("SyncBatch", { +export const syncBatchFieldResolvers = createFieldResolver("SyncBatch", { progress: async ({ id }) => {apps/web/src/components/card-setting/card-setting.tsx (1)
13-20: Rename interface to match component name.The interface is named
CardOpenableSettingsPropsbut the component is namedCardSetting. For consistency and clarity, consider renaming the interface toCardSettingProps.🔎 Proposed refactor
-interface CardOpenableSettingsProps extends Omit<PaperProps, "left" | "right"> { +interface CardSettingProps extends Omit<PaperProps, "left" | "right"> { left: React.ReactNode; right?: React.ReactNode; title: string; description: string; href?: string; isLoading?: boolean; } export const CardSetting = ({ left, right, title, description, href, isLoading, ...props -}: CardOpenableSettingsProps) => { +}: CardSettingProps) => {apps/web/src/api/sync-batch.api.ts (1)
64-68: Consider invalidating the sync-progress query after scheduling.After scheduling a sync batch, the
useWorkspaceLastSyncBatchQueryshould refetch to show the newly scheduled batch and its progress. Currently, only["workspace", ..., "settings"]is invalidated, but the query uses["workspace", ..., "sync-progress"].🔎 Proposed fix
onSettled: (_, __, args) => { queryClient.invalidateQueries({ queryKey: ["workspace", args.input.workspaceId, "settings"], }); + queryClient.invalidateQueries({ + queryKey: ["workspace", args.input.workspaceId, "sync-progress"], + }); },apps/web/src/app/settings/workspace/resync/page.tsx (2)
35-42: Aggressive polling interval may impact performance.The 1-second polling interval could generate significant network traffic, especially for workspaces with slow sync operations. Consider using a longer interval (e.g., 3-5 seconds) or enabling polling only when
isInProgressis true.🔎 Conditional polling suggestion
const { data: lastSyncBatchData, isLoading } = useWorkspaceLastSyncBatchQuery( { workspaceId: workspace.id, }, { - refetchInterval: 1000, + refetchInterval: isInProgress ? 2000 : false, }, );Note: This requires lifting
isInProgresscalculation or using a separate state to track if we should poll.
56-77: Async handler doesn't needasync/awaitwith mutation callbacks.The
handleResyncfunction usesasync/awaitbut the mutation is called with callbacks (onSuccess,onError). Theawaithas no effect here sincemutatedoesn't return a promise. Either usemutateAsyncwith try/catch or keepmutatewith callbacks but removeasync/await.🔎 Option 1: Remove unnecessary async/await
- const handleResync = async () => { - await scheduleSyncBatch( + const handleResync = () => { + scheduleSyncBatch( {apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (2)
24-24: Remove inline development note.The comment "use date fns throughout the codebase" appears to be a development note rather than documentation. Consider removing it.
🔎 Proposed fix
- // if last batch was scheduled less than 24 hours ago, throw an error - use date fns throughout the codebase + // Enforce 24-hour rate limit between sync operations
39-45: Consider adding a user-facing message for the no repositories error.The validation error for no syncable repositories doesn't include a
userFacingMessage. Based on the error provider in the frontend (lines 14-22 oferror-message.provider.ts),INPUT_VALIDATION_FAILEDerrors display theuserFacingMessageto users. Without it, users will see the generic "Validation error, please fix the input."🔎 Proposed fix
if (repositories.length === 0) { throw new InputValidationException("No syncable repositories found", { + userFacingMessage: "No syncable repositories found. Archived, fork, and mirror repositories are excluded.", validationErrors: { repositories: "No syncable repositories found", }, }); }apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
18-32: Clarify/defaultsinceDaysAgohandling inscheduleSyncBatch
sinceDaysAgois now required inCreateSyncBatchArgsand written straight into the DB. If any existing callers treat it as optional, this could surface as a runtime error or an unintendedNULL/default at the DB layer.Consider either:
- Making
sinceDaysAgostrictly required at the type level everywhere you callscheduleSyncBatch, or- Providing a safe default here, e.g.:
export const scheduleSyncBatch = async ({ workspaceId, scheduledAt, metadata, sinceDaysAgo = DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO, }: CreateSyncBatchArgs) => { /* ... */ }and still validating user-provided values (e.g., non‑negative, reasonable upper bound) in the resolver or service that constructs
CreateSyncBatchArgs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
apps/api/src/app/github/services/github-repository-pull-requests.service.tsapps/api/src/app/github/services/github-repository.service.tsapps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.tsapps/api/src/app/sync-batch/resolvers/queries/last-sync-batch.query.tsapps/api/src/app/sync-batch/resolvers/queries/sync-batch-progress.query.tsapps/api/src/app/sync-batch/resolvers/transformers/sync-batch.transformer.tsapps/api/src/app/sync-batch/services/sync-batch.service.tsapps/api/src/app/sync-batch/services/sync-batch.types.tsapps/api/src/app/sync-batch/sync-batch.schema.tsapps/api/src/app/workspaces/resolvers/workspaces.schema.tsapps/web/src/api/sync-batch.api.tsapps/web/src/app/humans/teams/[id]/alerts/page.tsxapps/web/src/app/humans/teams/[id]/health-and-performance/page.tsxapps/web/src/app/humans/teams/[id]/pull-requests/page.tsxapps/web/src/app/settings/components/subnav-settings/subnav-settings.tsxapps/web/src/app/settings/data-ingestion/page.tsxapps/web/src/app/settings/pull-request-settings/page.tsxapps/web/src/app/settings/workspace/page.tsxapps/web/src/app/settings/workspace/resync/page.tsxapps/web/src/components/card-openable-settings/card-openable-settings.tsxapps/web/src/components/card-openable-settings/index.tsapps/web/src/components/card-setting/card-setting.tsxapps/web/src/components/card-setting/index.tsapps/web/src/components/icon-info/icon-info.tsxapps/web/src/routes.tsxpackages/graphql-types/api.tspackages/graphql-types/frontend/gql.tspackages/graphql-types/frontend/graphql.ts
💤 Files with no reviewable changes (2)
- apps/web/src/components/card-openable-settings/index.ts
- apps/web/src/components/card-openable-settings/card-openable-settings.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:51:17.244Z
Learning: In the sync batch progress system, the "done" count represents "attempted sync" not "successfully completed sync". It's used for a user-facing progress bar to indicate how many PRs have been attempted, allowing users to know when they can start using the dashboard. Failed jobs get retried but don't affect the progress counter, and the early increment (after workspace lookup, before GitHub API calls) is intentional to show immediate progress to users.
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:48:34.731Z
Learning: In the sync batch system, `failCount === 0` indicates the first attempt at processing a job, not that the job won't fail. The increment of "done" progress happens early in the process, after finding the workspace but before GitHub API calls that could still fail.
📚 Learning: 2025-06-02T01:48:34.731Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:48:34.731Z
Learning: In the sync batch system, `failCount === 0` indicates the first attempt at processing a job, not that the job won't fail. The increment of "done" progress happens early in the process, after finding the workspace but before GitHub API calls that could still fail.
Applied to files:
apps/api/src/app/github/services/github-repository-pull-requests.service.ts
📚 Learning: 2025-06-02T01:51:17.244Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:51:17.244Z
Learning: In the sync batch progress system, the "done" count represents "attempted sync" not "successfully completed sync". It's used for a user-facing progress bar to indicate how many PRs have been attempted, allowing users to know when they can start using the dashboard. Failed jobs get retried but don't affect the progress counter, and the early increment (after workspace lookup, before GitHub API calls) is intentional to show immediate progress to users.
Applied to files:
apps/api/src/app/github/services/github-repository-pull-requests.service.ts
📚 Learning: 2025-01-30T06:09:36.043Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 44
File: apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts:30-30
Timestamp: 2025-01-30T06:09:36.043Z
Learning: GraphQL mutations in this codebase already handle errors and show user-facing error messages. Additional try-catch blocks in mutation resolvers are not needed as errors are properly propagated and handled at the GraphQL layer.
Applied to files:
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts
🧬 Code graph analysis (17)
apps/api/src/app/github/services/github-repository-pull-requests.service.ts (1)
apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
maybeFinishSyncBatch(150-191)
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (6)
apps/api/src/lib/graphql.ts (1)
createMutationResolver(11-19)apps/api/src/lib/logger.ts (1)
logger(21-76)apps/api/src/app/billing/services/billing.service.ts (1)
protectWithPaywall(142-156)apps/api/src/app/sync-batch/services/sync-batch.service.ts (4)
findLastSyncBatch(264-273)findSyncableRepositories(253-262)scheduleSyncBatch(20-42)DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO(18-18)apps/api/src/app/errors/exceptions/input-validation.exception.ts (1)
InputValidationException(11-26)apps/api/src/app/sync-batch/resolvers/transformers/sync-batch.transformer.ts (1)
transformSyncBatch(4-13)
apps/web/src/app/settings/data-ingestion/page.tsx (2)
apps/web/src/components/page-container/page-container.tsx (1)
PageContainer(11-29)apps/web/src/components/breadcrumbs/breadcrumbs.tsx (1)
Breadcrumbs(22-67)
apps/web/src/components/icon-info/icon-info.tsx (1)
packages/graphql-types/api.ts (1)
Omit(11-11)
apps/web/src/app/settings/workspace/resync/page.tsx (5)
apps/web/src/api/sync-batch.api.ts (2)
useWorkspaceLastSyncBatchQuery(17-40)useScheduleSyncBatchMutation(42-70)apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
scheduleSyncBatch(20-42)apps/web/src/providers/notification.provider.tsx (2)
showSuccessNotification(37-41)showErrorNotification(22-26)apps/web/src/providers/error-message.provider.ts (1)
getErrorMessage(4-31)apps/web/src/providers/date.provider.ts (1)
formatLocaleDate(60-78)
apps/web/src/app/settings/pull-request-settings/page.tsx (2)
apps/web/src/components/card-setting/card-setting.tsx (1)
CardSetting(22-71)apps/web/src/components/card-setting/index.ts (1)
CardSetting(1-1)
apps/api/src/app/github/services/github-repository.service.ts (1)
apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO(18-18)
apps/web/src/app/settings/workspace/page.tsx (3)
apps/web/src/api/sync-batch.api.ts (1)
useWorkspaceLastSyncBatchQuery(17-40)apps/web/src/components/card-setting/card-setting.tsx (1)
CardSetting(22-71)apps/web/src/providers/date.provider.ts (1)
formatLocaleDate(60-78)
apps/web/src/app/humans/teams/[id]/alerts/page.tsx (2)
apps/web/src/components/card-setting/card-setting.tsx (1)
CardSetting(22-71)apps/web/src/components/card-setting/index.ts (1)
CardSetting(1-1)
apps/api/src/app/sync-batch/resolvers/transformers/sync-batch.transformer.ts (2)
packages/graphql-types/api.ts (2)
SyncBatch(922-934)Omit(11-11)packages/graphql-types/frontend/graphql.ts (1)
SyncBatch(919-931)
apps/web/src/api/sync-batch.api.ts (3)
packages/graphql-types/frontend/graphql.ts (4)
WorkspaceLastSyncBatchQueryVariables(1640-1642)WorkspaceLastSyncBatchQuery(1645-1645)ScheduleSyncBatchMutation(1652-1652)MutationScheduleSyncBatchArgs(608-610)packages/graphql-types/frontend/gql.ts (1)
graphql(348-350)packages/graphql-types/api.ts (1)
MutationScheduleSyncBatchArgs(611-613)
apps/api/src/app/sync-batch/resolvers/queries/last-sync-batch.query.ts (5)
apps/api/src/app/sync-batch/resolvers/queries/sync-batch-progress.query.ts (1)
workspaceLastSyncBatchQuery(6-18)apps/api/src/lib/graphql.ts (1)
createFieldResolver(21-30)apps/api/src/lib/logger.ts (1)
logger(21-76)apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
findLastSyncBatch(264-273)apps/api/src/app/sync-batch/resolvers/transformers/sync-batch.transformer.ts (1)
transformSyncBatch(4-13)
apps/web/src/routes.tsx (1)
apps/web/src/app/settings/workspace/resync/page.tsx (1)
WorkspaceResyncPage(31-203)
apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
apps/api/src/prisma.ts (1)
getPrisma(23-49)
apps/web/src/components/card-setting/card-setting.tsx (1)
packages/graphql-types/api.ts (1)
Omit(11-11)
packages/graphql-types/api.ts (3)
packages/graphql-types/frontend/graphql.ts (4)
SyncBatch(919-931)ScheduleSyncBatchInput(901-906)Scalars(11-24)Maybe(3-3)apps/api/src/lib/type-helpers.ts (1)
DeepPartial(25-33)packages/graphql-types/shared.ts (1)
GraphQLContext(7-27)
packages/graphql-types/frontend/graphql.ts (1)
packages/graphql-types/api.ts (4)
SyncBatch(922-934)MutationScheduleSyncBatchArgs(611-613)ScheduleSyncBatchInput(904-909)Maybe(4-4)
🔇 Additional comments (21)
apps/web/src/app/humans/teams/[id]/health-and-performance/page.tsx (1)
9-9: Good icon choice for size measurement.IconRuler is more semantically appropriate than IconAspectRatio for representing size distribution, as it directly conveys measurement rather than proportions.
Also applies to: 37-37
apps/web/src/app/humans/teams/[id]/pull-requests/page.tsx (1)
23-23: LGTM! Improved icon semantics.The icon change from
IconAspectRatiotoIconRuleris semantically more appropriate for representing "Size" in the filter UI. This improves the visual clarity and consistency of the interface.Also applies to: 142-142
apps/web/src/app/settings/components/subnav-settings/subnav-settings.tsx (2)
7-7: LGTM! Icon and label updates improve clarity.The icon change from
IconUserstoIconUserHexagonand the label update from "Members" to "Users" are clear cosmetic improvements that enhance the UI consistency.Also applies to: 56-56, 63-63
31-31: Correct approach for nested route navigation.The change from exact path matching to prefix matching with
startsWith()is the right pattern for this navigation structure. Nested routes like/settings/workspace/resyncare already implemented, and this change ensures parent navigation items remain highlighted when viewing child pages—exactly as intended.The changes across lines 31, 39, 48, 68, and 76 are all consistent and appropriate.
apps/web/src/components/icon-info/icon-info.tsx (1)
7-7: LGTM! Type expansion is backward compatible.The change to allow
ReactNodein addition tostringfor the tooltip prop is backward compatible and aligns well with Mantine's Tooltip component, which already accepts ReactNode for its label prop. This enables richer tooltip content while maintaining existing functionality.Also applies to: 11-11
apps/web/src/app/settings/data-ingestion/page.tsx (1)
1-27: LGTM! Clean page component implementation.The DataIngestionPage component follows established patterns in the codebase with proper breadcrumb navigation, page container usage, and nested route support via Outlet. The structure is clean and maintainable.
apps/web/src/app/humans/teams/[id]/alerts/page.tsx (1)
5-5: LGTM! Component replacement is consistent.The replacement of
CardOpenableSettingswithCardSettingis straightforward and maintains the same props and behavior. This aligns with the broader UI refactoring effort across the codebase.Also applies to: 35-35, 63-63
apps/api/src/app/github/services/github-repository.service.ts (1)
10-13: LGTM! Good use of named constant.Replacing a hardcoded value with
DEFAULT_SYNC_BATCH_SINCE_DAYS_AGOimproves maintainability and makes the default behavior explicit. The 365-day default is reasonable for syncing historical data during repository onboarding.Also applies to: 43-43
apps/web/src/components/card-setting/index.ts (1)
1-1: LGTM! Standard barrel export pattern.The barrel export follows the established pattern in the codebase for component exports, enabling clean imports.
apps/api/src/app/github/services/github-repository-pull-requests.service.ts (1)
16-19: LGTM! Empty result batch completion handling added.The addition of
maybeFinishSyncBatchensures that sync batches complete properly even when no pull requests are found for a repository. This correctly handles the edge case where a repository has no PRs matching the sync criteria.Also applies to: 48-51
apps/web/src/routes.tsx (1)
73-73: LGTM! Nested route configuration is correct.The new
/settings/workspace/resyncroute is properly nested under/settings/workspace, enabling the resync modal UI to be rendered as a child route.Also applies to: 205-210
apps/api/src/app/sync-batch/resolvers/queries/last-sync-batch.query.ts (1)
7-23: LGTM! Well-structured field resolver.The resolver correctly fetches the last sync batch for a workspace, handles the null case appropriately, and transforms the result using the dedicated transformer.
apps/web/src/app/settings/pull-request-settings/page.tsx (1)
4-4: LGTM! Component and icon updates are appropriate.The refactor from
CardOpenableSettingstoCardSettingand the icon change fromIconAspectRatiotoIconRulerare both semantically appropriate for the "Size Calculation" setting.Also applies to: 6-6, 19-30
apps/web/src/app/settings/workspace/page.tsx (1)
30-36: LGTM! Data ingestion UI implemented correctly.The integration of
useWorkspaceLastSyncBatchQueryand the new "Data Ingestion" section provides clear visibility into the sync status. The date formatting withformatLocaleDateandparseISOis correct, and the fallback text is appropriate. The<Outlet />enables the nested resync route to render properly.Also applies to: 65-95, 130-130
apps/web/src/api/sync-batch.api.ts (1)
17-40: LGTM!The query hook is well-structured with proper typing and query key organization.
apps/web/src/app/settings/workspace/resync/page.tsx (1)
148-179: LGTM!The in-progress state UI is well implemented with clear messaging, proper date formatting, and an animated progress bar with a minimum value for visual feedback. The tooltip showing exact percentage is a nice UX touch.
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (1)
14-58: LGTM!The mutation implementation is solid with proper paywall protection, rate limiting, input validation, and clamping of
sinceDaysAgo. The use ofdate-fnsfor date comparisons and the transformer pattern are consistent with the codebase. Based on learnings, GraphQL errors are properly handled at the framework layer.packages/graphql-types/frontend/gql.ts (1)
63-64: LGTM!The generated GraphQL type definitions for
WorkspaceLastSyncBatchquery andScheduleSyncBatchmutation are correctly added and consistent with the new API surface.Also applies to: 283-290
packages/graphql-types/frontend/graphql.ts (1)
559-570: SyncBatch frontend schema additions look consistent and codegen-safeThe added
ScheduleSyncBatchInput,SyncBatch,Workspace.lastSyncBatch, and theWorkspaceLastSyncBatch/ScheduleSyncBatchoperation types and documents all line up with the backend API shapes and existing scalar conventions. Nothing stands out as inconsistent or risky here, especially given this file is generated.Also applies to: 608-610, 901-907, 919-931, 1144-1145, 1640-1653, 1805-1806
apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
253-273: New SyncBatch helpers (findSyncableRepositories,findLastSyncBatch) are straightforward and correctBoth helpers use
getPrisma(workspaceId)with explicitworkspaceIdpredicates and reasonable filters (exclude forks, mirrors, archived repositories; pick last batch byscheduledAt desc). They fit well with the existing service style and should be easy to reuse from resolvers.packages/graphql-types/api.ts (1)
562-573: SyncBatch GraphQL API surface and resolver typings are consistentThe additions of
ScheduleSyncBatchInput,SyncBatch,Mutation.scheduleSyncBatch, andWorkspace.lastSyncBatch, plus their entries inResolversTypes,ResolversParentTypes,SyncBatchResolvers,MutationResolvers, andWorkspaceResolvers, all look coherent and match the frontend/generated types and service behavior (including non‑nullprogress). No mismatches or schema/typing issues stand out here. Based on learnings, this keeps existing sync‑progress semantics intact.Also applies to: 611-614, 904-910, 922-935, 1147-1158, 1440-1445, 1550-1555, 1856-1867, 1991-1999, 2151-2163
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/deployment/services/deployment.service.ts (1)
87-148: Fix cursor-based pagination: cursor field must align with orderBy field.The cursor is based on
id(line 94), but the ordering is now bydeployedAt(line 102). This mismatch can cause pagination to skip or duplicate records when the chronological order ofdeployedAtdiffers from theidsequence.🔎 Proposed fixes
Option 1 (Recommended): Use compound ordering with
deployedAtprimary andidsecondaryThis maintains chronological ordering while ensuring deterministic, stable pagination:
orderBy: { - deployedAt: "desc", + deployedAt: "desc", + id: "desc", },Note: Prisma supports arrays for
orderByto achieve compound sorting. If the current version doesn't support object notation with multiple fields, use:- orderBy: { - deployedAt: "desc", - }, + orderBy: [ + { deployedAt: "desc" }, + { id: "desc" }, + ],Option 2: Revert to ID-based ordering
If chronological ordering by
deployedAtisn't essential:orderBy: { - deployedAt: "desc", + id: "desc", },
🧹 Nitpick comments (2)
apps/api/src/app/sync-batch/services/sync-batch.validation.ts (1)
4-7: Consider tightening numeric constraints to integers.Both
workspaceIdandsinceDaysAgolikely expect positive integers. The current schema allows decimals (e.g.,sinceDaysAgo: 1.5) and non-positive values forworkspaceId(e.g.,0or-1).🔎 Proposed fix
export const scheduleSyncBatchValidationSchema = z.object({ - workspaceId: z.number(), - sinceDaysAgo: z.number().min(1).max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO), + workspaceId: z.number().int().positive(), + sinceDaysAgo: z.number().int().min(1).max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO), });apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (1)
52-60: Redundant clamping ofsinceDaysAgo.The validation schema already enforces
.max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO), making theMath.minon line 55 redundant. You could simplify to just usesinceDaysAgodirectly, or keep this as intentional defense-in-depth.🔎 Proposed simplification
const syncBatch = await scheduleSyncBatch({ workspaceId, scheduledAt: new Date(), - sinceDaysAgo: Math.min(sinceDaysAgo, DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO), + sinceDaysAgo, metadata: { isOnboarding: false, repositories: repositories.map((repository) => repository.name), }, });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/app/deployment/services/deployment.service.tsapps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.tsapps/api/src/app/sync-batch/services/sync-batch.validation.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:51:17.244Z
Learning: In the sync batch progress system, the "done" count represents "attempted sync" not "successfully completed sync". It's used for a user-facing progress bar to indicate how many PRs have been attempted, allowing users to know when they can start using the dashboard. Failed jobs get retried but don't affect the progress counter, and the early increment (after workspace lookup, before GitHub API calls) is intentional to show immediate progress to users.
📚 Learning: 2024-12-01T02:31:20.571Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 33
File: apps/api/src/app/workspaces/services/workspace-settings.types.ts:5-11
Timestamp: 2024-12-01T02:31:20.571Z
Learning: You prefer using per-field validation in `zod` schemas (e.g., in `apps/api/src/app/workspaces/services/workspace-settings.types.ts`) over cross-field validation using `.refine()`, to allow displaying validation errors below each input in forms.
Applied to files:
apps/api/src/app/sync-batch/services/sync-batch.validation.ts
📚 Learning: 2025-01-30T06:09:36.043Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 44
File: apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts:30-30
Timestamp: 2025-01-30T06:09:36.043Z
Learning: GraphQL mutations in this codebase already handle errors and show user-facing error messages. Additional try-catch blocks in mutation resolvers are not needed as errors are properly propagated and handled at the GraphQL layer.
Applied to files:
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts
🧬 Code graph analysis (1)
apps/api/src/app/sync-batch/services/sync-batch.validation.ts (1)
apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO(18-18)
🔇 Additional comments (2)
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (2)
27-40: LGTM!The 24-hour cooldown logic correctly prevents abuse while providing a clear user-facing error message.
16-64: LGTM!The mutation follows a clean structure with proper validation, paywall protection, cooldown enforcement, and repository validation before scheduling. Error handling is appropriate for the GraphQL layer. Based on learnings, errors are properly propagated at the GraphQL level.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/src/app/sync-batch/services/sync-batch.validation.ts (1)
4-7: Consider reordering validators for better logical flow.The validator chain
.min(1).max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO).int()is functionally correct, but conventionally type refinements like.int()are placed before range checks for clearer intent:.int().min(1).max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO).🔎 Proposed refactor
export const scheduleSyncBatchValidationSchema = z.object({ workspaceId: z.number(), - sinceDaysAgo: z.number().min(1).max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO).int(), + sinceDaysAgo: z.number().int().min(1).max(DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO), });apps/web/src/app/settings/workspace/resync/page.tsx (1)
166-172: Consider the UX implications of the 10% minimum progress.The progress bar displays
Math.max(10, lastSyncBatch.progress)to ensure visibility, but this creates a discrepancy when actual progress is 0-9%. The tooltip shows the true percentage, but users might be confused seeing 10% on the bar when actual progress is lower. Consider whether this minimum is necessary, or if adding a visual indicator (e.g., pulsing animation for 0-9%) would be clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.tsapps/api/src/app/sync-batch/resolvers/queries/sync-batch-progress.query.tsapps/api/src/app/sync-batch/services/sync-batch.validation.tsapps/web/src/app/settings/workspace/resync/page.tsxapps/web/src/components/card-setting/card-setting.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:51:17.244Z
Learning: In the sync batch progress system, the "done" count represents "attempted sync" not "successfully completed sync". It's used for a user-facing progress bar to indicate how many PRs have been attempted, allowing users to know when they can start using the dashboard. Failed jobs get retried but don't affect the progress counter, and the early increment (after workspace lookup, before GitHub API calls) is intentional to show immediate progress to users.
📚 Learning: 2024-12-01T02:31:20.571Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 33
File: apps/api/src/app/workspaces/services/workspace-settings.types.ts:5-11
Timestamp: 2024-12-01T02:31:20.571Z
Learning: You prefer using per-field validation in `zod` schemas (e.g., in `apps/api/src/app/workspaces/services/workspace-settings.types.ts`) over cross-field validation using `.refine()`, to allow displaying validation errors below each input in forms.
Applied to files:
apps/api/src/app/sync-batch/services/sync-batch.validation.ts
📚 Learning: 2025-01-30T06:09:36.043Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 44
File: apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts:30-30
Timestamp: 2025-01-30T06:09:36.043Z
Learning: GraphQL mutations in this codebase already handle errors and show user-facing error messages. Additional try-catch blocks in mutation resolvers are not needed as errors are properly propagated and handled at the GraphQL layer.
Applied to files:
apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts
🧬 Code graph analysis (3)
apps/api/src/app/sync-batch/services/sync-batch.validation.ts (1)
apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
DEFAULT_SYNC_BATCH_SINCE_DAYS_AGO(18-18)
apps/api/src/app/sync-batch/resolvers/queries/sync-batch-progress.query.ts (3)
apps/api/src/lib/graphql.ts (1)
createFieldResolver(21-30)apps/api/src/lib/logger.ts (1)
logger(21-76)apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
getSyncBatchProgress(65-109)
apps/web/src/components/card-setting/card-setting.tsx (2)
packages/graphql-types/api.ts (1)
Omit(11-11)apps/web/src/components/card-setting/index.ts (1)
CardSetting(1-1)
🔇 Additional comments (9)
apps/web/src/components/card-setting/card-setting.tsx (3)
1-11: LGTM!The imports are well-organized and all necessary for the component implementation.
13-20: LGTM!The interface is well-designed with proper type safety. Omitting
leftandrightfromPaperPropscorrectly avoids naming conflicts with the custom props.
22-71: LGTM! Well-structured component with good loading state handling.The component implementation is clean and follows React best practices:
- Loading states consistently use Skeleton components
- Navigation is properly disabled during loading
- Conditional Anchor wrapper provides correct semantics for clickable cards
- Type safety is maintained throughout
The
grow-on-hoverCSS class (line 35) appears to be a global style. Ensure this class is defined in your global styles.apps/api/src/app/sync-batch/resolvers/queries/sync-batch-progress.query.ts (1)
6-18: LGTM!The resolver implementation is clean and follows established patterns. The ID validation provides appropriate error handling, and extracting only the
progressfield from the service response aligns with the GraphQL schema design.apps/web/src/app/settings/workspace/resync/page.tsx (2)
35-42: Verify the 3-second polling interval is intentional.The
refetchInterval: 3000creates a poll every 3 seconds to update the sync progress. While this provides a responsive UI, it generates frequent requests. Consider whether this interval balances UX needs with server load appropriately, or if a longer interval (e.g., 5-10 seconds) would be acceptable.
49-50: LGTM!The
isInProgresscalculation correctly determines sync status based on the presence ofscheduledAtand absence offinishedAt.apps/api/src/app/sync-batch/resolvers/mutations/schedule-sync-batch.mutation.ts (3)
19-24: LGTM!Input validation and paywall protection are correctly ordered and implemented.
26-38: LGTM!The 24-hour cooldown logic is correctly implemented. The
isAftercheck properly validates thatlastSyncBatch.scheduledAtis not within the last 24 hours, and provides a clear user-facing error message.
40-58: LGTM!The repository validation and sync batch scheduling logic is well-implemented. The check for syncable repositories prevents unnecessary operations, and the metadata structure appropriately captures context with
isOnboarding: falseand repository names.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/app/github/services/github-repository-pull-requests.service.tsapps/api/src/app/github/workers/github-sync-pull-request.worker.tsapps/api/src/app/sync-batch/services/sync-batch.service.tsapps/api/src/app/sync-batch/sync-batch.schema.tsapps/web/src/api/sync-batch.api.tsapps/web/src/app/settings/workspace/page.tsxapps/web/src/app/settings/workspace/resync/page.tsxpackages/graphql-types/api.tspackages/graphql-types/frontend/graphql.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/api/sync-batch.api.ts
- apps/web/src/app/settings/workspace/page.tsx
- apps/api/src/app/github/services/github-repository-pull-requests.service.ts
- apps/api/src/app/sync-batch/services/sync-batch.service.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T01:48:34.731Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:48:34.731Z
Learning: In the sync batch system, `failCount === 0` indicates the first attempt at processing a job, not that the job won't fail. The increment of "done" progress happens early in the process, after finding the workspace but before GitHub API calls that could still fail.
Applied to files:
apps/api/src/app/github/workers/github-sync-pull-request.worker.ts
📚 Learning: 2025-06-02T01:51:17.244Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:51:17.244Z
Learning: In the sync batch progress system, the "done" count represents "attempted sync" not "successfully completed sync". It's used for a user-facing progress bar to indicate how many PRs have been attempted, allowing users to know when they can start using the dashboard. Failed jobs get retried but don't affect the progress counter, and the early increment (after workspace lookup, before GitHub API calls) is intentional to show immediate progress to users.
Applied to files:
apps/api/src/app/github/workers/github-sync-pull-request.worker.ts
📚 Learning: 2025-06-02T01:53:02.589Z
Learnt from: waltergalvao
Repo: sweetr-dev/sweetr.dev PR: 55
File: apps/api/src/app/github/services/github-pull-request.service.ts:68-70
Timestamp: 2025-06-02T01:53:02.589Z
Learning: In sync batch systems, when tracking "attempted" progress rather than "completed" progress, the batch completion check (maybeFinishSyncBatch) should be called immediately after incrementing progress to handle edge cases where the last item in a batch fails after progress increment but before the completion check.
Applied to files:
apps/api/src/app/github/workers/github-sync-pull-request.worker.ts
🧬 Code graph analysis (4)
apps/api/src/app/github/workers/github-sync-pull-request.worker.ts (1)
apps/api/src/app/sync-batch/services/sync-batch.service.ts (2)
incrementSyncBatchProgress(55-63)maybeFinishSyncBatch(150-191)
apps/web/src/app/settings/workspace/resync/page.tsx (5)
apps/web/src/api/sync-batch.api.ts (2)
useWorkspaceLastSyncBatchQuery(17-40)useScheduleSyncBatchMutation(42-70)apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
scheduleSyncBatch(20-42)apps/web/src/providers/notification.provider.tsx (2)
showSuccessNotification(37-41)showErrorNotification(22-26)apps/web/src/providers/error-message.provider.ts (1)
getErrorMessage(4-31)apps/web/src/providers/date.provider.ts (1)
formatLocaleDate(60-78)
packages/graphql-types/frontend/graphql.ts (1)
packages/graphql-types/api.ts (5)
SyncBatch(922-934)MutationScheduleSyncBatchArgs(611-613)ScheduleSyncBatchInput(904-909)Scalars(14-27)Maybe(4-4)
packages/graphql-types/api.ts (3)
packages/graphql-types/frontend/graphql.ts (3)
SyncBatch(919-931)Scalars(11-24)Maybe(3-3)apps/api/src/lib/type-helpers.ts (1)
DeepPartial(25-33)packages/graphql-types/shared.ts (1)
GraphQLContext(7-27)
🔇 Additional comments (4)
apps/api/src/app/github/workers/github-sync-pull-request.worker.ts (1)
51-55: LGTM! Improved batch completion logic.The change to call
maybeFinishSyncBatchimmediately after incrementing progress (and only on first attempts) is a good improvement. This approach:
- Eliminates redundant batch completion checks on retries and non-batch jobs
- Handles the edge case where the last PR in a batch fails after progress increment
- Aligns with the batch progress model where "done" represents attempts rather than successful completions
The added comment clearly explains the intent.
Based on learnings, this implementation correctly follows the established pattern for sync batch progress tracking.
packages/graphql-types/api.ts (1)
562-573: SyncBatch GraphQL server types and resolvers are consistent and well‑wiredThe new API types for
ScheduleSyncBatchInput,SyncBatch,Mutation.scheduleSyncBatch, andWorkspace.lastSyncBatchlook internally consistent:
- SDL shape is preserved:
SyncBatchfields (id,scheduledAt,finishedAt,sinceDaysAgo,progress) andScheduleSyncBatchInput(workspaceId,sinceDaysAgo) match the descriptions.MutationScheduleSyncBatchArgsand theMutationResolvers['scheduleSyncBatch']signature correctly requireinput.ResolversTypes/ResolversParentTypesentries andSyncBatchResolvers/WorkspaceResolvers.lastSyncBatchfollow the existing DeepPartial + wrapper patterns.No issues from the server typing side.
Also applies to: 611-613, 904-909, 922-934, 1440-1445, 1550-1555, 1848-1857, 1991-1998, 2069-2070, 2151-2153
packages/graphql-types/frontend/graphql.ts (1)
559-570: Frontend GraphQL types and documents for sync‑batch align with the API and usageThe new sync‑batch frontend typings look solid:
ScheduleSyncBatchInputandSyncBatchmirror the server schema, including field names and nullability.Mutation.scheduleSyncBatchandMutationScheduleSyncBatchArgsare correctly added, andWorkspace.lastSyncBatch?: Maybe<SyncBatch>matches what the web app reads.WorkspaceLastSyncBatchQueryselects exactly the fields used in the resync page (scheduledAt,finishedAt,sinceDaysAgo,progress), andScheduleSyncBatchMutationreturns the fields the mutation handler expects.WorkspaceLastSyncBatchDocumentandScheduleSyncBatchDocumentdefinitions correspond to those types and to the hooks wired insync-batch.api.ts.Everything here is coherent with the new feature and doesn’t raise any typing or shape concerns.
Also applies to: 608-612, 901-907, 919-931, 1144-1145, 1640-1653, 1805-1806
apps/api/src/app/sync-batch/sync-batch.schema.ts (1)
1-34: Sync‑batch SDL matches the type/system usage elsewhereThe SDL here cleanly introduces the sync‑batch API surface:
Workspace.lastSyncBatch: SyncBatchmatches the generated Workspace types.SyncBatchfields (id,scheduledAt,finishedAt,sinceDaysAgo,progress) and their nullability align with both server and frontend generated types.scheduleSyncBatch(input: ScheduleSyncBatchInput!): SyncBatch!plusScheduleSyncBatchInput(workspaceId,sinceDaysAgo) match the mutation/argument types in the generated code.This SDL is consistent with the rest of the schema and the new client/server typings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/app/settings/workspace/resync/page.tsx (1)
58-62: Critical: Remove the effect that incorrectly setshasResyncedfor existing in-progress syncs.This effect sets
hasResynced = truewhenever an unfinished sync batch exists, regardless of whether the user triggered it in the current session. This causes two problems:
False success state: If an in-progress sync from a previous session completes while the modal is open, the success message "Resync completed successfully." displays at lines 183-192 even though the user didn't initiate it in this session.
Semantic confusion:
hasResyncedshould represent "user triggered a resync in THIS session" for preventing double-submits, not "any unfinished sync exists".The mutation's
onSuccesshandler (line 75) already correctly setshasResynced(true)when the user triggers a resync. TheisInProgressflag (lines 51-52) is sufficient for gating while a batch is running.🔎 Recommended fix
- useEffect(() => { - if (lastSyncBatch && !lastSyncBatch.finishedAt) { - setHasResynced(true); - } - }, [lastSyncBatch]); + // `hasResynced` prevents multiple manual resyncs in a single session. + // For existing in-progress batches, we rely on `isInProgress` and backend validation.This ensures:
- Existing in-progress sync:
isInProgress = true→canResync = false(button disabled) ✓- User triggers resync:
onSuccesssetshasResynced = true→canResync = falseafter scheduling ✓- Success message shows only when user triggered the resync in this session ✓
🧹 Nitpick comments (1)
apps/web/src/app/settings/workspace/resync/page.tsx (1)
122-135: Consider showing when the next resync will be available.Line 127 states "You can only resync once every 24 hours," but the UI doesn't indicate when the user can resync next if they're within the 24-hour window. This could improve UX by setting clear expectations.
You could compute and display the next available resync time based on
lastSyncBatch.scheduledAtwhen a completed sync exists and is less than 24 hours old.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/app/settings/workspace/resync/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/app/settings/workspace/resync/page.tsx (5)
apps/web/src/api/sync-batch.api.ts (2)
useWorkspaceLastSyncBatchQuery(17-40)useScheduleSyncBatchMutation(42-70)apps/api/src/app/sync-batch/services/sync-batch.service.ts (1)
scheduleSyncBatch(20-42)apps/web/src/providers/notification.provider.tsx (2)
showSuccessNotification(37-41)showErrorNotification(22-26)apps/web/src/providers/error-message.provider.ts (1)
getErrorMessage(4-31)apps/web/src/providers/date.provider.ts (1)
formatLocaleDate(60-78)
🔇 Additional comments (1)
apps/web/src/app/settings/workspace/resync/page.tsx (1)
154-181: Nice progress display with minimum visual feedback.The
Math.max(10, lastSyncBatch.progress)on line 173 ensures the progress bar always shows some visual movement even in early stages (0-9%), while the tooltip displays the exact percentage. This provides good UX for ongoing operations.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.