feat(dashboard): show account plan in request logs table#425
feat(dashboard): show account plan in request logs table#425stemirkhan wants to merge 3 commits intoSoju06:mainfrom
Conversation
|
Ready for review. This now persists a nullable |
|
Just curious that if a user account plan is changed, the account type in the history will be wrong as the joining is for the E.g. When the request is at I still think persisting the account type (aka table change w/ migration) is necessary in this case |
|
Thanks, that concern was correct. I reworked this in I also added regression coverage for the history case: a log written while the account is |
Soju06
left a comment
There was a problem hiding this comment.
PR #425 Code Review
Verdict
APPROVE WITH NITS — mergeable into v1.13.2. Solid small feature, no correctness issues, one optional performance nit.
Summary of changes
RequestLog.plan_type(String, nullable=True) snapshot column onrequest_logs- Alembic migration
20260417_000000_add_request_log_plan_type.py— idempotent, SQLite-safe viabatch_alter_table, properupgrade+downgrade RequestLog ↔ Accountbidirectional relationship added (previously only FK, norelationship())- Repository resolves
plan_typefrom the account row when the caller does not provide it - API schema + frontend:
planTypefield surfaces in dashboard request logs table with per-plan badge colors and in the detail flyout
Correctness findings
Blocking
- None.
Non-blocking
- [perf]
RequestLogsRepository.add_log()now triggers one extraSELECT plan_type FROM accounts WHERE id = ?per request log when callers do not passplan_type. All 10 existing_write_request_logcallsites inapp/modules/proxy/service.pyfall into this path. Impact is low because request-log writes run underanyio.CancelScope(shield=True)after the response is already settled (not on the hot path), so client latency is unaffected — but on high-traffic deployments this is +1 query per request. Mitigation option: threadplan_typethrough from the dispatch path whereAccountStatealready exposes.plan_typein memory. - [style] The
repository.pydiff removes a trailing blank line between imports and then re-introduces a cosmetic line-break in theorder_bystmt. Harmless, but reduces diff noise review-for-review. Not worth a change. - [schema] New
relationship()on both sides is bidirectional but has nocascadeoverride — relies on the existing FKondelete="CASCADE", which is correct for SQLAlchemy-side consistency.
Nits
PLAN_CLASS_MAPinrecent-requests-table.tsxhas four entries (free/plus/team/pro) and falls back tofreestyling for unknown plans (business,enterprise). That is what the backend surface exposes today, but worth tracking with #370 which already asks for richer plan breakdown.- Frontend schema uses
.nullable().optional().default(null)— the triple is intentional to keep legacy rows rendering withnull, per PR body.
Test coverage assessment
Good:
tests/integration/test_migrations.py— exercises the alembic upgrade/downgradetests/integration/test_request_logs_filters.py— new filter/list behaviortests/integration/test_request_logs_list_count.py— pagination totalfrontend/.../recent-requests-table.test.tsx— renders plan badgefrontend/.../schemas.test.ts— validates optional/null field
Verification commands in the PR body map to real files. Not bluffing.
Recommendation for merge
Merge for v1.13.2. The +1-query concern is a legitimate follow-up, not a blocker — add an issue after merge to thread plan_type through from the dispatch path for the hot-traffic use case.
Closes/advances: #410 (dashboard plan_type request in logs table) — per PR body.
Summary
plan_typesnapshot onrequest_logswhen a request log row is writtenplanTypeonGET /api/request-logsand show it in the dashboard recent request logs tableplanType: nullScreenshot
Testing
Notes
null