fix(mobile): hydration-safe code wrap default, anchor action mobile styles, code-renderer tests#30
Conversation
- Scope flex/ellipsis rules to toolbar buttons so labels do not clip; pair raw/rendered toggle on one row with horizontal scroll fallback. - Add overflow-x on code/diff/csv/json renderer toolbars at <=640px. - Default CodeMirror line wrapping on when viewport is <=640px (compact unchanged); desktop still starts with wrap off. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMobile toolbar CSS updated for wrapping, truncation, and horizontal scrolling; CodeRenderer now persists wrap preference and listens to a 640px media query; unit and Playwright tests added/extended to validate wrap and toolbar behavior; vitest config and an e2e helper timeout option were adjusted; docs updated. Changes
Sequence DiagramsequenceDiagram
participant User
participant CodeRenderer
participant MediaQuery
participant WrapPrefRef
rect rgba(100,150,200,0.5)
Note over CodeRenderer,MediaQuery: Initialization (mount, not compact)
CodeRenderer->>WrapPrefRef: read/init ("auto" | "on" | "off")
CodeRenderer->>MediaQuery: register matchMedia("(max-width: 640px)")
MediaQuery-->>CodeRenderer: emit initial matches state
CodeRenderer->>CodeRenderer: set wrapLines based on pref & matches
end
rect rgba(200,150,100,0.5)
Note over User,CodeRenderer: User toggles wrap
User->>CodeRenderer: click wrap button
CodeRenderer->>WrapPrefRef: set to "on" or "off"
CodeRenderer->>CodeRenderer: update wrapLines accordingly
end
rect rgba(150,200,100,0.5)
Note over MediaQuery,CodeRenderer: Viewport change
MediaQuery-->>CodeRenderer: change event
alt WrapPrefRef == "auto"
CodeRenderer->>CodeRenderer: update wrapLines to match media.matches
else WrapPrefRef == "on" or "off"
CodeRenderer->>CodeRenderer: ignore media change (preserve user choice)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying agent-render with
|
| Latest commit: |
1840872
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c9aed2bb.agent-render.pages.dev |
| Branch Preview URL: | https://cursor-mobile-toolbar-wrap-e.agent-render.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR improves the mobile viewing experience in the artifact viewer by addressing (1) toolbar button clipping on narrow screens and (2) the default line-wrapping behavior for code artifacts on small viewports.
Changes:
- Updates the mobile (≤640px) toolbar layout rules to avoid flex-induced label clipping and to add horizontal overflow handling.
- Adjusts
CodeRendererto defaultwrapLinesbased oncompactmode or a mobile-width media query.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/components/renderers/code-renderer.tsx |
Initializes code line-wrapping based on a 640px media query when not in compact mode. |
src/app/globals.css |
Refines mobile toolbar flex behavior to prevent button label clipping and enables horizontal scrolling for toolbars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const NARROW_CODE_BREAKPOINT = 640; | ||
| const MOBILE_CODE_MEDIA_QUERY = `(max-width: ${NARROW_CODE_BREAKPOINT}px)`; | ||
|
|
||
| function getDefaultWrapLines(compact: boolean) { | ||
| if (compact) { | ||
| return true; | ||
| } | ||
| if (typeof window === "undefined") { | ||
| return false; | ||
| } | ||
| return window.matchMedia(MOBILE_CODE_MEDIA_QUERY).matches; | ||
| } |
There was a problem hiding this comment.
Initializing wrapLines from window.matchMedia in the useState initializer can cause an SSR/CSR hydration mismatch: the server render always returns false (no window), while narrow clients may render with wrap enabled on first paint (changing the toolbar label and editor config). Consider keeping the initial state deterministic (e.g., compact) and then syncing to the media query in a useEffect after mount, or using useSyncExternalStore with a safe server snapshot. Also consider guarding window.matchMedia existence (tests/non-browser environments) before calling it.
| export function CodeRenderer({ artifact, compact = false, onReady }: CodeRendererProps) { | ||
| const hostRef = useRef<HTMLDivElement | null>(null); | ||
| const [wrapLines, setWrapLines] = useState(compact); | ||
| const [wrapLines, setWrapLines] = useState(() => getDefaultWrapLines(compact)); | ||
| const [languageExtension, setLanguageExtension] = useState<Awaited<ReturnType<typeof loadLanguageSupport>>>(null); |
There was a problem hiding this comment.
The new default wrap behavior is responsive to (max-width: 640px) but there are no component tests covering it. Consider adding a CodeRenderer test (similar to tests/components/diff-renderer.test.tsx) that stubs window.matchMedia and asserts the initial button label/state for narrow vs wide viewports, plus that compact still forces wrapping.
| /* Avoid flex-shrink on toolbar buttons so labels are not clipped; pair rows only inside raw/rendered toggle */ | ||
| .artifact-action:not(a) { | ||
| flex: 0 0 auto; | ||
| min-height: 2.55rem; | ||
| padding: 0.46rem 0.65rem; | ||
| font-size: 0.71rem; | ||
| white-space: nowrap; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| min-width: 0; | ||
| } |
There was a problem hiding this comment.
In the 640px breakpoint, switching from .artifact-action to .artifact-action:not(a) means anchor-based actions won’t receive the mobile sizing/overflow rules (min-height, padding, font-size, ellipsis). There are existing <a className="artifact-action"> usages (e.g. link-creator “Open in new tab”), so this may lead to inconsistent button sizing on mobile. Consider including a.artifact-action in the rule (or targeting the toolbar context more narrowly) so all action variants get consistent mobile treatment.
|
@copilot apply changes based on the comments in this thread |
…es, code-renderer tests Agent-Logs-Url: https://github.com/baanish/agent-render/sessions/28d4f0a2-dd44-4cee-9c7d-7af53206edf5 Co-authored-by: baanish <47579874+baanish@users.noreply.github.com>
Applied all three changes in commit
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (window.matchMedia(MOBILE_CODE_MEDIA_QUERY).matches) { | ||
| setWrapLines(true); | ||
| } | ||
| }, [compact]); |
There was a problem hiding this comment.
WARNING: Missing media query listener - The useEffect only checks matchMedia once at mount but doesn't listen for viewport changes. If the user resizes the window from wide to narrow after mount, wrapLines won't update to reflect the new viewport width.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Changes VerifiedThe current diff includes all the fixes from previous reviews:
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m2.5 · 360,670 tokens |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Track manual wrap toggles (on/off) vs auto so resize updates defaults until the user overrides. - Subscribe to (max-width: 640px) change events; compact mode resets to auto. - Extend tests with controllable matchMedia and resize scenarios. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/components/code-renderer.test.tsx (1)
127-224: Consider one Playwright regression for the mobile toolbar layout path.These unit tests validate behavior, but they won’t catch truncation/overflow regressions introduced by the mobile toolbar CSS updates.
Based on learnings: Verify visual changes with Playwright when they affect layout or renderer presentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/components/code-renderer.test.tsx` around lines 127 - 224, Add a Playwright visual/regression test that exercises the mobile toolbar layout of the CodeRenderer to catch truncation/overflow regressions: write an end-to-end test that mounts the CodeRenderer (using the same props/helpers: CodeRenderer, createArtifact, createControllableMatchMedia) with a narrow viewport, captures a screenshot of the toolbar (and compact vs non-compact states), and asserts against a baseline image; include a step to toggle wrap (simulate the user toggle used in the unit tests) and capture another screenshot so layout changes from the mobile toolbar and wrap state are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/components/code-renderer.test.tsx`:
- Around line 196-209: The test "keeps the user choice when the viewport changes
after a manual toggle" doesn't prove override protection because the viewport
change used doesn't conflict with the user's manual toggle; change the test to
use a conflicting viewport transition: initialize media with
createControllableMatchMedia(true) (so the natural state is the opposite),
render the CodeRenderer, perform the manual toggle via the enable/disable button
(e.g., click the button to flip the natural state), then call
media.setNarrowMatches(false) to transition to the opposite viewport and assert
the button still reflects the user's manual choice (use the same role/name
lookups for the button); this ensures the manual preference set in the test
overrides subsequent automatic viewport-driven changes.
---
Nitpick comments:
In `@tests/components/code-renderer.test.tsx`:
- Around line 127-224: Add a Playwright visual/regression test that exercises
the mobile toolbar layout of the CodeRenderer to catch truncation/overflow
regressions: write an end-to-end test that mounts the CodeRenderer (using the
same props/helpers: CodeRenderer, createArtifact, createControllableMatchMedia)
with a narrow viewport, captures a screenshot of the toolbar (and compact vs
non-compact states), and asserts against a baseline image; include a step to
toggle wrap (simulate the user toggle used in the unit tests) and capture
another screenshot so layout changes from the mobile toolbar and wrap state are
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d19a1c8-eea2-4319-b2fb-45b6cbcb7554
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/app/globals.csssrc/components/renderers/code-renderer.tsxtests/components/code-renderer.test.tsx
| it("keeps the user choice when the viewport changes after a manual toggle", async () => { | ||
| const media = createControllableMatchMedia(false); | ||
| render(<CodeRenderer artifact={createArtifact()} />); | ||
|
|
||
| const enableBtn = await screen.findByRole("button", { name: /enable wrap/i }); | ||
| await userEvent.click(enableBtn); | ||
|
|
||
| expect(screen.getByRole("button", { name: /disable wrap/i })).toBeVisible(); | ||
|
|
||
| media.setNarrowMatches(true); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByRole("button", { name: /disable wrap/i })).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Manual-override test does not prove override protection.
This scenario can pass even if manual preference is ignored, because switching to narrow after manually enabling wrap still results in “Disable wrap” in both cases. Use a conflicting transition to make the assertion meaningful.
Suggested test adjustment
describe("wrap preference after manual toggle", () => {
beforeEach(() => {
- createControllableMatchMedia(false);
+ createControllableMatchMedia(true);
});
it("keeps the user choice when the viewport changes after a manual toggle", async () => {
- const media = createControllableMatchMedia(false);
+ const media = createControllableMatchMedia(true);
render(<CodeRenderer artifact={createArtifact()} />);
- const enableBtn = await screen.findByRole("button", { name: /enable wrap/i });
- await userEvent.click(enableBtn);
+ const disableBtn = await screen.findByRole("button", { name: /disable wrap/i });
+ await userEvent.click(disableBtn);
- expect(screen.getByRole("button", { name: /disable wrap/i })).toBeVisible();
+ expect(screen.getByRole("button", { name: /enable wrap/i })).toBeVisible();
+ media.setNarrowMatches(false);
media.setNarrowMatches(true);
await waitFor(() => {
- expect(screen.getByRole("button", { name: /disable wrap/i })).toBeVisible();
+ expect(screen.getByRole("button", { name: /enable wrap/i })).toBeVisible();
});
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/components/code-renderer.test.tsx` around lines 196 - 209, The test
"keeps the user choice when the viewport changes after a manual toggle" doesn't
prove override protection because the viewport change used doesn't conflict with
the user's manual toggle; change the test to use a conflicting viewport
transition: initialize media with createControllableMatchMedia(true) (so the
natural state is the opposite), render the CodeRenderer, perform the manual
toggle via the enable/disable button (e.g., click the button to flip the natural
state), then call media.setNarrowMatches(false) to transition to the opposite
viewport and assert the button still reflects the user's manual choice (use the
same role/name lookups for the button); this ensures the manual preference set
in the test overrides subsequent automatic viewport-driven changes.
- Stack raw/rendered toolbar buttons full-width at <=360px (override pair flex). - Run wrap preference + matchMedia listener in useLayoutEffect so first paint matches narrow viewports without an extra CodeMirror teardown. - Set wrap preference ref outside setState on toolbar click. - Restore window.matchMedia in code-renderer tests afterEach for isolation. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
- Add Playwright mobile checks: code wrap defaults on, markdown toolbar button widths, 340px stacked raw/rendered toggle. - waitForViewerState accepts optional longer timeout for cold preview starts. - Vitest: esbuild automatic JSX so components need no default React import. - CodeRenderer: drop unused React import; document ref + compact contract. - Unit tests: single createControllableMatchMedia per test (no duplicate beforeEach); docs/testing.md notes mobile layout e2e coverage. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Resolve code-renderer import conflict: keep next-themes useTheme from main and useLayoutEffect from feature branch for mobile wrap defaults. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/viewer.spec.ts (1)
143-146: ScopeRaw/Renderedbutton locators to the toolbar for stronger test isolation.Using page-level role queries can become ambiguous if similarly named controls are added elsewhere. Prefer toolbar-scoped locators in both tests.
♻️ Suggested locator scoping update
- const rendered = page.getByRole("button", { name: /^Rendered$/ }); - const raw = page.getByRole("button", { name: /^Raw$/ }); + const rendered = toolbar.getByRole("button", { name: /^Rendered$/ }); + const raw = toolbar.getByRole("button", { name: /^Raw$/ });+ const toolbar = page.locator(".viewer-toolbar"); - const rendered = page.getByRole("button", { name: /^Rendered$/ }); - const raw = page.getByRole("button", { name: /^Raw$/ }); + const rendered = toolbar.getByRole("button", { name: /^Rendered$/ }); + const raw = toolbar.getByRole("button", { name: /^Raw$/ });Also applies to: 170-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/viewer.spec.ts` around lines 143 - 146, The test uses page-level role queries for the Rendered/Raw buttons which can be ambiguous; scope them to the toolbar locator by replacing the page-level queries for rendered and raw with toolbar-scoped queries (use toolbar.getByRole or toolbar.locator with role) so the variables rendered and raw are located within the const toolbar locator; make the same change for the other occurrences referenced (lines around where rendered/raw are defined again).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/viewer.spec.ts`:
- Around line 143-146: The test uses page-level role queries for the
Rendered/Raw buttons which can be ambiguous; scope them to the toolbar locator
by replacing the page-level queries for rendered and raw with toolbar-scoped
queries (use toolbar.getByRole or toolbar.locator with role) so the variables
rendered and raw are located within the const toolbar locator; make the same
change for the other occurrences referenced (lines around where rendered/raw are
defined again).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1c6fa00-849a-4f37-a1ff-34cc319ff7de
📒 Files selected for processing (7)
docs/testing.mdsrc/app/globals.csssrc/components/renderers/code-renderer.tsxtests/components/code-renderer.test.tsxtests/e2e/helpers.tstests/e2e/viewer.spec.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- docs/testing.md
- vitest.config.ts
- src/app/globals.css
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/components/code-renderer.test.tsx
- src/components/renderers/code-renderer.tsx
- Keep compact touch sizing on .artifact-action at <=640px globally; scope flex/ellipsis to .viewer-toolbar so renderer toolbars are not unintentionally affected. - Add pair-row rules for JSON tree/raw toggle on mobile; stack at <=360px like viewer raw/rendered. - Fix misleading CSS comment about specificity (source order, not higher specificity). - Drop unnecessary React import from code-renderer unit tests. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Three issues raised in review on the original mobile toolbar PR.
SSR/CSR hydration fix —
code-renderer.tsxwindow.matchMediawas called inside theuseStateinitializer, producingfalseon the server and potentiallytrueon the client, causing a hydration mismatch. State now initializes deterministically tocompact; auseEffectafter mount enables wrap when(max-width: 640px)matches:Anchor action mobile styles —
globals.css.artifact-action:not(a)was excluding anchor-based action elements from the mobile flex/padding/overflow rules. Changed to.artifact-actionand merged the separatebutton.artifact-action { max-width: 100% }into the single rule so all action variants (buttons and anchors) get consistent treatment at ≤640px.CodeRenderertests —tests/components/code-renderer.test.tsxAdded 4 tests: wide viewport → "Enable wrap" initial state, narrow viewport → "Disable wrap" after mount effect, toggle changes label, compact mode hides the toolbar.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Documentation