Propose an alternative to `PathEvent.bubbles` to make intent clearer by lawrence-forooghian · Pull Request #2223 · ably/ably-js

lawrence-forooghian · 2026-05-19T23:49:18Z

Note: This PR is based on top of #2222.

After noticing that the path-based API spec PR (ably/specification#427) didn't include map-entry events nor the bubbling-exclusion mechanism that's implemented in ably-js, I wanted to get a better understanding of this exclusion mechanism and why it exists.

From what I can tell, it exists to make sure that if there's a map at path "myMap", and this map emits a LiveMapUpdate having key "myKey", then a subscription that covers the path "myMap" will only receive one event (for "myMap"), as opposed to two (for "myMap" and "myMap.myKey").

If that is the only reason that this mechanism exists, then I don't think it's very obvious currently; the intended behaviour isn't documented. I think the implementation could be clearer if it makes the rules explicit:

for a given LiveObjectUpdate, a given subscription receives at most one event per path-to-root
in the case where a subscription covers both the "myMap" and "myMap.myKey" paths, "myMap" wins

That's what I've tried to do here.

Perhaps I've misunderstood the intent of bubbles (which, given that the tests still pass, would suggest a test gap), or perhaps others don't find what I'm proposing clearer (in which case, bubbles needs better documentation that explains its motivation). Thoughts, please.

Summary by CodeRabbit

Bug Fixes
- Improved path-based subscriptions so listeners are reliably notified once per reference path when the same object appears under multiple paths; notification selection and ordering have been refined.
Tests
- Added tests verifying subscription callbacks fire for each referenced path when an object is accessed via multiple root references.

coderabbitai · 2026-05-19T23:49:32Z

Warning

Rate limit exceeded

@lawrence-forooghian has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 20 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4b25ce31-df80-40aa-898d-dcddd8e04e48

📥 Commits

Reviewing files that changed from the base of the PR and between 067102a and 3abbc4d.

📒 Files selected for processing (3)

src/plugins/liveobjects/liveobject.ts
src/plugins/liveobjects/pathobjectsubscriptionregister.ts
test/realtime/liveobjects.test.js

Walkthrough

The PR refactors path-based subscription notification routing by replacing a bubbling-flag approach with ordered candidate paths. PathEvent now carries an array of candidate paths instead of a single path plus bubbling metadata. The dispatcher selects the first candidate path each subscription covers and invokes listeners. The sender produces events with base paths first, followed by deeper key-specific paths for map updates. A test validates callback delivery when an object is reachable from multiple root keys.

Changes

Path Subscription Notification Routing

Layer / File(s)	Summary
PathEvent interface contract `src/plugins/liveobjects/pathobjectsubscriptionregister.ts`	`PathEvent` interface replaces `path` and `bubbles` fields with `candidatePaths: Path[]` to enable ordered path selection during dispatch.
Subscription routing dispatcher `src/plugins/liveobjects/pathobjectsubscriptionregister.ts`	`notifyPathEvent()` dispatcher iterates subscriptions, finds the first `candidatePaths` entry each subscription covers via new `_subscriptionCoversPath()` helper, builds the `DefaultPathObject` with that chosen path, and invokes the listener with correct path context.
Path subscription notification sender `src/plugins/liveobjects/liveobject.ts`	`_notifyPathSubscriptions()` refactored to emit per-path `PathEvent`s with ordered `candidatePaths` (base path first, then key-specific deeper paths for `LiveMapUpdate`) instead of bulk emission with bubbling flags.
Multi-path subscription test `test/realtime/liveobjects.test.js`	New test verifies that when a counter object is referenced from two root keys (`counterA`, `counterB`), a single operation triggers the subscription callback exactly twice with correct reported paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

A rabbit hops through paths so bright,
Each candidate glows with ordered light,
Where once one path would bubble and boom,
Now many homes share one object's room,
subscriptions bloom 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ 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 accurately reflects the main change: replacing the `PathEvent.bubbles` mechanism with an alternative approach to make subscription notification behavior clearer.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch path-subscribe-dedup

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.}

