fix: depth calculation when add previous events

[debdutdeb]

Summary by CodeRabbit

• Bug Fixes

  • Corrected event depth calculation to handle out-of-order predecessors, improving timeline consistency and preventing occasional misordering in rooms.

• Tests

  • Added tests to verify depth updates across multiple and out-of-order predecessors, ensuring accurate event sequencing.

[coderabbitai]

Walkthrough

Updates addPrevEvents to compute depth using the maximum predecessor depth rather than list order. Adds a new test verifying depth recalculation with unordered predecessors and multiple insertions. No public APIs changed.

Changes

Cohort / File(s)	Summary
Depth calculation logic packages/room/src/manager/event-wrapper.ts	Revised addPrevEvents to compute deepest predecessor depth (max) and set rawEvent.depth = deepestDepth + 1, removing reliance on input order.
Test coverage packages/room/src/manager/event-wrapper.spec.ts	Added test "correctly calculate new depth" covering unordered predecessor addition and incremental depth updates; added Pdu type import.

Sequence Diagram(s)

sequenceDiagram
    actor Caller
    participant Event as EventWrapper
    participant Store as Prev Events

    Caller->>Event: addPrevEvents(events[])
    Event->>Store: Read depths of predecessors
    Note over Event: Compute deepestDepth = max(depths)
    alt deepestDepth >= current depth
        Event->>Event: rawEvent.depth = deepestDepth + 1
    else
        Event->>Event: Keep current depth
    end
    Event-->>Caller: return

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• refactor: propagate room version through PersistentEventBase #230 — Touches the same addPrevEvents method with a refactor to Version-generic handling; overlaps in target area and implementation surface.

Suggested reviewers

• ggazzo

Poem

A hop, a skip, through graphs I creep,
Counting depths instead of sleep.
Max I seek in leafy threads,
Then add a whisker to their heads.
Out-of-order? No distress—
This rabbit sorts the depthy mess. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly states that the pull request fixes the depth calculation when adding previous events, directly matching the changes to the addPrevEvents logic and associated tests. It is concise, specific, and free of extraneous detail, making the main change immediately apparent to reviewers.

✨ 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 fix-depth-calc

---

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.46%. Comparing base (613a536) to head (a06b429).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
+ Coverage   60.30%   60.46%   +0.16%     
==========================================
  Files          67       67              
  Lines        6673     6673              
==========================================
+ Hits         4024     4035      +11     
+ Misses       2649     2638      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

packages/room/src/manager/event-wrapper.ts (2)

458-459: Refactor to avoid array mutation and improve performance.

The current implementation sorts the input array in place and uses pop() to find the maximum depth. This has two issues:

1. It mutates the caller's array, which is an unexpected side effect
2. Sorting is O(n log n) when finding the maximum is O(n)

Apply this diff to use Math.max instead:

-	const deepestDepth =
-		events.sort((e1, e2) => e1.depth - e2.depth).pop()?.depth ?? 0;
+	const deepestDepth = events.length > 0 
+		? Math.max(...events.map(e => e.depth))
+		: 0;

Alternatively, for a more concise version:

-	const deepestDepth =
-		events.sort((e1, e2) => e1.depth - e2.depth).pop()?.depth ?? 0;
+	const deepestDepth = Math.max(0, ...events.map(e => e.depth));

---

460-462: Consider adding a clarifying comment.

The conditional depth update using <= is correct for supporting incremental addPrevEvents calls, where depth should increase with each batch of predecessors but never decrease. However, the reasoning might not be immediately obvious to future readers.

Consider adding a brief comment:

+	// Update depth if any predecessor is deeper or equal (allows incremental predecessor additions)
 	if (this.rawEvent.depth <= deepestDepth) {
 		this.rawEvent.depth = deepestDepth + 1;
 	}

📜 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 613a536 and a06b429.

📒 Files selected for processing (2)

• packages/room/src/manager/event-wrapper.spec.ts (2 hunks)
• packages/room/src/manager/event-wrapper.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

packages/room/src/manager/event-wrapper.spec.ts (2)

packages/room/src/manager/factory.ts (1)

• PersistentEventFactory (30-123)

packages/room/src/types/v3-11.ts (1)

• Pdu (769-769)

🔇 Additional comments (2)

packages/room/src/manager/event-wrapper.spec.ts (2)

4-4: LGTM!

The import of the Pdu type is appropriate for type casting in the test cases.

---

371-399: Excellent test coverage!

This test case thoroughly validates the depth calculation logic:

• Verifies depth increases correctly with each addPrevEvents call
• Tests the cumulative effect of multiple predecessor additions (1 → 2 → 6 → 8)
• Validates order-independence by intentionally passing predecessors out of order (e5, e4)

The test confirms that depth is correctly calculated as max(predecessor_depths) + 1 regardless of input order.