feat: add excluded users setting and recalculate feature#100
Conversation
- Add excludedUsers column to organization_settings table - Exclude 'Copilot' by default (GitHub API records it without [bot] suffix in discussions) - Add UI in admin settings to configure additional excluded users per organization - Update batch processing to filter out excluded users from reviews/discussions This fixes incorrect pickup time calculations caused by Copilot comments being counted as human reviews. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add SSE API endpoint for recalculating cycle times with progress updates - Add Recalculate UI component with progress bar - Extend analyze function to support progress callback - Integrate recalculate feature into organization settings page This allows administrators to trigger cycle time recalculation from the admin UI after changing excluded users settings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an SSE-driven organization "recalculate" feature with UI and API, persists a new excludedUsers setting (DB migration, schema, types), propagates exclusions into batch GitHub analysis with per-repo progress, adds an auto-format hook, and adjusts path helpers, package deps, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Recalculate UI
participant SSE as /api/admin/recalculate
participant Provider as GitHub Provider
participant DB as Database
User->>UI: Click "Recalculate"
UI->>SSE: Open SSE connection (GET)
SSE->>SSE: Check/Add runningJobs lock
SSE->>DB: Load organization & settings
SSE->>Provider: analyze(orgSetting, onProgress)
Provider->>Provider: For each repo -> onProgress(repo, current, total)
Provider->>SSE: Return shaped PullRequests
SSE->>DB: Upsert PullRequests
SSE->>UI: stream "progress"/"upsert" events
alt export configured
SSE->>DB: Export pulls & reviews to sheets
SSE->>UI: stream "export" event
end
SSE->>UI: stream "complete"
SSE->>SSE: Release runningJobs lock & close
alt error
SSE->>UI: stream "error"
SSE->>SSE: Release runningJobs lock & close
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
batch/provider/github/model.ts (1)
51-51: Consider usingNonNullablefor type consistency.The user field definition differs from similar fields in this file. Lines 44 and 58 use
NonNullable<...>['login'] | nullpattern, which safely handles cases where the user object itself might be null in the API response.Apply this diff for consistency and type safety:
- user: GitHubReviewComment['user']['login'] | null + user: NonNullable<GitHubReviewComment['user']>['login'] | nullapp/routes/admin+/$organization.settings/types.ts (1)
11-11: Consider adding validation or format documentation for the user list.The
excludedUsersfield accepts any string without constraints on format. If this is expected to be a comma-separated list, space-separated list, or follow another pattern, consider:
- Adding Zod validation to enforce the expected format (e.g.,
.regex(/^(\w+,)*\w*$/)for comma-separated)- Documenting the expected format in a comment or the schema description
This will help prevent inconsistent data entry and improve maintainability.
app/routes/admin+/$organization.settings/forms/organization-settings.action.server.ts (1)
27-42: Consider transaction safety for dual updates.The code performs two separate updates (organization and organizationSettings) without transaction wrapping. If the first succeeds and the second fails, the data could be in an inconsistent state.
Consider:
- Wrapping both updates in a transaction if Kysely supports it
- Or, ordering them so the less critical update (name) happens second
- Providing more specific error messages that indicate which operation failed
Check if Kysely transactions are available:
#!/bin/bash # Search for transaction usage patterns in the codebase rg -nP 'transaction|\.trx\(|withTransaction' --type=ts -g 'app/services/*.ts' -g 'app/routes/**/*.server.ts'app/routes/api.admin.recalculate.$organization.ts (1)
70-73: Sequential upserts may be slow for large PR datasets.Each PR is upserted one at a time, which can be slow with many PRs. Consider batching or parallel execution with concurrency limits.
Example using Promise.all with chunking:
// Batch upsert in chunks const BATCH_SIZE = 50 for (let i = 0; i < pulls.length; i += BATCH_SIZE) { const batch = pulls.slice(i, i + BATCH_SIZE) await Promise.all(batch.map((pr) => upsertPullRequest(pr))) }batch/provider/github/provider.ts (1)
154-155: Consider usingpushinstead of spread for array accumulation.Using spread operator in a loop (
[...allPulls, ...pulls]) creates a new array on each iteration, resulting in O(n²) time complexity for large datasets.Apply this diff for better performance:
- allPulls = [...allPulls, ...pulls] - allReviewResponses = [...allReviewResponses, ...reviewResponses] + allPulls.push(...pulls) + allReviewResponses.push(...reviewResponses)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.claude/hooks/format-on-edit.ts(1 hunks)CLAUDE.md(5 hunks)app/routes/admin+/$organization.settings/forms/index.ts(1 hunks)app/routes/admin+/$organization.settings/forms/organization-settings.action.server.ts(2 hunks)app/routes/admin+/$organization.settings/forms/organization-settings.tsx(2 hunks)app/routes/admin+/$organization.settings/forms/recalculate.tsx(1 hunks)app/routes/admin+/$organization.settings/functions/mutations.server.ts(1 hunks)app/routes/admin+/$organization.settings/route.tsx(2 hunks)app/routes/admin+/$organization.settings/types.ts(1 hunks)app/routes/api.admin.recalculate.$organization.ts(1 hunks)app/services/type.ts(1 hunks)batch/db/queries.ts(2 hunks)batch/provider/github/model.ts(1 hunks)batch/provider/github/provider.ts(3 hunks)batch/provider/github/pullrequest.ts(2 hunks)package.json(2 hunks)prisma/migrations/20251216104437_add_excluded_users_to_organization_settings/migration.sql(1 hunks)prisma/migrations/migration_lock.toml(1 hunks)prisma/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
batch/provider/github/pullrequest.ts (2)
batch/provider/github/model.ts (1)
ShapedGitHubPullRequest(15-33)batch/provider/github/review-response.ts (1)
analyzeReviewResponse(5-30)
app/routes/api.admin.recalculate.$organization.ts (4)
app/routes/admin+/$organization.settings/route.tsx (1)
loader(27-43)batch/provider/index.ts (1)
createProvider(5-8)batch/db/mutations.ts (1)
upsertPullRequest(4-46)batch/bizlogic/export-spreadsheet.ts (2)
exportPullsToSpreadsheet(13-79)exportReviewResponsesToSpreadsheet(81-117)
app/routes/admin+/$organization.settings/functions/mutations.server.ts (2)
app/services/type.ts (3)
DB(150-164)Organization(65-72)OrganizationSetting(73-82)app/services/db.server.ts (3)
Updateable(18-18)db(23-27)sql(17-17)
batch/provider/github/provider.ts (2)
app/services/db.server.ts (1)
Selectable(18-18)app/services/type.ts (2)
DB(150-164)Repository(105-116)
app/routes/admin+/$organization.settings/forms/recalculate.tsx (5)
app/components/ui/card.tsx (6)
Card(68-68)CardHeader(68-68)CardTitle(68-68)CardDescription(68-68)CardContent(68-68)CardFooter(68-68)app/components/ui/stack.tsx (1)
Stack(38-46)app/components/ui/progress.tsx (1)
Progress(29-29)app/components/ui/alert.tsx (2)
Alert(66-66)AlertDescription(66-66)app/components/ui/button.tsx (1)
Button(58-58)
app/routes/admin+/$organization.settings/route.tsx (1)
app/routes/admin+/$organization.settings/forms/recalculate.tsx (1)
Recalculate(25-127)
app/routes/admin+/$organization.settings/forms/organization-settings.tsx (2)
app/components/ui/label.tsx (1)
Label(22-22)app/components/ui/input.tsx (1)
Input(21-21)
app/routes/admin+/$organization.settings/forms/organization-settings.action.server.ts (1)
app/routes/admin+/$organization.settings/functions/mutations.server.ts (2)
updateOrganization(10-19)updateOrganizationSetting(21-36)
🔇 Additional comments (19)
prisma/schema.prisma (1)
46-46: LGTM!The
excludedUsersfield is properly defined with an appropriate default value and database mapping. The inline comment clearly documents the comma-separated format.batch/db/queries.ts (1)
31-31: LGTM!The
excludedUsersfield has been correctly added to both organization queries, following the existing patterns for selecting organization settings.Also applies to: 103-103
app/services/type.ts (1)
79-79: LGTM!The
excludedUsersfield has been correctly generated by prisma-kysely to match the Prisma schema definition.app/routes/admin+/$organization.settings/forms/organization-settings.tsx (1)
52-52: LGTM!The excluded users field has been properly integrated into the form with clear labeling, helpful placeholder text, and proper error handling. The implementation follows the existing form patterns consistently.
Also applies to: 122-137
package.json (1)
136-136: tsx in devDependencies is correct for Prisma seed scripts.Seeding is primarily helpful when working in a development environment. The
tsxpackage is appropriately placed in devDependencies because seeding is only triggered explicitly by running npx prisma db seed—not automatically in production. When the --production flag is set or NODE_ENV is set to production, npm will not install modules listed in devDependencies. This placement follows the standard pattern shown in Prisma's official Next.js documentation.app/routes/admin+/$organization.settings/route.tsx (1)
11-11: LGTM! Clean integration of the Recalculate component.The import and rendering of the Recalculate component follows the existing pattern and is appropriately positioned between the ExportSettings and DeleteOrganization sections.
Also applies to: 85-85
app/routes/admin+/$organization.settings/forms/index.ts (1)
5-5: LGTM! Consistent barrel export pattern.The re-export of the recalculate module follows the existing pattern in the file.
CLAUDE.md (1)
52-58: LGTM! Helpful documentation improvements.The added Tech Stack details and improved formatting enhance the documentation's clarity and usefulness for AI-assisted development.
prisma/migrations/20251216104437_add_excluded_users_to_organization_settings/migration.sql (1)
1-20: LGTM! Correct SQLite migration pattern.The migration correctly uses SQLite's RedefineTables pattern to add the
excluded_userscolumn with appropriate defaults, foreign key constraints, and data preservation.app/routes/admin+/$organization.settings/functions/mutations.server.ts (1)
21-36: Consider validating that the organizationSettings row exists before attempting to update it.Kysely's UpdateResult returns numUpdatedRows indicating how many rows were updated, but the function doesn't validate this. If an organization lacks a corresponding organizationSettings row, the update executes successfully but affects zero rows. The caller awaits this function without checking the return value, so the failure would be silent.
Either ensure organizationSettings rows are created when organizations are created, or add a check in this function to verify at least one row was updated and throw if not.
batch/provider/github/pullrequest.ts (4)
21-22: LGTM! Sensible default exclusion list.Good choice to exclude
Copilotby default since it's a known AI assistant that shouldn't count toward pickup time calculations.
56-68: LGTM! Comprehensive filtering logic.The filter correctly excludes:
- Bot accounts (suffix
[bot])- PR author (self-reviews shouldn't count)
- Custom excluded users list
Using
r.user ?? ''handles the null case appropriately for theincludescheck.
74-74: Good null handling for author.The
res.author ?? ''fallback aligns with theanalyzeReviewResponsefunction which can return null authors based on theShapedGitHubReviewCommenttype.
30-39: The null/undefined handling suggestion is not necessary.The Prisma schema defines
excludedUsersas a non-nullable field with a default value of""(line 46 of prisma/schema.prisma). The generated type enforcesexcludedUsers: string, and the Zod validation schema also treats it as a required string with a default. Since the database guarantees a non-null string value, the call to.split(',')will not throw a TypeError, and the suggested nullish coalescing operator is redundant.Likely an incorrect or invalid review comment.
app/routes/api.admin.recalculate.$organization.ts (2)
89-98: Error handling and cleanup look correct.The
finallyblock properly removes the organizationId fromrunningJobsand closes the controller, ensuring cleanup happens regardless of success or failure.
102-108: LGTM! Correct SSE response headers.The headers are properly set for SSE:
text/event-stream,no-cache, andkeep-alive.batch/provider/github/provider.ts (3)
108-118: LGTM! Well-typed progress callback.The optional
onProgresscallback with typed progress object (repo,current,total) provides a clean interface for tracking analysis progress.
129-134: LGTM! Progress tracking implementation.Progress is correctly tracked and reported before processing each repository, giving accurate real-time feedback to the SSE consumer.
150-150: EnsureexcludedUsersis not null before passing tobuildPullRequests.If
organizationSetting.excludedUserscan benullfrom the database, this will cause issues inbuildPullRequestswhen calling.split(). Ensure consistency with the null handling suggested inpullrequest.ts.
| const eventSource = new EventSource( | ||
| `/api/admin/recalculate/${organization}`, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find app/routes -name "*recalculate*" -o -path "*api*admin*" | head -20Repository: coji/upflow
Length of output: 167
🏁 Script executed:
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -path "*/api/*" | grep -i recalcRepository: coji/upflow
Length of output: 37
🏁 Script executed:
rg "recalculate" --type ts --type tsx -l app/routes/apiRepository: coji/upflow
Length of output: 82
🏁 Script executed:
fd "recalculate" app/routesRepository: coji/upflow
Length of output: 167
🏁 Script executed:
rg "EventSource" app/routes --type tsx -A 5 -B 5Repository: coji/upflow
Length of output: 82
🏁 Script executed:
find app/routes -type f | xargs grep -l "recalculate\|EventSource" | grep -E "\.(ts|tsx)$"Repository: coji/upflow
Length of output: 223
🏁 Script executed:
find app -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "/api/admin/recalculate" 2>/dev/null | head -5Repository: coji/upflow
Length of output: 117
🏁 Script executed:
cat app/routes/api.admin.recalculate.$organization.tsRepository: coji/upflow
Length of output: 123
🏁 Script executed:
cat "app/routes/api.admin.recalculate.\$organization.ts"Repository: coji/upflow
Length of output: 3287
🏁 Script executed:
rg "getSession\|requireAdmin\|requireUser" app/routes/api --type ts -A 3 | head -50Repository: coji/upflow
Length of output: 109
🏁 Script executed:
rg "getSession|requireAdmin|requireUser" app/routes --type ts -A 2 | head -80Repository: coji/upflow
Length of output: 1853
🏁 Script executed:
cat app/libs/auth.server.ts | head -100Repository: coji/upflow
Length of output: 2642
🏁 Script executed:
rg "export.*function|export const" app/libs/auth.server.ts | head -30Repository: coji/upflow
Length of output: 263
🏁 Script executed:
rg "export const requireUser|export const requireSuperAdmin" -A 15 app/libs/auth.server.tsRepository: coji/upflow
Length of output: 488
🏁 Script executed:
rg "export const loader.*LoaderArgs" app/routes/api.admin* -A 10 | head -60Repository: coji/upflow
Length of output: 456
Add authentication and authorization checks to the SSE endpoint.
The /api/admin/recalculate/{organization} endpoint currently has no authentication or permission validation. Import requireUser from ~/app/libs/auth.server and verify the authenticated user has admin access to the specified organization before processing the recalculation request. Currently, any unauthenticated user can trigger expensive operations by accessing this endpoint with any organization ID.
- Add requireSuperAdmin auth check to recalculate API endpoint - Simplify redundant ternary in Alert variant - Add error logging to format hook catch block 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove UPFLOW_DATA_DIR from cli.ts (no longer needed) - Add getDataDir() helper that defaults to ./data - Consolidate path resolution logic in path-builder.ts - Simplify tests to focus on default behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/routes/api.admin.recalculate.$organization.ts (1)
22-40: Race condition in concurrency check still exists.The concurrency check at line 22 and the
runningJobs.add()at line 40 are not atomic. Between checking and adding inside the stream'sstart(), another request could pass the check.Move
runningJobs.add(organizationId)immediately after the check passes (before creating the stream) to make the guard atomic within this process.🔎 Proposed fix
// 同時実行チェック if (runningJobs.has(organizationId)) { return new Response( JSON.stringify({ error: 'Recalculation already in progress' }), { status: 409, headers: { 'Content-Type': 'application/json' }, }, ) } + runningJobs.add(organizationId) // SSE ストリームを作成 const stream = new ReadableStream({ async start(controller) { const encoder = new TextEncoder() const send = (data: Record<string, unknown>) => { controller.enqueue(encoder.encode(`data: ${JSON.stringify(data)}\n\n`)) } - runningJobs.add(organizationId) try {app/routes/admin+/$organization.settings/forms/recalculate.tsx (2)
31-79: Missing EventSource cleanup and JSON parse error handling.Two previously flagged issues remain:
- Memory leak: The
EventSourceis not tracked in a ref and won't be closed if the component unmounts during the operation.- Uncaught exception:
JSON.parse(event.data)at line 43 can throw on malformed data, crashing the handler.🔎 Proposed fix
export const Recalculate = () => { const { organization } = useParams() const [status, setStatus] = useState<Status>('idle') const [progress, setProgress] = useState<ProgressData | null>(null) const [message, setMessage] = useState<string>('') + const eventSourceRef = useRef<EventSource | null>(null) + + useEffect(() => { + return () => { + eventSourceRef.current?.close() + } + }, []) const handleRecalculate = useCallback(() => { if (!organization) return + // Close any existing connection + eventSourceRef.current?.close() + setStatus('running') setProgress(null) setMessage('Starting...') const eventSource = new EventSource( `/api/admin/recalculate/${organization}`, ) + eventSourceRef.current = eventSource eventSource.onmessage = (event) => { - const data = JSON.parse(event.data) + let data + try { + data = JSON.parse(event.data) + } catch { + setStatus('error') + setMessage('Invalid server response') + eventSource.close() + return + } switch (data.type) { // ... cases unchanged } } eventSource.onerror = () => { setStatus('error') setMessage('Connection error occurred') eventSource.close() + eventSourceRef.current = null } }, [organization])
81-83: Guard against division by zero.If
progress.totalis0, the calculation producesNaN, which may cause rendering issues.🔎 Proposed fix
const progressPercent = progress - ? Math.round((progress.current / progress.total) * 100) + ? progress.total > 0 + ? Math.round((progress.current / progress.total) * 100) + : 0 : 0
🧹 Nitpick comments (1)
batch/helper/path-builder.test.ts (1)
4-8: Add test coverage for environment variable cases.The current test only validates the default behavior. Consider adding test cases for:
- When
UPFLOW_DATA_DIRis set to a valid path- When
UPFLOW_DATA_DIRis set to the string 'undefined' (to verify Line 9 in path-builder.ts)🔎 Suggested additional test cases
+import { beforeEach, afterEach } from 'vitest' + describe('getDataDir', () => { + const originalEnv = process.env.UPFLOW_DATA_DIR + + afterEach(() => { + if (originalEnv === undefined) { + delete process.env.UPFLOW_DATA_DIR + } else { + process.env.UPFLOW_DATA_DIR = originalEnv + } + }) + test('returns default ./data', () => { + delete process.env.UPFLOW_DATA_DIR expect(getDataDir()).toBe('./data') }) + + test('returns UPFLOW_DATA_DIR when set', () => { + process.env.UPFLOW_DATA_DIR = '/custom/data/path' + expect(getDataDir()).toBe('/custom/data/path') + }) + + test('returns default when UPFLOW_DATA_DIR is string "undefined"', () => { + process.env.UPFLOW_DATA_DIR = 'undefined' + expect(getDataDir()).toBe('./data') + }) })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.claude/hooks/format-on-edit.tsapp/routes/admin+/$organization.settings/forms/recalculate.tsxapp/routes/api.admin.recalculate.$organization.tsbatch/cli.tsbatch/helper/path-builder.test.tsbatch/helper/path-builder.tsbatch/helper/path.ts
💤 Files with no reviewable changes (1)
- batch/cli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/hooks/format-on-edit.ts
🧰 Additional context used
🧬 Code graph analysis (4)
batch/helper/path-builder.ts (1)
batch/helper/index.ts (1)
path(3-3)
batch/helper/path.ts (1)
batch/helper/path-builder.ts (1)
getDataDir(7-14)
app/routes/api.admin.recalculate.$organization.ts (4)
app/libs/auth.server.ts (1)
requireSuperAdmin(151-160)batch/provider/index.ts (1)
createProvider(5-8)batch/db/mutations.ts (1)
upsertPullRequest(4-46)batch/bizlogic/export-spreadsheet.ts (2)
exportPullsToSpreadsheet(13-79)exportReviewResponsesToSpreadsheet(81-117)
batch/helper/path-builder.test.ts (1)
batch/helper/path-builder.ts (2)
getDataDir(7-14)createPathBuilder(20-53)
🔇 Additional comments (8)
app/routes/api.admin.recalculate.$organization.ts (4)
1-12: LGTM on imports and module setup.The imports are appropriate, and
requireSuperAdmincorrectly addresses the previously flagged authentication concern. The in-memoryrunningJobsSet is acceptable for single-instance deployments.
42-70: LGTM on organization loading and analysis.The use of
invariantfor runtime validation is appropriate. The progress callback correctly streams per-repository updates to the client.
72-92: LGTM on data persistence and export.Sequential upsert is a safe choice for reliability. The conditional spreadsheet export and completion message are handled correctly.
93-113: LGTM on error handling and response.The
finallyblock ensures proper cleanup ofrunningJobsand stream closure regardless of success or failure. SSE headers are correctly configured.app/routes/admin+/$organization.settings/forms/recalculate.tsx (1)
85-118: LGTM on component rendering.The UI structure is clean with appropriate conditional rendering for progress and alerts. The button correctly disables during processing to prevent duplicate triggers.
batch/helper/path-builder.ts (1)
26-26: LGTM! Good refactoring to centralize data directory logic.The update to use
getDataDir()successfully centralizes the data directory resolution logic, making it easier to maintain.batch/helper/path.ts (1)
3-6: LGTM! Successful consolidation of data directory logic.The refactoring correctly replaces the inlined data directory logic with the centralized
getDataDir()helper, improving maintainability.batch/helper/path-builder.test.ts (1)
10-20: LGTM! Path-builder tests correctly validate the refactored behavior.The test properly validates that paths are constructed using the new
getDataDir()base. The expected path 'data/json/...' is correct sincepath.joinnormalizes away the leading './' from './data'.
| export const getDataDir = () => { | ||
| const upflowDataDir = process.env.UPFLOW_DATA_DIR | ||
| if (upflowDataDir && upflowDataDir !== 'undefined') { | ||
| return upflowDataDir | ||
| } | ||
| // デフォルト: プロジェクトルートの data/ | ||
| return './data' | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for scripts that might start the batch process
echo "=== Searching for batch startup patterns ==="
rg -n "batch|tsx|node" --type sh --type js --type json | grep -i "batch\|start\|run" | head -20
echo -e "\n=== Checking for package.json scripts ==="
fd -e json -e yml -e yaml | xargs grep -l "batch" 2>/dev/null | head -5
echo -e "\n=== Searching for documentation about working directory ==="
fd -e md | xargs rg -n "working directory|current working|cwd|project root" 2>/dev/null | head -20
echo -e "\n=== Looking for UPFLOW_DATA_DIR usage in codebase ==="
rg -n "UPFLOW_DATA_DIR" --type ts --type js
echo -e "\n=== Checking how getDataDir is used ==="
rg -n "getDataDir" --type ts --type js -A2Repository: coji/upflow
Length of output: 1908
🏁 Script executed:
#!/bin/bash
# Check if 'undefined' string checks are common in this codebase
echo "=== Checking for 'undefined' string checks ==="
rg -n "!== ['\"]undefined['\"]|== ['\"]undefined['\"]" --type ts --type js | head -10
echo -e "\n=== Examining the path-builder.ts file in full context ==="
cat -n batch/helper/path-builder.ts 2>/dev/null || echo "File not found at that path, searching..."
fd "path-builder" --type tsRepository: coji/upflow
Length of output: 2449
🏁 Script executed:
#!/bin/bash
# Search for Docker and deployment configs
echo "=== Looking for Docker and deployment configs ==="
fd -e Dockerfile -e dockerfile -e docker-compose.yml -e .env -e .env.example | head -10
echo -e "\n=== Checking Docker files for working directory ==="
fd -e Dockerfile | xargs cat 2>/dev/null | grep -A5 -B5 "WORKDIR\|RUN.*node\|ENTRYPOINT"
echo -e "\n=== Checking README or docs ==="
fd "README|readme" -e md | xargs head -50 2>/dev/null | head -100
echo -e "\n=== Looking for env variable patterns and comments ==="
rg -n "process.env" batch/helper/path-builder.ts -B2 -A2Repository: coji/upflow
Length of output: 878
Document the working directory assumption and the 'undefined' environment variable check.
The relative path './data' is intentional per the code comment (the test explicitly validates this behavior), but the dependency on process working directory should be documented. Additionally, the defensive check upflowDataDir !== 'undefined' (checking for the literal string "undefined") is not explained in comments—clarify why this pattern is needed, as it suggests handling for misconfigured environments that set the variable to the string 'undefined' rather than leaving it unset.
🤖 Prompt for AI Agents
In batch/helper/path-builder.ts around lines 7 to 14, add an inline comment
above the default return documenting that './data' is a relative path resolved
from the process working directory (i.e., process.cwd()) and that tests rely on
this behavior; also add a short comment explaining the defensive check
upflowDataDir !== 'undefined' (it handles environments that mistakenly set the
variable to the literal string "undefined" instead of leaving it unset) and
optionally suggest normalizing the env var (e.g., trim and treat empty or
'undefined' strings as unset) so future maintainers understand the intent and
can safely change behavior.
- Fix command injection vulnerability in format hook (use execFileSync) - Add EventSource cleanup on component unmount - Add JSON.parse error handling for SSE messages - Fix division by zero in progress calculation - Fix race condition in concurrent execution check 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/routes/admin+/$organization.settings/forms/recalculate.tsx (1)
54-56: Consider URL-encoding the organization parameter.If
organizationcould contain special characters (spaces, ampersands, etc.), the URL could break or be misinterpreted. While route parameters are typically URL-safe slugs, encoding defensively is a good practice.🔎 Proposed fix
const eventSource = new EventSource( - `/api/admin/recalculate/${organization}`, + `/api/admin/recalculate/${encodeURIComponent(organization)}`, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.claude/hooks/format-on-edit.tsapp/routes/admin+/$organization.settings/forms/recalculate.tsxapp/routes/api.admin.recalculate.$organization.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/routes/api.admin.recalculate.$organization.ts
- .claude/hooks/format-on-edit.ts
🧰 Additional context used
🧬 Code graph analysis (1)
app/routes/admin+/$organization.settings/forms/recalculate.tsx (1)
app/components/ui/stack.tsx (1)
Stack(38-46)
🔇 Additional comments (5)
app/routes/admin+/$organization.settings/forms/recalculate.tsx (5)
1-23: LGTM! Imports and type definitions are well-structured.The imports now include
useRefanduseEffectas required for proper EventSource lifecycle management. TheStatustype andProgressDatainterface provide clear type safety for the component state.
32-40: LGTM! EventSource cleanup on unmount is properly implemented.The cleanup effect correctly closes the EventSource connection and nulls the ref when the component unmounts, preventing memory leaks and orphaned connections.
67-76: LGTM! JSON parsing error handling is properly implemented.The try-catch block around
JSON.parseprevents crashes from malformed server responses, with appropriate error state updates, connection cleanup, and ref nulling.
117-120: LGTM! Division-by-zero protection is in place.The progress percentage calculation safely guards against
NaNby checkingprogress.total > 0before performing the division.
122-155: LGTM! UI rendering logic is well-structured.The component renders appropriate UI states:
- Progress bar with repository count shown during processing
- Alert with contextual variant (
destructivefor errors)- Button disabled while running to prevent duplicate submissions
1 participant
Summary
Background
GitHub API returns
Copilotfor discussion comments (without[bot]suffix), causing AI comments to be counted as human reviews in pickup time calculations. This fix:Copilotiris-tech-bot)Changes
Excluded Users Feature
excludedUserscolumn toorganization_settingstablebuildPullRequeststo filter out excluded users from cycle time calculationsRecalculate Feature
/api/admin/recalculate/{organizationId}) for real-time progressrequireSuperAdminauthentication checkData Directory Resolution
UPFLOW_DATA_DIRenvironment variable requirement./datapath (consistent with project structure)UPFLOW_DATA_DIRstill supported for custom deploymentsDeveloper Experience
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.