Add acknowledgedAt timestamp to notification status.#2834
Conversation
|
Warning Review limit reached
Next review available in: 45 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe alarm status contract now exposes an optional ChangesAlarm acknowledgement timestamps
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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 `@packages/server-api/src/typebox/protocol-schemas.ts`:
- Around line 120-124: Update the acknowledgedAt field in the protocol schema to
use the shared IsoTimeSchema instead of a plain Type.String definition, while
preserving its optionality.
In `@src/api/notifications/alarm.ts`:
- Around line 120-122: Update the else branch handling a non-'Yes'
acknowledgeStatus to delete acknowledgedAt alongside setting
this.status.acknowledged to false, matching the cleanup behavior in clear() and
the normal-state transition.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ae9601d-9f4a-4622-b9cf-7831dfc9aa27
📒 Files selected for processing (2)
packages/server-api/src/typebox/protocol-schemas.tssrc/api/notifications/alarm.ts
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 (2)
src/api/notifications/alarm.ts (2)
95-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd automated regression coverage for the timestamp lifecycle.
Cover external synchronization, manual acknowledgement, normal-state transitions, repeated updates, and clearing. Postman validation alone does not protect these distinct state transitions from regressions.
Also applies to: 228-249
🤖 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/api/notifications/alarm.ts` around lines 95 - 123, Add automated tests covering the alarm timestamp lifecycle through syncFromNotificationUpdate and the manual acknowledgement path: external synchronization, acknowledgement, transitions away from and back to ALARM_STATE.normal, repeated updates, and clearing. Assert acknowledgedAt is set, preserved, or removed appropriately at each transition, using existing alarm test fixtures and helpers.
95-123: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the incoming update timestamp for synchronized acknowledgements.
syncFromNotificationUpdate()setsacknowledgedAtfromnew Date(), so delayed or replayed updates record server receipt time instead of the source event time. Useupdate.timestamphere, with a fallback tonowonly when it’s missing.🤖 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/api/notifications/alarm.ts` around lines 95 - 123, Update syncFromNotificationUpdate() so acknowledgedAt uses update.timestamp when the incoming timestamp is present, falling back to the current time only when it is missing; preserve the existing acknowledgement state handling.
🤖 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 `@docs/develop/rest-api/notifications_api.md`:
- Line 485: Update the acknowledge result description near the existing
status.acknowledged documentation to add a bullet stating that successful
acknowledgement also sets status.acknowledgedAt, matching the timestamp field
shown in the response example.
- Line 106: Update the acknowledgedAt field description in the notifications API
documentation to state that it is optional and uses the ISO 8601 timestamp
format, while retaining that it records when the acknowledge action occurred.
---
Outside diff comments:
In `@src/api/notifications/alarm.ts`:
- Around line 95-123: Add automated tests covering the alarm timestamp lifecycle
through syncFromNotificationUpdate and the manual acknowledgement path: external
synchronization, acknowledgement, transitions away from and back to
ALARM_STATE.normal, repeated updates, and clearing. Assert acknowledgedAt is
set, preserved, or removed appropriately at each transition, using existing
alarm test fixtures and helpers.
- Around line 95-123: Update syncFromNotificationUpdate() so acknowledgedAt uses
update.timestamp when the incoming timestamp is present, falling back to the
current time only when it is missing; preserve the existing acknowledgement
state handling.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bdca332-b00f-4c10-9ebe-04a3f0eb277c
📒 Files selected for processing (2)
docs/develop/rest-api/notifications_api.mdsrc/api/notifications/alarm.ts
1 participant
Add a new property
acknowledgedAtto the notificationstatusobject to hold the timestamp of when the alarm was acknowledged.Tested using the Notifications API via postman to confirm operation.
Summary
This PR adds an optional
acknowledgedAttimestamp to notification alarm status. The server API schema andAlarmStatustype includeacknowledgedAtas an ISO 8601 time. The alarm records the current timestamp when it is acknowledged and removesacknowledgedAtwhen the alarm is cleared/reset or when the alarm is updated to a non-normalstate, so the timestamp is not present in the payload afterward.