Improve integration with Sairin and Togetherify the "S-Ecosystem"#2
Merged
Improve integration with Sairin and Togetherify the "S-Ecosystem"#2
Conversation
added 2 commits
March 16, 2026 04:24
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Member
Author
|
@coderabbitai review |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Sairin signal support across the codebase: runtime dependency updates and new Build/src/config.ts; extends base component with structural render, batched/synchronous mounting, and unified bind APIs; many primitive components now accept Readable/Signal props (with two‑way where applicable); tests and docs updated for reactivity. (30 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Comp as Sazami Component
participant Signal as Sairin Signal/Derived
participant DOM as DOM
App->>Comp: set prop = Signal<T> (or string/number)
Comp->>Comp: detect readable (isSignal / Derived?) and store signal
Comp->>Signal: bindText / bindProperty / bindAttribute(target, signal)
Signal->>Signal: register effect listener
Signal->>DOM: On change -> effect runs -> update DOM node/property
DOM->>Comp: visual state updated (shadow DOM)
App->>Signal: update signal value
Signal->>DOM: effect triggers DOM update
Comp->>Comp: onCleanup() on disconnect -> dispose binding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Build/src/runtime/transformer.ts (1)
74-79:⚠️ Potential issue | 🟠 MajorUse serialized inline value for
props.iconas well.
childrennow uses the serialized string, but Line 78 still assigns rawnode.value. Interpolated icon values can become object payloads instead of strings.💡 Proposed fix
- if (ICON_COMPONENTS.has(tag) && node.value && !props.icon) { - props.icon = node.value; + if (ICON_COMPONENTS.has(tag) && value && !props.icon) { + props.icon = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/runtime/transformer.ts` around lines 74 - 79, ICON_COMPONENTS handling assigns the raw node.value to props.icon which can be non-string for interpolated values; use the already computed serialized string instead. In the block that checks ICON_COMPONENTS.has(tag) and node.value and !props.icon, set props.icon = value (the result of serializeValue(node.value) / string fallback) rather than props.icon = node.value so interpolated/object payloads become serialized strings; keep the existing condition and existing serializeValue call.Build/tests/errors.test.ts (1)
112-118:⚠️ Potential issue | 🟡 MinorDuplicate test names will cause confusion in test reports.
Both tests at Lines 112-114 and 116-118 are named
"should throw when missing closing >". Most test runners will report them with the same name, making it difficult to identify which test failed. Additionally, they test the same input with slightly different assertions.Consider merging them or giving distinct names.
🧹 Proposed fix to merge duplicate tests
- test('should throw when missing closing >', () => { - expect(() => parseSakko('<page { text: Hello }')).toThrow(); - }); - test('should throw when missing closing >', () => { expect(() => parseSakko('<page { text: Hello }')).toThrow("Expected '>'"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/errors.test.ts` around lines 112 - 118, Two identical tests named "should throw when missing closing >" both call parseSakko('<page { text: Hello }') with different toThrow assertions; rename one test or merge them into a single clearer test so reports are unambiguous. Update the test descriptions (e.g., "should throw when missing closing > (generic)" and "should throw with message 'Expected >'") or combine into one test that asserts both that parseSakko throws and that the error message equals "Expected '>'" using the existing parseSakko call and expect(...).toThrow checks.Build/src/primitives/input.ts (1)
133-153:⚠️ Potential issue | 🟠 MajorDon't short-circuit non-
valueattributes when a signal is bound.The early return on Line 139 prevents later
disabled,placeholder, andtypeupdates from reaching the inner<input>whenever_valueSignalis set.Suggested fix
- if (!input || this._valueSignal) return; + if (!input) return; if (name === "value") { + if (this._valueSignal) return; if (newVal === null) { if (input.value !== "") input.value = ""; } else if (input.value !== newVal) {🤖 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 133 - 153, The attributeChangedCallback currently returns early when this._valueSignal is truthy, preventing non-value attributes from propagating to the inner input; change the guard so you only skip the "value" update when _valueSignal exists but still apply other attribute branches: in attributeChangedCallback (and where you get the input via this.$("input")), remove the combined early return and instead do if (!input) return; then wrap only the "value" branch with a check like if (!this._valueSignal) { ... } so that disabled, placeholder, and type still update the input even when _valueSignal is set.Build/src/primitives/select.ts (1)
327-332:⚠️ Potential issue | 🔴 CriticalBug:
_updateTabIndexusesthis.disabledwhich may be aReadable<boolean>.On line 330,
this.disabledis used directly for the tabindex check. However,this.disabledcan now returnReadable<boolean>instead ofboolean, causingthis.disabled ? "-1" : "0"to always evaluate to"-1"when a signal is bound (since the signal object is truthy).🐛 Fix: Use _getIsDisabled() instead
private _updateTabIndex() { const trigger = this.$(".trigger") as HTMLElement; if (trigger) { - trigger.setAttribute("tabindex", this.disabled ? "-1" : "0"); + trigger.setAttribute("tabindex", this._getIsDisabled() ? "-1" : "0"); } }🤖 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 327 - 332, The tabindex logic in _updateTabIndex currently checks this.disabled (which may be a Readable<boolean>) causing incorrect truthiness; update _updateTabIndex to call this._getIsDisabled() for the boolean check and use its result when calling trigger.setAttribute("tabindex", ...), e.g. replace uses of this.disabled in _updateTabIndex with this._getIsDisabled() so the tabindex becomes "-1" only when truly disabled.
🧹 Nitpick comments (26)
Build/tests/pipeline.test.ts (1)
4-4: Remove unuseditimport.The
itfunction is imported but not used in this file.-import { describe, test, expect, it, beforeEach } from '@jest/globals'; +import { describe, test, expect, beforeEach } from '@jest/globals';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/pipeline.test.ts` at line 4, The import list at the top of the test file includes an unused symbol `it`; remove `it` from the named imports (import { describe, test, expect, beforeEach } from '@jest/globals') so the file no longer imports an unused identifier and lints/compiles cleanly.Build/tests/accessibility.test.ts (1)
4-4: Remove unuseditimport.The
itfunction is imported but not used in this file.-import { describe, test, expect, it, beforeAll } from '@jest/globals'; +import { describe, test, expect, beforeAll } from '@jest/globals';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/accessibility.test.ts` at line 4, The import list includes an unused symbol "it" from '@jest/globals'; remove "it" from the import statement (keep describe, test, expect, beforeAll) so the top-level import no longer imports the unused identifier and re-run lint/tests to confirm no unused-import warnings remain.Build/tests/modifiers.test.ts (1)
1-1: Remove unuseditimport.The
itfunction is imported but not used in this file.-import { describe, test, expect, it } from '@jest/globals'; +import { describe, test, expect } from '@jest/globals';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/modifiers.test.ts` at line 1, The import statement in Build/tests/modifiers.test.ts unnecessarily includes the unused symbol "it"; remove "it" from the named imports (i.e., change the import { describe, test, expect, it } from '@jest/globals' to import { describe, test, expect } from '@jest/globals') so only used functions are imported and linter warnings are resolved.Build/tests/primitives.test.ts (1)
4-4: Remove unuseditimport.The
itfunction is imported but not used in this file—onlytestis used. Consider removing it for cleaner imports.-import { describe, test, expect, it, beforeEach } from '@jest/globals'; +import { describe, test, expect, beforeEach } from '@jest/globals';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/primitives.test.ts` at line 4, The import statement currently includes an unused symbol `it`; update the import from '@jest/globals' to remove `it` so only used symbols remain (e.g., keep describe, test, expect, beforeEach). Locate the import line that references `it` in this file (the top-level import) and edit it to drop `it`; run the test/lint to confirm no further unused-import warnings.Build/tests/generic.test.ts (1)
4-4: Remove unuseditimport.The
itfunction is imported but not used in this file.-import { describe, test, expect, it, beforeAll } from '@jest/globals'; +import { describe, test, expect, beforeAll } from '@jest/globals';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/generic.test.ts` at line 4, Remove the unused `it` import from the Jest import list to clean up the module; update the import statement that currently reads "import { describe, test, expect, it, beforeAll } from '@jest/globals';" by deleting `it` so it only imports the symbols actually used (describe, test, expect, beforeAll).Build/tests/curvomorphism.test.ts (1)
4-4: Remove unuseditimport.The
itfunction is imported but never used in this file.-import { describe, test, expect, it } from '@jest/globals'; +import { describe, test, expect } from '@jest/globals';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/curvomorphism.test.ts` at line 4, The import statement in curvomorphism.test.ts currently imports an unused symbol `it`; remove `it` from the named imports (leave `describe`, `test`, and `expect`) so the file no longer imports unused identifiers and re-run the tests/linter to ensure no other unused imports remain.Build/src/primitives/tag.ts (1)
3-4: UnusedSignalimport.Same as in
badge.ts,Signalis imported but not directly used.🤖 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 3 - 4, Remove the unused Signal import from the import list in tag.ts: locate the import statement that includes Signal, Derived, isSignal, Readable and bindText, and delete only the Signal symbol so the module imports remain valid (e.g., import { Derived, isSignal, type Readable } from "@nisoku/sairin"; and keep bindText import as-is).Build/src/primitives/label.ts (2)
2-3: UnusedSignalimport.Same pattern as other files.
🤖 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 2 - 3, The import list in this module unnecessarily includes the symbol Signal; remove Signal from the named imports from "@nisoku/sairin" so the line imports only the actually used symbols (e.g., Derived, isSignal, type Readable) and keep bindText import as-is; update the import statement that currently references Signal to exclude it (adjusting any comma/spacing as needed).
69-86: Redundant DOM query for label element.The
<label>element is queried twice: once at Line 69 and again at Line 82. Use the already-retrieved reference.♻️ Reuse the label reference
const label = this.shadow.querySelector("label"); if (label) { this._textNode = document.createTextNode(""); label.prepend(this._textNode); - } - if (this._contentSignal) { - this._setupSignalBinding(); - } else { - this._setTextContent(this._content as string); - } - if (this.hasAttribute("for")) { - const labelEl = this.shadowRoot?.querySelector("label"); - if (labelEl) { - labelEl.setAttribute("for", this.getAttribute("for") || ""); + if (this._contentSignal) { + this._setupSignalBinding(); + } else { + this._setTextContent(this._content as string); + } + + if (this.hasAttribute("for")) { + label.setAttribute("for", this.getAttribute("for") || ""); } }🤖 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 69 - 86, The code redundantly queries the label element twice; reuse the already retrieved reference instead of calling querySelector again. In the block that checks this.hasAttribute("for"), use the previously obtained constant label (from const label = this.shadow.querySelector("label");) rather than shadowRoot?.querySelector; if label exists set its "for" attribute to this.getAttribute("for") || "". Ensure this still runs after _textNode insertion and after handling _contentSignal/_setTextContent so the sequence and references (_textNode, _contentSignal, _setupSignalBinding, _setTextContent) remain unchanged.Build/src/primitives/heading.ts (1)
3-4: UnusedSignalimport.Consistent with other primitive files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/heading.ts` around lines 3 - 4, Remove the unused Signal import from the module import list: update the import that currently includes Signal (from "@nisoku/sairin") to only import the used symbols (e.g., Derived, isSignal, type Readable) so the unused Signal identifier is not imported; keep bindText import as-is.Build/src/primitives/image.ts (3)
80-82: Complex fallback chain forcurrentSrc.The ternary with multiple fallbacks is difficult to read. Consider extracting to a helper method.
♻️ Extract to helper for clarity
+ private _getCurrentSrc(): string { + if (this._srcSignal) return this._srcSignal.get(); + if (this._pendingSrc) return this._pendingSrc; + if ((this as any)._src) return (this as any)._src; + return this.getAttribute("src") || ""; + } + render() { - const currentSrc = this._srcSignal - ? this._srcSignal.get() - : (this._pendingSrc || (this as any)._src || this.getAttribute("src") || ""); + const currentSrc = this._getCurrentSrc(); if (!currentSrc) {🤖 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 80 - 82, The currentSrc assignment in Build/src/primitives/image.ts uses a complex inline ternary and fallback chain (references: this._srcSignal, this._pendingSrc, (this as any)._src, this.getAttribute("src")) which reduces readability; extract this logic into a small helper method (e.g., resolveCurrentSrc or getCurrentSrc) that returns the resolved string and replace the inline expression with a call to that method, ensuring the helper handles the same order of precedence and returns an empty string as the final fallback.
4-5: UnusedSignalimport.Consistent with other files.
🤖 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 4 - 5, Remove the unused Signal import from the import statement in Build/src/primitives/image.ts; update the first import to only include Derived, isSignal and type Readable (i.e., remove Signal) so the imports match usage and lint rules, leaving bindProperty import as-is; ensure no other references to Signal exist in the file before committing.
49-77: Same binding leak and type-unsafe patterns as other primitives.The
(this as any)._srcpattern and the potential for binding leaks on reassignment apply here as well. See comments onbutton.tsfor suggested improvements.🤖 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 49 - 77, The src setter/getter uses a type-unsafe (this as any)._src and can leak bindings on reassignment; add a properly typed private _src: string | null and a private _srcDispose: (() => void) | null, call and null out _srcDispose before creating a new binding in _setupSrcBinding, and ensure _pendingSrc and _srcSignal types are explicit (Readable<string> | null and string | null). Update the setter for src to clear previous dispose (if any) when switching modes, set _src when assigning a plain string (and call _updateSrc), and when assigning a Readable call _setupSrcBinding to create and store the new disposer via _srcDispose and register it with onCleanup; remove all (this as any)._src casts and use the typed _src field in the getter and _updateSrc.Build/src/primitives/badge.ts (2)
70-79: Text node initialized with empty string when signal is set before render.When
contentis set to a signal beforerender()is called,_textContentremains"". Line 73 initializes the text node with this empty value, then Line 78 re-triggers the setter. This works but causes an unnecessary initial empty render before the signal's value is applied.Consider initializing the text node with the signal's current value directly.
♻️ Initialize text node with signal's current value
const slot = this.shadow.querySelector("slot"); if (slot) { - this._textNode = document.createTextNode(this._textContent); + const initialText = this._contentSignal + ? this._contentSignal.get() + : this._textContent; + this._textNode = document.createTextNode(initialText); slot.replaceWith(this._textNode); } if (this._contentSignal) { - this.content = this._contentSignal; + const dispose = bindText(this._textNode!, this._contentSignal); + this.onCleanup(dispose); }🤖 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 70 - 79, When rendering, avoid creating the initial text node with an empty string if a signal was set earlier: in render() where you query the slot and create this._textNode (currently using this._textContent), check this._contentSignal and, if present, use its current value instead of this._textContent so the initial node reflects the signal value; update the logic around slot, this._textNode, this._textContent, this._contentSignal and the content setter to initialize the node with this._contentSignal() (or equivalent getter) so the subsequent content = this._contentSignal assignment no longer causes an unnecessary empty render.
3-4: UnusedSignalimport.
Signalis imported but never directly used in this file. OnlyisSignal,Derived,Readable, andbindTextare used.🧹 Remove unused import
-import { Signal, Derived, isSignal, type Readable } from "@nisoku/sairin"; +import { Derived, isSignal, type Readable } from "@nisoku/sairin";🤖 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 3 - 4, The import list in badge.ts includes an unused Symbol "Signal"; remove "Signal" from the named imports from "@nisoku/sairin" so only the actually used exports (isSignal, Derived, type Readable) remain, keeping bindText import as-is; update the import statement that currently references Signal to avoid lint errors and unused-variable warnings.Build/src/config.ts (1)
17-22: New logger instance created on every call.
getSazamiLogger()creates a new logger viacreateLogger("sazami")each time it's invoked. Depending on the Satori implementation, this may create redundant logger instances with associated overhead, or cause issues if loggers maintain internal state.Consider caching the logger instance.
♻️ Cache the logger instance
let currentConfig: SazamiConfig = { satori: null, }; +let cachedLogger: SatoriLogger | null = null; + export function configureSazami(config: Partial<SazamiConfig>): void { if (config.satori !== undefined) { currentConfig.satori = config.satori; + cachedLogger = null; // Invalidate cache when satori changes } } export function getSazamiLogger(): SatoriLogger | null { if (!currentConfig.satori) { return null; } - return currentConfig.satori.createLogger("sazami"); + if (!cachedLogger) { + cachedLogger = currentConfig.satori.createLogger("sazami"); + } + return cachedLogger; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/config.ts` around lines 17 - 22, getSazamiLogger currently calls currentConfig.satori.createLogger("sazami") on every invocation which can produce duplicate logger instances; change it to cache a single SatoriLogger instance (e.g., a module-level variable) and return that cached instance on subsequent calls, initializing it only once when currentConfig.satori is present; ensure to handle config changes/null by clearing or reinitializing the cached logger accordingly and reference getSazamiLogger, currentConfig.satori, and createLogger("sazami") in your update.Build/src/primitives/button.ts (2)
13-14: UnusedSignalimport.
Signalis imported but not directly used.🤖 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 13 - 14, Remove the unused Signal import from the import statement that currently reads "import { Signal, Derived, isSignal, type Readable } from \"@nisoku/sairin\""; update it to remove Signal (e.g., "import { Derived, isSignal, type Readable } from \"@nisoku/sairin\"") so there are no unused imports, or if Signal is intended to be used later, reference it where needed in the button logic (search for usages around bindDisabled/Derived/isSignal to decide which approach).
82-93: Type-unsafe access to_disabledviaanycast.Multiple lines use
(this as any)._disabledto access what appears to be a property generated by the@componentdecorator. This bypasses type checking and could mask errors if the decorator behavior changes.Consider adding proper type declarations or using a typed backing field.
♻️ Add explicit backing field
private _disabledSignal: Readable<boolean> | null = null; + private _disabledValue: boolean = false; private _isReadableBool(value: unknown): value is Readable<boolean> { return isSignal(value) || value instanceof Derived; } set disabled(value: boolean | Readable<boolean>) { if (this._isReadableBool(value)) { this._disabledSignal = value; const dispose = bindDisabled(this, value); this.onCleanup(dispose); } else { this._disabledSignal = null; this._setDisabled(value); } } get disabled(): boolean | Readable<boolean> { - return this._disabledSignal || (this as any)._disabled; + return this._disabledSignal || this._disabledValue; } private _setDisabled(value: boolean) { - (this as any)._disabled = value; + this._disabledValue = value; if (value) { this.setAttribute("disabled", ""); } else { this.removeAttribute("disabled"); } } private _getIsDisabled(): boolean { if (this._disabledSignal) return this._disabledSignal.get(); - if ((this as any)._disabled !== undefined) return !!(this as any)._disabled; + return this._disabledValue || this.hasAttribute("disabled") || !!(this as any).loading; - if (this.hasAttribute("disabled")) return true; - return !!(this as any).loading; }🤖 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 82 - 93, The code uses type-unsafe casts to access (this as any)._disabled; add an explicit typed backing field and stop using any: declare a private field like private _disabled: boolean | Readable<boolean> = false (or the correct union type), update the disabled getter to return this._disabledSignal || this._disabled, and change _setDisabled to assign this._disabled = value and remove the attribute access via any. Ensure the field name matches any decorator-generated semantics (or rename the decorator-driven property and adapt) so you no longer rely on (this as any)._disabled in get disabled and private _setDisabled.Build/tests/integration.test.ts (1)
539-541: Use a parser-only failure in this test.This input still dies in tokenization, so it duplicates the
Unterminated stringpropagation case above instead of covering parser error propagation. A syntax-valid token stream that fails during parse would keep this test meaningful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/integration.test.ts` around lines 539 - 541, The test "parser error on truly malformed input propagates" currently uses an input that fails during tokenization (an unterminated string); change it to a syntactically tokenizable string that still fails during parsing. Update the call to parseSakko in that test to use a balanced/quoted and brace-matched example (e.g. something like '<page { text: "ok" unexpectedToken }>' or any input with valid tokens but invalid grammar) so tokenization succeeds and the parser itself throws; keep the test name and expect(...).toThrow() but swap the input string accordingly.Build/tests/reactive-components.test.ts (2)
530-544: Variable shadows importedsignalfunction.The local
const signalon line 538 shadows thesignalfunction imported from@nisoku/sairinon line 2. While this works because the outersignalis not used within this test, it can cause confusion and maintenance issues.♻️ Suggested rename
test('switching from static to signal value', async () => { const el = document.createElement('saz-text') as any; el.content = 'static value'; document.body.appendChild(el); await Promise.resolve(); expect(el.shadowRoot?.textContent).toContain('static value'); - const signal = derived(path("test", "dynamic"), () => 'dynamic value'); - el.content = signal; + const dynamicContent = derived(path("test", "dynamic"), () => 'dynamic value'); + el.content = dynamicContent; await Promise.resolve(); expect(el.shadowRoot?.textContent).toContain('dynamic value'); el.remove(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 530 - 544, The test "switching from static to signal value" declares a local const named `signal` which shadows the imported `signal` symbol from `@nisoku/sairin`; rename the local variable (e.g., `dynamicSignal` or `contentSignal`) and update its usage (`el.content = ...` and any subsequent references) so the imported `signal` remains unshadowed and the test reads clearly.
547-572: Resource integration test relies on timing rather than deterministic completion.The test uses
setTimeout(resolve, 100)to wait for the async resource to complete. This is flaky and could fail on slower CI environments or pass despite broken behavior on faster machines.♻️ Consider using a more deterministic approach
test('resource.value signal updates component', async () => { const el = document.createElement('saz-text') as any; const { resource } = require('@nisoku/sairin'); const userData = resource( () => Promise.resolve({ name: 'John', age: 30 }), null ); const userName = signal(path("test", "userName"), ''); effect(() => { const data = userData.value.get(); userName.set(data?.name ?? ''); }); el.content = userName; document.body.appendChild(el); - await new Promise(resolve => setTimeout(resolve, 100)); - await Promise.resolve(); + // Wait for resource to resolve and effect to propagate + await new Promise(resolve => setTimeout(resolve, 0)); + // Poll until the expected value appears or timeout + const maxAttempts = 20; + for (let i = 0; i < maxAttempts; i++) { + await Promise.resolve(); + if (el.shadowRoot?.textContent?.includes('John')) break; + await new Promise(r => setTimeout(r, 10)); + } expect(el.shadowRoot?.textContent).toContain('John'); el.remove(); });Alternatively, consider exposing a promise or callback from the resource that the test can await directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 547 - 572, The test is flaky because it waits with setTimeout; instead await the resource resolution directly: remove the setTimeout and await the promise returned by the resource (e.g., await userData.value.get() or await the resource's completion promise if it exposes one), then flush microtasks (await Promise.resolve()) before asserting; reference symbols: resource, userData, userData.value.get, effect, el.Build/src/primitives/icon.ts (1)
68-95: Duplicate icon rendering logic.The icon update logic is duplicated between
_updateIcon()(lines 68-77) and theeffectcallback in_setupIconBinding()(lines 85-93). This violates DRY and could lead to inconsistencies if one path is updated but not the other.♻️ Consolidate into a single method
private _updateIcon(iconName: string) { if (this._iconElement) { const svg = ICON_SVGS[iconName]; if (svg) { this._iconElement.innerHTML = svg; } else { this._iconElement.innerHTML = `<span>${escapeHtml(iconName)}</span>`; } } } private _setupIconBinding() { if (!this._iconElement) return; const sig = this._iconSignal!; - const el = this._iconElement; this.onCleanup( effect(() => { const iconName = sig.get(); - const svg = ICON_SVGS[iconName]; - if (svg) { - el.innerHTML = svg; - } else { - el.innerHTML = `<span>${escapeHtml(iconName)}</span>`; - } + this._updateIcon(iconName); }) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon.ts` around lines 68 - 95, Consolidate the duplicated rendering logic by making _updateIcon(iconName: string) the single source of truth for setting el.innerHTML (using ICON_SVGS and escapeHtml) and update _setupIconBinding to call that method instead of duplicating the svg/else branch; ensure _setupIconBinding uses this._iconElement and this._iconSignal (sig.get()) then calls this._updateIcon(iconName) inside the effect registered with onCleanup(effect(...)), and remove the duplicate innerHTML branches from the effect so only _updateIcon performs the DOM update.Build/src/primitives/select.ts (1)
146-157: Unnecessary optional chaining onsig.get().Line 151 uses
(sig as Signal<string>).get?.() ?? '', butReadable<string>is typed to always have aget()method. The optional chaining suggests type uncertainty. If the type is correct, remove the optional chaining for clarity.♻️ Remove unnecessary optional chaining
private _setupValueBinding() { const sig = this._valueSignal as Readable<string>; const self = this; this.onCleanup( effect(() => { - const val = (sig as Signal<string>).get?.() ?? ''; + const val = sig.get(); (self as any)._value = val; self._updateDisplay(); self._updateSelectedState(); }) ); }🤖 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 146 - 157, The optional chaining on the signal getter is unnecessary; in _setupValueBinding() remove the ?. from (sig as Signal<string>).get?.() and call (sig as Signal<string>).get() (or just sig.get() if the type is already Signal<string>) so the value retrieval is definite, e.g. const val = (sig as Signal<string>).get() ?? ''; ensure you update the expression using _valueSignal / _setupValueBinding and keep the subsequent _updateDisplay() and _updateSelectedState() calls unchanged.Build/src/primitives/slider.ts (1)
260-265: Type cast ondispatchEventTypedsuggests API mismatch.The cast
(this.dispatchEventTyped as any)("input", { value: val })on line 265 indicates the event detail type may not match the config definition. Consider ensuring thesliderConfig.events.input.detailproperly types the value asnumber.🤖 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 260 - 265, The cast on dispatchEventTyped means the event detail type doesn't match the slider config; update the sliderConfig.events.input.detail type to { value: number } (or the appropriate numeric shape) and remove the unsafe any cast so you call this.dispatchEventTyped("input", { value: val }) with correct typing; verify the dispatchEventTyped signature and the event type definitions so the compiler accepts the call (check symbols: dispatchEventTyped, sliderConfig.events.input.detail, this._valueSignal.set / (this as any)._value).Build/src/primitives/base.ts (2)
223-277: Acknowledge XSS mitigation documentation, but consider sanitization or CSP.The JSDoc correctly documents that callers must escape user-provided data. However, relying solely on caller discipline for XSS prevention is fragile. Static analysis flagged the
innerHTMLassignments at lines 226 and 271.For defense-in-depth, consider:
- Using a library like DOMPurify for sanitization
- Documenting CSP requirements for consumers
- Adding runtime validation that templates don't contain suspicious patterns
The current approach is acceptable for a framework where templates are authored by developers, but be aware of the trust boundary.
🤖 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 223 - 277, The innerHTML assignments in mount() and _flush() (shadow.innerHTML = ...) are XSS risk points; implement defense-in-depth by sanitizing templates before assigning: add a sanitization step (e.g., call a new sanitizeTemplate(template: string): string helper that uses DOMPurify or equivalent) and apply it to both mount() and _flush() inputs, plus add an opt-in flag or config (e.g., enableTemplateSanitization) on the component base so consumers can toggle/opt-in; additionally, update docs/comments to require a CSP for consumers and add a lightweight runtime check in sanitizeTemplate that rejects obvious dangerous patterns (like <script> tags or on* attributes) if DOMPurify is unavailable.
17-17: Unused import:bindSelectValue.
bindSelectValueis imported but never used in this file. Consider removing it or using it in thebind()method forHTMLSelectElementhandling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/base.ts` at line 17, The import bindSelectValue is unused; either remove it from the import list or wire it into the element binding logic: update the bind() method to detect HTMLSelectElement (e.g., if (el instanceof HTMLSelectElement)) and call bindSelectValue(el, ...) with the same binding parameters used for inputs, or simply delete bindSelectValue from the import if select-specific binding isn't needed. Ensure the symbol name bindSelectValue and the bind() function are the points you edit so the change is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46914250-11ab-4728-afbb-e63eecf68024
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (49)
Build/package.jsonBuild/src/config.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/coverart.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/modifier-map.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.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/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/curvomorphism.test.tsBuild/tests/errors.test.tsBuild/tests/generic.test.tsBuild/tests/integration.test.tsBuild/tests/modifiers.test.tsBuild/tests/pipeline.test.tsBuild/tests/primitives.test.tsBuild/tests/reactive-components.test.tsBuild/tests/renderer.test.tsBuild/tests/tokens.test.tsBuild/tsconfig.jsonDocs/docmd.config.jsDocs/docs/api-reference.mdDocs/docs/component-base.mdDocs/docs/index.mdDocs/docs/primitives.md
added 2 commits
April 1, 2026 22:08
There was a problem hiding this comment.
Actionable comments posted: 6
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/base.ts (1)
264-276:⚠️ Potential issue | 🟡 MinorTemplate identity check skips style changes.
The
_flushmethod only comparestemplatefor identity (line 266), butstylescan change independently. If styles change while template remains identical, the DOM won't update.Suggested fix
private _flush(styles: string, template: string): void { - // Template identity check, skip if template hasn't changed - if (template === this._lastTemplate) return; - this._lastTemplate = template; + // Identity check, skip if neither changed + const combined = styles + template; + if (combined === this._lastTemplate) return; + this._lastTemplate = combined; try { this.shadow.innerHTML = `<style>${styles}</style>${template}`;🤖 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 264 - 276, The _flush method currently only compares template to this._lastTemplate so style-only changes are ignored; update the identity check to consider both styles and template (e.g., compute a combined key or store lastStyles and lastTemplate) and only skip when both are unchanged, ensure you update the stored identity after successful render (references: _flush, this._lastTemplate, styles, template, shadow.innerHTML) so style changes cause shadow.innerHTML to be updated.
🧹 Nitpick comments (3)
Build/src/primitives/input.ts (2)
108-110: Fragile check for writable Signal.Using
'set' in this._valueSignal!to determine if a signal is writable works but is fragile. If Sairin changes its Signal implementation or if someone passes a proxy, this could break.Consider using Sairin's
isSignal()which you already import, or a more robust type guard.- if ('set' in this._valueSignal!) { - (this._valueSignal as Signal<string>).set(target.value); - } + if (isSignal(this._valueSignal)) { + this._valueSignal.set(target.value); + }🤖 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 108 - 110, The check using `'set' in this._valueSignal!` is fragile; replace it with Sairin's type guard by calling isSignal(this._valueSignal) (and ensure this._valueSignal is non-null) before casting to Signal<string> and calling set(target.value); update the branch around _valueSignal so it uses isSignal(this._valueSignal) to detect a signal and then (this._valueSignal as Signal<string>).set(target.value).
106-114: Input handler registration doesn't use the handler registry.When
_valueSignalis present, the input handler is registered via rawaddEventListener(line 113) with manual cleanup (line 114), bypassing the component'saddHandlerregistry. This creates inconsistency with the non-signal path (lines 116-124) which usesaddHandler.Consider using
addHandlerfor both paths, or document why the signal path differs.🤖 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 106 - 114, The input handler registration currently attaches directly with input.addEventListener and this.onCleanup when this._valueSignal is set, bypassing the component handler registry used elsewhere; change the signal-path to register the same handler via the component's addHandler (the same API used in the non-signal branch) instead of raw addEventListener and remove the manual this.onCleanup removal, or if intentional, add a comment explaining the divergence; update references to the created handler variable (handler) and ensure the dispatched event still uses this.dispatchEventTyped("input", { value: target.value }) so behavior remains identical to the non-signal path.Build/src/primitives/spinner.ts (1)
121-121: Complex fallback chain forlabelTextis hard to follow.The ternary on line 121 has multiple fallbacks that could produce unexpected results. When
_labelSignalis null, it checks(this as any)._label, thenthis.label, then"Loading...". Butthis.labelgetter (line 85-87) returnsthis._labelSignal || (this as any)._label || "", creating circular logic.Consider simplifying:
- const labelText = this._labelSignal ? this._labelSignal.get() : ((this as any)._label || this.label || "Loading..."); + const labelText = this._labelSignal + ? this._labelSignal.get() + : ((this as any)._label || "Loading...");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/spinner.ts` at line 121, The labelText fallback is circular because it calls this.label (which itself references _labelSignal), so replace the chain to use only internal storage: compute labelText by checking _labelSignal and using its .get() when present, otherwise read the backing field (_label or (this as any)._label) and finally default to "Loading..."; update the expression in spinner.ts (labelText) to avoid calling the label getter (use this._labelSignal ? this._labelSignal.get() : ((this as any)._label ?? "Loading...")) so there is no circular dependency with the label getter.
🤖 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/avatar.ts`:
- Around line 83-85: The synthetic Readable created by bindProperty for `alt`
(assigned to `altDispose`) only provides a get() and won't update when the DOM
attribute changes; change the implementation so `alt` is backed by an actual
reactive signal that is updated on attribute changes (use a MutationObserver on
`img` or implement `observedAttributes` + `attributeChangedCallback` to update
the signal), then pass that signal into bindProperty instead of the static
get-only object; ensure the signal is updated whenever `getAttribute("alt")`
changes so any subscribers to the bindProperty binding receive updates.
- Around line 88-106: The initials textContent uses a synthetic Readable inside
bindProperty that only reacts to sig.get() and ignores attribute changes
(alt/initials); replace this with a real reactive value: create a derived signal
(or computed) that reads sig.get() and also updates when attributes change (use
an attribute observer/MutationObserver or hook into attributeChangedCallback)
and pass that signal to bindProperty instead of the inline get() function;
update references to initials, bindProperty, sig.get(), getAttribute, and
_getInitials so the derived signal returns either "" when sig is present or
initialsAttr || _getInitials(alt || textContent) and ensure the observer is
cleaned up in this.onCleanup.
In `@Build/src/primitives/base.ts`:
- Around line 392-435: bindWidthPercent and bindWidthPercentAttribute create
fake Readable objects that only implement get() so they never react to changes
in the passed readable; instead wrap the reactive logic in an effect that
subscribes to readable and updates the element via bindStyle/bindAttribute (or
directly sets style/attribute) and push the effect's disposer to
this._cleanupFns; specifically, inside bindWidthPercent and
bindWidthPercentAttribute use effect(() => { const value = readable.get();
compute percent; call bindStyle/bindAttribute or set element.style.width /
element.setAttribute(...) accordingly }) and ensure the effect's returned
cleanup function is stored in this._cleanupFns so updates and teardown work
correctly.
- Around line 372-390: bindToggleClass builds a fake Readable by only supplying
get(), so bindProperty never sees updates; change it to subscribe to the
original readable and provide a real reactive source (or use the existing effect
helper) so updates re-run: inside bindToggleClass (referencing bindProperty and
the readable parameter) create/return a proper Readable that implements
subscribe/ get (or call bindEffect/ effect with readable.subscribe) and in the
subscriber recompute className based on readable.get(), then push the resulting
dispose into this._cleanupFns so the class toggles reactively when readable
changes.
In `@Build/src/primitives/spinner.ts`:
- Around line 74-83: The label setter currently calls _setupLabelBinding() which
can run before _labelElement exists and again after render, causing fragile
behavior and potential duplicate bindings when render() runs later (or when
label is set after render and render() is called again); stop calling
_setupLabelBinding() from the setter and instead perform binding only inside
render() (or make _setupLabelBinding() no-op if _bound flag is set), ensure
_setupLabelBinding() checks for this._labelElement and a
_labelBindingEstablished (or unsubscribes before rebinding) to avoid duplicate
subscriptions, and update references in the setter, _setupLabelBinding, and
render methods to use that single guarded binding path (functions: the label
setter, _setupLabelBinding, render, and the _labelElement/_labelSignal fields).
In `@Docs/docs/api-reference.md`:
- Line 11: The markdown contains a broken internal link to
"/Sakko/language-reference/" in the note that mentions the external package
`@nisoku/sakko`; replace or remove that internal link by updating the snippet
that reads "**Note:** The Sakko parser is now in the separate
[`@nisoku/sakko`](https://github.com/nisoku/sakko) package. For the full Sakko
syntax, see the [Sakko Language Reference](/Sakko/language-reference/)." —
either remove the second sentence entirely or change the bracketed link target
to a valid external URL (e.g., https://github.com/nisoku/sakko) so the anchor
points to the external Sakko docs, and apply the same change across the other
files that contain the identical reference.
---
Outside diff comments:
In `@Build/src/primitives/base.ts`:
- Around line 264-276: The _flush method currently only compares template to
this._lastTemplate so style-only changes are ignored; update the identity check
to consider both styles and template (e.g., compute a combined key or store
lastStyles and lastTemplate) and only skip when both are unchanged, ensure you
update the stored identity after successful render (references: _flush,
this._lastTemplate, styles, template, shadow.innerHTML) so style changes cause
shadow.innerHTML to be updated.
---
Nitpick comments:
In `@Build/src/primitives/input.ts`:
- Around line 108-110: The check using `'set' in this._valueSignal!` is fragile;
replace it with Sairin's type guard by calling isSignal(this._valueSignal) (and
ensure this._valueSignal is non-null) before casting to Signal<string> and
calling set(target.value); update the branch around _valueSignal so it uses
isSignal(this._valueSignal) to detect a signal and then (this._valueSignal as
Signal<string>).set(target.value).
- Around line 106-114: The input handler registration currently attaches
directly with input.addEventListener and this.onCleanup when this._valueSignal
is set, bypassing the component handler registry used elsewhere; change the
signal-path to register the same handler via the component's addHandler (the
same API used in the non-signal branch) instead of raw addEventListener and
remove the manual this.onCleanup removal, or if intentional, add a comment
explaining the divergence; update references to the created handler variable
(handler) and ensure the dispatched event still uses
this.dispatchEventTyped("input", { value: target.value }) so behavior remains
identical to the non-signal path.
In `@Build/src/primitives/spinner.ts`:
- Line 121: The labelText fallback is circular because it calls this.label
(which itself references _labelSignal), so replace the chain to use only
internal storage: compute labelText by checking _labelSignal and using its
.get() when present, otherwise read the backing field (_label or (this as
any)._label) and finally default to "Loading..."; update the expression in
spinner.ts (labelText) to avoid calling the label getter (use this._labelSignal
? this._labelSignal.get() : ((this as any)._label ?? "Loading...")) so there is
no circular dependency with the label getter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8982f770-ef6a-4627-afc3-e6d41fa88e06
📒 Files selected for processing (7)
Build/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/input.tsBuild/src/primitives/select.tsBuild/src/primitives/spinner.tsBuild/src/primitives/text.tsDocs/docs/api-reference.md
✅ Files skipped from review due to trivial changes (1)
- Build/src/primitives/text.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- Build/src/primitives/select.ts
There was a problem hiding this comment.
Actionable comments posted: 4
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/label.ts (1)
66-84:⚠️ Potential issue | 🟡 MinorText node created on every
render()call without cleanup.Line 71 unconditionally creates a new
Textnode on eachrender()call. Ifrender()is invoked multiple times (e.g., due to attribute changes), this creates duplicate Text nodes without removing the previous ones. The same issue exists intext.ts(context snippet line 78).🛠️ Proposed fix: Guard against duplicate text nodes
render() { this.mount(STYLES, `<label><slot></slot></label>`); const label = this.shadow.querySelector("label"); if (label) { + if (!this._textNode) { this._textNode = document.createTextNode(""); label.prepend(this._textNode); + } if (this._contentSignal) { this._setupSignalBinding(); } else { this._setTextContent(this._content as string); } if (this.hasAttribute("for")) { label.setAttribute("for", this.getAttribute("for") || ""); } } }🤖 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 66 - 84, In render() of the Label component, avoid creating a new Text node every time: check for an existing this._textNode and reuse it (or remove existing text nodes from the label) before creating one; modify the block in render() that currently does this._textNode = document.createTextNode("") so it only creates when this._textNode is null/undefined (and ensure you attach it once), then keep the existing logic that calls _setupSignalBinding() or _setTextContent(this._content) and preserve the "for" attribute handling; apply the same guard pattern used here to the analogous code in text.ts to prevent duplicate text nodes.
♻️ Duplicate comments (1)
Build/src/primitives/base.ts (1)
294-298:⚠️ Potential issue | 🟡 MinorSilent failure when
targetis"value"but element is not an input/textarea.When
targetis"value"and the element is not anHTMLInputElementorHTMLTextAreaElement, no binding is created and no error is logged. This makes debugging difficult. Consider adding an else branch to log a warning or handleHTMLSelectElement.🛠️ Proposed fix
case "value": if (element instanceof HTMLInputElement || element instanceof HTMLTextAreaElement) { dispose = bindInputValue(element, readable as Signal<string>); + } else if (element instanceof HTMLSelectElement) { + // TODO: Add bindSelectValue support or use bindAttribute + dispose = bindAttribute(element, "value", readable); + } else { + bindingError(`Cannot bind "value" to element type: ${element.constructor.name}`, {}); } break;🤖 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 294 - 298, The case for target "value" in the switch (when handling element in the function that calls bindInputValue) silently does nothing if element is not an HTMLInputElement or HTMLTextAreaElement; add an else branch to either handle HTMLSelectElement (e.g., call a bindSelectValue helper) or at minimum call the logger/warn function to surface the mismatch (include element.tagName and the target "value" in the message); update the switch branch referencing bindInputValue and element to ensure non-input elements are explicitly handled or warned about.
🧹 Nitpick comments (3)
Build/src/primitives/button.ts (1)
96-101:_getIsDisabled()implementation is correct but usesanycast.The method properly checks signal, value, attribute, and loading state. The
(this as any).loadingcast works becauseloadingis a reflected property, but consider adding adeclare loading: booleanfor type safety.💡 Optional improvement for type safety
Add at line 67:
declare loading: boolean;Then change line 100 to:
return !!this.loading;🤖 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 96 - 101, The method _getIsDisabled currently uses an any cast to access loading; add a type declaration for the reflected property by adding "declare loading: boolean;" to the class (near other declarations, e.g., around line 67) and then replace the final check in _getIsDisabled to return !!this.loading instead of !!(this as any).loading so the code is type-safe while preserving behavior.Build/tests/reactive-components.test.ts (1)
564-566: Arbitrary timeout in resource test may cause flakiness in CI.Using
setTimeout(resolve, 100)to wait for async resource resolution is fragile. If the test environment is slow, this may fail intermittently.💡 Suggested approach: Poll for expected state
- await new Promise(resolve => setTimeout(resolve, 100)); - await Promise.resolve(); - - expect(el.shadowRoot?.textContent).toContain('John'); + // Poll until content appears or timeout + const maxWait = 1000; + const start = Date.now(); + while (Date.now() - start < maxWait) { + await new Promise(resolve => setTimeout(resolve, 10)); + if (el.shadowRoot?.textContent?.includes('John')) break; + } + expect(el.shadowRoot?.textContent).toContain('John');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 564 - 566, The test uses an arbitrary sleep (setTimeout(resolve, 100)) after appending the element (el) which is flaky; replace this with a polling/wait-for pattern that repeatedly checks the expected resolved state of the element/DOM (for example inspect el's textContent, a specific attribute, or a resolver flag) until it matches or a sensible timeout is reached. Remove the setTimeout call and implement a small-interval retry loop (or use an existing test helper like waitFor) that polls the condition related to el and only resolves when the condition is true, failing after the max timeout to avoid flaky CI.Build/src/primitives/select.ts (1)
274-274: Type cast(this.dispatchEventTyped as any)suggests event config mismatch.The cast to
anybefore callingdispatchEventTypedindicates the{ value: newValue }payload doesn't match the inferred type fromselectConfig.events.change.detail. The config at line 101 declaresdetail: { value: "value" }, which maps to a property type, not the actual value.Consider fixing the event config or using the untyped
dispatch()method instead.💡 Option: Use dispatch() for simpler typing
- (this.dispatchEventTyped as any)("change", { value: newValue }); + this.dispatch("saz-change", { value: newValue });Also applies to: 302-302
🤖 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 274, The cast on dispatchEventTyped hides a typing mismatch between the emitted payload and the declared event detail: update the event typing or use the untyped dispatcher; specifically either (A) fix selectConfig.events.change.detail so its type describes the actual payload (make detail accept a { value: <actual value type> } rather than a property-mapping) so dispatchEventTyped can be called without casting, or (B) remove the cast and call the untyped dispatch() method to emit { value: newValue }; locate references to dispatchEventTyped and selectConfig.events.change.detail (and the other similar call at the other occurrence) and apply one of these two fixes.
🤖 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/image.ts`:
- Around line 72-77: The _setupSrcBinding function creates a new binding each
time it's called (from the src setter and render) without disposing the previous
binding; modify _setupSrcBinding to track the previous binding's dispose
function (e.g., a private field like _srcDispose or _srcBindingDispose), call
and clear that dispose before calling bindProperty again, then assign the new
dispose to that field and register it with onCleanup; also ensure any existing
onCleanup registration is not duplicated (clear the stored dispose on cleanup)
so repeated calls to _setupSrcBinding won't leak bindings.
In `@Build/src/primitives/label.ts`:
- Around line 58-64: The code creates a new binding every time
_setupSignalBinding() is called (via the content setter and render) without
disposing the previous one; modify the class to track the current binding's
dispose function (e.g., a private field like _contentDispose), and in
_setupSignalBinding() call and before registering a new bindText(textNode,
this._contentSignal!) dispose the existing _contentDispose if present, then
store the new dispose and register it with onCleanup (or simply rely on
_contentDispose for later disposal); update the content setter and render flow
to reuse this behavior so only one active subscription to _contentSignal exists
at a time.
In `@Build/src/primitives/select.ts`:
- Around line 159-182: The disabled setter leaks bindings because it assigns a
new Readable to _disabledSignal and calls bindDisabled without disposing the
previous binding; modify the setter to dispose any existing disabled binding
before creating a new one (e.g., track the disposer returned by bindDisabled in
a field like _disposeDisabledBinding, call it when replacing the Readable or
when switching back to a boolean), and also ensure that when setting a plain
boolean path (_setDisabled) you dispose any existing binding and clear
_disabledSignal; update references in the disabled setter, _setDisabled, and
wherever bindDisabled is called to use this disposer field.
- Around line 146-157: The _setupValueBinding function and the value binding in
render create new effects each time without disposing previous ones; add a field
(e.g. _valueEffectDisposer) to the class and in _setupValueBinding check and
call the existing disposer before creating a new effect, store the new disposer
returned by effect into _valueEffectDisposer, and still register a cleanup via
onCleanup that clears/ calls that disposer when the component is torn down;
apply the same pattern for the analogous input binding (referencing
_valueSignal, _setupValueBinding, the value setter, and onCleanup/effect) so
only one active effect exists at a time.
---
Outside diff comments:
In `@Build/src/primitives/label.ts`:
- Around line 66-84: In render() of the Label component, avoid creating a new
Text node every time: check for an existing this._textNode and reuse it (or
remove existing text nodes from the label) before creating one; modify the block
in render() that currently does this._textNode = document.createTextNode("") so
it only creates when this._textNode is null/undefined (and ensure you attach it
once), then keep the existing logic that calls _setupSignalBinding() or
_setTextContent(this._content) and preserve the "for" attribute handling; apply
the same guard pattern used here to the analogous code in text.ts to prevent
duplicate text nodes.
---
Duplicate comments:
In `@Build/src/primitives/base.ts`:
- Around line 294-298: The case for target "value" in the switch (when handling
element in the function that calls bindInputValue) silently does nothing if
element is not an HTMLInputElement or HTMLTextAreaElement; add an else branch to
either handle HTMLSelectElement (e.g., call a bindSelectValue helper) or at
minimum call the logger/warn function to surface the mismatch (include
element.tagName and the target "value" in the message); update the switch branch
referencing bindInputValue and element to ensure non-input elements are
explicitly handled or warned about.
---
Nitpick comments:
In `@Build/src/primitives/button.ts`:
- Around line 96-101: The method _getIsDisabled currently uses an any cast to
access loading; add a type declaration for the reflected property by adding
"declare loading: boolean;" to the class (near other declarations, e.g., around
line 67) and then replace the final check in _getIsDisabled to return
!!this.loading instead of !!(this as any).loading so the code is type-safe while
preserving behavior.
In `@Build/src/primitives/select.ts`:
- Line 274: The cast on dispatchEventTyped hides a typing mismatch between the
emitted payload and the declared event detail: update the event typing or use
the untyped dispatcher; specifically either (A) fix
selectConfig.events.change.detail so its type describes the actual payload (make
detail accept a { value: <actual value type> } rather than a property-mapping)
so dispatchEventTyped can be called without casting, or (B) remove the cast and
call the untyped dispatch() method to emit { value: newValue }; locate
references to dispatchEventTyped and selectConfig.events.change.detail (and the
other similar call at the other occurrence) and apply one of these two fixes.
In `@Build/tests/reactive-components.test.ts`:
- Around line 564-566: The test uses an arbitrary sleep (setTimeout(resolve,
100)) after appending the element (el) which is flaky; replace this with a
polling/wait-for pattern that repeatedly checks the expected resolved state of
the element/DOM (for example inspect el's textContent, a specific attribute, or
a resolver flag) until it matches or a sensible timeout is reached. Remove the
setTimeout call and implement a small-interval retry loop (or use an existing
test helper like waitFor) that polls the condition related to el and only
resolves when the condition is true, failing after the max timeout to avoid
flaky CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87bb7bdb-06ba-4e42-85e5-bca497836b60
📒 Files selected for processing (18)
Build/src/config.tsBuild/src/primitives/badge.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/heading.tsBuild/src/primitives/image.tsBuild/src/primitives/label.tsBuild/src/primitives/select.tsBuild/src/primitives/tag.tsBuild/tests/accessibility.test.tsBuild/tests/curvomorphism.test.tsBuild/tests/errors.test.tsBuild/tests/generic.test.tsBuild/tests/integration.test.tsBuild/tests/modifiers.test.tsBuild/tests/pipeline.test.tsBuild/tests/primitives.test.tsBuild/tests/reactive-components.test.ts
✅ Files skipped from review due to trivial changes (6)
- Build/tests/curvomorphism.test.ts
- Build/tests/primitives.test.ts
- Build/tests/pipeline.test.ts
- Build/tests/generic.test.ts
- Build/tests/accessibility.test.ts
- Build/tests/modifiers.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- Build/src/primitives/badge.ts
- Build/tests/errors.test.ts
- Build/src/primitives/heading.ts
- Build/src/primitives/tag.ts
added 2 commits
April 1, 2026 22:47
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Build/src/runtime/transformer.ts (1)
81-92:⚠️ Potential issue | 🟡 MinorInconsistent value handling between
childrenandprops.icon.Line 82 serializes
node.valuefor use inchildren, but line 86 passes the rawnode.valuedirectly toprops.icon. Ifnode.valueis an interpolated object (not a string), this would pass an object toprops.icon, which expects a string orReadable<string>based on the icon component's setter (seeBuild/src/primitives/icon.ts:59-75).Suggested fix for consistency
// For icon components, pass the value as an "icon" attribute // so connectedCallback can read it reliably. if (ICON_COMPONENTS.has(tag) && node.value && !props.icon) { - props.icon = node.value; + props.icon = value; // Use the already-serialized value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/runtime/transformer.ts` around lines 81 - 92, The transformer currently serializes node.value for children but assigns the raw node.value to props.icon when tag is in ICON_COMPONENTS, causing type inconsistency; update the ICON_COMPONENTS branch in Build/src/runtime/transformer.ts to use serializeValue(node.value) (or otherwise convert to the same string/Readable<string> form) before assigning to props.icon, ensuring props.icon and children receive consistent, serialized values and match the icon component's setter expectations.
♻️ Duplicate comments (1)
Build/src/primitives/radio.ts (1)
180-193:⚠️ Potential issue | 🟠 MajorRead-only signal-backed radios can still lose or diverge from their binding.
The sibling path only preserves writable signals. A sibling backed by a non-settable readable still falls through to
(el as any).checked = false, which clears_checkedSignalin its setter, and the self path below has the same problem with_setChecked(true). Signal-backed radios should stay controlled unless the readable is actually writable.🤖 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 180 - 193, The issue is that radios backed by read-only signals are being mutated via DOM .checked assignments or _setChecked, which clears/diverges the binding; update the sibling loop and the self handling to only mutate when the signal is actually writable: for siblingSignal and this._checkedSignal check for the presence of a "set" method (as done already) and if absent do nothing (do not fall back to (el as any).checked = false or this._setChecked(true)). In other words, only call (siblingSignal as Signal<boolean>).set(false) or (this._checkedSignal as Signal<boolean>).set(true) when "set" in the signal; otherwise skip DOM/property writes so readable-only signals remain controlled.
🧹 Nitpick comments (2)
Build/src/primitives/card.ts (1)
54-54:_contentSlotis cached but appears unused.Line 89 caches a reference to the
<slot>element in_contentSlot, but this field is never read anywhere in the component. If this is preparation for future functionality, consider adding a TODO comment; otherwise, it could be removed to avoid confusion.Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/card.ts` at line 54, The private field _contentSlot on the Card component is assigned (cached from the <slot> at runtime) but never read; either remove the unused _contentSlot declaration and its assignment to avoid dead state, or keep it and add a short TODO comment explaining intended future use so reviewers understand it's deliberately reserved (update the constructor/connectedCallback code that sets _contentSlot and the field declaration accordingly). Ensure references use the existing symbol name _contentSlot and the slot assignment site in the Card component.Build/tests/reactive-components.test.ts (1)
178-193: Add avatar regression coverage for empty/non-emptysrctransitions.This only exercises
user1.jpg -> user2.jpg. The avatar implementation now has separate image and initials modes, so please add at least'' -> 'user.jpg'and'user.jpg' -> ''cases; otherwise the mode-switch bug in this PR will slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 178 - 193, Add two assertions/tests to cover avatar mode transitions: one that starts with src signal '' then sets it to 'user.jpg' ('' -> 'user.jpg') and one that starts with 'user.jpg' then sets it to '' ('user.jpg' -> '') using the same pattern as the existing test for 'user1.jpg' -> 'user2.jpg'; create the element (saz-avatar), bind the Signal<string> (src) to el.src, append to document, await microtask, then assert the presence/absence of the <img> in el.shadowRoot and the presence/absence of initials/text to verify the component switches between image and initials modes (use the existing src variable name and test name 'saz-avatar binds Signal<string> to src' to locate and extend the test).
🤖 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/avatar.ts`:
- Around line 64-80: The setter for src can leave the component in the wrong
render mode because it only updates the branch that existed on first render (see
src setter, _isReadableStr, _imgElement, _initialsElement); when the truthiness
of currentSrc flips you must re-render or ensure both DOM nodes exist. Fix by
detecting a mode flip (compare !!previousSrcReadable vs !!newSrcReadable inside
the src setter and related code paths) and call render() when the
readable-vs-initials state changes, or alternatively always create/mount both
_imgElement and _initialsElement up front so _setupSrcBinding and _updateDisplay
work regardless of initial branch. Ensure the same change is applied to the
other similar blocks referenced (around lines 87-106, 108-135, 137-155).
- Around line 64-80: The setter for src currently calls _setupSrcBinding()
without disposing prior bindings, so add a stored disposer (e.g.
this._srcDisposer) and call it before creating a new binding and before
switching from a signal to a plain string; modify _setupSrcBinding to
return/assign the disposer (cleanup from bindProperty/effect) and ensure the
setter calls this._srcDisposer?.() before assigning a new signal or calling
_setupSrcBinding(), and also clear that disposer when the component disconnects
or when setting _srcSignal to null so old subscriptions cannot still mutate the
<img>; update references in set src and in the similar branch around lines
87-106 to use this disposer.
In `@Build/src/primitives/base.ts`:
- Around line 426-452: The current bindToggleClass in bindToggleClass uses
string operations and current.includes(className) which can falsely match
substrings (e.g., "active" vs "inactive"); replace the className
string-manipulation with DOMTokenList methods on the element's classList: use
element.classList.contains(className) to check, element.classList.add(className)
when active, and element.classList.remove(className) when not active, leaving
the rest of the effect() subscription and _cleanupFns push unchanged.
In `@Build/src/primitives/button.ts`:
- Around line 104-108: The _getIsDisabled() implementation returns early when
_disabledSignal exists, so a readable that resolves to false won't fall through
to the loading check — make the method always incorporate the loading state into
the final result. Update _getIsDisabled (referencing _getIsDisabled,
_disabledSignal, _disabledValue, hasAttribute, and loading) to compute the base
disabled value from the signal/value/attribute and then OR that with
!!this.loading before returning, so loading disables the button regardless of
where disabled came from.
In `@Build/src/primitives/coverart.ts`:
- Around line 108-117: The effect created in render() for the signal-bound src
can accumulate on re-renders because each render() call registers a new effect
via effect() and onCleanup(), so change render() to track and reuse/teardown a
single disposer: add a private field (e.g., _srcEffectDispose), before creating
a new effect call and clear any existing disposer (invoke it and set to
undefined), then create the new effect and store its disposer in
_srcEffectDispose; update onCleanup()/disconnected cleanup to invoke
_srcEffectDispose and clear it so no duplicate effect subscriptions remain;
reference symbols: render(), _srcSignal, _imgElement, effect(), onCleanup(), and
disconnectedCallback.
In `@Build/src/primitives/icon.ts`:
- Around line 59-67: The icon setter currently mutates _iconElement.innerHTML
and starts new effects without disposing old ones; instead detect when the
incoming value type (SVG string vs text) requires a different root element and
replace the DOM node rather than patching innerHTML, and when creating a
reactive binding in _setupIconBinding() store and call a cleanup/disposer (e.g.,
this._iconEffectCleanup) before creating a new effect so previous subscriptions
are torn down; update _updateIcon() to create the correct element type (svg or
span), replace the existing this._iconElement via replaceWith/replaceChild, and
ensure _iconElement reference is updated accordingly.
In `@Build/src/primitives/image.ts`:
- Around line 50-64: The setter for src currently bails when this._imgElement is
null so late async updates get dropped; fix it by ensuring an <img> is created
when receiving a non-readable src and no _imgElement exists: in the set src
block (and the similar branches at the other locations) if this._imgElement is
null, create/attach the image element (the same initialization the first render
does), assign it to this._imgElement, and then run this._updateSrc(value) or
this._setupSrcBinding() as appropriate; keep using this._pendingSrc for storage,
clear _srcDispose if present, and reuse the existing helper methods (_updateSrc,
_setupSrcBinding, _imgElement, _pendingSrc) so async updates produce the element
instead of being no-ops.
In `@Build/src/primitives/input.ts`:
- Around line 82-83: The public getter get value() is out of sync with
attributeChangedCallback("value") because it only reads _value/_valueSignal and
ignores the actual DOM attribute or inner <input>, so setAttribute("value", ...)
can leave the visible input inconsistent; update the getter (get value()) to
fall back to the underlying input element or attribute when internal state is
empty (e.g., check this._valueSignal || (this as any)._value ||
this._input?.value || this.getAttribute('value') || ""), and/or modify
attributeChangedCallback("value") to update the internal backing field(s)
(_value and any signal) when the attribute changes so that attribute updates and
the visible <input> remain in sync; apply the same fix to the other value
accessor(s) referenced in the diff (the second getter around lines 149-155).
- Around line 69-80: The value setter and render signal-mode leak effects and
DOM listeners: before switching out of signal-mode or installing a new signal
effect in render(), explicitly dispose the prior effect and remove the raw DOM
listener that were created for this._valueSignal. Concretely, track and store
the effect disposer and the event listener reference when you call effect() and
addEventListener() (e.g., in fields like _valueSignalDisposer and
_valueListener), call those disposers and removeEventListener(...) before
setting this._valueSignal = null in the value setter and before creating a new
effect in render(); also ensure removeAllHandlers() does not replace the raw
listener removal so that the previously attached input event listener is removed
immediately rather than waiting for disconnectedCallback().
In `@Build/src/primitives/progress.ts`:
- Around line 158-162: The re-render creates new reactive bindings without
disposing prior ones, leaving old effects in _cleanupFns operating on detached
elements; update _setupValueBinding (and where render()/attributeChangedCallback
triggers it) to dispose the previous binding before creating a new one: when
_setupValueBinding is about to call bindWidthPercent (which pushes a cleanup
into _cleanupFns) first check for and call any existing specific cleanup for the
bindWidthPercent binding (or pop/shift and run the last binding), remove it from
_cleanupFns, then create and store the new binding; follow the same per-binding
dispose pattern used in badge.ts so stale effects are cleaned up between renders
(disconnectedCallback should remain for final cleanup).
In `@Build/src/primitives/radio.ts`:
- Around line 128-132: The _getIsChecked() method currently ignores the host
attribute path; update it so when there is no _checkedSignal it first returns
true if the host element has the "checked" attribute
(this.hasAttribute("checked")), otherwise fall back to the existing (this as
any)._checked value—i.e., modify _getIsChecked to check hasAttribute("checked")
before using _checked to preserve declarative/attribute-set radios' checked
state.
In `@Build/src/primitives/select.ts`:
- Around line 182-191: The effect watching disabled currently calls
this._wireHandlers(), causing duplicate event listeners because
attributeChangedCallback/_setDisabled() also rewires handlers; remove the
this._wireHandlers() call from the effect body (leave this._updateTabIndex()),
and rely on attributeChangedCallback/_setDisabled() to perform handler wiring so
handlers are only registered once; update any analogous effect block at lines
~204-212 the same way.
- Around line 139-152: The setter currently stores non-signal values on (this as
any)._value while render() reads getAttribute("value"), causing inconsistent
state; unify non-signal value resolution by adding/using a single accessor
(e.g., a helper like _resolveValue or _getNonSignalValue) and have all places
(the value setter, render(), _navigateOption(), _updateSelectedState(), and
_updateDisplay()) read from that accessor instead of mixing (this as any)._value
and getAttribute("value"); update the value setter to set the shared storage and
update render to prefer the helper, and make the three update/navigation methods
call the same helper so pre-render assignments (el.value = ...) are respected
and state stays consistent.
- Around line 301-305: The code currently falls back to this.value = newValue
when this._valueSignal exists but has no set, which triggers the value setter
that nulls _valueSignal and breaks the reactive binding; instead, detect the
readable-but-not-writable case (this._valueSignal exists && !("set" in
this._valueSignal)) and update the internal backing value directly (e.g., the
private raw/internal value field used by the getter) without invoking the public
setter, or provide a proper writable replacement for the signal; apply this
change for both the write site around _valueSignal check (the block using
Signal<string>.set) and the analogous block at lines 333-337 so the setter is
never called when the signal is read-only.
In `@Build/src/primitives/switch.ts`:
- Around line 175-184: The click handler currently falls back to local state
when this._checkedSignal is read-only, causing a mismatch between the emitted
change and the actual reactive source; change the logic so that when
this._checkedSignal exists but does not have a "set" method you do NOT call
_setChecked(newValue) and you DO NOT dispatchEventTyped("change", ...) (i.e.,
early-return after computing newValue or optionally log/warn), while preserving
the existing behavior when _checkedSignal has "set"; refer to symbols
_checkedSignal, _setChecked, _getIsDisabled, dispatchEventTyped, _updateAria and
the checked getter to locate and update the code.
In `@Build/src/primitives/toggle.ts`:
- Around line 138-147: The click/toggle handler currently flips state even when
_checkedSignal is a read-only signal; update it so that if this._checkedSignal
exists but does not expose a set method you DO NOT call this._setChecked or
mutate local/attribute/ARIA state — compute newValue as you do now,
dispatchEventTyped("change", { checked: newValue }) so the host can control the
value, and return; keep the existing branch that calls (this._checkedSignal as
Signal<boolean>).set(newValue) when set exists and the fallback to
this._setChecked(newValue) only when no _checkedSignal is present. Ensure
this._getIsDisabled() is still respected.
---
Outside diff comments:
In `@Build/src/runtime/transformer.ts`:
- Around line 81-92: The transformer currently serializes node.value for
children but assigns the raw node.value to props.icon when tag is in
ICON_COMPONENTS, causing type inconsistency; update the ICON_COMPONENTS branch
in Build/src/runtime/transformer.ts to use serializeValue(node.value) (or
otherwise convert to the same string/Readable<string> form) before assigning to
props.icon, ensuring props.icon and children receive consistent, serialized
values and match the icon component's setter expectations.
---
Duplicate comments:
In `@Build/src/primitives/radio.ts`:
- Around line 180-193: The issue is that radios backed by read-only signals are
being mutated via DOM .checked assignments or _setChecked, which clears/diverges
the binding; update the sibling loop and the self handling to only mutate when
the signal is actually writable: for siblingSignal and this._checkedSignal check
for the presence of a "set" method (as done already) and if absent do nothing
(do not fall back to (el as any).checked = false or this._setChecked(true)). In
other words, only call (siblingSignal as Signal<boolean>).set(false) or
(this._checkedSignal as Signal<boolean>).set(true) when "set" in the signal;
otherwise skip DOM/property writes so readable-only signals remain controlled.
---
Nitpick comments:
In `@Build/src/primitives/card.ts`:
- Line 54: The private field _contentSlot on the Card component is assigned
(cached from the <slot> at runtime) but never read; either remove the unused
_contentSlot declaration and its assignment to avoid dead state, or keep it and
add a short TODO comment explaining intended future use so reviewers understand
it's deliberately reserved (update the constructor/connectedCallback code that
sets _contentSlot and the field declaration accordingly). Ensure references use
the existing symbol name _contentSlot and the slot assignment site in the Card
component.
In `@Build/tests/reactive-components.test.ts`:
- Around line 178-193: Add two assertions/tests to cover avatar mode
transitions: one that starts with src signal '' then sets it to 'user.jpg' (''
-> 'user.jpg') and one that starts with 'user.jpg' then sets it to ''
('user.jpg' -> '') using the same pattern as the existing test for 'user1.jpg'
-> 'user2.jpg'; create the element (saz-avatar), bind the Signal<string> (src)
to el.src, append to document, await microtask, then assert the presence/absence
of the <img> in el.shadowRoot and the presence/absence of initials/text to
verify the component switches between image and initials modes (use the existing
src variable name and test name 'saz-avatar binds Signal<string> to src' to
locate and extend the test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c69278ff-66b8-4588-96ef-f3c3ad94b582
📒 Files selected for processing (30)
Build/src/curvomorphism/index.tsBuild/src/index.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/coverart.tsBuild/src/primitives/heading.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/label.tsBuild/src/primitives/modal.tsBuild/src/primitives/modifier-map.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/select.tsBuild/src/primitives/slider.tsBuild/src/primitives/spinner.tsBuild/src/primitives/switch.tsBuild/src/primitives/tag.tsBuild/src/primitives/text.tsBuild/src/primitives/toggle.tsBuild/src/runtime/transformer.tsBuild/tests/reactive-components.test.tsDocs/docs/api-reference.mdDocs/docs/index.mdDocs/docs/primitives.md
✅ Files skipped from review due to trivial changes (3)
- Build/src/curvomorphism/index.ts
- Build/src/index.ts
- Docs/docs/primitives.md
🚧 Files skipped from review as they are similar to previous changes (9)
- Build/src/primitives/modifier-map.ts
- Build/src/primitives/modal.ts
- Docs/docs/index.md
- Build/src/primitives/checkbox.ts
- Build/src/primitives/text.ts
- Build/src/primitives/label.ts
- Build/src/primitives/tag.ts
- Build/src/primitives/heading.ts
- Build/src/primitives/slider.ts
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Build/src/primitives/switch.ts (1)
159-178:⚠️ Potential issue | 🟠 MajorResolved checked state drops the host attribute path.
_updateAria()and_handleClick()only consult_checkedSignal/_checked. A declarative<saz-switch checked>therefore looks on via CSS whilearia-checkedstays false, and the next click computes from an unchecked state. Add a_getIsChecked()helper likeradio.tsand use it everywhere.🤖 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 159 - 178, The component's checked state logic only reads _checkedSignal/_checked directly causing host attribute-driven checked (e.g., <saz-switch checked>) to be ignored; add a _getIsChecked() helper (modeled after radio.ts) that computes checked by prioritizing _checkedSignal.get(), then falling back to (this as any)._checked or the presence of the host attribute, and replace direct reads in _updateAria() and _handleClick() with _getIsChecked() so aria-checked, click toggling, and CSS-driven checked state stay in sync.Build/src/primitives/radio.ts (1)
166-195:⚠️ Potential issue | 🟠 MajorGuard the current radio before mutating the group.
If this radio's bound checked source is read-only, the click path can still touch sibling state and emit
saz-change, even though this instance never becomes checked. Bail out before the group update when the current source has noset.🤖 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 166 - 195, The click handler _handleClick must bail before mutating sibling radios when this radio's bound checked source is read-only: before querying siblings, check the instance's _checkedSignal and if it exists but does not have a "set" method (read-only), return early so we don't change group state, call _setChecked, update aria, or emit saz-change; keep the current fallback behavior where if _checkedSignal is undefined we proceed and use _setChecked, and otherwise when "set" exists proceed to update siblings and set true via the signal, then call _updateAria and dispatchEventTyped("change", { value }).
♻️ Duplicate comments (4)
Build/src/primitives/icon.ts (1)
81-90:⚠️ Potential issue | 🟠 MajorReplace the cached icon root when the representation changes.
_iconElementcan be either<svg>or<span>, but both update paths patchinnerHTMLin place. Aknown -> unknowntransition writes span markup into an existing SVG node, and the reverse transition leaves SVG markup under a span. Recreate/replace the root node instead of mutating the cached one in place.Also applies to: 99-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon.ts` around lines 81 - 90, The _updateIcon method currently mutates the cached DOM node (this._iconElement) by setting innerHTML, which breaks when switching between SVG and span representations; instead, when the new representation type differs from the current node tag, create a new element of the correct type (an SVG element or a span), set its contents (SVG string or escaped text), replace the old node in the DOM, and update this._iconElement to point to the new node; apply the same replacement logic to the analogous update code block around lines 99-109 to ensure all transitions recreate the root node rather than patching innerHTML in place.Build/src/primitives/input.ts (1)
70-80:⚠️ Potential issue | 🟠 MajorReactive value mode is never initialized or torn down correctly.
The setter only stores
_valueSignal, so assigning a reactive source after mount does nothing until some unrelated rerender happens. On top of that, every reactive render adds a fresheffect()and rawinputlistener, and switching back to a string never disposes them. Because the effect readsthis._valueSignal!directly, an old reaction can hitnullafter leaving reactive mode.Also applies to: 103-127
🤖 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 70 - 80, The setter for value currently only assigns _valueSignal and never initializes or tears down reactive mode, which leads to stale effects and listeners; update the value setter in the Input class so that when assigning a Readable<string> it: disposes any existing effect/listener (track disposers on the instance, e.g. _disposeEffect/_removeInputListener), creates a new effect() that reads from the new _valueSignal and updates this._input.value safely, and registers a single input event listener that updates (this as any)._value; when switching back to a plain string dispose the reactive effect/listener and write the string to (this as any)._value and this._input.value. Ensure the effect never dereferences _valueSignal after it has been set to null by capturing the signal locally inside the created effect so disposers cleanly stop old reactions.Build/src/primitives/switch.ts (1)
174-186:⚠️ Potential issue | 🟠 MajorDon't emit
changewhen the bound checked source can't be written.If
_checkedSignalhas noset, this branch leaves the source untouched but still dispatcheschangewithnewValue. That event reports a transition that never happened.🤖 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 174 - 186, The _handleClick method is emitting a "change" even when the bound source is read-only; change should only be dispatched when the component actually updates the source. Modify _handleClick so it computes newValue, then only applies and dispatches when the update actually occurs: if _checkedSignal exists and has a "set" call (use (this._checkedSignal as Signal<boolean>).set(newValue)) dispatch "change"; otherwise if no _checkedSignal use this._setChecked(newValue) and dispatch "change" (since _setChecked mutates internal state). In short: guard the dispatchEventTyped("change", ...) behind the branches that perform the write (check "set" in _checkedSignal or the _setChecked path) and do not dispatch for read-only _checkedSignal.Build/src/primitives/radio.ts (1)
176-184:⚠️ Potential issue | 🔴 CriticalRadio-group exclusivity is broken for non-reactive siblings.
This loop only clears siblings backed by writable sources. Plain radios are skipped entirely, so a named group can end up with multiple checked items. Keep the signal-aware branch, but also clear siblings that use local state/attributes.
🤖 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 176 - 184, The exclusivity loop only clears writable signals (_checkedSignal) and skips plain DOM/component radios, so multiple radios can remain checked; update the loop inside root.querySelectorAll(`saz-radio[name="${escapedName}"]`) in radio.ts to preserve the existing signal branch for (el as any)._checkedSignal but add a fallback that clears non-reactive siblings by removing their checked state via the DOM/component API (e.g., removeAttribute("checked") and set the element's checked property to false or call its public clear/uncheck method if present) so both Signal<boolean> backed radios and plain/local-state radios are un-checked when another radio is selected.
🧹 Nitpick comments (1)
Build/tests/reactive-components.test.ts (1)
178-234: Please add avatar regressions for the other setter branches.These tests only cover a single readable changing over time. They won't catch the plain-string image path or swapping
el.srcto a different empty readable, which go through different branches inSazamiAvatar.src.Suggested coverage
+ test('saz-avatar binds plain string src', async () => { + const el = document.createElement('saz-avatar') as any; + el.src = 'user.jpg'; + document.body.appendChild(el); + await Promise.resolve(); + expect(el.shadowRoot?.querySelector('img')?.src).toContain('user.jpg'); + el.remove(); + }); + + test('saz-avatar rebinds to a different empty signal', async () => { + const el = document.createElement('saz-avatar') as any; + const first = signal(path("test", "avatarFirst"), ''); + const second = signal(path("test", "avatarSecond"), ''); + + el.src = first; + document.body.appendChild(el); + await Promise.resolve(); + + el.src = second; + second.set('user.jpg'); + await Promise.resolve(); + + expect(el.shadowRoot?.querySelector('img')?.src).toContain('user.jpg'); + el.remove(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 178 - 234, Tests currently only exercise updating a single Signal value over time; add tests in Build/tests/reactive-components.test.ts to cover the other setter branches in SazamiAvatar.src: (1) assign a plain string path (el.src = 'user.jpg') and verify an <img> appears, (2) replace a Signal with a plain string (start with signal('user1.jpg') then set el.src = 'user2.jpg'), (3) swap el.src from one Signal to a different Signal that is empty (to exercise the "switch to empty readable" branch) and (4) swap from an empty Signal to a non-empty Signal via replacing the readable (to exercise the "switch from empty readable" branch). For each test create the element, attach to document, await an update tick, assert presence/absence of .initials or img.src, then cleanup with el.remove().
🤖 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/avatar.ts`:
- Around line 105-107: When nowImageMode is true and this._imgElement exists,
the current sequence calls this._disposeSrcBinding() then
this._setupSrcBinding(), but _setupSrcBinding() can return early for
non-readable/plain-string sources so render() never triggers _updateDisplay()
and the <img> mounts without a src; change the branch in render (and the
analogous branch around _imgElement at lines ~190-197) so that after calling
_disposeSrcBinding() and _setupSrcBinding() you detect the “no binding created”
case and either (a) call this._updateDisplay() immediately to set the
plain-string src, or (b) directly set this._imgElement.src = this._src (and
alt/title as needed), ensuring plain-string image paths hydrate the <img> even
when _setupSrcBinding() returns early.
- Around line 108-110: When rebinding to a new Readable the current branch
disposes the old _modeEffectDisposer and calls _updateDisplay(), but never
re-establishes a watcher on the new Readable so later changes never flip back to
image mode; instead of only disposing and calling _updateDisplay() when
nowImageMode is false and _initialsElement exists, re-create the source/mode
binding for the newly supplied Readable (i.e. call the same routine that sets up
_modeEffectDisposer / subscribes to the src signal — the counterpart to
_disposeSrcBinding()) so _modeEffectDisposer is assigned for the new Readable
and future truthy values will trigger _updateDisplay() and switch to the image
branch. Ensure the old disposer is still disposed before creating the new
binding.
In `@Build/src/primitives/base.ts`:
- Around line 316-343: The "value" binding is one-way for HTMLSelectElement
because the select branch uses bindAttribute(element, "value", readable) instead
of a two-way binder; change the select branch in the "value" case to perform
two-way binding when the source is writable: if element is HTMLSelectElement and
readable has "set" (i.e., is a Signal) call a new or existing
bindSelectValue(element, readable as Signal<string>) that mirrors the Signal
into the select and listens for change/input to write back (similar to
bindInputValue), otherwise fall back to bindAttribute for read-only Readable;
ensure you reuse the same writable-check logic used for
HTMLInputElement/HTMLTextAreaElement and keep bindingError/suggestion behavior
for read-only vs wrong element types.
- Around line 222-237: The mount() method becomes asynchronous after the first
render (it calls scheduleRender which defers DOM swap to _flush), causing
callers that immediately query or bind into the DOM (e.g., code using
this.$(...) or caching elements/listeners) to operate against the old tree;
either make mount() perform the DOM update synchronously for rerenders (perform
the same innerHTML swap/path used on first render instead of scheduleRender) or
introduce a new mountSync(...) API and update all call sites that expect
immediate DOM availability to call mountSync instead of mount; locate the logic
in mount, scheduleRender, and _flush and ensure the DOM swap
(this.shadow.innerHTML = ...) happens synchronously when callers need it, or add
mountSync and change callers that call this.$(...)/install bindings immediately
after mount to use mountSync.
In `@Build/src/primitives/image.ts`:
- Around line 77-80: The _updateSrc method leaves a stale <img> node when called
with an empty string; change _updateSrc (and the similar code block around the
other src-update logic) to treat empty/falsy values as teardown: if value is
falsy, clear the element and dispose resources by calling and clearing
_srcDispose, set _imgElement to null (and ensure _imgElement.src is not left
set), otherwise update src as before. Ensure this same disposal behavior is
applied in the other src-update branch that currently skips clearing
_imgElement/_srcDispose when !currentSrc.
In `@Build/src/primitives/progress.ts`:
- Around line 93-125: The aria-valuenow handling must use a clamped value and be
removed for indeterminate state: in _setupValueBinding() compute const range =
this._rangeMax - this._rangeMin and const clamped = range > 0 ?
Math.min(this._rangeMax, Math.max(this._rangeMin, val)) : undefined; then if
clamped !== undefined call this.setAttribute("aria-valuenow",
String(Math.round(clamped))) else call this.removeAttribute("aria-valuenow");
apply the same logic to the render path that currently writes Math.round(value)
(the indeterminate branch) so it does not set aria-valuenow when range <= 0 and
uses the same clamped value when setting the attribute; keep references to
_setupValueBinding, bindWidthPercent, and the render code that sets
aria-valuenow consistent.
---
Outside diff comments:
In `@Build/src/primitives/radio.ts`:
- Around line 166-195: The click handler _handleClick must bail before mutating
sibling radios when this radio's bound checked source is read-only: before
querying siblings, check the instance's _checkedSignal and if it exists but does
not have a "set" method (read-only), return early so we don't change group
state, call _setChecked, update aria, or emit saz-change; keep the current
fallback behavior where if _checkedSignal is undefined we proceed and use
_setChecked, and otherwise when "set" exists proceed to update siblings and set
true via the signal, then call _updateAria and dispatchEventTyped("change", {
value }).
In `@Build/src/primitives/switch.ts`:
- Around line 159-178: The component's checked state logic only reads
_checkedSignal/_checked directly causing host attribute-driven checked (e.g.,
<saz-switch checked>) to be ignored; add a _getIsChecked() helper (modeled after
radio.ts) that computes checked by prioritizing _checkedSignal.get(), then
falling back to (this as any)._checked or the presence of the host attribute,
and replace direct reads in _updateAria() and _handleClick() with
_getIsChecked() so aria-checked, click toggling, and CSS-driven checked state
stay in sync.
---
Duplicate comments:
In `@Build/src/primitives/icon.ts`:
- Around line 81-90: The _updateIcon method currently mutates the cached DOM
node (this._iconElement) by setting innerHTML, which breaks when switching
between SVG and span representations; instead, when the new representation type
differs from the current node tag, create a new element of the correct type (an
SVG element or a span), set its contents (SVG string or escaped text), replace
the old node in the DOM, and update this._iconElement to point to the new node;
apply the same replacement logic to the analogous update code block around lines
99-109 to ensure all transitions recreate the root node rather than patching
innerHTML in place.
In `@Build/src/primitives/input.ts`:
- Around line 70-80: The setter for value currently only assigns _valueSignal
and never initializes or tears down reactive mode, which leads to stale effects
and listeners; update the value setter in the Input class so that when assigning
a Readable<string> it: disposes any existing effect/listener (track disposers on
the instance, e.g. _disposeEffect/_removeInputListener), creates a new effect()
that reads from the new _valueSignal and updates this._input.value safely, and
registers a single input event listener that updates (this as any)._value; when
switching back to a plain string dispose the reactive effect/listener and write
the string to (this as any)._value and this._input.value. Ensure the effect
never dereferences _valueSignal after it has been set to null by capturing the
signal locally inside the created effect so disposers cleanly stop old
reactions.
In `@Build/src/primitives/radio.ts`:
- Around line 176-184: The exclusivity loop only clears writable signals
(_checkedSignal) and skips plain DOM/component radios, so multiple radios can
remain checked; update the loop inside
root.querySelectorAll(`saz-radio[name="${escapedName}"]`) in radio.ts to
preserve the existing signal branch for (el as any)._checkedSignal but add a
fallback that clears non-reactive siblings by removing their checked state via
the DOM/component API (e.g., removeAttribute("checked") and set the element's
checked property to false or call its public clear/uncheck method if present) so
both Signal<boolean> backed radios and plain/local-state radios are un-checked
when another radio is selected.
In `@Build/src/primitives/switch.ts`:
- Around line 174-186: The _handleClick method is emitting a "change" even when
the bound source is read-only; change should only be dispatched when the
component actually updates the source. Modify _handleClick so it computes
newValue, then only applies and dispatches when the update actually occurs: if
_checkedSignal exists and has a "set" call (use (this._checkedSignal as
Signal<boolean>).set(newValue)) dispatch "change"; otherwise if no
_checkedSignal use this._setChecked(newValue) and dispatch "change" (since
_setChecked mutates internal state). In short: guard the
dispatchEventTyped("change", ...) behind the branches that perform the write
(check "set" in _checkedSignal or the _setChecked path) and do not dispatch for
read-only _checkedSignal.
---
Nitpick comments:
In `@Build/tests/reactive-components.test.ts`:
- Around line 178-234: Tests currently only exercise updating a single Signal
value over time; add tests in Build/tests/reactive-components.test.ts to cover
the other setter branches in SazamiAvatar.src: (1) assign a plain string path
(el.src = 'user.jpg') and verify an <img> appears, (2) replace a Signal with a
plain string (start with signal('user1.jpg') then set el.src = 'user2.jpg'), (3)
swap el.src from one Signal to a different Signal that is empty (to exercise the
"switch to empty readable" branch) and (4) swap from an empty Signal to a
non-empty Signal via replacing the readable (to exercise the "switch from empty
readable" branch). For each test create the element, attach to document, await
an update tick, assert presence/absence of .initials or img.src, then cleanup
with el.remove().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d5f3a67-2192-4952-a93c-9f8ad28cf54d
📒 Files selected for processing (15)
Build/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/card.tsBuild/src/primitives/coverart.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/select.tsBuild/src/primitives/switch.tsBuild/src/primitives/toggle.tsBuild/src/runtime/transformer.tsBuild/tests/reactive-components.test.ts
✅ Files skipped from review due to trivial changes (1)
- Build/src/primitives/select.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- Build/src/runtime/transformer.ts
- Build/src/primitives/card.ts
- Build/src/primitives/toggle.ts
- Build/src/primitives/coverart.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/radio.ts (1)
176-194:⚠️ Potential issue | 🟡 MinorSibling signal update logic is improved but incomplete.
The code now correctly updates sibling signals via
set(false)when present, but it doesn't explicitly clear thecheckedattribute on siblings that don't have a signal-backed state. The previous code used(el as any).checked = falsewhich would go through the setter.🐛 Proposed fix: Handle non-signal siblings
root .querySelectorAll(`saz-radio[name="${escapedName}"]`) .forEach((el) => { if (el === this) return; const siblingSignal = (el as any)._checkedSignal; if (siblingSignal && "set" in siblingSignal) { (siblingSignal as Signal<boolean>).set(false); + } else { + // For non-signal siblings, update via internal method + (el as any)._setChecked?.(false); } });🤖 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 176 - 194, The sibling-update loop in root.querySelectorAll(`saz-radio[name="${escapedName}"]`) only calls set(false) on signal-backed siblings but neglects to clear the checked state on siblings without _checkedSignal; update the loop to, for each sibling element (el), if (el as any)._checkedSignal exists and has "set" call .set(false), otherwise assign (el as any).checked = false so the element's setter runs; keep the rest of the logic that sets this element's _checkedSignal (or calls _setChecked(true)) and then calls _updateAria().
♻️ Duplicate comments (9)
Build/src/primitives/image.ts (1)
111-116:⚠️ Potential issue | 🟠 MajorEmpty renders leave a detached
<img>registered as live state.The
!currentSrcbranch clears the shadow DOM and returns, but_imgElementand_srcDisposestay populated. After that, latersrcupdates take the fast path and mutate the detached node/binding instead of recreating the image, so the component can stay blank.🛠️ Minimal fix
render() { const currentSrc = this._getCurrentSrc(); if (!currentSrc) { + if (this._srcDispose) { + this._srcDispose(); + this._srcDispose = null; + } + this._imgElement = null; this.mount(STYLES, ""); return; }🤖 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 111 - 116, The render() method's !currentSrc branch clears the shadow DOM but leaves this._imgElement and this._srcDispose intact, so later src updates mutate a detached node; update the !currentSrc branch (in render and using _getCurrentSrc reference) to call and clear the disposal function (this._srcDispose) and null out this._imgElement and this._srcDispose (or set to undefined) before returning so the component will recreate the <img> and its bindings on subsequent src changes.Build/src/primitives/avatar.ts (2)
116-118:⚠️ Potential issue | 🟠 MajorRebinding to a new empty readable still drops the mode watcher.
The initials-mode setter branch disposes the old wiring and only calls
_updateDisplay(). If the newly assigned readable starts as"", nothing re-subscribes to it there, so later truthy emissions never flip back to image mode. The render branch already uses_setupSignalWatcher()for this case; the setter needs the same follow-up.Also applies to: 212-214
🤖 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 116 - 118, In the initials-mode setter branch (where it checks !nowImageMode && this._initialsElement), after calling this._disposeSrcBinding() and this._updateDisplay(), call this._setupSignalWatcher() so the new readable is re-subscribed when it starts as an empty string; do the same fix for the other identical branch around the _updateDisplay() call (the one referenced at lines 212-214) to ensure future truthy emissions flip back to image mode.
113-115:⚠️ Potential issue | 🔴 CriticalThe plain-string image path still never hydrates the
<img>.In image mode,
render()mounts<img class="image" alt="" />and only calls_setupSrcBinding()for signal sources. The setter takes the same_setupSrcBinding()path after disposing, but that returns immediately when_srcSignalis null. Staticsrcvalues therefore produce an empty<img>with nosrcoraltassigned.🛠️ Minimal fix
if (nowImageMode && this._imgElement) { this._disposeSrcBinding(); - this._setupSrcBinding(); + if (this._srcSignal) { + this._setupSrcBinding(); + } else { + this._updateDisplay(); + } } else if (!nowImageMode && this._initialsElement) { this._disposeSrcBinding(); this._updateDisplay(); @@ if (this._isImageMode) { this.mount(STYLES, `<img class="image" alt="" />`); this._imgElement = this.$(".image"); this._initialsElement = null; if (this._srcSignal) { this._setupSrcBinding(); + } else { + this._updateDisplay(); } } else {Also applies to: 198-205
🤖 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 113 - 115, When switching into image mode or when the src setter disposes/reinstates bindings, the code currently calls _disposeSrcBinding() then _setupSrcBinding() but _setupSrcBinding() returns immediately if _srcSignal is null, leaving a static src unassigned; update the flow in render() and the src setter so that after disposing and calling _setupSrcBinding(), if _imgElement exists and _srcSignal is falsy, explicitly assign the static values to the element (e.g. this._imgElement.src = this._src and this._imgElement.alt = this._alt or whichever alt property is used), ensuring static strings hydrate the <img> when nowImageMode is true and when switching modes; keep using _disposeSrcBinding()/_setupSrcBinding() for signal handling and only apply the direct assignment when no _srcSignal is present.Build/src/primitives/icon.ts (1)
81-109:⚠️ Potential issue | 🟠 MajorSwitching between SVG and text icons still mutates the wrong element type.
_iconElementis cached as either the rendered<svg>or<span>, but both_updateIcon()and the reactive effect patch that node in place. Aknown -> unknowntransition ends up writing<span>...</span>inside an existing<svg>, and the reverse nests SVG markup inside a<span>. Replace the node or rerender when the icon kind changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon.ts` around lines 81 - 109, The cached _iconElement (used in _updateIcon and the effect in _setupIconBinding) can be a rendered <svg> or <span>, and both code paths mutate its innerHTML which causes nesting when switching kinds; update the logic to detect when the new value is an SVG (ICON_SVGS[iconName] exists) vs text and replace the entire DOM node instead of patching innerHTML: in _updateIcon and inside the effect created in _setupIconBinding, compute whether the current element tagName matches the desired kind (svg vs span using escapeHtml), and if it does not, create the correct element, set its content (svg string or escaped text), replace the old _iconElement in the DOM, and update the cached _iconElement reference (and ensure _iconEffectDispose handling stays intact).Build/src/primitives/base.ts (2)
245-261:⚠️ Potential issue | 🔴 CriticalSame-root rerenders are still not safe for immediate DOM work.
mount()only goes synchronous when the extracted root tag changes. For rerenders that keep the same root tag, it still defers the DOM swap to_flush(), but several primitives still callthis.$(...)or wire handlers immediately aftermount(). That means those call sites keep binding against the old tree until the microtask replaces it. Either keep these rerenders synchronous too, or split out an explicit sync API and move the immediate-query call sites over.Also applies to: 269-283
🤖 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 245 - 261, mount() only updates the shadow DOM synchronously when the extracted root element changes, which leaves same-root rerenders deferred via scheduleRender()/ _flush() and causes call sites that immediately use this.$(...) or attach handlers after mount() to operate on the old DOM; fix by making same-root rerenders synchronous (perform the DOM swap immediately in mount() when newRootElement === this._currentRootElement) or provide a clear sync API (e.g., renderSync()) and update all immediate-query sites to call it; update references in mount(), scheduleRender(), _flush(), and callers that use this.$(...) so they either call the new renderSync() or mount() performs the immediate DOM swap for same-root updates.
331-348:⚠️ Potential issue | 🟠 Major
"value"bindings on<select>are still write-only and target the wrong surface.This branch only does
bindAttribute(element, "value", readable). That does not give you the same semantics as the input/textarea path: user selection never writes back to a writable source, and the live selection state should be driven through the element property, not the content attribute.🤖 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 331 - 348, The select branch currently calls bindAttribute(element, "value", readable), which only sets the attribute and never wires user changes back to a writable Signal and doesn't use the element property; update the HTMLSelectElement branch in Base (the case "value" handler) to mirror the input/textarea path: check if readable is writable (has "set") and call a select-specific two-way binder (e.g., bindSelectValue(element, readable as Signal<string>)) when writable, otherwise call bindAttribute only for read-only Readable, and ensure the binder uses the element.value property and listens to change/input to write back to the Signal; reference the HTMLSelectElement branch, bindAttribute, and the bindInputValue pattern to implement this.Build/src/primitives/select.ts (1)
208-217:⚠️ Potential issue | 🟠 MajorEnabling the select still wires handlers twice.
_setDisabled()mutates the observeddisabledattribute, andattributeChangedCallback()already tears down and re-adds the click/keydown handlers for that change. Calling_wireHandlers()again here adds a second handler set whenever the component is re-enabled.🛠️ Minimal fix
private _setDisabled(value: boolean) { (this as any)._disabled = value; if (value) { this.setAttribute("disabled", ""); } else { this.removeAttribute("disabled"); } this._updateTabIndex(); - this._wireHandlers(); }🤖 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 208 - 217, _setDisabled currently calls _wireHandlers redundantly causing handlers to be added twice when re-enabling; remove the call to this._wireHandlers() from the _setDisabled method (leave _updateTabIndex call) so that attributeChangedCallback remains the single place that tears down and re-adds click/keydown handlers; alternatively, if you prefer explicit safety, make _wireHandlers idempotent by tracking a private flag (e.g., _handlersWired) and only attach handlers when not already wired, but the minimal fix is to delete the this._wireHandlers() invocation in _setDisabled.Build/src/primitives/progress.ts (1)
110-117:⚠️ Potential issue | 🟠 Major
aria-valuenowstill drifts out of sync with indeterminate/clamped state.
render()writesMath.round(value)without clamping, and the reactive effect always restoresaria-valuenoweven whenindeterminateis set. That can expose out-of-range values and reintroducearia-valuenowimmediately after the indeterminate branch removed it.Also applies to: 161-165
🤖 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 110 - 117, The aria-valuenow handling is inconsistent: update render() to write Math.round(clampedValue) (compute clampedValue using the same clamp logic based on _rangeMin/_rangeMax) instead of Math.round(value), and update the reactive effect ariaDisposer (and the similar block around lines 161-165) to skip restoring aria-valuenow when this.indeterminate is true — remove the attribute in the indeterminate branch and only call setAttribute("aria-valuenow", String(Math.round(clamped))) when !this.indeterminate and the value is clamped into range using this._valueSignal, this._rangeMin, and this._rangeMax.Build/src/primitives/coverart.ts (1)
52-61:⚠️ Potential issue | 🟠 Major
srcmode changes still have dead states.Two branches are still fragile here:
- Switching from a plain string to a
Readablewhile_imgElementalready exists does nothing unless_srcEffectDisposewas already non-null, so the new source never subscribes.- When
currentSrcis empty,render()tears down the image and returns without installing any watcher, so a readable that starts empty cannot create the<img>later.That still leaves reactive cover art stuck until some unrelated rerender happens.
Also applies to: 111-115, 129-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/coverart.ts` around lines 52 - 61, The src setter and render() leave reactive Readable sources in dead states: ensure whenever a new Readable is assigned (detected by _isReadableStr) you always call _setupSrcEffect() to subscribe to it (even if _imgElement exists and _srcEffectDispose is null), and when render() tears down the <img> because currentSrc is empty you still install a watcher for any pending Readable (_pendingSrc / _srcSignal) so that when it later becomes non-empty it will create the <img>. Concretely: in the src setter (the set src(...) method) always invoke _setupSrcEffect() after assigning _srcSignal if the value is readable (and dispose existing _srcEffectDispose if present), and in render() do not skip subscribing when currentSrc is empty—if _pendingSrc/_srcSignal is a Readable, call _setupSrcEffect() so the readable can trigger creation later; mirror the same logic at the other similar branches referenced (around the other set/src and render code paths).
🧹 Nitpick comments (5)
Build/tests/structural-detection.test.ts (2)
186-192: Doubleawait Promise.resolve()may be fragile for timing.Multiple consecutive
await Promise.resolve()calls are used to wait for signal propagation and re-renders. This works but is sensitive to internal implementation details of the reactive system's scheduling.Consider using a utility that waits for the component to stabilize (e.g.,
await el.updateCompleteif available, or a dedicated flush helper from Sairin) for more robust tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/structural-detection.test.ts` around lines 186 - 192, Replace the fragile double "await Promise.resolve()" timing hack with a deterministic component-stability wait: after calling src.set('user.jpg'), await the component's update completion using the element's built-in promise (e.g., await el.updateComplete) or the project's flush helper (e.g., await flush()) so that the DOM query for img.image is performed only after the component has fully re-rendered; update the test around the src.set call and the subsequent DOM assertions to use el.updateComplete or the Sairin flush utility instead of two Promise.resolve awaits.
174-246: Avatar structural mode tests overlap withreactive-components.test.ts.These tests (
switches from initials to image when src signal updates, etc.) appear to duplicate coverage already present inreactive-components.test.ts(lines 196-234). Consider consolidating to avoid maintenance burden of keeping both in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/structural-detection.test.ts` around lines 174 - 246, The four tests named "switches from initials to image when src signal updates", "switches from image to initials when src signal updates", "switches from empty string to image with different signal", and "switches from image to empty with different signal" duplicate coverage already in reactive-components.test; remove or consolidate them: either delete these four tests from structural-detection.test.ts, or refactor by extracting their setup/assertions into a shared helper (e.g., createAvatarSwitchTests) and call that helper from a single authoritative test file so both suites use the same implementation; ensure any imports/signals (signal, path) and the element setup (creating 'saz-avatar', setting el.src, appending to document) are preserved in the consolidated helper and update test references accordingly.Build/tests/reactive-components.test.ts (2)
606-616: Polling loop for async resource may be flaky.The test uses a polling loop with fixed iteration count (20) and timeout (10ms each). This can fail on slow CI environments or if the resource takes longer than 200ms.
Consider increasing the timeout or using a more robust waiting mechanism.
♻️ Suggested: Use longer timeout or waitFor utility
let found = false; - for (let i = 0; i < 20; i++) { - await new Promise(resolve => setTimeout(resolve, 10)); + for (let i = 0; i < 50; i++) { + await new Promise(resolve => setTimeout(resolve, 20)); await Promise.resolve(); if (el.shadowRoot?.textContent?.includes('John')) { found = true; break; } }Or use a dedicated
waitForutility if available in your test setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 606 - 616, The polling loop in the test repeatedly checks el.shadowRoot?.textContent for "John" with only 20 iterations of 10ms (≈200ms total), which is flaky on slow CI; replace the manual loop by using a more robust waiting mechanism (e.g., a waitFor utility or extend timeout) to wait until el.shadowRoot?.textContent includes 'John' or a reasonable timeout elapses, updating the test around the variables/el usage (the polling block that sets found and breaks) to call waitFor or increase iterations/delay so the expectation on el.shadowRoot?.textContent reliably passes.
537-538: AccessingchildNodes[1]to skip style element is fragile.This assumes a specific shadow DOM structure. If the component's internal structure changes, this test will break silently.
♻️ Suggested: Query for text content more robustly
- const textNode = el.shadowRoot?.childNodes[1]; // Skip style element - expect(textNode?.textContent || '').toBe(''); + // Get text content excluding style element + const textContent = Array.from(el.shadowRoot?.childNodes || []) + .filter(node => node.nodeName !== 'STYLE') + .map(node => node.textContent) + .join(''); + expect(textContent).toBe('');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 537 - 538, The test is fragile because it accesses el.shadowRoot?.childNodes[1] (variable textNode) to skip a style element; change the assertion to find the text node more robustly by querying the shadow DOM for the element that should contain the text (e.g., use el.shadowRoot?.querySelector with the component's text container selector or fallback to reading el.shadowRoot?.textContent) and assert its textContent is ''. Update the test in Build/tests/reactive-components.test.ts to replace the childNodes[1] access with a querySelector or shadowRoot.textContent-based check so structure changes won't break the test.Build/src/primitives/button.ts (1)
75-89: Binding lifecycle fix looks good, butonCleanupmay cause double-disposal.The setter now correctly disposes the previous binding before creating a new one, addressing the earlier leak concern. However, registering
disposewithonCleanup(dispose)on line 84 after already storing it in_disabledDisposemeans if the component is later cleaned up,dispose()could be called a second time (once from cleanup, once if the setter runs again before cleanup).Consider either:
- Not registering with
onCleanupsince you track it manually, or- Clearing the
onCleanupregistration when disposing early.♻️ Option: Remove onCleanup registration since disposal is tracked manually
set disabled(value: boolean | Readable<boolean>) { if (this._disabledDispose) { this._disabledDispose(); this._disabledDispose = null; } if (this._isReadableBool(value)) { this._disabledSignal = value; const dispose = bindDisabled(this, value); this._disabledDispose = dispose; - this.onCleanup(dispose); } else { this._disabledSignal = null; this._setDisabled(value); } }Then ensure
disconnectedCallbackor equivalent cleanup callsthis._disabledDispose?.().🤖 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 75 - 89, The disabled setter currently stores the dispose function in this._disabledDispose and also registers it with onCleanup(dispose), risking double-disposal; remove the onCleanup(dispose) call in the setter (keep this._disabledDispose = dispose) and ensure component teardown calls this._disabledDispose?.() (e.g., in disconnectedCallback or the component's cleanup path) so bindings are still cleaned up when the component is removed; reference the disabled setter, _disabledDispose, bindDisabled, onCleanup, and _setDisabled when making the change.
🤖 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/avatar.ts`:
- Around line 84-87: The _getCurrentSrc() method currently only checks
_srcSignal and the backing field _src, which ignores declarative attributes like
<saz-avatar src="..."> and attribute updates; update _getCurrentSrc to also read
the element attribute (e.g., return this._srcSignal?.get() || (this as any)._src
|| this.getAttribute('src') || "") so attribute-based usage works, and ensure
the component continues to observe/reflect the "src" attribute (add "src" to
observedAttributes or the reflected config / attributeChanged handling) so
attribute updates route to the property setter.
In `@Build/src/primitives/radio.ts`:
- Around line 76-84: The setter for checked must dispose any previous binding
before assigning a new Readable to avoid leaks: in set checked (the setter that
references _checkedSignal, bindAttribute and _setChecked) track the disposer
returned by bindAttribute (store e.g. this._checkedDisposer), call that disposer
before replacing _checkedSignal or when switching back to a plain boolean, and
clear the disposer when you call _setChecked; ensure you invoke the previous
disposer if it exists whenever a new Readable is assigned.
- Around line 99-107: The disabled setter currently assigns a Readable to
_disabledSignal and calls bindDisabled without disposing any previous binding;
update the disabled setter to track and dispose the previous binding/signal when
reassigning: when _isReadableBool(value) is true, if there is an existing
_disabledSignal or stored disposer, call its dispose/unsubscribe before
assigning the new Readable, store the new disposer returned by bindDisabled (or
create one) so future reassignments or setting to a non-Readable will dispose
it; when switching to the non-Readable branch (else) ensure you dispose any
existing tracked binding before clearing _disabledSignal and calling
_setDisabled. Reference: disabled setter, _isReadableBool, _disabledSignal,
bindDisabled, and _setDisabled.
---
Outside diff comments:
In `@Build/src/primitives/radio.ts`:
- Around line 176-194: The sibling-update loop in
root.querySelectorAll(`saz-radio[name="${escapedName}"]`) only calls set(false)
on signal-backed siblings but neglects to clear the checked state on siblings
without _checkedSignal; update the loop to, for each sibling element (el), if
(el as any)._checkedSignal exists and has "set" call .set(false), otherwise
assign (el as any).checked = false so the element's setter runs; keep the rest
of the logic that sets this element's _checkedSignal (or calls
_setChecked(true)) and then calls _updateAria().
---
Duplicate comments:
In `@Build/src/primitives/avatar.ts`:
- Around line 116-118: In the initials-mode setter branch (where it checks
!nowImageMode && this._initialsElement), after calling this._disposeSrcBinding()
and this._updateDisplay(), call this._setupSignalWatcher() so the new readable
is re-subscribed when it starts as an empty string; do the same fix for the
other identical branch around the _updateDisplay() call (the one referenced at
lines 212-214) to ensure future truthy emissions flip back to image mode.
- Around line 113-115: When switching into image mode or when the src setter
disposes/reinstates bindings, the code currently calls _disposeSrcBinding() then
_setupSrcBinding() but _setupSrcBinding() returns immediately if _srcSignal is
null, leaving a static src unassigned; update the flow in render() and the src
setter so that after disposing and calling _setupSrcBinding(), if _imgElement
exists and _srcSignal is falsy, explicitly assign the static values to the
element (e.g. this._imgElement.src = this._src and this._imgElement.alt =
this._alt or whichever alt property is used), ensuring static strings hydrate
the <img> when nowImageMode is true and when switching modes; keep using
_disposeSrcBinding()/_setupSrcBinding() for signal handling and only apply the
direct assignment when no _srcSignal is present.
In `@Build/src/primitives/base.ts`:
- Around line 245-261: mount() only updates the shadow DOM synchronously when
the extracted root element changes, which leaves same-root rerenders deferred
via scheduleRender()/ _flush() and causes call sites that immediately use
this.$(...) or attach handlers after mount() to operate on the old DOM; fix by
making same-root rerenders synchronous (perform the DOM swap immediately in
mount() when newRootElement === this._currentRootElement) or provide a clear
sync API (e.g., renderSync()) and update all immediate-query sites to call it;
update references in mount(), scheduleRender(), _flush(), and callers that use
this.$(...) so they either call the new renderSync() or mount() performs the
immediate DOM swap for same-root updates.
- Around line 331-348: The select branch currently calls bindAttribute(element,
"value", readable), which only sets the attribute and never wires user changes
back to a writable Signal and doesn't use the element property; update the
HTMLSelectElement branch in Base (the case "value" handler) to mirror the
input/textarea path: check if readable is writable (has "set") and call a
select-specific two-way binder (e.g., bindSelectValue(element, readable as
Signal<string>)) when writable, otherwise call bindAttribute only for read-only
Readable, and ensure the binder uses the element.value property and listens to
change/input to write back to the Signal; reference the HTMLSelectElement
branch, bindAttribute, and the bindInputValue pattern to implement this.
In `@Build/src/primitives/coverart.ts`:
- Around line 52-61: The src setter and render() leave reactive Readable sources
in dead states: ensure whenever a new Readable is assigned (detected by
_isReadableStr) you always call _setupSrcEffect() to subscribe to it (even if
_imgElement exists and _srcEffectDispose is null), and when render() tears down
the <img> because currentSrc is empty you still install a watcher for any
pending Readable (_pendingSrc / _srcSignal) so that when it later becomes
non-empty it will create the <img>. Concretely: in the src setter (the set
src(...) method) always invoke _setupSrcEffect() after assigning _srcSignal if
the value is readable (and dispose existing _srcEffectDispose if present), and
in render() do not skip subscribing when currentSrc is empty—if
_pendingSrc/_srcSignal is a Readable, call _setupSrcEffect() so the readable can
trigger creation later; mirror the same logic at the other similar branches
referenced (around the other set/src and render code paths).
In `@Build/src/primitives/icon.ts`:
- Around line 81-109: The cached _iconElement (used in _updateIcon and the
effect in _setupIconBinding) can be a rendered <svg> or <span>, and both code
paths mutate its innerHTML which causes nesting when switching kinds; update the
logic to detect when the new value is an SVG (ICON_SVGS[iconName] exists) vs
text and replace the entire DOM node instead of patching innerHTML: in
_updateIcon and inside the effect created in _setupIconBinding, compute whether
the current element tagName matches the desired kind (svg vs span using
escapeHtml), and if it does not, create the correct element, set its content
(svg string or escaped text), replace the old _iconElement in the DOM, and
update the cached _iconElement reference (and ensure _iconEffectDispose handling
stays intact).
In `@Build/src/primitives/image.ts`:
- Around line 111-116: The render() method's !currentSrc branch clears the
shadow DOM but leaves this._imgElement and this._srcDispose intact, so later src
updates mutate a detached node; update the !currentSrc branch (in render and
using _getCurrentSrc reference) to call and clear the disposal function
(this._srcDispose) and null out this._imgElement and this._srcDispose (or set to
undefined) before returning so the component will recreate the <img> and its
bindings on subsequent src changes.
In `@Build/src/primitives/progress.ts`:
- Around line 110-117: The aria-valuenow handling is inconsistent: update
render() to write Math.round(clampedValue) (compute clampedValue using the same
clamp logic based on _rangeMin/_rangeMax) instead of Math.round(value), and
update the reactive effect ariaDisposer (and the similar block around lines
161-165) to skip restoring aria-valuenow when this.indeterminate is true —
remove the attribute in the indeterminate branch and only call
setAttribute("aria-valuenow", String(Math.round(clamped))) when
!this.indeterminate and the value is clamped into range using this._valueSignal,
this._rangeMin, and this._rangeMax.
In `@Build/src/primitives/select.ts`:
- Around line 208-217: _setDisabled currently calls _wireHandlers redundantly
causing handlers to be added twice when re-enabling; remove the call to
this._wireHandlers() from the _setDisabled method (leave _updateTabIndex call)
so that attributeChangedCallback remains the single place that tears down and
re-adds click/keydown handlers; alternatively, if you prefer explicit safety,
make _wireHandlers idempotent by tracking a private flag (e.g., _handlersWired)
and only attach handlers when not already wired, but the minimal fix is to
delete the this._wireHandlers() invocation in _setDisabled.
---
Nitpick comments:
In `@Build/src/primitives/button.ts`:
- Around line 75-89: The disabled setter currently stores the dispose function
in this._disabledDispose and also registers it with onCleanup(dispose), risking
double-disposal; remove the onCleanup(dispose) call in the setter (keep
this._disabledDispose = dispose) and ensure component teardown calls
this._disabledDispose?.() (e.g., in disconnectedCallback or the component's
cleanup path) so bindings are still cleaned up when the component is removed;
reference the disabled setter, _disabledDispose, bindDisabled, onCleanup, and
_setDisabled when making the change.
In `@Build/tests/reactive-components.test.ts`:
- Around line 606-616: The polling loop in the test repeatedly checks
el.shadowRoot?.textContent for "John" with only 20 iterations of 10ms (≈200ms
total), which is flaky on slow CI; replace the manual loop by using a more
robust waiting mechanism (e.g., a waitFor utility or extend timeout) to wait
until el.shadowRoot?.textContent includes 'John' or a reasonable timeout
elapses, updating the test around the variables/el usage (the polling block that
sets found and breaks) to call waitFor or increase iterations/delay so the
expectation on el.shadowRoot?.textContent reliably passes.
- Around line 537-538: The test is fragile because it accesses
el.shadowRoot?.childNodes[1] (variable textNode) to skip a style element; change
the assertion to find the text node more robustly by querying the shadow DOM for
the element that should contain the text (e.g., use el.shadowRoot?.querySelector
with the component's text container selector or fallback to reading
el.shadowRoot?.textContent) and assert its textContent is ''. Update the test in
Build/tests/reactive-components.test.ts to replace the childNodes[1] access with
a querySelector or shadowRoot.textContent-based check so structure changes won't
break the test.
In `@Build/tests/structural-detection.test.ts`:
- Around line 186-192: Replace the fragile double "await Promise.resolve()"
timing hack with a deterministic component-stability wait: after calling
src.set('user.jpg'), await the component's update completion using the element's
built-in promise (e.g., await el.updateComplete) or the project's flush helper
(e.g., await flush()) so that the DOM query for img.image is performed only
after the component has fully re-rendered; update the test around the src.set
call and the subsequent DOM assertions to use el.updateComplete or the Sairin
flush utility instead of two Promise.resolve awaits.
- Around line 174-246: The four tests named "switches from initials to image
when src signal updates", "switches from image to initials when src signal
updates", "switches from empty string to image with different signal", and
"switches from image to empty with different signal" duplicate coverage already
in reactive-components.test; remove or consolidate them: either delete these
four tests from structural-detection.test.ts, or refactor by extracting their
setup/assertions into a shared helper (e.g., createAvatarSwitchTests) and call
that helper from a single authoritative test file so both suites use the same
implementation; ensure any imports/signals (signal, path) and the element setup
(creating 'saz-avatar', setting el.src, appending to document) are preserved in
the consolidated helper and update test references accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fd806ac-eecd-4be7-b5ca-b1d7f4110cbe
📒 Files selected for processing (16)
Build/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/card.tsBuild/src/primitives/coverart.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/select.tsBuild/src/primitives/switch.tsBuild/src/primitives/toggle.tsBuild/src/runtime/transformer.tsBuild/tests/reactive-components.test.tsBuild/tests/structural-detection.test.ts
✅ Files skipped from review due to trivial changes (1)
- Build/src/primitives/input.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- Build/src/primitives/card.ts
- Build/src/primitives/switch.ts
- Build/src/primitives/toggle.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Build/src/primitives/progress.ts (1)
197-203:attributeChangedCallbackdoesn't check for value equality.Unlike the base class implementation (line 228 checks
if (oldVal === newVal) return), this override callsrender()unconditionally. While the base class handles common cases, the progress component's override re-renders even for unchanged attributes.This is a minor inefficiency but may be intentional to ensure consistency. Consider adding the equality check if re-renders are expensive.
Optional: Add equality check
- attributeChangedCallback() { + attributeChangedCallback( + name: string, + oldVal: string | null, + newVal: string | null, + ) { + if (oldVal === newVal) return; this.render(); }🤖 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 197 - 203, The override of attributeChangedCallback in the progress component always calls render() even when the attribute value hasn't changed; update attributeChangedCallback to accept the standard params (name, oldValue, newValue) and add an early-return check (if oldValue === newValue) before calling this.render(), mirroring the base class behavior so unnecessary re-renders are avoided; ensure you reference the observedAttributes static getter and keep using this.render() when values differ.Build/src/primitives/image.ts (1)
135-138: Redundant_updateSrccall after mount.When
_pendingSrcis set and render is called, lines 135-138 call_updateSrc(this._pendingSrc). However, the<img>was already rendered withcurrentSrcwhich includes_pendingSrcvia_getCurrentSrc()(line 116). The_imgElement.srcis already set to the correct value from the template (line 130).This
_updateSrccall is a no-op in the success path (it just reassigns the same value), but the cleanup path in_updateSrcfor falsy values won't trigger since we already checked!currentSrcearlier.Consider removing this redundant branch or adding a comment explaining if there's a specific edge case it handles.
🤖 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 135 - 138, Remove the redundant post-mount call to _updateSrc: delete the block that checks this._pendingSrc, calls this._updateSrc(this._pendingSrc) and sets this._pendingSrc = null, since _getCurrentSrc already sets the img src in the template and _imgElement.src is assigned during render; if you prefer to keep the code for clarity instead, replace the block with a one-line comment referencing _getCurrentSrc, _imgElement.src and _updateSrc to explain why the extra call is unnecessary or what specific edge-case it would handle.Build/src/primitives/base.ts (1)
129-130: Component ID generation could cause collisions in server-side rendering.The
componentIdusesDate.now()andMath.random(), which work well in browser contexts but could produce collisions in SSR/pre-rendering scenarios where multiple components are instantiated synchronously. Consider using a monotonic counter as a fallback.Suggested improvement
+// Module-level counter for SSR-safe IDs +let _componentIdCounter = 0; + export class SazamiComponent< C extends SazamiComponentConfig = any, > extends HTMLElement { // ... - componentId: string = `${this.tagName?.toLowerCase() ?? "element"}_${Date.now()}_${Math.random().toString(36).slice(2, 9)}`; + componentId: string = `${this.tagName?.toLowerCase() ?? "element"}_${++_componentIdCounter}_${Math.random().toString(36).slice(2, 9)}`;🤖 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 129 - 130, The componentId field currently builds IDs from Date.now() and Math.random(), which can collide under SSR; change it to use a deterministic monotonic fallback by introducing a module-level counter (e.g., nextComponentId) and incorporate it when Date.now()/Math.random() appear non-unique or when running in SSR: update the componentId assignment (referencing componentId and tagName) to prefer a stable sequence value (nextComponentId++) as part of the ID composition so multiple synchronous instantiations get unique IDs.
🤖 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 290-306: scheduleRender captures pendingRoot at schedule time but
_flush compares against _currentRootElement later, risking stale-root
mismatches; modify scheduleRender to also capture the current root (e.g., const
scheduledRoot = this._currentRootElement) and pass that into _flush (or
alternatively change _flush to re-extract the root from the current
pendingTemplate via _extractRootElement before comparing), and update _flush's
signature/use of pendingRoot accordingly (referencing scheduleRender,
_extractRootElement, _flush, pendingRoot, and _currentRootElement).
In `@Build/src/primitives/image.ts`:
- Around line 97-106: The _setupSrcBinding method registers the same dispose
twice (stores it in _srcDispose and also calls this.onCleanup(dispose)), causing
duplicate calls; remove the this.onCleanup(dispose) call inside _setupSrcBinding
so the lifecycle is managed solely via the _srcDispose field and its manual
invocation, and ensure the component’s disconnectedCallback (or appropriate
base-class cleanup) calls _srcDispose() if set to clean up the binding when the
element is disconnected; references: _setupSrcBinding, _srcDispose, onCleanup,
bindProperty, _imgElement, disconnectedCallback, _cleanupFns.
---
Nitpick comments:
In `@Build/src/primitives/base.ts`:
- Around line 129-130: The componentId field currently builds IDs from
Date.now() and Math.random(), which can collide under SSR; change it to use a
deterministic monotonic fallback by introducing a module-level counter (e.g.,
nextComponentId) and incorporate it when Date.now()/Math.random() appear
non-unique or when running in SSR: update the componentId assignment
(referencing componentId and tagName) to prefer a stable sequence value
(nextComponentId++) as part of the ID composition so multiple synchronous
instantiations get unique IDs.
In `@Build/src/primitives/image.ts`:
- Around line 135-138: Remove the redundant post-mount call to _updateSrc:
delete the block that checks this._pendingSrc, calls
this._updateSrc(this._pendingSrc) and sets this._pendingSrc = null, since
_getCurrentSrc already sets the img src in the template and _imgElement.src is
assigned during render; if you prefer to keep the code for clarity instead,
replace the block with a one-line comment referencing _getCurrentSrc,
_imgElement.src and _updateSrc to explain why the extra call is unnecessary or
what specific edge-case it would handle.
In `@Build/src/primitives/progress.ts`:
- Around line 197-203: The override of attributeChangedCallback in the progress
component always calls render() even when the attribute value hasn't changed;
update attributeChangedCallback to accept the standard params (name, oldValue,
newValue) and add an early-return check (if oldValue === newValue) before
calling this.render(), mirroring the base class behavior so unnecessary
re-renders are avoided; ensure you reference the observedAttributes static
getter and keep using this.render() when values differ.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 238be078-905f-44e4-aebc-68f7d5ebe2db
📒 Files selected for processing (4)
Build/src/primitives/base.tsBuild/src/primitives/image.tsBuild/src/primitives/progress.tsBuild/src/primitives/switch.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- Build/src/primitives/switch.ts
There was a problem hiding this comment.
Actionable comments posted: 6
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/radio.ts (1)
166-197:⚠️ Potential issue | 🔴 CriticalDon't clear the group when this radio can't write back.
If
_checkedSignalis read-only, the click path still clears sibling radios and dispatchessaz-change, even though this radio never becomes checked. Bail out before the sibling loop unless the current source is writable.🛠️ Suggested change
private _handleClick = () => { if (this._getIsDisabled()) return; if (this._getIsChecked()) return; + if (this._checkedSignal && !("set" in this._checkedSignal)) return; const name = this.getAttribute("name") || ""; const value = this.getAttribute("value") || "";🤖 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 166 - 197, The click handler _handleClick clears siblings and emits a change even when this radio's backing _checkedSignal is read-only; detect whether the current radio can actually write before doing anything (e.g., compute canWrite = !_checkedSignal || "set" in _checkedSignal) and bail out early if not writable so you don't loop over siblings, call _setChecked, call (this._checkedSignal as Signal).set(true) or update ARIA, or dispatchEventTyped("change", ...) when the radio cannot be updated; apply this check at the top of _handleClick before the sibling query and subsequent state/dispatch logic.
♻️ Duplicate comments (5)
Build/src/primitives/icon.ts (1)
151-170:⚠️ Potential issue | 🟠 MajorUse
mountSync()before recaching_iconElement.
render()queries the shadow root immediately aftermount(). On same-root rerenders (svg→svg/span→span),mount()defers the swap, so_iconElementkeeps pointing at the old node and later_updateIcon()/_setupIconBinding()calls target a ghost element.Build/src/primitives/image.tsalready switched to the synchronous path for this pattern.🛠️ Suggested change
if (svg) { - this.mount(STYLES, svg); + this.mountSync(STYLES, svg); } else { - this.mount(STYLES, `<span>${escapeHtml(iconName)}</span>`); + this.mountSync(STYLES, `<span>${escapeHtml(iconName)}</span>`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/icon.ts` around lines 151 - 170, The render() method mounts new icon markup asynchronously via mount(), then immediately queries the shadowRoot and recaches this._iconElement which can point to the old/ghost node on same-root rerenders; change the mount call in render() to use mountSync(STYLES, svgOrSpan) (matching the pattern used in Build/src/primitives/image.ts) so that the DOM swap is synchronous before re-querying this.shadowRoot, ensuring subsequent calls to _updateIcon() and _setupIconBinding() operate on the real element; update the mount(...) call where ICON_SVGS[iconName] is handled and keep references to _iconElement, _setupIconBinding, and _updateIcon unchanged.Build/src/primitives/radio.ts (1)
76-107:⚠️ Potential issue | 🟠 MajorDispose old
checked/disabledbindings before swapping sources.These setters keep adding new
:hostbindings, but never stop the previous ones. If either prop is reassigned, the old signal can keep mutating the radio's attributes in parallel with the new source until disconnect.🤖 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 76 - 107, The setters for checked and disabled add new bindings (via bindAttribute(":host", "checked", ...) and bindDisabled(":host", ...)) but never dispose the previous bindings, so reassigning signals leaves old bindings active; fix by storing the disposer/teardown returned by bindAttribute/bindDisabled (e.g., this._checkedBinding / this._disabledBinding) and call it before creating a new binding and also when switching to a non-readable value, and ensure teardown is invoked on disconnect; update checked/disabled setters to call the previous disposer (if present) before assigning this._checkedSignal / this._disabledSignal and before invoking bindAttribute/bindDisabled, and invoke the disposers in the element's disconnect lifecycle as well.Build/src/primitives/coverart.ts (1)
52-79:⚠️ Potential issue | 🔴 CriticalEmpty
srcvalues still strand the component.When a signal-backed
srcstarts empty,render()clears the shadow DOM and_setupSrcEffect()immediately returns because_imgElementisnull, so later signal emissions never create the<img>. On the string path,_updateSrc("")also just assignsimg.src = ""instead of taking the same teardown branch. Keep a persistent hidden<img>or funnel both paths through one mount/unmount helper.Also applies to: 85-102, 114-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/src/primitives/coverart.ts` around lines 52 - 79, The setter for src can leave the component stranded when the incoming value is an empty string or an initially-empty signal because render() clears the shadow DOM and _setupSrcEffect() returns early when _imgElement is null; fix by unifying mount/teardown logic: add a single helper (e.g., _ensureImgMountedOrTeardown) that always ensures a persistent hidden <img> is created by render() or tears it down when src is empty, then call that helper from the src setter instead of branching; update the src setter to dispose _srcEffectDispose, set _srcSignal/_pendingSrc as now, and then call the unified helper so both the string path (_updateSrc) and signal path (_setupSrcEffect) funnel through the same mount/unmount behavior using the shared symbols _imgElement, _pendingSrc, _srcSignal, _srcEffectDispose, _updateSrc and _setupSrcEffect.Build/src/primitives/image.ts (1)
50-63:⚠️ Potential issue | 🟠 MajorThe signal path bypasses your
srcteardown logic.
_setupSrcBinding()writes straight toimg.src, so signal emissions of""never go through_updateSrc()'s clear path. The same binding also survives a later plain-string assignment because the setter never calls_srcDispose, and it is no longer registered with base cleanup on disconnect. Route signal updates through a single_applySrc()helper and clear_srcDisposewhenever the mode changes or the element unmounts.Also applies to: 70-85, 97-105
🤖 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 50 - 63, The src setter currently lets _setupSrcBinding write directly to img.src and never disposes that binding, so signal emissions bypass _updateSrc's clear logic and the binding survives a subsequent string assignment; change the flow so both signal and string modes route through a single helper _applySrc(value) that contains the logic currently in _updateSrc, have _setupSrcBinding call the source signal's subscription callback which invokes _applySrc, ensure the setter clears any existing binding by calling and nulling _srcDispose whenever switching modes (and also during element unmount/disconnect), and register the signal disposal with the base cleanup so the binding is removed on disconnect; update _createImageElement/_setupSrcBinding/_updateSrc to use _applySrc and ensure _srcDispose is set/unset appropriately.Build/src/primitives/avatar.ts (1)
200-222:⚠️ Potential issue | 🟠 MajorPlain-string
srcis never applied to the<img>on initial render.When
srcis set to a plain string before/during mount,render()correctly creates the<img>element but never assigns itssrcattribute. The setter's fallback (lines 117-120) only runs when_imgElementalready exists, which isn't the case on first render.🐛 Proposed fix
render() { const currentSrc = this._getCurrentSrc(); this._isImageMode = !!currentSrc; if (this._isImageMode) { this.mount(STYLES, `<img class="image" alt="" />`); this._imgElement = this.$(".image"); this._initialsElement = null; if (this._srcSignal) { this._setupSrcBinding(); + } else if (this._imgElement) { + this._imgElement.src = currentSrc; + this._imgElement.alt = this.getAttribute("alt") || ""; } } else { this.mount(STYLES, `<span class="initials"></span>`); this._imgElement = null; this._initialsElement = this.$(".initials"); this._updateDisplay(); if (this._srcSignal) { this._setupSignalWatcher(); } } }🤖 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 200 - 222, render() creates the <img> when a plain-string src exists but never assigns the src attribute on initial mount; update render() to compute currentSrc via _getCurrentSrc() and, after creating and assigning this._imgElement (this.$(".image")), set its src to currentSrc when this._srcSignal is falsy (or always set it), then proceed with _setupSrcBinding() if _srcSignal exists; this ensures the plain-string src is applied immediately without relying on the setter path that expects an existing _imgElement.
🧹 Nitpick comments (5)
Build/src/primitives/toggle.ts (1)
137-150: Minor inconsistency with checkbox pattern for ARIA updates.Unlike
checkbox.ts(see context snippet lines 157-174), this handler doesn't explicitly call_updateAria()after updating the signal. Incheckbox.ts,_updateAria(newValue)is called immediately aftersignal.set(newValue), ensuring ARIA state is synchronized before thechangeevent dispatches.Here, ARIA updates rely on the signal → attribute →
attributeChangedCallbackchain, which may introduce a microtask delay between the event dispatch and ARIA update.🔧 Suggested alignment with checkbox pattern
private _handleClick = () => { if (this._getIsDisabled()) return; const newValue = this._checkedSignal ? !this._checkedSignal.get() : !((this as any)._checked || false); if (this._checkedSignal) { if ("set" in this._checkedSignal) { (this._checkedSignal as Signal<boolean>).set(newValue); + this._updateAria(); } } else { this._setChecked(newValue); } this.dispatchEventTyped("change", { checked: newValue }); };🤖 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 137 - 150, The _handleClick handler updates the checked state via _checkedSignal or _setChecked but doesn't immediately sync ARIA; modify _handleClick (the logic around _checkedSignal, Signal<boolean>.set, and _setChecked) to call this._updateAria(newValue) right after you set the new value (both in the branch where you call (this._checkedSignal as Signal<boolean>).set(newValue) and in the else branch after this._setChecked(newValue)) and do that before calling this.dispatchEventTyped("change", { checked: newValue }) so the ARIA state is updated synchronously like checkbox.ts.Build/tests/reactive-components.test.ts (2)
529-545: Consider using a more robust selector for null signal test.The test assumes the style element is the first child and the text node is the second. This creates a coupling to the internal DOM structure.
💡 Alternative approach
test('signal with null initial value', async () => { const el = document.createElement('saz-text') as any; const text = signal<string | null>(path("test", "nullText"), null); el.content = text as any; document.body.appendChild(el); await Promise.resolve(); - const textNode = el.shadowRoot?.childNodes[1]; // Skip style element - expect(textNode?.textContent || '').toBe(''); + // Check that shadowRoot text doesn't contain unexpected content + const fullText = el.shadowRoot?.textContent || ''; + expect(fullText.trim()).toBe(''); text.set('has value'); await Promise.resolve(); expect(el.shadowRoot?.textContent).toContain('has value'); el.remove(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 529 - 545, The test "signal with null initial value" is brittle because it assumes the text node is shadowRoot.childNodes[1]; update the assertion to use a robust selector instead: obtain the host element (el) and query its shadowRoot for the element that renders the text (e.g., use shadowRoot.querySelector('span') or a stable data-attribute like data-role="text") and assert on that element's textContent (or safe trimmed value) both before and after text.set; change references to textNode and the childNodes index accordingly in the test.
587-620: Resource integration test may be flaky.The polling loop (lines 607-614) uses a fixed iteration count with 10ms delays. If the environment is slow or the promise resolution takes longer, the test could fail intermittently.
Consider using a longer timeout or a more robust waiting mechanism if flakiness is observed in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/reactive-components.test.ts` around lines 587 - 620, The polling loop in the 'resource.value signal updates component' test (test name and the loop referencing el, userData, and userName) is too fragile because it uses a fixed 20-iteration, 10ms sleep; replace it with a more robust wait strategy—either increase the timeout and iterations or use a proper async wait helper (e.g., a waitFor utility that retries until el.shadowRoot?.textContent includes 'John' or a timeout is reached) to await the resource-driven render; ensure the logic checks el.shadowRoot?.textContent and throws/asserts after the wait completes to avoid intermittent CI flakes.Build/src/primitives/select.ts (1)
5-12: Unused import:onCleanup.The
onCleanupfunction is imported from@nisoku/sairinbut the code usesthis.onCleanup()(the base class method) instead. The Sairin import is never used.🧹 Remove unused import
import { Signal, Derived, isSignal, effect, - onCleanup, type Readable, } from "@nisoku/sairin";🤖 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 5 - 12, The import list includes onCleanup from "@nisoku/sairin" but the module-level onCleanup is unused—code calls the base-class method this.onCleanup() instead—so remove onCleanup from the import specifier to eliminate the unused import; update the import line that currently lists Signal, Derived, isSignal, effect, onCleanup, type Readable to omit onCleanup and run the linter/build to confirm no other references to the package-level onCleanup remain.Build/src/primitives/avatar.ts (1)
155-164: Dead code:previousSrcis unused.The
previousSrcvariable is initialized and updated but never read for any comparison. It appears to be leftover from a previous implementation.🧹 Suggested cleanup
- let previousSrc = sig.get(); const checkModeChange = () => { const currentSrc = sig.get(); const shouldBeImageMode = !!currentSrc; if (this._isImageMode !== shouldBeImageMode) { this._disposeSrcBinding(); this.render(); } - previousSrc = currentSrc; };🤖 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 155 - 164, Remove the dead variable previousSrc and its assignments in the block where checkModeChange is defined: delete the initialization "let previousSrc = sig.get();" and the update "previousSrc = currentSrc;" inside checkModeChange; keep the logic that computes currentSrc via sig.get(), derives shouldBeImageMode, compares against this._isImageMode, calls this._disposeSrcBinding() and this.render() unchanged (checkModeChange, sig.get, _isImageMode, _disposeSrcBinding, render).
🤖 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 275-285: mountSync currently writes the DOM synchronously but
leaves any queued render in _pendingTemplate/_pendingStyles which can replay and
overwrite the sync mount; update mountSync (and any other immediate render path)
to clear _pendingTemplate and _pendingStyles and set _lastTemplate and
_lastStyles to the values just mounted (template and styles) so a later flush
becomes a no-op, and ensure _currentRootElement is updated to newRootElement
(from _extractRootElement) as part of that synchronous path before
returning/finishing.
- Around line 370-375: In the HTMLSelectElement branch of the binding logic (the
block that currently calls bindSelectValue for writable signals and
bindAttribute for read-only), replace the bindAttribute call with bindProperty
so read-only sources set the element.value property (not the value attribute)
and actually change the selected option; update the branch that references
bindSelectValue and bindAttribute to use bindProperty(element, "value",
readable) for non-writable readables.
In `@Build/src/primitives/icon.ts`:
- Around line 110-149: The effect in _setupIconBinding closes over the local el
variable so updates later run against a detached node; change the effect to read
the current element from this._iconElement each run (use this._iconElement,
this._iconSignal, ICON_SVGS) instead of the captured el, and when the rendered
markup or element type changes replace the entire node (update this._iconElement
to the new node) rather than mutating a previously captured element; for SVG→SVG
updates replace the whole SVG element (not just innerHTML) so root attributes
like viewBox stay in sync, and keep using _iconEffectDispose and onCleanup as
before.
In `@Build/src/primitives/image.ts`:
- Around line 107-111: The _getCurrentSrc() function treats empty string as
falsy and thus collapses explicit "" into "unset"; update the checks in
_getCurrentSrc to treat empty string as a valid explicit value by testing for
undefined/null rather than truthiness: when reading from _srcSignal
(this._srcSignal.get()), _pendingSrc, and (this as any)._src, return the value
if it is !== undefined && !== null (allowing ""), otherwise fall through to
getAttribute("src") || "". This preserves explicit empty src while still falling
back when a value is truly absent; locate the function _getCurrentSrc to apply
these conditional changes.
In `@Build/src/primitives/input.ts`:
- Around line 72-83: The value setter currently only assigns _valueSignal and
skips wiring when this._input is mounted, so update set value(valueOrSignal) to
handle Readable strings when this._input exists: detect with _isReadableStr,
call _disposeValueBindings() before replacing _valueSignal, assign the new
_valueSignal, and then reuse the same render-time binding logic used during
mount to subscribe the new signal and update this._input.value (and install the
input listener/effect) so the field is driven immediately and previous
subscriptions are cleaned up; ensure non-readable branches still clear bindings
and set (this as any)._value as before.
In `@Build/src/primitives/switch.ts`:
- Around line 87-119: The checked and disabled setters currently overwrite
_checkedSignal/_disabledSignal and call bindAttribute(":host", "checked", ...) /
bindDisabled(":host", ...) without tearing down previous subscriptions; add
per-prop disposer fields (e.g., _checkedDisposer and _disabledDisposer), call
and nullify them before assigning a new signal, then store the disposer returned
by bindAttribute/bindDisabled so the previous live binding is disposed when
reassigned and on disconnect; update set checked, set disabled, and any
teardown/disconnect logic to call these disposers.
---
Outside diff comments:
In `@Build/src/primitives/radio.ts`:
- Around line 166-197: The click handler _handleClick clears siblings and emits
a change even when this radio's backing _checkedSignal is read-only; detect
whether the current radio can actually write before doing anything (e.g.,
compute canWrite = !_checkedSignal || "set" in _checkedSignal) and bail out
early if not writable so you don't loop over siblings, call _setChecked, call
(this._checkedSignal as Signal).set(true) or update ARIA, or
dispatchEventTyped("change", ...) when the radio cannot be updated; apply this
check at the top of _handleClick before the sibling query and subsequent
state/dispatch logic.
---
Duplicate comments:
In `@Build/src/primitives/avatar.ts`:
- Around line 200-222: render() creates the <img> when a plain-string src exists
but never assigns the src attribute on initial mount; update render() to compute
currentSrc via _getCurrentSrc() and, after creating and assigning
this._imgElement (this.$(".image")), set its src to currentSrc when
this._srcSignal is falsy (or always set it), then proceed with
_setupSrcBinding() if _srcSignal exists; this ensures the plain-string src is
applied immediately without relying on the setter path that expects an existing
_imgElement.
In `@Build/src/primitives/coverart.ts`:
- Around line 52-79: The setter for src can leave the component stranded when
the incoming value is an empty string or an initially-empty signal because
render() clears the shadow DOM and _setupSrcEffect() returns early when
_imgElement is null; fix by unifying mount/teardown logic: add a single helper
(e.g., _ensureImgMountedOrTeardown) that always ensures a persistent hidden
<img> is created by render() or tears it down when src is empty, then call that
helper from the src setter instead of branching; update the src setter to
dispose _srcEffectDispose, set _srcSignal/_pendingSrc as now, and then call the
unified helper so both the string path (_updateSrc) and signal path
(_setupSrcEffect) funnel through the same mount/unmount behavior using the
shared symbols _imgElement, _pendingSrc, _srcSignal, _srcEffectDispose,
_updateSrc and _setupSrcEffect.
In `@Build/src/primitives/icon.ts`:
- Around line 151-170: The render() method mounts new icon markup asynchronously
via mount(), then immediately queries the shadowRoot and recaches
this._iconElement which can point to the old/ghost node on same-root rerenders;
change the mount call in render() to use mountSync(STYLES, svgOrSpan) (matching
the pattern used in Build/src/primitives/image.ts) so that the DOM swap is
synchronous before re-querying this.shadowRoot, ensuring subsequent calls to
_updateIcon() and _setupIconBinding() operate on the real element; update the
mount(...) call where ICON_SVGS[iconName] is handled and keep references to
_iconElement, _setupIconBinding, and _updateIcon unchanged.
In `@Build/src/primitives/image.ts`:
- Around line 50-63: The src setter currently lets _setupSrcBinding write
directly to img.src and never disposes that binding, so signal emissions bypass
_updateSrc's clear logic and the binding survives a subsequent string
assignment; change the flow so both signal and string modes route through a
single helper _applySrc(value) that contains the logic currently in _updateSrc,
have _setupSrcBinding call the source signal's subscription callback which
invokes _applySrc, ensure the setter clears any existing binding by calling and
nulling _srcDispose whenever switching modes (and also during element
unmount/disconnect), and register the signal disposal with the base cleanup so
the binding is removed on disconnect; update
_createImageElement/_setupSrcBinding/_updateSrc to use _applySrc and ensure
_srcDispose is set/unset appropriately.
In `@Build/src/primitives/radio.ts`:
- Around line 76-107: The setters for checked and disabled add new bindings (via
bindAttribute(":host", "checked", ...) and bindDisabled(":host", ...)) but never
dispose the previous bindings, so reassigning signals leaves old bindings
active; fix by storing the disposer/teardown returned by
bindAttribute/bindDisabled (e.g., this._checkedBinding / this._disabledBinding)
and call it before creating a new binding and also when switching to a
non-readable value, and ensure teardown is invoked on disconnect; update
checked/disabled setters to call the previous disposer (if present) before
assigning this._checkedSignal / this._disabledSignal and before invoking
bindAttribute/bindDisabled, and invoke the disposers in the element's disconnect
lifecycle as well.
---
Nitpick comments:
In `@Build/src/primitives/avatar.ts`:
- Around line 155-164: Remove the dead variable previousSrc and its assignments
in the block where checkModeChange is defined: delete the initialization "let
previousSrc = sig.get();" and the update "previousSrc = currentSrc;" inside
checkModeChange; keep the logic that computes currentSrc via sig.get(), derives
shouldBeImageMode, compares against this._isImageMode, calls
this._disposeSrcBinding() and this.render() unchanged (checkModeChange, sig.get,
_isImageMode, _disposeSrcBinding, render).
In `@Build/src/primitives/select.ts`:
- Around line 5-12: The import list includes onCleanup from "@nisoku/sairin" but
the module-level onCleanup is unused—code calls the base-class method
this.onCleanup() instead—so remove onCleanup from the import specifier to
eliminate the unused import; update the import line that currently lists Signal,
Derived, isSignal, effect, onCleanup, type Readable to omit onCleanup and run
the linter/build to confirm no other references to the package-level onCleanup
remain.
In `@Build/src/primitives/toggle.ts`:
- Around line 137-150: The _handleClick handler updates the checked state via
_checkedSignal or _setChecked but doesn't immediately sync ARIA; modify
_handleClick (the logic around _checkedSignal, Signal<boolean>.set, and
_setChecked) to call this._updateAria(newValue) right after you set the new
value (both in the branch where you call (this._checkedSignal as
Signal<boolean>).set(newValue) and in the else branch after
this._setChecked(newValue)) and do that before calling
this.dispatchEventTyped("change", { checked: newValue }) so the ARIA state is
updated synchronously like checkbox.ts.
In `@Build/tests/reactive-components.test.ts`:
- Around line 529-545: The test "signal with null initial value" is brittle
because it assumes the text node is shadowRoot.childNodes[1]; update the
assertion to use a robust selector instead: obtain the host element (el) and
query its shadowRoot for the element that renders the text (e.g., use
shadowRoot.querySelector('span') or a stable data-attribute like
data-role="text") and assert on that element's textContent (or safe trimmed
value) both before and after text.set; change references to textNode and the
childNodes index accordingly in the test.
- Around line 587-620: The polling loop in the 'resource.value signal updates
component' test (test name and the loop referencing el, userData, and userName)
is too fragile because it uses a fixed 20-iteration, 10ms sleep; replace it with
a more robust wait strategy—either increase the timeout and iterations or use a
proper async wait helper (e.g., a waitFor utility that retries until
el.shadowRoot?.textContent includes 'John' or a timeout is reached) to await the
resource-driven render; ensure the logic checks el.shadowRoot?.textContent and
throws/asserts after the wait completes to avoid intermittent CI flakes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 140bfedc-db78-44d8-bc3a-94219c064bbe
📒 Files selected for processing (16)
Build/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/card.tsBuild/src/primitives/coverart.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/progress.tsBuild/src/primitives/radio.tsBuild/src/primitives/select.tsBuild/src/primitives/switch.tsBuild/src/primitives/toggle.tsBuild/src/runtime/transformer.tsBuild/tests/reactive-components.test.tsBuild/tests/structural-detection.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- Build/src/runtime/transformer.ts
- Build/src/primitives/card.ts
Member
Author
|
NOOOOO there's moreeee |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Build/src/primitives/button.ts (1)
80-83: Button usesbindDisableddirectly onthisrather than via base class helper.Unlike other primitives that call
this.bindDisabled(":host", value), the button callsbindDisabled(this, value)directly from the Sairin import. While functionally equivalent for the host element, this bypasses the cleanup registration that the base class method provides (this._cleanupFns.push(dispose)).The dispose function is stored in
_disabledDisposeand can be called on reassignment, but it won't be automatically called duringdisconnectedCallback. Consider either:
- Also calling
this.onCleanup(dispose), or- Using the base class
this.bindDisabled(":host", value)pattern🛠️ Suggested fix
if (this._isReadableBool(value)) { this._disabledSignal = value; const dispose = bindDisabled(this, value); this._disabledDispose = dispose; + this.onCleanup(dispose); } else {🤖 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 80 - 83, The button currently calls the imported bindDisabled(this, value) and stores the dispose in _disabledDispose which skips the base-class cleanup registration; change it to use the base-class helper so disposes are automatically tracked: either call this.onCleanup(dispose) after the current bindDisabled(...) call or replace the call with this.bindDisabled(":host", value) so the returned dispose is pushed into the base class cleanup stack (affecting _disabledDispose/_disabledSignal handling accordingly); update references to bindDisabled, this.bindDisabled, _disabledDispose, _disabledSignal, and onCleanup as needed.Build/src/primitives/base.ts (1)
275-289:mountSyncdoesn't clear_dirtyflag, allowing stale queued flush to run.The past review suggested clearing pending queue in
mountSync, and_pendingStyles/_pendingTemplateare now nulled (lines 277-278), but_dirtyis not set tofalse. If a microtask was already scheduled beforemountSyncis called, the queued callback will still execute. Although_pendingTemplatebeingnullprevents_flush()from running, setting_dirty = falsewould be more defensive and consistent.🛠️ Suggested fix
protected mountSync(styles: string, template: string) { const newRootElement = this._extractRootElement(template); + this._dirty = false; this._pendingStyles = null; this._pendingTemplate = null; this._lastTemplate = template; this._lastStyles = styles;🤖 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 275 - 289, The mountSync method clears _pendingStyles and _pendingTemplate but doesn't reset the _dirty flag, so a previously scheduled microtask may still run; update mountSync to set this._dirty = false after nulling _pendingStyles/_pendingTemplate (and before/after updating _lastTemplate/_lastStyles) to defensively cancel any queued flushes (so _flush sees no work), keeping behavior consistent with the pending-queue clearing.Build/src/primitives/select.ts (1)
259-262: Value binding may be set up twice during render.
render()calls_setupValueBinding()at line 261 if_valueSignalexists. However,_setupValueBindingis also called from thevaluesetter (line 141) when a signal is assigned. Ifvalueis set before the component renders, then on first render,_setupValueBindingwill be called again, creating a redundant effect.While the disposal logic handles this (the second call disposes the first effect), it's wasteful. Consider tracking whether binding is already set up:
🛠️ Suggested improvement
render() { // ... existing code ... - if (this._valueSignal) { + if (this._valueSignal && !this._valueEffectDisposer) { this._setupValueBinding(); } }🤖 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 259 - 262, The render path currently calls _setupValueBinding() if this._valueSignal exists, but the value setter also calls _setupValueBinding(), causing duplicate effect setup when value is assigned before the first render; modify the logic to guard against double initialization by adding and checking a simple boolean flag (e.g., _valueBindingInitialized) or the existing effect handle (e.g., this._valueEffect) inside both the value setter and render() before calling _setupValueBinding(), ensure _setupValueBinding() sets the flag when it creates the effect and that disposal clears the flag so subsequent re-initialization still works.Build/tests/structural-detection.test.ts (1)
122-136: Test validates batching behavior but assertion could be more explicit.The test sets
showDivmultiple times in quick succession to verify batching, but it only asserts the final state. Consider adding an assertion that confirms no intermediate DOM writes occurred, or documenting that the test's value is in exercising the batching path without errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Build/tests/structural-detection.test.ts` around lines 122 - 136, The test "batches renders when root element stays the same" only asserts the final DOM state; add an explicit check that intermediate writes/renders didn't occur by observing the element before toggling showDiv: attach a MutationObserver to el.shadowRoot (or spy on el.requestUpdate/render) to count mutation/render invocations, perform the rapid showDiv toggles, await the microtask, then assert the mutation/render count is 1 (or matches the expected batched behavior) and that section.auto-section exists; reference the test name, the el.showDiv toggles, and el.shadowRoot/section.auto-section when adding the observer/spy and final assertions.
🤖 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/icon.ts`:
- Around line 117-149: The effect callback currently captures the old el
variable (const el = this._iconElement outside the effect) so after swapping
nodes updates go to a detached element; move the read of the instance field into
the effect body (i.e., inside effect(() => { const el = this._iconElement; ...
})) so each run uses the current DOM node, keep using ICON_SVGS and _iconSignal
as before, and ensure _iconElement still gets updated when replacing elements so
subsequent runs pick up the new node; no other behavioral changes required
(leave _iconEffectDispose and onCleanup untouched).
In `@Build/src/primitives/toggle.ts`:
- Around line 67-75: The checked setter in Toggle currently calls
bindAttribute(":host", "checked", value) without tracking/disposing previous
binding; update the setter to mirror radio.ts by adding a _checkedBindingDispose
field, call and clear any existing _checkedBindingDispose before creating a new
binding, store the returned dispose function from bindAttribute into
_checkedBindingDispose when value is a Readable, and ensure you clear/assign
_checkedSignal/_checkedBindingDispose appropriately (also ensure any disposal
happens when switching to a non-Readable path and when component disconnects if
not already handled); reference: checked setter, _checkedSignal, _setChecked,
bindAttribute, and _checkedBindingDispose.
- Around line 91-99: The disabled setter fails to dispose previous binding when
reassigned; update the setter for disabled (the set disabled(...) method) to
mirror checked’s behavior: if a previous binding/signal (this._disabledSignal or
existing binding) exists dispose/unbind it before assigning the new value, store
the new binding reference (similar to _checkedSignal/_checkedBinding pattern)
and ensure bindDisabled(":host", value) is tracked so you can call its dispose
when overwritten; if value is not a Readable, clear any stored binding (set
this._disabledSignal = null) and call this._setDisabled(value).
In `@Build/tests/reactive-components.test.ts`:
- Around line 598-607: The effect created by effect(...) is never disposed;
capture its disposer (e.g., const dispose = effect(...)) when creating the
effect that reads userData.value.get() and sets userName, then call dispose()
after the awaits (or at test teardown) so the effect is cleaned up; reference
the effect creation around effect(...), the reactive sources userData and
userName, and the DOM element el when placing the disposal after the async wait.
---
Nitpick comments:
In `@Build/src/primitives/base.ts`:
- Around line 275-289: The mountSync method clears _pendingStyles and
_pendingTemplate but doesn't reset the _dirty flag, so a previously scheduled
microtask may still run; update mountSync to set this._dirty = false after
nulling _pendingStyles/_pendingTemplate (and before/after updating
_lastTemplate/_lastStyles) to defensively cancel any queued flushes (so _flush
sees no work), keeping behavior consistent with the pending-queue clearing.
In `@Build/src/primitives/button.ts`:
- Around line 80-83: The button currently calls the imported bindDisabled(this,
value) and stores the dispose in _disabledDispose which skips the base-class
cleanup registration; change it to use the base-class helper so disposes are
automatically tracked: either call this.onCleanup(dispose) after the current
bindDisabled(...) call or replace the call with this.bindDisabled(":host",
value) so the returned dispose is pushed into the base class cleanup stack
(affecting _disabledDispose/_disabledSignal handling accordingly); update
references to bindDisabled, this.bindDisabled, _disabledDispose,
_disabledSignal, and onCleanup as needed.
In `@Build/src/primitives/select.ts`:
- Around line 259-262: The render path currently calls _setupValueBinding() if
this._valueSignal exists, but the value setter also calls _setupValueBinding(),
causing duplicate effect setup when value is assigned before the first render;
modify the logic to guard against double initialization by adding and checking a
simple boolean flag (e.g., _valueBindingInitialized) or the existing effect
handle (e.g., this._valueEffect) inside both the value setter and render()
before calling _setupValueBinding(), ensure _setupValueBinding() sets the flag
when it creates the effect and that disposal clears the flag so subsequent
re-initialization still works.
In `@Build/tests/structural-detection.test.ts`:
- Around line 122-136: The test "batches renders when root element stays the
same" only asserts the final DOM state; add an explicit check that intermediate
writes/renders didn't occur by observing the element before toggling showDiv:
attach a MutationObserver to el.shadowRoot (or spy on el.requestUpdate/render)
to count mutation/render invocations, perform the rapid showDiv toggles, await
the microtask, then assert the mutation/render count is 1 (or matches the
expected batched behavior) and that section.auto-section exists; reference the
test name, the el.showDiv toggles, and el.shadowRoot/section.auto-section when
adding the observer/spy and final assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78dfa92b-9931-4fa8-b3c4-8144e26bbd6e
📒 Files selected for processing (12)
Build/src/primitives/avatar.tsBuild/src/primitives/base.tsBuild/src/primitives/button.tsBuild/src/primitives/icon.tsBuild/src/primitives/image.tsBuild/src/primitives/input.tsBuild/src/primitives/radio.tsBuild/src/primitives/select.tsBuild/src/primitives/switch.tsBuild/src/primitives/toggle.tsBuild/tests/reactive-components.test.tsBuild/tests/structural-detection.test.ts
✅ Files skipped from review due to trivial changes (1)
- Build/src/primitives/image.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- Build/src/primitives/switch.ts
- Build/src/primitives/input.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
Documentation
Tests
Chores