lawrence-forooghian · 2026-05-19T23:53:26Z

+   * Candidate paths for surfacing this event to subscriptions. For a given
+   * subscription, the first candidate path it covers is used as the path of
+   * `event.object` passed to its listener. The caller is responsible for
+   * ordering this array so that the preferred path comes first when more


I also considered just making the rule be shortest-path-wins, which would achieve the same thing but perhaps not be as clear

I think letting the caller decide is fine, and we can just write it out explicitly in the caller (_notifyPathSubscriptions) that the intention is to have shortest-path-win semantics

Have added a comment alluding to this

VeskeR

From what I can tell, it exists to make sure that if there's a map at path "myMap", and this map emits a LiveMapUpdate having key "myKey", then a subscription that covers the path "myMap" will only receive one event (for "myMap"), as opposed to two (for "myMap" and "myMap.myKey").
If that is the only reason that this mechanism exists

Correct. I like the new approach, no need to have different path notification resolutions based on bubbling, and no concept of bubbling at all. Update just now says "here are all affected paths that subscribers might be interested in, in priority order", and subscribers react to that.

LGTM, with some minor nitpicks below. Also linter has failed

VeskeR · 2026-05-20T14:32:56Z

-            bubbles: false,
-          });
+
+    // For each subscription, emit at most one event per path-to-this


don't think this comment belongs in _notifyPathSubscriptions, it's reads to me more like something PathObjectSubscriptionRegister manages - ensuring that for each subscritpion at most one event per path is emitted

Yes but PathObjectSubscriptionRegister isn't aware that it's being invoked once for each path-to-this; all that it does is ensure that each subscription emits at most one event per invocation. It's _notifyPathSubscriptions which is responsible for doing one invocation per path-to-this

Will try making it clearer though

Tweaked the message

VeskeR · 2026-05-20T14:40:16Z

+   * ordering this array so that the preferred path comes first when more
+   * than one is covered.
+   */
+  candidatePaths: Path[];


make it orderedCandidatePaths so it's a bit clearer to the caller

Called it priorityOrderedCandidatePaths

VeskeR · 2026-05-20T15:44:00Z


