Skip to content

feat(dashboard): show account plan in request logs table#425

Open
stemirkhan wants to merge 3 commits intoSoju06:mainfrom
stemirkhan:feat/request-logs-plan-type
Open

feat(dashboard): show account plan in request logs table#425
stemirkhan wants to merge 3 commits intoSoju06:mainfrom
stemirkhan:feat/request-logs-plan-type

Conversation

@stemirkhan
Copy link
Copy Markdown
Contributor

@stemirkhan stemirkhan commented Apr 16, 2026

Summary

  • persist a nullable plan_type snapshot on request_logs when a request log row is written
  • expose the persisted planType on GET /api/request-logs and show it in the dashboard recent request logs table
  • keep legacy request-log rows renderable by leaving pre-existing rows with planType: null

Screenshot

Request logs plan column

Testing

  • /home/temirkhan/codex-lb-my/codex-lb-upstream/.venv/bin/python -m pytest tests/integration/test_request_logs_filters.py tests/integration/test_request_logs_list_count.py tests/integration/test_migrations.py -q
  • cd frontend && npx -y node@20 ./node_modules/vitest/vitest.mjs run src/features/dashboard/components/recent-requests-table.test.tsx src/features/dashboard/schemas.test.ts
  • openspec validate --specs

Notes

@stemirkhan
Copy link
Copy Markdown
Contributor Author

stemirkhan commented Apr 16, 2026

Ready for review. This now persists a nullable plan_type snapshot onto request_logs, so the dashboard shows the historical account plan for each row instead of joining against the current account state. I also updated the PR body screenshot/testing details to match the migration-based approach.

@stemirkhan stemirkhan marked this pull request as ready for review April 16, 2026 19:03
@huzky-v
Copy link
Copy Markdown
Collaborator

huzky-v commented Apr 17, 2026

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 current account type, not for the account type requested at that moment.

E.g. When the request is at free account type, but get upgraded to plus after the request, it may show plus, which i think is not showing the correct history in this case.

I still think persisting the account type (aka table change w/ migration) is necessary in this case

@stemirkhan
Copy link
Copy Markdown
Contributor Author

Thanks, that concern was correct. I reworked this in 62a6c55 to persist a nullable plan_type snapshot on request_logs with a migration, and the API/UI now read the persisted value instead of the current Account.plan_type.

I also added regression coverage for the history case: a log written while the account is free still returns planType: "free" after the account is later updated to team. Legacy rows created before the migration remain null intentionally.

Copy link
Copy Markdown
Owner

@Soju06 Soju06 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. RequestLog.plan_type (String, nullable=True) snapshot column on request_logs
  2. Alembic migration 20260417_000000_add_request_log_plan_type.py — idempotent, SQLite-safe via batch_alter_table, proper upgrade + downgrade
  3. RequestLog ↔ Account bidirectional relationship added (previously only FK, no relationship())
  4. Repository resolves plan_type from the account row when the caller does not provide it
  5. API schema + frontend: planType field 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 extra SELECT plan_type FROM accounts WHERE id = ? per request log when callers do not pass plan_type. All 10 existing _write_request_log callsites in app/modules/proxy/service.py fall into this path. Impact is low because request-log writes run under anyio.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: thread plan_type through from the dispatch path where AccountState already exposes .plan_type in memory.
  • [style] The repository.py diff removes a trailing blank line between imports and then re-introduces a cosmetic line-break in the order_by stmt. Harmless, but reduces diff noise review-for-review. Not worth a change.
  • [schema] New relationship() on both sides is bidirectional but has no cascade override — relies on the existing FK ondelete="CASCADE", which is correct for SQLAlchemy-side consistency.

Nits

  • PLAN_CLASS_MAP in recent-requests-table.tsx has four entries (free/plus/team/pro) and falls back to free styling 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 with null, per PR body.

Test coverage assessment

Good:

  • tests/integration/test_migrations.py — exercises the alembic upgrade/downgrade
  • tests/integration/test_request_logs_filters.py — new filter/list behavior
  • tests/integration/test_request_logs_list_count.py — pagination total
  • frontend/.../recent-requests-table.test.tsx — renders plan badge
  • frontend/.../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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants