Skip to content

Conversation

@AndyTWF
Copy link
Collaborator

@AndyTWF AndyTWF commented Oct 28, 2025

These were omitted and mapped to failed. This change aligns with PubSub.

CHA-1225

Summary by CodeRabbit

  • Bug Fixes
    • Connection lifecycle now distinguishes "closing" and "closed" states so they are reported accurately instead of being marked as failures, improving status visibility and reconnect behavior.
  • Documentation
    • Clarified descriptions for connection states to explain client-initiated closure and post-closure behavior.

@AndyTWF AndyTWF requested a review from ttypic October 28, 2025 19:54
@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

Added two ConnectionStatus enum values, Closing and Closed, and updated the internal mapping so PubSub closingConnectionStatus.Closing and closedConnectionStatus.Closed (previously both mapped to Failed).

Changes

Cohort / File(s) Summary
Connection enum & mapping
chat/src/commonMain/kotlin/com/ably/chat/Connection.kt
Added Closing("closing") and Closed("closed") to ConnectionStatus; updated mapping so PubSub closingConnectionStatus.Closing and closedConnectionStatus.Closed (previously mapped to Failed).
Public API enums (platform artifacts)
chat/api/android/chat.api, chat/api/jvm/chat.api
Exposed new enum constants Closing and Closed on the public com/ably/chat/ConnectionStatus enum in platform API artifacts.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant PubSub as PubSub
  participant Mapper as ConnectionStateMapper
  participant Chat as ConnectionStatus (Chat)

  rect rgb(245,250,255)
    PubSub->>Mapper: emits ConnectionState (opening/connected/closing/closed/failed)
    note right of Mapper #e6f2ff: map PubSub → Chat statuses
  end

  alt state == closing
    Mapper->>Chat: ConnectionStatus.Closing
  else state == closed
    Mapper->>Chat: ConnectionStatus.Closed
  else other states
    Mapper->>Chat: ConnectionStatus.(Opening/Connected/Failed/...)
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Pay attention to:
    • chat/src/commonMain/kotlin/com/ably/chat/Connection.kt — enum docs and mapping correctness.
    • chat/api/* artifacts — ensure API generation matches enum changes.
    • Consumer switch/when sites that assume exhaustive enums.

Poem

🐰 I hopped through code and found two more,
Closing and Closed — snug by the door.
PubSub now whispers the same little tune,
I nibble a carrot and hum by the moon. 🥕✨

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 (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "connection: add missing closed/closing values to enum" directly and clearly describes the main change: adding the missing Closing and Closed enum values to ConnectionStatus. The title is concise, specific, and accurately reflects the primary objective without unnecessary verbosity or vague language. A reviewer scanning the history would immediately understand that this PR adds missing enum values to the connection status type.
Linked Issues Check ✅ Passed The PR successfully addresses the coding requirements from linked issue CHA-1225. It adds the missing Closing and Closed enum constants to ConnectionStatus with appropriate state names ("closing" and "closed"), and updates the internal mapping so these states no longer map to Failed but instead map to their corresponding enum values. This aligns the behavior with PubSub as required. The non-coding requirement regarding a minor SemVer release is disregarded per instructions focused on coding-related requirements.
Out of Scope Changes Check ✅ Passed All changes in the pull request are directly in scope and related to the stated objectives. The modifications include adding two new enum constants to ConnectionStatus in Connection.kt, updating the PubSub state mapping logic, and reflecting these changes in the Android and JVM API declaration files. No extraneous changes, unrelated modifications, or side effects are evident; all alterations serve the single purpose of adding the missing enum values.
✨ 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 connection-status-missing-values

📜 Recent 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 497761d and 39b04ad.

📒 Files selected for processing (3)
  • chat/api/android/chat.api (1 hunks)
  • chat/api/jvm/chat.api (1 hunks)
  • chat/src/commonMain/kotlin/com/ably/chat/Connection.kt (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • chat/src/commonMain/kotlin/com/ably/chat/Connection.kt
  • chat/api/android/chat.api
⏰ 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: coverage
  • GitHub Check: build
  • GitHub Check: test (JVM tests, jvmTest)
  • GitHub Check: test (Android Release tests, testReleaseUnitTest)
  • GitHub Check: test (Android Debug tests, testDebugUnitTest)
  • GitHub Check: check
🔇 Additional comments (2)
chat/api/jvm/chat.api (2)

56-57: LGTM! Missing enum values properly added.

The addition of Closed and Closing enum values correctly addresses the issue where these PubSub connection states were previously being mapped to Failed. The alphabetical ordering is consistent with the existing enum structure.


56-68: No issues found - concerns are not substantiated by codebase analysis.

Verification confirms:

  • No ordinal() calls anywhere in the codebase
  • No exhaustive when expressions on ConnectionStatus requiring updates
  • Usage is limited to simple equality comparisons (e.g., if (it.current == ConnectionStatus.Connected))

The reordering of enum constants to alphabetical order does not pose risks to the codebase.


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.

@github-actions github-actions bot temporarily deployed to staging/pull/184/dokka October 28, 2025 19:58 Inactive
@AndyTWF AndyTWF force-pushed the connection-status-missing-values branch from 1e42ad7 to 497761d Compare October 28, 2025 20:02
@github-actions
Copy link

github-actions bot commented Oct 28, 2025

Code Coverage

File Coverage [92.31%]
chat/src/commonMain/kotlin/com/ably/chat/Connection.kt 92.31%
Total Project Coverage 85.41%

@github-actions github-actions bot temporarily deployed to staging/pull/184/dokka October 28, 2025 20:09 Inactive
These were omitted and mapped to failed. This change aligns with PubSub.

[CHA-1225]
Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ttypic ttypic merged commit 3643660 into main Oct 30, 2025
7 checks passed
@ttypic ttypic deleted the connection-status-missing-values branch October 30, 2025 14:35
@coderabbitai coderabbitai bot mentioned this pull request Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants