feat(notification): add durable PostgreSQL reminder inbox - #104
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughPostgreSQL 기반 알림 영속성, 원자적 클레임, 멱등 인앱 전달, 런타임 풀 구성을 추가했습니다. 스케줄러는 claim key로 상태 변경 소유권을 검증합니다. 통합 테스트, 운영 문서, CI 검증 절차도 추가했습니다. Changes내구성 알림 영속성
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReminderScheduler
participant PostgresReminderRepository
participant PostgresInAppDeliveryGateway
participant PostgreSQL
ReminderScheduler->>PostgresReminderRepository: due occurrence claim
PostgresReminderRepository->>PostgreSQL: conditional lease update
PostgresReminderRepository-->>ReminderScheduler: claim key
ReminderScheduler->>PostgresInAppDeliveryGateway: deliver reminder
PostgresInAppDeliveryGateway->>PostgreSQL: idempotent inbox insert
ReminderScheduler->>PostgresReminderRepository: markDelivered with claim key
PostgresReminderRepository->>PostgreSQL: append outcome and update state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/notification-service/src/reminder-scheduler.ts (1)
435-459: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
countDelivered실패는 여전히 배치 전체를 중단시킵니다.
defer,fail,markDelivered호출은 이제 레코드 단위로 오류를 격리하고persistenceFailures로 집계합니다. 그러나 435행의countDelivered는 try/catch 밖에 있습니다. 이 호출이 실패하면run이 거부되고, 남은 배치 레코드는 처리되지 않으며 이미 계산한 집계값도 반환되지 않습니다. 이 레코드의 claim은 이미 획득된 상태이므로 회복이 lease 만료까지 지연됩니다.같은 저장소 오류 격리를
countDelivered에도 적용하십시오.🔒️ `countDelivered` 오류 격리
- const deliveredToday = await this.repository.countDelivered( - reminder.workspaceId, - clock.localDate, - ); + let deliveredToday: number; + try { + deliveredToday = await this.repository.countDelivered( + reminder.workspaceId, + clock.localDate, + ); + } catch { + persistenceFailures += 1; + continue; + }🤖 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 `@apps/notification-service/src/reminder-scheduler.ts` around lines 435 - 459, Wrap the countDelivered call in the per-record processing flow with the same error isolation used for defer, fail, and markDelivered. When countDelivered fails, increment persistenceFailures and handle the record through the existing recovery path so run continues processing remaining records and returns accumulated results; do not let the repository error reject the batch.
🧹 Nitpick comments (9)
apps/notification-service/src/reminder-scheduler.ts (1)
409-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value표현식 안에 삽입된 문서 주석을 제거하십시오.
411행과 490행의 주석은 함수 호출 인자 위치에 있으며 새로운 정보를 제공하지 않습니다.
isWithinQuietHours와retryInstant의 동작은 각 정의 위에 이미 문서화되어 있습니다. 이 주석들을 제거하면 조건식과 인자 목록이 더 읽기 쉬워집니다. 나머지 오류 격리 구조는 정확합니다.Also applies to: 461-524
🤖 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 `@apps/notification-service/src/reminder-scheduler.ts` around lines 409 - 433, Remove the inline documentation comments embedded in the expressions around isWithinQuietHours and retryInstant, including the corresponding comment in the retry block covered by the additional range. Leave the surrounding conditions, function arguments, and error-isolation behavior unchanged.apps/notification-service/src/postgres-reminder-repository.coverage.test.ts (2)
602-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value스케줄러 시나리오가 저장소 커버리지 파일에 있습니다.
이 케이스는
ReminderScheduler의 daily-limit 지연 경로만 검증하며,PostgresReminderRepository를 사용하지 않습니다.reminder-scheduler.test.ts또는reminder-scheduler.integration.test.ts로 옮기면 파일 경계가 명확해집니다. 이동은 선택 사항입니다.🤖 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 `@apps/notification-service/src/postgres-reminder-repository.coverage.test.ts` around lines 602 - 634, Move the daily-limit deferral persistence failure test around ReminderScheduler.run from the Postgres repository coverage test into the scheduler unit or integration test file, such as reminder-scheduler.test.ts or reminder-scheduler.integration.test.ts. Preserve its existing repository stubs, gateway behavior, and expected scheduler result unchanged.
66-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value응답이 소진되면 테스트 클라이언트가 조용히 빈 결과를 반환합니다.
responses[index] ?? []는 준비된 응답보다 많은 쿼리가 실행될 때 빈 rows를 반환합니다. 이 동작은 예상보다 많은 SQL 호출을 감추고, 실패 원인을 카디널리티 오류로 바꿉니다. 준비되지 않은 호출에서 명시적으로 실패하도록 만드는 편이 진단에 유리합니다.♻️ 준비되지 않은 호출을 명시적으로 실패시키는 방법
calls.push({ text, values }); - const response = responses[index] ?? []; + const response = responses[index]; + if (response === undefined) { + throw new Error( + `unexpected query #${index + 1}: no response was prepared`, + ); + } index += 1;이 변경은
sequencedSqlClient([])로 SQL 호출을 전혀 기대하지 않는 케이스에서도 의도를 명확히 합니다.🤖 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 `@apps/notification-service/src/postgres-reminder-repository.coverage.test.ts` around lines 66 - 87, Update sequencedSqlClient so a query made after all configured responses are consumed explicitly throws an error instead of defaulting to an empty result. Preserve the existing ordered response and Error-throwing behavior, while keeping sequencedSqlClient([]) valid when no queries are executed.apps/notification-service/src/reminder-scheduler.integration.test.ts (1)
72-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value테스트 더블의 claim 토큰이 결정적입니다.
claim은${occurrenceKey}:claim을 반환합니다. 같은 occurrence를 다시 claim하면 동일한 토큰이 나옵니다. 프로덕션 저장소는 claim 시도마다 고유한 불투명 토큰을 생성하므로, 이 더블로는 "만료된 claim이 두 번째 claim 이후 전이할 수 없다"는 성질을 재현할 수 없습니다. 시도마다 증가하는 카운터를 토큰에 포함하면 더블이 프로덕션 계약에 더 가까워집니다. 실제 펜싱 증거는 PostgreSQL 통합 테스트에서 확보하십시오.🤖 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 `@apps/notification-service/src/reminder-scheduler.integration.test.ts` around lines 72 - 84, Update the test double’s claim implementation in claim to generate a unique opaque token for each claim attempt, using an incrementing counter rather than the fixed `${occurrenceKey}:claim` value. Preserve the existing duplicate-occurrence rejection while ensuring successive attempts produce distinct tokens, and cover fencing behavior with the PostgreSQL integration tests.apps/notification-service/src/docstring-coverage.test.ts (1)
122-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win게이트 범위를 좁히십시오.
discoverSourceFiles는src아래의 모든.ts파일을 포함합니다. 테스트 파일과 표현식 내부 선언도 JSDoc을 요구합니다. 그 결과 프로덕션 코드에 정보가 없는 주석이 삽입되었습니다. 예:apps/notification-service/src/notification-runtime.ts154행은 함수 인수 앞에 설명 주석을 넣습니다. 같은 형태가 통합 테스트 전반에 반복됩니다.
*.test.ts를 제외하고 최상위 선언과 클래스 멤버로 대상을 한정하십시오. 그러면 문서 계약은 유지되고 잡음 주석은 제거됩니다.♻️ 제안 변경
if (entry.isDirectory()) { return await discoverSourceFiles(path); } - return entry.isFile() && entry.name.endsWith('.ts') ? [path] : []; + return entry.isFile() && + entry.name.endsWith('.ts') && + !entry.name.endsWith('.test.ts') + ? [path] + : [];🤖 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 `@apps/notification-service/src/docstring-coverage.test.ts` around lines 122 - 134, Update discoverSourceFiles to exclude *.test.ts files and narrow docstring coverage to top-level declarations and class members only, preventing nested expression declarations and test code from being treated as documentation targets while preserving coverage for production API declarations.apps/notification-service/src/notification-runtime.ts (2)
43-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win오류 분류 정보를 추가로 기록하십시오.
현재 리스너는
error인자를 폐기합니다. 로그에는 고정 문자열만 남습니다. 운영자는 장애 원인(연결 종료, 인증 실패, 네트워크 단절)을 구분할 수 없습니다.
error.message는 호스트나 URL을 포함할 수 있으므로 기록하지 마십시오.error.name과pg오류 코드만 추가하면 자격 증명 없이 분류가 가능합니다.♻️ 제안 변경
/** Credential-free error logger used by the pool error boundary. */ export type NotificationPoolErrorLogger = ( message: string, context: string, + classification: string, ) => void; @@ export function registerNotificationPoolErrorHandler( pool: NotificationPoolErrorSource, logError: NotificationPoolErrorLogger = defaultNotificationPoolErrorLogger, ): void { - pool.on('error', () => { + pool.on('error', (error: Error) => { + const code = (error as { code?: unknown }).code; /** Performs the log error operation while preserving tenant-safe bounded behavior. */ - logError(NOTIFICATION_POOL_ERROR_MESSAGE, 'NotificationRuntime'); + logError( + NOTIFICATION_POOL_ERROR_MESSAGE, + 'NotificationRuntime', + typeof code === 'string' ? code : error.name, + ); }); }🤖 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 `@apps/notification-service/src/notification-runtime.ts` around lines 43 - 52, Update registerNotificationPoolErrorHandler to accept the emitted error, while preserving the existing sanitized message and logger context. Add only the error.name and PostgreSQL error code to the log metadata; do not record error.message or other host, URL, or credential-bearing details.
192-209: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win종료 프로미스를 보관하십시오.
close()와onApplicationShutdown()이 동시에 실행될 수 있습니다. 현재 구현은closed를 먼저 true로 설정합니다. 두 번째 호출자는 첫 번째pool.end()완료를 기다리지 않고 즉시 반환합니다. 그 결과 연결이 아직 열린 상태에서 종료 절차가 끝났다고 보고합니다.추가로
pool.end()가 거부되면 플래그가 이미 true이므로 재시도 경로가 없습니다.프로미스를 저장하면 두 경우 모두 해결됩니다.
♻️ 제안 변경
- private closed = false; + private closing: Promise<void> | undefined; @@ /** Closes the owned PostgreSQL pool exactly once. */ async close(): Promise<void> { - if (this.closed) { - return; - } - this.closed = true; - await this.pool.end(); + this.closing ??= this.pool.end(); + await this.closing; }🤖 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 `@apps/notification-service/src/notification-runtime.ts` around lines 192 - 209, Update the close() lifecycle in the class containing the closed flag so it stores the in-flight pool.end() promise and reuses it for concurrent callers, ensuring every caller waits for shutdown completion. Preserve the idempotent behavior, but clear or reset the stored state when pool.end() rejects so a later close attempt can retry.apps/notification-service/src/notification-runtime.test.ts (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win길이 초과와 값 누락을 구분하십시오.
이 단정은 8KiB URL에 대해 "missing" 메시지를 고정합니다. 실제 원인은 값의 길이 초과입니다. 운영자는 변수가 설정되지 않았다고 오해합니다.
requireConfiguration에서 길이 초과 전용 메시지를 반환하고, 이 테스트를 그 메시지로 갱신하십시오.🤖 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 `@apps/notification-service/src/notification-runtime.test.ts` around lines 104 - 111, Update requireConfiguration to distinguish an overlength NOTIFICATION_DATABASE_URL from a missing value by returning the length-specific error message. Revise the createNotificationPoolConfiguration test for the 8 KiB URL to assert that overlength message instead of “missing.”.github/workflows/ci.yml (1)
35-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPostgreSQL 시작 블록이 중복됩니다.
35-50행과 152-167행의 스크립트가 동일합니다. 한쪽만 수정하면 두 작업의 동작이 갈라집니다. 재사용 가능한 복합 액션이나 저장소 스크립트로 추출하고 두 작업에서 호출하십시오.
🤖 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 @.github/workflows/ci.yml around lines 35 - 50, Extract the duplicated PostgreSQL startup script into a reusable repository script or composite action, then replace both the “Start runner PostgreSQL” blocks with calls to it. Preserve the existing service startup, password configuration, database creation check, and readiness validation in the shared implementation so both jobs remain consistent.
🤖 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 @.github/workflows/ci.yml:
- Around line 17-33: Remove the temporary review_repair job from
.github/workflows/ci.yml lines 17-33, including its hardcoded branch checkout
and write permissions. Also remove needs: review_repair and the always()
condition from .github/workflows/ci.yml lines 132-137 so validate runs normally;
no direct changes are needed beyond updating that dependency and condition.
In `@apps/notification-service/src/reminder-scheduler.ts`:
- Around line 335-338: Remove the duplicate documentation comment immediately
above idempotencyKey, leaving only one of the two equivalent descriptions while
preserving the function and its implementation unchanged.
---
Outside diff comments:
In `@apps/notification-service/src/reminder-scheduler.ts`:
- Around line 435-459: Wrap the countDelivered call in the per-record processing
flow with the same error isolation used for defer, fail, and markDelivered. When
countDelivered fails, increment persistenceFailures and handle the record
through the existing recovery path so run continues processing remaining records
and returns accumulated results; do not let the repository error reject the
batch.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 35-50: Extract the duplicated PostgreSQL startup script into a
reusable repository script or composite action, then replace both the “Start
runner PostgreSQL” blocks with calls to it. Preserve the existing service
startup, password configuration, database creation check, and readiness
validation in the shared implementation so both jobs remain consistent.
In `@apps/notification-service/src/docstring-coverage.test.ts`:
- Around line 122-134: Update discoverSourceFiles to exclude *.test.ts files and
narrow docstring coverage to top-level declarations and class members only,
preventing nested expression declarations and test code from being treated as
documentation targets while preserving coverage for production API declarations.
In `@apps/notification-service/src/notification-runtime.test.ts`:
- Around line 104-111: Update requireConfiguration to distinguish an overlength
NOTIFICATION_DATABASE_URL from a missing value by returning the length-specific
error message. Revise the createNotificationPoolConfiguration test for the 8 KiB
URL to assert that overlength message instead of “missing.”
In `@apps/notification-service/src/notification-runtime.ts`:
- Around line 43-52: Update registerNotificationPoolErrorHandler to accept the
emitted error, while preserving the existing sanitized message and logger
context. Add only the error.name and PostgreSQL error code to the log metadata;
do not record error.message or other host, URL, or credential-bearing details.
- Around line 192-209: Update the close() lifecycle in the class containing the
closed flag so it stores the in-flight pool.end() promise and reuses it for
concurrent callers, ensuring every caller waits for shutdown completion.
Preserve the idempotent behavior, but clear or reset the stored state when
pool.end() rejects so a later close attempt can retry.
In `@apps/notification-service/src/postgres-reminder-repository.coverage.test.ts`:
- Around line 602-634: Move the daily-limit deferral persistence failure test
around ReminderScheduler.run from the Postgres repository coverage test into the
scheduler unit or integration test file, such as reminder-scheduler.test.ts or
reminder-scheduler.integration.test.ts. Preserve its existing repository stubs,
gateway behavior, and expected scheduler result unchanged.
- Around line 66-87: Update sequencedSqlClient so a query made after all
configured responses are consumed explicitly throws an error instead of
defaulting to an empty result. Preserve the existing ordered response and
Error-throwing behavior, while keeping sequencedSqlClient([]) valid when no
queries are executed.
In `@apps/notification-service/src/reminder-scheduler.integration.test.ts`:
- Around line 72-84: Update the test double’s claim implementation in claim to
generate a unique opaque token for each claim attempt, using an incrementing
counter rather than the fixed `${occurrenceKey}:claim` value. Preserve the
existing duplicate-occurrence rejection while ensuring successive attempts
produce distinct tokens, and cover fencing behavior with the PostgreSQL
integration tests.
In `@apps/notification-service/src/reminder-scheduler.ts`:
- Around line 409-433: Remove the inline documentation comments embedded in the
expressions around isWithinQuietHours and retryInstant, including the
corresponding comment in the retry block covered by the additional range. Leave
the surrounding conditions, function arguments, and error-isolation behavior
unchanged.
🪄 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 Plus
Run ID: d9ffeaf5-e87d-47d8-abe3-87e5a827bfea
📒 Files selected for processing (18)
.github/workflows/ci.ymlapps/notification-service/package.jsonapps/notification-service/src/docstring-coverage.test.tsapps/notification-service/src/main.test.tsapps/notification-service/src/main.tsapps/notification-service/src/notification-runtime.integration.test.tsapps/notification-service/src/notification-runtime.test.tsapps/notification-service/src/notification-runtime.tsapps/notification-service/src/postgres-reminder-migration.test.tsapps/notification-service/src/postgres-reminder-repository.coverage.test.tsapps/notification-service/src/postgres-reminder-repository.integration.test.tsapps/notification-service/src/postgres-reminder-repository.test.tsapps/notification-service/src/postgres-reminder-repository.tsapps/notification-service/src/reminder-scheduler.integration.test.tsapps/notification-service/src/reminder-scheduler.test.tsapps/notification-service/src/reminder-scheduler.tsapps/notification-service/vitest.config.tsturbo.json
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/notification-service/src/postgres-reminder-migration.test.ts
- apps/notification-service/src/postgres-reminder-repository.test.ts
- apps/notification-service/src/postgres-reminder-repository.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Current slice
Implements issue #103 as the durable persistence layer beneath the merged reminder scheduler.
Implemented outcome
notification_servicePostgreSQL schema with only multi-word snake_case objectsVerification gate
Formatting, lint, type checking, complete tests, build, Compose validation, AppGuardrail, Semgrep, Security Scan, Commercial Readiness, CodeRabbit, and every actionable review thread must pass on the exact current head before merge.
Closes #103.
Summary by CodeRabbit
새로운 기능
버그 수정
문서