Let Dialog waitUntil accept a transitioner and make its lifecycle events bubble - #635
Conversation
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. @studiometa/ui
Unchanged (121)@studiometa/ui
@studiometa/ui-mapbox
|
Code ReviewRisk: Medium — issues that should be addressed before merge. This change makes Dialog lifecycle events bubble and extends 1 issue found:
Still open from earlier reviews (2 findings):
Review usage: 47,214 in (29,361 cached) / 1,214 out tokens — $0.0163 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit f186010. Previous review runsPrevious run archived 2026-08-13T12:46:57ZCode ReviewRisk: Medium — issues that should be addressed before merge. This change makes Dialog lifecycle events bubble and extends 1 issue found:
Still open from earlier reviews (1 finding):
Review usage: 59,879 in (45,780 cached) / 1,368 out tokens — $0.0152 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 506d359. Previous run archived 2026-08-13T12:09:55ZCode ReviewRisk: Medium — issues that should be addressed before merge. This change makes Dialog lifecycle events bubble and extends 1 issue found:
Review usage: 47,623 in (33,697 cached) / 1,907 out tokens — $0.0161 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 506d359. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| this.$warn( | ||
| `\`waitUntil\` must be called synchronously while the \`${name}\` event dispatches.`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
issue: Catch synchronous transitioner method failures
Calling enter() or leave() occurs before Promise.resolve(value), so a transitioner that throws synchronously escapes waitUntil() entirely. The open() or close() promise then rejects without the warning-and-settle behavior documented for failed extensions, and a failed open() can leave the dialog shown with scroll locking active. Invoke the method inside a promise or try/catch so synchronous throws are handled like rejected results.
Confidence: high.
Reviewed by @weareikko/code-review v0.9.5 for commit 506d359.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #635 +/- ##
=========================================
Coverage 87.23% 87.24%
Complexity 145 145
=========================================
Files 147 147
Lines 5124 5128 +4
Branches 965 969 +4
=========================================
+ Hits 4470 4474 +4
Misses 571 571
Partials 83 83
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| `\`waitUntil\` must be called synchronously while the \`${name}\` event dispatches.`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
issue: Preserve thenable handling before transitioner detection
The method check runs before thenable normalization, so an object implementing both then() and enter()/leave() is treated as a transitioner and its phase method is invoked instead of awaiting the thenable. This breaks the documented compatibility that a thenable keeps the existing behavior; classify thenables first or explicitly prefer then() when present.
Confidence: high.
Reviewed by @weareikko/code-review v0.9.5 for commit 506d359.
…nts bubble Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVXrJ8idMfvt667yJadvB8
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVXrJ8idMfvt667yJadvB8
506d359 to
f186010
Compare
| } | ||
| const value = | ||
| typeof (extension as DialogTransitioner)?.[method] === 'function' | ||
| ? (extension as DialogTransitioner)[method]() |
There was a problem hiding this comment.
issue: Handle synchronously thrown transitioner methods
Calling [method]() occurs before Promise.resolve(), so a transitioner whose enter() or leave() throws synchronously escapes waitUntil() and interrupts event dispatch. This violates the documented rejection-handling guarantee that a failing extension is warned about and the dialog choreography still completes; invoke the method inside a promise boundary or catch the synchronous error and register it as a rejected extension.
Confidence: high.
Reviewed by @weareikko/code-review v0.9.5 for commit f186010.
1 participant
What
Two additions to the extendable
open/closelifecycle events introduced in #627:bubbles: true): ancestors can route them declaratively (e.g. through anActionhigher up the tree), and descendants listening at the document level can join the choreography without being a declared child.waitUntil()now also accepts a transitioner: a duck-typed object withenter()andleave()methods.__emitExtendable()knows which phase is dispatching, so it awaitsenter()onopenandleave()onclose— a single handler passing the object itself is enough, no phase-specific expression required. A thenable keeps today's behavior, and the sync-only registration guard and rejection handling (warn + settle, choreography always completes) are unchanged.Why
This is the duck-typed handshake the upcoming ambient
MotionViewfrom@studiometa/ui-motionbuilds on: a transitioner exposingenter()/leave()can follow the dialog lifecycle without knowing anything aboutDialog, andDialogneeds to know nothing about Motion. With bubbling events, the ambientMotionViewcan pick up a dialog's lifecycle from the tree by itself — the common case becomes pure nesting with no attributes, and theActionexpression above stays as the explicit escape hatch.Test plan
open/closeevents reach an ancestor listener (document.body); a transitioner given towaitUntil()getsenter()called on open andleave()on close, with the dialog's promises held pending until each phase's promise resolves (same ordering assertions as the existingwaitUntilspecs); a transitioner whose method rejects behaves like a rejected promise — warned and settled, the dialog still hides and cleans up.npm run test— 98 files, 806 tests passed.npm run lint— 0 errors, 19 warnings (baseline).cd packages/docs && node scripts/validate-reference.ts— "Documentation validation passed".🤖 Generated with Claude Code
https://claude.ai/code/session_01FVXrJ8idMfvt667yJadvB8