Uh oh!
There was an error while loading. Please reload this page.
feat(web): Add banner to notify user when permissions are syncing for the first time - #852
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Caution Review failedThe pull request is closed. WalkthroughAdds a permission-sync notification banner and supporting API/UI: server route to report first-time permission sync status, client polling and banner component, new Alert UI primitives, entitlement update, and changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser/Client
participant Banner as PermissionSyncBanner
participant ReactQuery as React Query
participant APIClient as API Client
participant Server as Server API
participant DB as Database
Browser->>Banner: Mount
Banner->>ReactQuery: useQuery(getPermissionSyncStatus)
loop while hasPendingFirstSync
ReactQuery->>APIClient: getPermissionSyncStatus()
APIClient->>Server: GET /api/ee/permissionSyncStatus
Server->>DB: Query accounts + latest permissionSyncJobs
DB-->>Server: account sync data
Server-->>APIClient: PermissionSyncStatusResponse(hasPendingFirstSync)
APIClient-->>ReactQuery: response
ReactQuery-->>Banner: update status
alt pending
Banner->>Browser: show Alert banner (spinner)
else completed
Banner->>Browser: router.refresh(), hide banner
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
🚥 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/web/src/app/`[domain]/components/permissionSyncBanner.tsx:
- Around line 23-27: The refetchInterval callback currently checks
query.state.data for truthiness, but that is the raw response object and always
truthy once loaded, causing infinite polling; update the logic in the
refetchInterval function to inspect the hasPendingFirstSync property on the data
(e.g., return query.state.data?.hasPendingFirstSync ? POLL_INTERVAL_MS : false)
so polling continues only while hasPendingFirstSync is true; reference symbols:
refetchInterval, query.state.data, hasPendingFirstSync, POLL_INTERVAL_MS.
In `@packages/web/src/app/api/`(server)/ee/permissionSyncStatus/route.ts:
- Around line 16-18: Update the JSDoc comment on the permission sync route so
the grammar is correct: change "Returns whether a user has a account that has
it's permissions synced for the first time." to "Returns whether a user has an
account that has its permissions synced for the first time." — edit the comment
above the exported route handler in route.ts (the file-level JSDoc for the
permissionSyncStatus endpoint).
- Around line 23-27: Replace the incorrect error code used in the permission
sync route: in the code path that returns serviceErrorResponse with statusCode
StatusCodes.FORBIDDEN and ErrorCode.NOT_FOUND, change the error code to
ErrorCode.INSUFFICIENT_PERMISSIONS (keep the existing message and status);
update the call in the route handler (the function returning
serviceErrorResponse in route.ts) so the error code matches the 403 semantics
and existing entitlement/plan denial conventions.
🧹 Nitpick comments (1)
packages/web/src/app/api/(server)/ee/permissionSyncStatus/route.ts (1)
1-1: Unnecessary'use server'directive for route handlers.Route handlers in Next.js App Router are server-side by default. The
'use server'directive is specifically for Server Actions (functions called from client components). This directive is unnecessary here and could cause confusion.🔧 Suggested fix
-'use server';- import { apiHandler } from "@/lib/apiHandler";
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Problem
There can be a delay between when a account is created and it's permissions are synced for the first time. This creates user confusion since all repositories will not be visible until this permission sync completes.
Solution
This PR adds a notification banner that is displayed at the top while permissions are syncing for the first time for the user.
permission-syncing.mp4
Fixes#817
Summary by CodeRabbit