[codex] Add subjective check-in calendar SLA#187
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a per-tenant ChangesCheck-in Calendar SLA Feature
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin Browser
participant Handler as adminCheckinCoverage
participant DB as Storage DB
participant SLA as buildCheckinCoverageWithSLA
rect rgba(100, 150, 255, 0.5)
note over AdminUI,DB: Save enabled-since (POST)
AdminUI->>Handler: POST /api/admin/checkin-coverage {enabled_since}
Handler->>DB: SaveCheckinEnabledSince(date)
DB-->>Handler: ok / error
end
rect rgba(100, 200, 150, 0.5)
note over AdminUI,SLA: Fetch SLA coverage (GET or after POST)
Handler->>DB: GetCheckinEnabledSince()
DB-->>Handler: enabledSince
Handler->>DB: GetCheckinCoverage(today, days)
DB->>SLA: buildCheckinCoverageWithSLA(rows, from, today)
SLA-->>DB: SLARows + SLASummary (pending/missing synthesized)
DB-->>Handler: CheckinCoverage {sla_active, sla_rows, sla_summary, rows, summary}
Handler-->>AdminUI: JSON response
AdminUI->>AdminUI: renderCheckinCoverage(d) — dual SLA/history blocks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (2)
internal/storage/subjective_checkin.go (2)
617-638: 💤 Low valueConsider using
checkinCoverageRowinbuildCheckinCoverageas well.This helper extracts the row conversion logic nicely. The same logic is duplicated inline in
buildCheckinCoverage(lines 479-517). Using this helper in both places would improve consistency and reduce maintenance burden.🤖 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 `@internal/storage/subjective_checkin.go` around lines 617 - 638, The checkinCoverageRow helper function has been created to encapsulate the conversion logic from CheckinRow to CheckinCoverageRow, but the same conversion logic is still duplicated inline in the buildCheckinCoverage function (around lines 479-517). Replace the duplicated row conversion logic in buildCheckinCoverage with calls to the checkinCoverageRow helper function to eliminate the duplication, improve consistency, and reduce maintenance burden when this logic needs to be updated in the future.
411-445: 💤 Low valueConsider extracting the row scanning logic to reduce duplication.
The row scanning loop here (lines 424-443) is nearly identical to the one in
GetCheckinCoverage(lines 371-390). Consider extracting a helper likescanCheckinRows(rows pgx.Rows) ([]CheckinRow, error)to consolidate this pattern.🤖 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 `@internal/storage/subjective_checkin.go` around lines 411 - 445, Extract the duplicate row scanning logic that appears in the getCheckinRowsBetween method (the loop iterating through rows.Next() and populating CheckinRow fields including handling nullable pointer fields like answer, msgID, and answeredAt) into a separate helper function called scanCheckinRows that accepts pgx.Rows as a parameter and returns []CheckinRow and error. Then replace the scanning loop in both getCheckinRowsBetween and the GetCheckinCoverage method with calls to this new helper function to eliminate the duplication.
🤖 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 `@internal/ui/handler.go`:
- Around line 2890-2893: The JSON decoding in the
json.NewDecoder(r.Body).Decode(&req) call lacks both request body size limiting
and strict parsing enforcement. Wrap r.Body with http.MaxBytesReader to impose a
reasonable size limit on incoming payloads, preventing memory exhaustion from
oversized requests. Additionally, enable strict JSON parsing by setting the
DisallowUnknownFields flag on the decoder instance before calling Decode to
reject partial or ambiguous JSON bodies that contain unknown fields.
In `@internal/ui/templates/pages/admin.html`:
- Around line 888-896: Replace the hardcoded English strings in the admin.html
check-in coverage fetch handler with localized strings. Specifically, replace
the progress message '...' on line 888 (the msg.textContent assignment) and the
error message 'Failed to save check-in SLA date' on line 895 with calls to
retrieve localized text using keys admin_saving and admin_checkin_save_failed
respectively. Additionally, add these two new localization keys (admin_saving
and admin_checkin_save_failed) to all three i18n files: i18n_en.go, i18n_ru.go,
and i18n_sr.go with appropriate English, Russian, and Serbian translations.
---
Nitpick comments:
In `@internal/storage/subjective_checkin.go`:
- Around line 617-638: The checkinCoverageRow helper function has been created
to encapsulate the conversion logic from CheckinRow to CheckinCoverageRow, but
the same conversion logic is still duplicated inline in the buildCheckinCoverage
function (around lines 479-517). Replace the duplicated row conversion logic in
buildCheckinCoverage with calls to the checkinCoverageRow helper function to
eliminate the duplication, improve consistency, and reduce maintenance burden
when this logic needs to be updated in the future.
- Around line 411-445: Extract the duplicate row scanning logic that appears in
the getCheckinRowsBetween method (the loop iterating through rows.Next() and
populating CheckinRow fields including handling nullable pointer fields like
answer, msgID, and answeredAt) into a separate helper function called
scanCheckinRows that accepts pgx.Rows as a parameter and returns []CheckinRow
and error. Then replace the scanning loop in both getCheckinRowsBetween and the
GetCheckinCoverage method with calls to this new helper function to eliminate
the duplication.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1028116-641f-461c-8104-bfda54e66aaf
📒 Files selected for processing (10)
docs/ai-plans/2026-06-18-checkin-calendar-sla.htmlinternal/storage/subjective_checkin.gointernal/storage/subjective_checkin_test.gointernal/ui/admin_checkin_coverage_test.gointernal/ui/handler.gointernal/ui/i18n_en.gointernal/ui/i18n_ru.gointernal/ui/i18n_sr.gointernal/ui/style.gointernal/ui/templates/pages/admin.html
1 participant
Summary
subjective_checkin_enabled_sincefor morning subjective check-in coverage.Why
The previous admin view intentionally showed only actual check-in rows so early rollout days would not appear as false missing failures. Now that the flow is live, operators need an explicit enabled-since date so missing expected prompt days can indicate scheduler, webhook, or prompt delivery failures.
Details
GET /api/admin/checkin-coveragekeepssummaryandrowsas actual history and addsenabled_since,sla_active,sla_summary, andsla_rows.POST /api/admin/checkin-coveragesavesenabled_sincefor the active tenant through the existingsettingstable.missingfor absent rows after enabled-since and usespendingfor today when no row exists yet, avoiding premature same-day failures.docs/ai-plans/2026-06-18-checkin-calendar-sla.htmland updated with implementation notes.Validation
go test ./internal/storage -run "Test.*Checkin|Test.*CheckinCoverage" -count=1go test ./internal/ui -run TestAdminCheckinCoverage -count=1go test ./internal/ui -run "TestAdmin(CheckinCoverage|Template|Page)" -count=1git diff --checkRelated
Summary by CodeRabbit
New Features
Tests