Skip to content

fix: review follow-ups for #228 and #259, plus the actions-runtime prototype lookup (#261) - #263

Merged
gilav21 merged 23 commits into
masterfrom
fix/review-followups-228-259-261
Nov 8, 2026
Merged

gilav21 merged 23 commits into
masterfrom
fix/review-followups-228-259-261

Conversation

@gilav21

@gilav21 gilav21 commented Nov 7, 2026

Copy link
Copy Markdown
Owner

What this is

Two independent code reviews of PRs that were merged without one (#228 and #259) found real defects in our own fixes, and the gil-elements agent filed one more (#261). Every finding was a hypothesis from reading code: each was validated (in real Chromium where it concerned layout, focus, caret or key handling) before changing anything. Three branches, one commit per finding, merged here.

Fixes #261.

Review of #259 (10 findings: 8 real and fixed, 1 judged no change, 1 doc-only)

  • input-mask caret: the new valueChanges subscription (format the initial value) also ran on every keystroke before handleInput, so the form's echo reformatted the text and threw the caret to the end whenever you typed mid-string. A capture-phase userEditing flag now keeps formatModelValue to programmatic writes.
  • Bento grip: it was shown only under (pointer: coarse), but touch drag now requires the grip, so a touch laptop (fine primary pointer, coarse secondary) lost touch reordering. It shows under (any-pointer: coarse), and its label is a new dragHandleLabel input (default "Drag to reorder") instead of hard-coded English.
  • navigation-menu: onPanelKey hijacked ArrowUp/Down/Home/End for any target in a panel, including an <input>; it now leaves keys to anything that is not a panel link.
  • Select Tab (changes navigation-menu has no arrow-key navigation or Escape, and focus can leave an open panel behind #236's behaviour, the Elements agent is holding this one): Tab in a projected select no longer preventDefaults and re-focuses the trigger (that cost a second Tab); focus returns to the trigger first, the panel closes without committing (like the data-driven path), and the same Tab leaves the select. The options-driven path (restoreFocusFromPanel) matches.
  • typing-animation: the empty-list guard ended the timer chain for good (['a','b'] -> [] -> ['c'] froze); it restarts when the list becomes non-empty and clears the text when emptied.
  • particles: the click-through default was an inline style that beat a consumer class="pointer-events-auto"; it is a host class now (consumer class last), same default.
  • emoji-picker settle: counting stable frames before the smooth scroll had started could cut it short; it now needs observed movement first, still bounded.
  • shortcut dialog: [&>h3] never matched through the display: contents accordion trigger; it is [&_h3] now (the test env has no Tailwind, so there is no committed geometry test for this one).
  • chart-interaction: doc comment said legend hovers anchor at the target centre; they anchor at the pointer, and the comment now says so.
  • Custom shortcut locale renders blanks: no change. Nothing in the library falls back per key (resolveLocale returns a custom object as-is), so the type check is the guard; a per-key fallback is a library-wide decision.

Review of #228 (9 findings: 7 real and fixed, 2 not applicable)

  • Tabs: arrow/Home/End ignored modifiers and hijacked Alt+Left (browser Back), Ctrl/Cmd+Arrow and Shift+Home; they are ignored when alt/ctrl/meta/shift is held. Arrow navigation focuses the target and calls selectTab (via a new data-value) instead of a synthetic click(), so a held arrow no longer fires consumer click handlers.
  • Switch: the dead translate-x-0 conditional is gone, round() is behind @supports with a valid fallback (the Tailwind v4 baseline is Chrome 111, round() needs 125), and the thumb is height: 100%; aspect-ratio: 1 instead of a hard-coded 4px, so a consumer border class cannot mis-size it.
  • Pagination: custom-mode previous/next kept h-9 px-4 py-2 while simple mode scaled with density; two density stylesheets now drive them (identical at density 1 by arithmetic, no screenshots), and the dead data-size attributes on the simple-mode buttons are removed.
  • Timeline: :last-child gave the real last item padding when a trailing non-item sat after it; it is :last-of-type now (a trailing sibling with the same tag as an item would still count: a gap on the container would be sturdier but changes the spacing contract).
  • Stepper: docs only. nextStep() keeps bypassing canNavigateTo (widening the gate would make the next step's trigger clickable in linear mode), and the docs now say so.
  • Rating: focusRangeInput uses a viewChild; touch focus stays (a range input opens no virtual keyboard).
  • Not applicable / not confirmed: tab orientation (tabs has no orientation concept) and the RTL-direction claim (computed direction inherits from the ancestor dir).

#261 (RTE actions runtime)

options.handlers[event.actionId] walked the prototype chain, and the id comes from untrusted rendered HTML (ID_PATTERN accepts __proto__, constructor, ...): __proto__ and __defineGetter__ threw before the '*' catch-all ran, and toString/valueOf/constructor/hasOwnProperty were called with the click event. Handlers are looked up with Object.hasOwn (a handler the app registers as toString still works), tested for click and hover over six names. The class audit found one more instance: the paste normalizer's sizeMap[size] turned <font size="constructor"> into font-size: function Object()...; it is a Map now. Other lookups in the editor tree were checked and are safe (uppercase DOM names, gated by a Set, or app-supplied keys).

Verification

  • npm run sonar:gate on e543bd1e: coverage current on both legs (12,345 browser + 2,059 CLI tests), scanned revision equals HEAD, quality gate OK, 0 new issues, new coverage 92.4%. This is the first time the three branches ran together.
  • The data-table package closure is now 188 files (two new pagination stylesheets); the spec's hard-coded count was updated (its set-equality assertion already held).
  • e2e: impact analysis says "all" specs (shared components), so GitHub CI is the full e2e run for this PR.
  • Mutation-checked by each agent (breaking a fix turned a test red) except where noted: the shortcut-dialog selector and the bento any-pointer media query have no committed test (the test env has no Tailwind and Chromium emulation flips both pointer features together).

Release consequences

Behaviour the gil-elements port should mirror

Select Tab (no preventDefault, close without commit, same Tab leaves); bento grip under (any-pointer: coarse) plus the dragHandleLabel input; nav-menu panel keys ignore non-link targets; typing-animation restarts when the list refills; particles click-through via a host class a consumer class can override; emoji settle needs observed movement; tabs ignore modified keys and select via the component, not a click; switch thumb sizing; pagination custom-mode sizing via density; timeline :last-of-type; actions lookup by own property and a Map for pasted font sizes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H6YokEbJxTcCyzsoYgCSQg

gilav21 and others added 23 commits November 7, 2026 18:10
…finding 1)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…synthetic click (review of #228, finding 3)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…op dead class (review of #228, finding 4)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
… custom mode (review of #228, finding 5)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…ons (review of #228, finding 6)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…(review of #228, finding 7)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…eave (review of #228, finding 8)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…ding 9)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
… own edit (review of #259, finding 1)

The form's input listener pushes the raw text through valueChanges before the mask re-formats it; re-masking there threw the caret to the end. A capture-phase flag tells a user edit from a programmatic write.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…nt (review of #259, finding 2)

ui-accordion-trigger sits between the row and its h3, so [&>h3] never matched.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…label an input (review of #259, finding 3)

A touch laptop has a fine primary pointer, so (pointer: coarse) hid the only touch drag affordance.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…nel (review of #259, finding 4)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…eview of #259, finding 5)

Focus goes to the trigger, the panel closes and the browser's Tab walks on from the trigger, in both the projected and the options-driven panel.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…filled (review of #259, finding 6)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…lass can override it (review of #259, finding 8)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…led one (review of #259, finding 9)

Stillness only counts as arrival after a seen movement or a bounded start grace; the SETTLE caps still bound the scroll-spy suppression.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…s the code does (review of #259, finding 10)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokCTX5ussLb7WYcDLug
…entries (#261)

Invariant: a key taken from content resolves only to an entry the app or
library registered itself, never an inherited Object.prototype name.
actions-runtime now uses Object.hasOwn; the pasted <font size> lookup is a Map.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokEbJxTcCyzsoYgCSQg
…us/next stylesheets

The #228 review fix moved the custom-mode previous/next sizing into two density stylesheets;
pagination is in the data-table closure, so it gained two files (the set-equality assertion in the
same spec already held; only the hard-coded count was stale).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6YokEbJxTcCyzsoYgCSQg
@netlify

netlify Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for shadcn-angular-demo ready!

Name Link
🔨 Latest commit e543bd1
🔍 Latest deploy log https://app.netlify.com/projects/shadcn-angular-demo/deploys/6aef9c91f0432a926e0765dc
😎 Deploy Preview https://deploy-preview-263--shadcn-angular-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@gilav21
gilav21 merged commit 663215f into master Nov 8, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RTE actions runtime: handler lookup walks the prototype chain, so a crafted action id (e.g. __proto__) throws or runs Object.prototype methods

1 participant