Skip to content

Conversation

@acharneski
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings April 23, 2025 09:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how ChatClient instances are obtained across the codebase by eliminating explicit instantiation in favor of ApplicationServices, thereby streamlining client management. Key changes include:

  • Removing the ChatClient parameter when initializing BasicChatApp in the test server.
  • Updating BasicChatApp and DocumentParserApp to obtain ChatClient instances via ApplicationServices.
  • Improving logging messages in ClientManager for clearer client creation reporting.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
webui/src/test/kotlin/com/simiacryptus/cognotik/webui/ActorTestAppServer.kt Removed explicit ChatClient initialization to use the updated BasicChatApp constructor.
webui/src/main/kotlin/com/simiacryptus/cognotik/webui/chat/BasicChatApp.kt Eliminated the ChatClient parameter and replaced it with a call to ApplicationServices for retrieving the client.
webui/src/main/kotlin/com/simiacryptus/cognotik/apps/parse/DocumentParserApp.kt Removed the default ChatClient instance and adjusted getChildClient call with an explicit cast removal.
core/src/main/kotlin/com/simiacryptus/cognotik/core/platform/ClientManager.kt Adjusted logging message texts for clarity in client creation.
Comments suppressed due to low confidence (3)

webui/src/test/kotlin/com/simiacryptus/cognotik/webui/ActorTestAppServer.kt:42

  • The removal of the explicit ChatClient instance may lead to unexpected behavior if ApplicationServices does not provide a fully functional ChatClient for testing. Verify that the new initialization covers all necessary test scenarios.
ChildWebApp("/chat", BasicChatApp(File("."), model, parsingModel))

webui/src/main/kotlin/com/simiacryptus/cognotik/webui/chat/BasicChatApp.kt:33

  • Ensure that 'session' and 'user' are in scope and properly initialized since they are now required for obtaining the ChatClient.
api = ApplicationServices.clientManager.getChatClient(session, user),

webui/src/main/kotlin/com/simiacryptus/cognotik/apps/parse/DocumentParserApp.kt:108

  • Removal of the explicit cast on 'api' assumes that it is always a ChatClient; confirm that getChildClient returns the correct type in all scenarios to prevent potential runtime exceptions.
val api = api.getChildClient(task)

@acharneski acharneski changed the title wip 2.0.0 Apr 27, 2025
@acharneski acharneski merged commit 814623f into main Apr 27, 2025
7 checks passed
@acharneski acharneski deleted the 1.9.9.7 branch April 27, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants