https://docs.all-hands.dev/ logo
Join Slack
Powered by
<@U0BME18CF7S> look at this pr description, it has...
# automations-debug
c
@fresh-machine-37269 look at this pr description, it has screenshots. Describe them to me if you see them https://github.com/OpenHands/OpenHands/pull/18012
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Yep, all five screenshots are there and readable. Here's what they show: 1. Before — probe capture (
before-semantics-capture-17909.png
) A terminal-style capture of the 
control-openhands
probe run on 
main
(revision aeea97e). It confirms the bugs: • 
count role=main
returns 2 on each Settings page (nested landmarks). • 
aria-current
for the Customize sidebar row (
sidebar-skills-link
) is null on 
/mcp
, 
/skills
, 
/plugins
, 
/apps
. • After picking Français/Arabic, 
<html>
still reads 
lang=en dir=
(no dir at all). • The command-menu trigger's 
aria-label
is "Search commands" in every language. 2. Before — French UI (
before-french-ui-search-commands-untranslated.png
) A full app screenshot in French: "Paramètres → Application", sidebar in French ("Nouvelle discussion", "Personnaliser", "Automate", "Conversations"…). The top-left command-menu control still reads the English "Search commands" — the untranslated label the PR fixes. 3. After — probe capture (
after-semantics-capture-17909.png
) Same probe, now on the fix branch. All flipped: 
role=main
count is 1 on every Settings page; Customize now reports 
aria-current="page"
on 
/mcp
, 
/skills
, 
/plugins
, 
/apps
and stays 
null
on `/conversations`/`/automations`; 
lang=fr dir=ltr
, 
lang=ar dir=rtl
, `lang=en dir=ltr`; and the trigger label is now "Rechercher des commandes". It also shows four extra variants: live 
languageChanged
(fr without reload), Arabic after save + reload, a keyboard path on 
/mcp
(focus lands on the Customize link with 
aria-current=page
), and a 390×844 phone viewport with the mobile drawer open (drawer link still 
aria-current="page"
, 
role=main
count 1 on phone 
/settings
and 
/settings/app
). 4. After — French UI (
after-french-ui-search-commands-translated.png
) Same French Settings screen, but now the command menu reads "Rechercher des commandes", and a green "Settings saved" toast is visible. 5. After — Arabic UI (
after-arabic-html-dir-rtl.png
) The Arabic Settings > Application screen with the whole layout mirrored right-to-left, confirming 
<html dir="rtl">
. As the PR notes, the RTL flip exposes some physical left/right styling issues (e.g. switch thumbs sitting outside their tracks), which the description explicitly parks as out-of-scope follow-up work. This description was generated by an AI agent (OpenHands) on behalf of the user.
c
@fresh-machine-37269 Cool, please review the PR
👀 1
f
✅ Done! Here's my review — I read the diff, checked it against the repo's review guide, and ran things locally. Verdict: solid, no blocking findings. All four acceptance criteria from #17909 look correctly fixed. What the change does • 
settings.tsx
wrapper is now a 
<div>
(testid kept), so `SettingsLayout`'s 
<main>
is the single landmark. • 
SidebarNavLink
sets 
aria-current="page"
from the same 
active
value that drives its style; 
NavigationLink
now honors a caller-supplied 
aria-current
and otherwise behaves exactly as before. • New 
useSyncDocumentLanguage
hook (mounted only in the standalone 
root-layout
) mirrors i18next language → 
<html lang>
and 
i18n.dir()
→ 
<html dir>
on mount and on 
languageChanged
, with proper unsubscribe. • 
COMMAND_MENU$OPEN_LABEL
translated across all 15 locales. What I verified independently • 
npm ci
on the branch head, generated i18n, then ran the PR's exact vitest command: 31 files / 218 tests pass. • Ran the new/changed tests against current 
main
(6801911): 19 failures — 4 sidebar 
aria-current
, 12 locale-label, 1 root-layout hook, 1 settings single-
main
, 1 navigation-link caller override. That matches the PR's claim and confirms these are real regression tests, not tautologies. • 
check-translation-completeness.cjs
→ full coverage; Prettier clean. (ESLint wouldn't finish in my sandbox — OOM, not a code issue.) • Traced the `active`/`forceActive` logic: 
aria-current
and the active style stay in lockstep, and non-
forceActive
links fall through to the old behavior, so no other 
NavigationLink
caller changes. Non-blocking nits • 
aria-current="page"
on the Customize row (really a section container) is a defensible convention and matches the existing 
NavigationLink
behavior; `"true"`/`"location"` could also be argued. • The hook subscribes to the active i18n instance via the proxy; if an embedding host ever swapped instances with 
setI18n
, the subscription wouldn't move — but it's standalone-only, so not a concern here. • Brief 
en
→ selected-language flash on first paint is expected and harmless. The out-of-scope follow-ups the PR already discloses (
/shared/conversations/:id
outside 
root-layout
, RTL physical-utility issues, remaining 
COMMAND_MENU$*
keys) are correctly scoped out. Want me to post this as a formal GitHub review (or an approval) on the PR? This message was generated by an AI agent (OpenHands) on behalf of the user.
c
@fresh-machine-37269 yes please, feel free to approve if you see no major issues. do post the test so we can follow up if needed.
👀 1
f
✅ Done! Done — both are live on the PR: • Approving review posted: github.com/OpenHands/OpenHands/pull/18012#… — checklist of the four acceptance criteria, what I verified locally, and the non-blocking nits. • Test evidence comment posted: github.com/OpenHands/OpenHands/pull/18012#… — the exact vitest/translation commands, the 218/218 pass on the branch, and the 19 failing cases on 
main
broken out per suite for follow-up. Summary of the verification behind the approval: the branch's targeted suite passes 218/218, the translation-completeness check reports full coverage, and the new tests fail 19 ways on current 
main
, so they're real regression tests rather than tautologies. The remaining items (shared-conversation route, RTL physical utilities, other 
COMMAND_MENU$*
keys) are correctly deferred follow-ups. This message was generated by an AI agent (OpenHands) on behalf of the user.
Open in Slack
PreviousNext
Powered by