diff --git a/apps/mobile/src/components/agents/new-session-repository-section.test.ts b/apps/mobile/src/components/agents/new-session-repository-section.test.ts index af2a3c65960..f803cfce944 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.test.ts +++ b/apps/mobile/src/components/agents/new-session-repository-section.test.ts @@ -211,6 +211,15 @@ describe('NewSessionRepositorySection connect card open action', () => { expect(label?.props.numberOfLines, key).toBe(1); } }); + + it('offers no open label on the connected-empty card', () => { + // The connected-empty card replaces the open action with its guidance, so + // it must leave no empty gap where the button used to be. + const renderer = mountSection({ groups: [group('gitlab', 'connected-empty')] }); + + expect(renderedText(renderer)).toContain(i18n.t('agentChat.newSession.gitlabConnected')); + expect(labelNode(renderer, i18n.t('agentChat.newSession.openGitlab'))).toBeUndefined(); + }); }); describe('NewSessionRepositorySection connect card collapse', () => { diff --git a/apps/mobile/src/components/agents/new-session-repository-section.tsx b/apps/mobile/src/components/agents/new-session-repository-section.tsx index bebb956c2df..532653abf04 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.tsx +++ b/apps/mobile/src/components/agents/new-session-repository-section.tsx @@ -275,6 +275,8 @@ export function NewSessionRepositorySection({ the label the free space keeps its box wider than the text, and `numberOfLines` pins the line. The explorer captures new-session-filled and new-session-kb-down both showed that wrap. + The logical `me-[24px]` inset keeps the centered label clear of the + leading glyph in RTL as well, which the physical `mr-` would not. */} {t(selectedRepository === null ? copy.openLabel : copy.connectTitle)} diff --git a/apps/mobile/src/lib/screen-insets.test.ts b/apps/mobile/src/lib/screen-insets.test.ts index 3d0120ec0f1..3c4a17f2ec1 100644 --- a/apps/mobile/src/lib/screen-insets.test.ts +++ b/apps/mobile/src/lib/screen-insets.test.ts @@ -95,6 +95,9 @@ describe('screen side insets: one implementation for both platforms', () => { expect(profile, `${PROFILE_SCREEN} imports the native safe-area module again`).not.toMatch( SAFE_AREA_MODULE ); + const alignment = alignmentPath(profile); + // `alignmentPath` starts at the `useScreenSideInsets` line, so this also + // proves the path it scans is the screen's insets read, not an empty slice. // The screen carries no platform branch at all: sign-out confirms through // the one shared alert on both platforms, so the insets path — and every // other line — stays a single implementation. Only the lines that carry the @@ -109,7 +112,11 @@ describe('screen side insets: one implementation for both platforms', () => { // hook call to the line that first applies a side inset. Keep both so a // fork is caught whichever way the insets are spelled. expect( - alignmentPath(profile), + alignment, + `${PROFILE_SCREEN} does not read its side insets from the entry point` + ).toMatch(/useScreenSideInsets/); + expect( + alignment, `${PROFILE_SCREEN}'s alignment path carries a per-platform branch` ).not.toMatch(PLATFORM_BRANCH); }); diff --git a/apps/web/src/app/cloud-agent-fork/review-md/route.test.ts b/apps/web/src/app/cloud-agent-fork/review-md/route.test.ts new file mode 100644 index 00000000000..d67c8e17f65 --- /dev/null +++ b/apps/web/src/app/cloud-agent-fork/review-md/route.test.ts @@ -0,0 +1,228 @@ +import { beforeAll, beforeEach, describe, expect, it, jest } from '@jest/globals'; +import { NextRequest } from 'next/server'; + +type TrpcContextFixture = { + user: { + id: string; + }; +}; + +type ReviewConfigFixture = { + customInstructions: string | null; + modelSlug: string | null; +}; + +type RepositoriesFixture = { + integrationInstalled: boolean; + repositories: { id: number; name: string; fullName: string; private: boolean }[]; + syncedAt: string | null; +}; + +type PrepareSessionOutput = { + kiloSessionId: string; +}; + +type RouteGet = (request: NextRequest) => Promise; + +const mockCreateTRPCContext = jest.fn<() => Promise>(); +const mockPersonalGetReviewConfig = + jest.fn<(input: { platform: string }) => Promise>(); +const mockOrganizationGetReviewConfig = + jest.fn<(input: { organizationId: string; platform: string }) => Promise>(); +const mockPersonalListGitHubRepositories = + jest.fn<(input: { forceRefresh: boolean }) => Promise>(); +const mockPersonalListGitLabRepositories = + jest.fn<(input: { forceRefresh: boolean }) => Promise>(); +const mockOrganizationListGitHubRepositories = + jest.fn< + (input: { organizationId: string; forceRefresh: boolean }) => Promise + >(); +const mockOrganizationListGitLabRepositories = + jest.fn< + (input: { organizationId: string; forceRefresh: boolean }) => Promise + >(); +const mockPersonalPrepareSession = jest.fn<() => Promise>(); +const mockOrganizationPrepareSession = jest.fn<() => Promise>(); + +const mockCaller = { + organizations: { + reviewAgent: { + getReviewConfig: mockOrganizationGetReviewConfig, + listGitHubRepositories: mockOrganizationListGitHubRepositories, + listGitLabRepositories: mockOrganizationListGitLabRepositories, + }, + cloudAgentNext: { + prepareSession: mockOrganizationPrepareSession, + }, + }, + personalReviewAgent: { + getReviewConfig: mockPersonalGetReviewConfig, + listGitHubRepositories: mockPersonalListGitHubRepositories, + listGitLabRepositories: mockPersonalListGitLabRepositories, + }, + cloudAgentNext: { + prepareSession: mockPersonalPrepareSession, + }, +}; +const mockCreateCaller = jest.fn((_: TrpcContextFixture) => mockCaller); +const mockCreateCallerFactory = jest.fn(() => mockCreateCaller); + +jest.mock('@/lib/trpc/init', () => ({ + createTRPCContext: () => mockCreateTRPCContext(), + createCallerFactory: () => mockCreateCallerFactory(), +})); + +jest.mock('@/routers/root-router', () => ({ + rootRouter: {}, +})); + +jest.mock('@/lib/posthog-feature-flags', () => ({ + isFeatureFlagEnabledOrDevelopment: () => Promise.resolve(true), +})); + +jest.mock('@/lib/redis', () => ({ + redisClient: { + incr: () => Promise.resolve(1), + expire: () => Promise.resolve(1), + }, +})); + +jest.mock('@/lib/ai-gateway/models', () => ({ + PRIMARY_DEFAULT_MODEL: 'test/primary-default-model', +})); + +let getRoute: RouteGet; + +const USER_ID = 'user_1'; +const ORG_ID = '11111111-1111-4111-8111-111111111111'; +const PERSONAL_KILO_SESSION_ID = 'ses_12345678901234567890123456'; +const ORG_KILO_SESSION_ID = 'ses_abcdefabcdefabcdefabcdefab'; + +const repositories: RepositoriesFixture = { + integrationInstalled: true, + repositories: [{ id: 1, name: 'repo', fullName: 'owner/repo', private: false }], + syncedAt: '2024-01-01T00:00:00Z', +}; + +const syncedEmptyRepositories: RepositoriesFixture = { + integrationInstalled: true, + repositories: [], + syncedAt: '2024-01-01T00:00:00Z', +}; + +function makeRequest(params: { + platform?: string; + repo?: string; + organizationId?: string; + secFetchSite?: string | null; +}): NextRequest { + const url = new URL('https://kilo.test/cloud-agent-fork/review-md'); + if (params.platform) url.searchParams.set('platform', params.platform); + if (params.repo) url.searchParams.set('repo', params.repo); + if (params.organizationId) url.searchParams.set('organizationId', params.organizationId); + + const headers = new Headers(); + const secFetchSite = params.secFetchSite === undefined ? 'same-origin' : params.secFetchSite; + if (secFetchSite !== null) headers.set('sec-fetch-site', secFetchSite); + + return new NextRequest(url.toString(), { headers }); +} + +function getRedirectUrl(response: Response): URL { + const location = response.headers.get('location'); + expect(location).toBeTruthy(); + // This route answers with a relative Location on purpose (the dev server's + // origin differs from the browser's), so resolve it against the request origin. + return new URL(location ?? '', 'https://kilo.test'); +} + +function expectErrorRedirect(response: Response, error: string) { + const redirectUrl = getRedirectUrl(response); + expect(`${redirectUrl.pathname}${redirectUrl.search}`).toBe(`/code-reviews?error=${error}`); +} + +describe('GET /cloud-agent-fork/review-md', () => { + beforeAll(async () => { + ({ GET: getRoute } = await import('./route')); + }); + + beforeEach(() => { + jest.clearAllMocks(); + mockCreateTRPCContext.mockResolvedValue({ user: { id: USER_ID } }); + mockPersonalGetReviewConfig.mockResolvedValue({ + customInstructions: 'Follow the repository conventions.', + modelSlug: null, + }); + mockOrganizationGetReviewConfig.mockResolvedValue({ + customInstructions: 'Follow the repository conventions.', + modelSlug: null, + }); + mockPersonalListGitHubRepositories.mockResolvedValue(repositories); + mockPersonalListGitLabRepositories.mockResolvedValue(repositories); + mockOrganizationListGitHubRepositories.mockResolvedValue(repositories); + mockOrganizationListGitLabRepositories.mockResolvedValue(repositories); + mockPersonalPrepareSession.mockResolvedValue({ kiloSessionId: PERSONAL_KILO_SESSION_ID }); + mockOrganizationPrepareSession.mockResolvedValue({ kiloSessionId: ORG_KILO_SESSION_ID }); + }); + + it('forces a provider read for the personal GitHub allowlist', async () => { + const response = await getRoute(makeRequest({ platform: 'github', repo: 'owner/repo' })); + + // The allowlist must not answer from a synced (possibly empty) snapshot. + expect(mockPersonalListGitHubRepositories).toHaveBeenCalledWith({ forceRefresh: true }); + expect(mockPersonalPrepareSession).toHaveBeenCalledTimes(1); + expect(response.status).toBe(303); + }); + + it('forces a provider read for the personal GitLab allowlist', async () => { + const response = await getRoute(makeRequest({ platform: 'gitlab', repo: 'owner/repo' })); + + expect(mockPersonalListGitLabRepositories).toHaveBeenCalledWith({ forceRefresh: true }); + expect(mockPersonalPrepareSession).toHaveBeenCalledTimes(1); + expect(response.status).toBe(303); + }); + + it('forces a provider read for the organization GitHub allowlist', async () => { + const response = await getRoute( + makeRequest({ platform: 'github', repo: 'owner/repo', organizationId: ORG_ID }) + ); + + expect(mockOrganizationListGitHubRepositories).toHaveBeenCalledWith({ + organizationId: ORG_ID, + forceRefresh: true, + }); + expect(mockOrganizationPrepareSession).toHaveBeenCalledTimes(1); + expect(response.status).toBe(303); + }); + + it('forces a provider read for the organization GitLab allowlist', async () => { + const response = await getRoute( + makeRequest({ platform: 'gitlab', repo: 'owner/repo', organizationId: ORG_ID }) + ); + + expect(mockOrganizationListGitLabRepositories).toHaveBeenCalledWith({ + organizationId: ORG_ID, + forceRefresh: true, + }); + expect(mockOrganizationPrepareSession).toHaveBeenCalledTimes(1); + expect(response.status).toBe(303); + }); + + it('still rejects a repository the provider does not report', async () => { + mockPersonalListGitHubRepositories.mockResolvedValue(syncedEmptyRepositories); + + const response = await getRoute(makeRequest({ platform: 'github', repo: 'owner/repo' })); + + expectErrorRedirect(response, 'repository_not_allowed'); + expect(mockPersonalPrepareSession).not.toHaveBeenCalled(); + }); + + it('rejects a cross-site request before reading repositories', async () => { + const response = await getRoute( + makeRequest({ platform: 'github', repo: 'owner/repo', secFetchSite: 'cross-site' }) + ); + + expectErrorRedirect(response, 'invalid_conversion_request'); + expect(mockPersonalListGitHubRepositories).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/app/cloud-agent-fork/review-md/route.ts b/apps/web/src/app/cloud-agent-fork/review-md/route.ts index 99d0660fa3a..0710b0b9d39 100644 --- a/apps/web/src/app/cloud-agent-fork/review-md/route.ts +++ b/apps/web/src/app/cloud-agent-fork/review-md/route.ts @@ -174,19 +174,22 @@ export async function GET(request: NextRequest) { // integration actually covers them), so they must NOT gate this credit-spending, PR-opening // action — otherwise a user could add an arbitrary full_name to their config and pass this // check. The worker enforces installation scope regardless; this keeps the fail-fast honest. + // forceRefresh keeps the allowlist off the cached snapshot: a synced-empty cache (connect-time + // sync stored zero repositories, then access was granted) would otherwise reject a repo the + // provider now reports, and GitLab never invalidates that cache. const repositoryList = organizationId ? platform === 'gitlab' ? await caller.organizations.reviewAgent.listGitLabRepositories({ organizationId, - forceRefresh: false, + forceRefresh: true, }) : await caller.organizations.reviewAgent.listGitHubRepositories({ organizationId, - forceRefresh: false, + forceRefresh: true, }) : platform === 'gitlab' - ? await caller.personalReviewAgent.listGitLabRepositories({ forceRefresh: false }) - : await caller.personalReviewAgent.listGitHubRepositories({ forceRefresh: false }); + ? await caller.personalReviewAgent.listGitLabRepositories({ forceRefresh: true }) + : await caller.personalReviewAgent.listGitHubRepositories({ forceRefresh: true }); const allowedRepoFullNames = buildAllowedRepositoryFullNames(repositoryList.repositories, []); if (!allowedRepoFullNames.has(repo)) { diff --git a/apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts b/apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts index d388919d070..8957fdf1b5b 100644 --- a/apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts +++ b/apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts @@ -93,6 +93,18 @@ describe('github-integration-helpers', () => { expect(mockFetchGitHubRepositories).not.toHaveBeenCalled(); }); + it('returns a synced empty list as the connected-empty snapshot', async () => { + mockGetIntegrationForOwner.mockResolvedValue(buildIntegration({ repositories: [] })); + + const { fetchGitHubRepositoriesForUser } = await import('./github-integration-helpers'); + const result = await fetchGitHubRepositoriesForUser('user-123'); + + expect(result.integrationInstalled).toBe(true); + expect(result.repositories).toEqual([]); + expect(mockFetchGitHubRepositories).not.toHaveBeenCalled(); + expect(mockUpdateRepositoriesForIntegration).not.toHaveBeenCalled(); + }); + it('returns integrationInstalled false when no integration exists', async () => { mockGetIntegrationForOwner.mockResolvedValue(null); @@ -217,6 +229,19 @@ describe('github-integration-helpers', () => { expect(mockFetchGitHubRepositories).not.toHaveBeenCalled(); }); + it('returns a synced empty list as the connected-empty snapshot', async () => { + mockGetIntegrationsByOrganization.mockResolvedValue([buildIntegration({ repositories: [] })]); + + const { fetchAllGitHubRepositoriesForOrganization } = + await import('./github-integration-helpers'); + const result = await fetchAllGitHubRepositoriesForOrganization('org-123'); + + expect(result.integrationInstalled).toBe(true); + expect(result.repositories).toEqual([]); + expect(mockFetchGitHubRepositories).not.toHaveBeenCalled(); + expect(mockUpdateRepositoriesForIntegration).not.toHaveBeenCalled(); + }); + it('preserves installation provenance across multiple GitHub organizations', async () => { mockGetIntegrationsByOrganization.mockResolvedValue([ buildIntegration({ diff --git a/apps/web/src/lib/cloud-agent/github-integration-helpers.ts b/apps/web/src/lib/cloud-agent/github-integration-helpers.ts index a2724802121..3fcaee87d15 100644 --- a/apps/web/src/lib/cloud-agent/github-integration-helpers.ts +++ b/apps/web/src/lib/cloud-agent/github-integration-helpers.ts @@ -19,6 +19,7 @@ import { } from '@/lib/integrations/core/health'; import { requireNumericPlatformRepositories, + shouldSyncProviderRepositories, type PlatformRepository, } from '@/lib/integrations/core/types'; @@ -190,7 +191,16 @@ export async function fetchGitHubRepositoriesForOrganization( try { const cachedRepositories = requireNumericPlatformRepositories(integration.repositories); - if (forceRefresh || !cachedRepositories?.length) { + // Answer from the cached snapshot unless the caller forces a sync or the + // integration has never synced: a synced empty snapshot is the + // connected-empty state, not a reason to re-query GitHub on every read. + if ( + shouldSyncProviderRepositories({ + forceRefresh, + cachedRepositories, + repositoriesSyncedAt: integration.repositories_synced_at, + }) + ) { const repositories = await fetchGitHubRepositories( integration.platform_installation_id, integration.github_app_type || 'standard', @@ -205,7 +215,7 @@ export async function fetchGitHubRepositoriesForOrganization( } return { integrationInstalled: true, - repositories: mapRepositories(cachedRepositories), + repositories: mapRepositories(cachedRepositories ?? []), syncedAt: integration.repositories_synced_at, }; } catch (_error) { @@ -239,7 +249,13 @@ async function fetchRepositoriesForIntegrations( integrations.map(async integration => { if (!integration.platform_installation_id) return { repositories: [], syncedAt: null }; const cachedRepositories = requireNumericPlatformRepositories(integration.repositories); - if (forceRefresh || !cachedRepositories?.length) { + if ( + shouldSyncProviderRepositories({ + forceRefresh, + cachedRepositories, + repositoriesSyncedAt: integration.repositories_synced_at, + }) + ) { const repositories = await fetchGitHubRepositories( integration.platform_installation_id, integration.github_app_type || 'standard', @@ -252,7 +268,7 @@ async function fetchRepositoriesForIntegrations( }; } return { - repositories: mapRepositories(cachedRepositories, integration), + repositories: mapRepositories(cachedRepositories ?? [], integration), syncedAt: integration.repositories_synced_at, }; }) @@ -313,8 +329,16 @@ export async function fetchGitHubRepositoriesForUser( try { const cachedRepositories = requireNumericPlatformRepositories(integration.repositories); - // If forceRefresh or no cached repos, fetch from GitHub and update cache - if (forceRefresh || !cachedRepositories?.length) { + // Answer from the cached snapshot unless the caller forces a sync or the + // integration has never synced: a synced empty snapshot is the + // connected-empty state, not a reason to re-query GitHub on every read. + if ( + shouldSyncProviderRepositories({ + forceRefresh, + cachedRepositories, + repositoriesSyncedAt: integration.repositories_synced_at, + }) + ) { const appType = integration.github_app_type || 'standard'; const repositories = await fetchGitHubRepositories( integration.platform_installation_id, @@ -332,7 +356,7 @@ export async function fetchGitHubRepositoriesForUser( // Return cached repos return { integrationInstalled: true, - repositories: mapRepositories(cachedRepositories, integration), + repositories: mapRepositories(cachedRepositories ?? [], integration), syncedAt: integration.repositories_synced_at, }; } catch (_error) { diff --git a/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.test.ts b/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.test.ts index cec31a2f19c..6243b9e8eb9 100644 --- a/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.test.ts +++ b/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.test.ts @@ -493,6 +493,48 @@ describe('gitlab-integration-helpers', () => { expect(mockFetchGitLabProjects).not.toHaveBeenCalled(); }); + it('should return a synced empty list as the connected-empty snapshot', async () => { + mockGetIntegrationForOwner.mockResolvedValue(buildIntegration({ repositories: [] })); + + const { fetchGitLabRepositoriesForUser } = await import('./gitlab-integration-helpers'); + const result = await fetchGitLabRepositoriesForUser('user-123'); + + expect(result.integrationInstalled).toBe(true); + expect(result.repositories).toEqual([]); + expect(mockFetchGitLabProjects).not.toHaveBeenCalled(); + expect(mockUpdateRepositoriesForIntegration).not.toHaveBeenCalled(); + }); + + it('should sync an integration whose repository list was never synced', async () => { + mockGetIntegrationForOwner.mockResolvedValue( + buildIntegration({ repositories: [], repositories_synced_at: null }) + ); + mockGetValidGitLabToken.mockResolvedValue('valid-token'); + mockFetchGitLabProjects.mockResolvedValue([]); + + const { fetchGitLabRepositoriesForUser } = await import('./gitlab-integration-helpers'); + const result = await fetchGitLabRepositoriesForUser('user-123'); + + expect(mockFetchGitLabProjects).toHaveBeenCalledTimes(1); + expect(result.repositories).toEqual([]); + }); + + it('should re-sync a synced empty list when forceRefresh is set', async () => { + mockGetIntegrationForOwner.mockResolvedValue(buildIntegration({ repositories: [] })); + mockGetValidGitLabToken.mockResolvedValue('valid-token'); + mockFetchGitLabProjects.mockResolvedValue([ + { id: 2, name: 'new', full_name: 'group/new', private: false }, + ]); + + const { fetchGitLabRepositoriesForUser } = await import('./gitlab-integration-helpers'); + const result = await fetchGitLabRepositoriesForUser('user-123', true); + + expect(mockFetchGitLabProjects).toHaveBeenCalledTimes(1); + expect(result.repositories).toEqual([ + { id: 2, name: 'new', fullName: 'group/new', private: false }, + ]); + }); + it('should return no repositories when the integration is suspended', async () => { mockGetIntegrationForOwner.mockResolvedValue( buildIntegration({ @@ -551,6 +593,51 @@ describe('gitlab-integration-helpers', () => { expect(mockFetchGitLabProjects).not.toHaveBeenCalled(); }); + it('should return a synced empty list as the connected-empty snapshot', async () => { + mockGetIntegrationForOrganization.mockResolvedValue(buildIntegration({ repositories: [] })); + + const { fetchGitLabRepositoriesForOrganization } = + await import('./gitlab-integration-helpers'); + const result = await fetchGitLabRepositoriesForOrganization('org-123', 'actor-123'); + + expect(result.integrationInstalled).toBe(true); + expect(result.repositories).toEqual([]); + expect(mockFetchGitLabProjects).not.toHaveBeenCalled(); + expect(mockUpdateRepositoriesForIntegration).not.toHaveBeenCalled(); + }); + + it('should sync an integration whose repository list was never synced', async () => { + mockGetIntegrationForOrganization.mockResolvedValue( + buildIntegration({ repositories: [], repositories_synced_at: null }) + ); + mockGetValidGitLabToken.mockResolvedValue('valid-token'); + mockFetchGitLabProjects.mockResolvedValue([]); + + const { fetchGitLabRepositoriesForOrganization } = + await import('./gitlab-integration-helpers'); + const result = await fetchGitLabRepositoriesForOrganization('org-123', 'actor-123'); + + expect(mockFetchGitLabProjects).toHaveBeenCalledTimes(1); + expect(result.repositories).toEqual([]); + }); + + it('should re-sync a synced empty list when forceRefresh is set', async () => { + mockGetIntegrationForOrganization.mockResolvedValue(buildIntegration({ repositories: [] })); + mockGetValidGitLabToken.mockResolvedValue('valid-token'); + mockFetchGitLabProjects.mockResolvedValue([ + { id: 2, name: 'new', full_name: 'org/new', private: false }, + ]); + + const { fetchGitLabRepositoriesForOrganization } = + await import('./gitlab-integration-helpers'); + const result = await fetchGitLabRepositoriesForOrganization('org-123', 'actor-123', true); + + expect(mockFetchGitLabProjects).toHaveBeenCalledTimes(1); + expect(result.repositories).toEqual([ + { id: 2, name: 'new', fullName: 'org/new', private: false }, + ]); + }); + it('should return no repositories when the integration is suspended', async () => { mockGetIntegrationForOrganization.mockResolvedValue( buildIntegration({ diff --git a/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.ts b/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.ts index 510c91b50dd..f9fd1e331f8 100644 --- a/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.ts +++ b/apps/web/src/lib/cloud-agent/gitlab-integration-helpers.ts @@ -10,6 +10,7 @@ import { PLATFORM } from '@/lib/integrations/core/constants'; import { isPlatformIntegrationSuspended } from '@/lib/integrations/core/health'; import { requireNumericPlatformRepositories, + shouldSyncProviderRepositories, type PlatformRepository, } from '@/lib/integrations/core/types'; @@ -129,8 +130,16 @@ export async function fetchGitLabRepositoriesForOrganization( try { const cachedRepositories = requireNumericPlatformRepositories(integration.repositories); - // If forceRefresh or no cached repos, fetch from GitLab and update cache - if (forceRefresh || !cachedRepositories?.length) { + // Answer from the cached snapshot unless the caller forces a sync or the + // integration has never synced: a synced empty snapshot is the + // connected-empty state, not a reason to re-query GitLab on every read. + if ( + shouldSyncProviderRepositories({ + forceRefresh, + cachedRepositories, + repositoriesSyncedAt: integration.repositories_synced_at, + }) + ) { const accessToken = await getValidGitLabToken(integration, { userId: actorUserId, organizationId, @@ -148,7 +157,7 @@ export async function fetchGitLabRepositoriesForOrganization( // Return cached repos return { integrationInstalled: true, - repositories: mapRepositories(cachedRepositories), + repositories: mapRepositories(cachedRepositories ?? []), syncedAt: integration.repositories_synced_at, instanceUrl, }; @@ -183,8 +192,16 @@ export async function fetchGitLabRepositoriesForUser( try { const cachedRepositories = requireNumericPlatformRepositories(integration.repositories); - // If forceRefresh or no cached repos, fetch from GitLab and update cache - if (forceRefresh || !cachedRepositories?.length) { + // Answer from the cached snapshot unless the caller forces a sync or the + // integration has never synced: a synced empty snapshot is the + // connected-empty state, not a reason to re-query GitLab on every read. + if ( + shouldSyncProviderRepositories({ + forceRefresh, + cachedRepositories, + repositoriesSyncedAt: integration.repositories_synced_at, + }) + ) { const accessToken = await getValidGitLabToken(integration, { userId }); const repositories = await fetchGitLabProjects(accessToken, instanceUrl); await updateRepositoriesForIntegration(integration.id, repositories); @@ -199,7 +216,7 @@ export async function fetchGitLabRepositoriesForUser( // Return cached repos return { integrationInstalled: true, - repositories: mapRepositories(cachedRepositories), + repositories: mapRepositories(cachedRepositories ?? []), syncedAt: integration.repositories_synced_at, instanceUrl, }; diff --git a/apps/web/src/lib/integrations/core/types.test.ts b/apps/web/src/lib/integrations/core/types.test.ts new file mode 100644 index 00000000000..5c2705ac067 --- /dev/null +++ b/apps/web/src/lib/integrations/core/types.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it } from '@jest/globals'; +import { shouldSyncProviderRepositories } from './types'; +import type { PlatformRepository } from '@kilocode/db/schema-types'; + +const repositories: PlatformRepository[] = [ + { id: 1, name: 'cloud', full_name: 'kilocode/cloud', private: true }, + { id: 2, name: 'extension', full_name: 'kilocode/extension', private: false }, +]; + +describe('shouldSyncProviderRepositories', () => { + it('answers from a non-empty snapshot without a provider round-trip', () => { + expect( + shouldSyncProviderRepositories({ + forceRefresh: false, + cachedRepositories: repositories, + repositoriesSyncedAt: '2024-01-01T00:00:00Z', + }) + ).toBe(false); + }); + + it('answers from a synced empty snapshot (the connected-empty state)', () => { + expect( + shouldSyncProviderRepositories({ + forceRefresh: false, + cachedRepositories: [], + repositoriesSyncedAt: '2024-01-01T00:00:00Z', + }) + ).toBe(false); + }); + + it('syncs an integration that has never synced', () => { + expect( + shouldSyncProviderRepositories({ + forceRefresh: false, + cachedRepositories: null, + repositoriesSyncedAt: null, + }) + ).toBe(true); + expect( + shouldSyncProviderRepositories({ + forceRefresh: false, + cachedRepositories: [], + repositoriesSyncedAt: null, + }) + ).toBe(true); + }); + + it('syncs whenever the caller forces a refresh', () => { + expect( + shouldSyncProviderRepositories({ + forceRefresh: true, + cachedRepositories: repositories, + repositoriesSyncedAt: '2024-01-01T00:00:00Z', + }) + ).toBe(true); + expect( + shouldSyncProviderRepositories({ + forceRefresh: true, + cachedRepositories: [], + repositoriesSyncedAt: '2024-01-01T00:00:00Z', + }) + ).toBe(true); + }); +}); diff --git a/apps/web/src/lib/integrations/core/types.ts b/apps/web/src/lib/integrations/core/types.ts index bab97daf8e8..aa153cae7ae 100644 --- a/apps/web/src/lib/integrations/core/types.ts +++ b/apps/web/src/lib/integrations/core/types.ts @@ -17,6 +17,33 @@ export function requireNumericPlatformRepositories( return repositories; } +/** + * Whether a provider repository read must go back to the provider instead of + * answering from the integration's cached snapshot. + * + * A snapshot is the answer for a non-force read even when it is empty: a + * connected provider that reports no repositories is the connected-empty + * state, and re-fetching on every read turns a provider hiccup into a load + * error instead of that state. An integration that has never synced (no + * repositories and no `repositories_synced_at`) still syncs here, as does an + * explicit `forceRefresh` from the section's refresh affordance. + * + * When this returns `false`, `cachedRepositories` is a list (never `null`). + */ +export function shouldSyncProviderRepositories({ + forceRefresh, + cachedRepositories, + repositoriesSyncedAt, +}: { + forceRefresh: boolean; + cachedRepositories: PlatformRepository[] | null; + repositoriesSyncedAt: string | null; +}): boolean { + if (forceRefresh) return true; + if (cachedRepositories === null) return true; + return cachedRepositories.length === 0 && repositoriesSyncedAt === null; +} + /** * Represents ownership of an integration * Can be either a user or an organization diff --git a/services/container-usage-meter/test/postgres.test.ts b/services/container-usage-meter/test/postgres.test.ts index c679f4418d5..9438b9dce97 100644 --- a/services/container-usage-meter/test/postgres.test.ts +++ b/services/container-usage-meter/test/postgres.test.ts @@ -965,6 +965,12 @@ describe('container usage PostgreSQL application', () => { raceFingerprint, 2_000 ); + // Attach the rejection assertion before releasing the blocker: otherwise the + // expected conflict can reject before it is awaited and Vitest reports it as + // an unhandled rejection. + const contenderRejection = expect(contender).rejects.toThrow( + 'Another usage interval is already open' + ); await expect .poll(async () => { const result = await client.pool.query<{ count: string }>( @@ -980,7 +986,7 @@ describe('container usage PostgreSQL application', () => { releaseBlocker(); await blocker; - await expect(contender).rejects.toThrow('Another usage interval is already open'); + await contenderRejection; const contenders = await client.db .select() .from(container_usage_interval)