ReviewOS

also looking at this

stacks/stx

fix(components): Progress max/indeterminate and Tooltip disabled are live props

#2008
Merged glennmichael123 wants to merge fix/progress-tooltip-reactive-props into main
5 files +242 -17
9 days ago

Follows #1997, which did this for Button. Checked all four components that issue speculated about, and the picture was not what it guessed:

componentmain state prop
Progressvaluealready reactive since #1704
Tooltipshowalready reactive since #1704
Notificationvisibilityimperative by design, not frozen
Skeletonanimateno <script client> at all

So the real gap was the props beside the ones already fixed.

What changed

Progress.max, Progress.indeterminate and Tooltip.disabled now come from useReactiveProp.

This matters most where the frozen value is the one that cannot be known at render time:

<Progress :max="total()" :indeterminate="!total()" :value="done()" />

A bar is indeterminate because the total is not known yet — so the moment the total arrives is the moment both props must change, and both were the ones that could not. The bar spun forever and aria-valuemax kept reporting a denominator the component was no longer dividing by.

The circular bar's spin follows the signal too, since the spin is part of what indeterminate means, and aria-valuemax is bound alongside its server attribute so it is right before and after hydration.

A trap worth recording

Naming the client local indeterminate — matching the server export — stops the server interpolating in the template. It shipped as literal text. I only caught it because a test asserted on the rendered class.

useReactiveProp watches the attribute named in its first argument, so the local is free to be called something else, and here it has to be: maxValue and isIndeterminate.

Related: a comment in a client script containing the literal text <script server> makes server-cache-purity.test.ts match it as a second server block and parse the prose as identifiers. Cheap to avoid once known, confusing until then.

Not changed, deliberately

Skeleton has no <script client>. Making animate reactive means adding one for a prop whose realistic use is toggling the skeleton itself, not its animation.

Notification is imperative by design — defineExpose({ open, close, isVisible, … }) plus an auto-hide timer. A reactive show would be a second, competing way to drive it that could fight open()/close() and the timer. That is an API decision rather than a bug, so I have left it alone — happy to add it if you want both.

Tests

packages/components/test/progress-tooltip-reactive-state.test.ts drives the real runtime through the SPA harness with a parent owning the signals:

  • indeterminate while the total is unknown, determinate once it arrives
  • animate-spin dropped from the circular bar
  • aria-valuemax updated for assistive tech
  • the percentage recomputed against the live max (3 of 10, not 3 of the 1 it was built with)
  • a tooltip that stops showing once disabled flips after render

Full package suite: 1220 pass, 0 fail. progress-template and tooltip-template snapshots regenerated per the procedure their helper documents.

5 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.