Show install prompt when GitHub App is missing from repo pages #1308
Conversation
… pages\n\n- Add repository-level layout to check if the Issue to PR GitHub App is installed\n- If not installed, render a clear message with a CTA to install the app\n- Reuse NEXT_PUBLIC_GITHUB_APP_SLUG to build installation URL\n- Add helper isAppInstalledForRepo using GET /repos/{owner}/{repo}/installation
youngchingjui
added
the
AI generated
WalkthroughAdds a repo-level Next.js layout that checks if the GitHub App is installed for the visited repo and conditionally renders either children or an install prompt. Introduces a utility to determine installation status by querying repo installation and handling 404 as not installed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Next.js Layout as RepoLayout
participant InstallCheck as isAppInstalledForRepo
participant ReposAPI as getInstallationFromRepo
participant GitHub
User->>RepoLayout: Request /:username/:repo/...
RepoLayout->>InstallCheck: isAppInstalledForRepo({ owner, repo })
InstallCheck->>ReposAPI: getInstallationFromRepo(owner, repo)
ReposAPI->>GitHub: GET /repos/{owner}/{repo}/installation
alt Installation exists (200)
GitHub-->>ReposAPI: Installation data
ReposAPI-->>InstallCheck: Resolved installation
InstallCheck-->>RepoLayout: true
RepoLayout-->>User: Render children
else Not installed (404)
GitHub-->>ReposAPI: 404
ReposAPI-->>InstallCheck: Throw 404
InstallCheck-->>RepoLayout: false
RepoLayout-->>User: Render install prompt with CTA URL
else Other error
GitHub-->>ReposAPI: Error
ReposAPI-->>InstallCheck: Error
InstallCheck-->>RepoLayout: false (after logging)
RepoLayout-->>User: Render install prompt (slug-dependent CTA)
end
note over RepoLayout: install URL uses NEXT_PUBLIC_GITHUB_APP_SLUG if set
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/github/install-check.ts (1)
16-31: Optional: cache checks briefly to reduce API load/rate-limit riskIf this runs per-request, consider a short TTL cache keyed by
${owner}/${repo}(e.g., 30–60s). In App Router, you could wrap the call with next/cache’s unstable_cache; otherwise a tiny in-memory LRU works in Node runtime.app/[username]/[repo]/layout.tsx (3)
26-49: Improve accessibility semantics for the blocking noticeAnnounce the blocking state to assistive tech.
Apply this diff:
- <div className="mb-6 rounded-md border border-yellow-300 bg-yellow-50 p-4 text-yellow-900"> + <div + role="alert" + aria-live="assertive" + className="mb-6 rounded-md border border-yellow-300 bg-yellow-50 p-4 text-yellow-900" + >
35-42: Use a plain anchor for external navigation (minor clarity)For external links, an is sufficient and avoids any Link-specific behaviors.
Apply this diff:
- <Link - href={installUrl} - target="_blank" - rel="noopener noreferrer" - className="inline-flex items-center rounded-md bg-stone-900 px-4 py-2 text-white hover:bg-stone-800" - > - Install Issue to PR on GitHub - </Link> + <a + href={installUrl} + target="_blank" + rel="noopener noreferrer" + className="inline-flex items-center rounded-md bg-stone-900 px-4 py-2 text-white hover:bg-stone-800" + > + Install Issue to PR on GitHub + </a>
22-23: Heads-up: 401/403 will look the same as “not installed”If credentials/rate limits fail, users still see the install prompt. Consider surfacing a distinct error in the future (e.g., “temporarily unavailable”) so we don’t mislead users when the app actually is installed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/[username]/[repo]/layout.tsx(1 hunks)lib/github/install-check.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
app/[username]/[repo]/layout.tsxlib/github/install-check.ts
🧬 Code graph analysis (2)
app/[username]/[repo]/layout.tsx (1)
lib/github/install-check.ts (1)
isAppInstalledForRepo(9-31)
lib/github/install-check.ts (1)
lib/github/repos.ts (1)
getInstallationFromRepo(107-125)
🔇 Additional comments (2)
lib/github/install-check.ts (1)
16-24: Reasonable 404→false mapping; simple, predictable boolean APITreating 404 as “not installed” and otherwise falling back to false is acceptable for now and aligns with the stated UX goals.
app/[username]/[repo]/layout.tsx (1)
1-3: Path alias and App Router verified – tsconfig.base.json defines the"@/*"alias, package.json specifies Next.js ^14.2.18 (which supports the App Router), and there is nopages/directory present, soapp/[username]/[repo]/layout.tsxwill load correctly.
| export default async function RepoLayout({ children, params }: LayoutProps) { | ||
| const { username, repo } = params | ||
|
|
||
| // Build GitHub App installation URL if available | ||
| const appSlug = process.env.NEXT_PUBLIC_GITHUB_APP_SLUG | ||
| const installUrl = appSlug | ||
| ? `https://github.com/apps/${appSlug}/installations/new` | ||
| : undefined | ||
|
|
||
| const installed = await isAppInstalledForRepo({ owner: username, repo }) | ||
|
|
There was a problem hiding this comment.
Force Node runtime and dynamic rendering for Octokit call
Without fetch-based signals, Next may attempt static optimization or edge runtime, breaking Octokit or caching the install state. Explicitly pin Node and disable caching.
Apply this diff near the top-level (after imports is fine):
+export const runtime = "nodejs"
+export const dynamic = "force-dynamic"
+export const revalidate = 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default async function RepoLayout({ children, params }: LayoutProps) { | |
| const { username, repo } = params | |
| // Build GitHub App installation URL if available | |
| const appSlug = process.env.NEXT_PUBLIC_GITHUB_APP_SLUG | |
| const installUrl = appSlug | |
| ? `https://github.com/apps/${appSlug}/installations/new` | |
| : undefined | |
| const installed = await isAppInstalledForRepo({ owner: username, repo }) | |
| export const runtime = "nodejs" | |
| export const dynamic = "force-dynamic" | |
| export const revalidate = 0 | |
| export default async function RepoLayout({ children, params }: LayoutProps) { | |
| const { username, repo } = params | |
| // Build GitHub App installation URL if available | |
| const appSlug = process.env.NEXT_PUBLIC_GITHUB_APP_SLUG | |
| const installUrl = appSlug | |
| ? `https://github.com/apps/${appSlug}/installations/new` | |
| : undefined | |
| const installed = await isAppInstalledForRepo({ owner: username, repo }) |
🤖 Prompt for AI Agents
In app/[username]/[repo]/layout.tsx around lines 13 to 23, Next may statically
optimize or choose an edge runtime which breaks the Octokit/install check; add
top-level exports immediately after the imports to force Node runtime and
disable caching: export runtime = 'nodejs', export dynamic = 'force-dynamic',
and export fetchCache = 'no-store' so the layout runs on Node, always renders
dynamically, and avoids cached fetches for the isAppInstalledForRepo call.
| console.error( | ||
| `[github/install-check] Failed to determine installation for ${owner}/${repo}:`, | ||
| error | ||
| ) | ||
| return false |
There was a problem hiding this comment.
Sanitize error logging to avoid leaking sensitive details
Octokit errors can include request metadata. Log only safe fields (status, name, message) instead of the whole error object.
Apply this diff:
- console.error(
- `[github/install-check] Failed to determine installation for ${owner}/${repo}:`,
- error
- )
+ const err = error as any
+ const message = err?.message ?? "unknown"
+ const name = err?.name ?? "Error"
+ console.error(
+ `[github/install-check] install check failed for ${owner}/${repo}: status=${status} name=${name} message=${message}`
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/github/install-check.ts around lines 25 to 29, the current console.error
prints the entire Octokit error object which may include sensitive request
metadata; modify the error logging to extract and log only safe fields (status,
name, message) from the error (use optional chaining/defaults) and include a
concise contextual message like "[github/install-check] Failed to determine
installation for owner/repo:" followed by a JSON or stringified object
containing only { status, name, message } to avoid leaking sensitive details.
Summary
Details
Why a layout?
Notes
How to test
Env
Ensure NEXT_PUBLIC_GITHUB_APP_SLUG is set to the correct GitHub App slug (e.g., issue-to-pr-dev or issue-to-pr).
Closes #1259
Summary by CodeRabbit