[CM-2442] File Titles and Comments Update to KMChat from ALK | iOS SDK (Part - 2)#504
[CM-2442] File Titles and Comments Update to KMChat from ALK | iOS SDK (Part - 2)#504kandpal025 merged 1 commit intodevfrom
Conversation
WalkthroughThis set of changes systematically replaces references to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KMChatConversationListViewController
participant KMChatConversationViewController
participant KMChatConversationViewModel
participant KMChatCustomEventHandler
User->>KMChatConversationListViewController: Tap conversation
KMChatConversationListViewController->>KMChatConversationViewModel: Initialize with KMChat types
KMChatConversationListViewController->>KMChatConversationViewController: Push with KMChatConversationViewModel
User->>KMChatConversationViewController: Interact (e.g., resolve, rate)
KMChatConversationViewController->>KMChatCustomEventHandler: Publish event
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Sources/Kommunicate/Classes/KMConversationListViewController.swift (1)
166-182: NotificationCenter closure capturesselfstrongly – causes retain cycle
selfis captured strongly inside theobserveclosure whileselfalso ownsconverastionListNavBarItemToken.
Neither the token nor the observer is removed indeinit, so the controller will never be released.- converastionListNavBarItemToken = NotificationCenter.default.observe( - name: NSNotification.Name(KMChatNavigationItem.NSNotificationForConversationListNavigationTap), - object: nil, - queue: nil) { notification in - ... - } + converastionListNavBarItemToken = NotificationCenter.default.observe( + name: NSNotification.Name(KMChatNavigationItem.NSNotificationForConversationListNavigationTap), + object: nil, + queue: nil) { [weak self] notification in + guard let self = self else { return } + ... + }Also remember to invalidate the token in
deinitorremoveObserver().
🧹 Nitpick comments (10)
Sources/Kommunicate/Classes/ConversationDetail.swift (1)
2-2: Header filename is now misleadingThe header still points to
KMChatConversationViewModel+Extension.swift, but the physical file isConversationDetail.swift. This invites confusion when grepping for extensions or generating docs.-// KMChatConversationViewModel+Extension.swift +// ConversationDetail.swiftExample/Tests/KMConversationViewControllerTests.swift (2)
19-26: Address SwiftLint warning for NSNumber initialiserSwiftLint flags
NSNumber(integerLiteral:)calls. Prefer the saferNSNumber(value:)shortcut:-let groupId = NSNumber(integerLiteral: 100) +let groupId = NSNumber(value: 100)This avoids the “compiler-protocol init” lint and mirrors Apple’s modern API.
36-41: Follow-up: apply the same fix fornewGroupId-let newGroupId = NSNumber(integerLiteral: 101) +let newGroupId = NSNumber(value: 101)Keeps the test free of the same warning.
Sources/Kommunicate/Classes/KMAppSettingsService.swift (2)
84-99: Minor: centraliseKMChatAppSettingsconstruction
KMChatAppSettings(primaryColor:)is instantiated twice (lines 86 and 117) with largely identical follow-up mutations. Extracting a small factory/helper would eliminate duplication and ease future changes to default mutations.
112-114: iOS 15+ navigation bar appearance reset is incompleteOnly
barTintColoris cleared.
For iOS 15+ you may also need to reset thescrollEdgeAppearance/standardAppearanceproperties to avoid residual styling:navigationBarProxy.scrollEdgeAppearance = nil navigationBarProxy.standardAppearance = nilSources/Kommunicate/Classes/Models/ChatMessage.swift (1)
34-48: Parameter shadowing is acceptable but can be clearerThe initializer parameter
messageand the property names overlap heavily.
A tiny readability win:-init(message: KMChatChatViewModelProtocol) { - avatar = message.avatar +init(viewModel: KMChatChatViewModelProtocol) { + avatar = viewModel.avatar … }Not critical, just reduces mental load when scanning.
Sources/Kommunicate/Classes/KMPushNotificationHelper.swift (1)
74-76:presentingViewControllermay not always be a navigation controllerUsing
topVC.presentingViewControllerassumes that any presented controller is embedded in aKMChatBaseNavigationViewController.
For pushed flows (navigationController?.topViewController) this cast will silently fail and skip the active-thread check.Consider falling back to
topVC.navigationControllerbefore giving up:let navVC = (topVC.presentingViewController as? KMChatBaseNavigationViewController) ?? (topVC.navigationController as? KMChatBaseNavigationViewController)Sources/Kommunicate/Classes/KMConversationListViewController.swift (2)
504-517:push(conversationVC:with:)mutatestopVC.viewModelwithout thread-safetyUpdating the live conversation VC’s
viewModelfrom the list screen is fine, but it should occur on the main thread to avoid UI race conditions (MQTT callbacks can arrive on background queues).DispatchQueue.main.async { topVC.unsubscribingChannel() ... topVC.refreshViewController() }
75-80: Prefer dependency injection over singletonKMChatAppSettingsUserDefaults()Creating a new
KMChatAppSettingsUserDefaultsinstance every time the color is needed hides the dependency, makes testing harder and is wasteful.Pass an already-constructed settings object (or the required color) through the initializer instead.
Sources/Kommunicate/Classes/Kommunicate.swift (1)
1058-1065: API break:openFaqnow requiresKMChatConfigurationonlyThe old signature accepted
KMConfiguration(now a typealias). Consider keeping a deprecated overload so SDK consumers don’t get an immediate compile-error:@available(*, deprecated, renamed: "openFaq(from:with:)") open class func openFaq(from vc: UIViewController, with configuration: KMConfiguration) { openFaq(from: vc, with: configuration as KMChatConfiguration) }This maintains source compatibility while steering users to the new name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Example/Tests/KMConversationViewControllerTests.swift(2 hunks)Sources/Kommunicate/Classes/ConversationDetail.swift(1 hunks)Sources/Kommunicate/Classes/ConversationVCNavBar.swift(1 hunks)Sources/Kommunicate/Classes/Extensions/KMConfiguration+Extension.swift(1 hunks)Sources/Kommunicate/Classes/FaqViewController.swift(1 hunks)Sources/Kommunicate/Classes/KMAppSettingsService.swift(3 hunks)Sources/Kommunicate/Classes/KMConversationListViewController.swift(13 hunks)Sources/Kommunicate/Classes/KMConversationViewController.swift(7 hunks)Sources/Kommunicate/Classes/KMPushNotificationHelper.swift(4 hunks)Sources/Kommunicate/Classes/Kommunicate.swift(12 hunks)Sources/Kommunicate/Classes/MessageCharacterLimitManager.swift(1 hunks)Sources/Kommunicate/Classes/Models/ChatMessage.swift(2 hunks)Sources/Kommunicate/Classes/Views/FeedbackRatingView.swift(1 hunks)Sources/Kommunicate/Classes/Views/KMFiveStarView.swift(1 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Example/Tests/KMConversationViewControllerTests.swift
[Warning] 21-21: Initializers declared in compiler protocol ExpressibleByIntegerLiteral shouldn't be called directly
(compiler_protocol_init)
Sources/Kommunicate/Classes/Kommunicate.swift
[Error] 1324-1324: Force casts should be avoided
(force_cast)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test
- GitHub Check: Build and Test
🔇 Additional comments (17)
Sources/Kommunicate/Classes/Views/KMFiveStarView.swift (1)
72-72: Prefix migration looks correct
KMChatCustomEventHandler.sharedaligns with the new namespace; no additional changes required here.Sources/Kommunicate/Classes/ConversationVCNavBar.swift (1)
310-313: Verify asset availability after bundle switchLoading
"groupPlaceholder"from
Bundle(for: KMChatConversationListViewController.self)will fail at runtime if that asset is not copied into the chat-UI bundle generated for the new class.
- Double-check that the image exists in the KMChatConversationListViewController bundle target.
- Consider falling back to
Bundle.kommunicateto avoid returningnil:placeHolder = UIImage( named: "groupPlaceholder", in: Bundle(for: KMChatConversationListViewController.self), compatibleWith: nil ) ?? UIImage(named: "groupPlaceholder", in: Bundle.kommunicate, compatibleWith: nil)Sources/Kommunicate/Classes/Extensions/KMConfiguration+Extension.swift (1)
11-11: Extension target updated toKMChatConfigurationNamespace shift looks good; be sure all call-sites now import the KMChat-prefixed module to avoid ambiguous symbol errors in mixed targets.
Sources/Kommunicate/Classes/Views/FeedbackRatingView.swift (1)
75-75: Event handler prefix updatedConsistent with other files; change is correct.
Sources/Kommunicate/Classes/MessageCharacterLimitManager.swift (1)
27-41: ValidateKMChatChatBarAPI compatibilityThe property and initializer now depend on
KMChatChatBar. Please double-check that:
addTextView(delegate:)anddisableSendButton(isSendButtonDisabled:)still exist on the new type with the same semantics.
A silent API drift here will only surface at runtime (the compiler can’t infer the concrete type used by the host app).If the methods were renamed or their signature changed during the prefix migration, fix the invocations or add shims.
Sources/Kommunicate/Classes/FaqViewController.swift (1)
14-18: Prefix migration looks goodThe switch from
ALKConfigurationtoKMChatConfigurationis consistent and no behavioural change is introduced.Sources/Kommunicate/Classes/Models/ChatMessage.swift (1)
12-15: Confirm enum value mapping after type switch
messageTypenow usesKMChatMessageType. Ensure that each formerALKMessageTypecase has a 1-to-1 counterpart, otherwise UI branches relying on specific raw values may misbehave.Sources/Kommunicate/Classes/Kommunicate.swift (10)
84-90: Prefix migration looks consistent here. LGTM.
The renamedKMChatNavigationIteminstances compile-time-prove the migration and the FAQ colours still refer to existing configuration constants. No further action needed.
1330-1331: ViewModel rename acknowledged
KMChatConversationViewModelinjection aligns with the new namespace. No functional concerns.
1350-1357: Constructor update is consistentThe same pattern is applied when the conversation list is not shown. Looks good.
1382-1391: Embedded-mode ViewModel renameRename is consistent with previous segments—no issues spotted.
1398-1399: List-of-VCs update
KMChatConversationListViewController.description()has replaced the oldALKname as expected. Verified only a namespace change.
1470-1470: Default chat view settings list updatedMatches the earlier embedded-mode change. No additional feedback.
1479-1484: Appearance proxy scope updatedLimiting
UINavigationBarstyling toKMChatBaseNavigationViewControlleris correct after the rename. 👍
1506-1518: Event-subscription callback rename is coherent
KMChatCustomEventCallbackis correctly threaded through. Ensure public documentation is regenerated so integrators pick up the new symbol.
1613-1613: Cache clear call migrated
KMChatFormDataCache.shared.clearCache()preserves behaviour. If the old class still exists for deprecations, double-check both implementations clear the same directory to avoid ghost files.
32-36: Suspicious double-Chatin typealiasKMChatChatBarConfiguration
KMChatChatBarConfigurationlooks like an accidental duplication (ChatChat). Please verify the real symbol name inKommunicateChatUI_iOS_SDK; typo here will break the build or silently reference the wrong type.-public typealias KMChatBarConfiguration = KMChatChatBarConfiguration +public typealias KMChatBarConfiguration = KMChatBarConfiguration // or the correct single-Chat symbolIf the SDK really exposes the double-Chat identifier, consider filing an upstream rename to avoid confusion.
Likely an incorrect or invalid review comment.
| var configuration: KMChatConfiguration! | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid implicitly-unwrapped optionals for configuration
configuration is assigned in the designated initializer and is never mutated afterwards.
Declaring it as let and non-optional eliminates a potential runtime crash if it is accessed before initialization and improves type-safety.
- var configuration: KMChatConfiguration!
+ let configuration: KMChatConfiguration📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var configuration: KMChatConfiguration! | |
| let configuration: KMChatConfiguration |
🤖 Prompt for AI Agents
In Sources/Kommunicate/Classes/KMPushNotificationHelper.swift at lines 15 to 16,
change the declaration of the variable 'configuration' from an
implicitly-unwrapped optional 'var configuration: KMChatConfiguration!' to a
non-optional constant 'let configuration: KMChatConfiguration'. This requires
ensuring 'configuration' is assigned a value in the designated initializer and
is not mutated afterwards, improving type safety and preventing runtime crashes.
| let cell = tableCell as! KMChatChatCell | ||
| let message = ChatMessage(message: messageModel) | ||
| cell.update(viewModel: message, identity: nil) | ||
| cell.delegate = vc.conversationListTableViewController.self | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid force-casting to KMChatChatCell
SwiftLint rightfully flags this as! cast. A safe cast eliminates potential runtime crashes:
-let cell = tableCell as! KMChatChatCell
+guard let cell = tableCell as? KMChatChatCell else { return }Return type of the configurator closure already tolerates Void, so early-exit is fine.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cell = tableCell as! KMChatChatCell | |
| let message = ChatMessage(message: messageModel) | |
| cell.update(viewModel: message, identity: nil) | |
| cell.delegate = vc.conversationListTableViewController.self | |
| } | |
| guard let cell = tableCell as? KMChatChatCell else { return } | |
| let message = ChatMessage(message: messageModel) | |
| cell.update(viewModel: message, identity: nil) | |
| cell.delegate = vc.conversationListTableViewController.self | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 1324-1324: Force casts should be avoided
(force_cast)
🤖 Prompt for AI Agents
In Sources/Kommunicate/Classes/Kommunicate.swift around lines 1324 to 1328,
replace the force-cast 'as!' to KMChatChatCell with a safe cast 'as?'. Use a
guard or if-let statement to safely unwrap the casted cell, and return early if
the cast fails. This prevents runtime crashes by avoiding forced downcasting.
3 participants
Summary
Testing Branches
KM_ChatUI_Branch :
CM-2442KM_Core_Branch:
devSummary by CodeRabbit