https://docs.all-hands.dev/ logo
Join Slack
Powered by
<@U0BME18CF7S> please review <https://github.com/O...
# automations-debug
c
@fresh-machine-37269 please review https://github.com/OpenHands/OpenHands/pull/18006 and post your review, with approve / request changes / comment as you think appropriate.
šŸ‘€ 1
f
šŸ¤– On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/ā€¦
āœ… Done! Done ā€” I reviewed PR #18006 and posted an approval (APPROVED on commit 
1d94b9d
). Summary of my take: ā€¢ Sound and correctly scoped. The diff is limited to 
eslint.config.js
and one new regression test ā€” no 
src/**
, dependency, or rendered-color changes. 
shadcn/no-raw-colors
is enabled at 
warn
with exactly eight verified HeroUI/legacy JS-config exemptions (exact-match, no palette or namespace wildcards). ā€¢ Verified locally. I ran the shipped config against a probe: raw palette classes (
text-red-500
, 
bg-amber-500
), an unknown token, and an SVG literal (
fill="#ec4899"
) all warn at severity 1; the eight exempt classes stay silent; the misspelled 
bg-content9
still warns. The focused vitest suites pass (2 files / 4 tests), and 
test-and-build (ubuntu)
is green on this head. ā€¢ Prior concern resolved. The earlier "request changes"/comment was about the failing 
Validate PR description
screenshot check. That check now passes on the current head, and the acceptance criteria for issue #18005 are all met, so it no longer blocks. One small note I flagged: I couldn't reproduce a full 
eslint src
in my sandbox (Node OOMs on the whole tree), but the focused real-config probe plus the passing CI check cover the behavior this PR changes. The 148 remaining findings are appropriately deferred to #18004.
c
@fresh-machine-37269 please review this too https://github.com/OpenHands/OpenHands/pull/16574 and similar post review
šŸ‘€ 1
f
āœ… Done! Done ā€” I reviewed PR #16574 and posted an approval (APPROVED on commit 
53b56ab
). My take: ā€¢ Clean, conservative test sweep. It removes mirror/tautology tests (fixture shapes, i18n label literals, launcher default-constant assertions, class-string mirrors) and pulls back the production exports those tests were the only consumer of. I verified every removed export is either still used internally or has no remaining reference at all. ā€¢ Verified locally. The retained suites pass: hover-reveal (2, including real Tailwind 
@media
compilation), dev-with-automation (63, including the new ephemeral-port busy-ingress case), and the sibling conversation-tab-content suite (11, which renders the component composing the deleted crossfade test). The affected test directories run 1463 passing; the only two failures are pre-existing and environmental (sandbox has no home dir, so the "uvx missing" subprocess tests error). 
test-and-build (ubuntu)
is green and the PR is mergeable. ā€¢ One process note, not a code issue: the PR description talks about ACP onboarding credential-policy work and a 
vitest.environment.ts
rework, but neither is in this PR's diff ā€” 
vitest.environment.ts
doesn't exist at this head. The description looks written against the branch's full history rather than the current base, so it's worth trimming. I also flagged two non-blocking coverage gaps (the now-deleted 
table-row-classes
contract, and the touch-default halves of the hover-reveal helpers) as informational only.
Open in Slack
PreviousNext
Powered by