fix: scrolling glitch in environment#7732
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
WalkthroughThis PR enhances the EnvironmentVariablesTable component with smooth height animations and improves virtualized table rendering. Changes include CSS transitions for animated height transitions, updated row wrapper prop handling, dynamic height calculations, overscan adjustments, and automatic empty row appending when editing the last variable's value. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.js (1)
22-33:⚠️ Potential issue | 🟠 MajorFix the
TableRowshape:itemis{ variable, index }, not the raw variable.
TableVirtuosopasses each element fromfilteredVariablesdirectly as theitemprop. SincefilteredVariablescontains{ variable, index }objects, accessingitem.uidanditem.namereturnsundefined. This collapses all rowdata-testidvalues toenv-var-row-undefined, breaking test selectors. The memoization comparator then comparesundefined === undefinedfor every row (always true), preventing necessary re-renders. Excludingstylefrom the comparison also breaks Virtuoso's positioning logic.Suggested fix
const TableRow = React.memo( ({ children, item, style, ...rest }) => ( - <tr key={item.uid} style={style} {...rest} data-testid={`env-var-row-${item?.name}`}> + <tr + style={style} + {...rest} + data-testid={`env-var-row-${item?.variable?.uid ?? item?.index}`} + > {children} </tr> - ), - (prevProps, nextProps) => { - const prevUid = prevProps?.item?.uid; - const nextUid = nextProps?.item?.uid; - return prevUid === nextUid && prevProps.children === nextProps.children; - } + ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` around lines 22 - 33, TableRow currently treats item as the raw variable but TableVirtuoso provides item as { variable, index }, causing uid/name to be undefined and memoization to incorrectly block renders; update the render to read const variable = item?.variable and use variable?.uid for the key and variable?.name in data-testid, and update the memo comparator to compare prevProps.item?.variable?.uid !== nextProps.item?.variable?.uid (or equality) plus prevProps.children === nextProps.children AND prevProps.style === nextProps.style so style changes from Virtuoso still trigger re-renders.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/EnvironmentVariablesTable/index.js (1)
627-628: Please rewrite or remove this inline note.The sentence is hard to parse and already stale: it mentions
0.1s, while the new height transition is0.15s. If this is a real follow-up, capture the concrete layout constraint instead of leaving a half-formed note.As per coding guidelines "Add in meaningful comments instead of obvious ones where complex code flow is explained properly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` around lines 627 - 628, The inline dev note in the EnvironmentVariablesTable component about button placement is stale and unclear—remove it or replace it with a concise, actionable comment that documents the real layout constraint: TableVirtuoso's height transition (0.15s) can cause buttons inside the virtuoso container to reflow during mount, so either render action buttons outside the TableVirtuoso virtualization container or use a stable positioning strategy (e.g., position: sticky/fixed or a separate toolbar) to avoid transition-driven layout shifts; update the comment to reference TableVirtuoso and the 0.15s transition and include the chosen mitigation so future maintainers know why the buttons are placed where they are.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 563-565: The placeholder logic in EnvironmentVariablesTable treats
falsy values like 0 and false as empty because it uses !variable.value; update
the placeholder expression to only consider null/undefined or blank strings as
empty. Specifically, replace the !variable.value check in the placeholder prop
for the variable.value field with an explicit check like variable.value ===
undefined || variable.value === null || (typeof variable.value === 'string' &&
variable.value.trim() === '') so numeric 0 and boolean false are preserved as
real values.
---
Outside diff comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 22-33: TableRow currently treats item as the raw variable but
TableVirtuoso provides item as { variable, index }, causing uid/name to be
undefined and memoization to incorrectly block renders; update the render to
read const variable = item?.variable and use variable?.uid for the key and
variable?.name in data-testid, and update the memo comparator to compare
prevProps.item?.variable?.uid !== nextProps.item?.variable?.uid (or equality)
plus prevProps.children === nextProps.children AND prevProps.style ===
nextProps.style so style changes from Virtuoso still trigger re-renders.
---
Nitpick comments:
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js`:
- Around line 627-628: The inline dev note in the EnvironmentVariablesTable
component about button placement is stale and unclear—remove it or replace it
with a concise, actionable comment that documents the real layout constraint:
TableVirtuoso's height transition (0.15s) can cause buttons inside the virtuoso
container to reflow during mount, so either render action buttons outside the
TableVirtuoso virtualization container or use a stable positioning strategy
(e.g., position: sticky/fixed or a separate toolbar) to avoid transition-driven
layout shifts; update the comment to reference TableVirtuoso and the 0.15s
transition and include the chosen mitigation so future maintainers know why the
buttons are placed where they are.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ba89434-5814-477e-9b21-ae763bdc0495
📒 Files selected for processing (2)
packages/bruno-app/src/components/EnvironmentVariablesTable/StyledWrapper.jspackages/bruno-app/src/components/EnvironmentVariablesTable/index.js
| value={variable.value} | ||
| placeholder={isLastEmptyRow ? 'Value' : ''} | ||
| placeholder={!variable.value || (typeof variable.value === 'string' && variable.value.trim() === '') ? 'Value' : ''} | ||
| isSecret={variable.secret} |
There was a problem hiding this comment.
Don't treat 0 and false as empty values here.
!variable.value sends legitimate falsy env values down the placeholder branch. That makes script-set values like 0 and false look empty even though they are valid values.
Suggested fix
- placeholder={!variable.value || (typeof variable.value === 'string' && variable.value.trim() === '') ? 'Value' : ''}
+ placeholder={
+ variable.value == null || (typeof variable.value === 'string' && variable.value.trim() === '')
+ ? 'Value'
+ : ''
+ }Based on learnings "falsy values are legitimate environment variable values and should not be filtered out."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/EnvironmentVariablesTable/index.js` around
lines 563 - 565, The placeholder logic in EnvironmentVariablesTable treats falsy
values like 0 and false as empty because it uses !variable.value; update the
placeholder expression to only consider null/undefined or blank strings as
empty. Specifically, replace the !variable.value check in the placeholder prop
for the variable.value field with an explicit check like variable.value ===
undefined || variable.value === null || (typeof variable.value === 'string' &&
variable.value.trim() === '') so numeric 0 and boolean false are preserved as
real values.
1 participant
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Release Notes
New Features
Style
Bug Fixes