feat: excludedUsersをcompanyGithubUsers.typeに統合#215
Merged
Conversation
excludedUsersをorganizationSettingsのカンマ区切りテキストから companyGithubUsers.type='Bot'に統合。batch処理のfilterActors()が DBのBot typeを直接参照するようになり、設定UIのexcludedUsersフィールドを削除。 - マイグレーションで既存excludedUsers値をcompanyGithubUsers.type='Bot'に移行 - DEFAULT_EXCLUDED_USERSハードコードを削除し、DBベースのBot管理に統一 - well-known bot（copilot, dependabot, renovate等）を新規登録時に自動Bot設定 - BuildConfig.excludedUsers → botLogins: Set<string>に変更（case-insensitive） - getBotLoginsをgetTenantDataに統合し並行クエリ化 Closes #146 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughorganizationSettings の Changes
Sequence Diagram(s)sequenceDiagram
participant UI as 組織設定UI
participant App as アプリサーバ
participant TenantDB as Tenant DB\n(organization_settings)
participant Batch as バッチプロセス
participant BatchDB as Batch DB\n(company_github_users)
participant Worker as 分析ワーカー
Note over UI,App: excludedUsers フィールドは UI から削除
UI->>App: 設定保存 (excludedUsers を送らない)
App->>TenantDB: organization_settings 更新（excluded_users 列なし）
Batch->>BatchDB: getBotLogins(organizationId)
BatchDB-->>Batch: botLogins (string[]／Set)
Batch->>Worker: Analyze 実行 (botLogins を含む)
Worker-->>Batch: result + botUsers
Batch->>BatchDB: upsertCompanyGithubUsers(..., botUsers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
batch/db/mutations.ts (1)
251-263:⚠️ Potential issue | 🟠 Major既存の既知Bot が
type='Bot'に昇格しない経路が残っています。
doNothing()のため、既に存在する login は Line 257 のtype付与が適用されません。
既知Bot が例えば app/routes の mutation 経由でtype=nullで作成されると、その後 batch function を実行しても type 更新されません。migration は一度きりのため、継続的なデータ整合性を保証しません。💡 修正案（既存行は
type IS NULLの場合のみ昇格）await tenantDb .insertInto('companyGithubUsers') .values( uniqueLogins.map((login) => ({ login, displayName: login, type: KNOWN_BOTS.has(login) ? 'Bot' : null, isActive: 0, updatedAt: now, })), ) .onConflict((oc) => oc.column('login').doNothing()) .execute() + + const knownBotLogins = uniqueLogins.filter((login) => KNOWN_BOTS.has(login)) + if (knownBotLogins.length > 0) { + await tenantDb + .updateTable('companyGithubUsers') + .set({ type: 'Bot', updatedAt: now }) + .where('login', 'in', knownBotLogins) + .where('type', 'is', null) + .execute() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/db/mutations.ts` around lines 251 - 263, The insert currently uses onConflict(...).doNothing(), so existing rows never get their type updated to 'Bot'; change the upsert to update existing rows only when their type IS NULL. Replace the onConflict(...).doNothing() call on tenantDb.insertInto('companyGithubUsers') with an onConflict handler that does an update (e.g., onConflict((oc) => oc.column('login').doUpdateSet(...).where('type', 'is', null'))) and set type: 'Bot' (and update updatedAt) for rows where companyGithubUsers.type IS NULL, using the same KNOWN_BOTS lookup for the values generated from uniqueLogins.
🧹 Nitpick comments (1)
batch/github/__tests__/buildPullRequests-filter.test.ts (1)
204-210:botLoginsの大小文字差異を検知する回帰テストを1本追加しておくと安全です。今回の移行ポイントが「ケースインセンシティブ」なので、
new Set(['Dependabot'])でもdependabotが除外されることを固定化しておくのをおすすめします。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/github/__tests__/buildPullRequests-filter.test.ts` around lines 204 - 210, Add a regression test to assert case-insensitive bot login matching: update the test suite in buildPullRequests-filter.test.ts to include a config where botLogins is populated with a differently-cased entry (e.g., new Set(['Dependabot'])), then run the existing filter logic (the code paths that read config.botLogins) and assert that a pull request authored by 'dependabot' is excluded; ensure you reference the existing config object and botLogins symbol and name the test to indicate case-insensitivity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@batch/db/queries.ts`:
- Around line 69-70: botLogins is returned without lower-case normalization
causing downstream matches (which call user.toLowerCase()) to miss entries like
"Dependabot"; update the mapping in getTenantData (the botLogins:
botLoginRows.map(...) expression) to normalize with .toLowerCase(), and also
ensure getBotLogins returns lower-cased logins too so both entry points
consistently yield lower-case bot login strings.
In `@db/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sql`:
- Around line 3-12: The migration currently builds a JSON array from
organization_settings.excluded_users and feeds it to json_each which will break
if excluded_users contains quotes, backslashes or newlines; instead, replace the
json_each approach with a safe comma-splitting function (e.g.
regexp_split_to_table(excluded_users, '\s*,\s*') or
unnest(string_to_array(excluded_users, ','))) and then TRIM/value-filter empty
strings, LOWER/TRIM each value and use that result to INSERT into
company_github_users (referencing organization_settings, excluded_users,
company_github_users and the ON CONFLICT clause) so you do not construct JSON at
all.
- Around line 14-17: The migration only backfills 'copilot' as a Bot but misses
other well-known bots listed in KNOWN_BOTS (e.g., dependabot, renovate,
github-actions), so update the INSERT/ON CONFLICT in the migration (file with
INSERT INTO company_github_users ...) to include those logins too (insert rows
for 'dependabot', 'renovate', 'github-actions', etc., all lowercase and with
type = 'Bot' and is_active = 0), ensuring the ON CONFLICT DO UPDATE still sets
type = 'Bot' so existing rows are converted; reference KNOWN_BOTS in
batch/db/mutations.ts to ensure parity of the list.
---
Outside diff comments:
In `@batch/db/mutations.ts`:
- Around line 251-263: The insert currently uses onConflict(...).doNothing(), so
existing rows never get their type updated to 'Bot'; change the upsert to update
existing rows only when their type IS NULL. Replace the
onConflict(...).doNothing() call on tenantDb.insertInto('companyGithubUsers')
with an onConflict handler that does an update (e.g., onConflict((oc) =>
oc.column('login').doUpdateSet(...).where('type', 'is', null'))) and set type:
'Bot' (and update updatedAt) for rows where companyGithubUsers.type IS NULL,
using the same KNOWN_BOTS lookup for the values generated from uniqueLogins.
---
Nitpick comments:
In `@batch/github/__tests__/buildPullRequests-filter.test.ts`:
- Around line 204-210: Add a regression test to assert case-insensitive bot
login matching: update the test suite in buildPullRequests-filter.test.ts to
include a config where botLogins is populated with a differently-cased entry
(e.g., new Set(['Dependabot'])), then run the existing filter logic (the code
paths that read config.botLogins) and assert that a pull request authored by
'dependabot' is excluded; ensure you reference the existing config object and
botLogins symbol and name the test to indicate case-insensitivity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e29fc7e7-d0c3-433d-b51f-fc4b333a1239
⛔ Files ignored due to path filters (1)
db/migrations/tenant/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
app/routes/$orgSlug/settings/_index/+forms/organization-settings.tsxapp/routes/$orgSlug/settings/_index/+functions/mutations.server.tsapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/_index/index.tsxapp/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsxapp/routes/$orgSlug/settings/repositories/$repository/$pull/queries.server.tsapp/services/jobs/analyze-worker.tsapp/services/jobs/crawl.server.tsapp/services/jobs/recalculate.server.tsapp/services/jobs/run-in-worker.tsapp/services/jobs/shared-steps.server.tsapp/services/tenant-type.tsbatch/db/mutations.tsbatch/db/queries.tsbatch/github/__tests__/buildPullRequests-filter.test.tsbatch/github/pullrequest.tsdb/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sqldb/tenant.sql
💤 Files with no reviewable changes (6)
- db/tenant.sql
- app/routes/$orgSlug/settings/_index/+schema.ts
- app/routes/$orgSlug/settings/_index/index.tsx
- app/routes/$orgSlug/settings/_index/+functions/mutations.server.ts
- app/routes/$orgSlug/settings/_index/+forms/organization-settings.tsx
- app/services/tenant-type.ts
batch/db/queries.ts
Outdated
Comment on lines
69
to
70
| botLogins: botLoginRows.map((r) => r.login), | ||
| } |
There was a problem hiding this comment.
botLogins を小文字正規化しないと Bot フィルタが漏れます。
下流の照合は user.toLowerCase() 前提です。ここで生値を返すと、DBに Dependabot などが入っていた場合に一致しません。
getTenantData / getBotLogins の両方で lower-case 正規化してください。
💡 修正案
return {
organizationSetting: organizationSetting ?? null,
integration: integration ?? null,
repositories,
exportSetting: exportSetting ?? null,
- botLogins: botLoginRows.map((r) => r.login),
+ botLogins: botLoginRows.map((r) => r.login.toLowerCase()),
}
}
@@
export async function getBotLogins(
organizationId: OrganizationId,
): Promise<Set<string>> {
@@
- return new Set(rows.map((r) => r.login))
+ return new Set(rows.map((r) => r.login.toLowerCase()))
}Also applies to: 73-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@batch/db/queries.ts` around lines 69 - 70, botLogins is returned without
lower-case normalization causing downstream matches (which call
user.toLowerCase()) to miss entries like "Dependabot"; update the mapping in
getTenantData (the botLogins: botLoginRows.map(...) expression) to normalize
with .toLowerCase(), and also ensure getBotLogins returns lower-cased logins too
so both entry points consistently yield lower-case bot login strings.
Comment on lines
+3
to
+12
| INSERT INTO company_github_users (login, display_name, type, is_active, updated_at) | ||
| SELECT | ||
| LOWER(TRIM(value)) AS login, | ||
| LOWER(TRIM(value)) AS display_name, | ||
| 'Bot' AS type, | ||
| 0 AS is_active, | ||
| CURRENT_TIMESTAMP AS updated_at | ||
| FROM organization_settings, json_each('["' || REPLACE(excluded_users, ',', '","') || '"]') | ||
| WHERE TRIM(value) != '' | ||
| ON CONFLICT (login) DO UPDATE SET type = 'Bot'; |
There was a problem hiding this comment.
excluded_users を JSON 文字列に組み立てる前提が危ないです。
元列は制約のない text なので、過去データに " / \ / 改行が 1 件でも入っていると json_each(...) が壊れて migration 全体が止まる可能性があります。ここは JSON を合成せず、カンマ分割をそのまま処理する形にしたほうが安全です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sql`
around lines 3 - 12, The migration currently builds a JSON array from
organization_settings.excluded_users and feeds it to json_each which will break
if excluded_users contains quotes, backslashes or newlines; instead, replace the
json_each approach with a safe comma-splitting function (e.g.
regexp_split_to_table(excluded_users, '\s*,\s*') or
unnest(string_to_array(excluded_users, ','))) and then TRIM/value-filter empty
strings, LOWER/TRIM each value and use that result to INSERT into
company_github_users (referencing organization_settings, excluded_users,
company_github_users and the ON CONFLICT clause) so you do not construct JSON at
all.
Comment on lines
+14
to
+17
| -- 2. Ensure 'copilot' is registered as Bot (case-insensitive - stored lowercase) | ||
| INSERT INTO company_github_users (login, display_name, type, is_active, updated_at) | ||
| VALUES ('copilot', 'copilot', 'Bot', 0, CURRENT_TIMESTAMP) | ||
| ON CONFLICT (login) DO UPDATE SET type = 'Bot'; |
There was a problem hiding this comment.
copilot だけでは既存 bot の backfill が足りません。
この PR では以後 company_github_users.type = 'Bot' だけを見に行くので、excluded_users に入っていない既存の well-known bot は Bot に寄せられず、tenant によっては人間扱いのまま残ります。少なくとも batch/db/mutations.ts の KNOWN_BOTS にある dependabot / renovate / github-actions などもここで backfill しておきたいです。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sql`
around lines 14 - 17, The migration only backfills 'copilot' as a Bot but misses
other well-known bots listed in KNOWN_BOTS (e.g., dependabot, renovate,
github-actions), so update the INSERT/ON CONFLICT in the migration (file with
INSERT INTO company_github_users ...) to include those logins too (insert rows
for 'dependabot', 'renovate', 'github-actions', etc., all lowercase and with
type = 'Bot' and is_active = 0), ensuring the ON CONFLICT DO UPDATE still sets
type = 'Bot' so existing rows are converted; reference KNOWN_BOTS in
batch/db/mutations.ts to ensure parity of the list.
- Remove 'claude' and 'chatgpt-codex-connector' from KNOWN_BOTS (human name collision risk) - Add COALESCE and empty-string guard to migration SQL for robustness Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
KNOWN_BOTSハードコードリストを削除し、GitHub APIの__typename === 'Bot' 情報をbuildPullRequestsからupsertCompanyGithubUsersに流すように変更。 新規ユーザー登録時にGitHub APIが返すbot判定を自動的にtype='Bot'として保存。 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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)
batch/db/mutations.ts (1)
252-260:⚠️ Potential issue | 🔴 Criticalマイグレーションで KNOWN_BOTS のすべてのボットが Bot として設定されていない
KNOWN_BOTSには 8 つのボット（copilot, copilot-pull-request-reviewer, copilot-swe-agent, github-actions, dependabot, renovate, coderabbitai, devin-ai-integration）が定義されていますが、マイグレーションファイル（db/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sql）では以下の問題があります：
- 明示的に Bot として設定されているのは
copilotのみ- 第 1 部では
organization_settings.excluded_usersから移行されるボットのみが処理されており、他の 6 つのボット（copilot-pull-request-reviewer, copilot-swe-agent, github-actions, renovate, coderabbitai, devin-ai-integration）はexcluded_usersに含まれていなければtype='Bot'に設定されませんmutations.tsのdoNothing()により、既存レコードは上書きされませんすべての
KNOWN_BOTSをマイグレーションでtype='Bot'に設定してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/db/mutations.ts` around lines 252 - 260, The insert currently builds rows from uniqueLogins and uses onConflict(...).doNothing(), so existing rows aren’t updated and only logins present in the excluded_users migration path get type='Bot'; update the logic to ensure every login in KNOWN_BOTS is persisted/updated with type='Bot' by (a) including all KNOWN_BOTS in the set of inserted/processed logins and (b) replacing onConflict(...).doNothing() with an upsert that merges/updates the type field (e.g., onConflict(...).merge() or an UPDATE setting type='Bot') so existing records for KNOWN_BOTS get type='Bot'; adjust the migration SQL (the migrate_excluded_users_to_bot_type migration logic) similarly so all entries in KNOWN_BOTS are set to type='Bot' regardless of excluded_users membership.
🤖 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 `@batch/db/mutations.ts`:
- Around line 252-260: The insert currently builds rows from uniqueLogins and
uses onConflict(...).doNothing(), so existing rows aren’t updated and only
logins present in the excluded_users migration path get type='Bot'; update the
logic to ensure every login in KNOWN_BOTS is persisted/updated with type='Bot'
by (a) including all KNOWN_BOTS in the set of inserted/processed logins and (b)
replacing onConflict(...).doNothing() with an upsert that merges/updates the
type field (e.g., onConflict(...).merge() or an UPDATE setting type='Bot') so
existing records for KNOWN_BOTS get type='Bot'; adjust the migration SQL (the
migrate_excluded_users_to_bot_type migration logic) similarly so all entries in
KNOWN_BOTS are set to type='Bot' regardless of excluded_users membership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0e38d3e-dc0f-41a3-a86c-507b068a711b
⛔ Files ignored due to path filters (1)
db/migrations/tenant/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
batch/db/mutations.tsdb/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrations/tenant/20260318120000_migrate_excluded_users_to_bot_type.sql
…lOrganizations - Fix duplicate step 3 comment in buildPullRequests (renumber 3-8 → 3-9) - Make getBotLogins the single source for bot login queries (remove from getTenantData) - getOrganization fetches botLogins in parallel with tenant data - listAllOrganizations no longer fetches unnecessary bot logins Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
batch/db/mutations.ts (1)
241-250: 既存ログイン衝突時に Bot 昇格できないため、判定の取りこぼしが残りますLine 249 の
doNothing()により、既存レコードがtype = nullの場合でも Line 244 の Bot 判定結果を反映できません。type未設定時のみBotに更新する条件付き upsert を検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/db/mutations.ts` around lines 241 - 250, The current insert uses onConflict(...).doNothing() so existing rows with type=null never get promoted to 'Bot' even when botUsers contains the login; replace the doNothing upsert with a conditional upsert that updates type to 'Bot' only when the existing row's type is null: change the onConflict handler (the builder that currently calls .doNothing()) to a doUpdate that sets the type column to 'Bot' when type IS NULL (or uses COALESCE/CASE comparing existing type vs excluded value), ensuring the uniqueLogins.map and botUsers check remain the source of truth for whether to attempt Bot promotion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@batch/github/pullrequest.ts`:
- Around line 227-233: The current loop only adds bot users from
rawArtifacts.reviews and rawArtifacts.discussions, missing cases where the PR
author is a bot; update the collection logic to also inspect rawArtifacts.author
(or the PR author object) and add it to botUsers when it indicates a bot (e.g.,
author.isBot or author.__typename === 'Bot' and author.username exists),
ensuring you call toLowerCase() like the others and handle a missing author
safely; modify the code around rawArtifacts.reviews/rawArtifacts.discussions and
botUsers to include this author check.
---
Nitpick comments:
In `@batch/db/mutations.ts`:
- Around line 241-250: The current insert uses onConflict(...).doNothing() so
existing rows with type=null never get promoted to 'Bot' even when botUsers
contains the login; replace the doNothing upsert with a conditional upsert that
updates type to 'Bot' only when the existing row's type is null: change the
onConflict handler (the builder that currently calls .doNothing()) to a doUpdate
that sets the type column to 'Bot' when type IS NULL (or uses COALESCE/CASE
comparing existing type vs excluded value), ensuring the uniqueLogins.map and
botUsers check remain the source of truth for whether to attempt Bot promotion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ff08635-988a-40c9-807d-0a849551d27c
📒 Files selected for processing (4)
app/services/jobs/analyze-worker.tsapp/services/jobs/shared-steps.server.tsbatch/db/mutations.tsbatch/github/pullrequest.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/jobs/analyze-worker.ts
- Add last_activity_at column to company_github_users with backfill migration - Update batch upsert to track latest PR/review activity per user - Default sort GitHub Users by last activity (desc), NULL last - Display last activity as relative time (fromNow) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… helper - NULL-last ordering only applies when sorting by lastActivityAt column - Extract trackLatest() helper to deduplicate Map.get/set pattern Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- getBotLogins returns lowercase-normalized logins for consistent matching - Add authorIsBot to ShapedGitHubPullRequest via GraphQL __typename - Collect PR author bots in buildPullRequests (e.g. dependabot PRs) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Change onConflict from doNothing to doUpdateSet with COALESCE — if an existing user has type=null and the API detects them as Bot, their type is now updated. Manually set types (User/Bot by admin) are preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/routes/$orgSlug/settings/github-users._index/+hooks/use-data-table-state.ts (1)
30-30: updateSort のフォールバック値との不整合デフォルトのソート順が
'desc'に変更されましたが、updateSort関数（Line 102）ではnewSort.sort_order || 'asc'としてフォールバックが'asc'のままです。これによりユーザーがカラムヘッダーをクリックした際に、デフォルトの期待値と異なる動作になる可能性があります。🔧 修正案
const updateSort = (newSort: Partial<Sort>) => { setSearchParams( (prev) => { if (newSort.sort_by) { prev.set('sort_by', newSort.sort_by) - prev.set('sort_order', newSort.sort_order || 'asc') + prev.set('sort_order', newSort.sort_order || 'desc') } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/github-users._index/+hooks/use-data-table-state.ts at line 30, The default sort order was changed to 'desc' but updateSort still falls back to 'asc', causing inconsistent behavior; update the fallback in the updateSort function (referenced as updateSort) to use 'desc' (i.e., use newSort.sort_order || 'desc') so clicks obey the declared default('desc') sorting behavior and keep sort handling consistent with the schema/default.batch/db/mutations.ts (1)
257-262:trackLatestの条件式を簡潔にできます
map.has(key)のチェック後はmap.get(key)が必ず値を返すため、?? ''は不要です。♻️ 提案する修正
function trackLatest(map: Map<string, string>, login: string, ts: string) { const key = login.toLowerCase() - if (!map.has(key) || ts > (map.get(key) ?? '')) { + const existing = map.get(key) + if (!existing || ts > existing) { map.set(key, ts) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/db/mutations.ts` around lines 257 - 262, The conditional in trackLatest currently uses "ts > (map.get(key) ?? '')" which is redundant because after checking map.has(key) the map.get(key) will always be defined; simplify the condition by removing the nullish coalescing and compare ts directly to map.get(key) (e.g., ts > map.get(key)) so the function reads clearly and avoids an unnecessary fallback. Ensure you only change the expression inside trackLatest and keep the key normalization and map.set behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/migrations/tenant/20260319120000_add_last_activity_at.sql`:
- Around line 5-13: The backfill compares LOWER(author) and LOWER(reviewer) to
company_github_users.login without normalizing the login side, so mixed-case
logins (e.g., "Dependabot") won't match; update the query to normalize both
sides (e.g., compare LOWER(author) = LOWER(company_github_users.login) and
LOWER(reviewer) = LOWER(company_github_users.login)) when computing
last_activity_at for company_github_users using the pull_requests and
pull_request_reviews subqueries so all case variants match correctly.
---
Nitpick comments:
In
`@app/routes/`$orgSlug/settings/github-users._index/+hooks/use-data-table-state.ts:
- Line 30: The default sort order was changed to 'desc' but updateSort still
falls back to 'asc', causing inconsistent behavior; update the fallback in the
updateSort function (referenced as updateSort) to use 'desc' (i.e., use
newSort.sort_order || 'desc') so clicks obey the declared default('desc')
sorting behavior and keep sort handling consistent with the schema/default.
In `@batch/db/mutations.ts`:
- Around line 257-262: The conditional in trackLatest currently uses "ts >
(map.get(key) ?? '')" which is redundant because after checking map.has(key) the
map.get(key) will always be defined; simplify the condition by removing the
nullish coalescing and compare ts directly to map.get(key) (e.g., ts >
map.get(key)) so the function reads clearly and avoids an unnecessary fallback.
Ensure you only change the expression inside trackLatest and keep the key
normalization and map.set behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4292d5f-692a-488a-b4f2-2a249533b6b0
⛔ Files ignored due to path filters (1)
db/migrations/tenant/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
app/routes/$orgSlug/settings/github-users._index/+components/github-users-columns.tsxapp/routes/$orgSlug/settings/github-users._index/+hooks/use-data-table-state.tsapp/routes/$orgSlug/settings/github-users._index/queries.server.tsapp/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsxapp/services/tenant-type.tsbatch/db/mutations.tsbatch/db/queries.tsbatch/github/__tests__/buildPullRequests-filter.test.tsbatch/github/__tests__/release-detect.test.tsbatch/github/fetcher.tsbatch/github/model.tsbatch/github/pullrequest.tsbatch/github/store.test.tsdb/migrations/tenant/20260319120000_add_last_activity_at.sqldb/tenant.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- app/services/tenant-type.ts
- app/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsx
- db/tenant.sql
- batch/github/tests/buildPullRequests-filter.test.ts
Apply LOWER() to company_github_users.login in the backfill query for defensive case-insensitive matching. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- updateSort fallback 'asc' → 'desc' to match SortSchema default - trackLatest: use map.get() result directly instead of redundant ?? '' Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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
organizationSettings.excludedUsers（カンマ区切りテキスト）を廃止し、companyGithubUsers.type='Bot'に統合filterActors()がDBのBot typeを直接参照するように変更__typename === 'Bot'情報を使って新規ユーザーを自動でBot設定lastActivityAtカラムを追加し、最終活動日の降順でデフォルトソート変更詳細
excludedUsers値をcompanyGithubUsers.type='Bot'に移行（lowercase）、excluded_usersカラムDROPlast_activity_atカラム追加、既存PR/reviewデータからbackfillDEFAULT_EXCLUDED_USERS定数とハードコードリストを削除、GitHub APIisBotフラグで自動判定BuildConfig.excludedUsers→botLogins: Set<string>（case-insensitive）upsertAnalyzedDataでユーザーごとの最終活動日を自動更新getBotLoginsをgetOrganizationで並行取得（listAllOrganizationsは対象外）lastActivityAt descに変更、fromNow()で相対表示Closes #146
Test plan
pnpm validateパス（lint, format, typecheck, build, test）pnpm db:setupパス（seed DBでマイグレーション適用確認）isBotなユーザーがtype='Bot'で自動登録されることを確認🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
New Features
Bug Fixes