feat: chat UI#1112
feat: chat UI#1112leonardmq wants to merge 6 commits intoleonard/kil-447-feat-stream-multiturn-ai-sdk-openai-protocolsfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
📊 Coverage ReportOverall Coverage: 90% Diff: origin/leonard/kil-447-feat-stream-multiturn-ai-sdk-openai-protocols...HEAD
Summary
Line-by-lineView line-by-line diff coverageapp/desktop/studio_server/chat_api.pyLines 28-46 28
29
30 def _find_task_run_by_id(task_run_id: str) -> TaskRun | None:
31 """Search all projects and tasks for a task run with the given ID."""
! 32 project_paths = Config.shared().projects or []
! 33 for project_path in project_paths:
! 34 try:
! 35 project = Project.load_from_file(project_path)
! 36 except Exception:
! 37 continue
! 38 for task in project.tasks():
! 39 run = TaskRun.from_id_and_parent_path(task_run_id, task.path)
! 40 if run is not None:
! 41 return run
! 42 return None
43
44
45 def _execute_client_tool(tool_name: str, arguments: dict[str, Any]) -> str:
46 """Execute a client-side tool and return the result as a string."""Lines 52-61 52 run = _find_task_run_by_id(task_run_id)
53 if run is None:
54 return json.dumps({"error": f"Task run not found: {task_run_id}"})
55 return run.model_dump_json(indent=2)
! 56 except Exception as e:
! 57 return json.dumps({"error": f"Failed to read task run: {e}"})
58 return json.dumps({"error": f"Unknown client tool: {tool_name}"})
59
60
61 def _parse_sse_events(Lines 79-88 79 and event.get("type") == "client-tool-call"
80 ):
81 client_tool_event = event
82 continue
! 83 except (json.JSONDecodeError, TypeError):
! 84 pass
85 lines_to_forward.append(line)
86
87 return lines_to_forward, client_tool_eventLines 120-129 120 detail = (
121 json.loads(error_body).get("message", detail)
122 or detail
123 )
! 124 except json.JSONDecodeError:
! 125 pass
126 yield f"data: {json.dumps({'type': 'error', 'message': detail})}\n\n".encode()
127 return
128
129 try:Lines 133-147 133 client_tool_event = tool_event
134 forward_bytes = b"\n".join(lines)
135 if forward_bytes.strip():
136 yield forward_bytes + b"\n"
! 137 except httpx.RemoteProtocolError:
! 138 if client_tool_event is not None:
! 139 logger.debug(
140 "Connection closed after client tool call event (expected)"
141 )
142 else:
! 143 raise
144
145 if client_tool_event is None:
146 returnlibs/core/kiln_ai/adapters/chat/chat_formatter.pyLines 274-282 274 self._prior_trace = prior_trace
275
276 def _is_tool_continuation(self) -> bool:
277 if not self._prior_trace:
! 278 return False
279 last = self._prior_trace[-1]
280 return isinstance(last, dict) and last.get("role") == "tool"
281
282 def initial_messages(self) -> list[ChatCompletionMessageIncludingLiteLLM]:libs/core/kiln_ai/adapters/model_adapters/base_adapter.pyLines 692-700 692 @property
693 def task_run(self) -> TaskRun:
694 if self._task_run is None:
695 if self.client_tool_pending:
! 696 raise RuntimeError(
697 "No task_run available: stream ended with a client tool call. "
698 "Check .client_tool_pending before accessing .task_run"
699 )
700 raise RuntimeError(Lines 732-742 732 elif isinstance(event, ToolCallEvent):
733 last_event_was_tool_call = True
734 for ai_event in converter.convert_tool_event(event):
735 yield ai_event
! 736 except ClientToolCallRequired as e:
! 737 self.client_tool_pending = True
! 738 yield AiSdkStreamEvent(
739 AiSdkEventType.CLIENT_TOOL_CALL,
740 {
741 "toolCallId": e.tool_call_id,
742 "toolName": e.tool_name,Lines 742-753 742 "toolName": e.tool_name,
743 "input": e.arguments,
744 },
745 )
! 746 for ai_event in converter.finalize():
! 747 yield ai_event
! 748 yield AiSdkStreamEvent(AiSdkEventType.FINISH_STEP)
! 749 return
750
751 for ai_event in converter.finalize():
752 yield ai_eventlibs/core/kiln_ai/adapters/model_adapters/litellm_adapter.pyLines 698-707 698
699 try:
700 result = await t.run(c, **args)
701 except ClientToolCallRequired as e:
! 702 e.tool_call_id = tc_id
! 703 raise
704 return ChatCompletionToolMessageParamWrapper(
705 role="tool",
706 tool_call_id=tc_id,
707 content=result.output,libs/core/kiln_ai/datamodel/tool_id.pyLines 85-93 85 # Client tools: client_tool::<tool_name>
86 if id.startswith(CLIENT_TOOL_ID_PREFIX):
87 tool_name = client_tool_name_from_id(id)
88 if not tool_name:
! 89 raise ValueError(
90 f"Invalid client tool ID: {id}. Expected format: 'client_tool::<tool_name>'."
91 )
92 return idlibs/core/kiln_ai/tools/client_tool.pyLines 53-61 53 async def name(self) -> str:
54 return self._name
55
56 async def description(self) -> str:
! 57 return self._description
58
59 async def toolcall_definition(self) -> ToolCallDefinition:
60 return {
61 "type": "function",
|
|
Bug:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, fully functional chat interface within the application. The core motivation for this feature was to provide an interactive AI chat experience, which necessitated a custom streaming implementation due to existing framework version constraints. The new UI integrates robust markdown rendering with security features, ensuring a rich and safe display of AI-generated content. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new chat UI, including dependencies, a markdown rendering component, client-side logic for streaming chat, and the chat page itself. A critical security concern has been identified: the DOMPurify configuration is overly permissive, allowing attributes like class and target which can be exploited for UI redressing and phishing attacks. Additionally, raw error messages from the backend API are exposed, potentially leading to information leakage. Beyond security, there are also opportunities to improve code maintainability by reducing redundancy.
| "span", | ||
| "div", | ||
| ] | ||
| const ALLOWED_ATTR = ["href", "target", "rel", "class"] |
There was a problem hiding this comment.
The DOMPurify configuration allows the class attribute on all allowed tags. When using a utility-first CSS framework like Tailwind CSS, this allows an attacker to inject arbitrary utility classes into the rendered markdown. An attacker can use this to perform UI redressing, such as overlaying fake buttons, hiding parts of the UI, or mimicking system notifications, which can lead to phishing or other social engineering attacks.
| "span", | ||
| "div", | ||
| ] | ||
| const ALLOWED_ATTR = ["href", "target", "rel", "class"] |
There was a problem hiding this comment.
The DOMPurify configuration allows the target attribute on <a> tags. An attacker can use this to set target="_self" or target="_top", allowing them to navigate the current tab or the entire window to a malicious site when the user clicks a link in the chat. This can be used for phishing attacks where the attacker mimics the application's UI on their own site.
| const text = await response.text() | ||
| onError( | ||
| new Error( | ||
| `Chat API error ${response.status}: ${text || response.statusText}`, |
There was a problem hiding this comment.
The streamChat function captures the raw response body from the API when a request fails and includes it in the Error object passed to the onError callback. This error message is then displayed directly in the UI in +page.svelte. If the backend API returns sensitive information (e.g., system paths, stack traces, or internal configuration) in the error response, it will be exposed to the user.
| hljs.registerLanguage("json", json) | ||
| hljs.registerLanguage("javascript", javascript) | ||
| hljs.registerLanguage("js", javascript) | ||
| hljs.registerLanguage("typescript", typescript) | ||
| hljs.registerLanguage("ts", typescript) | ||
| hljs.registerLanguage("python", python) | ||
| hljs.registerLanguage("py", python) | ||
| hljs.registerLanguage("bash", bash) | ||
| hljs.registerLanguage("shell", bash) | ||
| hljs.registerLanguage("sh", bash) |
There was a problem hiding this comment.
The language packs for highlight.js often include common aliases. For example, the javascript language pack includes the js alias, typescript includes ts, python includes py, and bash includes sh. Registering these aliases explicitly is redundant. You can simplify this section by removing the redundant registerLanguage calls.
hljs.registerLanguage("json", json)
hljs.registerLanguage("javascript", javascript)
hljs.registerLanguage("typescript", typescript)
hljs.registerLanguage("python", python)
hljs.registerLanguage("bash", bash)
hljs.registerLanguage("shell", bash)
| if (currentTextId === null) { | ||
| const id = nextSlotId() | ||
| partOrder.push({ kind: "text", id }) | ||
| currentTextId = id | ||
| textBlocks.set(id, "") | ||
| } |
There was a problem hiding this comment.
The logic to create a new text block when currentTextId is null is duplicated in multiple places (here, and in the text-delta handler). A similar duplication exists for reasoning blocks. This makes the code harder to maintain.
Consider refactoring this logic into a helper function. For example, a function like ensureCurrentTextBlock() could encapsulate this block creation logic and be called wherever needed. This would reduce code duplication and make the stream processing logic easier to follow.
leonardmq
force-pushed
the
leonard/kil-449-feat-svelte-ui-for-chatbot
branch
from
2bfe312 to
cdd99bd
Compare
leonardmq
force-pushed
the
leonard/kil-449-feat-svelte-ui-for-chatbot
branch
from
d411063 to
0bbab04
Compare
1 participant
What does this PR do?
Vercel AI SDK has libraries for Svelte, but we use Svelte 4, and those libraries require >= Svelte 5. An ancient version of the library exists for Svelte 4, but it is tied to an older version of the protocol so would require our backend / Kiln SDK to also be on an old protocol version.
Changes:
Checklists