-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Agents posts on mobile #9317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Agents posts on mobile #9317
Conversation
Implements Phase 4 from the agents mobile plan: - WebSocket event handling for annotations control signal - CitationsList component displaying sources below agent responses - Collapsible sources section with source count - Each citation shows title, domain, and opens URL on tap - Citations persisted in post.props.annotations - Touch-optimized UI with 44pt minimum tap targets
- Remove optimistic WebSocket event approach that didn't work - Clear component streaming state on ENDED event to force switch to persisted data - Remove delays in streaming store cleanup (500ms and 100ms) - POST_EDITED event now properly updates UI for both accept and reject - Remove debug console.log statements - Fix ESLint issues (unused vars, nested callbacks, nested ternary)
Coverage Comparison ReportGenerated on December 08, 2025 at 18:32:14 UTC |
enahum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall fantastic job, most comments are mostly about makeup, great first contribution to this project :)
|
|
||
| import {CONTROL_SIGNALS} from '@agents/constants'; | ||
| import {StreamingEvents, type StreamingState, type PostUpdateWebsocketMessage} from '@agents/types'; | ||
| import {DeviceEventEmitter} from 'react-native'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we could have this store use observables instead of emitting events? Did you look into that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just followed the pattern with DeviceEventEmitter for the typing indicator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok not a blocker.. but it would be nice to explore the use of observables instead of emitting events.
My reason for this is that observables are better for state management while DeviceEventEmitter is mostly to communicate between native and JS event bridges, sure it works here, but I think its worth exploring. You can check how it was done with @products/calls/state/*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Looks better I think. Good call.
enahum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have anything blocking, but I would love to hear your response to some of my comments before approving.
|
|
||
| ## Important Development Notes | ||
| - Always run the checks before committing code | ||
| - Always test any changes using the mobile mcp before calling something complete or committing code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, what mcp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| import {CONTROL_SIGNALS} from '@agents/constants'; | ||
| import {StreamingEvents, type StreamingState, type PostUpdateWebsocketMessage} from '@agents/types'; | ||
| import {DeviceEventEmitter} from 'react-native'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok not a blocker.. but it would be nice to explore the use of observables instead of emitting events.
My reason for this is that observables are better for state management while DeviceEventEmitter is mostly to communicate between native and JS event bridges, sure it works here, but I think its worth exploring. You can check how it was done with @products/calls/state/*
|
@enahum Ready for review again. Addressed feedback and moved to using observables. |
enahum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better with the observables, thank you for that!!
There are a couple of non blocking comments.
Great job on this one!
| @@ -0,0 +1,294 @@ | |||
| // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice
nickmisasi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
Adds AI agent streaming support to the Mattermost Mobile app, enabling interaction with AI agents similar to the web/desktop experience. The implementation includes:
The architecture uses an ephemeral streaming store (
StreamingStore) that handles transient WebSocket events (custom_mattermost-ai_uppsdate) and properly cleans up state on generation end, allowing components to seamlessly transition to reading persisted database data through standardPOST_EDITEDevents.It does NOT include any UI beyond the posts themselves. That UI is in a separate PR: #9318
Also adds a CLAUDE.md file for ease of agentic coding with the mobile app using learnings from making this PR and the follow up.
Ticket Link
https://mattermost.atlassian.net/browse/MM-66762
Device Information
This PR was tested on: Pixel 7 android 16
Screenshots
Release Note