[AIT-29] Handle cyclic references in PathObject/Instance .compact() methods

[VeskeR]

Cyclic references currently can be created via REST API which supports setting key in a map with a reference to another object by its id. To handle this in realtime we memoize compacted entries for visited maps and return references to those memoized entries when encountering the same map again during compaction.

Resolves AIT-29

Summary by CodeRabbit

• Bug Fixes

  • compact() now safely handles cyclic/shared references, avoiding infinite recursion and preserving nested/back-reference structure.

• Documentation

  • Expanded docs clarifying compact() behavior with cyclic/shared references and that outputs may contain shared references (may not be directly JSON-stringifiable).

• Tests

  • Added tests validating compact() behavior on cyclic object structures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds memoization to LiveMap.compact to handle cyclic/shared references, updates TypeScript doc-comments for compact() to document memoized outputs, and adds tests that create cycles via REST and assert compact() preserves back‑references.

Changes

Cohort / File(s)	Summary
Type definitions ably.d.ts	Doc-only updates: expanded comments for compact() across LiveMapPathObject, AnyPathObject, LiveMapBatchContext, AnyBatchContext, LiveMapInstance, and related types to document memoization, shared references in compacted outputs, and JSON.stringify limitations for cycles.
Memoization logic src/plugins/objects/livemap.ts	LiveMap.compact() signature changed to accept optional memoizedObjects?: Map<string, Record<string, any>>. Implements creation/usage of a memo map, early insertion of a partial compact result, and memoized reuse for nested LiveMap values to avoid infinite recursion and deduplicate shared references.
Test coverage test/realtime/objects.test.js	Added tests that construct cyclic object graphs via REST and assert compact() preserves nested values and that back‑references in the compacted output point to the memoized/root entry.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

• Inspect recursive memoization in src/plugins/objects/livemap.ts: correctness of early partial-result insertion, keying strategy, and safe reuse of memoized records.
• Verify interaction between externally supplied memoizedObjects vs. internally created map.
• Run and validate new tests in test/realtime/objects.test.js to ensure they reliably construct cycles via REST and assert reference identity.
• Confirm ably.d.ts documentation aligns with the runtime signature/behavior.

Poem

🐰 I hop through maps where loops entwine,
I mark each burrow, one at a time.
A memo trail where circles meet,
So compacted paths stay whole and neat.
🥕

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 clearly identifies the primary change: adding cyclic reference handling to the compact() methods for PathObject and Instance types.
Linked Issues check	✅ Passed	All objectives from AIT-29 are met: memoization implementation handles cyclic references, documentation comments explain JSON.stringify limitations, and tests validate cyclic structure handling.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to AIT-29: memoization in compact(), documentation updates, and cyclic reference tests. No unrelated modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch AIT-29/compact-handle-cyclic-references

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.40.0)

test/realtime/objects.test.js

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/plugins/objects/livemap.ts (1)

499-515: Cyclic memoization in LiveMap.compact looks correct; consider tightening the helper type

The new memoizedObjects plumbing correctly handles:

• self-referential maps (entry pointing back to the same LiveMap instance), and
• mutual cycles / shared submaps, by keying the memo on objectId and seeding it before iterating entries.

That gives you cycle safety and preserves aliasing across multiple references to the same LiveMap in the graph.

If you want to make this a bit clearer/safer for future readers, you could narrow the helper type slightly and convey intent in the name, e.g.:

