Repository navigation
[ZEPPELIN-6745] Add AI Assistant message and answer state UI - #5567
Conversation
There was a problem hiding this comment.
The message and answer states are easy to inspect in Storybook, and the components can be developed independently of the server. Thanks for putting these together.
Could you remove the TemporaryThemeCheck story now that the actual Assistant stories are available?
The PR description still says this was opened against master, but the current base is the temporary Storybook branch. Please update that explanation so the diff and dependencies are clear to reviewers.
Since different contributors are building the UI pieces, we may want one integration pass to align the design and state presentation afterward. I can open a follow-up issue if that becomes necessary.
|
Thanks for the review!
|
|
@kimyenac #5561 is on If it helps, c55b0cf on my branch (voidmatcha@c55b0cf7b) makes that change. It cherry-picks cleanly onto the rebased branch, and the typecheck and tests pass. Also, #5570 has merged. Please use its Markdown answer component as the default |
| } | ||
|
|
||
| /** The tools behind an answer. Open while one is still running, collapsed once they have all finished. */ | ||
| export const AssistantToolLog = ({ tools }: AssistantToolLogProps) => { |
There was a problem hiding this comment.
It looks like there’s no UI for failed tool calls. Because tool calls could fail sometimes, it would be helpful to make those failures easy to notice. How about adding a UI for that case?
cc @voidmatcha
There was a problem hiding this comment.
Thanks for pointing this out. I agree that failed tool calls should be distinguishable in the UI. The server already sends failure information, but we need to carry it through the frontend contract and event mapping to the UI. To keep this PR within its current scope, I suggest tracking this as a follow-up under ZEPPELIN-6754.
@kimyenac, would you be comfortable taking this on as a follow-up after this PR?
There was a problem hiding this comment.
Sure, I will take ZEPPELIN-6754 as a follow-up after this PR, including making failed tool calls easy to notice. Thanks for raising it, @seung-00.
1cc3ea2 to
06dcb82
Compare
|
Done, thanks:
|
### What is this PR for?
Add the AI Assistant question composer: a controlled component that takes a value, change and submit callbacks, and read-only and sending states. Nothing is sent anywhere. The parent receives the question and decides what to do with it. The component lives in `projects/zeppelin-react/src/entities/assistant/ui`.
Commits:
1. **Question composer**
* `AssistantComposer` is a `<form>` with an antd `Input.TextArea` (auto-sizing, 2–8 rows) and a **Send** `Button`.
* The value is controlled. `onSubmit` receives the question with surrounding whitespace trimmed. Clearing the input afterwards is left to the parent, so it can keep the text if sending fails.
* Nothing is submitted, by the button, Enter or the form, when:
* the input is empty or whitespace only (the server rejects a blank prompt with 400),
* `readOnly` is set (for example a conversation owned by another user). The textarea is disabled and a note explains why (`readOnlyReason`, with a default).
* `sending` is set (the server accepts one run per conversation). The textarea becomes read-only, so the question stays visible, and the button shows a spinner.
* Enter sends and Shift+Enter adds a new line. An Enter that confirms an IME composition (Korean, Japanese, …) does not send. Otherwise the last syllable of a Korean question would send it. Commit 3 extends this to Safari.
2. **Stories**
* `Assistant/Composer`: Empty, Draft, WhitespaceOnly, Sending, ReadOnly.
* `Assistant/Composer/Playground` keeps its own value: type, send with the button or Enter, see `onSubmit` in the Actions panel and the input cleared.
3. **Review: do not send on the Enter that confirms an IME composition in Safari** (by <at>voidmatcha)
* Safari fires `compositionend` before that Enter, so it arrives with `isComposing: false` and `keyCode: 229`. The check now covers both.
4. **Review: pin the controlled value of the composer in tests** (by <at>voidmatcha)
* The field keeps showing the parent's value when `onChange` is ignored, and shows and sends a value the parent sets later. Both tests fail if `value` becomes `defaultValue`.
5. **Review: raise the composer placeholder contrast**
* antd's placeholder colour was about 1.8:1 (light) and 2.3:1 (dark). It now uses the theme's `textTertiary`, which the theme lifts to 4.5:1. Measured in Storybook: 6.10:1 / 6.39:1 on the panel, 5.84:1 on the read-only field.
Styling uses `<at>vanilla-extract/css` through the Assistant theme contract only. The field draws a single focus ring for the textarea inside it. The HTML/CSS screens in `voidmatcha/assistant-storybook-share` were used as a design reference. The code is written from scratch.
**Out of scope** (per the issue): per-conversation draft persistence, actual sending, Stop and retry. These belong to panel assembly (ZEPPELIN-6748) or follow-up issues.
### What type of PR is it?
Feature
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-6746
### How should this be tested?
In `zeppelin-web-angular/projects/zeppelin-react`:
* `npm ci`, `npm test` (247 passed, 8 new), `npm run typecheck`, `npm run lint` and `npm run build:storybook` all pass.
* `typecheck` needs `npm ci` in `zeppelin-web-angular` first, because the SDK resolves `rxjs` from there.
* The new files also pass the frontend's `prettier --check`.
* `npm run storybook`: every story above checked in both themes, including typing Korean and pressing Enter mid-composition in Playground (Chrome). Safari was not tried; commit 3 is covered by a `keyCode: 229` unit test.
* The spec was mutation-checked: dropping the IME check, the Safari `keyCode` check, the trim, the sending guard, the read-only guard or the read-only textarea while sending, or using `defaultValue`, each fails the intended test.
* `npm run test:storybook -- --workers=1`: all 52 tests pass. It runs with one worker because the default 3 workers made the existing `python3 -m http.server` web server drop requests on the earlier base, independent of these stories (see #5567).
### Screenshots (if appropriate)
See the `Assistant/Composer` stories.
### Questions:
* Does the license files need to update? No. There are no new dependencies.
* Is there breaking changes for older versions? No. Only new files are added.
* Does this needs documentation? No.
Closes #5569 from kimyenac/ZEPPELIN-6746.
Signed-off-by: YONGJAE LEE <dev.yongjaelee@gmail.com>
|
For reference, here is a local reproduction of the current caret placement, using this PR's rendered markup and styles. To reproduce in Storybook:
The caret appears on a separate line below the code content.
This is a non-blocking visual suggestion for a follow-up. |
|
Merged into master (f801b2e). |
3 participants
What is this PR for?
Add the AI Assistant message and answer state UI: user and Assistant messages are shown distinctly, and an answer displays its loading, streaming, tool-use, completed and error states. Everything is driven by fixture props. There is no server, transport or panel state. The components live in
projects/zeppelin-react/src/entities/assistant/ui.This PR adds new files under
entities/assistant/uiand removes the temporaryAssistant/Setupstory. It builds on the shared contract from ZEPPELIN-6741 (#5561) and the Markdown answer from ZEPPELIN-6744 (#5570), both onmaster.Commits:
AssistantMessageItemrenders a question or an answer:aria-busyand shows a caret where the text ends.AssistantAnswerViewis the display union (loading/streaming/done/error). It reusesAssistantAnswerMessageandAssistantToolCallReffrom the ZEPPELIN-6741 contract.AssistantAnswerrenders one view at a time. Each state fully replaces the last.role="status") reads "Thinking…", "Writing answer…" or "Running <tool>…".AssistantToolLogis open while a tool runs and collapses once the answer completes. A tool without a name is labelled "Tool".AssistantAnswerErroris an antdAlert(role="alert"):failed(run.failed) andstream(AssistantStreamError) are errors with Retry.connection(AssistantConnectionError) is a warning with Reload conversation, because the answer may have finished on the server.onRetry, no button is shown.Assistant/Message: User, CompletedReply, StreamingReply, LongContent, Conversation.Assistant/Status: Loading, Streaming, ToolRunning, Completed, CompletedWithTools, Failed, StreamError, ConnectionLost, ErrorWithoutRetry.Assistant/Status/StateSwitcherswitches the fixture from the Controls panel, so you can check that the states stay consistent in both themes.Assistant/Setup › TemporaryThemeCheck(AssistantScope.stories.tsx) was a placeholder until the Assistant UI stories landed. Nothing else referenced it.AssistantAnswerMessagewith orderedsegments. Its text joins the rounds in order, and its tool log lists the tools from every round.renderContentnow defaults toMarkdownAnswerfrom ZEPPELIN-6744. A parent can still pass its own, for example one that resolves paragraph references.MarkdownAnswerwraps the text in blocks, so the streaming caret is now drawn inside the last block (or the last item of a closing list). It stays on the line where the text ends instead of dropping below it.Styling uses
@vanilla-extract/cssthrough the Assistant theme contract only, so light and dark need no per-component rules. The HTML/CSS screens invoidmatcha/assistant-storybook-sharewere used as a design reference. The code is written from scratch.Out of scope (per the issue / ZEPPELIN-6748): run event → view derivation, panel mounting in
AssistantWorkspace, real tool calls, approval, persistence and WebSocket handling. Showing failed tool calls (raised in review) is tracked in ZEPPELIN-6754.What type of PR is it?
Feature
Todos
renderContentonce it mergesWhat is the Jira issue?
How should this be tested?
In
zeppelin-web-angular/projects/zeppelin-react:npm ci,npm test(254 passed),npm run typecheck,npm run lintandnpm run build:storybookall pass.typecheckneedsnpm ciinzeppelin-web-angularfirst, because the SDK resolvesrxjsfrom there.prettier --check.npm run storybook: both themes checked manually for every story above, including StateSwitcher.renderContent, keeping the tool log open, or going back to a plain-text default each fails exactly the intended test.npm run test:storybook:--workers=1, all 68 tests pass.python3 -m http.serverweb server failing (status -1) and a blank page.4599ad2d9), its own story alone reproduced this: with--repeat-each=8, 1–2 of 16 time out.Screenshots (if appropriate)
See the
Assistant/MessageandAssistant/Statusstories.Questions: