refactor: pre-aggregate resolution rate in usage daily#39
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
Changes답변 집계 및 백필
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)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors how “resolution rate” is computed by pre-aggregating assistant answer counts (total_answers) and BAD feedback counts (bad_answers) into usage_daily, avoiding expensive scans of messages/message_feedbacks during dashboard reads and keeping writes/aggregations in the same transaction.
Changes:
- Add
total_answers/bad_answerscolumns + CHECK constraints tousage_daily, and update dashboard stats to read these aggregates only. - Increment
total_answerson assistant message insert; applybad_answersdeltas on feedback upsert, both transactionally with the source write. - Add an idempotent backfill implementation + CLI command to populate aggregates for existing data.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/usage/usage.service.ts | Adds aggregation helpers (increment/apply-delta) and switches stats reads to usage_daily totals. |
| src/usage/usage.service.spec.ts | Adds unit tests for delta computation and new aggregation write behaviors. |
| src/scripts/usage-backfill.ts | Implements pure aggregation + transactional, locked idempotent backfill. |
| src/scripts/usage-backfill.spec.ts | Tests bucket keying, aggregation logic, and backfill transactional/locking behavior. |
| src/scripts/backfill-usage-answers.ts | Adds CLI entrypoint to execute the backfill with env-based DB config. |
| src/db/usage-daily.schema.spec.ts | Verifies migration SQL adds columns/defaults and CHECK constraints. |
| src/db/schema.ts | Extends usageDaily schema with new columns and CHECK constraints. |
| src/chat/services/chat.service.ts | Wraps assistant message insert + total_answers increment in one tx; wraps feedback upsert + bad_answers delta in one tx with row locking. |
| src/chat/services/chat.service.spec.ts | Updates tests to cover new transactional aggregation calls and delta attribution. |
| package.json | Adds db:backfill:answers script for the new CLI. |
| drizzle/meta/0008_snapshot.json | Updates Drizzle snapshot for the new usage_daily columns and constraints. |
| drizzle/meta/_journal.json | Registers migration 0008 in the Drizzle journal. |
| drizzle/0008_add_usage_daily_answer_counts.sql | Adds total_answers/bad_answers columns and integrity constraints. |
| bun.lock | Moves patch-package to dependencies (lockfile update). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const lockTimeoutMs = Math.trunc( | ||
| options.lockTimeoutMs ?? DEFAULT_BACKFILL_LOCK_TIMEOUT_MS, | ||
| ); | ||
| const log = options.logger ?? ((message: string) => console.log(message)); |
| async function main(): Promise<void> { | ||
| const lockTimeoutMs = Number( | ||
| process.env.BACKFILL_LOCK_TIMEOUT_MS ?? DEFAULT_BACKFILL_LOCK_TIMEOUT_MS, | ||
| ); | ||
|
|
| * 피드백 rating 전이에 따른 bad_answers 변화량을 계산한다. | ||
| * - 없음/GOOD → BAD: +1 | ||
| * - BAD → 없음/GOOD: -1 | ||
| * - 그 외(상태 불변, GOOD↔없음): 0 | ||
| */ |
![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
🧹 Nitpick comments (1)
src/scripts/usage-backfill.ts (1)
129-162: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift대규모 데이터셋에서 메모리 부족(OOM) 위험 — 전체 assistant 메시지를 한 번에 로드.
select쿼리가 모든 assistant 메시지를 메모리로 로드합니다. 데이터가 수백만 건 이상이면 Node.js 프로세스가 OOM으로 종료될 수 있습니다. 커서 기반 배치 처리 또는LIMIT/OFFSET분할 처리를 권장합니다.♻️ 제안: 배치 처리로 메모리 사용량 제한
+ const BATCH_SIZE = 10_000; + let offset = 0; + let allBuckets = new Map<string, UsageAnswerBucket>(); + + while (true) { + const batch = await tx + .select({ + createdAt: messages.createdAt, + widgetKeyId: sessions.widgetKeyId, + pageUrl: sessions.pageUrl, + rating: messageFeedbacks.rating, + }) + .from(messages) + .innerJoin(sessions, eq(messages.sessionId, sessions.id)) + .leftJoin(messageFeedbacks, eq(messageFeedbacks.messageId, messages.id)) + .where(eq(messages.role, 'assistant')) + .limit(BATCH_SIZE) + .offset(offset); + + if (batch.length === 0) break; + + const batchBuckets = aggregateUsageAnswerRows(batch as UsageAnswerRow[]); + for (const bucket of batchBuckets) { + const key = makeUsageBucketKey(bucket.widgetKeyId, bucket.date, bucket.domain); + const existing = allBuckets.get(key); + if (existing) { + existing.totalAnswers += bucket.totalAnswers; + existing.badAnswers += bucket.badAnswers; + } else { + allBuckets.set(key, bucket); + } + } + offset += BATCH_SIZE; + } + + const buckets = Array.from(allBuckets.values());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/scripts/usage-backfill.ts` around lines 129 - 162, aggregateUsageAnswerRows에 전달할 assistant 메시지를 전체 조회하지 말고, 커서 기반 배치 또는 제한된 페이지 단위로 반복 조회하도록 변경하세요. 각 배치의 rows만 메모리에 유지하면서 aggregateUsageAnswerRows 결과를 누적·처리하고, 모든 배치가 완료된 뒤 usageDaily upsert를 수행하도록 관련 트랜잭션 흐름을 조정하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/usage/usage.service.spec.ts`:
- Around line 314-321: FakeSelect.then의 멀티라인 시그니처가 Prettier 형식과 일치하지 않으므로 해당 메서드
선언을 Prettier로 포맷하고, 변경 후 prettier/prettier 정적 분석 오류가 사라지는지 확인하세요.
In `@src/usage/usage.service.ts`:
- Around line 27-29: UsageDbExecutor의 멀티라인 유니온 타입 정의가 Prettier 형식과 일치하지 않습니다. 해당
타입 선언에 Prettier를 실행해 자동 포맷하고, 변경 후 lint를 다시 실행해 prettier/prettier 오류가 해결되는지
확인하세요.
- Around line 218-222: Update applyBadAnswerDelta to tolerate a missing
usage_daily aggregation row instead of throwing InternalServerErrorException,
allowing the feedback transaction to complete; alternatively ensure
db:backfill:answers runs as a required deployment step before enabling this
path.
---
Nitpick comments:
In `@src/scripts/usage-backfill.ts`:
- Around line 129-162: aggregateUsageAnswerRows에 전달할 assistant 메시지를 전체 조회하지 말고,
커서 기반 배치 또는 제한된 페이지 단위로 반복 조회하도록 변경하세요. 각 배치의 rows만 메모리에 유지하면서
aggregateUsageAnswerRows 결과를 누적·처리하고, 모든 배치가 완료된 뒤 usageDaily upsert를 수행하도록 관련
트랜잭션 흐름을 조정하세요.
🪄 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: 830e7783-e45c-45a8-a220-5434955d8187
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
drizzle/0008_add_usage_daily_answer_counts.sqldrizzle/meta/0008_snapshot.jsondrizzle/meta/_journal.jsonpackage.jsonsrc/chat/services/chat.service.spec.tssrc/chat/services/chat.service.tssrc/db/schema.tssrc/db/usage-daily.schema.spec.tssrc/scripts/backfill-usage-answers.tssrc/scripts/usage-backfill.spec.tssrc/scripts/usage-backfill.tssrc/usage/usage.service.spec.tssrc/usage/usage.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/usage/usage.service.spec.ts`:
- Around line 218-225: FakeAggSelect.then의 멀티라인 유니온 타입 선언이 Prettier 형식과 일치하지
않습니다. 해당 메서드의 타입 시그니처를 Prettier로 포맷하고, 변경 후 lint를 실행해 prettier/prettier 오류가 없는지
확인하세요.
🪄 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: 06c490f0-7c1c-48ec-a679-db9223539ef2
📒 Files selected for processing (5)
src/scripts/backfill-usage-answers.tssrc/scripts/usage-backfill.spec.tssrc/scripts/usage-backfill.tssrc/usage/usage.service.spec.tssrc/usage/usage.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/scripts/usage-backfill.ts
- src/scripts/usage-backfill.spec.ts
- src/scripts/backfill-usage-answers.ts
- validate lockTimeoutMs (finite, positive, >=1ms) before SQL in runBackfill - validate BACKFILL_LOCK_TIMEOUT_MS env before opening the DB connection - self-heal applyBadAnswerDelta: guarded arithmetic fast path, and reconcile the exact bucket from source-of-truth (with row lock) when the aggregation row is missing/not-yet-backfilled instead of throwing 500 - fix computeBadAnswerDelta docstring to match the GOOD|BAD next signature - add unit tests for the parser and reconcile paths
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/usage/usage.service.spec.ts`:
- Around line 218-225: FakeAggSelect.then의 멀티라인 유니온 타입 선언이 Prettier 규칙을 위반하고
있습니다. 해당 then 메서드의 onfulfilled 타입을 Prettier로 다시 포맷하거나 프로젝트의 Prettier 스타일에 맞게 정리한
뒤, prettier/prettier lint가 통과하는지 확인하세요.
🪄 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: f8d8e93b-b21a-4ac3-81e2-262a81371644
📒 Files selected for processing (5)
src/scripts/backfill-usage-answers.tssrc/scripts/usage-backfill.spec.tssrc/scripts/usage-backfill.tssrc/usage/usage.service.spec.tssrc/usage/usage.service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/scripts/backfill-usage-answers.ts
- src/scripts/usage-backfill.spec.ts
- src/scripts/usage-backfill.ts
- src/usage/usage.service.ts
| then<TResult1 = SourceRow[], TResult2 = never>( | ||
| onfulfilled?: | ||
| | ((value: SourceRow[]) => TResult1 | PromiseLike<TResult1>) | ||
| | null, | ||
| onrejected?: ((reason: unknown) => TResult2 | PromiseLike<TResult2>) | null, | ||
| ): PromiseLike<TResult1 | TResult2> { | ||
| return Promise.resolve(this.store.sourceRows).then(onfulfilled, onrejected); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prettier 포맷 오류가 여전히 해결되지 않았습니다.
이전 리뷰에서 지적된 FakeAggSelect.then의 멀티라인 유니온 타입(220-221) prettier/prettier 오류가 아직 미해결 상태입니다. 정적 분석이 동일한 오류를 재보고하고 있습니다. 포매터를 실행하거나 아래 diff를 적용해 lint 통과를 확보하세요.
🧹 Proposed fix
then<TResult1 = SourceRow[], TResult2 = never>(
onfulfilled?:
- | ((value: SourceRow[]) => TResult1 | PromiseLike<TResult1>)
- | null,
+ ((value: SourceRow[]) => TResult1 | PromiseLike<TResult1>) | null,
onrejected?: ((reason: unknown) => TResult2 | PromiseLike<TResult2>) | null,
): PromiseLike<TResult1 | TResult2> {🧰 Tools
🪛 ESLint
[error] 220-221: Replace |·((value:·SourceRow[])·=>·TResult1·|·PromiseLike<TResult1>)⏎····· with ((value:·SourceRow[])·=>·TResult1·|·PromiseLike<TResult1>)
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usage/usage.service.spec.ts` around lines 218 - 225, FakeAggSelect.then의
멀티라인 유니온 타입 선언이 Prettier 규칙을 위반하고 있습니다. 해당 then 메서드의 onfulfilled 타입을 Prettier로
다시 포맷하거나 프로젝트의 Prettier 스타일에 맞게 정리한 뒤, prettier/prettier lint가 통과하는지 확인하세요.
Source: Linters/SAST tools
Changes
usage_daily에total_answers,bad_answers컬럼 추가total_answers증가bad_answers반영resolutionRate를usage_daily집계값으로 계산Validation
Deployment note
Summary by CodeRabbit
usage_daily에 일일 총 답변/불량 답변 집계를 추가했습니다.db:backfill:answers)으로 기존 데이터를 일괄 계산할 수 있습니다.