bind) Make ko.applyBindings works in HTML-Shadow-Root-Elements #229
Conversation
📝 WalkthroughWalkthroughTreat DocumentFragment and HTMLTemplateElement as valid binding roots and cleanable nodes; virtualElements reads template.content; DOM containment/type guards expanded; disposal and template/slot/$data tests added (one test disabled, one duplicated); applyBindings and related APIs widened from HTMLElement to Node; package.json adds a test script. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as applyBindings caller
participant Binder as applyBindings
participant Utils as DOM utils (info/disposal/virtualElements)
participant DOM as DOM nodes (Element/Template/DocumentFragment)
Note over Caller,Binder: Start binding application
Caller->>Binder: applyBindings(rootNode: Node)
Binder->>Utils: isProviderForNode / isTemplateTag?
Utils-->>Binder: provider/template info
alt root is HTMLTemplateElement
Binder->>DOM: access rootNode.content (DocumentFragment)
Binder->>Binder: recurse into fragment children
else root is DocumentFragment
Binder->>Binder: recurse into fragment children
else normal root
Binder->>Binder: recurse into child nodes (skip script/textarea)
end
Binder->>Utils: domNodeIsAttachedToDocument / domNodeIsContainedBy checks
Binder->>Utils: cleanNode (register/execute disposals)
Binder->>DOM: apply binding handlers to nodes
Binder-->>Caller: bindings applied / complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/bind/src/applyBindings.ts (1)
46-46: Fix the type definition to use union instead of intersection.The type
BindingHandlerOrUndefinedshould use a union operator (|) instead of intersection (&). The functionasProperHandlerClassreturns either a BindingHandler class (constructor type), a BindingHandler instance, or undefined. Change line 46 to:type BindingHandlerOrUndefined = typeof BindingHandler | BindingHandler | undefinedAn intersection of a constructor and instance type is unsatisfiable and does not reflect the actual return values.
🧹 Nitpick comments (2)
packages/binding.component/spec/componentBindingBehaviors.ts (1)
1229-1229: Minor typo in variable name.Consider renaming
usedCopietousedCopyfor consistency with English spelling conventions.🔎 Proposed fix
- const usedCopie = template.cloneNode(true) as HTMLTemplateElement - applyBindings(outerViewModel, usedCopie) + const usedCopy = template.cloneNode(true) as HTMLTemplateElement + applyBindings(outerViewModel, usedCopy) - const innerText = (usedCopie.content.children[0] as HTMLElement).innerText.replace(/\s+/g, ' ').trim() + const innerText = (usedCopy.content.children[0] as HTMLElement).innerText.replace(/\s+/g, ' ').trim()packages/utils/src/dom/info.ts (1)
14-14: Typo in comment."envolved" should be "involved".
🔎 Proposed fix
- // This case also happens when the shadow DOM from a HTMLTemplateElement is envolved + // This case also happens when the shadow DOM from a HTMLTemplateElement is involved
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
builds/knockout/spec/bindingAttributeBehaviors.jspackages/bind/spec/bindingAttributeBehaviors.tspackages/bind/src/applyBindings.tspackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.template/spec/foreachBehaviors.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/utils/src/dom/disposal.tspackages/utils/src/dom/info.tspackages/utils/src/dom/virtualElements.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions may use naked arrow function syntax (zero-parameter arrows written without parentheses), e.g., data-bind='click: => was_clicked(true)' or lambdaLiteral: => null. This is valid TKO syntax, though not standard JavaScript. For code reviews, treat this as an allowed syntax specific to TKO in the analyzed file. Ensure tests cover this pattern, update documentation as needed, and avoid relying on parentheses-based parsing in this context. If similar patterns appear elsewhere, consider expanding the rule with a focused pattern to cover related TS files in the binding attribute behaviors area.
Applied to files:
packages/bind/spec/bindingAttributeBehaviors.ts
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions support "naked" arrow function syntax where zero-parameter arrow functions can be written without parentheses, e.g., `data-bind='click: => was_clicked(true)'` or `lambdaLiteral: => null`. This is valid TKO syntax even though it's not standard JavaScript.
Applied to files:
builds/knockout/spec/bindingAttributeBehaviors.js
🧬 Code graph analysis (5)
packages/utils/src/dom/virtualElements.ts (1)
packages/utils/src/dom/info.ts (1)
isTemplateTag(48-50)
packages/binding.component/spec/componentBindingBehaviors.ts (2)
packages/utils.component/src/ComponentABC.ts (1)
template(50-55)packages/bind/src/applyBindings.ts (1)
applyBindings(488-511)
packages/binding.template/spec/foreachBehaviors.ts (1)
packages/bind/src/applyBindings.ts (1)
applyBindings(488-511)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
packages/bind/src/applyBindings.ts (1)
applyBindings(488-511)
packages/bind/src/applyBindings.ts (1)
packages/provider/src/Provider.ts (1)
Provider(19-99)
🪛 ast-grep (0.40.3)
packages/binding.component/spec/componentBindingBehaviors.ts
[warning] 1210-1216: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML = <test-component> <template slot='alpha'>beep</template> Gamma <div>Zeta</div> </test-component>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 1210-1216: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML = <test-component> <template slot='alpha'>beep</template> Gamma <div>Zeta</div> </test-component>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
packages/binding.template/spec/foreachBehaviors.ts
[warning] 98-99: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: testNode.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 98-99: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: testNode.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
[warning] 225-226: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 236-237: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 225-226: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 236-237: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testheadless (22.x, off)
🔇 Additional comments (13)
builds/knockout/spec/bindingAttributeBehaviors.js (1)
597-602: LGTM - Test correctly disabled to reflect new template binding support.The test is appropriately disabled with a clear comment explaining that TKO now allows binding in
<template>elements, which aligns with the PR objectives. This preserves the test for historical context while acknowledging the behavioral change.packages/utils/src/dom/virtualElements.ts (2)
19-19: LGTM - Import added for template handling.The import of
isTemplateTagenables proper template element detection for the new template binding functionality.
189-191: LGTM - Correct handling of template element content.The implementation correctly accesses
node.content.firstChildfor template elements, as template content resides in a DocumentFragment accessible via thecontentproperty. This special case is appropriately placed before the virtual element (comment node) check.packages/utils/src/dom/disposal.ts (1)
14-16: LGTM - DocumentFragment properly added to cleanable node types.Adding DocumentFragment (nodeType 11) to both
cleanableNodeTypesandcleanableNodeTypesWithDescendantscorrectly extends cleanup support to document fragments, which is necessary for the new template binding functionality. This ensures proper disposal of bindings applied to DocumentFragment roots.packages/bind/spec/bindingAttributeBehaviors.ts (1)
881-881: LGTM - Test expectation correctly updated to reflect template binding support.The updated expectation from
<template>test</template>to<template>replaced</template>correctly validates that the custom binding provider now processes text nodes inside template elements. This demonstrates that TKO's new template binding capability is working as intended.packages/binding.template/spec/foreachBehaviors.ts (1)
98-106: LGTM - Good test coverage for template/slot binding in foreach.The test appropriately validates that foreach bindings work correctly within HTMLTemplateElement using HTMLSlotElement, mirroring the existing $data test pattern. Accessing template content via
.content.firstChildis correct for template elements.Note: The static analysis warning about
innerHTMLassignment on lines 99-100 is a false positive in this test context, as the content is a static string literal.packages/binding.component/spec/componentBindingBehaviors.ts (1)
1206-1237: LGTM - Excellent test coverage for template cloning with slots.This test correctly validates that slot processing works when bindings are applied to a cloned template, and that the original template remains unchanged. This is important for verifying that template-based components work correctly with the shadow DOM.
Note: The static analysis warning about
innerHTMLassignment on lines 1210-1216 is a false positive, as the content is a static test fixture string.packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
220-245: Good test coverage for template data context binding.These tests appropriately validate that
$databinding context works correctly within template elements and DocumentFragment scenarios, which is essential for the new shadow DOM binding support.Note: The static analysis warnings about
innerHTMLassignments are false positives in this test context, as the content is static string literals used for test fixtures.packages/utils/src/dom/info.ts (4)
15-17: LGTM - Important null-check for shadow DOM scenarios.The null-check for
containedByNodecorrectly handles cases where nodes are not attached to a container, which is particularly relevant for shadow DOM and template content scenarios introduced in this PR.
48-50: LGTM - Well-implemented template tag detection.The
isTemplateTagfunction correctly identifies HTMLTemplateElement nodes using both nodeType and tagName checks, with proper TypeScript type guard support.
52-58: LGTM - Improved type safety with explicit type guard.The explicit
obj is HTMLElementreturn type improves TypeScript type narrowing throughout the codebase.
60-66: LGTM - Improved type safety with explicit type guard.The explicit
obj is DocumentFragmentreturn type improves TypeScript type narrowing, which is particularly useful for the new DocumentFragment binding support.packages/bind/src/applyBindings.ts (1)
477-481: LGTM! Document fragment support properly added.The validation now correctly accepts
DOCUMENT_FRAGMENT_NODEalongside element and comment nodes, enabling bindings to apply to shadow DOM and template content. The parameter typeNodeappropriately covers all three accepted node types.
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
Outdated
Show resolved
Hide resolved
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/utils/src/dom/info.ts (1)
12-17: Fix typo in comment.The comment on line 14 contains a typo: "envolved" should be "involved".
🔎 Proposed fix
- // This case also happens when the shadow DOM from a HTMLTemplateElement is envolved + // This case also happens when the shadow DOM from a HTMLTemplateElement is involved
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/utils/spec/utilsDomBehaviors.tspackages/utils/src/dom/info.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/spec/utilsDomBehaviors.ts (1)
packages/utils/src/dom/info.ts (1)
domNodeIsContainedBy(6-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, on)
- GitHub Check: testheadless (22.x, off)
🔇 Additional comments (4)
packages/utils/src/dom/info.ts (3)
48-50: LGTM: Type guard for template elements.The new
isTemplateTagfunction with a proper TypeScript type guard is well-implemented and improves type safety for template element detection.
52-58: LGTM: Enhanced type guard for DOM elements.The updated signature using
obj is HTMLElementprovides better type narrowing for TypeScript consumers.
60-66: LGTM: Enhanced type guard for document fragments.The explicit type guard signature
obj is DocumentFragmentimproves type safety and aligns with the other type guard functions in this file.packages/utils/spec/utilsDomBehaviors.ts (1)
7-7: LGTM: Import added for new tests.The import of
domNodeIsContainedByis appropriate for the new test suite.
coderabbitai fixes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
9-11: Verify that runningmake sweepon every test execution is intentional.The test script performs a full clean (
make sweep) before building and testing. This ensures a pristine state but may slow down the development workflow significantly, as developers typically run tests frequently during active development.Consider whether:
- This script is intended primarily for CI/clean builds
- A separate lighter script for local development (without
sweep) would be beneficial💡 Suggested alternative approach
If frequent local testing is expected, consider adding both scripts:
"scripts": { - "test": "make sweep && make && make test-headless" + "test": "make && make test-headless", + "test:ci": "make sweep && make && make test-headless" }This allows developers to run
npm testquickly during development, while CI can usenpm run test:cifor a clean build.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.jsonpackages/binding.foreach/src/foreach.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/utils/spec/utilsDomBehaviors.tspackages/utils/src/dom/info.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/spec/utilsDomBehaviors.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/binding.foreach/src/foreach.ts (1)
packages/utils/src/dom/info.ts (1)
domNodeIsContainedBy(6-34)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
packages/bind/src/applyBindings.ts (1)
applyBindings(488-511)
🪛 ast-grep (0.40.3)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
[warning] 225-226: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 235-236: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 225-226: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 235-236: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testheadless (22.x, off)
🔇 Additional comments (5)
packages/binding.foreach/src/foreach.ts (1)
390-390: LGTM! Correct removal of non-null assertion.The change properly aligns with the updated
domNodeIsContainedBysignature that now acceptsNode | null. Sincedocument.activeElementcan returnnull, removing the non-null assertion operator is the correct approach, and the callee now handles null inputs gracefully.packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
220-243: LGTM! Good test coverage for template and DocumentFragment binding contexts.The new test suite appropriately validates that
$datais correctly bound whenapplyBindingsis called onHTMLTemplateElementand elements within aDocumentFragment. These tests directly support the PR's objective of enabling bindings in shadow root contexts.Regarding the static analysis warnings about
innerHTML: These are false positives. The test code uses hardcoded HTML string literals (not user input), which is standard practice in test suites and poses no XSS risk.packages/utils/src/dom/info.ts (3)
6-34: LGTM! Robust null handling and correct bitwise operation.The updated signature and implementation properly handle nullable inputs, which is essential for shadow DOM and template contexts where nodes may not have a container. The bitwise AND operation at lines 23-26 correctly checks the
DOCUMENT_POSITION_CONTAINED_BYflag in the bitmask returned bycompareDocumentPosition.
36-38: LGTM! Safe optional chaining for document element access.Using optional chaining (
?.) when accessingownerDocument.documentElementcorrectly handles cases where the node may not have an owner document, which can occur with detached or fragment nodes.
51-69: LGTM! Well-implemented type guards for template and fragment detection.The new type guards
isTemplateTag, andisDocumentFragmentare properly implemented with:
- Correct type narrowing syntax (
node is HTMLTemplateElement,obj is DocumentFragment)- Runtime checks for both modern (
instanceof) and legacy (nodeType) environments- Appropriate DOM constant comparisons
These enable the codebase to reliably detect and handle template elements and document fragments, which is core to the PR's shadow root support.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bind/src/applyBindings.ts (1)
1-1: Fix Prettier formatting before merging.The pipeline detected a Prettier formatting issue. Please run
prettier --write packages/bind/src/applyBindings.tsto fix the formatting.
♻️ Duplicate comments (3)
packages/utils/spec/utilsDomBehaviors.ts (1)
20-78: Good coverage for fragments/templates; consider adding a shadow‑DOM caseThe new
DOM-Info Toolspecs do a solid job exercisingdomNodeIsContainedByfor:
- element vs
DocumentFragment(before/after attachment),- null/undefined arguments,
<template>vstemplate.content,- basic parent/child and nested hierarchies.
To fully close the loop with the original goals (including shadow‑root support), consider adding at least one ShadowRoot scenario, e.g. asserting expected results for:
- a shadow host vs an element in its
shadowRoot, and- a light‑DOM node vs a shadow child.
This would pin down behavior when bindings operate inside shadow trees.
packages/bind/src/applyBindings.ts (2)
60-67: Verify that providers includeDOCUMENT_FRAGMENT_NODEin theirFOR_NODE_TYPES.The default node types array here still doesn't include
Node.DOCUMENT_FRAGMENT_NODE. While the formatting change improves readability, the more important concern from the previous review remains: all provider implementations that overrideFOR_NODE_TYPES(LegacyProvider, VirtualProvider, NativeProvider, TextMustacheProvider, AttributeMustacheProvider, DataBindProvider, ComponentProvider, and AttrProvider) must explicitly includeNode.DOCUMENT_FRAGMENT_NODEfor shadow DOM support to work consistently.Run the following script to verify provider implementations include the necessary node type:
#!/bin/bash # Description: Check if all provider implementations include DOCUMENT_FRAGMENT_NODE # Search for all providers and their FOR_NODE_TYPES definitions rg -n "FOR_NODE_TYPES.*=.*\[" --type ts -A 3
487-506: Update parameter type to reflect accepted node types.The function now accepts
DOCUMENT_FRAGMENT_NODEin addition toELEMENT_NODEandCOMMENT_NODE(lines 500-504), but the parameter type on line 489 remainsrootNode: HTMLElement. This creates a type mismatch sinceDocumentFragmentandCommentnodes are not subtypes ofHTMLElement.🔎 Recommended type fix
export function applyBindings<T = any>( viewModelOrBindingContext: BindingContext<T> | Observable<T> | T, - rootNode: HTMLElement, + rootNode: Element | Comment | DocumentFragment, extendContextCallback?: BindingContextExtendCallback<T> ): Promise<unknown> {
🧹 Nitpick comments (1)
packages/binding.component/spec/componentBindingBehaviors.ts (1)
1206-1237: Template‑clone slot test exercises the intended scenario wellThis test accurately covers the “clone a
<template>, apply bindings (including named + default slots), and leave the original template’s content unchanged” workflow. The extraDocumentFragmenthost and theusedCopienaming are slightly noisy but harmless; you could drop the fragment or rename the variable for clarity later if you touch this test again.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
builds/knockout/spec/bindingAttributeBehaviors.jspackage.jsonpackages/bind/spec/bindingAttributeBehaviors.tspackages/bind/src/applyBindings.tspackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.foreach/src/foreach.tspackages/binding.template/spec/foreachBehaviors.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/utils/spec/utilsDomBehaviors.tspackages/utils/src/dom/disposal.tspackages/utils/src/dom/info.tspackages/utils/src/dom/virtualElements.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions may use naked arrow function syntax (zero-parameter arrows written without parentheses), e.g., data-bind='click: => was_clicked(true)' or lambdaLiteral: => null. This is valid TKO syntax, though not standard JavaScript. For code reviews, treat this as an allowed syntax specific to TKO in the analyzed file. Ensure tests cover this pattern, update documentation as needed, and avoid relying on parentheses-based parsing in this context. If similar patterns appear elsewhere, consider expanding the rule with a focused pattern to cover related TS files in the binding attribute behaviors area.
Applied to files:
packages/bind/spec/bindingAttributeBehaviors.ts
📚 Learning: 2025-12-21T09:16:36.575Z
Learnt from: phillipc
Repo: knockout/tko PR: 221
File: packages/bind/spec/bindingAttributeBehaviors.ts:135-138
Timestamp: 2025-12-21T09:16:36.575Z
Learning: In the TKO framework, data-bind expressions support "naked" arrow function syntax where zero-parameter arrow functions can be written without parentheses, e.g., `data-bind='click: => was_clicked(true)'` or `lambdaLiteral: => null`. This is valid TKO syntax even though it's not standard JavaScript.
Applied to files:
builds/knockout/spec/bindingAttributeBehaviors.js
🧬 Code graph analysis (6)
packages/binding.template/spec/foreachBehaviors.ts (1)
packages/bind/src/applyBindings.ts (1)
applyBindings(487-510)
packages/bind/src/applyBindings.ts (2)
packages/provider/src/Provider.ts (1)
Provider(19-99)packages/utils.parser/src/Node.ts (1)
Node(7-123)
packages/binding.foreach/src/foreach.ts (1)
packages/utils/src/dom/info.ts (1)
domNodeIsContainedBy(6-34)
packages/binding.component/spec/componentBindingBehaviors.ts (3)
packages/utils.component/spec/ComponentABCBehaviors.ts (4)
template(68-70)template(86-88)template(107-109)template(143-145)packages/utils.component/src/ComponentABC.ts (1)
template(50-55)packages/bind/src/applyBindings.ts (1)
applyBindings(487-510)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
packages/bind/src/applyBindings.ts (1)
applyBindings(487-510)
packages/utils/spec/utilsDomBehaviors.ts (1)
packages/utils/src/dom/info.ts (1)
domNodeIsContainedBy(6-34)
🪛 ast-grep (0.40.3)
packages/binding.template/spec/foreachBehaviors.ts
[warning] 98-99: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: testNode.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 98-99: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: testNode.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
packages/binding.component/spec/componentBindingBehaviors.ts
[warning] 1210-1216: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML = <test-component> <template slot='alpha'>beep</template> Gamma <div>Zeta</div> </test-component>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 1210-1216: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML = <test-component> <template slot='alpha'>beep</template> Gamma <div>Zeta</div> </test-component>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
[warning] 225-226: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 235-236: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 225-226: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 235-236: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: Check code formatting with Prettier
packages/bind/src/applyBindings.ts
[warning] 1-1: Prettier formatting issue detected. Run 'prettier --write' to fix formatting in this file.
🔇 Additional comments (14)
package.json (1)
9-11: LGTM! Standard test script pattern.The test script follows a clean → build → test workflow with appropriate fail-fast behavior using
&&. This aligns well with the PR's test infrastructure updates for shadow root support.packages/binding.template/spec/foreachBehaviors.ts (1)
98-106: Template + slot foreach test is well‑shapedThe new test nicely mirrors the existing
$dataforeach test in a<template>+<slot>scenario and validates the intended behavior; using a staticinnerHTMLfixture here is fine despite the static‑analysis warning, since there’s no user input involved.builds/knockout/spec/bindingAttributeBehaviors.js (1)
597-602: Disabling legacy<template>expectation is appropriateMarking the legacy
<template>test asxitwith a clear comment is consistent with TKO’s behavior (allowing bindings inside<template>), while the TypeScript spec now asserts the new expected behavior.packages/bind/spec/bindingAttributeBehaviors.ts (1)
878-882: Updated<template>expectation correctly reflects new binding behaviorChanging the expectation to
<template>replaced</template>matches the intent that template contents are now processed by the binding provider (while<script>/<textarea>remain protected), and keeps this spec aligned with the disabled legacy KO test in the build suite.packages/utils/src/dom/virtualElements.ts (1)
19-23: ExtendfirstChildto traverse into<template>.contentUsing
isTemplateTagto routefirstChildtonode.content.firstChildis a clean way to make virtual traversal include template contents, which is needed for the new template/slot and component tests to see inner nodes while keeping non‑template behavior unchanged.Also applies to: 188-203
packages/binding.foreach/src/foreach.ts (1)
388-394: Use nullable‑awaredomNodeIsContainedByfor active elementPassing
document.activeElementdirectly intodomNodeIsContainedByleverages the new nullable‑aware signature and removes the unnecessary non‑null assertion without changing behavior.packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
220-243: Template data‑context tests correctly pin$datasemanticsThe new “Template data context” specs correctly assert that
$datainside atemplatebinding is the supplied data object when binding a<template>element or a detached<div>hosted in aDocumentFragment. Using staticinnerHTMLhere is fine for test fixtures and aligns with the newapplyBindingssupport for template/fragment scenarios.packages/bind/src/applyBindings.ts (2)
48-54: LGTM! Template bindings now enabled.Removing
templatefrom the exclusion list correctly enables bindings within template elements, which is essential for shadow DOM support as described in the PR objectives.
476-480: LGTM! Document fragment support correctly added.The addition of
Node.DOCUMENT_FRAGMENT_NODEenables bindings to work with document fragments, which is essential for shadow DOM and template content support.packages/utils/src/dom/info.ts (5)
6-17: LGTM! Improved null handling and shadow DOM awareness.The updated signature with nullable parameters and early return provides better type safety and prevents errors when working with shadow DOM scenarios. The explicit
DOCUMENT_FRAGMENT_NODEcheck correctly returnsfalsesince document fragments exist outside the normal DOM containment hierarchy.
22-27: LGTM! Correct bitwise check for document position.The
compareDocumentPositioncheck correctly uses bitwise AND to test for containment, addressing the previous review feedback. This properly handles cases where multiple position flags are set.
36-37: LGTM! Clean implementation with optional chaining.The updated signature with explicit
Nodetype and the use of optional chaining forownerDocument?.documentElementimproves type safety and handles edge cases gracefully.
51-53: LGTM! Useful type guard for template elements.The new
isTemplateTagtype guard provides a type-safe way to identify template elements, which is essential for the shadow DOM support added in this PR.
63-69: LGTM! Consistent type guard for document fragments.The new
isDocumentFragmenttype guard follows the same pattern asisDomElement, providing both moderninstanceofchecking and a fallback for older environments. This is essential for the document fragment binding support added in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/utils/src/dom/disposal.ts (1)
96-101: Nested element descendants of DocumentFragment won't be cleaned.For
Elementnodes,getElementsByTagName('*')returns all descendants. ForDocumentFragment,node.childNodesonly returns immediate children. When those children are processed, only their comment children are cleaned (line 55 usesonlyComments: true). Element descendants of those children are never processed.Example:
fragment → div → span— thespanwon't be cleaned, potentially causing memory leaks in complex shadow roots.Use
querySelectorAll('*')instead, which is supported onDocumentFragment:if (node instanceof Element) { cleanNodesInList(node.getElementsByTagName('*')) } else if (node instanceof Document || node instanceof DocumentFragment) { - cleanNodesInList(node.childNodes) + cleanNodesInList(node.querySelectorAll('*')) }
🧹 Nitpick comments (2)
packages/utils/spec/domNodeDisposalBehaviors.ts (2)
65-65: Minor: Test name mentions "detach from DOM" but only tests cleaning.The test description says "detach from DOM" but
cleanNodedoesn't remove nodes from their parent—that's whatremoveNodedoes. SinceDocumentFragmenttypically isn't attached to a DOM anyway, the name is misleading.Consider:
- it('Should run registered disposal callbacks and detach from DOM when a document fragment is cleanup', function () { + it('Should run registered disposal callbacks on a document fragment and its children when cleaned', function () {
65-82: Consider adding a test with deeply nested elements.This test validates disposal for a fragment with one immediate child, but doesn't cover nested descendants. Given the implementation uses
node.childNodes(immediate children only) forDocumentFragment, a test withfragment → div → spanwould help ensure nested elements are also cleaned.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/utils/spec/domNodeDisposalBehaviors.tspackages/utils/src/dom/disposal.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/spec/domNodeDisposalBehaviors.ts (1)
packages/utils/src/dom/disposal.ts (2)
addDisposeCallback(73-78)cleanNode(90-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: testheadless (22.x, on)
- GitHub Check: testheadless (22.x, off)
🔇 Additional comments (2)
packages/utils/src/dom/disposal.ts (2)
14-16: LGTM!Adding
DocumentFragment(nodeType 11) tocleanableNodeTypesandcleanableNodeTypesWithDescendantscorrectly enables disposal support for shadow roots and template content.
90-90: LGTM!The return type change from
typeof nodetoNodeis a reasonable simplification that maintains type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (3)
221-230: Clarify the purpose of the DocumentFragment or remove it.In this test, a
DocumentFragmentis created and the template is appended to it, but the fragment is never used afterward. SinceapplyBindingsis called directly on the template (line 228), the fragment serves no purpose in this test.Consider either:
- Removing the fragment creation if it's not needed, or
- Adding a comment explaining why the fragment is necessary for this test scenario
🔎 Proposed simplification
it('should set $data to the template data value', function () { - const fragment = document.createDocumentFragment() const template = document.createElement('template') as HTMLTemplateElement - fragment.appendChild(template) template.innerHTML = "<div data-bind='template: { data: someItem }'>" + "Value: <span data-bind='text: $data.val'></span>" + '</div>' applyBindings({ someItem: { val: 'abc' } }, template) expect(template.content.childNodes[0]).toContainText('Value: abc') })
232-241: Test logic is correct; consider clarifying the test name.This test correctly validates that
applyBindingscan descend from aDocumentFragmentinto child template elements. The key distinction from the previous test is that bindings are applied to the fragment (line 239) rather than directly to the template, which aligns with the PR objectives for shadow root support.However, the test name "applyBindings to fragment should set $data to the template data value" could be more explicit about what distinguishes this from the previous test.
Consider a more descriptive name such as:
- "should process template bindings when applyBindings is called on parent fragment"
- "should descend from fragment to template content when applying bindings"
243-253: Fragment appears unnecessary; clarify test name.This test creates a
DocumentFragmentand appends a div to it, but sinceapplyBindingsis called directly on the div (line 251), the fragment doesn't affect the test outcome. Similar to the first test, the fragment creation seems superfluous.Additionally, the test name "should set $data to the DIV at DocumentFragment" doesn't clearly communicate what's being validated.
🔎 Proposed changes
Option 1: Remove the fragment if not needed
it('should set $data to the DIV at DocumentFragment', function () { - const fragment = document.createDocumentFragment() const div = document.createElement('div') as HTMLDivElement div.innerHTML = "<div data-bind='template: { data: someItem }'>" + "Value: <span data-bind='text: $data.val'></span>" + '</div>' - fragment.appendChild(div) applyBindings({ someItem: { val: 'abc' } }, div) expect(div.childNodes[0]).toContainText('Value: abc') })Option 2: If fragment is intentionally part of the test setup, rename the test to clarify intent:
- it('should set $data to the DIV at DocumentFragment', function () { + it('should bind div containing template even when div is child of fragment', function () {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts (1)
packages/bind/src/applyBindings.ts (1)
applyBindings(483-506)
🪛 ast-grep (0.40.3)
packages/binding.template/spec/nativeTemplateEngineBehaviors.ts
[warning] 225-226: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 236-237: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 246-247: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 225-226: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 236-237: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: template.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 246-247: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML =
"
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testheadless (22.x, on)
1 participant
Inspired by #98, I adopted the changes from @avickers and finalized them in the current version of TKO. The use of shadow root was already implemented by @brianmhunt when he implemented the web component styled
ComponentABC. Now,applyBindingsis also allowed in HTMLTemplateElement. This allows you to define a template in shadow-root, copy it, apply bindings, and add the result to the regular DOM.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.