Skip to content

Revert "fix(dashboard): revalidate GitHub caches when installation access changes"#145

Closed
stylessh wants to merge 2 commits intomainfrom
claude/rollback-github-app-changes-uBNpH
Closed

Revert "fix(dashboard): revalidate GitHub caches when installation access changes"#145
stylessh wants to merge 2 commits intomainfrom
claude/rollback-github-app-changes-uBNpH

Conversation

@stylessh
Copy link
Copy Markdown
Owner

@stylessh stylessh commented Apr 17, 2026

This reverts commit 4f54751.

Summary by CodeRabbit

Release Notes

  • Removed Features

    • Removed automatic data refresh when returning to the application
    • Removed repository access visibility filtering logic
  • Improvements

    • Simplified GitHub installation data handling and caching strategies
  • Tests

    • Removed test coverage for deleted functionality

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 17, 2026

📝 Walkthrough

Walkthrough

This PR removes the GitHub App installation access indexing feature, including cache policies, revalidation signals, repository visibility filtering, type definitions, the useRefreshOnReturn hook, and related server functions across the dashboard application.

Changes

Cohort / File(s) Summary
Installation Access API & Types
apps/dashboard/src/lib/github-access.ts, apps/dashboard/src/lib/github-access.test.ts, apps/dashboard/src/lib/github.types.ts
Removed GitHubInstallationAccessIndex type, emptyInstallationAccessIndex constructor, and isRepoVisibleWithInstallationAccess helper. Deleted corresponding test coverage. Removed isPrivate field from RepositoryRef type.
Cache & Revalidation Configuration
apps/dashboard/src/lib/github-cache-policy.ts, apps/dashboard/src/lib/github-revalidation.ts
Removed installationAccess cache policy object and revalidation signal key. Removed special-case webhook event handling for installation, installation_repositories, and github_app_authorization events.
Server Functions & Data Fetching
apps/dashboard/src/lib/github.functions.ts
Removed getInstallationAccess and refreshInstallationAccess server functions. Simplified getGitHubAppUserInstallations to single-page request with error handling. Removed repository visibility/access filtering from getUserRepos, command palette search, getMyPulls, getMyIssues, getUserPinnedRepos, and getNotifications. Removed isPrivate from GraphQL queries and repository ref construction. Updated cache invalidation signals for pull/issue lists.
Refresh Hook Removal
apps/dashboard/src/lib/use-refresh-on-return.ts, apps/dashboard/src/components/layouts/github-access-dialog.tsx, apps/dashboard/src/routes/setup.tsx
Deleted useRefreshOnReturn hook entirely. Removed hook invocations from GitHubAccessDialog and SetupPage.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete and lacks required template sections (Summary, Changes, Test Plan), providing only a single revert statement without context or explanation. Add a proper summary explaining why the revert is necessary, detail the changes being reverted, and include a test plan with verification steps.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: reverting a specific commit that adds cache revalidation for GitHub installation access changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/rollback-github-app-changes-uBNpH

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
diffkit 31bf18c Apr 17 2026, 05:28 AM

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/dashboard/src/lib/github.functions.ts`:
- Around line 4043-4044: The caches using signalKeys/namespaceKeys of
githubRevalidationSignalKeys.pullsMine (and the similar one for "My Issues")
only key by { username } but also depend on the current installation set from
getMySearchSources(); update the cache key and signals so installation changes
invalidate them: include a stable installation identifier (e.g., a sorted list
or fingerprint of getMySearchSources() results or an installation IDs array) in
the cache key for the functions that build "My Pulls"/"My Issues", and add the
corresponding installation-change revalidation key (e.g.,
githubRevalidationSignalKeys.installations or an installations-specific signal)
to both signalKeys and namespaceKeys alongside
githubRevalidationSignalKeys.pullsMine so granting/revoking installs triggers
invalidation (also apply the same change to the other affected cache referenced
by lines ~4230-4231).
- Around line 1516-1524: The current code only fetches the first page of GET
/user/installations (per_page: 100), causing missing installations when there
are >100; update the fetch to paginate and collect all pages before calling
mapGitHubAppInstallations. Replace the single request using
appUserOctokit.request in the block that creates installationsResponse with a
paginated fetch (e.g., appUserOctokit.paginate or looping with page param) to
accumulate all installation items into a single array, then pass that full array
into mapGitHubAppInstallations so callers receive the complete installations
list.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8199a6a8-ef22-4941-82dd-7a66d4e9f6e9

📥 Commits

Reviewing files that changed from the base of the PR and between 4a0c0e2 and 31bf18c.

📒 Files selected for processing (9)
  • apps/dashboard/src/components/layouts/github-access-dialog.tsx
  • apps/dashboard/src/lib/github-access.test.ts
  • apps/dashboard/src/lib/github-access.ts
  • apps/dashboard/src/lib/github-cache-policy.ts
  • apps/dashboard/src/lib/github-revalidation.ts
  • apps/dashboard/src/lib/github.functions.ts
  • apps/dashboard/src/lib/github.types.ts
  • apps/dashboard/src/lib/use-refresh-on-return.ts
  • apps/dashboard/src/routes/setup.tsx
💤 Files with no reviewable changes (8)
  • apps/dashboard/src/routes/setup.tsx
  • apps/dashboard/src/components/layouts/github-access-dialog.tsx
  • apps/dashboard/src/lib/github.types.ts
  • apps/dashboard/src/lib/github-cache-policy.ts
  • apps/dashboard/src/lib/github-access.test.ts
  • apps/dashboard/src/lib/github-revalidation.ts
  • apps/dashboard/src/lib/use-refresh-on-return.ts
  • apps/dashboard/src/lib/github-access.ts

Comment on lines +1516 to +1524
const installationsResponse = await appUserOctokit.request(
"GET /user/installations",
{
per_page: 100,
},
);
const installations = mapGitHubAppInstallations(
installationsResponse.data as GitHubUserInstallationsPayload,
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Restore pagination when loading app installations.

Line 1516 now reads only the first page of GET /user/installations. Every caller here treats this list as complete, so accounts with more than 100 installations will silently miss owners, search sources, and access-state entries.

🐛 Proposed fix
-		const installationsResponse = await appUserOctokit.request(
-			"GET /user/installations",
-			{
-				per_page: 100,
-			},
-		);
-		const installations = mapGitHubAppInstallations(
-			installationsResponse.data as GitHubUserInstallationsPayload,
-		);
+		const installationItems =
+			await listPaginatedGitHubItems<GitHubUserInstallationPayload>({
+				label: "github app installations",
+				request: (page, signal) =>
+					appUserOctokit.request("GET /user/installations", {
+						page,
+						per_page: 100,
+						request: { signal },
+					}),
+				getItems: (payload) =>
+					(payload as GitHubUserInstallationsPayload).installations ?? [],
+			});
+		const installations = mapGitHubAppInstallations({
+			installations: installationItems,
+		});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/dashboard/src/lib/github.functions.ts` around lines 1516 - 1524, The
current code only fetches the first page of GET /user/installations (per_page:
100), causing missing installations when there are >100; update the fetch to
paginate and collect all pages before calling mapGitHubAppInstallations. Replace
the single request using appUserOctokit.request in the block that creates
installationsResponse with a paginated fetch (e.g., appUserOctokit.paginate or
looping with page param) to accumulate all installation items into a single
array, then pass that full array into mapGitHubAppInstallations so callers
receive the complete installations list.