-  compact(memoizedObjects?: Map<string, Record<string, any>>): API.CompactedValue<API.LiveMap<T>> {
-    const memo = memoizedObjects ?? new Map<string, Record<string, any>>();
+  compact(
+    memoizedObjects?: Map<string, Record<string, unknown>>,
+  ): API.CompactedValue<API.LiveMap<T>> {
+    const memo = memoizedObjects ?? new Map<string, Record<string, unknown>>();

or rename memo to something like visitedMapsById to document that it’s keyed by objectId.

These are optional readability tweaks; behaviour-wise this implementation looks solid.

ably.d.ts (1)

2603-2615: Document JSON.stringify behaviour for compacted cyclic graphs

The new sentences about handling cyclic references via memoization on the various compact() methods (PathObject, AnyPathObject, *BatchContext, *Instance) align with the implementation and tests.

However, the AIT‑29 objective also called out explicitly documenting that the resulting structure is not JSON‑stringifiable when cycles are present. Right now that point isn’t mentioned in these docblocks, so users might reasonably assume JSON.stringify(obj.compact()) is always safe.

Consider adding a brief note to each of the compact() docs you’ve just updated. For example, for LiveMapPathObject.compact:

   /**
    * Get a JavaScript object representation of the map at this path.
    * Binary values are returned as base64-encoded strings.
-   * Cyclic references are handled through memoization, returning shared compacted object references for already-visited objects.
+   * Cyclic references are handled through memoization, returning shared compacted object references for already-visited objects.
+   * Note: if the resulting structure contains cycles, calling `JSON.stringify()` on it will throw a `TypeError`
+   * due to JSON's inability to represent cyclic graphs.

And similarly for:

• AnyPathObject.compact
• LiveMapBatchContext.compact
• AnyBatchContext.compact
• LiveMapInstance.compact
• AnyInstance.compact

This keeps the behaviour clear and sets correct expectations for downstream serialization.

Also applies to: 2760-2767, 2862-2871, 3000-3008, 3469-3478, 3612-3620

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 166dc95 and f37add1.

📒 Files selected for processing (3)

• ably.d.ts (6 hunks)
• src/plugins/objects/livemap.ts (1 hunks)
• test/realtime/objects.test.js (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/plugins/objects/livemap.ts (3)

objects.d.ts (1)

• LiveMap (16-28)

src/plugins/objects/pathobject.ts (1)

• value (144-183)

src/plugins/objects/livecounter.ts (1)

• value (64-67)

⏰ 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: test-node (16.x)
• GitHub Check: test-node (18.x)
• GitHub Check: test-node (20.x)
• GitHub Check: test-browser (webkit)
• GitHub Check: test-browser (chromium)
• GitHub Check: test-browser (firefox)

[mschristensen]

This looks good, although we should include a comment in a doc string to indicate that the result of compact() cannot be blindly JSON-stringified if there are cyclic references.

[VeskeR]

Updated comment in https://github.com/ably/ably-js/compare/f37add1abb8300b0f98df048327ecefabd7ee944..d1a50f5b702ed66b0a978b2b7b7b9c8e84b40b11

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/plugins/objects/livemap.ts (1)

492-536: Memoization in compact() correctly handles cycles; consider minor doc clarification

The memoization strategy looks sound: inserting result into memo before iterating and keying by this.getObjectId() ensures both direct and indirect cycles (and shared submaps) are represented without infinite recursion, while keeping the existing call pattern (compact() with no args) intact.

Optional: since this method can now return objects with actual reference cycles, you might want to extend the comment here (even though it’s @internal) to explicitly note that the returned structure may be cyclic and therefore not JSON‑stringifiable, to mirror the behavior you’ve documented on the public API surface.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 520c502 and 662c5ee.

📒 Files selected for processing (3)

• ably.d.ts (6 hunks)
• src/plugins/objects/livemap.ts (1 hunks)
• test/realtime/objects.test.js (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• ably.d.ts
• test/realtime/objects.test.js

🧰 Additional context used

🧬 Code graph analysis (1)

src/plugins/objects/livemap.ts (2)

ably.d.ts (2)

• CompactedValue (2438-2463)
• LiveMap (2409-2412)

objects.d.ts (1)

• LiveMap (16-28)

⏰ 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: test-node (18.x)
• GitHub Check: test-node (16.x)
• GitHub Check: test-browser (chromium)
• GitHub Check: test-browser (webkit)
• GitHub Check: test-browser (firefox)
• GitHub Check: test-node (20.x)