Repository navigation
Fix clipped ActionMenu/HelpTooltip popovers; kick off UI/UX overhaul (Phase 0) - #14
Merged
Merged
Conversation
…plan (Phase 0)
The row-action menu (⋮) rendered inside each table's overflow-x-auto card
and always opened downward, so a row near the bottom of the viewport had
its menu clipped or entirely invisible - the last row on Expenses always
did this. HelpTooltip had the same hand-rolled-positioning shape.
Rebuild both on bits-ui: ActionMenu on DropdownMenu, HelpTooltip on
Popover. Both are now portalled and collision-aware, so they flip upward
near the viewport edge instead of clipping. Their public props are
unchanged except the custom `trigger` snippet, which now receives props to
spread rather than an (open, toggle) pair - updated its one caller
(Income's "Add" button).
This is Phase 0 of a broader UI/UX overhaul, planned out in
foundational/PLAN_01_OVERVIEW.md and DESIGN.md ("Polymer" - see that doc
for the concept, palette and signature element). Phase 0 also:
- installs the shadcn-svelte + bits-ui foundation (dropdown-menu, popover,
button only for now), with the new components' tokens mapped to the
app's *existing* slate/indigo palette so nothing else changes visually
yet - Phase 1 does the real palette swap
- updates AGENTS.md with the plan/design pointers, a mandatory
/frontend-design step for UI work, a browser-screenshot recipe, and a
per-phase checks-and-reviews workflow matched to this repo's actual CI
(lint/typecheck/test/e2e - no review bot)
- adds e2e/action-menu.spec.ts, a real-browser regression test asserting
the last row's menu lands fully inside the viewport
jsdom can't lay out bits-ui's floating content (no real layout engine), so
unit tests needed a few adjustments - see ActionMenu.spec.ts's top-of-file
comment and DESIGN.md's decisions log for what and why. A global
src/tests/setup.ts reset (body style + bits-ui's dismissable-layer
registry) will matter again for every future bits-ui overlay.
Verified: pnpm verify and pnpm test:e2e both green; API coverage
unaffected (no API changes). Manually confirmed against the dev server
that the last row's menu on /expenses is now fully visible in both light
and dark mode.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Kilo is active on this repo (confirmed on PR #14) - it was just paused when Dev was previously stopped. My earlier pass had concluded (from checking PR #11's reviews/comments and repo files) that no review bot was configured here and rewrote the workflow accordingly; that was wrong. Restore the "green check isn't proof there's nothing to fix - read every comment, reply and resolve each thread individually" steps in AGENTS.md and PLAN_01_OVERVIEW.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI's test/e2e jobs were failing with "Invalid command exported from demo_seed.js file. Invalid URL" - unrelated to this branch's own changes, since it throws while apps/api's test bootstrap boots the ace Kernel (testUtils.db().migrate()), which scans every command file in commands/ regardless of what's being tested. Root cause (already diagnosed and fixed the same way in EveryList, commit 468045b): Node 24.20.0's loader changes broke @adonisjs/ace's command-metadata validator (adonisjs/ace#169) - its jsonschema-based validator resolves schema $refs against an implicit base URL that 24.20.0's stricter URL parsing rejects. Fixed upstream in ace 14.1.1 (adonisjs/ace#170, which gives the schema a real $id), but @adonisjs/core@7.3.3 still depends on ^14.1.0, which resolves to the broken 14.1.0 by default. - pnpm-workspace.yaml: override @adonisjs/ace to 14.1.1 project-wide, same mechanism already used there for a couple of other transitive deps. Verified locally on Node 24.18.1: still 640/640 passing - the fix is backward compatible, not just forward-compatible with 24.20.0. - Pin Node to 24.20.0 everywhere, matching EveryList: new .nvmrc, both package.json engines fields (>=24.20.0 <25), CI's node-version, and the Dockerfile's base image tag. The Docker image tag pin is cosmetic consistency rather than a required fix - the built image runs precompiled JS, not the .ts loader path this bug is actually in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Approval is now needed once per phase (the commit+push that opens the PR), not again for every fix-up commit responding to a broken CI check or review/Kilo feedback within that same PR - the owner will say up front if they want to hold off on a round instead. Merging, and starting a new piece of work, still need explicit confirmation. Also drops the "dev server must stay running" rule - the owner will ask to hold off ahead of time instead of this needing a standing rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Incremental review of Previous Review Summaries (3 snapshots, latest commit 1e44cb9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1e44cb9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 2a5753f)Status: No Issues Found | Recommendation: Merge All four findings from the previous review at Files Reviewed (6 files)
Previous review (commit 7e50b2b)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (62 files)Hand-written code:
Vendored/generated (shadcn-svelte CLI output, excluded from lint/coverage per Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 |
- ActionMenu.svelte / income/+page.svelte: the default trigger's (and
Income's custom trigger's) literal `class` attribute was overwriting
`{...props}`'s class after the spread, silently dropping a caller's
`class` prop even though it's still declared in Props. Merge with
`cn()` instead of overwriting.
- HelpTooltip.svelte: `role="tooltip"` on the popover content
contradicted PopoverTrigger's own `aria-haspopup="dialog"` semantics
(a true ARIA tooltip is non-interactive, hover-triggered description
text - this panel is a click-to-open, focus-managed popover). Use
`role="status"` instead, so the explanation is announced the moment
it appears rather than needing an ARIA role that doesn't match its
actual behaviour. Updated the one spec that queried by the old role.
- e2e/action-menu.spec.ts: the regression test only checked the first
menu item's (Edit) bounding box, which could pass in exactly the
clipping failure mode it exists to catch if a menu opened with just
enough room for its first item but not its later ones. Check the
menu container's own bounding box instead - if the container fits,
every item inside it does too.
- pnpm-workspace.yaml: fixed the override comment's cited
@adonisjs/core version (7.3.3 -> the actually-resolved 7.3.5).
Verified: pnpm verify and pnpm test:e2e both still green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Note the unrelated Node/ace CI fix and the Kilo Code Review round (4 found, 4 fixed, re-review confirmed clean) in the phase doc's Notes and deviations. Log the role="tooltip" -> role="status" a11y decision in DESIGN.md's decisions log for future click-to-reveal panels to follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Kilo caught it: the phase doc said Node was "pinned to 24.20.0 everywhere", but the two package.json engines fields set a >=24.20.0 <25 range - only .nvmrc, CI and the Dockerfile are an exact pin. Reworded to say that accurately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
What
Fixes the reported bug: the row-action menu (⋮) rendered inside each table's
overflow-x-autocard and always opened downward, so a row near the bottom of the viewport (the last row on Expenses, always) had its menu clipped or entirely invisible.HelpTooltiphad the same hand-rolled-positioning shape.ActionMenuandHelpTooltipare rebuilt on bits-ui (DropdownMenu/Popover) — both are now portalled and collision-aware, so they flip upward near the viewport edge instead of clipping. Public props are unchanged except the customtriggersnippet, which now receives props to spread rather than an(open, toggle)pair — its one caller (Income's "Add" button) is updated.Why this is also "Phase 0" of a bigger plan
This kicks off a broader UI/UX overhaul (the app's inconsistent CRUDL pages, duplicated markup, no shared component vocabulary). The full plan is in
foundational/PLAN_01_OVERVIEW.mdand the design direction ("Polymer") is infoundational/DESIGN.md. This PR itself only:dropdown-menu,popover,buttononly for now). Their tokens are mapped to the app's existing slate/indigo palette, not "Polymer" yet — nothing else changes visually. Phase 1 does the real palette swap.AGENTS.mdwith the plan/design pointers, a mandatory/frontend-designstep for UI work, a browser-screenshot recipe (with a note about the login rate limit), and a per-phase checks-and-reviews workflow matched to this repo's actual CI (lint/typecheck/test/e2e — no review bot configured here).e2e/action-menu.spec.ts, a real-browser regression test that opens the last row's menu at a short viewport and asserts it lands fully inside it.Notes
ActionMenu.spec.ts's top-of-file comment andDESIGN.md's decisions log for the full explanation. A globalsrc/tests/setup.tsreset (body style + bits-ui's dismissable-layer registry) will matter again for every future bits-ui overlay (Dialog, Sheet, Drawer, Select, Tabs in later phases)./expensesis now fully visible, in both light and dark mode.Verification
pnpm verify(lint + typecheck + test, both apps) — greenpnpm test:e2e— green, 17/17 including the new regression test🤖 Generated with Claude Code