feature(component)-side-panel sizes#18298
Open
NDCallahan wants to merge 15 commits intoBudibase:masterfrom
Open
feature(component)-side-panel sizes#18298NDCallahan wants to merge 15 commits intoBudibase:masterfrom
NDCallahan wants to merge 15 commits intoBudibase:masterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 7 files
Confidence score: 3/5
- There is a concrete regression risk in
packages/client/src/components/app/SidePanel.svelte: closed instances can remove global#side-panel-containerbackground styles, so one panel closing may visually affect other panels that are still open. packages/client/src/components/app/Layout.sveltemay unintentionally constrainsize--maxto 400px, which could shrink nav/main areas configured as Max; this is user-facing layout impact, though confidence is low.packages/builder/src/components/design/settings/controls/ButtonActionEditor/actions/OpenSidePanel.svelteremoved.rootstyles while still usingclass="root", which likely disrupts the two-column action editor layout (alignment/width/centering).- Pay close attention to
packages/client/src/components/app/SidePanel.svelte,packages/client/src/components/app/Layout.svelte, andpackages/builder/src/components/design/settings/controls/ButtonActionEditor/actions/OpenSidePanel.svelte- potential shared-style side effects and layout regressions.
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/client/src/components/app/Layout.svelte">
<violation number="1" location="packages/client/src/components/app/Layout.svelte:556">
P2: `size--max` no longer provides full-width layouts; it now limits Max to 400px, constraining any nav or main area configured as Max.</violation>
</file>
<file name="packages/builder/src/components/design/settings/controls/ButtonActionEditor/actions/OpenSidePanel.svelte">
<violation number="1" location="packages/builder/src/components/design/settings/controls/ButtonActionEditor/actions/OpenSidePanel.svelte:46">
P2: The `.root` layout styles were removed but the component still relies on `class="root"`. This likely breaks the action editor’s two-column layout (grid, width, centering), causing the Side Panel/Size controls to misalign or stack incorrectly. Restore the `.root` styles or provide a replacement layout class.</violation>
</file>
<file name="packages/client/src/components/app/SidePanel.svelte">
<violation number="1" location="packages/client/src/components/app/SidePanel.svelte:95">
P2: Closed SidePanel instances can clear the shared container background because every instance calls applyContainerBackground(open, ...) and the false branch removes background/background-image on the global #side-panel-container. With multiple SidePanels, a closed instance can override the open panel’s background.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...lder/src/components/design/settings/controls/ButtonActionEditor/actions/OpenSidePanel.svelte
Show resolved
Hide resolved
NDCallahan
force-pushed
the
feature/sidepanel-sizes
branch
from
09093b6 to
7dd88cc
Compare
Contributor
Author
|
@cubic recheck |
Contributor
@NDCallahan I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 2/5
- There is a clear runtime breakage risk:
packages/client/src/components/app/SidePanel.svelteremovedhandleSidePanelClosewhile a live call remains, which is likely to throw aReferenceErrorwhen users close the panel. packages/client/src/components/app/Layout.svelteappears to introduce a user-facing layout regression by forcingsize--maxto 400px, which can incorrectly constrain views expected to be full width.- Given one high-confidence functional error plus a likely UI regression, this is high merge risk until these are corrected.
- Pay close attention to
packages/client/src/components/app/SidePanel.svelte,packages/client/src/components/app/Layout.svelte- close-handler runtime failure and max-width behavior regression.
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/client/src/components/app/SidePanel.svelte">
<violation number="1" location="packages/client/src/components/app/SidePanel.svelte:47">
P1: Removed `handleSidePanelClose` definition leaves a live call site that will throw `ReferenceError` when the panel closes.</violation>
</file>
<file name="packages/client/src/components/app/Layout.svelte">
<violation number="1" location="packages/client/src/components/app/Layout.svelte:556">
P2: `size--max` now forces a 400px width, but this class is still used for “Max” page/nav widths. This regresses full-width layouts by constraining them to 400px.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic recheck please |
Contributor
@NDCallahan I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- There is some real merge risk:
packages/client/src/components/app/SidePanel.sveltehas a concrete user-facing UI regression where a closed panel can clear the shared container background and override the active panel, leading to incorrect/flickering state. - A second medium issue in
packages/client/src/components/app/SidePanel.sveltecan cause reactive churn in builder mode becausesetSizeupdates the store even when size is unchanged, which may create self-retriggering updates whileopenandeffectiveSizestay constant. - Given one clear UI behavior bug (6/10, high confidence) plus a likely reactive loop/perf concern, this lands as moderate risk rather than merge-blocking.
- Pay close attention to
packages/client/src/components/app/SidePanel.svelte- shared background state cleanup and size-update reactivity need guarding to avoid flicker and update churn.
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/client/src/components/app/SidePanel.svelte">
<violation number="1" location="packages/client/src/components/app/SidePanel.svelte:61">
P2: Builder-mode reactive block can re-trigger itself because setSize always updates the store even when size is unchanged, causing a reactive loop/churn when open and effectiveSize are constant.</violation>
<violation number="2" location="packages/client/src/components/app/SidePanel.svelte:95">
P2: Closed SidePanel instances clear the shared side-panel container background, so they can override the active panel’s background and cause incorrect/flickering UI state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic recheck one more time please. :) |
Contributor
@NDCallahan I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- There is a concrete user-facing behavior risk in
packages/client/src/components/app/SidePanel.svelte: reactive background updates from non-open panel instances can clear a shared container background while another panel is active. packages/client/src/components/app/Layout.sveltealso has a likely UI regression:.nav-togglestill renders but lost its base CSS, which can reduce click-target clarity and affordance for collapsing the left nav.- Given one medium-severity functional interaction issue (6/10) plus a confirmed styling regression (5/10), this looks mergeable with caution but carries some regression risk.
- Pay close attention to
packages/client/src/components/app/SidePanel.svelteandpackages/client/src/components/app/Layout.svelte- shared background state conflicts and missing base.nav-togglestyling may impact active panel visuals and nav usability.
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/client/src/components/app/SidePanel.svelte">
<violation number="1" location="packages/client/src/components/app/SidePanel.svelte:102">
P2: Reactive background updates from non-open SidePanel instances can clear the shared container background while another panel is active.</violation>
</file>
<file name="packages/client/src/components/app/Layout.svelte">
<violation number="1" location="packages/client/src/components/app/Layout.svelte:634">
P2: The `.nav-toggle` control is still rendered but its base CSS was removed, leaving only `:hover` and regressing click-target/affordance styling for left-nav collapse.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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="hosting/docker-compose.yaml">
<violation number="1">
P1: CouchDB image was downgraded to `budibase/database:2.0.0` in the main compose manifest, creating DB version skew versus other deployment files pinned to `2.1.0`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
I remove this issue @cubic. Can you recheck |
Contributor
@NDCallahan I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- There is some merge risk:
packages/client/src/components/app/SidePanel.sveltehas a concrete behavior issue (6/10, high confidence) where reactive background effects run from every instance, so inactive panels can clear a shared container background. packages/client/src/components/app/Layout.sveltealso has a user-facing regression (5/10) where the left-nav collapse toggle remains clickable but lost its base.nav-togglestyling, which can reduce affordance and click-target consistency.- Pay close attention to
packages/client/src/components/app/SidePanel.svelteandpackages/client/src/components/app/Layout.svelte- cross-instance background side effects and nav-toggle styling regression can impact visible UI behavior.
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/client/src/components/app/SidePanel.svelte">
<violation number="1" location="packages/client/src/components/app/SidePanel.svelte:102">
P2: Reactive background side effects are applied from every SidePanel instance, allowing inactive panels to clear the shared container background.</violation>
</file>
<file name="packages/client/src/components/app/Layout.svelte">
<violation number="1" location="packages/client/src/components/app/Layout.svelte:634">
P2: The clickable left-nav collapse toggle lost its base `.nav-toggle` styling while still being interactive, degrading click affordance and target consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| : $component.styles | ||
|
|
||
| // Apply / remove background on the container whenever open or bg changes. | ||
| $: applyContainerBackground(open, panelBgColor, panelBgImage) |
Contributor
There was a problem hiding this comment.
P2: Reactive background side effects are applied from every SidePanel instance, allowing inactive panels to clear the shared container background.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/components/app/SidePanel.svelte, line 102:
<comment>Reactive background side effects are applied from every SidePanel instance, allowing inactive panels to clear the shared container background.</comment>
<file context>
@@ -44,12 +46,88 @@
+ : $component.styles
+
+ // Apply / remove background on the container whenever open or bg changes.
+ $: applyContainerBackground(open, panelBgColor, panelBgImage)
+
const handleSidePanelClose = async () => {
</file context>
Suggested change
| $: applyContainerBackground(open, panelBgColor, panelBgImage) | |
| $: if (open) { | |
| applyContainerBackground(true, panelBgColor, panelBgImage) | |
| } |
| flex-shrink: 0; | ||
| align-self: flex-start; | ||
| } | ||
| /* Note: size variants are defined earlier; keep those as the source of width values. */ |
Contributor
There was a problem hiding this comment.
P2: The clickable left-nav collapse toggle lost its base .nav-toggle styling while still being interactive, degrading click affordance and target consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/components/app/Layout.svelte, line 634:
<comment>The clickable left-nav collapse toggle lost its base `.nav-toggle` styling while still being interactive, degrading click affordance and target consistency.</comment>
<file context>
@@ -615,22 +631,7 @@
- flex-shrink: 0;
- align-self: flex-start;
- }
+ /* Note: size variants are defined earlier; keep those as the source of width values. */
.nav-toggle:hover {
background-color: rgba(255, 255, 255, 0.25);
</file context>
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
1 participant
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.
Description
Adds a
sizeoption to the Side Panel component to havesmallmediumlargefullscreenoptions available when using a size panel. Adds aDefaultsize option to the Open Side Panel action so it can inherit the size already configured on the Side Panel component (instead of always forcing the action to pick a size). This makes the action behave like “use whatever size the component is configured with” when the component already has a size set.Addresses
App Export
Screenshots
Launchcontrol
Add a small description in layman's terms of what this PR achieves. This will be used in the release notes.
Summary by cubic
Adds Side Panel size variants and a “Default” option in the
Open Side Panelaction to inherit the component’s size. Moves background styling to the outer container so the header matches, prevents flicker and reactive churn, and restores async close, the action editor’s two‑column layout, and the full‑width layout.Open Side Panelaction: Size selector; “Default” inherits the component’s size. If the component already has a size, the action defaults to “Default”; explicit sizes pass through to runtime.background/background-image. Inner background is stripped and the container is cleared on close to avoid flicker (including gradients) across panels.Written for commit 084d330. Summary will update on new commits.