feat(ui): multi-installation aware settings UI (#283 PR 4/7)#291
Merged
Conversation
This was referenced Apr 7, 2026
Owner
Author
3 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughGitHub Appの単一リンクモデルを複数インストール対応へ移行。インストール単位のメタデータ取得・表示ラベル生成・設定URL構築・個別切断フロー・インストール別リポジトリ取得／追加／検証を導入した。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "UI\n(Repo Add / Integration Settings)"
participant Route as "Route Action\n(/settings)"
participant Service as "getActiveInstallationOptions\n/ Integration Service"
participant GitHub as "GitHub API / Octokit"
participant DB as "DB / assertInstallationBelongsToOrg"
participant RepoMut as "addRepository\n& membership upsert"
User->>UI: リポジトリ追加/切断操作 (owner,name,installationId?)
UI->>Route: POST intent (installationId optional / intent)
Route->>Service: getActiveInstallationOptions(orgId)
Service-->>Route: ActiveInstallationOption[]
alt intent requires installation validation
Route->>DB: assertInstallationBelongsToOrg(orgId, installationId)
DB-->>Route: 所有/状態確認
end
alt app-installation flow
Route->>GitHub: resolveOctokitForInstallation(installationId)
else token flow
Route->>GitHub: createOctokit(token)
end
GitHub-->>Route: API results
Route->>RepoMut: addRepository(orgId, {owner,name,githubInstallationId})
RepoMut->>DB: upsert repositories (persist githubInstallationId)
alt githubInstallationId != null
RepoMut->>GitHub: upsertRepositoryMembership(...)
GitHub-->>RepoMut: success / error
end
RepoMut-->>Route: AddRepositoryResult
Route-->>UI: 成功/エラーレスポンス
UI-->>User: 結果表示
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coji
force-pushed
the
feat/issue-283-ui
branch
2 times, most recently
from
af71c5b to
be0b946
Compare
2 tasks
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.ts (1)
15-24: 型定義の配置順序を整理することを推奨します。
TaggedInstallationRepo型がextractOwners関数（15行目）で使用された後に定義されています（21-24行目）。TypeScript ではホイスティングにより動作しますが、可読性のため型定義を使用箇所より前に移動することをお勧めします。♻️ 提案する修正
type InstallationRepo = Awaited<ReturnType<typeof fetchAllInstallationRepos>>[0] +export type TaggedInstallationRepo = { + installationId: number + repo: InstallationRepo +} + /** Extract unique owners from pre-fetched installation repos. */ export function extractOwners(tagged: TaggedInstallationRepo[]): string[] { return [...new Set(tagged.map((t) => t.repo.owner.login))].sort((a, b) => a.localeCompare(b, undefined, { sensitivity: 'base' }), ) } - -export type TaggedInstallationRepo = { - installationId: number - repo: InstallationRepo -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/+functions/get-installation-repos.ts around lines 15 - 24, extractOwners が TaggedInstallationRepo 型を参照する前に型定義が後置されているので、可読性向上のため TaggedInstallationRepo の定義を extractOwners より上に移動してください。具体的にはファイル内で TaggedInstallationRepo 型宣言を extractOwners 関数の前に置き、型名（TaggedInstallationRepo）と関数名（extractOwners）をそのまま使って参照が先に来るように並べ替えてください。app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsx (1)
153-170:copyFetcherのuseEffect依存配列について。リポジトリのルールでは
fetcherオブジェクト自体を依存配列に含めることを避けるべきとされていますが、ここではcopyFetcher.data、copyFetcher.reset、copyFetcher.stateという個別のプロパティを使用しています。Biome がこれをフラグしない限り問題ありませんが、copyFetcher.resetが不安定な参照である可能性があることに注意してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.tsx around lines 153 - 170, The useEffect watching copyFetcher properties may be relying on an unstable reference (copyFetcher.reset); to fix, either depend on the whole fetcher object instead of individual props (replace the dependency array with [copyFetcher]) or stabilize the reset callback (create a stable wrapper via useCallback that calls copyFetcher.reset and use that in the deps). Locate the useEffect that references copyFetcher.data, copyFetcher.state, and copyFetcher.reset and apply one of these two fixes so the effect won't retrigger due to an unstable reset reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/`$orgSlug/settings/integration/index.tsx:
- Around line 70-82: Move the org membership/auth check to run before parsing
form data: call requireOrgMember (or requireOrgAdmin as appropriate) at the
start of the exported action function using the same context/org info, and only
after that call await request.formData() and parseWithZod(formData, { schema:
integrationActionSchema }); ensure you keep the existing use of
context.get(orgContext) or use the returned org from requireOrgMember and
preserve the same error handling/intent response flow if authentication fails.
---
Nitpick comments:
In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.tsx:
- Around line 153-170: The useEffect watching copyFetcher properties may be
relying on an unstable reference (copyFetcher.reset); to fix, either depend on
the whole fetcher object instead of individual props (replace the dependency
array with [copyFetcher]) or stabilize the reset callback (create a stable
wrapper via useCallback that calls copyFetcher.reset and use that in the deps).
Locate the useEffect that references copyFetcher.data, copyFetcher.state, and
copyFetcher.reset and apply one of these two fixes so the effect won't retrigger
due to an unstable reset reference.
In
`@app/routes/`$orgSlug/settings/repositories.add/+functions/get-installation-repos.ts:
- Around line 15-24: extractOwners が TaggedInstallationRepo
型を参照する前に型定義が後置されているので、可読性向上のため TaggedInstallationRepo の定義を extractOwners
より上に移動してください。具体的にはファイル内で TaggedInstallationRepo 型宣言を extractOwners
関数の前に置き、型名（TaggedInstallationRepo）と関数名（extractOwners）をそのまま使って参照が先に来るように並べ替えてください。
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8155a088-29a6-4de5-accc-73a0d974fda6
📒 Files selected for processing (13)
app/libs/github-account.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.tsapp/routes/$orgSlug/settings/integration/index.tsxapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.add/index.tsxapp/services/github-integration-queries.server.tsdocs/rdd/issue-283-work-plan.md
2 tasks
coji
force-pushed
the
feat/issue-283-webhook-membership
branch
from
6768429 to
1c62eb5
Compare
3 tasks
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coji
force-pushed
the
feat/issue-283-webhook-membership
branch
from
1c62eb5 to
e40a7c8
Compare
coji
force-pushed
the
feat/issue-283-ui
branch
2 times, most recently
from
ad26a55 to
9072111
Compare
coji
force-pushed
the
feat/issue-283-webhook-membership
branch
from
e40a7c8 to
763a5c9
Compare
coji
force-pushed
the
feat/issue-283-ui
branch
5 times, most recently
from
5d28a9b to
047cd20
Compare
coji
changed the base branch from
feat/issue-283-webhook-membership
to
graphite-base/291
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsx (1)
153-170:⚠️ Potential issue | 🟡 Minor
copyFetcher.dataとcopyFetcher.resetを依存配列から除外してください。この repo では
useFetcher()の戻り値は render ごとに新しい参照として扱う前提です。現在のコードは[copyFetcher.state, copyFetcher.data, copyFetcher.reset]ですが、copyFetcher.dataとcopyFetcher.resetは毎 render で新しい参照になるため、effect が毎 render 再実行されます。安全な選択肢は 2 つです：
- 依存配列を
[copyFetcher.state]のみにする（state は primitive で安定している）- 依存配列を
[]にして、biome-ignore lint/correctness/useExhaustiveDependenciesで意図を明示する🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.tsx around lines 153 - 170, The effect currently depends on unstable object refs (copyFetcher.data and copyFetcher.reset) causing it to run every render; change the dependency array to use only the stable primitive copyFetcher.state (i.e. useEffect(..., [copyFetcher.state])) or, if you intend a one-time effect, use an empty array with a biome-ignore comment; keep the effect body as-is (matching INTENTS.copyInstallUrl, calling navigator.clipboard.writeText and toast.success) and still call copyFetcher.reset inside the effect body but do not include copyFetcher.reset in the dependency list.
🧹 Nitpick comments (1)
app/routes/$orgSlug/settings/repositories.add/index.tsx (1)
282-287: エラーメッセージ抽出はgetErrorMessage()に揃えてください。ここだけ手書きの
instanceof Error分岐になっていて、route 側のエラーハンドリング規約から外れています。getErrorMessage(e)に寄せると、非Errorthrow も一貫して扱えます。♻️ 修正案
import { requireOrgOwner } from '~/app/libs/auth.server' +import { getErrorMessage } from '~/app/libs/error-message' import { captureExceptionToSentry } from '~/app/libs/sentry-node.server'- message: e instanceof Error ? e.message : 'Invalid installation id', + message: getErrorMessage(e),As per coding guidelines, "Use
getErrorMessage()fromapp/libs/error-message.tsto extract error messages, never useString(e)which returns[object Error]for Error instances."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx around lines 282 - 287, The catch block currently extracts the error message with an instanceof Error check and a hardcoded fallback; replace that manual extraction by calling getErrorMessage(e) and passing its result into dataWithError so non-Error throws are handled consistently; update the catch in the function that calls dataWithError (the same block where you return dataWithError({}, { message: ... })) to use getErrorMessage and ensure getErrorMessage is imported from app/libs/error-message.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx:
- Around line 192-198: The current validation throws "invalid owner" when the
query param owner is not found in the rebuilt owners list (computed from
apiOwners and registeredOwners), which causes the whole page to error when some
installations failed; change the check in the block that computes owners (the
variables apiOwners, registeredOwners and owner) to only enforce the strict
owners.includes(owner) throw when there are no failed installations (i.e., when
failedInstallationIds is empty/undefined), otherwise skip or log a warning and
allow the request to proceed so partial failures don't crash the page.
- Around line 277-281: The current check only validates the installation belongs
to the org (assertInstallationBelongsToOrg) but does not verify that the
submitted owner/name is actually accessible via that installation; add a
server-side reconciliation step that, given submission.value.installationId and
submission.value.owner/name, uses the installation-scoped GitHub API token (or
an equivalent helper, e.g., verifyInstallationCanAccessRepo or
fetchRepoWithInstallationToken) to fetch the repository or confirm it appears in
the installation's repo list and throw/return a validation error if not; apply
this extra verification at both places where assertInstallationBelongsToOrg is
called (the try block around submission handling and the similar block
referenced at the later lines 293-296).
---
Outside diff comments:
In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.tsx:
- Around line 153-170: The effect currently depends on unstable object refs
(copyFetcher.data and copyFetcher.reset) causing it to run every render; change
the dependency array to use only the stable primitive copyFetcher.state (i.e.
useEffect(..., [copyFetcher.state])) or, if you intend a one-time effect, use an
empty array with a biome-ignore comment; keep the effect body as-is (matching
INTENTS.copyInstallUrl, calling navigator.clipboard.writeText and toast.success)
and still call copyFetcher.reset inside the effect body but do not include
copyFetcher.reset in the dependency list.
---
Nitpick comments:
In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx:
- Around line 282-287: The catch block currently extracts the error message with
an instanceof Error check and a hardcoded fallback; replace that manual
extraction by calling getErrorMessage(e) and passing its result into
dataWithError so non-Error throws are handled consistently; update the catch in
the function that calls dataWithError (the same block where you return
dataWithError({}, { message: ... })) to use getErrorMessage and ensure
getErrorMessage is imported from app/libs/error-message.ts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89c4ea51-2d4d-4687-86c4-88997f5b9019
📒 Files selected for processing (13)
app/libs/github-account.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.tsapp/routes/$orgSlug/settings/integration/index.tsxapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.add/index.tsxapp/services/github-integration-queries.server.tsdocs/rdd/issue-283-work-plan.md
✅ Files skipped from review due to trivial changes (2)
- app/libs/github-account.ts
- app/services/github-integration-queries.server.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- app/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsx
- app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts
- app/routes/$orgSlug/settings/_index/+schema.ts
- app/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts
- docs/rdd/issue-283-work-plan.md
- app/routes/$orgSlug/settings/integration/index.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/routes/$orgSlug/settings/repositories.add/index.tsx (2)
270-297:⚠️ Potential issue | 🟠 Major
installationIdだけでなくowner/nameとの対応も追加前に再検証してください。ここで保証しているのは「その installation がこの org のもの」という点だけです。hidden input を改ざんされると、別 installation の
owner/nameを組み合わせたままgithubInstallationIdを保存でき、membershipUpsertFailedは insert 後の検知にしかなりません。addRepository()の前に、その installation から当該 repo が実際に見えているかをサーバー側で再照合した方が安全です。💡 最小修正例
if (integration.method === 'github_app') { if (submission.value.installationId === undefined) { return dataWithError( {}, { message: 'Installation id is required for GitHub App mode' }, ) } try { await assertInstallationBelongsToOrg( organization.id, submission.value.installationId, ) + const octokit = resolveOctokitForInstallation( + submission.value.installationId, + ) + const visibleRepos = await getOrgCachedData( + organization.id, + `app-installation-all-repos:${submission.value.installationId}`, + () => fetchAllInstallationRepos(octokit), + 300000, + ) + const canAccessRepo = visibleRepos.some( + (repo) => + repo.owner.login === submission.value.owner && + repo.name === submission.value.name, + ) + if (!canAccessRepo) { + return dataWithError( + {}, + { message: 'Selected installation cannot access this repository' }, + ) + } } catch (e) { return dataWithError( {}, { message: getErrorMessage(e),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx around lines 270 - 297, Ensure the repo (owner/name) is actually visible to the GitHub App installation before calling addRepository: after setting installationId (and after assertInstallationBelongsToOrg) call a verification that uses the installation's API (e.g., implement/ call verifyInstallationCanAccessRepo(installationId, submission.value.owner, submission.value.name) or fetch the repo via the installation's GitHub client) and return dataWithError if the installation cannot see the repo; only then proceed to call addRepository(organization.id, {...}) to prevent tampered hidden inputs from pairing a repo with a wrong installation.
193-199:⚠️ Potential issue | 🟠 Major部分失敗時に
invalid ownerで loader を落とさないでください。
ownersは成功した installation だけから再構築されるので、失敗した installation にしか存在しない?owner=が付いているとここでページ全体が落ちます。Promise.allSettled()で劣化運転する意図と逆なので、少なくともfailedInstallationIds.length > 0の間はこの検証を緩めた方が安全です。💡 最小修正例
- if (owner && !owners.includes(owner)) { + if (owner && !owners.includes(owner) && failedInstallationIds.length === 0) { throw new Error('invalid owner') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx around lines 193 - 199, The current strict check that throws new Error('invalid owner') when owner is not in the computed owners array (built from apiOwners and registeredOwners) should be relaxed when some installations failed; change the validation around the owner variable to consult failedInstallationIds (or the settled results) and only throw when there are no failed installations (failedInstallationIds.length === 0); when failures exist, avoid throwing—either log a warning and ignore the owner parameter or allow it through for degraded operation so the loader does not crash. Ensure you update the code that references apiOwners, owners, registeredOwners and owner to use this conditional logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx:
- Around line 270-297: Ensure the repo (owner/name) is actually visible to the
GitHub App installation before calling addRepository: after setting
installationId (and after assertInstallationBelongsToOrg) call a verification
that uses the installation's API (e.g., implement/ call
verifyInstallationCanAccessRepo(installationId, submission.value.owner,
submission.value.name) or fetch the repo via the installation's GitHub client)
and return dataWithError if the installation cannot see the repo; only then
proceed to call addRepository(organization.id, {...}) to prevent tampered hidden
inputs from pairing a repo with a wrong installation.
- Around line 193-199: The current strict check that throws new Error('invalid
owner') when owner is not in the computed owners array (built from apiOwners and
registeredOwners) should be relaxed when some installations failed; change the
validation around the owner variable to consult failedInstallationIds (or the
settled results) and only throw when there are no failed installations
(failedInstallationIds.length === 0); when failures exist, avoid throwing—either
log a warning and ignore the owner parameter or allow it through for degraded
operation so the loader does not crash. Ensure you update the code that
references apiOwners, owners, registeredOwners and owner to use this conditional
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd99d600-b282-428c-8bc2-f8f895ee47f9
📒 Files selected for processing (4)
app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/integration/index.tsxapp/routes/$orgSlug/settings/repositories.add/index.tsxapp/services/github-integration-queries.server.ts
✅ Files skipped from review due to trivial changes (2)
- app/routes/$orgSlug/settings/integration/index.tsx
- app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsx
Owner
Author
|
@coderabbitai 前回の outside-diff nitpick ( packages/react-router/lib/dom/lib.tsx の let reset = React.useCallback(..., [resetFetcher, fetcherKey])
let submit = React.useCallback(..., [fetcherKey, submitImpl])
let load = React.useCallback(..., [fetcherKey, routeId, routerFetch])
let fetcherWithComponents = React.useMemo(
() => ({ Form: FetcherForm, submit, load, reset, ...fetcher, data }),
[FetcherForm, submit, load, reset, fetcher, data],
)
return fetcherWithComponentsのように |
|
Tip For best results, initiate chat on the files or code changes.
React Router v7 の 将来のレビューのため、正しい情報を記録します。また、既存のラーニング（ID: ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
There was a problem hiding this comment.
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 `@app/routes/`$orgSlug/settings/repositories.add/index.tsx:
- Around line 58-62: The AddRepoSchema currently allows empty owner/name because
it uses z.string(); update the schema so the owner and name fields use
z.string().trim().min(1) to reject empty or whitespace-only strings (keep
installationId as z.coerce.number().int().positive().optional()); modify the
AddRepoSchema declaration to apply trim() and min(1) to both owner and name so
server-side validation prevents empty values from reaching addRepository().
- Around line 299-318: The visible-repos reconciliation can throw (e.g.,
fetchAllInstallationRepos failing) and currently bubbles to an ErrorBoundary;
wrap the call to getOrgCachedData/resolveOctokitForInstallation in a try/catch
and, on any failure, return dataWithError(...) instead of letting the exception
propagate. Specifically, catch errors around resolveOctokitForInstallation and
the await getOrgCachedData(...) call (which invokes fetchAllInstallationRepos),
and if an error occurs return a payload using dataWithError({}, { message:
'Selected installation cannot access this repository' } or a clearer message
including the caught error) so the action degrades gracefully before checking
canAccessRepo.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d2aba85-fd37-4025-96c2-2864293a8ce9
📒 Files selected for processing (1)
app/routes/$orgSlug/settings/repositories.add/index.tsx
integration page (settings/integration): - loader returns githubAppLinks[] array with installation status - action refactored to discriminated union + parseWithZod + match (CLAUDE.md 規約) - new INTENTS: disconnectGithubAppLink, confirmDisconnectGithubAppLink - per-installation disconnect with assertInstallationBelongsToOrg validation GitHub App section UI: - InstallationCard renders one card per active installation - per-installation fetcher + ConfirmDialog (preserves installationId) - "Add another GitHub account" button when at least one link exists - buildInstallationSettingsUrl branches by github_account_type (User → /settings/installations/<id>, Organization → /organizations/<login>/settings/installations) repositories.add page: - installation selector via shared <InstallationSelect> when 2+ active links - per-installation fetchAllInstallationRepos cache key - loader/action share resolveInstallationIdForApp helper for installation resolution - assertInstallationBelongsToOrg server-side validation - addRepository mutation accepts githubInstallationId + upsertRepositoryMembership github-users page: - loader returns installationOptions via shared getActiveInstallationOptions - searchGithubUsers accepts installationId, validates via assertInstallationBelongsToOrg - toolbar uses shared <InstallationSelect>; selection persisted in URL searchParams - Add button disabled when 2+ installations and none selected (UX feedback) shared helpers added: - app/libs/github-account.ts: formatGithubAccountLabel, isPersonalAccount, buildInstallationSettingsUrl - app/components/installation-select.tsx: <InstallationSelect> reusable component - app/services/github-integration-queries.server.ts: getActiveInstallationOptions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- useEffect on copyFetcher: depend on the stable primitive fields (state, data, reset) instead of the whole fetcher object, which is a fresh reference each render and caused the effect to wake up on unrelated parent re-renders. - Extract the duplicated "GitHub App is not configured" error path from installGithubApp / copyInstallUrl into a shared helper. - repositories.add action: use getErrorMessage(e) instead of the hand-rolled `e instanceof Error ? ... : 'Invalid installation id'` so the codebase-wide error extraction rule (CLAUDE.md) is honored. - assertInstallationBelongsToOrg: throw AppError (user-safe) instead of plain Error so getErrorMessage surfaces the real reason to the caller instead of the generic fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Repositories add action: defend against hidden-input tampering by verifying that the selected installation can actually see (owner, name) before inserting. Reuses the loader's installation repo cache (5 min TTL) so the extra call is free. 2. Repositories add loader (github_app mode): relax the strict `invalid owner` throw when at least one installation's repo list failed to fetch. With Promise.allSettled() we intentionally degrade instead of crashing. 3. integration-settings.tsx copyFetcher effect: depend only on the primitive `copyFetcher.state`. useFetcher() returns a fresh object each render so depending on `.data` / `.reset` caused the effect to rerun on every parent render. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous narrowing to `[copyFetcher.state]` with a biome-ignore followed a CodeRabbit suggestion that useFetcher() returns a fresh object each render. Verified against the react-router v7 source (packages/react-router/lib/dom/lib.tsx): `fetcherWithComponents` is wrapped in React.useMemo and `reset` / `submit` / `load` are each wrapped in React.useCallback with stable deps, so the object reference and its callbacks are preserved across renders. Restore `[copyFetcher.state, copyFetcher.data, copyFetcher.reset]` so biome is happy without an ignore comment and the dep list matches the real stability guarantees of useFetcher. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- AddRepoSchema: owner/name now use z.string().trim().min(1) so empty/whitespace-only hidden-input values are rejected at the schema boundary before reaching addRepository(). In token mode nothing else stops them; in github_app mode the visibility check would already reject them, but defense-in-depth at the parse step is cheap and consistent with CLAUDE.md boundary validation. - Visibility check (resolveOctokitForInstallation + getOrgCachedData → fetchAllInstallationRepos): wrap in try/catch so GitHub API outages or a newly-revoked installation surface as a dataWithError toast instead of bubbling to the action's ErrorBoundary. The loader already degrades via Promise.allSettled; the action should be symmetric. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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
`@app/routes/`$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts:
- Around line 27-31: Currently the code picks only the first active installation
([firstActive]) and returns [] if that one fails; instead iterate over the list
returned by getActiveInstallationOptions(organizationId), for each installation
call resolveOctokitForInstallation(installation.installationId), attempt the
search.users call with that octokit, and on any error
(rate-limit/revoke/temporary failure) catch and continue to the next
installation; only return [] after all installations have been tried and failed.
Ensure you reference resolveOctokitForInstallation,
getActiveInstallationOptions, firstActive, octokit and the search.users call
when implementing the sequential fallback.
In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx:
- Around line 299-309: The current visibility recheck uses the shared 5-minute
cache via getOrgCachedData when building visibleRepos (using
resolveOctokitForInstallation and fetchAllInstallationRepos), which can allow
stale results to pass canAccessRepo; update the action to bypass the shared
cache for this security-sensitive check by either calling
fetchAllInstallationRepos(octokit) directly (no getOrgCachedData) or, better,
perform per-repo fresh verification using the installation octokit (e.g., call
the GitHub API/permission check inside canAccessRepo for each
submission.value.installationId repo) so visibility is based on live data rather
than the cached visibleRepos.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9085e0f-7755-4d02-8833-c29326d1851d
📒 Files selected for processing (13)
app/libs/github-account.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.tsapp/routes/$orgSlug/settings/integration/index.tsxapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.add/index.tsxapp/services/github-integration-queries.server.tsdocs/rdd/issue-283-work-plan.md
✅ Files skipped from review due to trivial changes (3)
- app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts
- app/libs/github-account.ts
- app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.ts
- app/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsx
- app/services/github-integration-queries.server.ts
- app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.ts
- docs/rdd/issue-283-work-plan.md
Comment on lines
+299
to
+309
| let visibleRepos: Awaited<ReturnType<typeof fetchAllInstallationRepos>> | ||
| try { | ||
| const octokit = resolveOctokitForInstallation( | ||
| submission.value.installationId, | ||
| ) | ||
| visibleRepos = await getOrgCachedData( | ||
| organization.id, | ||
| `app-installation-all-repos:${submission.value.installationId}`, | ||
| () => fetchAllInstallationRepos(octokit), | ||
| 300000, | ||
| ) |
There was a problem hiding this comment.
可視性チェックに共有キャッシュを使うと、この再照合は最新状態を保証できません。
Line 304 で loader と同じ 5 分キャッシュを引いているため、GitHub 側で選択対象が変わった直後でも古い一覧で canAccessRepo が通ります。ここは hidden input 改ざん対策の根拠になる箇所なので、action 側はキャッシュを bypass するか、repo 単位の fresh な確認に寄せたいです。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx around lines 299 -
309, The current visibility recheck uses the shared 5-minute cache via
getOrgCachedData when building visibleRepos (using resolveOctokitForInstallation
and fetchAllInstallationRepos), which can allow stale results to pass
canAccessRepo; update the action to bypass the shared cache for this
security-sensitive check by either calling fetchAllInstallationRepos(octokit)
directly (no getOrgCachedData) or, better, perform per-repo fresh verification
using the installation octokit (e.g., call the GitHub API/permission check
inside canAccessRepo for each submission.value.installationId repo) so
visibility is based on live data rather than the cached visibleRepos.
Owner
Author
There was a problem hiding this comment.
この指摘は設計トレードオフとして defer します。以下の理由です:
スコープ: この visibility check の元々の目的は「hidden input 改ざん対策」で、installationId と owner/name の組み合わせを tamper した攻撃を防ぐことです。現在の実装は loader と同じキャッシュキーを見るので、この tamper 攻撃は確実に防げます。
5 分キャッシュ窓の脅威モデル: 攻撃者は requireOrgOwner を通過した org owner 本人です。古いキャッシュで canAccessRepo が通って broken な repository row が作られたとしても、攻撃者本人が自分の org に自己破壊的なレコードを作るだけで、情報漏洩・権限昇格は発生しません。
UX 一貫性: loader と action で別々のキャッシュ窓を使うと、"UI に表示されているリポジトリを Add ボタンで押したら 'アクセスできません' エラーになる" ような 5 分間の非一貫状態が発生します。これはユーザーから見ると不可解な挙動です。
コスト: security-sensitive な見方をするなら毎回 API コールになり、Add クリックごとに GitHub の rate limit を消費します。一方で防げる実害は "authenticated org owner が作った broken row"（crawl で拾えない無害な孤児行）に限定されます。
以上から、tamper 対策は現行で十分で、キャッシュ窓由来の不整合は "UI に表示されていたものを Add" という自然な操作なので許容、としています。将来的に installation の選択状態変更が高頻度で起きる前提が必要になったら再検討します。
There was a problem hiding this comment.
@coji、詳細なトレードオフ分析ありがとうございます。論点が整理されており、すべて理解しました。
loaderとactionが同一のキャッシュキー (app-installation-all-repos:${installationId}) を共有している点で tamper 攻撃は防げているrequireOrgOwner通過済みの攻撃者に対して、最悪ケースは "自分の org に無害な孤児行が作られる" に限定され、情報漏洩・権限昇格は発生しない- fresh API コールに変えると UX の不一致と rate limit 消費が生じる
いずれも妥当な判断です。指摘を取り下げます。
(╯°□°）╯︵ ┻━┻ → ┬─┬ノ( º _ ºノ)
✏️ Learnings added
Learnt from: coji
Repo: coji/upflow PR: 291
File: app/routes/$orgSlug/settings/repositories.add/index.tsx:299-309
Timestamp: 2026-04-11T16:42:46.701Z
Learning: In `app/routes/$orgSlug/settings/repositories.add/index.tsx`, the `action` for adding a GitHub App repository intentionally reuses the shared 5-minute `getOrgCachedData` cache (key: `app-installation-all-repos:${installationId}`) to verify repository visibility (`canAccessRepo`), matching the loader's cache window. This is a deliberate design tradeoff: (1) the tamper attack (pairing a valid org-owned installationId with a repo from a different installation) is still fully prevented because loader and action share the same cache key; (2) the attacker must already be an authenticated org owner (`requireOrgOwner`), so stale-cache false-positives can only create harmless broken rows in their own org — no data leakage or privilege escalation; (3) using a fresh API call per Add click would cause UX inconsistency (repos shown in UI could fail the action check) and consume GitHub rate limits unnecessarily. Do not flag this cached visibility check as a security issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-31T09:33:01.662Z
Learning: Applies to app/routes/$orgSlug/**/*.tsx : For child resource mutations by ID (repository, member), verify `resource.organizationId === organization.id` before any mutation at the route layer
Learnt from: coji
Repo: coji/upflow PR: 0
File: :0-0
Timestamp: 2026-04-07T13:45:02.175Z
Learning: In coji/upflow, issue `#283` (multiple GitHub App installations) is implemented as a 7-PR stack. PR `#288` is schema+type-generation only. PR `#289` adds `getGithubAppLinks()` array return, `disconnectGithubAppLink(orgId, installationId)`, per-repo Octokit resolution. PR `#290` rewrites `github-webhook-installation.server.ts` to per-installation lookup and adds canonical reassignment, audit log, and auto repair. Final strict cleanup (removing `OR github_installation_id IS NULL` etc.) happens in PR 7. Work plan is documented in `docs/rdd/issue-283-work-plan.md`. Do not flag "1 org = 1 row" patterns in PR `#288` as incomplete — they are intentionally deferred.
Learnt from: coji
Repo: coji/upflow PR: 291
File: app/routes/$orgSlug/settings/integration/index.tsx:82-95
Timestamp: 2026-04-11T15:27:39.052Z
Learning: In coji/upflow, `requireOrgOwner` (owner-only check) is a stricter alternative to `requireOrgMember`/`requireOrgAdmin`. When reviewing `app/routes/$orgSlug/settings/integration/index.tsx` or similar owner-only routes, do not flag missing auth check if `requireOrgOwner` is already called before `parseWithZod` — it satisfies the guideline requiring auth before form parsing.
Learnt from: coji
Repo: coji/upflow PR: 213
File: app/routes/$orgSlug/settings/data-management/index.tsx:248-266
Timestamp: 2026-03-18T12:45:09.072Z
Learning: In app/routes/$orgSlug/settings/data-management/index.tsx, isCrawlRunning/isRecalculateRunning are intentionally computed from the paginated runs array (page 0). This is acceptable because: (1) useRuns returns runs in createdAt descending order so active (pending/leased) jobs always appear on page 0; (2) server-side concurrencyKey prevents duplicate job execution even if the button were re-enabled; (3) SSE real-time updates only apply to page 0. Do not flag this pattern as a bug.
Learnt from: coji
Repo: coji/upflow PR: 0
File: :0-0
Timestamp: 2026-04-09T02:18:55.867Z
Learning: In coji/upflow, `useForm` from `conform-to/react` returns a new object reference on every render (not memoized/stable). Therefore, `form` returned by `useForm` should NOT be included in `useEffect`/`useCallback` dependency arrays — doing so causes the effect to run on every render. When Biome flags this omission, use `biome-ignore lint/correctness/useExhaustiveDependencies` with an explanatory comment. This pattern is confirmed and intentional in this repository (e.g., `app/routes/$orgSlug/settings/members/+components/member-row-actions.tsx`). Note: `fetcher` from `useFetcher` is separately stable (memoized in React Router v7) and does NOT need this treatment.
Learnt from: coji
Repo: coji/upflow PR: 220
File: app/components/ui/form.tsx:0-0
Timestamp: 2026-03-20T10:47:05.300Z
Learning: In coji/upflow, `react-hook-form` is not installed as a dependency. The `app/components/ui/form.tsx` shadcn/ui component (which depends on react-hook-form) was intentionally removed in PR `#220` as part of the policy to exclude components whose dependencies are not installed. Do not flag issues in form.tsx or suggest react-hook-form usage in this repository.
Learnt from: coji
Repo: coji/upflow PR: 269
File: batch/github/fetcher.ts:1208-1217
Timestamp: 2026-03-31T08:36:33.653Z
Learning: In coji/upflow's `batch/github/fetcher.ts`, the `comments()` function intentionally uses a single GraphQL query (`GetPullRequestCommentsQuery`) with two non-standard cursor variables (`commentsCursor` and `reviewThreadsCursor`). Passing `null` for the unused cursor on each side is by design — splitting into two separate queries would double API calls. The `reviewThreads.comments` being capped at `first: 100` without nested pagination is also a known, accepted constraint. Do not flag this pattern as a bug or suggest query splitting.
Learnt from: coji
Repo: coji/upflow PR: 141
File: app/components/form-dialog.tsx:43-43
Timestamp: 2026-03-09T08:24:49.797Z
Learning: Do not flag missing useCallback/useMemo dependency array entries in review unless Biome (the authoritative linter for dependency exhaustiveness in this repo) detects them. If Biome does not report an issue, treat it as non-actionable for all TSX files in the repository (including app/components/form-dialog.tsx).
Learnt from: coji
Repo: coji/upflow PR: 220
File: app/components/ui/tooltip.tsx:19-23
Timestamp: 2026-03-20T10:46:12.956Z
Learning: In coji/upflow, TooltipProvider (from app/components/ui/tooltip.tsx) is configured once at the root level (app/root.tsx) using the shadcn/ui pattern. When reviewing TSX files, do not expect (and do not flag) Tooltip usages to wrap TooltipPrimitive.Root (or individual tooltip primitives) with TooltipProvider—those descendants should rely on the root-level provider instead. The only TooltipProvider wrapping should be at the repository root configuration, not inside each tooltip component/usage.
Learnt from: coji
Repo: coji/upflow PR: 220
File: app/components/ui/dropdown-menu.tsx:74-76
Timestamp: 2026-03-20T10:52:59.295Z
Learning: When reviewing shadcn/ui + Tailwind v4 projects (including coji/upflow), do not flag the Tailwind v4 arbitrary variant syntax `*:[svg]:some-utility` (or similar `*:[<tag>]:...` / multi-variant chains like `data-[variant=destructive]:*:[svg]:text-destructive!`). In Tailwind v4, `*:` is the built-in “direct child” variant and `[svg]` is a valid bracket tag-name selector; these patterns are intentional and supported, so they are not invalid CSS and not incorrect Tailwind usage.
… search fallback Three functional regressions flagged on PR #291: P1 — integration/index.tsx disconnectGithubAppLink branch only soft-deleted the link, skipping the canonical reassignment that the webhook path runs via reassignCanonicalAfterLinkLoss(). In an org with multiple installations, disconnecting one through the UI would leave its repositories pointing at a deleted link, so resolveOctokitForRepository() started throwing and crawl/backfill skipped those repos until someone repaired them by hand. Call the same helper here with source='user_disconnect' so the two paths stay symmetric. P2 — github-account.ts treated every non-'User' githubAccountType as an organization, including null. Legacy github_app_links rows migrated before the column existed are null until a webhook or re-setup repopulates them, so the new settings screen rendered them with organization wording and built an /organizations/<login>/settings/installations URL that 404s for personal accounts. Introduce a getAccountKind() helper returning 'personal' | 'organization' | 'unknown'. The label drops the prefix entirely for unknown, and buildInstallationSettingsUrl() now returns string | null so InstallationCard can omit the link rather than guess. P3 — searchGithubUsers picked only the first active installation and returned [] if that one call failed. A single rate-limited or revoked installation would silently break the search UI for the whole org even though other installations were available. Iterate active installations in order, catch per-installation errors, and fall through to the next one. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.ts (1)
47-55:⚠️ Potential issue | 🟠 Major重複 POST で既存の
githubInstallationIdを消せます。ここは
onConflict時に常にexcluded.githubInstallationIdを採用しているので、既存 repo への stale / 細工済み POST がinstallationIdなしで飛ぶと、保存済みの GitHub App 紐付けがnullに上書きされます。UI のisAddedはクライアント側だけなので、nullが来たときは既存値を保持しないと、repositories.githubInstallationIdだけ消えてrepositoryInstallationMembershipsとズレます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/+functions/mutations.server.ts around lines 47 - 55, The onConflict doUpdateSet always overwrites githubInstallationId with excluded.githubInstallationId which allows a stale/empty POST to null out an existing installation; update the doUpdateSet mapping so githubInstallationId uses the excluded value only when it's non-null (e.g., COALESCE/existing-value pattern) instead of unconditionally assigning eb.ref('excluded.githubInstallationId'), keeping the current repositories.githubInstallationId when excluded.githubInstallationId is null; change the githubInstallationId assignment inside the onConflict -> doUpdateSet callback (the eb.ref('excluded.githubInstallationId') entry) to use a COALESCE or conditional expression that falls back to the existing repositories.githubInstallationId.
🧹 Nitpick comments (2)
app/routes/$orgSlug/settings/repositories.add/index.tsx (2)
299-318: 再照合エラーも Sentry に送っておくと追跡しやすいです。ここは
dataWithError()で劣化運転できていて良いですが、現在はconsole.errorだけなので、本番で installation revoke や GitHub 側障害が増えても検知しづらいです。下のmembership.upsert/crawl.triggerと同じ粒度でorganizationId・installationIdを付けて送っておくと運用しやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx around lines 299 - 318, The catch block that handles failures when calling resolveOctokitForInstallation/getOrgCachedData/fetchAllInstallationRepos currently only console.error's the exception; augment it to also report the error to Sentry (or the app's error-tracking API) including context fields organizationId (organization.id) and installationId (submission.value.installationId) and the caught error object, while preserving the existing dataWithError return path—mirror the same metadata granularity used around membership.upsert and crawl.trigger so operators can correlate installation revoke or GitHub outages.
170-188: 最古 installation 優先なら並び順をここで保証したいです。この first-win dedupe は
installationOptionsの順序に意味がありますが、app/services/github-integration-queries.server.ts:74-86のgetActiveInstallationOptions()側では並び順が明示されていません。上流の取得順が変わると、同じowner/repoでも reload ごとに採用されるinstallationIdが揺れるので、createdAt順を helper 側で明示しておくと安定します。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx around lines 170 - 188, The dedupe "first-win" behavior depends on the order of installationOptions, so ensure getActiveInstallationOptions() returns installation options sorted by createdAt ascending (oldest first) to make the first-win deterministic; update getActiveInstallationOptions() to sort its result by the installation.createdAt (or createdAt) field before returning, so the dedupe logic that iterates installationOptions/settled and uses seen/new allTagged picks the oldest installation consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/routes/`$orgSlug/settings/repositories.add/+functions/mutations.server.ts:
- Around line 47-55: The onConflict doUpdateSet always overwrites
githubInstallationId with excluded.githubInstallationId which allows a
stale/empty POST to null out an existing installation; update the doUpdateSet
mapping so githubInstallationId uses the excluded value only when it's non-null
(e.g., COALESCE/existing-value pattern) instead of unconditionally assigning
eb.ref('excluded.githubInstallationId'), keeping the current
repositories.githubInstallationId when excluded.githubInstallationId is null;
change the githubInstallationId assignment inside the onConflict -> doUpdateSet
callback (the eb.ref('excluded.githubInstallationId') entry) to use a COALESCE
or conditional expression that falls back to the existing
repositories.githubInstallationId.
---
Nitpick comments:
In `@app/routes/`$orgSlug/settings/repositories.add/index.tsx:
- Around line 299-318: The catch block that handles failures when calling
resolveOctokitForInstallation/getOrgCachedData/fetchAllInstallationRepos
currently only console.error's the exception; augment it to also report the
error to Sentry (or the app's error-tracking API) including context fields
organizationId (organization.id) and installationId
(submission.value.installationId) and the caught error object, while preserving
the existing dataWithError return path—mirror the same metadata granularity used
around membership.upsert and crawl.trigger so operators can correlate
installation revoke or GitHub outages.
- Around line 170-188: The dedupe "first-win" behavior depends on the order of
installationOptions, so ensure getActiveInstallationOptions() returns
installation options sorted by createdAt ascending (oldest first) to make the
first-win deterministic; update getActiveInstallationOptions() to sort its
result by the installation.createdAt (or createdAt) field before returning, so
the dedupe logic that iterates installationOptions/settled and uses seen/new
allTagged picks the oldest installation consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7060b61-5a0b-4ceb-bdc9-2bcfca33bdb2
📒 Files selected for processing (13)
app/libs/github-account.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.tsapp/routes/$orgSlug/settings/integration/index.tsxapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.add/index.tsxapp/services/github-integration-queries.server.tsdocs/rdd/issue-283-work-plan.md
✅ Files skipped from review due to trivial changes (3)
- app/routes/$orgSlug/settings/repositories.add/+functions/get-repositories-by-owner-and-keyword.ts
- app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts
- docs/rdd/issue-283-work-plan.md
🚧 Files skipped from review as they are similar to previous changes (5)
- app/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts
- app/routes/$orgSlug/settings/_index/+schema.ts
- app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.ts
- app/services/github-integration-queries.server.ts
- app/libs/github-account.ts
👮 Files not reviewed due to content moderation or server errors (2)
- app/routes/$orgSlug/settings/integration/index.tsx
- app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsx
1. mutations.server.ts: onConflict doUpdateSet now preserves the existing githubInstallationId when the incoming value is null (COALESCE). A stale POST from token mode no longer wipes out an existing installation link. 2. repositories.add action: the visibility-check catch block now sends the exception to Sentry with organizationId and installationId context, matching the granularity of the membership.upsert and crawl.trigger error paths. 3. getActiveInstallationOptions JSDoc: document that the createdAt ASC ordering (inherited from getGithubAppLinks) is load-bearing for the first-win dedupe in the repositories add loader. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
coji
added a commit
that referenced
this pull request
Apr 12, 2026
## Summary Issue #283 の実装 stack **PR 5/7** — broken state（`github_installation_id IS NULL`）の repository を救済する UI / mutation / batch CLI を追加。 設計根拠: [\`docs/rdd/issue-283-multiple-github-accounts.md\`](./docs/rdd/issue-283-multiple-github-accounts.md) 作業計画: [\`docs/rdd/issue-283-work-plan.md\`](./docs/rdd/issue-283-work-plan.md) 依存: #288 (PR 1: schema), #296 (PR 2: query/octokit), #290 (PR 3: webhook/membership), #291 (PR 4: UI) ## UX 哲学 PR 4 の Vercel 哲学を継承し、**通常時は repository 一覧/詳細に installation 名を出さない**。canonical installation を失った broken state の repository だけを救済導線として可視化する。 ## 変更内容 ### service helper (\`app/services/github-app-membership.server.ts\`) - \`fetchEligibleInstallationIds()\` 内部 helper 抽出 — \`reassignCanonicalAfterLinkLoss\` と \`reassignBrokenRepository\` で eligible link 取得ロジックを共有 - \`reassignBrokenRepository(orgId, repoId, source)\` 追加 - canonical installation を持たない repo に対して membership table から候補を引いて再割当を試みる - 結果を discriminated union で返す: \`reassigned | no_candidates | ambiguous | not_broken\` - 1 候補 → \`canonical_reassigned\` audit log を書く - 0 候補 / 2+ 候補 → 戻り値で表現、audit log は書かない（installationId が無いため） - \`isRepositoryBroken()\` を \`app/libs/github-account.ts\` に追加（client-safe） ### route mutation (\`app/routes/$orgSlug/settings/repositories._index/\`) - loader が \`integrationMethod\` を \`'token' | 'github_app' | null\` で返す - action に \`reassignBroken\` intent 追加（\`source: 'manual_reassign'\`） - discriminated union を \`match.exhaustive\` でハンドリング ### UI (\`+components/repo-columns.tsx\`) - \`NeedsReconnectionBadge\` コンポーネント - \`isRepositoryBroken(repo, integrationMethod)\` で判定 - destructive variant Badge + \`AlertTriangleIcon\` + tooltip - 1-click "Reassign" ボタン (per-row \`useFetcher\`) - 結果別 toast: - 成功 → "Repository reassigned to an active installation." - 候補 0 → "No active installation can see this repository. Reinstall the GitHub App and try again." - 候補 2+ → "Disconnect the unwanted installations to resolve." ### batch CLI (\`batch/commands/reassign-broken-repositories.ts\`) - \`pnpm batch reassign-broken-repositories <orgId> [--repository <id>]\` - broken repository を全て列挙 → 順次 \`reassignBrokenRepository\` (\`source: 'cli_repair'\`) - 結果集計と各 repo の状況を consola で出力 - \`match.exhaustive\` で網羅性確保 ### tests - \`github-app-membership.server.test.ts\` に 6 ケース追加: - 1 候補 → reassigned + canonical_reassigned event - 0 候補 → no_candidates (audit log なし) - 2+ 候補 → ambiguous (audit log なし) - not_broken（既に canonical あり） - suspended link は候補から除外 - 未初期化 link は候補から除外 ## 満たす受入条件 - **#7**: 失われた canonical installation の repository 救済経路 - **#18**: assignment required 状態の表示と再割当 UI ## Stack 位置 \`\`\`text PR 1 (#288): schema └ PR 2 (#296): query/octokit └ PR 3 (#290): webhook/membership └ PR 4 (#291): UI └ [PR 5: repo UI] ← this PR └ PR 6 (backfill) └ PR 7 (strict) \`\`\` ## テスト - [x] \`pnpm validate\` (lint / format / typecheck / build / test 全 345 tests) - [x] \`reassignBrokenRepository\` の 6 ケースをユニットテストでカバー 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **新機能** * GitHub App連携で「接続切れ（broken）」判定を共通化するチェックを追加 * リポジトリ一覧に警告バッジを表示し、バッジから再割り当て（再接続）操作を実行可能に（操作中の状態表示・ツールチップ付き） * 壊れたリポジトリを一括検出・再割り当てするCLIコマンドを追加 * **テスト** * 再割り当ての各結果（再割当て／候補なし／あいまい／初期化待ち／未該当／未発見）を検証する自動テストを追加 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
coji
added a commit
that referenced
this pull request
Apr 12, 2026
#283 PR 6/7) (#293) ## Summary Issue #283 の実装 stack **PR 6/7** — PR 7 (strict lookup 切替) のデプロイ準備を行う one-shot batch CLI。 設計根拠: [\`docs/rdd/issue-283-multiple-github-accounts.md\`](./docs/rdd/issue-283-multiple-github-accounts.md) 作業計画: [\`docs/rdd/issue-283-work-plan.md\`](./docs/rdd/issue-283-work-plan.md) 依存: #288 (PR 1: schema), #296 (PR 2), #290 (PR 3), #291 (PR 4), #292 (PR 5) ## 変更内容 ### batch CLI (\`batch/commands/backfill-installation-membership.ts\`) \`pnpm batch backfill-installation-membership [orgId] [--dry-run]\` 組織ごとに以下の判定: | 組織の状態 | 動作 | 結果 | |---|---|---| | \`integrations.method = 'token'\` | skip | \`skipped_token_method\` | | github_app + 0 active link | skip + warn | \`skipped_no_active_link\` (要 reinstall) | | github_app + 1 active link | bulk backfill | \`backfilled_single_link\` | | github_app + 2+ active links | skip + warn | \`skipped_multi_link_unmapped\` (要 \`reassign-broken-repositories\`) | ### 動作詳細 - **bulk backfill**: \`UPDATE repositories SET github_installation_id = ? WHERE github_installation_id IS NULL\` で一括設定 - **membership seed**: \`fetchInstallationRepositories\` (GitHub API) で installation の見える repo を全取得 → \`initializeMembershipsForInstallation\` で bulk 投入 - **API 失敗時 fallback**: orphan repository に対してのみ \`upsertRepositoryMembership\` を呼ぶ（最低限の membership を確保、本格的な seed は次回 crawl の repair に委譲） - **冪等性**: \`WHERE github_installation_id IS NULL\` で再実行 no-op - **\`--dry-run\`**: 書き込みも GitHub API 呼び出しも行わず判定だけ実行 ### Runbook (本番デプロイ手順) 1. **PR 1-5 がマージ・本番デプロイ済みであることを確認** 2. **本番 DB のスナップショット**: \`pnpm ops pull-db -- --app upflow\` 3. **dry-run で計画確認**: \`\`\`bash pnpm batch backfill-installation-membership -- --dry-run \`\`\` - skip 件数 / backfill 対象件数 / 要注意組織（multi-link / no-link）を確認 4. **実行**: \`\`\`bash pnpm batch backfill-installation-membership \`\`\` 5. **検証**: \`integrations.method = 'github_app'\` の組織に対して以下が 0 件であること: \`\`\`sql SELECT count(*) FROM repositories WHERE github_installation_id IS NULL; \`\`\` 6. **multi-link 組織の対応**: \`pnpm batch reassign-broken-repositories <orgId>\` で各 multi-link 組織を救済 7. **検証 OK 後** PR 7 をマージ ### Stack 位置 \`\`\`text PR 1 (#288): schema └ PR 2 (#296): query/octokit └ PR 3 (#290): webhook/membership └ PR 4 (#291): UI └ PR 5 (#292): repo UI └ [PR 6: backfill] ← this PR └ PR 7 (strict) \`\`\` ## 満たす受入条件 - **#20**: schema → backfill 完了 → strict lookup 切替 (前半) ## テスト - [x] \`pnpm validate\` (lint / format / typecheck / build / test 全 351 tests) - [x] CLI コマンドの 6 ケースを vitest でカバー (single / token / multi / dry-run / idempotent / API failure fallback) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## リリースノート * **新機能** * `backfill-installation-membership` コマンドを CLI に追加しました。このコマンドは GitHub App インストール連携時に、リポジトリのインストール ID とメンバーシップ情報を一括更新する機能を提供します。`--dryRun` フラグでプレビュー実行も可能です。 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
coji
added a commit
that referenced
this pull request
Apr 12, 2026
…acks (#283 PR 7/7) (#294) ## Summary Issue #283 stack の **最終 PR (7/7)** — 移行期間 fallback を全削除し、strict installation lookup に切替。これでシリーズ完結。 設計根拠: [\`docs/rdd/issue-283-multiple-github-accounts.md\`](./docs/rdd/issue-283-multiple-github-accounts.md) 作業計画: [\`docs/rdd/issue-283-work-plan.md\`](./docs/rdd/issue-283-work-plan.md) 依存: #288, #296, #290, #291, #292, #293 ##⚠️ デプロイ順序 このマージは **PR 6 (#293) の \`backfill-installation-membership\` を本番実行 + 検証完了後** にのみ実行してください。順序を守らないと既存リポジトリが strict lookup で弾かれてエラーになります。 詳細手順は #293 の Runbook 参照。 ## 変更内容 ### schema (\`db/shared.sql\` + migration) - \`integrations.app_suspended_at\` カラム削除（PR 1 で \`github_app_links.suspended_at\` に移行済み） - Atlas が table-rebuild migration を生成 (\`20260408001949.sql\`) - \`app/services/type.ts\` 再生成 ### Octokit 解決 (\`app/services/github-octokit.server.ts\`) - \`resolveOctokitForRepository\` 内の transitional fallback を全削除: - \`github_app + githubInstallationId IS NULL\` → エラー \"Repository has no canonical installation assigned. Run reassign-broken-repositories or reinstall.\" - \"active link 1 件で代用\" のロジック削除 - 削除した legacy export: - \`assertOrgGithubAuthResolvable\` - \`resolveOctokitFromOrg\` - \`OrgGithubAuthInput\` ### query 層 (\`app/services/github-integration-queries.server.ts\`) - \`getGithubAppLink\` (deprecated) を削除（caller ゼロ） ### PR webhook (\`app/services/github-webhook-pull.server.ts\`) - 移行期間の \`OR github_installation_id IS NULL\` を削除し strict 化: \`\`\`sql WHERE owner = ? AND repo = ? AND github_installation_id = ? \`\`\` - broken state webhook を ops が検知できるよう、各 silent return に \`debug()\` ログを追加（\`createDebug('app:github-webhook:pull')\`） ### route (\`app/routes/$orgSlug/settings/repositories/$repository/$pull/\`) - \`index.tsx\`: \`resolveOctokitFromOrg\` から \`resolveOctokitForRepository\` に migration - \`queries.server.ts\`: \`getRepository\` の select に \`githubInstallationId\` 追加 ### cleanup - \`appSuspendedAt\` への参照を全削除 (setup callback / webhook handler / mutation / batch query / test fixture) - \`github-octokit.server.test.ts\` から legacy 関数のテストと transitional fallback ケースを削除 ## 満たす受入条件 - **#16**: PR webhook strict lookup - **#20** (完全達成): schema → backfill → strict lookup 切替 ## デプロイ Rollback 戦略 - migration は destructive (column drop)。本番デプロイ前に staging で apply → rollback → re-apply のリハーサル必須 - デプロイ後 24 時間は \`github_app_link_events\` の異常パターン (連続 \`canonical_reassigned\`, \`webhook_dropped\` debug ログの急増) を監視 - 異常検知時: 1. PR 7 を revert 2. \`app_suspended_at\` を復活させる down migration を当てる (Atlas で生成済み) 3. 必要なら \`reassign-broken-repositories\` で個別 repo 救済 ## Stack 位置 (完成形) \`\`\`text PR 1 (#288): schema └ PR 2 (#296): query/octokit └ PR 3 (#290): webhook/membership └ PR 4 (#291): UI └ PR 5 (#292): repo UI └ PR 6 (#293): backfill └ [PR 7: strict] ← this PR \`\`\` ## テスト - [x] \`pnpm validate\` (lint / format / typecheck / build / test 全 342 tests) - [x] \`resolveOctokitForRepository\` の strict path テスト (canonical なし → throw) - [x] migration \`pnpm db:setup\` で再現可能 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **バグ修正** * リポジトリのGitHub Appインストール割り当てがない場合に厳格にエラーを返すようにし、エラーメッセージをより具体的に表示するよう改善しました。 * **リファクタリング** * 組織スコープの旧ヘルパーを削除し、Webhook処理にデバッグログを追加して追跡性を向上させました。 * 統合の解決フローを整理しました。 * **雑務** * 統合テーブルから未使用フィールドを削除するスキーマ変更と関連マイグレーション、CLI表示文言、テスト修正を行いました。 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #283 の実装 stack PR 4/7 — settings UI を multi-installation 対応に。Vercel ライクに installation の存在をユーザーに意識させない UX。
設計根拠: `docs/rdd/issue-283-multiple-github-accounts.md`
作業計画: `docs/rdd/issue-283-work-plan.md`
依存: #288 (PR 1: schema), #296 (PR 2: query/octokit), #290 (PR 3: webhook/membership)
変更内容
integration page (`app/routes/$orgSlug/settings/integration/index.tsx`)
GitHub App セクション UI
repositories.add page (Vercel 風 merge UI)
github-users page
共有 helpers
満たす受入条件
Stack 位置
```text
PR 1 (#288): schema
└ PR 2 (#296): query/octokit
└ PR 3 (#290): webhook/membership
└ [PR 4: UI] ← this PR
└ PR 5 (repo UI)
└ PR 6 (backfill)
└ PR 7 (strict)
```
UX 哲学
最初は installation selector で実装したが、Vercel など先行サービスが「installation を意識させない」UX を取っているのを参考に書き直し。ユーザーは repository を選ぶだけでよく、installation は内部の attribution として透過的に処理される。
テスト
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
改善
ドキュメント