You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "voidmatcha (via GitHub)" <gi...@apache.org> on 2026/10/09 01:29:13 UTC
[PR] [ZEPPELIN-6741] Define shared Assistant UI contracts and message history behavior [zeppelin]
voidmatcha opened a new pull request, #5561:
URL: https://github.com/apache/zeppelin/pull/5561
### What is this PR for?
Assistant UI와 host 사이에서 사용하는 공용 타입과 메시지 기록 동작을 정의합니다. 후속 UI 작업이 같은 계약을 기준으로 진행될 수 있도록 합니다.
Depends on #5558.
### What type of PR is it?
Improvement
### Todos
* [ ] - CI 결과 확인
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6741
### How should this be tested?
`zeppelin-web-angular/`에서 실행합니다.
```bash
npm run build-project:sdk
npm run typecheck:react
npm --prefix projects/zeppelin-react test -- src/entities/assistant/model/messageHistory.spec.ts
```
### Screenshots (if appropriate)
N/A
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285284034
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** The transport reverses the server's newest-first page so messages are oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load older messages; null at the start of the conversation. */
+ cursor: string | null;
+}
+export interface AssistantRunBody {
+ content: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ | { type: 'tool_call.done'; toolCallId: string };
Review Comment:
Just for reference, the server wraps the event data in payload. See [AssistantEventPayload](https://github.com/apache/zeppelin/blob/f1bb1b9a6e3b0ea4858f6a33f3089b2d455f7916/zeppelin-server/src/main/java/org/apache/zeppelin/service/assistant/AssistantEventPayload.java#L23)
Also, `run.failed` provides an error status rather than a message field.
For example:
```json
{
"conversationId": "...",
"type": "run.failed",
"payload": {
"runId": "...",
"error": {
"status": 500
}
}
}
```
We could add a `code` for more specific handling later if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285322920
##########
zeppelin-web-angular/projects/zeppelin-sdk/src/assistant-ui.ts:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/** The notebook sidebar's width: its default and its resize range. The assistant panel shares it. */
+export const NOTEBOOK_SIDEBAR_WIDTH = { initial: 370, min: 280, max: 800 } as const;
+
+/** Result of a host reveal: scrolled to it, already visible, or not rendered. */
+export type AssistantRevealResult = 'shown' | 'visible' | 'missing';
Review Comment:
When would this type be used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285139735
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
Review Comment:
The server’s `listMessages()` returns messages newest first, but this comment says “oldest first.” Should these match?
For example, assuming m5 is the newest message, the server returns `[m5, m4, m3]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#issuecomment-6220989246
Merged into master (c10a4a8e).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285406032
##########
zeppelin-web-angular/projects/zeppelin-sdk/src/assistant-ui.ts:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/** The notebook sidebar's width: its default and its resize range. The assistant panel shares it. */
+export const NOTEBOOK_SIDEBAR_WIDTH = { initial: 370, min: 280, max: 800 } as const;
+
+/** Result of a host reveal: scrolled to it, already visible, or not rendered. */
+export type AssistantRevealResult = 'shown' | 'visible' | 'missing';
Review Comment:
This type is for the `revealParagraph` callback planned in [ZEPPELIN-6749](https://issues.apache.org/jira/browse/ZEPPELIN-6749). The Angular host would report whether a linked paragraph was scrolled into view, was already visible, or could not be found, and the React panel would announce the `missing` case. Since it is not consumed in this PR, I removed the declaration here and will define the result type alongside that callback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285166970
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
Review Comment:
Good catch. The server returns messages newest first, while AssistantMessagePage is the UI-facing result after the transport reverses that order. In #5563, getMessages turns [m5, m4, m3] into [m3, m4, m5] before returning it. I updated the contract comment to make this normalization explicit, and the transport test covers the ordering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285166970
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
Review Comment:
Good catch. The server returns messages newest first, while AssistantMessagePage is the UI-facing result after the transport reverses that order. In #5563, getMessages turns [m5, m4, m3] into [m3, m4, m5] before returning it. I updated the contract comment to make this normalization explicit, and the transport test covers the ordering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285565827
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/messageHistory.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type { AssistantAnswerMessage, AssistantMessage, AssistantToolCallRef } from './assistantContract';
+
+export const answerText = (answer: AssistantAnswerMessage): string =>
+ answer.segments
+ .map(segment => segment.content)
+ .filter(Boolean)
+ .join('\n\n');
+
+export const answerToolCalls = (answer: AssistantAnswerMessage): AssistantToolCallRef[] =>
+ Array.from(
+ new Map(answer.segments.flatMap(segment => segment.toolCalls ?? []).map(call => [call.id, call])).values()
+ );
+
+export const mergeAnswers = (
+ earlier: AssistantAnswerMessage,
+ later: AssistantAnswerMessage
+): AssistantAnswerMessage => {
+ const segments = Array.from(
+ new Map([...earlier.segments, ...later.segments].map(segment => [segment.id, segment])).values()
+ );
+ return {
+ ...later,
+ segments
+ };
Review Comment:
Thanks for the careful review. `toolCalls` live on each `segment`, so the existing merge retains the calls from both pages. I agree that the top-level merge policy should be explicit, so I replaced `...later` with `id`, `role`, and `segments`. All five message-history tests pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4284161086
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/messageHistory.spec.ts:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { describe, expect, it } from 'vitest';
+
+import type { AssistantMessage } from './assistantContract';
+import { joinPages } from './messageHistory';
+
+describe('joinPages', () => {
+ it('joins a reply split across pages under the later id and retains both action logs', () => {
+ const earlier: AssistantMessage[] = [
+ { id: 'u1', role: 'user', content: 'Read this notebook' },
+ { id: 'a1', role: 'assistant', content: 'Looking.', toolCalls: [{ id: 'c1', name: 'read' }] }
+ ];
+ const later: AssistantMessage[] = [
+ { id: 'a2', role: 'assistant', content: 'Done.', toolCalls: [{ id: 'c2', name: 'summarize' }] }
+ ];
+ expect(joinPages(earlier, later)).toEqual([
+ earlier[0],
+ {
+ id: 'a2',
+ role: 'assistant',
+ content: 'Looking.\n\nDone.',
+ toolCalls: [
+ { id: 'c1', name: 'read' },
+ { id: 'c2', name: 'summarize' }
+ ]
+ }
+ ]);
+ expect(earlier[1].content).toBe('Looking.');
+ expect(later[0].content).toBe('Done.');
+ });
+
+ it('retains a tool-only reply across an empty intermediate page and deduplicates repeated calls', () => {
+ const call = { id: 'c1', name: 'read' };
+ const earlier: AssistantMessage[] = [{ id: 'a1', role: 'assistant', content: '', toolCalls: [call] }];
+ const later: AssistantMessage[] = [{ id: 'a2', role: 'assistant', content: 'Done.', toolCalls: [call] }];
+ expect(joinPages(joinPages(earlier, []), later)).toEqual([
+ { id: 'a2', role: 'assistant', content: 'Done.', toolCalls: [call] }
+ ]);
+ });
+
+ it('keeps separate turns apart and handles empty history', () => {
+ const earlier: AssistantMessage[] = [{ id: 'a1', role: 'assistant', content: 'First answer' }];
+ const later: AssistantMessage[] = [
+ { id: 'u2', role: 'user', content: 'Another question' },
+ { id: 'a2', role: 'assistant', content: 'Another answer' }
+ ];
+ expect(joinPages(earlier, later)).toEqual([...earlier, ...later]);
Review Comment:
The most common page boundary is a question at the end of the earlier page and its answer at the start of the later one, but no test covers it. All tests still pass after removing `last?.role === 'assistant' &&` from `joinPages`, which would merge the question into the answer. How about adding this case?
```suggestion
expect(joinPages(earlier, later)).toEqual([...earlier, ...later]);
expect(joinPages([later[0]], [later[1]])).toEqual(later);
```
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
+}
+export interface AssistantMessage {
+ id: string;
+ role: 'user' | 'assistant';
+ content: string;
+ /** From history: the tools the server called before this answer. Live runs report them as events instead. */
+ toolCalls?: AssistantToolCallRef[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+/** One page of a conversation's history, oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load the page before this one; null when this page starts the conversation. */
+ earlierCursor: string | null;
+}
+export interface AssistantRunBody {
+ prompt: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ // The server does not repeat the tool name on completion.
+ | { type: 'tool_call.done'; toolCallId: string; name?: string };
Review Comment:
The server's `tool_call.done` payload (`AssistantEventPayload.ToolCallDone`) only has `toolCallId` and `result`, so this `name` is always `undefined`. #5567 already made the tool name optional with a `'Tool'` fallback label because of it. Since `tool_call.started` always carries the name, how about dropping it here? `mapSocketEvent` in #5563 would need the same change.
```suggestion
| { type: 'tool_call.done'; toolCallId: string };
```
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
+}
+export interface AssistantMessage {
+ id: string;
+ role: 'user' | 'assistant';
+ content: string;
+ /** From history: the tools the server called before this answer. Live runs report them as events instead. */
+ toolCalls?: AssistantToolCallRef[];
Review Comment:
When one answer spans several rounds (text, tool, text, tool), this shape merges them into a single `content` and a single `toolCalls`, so the order between text and tools is lost. The server history keeps that order, and the Direction in the frontend proposal (a renderer per tool inside the action log, the approval flow) may later need the answer shown step by step. Changing it then would change the shape of this contract, and the transport and UI along with it. How about keeping each round as `segments` instead of merging them, and joining them in the current UI? If merging is intended, could you share why?
##########
zeppelin-web-angular/projects/zeppelin-sdk/src/public-api.ts:
##########
@@ -11,6 +11,8 @@
*/
export * from './angular-object';
+export * from './assistant-ui';
export * from './interfaces/public-api';
export * from './message';
+
Review Comment:
I think this blank line can be removed.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#issuecomment-6196257251
@seung-00 Thank you for the thorough review. I removed the duplicated fields and aligned the request and response contracts with the server. #5563 converts the server’s newest-first messages to chronological order for the UI. The ordering and client-only abort behavior are documented and covered by tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285405681
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** The transport reverses the server's newest-first page so messages are oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load older messages; null at the start of the conversation. */
+ cursor: string | null;
+}
+export interface AssistantRunBody {
+ content: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ | { type: 'tool_call.done'; toolCallId: string };
Review Comment:
Thanks for the clarification. is the UI-facing event, not the raw WebSocket event. The follow-up transport in #5563 reads and maps it to the field; this is covered by its tests.
##########
zeppelin-web-angular/projects/zeppelin-sdk/src/assistant-ui.ts:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/** The notebook sidebar's width: its default and its resize range. The assistant panel shares it. */
+export const NOTEBOOK_SIDEBAR_WIDTH = { initial: 370, min: 280, max: 800 } as const;
+
+/** Result of a host reveal: scrolled to it, already visible, or not rendered. */
+export type AssistantRevealResult = 'shown' | 'visible' | 'missing';
Review Comment:
This type is for the callback planned in [ZEPPELIN-6749](https://issues.apache.org/jira/browse/ZEPPELIN-6749). The Angular host reports whether a linked paragraph was scrolled into view, was already visible, or could not be found, and the React panel announces the case. It is not consumed in this PR yet.
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** The transport reverses the server's newest-first page so messages are oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load older messages; null at the start of the conversation. */
+ cursor: string | null;
+}
+export interface AssistantRunBody {
+ content: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ | { type: 'tool_call.done'; toolCallId: string };
Review Comment:
Thanks for the clarification. `AssistantRunEvent` is the UI-facing event, not the raw WebSocket event. The follow-up transport in #5563 reads `payload.error.status` and maps it to the `message` field; this is covered by its tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha closed pull request #5561: [ZEPPELIN-6741] Define shared Assistant UI contracts and message history behavior
URL: https://github.com/apache/zeppelin/pull/5561
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285406032
##########
zeppelin-web-angular/projects/zeppelin-sdk/src/assistant-ui.ts:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/** The notebook sidebar's width: its default and its resize range. The assistant panel shares it. */
+export const NOTEBOOK_SIDEBAR_WIDTH = { initial: 370, min: 280, max: 800 } as const;
+
+/** Result of a host reveal: scrolled to it, already visible, or not rendered. */
+export type AssistantRevealResult = 'shown' | 'visible' | 'missing';
Review Comment:
This type is for the `revealParagraph` callback planned in [ZEPPELIN-6749](https://issues.apache.org/jira/browse/ZEPPELIN-6749). The Angular host reports whether a linked paragraph was scrolled into view, was already visible, or could not be found, and the React panel announces the `missing` case. It is not consumed in this PR yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4284749847
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/messageHistory.spec.ts:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { describe, expect, it } from 'vitest';
+
+import type { AssistantMessage } from './assistantContract';
+import { joinPages } from './messageHistory';
+
+describe('joinPages', () => {
+ it('joins a reply split across pages under the later id and retains both action logs', () => {
+ const first = { id: 'a1', content: 'Looking.', toolCalls: [{ id: 'c1', name: 'read' }] };
+ const second = { id: 'a2', content: 'Done.', toolCalls: [{ id: 'c2', name: 'summarize' }] };
+ const earlier: AssistantMessage[] = [
+ { id: 'u1', role: 'user', content: 'Read this notebook' },
+ { id: 'a1', role: 'assistant', content: first.content, toolCalls: first.toolCalls, segments: [first] }
+ ];
+ const later: AssistantMessage[] = [
+ { id: 'a2', role: 'assistant', content: second.content, toolCalls: second.toolCalls, segments: [second] }
+ ];
+ expect(joinPages(earlier, later)).toEqual([
+ earlier[0],
+ {
+ id: 'a2',
+ role: 'assistant',
+ content: 'Looking.\n\nDone.',
+ toolCalls: [
+ { id: 'c1', name: 'read' },
+ { id: 'c2', name: 'summarize' }
+ ],
+ segments: [first, second]
+ }
+ ]);
+ expect(earlier[1].content).toBe('Looking.');
+ expect(later[0].content).toBe('Done.');
+ });
+
+ it('retains a tool-only reply across an empty intermediate page and deduplicates repeated calls', () => {
+ const call = { id: 'c1', name: 'read' };
+ const first = { id: 'a1', content: '', toolCalls: [call] };
+ const second = { id: 'a2', content: 'Done.', toolCalls: [call] };
+ const earlier: AssistantMessage[] = [
+ { id: 'a1', role: 'assistant', content: '', toolCalls: [call], segments: [first] }
+ ];
+ const later: AssistantMessage[] = [
+ { id: 'a2', role: 'assistant', content: 'Done.', toolCalls: [call], segments: [second] }
+ ];
+ expect(joinPages(joinPages(earlier, []), later)).toEqual([
+ { id: 'a2', role: 'assistant', content: 'Done.', toolCalls: [call], segments: [first, second] }
+ ]);
+ });
+
+ it('keeps interleaved text and tool rounds in server order across an overlapping page boundary', () => {
+ const rounds = [
+ { id: 'a1', content: 'First thought.', toolCalls: [{ id: 'c1', name: 'read' }] },
+ { id: 'a2', content: 'Second thought.', toolCalls: [{ id: 'c2', name: 'summarize' }] },
+ { id: 'a3', content: 'Final answer.' }
+ ];
+ const earlier: AssistantMessage[] = [
+ { id: 'a2', role: 'assistant', content: 'First thought.\n\nSecond thought.', segments: rounds.slice(0, 2) }
+ ];
+ const later: AssistantMessage[] = [
+ { id: 'a3', role: 'assistant', content: 'Second thought.\n\nFinal answer.', segments: rounds.slice(1) }
+ ];
+ expect(joinPages(earlier, later)).toEqual([
+ {
+ id: 'a3',
+ role: 'assistant',
+ content: 'First thought.\n\nSecond thought.\n\nFinal answer.',
+ toolCalls: [rounds[0].toolCalls![0], rounds[1].toolCalls![0]],
+ segments: rounds
+ }
+ ]);
+ });
+
+ it('keeps separate turns apart and handles empty history', () => {
+ const earlier: AssistantMessage[] = [
+ { id: 'a1', role: 'assistant', content: 'First answer', segments: [{ id: 'a1', content: 'First answer' }] }
+ ];
+ const later: AssistantMessage[] = [
+ { id: 'u2', role: 'user', content: 'Another question' },
+ { id: 'a2', role: 'assistant', content: 'Another answer', segments: [{ id: 'a2', content: 'Another answer' }] }
+ ];
+ expect(joinPages(earlier, later)).toEqual([...earlier, ...later]);
+ expect(joinPages([], later)).toEqual(later);
+ expect(joinPages(earlier, [])).toEqual(earlier);
+ expect(joinPages([], [])).toEqual([]);
+ });
+
+ it('does not merge an answer with the next question across pages', () => {
Review Comment:
The name reads the other way around from what the test checks.
```suggestion
it('keeps a question apart from its answer on the next page', () => {
```
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Current UI projections; render segments to show text and actions in their original order. */
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
Review Comment:
`content` and `toolCalls` can be derived from `segments`, so storing them as well lets them drift apart, as the input of the overlapping page test already does. How about dropping both fields, keeping only the rounds, and deriving them where needed?
```ts
export interface AssistantRound { id: string; content: string; toolCalls: AssistantToolCallRef[] }
export interface AssistantAnswerMessage { id: string; role: 'assistant'; rounds: AssistantRound[] }
export const answerText = (answer: AssistantAnswerMessage) =>
answer.rounds.map(round => round.content).filter(Boolean).join('\n\n');
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#issuecomment-6194696884
Thanks for the review. I updated the history model to preserve Assistant rounds as ordered segments, matching how the server stores them. I also removed the `name` field from `tool_call.done`, which the server does not send, and added tests for a question/answer split across page boundaries and overlapping pages. The focused unit tests, TypeScript check, and ESLint passed locally. The follow-up transport PR #5563 has been updated to use the same contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285284034
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** The transport reverses the server's newest-first page so messages are oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load older messages; null at the start of the conversation. */
+ cursor: string | null;
+}
+export interface AssistantRunBody {
+ content: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ | { type: 'tool_call.done'; toolCallId: string };
Review Comment:
Just for reference, the server wraps the event data in payload
Also, run.failed provides an error status rather than a message field.
For example:
```json
{
"conversationId": "...",
"type": "run.failed",
"payload": {
"runId": "...",
"error": {
"status": 500
}
}
}
```
We could add a `code` for more specific handling later if needed.
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** The transport reverses the server's newest-first page so messages are oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load older messages; null at the start of the conversation. */
+ cursor: string | null;
+}
+export interface AssistantRunBody {
+ content: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ | { type: 'tool_call.done'; toolCallId: string };
Review Comment:
Just for reference, the server wraps the event data in payload
Also, `run.failed` provides an error status rather than a message field.
For example:
```json
{
"conversationId": "...",
"type": "run.failed",
"payload": {
"runId": "...",
"error": {
"status": 500
}
}
}
```
We could add a `code` for more specific handling later if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4284989160
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Current UI projections; render segments to show text and actions in their original order. */
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load the page before this one; null when this page starts the conversation. */
+ earlierCursor: string | null;
+}
+export interface AssistantRunBody {
+ prompt: string;
+}
+export type AssistantRunEvent =
+ | { type: 'run.started'; runId?: string }
+ | { type: 'run.completed'; runId?: string }
+ | { type: 'run.failed'; runId?: string; message: string }
+ | { type: 'message.delta'; messageId: string; delta: string }
+ | { type: 'message.done'; messageId: string; content: string }
+ | { type: 'tool_call.started'; toolCallId: string; name: string }
+ | { type: 'tool_call.done'; toolCallId: string };
+
+export type AssistantRunState = 'idle' | 'running' | 'disconnected';
+
+export interface AssistantTransport {
+ /** Conversations of the note the transport was created for. */
+ listConversations(): Promise<AssistantConversation[]>;
+ createConversation(body: { title?: string }): Promise<AssistantConversation>;
+ deleteConversation(conversationId: string): Promise<void>;
+ /** The latest page of history, or the page before `before` (an `earlierCursor`). */
+ getMessages(conversationId: string, before?: string): Promise<AssistantMessagePage>;
+ /** Observe local run state, including a lost connection until history is reloaded. */
+ subscribeRunState?(conversationId: string, listener: (state: AssistantRunState) => void): () => void;
+ openRun(conversationId: string, body: AssistantRunBody, signal: AbortSignal): AsyncIterable<AssistantRunEvent>;
Review Comment:
It looks like cancellation isn't supported on the server side.
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
Review Comment:
Do these fields need to be optional?
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Current UI projections; render segments to show text and actions in their original order. */
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load the page before this one; null when this page starts the conversation. */
+ earlierCursor: string | null;
+}
+export interface AssistantRunBody {
+ prompt: string;
Review Comment:
The server currently expects this field to be named `content`.
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId?: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage?: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt?: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Current UI projections; render segments to show text and actions in their original order. */
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
+export interface AssistantMessagePage {
+ messages: AssistantMessage[];
+ /** Pass as `before` to load the page before this one; null when this page starts the conversation. */
+ earlierCursor: string | null;
Review Comment:
Should this be `cursor`?
See [MessagesResponse.java](https://github.com/apache/zeppelin/blob/f1bb1b9a6e3b0ea4858f6a33f3089b2d455f7916/zeppelin-server/src/main/java/org/apache/zeppelin/rest/message/MessagesResponse.java#L26).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#issuecomment-6074064064
This PR defines the types shared by the Assistant UI and its host, and adds the conversion from stored message history to visible UI messages. The full diff is 205 changed lines; excluding the 63 lines in `*.spec.*` tests, the review diff is **+142/-0**. There is no lockfile change.
This depends on #5558. Review and approval can happen now, but I will wait for #5558 to merge, then rebase this branch onto the latest `master` and retarget the PR to `master` before merging it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285509219
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/messageHistory.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type { AssistantAnswerMessage, AssistantMessage, AssistantToolCallRef } from './assistantContract';
+
+export const answerText = (answer: AssistantAnswerMessage): string =>
+ answer.segments
+ .map(segment => segment.content)
+ .filter(Boolean)
+ .join('\n\n');
+
+export const answerToolCalls = (answer: AssistantAnswerMessage): AssistantToolCallRef[] =>
+ Array.from(
+ new Map(answer.segments.flatMap(segment => segment.toolCalls ?? []).map(call => [call.id, call])).values()
+ );
+
+export const mergeAnswers = (
+ earlier: AssistantAnswerMessage,
+ later: AssistantAnswerMessage
+): AssistantAnswerMessage => {
+ const segments = Array.from(
+ new Map([...earlier.segments, ...later.segments].map(segment => [segment.id, segment])).values()
+ );
+ return {
+ ...later,
+ segments
+ };
Review Comment:
Could we make the return value more explicit? Spreading later ignores some of earlier’s fields, such as toolCalls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#issuecomment-6194221648
Rebased onto master after #5558 merged to remove duplicated prerequisite changes. This PR now contains only its five intended files. The relevant local checks passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285509219
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/messageHistory.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type { AssistantAnswerMessage, AssistantMessage, AssistantToolCallRef } from './assistantContract';
+
+export const answerText = (answer: AssistantAnswerMessage): string =>
+ answer.segments
+ .map(segment => segment.content)
+ .filter(Boolean)
+ .join('\n\n');
+
+export const answerToolCalls = (answer: AssistantAnswerMessage): AssistantToolCallRef[] =>
+ Array.from(
+ new Map(answer.segments.flatMap(segment => segment.toolCalls ?? []).map(call => [call.id, call])).values()
+ );
+
+export const mergeAnswers = (
+ earlier: AssistantAnswerMessage,
+ later: AssistantAnswerMessage
+): AssistantAnswerMessage => {
+ const segments = Array.from(
+ new Map([...earlier.segments, ...later.segments].map(segment => [segment.id, segment])).values()
+ );
+ return {
+ ...later,
+ segments
+ };
Review Comment:
Could we make the return value more explicit? Spreading later could ignore some of earlier’s fields, such as toolCalls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6741] Define shared AI Assistant UI contracts and message history behavior [zeppelin]
Posted by "seung-00 (via GitHub)" <gi...@apache.org>.
seung-00 commented on code in PR #5561:
URL: https://github.com/apache/zeppelin/pull/5561#discussion_r4285139735
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/model/assistantContract.ts:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface AssistantConversation {
+ id: string;
+ title?: string;
+ /** Account that created the conversation, shown as "Started by …". */
+ ownerId: string;
+ /** Whether the requesting user can send messages to the conversation, as the server decides. */
+ canSendMessage: boolean;
+ /** Last activity (ISO time), shown in the conversation list. */
+ updatedAt: string;
+}
+export type AssistantMessage = AssistantUserMessage | AssistantAnswerMessage;
+export interface AssistantUserMessage {
+ id: string;
+ role: 'user';
+ content: string;
+}
+export interface AssistantAnswerMessage {
+ /** The last assistant round's id, used as the bubble key. */
+ id: string;
+ role: 'assistant';
+ /** Ordered assistant rounds as stored by the server, including tool-only rounds. */
+ segments: AssistantMessageSegment[];
+}
+export interface AssistantToolCallRef {
+ id: string;
+ name: string;
+}
+export interface AssistantMessageSegment {
+ id: string;
+ content: string;
+ toolCalls?: AssistantToolCallRef[];
+}
+/** One page of a conversation's history, oldest first. */
Review Comment:
The server’s `listMessages()` returns messages newest first, but this comment says “oldest first.” Should these match?
For example, assuming `m5` is the newest message, the server returns `[m5, m4, m3]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org