fix: land issue 127 follow-up cleanup and hostname split#133
Conversation
Bundles all 8 follow-up items from issue #127. Code correctness: - recent-logs-table: switch virtualization from absolute-positioned <tr> (which browsers ignore on table rows) to the padding-rows technique per @tanstack/react-virtual table guidance. As a bonus, real <tr>s inherit thead's column widths, so the sticky header now aligns with body cells — obsoletes the column-drift TODO. - sluice_aca/main.tf: define `auto` model alias unconditionally. It's allowlisted by every vkey in keys.yaml, so a deployment with no extra providers configured was 404'ing `model: "auto"` requests. With no extras it now points at Azure (same target as `premium`). - sluice_aca/main.tf: split has_any_extra into per-alias guards. A gemini-only deployment was emitting fallback chains that referenced cheap-fast / cheap-reasoning (groq/together/fireworks only). Each chain entry is now gated on whether its source alias actually exists, and context_window_fallbacks is gated on has_cheap_long_context. - use-keys: change non-admin fallback from /v1/key/info to /key/info to match the rest of the codebase's key-management endpoints (the /v1/key/* prefix is OpenAI-compat surface, not wired for key-info). - analytics-view + admin-view: pass `null` to RefreshBar's lastUpdated until the first fetch completes, instead of constructing `new Date(0)` (= 1970-01-01) or `new Date(Date.now())`. Matches the pattern already used in components/dashboard.tsx:312. Hygiene: - Remove .claude/settings.local.json (developer-local Claude perms), .handoff.md (ephemeral session log), .pr-body.md (generated PR body). - Add .claude/ to .gitignore so future local settings don't reappear. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe PR updates dashboard timestamp and key lookup handling, reworks recent-log virtualization, adds dashboard public URL and custom-domain Terraform configuration, makes LiteLLM fallbacks provider-aware, and ignores local Claude settings. ChangesDashboard behavior
Dashboard public access
Conditional gateway routing
Repository local settings
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR finishes the Issue #127 follow-up cleanup and completes the hostname split so the dashboard and LiteLLM gateway/admin UI live on separate public domains, while keeping Terraform aligned with the out-of-band DNS/cert bindings.
Changes:
- Terraform: ensure the
autoalias is always present and make fallback/context-window chains conditional on which alias groups actually exist. - Terraform: add explicit
gateway_public_url,dashboard_public_url, and dashboard custom-domain bindings to support the split (sluice.phoenixvc.techvslitellm.sluice.phoenixvc.tech). - Dashboard: fix table virtualization for recent logs, correct non-admin key-info endpoint usage, and prevent “1970-01-01” last-updated rendering.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| infra/modules/sluice_aca/main.tf | Makes auto always defined and hardens fallbacks/context-window fallbacks against missing alias groups. |
| infra/modules/dashboard_aca/variables.tf | Adds public URL + custom domain inputs for the dashboard module. |
| infra/modules/dashboard_aca/main.tf | Uses public gateway/dashboard URLs for env vars and adds dashboard custom-domain bindings. |
| infra/modules/dashboard_aca/outputs.tf | Outputs dashboard URL preferring configured public URL; exposes bound custom domains. |
| infra/env/prod/variables.tf | Wires new public URL and dashboard custom-domain variables at the prod env layer. |
| infra/env/prod/terraform.tfvars | Sets prod values for split hostnames and dashboard/gateway public URLs. |
| infra/env/prod/main.tf | Updates out-of-band import adoption to match the new gateway + dashboard hostnames and passes new vars to the dashboard module. |
| dashboard/hooks/use-keys.ts | Uses /key/info (not /v1/key/info) for non-admin key lookup. |
| dashboard/components/tables/recent-logs-table.tsx | Switches recent-logs table virtualization to padding-row technique so <tr> stays in normal flow and aligns with sticky headers. |
| dashboard/components/analytics/analytics-view.tsx | Avoids rendering epoch time by passing null until first successful refresh. |
| dashboard/components/admin/admin-view.tsx | Matches refresh-bar behavior by passing null until first successful refresh. |
| .gitignore | Ignores .claude/ to prevent developer-local Claude settings from being committed. |
| .pr-body.md | Removes generated PR template file from the repo. |
| .handoff.md | Removes ephemeral handoff log from the repo. |
| .claude/settings.local.json | Removes developer-local Claude permissions config from the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbc4b6d37e
ℹ️ 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".
| # Public dashboard entrypoint. The dashboard links through to the LiteLLM | ||
| # admin UI at gateway_public_url /ui. | ||
| dashboard_custom_domains = ["sluice.phoenixvc.tech"] | ||
| dashboard_public_url = "https://sluice.phoenixvc.tech" |
There was a problem hiding this comment.
When this maps sluice.phoenixvc.tech to the dashboard, the repo still tells agent/SDK consumers to use that same host as the gateway (AGENTS.md documents OPENAI_BASE_URL=https://sluice.phoenixvc.tech/v1 and Anthropic base URLs on the same hostname). The dashboard app only proxies LiteLLM under /api/gateway/[...path], so clients following those instructions will send /v1 or Anthropic requests to the Next app instead of LiteLLM and fail; update the consumer-facing instructions or add compatible passthroughs as part of this hostname split.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
infra/modules/dashboard_aca/main.tf (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache normalized URLs in locals.
trimspace(var.gateway_public_url)andtrimspace(var.dashboard_public_url)are each evaluated twice. Define each normalized value once and reuse it.Proposed refactor
locals { prefix = "pvc-${var.env}-${var.projname}" ca_name = "${local.prefix}-dashboard" use_shared_token = trimspace(var.state_service_shared_token) != "" use_entra = var.auth_mode == "entra" - gateway_url = trimspace(var.gateway_public_url) != "" ? trimspace(var.gateway_public_url) : var.gateway_url + gateway_public_url = trimspace(var.gateway_public_url) + gateway_url = local.gateway_public_url != "" ? local.gateway_public_url : var.gateway_url cae_suffix = trimprefix(var.gateway_url, "https://${local.prefix}-ca.") derived_dashboard_url = local.cae_suffix == var.gateway_url ? "" : "https://${local.ca_name}.${local.cae_suffix}" - dashboard_url = trimspace(var.dashboard_public_url) != "" ? trimspace(var.dashboard_public_url) : local.derived_dashboard_url + dashboard_public_url = trimspace(var.dashboard_public_url) + dashboard_url = local.dashboard_public_url != "" ? local.dashboard_public_url : local.derived_dashboard_url }As per coding guidelines: Use local values for repeated expressions in Terraform.
🤖 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 `@infra/modules/dashboard_aca/main.tf` around lines 18 - 26, Define locals for the trimmed gateway and dashboard public URL values, then update gateway_url and dashboard_url to reuse those locals instead of calling trimspace on each variable twice. Keep the existing fallback behavior unchanged.Source: Coding guidelines
dashboard/components/tables/recent-logs-table.tsx (1)
262-304: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSolid padding-rows virtualization implementation.
Correctly follows the
@tanstack/react-virtualtable pattern: spacer<tr>s sized fromstart/getTotalSize(), real rows in normal flow,colSpanmatches the 8 header columns.One optional improvement:
estimateSize: () => 48is a static guess, butDataRowrenders two-line cells (App/Agent, Tokens), so actual row height can exceed 48px. Consider wiring upmeasureElementso the virtualizer measures real row heights, keepingpaddingTop/paddingBottomaccurate and avoiding scroll-position drift on longer lists.♻️ Optional: enable dynamic row measurement
{virtualItems.map((virtualRow) => { const d = sorted[virtualRow.index]; const tsFmt = d.tsMs > 0 ? new Date(d.tsMs).toLocaleString() : d.ts || "—"; return ( <DataRow key={d.raw.request_id || virtualRow.index} + ref={rowVirtualizer.measureElement} + data-index={virtualRow.index} tsFmt={tsFmt} derived={d} onSelect={() => setSelectedRow(d.raw)} /> ); })}(Requires forwarding a ref to the underlying
<tr>inDataRow.)🤖 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 `@dashboard/components/tables/recent-logs-table.tsx` around lines 262 - 304, Optionally enable dynamic row-height measurement for the table virtualizer: update the virtualizer configuration using measureElement and forward its ref through DataRow to the underlying tr, while retaining estimateSize as the fallback. Ensure measured two-line rows keep paddingTop, paddingBottom, and scroll positioning accurate.infra/modules/sluice_aca/main.tf (1)
439-469: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtract provider pricing into shared locals
The Groq/Together/Fireworks token prices are duplicated in both the provider model entries and theautofallback; a single source of truth would reduce drift when pricing changes.🤖 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 `@infra/modules/sluice_aca/main.tf` around lines 439 - 469, Extract the Groq, Together, and Fireworks input/output token prices into shared locals in the module’s locals block, then reuse those locals in both the provider model entries and the auto_model_cost_in/auto_model_cost_out fallback expressions. Remove the duplicated numeric literals while preserving the existing provider-specific pricing and fallback behavior.
🤖 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.
Nitpick comments:
In `@dashboard/components/tables/recent-logs-table.tsx`:
- Around line 262-304: Optionally enable dynamic row-height measurement for the
table virtualizer: update the virtualizer configuration using measureElement and
forward its ref through DataRow to the underlying tr, while retaining
estimateSize as the fallback. Ensure measured two-line rows keep paddingTop,
paddingBottom, and scroll positioning accurate.
In `@infra/modules/dashboard_aca/main.tf`:
- Around line 18-26: Define locals for the trimmed gateway and dashboard public
URL values, then update gateway_url and dashboard_url to reuse those locals
instead of calling trimspace on each variable twice. Keep the existing fallback
behavior unchanged.
In `@infra/modules/sluice_aca/main.tf`:
- Around line 439-469: Extract the Groq, Together, and Fireworks input/output
token prices into shared locals in the module’s locals block, then reuse those
locals in both the provider model entries and the
auto_model_cost_in/auto_model_cost_out fallback expressions. Remove the
duplicated numeric literals while preserving the existing provider-specific
pricing and fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d88375eb-8823-4985-9d5a-be7b7b1620b0
📒 Files selected for processing (15)
.claude/settings.local.json.gitignore.handoff.md.pr-body.mddashboard/components/admin/admin-view.tsxdashboard/components/analytics/analytics-view.tsxdashboard/components/tables/recent-logs-table.tsxdashboard/hooks/use-keys.tsinfra/env/prod/main.tfinfra/env/prod/terraform.tfvarsinfra/env/prod/variables.tfinfra/modules/dashboard_aca/main.tfinfra/modules/dashboard_aca/outputs.tfinfra/modules/dashboard_aca/variables.tfinfra/modules/sluice_aca/main.tf
💤 Files with no reviewable changes (3)
- .claude/settings.local.json
- .pr-body.md
- .handoff.md
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes