Prevent TUI bootstrap workers leaking into CI - #111
Merged
Conversation
Reviewer's GuideRefactors TUI shell/terminal and edit cards to render full inline content without separate detail views, adds a global test fixture to disable background bootstrap workers, adjusts unified diff view behavior, and updates CI Python test workflows and coverage thresholds to avoid long-running TUI background activity and lockfile issues in CI. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ScanLineCard._refresh_inline_bodyyou re-query the DOM for.scan-inline-contenton every refresh tick; consider caching a reference to this widget duringcompose()to avoid repeated selector lookups in the hot 250 ms refresh path. - The indentation change for the
verify=_HTTPX_VERIFYargument inget_shared_http_client/get_shared_async_http_clientlooks accidental and now misaligns that parameter relative to the others; it may be worth reverting for readability and consistency with the surrounding code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ScanLineCard._refresh_inline_body` you re-query the DOM for `.scan-inline-content` on every refresh tick; consider caching a reference to this widget during `compose()` to avoid repeated selector lookups in the hot 250 ms refresh path.
- The indentation change for the `verify=_HTTPX_VERIFY` argument in `get_shared_http_client` / `get_shared_async_http_client` looks accidental and now misaligns that parameter relative to the others; it may be worth reverting for readability and consistency with the surrounding code.
## Individual Comments
### Comment 1
<location path="backend/cli/tui/widgets/scan_line/card.py" line_range="251-253" />
<code_context>
+ return []
+ return [Static(renderable, classes='scan-inline-content')]
+
+ def _refresh_inline_body(self) -> None:
+ """Refresh a mounted text preview after live tool data changes."""
+ try:
+ body = self.query_one('.scan-inline-content', Static)
+ except Exception:
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid broad exception suppression when refreshing the inline body
Catching `Exception` here will hide real errors from `_inline_renderable` or the widget tree. Please narrow this to the specific exception raised when `.scan-inline-content` is missing (e.g., `MissingWidgetError`/`NoMatches` or the textual equivalent) and allow other exceptions to propagate.
</issue_to_address>
### Comment 2
<location path="backend/tests/unit/cli/tui/test_activity_card_policy.py" line_range="89-94" />
<code_context>
@pytest.mark.asyncio
-async def test_record_panel_stays_collapsed_until_user_expands(mock_config) -> None:
+async def test_record_card_shows_inline_preview_and_keeps_full_detail(
+ mock_config,
+) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for MCPCard inline preview when there are no arguments and no result yet ("Waiting…" path).
This covers the successful tool run path well. There’s still an untested case where `_arguments` is empty/None and `_result` is empty/None (e.g. tool still running or failed early). In that scenario `_inline_renderable` should hit the `Waiting…` branch and render a minimal inline body.
Please add a small test to cover that behavior, for example:
```python
card = MCPCard(name="tool", meta_lines=[], arguments=None)
inline = _plain_renderable(card._inline_renderable())
assert "Waiting…" in inline
```
Suggested implementation:
```python
assert detail._heading == 'docs_tool'
def test_mcp_card_inline_preview_waiting_state() -> None:
card = MCPCard(name="tool", meta_lines=[], arguments=None)
inline = _plain_renderable(card._inline_renderable())
assert "Waiting…" in inline
```
If this test file does not already import `MCPCard` or `_plain_renderable`, you will need to:
1. Import `MCPCard` from wherever it is defined (e.g. `from grinta.cli.tui.components import MCPCard` or the appropriate path in your codebase).
2. Import `_plain_renderable` from its module (e.g. `from grinta.cli.tui.utils import _plain_renderable` or similar).
Place these imports alongside the existing imports at the top of `test_activity_card_policy.py`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+251
to
+253
| def _refresh_inline_body(self) -> None: | ||
| """Refresh a mounted text preview after live tool data changes.""" | ||
| try: |
There was a problem hiding this comment.
issue (bug_risk): Avoid broad exception suppression when refreshing the inline body
Catching Exception here will hide real errors from _inline_renderable or the widget tree. Please narrow this to the specific exception raised when .scan-inline-content is missing (e.g., MissingWidgetError/NoMatches or the textual equivalent) and allow other exceptions to propagate.
Comment on lines
+89
to
94
| async def test_record_card_shows_inline_preview_and_keeps_full_detail( | ||
| mock_config, | ||
| ) -> None: | ||
| console = RichConsole() | ||
| loop = __import__('asyncio').get_running_loop() | ||
| app = GrintaTUIApp(config=mock_config, console=console, loop=loop) |
There was a problem hiding this comment.
suggestion (testing): Add a test for MCPCard inline preview when there are no arguments and no result yet ("Waiting…" path).
This covers the successful tool run path well. There’s still an untested case where _arguments is empty/None and _result is empty/None (e.g. tool still running or failed early). In that scenario _inline_renderable should hit the Waiting… branch and render a minimal inline body.
Please add a small test to cover that behavior, for example:
card = MCPCard(name="tool", meta_lines=[], arguments=None)
inline = _plain_renderable(card._inline_renderable())
assert "Waiting…" in inlineSuggested implementation:
assert detail._heading == 'docs_tool'
def test_mcp_card_inline_preview_waiting_state() -> None:
card = MCPCard(name="tool", meta_lines=[], arguments=None)
inline = _plain_renderable(card._inline_renderable())
assert "Waiting…" in inlineIf this test file does not already import MCPCard or _plain_renderable, you will need to:
- Import
MCPCardfrom wherever it is defined (e.g.from grinta.cli.tui.components import MCPCardor the appropriate path in your codebase). - Import
_plain_renderablefrom its module (e.g.from grinta.cli.tui.utils import _plain_renderableor similar).
Place these imports alongside the existing imports at the top oftest_activity_card_policy.py.
josephsenior
enabled auto-merge
July 28, 2026 21:21
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_command_output_previewand_terminal_inline_widgetsnow render full command output inline with no line cap; for very large outputs this could make individual cards unwieldy and slow to render, so consider reintroducing a bounded preview or a hard maximum with an explicit overflow indicator. - In
ShellCard._refresh_inline_bodyandTerminalCard._refresh_inline_body, the broadexcept Exceptionaroundquery_onewill hide unrelated errors; it would be safer to catch the specific Textual exception (e.g.NoMatches) so genuine failures still surface. - The CI workflow now runs
uv lock(without--check) on the runner, which may modifyuv.lockand leave the workspace dirty; if the intent is only to validate resolution, consider using a read‑only/locked mode or an alternative command that doesn’t rewrite the lockfile.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_command_output_preview` and `_terminal_inline_widgets` now render full command output inline with no line cap; for very large outputs this could make individual cards unwieldy and slow to render, so consider reintroducing a bounded preview or a hard maximum with an explicit overflow indicator.
- In `ShellCard._refresh_inline_body` and `TerminalCard._refresh_inline_body`, the broad `except Exception` around `query_one` will hide unrelated errors; it would be safer to catch the specific Textual exception (e.g. `NoMatches`) so genuine failures still surface.
- The CI workflow now runs `uv lock` (without `--check`) on the runner, which may modify `uv.lock` and leave the workspace dirty; if the intent is only to validate resolution, consider using a read‑only/locked mode or an alternative command that doesn’t rewrite the lockfile.
## Individual Comments
### Comment 1
<location path="backend/cli/tui/widgets/scan_line/cards.py" line_range="568-577" />
<code_context>
+ def _inline_widgets(self) -> list[Static]:
+ return _terminal_inline_widgets(self.command, self.output, title='Shell')
+
+ def _refresh_inline_body(self) -> None:
+ try:
+ command = self.query_one('.terminal-command', Static)
+ output = self.query_one('.terminal-output', Static)
+ except Exception:
+ return
+ command.update(_command_text(self.command))
+ from backend.cli.tui.transcript_typography import TX_BODY_DIM
+
+ output.update(Text((self.output or '').rstrip('\n'), style=TX_BODY_DIM))
+
def build_detail_screen(self) -> DetailScreen:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching a broad Exception in `_refresh_inline_body` may hide real errors.
This `try/except Exception` will hide any unexpected errors in that block (e.g., type issues during `update`). Prefer catching the specific Textual exceptions for missing or multiple widgets so you still handle the lookup race without masking real bugs.
```suggestion
def _refresh_inline_body(self) -> None:
# Handle transient lookup issues without masking real errors
from textual.app import NoMatches, TooManyMatches
try:
command = self.query_one('.terminal-command', Static)
output = self.query_one('.terminal-output', Static)
except (NoMatches, TooManyMatches):
# Inline widgets not present or ambiguous; nothing to refresh yet
return
command.update(_command_text(self.command))
from backend.cli.tui.transcript_typography import TX_BODY_DIM
output.update(Text((self.output or '').rstrip('\n'), style=TX_BODY_DIM))
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+568
to
+577
| def _refresh_inline_body(self) -> None: | ||
| try: | ||
| command = self.query_one('.terminal-command', Static) | ||
| output = self.query_one('.terminal-output', Static) | ||
| except Exception: | ||
| return | ||
| command.update(_command_text(self.command)) | ||
| from backend.cli.tui.transcript_typography import TX_BODY_DIM | ||
|
|
||
| output.update(Text((self.output or '').rstrip('\n'), style=TX_BODY_DIM)) |
There was a problem hiding this comment.
suggestion (bug_risk): Catching a broad Exception in _refresh_inline_body may hide real errors.
This try/except Exception will hide any unexpected errors in that block (e.g., type issues during update). Prefer catching the specific Textual exceptions for missing or multiple widgets so you still handle the lookup race without masking real bugs.
Suggested change
| def _refresh_inline_body(self) -> None: | |
| try: | |
| command = self.query_one('.terminal-command', Static) | |
| output = self.query_one('.terminal-output', Static) | |
| except Exception: | |
| return | |
| command.update(_command_text(self.command)) | |
| from backend.cli.tui.transcript_typography import TX_BODY_DIM | |
| output.update(Text((self.output or '').rstrip('\n'), style=TX_BODY_DIM)) | |
| def _refresh_inline_body(self) -> None: | |
| # Handle transient lookup issues without masking real errors | |
| from textual.app import NoMatches, TooManyMatches | |
| try: | |
| command = self.query_one('.terminal-command', Static) | |
| output = self.query_one('.terminal-output', Static) | |
| except (NoMatches, TooManyMatches): | |
| # Inline widgets not present or ambiguous; nothing to refresh yet | |
| return | |
| command.update(_command_text(self.command)) | |
| from backend.cli.tui.transcript_typography import TX_BODY_DIM | |
| output.update(Text((self.output or '').rstrip('\n'), style=TX_BODY_DIM)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary\n- disable real TUI background bootstrap workers in headless tests\n- prevent cross-file worker leakage that causes Linux unit suites to hit the 30-minute timeout\n\n## Validation\n- TUI test groups pass locally\n- ruff and diff checks pass for changed test files
Summary by Sourcery
Render full inline terminal and diff content in TUI cards while simplifying detail views and stabilizing CI test execution.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: