Skip to content

Commit 791d116

Browse files
ivanbanovclaude
andcommitted
refactor(solid): simplify the code comments
Cut the comments down to the constraint each one protects β€” one or two lines, no narration. Comment-only change on top of the Solid substrate (#44); no code changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent e8667f4 commit 791d116

17 files changed

Lines changed: 85 additions & 149 deletions

File tree

β€Žknip.config.tsβ€Ž

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,15 @@ const config: KnipConfig = {
1717
'packages/react/*': {
1818
entry: ['stories/*.stories.tsx'],
1919
},
20-
// knip's storybook plugin doesn't know the community solid framework, so
21-
// the harness config is declared an entry by hand. jest-dom is loaded via
22-
// a setup file vite-plugin-solid injects by bare specifier.
20+
// knip's storybook plugin doesn't know the community solid framework;
21+
// jest-dom is loaded via a setup file vite-plugin-solid injects.
2322
'packages/solid': {
2423
entry: ['.storybook/main.ts', '.storybook/manager.ts'],
2524
ignoreDependencies: ['@testing-library/jest-dom'],
2625
},
2726
'packages/solid/*': {
2827
entry: ['stories/*.stories.tsx'],
29-
// The babel presets are referenced by name (strings) in each package's
30-
// tsdown.config.ts β€” invisible to import analysis.
28+
// The babel presets are referenced as strings in tsdown.config.ts.
3129
ignoreDependencies: ['babel-preset-solid', '@babel/preset-typescript'],
3230
},
3331
},

β€Žpackages/solid/dialog/src/context.tsβ€Ž

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,30 +2,21 @@ import { createContext, useContext, type Accessor, type Context } from 'solid-js
22
import type { DialogApi, DialogMachine } from '@dunky.dev/dialog'
33

44
export interface DialogContextValue {
5-
// The connected api as a fine-grained store proxy β€” reading a field in JSX
6-
// or an effect subscribes to exactly that leaf.
5+
// Fine-grained store proxy: reading a field subscribes to exactly that leaf.
76
api: DialogApi
87
machine: DialogMachine
9-
// Nesting level (1 = top-level). Decides the topmost dialog of a stack for
10-
// Escape, focus, and assistive-tech containment.
8+
// Nesting level (1 = top-level); decides the topmost dialog of a stack.
119
depth: number
12-
// The element the Portal teleported into, or null for the page body β€”
13-
// Content scopes the scroll lock to it. An accessor so the Portal's prop
14-
// stays live: the root provides null; Portal re-provides the context with
15-
// the field filled in.
10+
// The Portal's container (null = page body); an accessor so it stays live.
1611
container: Accessor<HTMLElement | null>
17-
// The rendered Backdrop element, shared because Backdrop and Content are
18-
// sibling parts: Content's stack entry excepts its own backdrop from the
19-
// containment so it stays pressable while its dialog is topmost. A plain
20-
// mutable box, not a signal: the layer walk reads it synchronously inside
21-
// the same settle that mounts the backdrop, before a signal write would
22-
// commit.
12+
// The rendered Backdrop, shared so Content's stack entry can except it from
13+
// the containment. A plain box, not a signal: the layer walk reads it in the
14+
// same settle that mounts the backdrop, before a signal write would commit.
2315
backdropRef: { current: HTMLDivElement | null }
2416
}
2517

26-
// The `null` default keeps the root's parent-lookup non-throwing (depth
27-
// derives from an optional read; a default-less context throws on it); the
28-
// wrapper below restores the loud error for parts, naming the component.
18+
// A `null` default: a default-less context throws on the root's optional
19+
// parent lookup; the wrapper restores the loud error for parts.
2920
export const DialogContext: Context<DialogContextValue | null> =
3021
createContext<DialogContextValue | null>(null)
3122

β€Žpackages/solid/dialog/src/dialog.tsxβ€Ž

Lines changed: 32 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,13 @@ import { mergeProps, normalize } from '@dunky.dev/solid-state-machine'
2525
import { DialogContext, useDialogContext } from './context'
2626
import { useDialog } from './use-dialog'
2727

28-
// A part's bindings merge INSIDE the JSX spread: the compiler wraps the
29-
// expression in a reactive scope, so a machine transition re-translates it.
30-
// `children` never rides that spread β€” a re-evaluated spread re-CREATES the
31-
// children it carries, and a child whose lifecycle writes to the machine
32-
// (Title's presence) would then loop machine -> spread -> remount -> machine.
33-
// Every part strips it and renders `{props.children}` explicitly.
34-
35-
// A consumer ref that crossed a component boundary is a setter function or an
36-
// array of them (Solid 2.0 refs are functions; arrays compose); apply it
37-
// alongside the part's own element capture.
28+
// Bindings merge inside the JSX spread so they stay reactive. `children` must
29+
// never ride that spread: a re-evaluated spread re-creates the children, and a
30+
// child that writes to the machine on mount (Title) would loop forever. Every
31+
// part omits it and renders `{props.children}` explicitly.
32+
33+
// A consumer ref that crossed a component boundary is a function or an array
34+
// of functions.
3835
function applyConsumerRef<T>(ref: Ref<T> | undefined, element: T): void {
3936
if (typeof ref === 'function') (ref as (element: T) => void)(element)
4037
else if (Array.isArray(ref)) for (const entry of ref) applyConsumerRef(entry as Ref<T>, element)
@@ -56,11 +53,8 @@ export const Dialog: Component<DialogProps> & Parts = props => {
5653
const backdropRef: { current: HTMLDivElement | null } = { current: null }
5754

5855
// closeOnBack: while open, a guard entry in the session history turns the
59-
// host's Back into a dismissal instead of a navigation. Every decision
60-
// (gate, veto, controlled) lives in the core's backNavigate; this effect
61-
// only wires the web mechanics. It tracks `api.open` alone β€” fresh callback
62-
// identities never churn real session-history entries. It lives on the
63-
// root β€” the guard concerns the dialog's openness, not any rendered part.
56+
// browser's Back into a dismissal. The decision (gate, veto, controlled)
57+
// lives in the core's backNavigate; this only wires the web mechanics.
6458
createEffect(
6559
() => api.open,
6660
open => {
@@ -111,8 +105,8 @@ export const Portal: Component<DialogPortalProps> = props => {
111105
const context = useDialogContext()
112106
if (isServer) return null
113107
return (
114-
// `mounted`, not `open`: an animated dialog stays in the tree through
115-
// `closing` so its exit visual can play before everything unmounts.
108+
// `mounted`, not `open`: an animated dialog stays mounted through
109+
// `closing` so its exit visual can play.
116110
<Show when={context.api.mounted}>
117111
{/* keyed: the host portal's mount is fixed at creation, so a container
118112
swap re-creates the portal on the new target. */}
@@ -140,8 +134,6 @@ export interface DialogBackdropProps extends ComponentProps<'div'> {}
140134
export const Backdrop: Component<DialogBackdropProps> = props => {
141135
const { api, machine, backdropRef } = useDialogContext()
142136
const rest = omit(props, 'ref', 'children')
143-
// The shared slot must not outlive the element: the context box lives on
144-
// the root, the element only until this part's owner disposes.
145137
onSettled(() => () => (backdropRef.current = null))
146138

147139
const bindings = (): Record<string, unknown> => {
@@ -189,9 +181,8 @@ export const Viewport: Component<DialogViewportProps> = props => {
189181
} & Record<string, unknown>
190182
return {
191183
...attrs,
192-
// Content presses bubble up here β€” only a press that started on the
193-
// viewport itself is an outside interaction, and only the topmost dialog
194-
// of a stack answers it.
184+
// Only a press that started on the viewport itself is an outside
185+
// interaction, and only the topmost dialog of a stack answers it.
195186
onClick: (event: MouseEvent) => {
196187
if (event.target !== event.currentTarget) return
197188
if (!isTopmostLayer(machine.context.id)) return
@@ -210,8 +201,7 @@ export const Viewport: Component<DialogViewportProps> = props => {
210201

211202
export interface DialogContentProps extends ComponentProps<'div'> {
212203
/** The element to focus when the dialog opens β€” an element, or an accessor
213-
* resolved at open time (the Solid idiom for a ref variable that fills
214-
* during render). @default the dialog window */
204+
* resolved at open time. @default the dialog window */
215205
initialFocus?: HTMLElement | (() => HTMLElement | null | undefined)
216206
}
217207

@@ -223,14 +213,11 @@ export const Content: Component<DialogContentProps> = props => {
223213
const rest = omit(props, 'ref', 'initialFocus', 'children')
224214
let contentEl: HTMLDivElement | undefined
225215

226-
// The machine's `open` state is the edge, not mount/unmount β€” an animated
227-
// dialog stays mounted through `closing`, and the stack, containment, and
228-
// focus must release the moment the exit starts, not when it finishes.
229-
// One effect keeps the ordering right both ways: the stack joins before focus
230-
// moves in, and on close it must release the layers beneath (un-inert them)
231-
// before focus can move back out to one of them. Apply-phase reads go
232-
// through untrack β€” `api.open` is the one edge; the options must not re-run
233-
// the effect.
216+
// The `open` state is the edge, not mount/unmount: an animated dialog stays
217+
// mounted through `closing`, and the stack, containment, and focus must
218+
// release the moment the exit starts. One effect keeps the order right both
219+
// ways: the stack joins before focus moves in; on close it releases the
220+
// layers beneath before focus moves back out.
234221
createEffect(
235222
() => api.open,
236223
open => {
@@ -246,13 +233,12 @@ export const Content: Component<DialogContentProps> = props => {
246233
backdrop: () => backdropRef.current,
247234
})
248235

249-
// preventScroll everywhere: the scroll lock already froze the surface, so
250-
// moving focus must not scroll it β€” otherwise opening jumps the (top-of-
251-
// container) dialog into view and closing jumps back to the trigger.
236+
// preventScroll everywhere: moving focus must not scroll the locked
237+
// surface, or open/close jumps the view.
252238
const target =
253239
untrack(() => resolveInitialFocus(props.initialFocus)) ?? getInitialFocus(content)
254240
target.focus({ preventScroll: true })
255-
// A target that can't take focus (disabled, hidden) falls back to the panel.
241+
// A target that can't take focus falls back to the panel.
256242
if (document.activeElement !== target) content.focus({ preventScroll: true })
257243

258244
return () => {
@@ -262,10 +248,9 @@ export const Content: Component<DialogContentProps> = props => {
262248
},
263249
)
264250

265-
// The exit window: Content live while not open only happens in `closing`.
266-
// The layer has already released everything above, so hide the still-painting
267-
// layer from interaction and report when its visual is done; the cleanup is
268-
// the reopen interrupt (and final unmount) undoing both.
251+
// The exit window: mounted while not open only happens in `closing`. Hide
252+
// the still-painting layer and report when its visual is done; the cleanup
253+
// is the reopen interrupt (and final unmount) undoing both.
269254
createEffect(
270255
() => api.open,
271256
open => {
@@ -284,24 +269,19 @@ export const Content: Component<DialogContentProps> = props => {
284269
)
285270

286271
// The lock spans the whole mount β€” through `closing` too: releasing it
287-
// mid-exit would bring the scrollbar back and reflow the page under the
288-
// still-painting layer. A scoped dialog locks its portal container; a page
289-
// dialog locks the body.
272+
// mid-exit would reflow the page under the still-painting layer.
290273
useScrollLock(() => machine.context.modal, container)
291274

292275
useFocusTrap(() => contentEl ?? null, {
293-
// Only a modal dialog traps, and only while topmost β€” a nested dialog
294-
// owns focus while open.
276+
// Only a modal dialog traps, and only while topmost.
295277
enabled: () => machine.context.modal && isTopmostLayer(machine.context.id),
296-
// The Close part is the cycle's last stop wherever it renders (core
297-
// SPEC); found by its derived id.
278+
// Close is the cycle's last stop wherever it renders (core SPEC).
298279
last: () => document.getElementById(api.ids.close),
299280
})
300281

301-
// A neutral element with the role, not <dialog>: the window is the initial
302-
// focus target, so it carries tabindex β€” which HTML forbids on <dialog> β€”
303-
// and the native element only pays off via showModal(), which this contract
304-
// deliberately doesn't use.
282+
// A neutral element with the role, not <dialog>: the window carries
283+
// tabindex (forbidden on <dialog>), and this contract doesn't use
284+
// showModal() β€” see SPEC.md.
305285
return (
306286
<div
307287
{...mergeProps<DialogContentProps>(rest, normalize(api.parts.content))}
@@ -325,8 +305,7 @@ export const Title: Component<DialogTitleProps> = props => {
325305
const { api, machine } = useDialogContext()
326306
const rest = omit(props, 'children')
327307

328-
// Presence reports from the settled phase: the machine starts on the root's
329-
// settle, which owner order puts before this one.
308+
// onSettled: the machine starts on the root's settle, which runs first.
330309
onSettled(() => {
331310
machine.send({ type: 'part.presence', part: 'title', present: true })
332311
return () => machine.send({ type: 'part.presence', part: 'title', present: false })

β€Žpackages/solid/dialog/src/effects.tsβ€Ž

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,14 @@ import type { DialogMachine, DialogOptions } from '@dunky.dev/dialog'
33
import { dialogEffects } from '@dunky.dev/dialog'
44
import { isTopmostLayer } from '@dunky.dev/dom-overlay'
55

6-
// Substrate effects: the core's substrate-free list (the controlled-open
7-
// echo) plus the document-level work only this host can own.
86
type DialogEffect = ComponentEffect<DialogMachine, DialogOptions>
97

10-
// Escape is a document-level concern, not a part's β€” it must work wherever
11-
// focus is.
8+
// Escape is a document-level concern β€” it must work wherever focus is.
129
const trackEscape: DialogEffect = [
1310
(machine, props) => {
1411
const onKeyDown = (event: KeyboardEvent): void => {
1512
if (event.key !== 'Escape' || !machine.matches('open')) return
16-
// Only the topmost dialog answers Escape β€” a nested stack closes one
17-
// layer at a time.
13+
// Only the topmost dialog answers Escape β€” one layer per press.
1814
if (!isTopmostLayer(machine.context.id)) return
1915
props.onEscapeKeyDown?.(event)
2016
if (!event.defaultPrevented) machine.send({ type: 'escape' })
@@ -25,4 +21,5 @@ const trackEscape: DialogEffect = [
2521
['onEscapeKeyDown'],
2622
]
2723

24+
// The core's substrate-free effects plus the document-level work of this host.
2825
export const solidDialogEffects: DialogEffect[] = [...dialogEffects, trackEscape]

β€Žpackages/solid/dialog/src/use-dialog.tsβ€Ž

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,8 @@ import { solidDialogEffects } from './effects'
77

88
export function useDialog(options: DialogOptions): { api: DialogApi; machine: DialogMachine } {
99
const id = createUniqueId()
10-
// `?? id` (via a live getter, not merge order): an explicit `id={undefined}`
11-
// must not knock out the generated fallback β€” ids also key the dialog stack,
12-
// so they must exist. `merge` keeps the rest of the options a reactive proxy.
10+
// `?? id` via a live getter: an explicit `id={undefined}` must not knock out
11+
// the generated fallback β€” ids also key the dialog stack.
1312
const props = merge(options, {
1413
get id() {
1514
return options.id ?? id

β€Žpackages/solid/dialog/tests/dialog.test.tsxβ€Ž

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ const DefaultDialog = (props: DialogProps) => (
2323
</Dialog>
2424
)
2525

26-
// Solid 2.0 defers store commits + DOM updates to the microtask queue β€” every
27-
// interaction flushes before the test reads the tree.
26+
// Solid 2.0 defers store commits to the microtask queue β€” flush after every
27+
// interaction before reading the tree.
2828
const press = (element: HTMLElement): void => {
2929
element.click()
3030
flush()

β€Žpackages/solid/dialog/tsdown.config.tsβ€Ž

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
import { babel } from '@rollup/plugin-babel'
22
import { defineConfig } from 'tsdown'
33

4-
// Solid JSX needs Solid's own compiler: babel-preset-solid turns JSX into
5-
// reactive templates + effects, which neither oxc nor rolldown's React-shaped
6-
// JSX transform can produce. The plugin transforms .tsx before rolldown's own
7-
// transform sees it; everything else (entry, dts, publint) inherits the root
8-
// config. Babel applies presets last-to-first: TypeScript strips types while
9-
// keeping the JSX (isTSX), then the Solid preset compiles it.
4+
// Solid JSX needs Solid's own compiler (babel-preset-solid) β€” rolldown/oxc
5+
// only know React-shaped JSX. Presets apply last-to-first: TypeScript strips
6+
// types keeping the JSX, then the Solid preset compiles it. Everything else
7+
// inherits the root config.
108
export default defineConfig({
119
plugins: [
1210
babel({

β€Žpackages/solid/hooks/use-focus-trap/src/use-focus-trap.tsβ€Ž

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,17 @@ export interface UseFocusTrapOptions extends TrapFocusOptions {}
66

77
/**
88
* Traps Tab / Shift+Tab within `target` while it holds an element β€” the Solid
9-
* lifecycle around `trapFocus`. The trap follows the accessor: it arms when
10-
* the target first yields an element, releases when it clears or the owner is
11-
* disposed, and re-arms on a new element when the accessor is reactive.
9+
* lifecycle around `trapFocus`. Arms when the target yields an element,
10+
* releases on dispose, re-arms when a reactive accessor yields a new one.
1211
*/
1312
export function useFocusTrap(
1413
target: () => HTMLElement | null | undefined,
1514
options: UseFocusTrapOptions = {},
1615
): void {
17-
// The compute tracks a reactive target (re-arm on a new element); the apply
18-
// re-reads it fresh β€” compute runs eagerly at creation, before a plain ref
19-
// variable fills, so binding off the computed value would arm on nothing.
20-
// Options are read through the closure on each Tab press, so inline
21-
// `enabled` / `last` see the latest state without re-binding the listener.
16+
// The compute tracks a reactive target; the apply re-reads it fresh β€”
17+
// compute runs eagerly at creation, before a plain ref variable fills.
18+
// Options are read per Tab press, so inline `enabled` / `last` stay live
19+
// without re-binding the listener.
2220
createEffect(
2321
() => target(),
2422
() => {

β€Žpackages/solid/hooks/use-focus-trap/tests/use-focus-trap.test.tsxβ€Ž

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ import { useFocusTrap } from '@dunky.dev/solid-use-focus-trap'
77

88
function Trap(props: { enabled?: () => boolean }) {
99
let target: HTMLDivElement | undefined
10-
// The closure defers the props read to each Tab press β€” a direct
11-
// `props.enabled` here would be a top-level reactive read.
10+
// The closure defers the props read to each Tab press.
1211
useFocusTrap(() => target ?? null, { enabled: () => props.enabled?.() !== false })
1312
return (
1413
<div ref={el => (target = el)} tabindex={-1} data-testid='container'>

β€Žpackages/solid/hooks/use-scroll-lock/src/use-scroll-lock.tsβ€Ž

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import { createEffect } from 'solid-js'
22
import { lockScroll } from '@dunky.dev/dom-scroll-lock'
33

4-
/** A static value or an accessor β€” the Solid idiom for a parameter that may
5-
* be reactive; resolved fresh inside the tracking scope that reads it. */
4+
/** A static value or an accessor β€” for parameters that may be reactive. */
65
export type MaybeAccessor<T> = T | (() => T)
76

87
function access<T>(value: MaybeAccessor<T>): T {
@@ -11,10 +10,8 @@ function access<T>(value: MaybeAccessor<T>): T {
1110

1211
/**
1312
* Locks scrolling while the owner lives and `locked` β€” the Solid lifecycle
14-
* around `lockScroll`. Targets the page body unless a `target` element is
15-
* given (e.g. a scoped/portaled surface locks its own container, not the
16-
* page). The lock is shared per container: with several holders (e.g. nested
17-
* modal layers), the container is restored only when the last one releases.
13+
* around `lockScroll`. Targets the page body unless a `target` is given. The
14+
* lock is shared per container: it restores when the last holder releases.
1815
*/
1916
export function useScrollLock(
2017
locked: MaybeAccessor<boolean> = true,

0 commit comments

Comments
Β (0)