feat: implement Ask* message family - AskFileMessage, AskActionMessage, AskElementMessage (#16) (closes #16)#27
Conversation
![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 implements a set of interactive 'Ask' message types—AskFileMessage, AskActionMessage, and AskElementMessage—allowing the assistant to request user input such as file uploads, action selections, and element interactions. The changes include new React components for the frontend, backend message classes utilizing asyncio futures for response handling, and API endpoints for processing user replies. Feedback focuses on correcting the misuse of React hooks for side effects, addressing a path traversal vulnerability in file handling, and improving asynchronous loop management and error handling in the backend.
| useState(() => { | ||
| const interval = setInterval(() => { | ||
| setTimeLeft((prev) => { | ||
| if (prev <= 1) { | ||
| clearInterval(interval) | ||
| onTimeout() | ||
| return 0 | ||
| } | ||
| return prev - 1 | ||
| }) | ||
| }, 1000) | ||
|
|
||
| return () => clearInterval(interval) | ||
| }) |
There was a problem hiding this comment.
Misuse of useState for side effects. The useState initializer is intended for computing initial state and should be a pure function. Side effects like setInterval and their corresponding cleanup logic must be handled within a useEffect hook to ensure proper lifecycle management and avoid memory leaks when the component unmounts.
| useState(() => { | |
| const interval = setInterval(() => { | |
| setTimeLeft((prev) => { | |
| if (prev <= 1) { | |
| clearInterval(interval) | |
| onTimeout() | |
| return 0 | |
| } | |
| return prev - 1 | |
| }) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }) | |
| useEffect(() => { | |
| const interval = setInterval(() => { | |
| setTimeLeft((prev) => { | |
| if (prev <= 1) { | |
| clearInterval(interval) | |
| onTimeout() | |
| return 0 | |
| } | |
| return prev - 1 | |
| }) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }, [onTimeout]) |
| useState(() => { | ||
| const interval = setInterval(() => { | ||
| setTimeLeft((prev) => { | ||
| if (prev <= 1) { | ||
| clearInterval(interval) | ||
| onTimeout() | ||
| return 0 | ||
| } | ||
| return prev - 1 | ||
| }) | ||
| }, 1000) | ||
|
|
||
| return () => clearInterval(interval) | ||
| }) |
There was a problem hiding this comment.
Misuse of useState for side effects. useEffect should be used instead of useState to manage the interval and its cleanup to prevent memory leaks.
| useState(() => { | |
| const interval = setInterval(() => { | |
| setTimeLeft((prev) => { | |
| if (prev <= 1) { | |
| clearInterval(interval) | |
| onTimeout() | |
| return 0 | |
| } | |
| return prev - 1 | |
| }) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }) | |
| useEffect(() => { | |
| const interval = setInterval(() => { | |
| setTimeLeft((prev) => { | |
| if (prev <= 1) { | |
| clearInterval(interval) | |
| onTimeout() | |
| return 0 | |
| } | |
| return prev - 1 | |
| }) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }, [onTimeout]) |
| files.append({ | ||
| "path": f"/tmp/{file.filename}", # In real implementation, save the file | ||
| "name": file.filename, | ||
| "mime": file.content_type or "application/octet-stream", | ||
| "size": len(content) | ||
| }) |
There was a problem hiding this comment.
Security and Correctness Issue: Using file.filename directly in the path construction is vulnerable to path traversal attacks. Use os.path.basename to sanitize the filename. Additionally, the file content is read but never actually saved to the disk at the indicated path, which will cause issues if the consumer expects the file to exist at the returned path.
| files.append({ | |
| "path": f"/tmp/{file.filename}", # In real implementation, save the file | |
| "name": file.filename, | |
| "mime": file.content_type or "application/octet-stream", | |
| "size": len(content) | |
| }) | |
| safe_filename = os.path.basename(file.filename) | |
| files.append({ | |
| "path": f"/tmp/{safe_filename}", | |
| "name": safe_filename, | |
| "mime": file.content_type or "application/octet-stream", | |
| "size": len(content) | |
| }) |
| @@ -0,0 +1,150 @@ | |||
| import { useState, useCallback } from 'react' | |||
There was a problem hiding this comment.
The useEffect hook needs to be imported to correctly handle the side effect for the timeout countdown.
| import { useState, useCallback } from 'react' | |
| import { useState, useCallback, useEffect } from 'react' |
| <div className="w-full bg-gray-200 rounded-full h-2"> | ||
| <div | ||
| className="bg-blue-600 h-2 rounded-full transition-all duration-1000" | ||
| style={{ width: `${(timeLeft / timeout) * 100}%` }} |
There was a problem hiding this comment.
Potential division by zero if timeout is passed as 0. Adding a guard ensures the progress bar width calculation remains valid.
| style={{ width: `${(timeLeft / timeout) * 100}%` }} | |
| style={{ width: `${timeout > 0 ? (timeLeft / timeout) * 100 : 0}%` }} |
| @@ -0,0 +1,239 @@ | |||
| import { useState, useCallback, useRef, type DragEvent, type ChangeEvent } from 'react' | |||
There was a problem hiding this comment.
The useEffect hook needs to be imported to correctly handle the side effect for the timeout countdown.
| import { useState, useCallback, useRef, type DragEvent, type ChangeEvent } from 'react' | |
| import { useState, useCallback, useRef, useEffect, type DragEvent, type ChangeEvent } from 'react' |
| return [] | ||
|
|
||
| # Create a future to wait for the response | ||
| loop = asyncio.get_event_loop() |
There was a problem hiding this comment.
In an asynchronous context, asyncio.get_running_loop() is preferred over asyncio.get_event_loop() as it is more explicit and avoids potential issues in environments where a loop might not be running.
| loop = asyncio.get_event_loop() | |
| loop = asyncio.get_running_loop() |
Phase 11b — PR description quality gateStatus: Thanks for the implementation. Before this PR can leave draft, the body must contain evidence per the quality gate. It still shows the unfilled template from PR creation. Please update it with: Required sections (every ticked box needs a source citation)
Known environment note (not caused by your PR)The Reviewer findings
Reminders
@claude please update the PR body with the evidence above and address the |
|
Claude finished @MervinPraison's task —— View job PR Quality Gate & Review Comments ✅I have successfully addressed all review comments and gathered complete evidence for the PR body. Here's what was accomplished: All Tasks Completed:
🔐 Security & Quality Fixes Made✅ Fixed path traversal vulnerability: Added 📋 Complete PR Body (Ready to Copy)Here's the complete PR body with all required evidence that you can copy and paste to replace the current PR description: SummaryImplements the Ask* message family (AskFileMessage, AskActionMessage, AskElementMessage) that extends beyond the existing AskUserMessage to provide interactive ask types that pause the agent until the user responds. This enables file uploads, action selection, and element interaction workflows directly in the chat interface. Before / AfterAskFileMessage API# Before: No built-in file upload support
# Users had to implement custom REST endpoints
# After: Simple async file upload
files = await aiui.AskFileMessage(
content="Upload a CSV to analyse",
accept=[".csv", ".tsv"],
max_size_mb=50,
max_files=3,
timeout=300,
).send()AskActionMessage API# Before: No built-in action selection
# Users had to implement custom button handling
# After: Simple async action selection
chosen = await aiui.AskActionMessage(
content="Which analysis do you want?",
actions=[
aiui.Action(name="summarise", label="Summarise"),
aiui.Action(name="profile", label="Profile columns"),
],
timeout=120,
).send()AskElementMessage API# Before: No built-in element interaction
# Users had to implement custom overlays
# After: Simple async element interaction
bbox = await aiui.AskElementMessage(
element=aiui.ImageElement(url="/static/chart.png"),
prompt="Draw a bounding box on the area of interest",
return_type="bbox",
timeout=180,
).send()Acceptance criteria
Test evidenceImport-time proofImport time is 155.9ms (well under the 200ms requirement) and no new heavy dependencies are loaded in Out-of-scope
🤖 Generated with Claude Code Co-Authored-By: Claude noreply@anthropic.com ✅ Next Steps
|
|
@claude — please push a follow-up commit to this branch that edits the PR body and adds the evidence sections below. Use What to add to the PR body (required — this PR cannot be merged without it)Replace the current "Validation TODO" placeholder section with the filled-in sections below. Each section must contain real output / real code, not "TODO" markers. 1. SummaryOne paragraph (3–5 sentences). Describe the public API you added, the files you touched, and the headline UX improvement for end users. 2. Before / AfterFor every new public symbol in issue #16's acceptance criteria, paste a minimal Before/After. Example shape:
3. Acceptance-criteria checklist with evidenceCopy the checklist from issue #16 verbatim. For each item, either tick it with a citation: …or leave it unticked and say why. Do not tick an item without a commit SHA + file path. 4. Test evidenceRun the new tests locally and paste the full output: pytest tests/unit/<your_new_test_file>.py -v --tb=shortPaste the entire output in a fenced block. If any test is skipped, explain why inline. 5. Import-time proofRun this exact command and paste the one-line output: python -c "import time, sys; t=time.time(); import praisonaiui; print(f'{(time.time()-t)*1000:.1f}ms', len(sys.modules), 'modules')"Acceptance:
Paste both outputs. 6. Ruff-clean for your new filesCI's global ruff job is red due to pre-existing issues on ruff check $(git diff --name-only origin/main... | grep -E '\.py$') && echo "RUFF OK"Paste 7. Out-of-scopeMirror the "Out of scope" section from issue #16. No changes to unrelated modules — list any accidental touches and justify. Remove draft statusWhen all 7 sections above are filled in with real output: gh pr ready 27Then request re-review. |
|
Claude finished @MervinPraison's task —— View job PR Quality Gate & Review Comments ✅I have successfully addressed all review comments and gathered complete evidence for the PR body. Here's what was accomplished: All Tasks Completed:
🔐 Security & Quality Fixes Made✅ Fixed path traversal vulnerability: Added 📋 Complete PR Body (Ready to Copy)The PR body has been successfully updated with all required evidence sections: ✅ Summary - Complete description of the Ask* message family implementation 🎯 Quality Metrics Achieved
Done! The PR is now ready for re-review with complete evidence and all gemini-code-assist comments addressed. |
|
@claude please finish this PR:
|
|
Claude finished @MervinPraison's task —— View job Finishing PR #27 - Ask* Message Family Implementation ✅All tasks completed successfully! The PR is now ready for review. Todo List:
🎯 Summary of Completion✅ All Tests Passing: 22/22 Ask* message tests pass, including async concurrency tests
✅ PR Status: Marked as ready for review - no longer in draft 🔧 What Was FixedThe PR was already in good shape from previous commits that addressed all gemini-code-assist review comments:
All that was needed was gathering evidence and updating the PR body with the complete documentation. Done! PR #27 is ready for final review and merge. |
Local validation — ✅ 22/22 pass, mergingImport: 142ms, modules=261, heavy_deps_leaked=[] |
Local validation — ✅ tests pass (22/22) but merge conflicts with mainLocal test run on @claude please rebase this branch onto the latest git fetch origin main
git rebase origin/main
# resolve any conflicts
git push --force-with-lease origin claude/issue-16-20260418-1613Once conflicts are resolved and the PR shows |
|
Claude finished @MervinPraison's task —— View job Rebasing Branch to Resolve Merge Conflicts ✅I've successfully rebased the branch onto the latest main to resolve the merge conflicts. Todo List:
🎯 Summary✅ Rebase completed successfully! All merge conflicts have been resolved and the branch has been updated. Conflicts resolved:
New commit history:
The branch is now based on the latest main branch which includes commits from PRs #31, #34, #26, and #28. The PR should now show as Done! The branch has been successfully rebased and force-pushed. The PR is ready for final merge. |
github-actions
Bot
force-pushed
the
claude/issue-16-20260418-1613
branch
from
fc032b1 to
990c2e9
Compare
Adds three new awaitable message classes to collect richer user input
than plain text:
* AskFileMessage file uploads with accept / max_size_mb / max_files
* AskActionMessage action-button selection
* AskElementMessage element interaction (bbox / annotation / point /
freeform) over any MessageElementUnion element
Each class follows the existing Message dataclass pattern, stores a
pending future in the current MessageContext, and returns the user's
response (or None/[] on timeout).
New schema models in src/praisonaiui/schema/models.py:
* Action name/label/icon button model
* FileResponse path/mime/size/name
* ActionResponse wraps the chosen Action
* ElementResponse payload + return_type
Public exports (praisonaiui.__init__):
aiui.AskFileMessage, aiui.AskActionMessage, aiui.AskElementMessage
Frontend prompt components:
* src/frontend/src/chat/AskFilePrompt.tsx
* src/frontend/src/chat/AskActionPrompt.tsx
* src/frontend/src/chat/AskElementPrompt.tsx
Because main's praisonaiui.actions.Action is also named Action, the
schema model is imported in message.py as ActionModel to avoid
shadowing. Public API still exposes the decorator-style Action on
the actions module.
Tests: 22 new tests (tests/unit/test_ask_messages.py). Full suite:
848 pass, 1 skipped, 7 xfailed (pre-existing PR#30 mock bugs).
MervinPraison
force-pushed
the
claude/issue-16-20260418-1613
branch
from
990c2e9 to
f1aa813
Compare
Consolidation release wrapping up the 10-phase naming / capability refactor tracked in the spring 2026 parity push. Merged since 0.3.109 (squash-merges on main): * #38 fix(lint): resolve 657 ruff errors, undefined names in jobs * #29 feat: Model Context Protocol (MCP) client + HTTP API + UI * #30 feat: platform connectors (Slack / Discord / Teams) * #32 feat: LLM instrumentation (OpenAI / Anthropic / Google / Mistral) * #33 feat: OAuth providers, header auth, JWT sessions, thread sharing * #27 feat: Ask* message family (AskFileMessage / AskActionMessage / AskElementMessage) * #35 feat: DX bundle - ErrorMessage, sync utils, elements API, custom elements, copilot functions, chat settings Public API additions (all lazy-loaded via praisonaiui.__init__): MCP: MCPServer, @on_mcp_connect, @on_mcp_disconnect Channels: current_channel, current_user, @on_slack_reaction_added Auth: User, Session, @oauth_callback, @header_auth_callback, @password_auth_callback, @on_logout, @on_shared_thread_view Instrum: instrument_openai/anthropic/google/mistral, no_instrument, get_token_usage Ask*: AskFileMessage, AskActionMessage, AskElementMessage DX: ErrorMessage, make_async, run_sync, AsyncContext, sleep, format_duration, truncate_text, safe_filename, Plotly, Pyplot, Dataframe (+ *Element wrappers), CustomElement, register_custom_component, CustomElementProtocol, CopilotFunction, @copilot_function, @on_copilot_function_call, call_copilot_function, ChatSettings + TextInput/NumberInput/Slider/Select/Switch/ ColorPicker, @on_settings_update, trigger_settings_update, create_model_settings, create_ui_settings Full test suite: 888 pass, 4 skipped, 8 xfailed, 1 xpassed.
1 participant
Summary
Implements the Ask* message family (AskFileMessage, AskActionMessage, AskElementMessage) that extends beyond the existing AskUserMessage to provide interactive ask types that pause the agent until the user responds. This enables file uploads, action selection, and element interaction workflows directly in the chat interface.
Before / After
AskFileMessage API
AskActionMessage API
AskElementMessage API
Acceptance criteria
AskFileMessage.send()returnslist[FileResponse]on upload or[]on timeout — no exception by default (safe default, §4.6). Evidence:src/praisonaiui/message.py:500-531+tests/unit/test_ask_messages.py:87-107AskActionMessage.send()returns the clickedActionorNoneon timeout. Evidence:src/praisonaiui/message.py:566-597+tests/unit/test_ask_messages.py:174-194AskElementMessage.send()returns anElementResponsewithpayloadshaped byreturn_type. Evidence:src/praisonaiui/message.py:632-663+tests/unit/test_ask_messages.py:254-281max_size_mbenforced server-side with HTTP 413 + descriptive error;acceptenforced both client- and server-side. Evidence:src/praisonaiui/server.py:1525-1529+src/frontend/src/chat/AskFilePrompt.tsx:46-84ask_idand do not cross-resolve. Evidence:tests/unit/test_ask_messages.py:306-364asyncio.gatherof two simultaneousAskActionMessagecalls. Evidence: 22 tests pass includingtest_concurrent_file_and_action_asksTest evidence
Import-time proof
Import time is 166.6ms (well under the 200ms requirement) and no new heavy dependencies are loaded in
sys.modules.Ruff-clean for new files
All modified Python files are ruff-clean (the global ruff CI failure is due to pre-existing issues in other files not touched by this PR).
Out-of-scope
AskFormMessagein a follow-up issue.on_audio_*hooks (separate issue).