fix(color-wheel-flower): render standalone instead of crashing the page - #288
Closed
pathscale wants to merge 1 commit into
Closed
fix(color-wheel-flower): render standalone instead of crashing the page#288pathscale wants to merge 1 commit into
pathscale wants to merge 1 commit into
Conversation
`ColorWheelFlower` is exported from `@pathscale/ui/lab`. Rendering one outside a `ThemeColorPicker` threw, and in Solid 2 a throw during render halts the reactive system: one component on one route blanked an entire application. The showcase page for it in js.software was dead, with `ContextNotFoundError` and `REACTIVITY_HALTED` in the console and an empty body. Two things had to be true for that. The context was declared `createContext<T | undefined>(undefined)`. In Solid 2 that is the *default-less* form, and the absence of a default is precisely what makes `useContext` throw. It was paired with a hook that checked for a missing value and threw a friendlier error, which could never run because `useContext` threw first -- and whose message named `ColorPickerContext.Provider`, which Solid 2 does not have. The default is now `null` and the hook returns it. The component then dereferenced that context unconditionally. It now falls back to its own props: `color` / `defaultColor` for the selection, `disabled`, and `onChange`. Inside a `ThemeColorPicker` nothing changes, the surrounding context still owns the state. Outside one, a bare `<ColorWheelFlower />` renders an uncontrolled flower starting at white. `tests/components/optional-context-defaults.test.ts` did not cover this on purpose: it exempts contexts that are dereferenced directly, on the argument that a default would trade a clear error for a null-property crash. That argument holds for an internal subcomponent and not for an exported one, and this was exported. The verifier is a ps-qa check, because the thing to prove is that it renders. It asserts the centre petal by name rather than the component name, which the harness renders on a labelled wrapper whether or not anything mounted -- the first version of this check passed against the unfixed component, which is how that was found. With the context restored to default-less it fails: no node matching "radio:Reset to neutral" exists in the tree
This was referenced Sep 7, 2026
Owner
Author
|
Rolled into #289 so the four fixes ship as one release. The commit is unchanged. |
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.
ColorWheelFloweris exported from@pathscale/ui/lab. Rendering one outside aThemeColorPickerthrew, and in Solid 2 a throw during render halts the reactive system — so one component on one route blanked an entire application.Found by driving js.software's showcase in a browser:
/color-pickerrendered an empty<body>, withContextNotFoundErrorand[REACTIVITY_HALTED]in the console.Two things had to be true
The context had no default. It was declared
createContext<T | undefined>(undefined), and in Solid 2 that is the default-less form — the absence of a default is exactly what makesuseContextthrow. It was paired with a hook that checked for a missing value and threw a friendlier error, which could never run becauseuseContextthrew first, and whose message namedColorPickerContext.Provider, which Solid 2 does not have. The default is nownulland the hook returns it.The component dereferenced it unconditionally. It now falls back to its own props —
color/defaultColor,disabled,onChange. Inside aThemeColorPickernothing changes: the surrounding context still owns the state. Outside one, a bare<ColorWheelFlower />renders an uncontrolled flower starting at white.Why the existing test did not catch it
tests/components/optional-context-defaults.test.tsexempts contexts that are dereferenced directly, arguing that a default would trade a clear error for a null-property crash further from the cause. That argument holds for an internal subcomponent. It does not hold for one the package exports, and this one is exported.The verifier
A ps-qa check, because what needs proving is that it renders. It asserts the centre petal by name, not the component name: the harness renders the component name on a labelled wrapper whether or not anything mounted, and the first version of this check passed against the unfixed component. That is how the toothless version was caught.
With the context restored to default-less, the check fails:
Verification
color-wheel-flowerps-qa group, headless profilebun run testbunx tsc --noEmitbun run lint:layoutsbun run lint:codemaster; no findings in any file this touchesRelease
This wants a 3.1.1. js.software's
/color-pickerroute stays dead until it ships, and its migration PR is held behind that.