feat(channels): add Discord streaming responses and typing indicator#1389
feat(channels): add Discord streaming responses and typing indicator#1389Chiytako wants to merge 2 commits intoagentscope-ai:mainfrom
Conversation
Chiytako
requested a deployment
to
maintainer-approved
GitHub Actions
Waiting
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for the Discord channel by introducing real-time streaming capabilities for LLM responses and a visible typing indicator. Instead of waiting for a complete response, users will now see a live-updating draft message and know the bot is actively processing their request, making interactions feel more dynamic and responsive. The feature is opt-in via a new configuration setting, ensuring backward compatibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent feature: streaming responses and a typing indicator for the Discord channel. The implementation is well-structured, introducing DiscordTypingController and DiscordDraftStream to manage the new asynchronous logic, and includes a good set of unit tests. However, I've identified a critical issue where the new streaming_mode is not configurable via environment variables due to an omission in the from_env method. Additionally, I've found a couple of bugs in the new tests and have some suggestions for minor improvements to enhance consistency and readability in the implementation.
| class TestFromEnvStreamingMode: | ||
| def test_default_streaming_mode_from_env_is_off(self, monkeypatch): | ||
| """DISCORD_STREAMING_MODE not set -> off.""" | ||
| monkeypatch.delenv("DISCORD_STREAMING_MODE", raising=False) | ||
| # We don't need a real process handler for this test | ||
| from copaw.app.channels.discord_.channel import DiscordChannel | ||
|
|
||
| with patch.object(DiscordChannel, "__init__", return_value=None): | ||
| channel = DiscordChannel.__new__(DiscordChannel) | ||
| # Simulate from_env calling cls(**kwargs) with streaming_mode | ||
| import os | ||
|
|
||
| raw = os.getenv("DISCORD_STREAMING_MODE", "off").strip().lower() | ||
| mode = "partial" if raw == "partial" else "off" | ||
| assert mode == "off" | ||
|
|
||
| def test_partial_streaming_mode_from_env(self, monkeypatch): | ||
| """DISCORD_STREAMING_MODE=partial -> partial.""" | ||
| monkeypatch.setenv("DISCORD_STREAMING_MODE", "partial") | ||
| import os | ||
|
|
||
| raw = os.getenv("DISCORD_STREAMING_MODE", "off").strip().lower() | ||
| mode = "partial" if raw == "partial" else "off" | ||
| assert mode == "partial" | ||
|
|
||
| def test_unknown_streaming_mode_from_env_defaults_to_off(self, monkeypatch): | ||
| monkeypatch.setenv("DISCORD_STREAMING_MODE", "full") | ||
| import os | ||
|
|
||
| raw = os.getenv("DISCORD_STREAMING_MODE", "off").strip().lower() | ||
| mode = "partial" if raw == "partial" else "off" | ||
| assert mode == "off" |
There was a problem hiding this comment.
These tests for from_env are testing logic that is not actually implemented in DiscordChannel.from_env. The streaming_mode parameter is missing from the cls(...) call in from_env, which means this feature cannot be configured via environment variables. This is a critical omission.
Please add the logic to parse the DISCORD_STREAMING_MODE environment variable in DiscordChannel.from_env.
| draft = DiscordDraftStream(channel) | ||
| await draft.send_placeholder() | ||
| await draft.clear() | ||
| draft._draft_msg is None or None # after clear, ref is gone |
There was a problem hiding this comment.
This line is a statement, not an assertion. It doesn't verify that _draft_msg is set to None. This is likely a typo and should be an assert statement to make the test effective.
| draft._draft_msg is None or None # after clear, ref is gone | |
| assert draft._draft_msg is None # after clear, ref is gone |
| deadline = asyncio.get_event_loop().time() + _TYPING_TIMEOUT_S | ||
| try: | ||
| while not self._stopped: | ||
| try: | ||
| await channel.trigger_typing() | ||
| except Exception: | ||
| logger.debug( | ||
| "discord: trigger_typing failed", | ||
| exc_info=True, | ||
| ) | ||
| await asyncio.sleep(_TYPING_INTERVAL_S) | ||
| if asyncio.get_event_loop().time() >= deadline: | ||
| logger.debug("discord: typing loop timed out") | ||
| break |
There was a problem hiding this comment.
The _typing_loop uses asyncio.get_event_loop().time() to manage its timeout. It's recommended to use time.monotonic() for consistency with DiscordDraftStream and to align with modern Python asyncio practices, as get_event_loop() is being phased out.
Using time.monotonic() is more explicit for measuring time durations and is not affected by system time changes.
| deadline = asyncio.get_event_loop().time() + _TYPING_TIMEOUT_S | |
| try: | |
| while not self._stopped: | |
| try: | |
| await channel.trigger_typing() | |
| except Exception: | |
| logger.debug( | |
| "discord: trigger_typing failed", | |
| exc_info=True, | |
| ) | |
| await asyncio.sleep(_TYPING_INTERVAL_S) | |
| if asyncio.get_event_loop().time() >= deadline: | |
| logger.debug("discord: typing loop timed out") | |
| break | |
| deadline = time.monotonic() + _TYPING_TIMEOUT_S | |
| try: | |
| while not self._stopped: | |
| try: | |
| await channel.trigger_typing() | |
| except Exception: | |
| logger.debug( | |
| "discord: trigger_typing failed", | |
| exc_info=True, | |
| ) | |
| await asyncio.sleep(_TYPING_INTERVAL_S) | |
| if time.monotonic() >= deadline: | |
| logger.debug("discord: typing loop timed out") | |
| break |
| draft = DiscordDraftStream( | ||
| target, | ||
| max_len=getattr(self, "_DISCORD_MAX_LEN", 2000), | ||
| ) |
There was a problem hiding this comment.
The expression getattr(self, "_DISCORD_MAX_LEN", 2000) is used multiple times within this method. Since _DISCORD_MAX_LEN is a class attribute, it's safe to access it directly via self._DISCORD_MAX_LEN. To improve readability and avoid repetition, consider storing this value in a local variable at the beginning of the method (e.g., max_len = self._DISCORD_MAX_LEN) and reusing it throughout.
| max_len=getattr(self, "_DISCORD_MAX_LEN", 2000), | ||
| ) | ||
| last_response = None | ||
| from agentscope_runtime.engine.schemas.agent_schemas import RunStatus |
There was a problem hiding this comment.
This local import of RunStatus should be moved to the top of the file with the other imports. This improves code readability and follows standard Python conventions. Local imports are generally used to avoid circular dependencies, which doesn't seem to be the case here.
| async def test_stopped_flag_prevents_restart(self): | ||
| from copaw.app.channels.discord_.channel import DiscordTypingController | ||
|
|
||
| channel = _make_channel() | ||
| ctrl = DiscordTypingController() | ||
| ctrl.start(channel) | ||
| ctrl.stop() | ||
| # After stop, _stopped should be True | ||
| assert ctrl._stopped is True |
There was a problem hiding this comment.
This test's name is test_stopped_flag_prevents_restart, but it only checks that the _stopped flag is set to True. It doesn't verify that the controller cannot be restarted. To make the test more robust and align with its name, it should attempt to call start() again after stop() and assert that no new task is created.
| async def test_stopped_flag_prevents_restart(self): | |
| from copaw.app.channels.discord_.channel import DiscordTypingController | |
| channel = _make_channel() | |
| ctrl = DiscordTypingController() | |
| ctrl.start(channel) | |
| ctrl.stop() | |
| # After stop, _stopped should be True | |
| assert ctrl._stopped is True | |
| async def test_stopped_flag_prevents_restart(self): | |
| from copaw.app.channels.discord_.channel import DiscordTypingController | |
| channel = _make_channel() | |
| ctrl = DiscordTypingController() | |
| ctrl.start(channel) | |
| await asyncio.sleep(0.01) # allow task to start | |
| assert ctrl._task is not None | |
| ctrl.stop() | |
| assert ctrl._stopped is True | |
| assert ctrl._task is None | |
| # Attempt to restart and verify it doesn't create a new task | |
| ctrl.start(channel) | |
| assert ctrl._task is None |
Chiytako
requested a deployment
to
maintainer-approved
GitHub Actions
Waiting
… dedicated unit tests.
Chiytako
force-pushed
the
feat/discord-streaming
branch
from
4ec6406 to
f228c51
Compare
Chiytako
requested a deployment
to
maintainer-approved
GitHub Actions
Waiting
1 participant
Summary
Add edit-based streaming and typing indicator support to the Discord channel. While the LLM generates, a live-updating draft message is shown and edited in place; a typing indicator is sent every 10 s to keep it visible. Both are opt-in via a new streaming_mode config field.
Related Issue
Closes #1296
Security Considerations
None — no changes to auth flow, permission model, or credential handling. The existing bot token is reused for REST calls; no new secrets are introduced.
Type of Change
Changes
src/copaw/app/channels/discord_/channel.py (modified)
/typingevery 10 s with a max-duration cap and clean stop() cleanup.streaming_mode = "off".src/copaw/config/config.py (modified)
streaming_mode: Literal["off", "partial"] = "off"to DiscordConfig"off"preserves full backward compatibility.tests/unit/channels/test_discord_streaming.py (new file)
How to Test
streaming_mode: "partial"in your Discord channel configstreaming_mode: "off"and confirm legacy behavior is unchangedChecklist
pre-commit run --all-filespassedstreaming_mode: "off") preserves existing behavior