[codex] Fix AG-UI App-mode sidebar collapse regression#752
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @blove's task in 1m 33s —— View job PR Review
SummaryThe fix is correct and well-scoped. The root cause (toolbar z-index intercepting the panel close button) is accurately described, and changing The new regression test is thorough — it covers open, close, and reopen state transitions with layout assertions at each step. Three minor notes left as inline comments:
No correctness bugs, no security issues, no type-safety problems. Good to merge after considering the inline suggestions. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| await expect(sidebar).toHaveAttribute('data-open', 'false'); | ||
| await expect(panel).toHaveAttribute('data-open', 'false'); | ||
| await expect(page.getByRole('button', { name: 'Open chat' })).toBeVisible(); | ||
| await expect.poll(async () => parseFloat((await layoutState()).marginRight)).toBe(0); |
There was a problem hiding this comment.
Minor: after expect.poll() resolves on this line, layoutState() is called again immediately on line 112. There's a tiny window where layout could still be transitioning. In practice the poll acts as a synchronisation barrier, so this is unlikely to flake — just worth knowing if this test is occasionally unstable in CI.
|
|
||
| const closedLayout = await layoutState(); | ||
| expect(closedLayout.rootClaim).toBeNull(); | ||
| expect(closedLayout.occupyRight).toBe('0px'); |
There was a problem hiding this comment.
Potential brittleness: asserts the CSS variable is exactly '0px'. If the library ever sets --tplane-chat-occupy-right to '0' (no unit suffix) on close, this fails incorrectly. Using parseFloat would be more robust and consistent with how marginRight is checked elsewhere.
| expect(closedLayout.occupyRight).toBe('0px'); | |
| expect(parseFloat(closedLayout.occupyRight)).toBe(0); |
| await expect.poll(async () => parseFloat((await layoutState()).marginRight)).toBeGreaterThan(300); | ||
| const openLayout = await layoutState(); | ||
| expect(openLayout.rootClaim).toBe('open'); | ||
| expect(parseFloat(openLayout.marginRight)).toBeGreaterThan(300); | ||
| expect(openLayout.contentWidth).toBeGreaterThan(500); | ||
| expect(openLayout.mapWidth).toBeCloseTo(openLayout.contentWidth, 0); | ||
| expect(openLayout.mapRight).toBeCloseTo(openLayout.contentRight, 0); |
There was a problem hiding this comment.
The > 300 (margin-right) and > 500 (content width) thresholds are implicit assumptions about the sidebar panel width at 1280 × 800. A short comment — e.g. // sidebar panel is ~350px wide — makes the intent clear and makes future threshold adjustments obvious if the default panel width ever changes.
| await expect(sidebar).toHaveAttribute('data-open', 'false'); | ||
| await expect(panel).toHaveAttribute('data-open', 'false'); | ||
| await expect(page.getByRole('button', { name: 'Open chat' })).toBeVisible(); | ||
| await expect.poll(async () => parseFloat((await layoutState()).marginRight)).toBe(0); |
There was a problem hiding this comment.
Minor: parseFloat((await layoutState()).marginRight).toBe(0) is consistent with the rest of the test, but once the sidebar is closed the computed marginRight will be "0px" → parseFloat → 0, so this works. The same approach as line 98 / 123 is fine.
One thing worth noting: after expect.poll() resolves on line 110, the test calls layoutState() again on line 112. There's a tiny window where the layout could still be transitioning. In practice the poll settling acts as a synchronisation barrier, so this is unlikely to flake — just worth knowing if this test is occasionally unstable in CI.
|
|
||
| const closedLayout = await layoutState(); | ||
| expect(closedLayout.rootClaim).toBeNull(); | ||
| expect(closedLayout.occupyRight).toBe('0px'); |
There was a problem hiding this comment.
Potential brittleness: this asserts the CSS variable is the string '0px' exactly. If the library ever sets --tplane-chat-occupy-right to '0' (no unit) when the sidebar closes, this assertion fails incorrectly. Using parseFloat(closedLayout.occupyRight) → toBe(0) would be more robust and consistent with how marginRight is checked elsewhere.
| expect(closedLayout.occupyRight).toBe('0px'); | |
| expect(parseFloat(closedLayout.occupyRight)).toBe(0); |
| await expect.poll(async () => parseFloat((await layoutState()).marginRight)).toBeGreaterThan(300); | ||
| const openLayout = await layoutState(); | ||
| expect(openLayout.rootClaim).toBe('open'); | ||
| expect(parseFloat(openLayout.marginRight)).toBeGreaterThan(300); | ||
| expect(openLayout.contentWidth).toBeGreaterThan(500); | ||
| expect(openLayout.mapWidth).toBeCloseTo(openLayout.contentWidth, 0); | ||
| expect(openLayout.mapRight).toBeCloseTo(openLayout.contentRight, 0); |
There was a problem hiding this comment.
The > 300 (margin-right) and > 500 (content width) thresholds are implicit assumptions about the sidebar panel width at a 1280 × 800 viewport. They'll silently regress if the default panel width changes. A short comment stating the expected relationship — e.g. "sidebar panel is ~350px wide" — makes the intent clear and makes future threshold adjustments obvious.
1 participant
Summary
Root cause
The demo toolbar has a higher z-index for portaled select menus. The App-mode chat sidebar panel was top-aligned under that toolbar stacking context, leaving the close button visible but intercepted by toolbar elements.
Validation
npx playwright test examples/ag-ui/angular/e2e/app-mode-promo.spec.ts -c examples/ag-ui/angular/e2e/playwright.config.tsnpx nx lint examples-ag-ui-angular(0 errors; existing warnings remain)