diff --git a/hub/app/controllers/concerns/has_repository_workspace.rb b/hub/app/controllers/concerns/has_repository_workspace.rb index 1241edc..10f67ec 100644 --- a/hub/app/controllers/concerns/has_repository_workspace.rb +++ b/hub/app/controllers/concerns/has_repository_workspace.rb @@ -10,16 +10,16 @@ module HasRepositoryWorkspace private # Loads one readable repository and the repository choices for the workspace. - def load_repository_workspace(fresh: false) + def load_repository_workspace(fresh: false, repository_choices: true) github_id = params[:github_id] || params[:repository_github_id] @repository = Repository.find_by_route_id!(github_id) - configuration = GithubAppConfiguration.current + configuration = @github_configuration = GithubAppConfiguration.current @repository_permission = @repository.authorization.permission_for(Current.user, configuration:, fresh:) if !@repository.authorization.sufficient?(@repository_permission, minimum: :read) return head :not_found end - @repositories = authorized_repositories(configuration, including: @repository) + @repositories = repository_choices ? authorized_repositories(configuration, including: @repository) : [ @repository ] end # Returns active repositories from a cached list of current GitHub permissions. @@ -31,9 +31,36 @@ def authorized_repositories(configuration, including: nil, cache: Rails.cache) configuration&.cache_key_with_version, repositories.cache_key_with_version ] - route_ids = cache.fetch(cache_key, expires_in: REPOSITORY_SELECTOR_CACHE_TTL) do - repositories.filter_map do |repository| - if repository == including || repository.authorization.authorized?(Current.user, configuration:) + route_ids = cache.read(cache_key) + if route_ids.nil? + entries = repositories.to_a + payloads = {} + if !LocalDevelopment.enabled? && configuration&.installation_state == "active" + uncached = entries.reject do |repository| + repository == including || repository.authorization.cached_permission(Current.user, cache:).present? + end + if uncached.any? + payloads = begin + configuration.github_client.repositories.permissions \ + uncached, + Current.user, + token: configuration.tokens.installation + rescue GithubClient::Error => error + uncached.to_h { |repository| [ repository.github_id, error ] } + end + end + end + route_ids = entries.filter_map do |repository| + permission = if repository == including + "read" + else + repository.authorization.permission_for \ + Current.user, + configuration:, + cache:, + payload: payloads[repository.github_id] + end + if repository.authorization.sufficient?(permission, minimum: :read) repository.route_id end rescue GithubClient::Error => error @@ -41,6 +68,9 @@ def authorized_repositories(configuration, including: nil, cache: Rails.cache) Rails.logger.warn("Repository navigation permission check failed: #{error.class}") nil end + if !@repository_navigation_unavailable + cache.write(cache_key, route_ids, expires_in: REPOSITORY_SELECTOR_CACHE_TTL) + end end authorized = if LocalDevelopment.enabled? diff --git a/hub/app/controllers/prototype_management_controller.rb b/hub/app/controllers/prototype_management_controller.rb index 93ba55f..caa4060 100644 --- a/hub/app/controllers/prototype_management_controller.rb +++ b/hub/app/controllers/prototype_management_controller.rb @@ -6,7 +6,7 @@ class PrototypeManagementController < SuperglueController # @route GET /repositories/:repository_github_id/prototypes/manage (repository_prototype_management) def show - load_repository_workspace + load_repository_workspace(repository_choices: false) return if performed? return head :not_found if !@repository.authorization.sufficient?(@repository_permission, minimum: :write) page = params[:page] @@ -15,16 +15,17 @@ def show @candidate_page = page ? page.to_i : 1 @candidate_page_path = repository_prototype_management_path(@repository.route_id) @registered_prototypes = @repository.reviewable_prototypes.active.registered.order(:branch_name) - begin - @unregistered_prototypes, @candidate_page_count = @repository.prototype_candidates.available_page \ - Current.user, - permission: @repository_permission, - page: @candidate_page - head :not_found if @candidate_page > [ @candidate_page_count, 1 ].max - rescue GithubClient::Error, LocalDevelopment::Error - @candidate_check_failed = true - @unregistered_prototypes = [] - @candidate_page_count = 0 + @unregistered_prototypes = [] + @candidate_page_count = 0 + return if !request.format.json? + + case params[:props_at] + when "data.management" + load_candidates + when "data.repositoryChoices" + @repositories = authorized_repositories(@github_configuration, including: @repository) + else + head :not_found if params[:props_at] end rescue LocalDevelopment::Error => error response.status = 422 @@ -34,6 +35,21 @@ def show private + # Checks available branches independently of the workspace and listed prototypes. + def load_candidates + @unregistered_prototypes, @candidate_page_count = @repository.prototype_candidates.available_page \ + Current.user, + permission: @repository_permission, + page: @candidate_page, + configuration: @github_configuration + head :not_found if @candidate_page > [ @candidate_page_count, 1 ].max + rescue GithubClient::Error, LocalDevelopment::Error => error + Rails.logger.warn("Prototype candidate check failed: #{error.class}") + @candidate_check_failed = true + @unregistered_prototypes = [] + @candidate_page_count = 0 + end + # Does not present an incomplete branch list as authoritative. def repository_unavailable head :service_unavailable diff --git a/hub/app/javascript/application/build-visit-and-remote/management.test.tsx b/hub/app/javascript/application/build-visit-and-remote/management.test.tsx new file mode 100644 index 0000000..fbf5189 --- /dev/null +++ b/hub/app/javascript/application/build-visit-and-remote/management.test.tsx @@ -0,0 +1,236 @@ +import { act, fireEvent, render, screen, waitFor, within } from "@testing-library/react" +import { Application, NavigationContext } from "@thoughtbot/superglue" +import { useContext } from "react" +import { describe, expect, it, vi } from "vitest" + +import { store } from "@application/store" +import { HubThemeProvider } from "@hooks/use-hub-theme" +import managementStory from "@pages/prototype-management-page/stories" + +import ManagementView from "../../../views/prototype_management/show" +import { buildVisitAndRemote } from "." + +describe("async prototype management", () => { + it("mounts the shell and grafts independent section responses through Superglue", async () => { + const data = managementStory.args.data + const path = "/repositories/2001/prototypes/manage" + const initialPage = { + data: { + ...data, + sectionLoadId: "first-shell", + liveInbox: undefined, + notificationInbox: data.notificationInbox + ? { ...data.notificationInbox, copy: { ...data.notificationInbox.copy } } + : undefined, + repository: null, + management: { + ...data.management, + registered: data.management.registered.map((entry) => ({ ...entry })), + unlisted: [], + candidatePageCount: 0, + }, + repositoryChoices: { + ...data.repositoryChoices, + repositories: data.repositoryChoices.repositories + .slice(0, 1) + .map((repository) => ({ ...repository })), + }, + }, + componentIdentifier: "prototype_management/show", + assets: [], + defers: [], + fragments: [], + slices: {}, + renderedAt: 1, + restoreStrategy: "revisitOnly" as const, + } + let finishBranches!: (response: Response) => void + let finishRepositories!: (response: Response) => void + const branches = new Promise((resolve) => { + finishBranches = resolve + }) + const repositories = new Promise((resolve) => { + finishRepositories = resolve + }) + const fetchMock = vi.fn().mockImplementation((input) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url + return url.includes("props_at=data.management") ? branches : repositories + }) + vi.stubGlobal("fetch", fetchMock) + vi.spyOn(window, "scrollTo").mockImplementation(() => {}) + vi.stubGlobal( + "ResizeObserver", + class { + observe() {} + unobserve() {} + disconnect() {} + }, + ) + render( + + + , + ) + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(screen.getByText(data.management.candidateLoadingLabel)).toBeVisible() + expect(screen.getByRole("button", { name: "Edit details: prototype/welcome" })).toBeEnabled() + expect(screen.queryByText(data.management.emptyLabel)).not.toBeInTheDocument() + fireEvent.click(screen.getByRole("button", { name: "Edit details: prototype/welcome" })) + const dialog = screen.getByRole("dialog", { name: data.management.editDialogHeading }) + const name = within(dialog).getByRole("textbox", { name: data.management.nameLabel }) + fireEvent.change(name, { target: { value: "Unsaved prototype name" } }) + + const repositoryResponse = Response.json({ + ...initialPage, + action: "graft", + path: "data.repositoryChoices", + data: data.repositoryChoices, + }) + Object.defineProperty(repositoryResponse, "url", { + value: `http://localhost${path}?props_at=data.repositoryChoices`, + }) + await act(async () => { + finishRepositories(repositoryResponse) + await repositories + }) + await waitFor(() => + expect(screen.queryByText(data.repositoryChoices.loadingLabel)).not.toBeInTheDocument(), + ) + fireEvent.click(screen.getByRole("button", { name: data.repositorySelectorLabel })) + expect(screen.getByRole("link", { name: "github/empty" })).toBeInTheDocument() + expect(screen.getByText(data.management.candidateLoadingLabel)).toBeVisible() + expect(name).toHaveValue("Unsaved prototype name") + + const branchResponse = Response.json({ + ...initialPage, + action: "graft", + path: "data.management", + data: data.management, + }) + Object.defineProperty(branchResponse, "url", { + value: `http://localhost${path}?props_at=data.management`, + }) + await act(async () => { + finishBranches(branchResponse) + await branches + }) + await waitFor(() => + expect(screen.getByRole("button", { name: "Add to Hub: prototype/new" })).toBeEnabled(), + ) + expect(screen.queryByText(data.management.candidateLoadingLabel)).not.toBeInTheDocument() + expect(screen.getByRole("dialog", { name: data.management.editDialogHeading })).toBe(dialog) + expect(name).toHaveValue("Unsaved prototype name") + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + it("reloads both sections once after a full visit replaces the shell at the same URL", async () => { + const data = structuredClone(managementStory.args.data) + const path = "/repositories/2001/prototypes/manage" + const shell = { + data: { + ...data, + sectionLoadId: "before-refresh", + liveInbox: undefined, + notificationInbox: data.notificationInbox + ? { ...data.notificationInbox, copy: { ...data.notificationInbox.copy } } + : undefined, + repository: null, + management: { + ...data.management, + registered: data.management.registered.map((entry) => ({ ...entry })), + unlisted: [], + candidatePageCount: 0, + }, + repositoryChoices: { + ...data.repositoryChoices, + repositories: data.repositoryChoices.repositories + .slice(0, 1) + .map((repository) => ({ ...repository })), + }, + }, + componentIdentifier: "prototype_management/show", + assets: [], + defers: [], + fragments: [], + slices: {}, + renderedAt: 1, + restoreStrategy: "revisitOnly" as const, + } + const fetchMock = vi.fn().mockImplementation((input) => { + const url = new URL( + typeof input === "string" ? input : input instanceof URL ? input.href : input.url, + "http://localhost", + ) + const section = url.searchParams.get("props_at") + const payload = section + ? { + ...shell, + action: "graft", + path: section, + data: section === "data.management" ? data.management : data.repositoryChoices, + } + : { ...shell, data: { ...shell.data, sectionLoadId: "after-refresh" } } + const response = Response.json(payload) + Object.defineProperty(response, "url", { value: url.href }) + return Promise.resolve(response) + }) + vi.stubGlobal("fetch", fetchMock) + vi.spyOn(window, "scrollTo").mockImplementation(() => {}) + vi.stubGlobal( + "ResizeObserver", + class { + observe() {} + unobserve() {} + disconnect() {} + }, + ) + function RefreshableManagement() { + const { visit } = useContext(NavigationContext) + return ( + <> + + + + ) + } + render( + + + , + ) + await waitFor(() => + expect(screen.getByRole("button", { name: "Add to Hub: prototype/new" })).toBeEnabled(), + ) + expect(fetchMock).toHaveBeenCalledTimes(2) + + fireEvent.click(screen.getByRole("button", { name: "Refresh management" })) + + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(5)) + await waitFor(() => + expect(screen.getByRole("button", { name: "Add to Hub: prototype/new" })).toBeEnabled(), + ) + expect(screen.queryByText(data.management.emptyLabel)).not.toBeInTheDocument() + const requests = fetchMock.mock.calls.map(([input]) => + typeof input === "string" ? input : input instanceof URL ? input.href : input.url, + ) + expect(requests.filter((url) => url.includes("props_at=data.management"))).toHaveLength(2) + expect(requests.filter((url) => url.includes("props_at=data.repositoryChoices"))).toHaveLength( + 2, + ) + expect(requests.filter((url) => !url.includes("props_at="))).toHaveLength(1) + }) +}) diff --git a/hub/app/javascript/application/sidebar-inbox-views.test.tsx b/hub/app/javascript/application/sidebar-inbox-views.test.tsx index 919a859..c764979 100644 --- a/hub/app/javascript/application/sidebar-inbox-views.test.tsx +++ b/hub/app/javascript/application/sidebar-inbox-views.test.tsx @@ -10,6 +10,7 @@ import type { RepositoryWorkspaceData } from "@features/repository-workspace/typ import { HubWorkspace } from "@features/hub-workspace" import type { HubWorkspaceData } from "@features/hub-workspace/types" import repositoryStories from "@pages/repositories-show-page/stories" +import managementStories from "@pages/prototype-management-page/stories" import inboxStories from "@pages/notifications-page/stories" import AnalyticsView from "../../views/analytics/show" @@ -78,6 +79,8 @@ vi.mock("@hooks/use-page-data", () => ({ usePageData: () => { const data = { ...repositoryStories.args.data, + management: managementStories.args.data.management, + repositoryChoices: managementStories.args.data.repositoryChoices, notificationInbox: inboxStories.args.data.notificationInbox, notificationsPath: "/notifications", notificationsLabel: "Inbox", @@ -96,6 +99,9 @@ vi.mock("@hooks/use-page-data", () => ({ } }, })) +vi.mock("@hooks/use-page-section", () => ({ + usePageSection: () => ({ loading: false, failed: false, retry: () => {} }), +})) vi.mock("@hooks/use-hub-theme", () => ({ useHubTheme: () => ({ resolvedColorMode: "light" }), useOptionalHubTheme: () => null, diff --git a/hub/app/javascript/components/repository-selector/index.test.tsx b/hub/app/javascript/components/repository-selector/index.test.tsx index 9d19984..5730387 100644 --- a/hub/app/javascript/components/repository-selector/index.test.tsx +++ b/hub/app/javascript/components/repository-selector/index.test.tsx @@ -64,6 +64,52 @@ describe("RepositorySelector()", () => { expect(screen.getByRole("menuitem", { name: "github/empty" })).toBeInTheDocument() }) + it("shows loading and retry states without losing the current repository", () => { + const retry = vi.fn() + const status = { + loading: true, + failed: false, + loadingLabel: "Loading repositories...", + errorLabel: "Hub could not load all repository choices.", + retryLabel: "Retry repository choices", + onRetry: retry, + } + const { rerender } = render() + expect(screen.getByRole("status")).toHaveTextContent(status.loadingLabel) + fireEvent.click(screen.getByRole("button", { name: data.ariaLabel })) + expect(screen.getByRole("menuitem", { name: status.loadingLabel })).toHaveAttribute( + "aria-disabled", + "true", + ) + expect(screen.getByRole("link", { name: "github/prototype" })).toBeInTheDocument() + + rerender() + expect(screen.getByRole("status")).toHaveTextContent(status.errorLabel) + expect(screen.getByRole("menuitem", { name: status.errorLabel })).toBeVisible() + fireEvent.click(screen.getByRole("menuitem", { name: status.retryLabel })) + expect(retry).toHaveBeenCalledOnce() + }) + + it.each(["Enter", " "])("retries repository choices with %s", (key) => { + const retry = vi.fn() + render( + , + ) + fireEvent.click(screen.getByRole("button", { name: data.ariaLabel })) + fireEvent.keyDown(screen.getByRole("menuitem", { name: "Retry repository choices" }), { key }) + expect(retry).toHaveBeenCalledOnce() + }) + it.each(["Enter", " "])("activates a repository link with %s", (key) => { const click = vi.spyOn(HTMLAnchorElement.prototype, "click").mockImplementation(() => {}) render() diff --git a/hub/app/javascript/components/repository-selector/index.tsx b/hub/app/javascript/components/repository-selector/index.tsx index a70e5df..fede300 100644 --- a/hub/app/javascript/components/repository-selector/index.tsx +++ b/hub/app/javascript/components/repository-selector/index.tsx @@ -1,13 +1,11 @@ import { CheckIcon, RepoIcon } from "@primer/octicons-react" import { ActionMenu } from "@primer/react-brand" -import "./styles.css" +import { HubSpinner } from "@components/hub-spinner" -export interface RepositorySelectorRepository { - id: string - nameWithOwner: string - path: string -} +import type { RepositorySelectorRepository, RepositorySelectorStatus } from "./types" + +import "./styles.css" /** Switches between repositories already available in Replica Hub. */ export function RepositorySelector({ @@ -16,12 +14,14 @@ export function RepositorySelector({ menuLabel, repositories, selectedRepositoryId, + status, }: { ariaLabel: string collapsed?: boolean menuLabel: string repositories: RepositorySelectorRepository[] selectedRepositoryId: string + status?: RepositorySelectorStatus }) { const selectedRepository = repositories.find( (repository) => repository.id === selectedRepositoryId, @@ -44,6 +44,26 @@ export function RepositorySelector({ )} + {status?.loading ? ( + }> + {status.loadingLabel} + + ) : status?.failed ? ( + {status.errorLabel} + ) : null} + {!status?.loading && status?.failed ? ( + { + if (event.key === "Enter" || event.key === " ") { + event.preventDefault() + status.onRetry?.() + } + }} + > + {status.retryLabel} + + ) : null} {repositories.map((repository) => ( + {status?.loading || status?.failed ? ( + + {status.loading ? status.loadingLabel : status.errorLabel} + + ) : null} ) } diff --git a/hub/app/javascript/components/repository-selector/types.ts b/hub/app/javascript/components/repository-selector/types.ts new file mode 100644 index 0000000..36ca4ac --- /dev/null +++ b/hub/app/javascript/components/repository-selector/types.ts @@ -0,0 +1,14 @@ +export interface RepositorySelectorRepository { + id: string + nameWithOwner: string + path: string +} + +export interface RepositorySelectorStatus { + loading: boolean + failed: boolean + loadingLabel: string + errorLabel: string + retryLabel: string + onRetry?: () => void +} diff --git a/hub/app/javascript/features/prototype-metadata-management/index.test.tsx b/hub/app/javascript/features/prototype-metadata-management/index.test.tsx index d92a6e1..7dc86db 100644 --- a/hub/app/javascript/features/prototype-metadata-management/index.test.tsx +++ b/hub/app/javascript/features/prototype-metadata-management/index.test.tsx @@ -1,5 +1,5 @@ import { fireEvent, render, screen, within } from "@testing-library/react" -import { describe, it } from "vitest" +import { describe, it, vi } from "vitest" import { PrototypeMetadataManagement } from "." import type { PrototypeMetadataManagementData } from "./types" @@ -29,6 +29,9 @@ const data: PrototypeMetadataManagementData = { descriptionMaximumLength: 500, candidateCheckFailed: false, candidateCheckFailedLabel: "Hub could not check which branches you can add.", + candidateLoadingLabel: "Checking branches you can add...", + candidateRetryLabel: "Retry branch check", + candidateDataPath: "/repositories/2001/prototypes/manage?props_at=data.management", candidatePage: 1, candidatePageCount: 1, candidatePagePath: "/repositories/2001/prototypes/manage", @@ -68,6 +71,30 @@ HTMLDialogElement.prototype.close = function () { } describe("PrototypeMetadataManagement()", () => { + it("shows loading instead of empty results while keeping registered edits available", () => { + render() + expect(screen.getByRole("status")).toHaveTextContent(data.candidateLoadingLabel) + expect(screen.queryByText(data.emptyLabel)).not.toBeInTheDocument() + expect( + screen.queryByRole("button", { name: "Add to Hub: prototype/new" }), + ).not.toBeInTheDocument() + expect(screen.getByRole("button", { name: "Edit details: prototype/welcome" })).toBeEnabled() + }) + + it("offers an explicit retry when the async branch request fails", () => { + const retry = vi.fn() + render( + , + ) + expect(screen.getByText(data.candidateCheckFailedLabel)).toBeVisible() + fireEvent.click(screen.getByRole("button", { name: data.candidateRetryLabel })) + expect(retry).toHaveBeenCalledOnce() + }) it("shows repository metadata import errors in the editor", () => { const message = "Could not import .prototype: fix the file or set the start page here." render( diff --git a/hub/app/javascript/features/prototype-metadata-management/index.tsx b/hub/app/javascript/features/prototype-metadata-management/index.tsx index 7899232..7ce2828 100644 --- a/hub/app/javascript/features/prototype-metadata-management/index.tsx +++ b/hub/app/javascript/features/prototype-metadata-management/index.tsx @@ -6,6 +6,7 @@ import { Button, FormControl, Heading, Pagination, Text, TextInput } from "@prim import { HubAlert } from "@components/hub-alert" import { HubIconButton } from "@components/hub-icon-button" +import { HubSpinner } from "@components/hub-spinner" import type { PrototypeMetadataEntry, PrototypeMetadataManagementData } from "./types" @@ -15,9 +16,15 @@ import "./styles.css" export function PrototypeMetadataManagement({ authenticityToken, data, + loading = false, + loadFailed = false, + onRetry, }: { authenticityToken: string data: PrototypeMetadataManagementData + loading?: boolean + loadFailed?: boolean + onRetry?: () => void }) { const [selected, setSelected] = useState<{ entry: PrototypeMetadataEntry @@ -53,12 +60,28 @@ export function PrototypeMetadataManagement({ -
+
{data.unlistedHeading} - {data.candidateCheckFailed ? ( - + {loading ? ( +
+ + {data.candidateLoadingLabel} +
+ ) : loadFailed || data.candidateCheckFailed ? ( + <> + + {onRetry ? ( + + ) : null} + ) : data.unlisted.length ? ( <> {data.unlistedDescription} @@ -83,7 +106,7 @@ export function PrototypeMetadataManagement({ {data.emptyLabel} )} - {!data.candidateCheckFailed && data.candidatePageCount > 1 ? ( + {!loading && !loadFailed && !data.candidateCheckFailed && data.candidatePageCount > 1 ? ( void) => ReactNode onInboxClick?: MouseEventHandler onCollapsedChange: (collapsed: boolean) => void + repositoryChoices?: RepositorySelectorStatus }) { return ( ) } diff --git a/hub/app/javascript/features/repository-workspace/index.tsx b/hub/app/javascript/features/repository-workspace/index.tsx index bae773d..1641002 100644 --- a/hub/app/javascript/features/repository-workspace/index.tsx +++ b/hub/app/javascript/features/repository-workspace/index.tsx @@ -1,5 +1,6 @@ import type { MouseEventHandler, PropsWithChildren, ReactNode } from "react" +import type { RepositorySelectorStatus } from "@components/repository-selector/types" import { RepositoryNavigation } from "@features/repository-navigation" import { WorkspaceFrame } from "@features/workspace-frame" @@ -12,11 +13,13 @@ export function RepositoryWorkspace({ defaultNavigationCollapsed = false, inboxPreview, onInboxClick, + repositoryChoices, }: PropsWithChildren<{ data: RepositoryWorkspaceData defaultNavigationCollapsed?: boolean inboxPreview?: (close: () => void) => ReactNode onInboxClick?: MouseEventHandler + repositoryChoices?: RepositorySelectorStatus }>) { return ( )} > diff --git a/hub/app/javascript/features/workspace-sidebar/index.tsx b/hub/app/javascript/features/workspace-sidebar/index.tsx index 2077c07..c882536 100644 --- a/hub/app/javascript/features/workspace-sidebar/index.tsx +++ b/hub/app/javascript/features/workspace-sidebar/index.tsx @@ -19,6 +19,7 @@ import { Tooltip } from "@primer/react-brand" import { RepositoryNavigationItem } from "@components/repository-navigation-item" import { RepositorySelector } from "@components/repository-selector" +import type { RepositorySelectorStatus } from "@components/repository-selector/types" import { WorkspaceNavigation } from "@components/workspace-navigation" import type { WorkspaceSidebarData } from "./types" @@ -39,12 +40,14 @@ export function WorkspaceSidebar({ inboxPreview, onInboxClick, onCollapsedChange, + repositoryChoices, }: { collapsed: boolean data: WorkspaceSidebarData inboxPreview?: (close: () => void) => ReactNode onInboxClick?: MouseEventHandler onCollapsedChange: (collapsed: boolean) => void + repositoryChoices?: RepositorySelectorStatus }) { const repository = data.repository const repositoryLinks = repository @@ -107,6 +110,7 @@ export function WorkspaceSidebar({ menuLabel={repository.repositoryMenuLabel} repositories={repository.repositories} selectedRepositoryId={repository.selectedRepositoryId} + status={repositoryChoices} /> ) : null } diff --git a/hub/app/javascript/hooks/use-page-section/index.test.tsx b/hub/app/javascript/hooks/use-page-section/index.test.tsx new file mode 100644 index 0000000..67a590b --- /dev/null +++ b/hub/app/javascript/hooks/use-page-section/index.test.tsx @@ -0,0 +1,145 @@ +import { act, renderHook, waitFor } from "@testing-library/react" +import { NavigationContext } from "@thoughtbot/superglue" +import type { Meta } from "@thoughtbot/superglue" +import type * as Superglue from "@thoughtbot/superglue" +import type { ApplicationRemote } from "@thoughtbot/superglue" +import type { PropsWithChildren } from "react" +import { describe, expect, it, vi } from "vitest" + +import { usePageSection } from "." + +vi.mock("@thoughtbot/superglue", async (importOriginal) => { + const original = await importOriginal() + return { + ...original, + useSuperglue: () => ({ currentPageKey: "/repositories/2001/prototypes/manage" }), + } +}) + +function metadata(): Meta { + const page = { + data: {}, + componentIdentifier: "prototype_management/show", + assets: [], + defers: [], + fragments: [], + slices: {}, + renderedAt: 1, + restoreStrategy: "revisitOnly" as const, + } + return { + page, + pageKey: "/repositories/2001/prototypes/manage", + redirected: false, + needsRefresh: false, + componentIdentifier: page.componentIdentifier, + rsp: new Response(), + fetchArgs: ["/repositories/2001/prototypes/manage", { method: "GET" }], + } +} + +describe("usePageSection()", () => { + it("loads independent sections without waiting for one another", async () => { + let finishBranches!: (result: Meta) => void + const branches = new Promise((resolve) => { + finishBranches = resolve + }) + const remote = vi + .fn() + .mockImplementation((path: string) => + path.includes("management") ? branches : Promise.resolve(metadata()), + ) + function Wrapper({ children }: PropsWithChildren) { + return ( + + {children} + + ) + } + const { result } = renderHook( + () => ({ + branches: usePageSection("/manage?props_at=data.management", "shell"), + repositories: usePageSection("/manage?props_at=data.repositoryChoices", "shell"), + }), + { wrapper: Wrapper }, + ) + expect(remote).toHaveBeenCalledTimes(2) + await waitFor(() => expect(result.current.repositories.loading).toBe(false)) + expect(result.current.branches.loading).toBe(true) + await act(async () => { + finishBranches(metadata()) + await branches + }) + expect(result.current.branches.loading).toBe(false) + expect(result.current.branches.failed).toBe(false) + }) + + it("reports a failed branch request and retries without reloading repository choices", async () => { + const remote = vi + .fn() + .mockRejectedValueOnce(new Error("Unavailable")) + .mockResolvedValue(metadata()) + function Wrapper({ children }: PropsWithChildren) { + return ( + + {children} + + ) + } + const { result } = renderHook( + () => ({ + branches: usePageSection("/manage?props_at=data.management", "shell"), + repositories: usePageSection("/manage?props_at=data.repositoryChoices", "shell"), + }), + { wrapper: Wrapper }, + ) + await waitFor(() => expect(result.current.branches.failed).toBe(true)) + await waitFor(() => expect(result.current.repositories.loading).toBe(false)) + expect(remote).toHaveBeenCalledTimes(2) + act(() => result.current.branches.retry()) + await waitFor(() => expect(result.current.branches.loading).toBe(false)) + expect(result.current.branches.failed).toBe(false) + expect(remote).toHaveBeenCalledTimes(3) + expect(remote.mock.calls.filter(([path]) => path.includes("repositoryChoices"))).toHaveLength(1) + }) + + it("aborts superseded and unmounted requests and ignores late results", async () => { + let finishFirst!: (result: Meta) => void + const first = new Promise((resolve) => { + finishFirst = resolve + }) + const remote = vi + .fn() + .mockReturnValueOnce(first) + .mockResolvedValue(metadata()) + function Wrapper({ children }: PropsWithChildren) { + return ( + + {children} + + ) + } + const { result, rerender, unmount } = renderHook(({ path }) => usePageSection(path, "shell"), { + initialProps: { path: "/manage?page=1&props_at=data.management" }, + wrapper: Wrapper, + }) + const firstSignal = remote.mock.calls[0]?.[1]?.signal + rerender({ path: "/manage?page=2&props_at=data.management" }) + expect(firstSignal?.aborted).toBe(true) + await waitFor(() => expect(result.current.loading).toBe(false)) + await act(async () => { + finishFirst({ ...metadata(), needsRefresh: true }) + await first + }) + expect(result.current.failed).toBe(false) + const lastSignal = remote.mock.calls[1]?.[1]?.signal + unmount() + expect(lastSignal?.aborted).toBe(true) + }) +}) diff --git a/hub/app/javascript/hooks/use-page-section/index.ts b/hub/app/javascript/hooks/use-page-section/index.ts new file mode 100644 index 0000000..3097f0a --- /dev/null +++ b/hub/app/javascript/hooks/use-page-section/index.ts @@ -0,0 +1,34 @@ +import { useCallback, useContext, useEffect, useState } from "react" +import { NavigationContext, useSuperglue } from "@thoughtbot/superglue" + +/** + * Loads an authorized section through Superglue's partial page updates without blocking navigation. + * Use it for slow sections that can load independently while the rest of the page stays usable. + * Superglue updates the page props for rendering; this hook only returns loading, failure, and retry state. + */ +export function usePageSection(path: string, loadId: string) { + const { remote } = useContext(NavigationContext) + const { currentPageKey } = useSuperglue() + const [attempt, setAttempt] = useState(0) + const [result, setResult] = useState<{ key: string; failed: boolean } | null>(null) + const key = `${loadId}:${currentPageKey}:${path}:${attempt}` + + useEffect(() => { + const controller = new AbortController() + void remote(path, { pageKey: currentPageKey, signal: controller.signal }) + .then((metadata) => { + if (!controller.signal.aborted) setResult({ key, failed: metadata.needsRefresh }) + }) + .catch(() => { + if (!controller.signal.aborted) setResult({ key, failed: true }) + }) + return () => controller.abort() + }, [currentPageKey, key, path, remote]) + + const retry = useCallback(() => setAttempt((previous) => previous + 1), []) + return { + loading: result?.key !== key, + failed: result?.key === key && result.failed, + retry, + } +} diff --git a/hub/app/javascript/pages/prototype-management-page/index.tsx b/hub/app/javascript/pages/prototype-management-page/index.tsx index 32b8ecf..19f0a55 100644 --- a/hub/app/javascript/pages/prototype-management-page/index.tsx +++ b/hub/app/javascript/pages/prototype-management-page/index.tsx @@ -6,14 +6,30 @@ import { HubIconButton } from "@components/hub-icon-button" import { PrototypeMetadataManagement } from "@features/prototype-metadata-management" import { RepositoryWorkspace } from "@features/repository-workspace" -import type { PrototypeManagementPageData } from "./types" +import type { PageSectionState, PrototypeManagementPageData } from "./types" import "./styles.css" /** Gives repository writers one place to add branches and edit gallery details. */ -export function PrototypeManagementPage({ data }: { data: PrototypeManagementPageData }) { +export function PrototypeManagementPage({ + data, + candidates, + repositoryChoices, +}: { + data: PrototypeManagementPageData + candidates?: PageSectionState + repositoryChoices?: PageSectionState +}) { return ( - +
diff --git a/hub/app/javascript/pages/prototype-management-page/stories.tsx b/hub/app/javascript/pages/prototype-management-page/stories.tsx index a01777e..bd3f6fa 100644 --- a/hub/app/javascript/pages/prototype-management-page/stories.tsx +++ b/hub/app/javascript/pages/prototype-management-page/stories.tsx @@ -22,6 +22,15 @@ const data: PrototypeManagementPageData = { repositoryMenuLabel: "Repositories", repositorySelectorLabel: "Choose repository. github/prototype selected", selectedRepositoryId: "2001", + sectionLoadId: "management-story", + repositoryChoices: { + repositories: [githubPrototype, githubEmpty], + path: "/repositories/2001/prototypes/manage?props_at=data.repositoryChoices", + loadingLabel: "Loading repositories...", + errorLabel: "Hub could not load all repository choices.", + retryLabel: "Retry repository choices", + failed: false, + }, management: { heading: "Manage prototypes", unlistedHeading: "Available branches", @@ -34,6 +43,9 @@ const data: PrototypeManagementPageData = { "No branches available to add. When you create a new branch, it will appear here so you can add it to Hub.", candidateCheckFailed: false, candidateCheckFailedLabel: "Hub could not check which branches you can add.", + candidateLoadingLabel: "Checking branches you can add...", + candidateRetryLabel: "Retry branch check", + candidateDataPath: "/repositories/2001/prototypes/manage?props_at=data.management", candidatePage: 1, candidatePageCount: 1, candidatePagePath: "/repositories/2001/prototypes/manage", @@ -107,6 +119,25 @@ type Story = StoryObj export const Multiple: Story = {} +export const Loading: Story = { + args: { + candidates: { loading: true, failed: false, retry: () => {} }, + repositoryChoices: { loading: true, failed: false, retry: () => {} }, + data: { + ...data, + repositoryChoices: { ...data.repositoryChoices, repositories: [githubPrototype] }, + management: { ...data.management, unlisted: [], candidatePageCount: 0 }, + }, + }, +} + +export const Retry: Story = { + args: { + candidates: { loading: false, failed: true, retry: () => {} }, + repositoryChoices: { loading: false, failed: true, retry: () => {} }, + }, +} + export const EditStartPage: Story = { play: async ({ canvas, userEvent }) => { await userEvent.click(canvas.getByRole("button", { name: "Edit details: prototype/welcome" })) @@ -201,6 +232,10 @@ export const Local: Story = { repositories: [{ id: "1", nameWithOwner: "local/example", path: "/repositories/1" }], repositorySelectorLabel: "Choose repository. local/example selected", selectedRepositoryId: "1", + repositoryChoices: { + ...data.repositoryChoices, + repositories: [{ id: "1", nameWithOwner: "local/example", path: "/repositories/1" }], + }, management: { ...data.management, candidatePagePath: "/repositories/1/prototypes/manage", diff --git a/hub/app/javascript/pages/prototype-management-page/types.ts b/hub/app/javascript/pages/prototype-management-page/types.ts index 170a8e2..d3ce832 100644 --- a/hub/app/javascript/pages/prototype-management-page/types.ts +++ b/hub/app/javascript/pages/prototype-management-page/types.ts @@ -1,9 +1,25 @@ +import type { RepositorySelectorRepository } from "@components/repository-selector/types" import type { RepositoryWorkspaceData } from "@features/repository-workspace/types" import type { PrototypeMetadataManagementData } from "@features/prototype-metadata-management/types" +export interface PageSectionState { + loading: boolean + failed: boolean + retry: () => void +} + export interface PrototypeManagementPageData extends RepositoryWorkspaceData { alert: string | null backLabel: string management: PrototypeMetadataManagementData pageTitle: string + sectionLoadId: string + repositoryChoices: { + repositories: RepositorySelectorRepository[] + path: string + loadingLabel: string + errorLabel: string + retryLabel: string + failed: boolean + } } diff --git a/hub/app/models/github_client/batch.rb b/hub/app/models/github_client/batch.rb new file mode 100644 index 0000000..0b62a62 --- /dev/null +++ b/hub/app/models/github_client/batch.rb @@ -0,0 +1,20 @@ +require "concurrent" + +# Bounds independent GitHub calls without moving database work into worker threads. +class GithubClient::Batch + MAX_CONCURRENCY = 4 + + # Returns results in input order and raises failures after draining the workers. + def self.map(items, &operation) + return [] if items.empty? + + executor = Concurrent::FixedThreadPool.new([ items.size, MAX_CONCURRENCY ].min) + futures = items.map do |item| + Concurrent::Promises.future_on(executor) { operation.call(item) } + end + futures.map(&:value!) + ensure + executor&.shutdown + executor&.wait_for_termination + end +end diff --git a/hub/app/models/github_client/repositories.rb b/hub/app/models/github_client/repositories.rb index 8d5d4eb..2f13915 100644 --- a/hub/app/models/github_client/repositories.rb +++ b/hub/app/models/github_client/repositories.rb @@ -60,6 +60,15 @@ def mention_candidates(repository, token:) raise GithubClient::InvalidResponse, "GitHub returned an incomplete collaborator list" end + # Fetches independent permissions while retaining each repository's failure. + def permissions(repositories, user, token:) + GithubClient::Batch.map(repositories) do |repository| + [ repository.github_id, permission(repository, user, token:) ] + rescue GithubClient::Error => error + [ repository.github_id, error ] + end.to_h + end + # Lists every current branch and head commit in one repository. def branches(repository, token:) branches = [] diff --git a/hub/app/models/repository/authorization.rb b/hub/app/models/repository/authorization.rb index f745b0b..2941142 100644 --- a/hub/app/models/repository/authorization.rb +++ b/hub/app/models/repository/authorization.rb @@ -22,7 +22,7 @@ def authorized?( # Returns the user's current normalized GitHub permission. def permission_for( user, fresh: false, configuration: GithubAppConfiguration.current, - cache: Rails.cache + cache: Rails.cache, payload: nil ) return "none" if record.installation_state != "active" if LocalDevelopment.enabled? @@ -37,11 +37,16 @@ def permission_for( fetch_permission(user, configuration) else cache.fetch(cache_key(user), expires_in: CACHE_TTL) do - fetch_permission(user, configuration) + fetch_permission(user, configuration, payload:) end end end + # Avoids fetching selector permissions that are still cached. + def cached_permission(user, cache: Rails.cache) + cache.read(cache_key(user)) + end + # Reports whether one normalized permission meets the required level. def sufficient?(permission, minimum:) PERMISSION_LEVELS.fetch(permission.to_s, 0) >= PERMISSION_LEVELS.fetch(minimum.to_s) @@ -50,10 +55,12 @@ def sufficient?(permission, minimum:) private # Reads effective permission and verifies GitHub returned the expected user. - def fetch_permission(user, configuration) + def fetch_permission(user, configuration, payload: nil) return "none" if configuration.nil? || configuration.installation_state != "active" - payload = configuration.github_client.repositories.permission \ + raise payload if payload.is_a?(GithubClient::Error) + + payload ||= configuration.github_client.repositories.permission \ record, user, token: configuration.tokens.installation diff --git a/hub/app/models/repository/prototype_candidates.rb b/hub/app/models/repository/prototype_candidates.rb index 80a39aa..2cfd14f 100644 --- a/hub/app/models/repository/prototype_candidates.rb +++ b/hub/app/models/repository/prototype_candidates.rb @@ -5,15 +5,18 @@ class Repository::PrototypeCandidates < ActiveRecord::AssociatedObject CANDIDATES_PER_PAGE = 10 # Paginates eligible branches, leaving registration checks fresh. - def available_page(user, permission:, page: 1) + def available_page(user, permission:, page: 1, configuration: GithubAppConfiguration.current) return [ [], 0 ] if !record.authorization.sufficient?(permission, minimum: :write) candidates = candidate_branches.to_a return [ [], 0 ] if candidates.empty? - heads = current_heads + heads = current_heads(configuration:) + authors = if !record.local? && !record.authorization.sufficient?(permission, minimum: :admin) + warm_authors(candidates, heads, configuration:) + end eligible = candidates.select do |prototype| - eligible?(prototype, user, permission:, heads:) + eligible?(prototype, user, permission:, heads:, authors:) end page_count = (eligible.size + CANDIDATES_PER_PAGE - 1) / CANDIDATES_PER_PAGE [ eligible.slice((page - 1) * CANDIDATES_PER_PAGE, CANDIDATES_PER_PAGE) || [], page_count ] @@ -76,12 +79,12 @@ def recently_active_by(user, permission:) end # Rechecks one branch at registration, independent of the page's filtered list. - def eligible?(prototype, user, permission:, heads: nil, fresh: false) - registration_ineligibility_reason(prototype, user, permission:, heads:, fresh:).nil? + def eligible?(prototype, user, permission:, heads: nil, fresh: false, authors: nil) + registration_ineligibility_reason(prototype, user, permission:, heads:, fresh:, authors:).nil? end # Explains the first failed registration check without changing eligibility. - def registration_ineligibility_reason(prototype, user, permission:, heads: nil, fresh: false) + def registration_ineligibility_reason(prototype, user, permission:, heads: nil, fresh: false, authors: nil) return "write_access_required" if !record.authorization.sufficient?(permission, minimum: :write) return "repository_mismatch" if prototype.repository_id != record.id return "remote_branch_missing" if prototype.branch_deleted_at.present? @@ -96,7 +99,8 @@ def registration_ineligibility_reason(prototype, user, permission:, heads: nil, default_head = heads[record.default_branch] raise GithubClient::InvalidResponse, "GitHub omitted the default branch" if default_head.nil? - "branch_authorship_required" unless author_ids(default_head, head, fresh:).include?(user.github_id) + ids = authors && !fresh ? authors.fetch(head) : author_ids(default_head, head, fresh:) + "branch_authorship_required" unless ids.include?(user.github_id) end private @@ -107,14 +111,13 @@ def candidate_branches end # Uses current heads, not persisted Builds, to exclude deleted and moved branches. - def current_heads + def current_heads(configuration: GithubAppConfiguration.current) if record.local? refs = Repository::LocalDiscovery.git!("-C", record.local_path, "for-each-ref", "--format=%(refname:short) %(objectname)", "refs/heads") return refs.each_line.to_h { |line| line.strip.split(" ", 2) } end - configuration = GithubAppConfiguration.current raise GithubClient::InvalidResponse, "GitHub App is not configured" if configuration.nil? configuration.github_client.repositories.branches( @@ -142,22 +145,55 @@ def author_ids(base, head, fresh:) return compare_authors(base, head) if fresh Rails.cache.fetch( - [ "prototype-branch-authors", record.github_id, base, head ], + author_cache_key(base, head), expires_in: AUTHOR_CACHE_TTL ) { compare_authors(base, head) } end + # Retains successful comparisons on this thread before surfacing any batch failure. + def warm_authors(candidates, heads, configuration:) + base = heads[record.default_branch] + raise GithubClient::InvalidResponse, "GitHub omitted the default branch" if base.nil? + + live_heads = candidates.filter_map { heads[_1.branch_name] }.uniq + cached = Rails.cache.read_multi(*live_heads.map { author_cache_key(base, _1) }) + missing = live_heads.reject { cached.key?(author_cache_key(base, _1)) } + if missing.any? + token = configuration.tokens.installation + failures = Queue.new + comparisons = GithubClient::Batch.map(missing) do |head| + next if !failures.empty? + + [ head, compare_authors(base, head, configuration:, token:) ] + rescue GithubClient::Error => error + failures << error + nil + end + comparisons.compact.each do |head, authors| + cached[author_cache_key(base, head)] = authors + Rails.cache.write(author_cache_key(base, head), authors, expires_in: AUTHOR_CACHE_TTL) + end + raise failures.pop if !failures.empty? + end + live_heads.to_h { |head| [ head, cached.fetch(author_cache_key(base, head)) ] } + end + + # Separates comparisons by the exact default and branch commits. + def author_cache_key(base, head) + [ "prototype-branch-authors", record.github_id, base, head ] + end + # Paginates the entire branch-only history; the latest Build author is not enough. - def compare_authors(base, head) - configuration = GithubAppConfiguration.current + def compare_authors(base, head, configuration: GithubAppConfiguration.current, token: nil) raise GithubClient::InvalidResponse, "GitHub App is not configured" if configuration.nil? + token ||= configuration.tokens.installation authors = [] seen = 0 page = 1 loop do comparison = configuration.github_client.repositories.compare( - record, base, head, token: configuration.tokens.installation, page: + record, base, head, token:, page: ) if !comparison.is_a?(Hash) raise GithubClient::InvalidResponse, "GitHub returned an incomplete branch comparison" diff --git a/hub/app/views/prototype_management/show.json.props b/hub/app/views/prototype_management/show.json.props index ff22564..34b7a28 100644 --- a/hub/app/views/prototype_management/show.json.props +++ b/hub/app/views/prototype_management/show.json.props @@ -1,43 +1,61 @@ -json.pageTitle t("repository_workspace.prototypes") -json.signOutLabel t("authenticated.sign_out") -json.accountMenuLabel t("authenticated.account_menu", login: Current.user.login) -json.partial! partial: "shared/hub_administration_navigation" -json.collapseNavigationLabel t("repository_workspace.collapse_navigation") -json.expandNavigationLabel t("repository_workspace.expand_navigation") -json.systemThemeLabel t("repository_workspace.theme_system") -json.lightThemeLabel t("repository_workspace.theme_light") -json.darkThemeLabel t("repository_workspace.theme_dark") -json.signOutPath session_path -json.authenticityToken form_authenticity_token +unless request.format.json? && params[:props_at].present? + json.sectionLoadId SecureRandom.uuid + json.pageTitle t("repository_workspace.prototypes") + json.signOutLabel t("authenticated.sign_out") + json.accountMenuLabel t("authenticated.account_menu", login: Current.user.login) + json.partial! partial: "shared/hub_administration_navigation" + json.collapseNavigationLabel t("repository_workspace.collapse_navigation") + json.expandNavigationLabel t("repository_workspace.expand_navigation") + json.systemThemeLabel t("repository_workspace.theme_system") + json.lightThemeLabel t("repository_workspace.theme_light") + json.darkThemeLabel t("repository_workspace.theme_dark") + json.signOutPath session_path + json.authenticityToken form_authenticity_token -json.currentUser do - json.login Current.user.login - json.name Current.user.name - json.avatarUrl Current.user.avatar_url -end + json.currentUser do + json.login Current.user.login + json.name Current.user.name + json.avatarUrl Current.user.avatar_url + end -json.repositorySelectorLabel t \ - "repository_workspace.repository_selector", - repository: @repository.name_with_owner -json.repositoryMenuLabel t("repository_workspace.repositories") -json.navigationLabel t("repository_workspace.navigation") -json.newPrototypeLabel t("repository_workspace.new_prototype") -json.newPrototypePath repository_new_prototype_path(@repository.route_id) -json.prototypesLabel t("repository_workspace.prototypes") -json.prototypesPath repository_path(@repository.route_id) -json.partial! partial: "shared/analytics_navigation" -json.selectedRepositoryId @repository.route_id.to_s + json.repositorySelectorLabel t \ + "repository_workspace.repository_selector", + repository: @repository.name_with_owner + json.repositoryMenuLabel t("repository_workspace.repositories") + json.navigationLabel t("repository_workspace.navigation") + json.newPrototypeLabel t("repository_workspace.new_prototype") + json.newPrototypePath repository_new_prototype_path(@repository.route_id) + json.prototypesLabel t("repository_workspace.prototypes") + json.prototypesPath repository_path(@repository.route_id) + json.partial! partial: "shared/analytics_navigation" + json.selectedRepositoryId @repository.route_id.to_s -json.repositories do - json.array! @repositories do |repository| - json.id repository.route_id.to_s - json.nameWithOwner repository.name_with_owner - json.path repository_path(repository.route_id) + json.repositories do + json.array! @repositories do |repository| + json.id repository.route_id.to_s + json.nameWithOwner repository.name_with_owner + json.path repository_path(repository.route_id) + end end + + json.alert flash[:alert] + json.backLabel t("prototype_management.show.back_label") end -json.alert flash[:alert] -json.backLabel t("prototype_management.show.back_label") +json.repositoryChoices do + json.path repository_prototype_management_path(@repository.route_id, props_at: "data.repositoryChoices") + json.loadingLabel t("repository_workspace.loading_repositories") + json.errorLabel t("repository_workspace.repositories_unavailable") + json.retryLabel t("repository_workspace.retry") + json.failed !!@repository_navigation_unavailable + json.repositories do + json.array! @repositories do |repository| + json.id repository.route_id.to_s + json.nameWithOwner repository.name_with_owner + json.path repository_path(repository.route_id) + end + end +end json.management do %w[ @@ -52,6 +70,12 @@ json.management do json.descriptionMaximumLength Prototype::DESCRIPTION_MAXIMUM_LENGTH json.candidateCheckFailed !!@candidate_check_failed json.candidateCheckFailedLabel t("repositories.show.candidate_check_failed") + json.candidateLoadingLabel t("prototype_management.show.loading_branches") + json.candidateRetryLabel t("prototype_management.show.retry") + json.candidateDataPath repository_prototype_management_path \ + @repository.route_id, + page: @candidate_page, + props_at: "data.management" json.candidatePage @candidate_page json.candidatePageCount @candidate_page_count json.candidatePagePath @candidate_page_path diff --git a/hub/app/views/prototype_management/show.tsx b/hub/app/views/prototype_management/show.tsx index 00cb1a1..80d906a 100644 --- a/hub/app/views/prototype_management/show.tsx +++ b/hub/app/views/prototype_management/show.tsx @@ -1,13 +1,16 @@ import { usePageData } from "@hooks/use-page-data" import { useRepositoryInbox } from "@hooks/use-repository-inbox" +import { usePageSection } from "@hooks/use-page-section" import { PrototypeManagementPage } from "@pages/prototype-management-page" import type { PrototypeManagementPageData } from "@pages/prototype-management-page/types" export default function PrototypeManagementShowView() { + const data = useRepositoryInbox(usePageData()) + const candidates = usePageSection(data.management.candidateDataPath, data.sectionLoadId) + const repositories = usePageSection(data.repositoryChoices.path, data.sectionLoadId) + return ( - ())} - /> + ) } diff --git a/hub/config/locales/gallery.en.yml b/hub/config/locales/gallery.en.yml index cff79e3..d05bb29 100644 --- a/hub/config/locales/gallery.en.yml +++ b/hub/config/locales/gallery.en.yml @@ -96,6 +96,8 @@ en: edit_dialog_heading: Edit prototype details close_dialog_label: Close dialog back_label: Back to prototypes + loading_branches: Checking branches you can add... + retry: Retry branch check repository_workspace: repositories: Repositories repository_selector: "Choose repository. %{repository} selected" @@ -107,6 +109,9 @@ en: theme_system: System theme_light: Light theme_dark: Dark + loading_repositories: Loading repositories... + repositories_unavailable: Hub could not load all repository choices. + retry: Retry repository choices new_prototypes: show: page_title: Create diff --git a/hub/docs/architecture.md b/hub/docs/architecture.md index f8bec8f..b46ea54 100644 --- a/hub/docs/architecture.md +++ b/hub/docs/architecture.md @@ -47,6 +47,8 @@ Rails models apply product rules and save changes. React handles the Hub interfa Rails TSX page views connect Superglue and browser workflows to typed presentation Pages. Vite discovers those views from their Rails virtual paths, while Storybook renders the same Pages with display-ready Oaken-based fixtures. +Manage prototypes checks access to the selected repository before rendering its listed prototypes. Available branches and other repository choices load independently through Superglue partial-page updates. Each request checks access again, shares an installation token, and runs at most four independent GitHub calls at a time. Database and cache writes stay on the request thread. If a branch comparison fails, Hub stops starting new comparisons, retains completed successes for retries, and reports the failure. Registration still checks permission, branch existence, and authorship afresh. + Prototype JavaScript runs at a separate preview address. Hub cookies and credentials never reach that address. A small preview bridge connects conversations to selected elements and highlighted regions. [Prototype controls](preview-serving.md#prototype-controls) use a separate controls-only bridge on the same authorized Build origins. Each Build serves its own versioned `controls.replica.json`; Hub and the Copilot plugin supply one shared injected engine and bindings adapter. Overrides stay in prototype URLs, not server state or private capability-bearing addresses. Screenshot capture uses the same adapter and retained defaults. diff --git a/hub/docs/gallery.md b/hub/docs/gallery.md index 69c81ff..22f748d 100644 --- a/hub/docs/gallery.md +++ b/hub/docs/gallery.md @@ -94,7 +94,9 @@ Hub uses GitHub's repository activity timestamps and actor identity, not commit Local checkouts have no GitHub push history. For the local identity, the same alerts instead say that Hub detected a branch locally. Hub saves the time it first observes a branch head and updates that time only when the observed head changes, including a reset to a previously built commit. Unchanged heads keep their age across scans, reloads, and restarts. Existing local branches receive their first observation on the next scan after upgrading. These alerts also expire after 12 hours and exclude registered or deleted branches. Commit dates and Build completion times do not determine local activity. -On the separate management page, Hub lists eligible branches ten per page. Page numbers reflect branches the current person can add, so ineligible branches do not leave empty pages. A repository admin can select any current, non-default branch except `replica/install-workflow`. Other people need current repository write access and a commit linked to their GitHub account that is unique to the branch relative to the default branch. Hub checks the full branch-only commit history, not just the latest Build. Deleted branches and branches not yet discovered by Hub cannot be added. The server checks eligibility again when the form is submitted; a failed GitHub check does not grant access. Local mode's one identity is an admin and does not call GitHub. +The management page opens after Hub checks access to the selected repository. Listed prototypes are available for editing while eligible branches and other repository choices load independently. Each section shows its loading state and offers a retry if its check fails; one failure does not block the other section. + +Hub lists eligible branches ten per page. Page numbers reflect branches the current person can add, so ineligible branches do not leave empty pages. A repository admin can select any current, non-default branch except `replica/install-workflow`. Other people need current repository write access and a commit linked to their GitHub account that is unique to the branch relative to the default branch. Hub checks the full branch-only commit history, not just the latest Build. Deleted branches and branches not yet discovered by Hub cannot be added. A requested page that no longer exists reports a failed branch check instead of an empty list. The server checks eligibility again when the form is submitted; a failed GitHub check does not grant access. Local mode's one identity is an admin and does not call GitHub. Select a branch, enter a name, optional description, and optional **Start page** in the dialog, then choose **Add to Hub**. The branch appears as a card for everyone with repository read access. Hub records the first person to add it as its author; later edits do not change that attribution. Any current repository writer can edit the same fields for a listed prototype, even if they did not author it. If Hub cannot check branch eligibility, the gallery and management page still show registered prototypes and a warning. Editing remains available, but adding branches waits until the check succeeds. diff --git a/hub/test/integration/local_development_test.rb b/hub/test/integration/local_development_test.rb index 147678d..d5ef633 100644 --- a/hub/test/integration/local_development_test.rb +++ b/hub/test/integration/local_development_test.rb @@ -1098,6 +1098,38 @@ class LocalDevelopmentTest < ActionDispatch::IntegrationTest end end + test "loads local management sections independently and rechecks branch existence before registration" do + path = create_repository("management") + git!(path, "branch", "prototype/local") + + in_local_mode do + post local_authentication_path + repository = Repository.find_by!(name_with_owner: "local/management") + prototype = prototypes.create(repository:, branch_name: "prototype/local") + management_path = repository_prototype_management_path(repository.route_id) + + get management_path + assert_response :success + assert_includes response.body, '"unlisted":[]' + + get management_path, params: { props_at: "data.repositoryChoices" }, as: :json + assert_response :success + assert_includes response.body, '"path":"data.repositoryChoices"' + assert_includes response.body, "local/management" + assert_not_includes response.body, "prototype/local" + + get management_path, params: { props_at: "data.management" }, as: :json + assert_response :success + assert_includes response.body, '"branchName":"prototype/local"' + git!(path, "branch", "-D", "prototype/local") + + patch repository_prototype_registration_path(repository.route_id, prototype), + params: { prototype: { name: "Deleted locally" } } + assert_response :not_found + assert_not prototype.reload.registered? + end + end + test "builds an exact commit and lists it after registration without changing the checkout" do SharingSetting.create!(id: 1, public_sharing_enabled: true) path = create_repository("one", buildable: true) @@ -1137,6 +1169,11 @@ class LocalDevelopmentTest < ActionDispatch::IntegrationTest get repository_prototype_management_path(repository.route_id) assert_response :success + assert_includes response.body, '"unlisted":[]' + get repository_prototype_management_path(repository.route_id), + params: { props_at: "data.management" }, + headers: { "Accept" => "application/json" } + assert_response :success assert_includes response.body, "prototype/welcome" patch repository_prototype_registration_path(repository.route_id, build.prototype), diff --git a/hub/test/integration/prototype_registrations_test.rb b/hub/test/integration/prototype_registrations_test.rb index 011de15..6c274fc 100644 --- a/hub/test/integration/prototype_registrations_test.rb +++ b/hub/test/integration/prototype_registrations_test.rb @@ -5,6 +5,184 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest github_app_configurations.create_installed end + test "renders the management shell without branches or other repository checks" do + payloads = [ repository_payload(2001, "github/prototype"), repository_payload(2002, "github/other") ] + stub_github_sign_in(repositories: payloads, permissions: { 2001 => "push", 2002 => "pull" }) + sign_in + repository = Repository.find_by!(github_id: 2001) + prototypes.create(repository:, branch_name: "prototype/listed").register!(name: "Listed") + prototypes.create(repository:, branch_name: "prototype/new") + WebMock.reset_executed_requests! + + load_ids = [ {}, { "Accept" => "application/json" } ].map do |headers| + get repository_prototype_management_path(repository.route_id), headers: headers + assert_response :success + assert_includes response.body, '"name":"Listed"' + assert_includes response.body, '"unlisted":[]' + assert_not_includes response.body, "github/other" + response.body[/"sectionLoadId":"([^"]+)"/, 1] + end + assert load_ids.all?(&:present?) + assert_equal 2, load_ids.uniq.size + assert_not_requested :get, %r{/branches} + assert_not_requested :get, %r{/compare/} + assert_not_requested :get, "#{GITHUB_API_URL}/repos/github/other/collaborators/octocat/permission" + end + + test "loads repository choices independently and excludes inaccessible repositories" do + payloads = [ repository_payload(2001, "github/prototype"), repository_payload(2002, "github/other"), + repository_payload(2003, "github/private") ] + stub_github_sign_in(repositories: payloads, permissions: { 2001 => "push", 2002 => "pull", 2003 => nil }) + sign_in + repository = Repository.find_by!(github_id: 2001) + prototypes.create(repository:, branch_name: "prototype/new") + WebMock.reset_executed_requests! + + get repository_prototype_management_path(repository.route_id), + params: { props_at: "data.repositoryChoices" }, + headers: { "Accept" => "application/json" } + + assert_response :success + assert_includes response.body, '"action":"graft"' + assert_includes response.body, '"path":"data.repositoryChoices"' + assert_includes response.body, "github/other" + assert_not_includes response.body, "github/private" + assert_not_includes response.body, "unlisted" + assert_not_includes response.body, "sectionLoadId" + assert_not_requested :get, %r{/branches} + assert_not_requested :get, %r{/compare/} + assert_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens", times: 1 + end + + test "rejects unsupported management sections without loading branch data" do + payload = repository_payload(2001, "github/prototype") + stub_github_sign_in(repositories: [ payload ], permissions: { 2001 => "push" }) + sign_in + repository = Repository.find_by!(github_id: 2001) + WebMock.reset_executed_requests! + + get repository_prototype_management_path(repository.route_id), + params: { props_at: "data.unknown" }, + headers: { "Accept" => "application/json" } + + assert_response :not_found + assert_not_requested :get, %r{/branches} + assert_not_requested :get, %r{/compare/} + end + + test "reports partial repository failures and permits a retry instead of caching them" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + payloads = [ repository_payload(2001, "github/prototype"), repository_payload(2002, "github/other") ] + stub_github_sign_in(repositories: payloads, permissions: { 2001 => "push", 2002 => "pull" }) + sign_in + repository = Repository.find_by!(github_id: 2001) + Rails.cache.clear + stub_github_permissions(payloads, { 2001 => "push", 2002 => :failure }) + path = repository_prototype_management_path(repository.route_id) + options = { params: { props_at: "data.repositoryChoices" }, headers: { "Accept" => "application/json" } } + + get path, **options + assert_response :success + assert_includes response.body, '"failed":true' + assert_not_includes response.body, "github/other" + + stub_github_permissions(payloads, { 2001 => "push", 2002 => "pull" }) + get path, **options + assert_response :success + assert_includes response.body, '"failed":false' + assert_includes response.body, "github/other" + ensure + Rails.cache = original_cache + end + + test "rechecks write access for both async sections" do + payload = repository_payload(2001, "github/prototype") + stub_github_sign_in(repositories: [ payload ], permissions: { 2001 => "push" }) + sign_in + repository = Repository.find_by!(github_id: 2001) + prototypes.create(repository:, branch_name: "prototype/listed").register!(name: "Listed") + get repository_prototype_management_path(repository.route_id) + assert_response :success + stub_github_permissions([ payload ], { 2001 => "pull" }) + + %w[data.management data.repositoryChoices].each do |section| + get repository_prototype_management_path(repository.route_id), + params: { props_at: section }, headers: { "Accept" => "application/json" } + assert_response :not_found + assert_not_includes response.body, "Listed" + end + end + + test "reuses comparison caches but checks authorship freshly on registration" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + payload = repository_payload(2001, "github/prototype") + stub_github_sign_in(repositories: [ payload ], permissions: { 2001 => "push" }) + sign_in + repository = Repository.find_by!(github_id: 2001) + branch = prototypes.create(repository:, branch_name: "prototype/new") + stub_branches(repository, "main" => "a" * 40, branch.branch_name => "b" * 40) + stub_comparison(repository, "a" * 40, "b" * 40, author_ids: [ 1001 ]) + get_management(repository) + assert_response :success + assert_includes response.body, branch.branch_name + WebMock.reset_executed_requests! + + get_management(repository) + assert_response :success + assert_includes response.body, branch.branch_name + assert_not_requested :get, %r{/compare/} + + stub_comparison(repository, "a" * 40, "b" * 40, author_ids: [ 1002 ]) + patch repository_prototype_registration_path(repository.route_id, branch), + params: { prototype: { name: "Not eligible now" } } + assert_response :not_found + assert_not branch.reload.registered? + assert_requested :get, %r{/compare/}, times: 1 + + stub_branches(repository, "main" => "c" * 40, branch.branch_name => "b" * 40) + stub_comparison(repository, "c" * 40, "b" * 40, author_ids: [ 1002 ]) + get_management(repository) + assert_response :success + assert_not_includes response.body, branch.branch_name + assert_requested :get, %r{/compare/#{"c" * 40}}, times: 1 + + stub_branches(repository, "main" => "c" * 40, branch.branch_name => "d" * 40) + stub_comparison(repository, "c" * 40, "d" * 40, author_ids: [ 1001 ]) + get_management(repository) + assert_response :success + assert_includes response.body, branch.branch_name + assert_requested :get, %r{/compare/#{"c" * 40}\.\.\.#{"d" * 40}}, times: 1 + ensure + Rails.cache = original_cache + end + + test "retries branch comparisons after an upstream failure instead of caching an empty result" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + payload = repository_payload(2001, "github/prototype") + stub_github_sign_in(repositories: [ payload ], permissions: { 2001 => "push" }) + sign_in + repository = Repository.find_by!(github_id: 2001) + prototype = prototypes.create(repository:, branch_name: "prototype/retry") + stub_branches(repository, "main" => "a" * 40, prototype.branch_name => "b" * 40) + stub_request(:get, %r{/compare/}).to_timeout + + get_management(repository) + assert_response :success + assert_includes response.body, '"candidateCheckFailed":true' + assert_not_includes response.body, '"branchName":"prototype/retry"' + + stub_comparison(repository, "a" * 40, "b" * 40, author_ids: [ 1001 ]) + get_management(repository) + assert_response :success + assert_includes response.body, '"candidateCheckFailed":false' + assert_includes response.body, '"branchName":"prototype/retry"' + ensure + Rails.cache = original_cache + end + test "retired histories leave lists but keep their exact review and history links" do payload = repository_payload(2001, "github/prototype") stub_github_sign_in(repositories: [ payload ], permissions: { 2001 => "admin" }) @@ -28,6 +206,11 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest assert_not_includes response.body, "Retained original" get repository_prototype_management_path(repository.github_id) assert_response :success + assert_includes response.body, "Replacement" + assert_not_includes response.body, "Retained original" + get_management(repository) + assert_response :success + assert_includes response.body, "Replacement" assert_not_includes response.body, "Retained original" get repository_prototype_path(repository.github_id, original, build: old_build.number) @@ -58,7 +241,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest assert_includes response.body, repository_prototype_management_path(repository.github_id) assert_includes response.body, '"prototypes":[]' - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success assert_includes response.body, path @@ -221,7 +404,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest assert_includes response.body, '"recentBranches":[]' assert_includes response.body, "Manage prototypes" - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success assert_not_includes response.body, "prototype/someone-else" assert_includes response.body, "prototype/existing" @@ -307,7 +490,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest headers: { "Content-Type" => "application/json" }) assert_includes repository.prototype_candidates.recently_active_by(other_user, permission: "write"), declined - get repository_prototype_management_path(repository.route_id) + get_management(repository) assert_response :success assert_includes response.body, '"branchName":"prototype/declined"' @@ -384,7 +567,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest get repository_path(repository.github_id) assert_not_includes response.body, "You pushed prototype/recent." - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success %w[boundary old someone-else recent].each { |name| assert_includes response.body, "prototype/#{name}" } end @@ -590,7 +773,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest stub_branches(repository, "main" => "a" * 40, "feature/navigation" => "b" * 40, "replica/install-workflow" => "c" * 40) - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success assert_includes response.body, "feature/navigation" assert_not_includes response.body, "prototype/deleted" @@ -621,7 +804,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest stub_comparison(repository, "a" * 40, "b" * 40, author_ids: [ 1001 ], page: 2, total: 101) - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success assert_includes response.body, "prototype/long-history" @@ -646,8 +829,9 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest end stub_branches(repository, heads) path = repository_prototype_management_path(repository.github_id) + WebMock.reset_executed_requests! - get path + get_management(repository) assert_response :success assert_equal 10, response.body.scan(/"branchName":"prototype\/branch-/).size @@ -658,8 +842,10 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest assert_includes response.body, '"candidatePage":1' assert_includes response.body, '"candidatePageCount":2' assert_includes response.body, %("candidatePagePath":"#{path}") + assert_requested :get, %r{/compare/}, times: 22 + assert_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens", times: 1 - get path, params: { page: 2 } + get_management(repository, page: 2) assert_response :success assert_equal 1, response.body.scan(/"branchName":"prototype\/branch-/).size @@ -667,7 +853,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest assert_includes response.body, '"candidatePage":2' assert_includes response.body, '"candidatePageCount":2' - get path, params: { page: 3 } + get_management(repository, page: 3) assert_response :not_found end @@ -681,12 +867,12 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest stub_comparison(repository, "a" * 40, "b" * 40, author_ids: [ 1002 ]) path = repository_prototype_management_path(repository.github_id) - get path + get_management(repository) assert_response :success assert_includes response.body, '"candidatePageCount":0' assert_includes response.body, '"unlisted":[]' - get path, params: { page: 2 } + get_management(repository, page: 2) assert_response :not_found get path, params: { page: "0" } @@ -706,7 +892,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest stub_comparison(repository, "a" * 40, "b" * 40, author_ids: [ 1001 ], total: 101) - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success assert_includes response.body, "could not check which branches" assert_not_includes response.body, '"branchName":"prototype/incomplete"' @@ -735,7 +921,7 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest assert_includes response.body, "could not check which branches" assert_includes response.body, '"recentBranches":[]' - get repository_prototype_management_path(repository.github_id) + get_management(repository) assert_response :success assert_includes response.body, '"name":"Listed"' assert_includes response.body, "could not check which branches" @@ -832,6 +1018,12 @@ class PrototypeRegistrationsTest < ActionDispatch::IntegrationTest private + def get_management(repository, page: 1) + get repository_prototype_management_path(repository.route_id), + params: { page:, props_at: "data.management" }, + headers: { "Accept" => "application/json" } + end + def push_activity(branch, timestamp: 1.hour.ago, actor_id: 1001, activity_type: "push") { activity_type:, timestamp: timestamp.iso8601, actor: { id: actor_id }, ref: "refs/heads/#{branch}" } end diff --git a/hub/test/integration/repositories_test.rb b/hub/test/integration/repositories_test.rb index 12bfdda..7b54403 100644 --- a/hub/test/integration/repositories_test.rb +++ b/hub/test/integration/repositories_test.rb @@ -107,16 +107,11 @@ class RepositoriesTest < ActionDispatch::IntegrationTest alpha = repositories.create(github_id: 2001, name_with_owner: "github/alpha") beta = repositories.create(github_id: 2002, name_with_owner: "github/beta") configuration = GithubAppConfiguration.current - configuration.tokens.define_singleton_method(:installation) { "installation-token" } - calls = 0 - github_repositories = Object.new - github_repositories.define_singleton_method(:permission) do |_repository, current_user, token:| - calls += 1 - { "permission" => "pull", "user" => { "id" => current_user.github_id } } - end - github_client = Object.new - github_client.define_singleton_method(:repositories) { github_repositories } - configuration.define_singleton_method(:github_client) { github_client } + stub_github_installation_token + stub_github_permissions( + [ repository_payload(2001, alpha.name_with_owner), repository_payload(2002, beta.name_with_owner) ], + { 2001 => "pull", 2002 => "pull" } + ) cache = ActiveSupport::Cache::MemoryStore.new controller = RepositoriesController.new @@ -128,7 +123,8 @@ class RepositoriesTest < ActionDispatch::IntegrationTest assert_equal first, second end - assert_equal 2, calls + assert_requested :get, %r{/collaborators/octocat/permission}, times: 2 + assert_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens", times: 1 end test "allows a writer to request current branch polling" do diff --git a/hub/test/models/github_client_batch_test.rb b/hub/test/models/github_client_batch_test.rb new file mode 100644 index 0000000..ada6118 --- /dev/null +++ b/hub/test/models/github_client_batch_test.rb @@ -0,0 +1,43 @@ +require "test_helper" +require "timeout" + +class GithubClientBatchTest < ActiveSupport::TestCase + test "bounds concurrent calls and preserves input order" do + started = Queue.new + release = Queue.new + count = GithubClient::Batch::MAX_CONCURRENCY + 2 + worker = Thread.new do + GithubClient::Batch.map((1..count).to_a) do |item| + started << item + release.pop + item * 2 + end + end + + Timeout.timeout(5) do + GithubClient::Batch::MAX_CONCURRENCY.times { started.pop } + assert started.empty? + GithubClient::Batch::MAX_CONCURRENCY.times { release << true } + 2.times { started.pop } + 2.times { release << true } + assert_equal (1..count).map { _1 * 2 }, worker.value + end + ensure + count.times { release << true } + worker&.join + end + + test "surfaces a GitHub failure and drains all worker calls" do + completed = Queue.new + assert_raises(GithubClient::Error) do + GithubClient::Batch.map([ 1, 2, 3 ]) do |item| + raise GithubClient::Error, "Unavailable" if item == 2 + + item + ensure + completed << item + end + end + assert_equal 3, completed.size + end +end diff --git a/hub/test/models/repository/authorization_test.rb b/hub/test/models/repository/authorization_test.rb index e93b376..d7fb4d5 100644 --- a/hub/test/models/repository/authorization_test.rb +++ b/hub/test/models/repository/authorization_test.rb @@ -97,6 +97,17 @@ class Repository::AuthorizationTest < ActiveSupport::TestCase assert_equal user.id, repository.authorization.share_link_creation_proof.user_id end + test "fresh permission checks ignore prefetched selector payloads" do + user = users.create + repository = repositories.create + configuration = installed_configuration + client = permission_client(user, "pull") + configuration.define_singleton_method(:github_client) { client } + payload = { "permission" => "admin", "user" => { "id" => user.github_id } } + + assert_equal "read", repository.authorization.permission_for(user, fresh: true, configuration:, payload:) + end + private def installed_configuration diff --git a/hub/test/models/repository/prototype_candidates_test.rb b/hub/test/models/repository/prototype_candidates_test.rb new file mode 100644 index 0000000..aed0e4a --- /dev/null +++ b/hub/test/models/repository/prototype_candidates_test.rb @@ -0,0 +1,245 @@ +require "test_helper" +require "timeout" + +class Repository::PrototypeCandidatesTest < ActiveSupport::TestCase + test "overlaps branch comparisons within four calls and deduplicates heads before caching on the request thread" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + github_app_configurations.create_installed + user = users.create + repository = repositories.create + heads = { "main" => "a" * 40 } + 6.times do |index| + name = "prototype/branch-#{index}" + heads[name] = format("%040x", index + 1) + prototypes.create(repository:, branch_name: name) + end + heads["prototype/shared-head"] = heads.fetch("prototype/branch-0") + prototypes.create(repository:, branch_name: "prototype/shared-head") + prototypes.create(repository:, branch_name: "prototype/deleted") + stub_github_installation_token + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { "per_page" => "100", "page" => "1" }) + .to_return \ + status: 200, + body: JSON.generate(heads.map { |name, sha| { name:, commit: { sha: } } }), + headers: { "Content-Type" => "application/json" } + started = Queue.new + release = Queue.new + mutex = Mutex.new + active = 0 + maximum = 0 + stub_request(:get, %r{/repos/#{repository.github_path}/compare/}) + .to_return do + mutex.synchronize do + active += 1 + maximum = [ maximum, active ].max + end + started << true + release.pop + { + status: 200, + body: JSON.generate(total_commits: 1, commits: [ { author: { id: user.github_id } } ]), + headers: { "Content-Type" => "application/json" } + } + ensure + mutex.synchronize { active -= 1 } + end + coordinator = Thread.new do + Timeout.timeout(5) do + 4.times { started.pop } + 4.times { release << true } + 2.times { started.pop } + 2.times { release << true } + end + ensure + 6.times { release << true } + end + request_thread = Thread.current + configuration_reads = [] + cache_writers = [] + sql = ->(*arguments) do + payload = arguments.last + configuration_reads << Thread.current if payload[:sql].include?('FROM "github_app_configurations"') + end + cache = ->(*) { cache_writers << Thread.current } + result = nil + + ActiveSupport::Notifications.subscribed(sql, "sql.active_record") do + ActiveSupport::Notifications.subscribed(cache, "cache_write.active_support") do + result = repository.prototype_candidates.available_page(user, permission: "write") + end + end + coordinator.value + + assert_equal heads.keys.grep(/^prototype/).sort, result.first.map(&:branch_name).sort + assert_equal 1, result.last + assert_equal 4, maximum + assert_equal [ request_thread ], configuration_reads + assert_equal [ request_thread ] * 6, cache_writers + assert_requested :get, %r{/compare/}, times: 6 + assert_requested :post, "#{GITHUB_API_URL}/app/installations/456/access_tokens", times: 1 + WebMock.reset_executed_requests! + + assert_equal result, repository.prototype_candidates.available_page(user, permission: "write") + assert_not_requested :get, %r{/compare/} + ensure + 6.times { release << true } if release + coordinator&.join + Rails.cache = original_cache + end + + test "retains a successful in-flight comparison after another head fails and retries incomplete history" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + github_app_configurations.create_installed + user = users.create + repository = repositories.create + base = "a" * 40 + failed_head = "b" * 40 + successful_head = "c" * 40 + heads = { "main" => base, "prototype/failed" => failed_head, "prototype/successful" => successful_head } + heads.keys.grep(/^prototype/).each { |name| prototypes.create(repository:, branch_name: name) } + stub_github_installation_token + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { "per_page" => "100", "page" => "1" }) + .to_return \ + status: 200, + body: JSON.generate(heads.map { |name, sha| { name:, commit: { sha: } } }), + headers: { "Content-Type" => "application/json" } + response = { + status: 200, + body: JSON.generate(total_commits: 1, commits: [ { author: { id: user.github_id } } ]), + headers: { "Content-Type" => "application/json" } + } + comparison_url = "#{GITHUB_API_URL}/repos/#{repository.github_path}/compare/#{base}..." + incomplete_request = stub_request(:get, "#{comparison_url}#{failed_head}") + .with(query: { "per_page" => "100", "page" => "1" }) + .to_return \ + status: 200, + body: JSON.generate(total_commits: 101, commits: Array.new(100) { { author: { id: user.github_id } } }), + headers: { "Content-Type" => "application/json" } + successful_started = Queue.new + failure_raised = Queue.new + failure = GithubClient::Error.new("Comparison temporarily unavailable") + stub_request(:get, "#{comparison_url}#{failed_head}") + .with(query: { "per_page" => "100", "page" => "2" }) + .to_return do + Timeout.timeout(5) { successful_started.pop } + raise failure + end + successful_request = stub_request(:get, "#{comparison_url}#{successful_head}") + .with(query: { "per_page" => "100", "page" => "1" }) + .to_return do + successful_started << true + Timeout.timeout(5) { failure_raised.pop } + response + end + # Observe the error itself so the successful HTTP response cannot finish before it is raised. + trace = TracePoint.new(:raise) do |event| + failure_raised << true if event.raised_exception.equal?(failure) + end + request_thread = Thread.current + cache_writers = [] + cache = ->(*) { cache_writers << Thread.current } + + trace.enable(target_thread: nil) do + ActiveSupport::Notifications.subscribed(cache, "cache_write.active_support") do + error = assert_raises(GithubClient::Error) do + repository.prototype_candidates.available_page(user, permission: "write") + end + assert_same failure, error + end + end + successful_request.to_return(response) + remaining_request = stub_request(:get, "#{comparison_url}#{failed_head}") + .with(query: { "per_page" => "100", "page" => "2" }) + .to_return(response.merge(body: JSON.generate(total_commits: 101, commits: [ { author: { id: user.github_id } } ]))) + WebMock.reset_executed_requests! + + branches, page_count = repository.prototype_candidates.available_page(user, permission: "write") + + assert_equal heads.keys.grep(/^prototype/).sort, branches.map(&:branch_name).sort + assert_equal 1, page_count + assert_requested incomplete_request, times: 1 + assert_requested remaining_request, times: 1 + assert_not_requested successful_request + assert_equal [ request_thread ], cache_writers + ensure + trace&.disable + successful_started << true if successful_started + failure_raised << true if failure_raised + Rails.cache = original_cache + end + + test "skips queued comparisons after running calls fail and retries every uncached head" do + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + github_app_configurations.create_installed + user = users.create + repository = repositories.create + base = "a" * 40 + heads = { "main" => base } + 6.times do |index| + name = "prototype/branch-#{index}" + heads[name] = format("%040x", index + 1) + prototypes.create(repository:, branch_name: name) + end + stub_github_installation_token + stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/branches") + .with(query: { "per_page" => "100", "page" => "1" }) + .to_return \ + status: 200, + body: JSON.generate(heads.map { |name, sha| { name:, commit: { sha: } } }), + headers: { "Content-Type" => "application/json" } + response = { + status: 200, + body: JSON.generate(total_commits: 1, commits: [ { author: { id: user.github_id } } ]), + headers: { "Content-Type" => "application/json" } + } + started = Queue.new + release = Queue.new + comparisons = heads.values.drop(1).map.with_index do |head, index| + request = stub_request(:get, "#{GITHUB_API_URL}/repos/#{repository.github_path}/compare/#{base}...#{head}") + .with(query: { "per_page" => "100", "page" => "1" }) + if index < 4 + request.to_return do + started << true + Timeout.timeout(5) { release.pop } + { status: 503, body: "{}" } + end + else + request.to_return(response) + end + request + end + coordinator = Thread.new do + Timeout.timeout(5) do + 4.times { started.pop } + 4.times { release << true } + end + ensure + 4.times { release << true } + end + + error = assert_raises(GithubClient::Error) do + repository.prototype_candidates.available_page(user, permission: "write") + end + coordinator.value + + assert_equal 503, error.status + comparisons.drop(4).each { |request| assert_not_requested request } + comparisons.each { |request| request.to_return(response) } + WebMock.reset_executed_requests! + + branches, page_count = repository.prototype_candidates.available_page(user, permission: "write") + + assert_equal heads.keys.grep(/^prototype/).sort, branches.map(&:branch_name).sort + assert_equal 1, page_count + comparisons.each { |request| assert_requested request, times: 1 } + ensure + 4.times { release << true } if release + coordinator&.join + Rails.cache = original_cache + end +end