Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR migrates the codebase from "jetpack" to "chat_android" language annotations across UI and documentation. It adds chat_android support in language resolution logic, updates configuration files, and replaces all jetpack code blocks and language guards with chat_android/android equivalents in 13 chat MDX files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Updates the Chat documentation snippet language tagging to treat Android (Jetpack Compose) examples as an Android variant with Kotlin highlighting, addressing DX-553.
Changes:
- Replaced
jetpacklanguage conditionals withandroidacross Chat MDX pages. - Migrated Jetpack/Compose code fences to a new
chat_androidsnippet key. - Updated language configuration (
types,languageInfo,languageData) to supportchat_androidand includeandroidfor Chat.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/docs/chat/setup.mdx | Switches Jetpack conditionals/snippets to Android + chat_android for setup examples. |
| src/pages/docs/chat/rooms/typing.mdx | Updates Android-specific typing indicator sections/snippets to use android + chat_android. |
| src/pages/docs/chat/rooms/replies.mdx | Migrates Compose-related code fences to chat_android. |
| src/pages/docs/chat/rooms/reactions.mdx | Migrates Compose-related code fences and conditionals to android + chat_android. |
| src/pages/docs/chat/rooms/presence.mdx | Migrates presence Compose snippets/conditionals to android + chat_android. |
| src/pages/docs/chat/rooms/occupancy.mdx | Migrates occupancy Compose snippets/conditionals to android + chat_android. |
| src/pages/docs/chat/rooms/messages.mdx | Migrates message Compose snippets/conditionals to android + chat_android. |
| src/pages/docs/chat/rooms/message-reactions.mdx | Migrates message reaction snippets/conditionals to android + chat_android. |
| src/pages/docs/chat/rooms/media.mdx | Migrates media Compose snippets to chat_android. |
| src/pages/docs/chat/rooms/index.mdx | Migrates room lifecycle snippets/conditionals to android + chat_android. |
| src/pages/docs/chat/rooms/history.mdx | Migrates history Compose snippets/conditionals to android + chat_android. |
| src/pages/docs/chat/getting-started/android.mdx | Migrates Android getting-started Kotlin snippets to chat_android. |
| src/pages/docs/chat/connect.mdx | Migrates connection-status Compose snippets/conditionals to android + chat_android. |
| src/data/languages/types.ts | Replaces jetpack language key with chat_android in the LanguageKey set. |
| src/data/languages/languageInfo.ts | Adds chat_android language info (Android label, Kotlin highlighter). |
| src/data/languages/languageData.ts | Switches Chat product language key from jetpack to android. |
| jetpack_to_android.md | Adds an implementation plan/migration notes for the language-key change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| swift: '1.0', | ||
| kotlin: '1.1', | ||
| jetpack: '1.1', | ||
| android: '1.1', |
There was a problem hiding this comment.
chat_android code fences are now being used in Chat MDX pages, but languageData.chat is keyed on android. This only works if the runtime language normalization (via stripSdkType from @ably/ui) strips the chat_ prefix and the CodeSnippet component can map the active android selection back to chat_android blocks. If @ably/ui in this repo doesn’t yet support chat_ prefixes, Android snippets/conditionals will fail to render. Please ensure the required @ably/ui changes/version bump lands alongside this PR (or adjust the docs to use the currently supported language key).
| android: '1.1', | |
| android: '1.1', | |
| chat_android: '1.1', |
| @@ -26,7 +26,7 @@ export const languageKeys = [ | |||
| 'css', | |||
| 'laravel', | |||
| 'typescript', | |||
There was a problem hiding this comment.
languageKeys contains typescript twice. This is harmless at runtime but adds noise and can make grepping / maintenance harder; consider removing the duplicate entry so the list stays canonical.
| 'typescript', |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/pages/docs/chat/getting-started/android.mdx (1)
966-971:⚠️ Potential issue | 🟡 MinorLinks point to
?lang=kotlin— should these use?lang=androidinstead?Since this is the Android getting-started guide, users following these links would be switched to the Kotlin tab rather than the Android tab on the target pages. If those pages have
chat_androidcode blocks (which many do per this PR), the links should arguably use?lang=androidto maintain continuity.src/pages/docs/chat/rooms/media.mdx (1)
609-638:⚠️ Potential issue | 🟡 MinorThe
chat_androidsnippet usescoil.compose.AsyncImage(line 613) without mentioning the Coil dependency.Users following this documentation would get a compilation error since Coil isn't included in the project dependencies mentioned earlier. Consider adding a note about the required Coil dependency, or use a placeholder/comment like the
kotlinblock does (line 595-596:// Load image from URL (using Coil, Glide, or Picasso)).src/pages/docs/chat/rooms/messages.mdx (2)
339-364:⚠️ Potential issue | 🟡 MinorUninitialized
valinside@Composablefunction won't compile.Line 351:
val originalMessage: Message // assume this is available— this is a declaration without initialization, which is a Kotlin compile error. In thekotlinblock (Line 332–337),originalMessageis similarly declared but in a non-Composable context where it reads as pseudocode. Inside a@Composablefunction with a full component structure, this looks like broken code.Consider accepting
originalMessageas a parameter or using aremember/state variable.Proposed fix
`@Composable` -fun MyComponent(room: Room) { +fun MyComponent(room: Room, originalMessage: Message) { val coroutineScope = rememberCoroutineScope() - val originalMessage: Message // assume this is available
594-618:⚠️ Potential issue | 🟡 MinorSame uninitialized
valissue in delete message sample.Line 605:
val messageToDelete: Message // assume this is availablehas the same compile error as the update sample. Should be a function parameter.Proposed fix
`@Composable` -fun MyComponent(room: Room) { +fun MyComponent(room: Room, messageToDelete: Message) { val coroutineScope = rememberCoroutineScope() - val messageToDelete: Message // assume this is availablesrc/pages/docs/chat/rooms/presence.mdx (1)
213-242:⚠️ Potential issue | 🟡 MinorIncomplete
DisposableEffectwith emptyonDisposeis confusing.Lines 234–240 show a
DisposableEffectwith an emptyonDisposeand a comment suggesting the developer should "consider using a coroutine scope." This is unhelpful in documentation — it introduces a pattern without completing it, leaving the reader unsure how to properly leave presence on disposal.Either show a working pattern (e.g., using
rememberCoroutineScopeorLaunchedEffectwithtry/finally) or remove theDisposableEffectblock entirely and add a note explaining that presence leave on disposal requires additional handling.Proposed fix — use try/finally in LaunchedEffect
`@Composable` fun EnterPresenceComponent(room: Room) { LaunchedEffect(room) { - // Ensure room is attached before entering presence - if (room.status != RoomStatus.Attached) { - room.attach() - } - - room.presence.enter( - jsonObject { - put("status", "Online") - } - ) - } - - // Leave presence when component is disposed - DisposableEffect(room) { - onDispose { - // Note: Consider using a coroutine scope for leave() - // as DisposableEffect doesn't support suspend functions directly + try { + // Ensure room is attached before entering presence + if (room.status != RoomStatus.Attached) { + room.attach() + } + room.presence.enter( + jsonObject { + put("status", "Online") + } + ) + // Keep the coroutine alive until disposal + kotlinx.coroutines.awaitCancellation() + } finally { + room.presence.leave() } } }
🧹 Nitpick comments (13)
src/pages/docs/chat/rooms/replies.mdx (1)
194-204: Thechat_androidprepareReplyfunction is nearly identical to thekotlinblock (lines 183–192).The only difference is the added import line. This is acceptable for documentation to ensure tab content isn't empty, but if the intent is to differentiate Android snippets, consider whether a Compose-based variant would be more useful here.
src/pages/docs/chat/rooms/media.mdx (1)
97-121: Thechat_androiduploadMediafunction is identical to thekotlinblock (lines 71–95).Same observation as in replies.mdx — this is acceptable for tab completeness but adds no Android/Compose-specific value.
src/pages/docs/chat/rooms/message-reactions.mdx (2)
871-892: Inconsistent subscription pattern in raw reactions Android sample.The summary reactions subscription (Line 593) uses the
asFlow().collectpattern withLaunchedEffect, but the raw reactions subscription here uses the callback-basedsubscribeRawwithDisposableEffect. IfsubscribeRawdoesn't have a Flow equivalent, consider adding a brief comment explaining why the pattern differs to avoid confusion for developers reading both snippets.
62-68: Severalchat_androidblocks are identical to theirkotlincounterparts.Lines 62–68 and 805–811 (and similarly Line 522–550) contain
chat_androidcode blocks that are exact duplicates of the adjacentkotlinblocks. While this is part of the migration strategy to show a separate Android tab, these identical snippets provide no additional value to the reader and will create a maintenance burden — every future edit must be mirrored in both blocks.Consider whether non-Compose snippets (pure Kotlin API calls with no UI) truly need a separate
chat_androidvariant. Reservingchat_androidfor Compose-specific examples (like Lines 173–208, 593–604) would reduce duplication.Also applies to: 805-811
src/pages/docs/chat/rooms/history.mdx (1)
75-86: Duplicate of Kotlin block — same concern as in message-reactions.mdx.This
chat_androidblock is byte-for-byte identical to thekotlinblock above (Lines 62–73). Same recommendation: consider omittingchat_androidvariants that contain no Compose-specific code.src/pages/docs/chat/connect.mdx (1)
60-62: Same duplication pattern —chat_androidblock identical tokotlin.
val connectionStatus = chatClient.connection.statusis pure Kotlin with no Compose involvement. Same recommendation as other files.src/pages/docs/chat/rooms/index.mdx (1)
74-76: Multiplechat_androidblocks are exact duplicates ofkotlinblocks.Six
chat_androidcode blocks in this file are identical to theirkotlincounterparts (e.g.,rooms.get,rooms.release,room.attach,room.detach,room.status). This amplifies the maintenance burden across the docs. Consider a consolidated approach where only Compose-specific examples get a dedicatedchat_androidblock.Also applies to: 128-140, 175-177, 229-231, 260-262, 334-336
src/pages/docs/chat/rooms/messages.mdx (1)
272-274: Duplicate of Kotlin block —messages.get()call has no Compose-specific content.src/pages/docs/chat/rooms/occupancy.mdx (1)
206-208: Two morechat_androidblocks identical tokotlin—occupancy.currentandoccupancy.get().Same duplication concern as across other files.
Also applies to: 255-257
src/pages/docs/chat/rooms/typing.mdx (1)
335-337:chat_androidblock identical tokotlinfortyping.current.Same duplication concern.
src/pages/docs/chat/rooms/presence.mdx (1)
498-504: Two morechat_androidblocks identical tokotlin—presence.get()andisUserPresent().Same duplication concern as across other files.
Also applies to: 522-524
jetpack_to_android.md (2)
348-381: Specify language for the data flow diagram code fence.The data flow walkthrough is excellent and provides valuable end-to-end verification of the approach. However, the fenced code block at line 348 should specify a language to satisfy the linter.
📝 Suggested fix
-``` +```text MDX file: ```chat_android
325-393: Consider adding testing and verification strategy.The implementation plan is comprehensive, but it would benefit from a section describing how to test and verify the changes work correctly. Given the complexity and cross-repository coordination, consider adding:
- Manual testing checklist: Verify code blocks render with Kotlin highlighting, language selector shows "Android",
<If>conditionals display correctly, etc.- Automated tests: If applicable, unit tests for
stripSdkType(),getLanguageInfo(), andactiveLanguageresolution logic- Visual regression tests: Screenshots comparing before/after rendering of chat pages
- Verification in both contexts: Test that pub/sub
androidpages still work with Java highlightingThe risk assessment (lines 383-393) is thorough, but explicit test cases would provide additional confidence in the migration.
|
closing in favor of #3195 |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation