Assistant - #4
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 554581c719
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const main = document.querySelector("main") | ||
| const panel = document.querySelector(".l-ui-container--panel") | ||
| if (main) main.removeAttribute("inert") | ||
| if (panel) panel.removeAttribute("inert") |
There was a problem hiding this comment.
This always removes inert from .l-ui-container--panel when the mobile menu closes, even if the panel is still closed. On mobile, a user can open and close navigation once and end up with an off-canvas panel that is aria-hidden but no longer inert, so its hidden controls are reachable via keyboard focus. Please only clear inert when the panel is actually open (or restore it when closed).
Useful? React with 👍 / 👎.
| if (this.previousActiveElement && typeof this.previousActiveElement.focus === "function") { | ||
| this.previousActiveElement.focus() | ||
| } | ||
| this.announce("Dialog closed") |
There was a problem hiding this comment.
The close event handler now announces "Dialog closed", but close() already announces the same message after calling dialogTarget.close(). Since HTMLDialogElement.close() emits the close event, normal close-button flows now send two live-region updates, which can make screen readers announce the closure twice. Keep the announcement in only one of these paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d64ee0d13b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Restore background content tabbability | ||
| const main = document.querySelector("main") | ||
| const panel = document.querySelector(".l-ui-container--panel") | ||
| if (main) main.removeAttribute("inert") |
There was a problem hiding this comment.
closeNavigation() always calls main.removeAttribute("inert"), but l-ui--panel#open now sets main to inert on mobile to keep background content out of the tab order while the panel is open. If a user opens the panel, then opens and closes mobile navigation, this line clears inert even though the panel is still open, so hidden background controls become reachable again. Recompute inert state from both UI states (nav + panel) before removing it.
Useful? React with 👍 / 👎.
| if (this.isMobile()) { | ||
| const main = document.querySelector("main") | ||
| if (main) main.setAttribute("inert", "") | ||
| } |
There was a problem hiding this comment.
open() applies inert to <main> on mobile, but there is no matching removal path when the viewport crosses to desktop while the panel stays open. Because handleWindowResize() only updates width/margins, rotating/resizing from <768px to desktop leaves main content inert and non-interactive until the panel is manually closed. The resize path should explicitly drop inert for the desktop-open panel case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c4118b3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| data-action="click->l-ui--panel#toggle" | ||
| title="Toggle panel (Ctrl+i / ⌘i) - drag to move" | ||
| data-controller="l-ui--panel-button" | ||
| data-action="click->l-ui--panel#toggle mousedown->l-ui--panel-button#startDrag touchstart:passive->l-ui--panel-button#startDrag" |
There was a problem hiding this comment.
The data-action token touchstart:passive->l-ui--panel-button#startDrag is not valid Stimulus option syntax, so on touch devices the handler is bound to a non-existent touchstart:passive event instead of touchstart. That means startDrag never runs for taps/drags on mobile, and the new draggable panel button cannot be repositioned there. Use Stimulus’s option placement (touchstart->...:passive) so the touch listener is actually registered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72dc90002c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| try { localStorage.setItem("panelOpen", "false") } catch (e) { /* localStorage unavailable */ } | ||
| const main = document.querySelector("main") | ||
| if (main) main.removeAttribute("inert") |
There was a problem hiding this comment.
openNavigation() now sets main to inert, but close() always calls main.removeAttribute("inert") without checking whether the mobile menu is currently open. On mobile, if both overlays are active and the panel is closed via the keyboard shortcut (Ctrl/⌘+i), the navigation remains open while background content becomes tabbable again, which breaks the modal-like focus isolation introduced in mobile_navigation_controller.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a51fdd054
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const main = document.querySelector("main") | ||
| const panel = document.querySelector(".l-ui-container--panel") | ||
| if (main) main.removeAttribute("inert") | ||
| if (panel) panel.removeAttribute("inert") |
There was a problem hiding this comment.
The desktop branch of handleResize() always runs panel.removeAttribute("inert"), which makes a closed off-canvas panel keyboard-focusable after any resize event (for example, resizing a desktop browser window while the panel is closed). This bypasses the panel controller’s closed-state focus isolation and allows hidden controls inside the panel to re-enter the tab order.
Useful? React with 👍 / 👎.
| if (this.isMobile()) { | ||
| const main = document.querySelector("main") | ||
| if (main) main.setAttribute("inert", "") |
There was a problem hiding this comment.
The panel only sets <main> inert at open time when isMobile() is already true, so if a user opens the panel on desktop and then shrinks the viewport below 768px, background content stays tabbable behind the now-mobile overlay. This breaks the intended modal-like focus isolation on mobile until the panel is explicitly closed and reopened.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e28e7fc026
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.element.removeEventListener("click", suppressClick, true) | ||
| } | ||
| this.element.addEventListener("click", suppressClick, true) |
There was a problem hiding this comment.
The drag-end path adds a capture-phase click suppressor but only removes it from inside that same click handler. If no click is emitted after mouseup (for example, when release happens outside the button after edge clamping, or on touch flows where preventDefault() suppresses the synthetic click), the suppressor stays attached and the next real tap/click on the panel button is swallowed. This makes the toggle appear unresponsive until an extra click and should be cleaned up even when no click arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39944cd26d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.actionButtonTarget.disabled = false | ||
| this.actionButtonTarget.setAttribute("aria-expanded", "false") | ||
| this.actionButtonTarget.removeAttribute("aria-hidden") | ||
| this.actionButtonTarget.setAttribute("tabindex", "-1") |
There was a problem hiding this comment.
Setting the action button to tabindex="-1" in the close path makes the visible "Open panel" control unreachable for keyboard-only users after any close (including initial load, because connect() calls close(false)). In this state, users who do not know the Ctrl/⌘+i shortcut cannot reopen the panel at all, which is a functional accessibility regression from the previous behaviour where the button was tabbable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c3bad12eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clearTimeout(this.toggleTimeout) | ||
| this.toggleTimeout = setTimeout(() => { | ||
| this.dispatch("toggle", { bubbles: true }) | ||
| }, TOGGLE_DELAY) |
There was a problem hiding this comment.
queueToggle fires the panel toggle after a fixed 220ms timeout, but the timeout is only cancelled in cycleCorner (the dblclick handler). Any valid double-click whose second click lands after 220ms (still within the browser/OS double-click window) will trigger a panel toggle before the corner-cycle path can cancel it, so users trying to reposition the button can unintentionally open/close the panel first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 273d092055
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| connect() { | ||
| this.previousActiveElement = null | ||
| this.isOpen = false |
There was a problem hiding this comment.
Setting this.isOpen to false on every connect can desynchronise controller state from the actual DOM state after Turbo snapshot restoration (Turbo is enabled in test/dummy/app/javascript/application.js). If a page is restored while the menu/backdrop still have the open class, close() becomes a no-op because it gates on this.isOpen, leaving a visibly open backdrop/menu that cannot be dismissed with outside click or Escape until users toggle twice. Initialise isOpen from the existing .open class (or normalise classes during connect) to keep state consistent.
Useful? React with 👍 / 👎.
💡 Codex Reviewlayered-ui-rails/app/assets/tailwind/layered/ui/styles.css Lines 169 to 171 in 9d8b1ac Limit reduced-motion reset to layered-ui scope
This selector change applies the reduced-motion override to ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 367856bd06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasModifier = event.metaKey || event.ctrlKey | ||
| if (!hasModifier || !event.altKey) return |
There was a problem hiding this comment.
This handler treats any Ctrl/⌘ + Alt + 1..4 keydown as a panel-button command and always calls preventDefault(), but on many non-US Windows layouts the AltGr key reports as ctrlKey + altKey. In those environments, typing AltGr+digit in form fields (or any focused editable control) will be intercepted and the character input is blocked while the floating button moves unexpectedly. Please ignore AltGraph/editable-input contexts before consuming the event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a49b6876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <%= 'role="alert"' if error_message.present? %> | ||
| <%= 'hidden aria-hidden="true"' unless error_message.present? %> |
There was a problem hiding this comment.
The dynamic attribute strings are rendered via <%= ... %>, so Rails escapes the embedded quotes and outputs values like role="alert" and aria-hidden="true"; HTML parsers treat those as literal values ("alert", "true") rather than valid ARIA tokens. In validation flows, this can stop assistive tech from reliably announcing field-level errors and from correctly interpreting the hidden state. Build these attributes with tag helpers or unescaped attribute values instead of interpolated escaped strings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84f8ce82c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| d="M12 3v1m0 16v1m9-9h-1M4 12H3m15.364 6.364l-.707-.707M6.343 6.343l-.707-.707m12.728 0l-.707.707M6.343 17.657l-.707.707M16 12a4 4 0 11-8 0 4 4 0 018 0z" | ||
| ></path> | ||
| </svg> | ||
| <%= image_tag "layered_ui/icon_moon.svg", alt: "", class: "l-ui-icon--sm l-ui-theme-toggle__icon--light", aria: { hidden: true } %> |
There was a problem hiding this comment.
The new header control icons are now rendered with image_tag, but the SVG assets still rely on stroke="currentColor"; when an SVG is loaded through <img>, it no longer inherits the button text colour from .l-ui-button--icon/.l-ui-button--mobile-navigation, so these icons render with a fixed colour (and only a coarse dark:invert fallback) instead of following theme/token colours. This causes visible contrast/theming regressions in dark mode and custom colour schemes where control icons are expected to track foreground tokens.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
No description provided.