-
Notifications
You must be signed in to change notification settings - Fork 59
fix expression overflow in record config model #1004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix expression overflow in record config model #1004
Conversation
WalkthroughModified the ChipExpressionEditor component to improve height and sizing constraints for CodeMirror editor. Added maxHeight constraints to the scroller across expanded and non-expanded states, reworked conditional height logic based on expanded mode or explicit sx height, and standardized editor container sizing to use 100% width and height when applicable. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (2)
304-317: Eliminate duplicate theme configuration.The first two conditional branches (lines 305-308 and 310-313) are identical. They can be combined to reduce code duplication and improve maintainability.
Apply this diff to consolidate the logic:
- ...(props.isInExpandedMode - ? [EditorView.theme({ - "&": { height: "100%" }, - ".cm-scroller": { overflow: "auto", maxHeight: "100%" } - })] - : props.sx && 'height' in props.sx - ? [EditorView.theme({ - "&": { height: "100%" }, - ".cm-scroller": { overflow: "auto", maxHeight: "100%" } - })] - : [EditorView.theme({ - "&": { maxHeight: "150px" }, - ".cm-scroller": { overflow: "auto", maxHeight: "150px" } - })]) + ...(props.isInExpandedMode || (props.sx && 'height' in props.sx) + ? [EditorView.theme({ + "&": { height: "100%" }, + ".cm-scroller": { overflow: "auto", maxHeight: "100%" } + })] + : [EditorView.theme({ + "&": { maxHeight: "150px" }, + ".cm-scroller": { overflow: "auto", maxHeight: "150px" } + })])
434-434: Simplify the nested ternary for better readability.The nested ternary checking
isInExpandedMode, thenprops.sx && 'height' in props.sx, is difficult to parse quickly. Consider extracting this logic to improve clarity.Consider this approach:
+ const containerHeight = props.isInExpandedMode + ? '100%' + : (props.sx && 'height' in props.sx) + ? undefined // Let sx.height take effect + : 'auto'; + <ChipEditorContainer ref={fieldContainerRef} style={{ position: 'relative', ...props.sx, - ...(props.isInExpandedMode ? { height: '100%' } : props.sx && 'height' in props.sx ? {} : { height: 'auto' }) + ...(containerHeight !== undefined && { height: containerHeight }) }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-24T22:16:28.380Z
Learnt from: dan-niles
Repo: wso2/vscode-extensions PR: 980
File: workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx:384-412
Timestamp: 2025-11-24T22:16:28.380Z
Learning: In the RichTextTemplateEditor component (workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx), token fetching on external `value` prop changes is intentionally disabled. Users cannot edit both the minimized and expanded editors simultaneously, so tokens only need to be generated based on user typing in the expanded editor view, not on external prop updates.
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/**/*.{ts,tsx} : Define all constants (node types, sizing, spacing) in src/resources/constants.ts and import them where needed instead of hardcoding values
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/**/*.tsx : Use React 18.2.0 features including concurrent rendering and automatic batching; avoid class components in favor of functional components with hooks
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/utils/diagram.ts : Implement dynamic node height calculation based on visible function count: BASE_HEIGHT (72px) + (FUNCTION_HEIGHT (48px) per visible function) + VIEW_ALL_BUTTON_HEIGHT (40px if needed)
Applied to files:
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
🔇 Additional comments (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
437-446: I'm unable to proceed with verification due to a repository cloning failure. Let me explain the situation:Repository Access Issue
The sandbox environment encountered an error while attempting to clone the
wso2/vscode-extensionsrepository. This prevents me from:
- Examining the actual code around lines 437-446 to verify the height styling logic
- Confirming the CodeMirror theme configuration to validate the claim about
maxHeight: "150px"- Inspecting related code to understand the original issue context or test patterns
- Verifying the actual impact of the alleged circular dependency in different render modes
What the review comment claims (unverified):
- Editor div unconditionally sets
height: '100%'- Wrapper div conditionally sets height only in expanded or explicit height modes
- CodeMirror theme's
maxHeight: "150px"prevents unbounded growth- The logic is "fragile" and could behave unexpectedly
Without access to the repository, I cannot confirm whether these technical claims are accurate or whether the suggested fix actually addresses a real problem.
Purpose
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.