refactor(dashboard): modular styles, TypeScript foundation, shared editor shells - #613
Open
SantiagoDePolonia wants to merge 7 commits into
Open
refactor(dashboard): modular styles, TypeScript foundation, shared editor shells#613SantiagoDePolonia wants to merge 7 commits into
SantiagoDePolonia wants to merge 7 commits into
Conversation
The 2453-line monolith becomes 14 files under src/styles/ imported in cascade order from dashboard.css. Pure partition at section boundaries: the concatenation — and the emitted Vite CSS bundle — is byte-identical to the previous build. No preprocessor added on purpose: Svelte scoped styles, custom properties, and native nesting already cover the need. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shared layer (api, utils, stores, datePickerLogic) moves from .js/.svelte.js to strictly-typed .ts/.svelte.ts: ApiResult envelope, store state shapes, Window globals in globals.d.ts. Import specifiers updated across pages and tests; node --test runs the .ts imports via native type stripping (Node 22.18+). jsconfig.json becomes a strict tsconfig.json (allowJs keeps page code unchecked during migration). No behavior changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gle shells Every editor modal (11 files) repeated the same Modal + header + error-banner + actions markup and an Enabled/Disabled toggle. They now compose three shared components; the shell also adds the previously inconsistent Escape-under-auth-dialog guard to the five editors that lacked it. Store wiring, ids, autofocus and validation behavior are unchanged. (dist rebuilt in the closing commit of this series.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ladder Seven page stores hand-rolled the same stale/503/error guards around getJSON/sendJSON, with drift: some checked 503 before staleness (letting an old API key's response clobber the availability flag) and 401 load errors were inconsistently surfaced. loadAdminList/sendAdminMutation in $lib/api/adminCrud.ts now encode the ladder once — stale first, then unavailable, then errors with silent-401 loads. Public store APIs and user-visible copy are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ProviderStatusCard (428->239) hands its collapsible body to ProviderStatusCardDetails; ConversationDrawer (377->147) extracts ChatMessage; AuditEntrySummary extracts AuditAttemptTrack and swaps its hand-rolled chevron SVG for the Icon atom; Sidebar moves its nav-item table to a typed navigation.ts module and the logo into an atom; SummaryCards dedupes its twin token/status cards with local snippets. Computed-class CSS stays where the compiler can see its markup per CONVENTIONS; rendered DOM is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ls in CONVENTIONS Also syncs the embedded dist with the refactored sources. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Too many files changed for review. ( Bypass the limit by tagging |
Contributor
📝 WalkthroughWalkthroughThe dashboard now centralizes admin CRUD request outcomes, standardizes editor dialogs and form fields, extracts audit and overview UI components, moves navigation definitions into a registry, and splits the monolithic stylesheet into ordered CSS modules. ChangesDashboard consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminPage
participant EditorDialog
participant AdminCrud
participant DashboardAPI
AdminPage->>EditorDialog: render editor and submit handlers
AdminPage->>AdminCrud: load or mutate admin data
AdminCrud->>DashboardAPI: request admin endpoint
DashboardAPI-->>AdminCrud: response envelope
AdminCrud-->>AdminPage: outcome status and data
AdminPage->>EditorDialog: update error, availability, and submitting state
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
Scope trim: the TS conversion of $lib was too much for this iteration. The foundation returns to .js/.svelte.js with jsconfig.json, and the two modules born as TS in this branch (adminCrud, navigation) are ported to plain JS with their behavior unchanged. Everything else from the refactoring — style modules, EditorDialog/FormField/EnabledToggle, the adminCrud request ladder, component splits — stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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.
Inline comments:
In `@web/dashboard/src/lib/components/atoms/GoModelLogo.svelte`:
- Around line 4-8: Update the decorative SVG in GoModelLogo to include
aria-hidden="true", ensuring assistive technologies ignore the unlabeled logo
while preserving its visual rendering.
In `@web/dashboard/src/lib/components/organisms/EditorDialog.svelte`:
- Around line 107-114: Separate the EditorDialog submit button’s in-flight state
from its disabled state by adding and using a dedicated submitDisabled prop for
the disabled attribute, while keeping submitting responsible only for the
submitting label and icon state. Update callers that currently fake submitting
or override submittingLabel for failoverFormManaged, vmDeleting, or
vmFormManaged to pass their real saving/deleting state as submitting and their
permanent non-submittable conditions as submitDisabled, removing redundant
custom submittingLabel values.
In `@web/dashboard/src/pages/auth-keys/AuthKeyEditor.svelte`:
- Around line 48-135: Update AuthKeyEditor’s name and expires fields to use the
shared FormField component instead of hand-rolled .form-field wrappers,
preserving their existing labels, required/optional hints, input bindings, IDs,
and attributes. Keep the InlineHelpSection-based fields unchanged because their
custom label/help structure cannot use FormField directly.
- Around line 12-26: Update the store’s closeForm() method to clear the
issuedValue state when closing the dialog, matching dismissIssuedKey() behavior
so reopened dialogs cannot re-display the one-time secret. Keep the existing
form-closing behavior unchanged.
In `@web/dashboard/src/pages/guardrails/guardrails.svelte.js`:
- Around line 151-152: Update both fetcher paths around the assignments to
this.available and this.types so available is set to true only when
loadAdminList returns a non-null result, indicating the gateway responded.
Preserve the unavailable state for thrown requests with result: null, while
retaining the existing item assignment and error handling for valid responses.
In `@web/dashboard/src/pages/providers-config/ProviderCredentialEditor.svelte`:
- Around line 57-66: Update the error handling between providersConfig.svelte.js
and ProviderCredentialEditor.svelte so list/load failures remain in a scoped
list error and are not passed to EditorDialog. Ensure the dialog’s error prop
receives only form-local, non-field save errors, using the existing
providersConfig form state or a dedicated form-local error field.
In `@web/dashboard/src/pages/rate-limits/RateLimitEditor.svelte`:
- Around line 10-21: Update the comment above EditorDialog in
RateLimitEditor.svelte to reflect the current form behavior: period-seconds is
removed from the DOM unless period is "custom", so it is not a hidden invalid
control. If rateLimitFormPayload() no longer requires disabling native
validation, remove the novalidate attribute; otherwise retain it with an
accurate justification.
In `@web/dashboard/src/pages/workflows/WorkflowEditor.svelte`:
- Around line 184-193: Update the guardrail step number input in the workflow
editor to allow arbitrary integer ordering values by changing the step
constraint from 10 to 1, while preserving the existing nonnegative minimum and
binding.
In `@web/dashboard/src/pages/workflows/workflows.svelte.js`:
- Line 230: Guard availability updates on successful fetch results so network
failures do not restore availability after a 503. In the workflows load flow,
wrap the existing this.available = true in an outcome.result check; apply the
same guard to the availability assignments in guardrails fetchTypes and
fetchGuardrails, leaving their existing error handling unchanged.
In `@web/dashboard/src/styles/base.css`:
- Line 3: In the font-family declaration containing “Inter”, remove the
quotation marks around the Inter font name while preserving the existing
fallback fonts and declaration structure.
In `@web/dashboard/src/styles/themes.css`:
- Line 77: Add a blank line immediately before each of the three color-scheme
declarations in themes.css, including the declarations near the referenced
locations, to satisfy the configured Stylelint declaration-spacing rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64525026-a558-49c5-8635-ec7d9e4459f9
⛔ Files ignored due to path filters (4)
internal/admin/dashboard/static/dist/assets/index-BGJkl_-O.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-BOEWOpVo.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-Dd3LDA86.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (48)
web/dashboard/CONVENTIONS.mdweb/dashboard/src/lib/api/adminCrud.jsweb/dashboard/src/lib/components/atoms/EnabledToggle.svelteweb/dashboard/src/lib/components/atoms/GoModelLogo.svelteweb/dashboard/src/lib/components/molecules/FormField.svelteweb/dashboard/src/lib/components/organisms/EditorDialog.svelteweb/dashboard/src/lib/components/organisms/Sidebar.svelteweb/dashboard/src/lib/components/organisms/navigation.jsweb/dashboard/src/pages/audit-logs/AuditAttemptTrack.svelteweb/dashboard/src/pages/audit-logs/AuditEntrySummary.svelteweb/dashboard/src/pages/audit-logs/ChatMessage.svelteweb/dashboard/src/pages/audit-logs/ConversationDrawer.svelteweb/dashboard/src/pages/auth-keys/AuthKeyEditor.svelteweb/dashboard/src/pages/auth-keys/AuthKeyLabelsEditor.svelteweb/dashboard/src/pages/auth-keys/authKeys.svelte.jsweb/dashboard/src/pages/budgets/BudgetEditor.svelteweb/dashboard/src/pages/budgets/budgets.svelte.jsweb/dashboard/src/pages/guardrails/GuardrailEditor.svelteweb/dashboard/src/pages/guardrails/guardrails.svelte.jsweb/dashboard/src/pages/mcp-servers/McpServerEditor.svelteweb/dashboard/src/pages/mcp-servers/mcpServers.svelte.jsweb/dashboard/src/pages/models/FailoverEditor.svelteweb/dashboard/src/pages/models/PricingOverrideEditor.svelteweb/dashboard/src/pages/models/VirtualModelEditor.svelteweb/dashboard/src/pages/overview/ProviderStatusCard.svelteweb/dashboard/src/pages/overview/ProviderStatusCardDetails.svelteweb/dashboard/src/pages/overview/SummaryCards.svelteweb/dashboard/src/pages/providers-config/ProviderCredentialEditor.svelteweb/dashboard/src/pages/providers-config/providersConfig.svelte.jsweb/dashboard/src/pages/rate-limits/RateLimitEditor.svelteweb/dashboard/src/pages/rate-limits/rateLimits.svelte.jsweb/dashboard/src/pages/workflows/WorkflowEditor.svelteweb/dashboard/src/pages/workflows/workflows.svelte.jsweb/dashboard/src/styles/alerts.cssweb/dashboard/src/styles/auth-dialog.cssweb/dashboard/src/styles/base.cssweb/dashboard/src/styles/budgets.cssweb/dashboard/src/styles/buttons.cssweb/dashboard/src/styles/cards-charts.cssweb/dashboard/src/styles/dashboard.cssweb/dashboard/src/styles/forms.cssweb/dashboard/src/styles/layout.cssweb/dashboard/src/styles/page-globals.cssweb/dashboard/src/styles/responsive.cssweb/dashboard/src/styles/settings.cssweb/dashboard/src/styles/tables.cssweb/dashboard/src/styles/themes.cssweb/dashboard/src/styles/usage-audit.css
Comment on lines
+4
to
+8
| <svg viewBox="0 0 30 30" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path d="M15 3L25.39 9L25.39 21L15 27L4.61 21L4.61 9Z" stroke="currentColor" stroke-width="2" fill="none"/> | ||
| <circle cx="15" cy="15" r="4" fill="currentColor" opacity="0.3"/> | ||
| <path d="M15 9.5L15 6M15 20.5L15 24M19.76 12.25L22.79 10.5M10.24 17.75L7.21 19.5M19.76 17.75L22.79 19.5M10.24 12.25L7.21 10.5" stroke="currentColor" stroke-width="1.5" stroke-linecap="round"/> | ||
| </svg> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the decorative logo as hidden from assistive tech.
The SVG has no accessible name and no aria-hidden, so screen readers may announce it as an unlabeled graphic next to the product name.
♿ Proposed fix
-<svg viewBox="0 0 30 30" fill="none" xmlns="http://www.w3.org/2000/svg">
+<svg viewBox="0 0 30 30" fill="none" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" focusable="false">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <svg viewBox="0 0 30 30" fill="none" xmlns="http://www.w3.org/2000/svg"> | |
| <path d="M15 3L25.39 9L25.39 21L15 27L4.61 21L4.61 9Z" stroke="currentColor" stroke-width="2" fill="none"/> | |
| <circle cx="15" cy="15" r="4" fill="currentColor" opacity="0.3"/> | |
| <path d="M15 9.5L15 6M15 20.5L15 24M19.76 12.25L22.79 10.5M10.24 17.75L7.21 19.5M19.76 17.75L22.79 19.5M10.24 12.25L7.21 10.5" stroke="currentColor" stroke-width="1.5" stroke-linecap="round"/> | |
| </svg> | |
| <svg viewBox="0 0 30 30" fill="none" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" focusable="false"> | |
| <path d="M15 3L25.39 9L25.39 21L15 27L4.61 21L4.61 9Z" stroke="currentColor" stroke-width="2" fill="none"/> | |
| <circle cx="15" cy="15" r="4" fill="currentColor" opacity="0.3"/> | |
| <path d="M15 9.5L15 6M15 20.5L15 24M19.76 12.25L22.79 10.5M10.24 17.75L7.21 19.5M19.76 17.75L22.79 19.5M10.24 12.25L7.21 10.5" stroke="currentColor" stroke-width="1.5" stroke-linecap="round"/> | |
| </svg> |
🤖 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 `@web/dashboard/src/lib/components/atoms/GoModelLogo.svelte` around lines 4 -
8, Update the decorative SVG in GoModelLogo to include aria-hidden="true",
ensuring assistive technologies ignore the unlabeled logo while preserving its
visual rendering.
Comment on lines
+107
to
+114
| <button | ||
| type="submit" | ||
| class="btn btn-primary btn-with-icon" | ||
| disabled={submitting} | ||
| > | ||
| <Icon name={submitIcon} class="form-action-icon" /> | ||
| <span>{submitting ? submittingLabel : submitLabel}</span> | ||
| </button> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Separate "in flight" from "not submittable".
submitting drives both disabled and the label swap, so consumers that need a permanently-disabled submit (failoverFormManaged, vmDeleting, vmFormManaged) must pass a fake submitting and then re-derive submittingLabel to suppress "Saving...". Two callers already duplicate that logic; a dedicated submitDisabled prop keeps the label honest.
♻️ Proposed refactor
submitting = false,
+ submitDisabled = false,
submitLabel = "Save", <button
type="submit"
class="btn btn-primary btn-with-icon"
- disabled={submitting}
+ disabled={submitting || submitDisabled}
>Callers then pass e.g. submitting={failover.failoverSaving} and submitDisabled={failover.failoverFormManaged || failover.failoverGenerating} and drop the custom submittingLabel.
🤖 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 `@web/dashboard/src/lib/components/organisms/EditorDialog.svelte` around lines
107 - 114, Separate the EditorDialog submit button’s in-flight state from its
disabled state by adding and using a dedicated submitDisabled prop for the
disabled attribute, while keeping submitting responsible only for the submitting
label and icon state. Update callers that currently fake submitting or override
submittingLabel for failoverFormManaged, vmDeleting, or vmFormManaged to pass
their real saving/deleting state as submitting and their permanent
non-submittable conditions as submitDisabled, removing redundant custom
submittingLabel values.
Comment on lines
+12
to
+26
| <EditorDialog | ||
| open={store.formOpen} | ||
| title="Create API Key" | ||
| ariaLabel="API key editor" | ||
| error={store.issuedValue ? "" : store.error} | ||
| submitting={store.formSubmitting} | ||
| submitLabel={store.issuedValue ? "Done, I’ve stored it" : "Create API Key"} | ||
| submittingLabel="Creating..." | ||
| submitIcon={store.issuedValue ? "check" : "plus"} | ||
| cancel={false} | ||
| dialogClass="auth-key-editor" | ||
| onclose={() => store.closeForm()} | ||
| onsubmit={() => | ||
| store.issuedValue ? store.dismissIssuedKey() : store.submitForm()} | ||
| > |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd authKeys.svelte.js --exec rg -n -A12 'closeForm|dismissIssuedKey'Repository: ENTERPILOT/GoModel
Length of output: 799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the auth-keys store and editor dialog files and inspect the relevant code.
fd 'AuthKeyEditor\.svelte|authKeys\.svelte\.js|EditorDialog' . -x sh -c '
echo "===== $1 ====="
wc -l "$1"
sed -n "1,180p" "$1" | cat -n
' sh {}Repository: ENTERPILOT/GoModel
Length of output: 18927
Clear issued secrets before closing the dialog.
closeForm() leaves issuedValue intact when submitted has already displayed it, so using the dialog close button or Escape can reopen the dialog and re-display the one-time API key secret. Clear the issued state in closeForm() as well as in dismissIssuedKey().
🤖 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 `@web/dashboard/src/pages/auth-keys/AuthKeyEditor.svelte` around lines 12 - 26,
Update the store’s closeForm() method to clear the issuedValue state when
closing the dialog, matching dismissIssuedKey() behavior so reopened dialogs
cannot re-display the one-time secret. Keep the existing form-closing behavior
unchanged.
Comment on lines
+48
to
+135
| <div class="form-field"> | ||
| <label class="form-field-label" for="auth-key-name"> | ||
| Name <span class="form-hint">(required)</span> | ||
| </label> | ||
| <input | ||
| id="auth-key-name" | ||
| type="text" | ||
| placeholder="e.g. ci-deploy" | ||
| autocomplete="off" | ||
| data-modal-autofocus | ||
| bind:value={store.form.name} | ||
| /> | ||
| </div> | ||
| {:else} | ||
| <div class="auth-key-form-fields"> | ||
| <div class="form-grid"> | ||
| <div class="form-field"> | ||
| <label class="form-field-label" for="auth-key-name"> | ||
| Name <span class="form-hint">(required)</span> | ||
| </label> | ||
| <input | ||
| id="auth-key-name" | ||
| type="text" | ||
| placeholder="e.g. ci-deploy" | ||
| autocomplete="off" | ||
| data-modal-autofocus | ||
| bind:value={store.form.name} | ||
| /> | ||
| </div> | ||
| <div class="form-field"> | ||
| <label class="form-field-label" for="auth-key-expires"> | ||
| Expires <span class="form-hint">(optional, valid through the selected date)</span> | ||
| </label> | ||
| <input id="auth-key-expires" type="date" bind:value={store.form.expires_at} /> | ||
| </div> | ||
| </div> | ||
| <div class="form-field"> | ||
| <InlineHelpSection copyId="auth-key-user-path-help-copy" label="API key user path help"> | ||
| {#snippet title()} | ||
| <label class="form-field-label" for="auth-key-user-path">User Path (optional)</label> | ||
| {/snippet} | ||
| {#snippet help()} | ||
| When set, this key overrides the configured user path request | ||
| header for audit logging and downstream request context. | ||
| {/snippet} | ||
| </InlineHelpSection> | ||
| <input | ||
| id="auth-key-user-path" | ||
| type="text" | ||
| placeholder="ex. /department1/team-a" | ||
| aria-describedby="auth-key-user-path-help-copy" | ||
| bind:value={store.form.user_path} | ||
| /> | ||
| </div> | ||
| <div class="form-field"> | ||
| <InlineHelpSection copyId="auth-key-labels-help-copy" label="API key labels help"> | ||
| {#snippet title()} | ||
| <label class="form-field-label" for="auth-key-labels"> | ||
| Labels (optional, comma-separated) | ||
| </label> | ||
| {/snippet} | ||
| {#snippet help()} | ||
| Every request authenticated with this key gets these labels, in | ||
| addition to any labels from tagging headers. Labels show up in | ||
| usage analytics, the request log, and audit logs. | ||
| {/snippet} | ||
| </InlineHelpSection> | ||
| <input | ||
| id="auth-key-labels" | ||
| type="text" | ||
| placeholder="ex. team-a, batch-jobs" | ||
| aria-describedby="auth-key-labels-help-copy" | ||
| bind:value={store.form.labels} | ||
| /> | ||
| </div> | ||
| <div class="form-field"> | ||
| <InlineHelpSection copyId="auth-key-dashboard-access-help-copy" label="API key dashboard access help"> | ||
| {#snippet title()} | ||
| <label class="form-field-label" for="auth-key-dashboard-access">Dashboard access</label> | ||
| {/snippet} | ||
| {#snippet help()} | ||
| When off, this key is denied the dashboard and every /admin API | ||
| endpoint. Model endpoints and GET /v1/usage stay available to | ||
| the key. The master key always has dashboard access. | ||
| {/snippet} | ||
| </InlineHelpSection> | ||
| <label class="auth-key-dashboard-toggle"> | ||
| <input | ||
| id="auth-key-dashboard-access" | ||
| type="checkbox" | ||
| aria-describedby="auth-key-dashboard-access-help-copy" | ||
| bind:checked={store.form.dashboard_access} | ||
| /> | ||
| <span>Allow this key to use the dashboard and /admin API</span> | ||
| </label> | ||
| </div> | ||
| <div class="form-field"> | ||
| <label class="form-field-label" for="auth-key-description">Description (optional)</label> | ||
| <textarea | ||
| id="auth-key-description" | ||
| rows="2" | ||
| placeholder="What is this key used for?" | ||
| bind:value={store.form.description} | ||
| ></textarea> | ||
| </div> | ||
| {#if store.error} | ||
| <p class="form-error" role="alert" aria-live="assertive">{store.error}</p> | ||
| {/if} | ||
| <div class="form-actions"> | ||
| <button | ||
| type="submit" | ||
| class="btn btn-primary btn-with-icon" | ||
| disabled={store.formSubmitting} | ||
| > | ||
| {#if !store.formSubmitting} | ||
| <span aria-hidden="true"><Icon name="plus" class="table-icon-svg" /></span> | ||
| {/if} | ||
| <span>{store.formSubmitting ? "Creating..." : "Create API Key"}</span> | ||
| </button> | ||
| </div> | ||
| <div class="form-field"> | ||
| <label class="form-field-label" for="auth-key-expires"> | ||
| Expires <span class="form-hint">(optional, valid through the selected date)</span> | ||
| </label> | ||
| <input id="auth-key-expires" type="date" bind:value={store.form.expires_at} /> | ||
| </div> | ||
| {/if} | ||
| </form> | ||
| </div> | ||
| </Modal> | ||
| </div> | ||
| <div class="form-field"> | ||
| <InlineHelpSection copyId="auth-key-user-path-help-copy" label="API key user path help"> | ||
| {#snippet title()} | ||
| <label class="form-field-label" for="auth-key-user-path">User Path (optional)</label> | ||
| {/snippet} | ||
| {#snippet help()} | ||
| When set, this key overrides the configured user path request | ||
| header for audit logging and downstream request context. | ||
| {/snippet} | ||
| </InlineHelpSection> | ||
| <input | ||
| id="auth-key-user-path" | ||
| type="text" | ||
| placeholder="ex. /department1/team-a" | ||
| aria-describedby="auth-key-user-path-help-copy" | ||
| bind:value={store.form.user_path} | ||
| /> | ||
| </div> | ||
| <div class="form-field"> | ||
| <InlineHelpSection copyId="auth-key-labels-help-copy" label="API key labels help"> | ||
| {#snippet title()} | ||
| <label class="form-field-label" for="auth-key-labels"> | ||
| Labels (optional, comma-separated) | ||
| </label> | ||
| {/snippet} | ||
| {#snippet help()} | ||
| Every request authenticated with this key gets these labels, in | ||
| addition to any labels from tagging headers. Labels show up in | ||
| usage analytics, the request log, and audit logs. | ||
| {/snippet} | ||
| </InlineHelpSection> | ||
| <input | ||
| id="auth-key-labels" | ||
| type="text" | ||
| placeholder="ex. team-a, batch-jobs" | ||
| aria-describedby="auth-key-labels-help-copy" | ||
| bind:value={store.form.labels} | ||
| /> | ||
| </div> | ||
| <div class="form-field"> | ||
| <InlineHelpSection copyId="auth-key-dashboard-access-help-copy" label="API key dashboard access help"> | ||
| {#snippet title()} | ||
| <label class="form-field-label" for="auth-key-dashboard-access">Dashboard access</label> | ||
| {/snippet} | ||
| {#snippet help()} | ||
| When off, this key is denied the dashboard and every /admin API | ||
| endpoint. Model endpoints and GET /v1/usage stay available to | ||
| the key. The master key always has dashboard access. | ||
| {/snippet} | ||
| </InlineHelpSection> | ||
| <label class="auth-key-dashboard-toggle"> | ||
| <input | ||
| id="auth-key-dashboard-access" | ||
| type="checkbox" | ||
| aria-describedby="auth-key-dashboard-access-help-copy" | ||
| bind:checked={store.form.dashboard_access} | ||
| /> | ||
| <span>Allow this key to use the dashboard and /admin API</span> | ||
| </label> | ||
| </div> | ||
| <FormField id="auth-key-description" label="Description (optional)"> | ||
| <textarea | ||
| id="auth-key-description" | ||
| rows="2" | ||
| placeholder="What is this key used for?" | ||
| bind:value={store.form.description} | ||
| ></textarea> | ||
| </FormField> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Mixed field markup in one file.
Lines 48-127 hand-roll .form-field wrappers while line 128 uses FormField. Where the label is a plain string (name, expires), FormField applies; the InlineHelpSection cases genuinely can't. Consider converting the convertible ones so the shared molecule is the default here too.
🤖 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 `@web/dashboard/src/pages/auth-keys/AuthKeyEditor.svelte` around lines 48 -
135, Update AuthKeyEditor’s name and expires fields to use the shared FormField
component instead of hand-rolled .form-field wrappers, preserving their existing
labels, required/optional hints, input bindings, IDs, and attributes. Keep the
InlineHelpSection-based fields unchanged because their custom label/help
structure cannot use FormField directly.
Comment on lines
151
to
+152
| this.available = true; | ||
| if (!result.ok) { | ||
| this.types = []; | ||
| this.types = outcome.items; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Network failures flip available back to true.
loadAdminList returns { status: "error", result: null } for a thrown request (offline/DNS/abort). Both fetchers set this.available = true before checking outcome.result, so an offline reload after a 503 replaces the "feature unavailable" state with an empty list plus a load error. The sibling stores (authKeys.svelte.js lines 66-73, mcpServers.svelte.js lines 77-86, providersConfig.svelte.js lines 111-119) deliberately gate the flag on a gateway having actually answered.
🐛 Proposed fix: only trust a real gateway response
- this.available = true;
+ if (outcome.result) {
+ this.available = true;
+ }
this.types = outcome.items;
if (outcome.status === "error") {
this.error = outcome.error;
return;
}- this.available = true;
+ if (outcome.result) {
+ this.available = true;
+ }
this.guardrails = outcome.items;
this.error = outcome.error;Also applies to: 185-187
🤖 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 `@web/dashboard/src/pages/guardrails/guardrails.svelte.js` around lines 151 -
152, Update both fetcher paths around the assignments to this.available and
this.types so available is set to true only when loadAdminList returns a
non-null result, indicating the gateway responded. Preserve the unavailable
state for thrown requests with result: null, while retaining the existing item
assignment and error handling for valid responses.
Comment on lines
+10
to
+21
| <!-- novalidate: the concurrent period keeps 0 in the hidden period-seconds | ||
| input (min=1), and native validation silently blocks submit on hidden | ||
| invalid controls. Validation lives in rateLimitFormPayload(). --> | ||
| <EditorDialog | ||
| open={rateLimits.rateLimitFormOpen} | ||
| title={rateLimits.rateLimitEditing ? "Edit Rate Limit" : "Create Rate Limit"} | ||
| ariaLabel="Rate limit editor" | ||
| error={rateLimits.rateLimitFormError} | ||
| submitting={rateLimits.rateLimitFormSubmitting} | ||
| submitLabel="Save Rate Limit" | ||
| dialogClass="budget-editor" | ||
| novalidate |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Stale novalidate rationale.
The comment describes a hidden period-seconds input holding 0, but line 65 removes that input from the DOM entirely unless period === "custom", so no hidden invalid control exists. Update the justification (or drop novalidate if it is no longer needed) so the next reader isn't chasing a condition that can't occur.
🤖 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 `@web/dashboard/src/pages/rate-limits/RateLimitEditor.svelte` around lines 10 -
21, Update the comment above EditorDialog in RateLimitEditor.svelte to reflect
the current form behavior: period-seconds is removed from the DOM unless period
is "custom", so it is not a hidden invalid control. If rateLimitFormPayload() no
longer requires disabling native validation, remove the novalidate attribute;
otherwise retain it with an accurate justification.
Comment on lines
+184
to
+193
| <input | ||
| type="number" | ||
| class="workflow-step-input" | ||
| id={"workflow-guardrail-step-" + index} | ||
| min="0" | ||
| step="10" | ||
| placeholder="Step" | ||
| bind:value={step.step} | ||
| aria-label={"Guardrail step " + (index + 1)} | ||
| /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
step="10" constrains guardrail step numbers to multiples of 10.
min="0" step="10" makes any other integer (e.g. 15) fail native constraint validation. Since this form is submitted without novalidate, the submit is rejected by the browser rather than by wf.submitForm(). If arbitrary ordering values are allowed, use step="1" (or step="any") and keep 10 only as the increment hint.
🐛 Proposed fix
min="0"
- step="10"
+ step="1"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <input | |
| type="number" | |
| class="workflow-step-input" | |
| id={"workflow-guardrail-step-" + index} | |
| min="0" | |
| step="10" | |
| placeholder="Step" | |
| bind:value={step.step} | |
| aria-label={"Guardrail step " + (index + 1)} | |
| /> | |
| <input | |
| type="number" | |
| class="workflow-step-input" | |
| id={"workflow-guardrail-step-" + index} | |
| min="0" | |
| step="1" | |
| placeholder="Step" | |
| bind:value={step.step} | |
| aria-label={"Guardrail step " + (index + 1)} | |
| /> |
🤖 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 `@web/dashboard/src/pages/workflows/WorkflowEditor.svelte` around lines 184 -
193, Update the guardrail step number input in the workflow editor to allow
arbitrary integer ordering values by changing the step constraint from 10 to 1,
while preserving the existing nonnegative minimum and binding.
| this.error = isAbortError(e) | ||
| return; | ||
| } | ||
| this.available = true; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
available is flipped back to true on a network failure. loadAdminList returns { status: "error", result: null } when the request throws (offline, DNS, watchdog abort), so setting the flag before inspecting outcome.result undoes an earlier 503-driven available = false. Same root cause as the guardrails fetchers flagged in web/dashboard/src/pages/guardrails/guardrails.svelte.js.
web/dashboard/src/pages/workflows/workflows.svelte.js#L230-L230: wrapthis.available = trueinif (outcome.result)so a thrown request leaves the flag as-is; the timeout/error branch below already clearsworkflowsand setserror.web/dashboard/src/pages/guardrails/guardrails.svelte.js#L151-L152,#L185-L187``: apply the sameoutcome.resultguard in `fetchTypes` and `fetchGuardrails`.
🤖 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 `@web/dashboard/src/pages/workflows/workflows.svelte.js` at line 230, Guard
availability updates on successful fetch results so network failures do not
restore availability after a 503. In the workflows load flow, wrap the existing
this.available = true in an outcome.result check; apply the same guard to the
availability assignments in guardrails fetchTypes and fetchGuardrails, leaving
their existing error handling unchanged.
| @@ -0,0 +1,32 @@ | |||
| body { | |||
| font-family: | |||
| "Inter", | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove quotes around Inter to satisfy Stylelint.
🧰 Tools
🪛 Stylelint (17.14.1)
[error] 3-3: Expected no quotes around "Inter" (font-family-name-quotes)
(font-family-name-quotes)
🤖 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 `@web/dashboard/src/styles/base.css` at line 3, In the font-family declaration
containing “Inter”, remove the quotation marks around the Inter font name while
preserving the existing fallback fonts and declaration structure.
Source: Linters/SAST tools
| var(--bg-surface-hover) 72%, | ||
| #fff 28% | ||
| ); | ||
| color-scheme: dark; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the three Stylelint declaration-spacing errors.
Add an empty line before each color-scheme declaration so this new stylesheet passes the configured lint rule.
Also applies to: 125-125, 173-173
🧰 Tools
🪛 Stylelint (17.14.1)
[error] 77-77: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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 `@web/dashboard/src/styles/themes.css` at line 77, Add a blank line immediately
before each of the three color-scheme declarations in themes.css, including the
declarations near the referenced locations, to satisfy the configured Stylelint
declaration-spacing rule.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Front-end refactoring pass over the Svelte 5 dashboard:
dashboard.css(2,453 lines) split into 14 ordered modules undersrc/styles/;dashboard.cssis now an@importentry whose order preserves the cascade. Pure partition: the emitted CSS bundle is byte-identical. No preprocessor added on purpose — scoped styles + custom properties + native nesting cover the need with zero new dependencies.EditorDialogorganism +FormFieldmolecule +EnabledToggleatom replace the modal shell all 11 editors hand-rolled. Also makes the Escape-under-auth-dialog guard uniform (five editors previously discarded the form when Escape was pressed under the auth dialog).$lib/api/adminCrud.js(loadAdminList/sendAdminMutation) replaces the fetch/submit/delete guard branches seven CRUD stores duplicated, applying the guards in the one correct order: stale first (a stale response never touches state — some stores previously checked 503 first and could clobber the availability flag from an old API key's response), then unavailable, then errors with silent-401 loads.ProviderStatusCardDetails), ConversationDrawer 377→147 (+ChatMessage), AuditEntrySummary 328→272 (+AuditAttemptTrack, hand-rolled chevron →Iconatom), Sidebar nav items →navigation.js, SummaryCards deduped with local snippets. All splits respect the scope-hash rules in CONVENTIONS.md;WorkflowChartdeliberately not split (computed-class CSS the compiler cannot see).EditorDialog, never hand-roll the shell" rule; embedded dist rebuilt.Note on history: a TypeScript migration of
$libwas made and then reverted within this branch (ecc2e8ca) — deemed too much for one iteration; the tip is plain JS throughout. Squash-merge leaves no TS in the history ofmain.User-visible impact
None intended. Two accepted normalizations: form error banners render uniformly just above the actions row, and AuthKeyEditor's "Done, I've stored it" action moved into the standard footer. Behavior fixes: uniform Escape guard under the auth dialog; stale responses can no longer flip a page's availability flag.
Verification
npm run check— 0 errorsnpm test— 386/386npm run build— clean; emitted CSS byte-identical after the style split; dist in sync (pre-commit hook verified on the tip commit)🤖 Generated with Claude Code
Summary by CodeRabbit