feat(llma): configurable cooldown for count-triggered eval reports#55544
feat(llma): configurable cooldown for count-triggered eval reports#55544andrewm4894 merged 4 commits intomasterfrom
Conversation
Users can now set the minimum hours between count-triggered evaluation reports (1-24h) instead of being stuck with a hardcoded 1h floor. The backing `cooldown_minutes` field already existed on the model; this exposes it in the UI, adds a max bound (1440), and validates both ends. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MCP UI Apps size report
|
Prompt To Fix All With AIThis is a comment left during a code review.
Path: products/llm_analytics/backend/api/test/test_evaluation_reports.py
Line: 165-210
Comment:
**Prefer parameterised tests**
The three new cooldown tests (`test_create_enforces_cooldown_minutes_min`, `test_create_enforces_cooldown_minutes_max`, `test_create_accepts_custom_cooldown_minutes`) follow the same request-and-assert structure as the existing threshold tests. Per the codebase's preference for parameterised tests, the two boundary-rejection cases at least could be collapsed into a single `@pytest.mark.parametrize` (or `subTest`) block.
```python
@pytest.mark.parametrize(
"cooldown_minutes,expected_status",
[
(EvaluationReport.COOLDOWN_MINUTES_MIN - 1, status.HTTP_400_BAD_REQUEST),
(EvaluationReport.COOLDOWN_MINUTES_MAX + 1, status.HTTP_400_BAD_REQUEST),
(6 * 60, status.HTTP_201_CREATED),
],
)
def test_create_validates_cooldown_minutes(self, cooldown_minutes, expected_status):
...
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: products/llm_analytics/frontend/evaluations/components/EvaluationReportConfig.tsx
Line: 372-373
Comment:
**Plural/singular can mismatch the displayed value**
The condition `(activeReport.cooldown_minutes ?? 60) <= 60` checks the raw minutes value to choose "hour" vs "hours", but the displayed number is `Math.round(cooldown_minutes / 60)`. For any `cooldown_minutes` between 61 and 89 (possible via direct API calls), `Math.round` rounds to 1 yet the condition is `false`, producing "1 hours". Deriving the plural from the already-computed count avoids the mismatch:
```tsx
{(() => {
const hours = Math.max(1, Math.round((activeReport.cooldown_minutes ?? 60) / 60))
return <>at most once every {hours} {hours === 1 ? 'hour' : 'hours'}. Checked every 5 minutes.</>
})()}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(llma): configurable cooldown for co..." | Re-trigger Greptile |
|
Size Change: +1.17 kB (0%) Total Size: 129 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72405c3b8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- parametrize the two out-of-bounds cooldown_minutes rejection tests - derive plural/singular from the displayed hour count to avoid "1 hours" for values like 89 minutes (set via direct API) - only patch cooldown_minutes when the user actually changed the hours field, so sub-hour values set via the API aren't silently rounded down on the next unrelated save Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
There was a problem hiding this comment.
Additive change that adds a configurable upper bound (1440 min / 24 h) to an existing cooldown field. All substantive bot-review comments were addressed in the current diff (parameterized tests, plural fix, silent-rounding regression), and no data model, migration, or dependency changes are involved.
…55544) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: tests-posthog[bot] <250237707+tests-posthog[bot]@users.noreply.github.com>
Problem
Count-triggered evaluation reports were locked to a 1-hour cooldown. Users who only want a report every 6h / 12h / 24h had to rely on the
daily_run_capsafety net, which doesn't express intent clearly and still allows a burst in the first few hours of the day.Changes
EvaluationReportmodel: addedCOOLDOWN_MINUTES_MAX = 1440constant.help_text.cooldown_minutesfield. Saved-state summary reads the user's chosen cadence instead of hardcoded "once per hour".tool-definitions-all.json/generated-tool-definitions.json.The backing field and activity-side cooldown check (
activities.py:100) already existed and respectedcooldown_minutes, so no workflow changes were needed.How did you test this code?
test_create_enforces_cooldown_minutes_min,test_create_enforces_cooldown_minutes_max,test_create_accepts_custom_cooldown_minutes.ruff checkclean.tsc --noEmitclean after regenerating kea logic types for the newsetDraftCooldownHoursaction.Publish to changelog?
no
🤖 LLM context
Authored by Claude Opus 4.7 in a session with Andy. Scope was deliberately kept to the cooldown control —
daily_run_capwas left in place as a safety net since it's doing real work at the 1h preset.