Merged
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a typed SazamiComponent framework with a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as Host/App
participant Comp as SazamiComponent
participant Shadow as ShadowRoot
participant Logger as ErrorLogger
participant RO as ResizeObserver
Host->>Comp: construct / set attrs / connect
Comp->>Comp: install reflectors / render()
Comp->>Shadow: mount(STYLES + template)
Host->>Comp: user interaction (click / key)
Comp->>Comp: handle interaction -> dispatchEventTyped(event, detail)
Comp->>Logger: on error/warn -> getLogger("sazami") -> log(...)
RO->>Comp: resize -> recompute centers -> applyCurvomorphism(element, center, groupBounds)
Comp->>Comp: enableCurvomorphism(element, center, groupBounds)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Docs/docs/config-theming.md (1)
27-43:⚠️ Potential issue | 🟡 MinorToken tables are missing newly added public tokens.
The docs should include
color.on-secondary,space.xsmall,text.size.xsmall,text.size.tiny, andicon.size.xsmallto stay aligned withBuild/src/config/tokens.ts.📝 Suggested doc patch
| `color.on-primary` | `#ffffff` | `--saz-color-on-primary` | | `color.on-accent` | `#ffffff` | `--saz-color-on-accent` | +| `color.on-secondary` | `#1f2937` | `--saz-color-on-secondary` | | `color.on-success` | `#ffffff` | `--saz-color-on-success` | | `color.on-danger` | `#ffffff` | `--saz-color-on-danger` | | Token | Default | CSS Variable | | ------- | --------- | ------------- | +| `space.xsmall` | `2px` | `--saz-space-xsmall` | | `space.tiny` | `4px` | `--saz-space-tiny` | | Token | Default | CSS Variable | | ------- | --------- | ------------- | +| `text.size.xsmall` | `10px` | `--saz-text-size-xsmall` | +| `text.size.tiny` | `11px` | `--saz-text-size-tiny` | | `text.size.small` | `12px` | `--saz-text-size-small` | | Token | Default | CSS Variable | | ------- | --------- | ------------- | +| `icon.size.xsmall` | `12px` | `--saz-icon-size-xsmall` | | `icon.size.small` | `16px` | `--saz-icon-size-small` |Also applies to: 46-54, 57-70, 92-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Docs/docs/config-theming.md` around lines 27 - 43, Update the token tables in the theming docs to include the newly added public tokens from Build/src/config/tokens.ts: add color.on-secondary (with its default and CSS variable), add space.xsmall, add text.size.xsmall and text.size.tiny, and add icon.size.xsmall into the appropriate sections of the table(s) (the color table and the size/space/icon tables referenced around the shown ranges). Ensure each entry uses the correct token name (e.g., color.on-secondary, space.xsmall, text.size.xsmall, text.size.tiny, icon.size.xsmall) and the matching CSS variable names used in tokens.ts so the docs stay aligned with the source.Build/src/primitives/section.ts (1)
30-39:⚠️ Potential issue | 🟡 MinorRecompute
center-pointafter layout changes.This snapshot only happens during the initial render. If
center-pointis added later, or the section/slotted content resizes after the first frame,data-center-x/data-center-ystay stale. Re-run this from attribute changes plus aslotchange/ResizeObserverpath while the flag is present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/section.ts` around lines 30 - 39, The current center-point calculation in render() only runs once; update it to recompute whenever the center-point attribute is added/removed or the content/layout changes by: extract the center calculation into a helper (e.g., computeAndSetCenter()) that reads getBoundingClientRect() and sets this.dataset.centerX/Y, call that helper from render(), implement attributeChangedCallback (and add "center-point" to observedAttributes) to call the helper when the attribute is present, add a slotchange listener on the component's slot to call the helper when slotted content changes, install a ResizeObserver on the element (or its slotted elements) to call the helper while the attribute exists, and ensure you disconnect the observer and remove listeners in disconnectedCallback; reference render(), computeAndSetCenter (new helper), attributeChangedCallback, observedAttributes, slotchange listener, ResizeObserver, and disconnectedCallback when making changes.Build/src/primitives/toast.ts (1)
122-124:⚠️ Potential issue | 🟡 MinorAuto-hide timeout not cleaned up on disconnect.
If the toast is removed from the DOM before the duration elapses (e.g., manually closed or parent removed), the timeout will still fire, calling
hide()on a detached element. This can cause unexpected behavior.Proposed fix: track and cleanup the timeout
+ private _hideTimeout?: ReturnType<typeof setTimeout>; + render() { // ... existing code ... this.setAttribute("visible", ""); if (duration > 0) { - setTimeout(() => this.hide(), duration); + this._hideTimeout = setTimeout(() => this.hide(), duration); + this.onCleanup(() => { + if (this._hideTimeout) clearTimeout(this._hideTimeout); + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` around lines 122 - 124, The auto-hide timeout in the Toast (where show() currently calls setTimeout(() => this.hide(), duration)) must be tracked and cleared on disconnect/hide to avoid calling hide() on a removed element; store the timer id on the instance (e.g., this._autoHideTimer) when calling setTimeout, call clearTimeout(this._autoHideTimer) and null it inside hide() and in the element teardown/disconnected method (e.g., disconnectedCallback or destroy) so any pending timeout is cancelled when the toast is removed.
🟠 Major comments (19)
Build/src/primitives/avatar.ts-36-65 (1)
36-65:⚠️ Potential issue | 🟠 MajorThese declared avatar props do not actually drive rendering.
In
Build/src/primitives/base.ts, onlyreflect: trueproperties get accessors, andconnectedCallback()renders once. Here Lines 38-42 mark every prop asreflect: false, while Lines 55-57 read only attributes, soel.src = .../el.initials = ...is a no-op and post-mount attribute changes will not redraw the shadow DOM either.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/avatar.ts` around lines 36 - 65, The avatar props are declared with reflect: false so property assignments won't create accessors or trigger rerenders; update avatarConfig.properties (src, alt, initials, size, shape) to use reflect: true (or implement attributeChangedCallback/getters/setters on SazamiAvatar) so setting el.src or el.initials creates reactive accessors and triggers a re-render; ensure render() continues to read attributes/getters (getAttribute or the new accessors) and that connectedCallback / mount behavior triggers when those reflected attributes change so updates to properties re-mount the template (use the existing avatarConfig, SazamiAvatar class and _getInitials helper as the points of change).Build/src/primitives/label.ts-16-28 (1)
16-28:⚠️ Potential issue | 🟠 MajorThe new
forAPI is effectively dead.Line 18 declares
for, but Line 27 only mounts a slot and never uses it. On an autonomous custom element that means users do not get native label behavior, so clickingsaz-labelwill not focus or toggle the associated control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/label.ts` around lines 16 - 28, The declared "for" property on labelConfig / SazamiLabel is never used in render(), so the custom element currently does not provide native label behavior; update SazamiLabel.render to output a real <label> element that binds the for attribute to this.for (e.g. render "<label for={this.for}><slot/></label>") and, for implicit association when this.for is absent, wrap the slot directly in a <label> so clicks focus the contained control; ensure the mount call uses that label markup (and keep STYLES) so native focus/toggle behavior is preserved.Build/tests/base-component.test.ts-16-74 (1)
16-74:⚠️ Potential issue | 🟠 MajorThese tests are accessing protected members through untyped custom element instances.
document.createElement("saz-button")returnsHTMLElementtype, notSazamiComponent. This causes lines 18, 36, 48, 62, and 70 to accessshadow,$,dispatch, andonCleanup—allprotectedmembers inBuild/src/primitives/base.ts—in violation of TypeScript's access control. The tests fail strict type-checking with errors like "Property 'shadow' does not exist on type 'HTMLElement'".While
disconnectedCallback()at line 72 is public and valid, the pattern couples tests to implementation details rather than the public contract. Consider a test-only subclass that exposes public wrappers around the protected helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/base-component.test.ts` around lines 16 - 74, The tests currently access protected members (shadow, $, dispatch, onCleanup) on untyped HTMLElement instances; create a test-only subclass (e.g. TestableSazamiComponent extends SazamiComponent from Build/src/primitives/base.ts) that exposes public wrappers/getters for shadow, $ (query selector), dispatch, and onCleanup, then replace the raw document.createElement usage in Build/tests/base-component.test.ts with instances of this TestableSazamiComponent (or cast created elements to that subclass) so tests call the public wrapper methods (and keep calling disconnectedCallback directly) instead of accessing protected members on HTMLElement.Build/src/primitives/tag.ts-19-27 (1)
19-27:⚠️ Potential issue | 🟠 Major
variantis declared publicly but never wired at runtime.
SazamiComponentonly installs runtime accessors for props withreflect: true, so Line 21 makesdeclare variant: stringtype-only.tag.variant = "accent"will not set the host attribute, and the:host([variant=...])styles will never match.🛠️ Suggested fix
const tagConfig = { properties: { - variant: { type: "string" as const, reflect: false }, + variant: { type: "string" as const, reflect: true }, }, } as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/tag.ts` around lines 19 - 27, The declared public prop variant on SazamiTag is only type-level because SazamiComponent only creates runtime accessors for props with reflect: true; update tagConfig.properties.variant to enable runtime wiring (e.g., set reflect: true) or add an explicit runtime accessor in SazamiTag so that assigning tag.variant updates the host attribute and allows :host([variant=...]) CSS to match; reference tagConfig, properties.variant, SazamiTag, and SazamiComponent when making the change.Build/src/primitives/tabs.ts-125-139 (1)
125-139:⚠️ Potential issue | 🟠 MajorSync the DOM when
activechanges after mount.
render()snapshotsactiveonce, and_activateTab()is the only code path that updates the rendered state. ExternalsetAttribute("active", ...)/tabs.active = ...changes after mount will update the reflected attribute but leave the selected tab/panel stale. Also, Line 135 never updates the panelactiveclass, so CSS/query state drifts after the first click.🛠️ Suggested fix
- private _activateTab(index: number) { + private _activateTab(index: number, emit = true) { const tabButtons = this.shadow.querySelectorAll(".tab"); const panelElements = this.shadow.querySelectorAll(".panel"); tabButtons.forEach((b, j) => { b.classList.toggle("active", j === index); (b as HTMLElement).setAttribute( "aria-selected", j === index ? "true" : "false", ); }); panelElements.forEach((p, j) => { + p.classList.toggle("active", j === index); (p as HTMLElement).style.display = j === index ? "block" : "none"; }); - this.active = index.toString(); - this.dispatchEventTyped("change", { activeIndex: index.toString() }); + if (this.active !== index.toString()) { + this.active = index.toString(); + } + if (emit) { + this.dispatchEventTyped("change", { activeIndex: index.toString() }); + } } + + attributeChangedCallback(name: string, oldVal: string | null, newVal: string | null) { + if (name === "active" && oldVal !== newVal && this.shadow.childNodes.length) { + this._activateTab(Number(newVal ?? 0), false); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/tabs.ts` around lines 125 - 139, The component currently only updates DOM state inside _activateTab and never responds when the reflected attribute/property active is changed externally after mount, and it also fails to toggle the panel "active" class; fix by wiring the active property/attribute change to call _activateTab (e.g., from the active setter or attributeChangedCallback) so external setAttribute("active", ...) or tabs.active = ... re-syncs the UI, and modify _activateTab to also add/remove the "active" class on the corresponding panel element (and update any aria-hidden/aria-selected as needed) so panels and buttons remain in sync with render().Build/src/primitives/select.ts-121-155 (1)
121-155:⚠️ Potential issue | 🟠 MajorMake the combobox trigger actually focusable.
Line 121 renders a plain
divwithrole="combobox"but notabindex, and the keydown handler is attached to the host instead of the trigger. Keyboard users cannot reliably focus the control to open it or navigate options.aria-expandedis also hard-coded to"false", so an initially open select is announced incorrectly.🛠️ Suggested fix
- <div class="trigger" role="combobox" aria-haspopup="listbox" aria-expanded="false"> + <div + class="trigger" + role="combobox" + tabindex="${this.disabled ? "-1" : "0"}" + aria-haspopup="listbox" + aria-expanded="${this.open ? "true" : "false"}" + > <span class="value">${selectedOption?.label || placeholder}</span> <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><path d="M6 9l6 6 6-6"/></svg> </div> @@ - this.addHandler("keydown", handleKeydown, { internal: true }); + this.addHandler("keydown", handleKeydown, { + internal: true, + element: trigger as HTMLElement, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 121 - 155, The combobox trigger div is not keyboard-focusable and aria-expanded is hard-coded; update the rendered trigger in the render/template code to include tabindex (e.g., tabindex="0" when not disabled) and set aria-expanded based on this.open/this.hasAttribute("open") so the string reflects state; attach the keydown handler (handleKeydown) to the trigger element instead of the host by changing the this.addHandler call to target the trigger element, keep the click handler using toggleOpen, and ensure when opening/closing (toggleOpen/open setter) you update focus and the trigger's aria-expanded attribute accordingly and avoid adding tabindex when this.disabled is true.Build/src/primitives/select.ts-168-172 (1)
168-172:⚠️ Potential issue | 🟠 MajorTear down the global click listener.
This adds a
documentlistener per instance and never unregisters it. Detached selects will keep a live closure and continue processing page clicks, which leaks in dynamic lists/dialogs.🛠️ Suggested fix
export class SazamiSelect extends SazamiComponent<typeof selectConfig> { + private _handleDocumentClick = (e: Event) => { + if (!this.contains(e.target as Node)) { + this.open = false; + } + }; + + connectedCallback() { + super.connectedCallback(); + document.addEventListener("click", this._handleDocumentClick); + } + + disconnectedCallback() { + document.removeEventListener("click", this._handleDocumentClick); + super.disconnectedCallback(); + } + render() { @@ - document.addEventListener("click", (e: Event) => { - if (!this.contains(e.target as Node)) { - this.open = false; - } - }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 168 - 172, The document click listener is registered per instance and never removed; change the implementation to store the handler (e.g. a bound method like this._onDocumentClick = (e: Event) => { if (!this.contains(e.target as Node)) this.open = false; }) and register it in connectedCallback (or the constructor) via document.addEventListener and remove it in disconnectedCallback via document.removeEventListener using the same handler reference; update the class to define/attach _onDocumentClick and ensure removal when the element is detached to avoid leaks from lingering closures.Build/src/primitives/progress.ts-55-73 (1)
55-73:⚠️ Potential issue | 🟠 MajorExpose progress semantics on the host.
The component never sets
role="progressbar"or the ARIA value attributes, so assistive tech cannot interpret it. Setaria-valuemin/aria-valuemax/aria-valuenowin determinate mode, and omitaria-valuenowin indeterminate mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/progress.ts` around lines 55 - 73, The render() method currently calculates percent but does not expose accessible semantics; update render() to set role="progressbar" on the host and set aria-valuemin and aria-valuemax from this.getAttribute("min"/"max"), and only set aria-valuenow (from this.getAttribute("value")) when not indeterminate; when indeterminate remove aria-valuenow. Use the existing symbols (render, this.getAttribute, indeterminate, percent, this.mount) and set/remove attributes on the host (e.g., this.setAttribute / this.removeAttribute) before calling this.mount so assistive tech receives role and appropriate aria-valu* values for determinate vs indeterminate states.Build/src/primitives/spinner.ts-26-27 (1)
26-27:⚠️ Potential issue | 🟠 MajorThe default loading state is silent to assistive tech.
When no
labelattribute is provided, render falls back to"Loading..."but lines 26-27 immediately hide that span, and the host never exposesrole="status"/aria-live. The spinner therefore has no announced loading text unless callers add their own semantics externally.Also applies to: 48-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/spinner.ts` around lines 26 - 27, The label span is being completely hidden when no label attr is provided so the fallback "Loading..." and ARIA semantics are never exposed; change the CSS rules targeting :host([label=""]) .label and :host(:not([label])) .label (and the similar block at 48-60) so the fallback label is not display:none but kept available to assistive tech (e.g., visually hidden with an accessible-only utility or remove the selector that hides it), and ensure the host (or the label span) exposes ARIA by adding role="status" and aria-live="polite" when no label attribute is present (update the component render logic that sets the host attributes and the .label element in Spinner/SpinnerComponent to reflect this).Build/src/primitives/icon-button.ts-62-64 (1)
62-64:⚠️ Potential issue | 🟠 Major
disabledis only cosmetic here, andsaz-clicknever fires.On a custom element,
[disabled]does not block activation by itself. The host stays focusable,keydownstill callsclick(), and nothing ever callsdispatchEventTyped("click", {}), so listeners forsaz-clicksee nothing. Route activation through one internal click handler that bails whenthis.disabledis true, syncsaria-disabled/tabindex, and emits the typed event.Also applies to: 85-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon-button.ts` around lines 62 - 64, The component currently treats the disabled attribute as cosmetic so keyboard activation still invokes clicks but no typed event is emitted; add a single internal click handler (e.g., handleClick or onInternalClick) on the host that first checks this.disabled and returns early if true, updates aria-disabled and tabindex to reflect the disabled state, and then emits the typed event via dispatchEventTyped("saz-click", {}) (and/or forwards the native click if needed). Apply the same change for the other interactive paths mentioned (lines ~85-99) so all activation funnels through this handler and disabled consistently prevents activation.Build/src/primitives/row.ts-24-38 (1)
24-38:⚠️ Potential issue | 🟠 MajorThese declared props never drive the attribute-based layout styles.
Build/src/primitives/base.tsonly installs runtime accessors forreflect: true. With these entries set tofalse, assignments likerow.wrap = trueorrow.gap = "large"never reach the[wrap]/[gap]selectors here. Either reflect these props or stop exposing them as public instance properties. The same migration pattern appears in the sibling layout primitives too.🛠️ Minimal fix
const rowConfig = { properties: { - justify: { type: "string" as const, reflect: false }, - align: { type: "string" as const, reflect: false }, - wrap: { type: "boolean" as const, reflect: false }, - gap: { type: "string" as const, reflect: false }, + justify: { type: "string" as const, reflect: true }, + align: { type: "string" as const, reflect: true }, + wrap: { type: "boolean" as const, reflect: true }, + gap: { type: "string" as const, reflect: true }, }, } as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/row.ts` around lines 24 - 38, The declared props in rowConfig (justify, align, wrap, gap) on SazamiRow are marked reflect: false so runtime accessors in SazamiComponent never propagate assignments to attributes and thus the CSS selectors like [wrap] / [gap] won't work; update the rowConfig properties to reflect: true for the attributes you intend to drive with CSS (at minimum wrap and gap, and justify/align if they should drive attribute selectors) so assignments like row.wrap = true reflect to attributes, or alternatively remove the public declarations (declare wrap/ gap/etc.) and keep them internal-only if reflection is not desired—make the chosen change consistently across the sibling layout primitives too.Build/src/primitives/progress.ts-55-64 (1)
55-64:⚠️ Potential issue | 🟠 MajorValidate the numeric range before dividing.
max === min,max < min, or any non-numeric attribute makes this calculation produceInfinity/NaN, which then leaks into the inlinewidthstyle. Guard for finite numbers and a positive range before clamping.🛠️ Minimal fix
- const value = parseFloat(this.getAttribute("value") || "50"); - const max = parseFloat(this.getAttribute("max") || "100"); - const min = parseFloat(this.getAttribute("min") || "0"); + const rawValue = Number(this.getAttribute("value") || "50"); + const rawMax = Number(this.getAttribute("max") || "100"); + const rawMin = Number(this.getAttribute("min") || "0"); + const value = Number.isFinite(rawValue) ? rawValue : 50; + const max = Number.isFinite(rawMax) ? rawMax : 100; + const min = Number.isFinite(rawMin) ? rawMin : 0; const indeterminate = this.hasAttribute("indeterminate"); - const percent = Math.min( - 100, - Math.max(0, ((value - min) / (max - min)) * 100), - ); + const range = max - min; + const percent = + range > 0 ? Math.min(100, Math.max(0, ((value - min) / range) * 100)) : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/progress.ts` around lines 55 - 64, In render() validate that value, max and min are finite numbers and that max > min before computing percent: check Number.isFinite(value) && Number.isFinite(max) && Number.isFinite(min) and that (max - min) > 0; if the check fails, set percent to a safe fallback (e.g., 0) or treat as indeterminate, otherwise compute percent as before and clamp with Math.min/Math.max; update any places that use percent (inline width style) to rely on this validated percent to avoid NaN/Infinity leaking into the DOM.Build/src/primitives/badge.ts-24-29 (1)
24-29:⚠️ Potential issue | 🟠 MajorReflect the style-driving props to the host.
SIZE_PADDING_RULES,VARIANT_BG_RULES, andSHAPE_RULESall key off host attributes, but these props are configured withreflect: false. InBuild/src/primitives/base.ts, onlyreflect: trueinstalls the accessor that updates the attribute, sobadge.size = "small"/badge.variant = "accent"become no-ops after this refactor.Suggested fix
const badgeConfig = { properties: { - size: { type: "string" as const, reflect: false }, - variant: { type: "string" as const, reflect: false }, - shape: { type: "string" as const, reflect: false }, + size: { type: "string" as const, reflect: true }, + variant: { type: "string" as const, reflect: true }, + shape: { type: "string" as const, reflect: true }, }, } as const;Also applies to: 34-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/badge.ts` around lines 24 - 29, The props in badgeConfig (properties.size, properties.variant, properties.shape) are declared with reflect: false but styles read host attributes via SIZE_PADDING_RULES, VARIANT_BG_RULES, and SHAPE_RULES, so setting badge.size/variant/shape won't update attributes; change those property descriptors to reflect: true so the base accessor will write attributes when the properties change (ensure the same change is applied for the other similar props declared later in the file).Build/src/primitives/input.ts-61-75 (1)
61-75:⚠️ Potential issue | 🟠 MajorKeep the inner input synced after the first render.
After
render(), laterthis.value = ...orthis.disabled = ...only update host attributes viaBuild/src/primitives/base.ts; the shadow<input>keeps its old state. That leaves the visible control out of sync with the component API unless you rerender or implementattributeChangedCallback().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/input.ts` around lines 61 - 75, The shadow <input> is only initialized in render() via this.mount(...) so later changes to this.value or this.disabled (which update host attributes in Base) leave the inner input stale; implement attributeChangedCallback (or update logic in the setters used by Base) to locate the inner element with this.$("input") and sync its properties (set input.value = this.value and input.disabled = !!this.disabled) whenever the component's value/disabled attributes change; update the event listener setup in render() to tolerate the input being replaced (i.e., re-run the sync in attributeChangedCallback rather than relying on a one-time render).Build/src/primitives/switch.ts-91-95 (1)
91-95:⚠️ Potential issue | 🟠 MajorExpose checked and disabled state through ARIA.
A custom
role="switch"needsaria-checked, and a disabled switch should not stay tabbable. As written, assistive tech never sees the checked state, and keyboard users can still tab to a disabled control.Also applies to: 98-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/switch.ts` around lines 91 - 95, Set and keep ARIA attributes and tabbability in sync with the component's checked/disabled state: when initializing and whenever the state changes, set aria-checked to the element's checked boolean (e.g., this.setAttribute('aria-checked', String(this.checked))) and set/remove tabindex based on disabled (either remove tabindex or set tabindex="-1" and set aria-disabled="true" when this.disabled is true, otherwise ensure tabindex="0" and aria-disabled="false"/remove). Update the places that mutate state and the initial setup around the click/keydown handlers (references: role="switch", methods _handleClick and _handleKeydown, and the component properties checked and disabled) so assistive tech sees the current checked state and a disabled switch is not tabbable.Build/src/primitives/chip.ts-119-129 (1)
119-129:⚠️ Potential issue | 🟠 MajorGive the selectable chip keyboard semantics.
This branch makes the host clickable, but it never gets a role, tab stop, or key handling. The result is a mouse-only toggle. Mirror the
Build/src/primitives/icon-button.tspattern here and expose the selected state with ARIA.Also applies to: 131-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/chip.ts` around lines 119 - 129, The host is clickable but lacks keyboard/ARIA semantics; make the chip focusable and operable via keyboard by assigning a role and tabindex and wiring a key handler that mirrors the icon-button pattern: set this.setAttribute("role","button") and this.tabIndex = 0 (or use aria-selected/aria-pressed to reflect state), update the element whenever selection changes to set aria-pressed or aria-selected from the selected property, and add/remove a keydown listener that calls this._handleClick when Enter or Space is pressed; ensure you still keep the existing remove button (".remove-btn") logic and remove any added listeners in the component teardown.Build/src/primitives/slider.ts-146-152 (1)
146-152:⚠️ Potential issue | 🟠 MajorEvent listener leak on re-render.
Using
slider?.addEventListenerdirectly insiderender()combined withattributeChangedCallbackcallingrender()will attach duplicate listeners each time an observed attribute changes. This causes memory leaks and duplicate event dispatches.Proposed fix: use addHandler with element option
- slider?.addEventListener("input", () => { - const val = parseFloat(slider.value); - const pct = ((val - min) / (max - min)) * 100; - filled.style.width = `${pct}%`; - this.value = val; - this.dispatchEventTyped("input", { value: val }); - }); + if (slider) { + this.addHandler("input", () => { + const val = parseFloat(slider.value); + const pct = ((val - min) / (max - min)) * 100; + filled.style.width = `${pct}%`; + this.value = val; + this.dispatchEventTyped("input", { value: val }); + }, { internal: true, element: slider }); + }Note: You may also want to consider updating only the relevant parts (percent, slider value) in
attributeChangedCallbackrather than fully re-rendering, which would be more efficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 146 - 152, The input listener is being attached directly in render() causing duplicate handlers on re-renders; replace the direct slider.addEventListener call with the shared event registration helper (addHandler) and pass the element option so the handler is auto-managed on re-renders (e.g., call addHandler(slider, "input", handler, { element: this }) or equivalent), keep the handler logic that computes val/pct, updates filled.style.width, sets this.value and dispatches "input", and ensure the handler is a stable function reference (method or closure) so it can be properly tracked by addHandler; also consider updating only the slider value/percent in attributeChangedCallback instead of full render for efficiency.Build/src/primitives/modal.ts-128-147 (1)
128-147:⚠️ Potential issue | 🟠 MajorDouble event dispatch on open/close.
When
_open()or_close()is called:
- They dispatch the event via
dispatchEventTyped- They modify the attribute, triggering
attributeChangedCallbackattributeChangedCallbackdispatches the event againThis results in duplicate
saz-open/saz-closeevents being fired.🔧 Proposed fix: dispatch only in attributeChangedCallback
private _open() { this.setAttribute("open", ""); - this.dispatchEventTyped("open", {}); } private _close() { this.removeAttribute("open"); - this.dispatchEventTyped("close", {}); }Or alternatively, dispatch only in
_open()/_close()and remove fromattributeChangedCallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/modal.ts` around lines 128 - 147, Double event dispatch occurs because _open()/_close() both call dispatchEventTyped and then change the "open" attribute which causes attributeChangedCallback to dispatch again; to fix, remove the dispatchEventTyped calls from the _open() and _close() methods (keep setAttribute/removeAttribute there) and let attributeChangedCallback handle dispatching "open"/"close" via dispatchEventTyped based on the "open" attribute changes in attributeChangedCallback.Build/src/primitives/modal.ts-102-102 (1)
102-102:⚠️ Potential issue | 🟠 MajorXSS vulnerability: Title is interpolated without HTML escaping.
The
titlevalue is inserted directly into the HTML template. If the title contains malicious content like<script>tags or event handlers, it will be executed.🛡️ Proposed fix to escape HTML entities
Add an escape helper and use it:
+const escapeHtml = (str: string) => + str.replace(/[&<>"']/g, (c) => ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' }[c] || c)); + render() { - const title = this.getAttribute("title") || ""; + const title = escapeHtml(this.getAttribute("title") || "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/modal.ts` at line 102, The modal template in Build/src/primitives/modal.ts interpolates title directly into the HTML (<h2 class="title" id="modal-title">${title}</h2>), creating an XSS risk; add an escapeHtml helper (e.g., function escapeHtml(s:string){ return s.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>').replace(/"/g,'"').replace(/'/g,'''); }) and use it when rendering the template (replace ${title} with ${escapeHtml(title)}), exporting or reusing the helper where appropriate so all modal title insertions are properly escaped.
🟡 Minor comments (12)
Build/tests/generic.test.ts-22-29 (1)
22-29:⚠️ Potential issue | 🟡 MinorThis does not prove slot projection.
The assertion only checks that a
<slot>exists. A regression that stops projecting the light-DOM text would still pass, so this should assert the assigned node/text instead of the placeholder element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/generic.test.ts` around lines 22 - 29, The test "saz-details renders slot content" currently only asserts a <slot> exists; change it to verify actual slot projection by grabbing the slot from el.shadowRoot (querySelector("slot")), calling slot.assignedNodes() or slot.assignedElements() and asserting that the assigned node(s) contain the expected text "Hello World" (or that assignedElements()[0].textContent === "Hello World"), then clean up the element as before; keep the test name and element creation (const el = document.createElement("saz-details")) but replace the expect(slot).toBeTruthy() check with an assertion on slot.assignedNodes()/assignedElements() content.Build/tests/accessibility.test.ts-189-194 (1)
189-194:⚠️ Potential issue | 🟡 MinorThe assertion does not match the test's claim.
This only proves that the inner
<input>renders. A regression that removes every focus rule would still pass, so the test currently provides no coverage for focus styles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/accessibility.test.ts` around lines 189 - 194, The test "saz-input input has focus styles" only checks that the inner input exists but never verifies focus styling; update the test to actually focus the inner input (e.g., call input.focus() or dispatch a focus event on the input inside the "saz-input" instance) and then assert the expected visual change — for example check that document.activeElement is the shadow input or that window.getComputedStyle(input) shows a non-none outline/border or that a specific focus CSS class/attribute is present; locate the test by the string "saz-input input has focus styles" and the variables el and input and replace the simple existence assertion with focus behavior + computed-style or class/attribute assertions.Build/tests/tokens.test.ts-13-17 (1)
13-17:⚠️ Potential issue | 🟡 MinorAssert the
:rootwrapper ongenerateThemeCSS(), notgenerateCSSVariables().
Docs/docs/api-reference.mddocumentsgenerateCSSVariables()as declarations-only, whilegenerateThemeCSS()is the wrapper that emits:root { ... }. Keeping this expectation here either breaks the documented contract or makes the two helpers redundant.🧪 Proposed fix
- test("generates :root selector", () => { - const css = generateCSSVariables({ "color.primary": "#ff0000" }); + test("generateThemeCSS wraps variables in :root", () => { + const css = generateThemeCSS({ "color.primary": "#ff0000" }); expect(css).toContain(":root {"); expect(css).toContain("}"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/tokens.test.ts` around lines 13 - 17, The test currently asserts a :root wrapper on generateCSSVariables but per the docs generateCSSVariables should emit declarations only; update the test to assert :root is produced by generateThemeCSS instead (or replace the call to generateCSSVariables with generateThemeCSS in this test). Locate the assertions in Build/tests/tokens.test.ts referencing generateCSSVariables and move/modify them so generateCSSVariables tests only declarations and generateThemeCSS tests include expect(css).toContain(":root {") and the closing brace.Build/src/primitives/divider.ts-12-20 (1)
12-20:⚠️ Potential issue | 🟡 MinorPreserve inline spacing for vertical dividers with
size.The size rules override the vertical rule's
margin: 0 ..., so<saz-divider vertical size="small">ends up with top/bottom spacing instead of left/right spacing.🛠️ Suggested fix
:host([vertical]) { width: 1px; height: 100%; margin: 0 var(--saz-space-medium); align-self: stretch; } -:host([size="small"]) { margin: var(--saz-space-small) 0; } -:host([size="large"]) { margin: var(--saz-space-large) 0; } -:host([size="xlarge"]) { margin: var(--saz-space-xlarge) 0; } +:host(:not([vertical])[size="small"]) { margin: var(--saz-space-small) 0; } +:host(:not([vertical])[size="large"]) { margin: var(--saz-space-large) 0; } +:host(:not([vertical])[size="xlarge"]) { margin: var(--saz-space-xlarge) 0; } +:host([vertical][size="small"]) { margin: 0 var(--saz-space-small); } +:host([vertical][size="large"]) { margin: 0 var(--saz-space-large); } +:host([vertical][size="xlarge"]) { margin: 0 var(--saz-space-xlarge); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/divider.ts` around lines 12 - 20, The size-specific CSS rules for :host([size="small"/"large"/"xlarge"]) are overriding the vertical margin set in :host([vertical]), causing vertical dividers to get top/bottom spacing; update the stylesheet so size rules do not apply to vertical dividers—either make the size selectors conditional (e.g., :host(:not([vertical])[size="..."])) or add explicit :host([vertical][size="..."]) rules that preserve margin: 0 var(--saz-space-...)—target the existing :host([vertical]) and :host([size="..."]) selectors in divider.ts to implement this fix.Build/src/primitives/progress.ts-76-81 (1)
76-81:⚠️ Potential issue | 🟡 MinorObserve
indeterminateas well.If
valuechanges while the component is indeterminate, removing[indeterminate]later leaves the inlinewidth: 30%behind because this rerender path never runs for that attribute. AddindeterminatetoobservedAttributesor stop hard-coding the inline width in that mode.🛠️ Minimal fix
static get observedAttributes() { - return ["value", "max", "min"]; + return ["value", "max", "min", "indeterminate"]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/progress.ts` around lines 76 - 81, The component doesn't observe the "indeterminate" attribute, so toggling it doesn't trigger attributeChangedCallback/render and leaves a stale inline width; update the static get observedAttributes() to include "indeterminate" (e.g. return ["value","max","min","indeterminate"]) so changes to indeterminate cause render to run, and verify the render() implementation in the Progress class removes/adjusts any hard-coded inline width when indeterminate is added or removed.Build/src/primitives/checkbox.ts-84-85 (1)
84-85:⚠️ Potential issue | 🟡 MinorInconsistent event registration: click handler not tracked.
Similar to radio.ts, line 84 uses
this.addEventListenerdirectly while line 85 usesthis.addHandler. The click handler won't be tracked for cleanup.Proposed fix
- this.addEventListener("click", this._handleClick); + this.addHandler("click", this._handleClick, { internal: true }); this.addHandler("keydown", this._handleKeydown, { internal: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/checkbox.ts` around lines 84 - 85, The click event is registered with this.addEventListener while other events use this.addHandler so it won't be tracked for cleanup; update the registration of the click handler in Checkbox (replace the this.addEventListener("click", this._handleClick) call) to use this.addHandler("click", this._handleClick, { internal: true }) (matching how _handleKeydown is registered) so the handler is tracked and cleaned up properly.Build/src/primitives/radio.ts-68-86 (1)
68-86:⚠️ Potential issue | 🟡 MinorInconsistent event registration pattern and unused variables.
Line 84 uses
this.addEventListenerdirectly while line 85 usesthis.addHandler. This inconsistency means the click handler won't be tracked by the component's handler registry and won't be cleaned up on disconnect.Lines 70-71 declare
nameandvaluebut never use them (they're re-read in_handleClick).Proposed fix
render() { const label = this.textContent?.trim() || ""; - const name = this.getAttribute("name") || ""; - const value = this.getAttribute("value") || ""; this.mount( STYLES, ` <div class="radio"><div class="dot"></div></div> ${label ? `<span class="label">${label}</span>` : ""} `, ); if (!this.hasAttribute("role")) this.setAttribute("role", "radio"); if (!this.hasAttribute("tabindex")) this.setAttribute("tabindex", "0"); - this.addEventListener("click", this._handleClick); + this.addHandler("click", this._handleClick, { internal: true }); this.addHandler("keydown", this._handleKeydown, { internal: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/radio.ts` around lines 68 - 86, In render(), remove the unused local variables name and value (they’re re-read in _handleClick) and register the click listener via the component handler registry instead of DOM API by replacing the direct this.addEventListener("click", this._handleClick) call with this.addHandler("click", this._handleClick, { internal: true }) so the click handler is tracked/cleaned up like the keydown handler; keep the role/tabindex logic and the mount call unchanged and ensure _handleClick continues to read attributes as before.Build/src/primitives/toggle.ts-74-75 (1)
74-75:⚠️ Potential issue | 🟡 MinorInconsistent event registration: click handler not tracked.
Same issue as checkbox.ts and radio.ts. Use
addHandlerfor consistency and proper cleanup.Proposed fix
- this.addEventListener("click", this._handleClick); + this.addHandler("click", this._handleClick, { internal: true }); this.addHandler("keydown", this._handleKeydown, { internal: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toggle.ts` around lines 74 - 75, Replace the inconsistent raw DOM listener registration by using the component's event tracking helper: change the call using this.addEventListener("click", this._handleClick) to this.addHandler("click", this._handleClick, { internal: true }) so the click handler (_handleClick) is registered and cleaned up consistently with the keydown handler (_handleKeydown) and matches checkbox.ts / radio.ts patterns.Build/src/primitives/accordion.ts-116-121 (1)
116-121:⚠️ Potential issue | 🟡 MinorFix forEach callbacks to not return values.
The static analyzer flags these arrow functions for implicitly returning values (even though
removeAttribute/setAttributereturnundefined). Use block bodies for clarity and linter compliance.Proposed fix
if (this.hasAttribute("single-open")) { this.shadow .querySelectorAll(".item") - .forEach((el) => el.removeAttribute("open")); - headers.forEach((h) => - (h as HTMLElement).setAttribute("aria-expanded", "false"), - ); + .forEach((el) => { el.removeAttribute("open"); }); + headers.forEach((h) => { + (h as HTMLElement).setAttribute("aria-expanded", "false"); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/accordion.ts` around lines 116 - 121, The forEach callbacks in the accordion (the this.shadow.querySelectorAll(".item").forEach and headers.forEach blocks) are using concise arrow bodies that implicitly return the result of removeAttribute/setAttribute; replace each concise arrow with a block-bodied arrow (e.g., (el) => { el.removeAttribute("open"); } and (h) => { (h as HTMLElement).setAttribute("aria-expanded", "false"); }) so the callbacks explicitly perform the side effects without an implicit return and satisfy the linter/static analyzer.Build/src/primitives/base.ts-325-330 (1)
325-330:⚠️ Potential issue | 🟡 MinorType inconsistency for number properties.
The getter always returns a string (from
getAttribute), butPropTypemaps"number"config to TypeScriptnumbertype. This creates a runtime type mismatch where the declared type isnumberbut the actual value is a string.🔧 Proposed fix to coerce number types
get() { if (type === "boolean") { return this.hasAttribute(attr); } - return this.getAttribute(attr) ?? defaultValue ?? ""; + const raw = this.getAttribute(attr); + if (type === "number") { + const val = raw !== null ? parseFloat(raw) : defaultValue; + return typeof val === "number" && !isNaN(val) ? val : 0; + } + return raw ?? defaultValue ?? ""; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` around lines 325 - 330, The getter currently always returns a string from getAttribute, causing a mismatch when PropType maps "number" to a TypeScript number; update the get() implementation (the get() method that checks type and uses attr and defaultValue) to explicitly handle type === "number" by reading the attribute, coercing/parsing it to a numeric value (e.g., Number(...) or parseFloat(...)), and falling back to the provided defaultValue (or a safe numeric fallback) when parsing yields NaN or the attribute is missing; keep the existing boolean branch using this.hasAttribute(attr) unchanged.Build/src/primitives/modal.ts-118-123 (1)
118-123:⚠️ Potential issue | 🟡 MinorEscape key handler may not trigger reliably.
The keydown handler is attached to
this(the modal element itself), but the modal may not have keyboard focus. Typically, modal Escape handlers are attached todocumentto ensure they catch the key regardless of focus.🔧 Proposed fix to use document-level handler
- const handleKeydown = (e: KeyboardEvent) => { - if (e.key === "Escape" && this.hasAttribute("open")) { - this._close(); - } - }; - this.addHandler("keydown", handleKeydown, { internal: true }); + const handleKeydown = (e: KeyboardEvent) => { + if (e.key === "Escape" && this.hasAttribute("open")) { + this._close(); + } + }; + document.addEventListener("keydown", handleKeydown); + this.onCleanup(() => document.removeEventListener("keydown", handleKeydown));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/modal.ts` around lines 118 - 123, The Escape key handler is bound to the modal element (via handleKeydown and this.addHandler) so it may miss key events when the modal doesn't have focus; change the binding to listen on document (or window) instead: register the handleKeydown listener on document (while preserving the internal flag/cleanup via the class' handler removal logic), ensure the handler still checks e.key === "Escape" and this.hasAttribute("open") before calling this._close(), and update the corresponding removal/unregister path to remove the document-level listener when the component disconnects or the modal closes.Build/src/primitives/modal.ts-125-125 (1)
125-125:⚠️ Potential issue | 🟡 MinorRedundant
_open()call may dispatch duplicate event.When
render()is called and theopenattribute is already present, calling_open()sets the attribute again (no-op) and dispatches thesaz-openevent. However,attributeChangedCallbackwill also dispatch when the attribute was initially set, potentially causing duplicate events.🔧 Proposed fix to avoid double dispatch
Since
attributeChangedCallbackhandles event dispatch, you can skip dispatching in_open()when called fromrender():- if (this.hasAttribute("open")) this._open(); + // Attribute change already handled by attributeChangedCallbackOr, if
_open()is needed for other initialization, avoid the duplicate dispatch:- if (this.hasAttribute("open")) this._open(); + // attributeChangedCallback handles initial open state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/modal.ts` at line 125, render() calls this._open() when the open attribute already exists which causes _open() to dispatch the 'saz-open' event while attributeChangedCallback will also dispatch it, producing duplicates; update render() and/or _open() so that when render() detects the open attribute it performs any necessary initialization without triggering the event (e.g. call a non-dispatching initialization helper or add a flag/parameter to _open(initOnly: boolean) to skip dispatch), and ensure attributeChangedCallback remains the single place that emits 'saz-open' when the attribute actually changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b549e0ab-446d-46f5-a8e0-5a4db9fcb056
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
Build/package.jsonBuild/src/config/tokens.tsBuild/src/errors.tsBuild/src/primitives/accordion.tsBuild/src/primitives/avatar.tsBuild/src/primitives/badge.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/card.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/chip.tsBuild/src/primitives/column.tsBuild/src/primitives/coverart.tsBuild/src/primitives/divider.tsBuild/src/primitives/generic.tsBuild/src/primitives/grid.tsBuild/src/primitives/heading.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/label.tsBuild/src/primitives/modal.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/registry.tsBuild/src/primitives/row.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/shared.tsBuild/src/primitives/slider.tsBuild/src/primitives/spacer.tsBuild/src/primitives/spinner.tsBuild/src/primitives/stack.tsBuild/src/primitives/switch.tsBuild/src/primitives/tabs.tsBuild/src/primitives/tag.tsBuild/src/primitives/text.tsBuild/src/primitives/toast.tsBuild/src/primitives/toggle.tsBuild/src/runtime/transformer.tsBuild/tests/accessibility.test.tsBuild/tests/base-component.test.tsBuild/tests/components.test.tsBuild/tests/generic.test.tsBuild/tests/modifiers.test.tsBuild/tests/tokens.test.tsDocs/docs/api-reference.mdDocs/docs/config-theming.mdDocs/docs/curvomorphism.mdDocs/docs/primitives.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Docs/docs/api-reference.md (1)
216-226:⚠️ Potential issue | 🟡 MinorFix icon count mismatch in docs
Line 216 says there are 41 built-in icons, but the listed names on Lines 221-225 exceed that count. Please align the numeric claim with the actual registry content to avoid misleading API consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Docs/docs/api-reference.md` around lines 216 - 226, The docs claim "All 41 built-in icons" but the enumerated list for ICON_SVGS contains a different count; recount the actual entries in the ICON_SVGS registry and update the text to match (either change "41" to the correct number or remove/add names so the list and count match). Locate the ICON_SVGS reference and the following name list in the API docs and ensure the numeric claim and the comma-separated icon names are consistent with the source registry (ICON_SVGS) so consumers see an accurate count and list.
🧹 Nitpick comments (1)
Docs/docs/api-reference.md (1)
231-367: Reduce duplicated base-system docs to avoid driftThis section substantially overlaps with
Docs/docs/component-base.md. Consider keeping a concise summary here and linking to the dedicated page as the source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Docs/docs/api-reference.md` around lines 231 - 367, The "Base Component System" docs duplicate content from Docs/docs/component-base.md; replace the verbose duplicate blocks (the `@component(config)` example, `SazamiComponent` description, Handler Management methods `addHandler`, `removeHandler`, `removeAllHandlers`, Event Dispatch methods `dispatch`, `dispatchEventTyped`, and `onCleanup`) with a concise summary and a clear link to the canonical page (component-base.md) as the source of truth; keep only a short one-line description for each symbol (`@component(config)`, `SazamiComponent`, `addHandler`, `removeHandler`, `removeAllHandlers`, `dispatch`, `dispatchEventTyped`, `onCleanup`) and add a "See also" link to the detailed guide so the example code and full API live in component-base.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Docs/docs/api-reference.md`:
- Around line 216-226: The docs claim "All 41 built-in icons" but the enumerated
list for ICON_SVGS contains a different count; recount the actual entries in the
ICON_SVGS registry and update the text to match (either change "41" to the
correct number or remove/add names so the list and count match). Locate the
ICON_SVGS reference and the following name list in the API docs and ensure the
numeric claim and the comma-separated icon names are consistent with the source
registry (ICON_SVGS) so consumers see an accurate count and list.
---
Nitpick comments:
In `@Docs/docs/api-reference.md`:
- Around line 231-367: The "Base Component System" docs duplicate content from
Docs/docs/component-base.md; replace the verbose duplicate blocks (the
`@component(config)` example, `SazamiComponent` description, Handler Management
methods `addHandler`, `removeHandler`, `removeAllHandlers`, Event Dispatch
methods `dispatch`, `dispatchEventTyped`, and `onCleanup`) with a concise
summary and a clear link to the canonical page (component-base.md) as the source
of truth; keep only a short one-line description for each symbol
(`@component(config)`, `SazamiComponent`, `addHandler`, `removeHandler`,
`removeAllHandlers`, `dispatch`, `dispatchEventTyped`, `onCleanup`) and add a
"See also" link to the detailed guide so the example code and full API live in
component-base.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ae15dcd-32f8-4387-ac14-0cd51bbaea9c
📒 Files selected for processing (2)
Docs/docs/api-reference.mdDocs/docs/component-base.md
added 2 commits
March 15, 2026 00:02
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (6)
Build/src/primitives/base.ts (1)
233-239:⚠️ Potential issue | 🔴 CriticalDetach handlers from their original targets.
addHandler()stores each listener'starget, butremoveAllHandlers()still removes fromthis. Any listener registered withoptions.elementsurvives cleanup and can keep firing after teardown.Minimal fix
toRemove.forEach((h) => { - this.removeEventListener(type, h.fn); + h.target.removeEventListener(type, h.fn); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` around lines 233 - 239, removeAllHandlers currently always calls this.removeEventListener(type, h.fn) so listeners attached to a different element (stored as h.target by addHandler) are not removed; update removeAllHandlers to call removeEventListener on the original target for each handler (e.g., use (h.target || this).removeEventListener(type, h.fn)) when iterating handlers so listeners added with options.element are properly detached; reference handlers, h.target, h.fn, removeAllHandlers, addHandler, and type to locate the change.Build/src/primitives/switch.ts (1)
80-88:⚠️ Potential issue | 🔴 CriticalRender the label via a slot or
textContent.
labelcomes fromthis.textContentand is interpolated into the template string. HTML-like text becomes active markup in the shadow root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/switch.ts` around lines 80 - 88, The render() method injects this.textContent directly into the template string causing HTML to be parsed in the shadow root; change render() to render the label via a <slot> or by inserting an empty <span class="label"></span> in the template and then set its textContent after mount to the trimmed label value. Locate render(), STYLES, and the mount(...) call: replace the `${label ? `<span class="label">${label}</span>` : ""}` interpolation with a persistent slot or span, and if using the span approach, query the .label element after mount and assign its textContent = label to ensure the label is treated as plain text.Build/src/primitives/tabs.ts (1)
92-92:⚠️ Potential issue | 🔴 CriticalRender tab labels as text, not HTML.
t.labelis derived from light DOM/attributes and interpolated into the template. A label containing markup or quotes becomes active DOM inside the shadow root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/tabs.ts` at line 92, The template is inserting t.label directly into HTML which allows markup from light DOM to be parsed; change the render to set button textContent (or escape HTML) instead of interpolating raw label text: locate the map that builds buttons (the expression using this._tabs.map(...) that creates <button ...>${t.label}</button>) and replace it by creating buttons programmatically or using a safe escape utility so each button’s label is assigned via element.textContent = t.label (or escapeHtml(t.label)) rather than embedding t.label into the template string.Build/src/primitives/select.ts (1)
133-142:⚠️ Potential issue | 🔴 CriticalDon't concatenate placeholder and option data into the template.
placeholder,selectedOption?.label,opt.label, andopt.valueare all injected intoinnerHTML. Text or attribute values containing markup/quotes can break the DOM or land as active HTML in the shadow root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 133 - 142, The template is unsafe because placeholder, selectedOption?.label, and opt.label/opt.value are injected into a string passed to this.mount which becomes innerHTML; instead, in the mount/render code that uses STYLES and the template string, build the DOM nodes programmatically: create the trigger div and dropdown container with document.createElement, set attributes with setAttribute (or element.role/tabindex/aria-*), and assign visible text via textContent for selectedOption.label or placeholder; for options iterate this._options and create option elements setting data-value with setAttribute and label via textContent (compare opt.value to value to add the "selected" class). Keep use of mount/STYLES but swap the HTML string for constructed nodes to prevent HTML injection.Build/src/primitives/spinner.ts (1)
58-78:⚠️ Potential issue | 🔴 CriticalSet the label with
textContent.
labelis read from an attribute and interpolated into the mounted HTML. Attribute text containing markup becomes active DOM in the shadow root.Minimal fix
this.mount( STYLES, ` <svg class="spinner" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="3"> <circle cx="12" cy="12" r="10" stroke-opacity="0.25"/> <path d="M12 2a10 10 0 0 1 10 10" stroke-linecap="round"/> </svg> - ${label ? `<span class="label">${label}</span>` : ""} + <span class="label"></span> `, ); + + this.$<HTMLSpanElement>(".label")!.textContent = label;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/spinner.ts` around lines 58 - 78, The render() method currently interpolates the label into the HTML string causing potential HTML injection; change it to mount the SVG and an empty <span class="label"></span> (keep existing this.mount call but remove ${label} interpolation), then after mount call this.shadowRoot.querySelector('.label') and set its textContent = label (obtained via getAttribute("label") or default). This ensures label content is inserted as plain text; update references in render(), getAttribute(), and the mount() usage to follow this pattern.Build/src/primitives/icon-button.ts (1)
78-82:⚠️ Potential issue | 🔴 CriticalEscape the fallback glyph.
When
ICON_SVGS[icon]misses,iconcomes straight from an attribute/text node and is interpolated into the template. That turns arbitrary text into markup inside the shadow root.Minimal fix
this.mount( STYLES, ` - <div class="icon">${svg || `<span class="glyph">${icon}</span>`}</div> + <div class="icon">${svg || `<span class="glyph"></span>`}</div> `, ); + + if (!svg) { + this.$<HTMLSpanElement>(".glyph")!.textContent = icon; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon-button.ts` around lines 78 - 82, The fallback glyph is injected as raw HTML into the template (in the mount call using STYLES and the svg || `<span class="glyph">${icon}</span>` expression), so untrusted icon text can become markup; fix by not interpolating raw icon into the template—either HTML-escape icon before interpolation or render a plain element and set its textContent after mounting (e.g., render `<span class="glyph"></span>` when svg is falsy, then set the element's textContent = icon), or add a utility escapeHtml and use it instead of direct ${icon}; update the code around mount, the svg/icon selection, and any related IconButton logic to ensure ICON_SVGS, svg, and icon never produce raw HTML.
🧹 Nitpick comments (1)
Build/src/primitives/slider.ts (1)
161-163: Full re-render on attribute change is inefficient.Calling
render()on every attribute change replaces the entire shadow DOM. For slider value updates, consider updating only the affected elements (filled width, input value) rather than re-mounting.♻️ Suggested approach
attributeChangedCallback(name: string, oldVal: string | null, newVal: string | null) { + if (oldVal === newVal) return; + + const slider = this.$(".slider") as HTMLInputElement; + const filled = this.$(".filled") as HTMLElement; + if (!slider || !filled) { + // Not rendered yet, render() will be called by connectedCallback + return; + } + + if (name === "value" && newVal !== null) { + const val = parseFloat(newVal); + const min = parseFloat(this.getAttribute("min") || "0"); + const max = parseFloat(this.getAttribute("max") || "100"); + const pct = ((val - min) / (max - min)) * 100; + slider.value = newVal; + filled.style.width = `${pct}%`; + } else { + // For structural changes (min, max, size), re-render - this.render(); + this.render(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 161 - 163, attributeChangedCallback currently calls render() for any attribute change which remounts the whole shadow DOM; change attributeChangedCallback to accept (name, oldValue, newValue) and handle only the slider-specific attributes (e.g., "value", "min", "max", "step") by updating the corresponding elements instead of re-rendering: set the inputElement.value for "value", adjust inputElement.min/max/step for those attributes, and update the filledElement style (or call a new helper updateFilledWidth(value)) to change the fill width; keep render() for initial mount but remove full re-render on simple attribute updates to avoid replacing the shadow DOM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/checkbox.ts`:
- Around line 80-85: The render() method always sets tabindex="0" which keeps
disabled checkboxes keyboard-focusable; update render() and _updateAria() so
disabled state controls both aria-disabled and tabindex: detect the disabled
state (e.g., this.hasAttribute("disabled") or this.disabled) and in
_updateAria() set aria-disabled="true"/"false" and set tabindex to "-1" when
disabled (and "0" when enabled), and remove or avoid unconditionally calling
this.setAttribute("tabindex", "0") in render(); ensure the click/keydown
handlers remain gated by the disabled check to prevent interaction.
In `@Build/src/primitives/icon-button.ts`:
- Around line 85-92: The host's focusability and aria announcement don't update
when the reflected disabled attribute changes; make attribute changes reactive
by implementing/augmenting attributeChangedCallback (or the class's observed
attribute handler) to call _updateTabIndex() and update aria-disabled
accordingly whenever "disabled" changes; ensure _updateTabIndex() uses the
current this.disabled value to set/remove tabindex and that
attributeChangedCallback also sets this.setAttribute("aria-disabled",
String(this.disabled)) (or removes it when appropriate); apply the same fix for
the other block referenced around the 95-103 area so both interactive updates
respond to runtime attribute changes.
In `@Build/src/primitives/label.ts`:
- Around line 28-30: The template for labelContent builds HTML but the else
branch is missing a closing </slot> tag causing malformed HTML; update the
labelContent assignment (the ternary using forAttr and the labelContent
variable) so the else string is `<label><slot></slot></label>` (i.e., add the
closing `</slot>`), ensuring both branches produce well-formed label HTML.
- Around line 27-32: The label generation currently interpolates forAttr (from
this.getAttribute("for")) directly into labelContent which can lead to HTML
attribute injection; instead, build the label without interpolating the raw
value and set the for attribute using DOM APIs after mounting: keep STYLES and
the base <label><slot></slot></label> markup in mount, then locate the mounted
label element (e.g., via this.shadowRoot or container returned by this.mount)
and call setAttribute("for", forAttr) or assign its .htmlFor property, ensuring
you only use the safe DOM API and/or explicitly escape quotes before assigning;
update references to forAttr, labelContent, mount, and STYLES accordingly.
In `@Build/src/primitives/progress.ts`:
- Around line 37-45: The component's declared props in progressConfig are not
wired into SazamiComponent's reactive accessors, so property assignments (e.g.,
el.value, el.variant, el.indeterminate) don't trigger updates; update
progressConfig to include all runtime props (add indeterminate) and mark the
props that should be reactive with reflect: true (value, max, min, size,
variant, indeterminate) so SazamiComponent will install the getters/setters and
mirror changes into attributes, then verify the component's render/update path
(the element class that extends SazamiComponent) reads from those properties
(not raw attributes) so property changes cause re-rendering.
- Around line 64-75: Clamp the aria-valuenow to the same rendered value: compute
a clamped value from the already calculated percent (e.g. let clampedValue = min
+ (percent / 100) * (max - min)) or by bounding value into [min,max], and when
setting aria-valuenow in Build/src/primitives/progress.ts use
String(clampedValue) instead of the raw value so screen readers see the exact
state shown by the visual bar (apply this change in the block that sets
aria-valuenow and references percent/aria-valuenow).
In `@Build/src/primitives/radio.ts`:
- Around line 86-99: _handleClick currently always dispatches a change and
queries the whole document for matching radios, causing duplicate events and
cross-group interference; update _handleClick to no-op if this.checked is
already true, scope the selector to this.getRootNode() (use const root =
this.getRootNode() as ParentNode) instead of document, escape the name with
CSS.escape(name) before building the selector, only removeAttribute("checked")
from matching saz-radio elements that are not the current element, then set
this.checked = true and call this.dispatchEventTyped("change", { value }) only
when the checked state actually changed; reference _handleClick,
this.getRootNode(), CSS.escape, and dispatchEventTyped in your changes.
- Around line 79-83: The radio element must expose state via ARIA and remove
disabled items from the tab order: in render() (or wherever attributes are
initialized) set aria-checked to the component's checked state and aria-disabled
to the disabled state (e.g., this.setAttribute("aria-checked",
String(this.checked)) and this.setAttribute("aria-disabled",
String(this.disabled))). Also stop hard-coding tabindex to "0": when disabled
set tabindex to "-1" (or remove it) and when enabled set it to "0" so disabled
radios are not tabbable. Update the same initialization that currently sets
role/tabindex (the block containing this.hasAttribute("role"),
this.hasAttribute("tabindex")) and ensure these ARIA/tabindex updates run
whenever checked/disabled properties change.
In `@Build/src/primitives/section.ts`:
- Around line 53-62: The slot listener is removed with a different function
reference than was added, so change the code to store and reuse a single bound
handler: create a property like this._boundComputeAndSetCenter and assign it
once (e.g., this._boundComputeAndSetCenter =
this._computeAndSetCenter.bind(this) before calling slot.addEventListener), use
that stored reference for addEventListener, and then call
slot.removeEventListener with the same this._boundComputeAndSetCenter in
disconnectedCallback; alternatively, convert _computeAndSetCenter to an
arrow-function class property so the same function reference is used for both
add/remove.
- Around line 39-51: The slotchange handler is passing the unbound method
_computeAndSetCenter so inside connectedCallback the method loses its this
context; fix by binding the method to the instance (e.g., assign
this._boundComputeAndSetCenter = this._computeAndSetCenter.bind(this) in the
constructor and use slot.addEventListener("slotchange",
this._boundComputeAndSetCenter) here) or by wrapping the call in an arrow
function in connectedCallback (slot.addEventListener("slotchange", () =>
this._computeAndSetCenter())); if you bind, store the bound reference so you can
remove the listener later.
In `@Build/src/primitives/select.ts`:
- Around line 154-166: The ArrowUp/ArrowDown handlers call
this._navigateOption(...) without ensuring there are any options, causing a
crash when this._options is empty; add a guard in handleKeydown to return early
if !this._options || this._options.length === 0 before calling
this._navigateOption, and also add the same early-return guard wherever else
Arrow key navigation is invoked (the other Arrow handler around the second
block), and optionally harden _navigateOption itself to return immediately when
this._options is falsy or empty to prevent future regressions.
- Around line 140-141: The dropdown option elements are not kept in sync with
the component's current value: _updateDisplay() updates only the trigger label
but never moves the "selected" class or updates aria-selected on each option, so
screen readers and open lists can announce the wrong item; update the code that
renders and updates options (the template mapping at the shown dropdown and the
later mappings around lines referenced) to set aria-selected and the "selected"
class based on the current value, and modify _updateDisplay() (and any
keyboard/click handlers that change value) to find the option elements (e.g., by
data-value) and toggle the "selected" class and aria-selected attributes
accordingly so visual state and accessibility state always reflect the
component's value.
In `@Build/src/primitives/slider.ts`:
- Around line 146-154: attributeChangedCallback calls render() which re-runs the
block that calls this.addHandler("input", ...) causing duplicate handlers to
accumulate; fix by guarding or removing before re-adding: add a boolean guard
(e.g., this._sliderInputHandlerAdded) or check an attribute on the slider
element to skip adding if already added, then set the flag after adding; update
the code around the slider variable and the this.addHandler("input", ...) call
in render() (or in attributeChangedCallback) so the handler for updating
filled.style.width, this.value and dispatchEventTyped("input") is only
registered once.
In `@Build/src/primitives/switch.ts`:
- Around line 91-113: The ARIA state is only updated on construction and
internal clicks, so make external changes to checked/disabled reflect into ARIA
by invoking _updateAria whenever those attributes/properties change: add
'checked' and 'disabled' to observedAttributes and implement
attributeChangedCallback(attributeName) to call this._updateAria(), or
alternatively ensure the checked and disabled property setters call
this._updateAria() after updating attributes; update reference points are the
private method _updateAria(), the _handleClick handler, and the class's
attribute/property change hooks (observedAttributes/attributeChangedCallback or
the checked/disabled setters).
In `@Build/src/primitives/tabs.ts`:
- Around line 109-121: The arrow-key handler updates the active tab but doesn't
move keyboard focus, causing navigation to stall; after calling
this._activateTab(...) in the handleKeydown inside tabButtons.forEach (and the
similar block at 125-133), compute the new index (e.g. const newIndex = (i + 1)
% this._tabs.length or (i - 1 + this._tabs.length) % this._tabs.length), call
this._activateTab(newIndex) and then call tabButtons[newIndex].focus() (or the
equivalent element reference) so focus moves to the newly activated tab button.
In `@Build/src/primitives/toast.ts`:
- Around line 92-113: The code injects user-controlled message into mount(...)
HTML which writes to shadow.innerHTML and enables XSS; update the toast
rendering in the mount call (used by the mount function and SazamiToast.show
path) to build DOM nodes instead of interpolating message into a template
string: create an element for the icon (using ICON_SVGS.* as before), create a
span for the message and set its textContent to the message value (not
innerHTML), and if showClose create a button element for the close action, then
append these nodes to the shadow root (or to the element mount inserts) while
still applying STYLES — ensure message is never concatenated into an HTML
string.
In `@Build/src/primitives/toggle.ts`:
- Around line 92-100: The _updateAria method currently only sets aria-checked;
extend it to also set aria-disabled based on the component's disabled state
(e.g., this.setAttribute("aria-disabled", this.disabled ? "true" : "false") or
remove the attribute when not disabled). Update the _updateAria() implementation
in the Toggle class and ensure any callers (attributeChangedCallback,
connectedCallback or initializers) continue to call _updateAria() so both
aria-checked and aria-disabled stay in sync with this.checked and this.disabled.
---
Duplicate comments:
In `@Build/src/primitives/base.ts`:
- Around line 233-239: removeAllHandlers currently always calls
this.removeEventListener(type, h.fn) so listeners attached to a different
element (stored as h.target by addHandler) are not removed; update
removeAllHandlers to call removeEventListener on the original target for each
handler (e.g., use (h.target || this).removeEventListener(type, h.fn)) when
iterating handlers so listeners added with options.element are properly
detached; reference handlers, h.target, h.fn, removeAllHandlers, addHandler, and
type to locate the change.
In `@Build/src/primitives/icon-button.ts`:
- Around line 78-82: The fallback glyph is injected as raw HTML into the
template (in the mount call using STYLES and the svg || `<span
class="glyph">${icon}</span>` expression), so untrusted icon text can become
markup; fix by not interpolating raw icon into the template—either HTML-escape
icon before interpolation or render a plain element and set its textContent
after mounting (e.g., render `<span class="glyph"></span>` when svg is falsy,
then set the element's textContent = icon), or add a utility escapeHtml and use
it instead of direct ${icon}; update the code around mount, the svg/icon
selection, and any related IconButton logic to ensure ICON_SVGS, svg, and icon
never produce raw HTML.
In `@Build/src/primitives/select.ts`:
- Around line 133-142: The template is unsafe because placeholder,
selectedOption?.label, and opt.label/opt.value are injected into a string passed
to this.mount which becomes innerHTML; instead, in the mount/render code that
uses STYLES and the template string, build the DOM nodes programmatically:
create the trigger div and dropdown container with document.createElement, set
attributes with setAttribute (or element.role/tabindex/aria-*), and assign
visible text via textContent for selectedOption.label or placeholder; for
options iterate this._options and create option elements setting data-value with
setAttribute and label via textContent (compare opt.value to value to add the
"selected" class). Keep use of mount/STYLES but swap the HTML string for
constructed nodes to prevent HTML injection.
In `@Build/src/primitives/spinner.ts`:
- Around line 58-78: The render() method currently interpolates the label into
the HTML string causing potential HTML injection; change it to mount the SVG and
an empty <span class="label"></span> (keep existing this.mount call but remove
${label} interpolation), then after mount call
this.shadowRoot.querySelector('.label') and set its textContent = label
(obtained via getAttribute("label") or default). This ensures label content is
inserted as plain text; update references in render(), getAttribute(), and the
mount() usage to follow this pattern.
In `@Build/src/primitives/switch.ts`:
- Around line 80-88: The render() method injects this.textContent directly into
the template string causing HTML to be parsed in the shadow root; change
render() to render the label via a <slot> or by inserting an empty <span
class="label"></span> in the template and then set its textContent after mount
to the trimmed label value. Locate render(), STYLES, and the mount(...) call:
replace the `${label ? `<span class="label">${label}</span>` : ""}`
interpolation with a persistent slot or span, and if using the span approach,
query the .label element after mount and assign its textContent = label to
ensure the label is treated as plain text.
In `@Build/src/primitives/tabs.ts`:
- Line 92: The template is inserting t.label directly into HTML which allows
markup from light DOM to be parsed; change the render to set button textContent
(or escape HTML) instead of interpolating raw label text: locate the map that
builds buttons (the expression using this._tabs.map(...) that creates <button
...>${t.label}</button>) and replace it by creating buttons programmatically or
using a safe escape utility so each button’s label is assigned via
element.textContent = t.label (or escapeHtml(t.label)) rather than embedding
t.label into the template string.
---
Nitpick comments:
In `@Build/src/primitives/slider.ts`:
- Around line 161-163: attributeChangedCallback currently calls render() for any
attribute change which remounts the whole shadow DOM; change
attributeChangedCallback to accept (name, oldValue, newValue) and handle only
the slider-specific attributes (e.g., "value", "min", "max", "step") by updating
the corresponding elements instead of re-rendering: set the inputElement.value
for "value", adjust inputElement.min/max/step for those attributes, and update
the filledElement style (or call a new helper updateFilledWidth(value)) to
change the fill width; keep render() for initial mount but remove full re-render
on simple attribute updates to avoid replacing the shadow DOM.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39fb55ba-8d50-4df8-ad29-2df7e38972e4
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
Build/jest.config.jsBuild/package.jsonBuild/src/primitives/avatar.tsBuild/src/primitives/badge.tsBuild/src/primitives/base.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/chip.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/input.tsBuild/src/primitives/label.tsBuild/src/primitives/modal.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/row.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/switch.tsBuild/src/primitives/tabs.tsBuild/src/primitives/tag.tsBuild/src/primitives/toast.tsBuild/src/primitives/toggle.tsBuild/tests/accessibility.test.tsBuild/tests/jest-setup.js
🚧 Files skipped from review as they are similar to previous changes (2)
- Build/package.json
- Build/tests/accessibility.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Docs/docs/api-reference.md (2)
242-302: Consider adding error functions to a dedicated exports section.The error handling functions (propertyError, eventError, renderError, bindingError, unknownComponentError) are well-documented with examples, but they appear only in this section without being listed in a main "exports" overview. For better discoverability, consider adding a brief mention or cross-reference in the main API sections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Docs/docs/api-reference.md` around lines 242 - 302, The docs show error helpers (propertyError, eventError, renderError, bindingError, unknownComponentError, plus tokenizerError and parserError) but they are not included in the main API exports overview; update the main "exports" or "API reference" section to list these functions and add a short cross-reference from the "Error Handling" section pointing to that exports list (mentioning the exact symbols propertyError, eventError, renderError, bindingError, unknownComponentError, tokenizerError, parserError) so users can discover them from the top-level API index.
343-345: Consider documenting the structure of new exported types.The new types (SazamiComponentConfig, PropertyConfig, EventConfig) are listed as exports but their structure and properties are not documented. Consider adding a brief description or example of each type's shape to help users understand how to use them, especially since they're part of the new component decorator system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Docs/docs/api-reference.md` around lines 343 - 345, Add brief type documentation for the newly exported types SazamiComponentConfig, PropertyConfig, and EventConfig: for each type list the key properties and their types/optionality and provide a short example or JSON-like snippet showing typical usage with the component decorator (e.g., typical fields like name, selector, properties: {key: type, default}, events: {name: payloadType}), and ensure the docs mention any default behaviors or required fields used by the decorator system (reference SazamiComponentConfig, PropertyConfig, EventConfig in the text so readers can map them to the exports).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Docs/docs/api-reference.md`:
- Around line 305-332: The Accessibility section lists components (Select, Tabs,
Modal, Toast, Slider, Button, Checkbox, Toggle, Slider) that lack API links or
documentation; either add inline API entries for each component under
"Primitives API" or add a clarifying sentence with links to their docs (for
example reference the "Primitives API" section and the "Component Base" page) so
readers know where to find component-specific props/methods; update the
Accessibility table to include link targets or a parenthetical note like "see
Component Base / per-component docs" and ensure component names (Select, Tabs,
Modal, Toast, Slider) match the actual doc filenames/anchors.
---
Nitpick comments:
In `@Docs/docs/api-reference.md`:
- Around line 242-302: The docs show error helpers (propertyError, eventError,
renderError, bindingError, unknownComponentError, plus tokenizerError and
parserError) but they are not included in the main API exports overview; update
the main "exports" or "API reference" section to list these functions and add a
short cross-reference from the "Error Handling" section pointing to that exports
list (mentioning the exact symbols propertyError, eventError, renderError,
bindingError, unknownComponentError, tokenizerError, parserError) so users can
discover them from the top-level API index.
- Around line 343-345: Add brief type documentation for the newly exported types
SazamiComponentConfig, PropertyConfig, and EventConfig: for each type list the
key properties and their types/optionality and provide a short example or
JSON-like snippet showing typical usage with the component decorator (e.g.,
typical fields like name, selector, properties: {key: type, default}, events:
{name: payloadType}), and ensure the docs mention any default behaviors or
required fields used by the decorator system (reference SazamiComponentConfig,
PropertyConfig, EventConfig in the text so readers can map them to the exports).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b8518b8-3191-4d5b-b0a2-8288be86af18
📒 Files selected for processing (2)
Build/jest.config.jsDocs/docs/api-reference.md
added 2 commits
March 15, 2026 00:27
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (6)
Build/src/primitives/select.ts (1)
202-220:⚠️ Potential issue | 🟠 MajorUpdate option selection state when
valuechanges.The
valuebranch only updates the trigger label. Clicks and externalvaluemutations leave.selectedandaria-selectedon the old option until_navigateOption()happens to run.Suggested fix
if (name === "value") { this._updateDisplay(); + this._updateSelectedState(); }Also applies to: 234-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 202 - 220, The display update only changes the trigger label and doesn't update option selection classes/ARIA, so modify the value-change path to also refresh option state: call the existing _updateSelectedState() whenever _updateDisplay() is invoked for a value change (or directly from the value setter/attributeChangedCallback that currently updates the trigger); ensure both the branch around _updateDisplay() and the other branch noted (lines referenced near 234-236) invoke _updateSelectedState() so .selected and aria-selected are updated for external value mutations as well (keep _navigateOption() behavior unchanged).Build/src/primitives/switch.ts (1)
81-96:⚠️ Potential issue | 🟠 MajorKeep ARIA synced for external
checked/disabledchanges.
_updateAria()only runs during the initial render and internal clicks. If a parent flips either reflected attribute,aria-checked,aria-disabled, andtabindexstay stale.Suggested fix
export class SazamiSwitch extends SazamiComponent<typeof switchConfig> { @@ private _updateAria() { this.setAttribute("aria-checked", String(this.checked)); @@ } } + + attributeChangedCallback(name: string, oldVal: string | null, newVal: string | null) { + if (oldVal === newVal) return; + if (name === "checked" || name === "disabled") { + this._updateAria(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/switch.ts` around lines 81 - 96, render currently calls _updateAria only on initial render and internal events, so external changes to reflected attributes leave aria-checked/aria-disabled/tabindex stale; fix by adding attribute change observation: implement static get observedAttributes() to include "checked" and "disabled" and add attributeChangedCallback(name, oldVal, newVal) that calls this._updateAria() (and any related state sync) when those attributes change, ensuring ARIA stays in sync with external updates to the component's checked/disabled state.Build/src/primitives/slider.ts (1)
107-156:⚠️ Potential issue | 🟠 MajorThe handler guard breaks the slider after the first re-render.
mount()replaces.sliderevery render, but_sliderHandlerAddednever resets. After any observed attribute change—and even on initial upgrade when observed attrs are present in markup—the new<input>has noinputlistener, so fill width,value, andsaz-inputstop updating.Suggested fix
export class SazamiSlider extends SazamiComponent<typeof sliderConfig> { @@ - private _sliderHandlerAdded = false; + private _inputHandlerId?: number; @@ - if (slider && !this._sliderHandlerAdded) { - this._sliderHandlerAdded = true; - this.addHandler("input", () => { + if (slider) { + if (this._inputHandlerId !== undefined) { + this.removeHandler(this._inputHandlerId); + } + this._inputHandlerId = this.addHandler("input", () => { const val = parseFloat(slider.value); const pct = ((val - min) / (max - min)) * 100; filled.style.width = `${pct}%`; this.value = val; this.dispatchEventTyped("input", { value: val }); }, { internal: true, element: slider }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 107 - 156, The guard _sliderHandlerAdded prevents re-attaching the input listener after mount() replaces the DOM; update render() so the handler is tracked per-input element instead of a single component flag: after getting slider (from this.$(".slider")) check/set a per-element marker (e.g., slider.dataset.sliderHandlerAdded or a symbol property) and only call addHandler("input", ...) when that element lacks the marker, then set the marker; alternatively remove _sliderHandlerAdded and always attach the handler after mount() while ensuring duplicate handlers are avoided by using the per-element marker. Ensure you reference render(), mount(), addHandler(), slider and filled when making the change.Build/src/primitives/radio.ts (2)
80-96:⚠️ Potential issue | 🟠 MajorUnchecked siblings keep stale ARIA state.
_handleClick()clears other radios by removingchecked, but this class never reacts to reflectedchecked/disabledchanges. The previously selected radio can keeparia-checked="true"and a staletabindexuntil something else rerenders it.Also applies to: 105-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/radio.ts` around lines 80 - 96, The radio element never updates ARIA when the checked/disabled attributes are changed externally; add an attributeChangedCallback (or otherwise observe reflected property changes) to call the existing _updateAria() whenever "checked" or "disabled" (and any reflected attributes) change so that aria-checked, tabindex and aria-disabled are kept in sync; modify the class to declare static get observedAttributes() { return ['checked','disabled']; } and implement attributeChangedCallback(name, oldV, newV) { if (oldV !== newV) this._updateAria(); } so that _updateAria() runs for both user clicks (_handleClick) and external attribute/property updates (also apply the same change for the other radio-related block that mirrors lines ~105-116).
102-112:⚠️ Potential issue | 🟠 MajorClear sibling radios by element identity, not by
value.
:not([value="..."])skips every radio that shares the same value, so duplicate values can leave multiple items checked. Querying the whole root also still couples same-named radios across different forms.Suggested fix
const name = this.getAttribute("name") || ""; const value = this.getAttribute("value") || ""; // Uncheck other radios with same name - const root = this.getRootNode() as ParentNode; - if (root) { - const escapedName = CSS.escape(name); - root.querySelectorAll(`saz-radio[name="${escapedName}"]:not([value="${CSS.escape(value)}"])`).forEach((el) => { - el.removeAttribute("checked"); - }); - } + const scope = this.closest("form") ?? (this.getRootNode() as ParentNode); + scope.querySelectorAll("saz-radio").forEach((el) => { + if (el !== this && el.getAttribute("name") === name) { + el.removeAttribute("checked"); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/radio.ts` around lines 102 - 112, The unchecking logic currently uses a selector with :not([value="..."]) which fails when multiple radios share the same value and also queries the entire root, coupling radios across forms; update the block that uses getRootNode() and CSS.escape(name)/value to instead scope to the nearest form or root (e.g. const scope = this.closest('form') || this.getRootNode()) and query all matching radios by name within that scope (querySelectorAll(`saz-radio[name="${escapedName}"]`)), then iterate and removeAttribute('checked') only when the element identity differs from the current radio (if (el !== this) el.removeAttribute('checked')), ensuring radios are cleared by element identity and limited to the local form/root.Build/src/primitives/toast.ts (1)
91-145:⚠️ Potential issue | 🟠 MajorAdd live-region semantics for toast announcements.
Transient toast content is still not announced to screen readers because
role/aria-live/aria-atomicare missing inrender().Suggested patch
render() { const variant = this.getAttribute("variant") || "default"; @@ const icon = variant === "success" ? ICON_SVGS.check : variant === "error" || variant === "danger" ? ICON_SVGS.close : variant === "warning" ? "⚠" : "ℹ"; + const urgent = variant === "error" || variant === "danger"; + + if (!this.hasAttribute("role")) { + this.setAttribute("role", urgent ? "alert" : "status"); + } + if (!this.hasAttribute("aria-live")) { + this.setAttribute("aria-live", urgent ? "assertive" : "polite"); + } + this.setAttribute("aria-atomic", "true");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` around lines 91 - 145, The toast lacks ARIA live-region attributes so screen readers won't announce it; in render() update the element you mount (via this.mount(...)) to include live-region semantics (e.g., role="status", aria-live="polite", aria-atomic="true") on the toast container (or the element wrapping the .message span) so the message text inserted into the .message span is announced; ensure these attributes are present in the markup passed to this.mount and retained when setAttribute("visible") is applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/escape.ts`:
- Around line 49-71: The exported helper html currently implies safe general
HTML but only performs text-node escaping; change the API to explicitly be a
text-node interpolator: rename/export html as text (or textNode) and update the
doc comment to state it only escapes interpolated values for insertion into text
nodes, not attributes or URLs; keep using escapeHtml inside the function but
remove any broader “safe HTML” claim and add a note telling callers to use
context-specific escaping for attributes, URLs, styles, or event handlers (e.g.,
provide/point to separate functions for attribute/url/style escaping) so callers
must opt into context-aware escaping rather than relying on html/escapeHtml for
all contexts.
In `@Build/src/primitives/base.ts`:
- Around line 137-140: disconnectedCallback() currently only runs _cleanupFns
but does not clear the component's handler registry, so handlers added via
addHandler(options.element) remain active; update disconnectedCallback() to call
removeAllHandlers() (the method that clears the addHandler registry and
unregisters external listeners) before or in addition to running and clearing
this._cleanupFns, and ensure removeAllHandlers() itself removes any
element-specific listeners and clears related internal maps so no stale
references remain (also apply the same change where similar teardown logic
exists for other lifecycle hooks using _cleanupFns).
- Around line 107-145: The base class exposes static observedAttributes but
lacks attributeChangedCallback, so attribute reflections never trigger updates;
add an attributeChangedCallback(name, oldValue, newValue) method on the base
class (alongside observedAttributes, connectedCallback, disconnectedCallback,
render) that ignores no-op changes (oldValue === newValue) and calls
this.render() when the element is already rendered (check this._rendered) so
reflected attribute updates cause a rerender of subclasses like avatar and
label; keep the method lightweight and reference the existing symbols
observedAttributes, attributeChangedCallback, connectedCallback, _rendered, and
render.
In `@Build/src/primitives/icon-button.ts`:
- Around line 56-61: The icon stops updating because render() reads
getAttribute("icon") while the icon property is declared with reflect: false and
is not observed after initial render; fix by making the property reactive and
in-sync with attributes: update icon (and similarly size/variant if needed) in
iconButtonConfig.properties to reflect: true and/or ensure the element observes
the attribute (so attribute changes trigger re-render), and modify render() to
use the reactive property (this.icon) instead of getAttribute("icon") to keep
mounted glyph/SVG and aria-label in sync.
In `@Build/src/primitives/icon.ts`:
- Around line 32-38: The icon property is not observed so changes after first
render are ignored; update iconConfig to mark icon as reflective (set
properties.icon.reflect to true) so the base class auto-installs the property
reflector and the component will receive updates, and apply the same change to
size/variant if they must be reactive as well (alternatively implement
attributeChangedCallback/requestUpdate inside the component's render path to
resample the SVG when attributes change).
In `@Build/src/primitives/modal.ts`:
- Around line 136-145: Update the attributeChangedCallback signatures to accept
nullable values: change the parameter types for oldVal and newVal from string to
string | null in modal's attributeChangedCallback (and mirror the same change in
select.ts, toggle.ts, and checkbox.ts) so the method matches the DOM spec and
TypeScript strict mode; keep the existing null check (if (newVal !== null) ...)
as-is and optionally add a defensive super.attributeChangedCallback(name,
oldVal, newVal) call at the start of the method if you want to future-proof the
class hierarchy.
In `@Build/src/primitives/section.ts`:
- Around line 46-50: attributeChangedCallback/connectedCallback are creating a
new ResizeObserver unconditionally when the "center-point" attribute is present,
which can allocate two observers; before creating a new observer in
connectedCallback and in attributeChangedCallback, check if this._resizeObserver
already exists and reuse it, or call this._resizeObserver.disconnect() and null
it out before assigning a new ResizeObserver; ensure the observer creation logic
that calls this._computeAndSetCenter() is centralized (reuse the same creation
branch) and also ensure disconnectedCallback disconnects and clears
this._resizeObserver to avoid leaks.
In `@Build/src/primitives/select.ts`:
- Around line 147-183: The render() currently attaches event handlers and sets
tabindex only once, so when the disabled state toggles the trigger stays in the
wrong state; update attributeChangedCallback() to observe the "disabled"
attribute and, when it changes, enable or disable interactivity by
adding/removing or enabling the same handlers you registered via addHandler
(click on trigger, keydown on trigger, click on dropdown) and updating the
trigger's tabindex accordingly; refactor the handler attachment logic into a
private helper (e.g., _wireTriggerHandlers or _setInteractiveState) and call it
from both render() and attributeChangedCallback() so re-enabled selects reattach
handlers and disabled selects remove them (apply same change to the other
handler block referenced around lines 222-236).
In `@Build/src/primitives/slider.ts`:
- Around line 160-165: The override of observedAttributes in the Slider class
drops the base attributes and omits "step" and "disabled", so changes to those
attributes don't update the internal <input>; fix by returning the superclass's
observedAttributes merged with the missing attributes (e.g. return
[...super.observedAttributes, "step", "disabled"] in the observedAttributes
getter) and ensure attributeChangedCallback has the proper signature
(attributeName, oldValue, newValue) and calls this.render() (or delegates to
super.attributeChangedCallback) so changes to "step" and "disabled" refresh the
shadow <input>.
In `@Build/src/primitives/tabs.ts`:
- Around line 96-100: The code rebuilds panels by serializing panel.innerHTML
into new HTML strings (panels.map ... returning `<div ...>${content}</div>`),
which re-parses and recreates elements and listeners; instead, keep and reuse
the original panel DOM nodes: in the render routine that uses panels and
activeTab, for each panel element set its role/id/aria-labelledby/classList and
style.display based on activeTab (e.g., panel.classList.toggle("active",
i.toString() === activeTab); panel.style.display = i.toString() === activeTab ?
"block" : "none"), append the existing panel node to the panels container if not
already a child, and avoid using innerHTML or creating new string DOM; update
any code that references the generated id/class (e.g., "panel-{i}", "panel"
class) to match this in-place approach.
In `@Build/tests/jest-setup.js`:
- Around line 16-18: Replace the ad-hoc global.CSS.escape implementation in
Build/tests/jest-setup.js with the spec-compliant implementation from the
css.escape package: add "css.escape" to devDependencies in package.json,
require/import cssEscape (from "css.escape") inside the jest-setup file, and
assign global.CSS.escape = cssEscape so all edge cases (NULL, control chars,
leading digits/hyphen, lone "-" etc.) are handled correctly instead of the
current regex-only function.
---
Duplicate comments:
In `@Build/src/primitives/radio.ts`:
- Around line 80-96: The radio element never updates ARIA when the
checked/disabled attributes are changed externally; add an
attributeChangedCallback (or otherwise observe reflected property changes) to
call the existing _updateAria() whenever "checked" or "disabled" (and any
reflected attributes) change so that aria-checked, tabindex and aria-disabled
are kept in sync; modify the class to declare static get observedAttributes() {
return ['checked','disabled']; } and implement attributeChangedCallback(name,
oldV, newV) { if (oldV !== newV) this._updateAria(); } so that _updateAria()
runs for both user clicks (_handleClick) and external attribute/property updates
(also apply the same change for the other radio-related block that mirrors lines
~105-116).
- Around line 102-112: The unchecking logic currently uses a selector with
:not([value="..."]) which fails when multiple radios share the same value and
also queries the entire root, coupling radios across forms; update the block
that uses getRootNode() and CSS.escape(name)/value to instead scope to the
nearest form or root (e.g. const scope = this.closest('form') ||
this.getRootNode()) and query all matching radios by name within that scope
(querySelectorAll(`saz-radio[name="${escapedName}"]`)), then iterate and
removeAttribute('checked') only when the element identity differs from the
current radio (if (el !== this) el.removeAttribute('checked')), ensuring radios
are cleared by element identity and limited to the local form/root.
In `@Build/src/primitives/select.ts`:
- Around line 202-220: The display update only changes the trigger label and
doesn't update option selection classes/ARIA, so modify the value-change path to
also refresh option state: call the existing _updateSelectedState() whenever
_updateDisplay() is invoked for a value change (or directly from the value
setter/attributeChangedCallback that currently updates the trigger); ensure both
the branch around _updateDisplay() and the other branch noted (lines referenced
near 234-236) invoke _updateSelectedState() so .selected and aria-selected are
updated for external value mutations as well (keep _navigateOption() behavior
unchanged).
In `@Build/src/primitives/slider.ts`:
- Around line 107-156: The guard _sliderHandlerAdded prevents re-attaching the
input listener after mount() replaces the DOM; update render() so the handler is
tracked per-input element instead of a single component flag: after getting
slider (from this.$(".slider")) check/set a per-element marker (e.g.,
slider.dataset.sliderHandlerAdded or a symbol property) and only call
addHandler("input", ...) when that element lacks the marker, then set the
marker; alternatively remove _sliderHandlerAdded and always attach the handler
after mount() while ensuring duplicate handlers are avoided by using the
per-element marker. Ensure you reference render(), mount(), addHandler(), slider
and filled when making the change.
In `@Build/src/primitives/switch.ts`:
- Around line 81-96: render currently calls _updateAria only on initial render
and internal events, so external changes to reflected attributes leave
aria-checked/aria-disabled/tabindex stale; fix by adding attribute change
observation: implement static get observedAttributes() to include "checked" and
"disabled" and add attributeChangedCallback(name, oldVal, newVal) that calls
this._updateAria() (and any related state sync) when those attributes change,
ensuring ARIA stays in sync with external updates to the component's
checked/disabled state.
In `@Build/src/primitives/toast.ts`:
- Around line 91-145: The toast lacks ARIA live-region attributes so screen
readers won't announce it; in render() update the element you mount (via
this.mount(...)) to include live-region semantics (e.g., role="status",
aria-live="polite", aria-atomic="true") on the toast container (or the element
wrapping the .message span) so the message text inserted into the .message span
is announced; ensure these attributes are present in the markup passed to
this.mount and retained when setAttribute("visible") is applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f2b96a7-1b2f-46f0-aa02-c16c5dc7cbdd
📒 Files selected for processing (20)
Build/src/escape.tsBuild/src/index.tsBuild/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/icon.tsBuild/src/primitives/label.tsBuild/src/primitives/modal.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/switch.tsBuild/src/primitives/tabs.tsBuild/src/primitives/toast.tsBuild/src/primitives/toggle.tsBuild/tests/jest-setup.js
added 2 commits
March 15, 2026 00:45
coderabbit i'm tiredddd
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Build/src/primitives/accordion.ts (1)
86-103:⚠️ Potential issue | 🟠 MajorAccordion body content is re-parsed from raw
innerHTML.Content is serialized and injected back into a template string, which recreates descendant DOM and keeps an avoidable injection surface. Prefer preserving original slotted nodes and toggling visibility/state on those nodes instead of string rehydration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/accordion.ts` around lines 86 - 103, The code currently serializes each accordion panel via item.content = el.innerHTML and then re-injects that HTML inside the template string in mount, which recreates DOM and re-parses user content; instead, preserve and reuse the original element nodes: when building items replace item.content with a reference to the original element (e.g., store el or el.children/childNodes), and update the mounting logic in mount (the STYLES + items.map block) to create DOM nodes programmatically and append or move the preserved node into the ".inner-content" container (or toggle visibility/class on the original panel), rather than interpolating item.content into a string; update uses of item.open to set attributes/aria-expanded on the created button and ensure you do not use innerHTML for user content anywhere in accordion.ts.
♻️ Duplicate comments (5)
Build/src/primitives/icon-button.ts (1)
115-124:⚠️ Potential issue | 🟠 Major
iconattribute changes won't trigger re-render due to overriddenobservedAttributes.The
iconButtonConfigsetsiconwithreflect: true, which would normally cause the base class to observe it. However, this class overridesstatic get observedAttributes()to return only["disabled"], so changing theiconattribute after initial render won't update the displayed icon.This was flagged in a past review but appears unresolved. Consider including
"icon"in observed attributes, or delegate to the base class and only add special handling fordisabled.🐛 Proposed fix
static get observedAttributes() { - return ["disabled"]; + return ["disabled", "icon"]; } + + attributeChangedCallback(name: string, oldVal: string | null, newVal: string | null) { + if (oldVal === newVal) return; + if (name === "disabled") { + this._updateTabIndex(); + } + if (name === "icon") { + // Re-render to update the displayed icon + this.render(); + } + }Alternatively, call the base class's
observedAttributesand merge:static get observedAttributes() { - return ["disabled"]; + return [...SazamiComponent.observedAttributes, "disabled"]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon-button.ts` around lines 115 - 124, The class currently overrides static get observedAttributes() to return only ["disabled"], which prevents changes to the reflected "icon" attribute (set via iconButtonConfig) from triggering attributeChangedCallback and re-render; update observedAttributes to include "icon" (or merge super.observedAttributes() with ["disabled", "icon"]) and keep attributeChangedCallback handling for "disabled" (and allow the base class to handle "icon" updates) so changes to the icon attribute will properly re-render the component.Build/src/primitives/tabs.ts (1)
96-100:⚠️ Potential issue | 🟠 MajorPanels are still rebuilt from
innerHTMLeach render.This reparses panel markup and recreates DOM nodes on every render, which can drop nested state/listeners and keeps an avoidable HTML-injection surface. Prefer reusing slotted panel nodes and toggling visibility/ARIA in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/tabs.ts` around lines 96 - 100, The current render rebuilds panels from panel.innerHTML (the panels.map in Build/src/primitives/tabs.ts), which reparses markup and loses nested state/listeners; instead locate and reuse the existing slotted panel DOM nodes (the original panels NodeList or Slot children) and update their attributes in place: toggle the "active" class, set style.display to "block"/"none" (or aria-hidden), update role/aria-labelledby/id as needed for the activeTab value, and avoid generating new innerHTML strings; modify the code that currently returns `<div class="panel...">${content}</div>` to operate on the existing element references (panel elements) and only change visibility/ARIA/ classes.Build/src/primitives/select.ts (3)
222-222:⚠️ Potential issue | 🟡 MinorUse nullable DOM callback parameter types.
attributeChangedCallbackshould acceptoldVal/newValasstring | null(DOM contract). Current signature is narrower.#!/bin/bash set -euo pipefail rg -nP 'attributeChangedCallback\(\s*name:\s*string,\s*oldVal:\s*string,\s*newVal:\s*string\)' Build/src/primitives🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` at line 222, The attributeChangedCallback signature is too narrow—change the method signature in the Select element (attributeChangedCallback) so oldVal and newVal are typed as string | null instead of string, and update any internal uses inside attributeChangedCallback (and any helpers it calls) to handle null safely (e.g., use null-safe comparisons or coalesce before string ops) to satisfy the DOM contract; ensure any overrides or calls match the new signature.
173-181:⚠️ Potential issue | 🟠 MajorSelection state is not fully synced after click/value updates.
At Lines 177-181, click selection updates
valueand display text but doesn’t update.selected/aria-selectedstate in that same path.Suggested fix
if (target.classList.contains("option")) { const newValue = target.getAttribute("data-value") || ""; this.value = newValue; this.open = false; this.dispatchEventTyped("change", { value: newValue }); + this._updateSelectedState(); this._updateDisplay(); } @@ if (name === "value") { + this._updateSelectedState(); this._updateDisplay(); }Also applies to: 234-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 173 - 181, The click handler handleDropdownClick sets this.value, closes the dropdown and updates the display but never syncs the option elements' visual/ARIA selection state; after setting this.value and before/after this._updateDisplay() update the option elements (e.g., iterate this.shadowRoot.querySelectorAll('.option') or use the existing selection update helper) to add/remove the "selected" class and set aria-selected="true"/"false" based on each option's data-value === this.value; apply the same change to the analogous update path referenced around the other handler (the lines noted at 234-236) so both click and the other value-update path keep .selected and aria-selected in sync with this.value.
222-237:⚠️ Potential issue | 🟠 Major
disabledchanges are still not re-wiring interactivity.At Line 222 onward, the callback ignores
disabled, so toggling disabled state can leave handlers/tabindex stale.Suggested fix
attributeChangedCallback(name: string, oldVal: string, newVal: string) { if (oldVal === newVal) return; + if (name === "disabled") { + this.removeAllHandlers({ source: "internal" }); + this.render(); + return; + } if (name === "open") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 222 - 237, attributeChangedCallback currently ignores changes to the "disabled" attribute which leaves event handlers and focusability stale; add a branch for if (name === "disabled") that reads newVal !== null and updates the control: call a new or existing helper (e.g., this._updateInteractivity or inline logic) to set trigger.setAttribute('aria-disabled', value), adjust trigger.tabIndex (e.g., -1 when disabled, 0 when enabled), close the dropdown if disabling (remove "open"), and attach/detach pointer/click/keyboard handlers accordingly so the component's interactivity and focus state are rewired when disabled toggles (use identifiers attributeChangedCallback, this.$('.trigger'), this.$('.dropdown'), and this._updateDisplay to locate where to integrate).
🧹 Nitpick comments (2)
Build/src/primitives/image.ts (2)
41-46: XSS fix applied withescapeHtml, but consider handling emptysrc.The
escapeHtmlusage addresses the past XSS concern. However, whensrcis empty, this still renders<img src="" alt="" />, which may cause a broken image indicator or an unnecessary network request to the page URL.♻️ Proposed fix to skip rendering when no source
render() { const src = this.getAttribute("src") || this.textContent?.trim() || ""; const alt = this.getAttribute("alt") || ""; - this.mount(STYLES, `<img src="${escapeHtml(src)}" alt="${escapeHtml(alt)}" />`); + this.mount(STYLES, src ? `<img src="${escapeHtml(src)}" alt="${escapeHtml(alt)}" />` : ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/image.ts` around lines 41 - 46, The render method in the image primitive (render) should skip mounting/rendering when there's no effective image source to avoid an empty src attribute causing broken images or unwanted requests; after computing src (using this.getAttribute("src") || this.textContent?.trim() || ""), check if src is falsy/empty and return early instead of calling this.mount(STYLES, ...); keep escapeHtml for src and alt and only call this.mount when src is non-empty.
2-2: Unused import:SIZE_RULES.
SIZE_RULESis imported but not used in theSTYLESconstant. The file has its own inline size rules (lines 19-22).♻️ Proposed fix
-import { SHAPE_RULES, SIZE_RULES } from "./shared"; +import { SHAPE_RULES } from "./shared";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/image.ts` at line 2, Remove the unused SIZE_RULES import from the top import list (the import of SIZE_RULES from "./shared") since the STYLES constant in this module uses its own inline size rules; update the import statement to only import SHAPE_RULES, and ensure no other symbol in this file references SIZE_RULES (check for any usage in STYLES or related helpers and remove or replace it if found).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/accordion.ts`:
- Around line 120-122: The forEach callback uses a concise arrow-body which
violates Biome’s iterable-callback rule; update the callback to a block body so
the linter accepts it by changing the call on headers.forEach to use a block
callback (e.g., headers.forEach((h) => { (h as
HTMLElement).setAttribute("aria-expanded", "false"); });), locating the usage in
Build/src/primitives/accordion.ts where headers and setAttribute are referenced.
In `@Build/src/primitives/chip.ts`:
- Around line 120-122: Render currently sets role and tabIndex but doesn't
initialize aria-pressed, which is only updated later in the click handler; in
the render method (where you call this._updateTabIndex()) add initialization of
aria-pressed by calling this.setAttribute('aria-pressed', this.selected ? 'true'
: 'false') (or removing it if not applicable), and ensure the same attribute
name is used in the click/toggle handler (the method that currently updates
aria-pressed on click) so initial render and subsequent interactions stay in
sync.
In `@Build/src/primitives/icon-button.ts`:
- Around line 101-103: render() currently calls this.addHandler("click",
this._handleClick, { internal: true }) and this.addHandler("keydown",
this._handleKeydown, { internal: true }) on every render which accumulates
duplicates; before adding handlers, call the corresponding removal to avoid
duplicates (e.g., call this.removeHandler("click", this._handleClick) and
this.removeHandler("keydown", this._handleKeydown) or use your class's handler
cleanup API) then re-add them, ensuring you reference the same handler functions
(_handleClick and _handleKeydown) used in addHandler so removals match
additions.
In `@Build/src/primitives/input.ts`:
- Around line 57-61: The render() method reads placeholder and type from
attributes but those attrs aren’t observed or applied in
attributeChangedCallback, so runtime changes can get out of sync; add
'placeholder' and 'type' to the element's static get observedAttributes(),
implement attributeChangedCallback(name, oldV, newV) to update the internal
<input> (e.g. update this._input.placeholder and this._input.type when those
attributes change), and ensure any property setters for placeholder/type either
reflect to attributes or directly update the internal input so render(),
attribute changes and property assignments remain in sync (look for render(),
attributeChangedCallback, and any placeholder/type property accessors to
modify).
- Around line 87-90: The handler that updates the DOM input's value currently
ignores newVal === null and leaves stale text; in the block handling name ===
"value" (the conditional that checks input.value and assigns newVal) update
logic to handle attribute removal by clearing the DOM value: when newVal ===
null set input.value = "" (if it differs) else proceed with the existing
assignment path (set input.value = newVal when different).
In `@Build/src/primitives/radio.ts`:
- Around line 105-111: The selector currently excludes by value which can leave
multiple checked radios; instead select all peers by name (use the same
`escapedName` with `saz-radio[name="..."]`) and in the forEach compare element
identity to `this` (skip when `el === this`) and call
`el.removeAttribute("checked")` for others; locate the logic around
`this.getRootNode()`, `CSS.escape(name)`, the `saz-radio` query and the
`removeAttribute("checked")` call and replace the `:not([value="..."])`
exclusion with an identity check in the loop.
In `@Build/src/primitives/section.ts`:
- Around line 80-100: The override of attributeChangedCallback in the Section
component must call the base class implementation to preserve
SazamiComponent.attributeChangedCallback behavior; update
attributeChangedCallback(name, oldVal, newVal) to invoke
super.attributeChangedCallback(name, oldVal, newVal) (either at the start or end
of the method) while keeping the existing logic that handles "center-point"
(calls to _computeAndSetCenter and _setupResizeObserver, and disconnecting
_resizeObserver and deleting dataset.centerX/centerY).
In `@Build/src/primitives/slider.ts`:
- Line 127: Guard the percent calculation so you never divide by zero: when
computing percent from (value - min) / (max - min) (the occurrences that assign
to the percent variable), check if max === min first and set percent to 0 (or a
sensible default), otherwise compute and then clamp the result to the 0–100
range; update both places where percent is calculated (the assignments that use
value, min, max and compute percent) to use this guarded logic so filled width
rendering cannot get NaN/Infinity.
- Around line 107-157: The problem is that the single boolean
_sliderHandlerAdded prevents re-attaching the input handler after render
replaces the slider element; in render() (and the slider binding block) change
the logic to track handlers per DOM element instead of a single flag: detect
whether the current slider element already has a handler (e.g. set a marker on
the slider element such as slider.dataset.handlerAdded or use a WeakMap keyed by
the element) and only call addHandler when the current slider element lacks that
marker, then mark it after adding; alternatively reset _sliderHandlerAdded to
false before mounting so the new element gets bound — update references in
render(), the slider variable, and the addHandler call accordingly.
In `@Build/src/primitives/tabs.ts`:
- Around line 155-158: attributeChangedCallback currently calls
this._activateTab(Number(newVal ?? 0), false) without validating the parsed
index; parse newVal safely, ensure it's a finite integer and within the valid
tab range before calling _activateTab, and otherwise default to a safe index
(e.g. 0) or skip activation. Update attributeChangedCallback to parse
Number(newVal), check Number.isFinite and Number.isInteger, clamp to
0..(this._tabs?.length - 1) or handle empty tabs, then call
this._activateTab(validIndex, false) only when validIndex is determined;
reference attributeChangedCallback and _activateTab to locate the change.
---
Outside diff comments:
In `@Build/src/primitives/accordion.ts`:
- Around line 86-103: The code currently serializes each accordion panel via
item.content = el.innerHTML and then re-injects that HTML inside the template
string in mount, which recreates DOM and re-parses user content; instead,
preserve and reuse the original element nodes: when building items replace
item.content with a reference to the original element (e.g., store el or
el.children/childNodes), and update the mounting logic in mount (the STYLES +
items.map block) to create DOM nodes programmatically and append or move the
preserved node into the ".inner-content" container (or toggle visibility/class
on the original panel), rather than interpolating item.content into a string;
update uses of item.open to set attributes/aria-expanded on the created button
and ensure you do not use innerHTML for user content anywhere in accordion.ts.
---
Duplicate comments:
In `@Build/src/primitives/icon-button.ts`:
- Around line 115-124: The class currently overrides static get
observedAttributes() to return only ["disabled"], which prevents changes to the
reflected "icon" attribute (set via iconButtonConfig) from triggering
attributeChangedCallback and re-render; update observedAttributes to include
"icon" (or merge super.observedAttributes() with ["disabled", "icon"]) and keep
attributeChangedCallback handling for "disabled" (and allow the base class to
handle "icon" updates) so changes to the icon attribute will properly re-render
the component.
In `@Build/src/primitives/select.ts`:
- Line 222: The attributeChangedCallback signature is too narrow—change the
method signature in the Select element (attributeChangedCallback) so oldVal and
newVal are typed as string | null instead of string, and update any internal
uses inside attributeChangedCallback (and any helpers it calls) to handle null
safely (e.g., use null-safe comparisons or coalesce before string ops) to
satisfy the DOM contract; ensure any overrides or calls match the new signature.
- Around line 173-181: The click handler handleDropdownClick sets this.value,
closes the dropdown and updates the display but never syncs the option elements'
visual/ARIA selection state; after setting this.value and before/after
this._updateDisplay() update the option elements (e.g., iterate
this.shadowRoot.querySelectorAll('.option') or use the existing selection update
helper) to add/remove the "selected" class and set aria-selected="true"/"false"
based on each option's data-value === this.value; apply the same change to the
analogous update path referenced around the other handler (the lines noted at
234-236) so both click and the other value-update path keep .selected and
aria-selected in sync with this.value.
- Around line 222-237: attributeChangedCallback currently ignores changes to the
"disabled" attribute which leaves event handlers and focusability stale; add a
branch for if (name === "disabled") that reads newVal !== null and updates the
control: call a new or existing helper (e.g., this._updateInteractivity or
inline logic) to set trigger.setAttribute('aria-disabled', value), adjust
trigger.tabIndex (e.g., -1 when disabled, 0 when enabled), close the dropdown if
disabling (remove "open"), and attach/detach pointer/click/keyboard handlers
accordingly so the component's interactivity and focus state are rewired when
disabled toggles (use identifiers attributeChangedCallback, this.$('.trigger'),
this.$('.dropdown'), and this._updateDisplay to locate where to integrate).
In `@Build/src/primitives/tabs.ts`:
- Around line 96-100: The current render rebuilds panels from panel.innerHTML
(the panels.map in Build/src/primitives/tabs.ts), which reparses markup and
loses nested state/listeners; instead locate and reuse the existing slotted
panel DOM nodes (the original panels NodeList or Slot children) and update their
attributes in place: toggle the "active" class, set style.display to
"block"/"none" (or aria-hidden), update role/aria-labelledby/id as needed for
the activeTab value, and avoid generating new innerHTML strings; modify the code
that currently returns `<div class="panel...">${content}</div>` to operate on
the existing element references (panel elements) and only change
visibility/ARIA/ classes.
---
Nitpick comments:
In `@Build/src/primitives/image.ts`:
- Around line 41-46: The render method in the image primitive (render) should
skip mounting/rendering when there's no effective image source to avoid an empty
src attribute causing broken images or unwanted requests; after computing src
(using this.getAttribute("src") || this.textContent?.trim() || ""), check if src
is falsy/empty and return early instead of calling this.mount(STYLES, ...); keep
escapeHtml for src and alt and only call this.mount when src is non-empty.
- Line 2: Remove the unused SIZE_RULES import from the top import list (the
import of SIZE_RULES from "./shared") since the STYLES constant in this module
uses its own inline size rules; update the import statement to only import
SHAPE_RULES, and ensure no other symbol in this file references SIZE_RULES
(check for any usage in STYLES or related helpers and remove or replace it if
found).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e9acdcf-6a27-4fe8-9b3f-6818a0b9a209
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
Build/package.jsonBuild/src/escape.tsBuild/src/index.tsBuild/src/primitives/accordion.tsBuild/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/chip.tsBuild/src/primitives/coverart.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/label.tsBuild/src/primitives/modal.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/switch.tsBuild/src/primitives/tabs.tsBuild/src/primitives/toast.tsBuild/src/primitives/toggle.tsBuild/tests/jest-setup.js
🚧 Files skipped from review as they are similar to previous changes (2)
- Build/package.json
- Build/src/primitives/progress.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Build/src/primitives/accordion.ts (1)
81-108:⚠️ Potential issue | 🟠 MajorConsider avoiding
innerHTMLserialization for accordion content.Line 86 captures
el.innerHTMLand Line 102 interpolates it directly into the template. WhileescapeHtmlis correctly used foritem.title, the content is not escaped. If accordion items contain user-generated or data-driven HTML, this creates an XSS risk.Additionally, re-serializing
innerHTMLon every render destroys event listeners and element state in the original content.Alternative approach
Instead of serializing content, consider:
- Mount the accordion structure with empty content containers
- Move the original light-DOM elements into the shadow DOM content areas
- Toggle visibility via attributes/classes
This preserves existing DOM nodes and their event listeners while avoiding HTML re-parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/accordion.ts` around lines 81 - 108, The render() method currently serializes light-DOM content via el.innerHTML into items and interpolates it into the template (see render, items, mount, STYLES, escapeHtml), which causes XSS risk and destroys original DOM/event state; instead, build the accordion markup with empty content containers (mount the header and an empty .inner-content for each item), then move each original child element into its corresponding content container using DOM operations (appendChild/replaceChild) so the original nodes and listeners are preserved, toggle open state via attributes/classes and aria-expanded, and keep escapeHtml only for titles while removing any direct innerHTML interpolation.
♻️ Duplicate comments (6)
Build/src/primitives/input.ts (1)
87-105:⚠️ Potential issue | 🟡 MinorHandle removed attributes in the sync path.
Lines 95-104 only apply updates when
newVal !== null.removeAttribute("value"),removeAttribute("placeholder"), orremoveAttribute("type")therefore leaves stale state in the inner<input>. Clearvalue/placeholderand resettypeto"text"when the attribute disappears.Possible fix
- if (name === "value" && newVal !== null) { - if (input.value !== newVal) { - input.value = newVal; - } + if (name === "value") { + const next = newVal ?? ""; + if (input.value !== next) { + input.value = next; + } } else if (name === "disabled") { input.disabled = newVal !== null; - } else if (name === "placeholder" && newVal !== null) { - input.placeholder = newVal; - } else if (name === "type" && newVal !== null) { - input.type = newVal; + } else if (name === "placeholder") { + input.placeholder = newVal ?? ""; + } else if (name === "type") { + input.type = newVal || "text"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/input.ts` around lines 87 - 105, In attributeChangedCallback, handle attribute removals (newVal === null) for the inner input: when name === "value" and newVal is null, clear input.value; when name === "placeholder" and newVal is null, clear input.placeholder; when name === "type" and newVal is null, reset input.type to "text"; keep the existing behaviour for "disabled" (toggling input.disabled based on presence). Update the branch logic in attributeChangedCallback (the function and the local input variable) to cover these null-removal cases so the inner <input> doesn't retain stale state.Build/src/primitives/image.ts (1)
41-48:⚠️ Potential issue | 🟡 MinorSkip rendering the
<img>whensrcis empty.Line 47 still mounts
<img src="">. That can resolve to the current document URL, causing a broken-image flash and an unnecessary fetch. Returning an empty template until a source exists avoids the edge case.Possible fix
this.mount( STYLES, - `<img src="${escapeHtml(src)}" alt="${escapeHtml(alt)}" />`, + src ? `<img src="${escapeHtml(src)}" alt="${escapeHtml(alt)}" />` : "", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/image.ts` around lines 41 - 48, The render() method currently mounts an <img> even when src is empty, causing a broken-image fetch; modify render() (the function named render and the call to this.mount with STYLES) to early-return or mount an empty template when src is falsy/empty (i.e., check the computed src from getAttribute("src") || textContent before calling this.mount) so the <img> element is only created when a non-empty src exists.Build/src/primitives/slider.ts (1)
107-107:⚠️ Potential issue | 🟠 MajorRebind the current slider element instead of using a one-shot flag.
this.mount()replaces the internal<input>on every render, but Line 149 prevents rebinding after the first pass. After any observed attribute change—and even during initial upgrade when Line 175 can run before connection—the new slider stops updatingfilled,this.value, and the emittedsaz-inputevent.Possible fix
- private _sliderHandlerAdded = false; + private _sliderInputHandlerId?: number; @@ - if (slider && !this._sliderHandlerAdded) { - this._sliderHandlerAdded = true; - this.addHandler( + if (slider) { + if (this._sliderInputHandlerId !== undefined) { + this.removeHandler(this._sliderInputHandlerId); + } + this._sliderInputHandlerId = this.addHandler( "input", () => { const val = parseFloat(slider.value); const pct = range !== 0 ? ((val - min) / range) * 100 : 0; filled.style.width = `${pct}%`; this.value = val; this.dispatchEventTyped("input", { value: val }); }, { internal: true, element: slider }, ); }Also applies to: 149-160, 169-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` at line 107, The one-shot boolean _sliderHandlerAdded prevents rebinding when mount() replaces the internal <input>, causing filled, this.value and the saz-input event to stop updating; replace the flag logic with an element-aware binding strategy: track the specific input element (e.g., this._sliderElement) and on mount() always compare the current input to the tracked element, remove listeners from the old element if present, attach listeners to the new input, and update the tracked reference; update the handler attachment/removal code paths where _sliderHandlerAdded is read/set so events that update filled, this.value and emit saz-input are always attached to the live input element.Build/src/primitives/base.ts (1)
142-145:⚠️ Potential issue | 🟡 MinorWrap the cleanup callback body to satisfy Biome.
Line 144 uses an expression-bodied
forEachcallback, which triggerslint/suspicious/useIterableCallbackReturn. A block body (orfor...of) avoids the lint error.Possible fix
- this._cleanupFns.forEach((fn) => fn()); + this._cleanupFns.forEach((fn) => { + fn(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` around lines 142 - 145, The expression-bodied arrow used in disconnectedCallback's cleanup (_cleanupFns.forEach((fn) => fn())) triggers the lint rule; change it to a block-bodied callback or a for...of loop so the callback has a statement body—e.g., update the cleanup step in disconnectedCallback (which calls removeAllHandlers and iterates _cleanupFns) to use a block body like _cleanupFns.forEach((fn) => { fn(); }) or iterate with for (const fn of this._cleanupFns) { fn(); } and keep the subsequent this._cleanupFns = [].Build/src/primitives/radio.ts (1)
105-116:⚠️ Potential issue | 🟠 MajorUncheck siblings by element identity, not by
value.The condition at Line 112 (
radio.getAttribute("value") !== value) skips radios that share the same value as the clicked radio. If two radios in a group have identical values, both can remain checked. Compare element identity instead.Suggested fix
root.querySelectorAll("saz-radio").forEach((el) => { const radio = el as SazamiRadio; if ( radio.hasAttribute("name") && - radio.getAttribute("name") === name && - radio.getAttribute("value") !== value + radio.getAttribute("name") === name && + radio !== this ) { radio.removeAttribute("checked"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/radio.ts` around lines 105 - 116, The sibling-uncheck logic in radio.ts currently compares radio values and can leave two elements checked if they share the same value; update the loop that queries "saz-radio" (inside the method using this.getRootNode()) to uncheck radios by identity—i.e., skip only the clicked element itself (compare the iterated radio element to the source/target element reference) rather than comparing radio.getAttribute("value") to value, and call radio.removeAttribute("checked") for all other radios in the same name group (use the SazamiRadio element reference as the identity to exclude).Build/src/primitives/tabs.ts (1)
96-104:⚠️ Potential issue | 🟠 MajorDon't rebuild panels from
panel.innerHTML.This reparses each light-DOM panel into a brand-new HTML string on every render. Nested listeners, element state, and custom elements are recreated, and any raw panel markup is fed back into
mount(). Consider keeping the original panel nodes and toggling visibility instead of copyinginnerHTML.Alternative approach
Instead of serializing
panel.innerHTMLinto the template, you could:
- Mount only the tab buttons and an empty panels container
- Move/append the original light-DOM panel elements into the shadow DOM panels container
- Toggle
displayor a class on the original elementsThis preserves event listeners, element state, and avoids re-parsing HTML.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/tabs.ts` around lines 96 - 104, The current render is rebuilding panels from panel.innerHTML which reparses and recreates elements; instead, stop serializing panel.innerHTML in the template and instead render only the tab buttons and an empty panels container, then move/append the original light-DOM nodes (the existing panels variable) into that container inside mount/connectedCallback so their listeners/state are preserved; update the logic that sets "activeTab" (and the panel ids like panel-${i} / aria-labelledby="tab-${i}") to toggle a class or style on the actual moved nodes rather than injecting new HTML, and remove usage of panel.innerHTML in the code that generates the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/base.ts`:
- Around line 323-334: The property accessor layer is only being installed for
cfg.reflect === true in _installPropertyReflectors, so non-reflected properties
never get runtime accessors/defaults; change _installPropertyReflectors to
iterate all entries in sazamiConfig.properties and call _createReflector(prop,
cfg.type, cfg.default, cfg.reflect) (or call _createReflector for every prop and
let the reflector internally check cfg.reflect before touching attributes) so
every configured property gets an accessor and default handling while the
reflector implementation still only updates attributes when cfg.reflect is true;
ensure _createReflector/_createPropertyReflector respects the reflect flag and
still applies defaults and update hooks for non-reflected props.
In `@Build/src/primitives/icon-button.ts`:
- Around line 121-133: The attributeChangedCallback currently unconditionally
removes aria-label when name is "icon", "size", or "variant", overwriting
author-provided accessible names; change it so only auto-generated labels are
replaced: detect whether the current aria-label was produced by this component
(e.g., by checking a marker attribute or a stored flag) before removing or
regenerating it in attributeChangedCallback (function attributeChangedCallback
and methods render/_updateTabIndex), and do not touch aria-label at all when the
change is only "size" or "variant"; only regenerate the label on an "icon"
change if the existing aria-label is the component-generated one.
In `@Build/src/primitives/section.ts`:
- Around line 40-46: The slotchange listener is only added once in
connectedCallback so when the shadow <slot> is recreated later the listener is
lost; refactor to always (re)attach the listener whenever the slot element
exists and to remove the previous listener first: extract the logic into a small
helper like _attachSlotListener that finds the slot, calls
slot.removeEventListener("slotchange", this._boundComputeAndSetCenter) if a
prior slot reference exists, then adds slot.addEventListener("slotchange",
this._boundComputeAndSetCenter) and stores the current slot reference; call this
helper from connectedCallback and from the code that recreates the <slot> (the
block that recreates the shadow slot / inline template) and implement
disconnectedCallback to remove the listener and clear the stored slot reference.
---
Outside diff comments:
In `@Build/src/primitives/accordion.ts`:
- Around line 81-108: The render() method currently serializes light-DOM content
via el.innerHTML into items and interpolates it into the template (see render,
items, mount, STYLES, escapeHtml), which causes XSS risk and destroys original
DOM/event state; instead, build the accordion markup with empty content
containers (mount the header and an empty .inner-content for each item), then
move each original child element into its corresponding content container using
DOM operations (appendChild/replaceChild) so the original nodes and listeners
are preserved, toggle open state via attributes/classes and aria-expanded, and
keep escapeHtml only for titles while removing any direct innerHTML
interpolation.
---
Duplicate comments:
In `@Build/src/primitives/base.ts`:
- Around line 142-145: The expression-bodied arrow used in
disconnectedCallback's cleanup (_cleanupFns.forEach((fn) => fn())) triggers the
lint rule; change it to a block-bodied callback or a for...of loop so the
callback has a statement body—e.g., update the cleanup step in
disconnectedCallback (which calls removeAllHandlers and iterates _cleanupFns) to
use a block body like _cleanupFns.forEach((fn) => { fn(); }) or iterate with for
(const fn of this._cleanupFns) { fn(); } and keep the subsequent
this._cleanupFns = [].
In `@Build/src/primitives/image.ts`:
- Around line 41-48: The render() method currently mounts an <img> even when src
is empty, causing a broken-image fetch; modify render() (the function named
render and the call to this.mount with STYLES) to early-return or mount an empty
template when src is falsy/empty (i.e., check the computed src from
getAttribute("src") || textContent before calling this.mount) so the <img>
element is only created when a non-empty src exists.
In `@Build/src/primitives/input.ts`:
- Around line 87-105: In attributeChangedCallback, handle attribute removals
(newVal === null) for the inner input: when name === "value" and newVal is null,
clear input.value; when name === "placeholder" and newVal is null, clear
input.placeholder; when name === "type" and newVal is null, reset input.type to
"text"; keep the existing behaviour for "disabled" (toggling input.disabled
based on presence). Update the branch logic in attributeChangedCallback (the
function and the local input variable) to cover these null-removal cases so the
inner <input> doesn't retain stale state.
In `@Build/src/primitives/radio.ts`:
- Around line 105-116: The sibling-uncheck logic in radio.ts currently compares
radio values and can leave two elements checked if they share the same value;
update the loop that queries "saz-radio" (inside the method using
this.getRootNode()) to uncheck radios by identity—i.e., skip only the clicked
element itself (compare the iterated radio element to the source/target element
reference) rather than comparing radio.getAttribute("value") to value, and call
radio.removeAttribute("checked") for all other radios in the same name group
(use the SazamiRadio element reference as the identity to exclude).
In `@Build/src/primitives/slider.ts`:
- Line 107: The one-shot boolean _sliderHandlerAdded prevents rebinding when
mount() replaces the internal <input>, causing filled, this.value and the
saz-input event to stop updating; replace the flag logic with an element-aware
binding strategy: track the specific input element (e.g., this._sliderElement)
and on mount() always compare the current input to the tracked element, remove
listeners from the old element if present, attach listeners to the new input,
and update the tracked reference; update the handler attachment/removal code
paths where _sliderHandlerAdded is read/set so events that update filled,
this.value and emit saz-input are always attached to the live input element.
In `@Build/src/primitives/tabs.ts`:
- Around line 96-104: The current render is rebuilding panels from
panel.innerHTML which reparses and recreates elements; instead, stop serializing
panel.innerHTML in the template and instead render only the tab buttons and an
empty panels container, then move/append the original light-DOM nodes (the
existing panels variable) into that container inside mount/connectedCallback so
their listeners/state are preserved; update the logic that sets "activeTab" (and
the panel ids like panel-${i} / aria-labelledby="tab-${i}") to toggle a class or
style on the actual moved nodes rather than injecting new HTML, and remove usage
of panel.innerHTML in the code that generates the template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ceead9c2-02f7-40fc-b0b0-d33cf03be43b
📒 Files selected for processing (19)
Build/src/escape.tsBuild/src/primitives/accordion.tsBuild/src/primitives/base.tsBuild/src/primitives/chip.tsBuild/src/primitives/coverart.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/modal.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/switch.tsBuild/src/primitives/tabs.tsBuild/src/primitives/toast.tsDocs/docs/api-reference.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Build/src/primitives/progress.ts
- Build/src/escape.ts
- Build/src/primitives/coverart.ts
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
Build/src/primitives/icon-button.ts (1)
9-9: Unused import:escapeHtmlis no longer used.The XSS fix now uses
textContent(line 92) instead ofescapeHtml. This import can be removed.🧹 Proposed cleanup
-import { escapeHtml } from "../escape";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon-button.ts` at line 9, Remove the now-unused import of escapeHtml from Build/src/primitives/icon-button.ts: delete the import specifier "escapeHtml" from the top-level import statement (the one importing from "../escape") and ensure there are no remaining references to escapeHtml in the file (the component uses element.textContent for XSS protection already).Build/src/primitives/base.ts (2)
1-13: Import statement placed after interface definition.The interface
SazamiComponentConfigis declared before its dependencies are imported (lines 8-13). While this works in TypeScript due to hoisting, it's unconventional and reduces readability.♻️ Suggested reordering
+import { + propertyError, + eventError, + bindingError, + renderError, +} from "../errors"; + export interface SazamiComponentConfig { observedAttributes?: readonly string[] | string[]; properties?: Record<string, PropertyConfig>; events?: Record<string, EventConfig>; binds?: Record<string, BindingType>; } - -import { - propertyError, - eventError, - bindingError, - renderError, -} from "../errors";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` around lines 1 - 13, The import block is declared after the SazamiComponentConfig interface which is unconventional; move the import statements (importing propertyError, eventError, bindingError, renderError from "../errors") above the SazamiComponentConfig declaration so the file starts with the imports and then declares the interface SazamiComponentConfig (which references PropertyConfig/EventConfig/BindingType in its fields) to restore conventional ordering and improve readability.
167-175: Consider documenting XSS responsibility formount()callers.The
mount()method directly assigns toinnerHTML. While styles and templates are typically component-authored, subclasses interpolating user-provided data (attributes, text content) must escape them. The framework providesescapeHtml()for this purpose.Static analysis tools flag this pattern. Consider adding a JSDoc comment clarifying that callers must sanitize user input before interpolation.
📝 Suggested documentation
+ /** + * Mount template and styles into shadow DOM. + * `@warning` Callers must escape user-provided data using `escapeHtml()` + * before interpolating into the template to prevent XSS. + */ protected mount(styles: string, template: string) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` around lines 167 - 175, The mount() method assigns unsanitized HTML to this.shadow.innerHTML which can create XSS risks; add a JSDoc comment above the protected mount(styles: string, template: string) method stating that callers/subclasses are responsible for escaping or sanitizing any user-provided data before interpolating into styles or template (and reference the framework's escapeHtml() helper as the recommended sanitizer); do not change runtime behavior—only add the documentation to clarify responsibility for callers like subclasses that build template strings.Build/src/primitives/slider.ts (1)
107-164: Handler management approach is correct but fragile.Resetting
_sliderHandlerAdded = falseat render start (line 110) works becausemount()destroys the old slider element. However, this pattern is fragile - if the mount strategy changes to update rather than replace, handlers would accumulate.Consider documenting this assumption or using the base class's
removeAllHandlers({ type: "input" })before adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 107 - 164, The render method relies on resetting _sliderHandlerAdded at the top to avoid duplicate listeners when mount() replaces the element; this is fragile if mount() becomes non-destructive. Instead, before adding the new input listener in render(), call the base removal utility (e.g., removeAllHandlers or removeAllHandlers({ type: "input", element: slider })) to clear any existing "input" handlers bound to the slider element, then add the new handler with addHandler; keep the _sliderHandlerAdded flag or drop it after ensuring handlers are explicitly removed. Ensure you reference the same symbols: render, mount, _sliderHandlerAdded, addHandler, and removeAllHandlers when making the change.Build/src/primitives/radio.ts (2)
80-85: Event handlers added on everyrender()call.If
render()is called multiple times (e.g., by manually calling it or ifsuper.attributeChangedCallback()is added), handlers forclickandkeydownwill accumulate. Consider guarding or tracking handler IDs.♻️ Proposed fix: guard handler addition
+ private _handlersAdded = false; + render() { const label = this.textContent?.trim() || ""; this.mount( STYLES, ` <div class="radio"><div class="dot"></div></div> ${label ? `<span class="label">${escapeHtml(label)}</span>` : ""} `, ); if (!this.hasAttribute("role")) this.setAttribute("role", "radio"); this._updateAria(); - this.addHandler("click", this._handleClick, { internal: true }); - this.addHandler("keydown", this._handleKeydown, { internal: true }); + if (!this._handlersAdded) { + this._handlersAdded = true; + this.addHandler("click", this._handleClick, { internal: true }); + this.addHandler("keydown", this._handleKeydown, { internal: true }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/radio.ts` around lines 80 - 85, The render() method currently calls this.addHandler("click", this._handleClick, ...) and this.addHandler("keydown", this._handleKeydown, ...) every time, which accumulates handlers on repeated renders; modify render() to guard against duplicate installs by tracking the handler IDs or a boolean flag (e.g., this._handlersInstalled or this._clickHandlerId/this._keydownHandlerId), only call addHandler when not already installed, and if you store IDs ensure you remove existing handlers before re-adding (using the component's removeHandler/unsubscribe API if available) so _handleClick and _handleKeydown are attached exactly once.
132-141: Missingsuper.attributeChangedCallback()call.The override doesn't call
super.attributeChangedCallback(). While_updateAria()handles the ARIA state correctly, the base class's callback triggers a re-render when attributes change. This means changes tocheckedordisabledattributes won't re-render the component (updating visual dot state).However, since the visual state is CSS-driven via
:host([checked])(line 34-40), this may work correctly. Verify if re-render is needed for these attributes.♻️ Proposed fix to call super
attributeChangedCallback( name: string, oldVal: string | null, newVal: string | null, ) { + super.attributeChangedCallback(name, oldVal, newVal); if (oldVal === newVal) return; if (name === "checked" || name === "disabled") { this._updateAria(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/radio.ts` around lines 132 - 141, The attributeChangedCallback override in the Radio component omits calling the base implementation; add a call to super.attributeChangedCallback(name, oldVal, newVal) (preferably before invoking this._updateAria()) inside attributeChangedCallback so the base class can perform its attribute handling/re-rendering while preserving the existing this._updateAria() call for ARIA updates.Build/src/primitives/select.ts (2)
73-80: Selected option loses visual distinction when hovering other options.Both
.option.selectedand.option:hoverapply identical styling. When the user hovers over a non-selected option, the currently selected option loses its visual highlight, which can make it unclear which option is actually selected.Consider differentiating them:
♻️ Suggested styling differentiation
.option:hover { background: var(--saz-color-surface); } -.option.selected, -.option:hover { +.option.selected { background: var(--saz-color-primary); color: var(--saz-color-on-primary); } +.option.selected:hover { + background: var(--saz-color-primary); + color: var(--saz-color-on-primary); + filter: brightness(1.1); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 73 - 80, The current CSS gives identical rules to .option.selected and .option:hover so hovering any option overrides the selected visual; change the selectors to preserve the selected style by making hover specific to non-selected options (e.g., use .option:hover:not(.selected)) or add a more specific rule for .option.selected:hover to keep the selected background/color; update the selectors around .option.selected and .option:hover in Build/src/primitives/select.ts so the selected state remains visually distinct when other options are hovered.
183-191: Redundant display updates.Since
valuehasreflect: true, settingthis.value(line 187) triggersattributeChangedCallback, which already calls_updateDisplay()and_updateSelectedState()(lines 263-264). The explicit_updateDisplay()call at line 190 is redundant.Same applies in
_navigateOptionat line 211.♻️ Remove redundant calls
const handleDropdownClick = (e: Event) => { const target = e.target as HTMLElement; if (target.classList.contains("option")) { const newValue = target.getAttribute("data-value") || ""; this.value = newValue; this.open = false; this.dispatchEventTyped("change", { value: newValue }); - this._updateDisplay(); } };private _navigateOption(delta: number) { if (!this._options || this._options.length === 0) return; const currentIndex = this._options.findIndex((o) => o.value === this.value); let newIndex = currentIndex + delta; if (newIndex < 0) newIndex = this._options.length - 1; if (newIndex >= this._options.length) newIndex = 0; this.value = this._options[newIndex].value; - this._updateSelectedState(); this.dispatchEventTyped("change", { value: this.value }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/select.ts` around lines 183 - 191, The explicit calls to _updateDisplay() after assigning this.value in handleDropdownClick and _navigateOption are redundant because value is defined with reflect: true and its attributeChangedCallback already invokes _updateDisplay() and _updateSelectedState(); remove the redundant this._updateDisplay() calls from the handleDropdownClick and _navigateOption implementations so the update flow relies on the attributeChangedCallback logic (leave the this.value assignment, dispatchEventTyped("change", ...) and this.open changes intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/base.ts`:
- Around line 357-370: The setter for non-boolean properties in set(...)
currently only updates the DOM when reflect is true, causing assignments like
this.prop = "x" to be lost because the getter reads from getAttribute(); fix by
adding per-instance internal storage for property values (e.g., a backing Map or
object on the element instance) and update set(attr) to store String(value) into
that storage when reflect is false; also adjust the corresponding getter (which
reads getAttribute()) to return the internal stored value when
getAttribute(attr) is null/undefined so non-reflected string/number properties
persist.
In `@Build/src/primitives/icon-button.ts`:
- Around line 75-108: The issue is that _handlersAdded is never reset after
handlers are removed, so after disconnectedCallback calls removeAllHandlers()
the component won't re-register handlers on subsequent render(); fix by
overriding disconnectedCallback (or extending the existing one) to call the
base/disconnected logic and then set this._handlersAdded = false so render()
will add handlers again; touch the class containing render(), modify
disconnectedCallback to call removeAllHandlers() if not already done (or call
super.disconnectedCallback()), then reset the _handlersAdded flag so
addHandler("click", this._handleClick, ...) and addHandler("keydown",
this._handleKeydown, ...) in render() will run after reconnect.
In `@Build/src/primitives/image.ts`:
- Around line 25-32: The imageConfig's size and shape properties are configured
with reflect: false so programmatic updates won't trigger CSS attribute
selectors (e.g., :host([size="..."]) and SHAPE_RULES); update imageConfig (the
properties block for src, alt, size, shape) to set reflect: true for size and
shape (leave src and alt as-is) so changes to the size and shape attributes
reflect to the DOM and activate the CSS rules, mirroring the behavior used in
section.ts.
In `@Build/src/primitives/section.ts`:
- Around line 98-100: The static getter observedAttributes currently only
returns "center-point" so attribute changes to layout, align, and gap won't
trigger attributeChangedCallback; update the Section component by adding
"layout", "align", and "gap" to the array returned by static get
observedAttributes (or alternatively make those properties reflect: true in
their property declarations), ensuring attribute changes on the element invoke
attributeChangedCallback and cause a re-render; look for observedAttributes and
attribute/property definitions in the Section class to apply the change.
- Around line 14-21: The CSS :host([gap="..."]) selectors in GAP_RULES won't
match because the sectionConfig property descriptors (layout, align, gap) have
reflect: false; update the property descriptors for "gap", "layout", and "align"
in sectionConfig to reflect: true so setting this.gap/this.layout/this.align
programmatically updates the corresponding attributes and allows the CSS
attribute selectors to apply; leave "center-point" reflection unchanged unless
you also rely on an attribute selector for it.
In `@Build/src/primitives/select.ts`:
- Around line 229-232: Replace the non-null assertion on the querySelector in
the select component to avoid possible runtime throws: instead of using
trigger.querySelector(".value")! use a safe access pattern (e.g., store
trigger.querySelector(".value") in a variable and check for null, or use
optional chaining like trigger.querySelector(".value")?.textContent) so that the
assignment to .textContent only runs when the element exists; update the code
around the trigger, selectedOption and placeholder usage in the select logic to
use this safe access.
In `@Build/src/primitives/slider.ts`:
- Around line 179-187: The attributeChangedCallback implementation is causing
render() to run twice because super.attributeChangedCallback(name, oldVal,
newVal) already triggers this.render() when _rendered is true; remove the
redundant this.render() call at the end of attributeChangedCallback in
Build/src/primitives/slider.ts, or if you need custom attribute handling, avoid
calling super.attributeChangedCallback inside attributeChangedCallback and call
this.render() exactly once after your custom logic (referencing
attributeChangedCallback, super.attributeChangedCallback, and render).
In `@Build/src/primitives/spinner.ts`:
- Around line 45-51: The spinnerConfig's property metadata currently sets
reflect: false for CSS-driven attributes, so the :host([size="..."]) and
:host([variant="..."]) selectors won't match; update spinnerConfig -> properties
to set reflect: true for both size and variant (leave label as-is) so the size
and variant properties are reflected to attributes and the CSS attribute
selectors in the component will work correctly.
---
Nitpick comments:
In `@Build/src/primitives/base.ts`:
- Around line 1-13: The import block is declared after the SazamiComponentConfig
interface which is unconventional; move the import statements (importing
propertyError, eventError, bindingError, renderError from "../errors") above the
SazamiComponentConfig declaration so the file starts with the imports and then
declares the interface SazamiComponentConfig (which references
PropertyConfig/EventConfig/BindingType in its fields) to restore conventional
ordering and improve readability.
- Around line 167-175: The mount() method assigns unsanitized HTML to
this.shadow.innerHTML which can create XSS risks; add a JSDoc comment above the
protected mount(styles: string, template: string) method stating that
callers/subclasses are responsible for escaping or sanitizing any user-provided
data before interpolating into styles or template (and reference the framework's
escapeHtml() helper as the recommended sanitizer); do not change runtime
behavior—only add the documentation to clarify responsibility for callers like
subclasses that build template strings.
In `@Build/src/primitives/icon-button.ts`:
- Line 9: Remove the now-unused import of escapeHtml from
Build/src/primitives/icon-button.ts: delete the import specifier "escapeHtml"
from the top-level import statement (the one importing from "../escape") and
ensure there are no remaining references to escapeHtml in the file (the
component uses element.textContent for XSS protection already).
In `@Build/src/primitives/radio.ts`:
- Around line 80-85: The render() method currently calls
this.addHandler("click", this._handleClick, ...) and this.addHandler("keydown",
this._handleKeydown, ...) every time, which accumulates handlers on repeated
renders; modify render() to guard against duplicate installs by tracking the
handler IDs or a boolean flag (e.g., this._handlersInstalled or
this._clickHandlerId/this._keydownHandlerId), only call addHandler when not
already installed, and if you store IDs ensure you remove existing handlers
before re-adding (using the component's removeHandler/unsubscribe API if
available) so _handleClick and _handleKeydown are attached exactly once.
- Around line 132-141: The attributeChangedCallback override in the Radio
component omits calling the base implementation; add a call to
super.attributeChangedCallback(name, oldVal, newVal) (preferably before invoking
this._updateAria()) inside attributeChangedCallback so the base class can
perform its attribute handling/re-rendering while preserving the existing
this._updateAria() call for ARIA updates.
In `@Build/src/primitives/select.ts`:
- Around line 73-80: The current CSS gives identical rules to .option.selected
and .option:hover so hovering any option overrides the selected visual; change
the selectors to preserve the selected style by making hover specific to
non-selected options (e.g., use .option:hover:not(.selected)) or add a more
specific rule for .option.selected:hover to keep the selected background/color;
update the selectors around .option.selected and .option:hover in
Build/src/primitives/select.ts so the selected state remains visually distinct
when other options are hovered.
- Around line 183-191: The explicit calls to _updateDisplay() after assigning
this.value in handleDropdownClick and _navigateOption are redundant because
value is defined with reflect: true and its attributeChangedCallback already
invokes _updateDisplay() and _updateSelectedState(); remove the redundant
this._updateDisplay() calls from the handleDropdownClick and _navigateOption
implementations so the update flow relies on the attributeChangedCallback logic
(leave the this.value assignment, dispatchEventTyped("change", ...) and
this.open changes intact).
In `@Build/src/primitives/slider.ts`:
- Around line 107-164: The render method relies on resetting _sliderHandlerAdded
at the top to avoid duplicate listeners when mount() replaces the element; this
is fragile if mount() becomes non-destructive. Instead, before adding the new
input listener in render(), call the base removal utility (e.g.,
removeAllHandlers or removeAllHandlers({ type: "input", element: slider })) to
clear any existing "input" handlers bound to the slider element, then add the
new handler with addHandler; keep the _sliderHandlerAdded flag or drop it after
ensuring handlers are explicitly removed. Ensure you reference the same symbols:
render, mount, _sliderHandlerAdded, addHandler, and removeAllHandlers when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69130794-01bf-4df1-b8d9-1d102f0fe61b
📒 Files selected for processing (11)
Build/src/primitives/accordion.tsBuild/src/primitives/base.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/radio.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/tabs.ts
added 2 commits
March 15, 2026 02:33
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Build/src/primitives/slider.ts (1)
163-173: SimplifyobservedAttributesto avoid duplicate declarations.With
@component(sliderConfig), these attributes are already derived bysuper.observedAttributes. Re-listing them here creates duplication and drift risk.💡 Proposed cleanup
static get observedAttributes() { - return [ - ...super.observedAttributes, - "value", - "min", - "max", - "step", - "disabled", - "size", - ]; + return super.observedAttributes; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 163 - 173, The observedAttributes getter in the Slider component redundantly re-lists attributes already provided by super.observedAttributes (due to `@component`(sliderConfig)), so remove the duplicate array and simply return super.observedAttributes; update the static get observedAttributes() method in the Slider class to delegate entirely to super.observedAttributes to avoid drift and duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/base.ts`:
- Around line 136-149: The disconnect flow tears down handlers and cleanup
functions but leaves the _rendered flag true so connectedCallback will skip
re-rendering; update disconnectedCallback (alongside removeAllHandlers and the
loop over _cleanupFns) to reset this._rendered = false after cleanup so that
connectedCallback will call render() and reattach handlers on reconnect.
In `@Build/src/primitives/image.ts`:
- Around line 25-52: The component currently defines src and alt with reflect:
false in imageConfig and reads them via this.getAttribute(...) inside
SazamiImage.render(), so attribute changes after mount don’t trigger re-render;
to fix, either set src and alt to reflect: true in imageConfig or implement
attribute change observation in SazamiImage (e.g., add
observedAttributes/attributeChangedCallback or the framework-equivalent) to call
this.render()/this.mount(STYLES, ...) when src/alt change; update references in
imageConfig, SazamiImage, render(), and any attribute-reading logic using
getAttribute(...) to ensure runtime updates refresh the <img>.
In `@Build/src/primitives/slider.ts`:
- Around line 108-127: The parsed slider numeric attributes (min, max, value,
step) can become NaN and produce invalid CSS percentages; update the parsing
logic in the slider initialization (where min, max, value, step are computed and
percent/range are derived) to validate each parsed number (use Number.isFinite
or isNaN checks) and fall back to safe defaults (e.g., min=0, max=100,
value=clamped default, step=1) if parsing fails; clamp value to the [min, max]
range before computing range and percent, compute percent as range !== 0 ?
((value - min) / range) * 100 : 0 and then clamp percent to [0,100] to avoid
"NaN%" or out-of-bounds widths; apply the same validation/clamping fixes to the
other occurrence that computes percent/range around the code referenced at lines
~151-156 (the same variables: min, max, value, step, range, percent).
In `@Build/src/primitives/spinner.ts`:
- Around line 51-67: The component's label is non-reactive because it's not
reflected/observed and render reads attributes directly; update spinnerConfig to
make label reflect: true (change properties.label to { type: "string" as const,
reflect: true }), then in SazamiSpinner.render() read the reactive property (use
this.label with fallback like const label = this.label || "Loading...") and base
hasCustomLabel on the property (e.g. const hasCustomLabel = this.label !==
undefined && this.label !== null) instead of getAttribute/hasAttribute so
updates via setAttribute or property assignment trigger rerenders.
---
Nitpick comments:
In `@Build/src/primitives/slider.ts`:
- Around line 163-173: The observedAttributes getter in the Slider component
redundantly re-lists attributes already provided by super.observedAttributes
(due to `@component`(sliderConfig)), so remove the duplicate array and simply
return super.observedAttributes; update the static get observedAttributes()
method in the Slider class to delegate entirely to super.observedAttributes to
avoid drift and duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7752874f-1ac1-440b-b9f8-f5252bc1bcb0
📒 Files selected for processing (9)
Build/src/primitives/accordion.tsBuild/src/primitives/base.tsBuild/src/primitives/icon-button.tsBuild/src/primitives/image.tsBuild/src/primitives/radio.tsBuild/src/primitives/section.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.ts
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
Build/src/primitives/base.ts (1)
352-384:⚠️ Potential issue | 🟠 MajorPrefer
_propStorageover the initial attribute for non-reflected props.After a non-reflected property is assigned once, the getter still lets the original markup attribute win forever. That leaves APIs like
SazamiSlider.valuestuck on the initial HTML value whenever the element started withvalue="...".Suggested fix
get() { if (type === "boolean") { return this.hasAttribute(attr); } + if (!reflect && this._propStorage.has(attr)) { + return this._propStorage.get(attr); + } const raw = this.getAttribute(attr); if (raw !== null) { if (type === "number") { const val = parseFloat(raw); return !isNaN(val) ? val : (defaultValue ?? 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` around lines 352 - 384, The getter currently prefers the original attribute over stored values, so non-reflected properties (reflect === false) never update after initial markup; change the getter in the Object.defineProperty for prop to first check this._propStorage.has(attr) and return this._propStorage.get(attr) (with appropriate typing/number handling) before falling back to reading this.getAttribute(attr); keep boolean handling and the setter behavior that uses this._propStorage.set(attr, value) when reflect is false so runtime assignments win over initial attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/button.ts`:
- Around line 71-85: render is currently attaching a new internal keydown
listener every render and _handleKeydown always calls click() even when the host
is inert; before adding the handler in render call the counterpart removal
(e.g., this.removeHandler("keydown", this._handleKeydown)) to avoid duplicate
listeners, and update _handleKeydown to early-return when the element is
disabled or loading (check the reflected attributes/properties like
disabled/loading or hasAttribute("disabled")/hasAttribute("loading")) so
keyboard activation is idempotent and inert states are honored.
- Around line 56-58: buttonConfig declares a public event "saz-click" but the
component never emits it; locate the component's click handler (e.g., the
element's onClick/handleClick method or the place where DOM "click" is listened
for) and call dispatchEventTyped("click", { /* include desired detail payload or
empty object */ }) when the button is activated so that the declared saz-click
event is actually dispatched; reference the events property on buttonConfig and
the dispatchEventTyped function to add the emit, and ensure the detail shape
matches the declared type.
In `@Build/src/primitives/image.ts`:
- Around line 25-31: The image component currently declares src and alt in
imageConfig.properties with reflect: false, but the render path reads attributes
via getAttribute(...) so setting image.src = "..." or image.alt = "..." does not
trigger re-render; either mark src and alt as reflect: true in imageConfig or
update the component render/update logic to read from the reactive properties
(e.g., this.src / this.alt) and ensure property setters trigger repaint;
specifically change the imageConfig.properties entries for src and alt or modify
the render functions that call getAttribute(...) to use the reactive properties
instead (also apply the same change to the other occurrences around the image
component at the noted ranges).
In `@Build/src/primitives/select.ts`:
- Around line 94-99: The placeholder attribute is never reflected or observed so
updates to the host attribute don't change the UI; update selectConfig to
reflect the placeholder (set placeholder: { type: "string" as const, reflect:
true }) or add "placeholder" to the component's observed attributes and handle
it in attributeChangedCallback by updating the internal placeholder state and
re-rendering (ensure when value is empty/undefined the trigger text is
refreshed). Modify attributeChangedCallback to handle the 'placeholder' case and
call the same update/refresh logic used when selection changes so the trigger
text stays in sync.
In `@Build/src/primitives/slider.ts`:
- Around line 84-96: The component has no single source of truth: sliderConfig
marks value/min/max/step/size as non-reflected while render() reads
getAttribute(...) and observedAttributes only includes base attributes, so the
internal <input> can revert to markup values after user input; fix by choosing
one approach—either set reflect: true for the properties in sliderConfig (value,
min, max, step, size) so attribute and property stay in sync, or update
render(), the input binding, and observedAttributes to read and write
exclusively from the component properties (this.value, this.min, this.max,
this.step, this.size) and ensure attribute-to-property syncing by adding those
names to observedAttributes and handling attributeChangedCallback; reference
sliderConfig, render(), getAttribute(...), observedAttributes, and
attributeChangedCallback when making the change.
---
Duplicate comments:
In `@Build/src/primitives/base.ts`:
- Around line 352-384: The getter currently prefers the original attribute over
stored values, so non-reflected properties (reflect === false) never update
after initial markup; change the getter in the Object.defineProperty for prop to
first check this._propStorage.has(attr) and return this._propStorage.get(attr)
(with appropriate typing/number handling) before falling back to reading
this.getAttribute(attr); keep boolean handling and the setter behavior that uses
this._propStorage.set(attr, value) when reflect is false so runtime assignments
win over initial attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dab8cda-7d71-4122-acab-2c5db90e4d7d
📒 Files selected for processing (7)
Build/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/image.tsBuild/src/primitives/select.tsBuild/src/primitives/shared.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.ts
added 2 commits
March 15, 2026 04:01
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
Build/src/primitives/image.ts (1)
55-57:observedAttributesoverride is redundant here.Because all four properties are reflected,
super.observedAttributesalready includes them; appending"src"/"alt"adds duplication without extra behavior.🧹 Simplification
- static get observedAttributes() { - return [...super.observedAttributes, "src", "alt"]; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/image.ts` around lines 55 - 57, The observedAttributes override in the Image element is redundant and duplicates attributes already provided by the base class; remove the static get observedAttributes() override (or return super.observedAttributes only) in the Image class so you don't append "src"/"alt" unnecessarily—locate the static observedAttributes getter in Build/src/primitives/image.ts and delete the override or change it to simply return super.observedAttributes to eliminate duplication.Build/package.json (1)
32-32: Makebuild:democross-platform.Using
rm -rf+cp -rwill fail on non-POSIX environments. Consider a cross-platform script (Node script or portable CLI tools).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/package.json` at line 32, The "build:demo" npm script uses POSIX-only commands (rm -rf and cp -r); replace it with a cross-platform approach by either adding a Node cleanup/copy script (e.g., create a small JS file that deletes ../Demo/dist and copies dist using fs/promises) and updating "build:demo" to run that script after "vite build", or install cross-platform CLI packages (e.g., rimraf and cpy or cpx) and change "build:demo" to use those instead of rm and cp; ensure the package.json "build:demo" entry is updated to invoke the chosen solution so it works on Windows/macOS/Linux.Build/src/primitives/checkbox.ts (1)
122-127: Consider callingsuper.attributeChangedCallback()for consistency.The override handles
checkedanddisabledbut doesn't callsuper.attributeChangedCallback(). While this works for the current observed attributes, it deviates from the pattern used in other components (e.g.,radio.tsline 144,slider.tsline 193) and could cause issues if additional observed attributes are added later.♻️ Suggested improvement
attributeChangedCallback(name: string, oldVal: string, newVal: string) { + super.attributeChangedCallback(name, oldVal, newVal); if (oldVal === newVal) return; if (name === "checked" || name === "disabled") { this._updateAria(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/checkbox.ts` around lines 122 - 127, The attributeChangedCallback override in the Checkbox class doesn't call super.attributeChangedCallback, so update the attributeChangedCallback(name: string, oldVal: string, newVal: string) method to invoke super.attributeChangedCallback(name, oldVal, newVal) (either at the start or end of the method) in addition to the existing handling of "checked" and "disabled" to maintain consistency with Radio/Slider and ensure future observed attributes are processed by the base class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/package.json`:
- Around line 46-47: The package.json lists mismatched major versions for Jest
and its jsdom environment; update the dependency "jest-environment-jsdom" to the
Jest 29 major (e.g., change "^30.3.0" to a 29.x range) so that it aligns with
the existing "jest" v29 dependency, then run install to refresh lockfile; look
for the dependency string "jest-environment-jsdom" in package.json to make the
edit.
In `@Build/src/index.ts`:
- Around line 117-127: The call to enableCurvomorphism inside the
elements.forEach is leaking resize listeners because its returned disposer is
ignored; capture each disposer (the return value of enableCurvomorphism) into an
array when iterating elements and provide a single cleanup that iterates that
array and calls each disposer (and clears the array) before re-running or on
module teardown (e.g., before recompile/unmount). Update the block that
currently calls enableCurvomorphism in the elements.forEach to push each
returned disposer into a disposers array and ensure that the module exposes or
calls a cleanup function to run those disposers to remove the resize listeners
and drop stale element references.
In `@Build/src/primitives/button.ts`:
- Line 54: The tone property currently has reflect: false which causes
programmatic writes to Button.tone (e.g., button.tone = "dim") to not update the
host attribute and thus not trigger TYPO_TONE-driven CSS selectors; change the
property definition for tone in Build/src/primitives/button.ts to reflect the
value (set reflect: true) so the host attribute stays in sync with the property
and styling updates correctly (update the tone property descriptor where tone: {
type: "string" as const, reflect: false } is declared).
- Around line 74-90: Ensure the button exposes inert state accessibly by setting
aria-disabled and removing it from the tab order when disabled or loading:
update the component's state update path (where role/tabindex are initialized
and events attached) to set this.setAttribute("aria-disabled", "true") and
remove focusability (e.g., this.removeAttribute("tabindex") or set
tabindex="-1") whenever this.disabled || this.loading is true, and restore
aria-disabled to "false" and tabindex="0" (or remove tabindex override if the
host originally provided one) when not disabled; keep _handleClick and
_handleKeydown guards as-is but ensure they rely on aria-disabled/tabindex
changes to prevent keyboard/focus activation.
In `@Build/src/primitives/checkbox.ts`:
- Around line 92-94: render() is adding internal event handlers unconditionally
which causes handler accumulation on each re-render; before re-adding the click
and keydown handlers in render(), remove any previously-registered internal
handlers for this._handleClick and this._handleKeydown (use the class's
removeHandler or equivalent) or guard so addHandler runs only once (idempotent),
ensuring the handlers on the Checkbox component (methods _handleClick and
_handleKeydown) are not registered multiple times across
attributeChangedCallback-triggered renders.
In `@Build/src/primitives/toast.ts`:
- Around line 152-156: The render logic schedules a new hide timer without
clearing any existing one, causing overlapping timeouts; update the code in the
Toast class where this._hideTimeout is set (the render or show logic that calls
setTimeout(() => this.hide(), duration)) to first check and clear the existing
timer (clearTimeout(this._hideTimeout) and null/undefined it) before assigning a
new setTimeout, and ensure any hide() path also clears and resets
this._hideTimeout to avoid stale timers.
- Around line 135-150: The render() logic is adding duplicate listeners on
re-render; before calling addHandler for the close button and keydown you should
remove any prior handlers: keep refs like this._closeHandler and
this._keydownHandler (or use the component's removeHandler API) and call
removeHandler/remove the previous listener for the close button element and for
"keydown" before re-adding, then store the new handler refs so subsequent
renders can clean them up; update the block that currently creates closeBtn,
addHandler("click", …) and the handleKeydown/addHandler("keydown", …) to follow
this pattern.
---
Nitpick comments:
In `@Build/package.json`:
- Line 32: The "build:demo" npm script uses POSIX-only commands (rm -rf and cp
-r); replace it with a cross-platform approach by either adding a Node
cleanup/copy script (e.g., create a small JS file that deletes ../Demo/dist and
copies dist using fs/promises) and updating "build:demo" to run that script
after "vite build", or install cross-platform CLI packages (e.g., rimraf and cpy
or cpx) and change "build:demo" to use those instead of rm and cp; ensure the
package.json "build:demo" entry is updated to invoke the chosen solution so it
works on Windows/macOS/Linux.
In `@Build/src/primitives/checkbox.ts`:
- Around line 122-127: The attributeChangedCallback override in the Checkbox
class doesn't call super.attributeChangedCallback, so update the
attributeChangedCallback(name: string, oldVal: string, newVal: string) method to
invoke super.attributeChangedCallback(name, oldVal, newVal) (either at the start
or end of the method) in addition to the existing handling of "checked" and
"disabled" to maintain consistency with Radio/Slider and ensure future observed
attributes are processed by the base class.
In `@Build/src/primitives/image.ts`:
- Around line 55-57: The observedAttributes override in the Image element is
redundant and duplicates attributes already provided by the base class; remove
the static get observedAttributes() override (or return super.observedAttributes
only) in the Image class so you don't append "src"/"alt" unnecessarily—locate
the static observedAttributes getter in Build/src/primitives/image.ts and delete
the override or change it to simply return super.observedAttributes to eliminate
duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 220bcab5-231d-4d96-b0ac-dae7b91d86f6
📒 Files selected for processing (17)
BUGS.mdBuild/package.jsonBuild/src/curvomorphism/index.tsBuild/src/index.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/image.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/select.tsBuild/src/primitives/shared.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/toast.tsExamples/curvomorphic-player.sakoExamples/curvomorphism-test.sako
🚧 Files skipped from review as they are similar to previous changes (1)
- Build/src/primitives/progress.ts
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
Build/src/primitives/button.ts (1)
76-83:⚠️ Potential issue | 🟠 MajorRestore
tabindexcorrectly after inert state; current flow can leave enabled buttons non-focusable.
Line 79forcestabindex="-1"while inert, butLine 82only sets"0"whentabindexis missing. After re-enabling, the attribute often remains-1, so keyboard users can’t focus the button anymore.♿ Suggested fix
export class SazamiButton extends SazamiComponent<typeof buttonConfig> { declare disabled: boolean; declare loading: boolean; declare active: boolean; declare size: string; declare variant: string; declare shape: string; declare tone: string; + private _previousTabIndex: string | null | undefined; render() { this.mount(STYLES, `<slot></slot>`); if (!this.hasAttribute("role")) this.setAttribute("role", "button"); const isInert = this.disabled || this.loading; if (isInert) { this.setAttribute("aria-disabled", "true"); + if (this._previousTabIndex === undefined) { + this._previousTabIndex = this.getAttribute("tabindex"); + } this.setAttribute("tabindex", "-1"); } else { this.removeAttribute("aria-disabled"); - if (!this.hasAttribute("tabindex")) this.setAttribute("tabindex", "0"); + if (this._previousTabIndex !== undefined) { + if (this._previousTabIndex === null) this.removeAttribute("tabindex"); + else this.setAttribute("tabindex", this._previousTabIndex); + this._previousTabIndex = undefined; + } else if (!this.hasAttribute("tabindex")) { + this.setAttribute("tabindex", "0"); + } } this.removeHandler("click", this._handleClick);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/button.ts` around lines 76 - 83, The inert-handling logic in the component (computed via isInert = this.disabled || this.loading in the Button class/file) sets tabindex="-1" when inert but only restores tabindex="0" when the attribute is missing, which leaves re-enabled buttons non-focusable; update the non-inert branch in the same block so that when !isInert you either remove the tabindex attribute if it was absent originally or explicitly set tabindex="0" when its current value is "-1" (check this.getAttribute("tabindex") or this.hasAttribute("tabindex") to decide), ensuring enabled buttons become focusable again while preserving any explicitly provided tabindex values.Build/src/primitives/toast.ts (1)
146-153:⚠️ Potential issue | 🟠 MajorKeydown listener removal is ineffective due to function identity mismatch.
A new
handleKeydownfunction is created each render, soremoveHandler("keydown", handleKeydown)cannot remove the prior listener. Handlers accumulate across renders.💡 Proposed fix
export class SazamiToast extends SazamiComponent<typeof toastConfig> { @@ private _hideTimeout?: ReturnType<typeof setTimeout>; private _closeHandler = () => this.hide(); + private _keydownHandler = (e: KeyboardEvent) => { + if (e.key === "Escape") { + e.preventDefault(); + this.hide(); + } + }; @@ - // Keyboard support: Escape to dismiss - const handleKeydown = (e: KeyboardEvent) => { - if (e.key === "Escape") { - e.preventDefault(); - this.hide(); - } - }; - this.removeHandler("keydown", handleKeydown); - this.addHandler("keydown", handleKeydown, { internal: true }); + this.removeHandler("keydown", this._keydownHandler); + this.addHandler("keydown", this._keydownHandler, { internal: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` around lines 146 - 153, The keydown listener is recreated each render because handleKeydown is defined inline, so removeHandler("keydown", handleKeydown) can't match the previously added listener; hoist the handler to a stable reference (e.g., make a class method or an instance property like this.handleKeydown or _handleKeydown bound once in the constructor) and then call this.removeHandler("keydown", this.handleKeydown) and this.addHandler("keydown", this.handleKeydown, { internal: true }) so the same function identity is used for removal and addition.
🧹 Nitpick comments (4)
Build/src/primitives/image.ts (1)
42-43: Consider a single source of truth forsrc(optional cleanup).Using both
this.srcandthis.textContentcan cause surprising fallback behavior (e.g., clearingsrcmay still render from light DOM text). If text fallback isn’t required, simplify tothis.srconly.♻️ Optional simplification
- const src = this.src || this.textContent?.trim() || ""; + const src = this.src || "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/image.ts` around lines 42 - 43, The src fallback uses both this.src and this.textContent which can produce surprising behavior; change the logic to use a single source of truth by removing the textContent fallback and base src only on this.src (e.g., replace the expression that defines the local src variable so it reads from this.src only and adjust the subsequent check that uses src). Update any associated uses of the local src variable (the const src assignment and the following if (!src) branch) to rely solely on this.src to avoid light DOM text unexpectedly supplying image data.Build/package.json (1)
32-32: Consider extracting the inline script for maintainability.The embedded Node.js one-liner works but is dense and harder to debug. A separate script file (e.g.,
scripts/build-demo.js) would improve readability and error handling.♻️ Suggested refactor
Create
scripts/build-demo.js:const fs = require('fs'); const path = require('path'); const demoDist = path.resolve(__dirname, '..', '..', 'Demo', 'dist'); const buildDist = path.resolve(__dirname, '..', 'dist'); fs.rmSync(demoDist, { recursive: true, force: true }); fs.cpSync(buildDist, demoDist, { recursive: true }); console.log('Demo dist synced successfully');Then update
package.json:- "build:demo": "vite build && node -e \"const fs=require('fs');fs.rmSync('../Demo/dist',{recursive:true,force:true});fs.cpSync('dist','../Demo/dist',{recursive:true})\"", + "build:demo": "vite build && node scripts/build-demo.js",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/package.json` at line 32, Replace the dense inline Node one-liner in the "build:demo" script with a separate script file to improve readability and error handling: create a new scripts/build-demo.js that performs the rmSync/cpSync operations with path resolution and proper logging/error handling, then update the "build:demo" entry in package.json to run "vite build && node ./scripts/build-demo.js"; reference the existing "build:demo" script name and the new scripts/build-demo.js file when making the changes.Build/src/primitives/checkbox.ts (1)
124-130: Minor:_updateAria()is called twice on attribute change.When
checkedordisabledchanges:
super.attributeChangedCallback()callsrender(), which calls_updateAria()(line 90)- Then this override calls
_updateAria()again (line 128)This is harmless but redundant. Consider removing the explicit
_updateAria()call here sincerender()already handles it.♻️ Suggested simplification
attributeChangedCallback(name: string, oldVal: string, newVal: string) { super.attributeChangedCallback(name, oldVal, newVal); - if (oldVal === newVal) return; - if (name === "checked" || name === "disabled") { - this._updateAria(); - } }Or remove the override entirely if no additional logic is needed beyond what the base class provides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/checkbox.ts` around lines 124 - 130, The override of attributeChangedCallback duplicates _updateAria() because super.attributeChangedCallback(name, oldVal, newVal) triggers render(), which already calls _updateAria() in render(); remove the explicit call to this._updateAria() inside the attributeChangedCallback override (or remove the entire attributeChangedCallback override if it contains no other logic) so that _updateAria() is invoked only via render()—look for attributeChangedCallback, render, and _updateAria in the Checkbox class to apply the change.Build/tests/curvomorphism.test.ts (1)
82-87: Assert left-side corners explicitly in this case.Line 85 and Line 86 only check right-side corners. Add left-side
0pxassertions so this test fails if all corners accidentally round.🧪 Proposed tightening
test("element to the left of center, vertically centered: right-side corners round", () => { const el = mockElement(0, 200, 100, 100); applyCurvomorphism(el, 500, 250, "10px"); + expect(el.style.borderTopLeftRadius).toBe("0px"); expect(el.style.borderTopRightRadius).toBe("10px"); + expect(el.style.borderBottomLeftRadius).toBe("0px"); expect(el.style.borderBottomRightRadius).toBe("10px"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/curvomorphism.test.ts` around lines 82 - 87, The test "element to the left of center, vertically centered: right-side corners round" currently only asserts right-side radii; update it to also assert left-side radii are "0px" to prevent regressions that round all corners—after calling mockElement(...) and applyCurvomorphism(...), add expectations for el.style.borderTopLeftRadius and el.style.borderBottomLeftRadius toEqual "0px" so the test validates left corners remain square.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/package.json`:
- Around line 42-49: Update the ts-jest dependency in package.json to a
compatible version for Jest 30: change the "ts-jest" entry from "^29.1.0" to at
least "^29.4.0" (or a later 29.x/30.x compatible release) so its
peerDependencies include Jest 30; ensure you run install/lockfile update
afterwards to persist the new version.
In `@Build/src/index.ts`:
- Around line 104-107: The ResizeObserver callback currently disconnects and
deletes the observer (ro.disconnect() and delete (target as any).__sazamiRO),
causing it to only run once and preventing recomputation of bounds used by
enableCurvomorphism; remove the early disconnect/delete so the ResizeObserver
instance (ro) remains active and continues updating the shared center and group
coordinates on subsequent layout changes, ensuring the observer is stored on
target.__sazamiRO as before and only cleaned up when the element is truly
removed or during explicit teardown.
- Around line 99-101: The forEach callbacks currently use expression-bodied
arrows like prevDisposers.forEach((d) => d()) which implicitly return a value
and trigger the useIterableCallbackReturn rule; change these to block-bodied
callbacks (e.g., prevDisposers.forEach((d) => { d(); })) so the callback does
not return a value—apply the same change to any other forEach usages (e.g., the
one around line 124) that use expression-bodied arrows.
In `@Build/src/primitives/checkbox.ts`:
- Line 83: ICON_SVGS.check may be undefined at runtime causing the literal
"undefined" to render; update the checkbox render to use a safe fallback such as
ICON_SVGS.check || "" (or perform a null check before insertion) so the span
uses an empty string when the SVG isn't registered; specifically modify the
expression that injects ${ICON_SVGS.check} in the checkbox render to use the
fallback pattern used in modal.ts/select.ts or the null-check pattern used in
icon.ts.
In `@Build/src/primitives/toast.ts`:
- Line 96: The current parseInt(this.getAttribute("duration") || "3000") can
yield NaN and disable auto-dismiss; replace it with a numeric parse + fallback
so invalid values use the default (e.g. 3000). Locate the occurrences using
this.getAttribute("duration") (the `duration` variable at the top-level and the
similar code around lines 157-160) and change to something like: parseInt(...),
then if Number.isNaN(result) (or !Number.isFinite(result)) set duration = 3000
(or your intended default), ensuring the code treats 0 intentionally if you want
to allow disabling. Ensure both occurrences use the same validated fallback
logic so auto-hide is not silently disabled by invalid attributes.
- Around line 68-69: The reflective `visible` property is causing a render loop
because hide() removes the attribute (triggering render) while render() resets
it; to fix, stop removing the attribute and instead toggle the property directly
or remove reflection: update the Toast's property declaration (visible: { type:
"boolean", reflect: true }) by removing reflect: true, or modify the hide()
method to set this.visible = false (and avoid calling removeAttribute or
deleting the property) so the base class won't observe an attribute removal that
re-triggers render.
In `@Build/tests/curvomorphism.test.ts`:
- Around line 64-70: The test name is misleading: it says “no corners round” but
asserts all four corners equal "8px"; rename the test description string in the
test that calls mockElement(...) and applyCurvomorphism(...) to reflect the
actual behavior (e.g., "element exactly at center: all corners rounded
(dead-zone)" or similar) so the name matches the assertions referencing
applyCurvomorphism and mockElement in this test.
---
Duplicate comments:
In `@Build/src/primitives/button.ts`:
- Around line 76-83: The inert-handling logic in the component (computed via
isInert = this.disabled || this.loading in the Button class/file) sets
tabindex="-1" when inert but only restores tabindex="0" when the attribute is
missing, which leaves re-enabled buttons non-focusable; update the non-inert
branch in the same block so that when !isInert you either remove the tabindex
attribute if it was absent originally or explicitly set tabindex="0" when its
current value is "-1" (check this.getAttribute("tabindex") or
this.hasAttribute("tabindex") to decide), ensuring enabled buttons become
focusable again while preserving any explicitly provided tabindex values.
In `@Build/src/primitives/toast.ts`:
- Around line 146-153: The keydown listener is recreated each render because
handleKeydown is defined inline, so removeHandler("keydown", handleKeydown)
can't match the previously added listener; hoist the handler to a stable
reference (e.g., make a class method or an instance property like
this.handleKeydown or _handleKeydown bound once in the constructor) and then
call this.removeHandler("keydown", this.handleKeydown) and
this.addHandler("keydown", this.handleKeydown, { internal: true }) so the same
function identity is used for removal and addition.
---
Nitpick comments:
In `@Build/package.json`:
- Line 32: Replace the dense inline Node one-liner in the "build:demo" script
with a separate script file to improve readability and error handling: create a
new scripts/build-demo.js that performs the rmSync/cpSync operations with path
resolution and proper logging/error handling, then update the "build:demo" entry
in package.json to run "vite build && node ./scripts/build-demo.js"; reference
the existing "build:demo" script name and the new scripts/build-demo.js file
when making the changes.
In `@Build/src/primitives/checkbox.ts`:
- Around line 124-130: The override of attributeChangedCallback duplicates
_updateAria() because super.attributeChangedCallback(name, oldVal, newVal)
triggers render(), which already calls _updateAria() in render(); remove the
explicit call to this._updateAria() inside the attributeChangedCallback override
(or remove the entire attributeChangedCallback override if it contains no other
logic) so that _updateAria() is invoked only via render()—look for
attributeChangedCallback, render, and _updateAria in the Checkbox class to apply
the change.
In `@Build/src/primitives/image.ts`:
- Around line 42-43: The src fallback uses both this.src and this.textContent
which can produce surprising behavior; change the logic to use a single source
of truth by removing the textContent fallback and base src only on this.src
(e.g., replace the expression that defines the local src variable so it reads
from this.src only and adjust the subsequent check that uses src). Update any
associated uses of the local src variable (the const src assignment and the
following if (!src) branch) to rely solely on this.src to avoid light DOM text
unexpectedly supplying image data.
In `@Build/tests/curvomorphism.test.ts`:
- Around line 82-87: The test "element to the left of center, vertically
centered: right-side corners round" currently only asserts right-side radii;
update it to also assert left-side radii are "0px" to prevent regressions that
round all corners—after calling mockElement(...) and applyCurvomorphism(...),
add expectations for el.style.borderTopLeftRadius and
el.style.borderBottomLeftRadius toEqual "0px" so the test validates left corners
remain square.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7392bf96-c983-4483-9fd1-81aec557a942
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
Build/package.jsonBuild/src/index.tsBuild/src/primitives/button.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/image.tsBuild/src/primitives/toast.tsBuild/tests/curvomorphism.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Build/src/primitives/toast.ts (2)
102-102:⚠️ Potential issue | 🟠 MajorPreserve
duration=0instead of coercing it to 3000.
parseInt(...) || 3000treats the valid value0as missing, soSazamiToast.show(message, variant, 0)still auto-dismisses even though Lines 159-161 clearly modelduration > 0as the opt-in condition.Suggested fix
- const duration = parseInt(this.getAttribute("duration") || "3000") || 3000; + const parsedDuration = Number(this.getAttribute("duration")); + const duration = + Number.isFinite(parsedDuration) && parsedDuration >= 0 + ? parsedDuration + : 3000;Also applies to: 159-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` at line 102, The duration parsing currently uses parseInt(... ) || 3000 which converts a valid 0 to 3000; change the logic in the toast creation (the line setting duration and any uses in SazamiToast.show) to treat an explicit "0" as a valid value: first read the raw attribute/string, if attribute is null or empty use the default 3000, otherwise parseInt and if parseInt returns NaN fall back to 3000; do not use the || operator that coerces 0 to the default. Ensure the same fix is applied to the related checks around duration > 0 (Lines 159-161 / SazamiToast.show usage).
63-69:⚠️ Potential issue | 🟠 Major
hide()can still re-show the toast viavisiblereflection.
visibleis reflective,render()restores the attribute, andhide()removes it. In this component model that means dismissing can immediately feed back into another render and put the toast back into the visible state instead of letting the exit path complete.Keep
visibleout of the observed/reflected state path and use the attribute purely as the CSS flag for the animation.Also applies to: 155-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` around lines 63 - 69, The component currently reflects the "visible" property (toastConfig.properties.visible) so render() re-adds the attribute when the internal state changes and hide() can inadvertently re-show the toast; remove "visible" from the observed/reflected properties in toastConfig so it is an internal state only, stop relying on property reflection to set the attribute, and instead have render() and hide() directly manage the DOM attribute used for CSS (e.g., set/remove the "visible" attribute via element.setAttribute/removeAttribute based on the internal visible flag), ensure hide() updates the internal visible flag and delays removing the CSS attribute until the exit animation completes (or use a microtask/animation frame) to avoid immediate re-rendering that would re-add the attribute; apply the same change to the other visible handling code path referenced around render() and hide() (the other occurrence at 155-170).
🧹 Nitpick comments (2)
Build/src/curvomorphism/index.ts (1)
98-111: RedundantfindCentercalls when both coordinates are missing.When neither
centerXnorcenterYis provided,findCenter(element)is called twice (once for each coordinate). Consider caching the result.♻️ Suggested optimization
const apply = () => { - const cx = options.centerX ?? findCenter(element).x; - const cy = options.centerY ?? findCenter(element).y; + const needsCenter = options.centerX === undefined || options.centerY === undefined; + const center = needsCenter ? findCenter(element) : null; + const cx = options.centerX ?? center!.x; + const cy = options.centerY ?? center!.y; applyCurvomorphism(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/curvomorphism/index.ts` around lines 98 - 111, The apply function currently calls findCenter(element) twice when both options.centerX and options.centerY are undefined; update apply to call findCenter(element) once, cache its result (e.g. const {x, y} = findCenter(element) or cachedCenter = findCenter(element)) and then use cachedCenter.x / cachedCenter.y to compute cx and cy before calling applyCurvomorphism with element, cx, cy, radiusValue and the options.group* flags to avoid redundant work.Build/src/primitives/spinner.ts (1)
70-76: Consider keepingrole="status"regardless of custom label.The current logic removes
role="status"andaria-livewhen a custom label is provided. However, a spinner typically benefits from these attributes regardless of label presence—the role communicates the loading state semantically, andaria-liveensures dynamic updates are announced.♿ Suggested accessibility improvement
- if (!hasCustomLabel) { - this.setAttribute("role", "status"); - this.setAttribute("aria-live", "polite"); - } else { - this.removeAttribute("role"); - this.removeAttribute("aria-live"); - } + // Always indicate loading status semantically + if (!this.hasAttribute("role")) { + this.setAttribute("role", "status"); + } + if (!this.hasAttribute("aria-live")) { + this.setAttribute("aria-live", "polite"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/spinner.ts` around lines 70 - 76, The spinner currently removes role="status" and aria-live when hasCustomLabel is true; instead always set role="status" and aria-live="polite" so the loading state is announced regardless of label. In Build/src/primitives/spinner.ts update the logic around hasCustomLabel (the code calling this.setAttribute/removeAttribute) to always call this.setAttribute("role", "status") and this.setAttribute("aria-live", "polite") and remove the branch that calls this.removeAttribute for those attributes; leave any other custom-label handling intact (e.g., aria-hidden or label-specific attributes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/slider.ts`:
- Around line 84-91: The attribute-sync logic currently collapses removed
numeric attributes to 0 and allows invalid strings to become NaN; update the
attribute parsing/sync path (attributeChangedCallback / syncAttributeToState /
the code around the numeric attribute handlers referenced in the diff) to: when
an attribute is removed, fall back to the corresponding default from
sliderConfig (value:50, min:0, max:100, step:1) instead of 0; when parsing
strings use safe parsing with a NaN check and fall back to the sliderConfig
default if parsing fails; and ensure after parsing you clamp/normalize value
between min and max and keep step > 0 before writing into the public state so
render() no longer has to mask invalid state.
In `@Build/src/primitives/toast.ts`:
- Around line 33-36: Stylesheet missing a selector for variant="danger" so
elements using variant="danger" fall back to default icon color; add a rule
matching the existing pattern (e.g., :host([variant="danger"]) .icon) and set it
to the same color as :host([variant="error"]) .icon (var(--saz-color-danger)) so
the .icon color for the danger variant is handled consistently with the JS
handling of "danger".
---
Duplicate comments:
In `@Build/src/primitives/toast.ts`:
- Line 102: The duration parsing currently uses parseInt(... ) || 3000 which
converts a valid 0 to 3000; change the logic in the toast creation (the line
setting duration and any uses in SazamiToast.show) to treat an explicit "0" as a
valid value: first read the raw attribute/string, if attribute is null or empty
use the default 3000, otherwise parseInt and if parseInt returns NaN fall back
to 3000; do not use the || operator that coerces 0 to the default. Ensure the
same fix is applied to the related checks around duration > 0 (Lines 159-161 /
SazamiToast.show usage).
- Around line 63-69: The component currently reflects the "visible" property
(toastConfig.properties.visible) so render() re-adds the attribute when the
internal state changes and hide() can inadvertently re-show the toast; remove
"visible" from the observed/reflected properties in toastConfig so it is an
internal state only, stop relying on property reflection to set the attribute,
and instead have render() and hide() directly manage the DOM attribute used for
CSS (e.g., set/remove the "visible" attribute via
element.setAttribute/removeAttribute based on the internal visible flag), ensure
hide() updates the internal visible flag and delays removing the CSS attribute
until the exit animation completes (or use a microtask/animation frame) to avoid
immediate re-rendering that would re-add the attribute; apply the same change to
the other visible handling code path referenced around render() and hide() (the
other occurrence at 155-170).
---
Nitpick comments:
In `@Build/src/curvomorphism/index.ts`:
- Around line 98-111: The apply function currently calls findCenter(element)
twice when both options.centerX and options.centerY are undefined; update apply
to call findCenter(element) once, cache its result (e.g. const {x, y} =
findCenter(element) or cachedCenter = findCenter(element)) and then use
cachedCenter.x / cachedCenter.y to compute cx and cy before calling
applyCurvomorphism with element, cx, cy, radiusValue and the options.group*
flags to avoid redundant work.
In `@Build/src/primitives/spinner.ts`:
- Around line 70-76: The spinner currently removes role="status" and aria-live
when hasCustomLabel is true; instead always set role="status" and
aria-live="polite" so the loading state is announced regardless of label. In
Build/src/primitives/spinner.ts update the logic around hasCustomLabel (the code
calling this.setAttribute/removeAttribute) to always call
this.setAttribute("role", "status") and this.setAttribute("aria-live", "polite")
and remove the branch that calls this.removeAttribute for those attributes;
leave any other custom-label handling intact (e.g., aria-hidden or
label-specific attributes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c993fe68-15de-43fa-9691-d4442461b620
📒 Files selected for processing (12)
Build/package.jsonBuild/scripts/build-demo.jsBuild/src/curvomorphism/index.tsBuild/src/index.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/checkbox.tsBuild/src/primitives/image.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/toast.tsBuild/tests/curvomorphism.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- Build/tests/curvomorphism.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Build/src/primitives/slider.ts (1)
189-192:⚠️ Potential issue | 🟠 MajorUse slider defaults when numeric attributes are removed or invalid.
On Line 191, removing
maxcurrently falls back to0(not100fromsliderConfig), which can collapse the range unexpectedly. This was flagged earlier and is still present.Suggested fix
+ const numericDefaults = { + value: 50, + min: 0, + max: 100, + step: 1, + } as const; + - let parsed = newVal !== null ? parseFloat(newVal) : null; - if (parsed === null || Number.isNaN(parsed)) { - parsed = name === "value" ? 50 : name === "step" ? 1 : 0; - } + const raw = newVal !== null ? Number(newVal) : numericDefaults[name]; + let parsed = Number.isFinite(raw) ? raw : numericDefaults[name]; if (name === "step" && parsed <= 0) { parsed = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 189 - 192, The current fallback sets parsed to hardcoded defaults (50/1/0) and causes attributes like max to revert to 0; update the invalid/null handling in the setter where parsed is computed (variables parsed, newVal, name) to use the sliderConfig defaults instead of the hardcoded values: when parsed === null || Number.isNaN(parsed) assign parsed = sliderConfig[name] (or if sliderConfig doesn't have that key, use the existing sensible defaults: value:50, step:1, min:0, max:100); ensure you reference sliderConfig explicitly so removed/invalid numeric attributes restore their configured defaults.
🧹 Nitpick comments (1)
Build/src/primitives/toast.ts (1)
4-4: Remove unused import.
escapeHtmlis imported but never used in this file. The XSS concern was addressed by usingtextContentinstead ofinnerHTML(Line 140), making this import redundant.🧹 Proposed fix
import { SazamiComponent, component } from "./base"; import { INTERACTIVE_HOVER } from "./shared"; import { ICON_SVGS } from "../icons/index"; -import { escapeHtml } from "../escape";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` at line 4, Remove the unused import escapeHtml from the top of Build/src/primitives/toast.ts (the import statement "import { escapeHtml } from \"../escape\""); confirm there are no remaining references to escapeHtml in the file (the toast rendering code uses textContent instead of innerHTML), then run the type-check/build to ensure no unused-import errors remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/primitives/slider.ts`:
- Around line 197-202: The clamp logic only runs when name === "value", so
updates to this.min or this.max can leave this.value out of range; update the
handler to also normalize this.value when name is "min" or "max" (or extract the
clamping into a helper like clampValue() and call it from the branches for
"value", "min", and "max"), using this.min, this.max and this.value to compute
and assign the corrected value.
---
Duplicate comments:
In `@Build/src/primitives/slider.ts`:
- Around line 189-192: The current fallback sets parsed to hardcoded defaults
(50/1/0) and causes attributes like max to revert to 0; update the invalid/null
handling in the setter where parsed is computed (variables parsed, newVal, name)
to use the sliderConfig defaults instead of the hardcoded values: when parsed
=== null || Number.isNaN(parsed) assign parsed = sliderConfig[name] (or if
sliderConfig doesn't have that key, use the existing sensible defaults:
value:50, step:1, min:0, max:100); ensure you reference sliderConfig explicitly
so removed/invalid numeric attributes restore their configured defaults.
---
Nitpick comments:
In `@Build/src/primitives/toast.ts`:
- Line 4: Remove the unused import escapeHtml from the top of
Build/src/primitives/toast.ts (the import statement "import { escapeHtml } from
\"../escape\""); confirm there are no remaining references to escapeHtml in the
file (the toast rendering code uses textContent instead of innerHTML), then run
the type-check/build to ensure no unused-import errors remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5ec98b9-d020-4b81-bb96-cf2153b6c1a9
📒 Files selected for processing (2)
Build/src/primitives/slider.tsBuild/src/primitives/toast.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Build/src/primitives/slider.ts (1)
189-195:⚠️ Potential issue | 🟡 MinorDefault for
maxdoes not matchsliderConfig.When parsing fails or the attribute is removed,
maxfalls back to0(line 191), butsliderConfigdeclaresdefault: 100. This causesremoveAttribute("max")or an invalid max value to break the slider range.🐛 Proposed fix: use config-consistent defaults
let parsed = newVal !== null ? parseFloat(newVal) : null; if (parsed === null || Number.isNaN(parsed)) { - parsed = name === "value" ? 50 : name === "step" ? 1 : 0; + const defaults: Record<string, number> = { value: 50, min: 0, max: 100, step: 1 }; + parsed = defaults[name]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/slider.ts` around lines 189 - 195, The fallback for parsed numeric attributes uses 0 for unknown names which makes "max" default to 0 and breaks the slider; update the parsing branch that sets `parsed` (when `newVal` is null or NaN) to use config-consistent defaults by mapping `name === "value"` => 50, `name === "step"` => 1, `name === "max"` => 100 (matching `sliderConfig`), otherwise 0, and keep the existing `if (name === "step" && parsed <= 0) parsed = 1;` check to ensure step stays positive; adjust the code around the `parsed` and `name` variables accordingly.Build/src/primitives/toast.ts (1)
121-127:⚠️ Potential issue | 🟠 MajorUpdate live-region attributes when variant changes.
role/aria-liveare only set once. If the toast instance re-renders with a new variant (e.g., info → error), Line 122-127 keeps stale values and urgency is announced incorrectly.💡 Suggested fix
- if (!this.hasAttribute("role")) { - this.setAttribute("role", urgent ? "alert" : "status"); - } - if (!this.hasAttribute("aria-live")) { - this.setAttribute("aria-live", urgent ? "assertive" : "polite"); - } + const computedRole = urgent ? "alert" : "status"; + const computedLive = urgent ? "assertive" : "polite"; + + const currentRole = this.getAttribute("role"); + if (currentRole === null || currentRole === "status" || currentRole === "alert") { + this.setAttribute("role", computedRole); + } + + const currentLive = this.getAttribute("aria-live"); + if ( + currentLive === null || + currentLive === "polite" || + currentLive === "assertive" + ) { + this.setAttribute("aria-live", computedLive); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/toast.ts` around lines 121 - 127, The current logic in the toast component only sets role/aria-live once via the hasAttribute guards, so when the toast's variant changes (e.g., info → error) the urgency stays stale; update the code in the host/component update path (the block that computes const urgent = variant === "error" || variant === "danger") to always set this.setAttribute("role", urgent ? "alert" : "status") and this.setAttribute("aria-live", urgent ? "assertive" : "polite") (remove the this.hasAttribute checks) so the attributes are rewritten whenever variant changes (reference the variant/urgent computation and the setAttribute calls to locate where to change).
🧹 Nitpick comments (1)
Build/src/primitives/spinner.ts (1)
68-69: Avoid forcing ARIA attributes on every render.This currently overwrites consumer-provided semantics each rerender. Prefer setting defaults only when absent.
♻️ Proposed tweak
- this.setAttribute("role", "status"); - this.setAttribute("aria-live", "polite"); + if (!this.hasAttribute("role")) this.setAttribute("role", "status"); + if (!this.hasAttribute("aria-live")) this.setAttribute("aria-live", "polite");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/spinner.ts` around lines 68 - 69, The Spinner currently overwrites consumer ARIA semantics on every render by unconditionally calling this.setAttribute("role", "status") and this.setAttribute("aria-live", "polite"); update the Spinner implementation so it only sets those defaults when absent (e.g., check this.hasAttribute("role") and this.hasAttribute("aria-live") before calling setAttribute) so consumer-provided attributes are preserved across rerenders—apply this change in the Spinner class where those setAttribute calls occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Build/src/curvomorphism/index.ts`:
- Around line 20-21: The computation of tx and ty assumes ordered bounds so
negative spans collapse to 1 when callers pass reversed bounds; update the logic
that computes tx and ty (referencing tx, ty and the inputs groupRight,
groupLeft, groupBottom, groupTop) to use the absolute span (e.g.,
Math.abs(groupRight - groupLeft) and Math.abs(groupBottom - groupTop)) before
dividing and applying Math.max(1, ...), so the dead-zone thresholds are correct
regardless of bound ordering.
- Around line 99-103: The bug is that cachedCenter is set to {x:0,y:0} whenever
at least one of options.centerX or options.centerY is provided, causing the
other axis to default to 0; change the logic so cachedCenter is computed via
findCenter(element) whenever either centerX or centerY is undefined (instead of
only when both are undefined), then keep the existing cx = options.centerX ??
cachedCenter.x and cy = options.centerY ?? cachedCenter.y assignments; reference
the cachedCenter variable and the cx/cy assignments as the locations to update
and use findCenter(element) when needed.
In `@Build/src/primitives/toast.ts`:
- Around line 82-95: The hide() method schedules a 300ms timeout but doesn't
store its handle, so repeated hide() calls or disconnects can leave pending
callbacks; update hide() to clear any existing this._hideTimeout, assign the
result of setTimeout to this._hideTimeout (type ReturnType<typeof setTimeout>),
and inside the timeout callback set this._hideTimeout = undefined after running
the removal logic; keep disconnectedCallback() as-is (it already clears
this._hideTimeout) so it will cancel the stored timeout correctly.
- Around line 149-155: The code force-casts closeBtn to HTMLElement which can
throw if the lookup fails; update the showClose branch in the component where
showClose, closeBtn, this.$, addHandler, removeHandler and _closeHandler are
used so you first get the element safely (e.g., const closeBtn =
this.$(".close-btn")) and check for null/undefined before calling addHandler;
still call removeHandler unconditionally if needed, but only call
addHandler(..., { element: closeBtn }) when closeBtn is non-null to avoid the
unsafe cast and runtime errors.
---
Duplicate comments:
In `@Build/src/primitives/slider.ts`:
- Around line 189-195: The fallback for parsed numeric attributes uses 0 for
unknown names which makes "max" default to 0 and breaks the slider; update the
parsing branch that sets `parsed` (when `newVal` is null or NaN) to use
config-consistent defaults by mapping `name === "value"` => 50, `name ===
"step"` => 1, `name === "max"` => 100 (matching `sliderConfig`), otherwise 0,
and keep the existing `if (name === "step" && parsed <= 0) parsed = 1;` check to
ensure step stays positive; adjust the code around the `parsed` and `name`
variables accordingly.
In `@Build/src/primitives/toast.ts`:
- Around line 121-127: The current logic in the toast component only sets
role/aria-live once via the hasAttribute guards, so when the toast's variant
changes (e.g., info → error) the urgency stays stale; update the code in the
host/component update path (the block that computes const urgent = variant ===
"error" || variant === "danger") to always set this.setAttribute("role", urgent
? "alert" : "status") and this.setAttribute("aria-live", urgent ? "assertive" :
"polite") (remove the this.hasAttribute checks) so the attributes are rewritten
whenever variant changes (reference the variant/urgent computation and the
setAttribute calls to locate where to change).
---
Nitpick comments:
In `@Build/src/primitives/spinner.ts`:
- Around line 68-69: The Spinner currently overwrites consumer ARIA semantics on
every render by unconditionally calling this.setAttribute("role", "status") and
this.setAttribute("aria-live", "polite"); update the Spinner implementation so
it only sets those defaults when absent (e.g., check this.hasAttribute("role")
and this.hasAttribute("aria-live") before calling setAttribute) so
consumer-provided attributes are preserved across rerenders—apply this change in
the Spinner class where those setAttribute calls occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09e8797e-eb34-4612-aef2-75d120099041
📒 Files selected for processing (4)
Build/src/curvomorphism/index.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/toast.ts
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.
Summary by CodeRabbit
New Features
Refactor
Tests
Docs
Chores / Bug Fixes