Test - changed hovered portal border behavior#19050
Test - changed hovered portal border behavior#19050lucasbordeau wants to merge 1 commit intomainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellHoveredPortalContent.tsx">
<violation number="1" location="packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellHoveredPortalContent.tsx:109">
P2: `isTouchingFirstColumn` is off by one. This treats the second column as the left edge and still expands the real first column outside the table.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
|
|
||
| const isTouchingHeader = recordTableHoverPosition?.row === 0; | ||
| const isTouchingFirstColumn = recordTableHoverPosition?.column === 1; |
There was a problem hiding this comment.
P2: isTouchingFirstColumn is off by one. This treats the second column as the left edge and still expands the real first column outside the table.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellHoveredPortalContent.tsx, line 109:
<comment>`isTouchingFirstColumn` is off by one. This treats the second column as the left edge and still expands the real first column outside the table.</comment>
<file context>
@@ -75,23 +105,51 @@ export const RecordTableCellHoveredPortalContent = () => {
);
+ const isTouchingHeader = recordTableHoverPosition?.row === 0;
+ const isTouchingFirstColumn = recordTableHoverPosition?.column === 1;
+
+ const topExpansion = showInteractiveStyle ? (isTouchingHeader ? 0 : 1) : 1;
</file context>
| const isTouchingFirstColumn = recordTableHoverPosition?.column === 1; | |
| const isTouchingFirstColumn = recordTableHoverPosition?.column === 0; |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the record-table hovered/focused/edit-mode portal layout so portal borders can extend/overlap neighboring cell borders (especially near table edges) to evaluate a “nicer overlapping UI” behavior.
Changes:
- Added offset/expansion controls to
RecordTableCellPortalRootContainerto allow positioning and sizing beyond the anchor cell. - Reworked hovered and focused portal content sizing by expanding the outer container and absolutely positioning an inner content wrapper.
- Updated hovered/focused/edit-mode portal instantiation to pass the new offset/expansion props.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellPortalRootContainer.tsx | Adds top/left offsets and width/height expansion to the portal root container styling. |
| packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellHoveredPortalContent.tsx | Expands hovered portal container and introduces an inner absolute wrapper to keep content aligned while border overlaps. |
| packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellHoveredPortal.tsx | Passes the new offset/expansion props (currently zeros) to the hovered portal root. |
| packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellFocusedPortalContent.tsx | Switches focused portal content to a 100% height container + inner wrapper for border inset control. |
| packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellFocusedPortal.tsx | Expands/offsets the focused portal root container based on whether the focus is at top/left edges. |
| packages/twenty-front/src/modules/object-record/record-table/record-table-cell/components/RecordTableCellEditModePortal.tsx | Passes the new offset/expansion props (zeros) to the edit-mode portal root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const StyledRecordTableCellPortalRootContainer = styled.div<{ | ||
| zIndex?: number; | ||
| topOffset: number; | ||
| leftOffset: number; | ||
| widthExpansion: number; | ||
| heightExpansion: number; | ||
| }>` | ||
| height: 100%; | ||
| left: 0; | ||
| height: calc(100% + ${({ heightExpansion }) => heightExpansion}px); | ||
| left: ${({ leftOffset }) => leftOffset}px; | ||
| position: absolute; | ||
| top: 0; | ||
| width: 100%; | ||
| top: ${({ topOffset }) => topOffset}px; | ||
| width: calc(100% + ${({ widthExpansion }) => widthExpansion}px); | ||
| z-index: ${({ zIndex }) => zIndex ?? 'auto'}; |
There was a problem hiding this comment.
RecordTableCellPortalRootContainer now requires topOffset/leftOffset/widthExpansion/heightExpansion at every call site. Since most usages pass 0, consider making these props optional with a default of 0 (and keep the “expanded” behavior only where needed). This reduces repetitive boilerplate and prevents future call sites from accidentally breaking due to missing offsets.
| const isTouchingHeader = recordTableHoverPosition?.row === 0; | ||
| const isTouchingFirstColumn = recordTableHoverPosition?.column === 1; | ||
|
|
||
| const topExpansion = showInteractiveStyle ? (isTouchingHeader ? 0 : 1) : 1; | ||
| const leftExpansion = showInteractiveStyle | ||
| ? isTouchingFirstColumn | ||
| ? 0 | ||
| : 1 | ||
| : 1; |
There was a problem hiding this comment.
isTouchingFirstColumn is computed as recordTableHoverPosition?.column === 1, but elsewhere in this component (and in other record-table code) the first field column is treated as column === 0 (see isFirstColumn). Since recordTableHoverPosition.column comes from RecordTableCellWrapper's recordFieldIndex, this looks off-by-one and will apply the “edge” expansion logic to the second field column instead of the first. Consider using column === 0 (or reusing isFirstColumn) or renaming the variable if the intent is actually “second column” / “first non-sticky column”.
| const isTouchingHeader = recordTableFocusPosition?.row === 0; | ||
| const isTouchingFirstColumn = recordTableFocusPosition?.column === 1; | ||
|
|
||
| const contentTop = isTouchingHeader ? 0 : 1; | ||
| const contentLeft = isTouchingFirstColumn ? 0 : 1; | ||
| const contentWidth = isTouchingFirstColumn | ||
| ? 'calc(100% - 1px)' | ||
| : 'calc(100% - 2px)'; | ||
| const contentHeight = isTouchingHeader |
There was a problem hiding this comment.
isTouchingFirstColumn uses recordTableFocusPosition?.column === 1, but the record-table field columns are 0-indexed (coming from recordFieldIndex), and other logic treats column === 0 as the first field column. This likely shifts the inset/size calculations by one column, making the focused portal content layout incorrect for the actual leftmost field column. Use column === 0 for the first field column (or rename if the intent is “second column”).
| const isTouchingHeader = recordTableFocusPosition.row === 0; | ||
| const isTouchingFirstColumn = recordTableFocusPosition.column === 1; | ||
|
|
||
| const topOffset = isTouchingHeader ? 0 : -1; | ||
| const leftOffset = isTouchingFirstColumn ? 0 : -1; | ||
| const widthExpansion = isTouchingFirstColumn ? 1 : 2; | ||
| const heightExpansion = isTouchingHeader ? 1 : 2; |
There was a problem hiding this comment.
isTouchingFirstColumn is currently recordTableFocusPosition.column === 1, but focus positions use the field recordFieldIndex (0-based). If the goal is to avoid expanding into the left edge/sticky boundary for the first field column, this should likely be column === 0; as written it will treat the second field column as the left edge and still expand left for column 0.
This PR is a visual test to see if a new hovered portal border behavior could bring the old overlapping nicer UI we add.
With the exception of cells that are touching the header row or the first column :