+        {
+          description:
+            'PathObject.subscribe() fires once per path when the same object is referenced from multiple paths',


I don't know whether the current behaviour is intentional or not, but it
seemed worth writing a test for since Claude told me I'd introduced a
regression of this behaviour when I was trying to do a refactor.

Intentional, if an object is observable from multiple paths then each path should be notified of a change.
It is correct that current Realtime API doesn't allow to end with such a state, but it is possible via REST API atm, and we also discussed before that we may have such a functionality for Realtime - you would pass an Instance to collection set methods (map.set, list.add etc) instead of a ValueType.
Thanks for closing the gap

Thanks, I've updated the commit message

VeskeR · 2026-05-20T15:45:11Z

+   * Candidate paths for surfacing this event to subscriptions. For a given
+   * subscription, the first candidate path it covers is used as the path of
+   * `event.object` passed to its listener. The caller is responsible for
+   * ordering this array so that the preferred path comes first when more


I think letting the caller decide is fine, and we can just write it out explicitly in the caller (_notifyPathSubscriptions) that the intention is to have shortest-path-win semantics

Claude told me I'd introduced a regression of this behaviour when I was trying to do a refactor. Wasn't initially sure whether it was an intentional behaviour or not, but Andrii has confirmed [1] that it is, so worth a test. [1] #2223 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

lawrence-forooghian · 2026-05-20T17:19:57Z

Also linter has failed

fixed in #2225

Claude told me I'd introduced a regression of this behaviour when I was trying to do a refactor. Wasn't initially sure whether it was an intentional behaviour or not, but Andrii has confirmed [1] that it is, so worth a test. [1] #2223 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

After noticing that the path-based API spec PR [1] didn't include map-entry events nor the bubbling-exclusion mechanism that's implemented in ably-js, I wanted to get a better understanding of this exclusion mechanism and why it exists. From what I can tell, it exists to make sure that if there's a map at path "myMap", and this map emits a LiveMapUpdate having key "myKey", then a subscription that covers the path "myMap" will only receive one event (for "myMap"), as opposed to two (for "myMap" and "myMap.myKey"). If that _is_ the only reason that this mechanism exists, then I don't think it's very obvious currently; the intended behaviour isn't documented. I think the implementation could be clearer if it makes the rules explicit: - for a given LiveObjectUpdate, a given subscription receives at most one event per path-to-object - in the case where a subscription covers both the "myMap" and "myMap.myKey" paths, "myMap" wins That's what I've tried to do here. Perhaps I've misunderstood the intent of `bubbles` (which, given that the tests still pass, would suggest a test gap), or perhaps others don't find what I'm proposing clearer (in which case, `bubbles` needs better documentation that explains its motivation). Thoughts, please. [1] ably/specification#427 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

lawrence-forooghian requested review from VeskeR, mschristensen and sacOO7 May 19, 2026 23:49

github-actions Bot temporarily deployed to staging/pull/2223/bundle-report May 19, 2026 23:50 Inactive

github-actions Bot temporarily deployed to staging/pull/2223/features May 19, 2026 23:50 Inactive

github-actions Bot temporarily deployed to staging/pull/2223/typedoc May 19, 2026 23:50 Inactive

lawrence-forooghian commented May 19, 2026

View reviewed changes

VeskeR approved these changes May 20, 2026

View reviewed changes

lawrence-forooghian force-pushed the typealias-Path branch from d3e8b5b to e454f57 Compare May 20, 2026 17:00

lawrence-forooghian force-pushed the path-subscribe-dedup branch from 63be5ab to 067102a Compare May 20, 2026 17:03

github-actions Bot temporarily deployed to staging/pull/2223/bundle-report May 20, 2026 17:04 Inactive

github-actions Bot temporarily deployed to staging/pull/2223/typedoc May 20, 2026 17:05 Inactive

Base automatically changed from typealias-Path to main May 20, 2026 17:05

github-actions Bot temporarily deployed to staging/pull/2223/features May 20, 2026 17:08 Inactive

lawrence-forooghian force-pushed the path-subscribe-dedup branch from 067102a to 4b8352d Compare May 20, 2026 17:21

github-actions Bot temporarily deployed to staging/pull/2223/bundle-report May 20, 2026 17:22 Inactive

github-actions Bot temporarily deployed to staging/pull/2223/features May 20, 2026 17:22 Inactive

github-actions Bot temporarily deployed to staging/pull/2223/typedoc May 20, 2026 17:22 Inactive

lawrence-forooghian force-pushed the path-subscribe-dedup branch from 4b8352d to f0238d8 Compare May 20, 2026 17:22

github-actions Bot temporarily deployed to staging/pull/2223/bundle-report May 20, 2026 17:23 Inactive

github-actions Bot temporarily deployed to staging/pull/2223/typedoc May 20, 2026 17:23 Inactive

lawrence-forooghian force-pushed the path-subscribe-dedup branch from f0238d8 to 3abbc4d Compare May 20, 2026 17:23

github-actions Bot deployed to staging/pull/2223/bundle-report May 20, 2026 17:24 View deployment

github-actions Bot deployed to staging/pull/2223/features May 20, 2026 17:24 View deployment

github-actions Bot deployed to staging/pull/2223/typedoc May 20, 2026 17:25 View deployment

lawrence-forooghian merged commit 50f25c9 into main May 20, 2026
12 of 14 checks passed

lawrence-forooghian deleted the path-subscribe-dedup branch May 20, 2026 17:30

This was referenced May 20, 2026

Align path-based LiveObjects API spec with ably-js implementation ably/specification#477

Draft

Propose an alternative to PathEvent.bubbles #2221

Closed

[AIT-30] LiveObjects Path-based API spec ably/specification#427

Open

Conversation

lawrence-forooghian commented May 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeskeR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lawrence-forooghian May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lawrence-forooghian commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

lawrence-forooghian commented May 19, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 19, 2026 •

edited

Loading

VeskeR left a comment •

edited

Loading

lawrence-forooghian May 20, 2026 •

edited

Loading