Comment on lines +4043 to 4044
signalKeys: [githubRevalidationSignalKeys.pullsMine],
namespaceKeys: [githubRevalidationSignalKeys.pullsMine],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep an installation-change invalidation path for these caches.

These two caches still depend on the current installation set via getMySearchSources(), but the cache key here is only { username }. After a user grants or revokes installation access, “My Pulls” and “My Issues” can stay stale until the list stale time expires.

Also applies to: 4230-4231

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/dashboard/src/lib/github.functions.ts` around lines 4043 - 4044, The
caches using signalKeys/namespaceKeys of githubRevalidationSignalKeys.pullsMine
(and the similar one for "My Issues") only key by { username } but also depend
on the current installation set from getMySearchSources(); update the cache key
and signals so installation changes invalidate them: include a stable
installation identifier (e.g., a sorted list or fingerprint of
getMySearchSources() results or an installation IDs array) in the cache key for
the functions that build "My Pulls"/"My Issues", and add the corresponding
installation-change revalidation key (e.g.,
githubRevalidationSignalKeys.installations or an installations-specific signal)
to both signalKeys and namespaceKeys alongside
githubRevalidationSignalKeys.pullsMine so granting/revoking installs triggers
invalidation (also apply the same change to the other affected cache referenced
by lines ~4230-4231).

@stylessh stylessh closed this Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants