refactor(dashboard): polish guardrails admin forms#247
Merged
SantiagoDePolonia merged 6 commits intomainfrom Apr 21, 2026
Merged
refactor(dashboard): polish guardrails admin forms#247SantiagoDePolonia merged 6 commits intomainfrom
SantiagoDePolonia merged 6 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughReorganized the admin dashboard into page-specific templates, replaced scroll-into-view editor behavior with deferred focus handlers, added structured Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR polishes the guardrails admin UI as part of a broader dashboard refactor: admin forms are lifted out of inline sections and into modal overlays, HTTP error messages from the server are now surfaced inside the active form, a new shared Key changes:
Confidence Score: 4/5Safe to merge — all critical paths work correctly; the two flagged issues are non-blocking UX and defensive-programming concerns. The refactoring is well-structured: Go-side logging is clean, template splitting is correct, the modal pattern is consistent across guardrails and auth-keys pages, and the helper-disclosure rename is fully applied. The two P2 issues (autofocus no-op on disabled input in edit mode; write ops not using stale-auth guard) don't break functionality — the edit form is still usable and 401s are handled explicitly. No security or data-loss concerns. guardrails.js — autofocus selector and stale-auth handling in write operations. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks Create / Edit Guardrail] --> B{guardrailFormMode}
B -- create --> C[openGuardrailCreate\nresets form + sets defaults]
B -- edit --> D[openGuardrailEdit\nloads existing data]
C --> E[guardrailFormOpen = true]
D --> E
E --> F[focusGuardrailForm\nquerySelector autofocus field]
F --> G[Modal overlay rendered\neditor-modal-shell]
G --> H{User action}
H -- types + submits --> I[submitGuardrailForm\nPUT /admin/api/v1/guardrails/:name]
H -- clicks backdrop or Escape --> J[closeGuardrailForm]
H -- clicks Cancel --> J
I --> K{HTTP status}
K -- 200 --> L[fetchGuardrails\nguardrailNotice set\ncloseGuardrailForm]
K -- 400/422 --> M[guardrailResponseMessage\nreads error.message\nguardrailError shown in form]
K -- 401 --> N[authError = true\nneedsAuth = true]
K -- 503 --> O[guardrailsAvailable = false]
H -- clicks Delete --> P[deleteGuardrail\nDELETE /admin/api/v1/guardrails/:name]
P --> Q{HTTP status}
Q -- 204 --> R[fetchGuardrails\nguardrailNotice set]
Q -- 400/404 --> S[guardrailResponseMessage\nguardrailError shown outside form]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+196
to
+222
| const focus = () => { | ||
| const refs = this.$refs || {}; | ||
| const editor = refs.guardrailEditor || null; | ||
| if (!editor || typeof editor.querySelector !== 'function') { | ||
| return; | ||
| } | ||
| const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); | ||
| if (!field || typeof field.focus !== 'function') { | ||
| return; | ||
| } | ||
| field.focus({ preventScroll: true }); | ||
| }; | ||
|
|
||
| const focusAfterPaint = () => { | ||
| if (typeof global.requestAnimationFrame === 'function') { | ||
| global.requestAnimationFrame(focus); | ||
| return; | ||
| } | ||
| focus(); | ||
| }; | ||
|
|
||
| if (typeof this.$nextTick === 'function') { | ||
| this.$nextTick(focusAfterPaint); | ||
| return; | ||
| } | ||
| focusAfterPaint(); | ||
| }, |
There was a problem hiding this comment.
focusGuardrailForm uses the selector [data-modal-autofocus] to find the first focusable field. In edit mode the name <input> is disabled but still carries the data-modal-autofocus attribute (see page-guardrails.html line 74), so querySelector('[data-modal-autofocus], ...]') returns the disabled element first — and focus() on a disabled element is a browser no-op. The user opens an edit form and no field receives focus, forcing them to click or Tab before they can start typing.
To fix this, exclude disabled elements from the [data-modal-autofocus] match:
Suggested change
| const focus = () => { | |
| const refs = this.$refs || {}; | |
| const editor = refs.guardrailEditor || null; | |
| if (!editor || typeof editor.querySelector !== 'function') { | |
| return; | |
| } | |
| const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); | |
| if (!field || typeof field.focus !== 'function') { | |
| return; | |
| } | |
| field.focus({ preventScroll: true }); | |
| }; | |
| const focusAfterPaint = () => { | |
| if (typeof global.requestAnimationFrame === 'function') { | |
| global.requestAnimationFrame(focus); | |
| return; | |
| } | |
| focus(); | |
| }; | |
| if (typeof this.$nextTick === 'function') { | |
| this.$nextTick(focusAfterPaint); | |
| return; | |
| } | |
| focusAfterPaint(); | |
| }, | |
| const field = editor.querySelector('[data-modal-autofocus]:not([disabled]), input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); |
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/admin/dashboard/static/css/dashboard.css (1)
295-332: 🧹 Nitpick | 🔵 TrivialDuplicate rules: fold editor-modal into auth-dialog shells.
.editor-modal-backdrop(295–300) is byte-for-byte identical to.auth-dialog-backdrop(288–293), and.editor-modal-shell(311–319) duplicates.auth-dialog-shell(302–309) apart fromoverflow-y: auto. The mobile override at 3565–3568 likewise repeats 3561–3564. Combine the selectors to prevent drift between the two modal families:♻️ Proposed consolidation
-.auth-dialog-backdrop { - position: fixed; - inset: 0; - background: rgba(0, 0, 0, 0.48); - z-index: 80; -} - -.editor-modal-backdrop { +.auth-dialog-backdrop, +.editor-modal-backdrop { position: fixed; inset: 0; background: rgba(0, 0, 0, 0.48); z-index: 80; } -.auth-dialog-shell { +.auth-dialog-shell, +.editor-modal-shell { position: fixed; inset: 0; z-index: 90; display: grid; place-items: center; padding: 20px; } -.editor-modal-shell { - position: fixed; - inset: 0; - z-index: 90; - display: grid; - place-items: center; - padding: 20px; +.editor-modal-shell { overflow-y: auto; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/css/dashboard.css` around lines 295 - 332, The CSS duplicates for modal backdrops and shells should be consolidated: replace the two identical backdrop rules (.editor-modal-backdrop and .auth-dialog-backdrop) with a single combined selector (.editor-modal-backdrop, .auth-dialog-backdrop) and collapse the shared shell properties into one combined selector (.editor-modal-shell, .auth-dialog-shell) keeping only the common declarations; then add the editor-specific overflow override separately (e.g., .editor-modal-shell { ...common... } and .editor-modal-shell { overflow-y: auto; } or a combined selector with a later rule for .editor-modal-shell) and do the same consolidation for the mobile override rules to avoid repeating the same declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 1688-1697: The CSS uses an overly generic selector set (.form,
.form-kicker, .form-hint etc.) that risks collisions; rename these selectors to
a more specific editor-scoped prefix (e.g., change .form → .editor-form and
likewise .form-kicker → .editor-form-kicker, .form-hint → .editor-form-hint) and
update all related variants mentioned (e.g., .form-grid, .form-field,
.form-error, .form-actions) to the same .editor-form-* pattern to match existing
.editor-modal-* / .editor-header naming; ensure you update every occurrence in
the stylesheet so class names remain consistent with the editor modal
components.
- Around line 1359-1373: The bare selector input:is([type="text"],
[type="date"], [type="number"]) is applying width:100% and other form styles
globally and also omits other common input types; restrict the rule to the
intended scope (for example replace the bare selector with a scoped one like
.form input:is([type="text"],[type="date"],[type="number"]) or
.editor-modal-shell input:is(...) so it only affects inputs inside form/editor
containers) and/or expand the type list to include
email/password/search/url/tel/time if a broader default look is desired; update
the matching focus rule input:is(...):focus the same way to keep styles
consistent.
In `@internal/admin/dashboard/static/js/modules/aliases.js`:
- Around line 478-489: In focusEditorField(refName) the single querySelector
with multiple comma-separated selectors can match the close button before the
intended [data-modal-autofocus] field; change the logic to first attempt a
focused element lookup using editor.querySelector('[data-modal-autofocus]') and,
only if that returns null, perform a second query with the fallback selector
(input:not([type="hidden"]):not([disabled]), textarea:not([disabled]),
select:not([disabled]), button:not([disabled]')); keep the existing guards
(editor existence and typeof field.focus === 'function') and call field.focus({
preventScroll: true }) on the chosen element so the modal reliably focuses the
intended field.
In `@internal/admin/dashboard/static/js/modules/guardrails.js`:
- Around line 195-222: The current focusGuardrailForm() uses a single
querySelector list that lets the close button match before the intended field;
update the logic in focusGuardrailForm() so it first attempts
editor.querySelector('[data-modal-autofocus]') and, if that returns null or is
not focusable, then falls back to
editor.querySelector('input:not([type="hidden"]):not([disabled]),
textarea:not([disabled]), select:not([disabled])') — do not include generic
button selectors in the fallback. Keep the existing focus(), focusAfterPaint(),
and $nextTick handling but change the selector checks and focusability tests
accordingly; apply the same selector-change fix to the analogous functions in
workflows.js and aliases.js (the functions that mirror focusGuardrailForm's
behavior).
In `@internal/admin/dashboard/static/js/modules/workflows.js`:
- Around line 483-508: The focusWorkflowForm method duplicates shared logic
already implemented in focusEditorField(refName); replace the body of
focusWorkflowForm by delegating to the shared helper (call
this.focusEditorField('workflowEditor')) and remove any redundant local
implementation; do the same for other duplicated helpers like focusGuardrailForm
and focusAliasForm so they simply call focusEditorField with the appropriate ref
name, ensuring existing usage of this.$nextTick/requestAnimationFrame continues
to be handled by the centralized focusEditorField implementation.
In `@internal/admin/dashboard/templates/page-auth-keys.html`:
- Around line 47-49: This close button embeds the raw "✕" glyph instead of using
the shared template; replace the glyph inside the button in the template
fragment that contains the alias-close-btn and the `@click`="closeAuthKeyForm()"
with the shared Handlebars template invocation {{template "x-icon"}} so the
button uses the same icon SVG/styling as other modals (referenced elsewhere in
page-workflows.html and page-guardrails.html) and preserves sizing, stroke
weight and theming consistency.
In `@internal/admin/dashboard/templates/page-guardrails.html`:
- Around line 58-70: The guardrail editor uses a <div class="form"> which
prevents native Enter-to-submit and browser validation; change that container to
a real <form> and attach the submit handler via
`@submit.prevent`="submitGuardrailForm()" (keep the submit button as type="submit"
and remove its `@click`), ensure the close button (closeGuardrailForm) remains
type="button" so it doesn't trigger submit, and leave existing bindings like
guardrailFormMode and guardrailError unchanged so validation messages and header
logic continue to work.
- Around line 72-84: The modal autofocus currently targets the name input via
the data-modal-autofocus attribute while the input is disabled when
guardrailFormMode === 'edit', so move or make the autofocus conditional: either
place data-modal-autofocus on a non-disabled field such as the
guardrail-description or the first type-specific field, or change the attribute
to be conditional (e.g., bind it based on guardrailFormMode) so focusEditorField
doesn't call .focus() on a disabled element; ensure the same change is applied
to the guardrail-type <select> (which uses x-ref="guardrailTypeSelect" and is
tied to guardrailForm.type, guardrailTypes, syncGuardrailTypeSelectValue, and
onGuardrailTypeChange) if it could be targeted for autofocus.
In `@internal/admin/dashboard/templates/page-overview.html`:
- Around line 8-13: The interval-picker buttons (elements with class
"interval-btn" that call setInterval('daily'|'weekly'|'monthly'|'yearly') and
bind to interval) lack an explicit type attribute; add type="button" to each of
those four <button> elements so they don't default to "submit" if moved into a
form, matching the rest of the template pattern and avoiding future regressions.
In `@internal/admin/dashboard/templates/page-settings.html`:
- Around line 24-33: Add the missing aria-describedby attributes to wire the
inline help copy to the controls: update the select with id
"timezone-override-select" (x-ref "timezoneOverrideSelect", x-model
"timezoneOverride", using timezoneOptions and syncTimezoneOverrideSelectValue)
to include aria-describedby="timezone-help-copy", and likewise add
aria-describedby="runtime-refresh-help-copy" to the runtime-refresh button
referenced near the runtime refresh logic so screen readers announce the
corresponding help regions.
In `@internal/admin/dashboard/templates/page-usage.html`:
- Around line 131-156: The search input bound to usageLogSearch in the template
currently only uses a placeholder (input with x-model="usageLogSearch") which is
not an accessible name; add an explicit aria-label (e.g., aria-label="Search by
request ID, model, provider") to that input to match the pattern used by the
usageLogUserPath control and ensure screen readers can identify the field,
keeping the existing x-model, `@input.debounce.300ms`="fetchUsageLog(true)", and
class attributes unchanged.
In `@internal/admin/handler_test.go`:
- Around line 1934-2010: Add a new unit test that exercises the X-Request-ID
fallback by calling handleError with an HTTP request that does NOT have a
context request ID but DOES have the "X-Request-ID" header set; mirror the
existing
TestHandleError_LogsClientErrorsAtWarnLevel/TestHandleError_LogsServerErrorsAtErrorLevel
pattern (create buf, set slog.Default to JSON handler, build echo.Context via
e.NewContext), set req.Header.Set("X-Request-ID","admin-header-req-789"), call
handleError (e.g., pass core.NewInvalidRequestError or core.NewProviderError),
assert response code and verify the JSON log contains the header value
`"request_id":"admin-header-req-789"` along with the expected
level/msg/path/provider/message fields.
---
Outside diff comments:
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 295-332: The CSS duplicates for modal backdrops and shells should
be consolidated: replace the two identical backdrop rules
(.editor-modal-backdrop and .auth-dialog-backdrop) with a single combined
selector (.editor-modal-backdrop, .auth-dialog-backdrop) and collapse the shared
shell properties into one combined selector (.editor-modal-shell,
.auth-dialog-shell) keeping only the common declarations; then add the
editor-specific overflow override separately (e.g., .editor-modal-shell {
...common... } and .editor-modal-shell { overflow-y: auto; } or a combined
selector with a later rule for .editor-modal-shell) and do the same
consolidation for the mobile override rules to avoid repeating the same
declarations.
🪄 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
Run ID: e203e7cf-5f81-4541-9368-e3f5903c0b21
📒 Files selected for processing (28)
internal/admin/dashboard/dashboard.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/aliases.test.jsinternal/admin/dashboard/static/js/modules/auth-keys.jsinternal/admin/dashboard/static/js/modules/auth-keys.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/guardrails.test.jsinternal/admin/dashboard/static/js/modules/timezone-layout.test.jsinternal/admin/dashboard/static/js/modules/workflows-layout.test.jsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.jsinternal/admin/dashboard/templates/helper-disclosure.htmlinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/inline-help-toggle.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-audit-logs.htmlinternal/admin/dashboard/templates/page-auth-keys.htmlinternal/admin/dashboard/templates/page-guardrails.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/dashboard/templates/page-overview.htmlinternal/admin/dashboard/templates/page-settings.htmlinternal/admin/dashboard/templates/page-usage.htmlinternal/admin/dashboard/templates/page-workflows.htmlinternal/admin/handler.gointernal/admin/handler_test.go
💤 Files with no reviewable changes (1)
- internal/admin/dashboard/templates/helper-disclosure.html
Comment on lines
+1359
to
+1373
| input:is([type="text"], [type="date"], [type="number"]) { | ||
| width: 100%; | ||
| padding: 8px 12px; | ||
| background: var(--bg-surface); | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--radius); | ||
| color: var(--text); | ||
| font-size: 13px; | ||
| font-family: inherit; | ||
| outline: none; | ||
| } | ||
|
|
||
| input:is([type="text"], [type="date"], [type="number"]):focus { | ||
| border-color: var(--accent); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Bare input type selector is too broad and narrow at the same time.
Switching from a class-scoped rule to the bare selector input:is([type="text"], [type="date"], [type="number"]) forces width: 100% (plus padding/border/background) on every matching input on the page — including any inline/filter/date-picker inputs that previously relied on intrinsic sizing or other class rules. This can silently regress layouts outside the editor modals.
It is also inconsistent across input types: email, password, search, url, tel, and time inputs (which are likely to appear in admin forms) won't pick up the shared look, producing visual drift.
Consider scoping this to form containers (e.g., .form input[type=...], .editor-modal-shell input[type=...]) or widening the type list, rather than applying a body-wide default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/css/dashboard.css` around lines 1359 - 1373,
The bare selector input:is([type="text"], [type="date"], [type="number"]) is
applying width:100% and other form styles globally and also omits other common
input types; restrict the rule to the intended scope (for example replace the
bare selector with a scoped one like .form
input:is([type="text"],[type="date"],[type="number"]) or .editor-modal-shell
input:is(...) so it only affects inputs inside form/editor containers) and/or
expand the type list to include email/password/search/url/tel/time if a broader
default look is desired; update the matching focus rule input:is(...):focus the
same way to keep styles consistent.
Comment on lines
+1688
to
1697
| .form h3 { | ||
| font-size: 20px; | ||
| font-weight: 700; | ||
| } | ||
|
|
||
| .alias-form-kicker, | ||
| .alias-form-hint { | ||
| .form-kicker, | ||
| .form-hint { | ||
| color: var(--text-muted); | ||
| font-size: 13px; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
.form is an overly generic class name.
A top-level .form selector (and .form-grid, .form-field, .form-error, .form-actions, …) is extremely likely to collide with future markup or third-party snippets embedded in the dashboard. Given these rules are tightly coupled to the editor modals, consider a more specific prefix such as .editor-form / .editor-form-grid / .editor-form-error to match the new .editor-modal-* / .editor-header naming introduced in this PR.
Also applies to: 1784-1788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/css/dashboard.css` around lines 1688 - 1697,
The CSS uses an overly generic selector set (.form, .form-kicker, .form-hint
etc.) that risks collisions; rename these selectors to a more specific
editor-scoped prefix (e.g., change .form → .editor-form and likewise
.form-kicker → .editor-form-kicker, .form-hint → .editor-form-hint) and update
all related variants mentioned (e.g., .form-grid, .form-field, .form-error,
.form-actions) to the same .editor-form-* pattern to match existing
.editor-modal-* / .editor-header naming; ensure you update every occurrence in
the stylesheet so class names remain consistent with the editor modal
components.
Comment on lines
+478
to
+489
| focusEditorField(refName) { | ||
| const focus = () => { | ||
| const refs = this.$refs || {}; | ||
| const editor = refs[refName] || null; | ||
| if (!editor || typeof editor.querySelector !== 'function') { | ||
| return; | ||
| } | ||
| const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); | ||
| if (!field || typeof field.focus !== 'function') { | ||
| return; | ||
| } | ||
| field.focus({ preventScroll: true }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MDN querySelector selector list first element document order
💡 Result:
Yes, document.querySelector with a selector list (comma-separated selectors, e.g., "p, div, pre") returns the first matching element in document order. It performs a depth-first pre-order traversal starting from the first element in the document's markup, returning the first match found across all selectors, regardless of selector order in the list.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector
- 2: https://developer.mozilla.org/en-US/docs/Web/API/document/querySelector
- 3: https://developer.mozilla.org/en-US/docs/DOM/Document.querySelector
- 4: https://stackoverflow.com/questions/5684811/in-queryselector-how-to-get-the-first-and-get-the-last-elements-what-traversal
- 5: https://stackoverflow.com/questions/35924268/if-a-selector-in-a-queryselector-call-matches-multiple-nodes-which-one-is-retur
🏁 Script executed:
# Check if the file exists and get the line count
wc -l internal/admin/dashboard/static/js/modules/aliases.jsRepository: ENTERPILOT/GoModel
Length of output: 120
🏁 Script executed:
# Read the specific lines mentioned in the review (478-489)
sed -n '475,495p' internal/admin/dashboard/static/js/modules/aliases.jsRepository: ENTERPILOT/GoModel
Length of output: 1075
🏁 Script executed:
# Search for calls to focusEditorField to understand the impact
rg "focusEditorField" internal/admin/dashboard/static/js/modules/aliases.js -A 2 -B 2Repository: ENTERPILOT/GoModel
Length of output: 811
🏁 Script executed:
# Search for data-modal-autofocus in templates to see if it exists
fd -e vue -e html -e js | xargs rg "data-modal-autofocus" || echo "No data-modal-autofocus found"Repository: ENTERPILOT/GoModel
Length of output: 2114
🏁 Script executed:
# Find the template that contains aliasEditor ref
rg "x-ref=\"aliasEditor\"" internal/admin/dashboard/templates/ -B 5 -A 15Repository: ENTERPILOT/GoModel
Length of output: 2364
🏁 Script executed:
# Find the template that contains modelOverrideEditor ref
rg "x-ref=\"modelOverrideEditor\"" internal/admin/dashboard/templates/ -B 5 -A 15Repository: ENTERPILOT/GoModel
Length of output: 2495
Split autofocus and fallback lookup so the modal focuses the intended field.
The current implementation on line 485 does not prioritize [data-modal-autofocus] fields. A selector list in querySelector returns the first match in document order regardless of selector position. In the actual templates, the close button appears before the data-modal-autofocus input, so querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), ..., button:not([disabled])') will match and focus the close button instead of the form field, breaking keyboard navigation and accessibility.
Separate the autofocus lookup from fallback selectors to ensure the intended field receives focus.
♿ Proposed fix
focusEditorField(refName) {
const focus = () => {
const refs = this.$refs || {};
const editor = refs[refName] || null;
if (!editor || typeof editor.querySelector !== 'function') {
return;
}
- const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])');
+ const isFocusable = (candidate) => {
+ if (!candidate || candidate.disabled || candidate.hidden) {
+ return false;
+ }
+ if (candidate.getAttribute && candidate.getAttribute('aria-hidden') === 'true') {
+ return false;
+ }
+ if (candidate.type === 'hidden') {
+ return false;
+ }
+ if (typeof global.getComputedStyle === 'function') {
+ const style = global.getComputedStyle(candidate);
+ if (style.display === 'none' || style.visibility === 'hidden') {
+ return false;
+ }
+ }
+ return typeof candidate.focus === 'function';
+ };
+ const firstFocusable = (selector) => Array.from(editor.querySelectorAll(selector)).find(isFocusable);
+ const field = firstFocusable('[data-modal-autofocus]')
+ || firstFocusable('input:not([type="hidden"]), textarea, select, button');
if (!field || typeof field.focus !== 'function') {
return;
}
field.focus({ preventScroll: true });
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/modules/aliases.js` around lines 478 -
489, In focusEditorField(refName) the single querySelector with multiple
comma-separated selectors can match the close button before the intended
[data-modal-autofocus] field; change the logic to first attempt a focused
element lookup using editor.querySelector('[data-modal-autofocus]') and, only if
that returns null, perform a second query with the fallback selector
(input:not([type="hidden"]):not([disabled]), textarea:not([disabled]),
select:not([disabled]), button:not([disabled]')); keep the existing guards
(editor existence and typeof field.focus === 'function') and call field.focus({
preventScroll: true }) on the chosen element so the modal reliably focuses the
intended field.
Comment on lines
+195
to
+222
| focusGuardrailForm() { | ||
| const focus = () => { | ||
| const refs = this.$refs || {}; | ||
| const editor = refs.guardrailEditor || null; | ||
| if (!editor || typeof editor.querySelector !== 'function') { | ||
| return; | ||
| } | ||
| const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); | ||
| if (!field || typeof field.focus !== 'function') { | ||
| return; | ||
| } | ||
| field.focus({ preventScroll: true }); | ||
| }; | ||
|
|
||
| const focusAfterPaint = () => { | ||
| if (typeof global.requestAnimationFrame === 'function') { | ||
| global.requestAnimationFrame(focus); | ||
| return; | ||
| } | ||
| focus(); | ||
| }; | ||
|
|
||
| if (typeof this.$nextTick === 'function') { | ||
| this.$nextTick(focusAfterPaint); | ||
| return; | ||
| } | ||
| focusAfterPaint(); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the DOM, when Element.querySelector is called with a selector list, is the returned element selected by document order across all selectors, or by which selector in the list matches first?
💡 Result:
The returned element is selected by document order across all selectors, not by which selector in the list matches first. Element.querySelector (and Document.querySelector) performs a depth-first pre-order traversal of the DOM tree, starting from the first element in the document's markup. During this traversal, it checks each element against the entire selector list (comma-separated selectors treated as a group). The first element encountered that matches any selector in the list is returned. The order of selectors in the string does not affect the result; reversing the selector order yields the same element if the same matches exist in the DOM.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector.
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector
- 3: https://developer.mozilla.org/en-US/docs/DOM/Document.querySelector
- 4: https://stackoverflow.com/questions/35924268/if-a-selector-in-a-queryselector-call-matches-multiple-nodes-which-one-is-retur
- 5: https://www.tutorialpedia.org/blog/if-a-selector-in-a-queryselector-call-matches-multiple-nodes-which-one-is-returned/
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Document_object_model/Locating_DOM_elements_using_selectors
🏁 Script executed:
# Find the guardrails template file
fd -e html -e vue . internal/admin/dashboard/static | grep -i guardrailRepository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Find aliases.js and workflows.js to check for similar patterns
fd -e js . internal/admin/dashboard/static | grep -E "(aliases|workflows)\.js"Repository: ENTERPILOT/GoModel
Length of output: 171
🏁 Script executed:
# Search for the focusGuardrailForm method and related template structure
rg "data-modal-autofocus|guardrailEditor|focusGuardrailForm" internal/admin/dashboard/static --type js --type htmlRepository: ENTERPILOT/GoModel
Length of output: 1379
🏁 Script executed:
# Find the page-guardrails.html template file
fd -e html . internal/admin/dashboard | grep -i guardrailRepository: ENTERPILOT/GoModel
Length of output: 117
🏁 Script executed:
# Search for template files more broadly
fd -e html . internal/admin/dashboard/templatesRepository: ENTERPILOT/GoModel
Length of output: 1159
🏁 Script executed:
# Look for guardrails template file with different naming patterns
find internal/admin/dashboard -name "*guardrail*" -type fRepository: ENTERPILOT/GoModel
Length of output: 236
🏁 Script executed:
# Inspect the guardrails template to see editor structure and button/input order
cat internal/admin/dashboard/templates/page-guardrails.htmlRepository: ENTERPILOT/GoModel
Length of output: 15946
🏁 Script executed:
# Search for the focus functions in aliases and workflows modules
rg "focusAliasForm|focusWorkflowForm" internal/admin/dashboard/static/js/modules/ -A 20Repository: ENTERPILOT/GoModel
Length of output: 12310
🏁 Script executed:
# Check the templates for aliases and workflows
cat internal/admin/dashboard/templates/page-workflows.html | grep -A 30 "editor-header"Repository: ENTERPILOT/GoModel
Length of output: 2383
🏁 Script executed:
# Check aliases template
cat internal/admin/dashboard/templates/page-aliases.html 2>/dev/null | grep -A 30 "editor-header" || echo "File may not exist or pattern not found"Repository: ENTERPILOT/GoModel
Length of output: 101
🏁 Script executed:
# Find aliases template file
fd . internal/admin/dashboard/templates | grep -i aliasRepository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Search for focusAliasForm in aliases.js
rg "focusAliasForm" internal/admin/dashboard/static/js/modules/aliases.js -A 20Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Search for alias editors and templates more broadly
fd . internal/admin/dashboard/templates | head -20Repository: ENTERPILOT/GoModel
Length of output: 1112
🏁 Script executed:
# Search for the focus function calls and patterns in aliases.js
rg "querySelector.*data-modal-autofocus" internal/admin/dashboard/static/js/modules/aliases.js -B 5 -A 5Repository: ENTERPILOT/GoModel
Length of output: 707
🏁 Script executed:
# Check what template the aliases.js module is using
rg "refName|Editor" internal/admin/dashboard/static/js/modules/aliases.js | head -20Repository: ENTERPILOT/GoModel
Length of output: 391
🏁 Script executed:
# Look for page-models.html to see if it has similar editor structure
grep -A 30 "editor-header" internal/admin/dashboard/templates/page-models.html | head -40Repository: ENTERPILOT/GoModel
Length of output: 2377
Autofocus lands on the close button, not the first form field.
Element.querySelector() with a selector list returns the first element in document order that matches any selector in the list — not the first selector that has a match. In page-guardrails.html, the editor-header close <button> (not disabled) is rendered before the name input that carries data-modal-autofocus, so button:not([disabled]) wins and the close button receives focus on both create and edit opens. In edit mode this is worse: the name input is :disabled="guardrailFormMode === 'edit'", so it wouldn't be focusable anyway, and the close button is still the first match.
Resolve the intent explicitly — try the marker first, then fall back to form controls without including button:
🛠️ Proposed fix
focusGuardrailForm() {
const focus = () => {
const refs = this.$refs || {};
const editor = refs.guardrailEditor || null;
if (!editor || typeof editor.querySelector !== 'function') {
return;
}
- const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])');
+ const field = editor.querySelector('[data-modal-autofocus]:not([disabled])')
+ || editor.querySelector('input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled])');
if (!field || typeof field.focus !== 'function') {
return;
}
field.focus({ preventScroll: true });
};The same pattern exists in workflows.js and aliases.js — their templates also place close buttons before form fields with data-modal-autofocus, causing the same bug. Tests only stub querySelector to return a focus spy, so they don't catch which element was actually selected.
📝 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
| focusGuardrailForm() { | |
| const focus = () => { | |
| const refs = this.$refs || {}; | |
| const editor = refs.guardrailEditor || null; | |
| if (!editor || typeof editor.querySelector !== 'function') { | |
| return; | |
| } | |
| const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); | |
| if (!field || typeof field.focus !== 'function') { | |
| return; | |
| } | |
| field.focus({ preventScroll: true }); | |
| }; | |
| const focusAfterPaint = () => { | |
| if (typeof global.requestAnimationFrame === 'function') { | |
| global.requestAnimationFrame(focus); | |
| return; | |
| } | |
| focus(); | |
| }; | |
| if (typeof this.$nextTick === 'function') { | |
| this.$nextTick(focusAfterPaint); | |
| return; | |
| } | |
| focusAfterPaint(); | |
| }, | |
| focusGuardrailForm() { | |
| const focus = () => { | |
| const refs = this.$refs || {}; | |
| const editor = refs.guardrailEditor || null; | |
| if (!editor || typeof editor.querySelector !== 'function') { | |
| return; | |
| } | |
| const field = editor.querySelector('[data-modal-autofocus]:not([disabled])') | |
| || editor.querySelector('input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled])'); | |
| if (!field || typeof field.focus !== 'function') { | |
| return; | |
| } | |
| field.focus({ preventScroll: true }); | |
| }; | |
| const focusAfterPaint = () => { | |
| if (typeof global.requestAnimationFrame === 'function') { | |
| global.requestAnimationFrame(focus); | |
| return; | |
| } | |
| focus(); | |
| }; | |
| if (typeof this.$nextTick === 'function') { | |
| this.$nextTick(focusAfterPaint); | |
| return; | |
| } | |
| focusAfterPaint(); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/modules/guardrails.js` around lines 195 -
222, The current focusGuardrailForm() uses a single querySelector list that lets
the close button match before the intended field; update the logic in
focusGuardrailForm() so it first attempts
editor.querySelector('[data-modal-autofocus]') and, if that returns null or is
not focusable, then falls back to
editor.querySelector('input:not([type="hidden"]):not([disabled]),
textarea:not([disabled]), select:not([disabled])') — do not include generic
button selectors in the fallback. Keep the existing focus(), focusAfterPaint(),
and $nextTick handling but change the selector checks and focusability tests
accordingly; apply the same selector-change fix to the analogous functions in
workflows.js and aliases.js (the functions that mirror focusGuardrailForm's
behavior).
Comment on lines
+483
to
508
| focusWorkflowForm() { | ||
| const focus = () => { | ||
| const refs = this.$refs || {}; | ||
| const editor = refs.workflowEditor || null; | ||
| if (!editor || typeof editor.querySelector !== 'function') { | ||
| return; | ||
| } | ||
| const field = editor.querySelector('[data-modal-autofocus], input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); | ||
| if (!field || typeof field.focus !== 'function') { | ||
| return; | ||
| } | ||
| field.focus({ preventScroll: true }); | ||
| }; | ||
| const focusAfterPaint = () => { | ||
| if (typeof global.requestAnimationFrame === 'function') { | ||
| global.requestAnimationFrame(focus); | ||
| return; | ||
| } | ||
| focus(); | ||
| }; | ||
| if (typeof global.requestAnimationFrame === 'function') { | ||
| global.requestAnimationFrame(scroll); | ||
| if (typeof this.$nextTick === 'function') { | ||
| this.$nextTick(focusAfterPaint); | ||
| return; | ||
| } | ||
| scroll(); | ||
| focusAfterPaint(); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicated focus helper — consolidate with focusEditorField(refName).
Per the relevant snippet, aliases.js already defines a parameterized focusEditorField(refName) (line 478–506) that implements the exact same logic. Re-implementing it per-module duplicates non-trivial focus/$nextTick/requestAnimationFrame logic and drifts easily. Prefer calling the shared helper (e.g. this.focusEditorField('workflowEditor')) and delete focusWorkflowForm(). Same advice applies wherever focusGuardrailForm/focusAliasForm/etc. were introduced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/modules/workflows.js` around lines 483 -
508, The focusWorkflowForm method duplicates shared logic already implemented in
focusEditorField(refName); replace the body of focusWorkflowForm by delegating
to the shared helper (call this.focusEditorField('workflowEditor')) and remove
any redundant local implementation; do the same for other duplicated helpers
like focusGuardrailForm and focusAliasForm so they simply call focusEditorField
with the appropriate ref name, ensuring existing usage of
this.$nextTick/requestAnimationFrame continues to be handled by the centralized
focusEditorField implementation.
Comment on lines
+72
to
+84
| <div class="form-field"> | ||
| <label class="form-field-label" for="guardrail-name">Name</label> | ||
| <input id="guardrail-name" type="text" placeholder="safety-system-prompt" x-model="guardrailForm.name" :disabled="guardrailFormMode === 'edit'" data-modal-autofocus> | ||
| </div> | ||
|
|
||
| <div class="form-field"> | ||
| <label class="form-field-label" for="guardrail-type">Type</label> | ||
| <select id="guardrail-type" class="usage-log-select settings-select" x-ref="guardrailTypeSelect" x-model="guardrailForm.type" x-effect="guardrailTypes.length; guardrailForm.type; $nextTick(() => syncGuardrailTypeSelectValue())" :disabled="guardrailFormMode === 'edit'" @change="onGuardrailTypeChange()"> | ||
| <template x-for="typeDef in guardrailTypes" :key="typeDef.type"> | ||
| <option :value="typeDef.type" x-text="typeDef.label"></option> | ||
| </template> | ||
| </select> | ||
| </div> |
Contributor
There was a problem hiding this comment.
data-modal-autofocus targets a field that is :disabled in edit mode.
The shared focusEditorField selector matches [data-modal-autofocus] unconditionally, but the name input here is disabled when guardrailFormMode === 'edit'. Calling .focus() on a disabled element is a no-op in every major browser, so the edit modal opens with no focused field and keyboard users land nowhere useful. Options:
- Put
data-modal-autofocusonguardrail-description(or the first type-specific field) instead; or - Make the attribute conditional:
:data-modal-autofocus="guardrailFormMode === 'edit' ? null : true".
The same concern applies to the type <select> at Line 79 if it ever becomes the preferred autofocus target in edit mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-guardrails.html` around lines 72 -
84, The modal autofocus currently targets the name input via the
data-modal-autofocus attribute while the input is disabled when
guardrailFormMode === 'edit', so move or make the autofocus conditional: either
place data-modal-autofocus on a non-disabled field such as the
guardrail-description or the first type-specific field, or change the attribute
to be conditional (e.g., bind it based on guardrailFormMode) so focusEditorField
doesn't call .focus() on a disabled element; ensure the same change is applied
to the guardrail-type <select> (which uses x-ref="guardrailTypeSelect" and is
tied to guardrailForm.type, guardrailTypes, syncGuardrailTypeSelectValue, and
onGuardrailTypeChange) if it could be targeted for autofocus.
Comment on lines
+1934
to
+2010
| func TestHandleError_LogsClientErrorsAtWarnLevel(t *testing.T) { | ||
| var buf bytes.Buffer | ||
| original := slog.Default() | ||
| slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) | ||
| t.Cleanup(func() { | ||
| slog.SetDefault(original) | ||
| }) | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodPost, "/admin/api/v1/workflows", nil) | ||
| req = req.WithContext(core.WithRequestID(req.Context(), "admin-warn-req-123")) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| if err := handleError(c, core.NewInvalidRequestError("unknown provider name: missing", nil)); err != nil { | ||
| t.Fatalf("handleError() error = %v", err) | ||
| } | ||
|
|
||
| if rec.Code != http.StatusBadRequest { | ||
| t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) | ||
| } | ||
|
|
||
| logOutput := buf.String() | ||
| if !strings.Contains(logOutput, `"level":"WARN"`) { | ||
| t.Fatalf("expected WARN log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"msg":"admin request failed"`) { | ||
| t.Fatalf("expected admin request failed log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"path":"/admin/api/v1/workflows"`) { | ||
| t.Fatalf("expected admin path in log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"request_id":"admin-warn-req-123"`) { | ||
| t.Fatalf("expected request_id in log, got %q", logOutput) | ||
| } | ||
| } | ||
|
|
||
| func TestHandleError_LogsServerErrorsAtErrorLevel(t *testing.T) { | ||
| var buf bytes.Buffer | ||
| original := slog.Default() | ||
| slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) | ||
| t.Cleanup(func() { | ||
| slog.SetDefault(original) | ||
| }) | ||
|
|
||
| e := echo.New() | ||
| req := httptest.NewRequest(http.MethodPut, "/admin/api/v1/guardrails/privacy", nil) | ||
| req = req.WithContext(core.WithRequestID(req.Context(), "admin-error-req-456")) | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| upstreamErr := errors.New("storage unavailable") | ||
| if err := handleError(c, core.NewProviderError("guardrails", http.StatusInternalServerError, "failed to refresh guardrails", upstreamErr)); err != nil { | ||
| t.Fatalf("handleError() error = %v", err) | ||
| } | ||
|
|
||
| if rec.Code != http.StatusInternalServerError { | ||
| t.Fatalf("status = %d, want %d", rec.Code, http.StatusInternalServerError) | ||
| } | ||
|
|
||
| logOutput := buf.String() | ||
| if !strings.Contains(logOutput, `"level":"ERROR"`) { | ||
| t.Fatalf("expected ERROR log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"msg":"admin request failed"`) { | ||
| t.Fatalf("expected admin request failed log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"provider":"guardrails"`) { | ||
| t.Fatalf("expected provider in log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"request_id":"admin-error-req-456"`) { | ||
| t.Fatalf("expected request_id in log, got %q", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, `"message":"failed to refresh guardrails"`) { | ||
| t.Fatalf("expected error message in log, got %q", logOutput) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add coverage for the X-Request-ID fallback branch.
The production helper now falls back to the request header when the context has no request ID, but the new tests only exercise the context path.
Suggested test coverage
+func TestHandleError_LogsRequestIDFromHeaderWhenContextMissing(t *testing.T) {
+ var buf bytes.Buffer
+ original := slog.Default()
+ slog.SetDefault(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})))
+ t.Cleanup(func() {
+ slog.SetDefault(original)
+ })
+
+ e := echo.New()
+ req := httptest.NewRequest(http.MethodGet, "/admin/api/v1/aliases", nil)
+ req.Header.Set("X-Request-ID", "admin-header-req-789")
+ rec := httptest.NewRecorder()
+ c := e.NewContext(req, rec)
+
+ if err := handleError(c, core.NewNotFoundError("alias not found: missing")); err != nil {
+ t.Fatalf("handleError() error = %v", err)
+ }
+
+ if rec.Code != http.StatusNotFound {
+ t.Fatalf("status = %d, want %d", rec.Code, http.StatusNotFound)
+ }
+ if logOutput := buf.String(); !strings.Contains(logOutput, `"request_id":"admin-header-req-789"`) {
+ t.Fatalf("expected header request_id in log, got %q", logOutput)
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/handler_test.go` around lines 1934 - 2010, Add a new unit test
that exercises the X-Request-ID fallback by calling handleError with an HTTP
request that does NOT have a context request ID but DOES have the "X-Request-ID"
header set; mirror the existing
TestHandleError_LogsClientErrorsAtWarnLevel/TestHandleError_LogsServerErrorsAtErrorLevel
pattern (create buf, set slog.Default to JSON handler, build echo.Context via
e.NewContext), set req.Header.Set("X-Request-ID","admin-header-req-789"), call
handleError (e.g., pass core.NewInvalidRequestError or core.NewProviderError),
assert response code and verify the JSON log contains the header value
`"request_id":"admin-header-req-789"` along with the expected
level/msg/path/provider/message fields.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
internal/admin/dashboard/static/js/modules/guardrails.js (1)
195-207:⚠️ Potential issue | 🟠 MajorAutofocus still lands on the close button first.
querySelector()does not prioritize the first selector in the list; it returns the first matching element in DOM order. Inpage-guardrails.html, the header close button appears before thedata-modal-autofocusinput, andbutton:not([disabled])matches it first. Try the explicit autofocus marker first, then fall back to non-button form fields.🐛 Proposed fix
- const field = editor.querySelector('[data-modal-autofocus]:not([disabled]), input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled]), button:not([disabled])'); + const field = editor.querySelector('[data-modal-autofocus]:not([disabled])') + || editor.querySelector('input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled])');Verify the DOM-order condition and any analogous selector-list implementations:
#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path js = Path("internal/admin/dashboard/static/js/modules/guardrails.js").read_text() tpl = Path("internal/admin/dashboard/templates/page-guardrails.html").read_text() selector = "button:not([disabled])" if selector not in js: raise SystemExit("Expected button fallback selector was not found in focusGuardrailForm") close_index = tpl.index("alias-close-btn") autofocus_index = tpl.index("data-modal-autofocus") print(f"close button index: {close_index}") print(f"autofocus field index: {autofocus_index}") if close_index < autofocus_index: print("Close button is before the intended autofocus field; a single querySelector selector list can select it first.") else: raise SystemExit("Template order no longer reproduces the focus issue") PY rg -n -C 3 "querySelector\\('\\[data-modal-autofocus][^']*button:not\\(\\[disabled\\]\\)" internal/admin/dashboard/static/js/modulesDoes Element.querySelector with a comma-separated selector list return the first matching element in document order rather than prioritizing the first selector?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/guardrails.js` around lines 195 - 207, The focusGuardrailForm function currently uses a single comma-separated querySelector which returns the first matching element in DOM order (causing the header close button to get focused); update focus logic in focusGuardrailForm to explicitly try editor.querySelector('[data-modal-autofocus]:not([disabled])') first, then fall back to editor.querySelector('input:not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled])') and only as a last resort querySelector('button:not([disabled])'); ensure you keep the existing guards (check editor and field and typeof field.focus) and call field.focus({ preventScroll: true }) when a field is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/workflows-layout.test.js`:
- Around line 31-37: The readCSSRule helper currently requires an unindented
newline before the closing brace which breaks valid CSS with CRLF or indented
braces; update the RegExp in readCSSRule (the function named readCSSRule) to be
more tolerant by allowing optional CR/LF and any whitespace before the `}` (e.g.
replace `\\n\\}` with `\\s*\\}` and/or support `\\r?\\n`), so the pattern
becomes something like `${escapedSelector}\\s*\\{([\\s\\S]*?)\\s*\\}` with the
"m" flag to match rules regardless of closing-brace indentation or line endings.
In `@internal/admin/dashboard/templates/page-models.html`:
- Line 25: The category tab button element (class "category-tab") is missing an
explicit type attribute; update the <button class="category-tab" :class="{
active: activeCategory === cat.category }"
`@click`="selectCategory(cat.category)"> element to include type="button" so it
matches other buttons in the template and avoids HTMLHint warnings and future
form-related regressions.
In `@internal/admin/dashboard/templates/page-usage.html`:
- Around line 9-10: The two usage-mode buttons (class "usage-mode-btn", bound to
usageMode and calling toggleUsageMode('tokens') / toggleUsageMode('costs')) lack
an explicit type attribute; add type="button" to both button elements to match
other buttons (e.g., chart-view-btn) and prevent default form-submit behavior if
moved into a form in the future. Locate the buttons by the class
"usage-mode-btn" and update their markup to include type="button" for both the
Tokens and Costs buttons.
In `@internal/admin/dashboard/templates/page-workflows.html`:
- Line 169: Replace the stray tab characters at the start of the affected
template lines with spaces so indentation matches the rest of the file;
specifically remove the leading TAB before the "<div
class=\"workflow-guardrail-editor\" x-show=\"workflowForm.features.guardrails &&
workflowGuardrailsVisible()\">" line (and the other occurrence around line 236)
so both divs use the same space-based indentation as the surrounding template.
- Around line 37-46: The workflow modal backdrop and shell (elements using
x-cloak x-show="workflowFormOpen") currently lack the fade transition used by
other editors; add the same Alpine transition modifier
x-transition.opacity.duration.160ms to both the backdrop div and the shell div
(the elements tied to workflowFormOpen and the shell that has
`@click`="closeWorkflowForm()") so the workflow editor (x-ref="workflowEditor")
fades in/out consistently with the alias and model-access modals.
---
Duplicate comments:
In `@internal/admin/dashboard/static/js/modules/guardrails.js`:
- Around line 195-207: The focusGuardrailForm function currently uses a single
comma-separated querySelector which returns the first matching element in DOM
order (causing the header close button to get focused); update focus logic in
focusGuardrailForm to explicitly try
editor.querySelector('[data-modal-autofocus]:not([disabled])') first, then fall
back to editor.querySelector('input:not([type="hidden"]):not([disabled]),
textarea:not([disabled]), select:not([disabled])') and only as a last resort
querySelector('button:not([disabled])'); ensure you keep the existing guards
(check editor and field and typeof field.focus) and call field.focus({
preventScroll: true }) when a field is found.
🪄 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
Run ID: e0485495-fa83-4d45-995f-0bc4c982a442
📒 Files selected for processing (14)
internal/admin/dashboard/static/js/modules/auth-keys.jsinternal/admin/dashboard/static/js/modules/auth-keys.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/guardrails.test.jsinternal/admin/dashboard/static/js/modules/timezone-layout.test.jsinternal/admin/dashboard/static/js/modules/workflows-layout.test.jsinternal/admin/dashboard/templates/page-auth-keys.htmlinternal/admin/dashboard/templates/page-guardrails.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/dashboard/templates/page-overview.htmlinternal/admin/dashboard/templates/page-settings.htmlinternal/admin/dashboard/templates/page-usage.htmlinternal/admin/dashboard/templates/page-workflows.html
Comment on lines
31
to
+37
| function readCSSRule(source, selector) { | ||
| const escapedSelector = selector.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const match = source.match(new RegExp(`${escapedSelector}\\s*\\{([\\s\\S]*?)\\n\\}`, 'm')); | ||
| assert.ok(match, `Expected CSS rule for ${selector}`); | ||
| return match[1]; | ||
| const escapedSelector = selector.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const match = source.match( | ||
| new RegExp(`${escapedSelector}\\s*\\{([\\s\\S]*?)\\n\\}`, "m"), | ||
| ); | ||
| assert.ok(match, `Expected CSS rule for ${selector}`); | ||
| return match[1]; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Keep readCSSRule tolerant of valid CSS formatting.
This copy still requires an unindented } after \n, so CRLF or indented closing braces can break workflow layout tests even though the CSS is valid. Align it with the more robust helper used in dashboard-layout.test.js.
♻️ Proposed fix
function readCSSRule(source, selector) {
const escapedSelector = selector.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const match = source.match(
- new RegExp(`${escapedSelector}\\s*\\{([\\s\\S]*?)\\n\\}`, "m"),
+ new RegExp(`${escapedSelector}\\s*\\{([\\s\\S]*?)\\s*\\}`, "m"),
);
assert.ok(match, `Expected CSS rule for ${selector}`);
return match[1];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/modules/workflows-layout.test.js` around
lines 31 - 37, The readCSSRule helper currently requires an unindented newline
before the closing brace which breaks valid CSS with CRLF or indented braces;
update the RegExp in readCSSRule (the function named readCSSRule) to be more
tolerant by allowing optional CR/LF and any whitespace before the `}` (e.g.
replace `\\n\\}` with `\\s*\\}` and/or support `\\r?\\n`), so the pattern
becomes something like `${escapedSelector}\\s*\\{([\\s\\S]*?)\\s*\\}` with the
"m" flag to match rules regardless of closing-brace indentation or line endings.
| </article> | ||
| </div> | ||
|
|
||
| <div class="workflow-guardrail-editor" x-show="workflowForm.features.guardrails && workflowGuardrailsVisible()"> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stray tab characters break indentation.
Lines 169 and 236 start with a literal TAB followed by spaces (\t <div ...>), while the surrounding template uses only spaces. This trips up editors configured with different tab widths and shows up oddly in diffs/grep. Replace the tabs with spaces to match the rest of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-workflows.html` at line 169, Replace
the stray tab characters at the start of the affected template lines with spaces
so indentation matches the rest of the file; specifically remove the leading TAB
before the "<div class=\"workflow-guardrail-editor\"
x-show=\"workflowForm.features.guardrails && workflowGuardrailsVisible()\">"
line (and the other occurrence around line 236) so both divs use the same
space-based indentation as the surrounding template.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
internal/admin/dashboard/templates/page-workflows.html (1)
171-171: 🧹 Nitpick | 🔵 TrivialRemove the stray tab indentation.
Lines 171 and 238 still start with a literal tab before spaces; normalize to spaces to match the surrounding template.
Also applies to: 238-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/templates/page-workflows.html` at line 171, Remove the stray tab characters at the start of the affected template lines so indentation uses spaces consistently; specifically, edit the lines containing the div with class "workflow-guardrail-editor" (the element with x-show="workflowForm.features.guardrails && workflowGuardrailsVisible()") and the similar line around the other occurrence (line with the same class/attribute near the second mention) to replace the leading tab with the project's standard spaces so the template's indentation is normalized.internal/admin/dashboard/static/css/dashboard.css (2)
1700-1709: 🧹 Nitpick | 🔵 TrivialStill using generic
.form*selectors for modal-specific forms.These selectors remain broad enough to collide with future dashboard markup. Prefer an editor-scoped prefix such as
.editor-form,.editor-form-field,.editor-form-error, and.editor-form-actions.Also applies to: 1786-1889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/css/dashboard.css` around lines 1700 - 1709, Replace the broad form selectors with editor-scoped ones to avoid global collisions: change occurrences of the selectors .form h3, .form-kicker, .form-hint (and the other .form* selectors in the block covering lines referenced 1786-1889) to namespaced versions such as .editor-form h3, .editor-form-kicker, .editor-form-hint (and .editor-form-field, .editor-form-error, .editor-form-actions where applicable), then update any corresponding markup using these selectors to match the new class names so styles remain scoped to the editor modal.
1371-1385: 🧹 Nitpick | 🔵 TrivialStill broad-scoping base input styles.
This global
input:is(...)rule can still affect unrelated dashboard controls and omits common form types likepassword,search,url,tel, andtime. Scope it to the intended form/editor containers or broaden the supported input types consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/css/dashboard.css` around lines 1371 - 1385, The global selector input:is([type="text"], [type="date"], [type="number"]) is too broad and misses common types; narrow it to the intended container (e.g., prefix with the editor/form container class such as .dashboard-editor or .form-container so only inputs inside that component are styled) and extend the type list to include email, password, search, url, tel, and time (update both the base selector and the :focus rule for consistency). Ensure you replace input:is(...) with the scoped selector (e.g., .dashboard-editor input:is([type="text"], [type="date"], [type="number"], [type="email"], [type="password"], [type="search"], [type="url"], [type="tel"], [type="time"])) so unrelated dashboard controls aren’t affected and focus styling remains consistent.internal/admin/dashboard/static/js/modules/workflows-layout.test.js (1)
31-37: 🧹 Nitpick | 🔵 TrivialKeep
readCSSRuletolerant of valid CSS formatting.This helper still requires an unindented LF before
}, so valid CRLF or indented closing braces can break these tests. Match the more robust helper already used indashboard-layout.test.js.♻️ Proposed fix
function readCSSRule(source, selector) { const escapedSelector = selector.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const match = source.match( - new RegExp(`${escapedSelector}\\s*\\{([\\s\\S]*?)\\n\\}`, "m"), + new RegExp(`${escapedSelector}\\s*\\{([\\s\\S]*?)\\s*\\}`, "m"), ); assert.ok(match, `Expected CSS rule for ${selector}`); return match[1]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/workflows-layout.test.js` around lines 31 - 37, The readCSSRule helper's regex requires an unindented LF before the closing brace and fails on CRLF or indented braces; update the regexp in function readCSSRule to accept optional CR and any whitespace before the closing "}" (e.g., allow \r?\n or any whitespace characters) and/or use a non-greedy capture that terminates at a closing brace preceded by optional whitespace instead of requiring "\n\}"; keep the escapedSelector handling and assertion (readCSSRule, escapedSelector, match) otherwise unchanged.internal/admin/dashboard/templates/page-guardrails.html (1)
72-88:⚠️ Potential issue | 🟡 MinorDon’t autofocus a disabled field in edit mode.
Line 74 keeps
data-modal-autofocuson the name input even when edit mode disables it, so the edit modal can open without useful keyboard focus.♿ Proposed fix
- <input id="guardrail-name" type="text" placeholder="safety-system-prompt" x-model="guardrailForm.name" :disabled="guardrailFormMode === 'edit'" data-modal-autofocus> + <input id="guardrail-name" type="text" placeholder="safety-system-prompt" x-model="guardrailForm.name" :disabled="guardrailFormMode === 'edit'" :data-modal-autofocus="guardrailFormMode === 'create' ? true : null"> @@ - <input id="guardrail-description" type="text" placeholder="What this guardrail is meant to do" x-model="guardrailForm.description"> + <input id="guardrail-description" type="text" placeholder="What this guardrail is meant to do" x-model="guardrailForm.description" :data-modal-autofocus="guardrailFormMode === 'edit' ? true : null">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/templates/page-guardrails.html` around lines 72 - 88, The name input currently always has data-modal-autofocus even when guardrailFormMode === 'edit', causing focus to land on a disabled field; update the guardrail-name input to only include the autofocus behavior when not in edit mode by conditionally applying or removing data-modal-autofocus based on guardrailFormMode (use the existing guardrailFormMode and guardrailForm.name identifiers to locate the element and ensure the autofocus attribute is not present when guardrailFormMode === 'edit').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/templates/page-audit-logs.html`:
- Around line 121-174: Add x-cloak to the overlay div and the <aside
class="conversation-drawer"> and add :inert="!conversationOpen" to the same
aside so the drawer and its focusable descendants (e.g., the close button
x-ref="conversationCloseBtn" and details toggles in the conversation thread) are
removed from keyboard navigation when conversationOpen is false; keep bindings
that reference conversationOpen, x-ref="conversationDialog", and aria-hidden
as-is to preserve existing behavior.
In `@internal/admin/dashboard/templates/page-auth-keys.html`:
- Around line 83-85: The Name input (id="auth-key-name",
x-model="authKeyForm.name", x-ref="authKeyNameInput") is marked required in the
label but lacks native validation—add the required attribute (and optionally
minlength or pattern) to the input so the browser enforces it, and update the
form submission flow to call the form's checkValidity()/reportValidity() (or
return early if invalid) before sending (ensure the submit handler that reads
authKeyForm runs this check). This ensures the visible create form prevents
empty names client-side using native HTML validation tied to authKeyNameInput.
In `@internal/admin/dashboard/templates/page-guardrails.html`:
- Around line 43-44: The dynamic guardrail alert DIVs lack ARIA live-region
semantics so screen readers won't announce the Alpine-driven changes; update the
two elements rendered in page-guardrails.html (the divs using x-show with
guardrailError and guardrailNotice) to include accessibility attributes such as
role="status", aria-live="polite" and aria-atomic="true" (and optionally
tabindex="-1" if focusable announcement is desired) so assistive tech will
announce the messages when the Alpine state (guardrailError / guardrailNotice)
changes.
- Around line 130-142: Checkbox checkbox fields always show the help text
instead of using the inline question-mark helper like other field types; update
the template under the block where field.input === 'checkboxes' to use the same
inline helper pattern as the other type-specific fields. Replace the
always-visible <small class="form-hint" ...> help with the question-mark/inline
helper markup used elsewhere (keep the same aria-describedby id pattern
'guardrail-field-help-' + field.key and only render the helper when field.help
exists), wire the helper's toggle/tooltip to the same accessibility attributes
so guardrailArrayFieldSelected and toggleGuardrailArrayFieldValue behavior and
focus/aria semantics remain unchanged. Ensure the helper reads field.help and
preserves keyboard accessibility for the checkbox inputs.
In `@internal/admin/dashboard/templates/page-usage.html`:
- Around line 139-149: The model/provider <select> controls (bound to
usageLogModel and usageLogProvider, class usage-log-select) lack accessible
names; add aria-label attributes to each select (e.g., aria-label="Model filter"
on the usageLogModel select and aria-label="Provider filter" on the
usageLogProvider select) so screen readers announce the control purpose
consistently after a value is chosen.
---
Duplicate comments:
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 1700-1709: Replace the broad form selectors with editor-scoped
ones to avoid global collisions: change occurrences of the selectors .form h3,
.form-kicker, .form-hint (and the other .form* selectors in the block covering
lines referenced 1786-1889) to namespaced versions such as .editor-form h3,
.editor-form-kicker, .editor-form-hint (and .editor-form-field,
.editor-form-error, .editor-form-actions where applicable), then update any
corresponding markup using these selectors to match the new class names so
styles remain scoped to the editor modal.
- Around line 1371-1385: The global selector input:is([type="text"],
[type="date"], [type="number"]) is too broad and misses common types; narrow it
to the intended container (e.g., prefix with the editor/form container class
such as .dashboard-editor or .form-container so only inputs inside that
component are styled) and extend the type list to include email, password,
search, url, tel, and time (update both the base selector and the :focus rule
for consistency). Ensure you replace input:is(...) with the scoped selector
(e.g., .dashboard-editor input:is([type="text"], [type="date"], [type="number"],
[type="email"], [type="password"], [type="search"], [type="url"], [type="tel"],
[type="time"])) so unrelated dashboard controls aren’t affected and focus
styling remains consistent.
In `@internal/admin/dashboard/static/js/modules/workflows-layout.test.js`:
- Around line 31-37: The readCSSRule helper's regex requires an unindented LF
before the closing brace and fails on CRLF or indented braces; update the regexp
in function readCSSRule to accept optional CR and any whitespace before the
closing "}" (e.g., allow \r?\n or any whitespace characters) and/or use a
non-greedy capture that terminates at a closing brace preceded by optional
whitespace instead of requiring "\n\}"; keep the escapedSelector handling and
assertion (readCSSRule, escapedSelector, match) otherwise unchanged.
In `@internal/admin/dashboard/templates/page-guardrails.html`:
- Around line 72-88: The name input currently always has data-modal-autofocus
even when guardrailFormMode === 'edit', causing focus to land on a disabled
field; update the guardrail-name input to only include the autofocus behavior
when not in edit mode by conditionally applying or removing data-modal-autofocus
based on guardrailFormMode (use the existing guardrailFormMode and
guardrailForm.name identifiers to locate the element and ensure the autofocus
attribute is not present when guardrailFormMode === 'edit').
In `@internal/admin/dashboard/templates/page-workflows.html`:
- Line 171: Remove the stray tab characters at the start of the affected
template lines so indentation uses spaces consistently; specifically, edit the
lines containing the div with class "workflow-guardrail-editor" (the element
with x-show="workflowForm.features.guardrails && workflowGuardrailsVisible()")
and the similar line around the other occurrence (line with the same
class/attribute near the second mention) to replace the leading tab with the
project's standard spaces so the template's indentation is normalized.
🪄 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
Run ID: b1e2037b-1445-4c7b-81b0-e48ab6e99d10
📒 Files selected for processing (10)
internal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/workflows-layout.test.jsinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-audit-logs.htmlinternal/admin/dashboard/templates/page-auth-keys.htmlinternal/admin/dashboard/templates/page-guardrails.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/dashboard/templates/page-usage.htmlinternal/admin/dashboard/templates/page-workflows.html
Comment on lines
+121
to
+174
| <div class="conversation-overlay" x-show="conversationOpen" x-transition.opacity.duration.160ms aria-hidden="true" @click="closeConversation()"></div> | ||
| <aside class="conversation-drawer" | ||
| :class="{ open: conversationOpen }" | ||
| x-ref="conversationDialog" | ||
| tabindex="-1" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| :aria-hidden="(!conversationOpen).toString()" | ||
| aria-labelledby="interactions-drawer-title" | ||
| aria-describedby="interactions-drawer-content" | ||
| @keydown.escape.window="conversationOpen && closeConversation()"> | ||
| <div class="conversation-drawer-header"> | ||
| <h3 id="interactions-drawer-title">Interactions</h3> | ||
| <button type="button" class="dialog-close-btn" x-ref="conversationCloseBtn" aria-label="Close interactions" @click="closeConversation()"> | ||
| {{template "x-icon"}} | ||
| </button> | ||
| </div> | ||
|
|
||
| <div id="interactions-drawer-content"> | ||
| <div class="alert alert-warning" x-show="conversationError" x-text="conversationError"></div> | ||
| <p class="empty-state" x-show="conversationLoading">Loading interactions...</p> | ||
| <p class="empty-state" x-show="!conversationLoading && !conversationError && conversationMessages.length === 0">No interaction data available for this entry.</p> | ||
|
|
||
| <div class="conversation-thread" x-show="conversationMessages.length > 0"> | ||
| <template x-for="msg in conversationMessages" :key="msg.uid"> | ||
| <article :class="[msg.role === 'function_call' || msg.role === 'function_result' ? 'chat-function-note' : 'chat-message', msg.roleClass, msg.isAnchor ? 'is-anchor' : '']"> | ||
| <details class="chat-function-note-details" x-show="msg.role === 'function_call' || msg.role === 'function_result'"> | ||
| <summary class="chat-function-note-inner"> | ||
| <span class="chat-function-label" x-text="msg.roleLabel"></span> | ||
| <span class="chat-function-detail" x-text="msg.role === 'function_call' ? (msg.toolCalls || []).map(tc => tc.name + '()').join(', ') : (msg.functionName ? msg.functionName + ': ' : '') + msg.text"></span> | ||
| </summary> | ||
| <pre class="chat-function-expanded" x-text="functionExpandedContent(msg)"></pre> | ||
| </details> | ||
| <header class="chat-message-meta" x-show="msg.role !== 'function_call' && msg.role !== 'function_result'"> | ||
| <span class="chat-role" x-text="msg.roleLabel"></span> | ||
| <span class="mono chat-time" x-text="formatTimestamp(msg.timestamp)"></span> | ||
| </header> | ||
| <pre class="chat-content" x-show="msg.text && msg.role !== 'function_call' && msg.role !== 'function_result'" x-text="msg.text"></pre> | ||
| <footer class="chat-tool-calls" x-show="msg.toolCalls && msg.role !== 'function_call'"> | ||
| <template x-for="(tc, tcIdx) in (msg.toolCalls || [])" :key="tc.name + '-' + tcIdx"> | ||
| <div class="chat-tool-call"> | ||
| <span class="chat-tool-call-name" x-text="tc.name + '()'"></span> | ||
| </div> | ||
| </template> | ||
| </footer> | ||
| </article> | ||
| </template> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="conversation-drawer-footer" x-show="conversationAnchorID"> | ||
| <p class="conversation-meta" x-text="'Opened from log: ' + conversationAnchorID"></p> | ||
| </div> | ||
| </aside> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and verify it exists
find . -name "page-audit-logs.html" -type fRepository: ENTERPILOT/GoModel
Length of output: 119
🏁 Script executed:
# Check the content around lines 121-174
head -n 180 internal/admin/dashboard/templates/page-audit-logs.html | tail -n 65Repository: ENTERPILOT/GoModel
Length of output: 4077
🌐 Web query:
Alpine.js :inert binding support and x-cloak behavior
💡 Result:
Alpine.js supports the inert attribute via x-bind binding (e.g., :inert="true"), as 'inert' is recognized as a boolean attribute in recent versions (confirmed in Alpine.js PR #4325 merged July 2024, adding 'inert' to the list of boolean attributes handled by x-bind). There is no dedicated x-inert directive; use x-bind or its shorthand ':' for dynamic control. Note that browser support for the native inert attribute is good in modern browsers (Chrome 102+, Firefox 118+, Safari 15.4+ as of 2026). x-cloak hides elements with the x-cloak attribute until Alpine initializes, preventing FOUC (Flash of Unstyled Content). It requires CSS: [x-cloak] { display: none !important; }. Alpine removes x-cloak attributes post-initialization, revealing the content. Commonly used with x-show, x-text, etc., on elements that might briefly show raw templates before reactivity kicks in. Alternative: for similar effect without global CSS.
Citations:
- 1: https://alpinejs.dev/directives/bind
- 2: https://alpinejs.dev/directives/cloak
- 3: 🚧 Updates List of boolean attributes to follow spec alpinejs/alpine#4325
- 4: https://alpinejs.dev/plugins/focus
🏁 Script executed:
# Search for Alpine.js version and configuration
find . -name "package.json" -o -name "go.mod" -o -name "*.lock" | head -5 | xargs grep -l "alpine\|Alpine" 2>/dev/nullRepository: ENTERPILOT/GoModel
Length of output: 46
🏁 Script executed:
# Look for x-cloak CSS definition in the codebase
rg "x-cloak" --type html --type css -A 3Repository: ENTERPILOT/GoModel
Length of output: 4510
Add x-cloak and :inert binding to prevent closed drawer from remaining focusable.
The drawer uses CSS class binding only and leaves all focusable descendants (close button, expandable content) in the tab order when conversationOpen is false. This creates an accessibility issue where keyboard users can focus hidden controls.
Add x-cloak on both the overlay and drawer (consistent with all other modals in the codebase like page-guardrails.html, page-workflows.html, and page-models.html) to prevent initial paint flicker. Add :inert="!conversationOpen" to the drawer to properly remove it from keyboard navigation when closed.
Proposed fix
- <div class="conversation-overlay" x-show="conversationOpen" x-transition.opacity.duration.160ms aria-hidden="true" `@click`="closeConversation()"></div>
+ <div class="conversation-overlay" x-cloak x-show="conversationOpen" x-transition.opacity.duration.160ms aria-hidden="true" `@click`="closeConversation()"></div>
<aside class="conversation-drawer"
+ x-cloak
:class="{ open: conversationOpen }"
+ :inert="!conversationOpen"
x-ref="conversationDialog"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-audit-logs.html` around lines 121 -
174, Add x-cloak to the overlay div and the <aside class="conversation-drawer">
and add :inert="!conversationOpen" to the same aside so the drawer and its
focusable descendants (e.g., the close button x-ref="conversationCloseBtn" and
details toggles in the conversation thread) are removed from keyboard navigation
when conversationOpen is false; keep bindings that reference conversationOpen,
x-ref="conversationDialog", and aria-hidden as-is to preserve existing behavior.
Comment on lines
+83
to
+85
| <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" | ||
| x-model="authKeyForm.name" autocomplete="off" x-ref="authKeyNameInput" data-modal-autofocus> |
Contributor
There was a problem hiding this comment.
Make the required API key name enforceable.
The label marks this field required, but the input can still submit blank and rely on a server error. Add native validation for the visible create form.
🐛 Proposed fix
- <input id="auth-key-name" type="text" placeholder="e.g. ci-deploy"
- x-model="authKeyForm.name" autocomplete="off" x-ref="authKeyNameInput" data-modal-autofocus>
+ <input id="auth-key-name" type="text" placeholder="e.g. ci-deploy"
+ x-model="authKeyForm.name" autocomplete="off" x-ref="authKeyNameInput"
+ data-modal-autofocus :required="!authKeyIssuedValue">📝 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
| <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" | |
| x-model="authKeyForm.name" autocomplete="off" x-ref="authKeyNameInput" data-modal-autofocus> | |
| <input id="auth-key-name" type="text" placeholder="e.g. ci-deploy" | |
| x-model="authKeyForm.name" autocomplete="off" x-ref="authKeyNameInput" | |
| data-modal-autofocus :required="!authKeyIssuedValue"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-auth-keys.html` around lines 83 - 85,
The Name input (id="auth-key-name", x-model="authKeyForm.name",
x-ref="authKeyNameInput") is marked required in the label but lacks native
validation—add the required attribute (and optionally minlength or pattern) to
the input so the browser enforces it, and update the form submission flow to
call the form's checkValidity()/reportValidity() (or return early if invalid)
before sending (ensure the submit handler that reads authKeyForm runs this
check). This ensures the visible create form prevents empty names client-side
using native HTML validation tied to authKeyNameInput.
Comment on lines
+43
to
+44
| <div class="alert alert-warning" x-show="!authError && guardrailError && !guardrailFormOpen" x-text="guardrailError"></div> | ||
| <div class="alert alert-success" x-show="!authError && guardrailNotice && !guardrailError" x-text="guardrailNotice"></div> |
Contributor
There was a problem hiding this comment.
Announce dynamic guardrail messages to assistive tech.
These messages appear via Alpine state changes, but without live-region semantics screen readers may not announce them.
♿ Proposed fix
- <div class="alert alert-warning" x-show="!authError && guardrailError && !guardrailFormOpen" x-text="guardrailError"></div>
- <div class="alert alert-success" x-show="!authError && guardrailNotice && !guardrailError" x-text="guardrailNotice"></div>
+ <div class="alert alert-warning" x-show="!authError && guardrailError && !guardrailFormOpen" x-text="guardrailError" role="alert" aria-live="assertive"></div>
+ <div class="alert alert-success" x-show="!authError && guardrailNotice && !guardrailError" x-text="guardrailNotice" role="status" aria-live="polite"></div>📝 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
| <div class="alert alert-warning" x-show="!authError && guardrailError && !guardrailFormOpen" x-text="guardrailError"></div> | |
| <div class="alert alert-success" x-show="!authError && guardrailNotice && !guardrailError" x-text="guardrailNotice"></div> | |
| <div class="alert alert-warning" x-show="!authError && guardrailError && !guardrailFormOpen" x-text="guardrailError" role="alert" aria-live="assertive"></div> | |
| <div class="alert alert-success" x-show="!authError && guardrailNotice && !guardrailError" x-text="guardrailNotice" role="status" aria-live="polite"></div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-guardrails.html` around lines 43 -
44, The dynamic guardrail alert DIVs lack ARIA live-region semantics so screen
readers won't announce the Alpine-driven changes; update the two elements
rendered in page-guardrails.html (the divs using x-show with guardrailError and
guardrailNotice) to include accessibility attributes such as role="status",
aria-live="polite" and aria-atomic="true" (and optionally tabindex="-1" if
focusable announcement is desired) so assistive tech will announce the messages
when the Alpine state (guardrailError / guardrailNotice) changes.
Comment on lines
+130
to
+142
| <template x-if="field.input === 'checkboxes'"> | ||
| <fieldset class="form-field form-field-wide form-field-fieldset" :aria-describedby="field.help ? 'guardrail-field-help-' + field.key : null"> | ||
| <legend class="form-field-legend" x-text="field.label"></legend> | ||
| <div class="workflow-feature-toggles"> | ||
| <template x-for="option in field.options" :key="field.key + '-' + option.value"> | ||
| <label class="workflow-feature-toggle"> | ||
| <input type="checkbox" :checked="guardrailArrayFieldSelected(field, option.value)" @change="toggleGuardrailArrayFieldValue(field, option.value, $event.target.checked)"> | ||
| <span x-text="option.label"></span> | ||
| </label> | ||
| </template> | ||
| </div> | ||
| <small class="form-hint" :id="'guardrail-field-help-' + field.key" x-show="field.help" x-text="field.help"></small> | ||
| </fieldset> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the inline helper pattern for checkbox fields too.
Checkbox guardrail fields still render always-visible help text, unlike the other type-specific fields that use the question-mark helper.
♻️ Proposed refactor
- <fieldset class="form-field form-field-wide form-field-fieldset" :aria-describedby="field.help ? 'guardrail-field-help-' + field.key : null">
- <legend class="form-field-legend" x-text="field.label"></legend>
+ <fieldset class="form-field form-field-wide form-field-fieldset" x-data="{ open: false, get copyId() { return 'guardrail-field-help-' + field.key }, get showLabel() { return 'Show ' + field.label + ' help' }, get hideLabel() { return 'Hide ' + field.label + ' help' }, get text() { return field.help || '' } }" :aria-describedby="field.help ? copyId : null">
+ <legend class="form-field-legend inline-help-title-row">
+ <span x-text="field.label"></span>
+ <template x-if="field.help">
+ {{template "inline-help-toggle" .}}
+ </template>
+ </legend>
+ <p :id="copyId" class="inline-help-copy" x-show="open && text" x-transition.opacity.duration.200ms x-text="text"></p>
<div class="workflow-feature-toggles">
<template x-for="option in field.options" :key="field.key + '-' + option.value">
<label class="workflow-feature-toggle">
<input type="checkbox" :checked="guardrailArrayFieldSelected(field, option.value)" `@change`="toggleGuardrailArrayFieldValue(field, option.value, $event.target.checked)">
<span x-text="option.label"></span>
@@
- <small class="form-hint" :id="'guardrail-field-help-' + field.key" x-show="field.help" x-text="field.help"></small>
</fieldset>🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 136-136: No matching [ label ] tag found.
(input-requires-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-guardrails.html` around lines 130 -
142, Checkbox checkbox fields always show the help text instead of using the
inline question-mark helper like other field types; update the template under
the block where field.input === 'checkboxes' to use the same inline helper
pattern as the other type-specific fields. Replace the always-visible <small
class="form-hint" ...> help with the question-mark/inline helper markup used
elsewhere (keep the same aria-describedby id pattern 'guardrail-field-help-' +
field.key and only render the helper when field.help exists), wire the helper's
toggle/tooltip to the same accessibility attributes so
guardrailArrayFieldSelected and toggleGuardrailArrayFieldValue behavior and
focus/aria semantics remain unchanged. Ensure the helper reads field.help and
preserves keyboard accessibility for the checkbox inputs.
Comment on lines
+139
to
+149
| <select x-model="usageLogModel" @change="fetchUsageLog(true)" class="usage-log-select"> | ||
| <option value="">All Models</option> | ||
| <template x-for="m in usageLogModelOptions()" :key="m"> | ||
| <option :value="m" x-text="m"></option> | ||
| </template> | ||
| </select> | ||
| <select x-model="usageLogProvider" @change="fetchUsageLog(true)" class="usage-log-select"> | ||
| <option value="">All Providers</option> | ||
| <template x-for="p in usageLogProviderOptions()" :key="p"> | ||
| <option :value="p" x-text="p"></option> | ||
| </template> |
Contributor
There was a problem hiding this comment.
Add accessible names to the model/provider filters.
The selects rely on option text only, so screen-reader users do not get a stable control purpose after a value is selected. Add aria-label values like the adjacent text inputs.
♿ Proposed fix
- <select x-model="usageLogModel" `@change`="fetchUsageLog(true)" class="usage-log-select">
+ <select x-model="usageLogModel" `@change`="fetchUsageLog(true)" class="usage-log-select" aria-label="Model filter">
@@
- <select x-model="usageLogProvider" `@change`="fetchUsageLog(true)" class="usage-log-select">
+ <select x-model="usageLogProvider" `@change`="fetchUsageLog(true)" class="usage-log-select" aria-label="Provider filter">📝 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
| <select x-model="usageLogModel" @change="fetchUsageLog(true)" class="usage-log-select"> | |
| <option value="">All Models</option> | |
| <template x-for="m in usageLogModelOptions()" :key="m"> | |
| <option :value="m" x-text="m"></option> | |
| </template> | |
| </select> | |
| <select x-model="usageLogProvider" @change="fetchUsageLog(true)" class="usage-log-select"> | |
| <option value="">All Providers</option> | |
| <template x-for="p in usageLogProviderOptions()" :key="p"> | |
| <option :value="p" x-text="p"></option> | |
| </template> | |
| <select x-model="usageLogModel" `@change`="fetchUsageLog(true)" class="usage-log-select" aria-label="Model filter"> | |
| <option value="">All Models</option> | |
| <template x-for="m in usageLogModelOptions()" :key="m"> | |
| <option :value="m" x-text="m"></option> | |
| </template> | |
| </select> | |
| <select x-model="usageLogProvider" `@change`="fetchUsageLog(true)" class="usage-log-select" aria-label="Provider filter"> | |
| <option value="">All Providers</option> | |
| <template x-for="p in usageLogProviderOptions()" :key="p"> | |
| <option :value="p" x-text="p"></option> | |
| </template> | |
| </select> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/templates/page-usage.html` around lines 139 - 149,
The model/provider <select> controls (bound to usageLogModel and
usageLogProvider, class usage-log-select) lack accessible names; add aria-label
attributes to each select (e.g., aria-label="Model filter" on the usageLogModel
select and aria-label="Provider filter" on the usageLogProvider select) so
screen readers announce the control purpose consistently after a value is
chosen.
This was referenced Apr 27, 2026
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
1 participant
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
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style