Fix Grid Layout for Active Session Summary on Desktop#3820
Merged
Conversation
In the detailed view of the /ux-auditor page, replaced raw layout classes with standard layout primitive props (display, truncate, minWidth) on the active URL Text component to guarantee robust ellipsis truncation. Added proper layout constraints (width, minWidth) to the Current Session active header container to prevent horizontal overflow and visual regression on smaller desktop and tablet viewports.
Contributor
Author
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
🐙 GitHub Models Visual Review
Summary: 🔴 9 high · 🟡 0 medium · 🟢 1 low Model: gpt-4.1 🔴
|
Contributor
|
🚀 Deployment Details (Last updated: Jul 22, 2026, 9:28 PM PST) 🚀 Pushed to gh-pages; publish in progress
|
…on Active Session card - Eliminated inline style override for gridColumn and rely on native span properties. - Configured active URL text wrapper with native truncate, block display, and minWidth boundaries to prevent horizontal overflow on responsive breakpoints.
…on Active Session card - Eliminated inline style override for gridColumn and rely on native span properties. - Configured active URL text wrapper with native truncate, block display, and minWidth boundaries to prevent horizontal overflow on responsive breakpoints. - Updated Playwright visual regression snapshots for the updated page layout.
Contributor
🐙 GitHub Models Code Review
Reviewing: PR #3820 Model: gpt-4o-mini Code Review Feedback[ARCHITECTURE] ReviewThe changes in this pull request aim to improve the grid layout and flexbox constraints for the active session summary on the Review Findings
SummaryOverall, the pull request addresses the main goals of improving the grid layout and handling long URLs effectively. However, there are still some areas for improvement, particularly regarding the use of inline styles and the potential conflicts between flex and grid layouts. Final VerdictGiven the identified issues, I recommend the following:
Based on these findings, I would classify this pull request as needing further adjustments before it can be approved. #### [PERFORMANCE] Review
Upon reviewing the provided pull request changes, I have identified a few points regarding performance, layout, and adherence to design guidelines. Here are my findings:
### Findings
1. **Inline Styles Usage**:
- **Line**: `style={{ transform: `scale(${scale})`, width: `${width}px`, height: `${height}px`, minWidth: `${width}px`, minHeight: `${height}px`, maxWidth: 'none', maxHeight: 'none' }}`
- **Issue**: The use of inline styles for layout (especially for width and height) violates the design system's principle of using layout primitives. This can lead to performance issues due to unnecessary re-renders and complexity in managing styles.
- **Status**: Open
- **Confidence**: High
- **Counterexample**: Instead of inline styles, consider using `<Box>` or `<Grid>` components with appropriate props to manage layout.
2. **Redundant `minWidth` Prop**:
- **Line**: `minWidth={0}`
- **Issue**: The `minWidth={0}` prop is applied multiple times across different components. This redundancy can be simplified to improve code clarity and maintainability.
- **Status**: Open
- **Confidence**: Medium
- **Counterexample**: If `minWidth={0}` is set on a parent component, it may not be necessary to set it on all child components.
3. **Flex Properties Conflict**:
- **Line**: `flex={1}`
- **Issue**: The use of `flex` properties in conjunction with grid layout can lead to conflicts and unexpected behavior in layout rendering. It is advisable to stick to one layout system (either grid or flex) to avoid performance issues and layout bugs.
- **Status**: Open
- **Confidence**: Medium
- **Counterexample**: If a component is using grid layout, it should not also use flex properties unless absolutely necessary.
4. **Truncation Functionality**:
- **Line**: `truncate={true}`
- **Issue**: While truncation has been implemented, it is important to ensure that it works correctly across all viewports. Testing should confirm that long URLs are truncated as expected without causing layout shifts or overflow.
- **Status**: Open
- **Confidence**: Medium
- **Counterexample**: If a long URL does not truncate properly, it could lead to overflow issues in smaller viewports.
### Summary
The changes made in this pull request address some layout issues, but there are still concerns regarding the use of inline styles, redundant properties, and potential conflicts between layout systems. These issues could lead to performance degradation and maintenance challenges in the future.
### Recommendations
- Refactor the inline styles to use layout primitives (`<Box>`, `<Grid>`, etc.) to manage dimensions and positioning.
- Review the necessity of `minWidth={0}` on multiple components and simplify where possible.
- Ensure that the layout system is consistent (either grid or flex) to avoid conflicts.
- Test the truncation functionality thoroughly across different viewports to confirm it behaves as expected.
Based on the findings, I recommend addressing the open issues before merging this pull request.
#### [SECURITY] Review
Upon reviewing the provided pull request, I have identified the following points related to security, particularly focusing on the OWASP Top 10, data validation, sanitization, and secure communication.
### Review Findings:
1. **Sanitization of URL Inputs**:
- The introduction of the `sanitizeUrlForDisplay` function in `src/utils/url.ts` is a positive enhancement. This function sanitizes URLs before they are rendered, which helps mitigate XSS vulnerabilities. The function checks if the URL is valid and returns a safe fallback if it is not.
- The use of this function in the `title` attribute of the input field and the display of the active report URL is appropriate and enhances security by ensuring that potentially harmful URLs are not rendered directly.
**Status**: Resolved
2. **Validation of URL Inputs**:
- The `isValidUrl` function is well-implemented to prevent phishing and open redirect vulnerabilities by ensuring that only valid HTTP and HTTPS protocols are accepted. This is crucial for maintaining the integrity of the application and protecting users from malicious links.
- The validation logic is sound, and the fallback to 'about:blank' in the `sanitizeUrlForDisplay` function is a good practice to avoid executing any potentially harmful scripts.
**Status**: Resolved
3. **Untrusted Input Paths**:
- There are no new untrusted input paths introduced in this pull request. The changes made to the URL handling are focused on sanitization and validation, which are essential for maintaining security.
- The existing input paths for URLs remain unchanged, and the new sanitization function effectively addresses potential vulnerabilities.
**Status**: Resolved
### Additional Observations:
- The changes made to the layout and structure of the components do not introduce any security concerns. The focus on using layout primitives (like `Stack`, `Grid`, and `Box`) is commendable and aligns with the design system guidelines.
- The overall implementation appears to adhere to best practices for security and data handling, particularly concerning user input.
### Conclusion:
The pull request effectively addresses the previous findings related to XSS vulnerabilities and URL handling. The introduction of proper sanitization and validation functions enhances the security posture of the application without introducing new risks.
Based on the review, I find that the changes are appropriate and do not introduce any new security issues.
```json
#### [STYLE] Review
Upon reviewing the provided pull request changes, I have identified a few key points regarding style, maintainability, and adherence to design tokens. Here are my findings:
1. **Inline Styles Usage**:
- The use of inline styles in the `ViewportFrame` component is still present:
```tsx
style={{ // impeccable-ignore - Dynamic scaling for iframe preview
transform: `scale(${scale})`,
width: `${width}px`,
height: `${height}px`,
minWidth: `${width}px`,
minHeight: `${height}px`,
maxWidth: 'none',
maxHeight: 'none',
}}
```
- **Issue**: Inline styles should be avoided in favor of layout primitives. This violates the design system's guidelines.
- **Recommendation**: Refactor to use layout primitives for width and height instead of inline styles.
2. **Redundant `minWidth` Prop**:
- The `minWidth={0}` prop is used multiple times, including in the `Stack` component:
```tsx
<Stack gap={1} minWidth="0" flex={1}>
```
- **Issue**: This prop is redundant if it is not necessary for layout purposes.
- **Recommendation**: Review and remove unnecessary `minWidth` props to simplify the code.
3. **Flex Properties in Grid Context**:
- The `flex={1}` property is used in the input field:
```tsx
<Box as="input" ... flex={1} minWidth={0} ... />
```
- **Issue**: Using flex properties within a grid layout can lead to conflicts and unexpected behavior.
- **Recommendation**: Ensure that the layout structure is consistent and does not mix flex and grid properties unnecessarily.
4. **Truncation Functionality**:
- The `truncate` prop has been added to the `Text` component for the active report URL:
```tsx
<Text
...
truncate={true}
title={sanitizeUrlForDisplay(activeReport.url)}
>
```
- **Status**: This is a positive change as it addresses the overflow issue for long URLs.
5. **Sanitization of URLs**:
- The introduction of the `sanitizeUrlForDisplay` function is a good practice to mitigate XSS vulnerabilities:
```tsx
export const sanitizeUrlForDisplay = (urlStr: string | null | undefined): string => {
...
};
```
- **Status**: This is a positive change that enhances security.
### Summary of Findings:
- The inline styles in `ViewportFrame` should be refactored to use layout primitives.
- Redundant `minWidth` props should be reviewed and removed for clarity.
- Mixing flex properties in a grid context should be avoided to prevent layout conflicts.
Given these observations, I recommend addressing the inline styles and redundant props to improve maintainability and adherence to the design system.
```json
Generated by github-models-code-review |
…ributes - Configured responsive container span, min-width, and text truncation constraints on Active Session card. - Sanitized `url` and `activeReport.url` inside HTML display attributes (`title`) using `isValidUrl` to prevent security and XSS warnings. - Updated Playwright visual regression baselines.
…ributes - Configured responsive container span, min-width, and text truncation constraints on Active Session card. - Sanitized `url` and `activeReport.url` inside HTML display attributes (`title`) using `isValidUrl` to prevent security and XSS warnings. - Updated Playwright visual regression baselines.
…ributes - Configured responsive container span, min-width, and text truncation constraints on Active Session card. - Sanitized `url` and `activeReport.url` inside HTML display attributes (`title`) using `isValidUrl` to prevent security and XSS warnings. - Verified visual impact analysis locally across all responsive viewports. - Updated Playwright visual regression baselines.
…ributes - Configured responsive container span, min-width, and text truncation constraints on Active Session card. - Sanitized `url` and `activeReport.url` inside HTML display attributes (`title`) using `isValidUrl` to prevent security and XSS warnings. - Verified visual impact analysis locally across all responsive viewports, validating cropped layout changes successfully. - Updated Playwright visual regression baselines.
…ion summary - Removed hardcoded inline grid overrides on the UX Auditor detailed view stack container, opting for standard responsive layout properties. - Converted URL display text to standard ellipsis truncation using design primitives to handle extremely long URLs safely on tablet/desktop viewports. - Sanitized titles against XSS vulnerability vectors using isValidUrl helpers. - Overrode Tailwind preflight styles on simulation iframe elements to prevent them from shrinking and collapsing, guaranteeing desktop and mobile simulated viewports scale correctly and preserve device layouts. - Updated Playwright visual regression test baselines.
arii
force-pushed
the
jules-8606747082813661780-8d785156
branch
from
July 19, 2026 23:15
cbfe9e7 to
dcebb87
Compare
Owner
🤖 AI Technical AuditANTI-AI-SLOPThe implementation of FINAL RECOMMENDATIONApproved with Minor Changes DEFINITION OF DONE
Review automatically published via RepoAuditor. |
arii
requested changes
Jul 20, 2026
arii
left a comment
arii
left a comment
Owner
There was a problem hiding this comment.
PR Review: #3820
Context
- Last Commit Tracked (SHA): dcebb87
Audit Checklist
For EVERY changed file, verify against these standards. Mark as - [x] when verified.
- Dead abstractions: No new class, context, or hook that a simpler primitive handles.
- Unnecessary indirection: No layer of wrapping where a direct function call suffices.
- Responsibility creep: Component does not take on state/logic belonging in parent/hook.
- Import bloat: No unnecessary
import React from 'react'(React 17+). - Token compliance: Uses established design tokens (no raw Tailwind values or inline styles).
- Audit ratio: If > 100 lines added, identified at least 10 lines to refactor/remove.
CI Log Triage
(Populated if CI failures detected)
-
Failed Checks:
-
Deployment Impact Analysis
-
Detected Errors:
None detected by parser. -
Root Cause Analysis:
-
Visual snapshots failed due to the addition of
truncate={true}to truncate long URLs, resolving layout shifts from grid overflows. This is an expected change. -
Remediation Steps:
-
Manually review the visual diff artifacts to verify that the UI hasn't functionally degraded.
-
Dead abstractions: N/A.
-
Unnecessary indirection: N/A.
-
Responsibility creep: Introduced
sanitizeUrlForDisplaydirectly in theUXAuditor.tsxUI file, violating the architectural rule that utility functions for security and sanitization should be extracted to shared utility modules (src/utils/url.ts). -
Import bloat: No unnecessary imports found.
-
Token compliance: Correctly utilized standard layout primitive props (e.g.
truncate={true}) instead of manual Tailwind truncation classes. -
Audit ratio: N/A.
-
The layout issue regarding grid/flex boundaries for overflowing URLs is correctly resolved by applying
maxWidth='none',truncate={true}, and modifyingStackprops. -
Violation: The PR violates the architectural directive to extract URL sanitizers into shared utilities.
sanitizeUrlForDisplaymust be moved out ofsrc/pages/UXAuditor.tsxand intosrc/utils/url.ts. -
Failing CI Checks: Deployment Impact Analysis (Visual Diffs) failed, which is expected due to stylistic layout adjustments. However, it blocks approval until manually reviewed.
Not Approved
Inline Comments (Fallback due to line resolution errors)
- src/pages/UXAuditor.tsx:20: Anti-pattern: Utility functions for security and sanitization (such as URL sanitizers to prevent XSS) should not be defined inline within UI components. They must be extracted to shared utility modules (e.g.,
src/utils/url.ts) to strictly enforce separation of concerns.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Owner
🤖 AI Technical AuditANTI-AI-SLOPThe changes introduced in this PR are focused and necessary. The addition of a sanitization utility for display attributes ( FINAL RECOMMENDATIONApproved DEFINITION OF DONEN/A - Changes approved as is. Review automatically published via RepoAuditor. |
#3882) Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
arii
marked this pull request as ready for review
July 21, 2026 17:49
arii
enabled auto-merge (squash)
July 21, 2026 17:49
arii
disabled auto-merge
July 21, 2026 17:49
Owner
🤖 AI Technical AuditANTI-AI-SLOPThis PR demonstrates a disciplined approach to solving common flexbox/grid layout issues. The use of FINAL RECOMMENDATIONApproved DEFINITION OF DONEN/A - No modifications required. Review automatically published via RepoAuditor. |
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
2 participants
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.
Fixed grid layout and flexbox constraints on the active session summary header inside the /ux-auditor page to support clean truncation of long URLs and prevent overlap and overflow across responsive viewports.
Fixes #3773
PR created automatically by Jules for task 8606747082813661780 started by @arii