Conversation
Ktl-XV
force-pushed
the
chore/refactor-audit-dashboard
branch
3 times, most recently
from
4629fa1 to
7b6097d
Compare
Ktl-XV
force-pushed
the
chore/refactor-audit-dashboard
branch
from
7b6097d to
ed68352
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR primarily refactors the audit dashboard by removing redundant Filter types, standardizing enum usage via strum, and splitting the API calls for audit details and statistics. Key changes include:
- Removal of legacy Filter types and deprecation of the "Latest State Audit" strategy.
- Migration to strum-based enum conversions with updated naming (e.g. renaming "SelectOldestUnaudited" to "OldestUnaudited").
- Updates to the audit dashboard’s frontend and backend integration with new API endpoints.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| migration/src/m20250521_164847_remove_state_latest_stats.rs | Removes the obsolete column/logic for Latest State Audit stats. |
| glados-web/templates/*.html | Updates template syntax to invoke enum message getters and reflect new enum naming. |
| glados-web/src/templates.rs & routes.rs | Adjustments to pass new enum types and API integration changes. |
| glados-core/src/stats.rs, glados-audit/, entity/ | Refactoring of audit filtering, enum conversion, and removal of legacy fields. |
| glados-web/assets/js/*.js | Modifications in JS components to align with the updated API endpoints and filters. |
Comments suppressed due to low confidence (1)
entity/src/audit_stats.rs:36
- The 'success_rate_state_latest' field has been removed. Confirm that all downstream components and consumers of AuditStats have been updated accordingly to avoid runtime or integration issues.
pub success_rate_state_latest: f32,
| No explanation available for {{ strategy.get_message().expect("Strategy missing message") }} | ||
| {% endmatch %} | ||
|
|
||
| </div> | ||
| <h2>{{ strategy }} stats</h2> | ||
| <h2>{{ strategy.get_message().expect("Strategy missing message") }} stats</h2> |
There was a problem hiding this comment.
Directly calling expect on get_message may lead to runtime panics if a strategy is missing its message. Consider providing a safe fallback or logging a clear error rather than panicking.
There was a problem hiding this comment.
I agree with the bot. We don't want to panic the server. Maybe .or_else("Strategy missing message") and/or an error log?
|
I think we should have a very good reason to review PR's this big: I'm not sure I see it here, except that reviews are a little slow (which is something I hope we can improve!). As a preview: I don't think we need to remove the latest state stats column from the DB. Migrations are always a risk point, and we will eventually want a latest state audit. Maybe within a few months even. Anyway, it looks like you've already got it cleanly separated into 3 commits, so that seems like the right separation to start with for PRs |
3 participants
closes #393
Changes:
Filtertypes, reuseSubProtocol,ContentType,SelectionStrategyandAuditResultinstead