Skip to content

Conversation

alex-snezhko
Copy link
Contributor

@alex-snezhko alex-snezhko commented Sep 16, 2025

close #13315

Calling MutationObserver.disconnect() may cause problems if there are still pending mutations that have not been asynchronously processed yet; before disconnecting flush/process the pending mutations queue to ensure all attribute updates are accounted for.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of attribute/property interop in custom elements: primitive props now consistently reflect to attributes; non-primitives are not reflected.
    • Reduced missed or out-of-order updates when mixing attribute and property changes for more consistent rendering.
  • Refactor

    • Streamlined mutation handling in custom elements to minimize race conditions during rapid updates. No API changes.
  • Tests

    • Added tests covering mixed attribute/property updates to prevent regressions.

Copy link

coderabbitai bot commented Sep 16, 2025

Walkthrough

Introduces VueElement._processMutations to centralize MutationObserver handling, replaces inline observer callbacks with a bound handler, updates observer lifecycle during attribute reflection, and adds tests verifying combined attribute/property updates on defineCustomElement components (including primitive reflection and non-primitive non-reflection).

Changes

Cohort / File(s) Summary
Runtime DOM: Custom Element observer refactor
packages/runtime-dom/src/apiCustomElement.ts
Added private method VueElement._processMutations(mutations) and replaced inline MutationObserver callbacks with a bound reference to it. In _setAttr, pending records from ob.takeRecords() are processed via _processMutations before disconnect() and the observer is re-attached after applying reflected attributes. _resolveDef now uses the bound handler.
Tests: Attribute/property interop
packages/runtime-dom/__tests__/customElement.spec.ts
Added test(s) "props via attributes and properties changed together" that set attributes and properties together, wait for updates, and assert shadow DOM rendering, attribute reflection for primitive foo, and absence of attribute for non-primitive bar. Test is inserted in two locations (duplicated coverage).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User code
  participant CE as VueElement (Custom Element)
  participant OB as MutationObserver
  participant DEF as Component definition

  Note over CE: Setup — observer bound to CE._processMutations
  U->>CE: set property (bar)\nsetAttribute('foo', v)
  alt attribute mutation observed
    OB-->>CE: mutations[]
    CE->>CE: _processMutations(mutations)
    CE->>CE: _setAttr('foo') → may reflect to attribute
    CE->>DEF: apply prop update (foo)
  else property update path
    CE->>DEF: apply prop update (bar)
  end

  Note over CE: During reflection in _setAttr
  CE->>OB: ob.takeRecords()
  CE->>CE: _processMutations(pending)
  CE->>OB: disconnect()
  CE->>OB: observe(attributes: true)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

I twitch my ears at changing states,
Attributes knock while props conspire,
I gather mutations, process, and then,
Reflect the small and hide the complex —
A hop, a patch, the DOM sings higher. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix(custom-element): set prop runs pending mutations before disconnect" is concise and accurately summarizes the primary change in the diff, which is ensuring pending MutationObserver records are processed before disconnect in the custom-element set-prop flow; it names the subsystem and the behavior being fixed and is specific enough for a reviewer to understand the main intent.
Linked Issues Check ✅ Passed The changes add VueElement._processMutations, invoke ob.takeRecords() before disconnect in _setAttr/_resolveDef, and add tests exercising attribute/property interop, which directly address the core problem described in linked issue [#13315] where MutationObserver.disconnect() could cause missed attribute-to-property updates.
Out of Scope Changes Check ✅ Passed All code modifications are limited to packages/runtime-dom (apiCustomElement.ts) and related unit tests (customElement.spec.ts), which are within the scope of fixing mutation handling for custom elements; there are no unrelated file or public API changes, although a duplicated test was introduced in the spec file.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b82824 and d5e077e.

📒 Files selected for processing (2)
  • packages/runtime-dom/__tests__/customElement.spec.ts (1 hunks)
  • packages/runtime-dom/src/apiCustomElement.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/runtime-dom/tests/customElement.spec.ts
  • packages/runtime-dom/src/apiCustomElement.ts
⏰ 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). (6)
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed
  • GitHub Check: test / e2e-test
  • GitHub Check: test / lint-and-test-dts
  • GitHub Check: test / unit-test-windows

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

github-actions bot commented Sep 16, 2025

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 102 kB (+92 B) 38.6 kB (+34 B) 34.7 kB (-9 B)
vue.global.prod.js 160 kB (+92 B) 58.7 kB (+36 B) 52.3 kB (+59 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.7 kB 18.3 kB 16.7 kB
createApp 54.7 kB 21.3 kB 19.5 kB
createSSRApp 58.9 kB 23 kB 21 kB
defineCustomElement 60 kB (+92 B) 23 kB (+40 B) 20.9 kB (+25 B)
overall 68.8 kB 26.5 kB 24.2 kB

Copy link

pkg-pr-new bot commented Sep 16, 2025

Open in StackBlitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@13897

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@13897

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@13897

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@13897

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@13897

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@13897

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@13897

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@13897

@vue/shared

npm i https://pkg.pr.new/@vue/shared@13897

vue

npm i https://pkg.pr.new/vue@13897

@vue/compat

npm i https://pkg.pr.new/@vue/compat@13897

commit: d5e077e

@edison1105 edison1105 added ready to merge The PR is ready to be merged. 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: custom elements labels Sep 23, 2025
@edison1105
Copy link
Member

/ecosystem-ci run

@vuejs vuejs deleted a comment from edison1105 Sep 24, 2025
@vue-bot
Copy link
Contributor

vue-bot commented Sep 24, 2025

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
pinia success success
nuxt success success
quasar success success
radix-vue success success
router success success
vant success success
test-utils success success
vitepress success success
vue-i18n success success
vite-plugin-vue success success
vueuse success success
vue-simple-compiler success success
primevue success success
vuetify failure failure
vue-macros failure success

@edison1105 edison1105 merged commit c4a88cd into vuejs:main Sep 24, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready to merge The PR is ready to be merged. scope: custom elements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MutationObserver.disconnect() in _setProp of core/packages/runtime-dom/src/apiCustomElement.ts may cause some confused problem
3 participants