Skip to content

Keep a hidden tool group off zero height in the chat list - #1379

Merged
alexeyzimarev merged 2 commits into
mainfrom
fix-question-group-layout-loop
Oct 9, 2026
Merged

alexeyzimarev merged 2 commits into
mainfrom
fix-question-group-layout-loop

Conversation

@alexeyzimarev

@alexeyzimarev alexeyzimarev commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Closes #1378 — AI-3578

What & why

A session whose transcript ends on an unanswered AskUserQuestion, with no question card yet, crashed the app with Infinite layout loop detected. The trailing tool group hides behind the question card, leaving a zero-height last row. The virtualizing panel re-estimates that row at the average row size on every pass, so the extent grew each pass and follow-tail chased it. A 1px floor on the group row keeps it measured.

Where to look

The floor is on the tool-group row only: no other top-level chat row hides itself.

Verification

  • Replayed the crashing session's real transcript headless at 800×600: the extent grew +93px per pass until the exception; with the fix it holds at 48530 from the first pass.
  • New A_question_waiting_for_its_card_does_not_loop_layout fails with the loop exception without the fix and passes with it.
  • dotnet run --project test/Capacitor.App.Tests.Unit/…: 3108 succeeded, 0 failed.
  • dotnet build src/Capacitor.App --no-incremental: 0 warnings, 0 errors.
  • Not run: the fixed app against the live session.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved chat transcript layout stability when a pending question appears after rows of varying heights. The transcript no longer repeatedly recalculates its layout during rendering, and the reader remains at the bottom. Existing tool-group visibility and expansion behavior is unchanged, including while the question’s card is pending. This helps keep the chat view steady as it renders.

The virtualizing panel re-estimates a zero-height row on every pass, so a trailing
group hidden behind a question card grew the extent each pass until layout looped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T07:32:00.200778Z befde01 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: a8dd6787-e762-47d8-9a79-f39fc0ffa6a9

📥 Commits

Reviewing files that changed from the base of the PR and between befde01 and fb36fe0.


📒 Files selected for processing (1)
  • test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:


Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.



📝 Walkthrough

Walkthrough

The ToolGroupItem row now has a minimum height of one pixel. A smoke test checks that a pending question with a suppressed trailing tool group does not change scroll extent across render ticks.

Changes

Chat layout stability

Layer / File(s) Summary
Tool group row height and regression test
src/Capacitor.App/Views/ChatTabView.axaml, test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs
The ToolGroupItem row has a one-pixel minimum height. A smoke test checks that scroll extent stays constant and the view remains at the bottom across five render ticks when the trailing tool group is suppressed.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: nortonandreev


Merge Risk: ⚪ Minimal · up to fb36f

This change keeps a hidden tool group from collapsing to zero height in the chat list, which prevents the layout loop crash. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: preventing a hidden tool group from reaching zero height in the chat list. It is concise and specific.
Linked Issues check Passed The pull request satisfies the coding requirements in [#1378]. ToolGroupItem now has MinHeight="1", which prevents a hidden trailing tool-group row from having zero height. The new `A_question_wai…
Out of Scope Changes check Passed The changes stay within [#1378]. The XAML change affects only tool-group row height. The added headless test verifies the reported layout-loop scenario. No unrelated changes appear in the reviewed cha…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Prevent a pending question from looping chat layout

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Give suppressed tool-group rows a 1px height floor to prevent repeated virtual-list re-estimation.
• Add a headless regression test for stable scroll extent and follow-tail behavior before the
 question card arrives.
Diagram

graph TD
    Replay["Transcript replay"] --> Pending{"Question pending?"} --> Group["Suppressed tool group"] --> Floor["1px row floor"] --> Panel["Virtualizing panel"] --> Extent["Stable scroll extent"]
Loading
High-Level Assessment

Keep the targeted height floor. Removing the pending group from the item source would require more invasive list-state changes, while applying a floor to every chat row would broaden the behavior unnecessarily.

Files changed (2) +36 / -1

Bug fix (1) +4 / -1
ChatTabView.axamlPrevent suppressed tool-group rows from measuring at zero height +4/-1

Prevent suppressed tool-group rows from measuring at zero height

• Sets a 1px minimum height on the tool-group ChatCardRow, which stays in the virtualized list when its content is hidden for a pending question. This prevents repeated extent growth and the resulting layout loop without changing other row types.

src/Capacitor.App/Views/ChatTabView.axaml

Tests (1) +32 / -0
ChatTabViewSmokeTests.csCover layout stability while a question card is pending +32/-0

Cover layout stability while a question card is pending

• Adds a headless transcript test with uneven row heights and a trailing suppressed question group. It advances render passes and asserts that scroll extent stays fixed and the reader remains at the bottom.

test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs

@qodo-code-review

qodo-code-review Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Failed layout tests leave windows open ✓ Resolved
Description
A_question_waiting_for_its_card_does_not_loop_layout calls host.CloseAsync() only after its
render ticks and assertions complete. If the layout failure it tests throws during a tick, the test
skips window closure and view-model teardown, leaving those resources in the process-wide Avalonia
session used by later tests.
Code

test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs[1496]

+            await host.CloseAsync();
Relevance

●●● Strong

Recent precedents accept try/finally cleanup for Avalonia smoke-test hosts and shared-session
resources.

PR-#858
PR-#929

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test runs render ticks and assertions before its sole cleanup call. Host.CloseAsync is
responsible for closing the window and tearing down both view models; chat teardown disposes its
feed, timer, and subscriptions. The Avalonia test session is shared across the assembly rather than
recreated per test.

test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs[1475-1496]
test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs[223-229]
src/Capacitor.App/ViewModels/ChatTabViewModel.cs[1148-1164]
test/Capacitor.App.Tests.Unit/AvaloniaSession.cs[24-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new layout regression test skips host cleanup if rendering or an assertion throws, leaving a window and view-model resources alive in the shared test session.
## Fix Focus Areas
- test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs[1475-1496]
## Recommended Fix
Wrap the test body after host construction in `try/finally`, and await `host.CloseAsync()` in the `finally` block.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: Runtime layout behavior and regression test affect a core UI path.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs Outdated
coderabbitai[bot]
coderabbitai Bot previously approved these changes Oct 9, 2026
A window left looping in the shared Avalonia session fails every test after it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit 3820194 into main Oct 9, 2026
10 checks passed
@alexeyzimarev
alexeyzimarev deleted the fix-question-group-layout-loop branch October 9, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop app crashes opening a session whose question card has not arrived

1 participant