Skip to content

fix(theme): fill SidebarPage viewport to close NFS layout gap - #4151

Merged
ciiay merged 1 commit into
redhat-developer:mainfrom
ciiay:rhdhbugs-3498-fix-layout-gap
Aug 4, 2026
Merged

fix(theme): fill SidebarPage viewport to close NFS layout gap#4151
ciiay merged 1 commit into
redhat-developer:mainfrom
ciiay:rhdhbugs-3498-fix-layout-gap

Conversation

@ciiay

@ciiay ciiay commented Aug 4, 2026

Copy link
Copy Markdown
Member

Description

Fixes the NFS viewport gap on short pages (e.g. Settings) by restoring height participation on BackstageSidebarPage in the theme plugin. Short pages no longer collapse to content height and leave a body/page-inset bleed under the shell.

The page root now uses minHeight: 100vh with a column flex layout so BUI containers can grow, and desktop content wells (main, article/BackstageContent, non-header bui-Container) use mainSectionBackgroundColor with a matching minHeight on main. This is a shell-level theme fix — not an ApplicationDrawer change (supersedes the approach in #4077).

Fixed

  • RHDHBUGS-3498 — [ApplicationDrawer] Empty space at bottom of viewport in NFS mode

Test Plan

  • Start an NFS app that loads the RHDH theme (e.g. workspaces/intelligent-assistant, workspaces/app-defaults with theme, or RHDH yarn workspace backend start:next)
  • Open Settings (or another short page) in dark theme — no empty band below content; page-inset / #root fills the viewport (no body background bleed)
  • Repeat in light theme — same: no empty gap under short content
  • Open/close ApplicationDrawer — content still shifts correctly; no layout regression
  • Spot-check a longer page (Catalog index / entity Overview) — no unexpected scrollbars or overflow from main minHeight/maxHeight
  • Confirm TechDocs (if available) still avoids double scrollbar (main:has([data-testid='techdocs-native-shadowroot']) override)

Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

Made with Cursor

image

Fixes: https://redhat.atlassian.net/browse/RHDHBUGS-3498
Signed-off-by: Yi Cai <yicai@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@rhdh-gh-app

rhdh-gh-app Bot commented Aug 4, 2026

Copy link
Copy Markdown

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-theme workspaces/theme/plugins/theme patch v1.0.1

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Fix theme: make BackstageSidebarPage fill viewport to remove NFS layout gap

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Make BackstageSidebarPage participate in viewport height to prevent short-page shell gaps.
• Ensure desktop inset content wells paint with mainSectionBackgroundColor for consistent
 backgrounds.
• Add regression tests covering minHeight/flex growth and NFS (BUI Container) selectors.
Diagram

graph TD
  A["Theme config"] --> B["createComponents.ts"] --> C["BackstageSidebarPage overrides"] --> D["Inset wells (main/article/BUI)"] --> E["Short NFS pages"]
  F["createComponents.test.ts"] --> B
  G[".changeset entry"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Fix via ApplicationDrawer/root wrapper layout
  • ➕ Keeps BackstageSidebarPage overrides simpler
  • ➕ May address other wrappers that collapse to content height
  • ➖ Couples the fix to a specific shell wrapper; less reusable across apps
  • ➖ Risk of regressions in other layouts/drawers; harder to reason about across pages
2. Global html/body/#root height:100% + flex column
  • ➕ One-time global baseline that can prevent similar viewport gaps elsewhere
  • ➖ Broad blast radius; can introduce unexpected scroll/overflow interactions
  • ➖ Doesn’t directly address desktop inset well background mismatch semantics
3. Use `flex: 1`/minHeight only on specific content components
  • ➕ More targeted to known offenders (e.g., Settings/BackstageContent)
  • ➖ Misses other short pages using different structures (BUI Container vs main vs article)
  • ➖ Requires ongoing per-page fixes instead of a single page-shell rule

Recommendation: The PR’s approach (minHeight: 100vh + flex column on BackstageSidebarPage, plus desktop inset painting rules) is the best balance: it fixes the shell-level root cause for both Backstage (/) and NFS/BUI (Container) page structures while keeping changes localized to the theme’s page wrapper, not app-specific drawers/wrappers.

Files changed (3) +96 / -0

Bug fix (1) +25 / -0
createComponents.tsMake BackstageSidebarPage fill viewport and align inset well background +25/-0

Make BackstageSidebarPage fill viewport and align inset well background

• Updates BackstageSidebarPage styleOverrides to enforce minHeight: 100vh and a column flex layout so short pages don’t collapse. On desktop, applies mainSectionBackgroundColor and inset minHeight to main/progress, and extends painting/growth rules to NFS/BUI Containers and article-based BackstageContent to eliminate the bottom band/bleed-through.

workspaces/theme/plugins/theme/src/utils/createComponents.ts

Tests (1) +66 / -0
createComponents.test.tsAdd regression tests for BackstageSidebarPage viewport fill and inset painting +66/-0

Add regression tests for BackstageSidebarPage viewport fill and inset painting

• Adds Jest assertions verifying BackstageSidebarPage uses a flex column root with minHeight: 100vh. Also validates desktop selectors apply mainSectionBackgroundColor/minHeight to main, BUI Containers, and BackstageContent rendered as article.

workspaces/theme/plugins/theme/src/utils/createComponents.test.ts

Other (1) +5 / -0
fix-sidebar-page-viewport-gap.mdAdd changeset for NFS short-page viewport gap fix +5/-0

Add changeset for NFS short-page viewport gap fix

• Introduces a patch changeset for the theme plugin documenting the BackstageSidebarPage viewport fill and background-color alignment fix for short NFS pages.

workspaces/theme/.changeset/fix-sidebar-page-viewport-gap.md

@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.06%. Comparing base (b261d26) to head (1ce9fd0).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4151   +/-   ##
=======================================
  Coverage   58.06%   58.06%           
=======================================
  Files        2411     2411           
  Lines       96367    96367           
  Branches    26806    26806           
=======================================
+ Hits        55953    55954    +1     
+ Misses      40218    40217    -1     
  Partials      196      196           
Flag Coverage Δ *Carryforward flag
adoption-insights 84.55% <ø> (ø) Carriedforward from b261d26
ai-integrations 69.71% <ø> (ø) Carriedforward from b261d26
app-defaults 69.79% <ø> (ø) Carriedforward from b261d26
augment 46.67% <ø> (ø) Carriedforward from b261d26
boost 76.77% <ø> (ø) Carriedforward from b261d26
bulk-import 72.56% <ø> (ø) Carriedforward from b261d26
cost-management 13.55% <ø> (ø) Carriedforward from b261d26
dcm 60.72% <ø> (ø) Carriedforward from b261d26
extensions 56.59% <ø> (ø) Carriedforward from b261d26
global-floating-action-button 71.18% <ø> (ø) Carriedforward from b261d26
global-header 66.50% <ø> (ø) Carriedforward from b261d26
homepage 47.50% <ø> (ø) Carriedforward from b261d26
install-dynamic-plugins 59.95% <ø> (ø) Carriedforward from b261d26
intelligent-assistant 74.61% <ø> (ø) Carriedforward from b261d26
konflux 91.98% <ø> (ø) Carriedforward from b261d26
lightspeed 69.02% <ø> (ø) Carriedforward from b261d26
mcp-integrations 83.40% <ø> (ø) Carriedforward from b261d26
orchestrator 66.87% <ø> (ø) Carriedforward from b261d26
quickstart 63.74% <ø> (ø) Carriedforward from b261d26
sandbox 79.56% <ø> (ø) Carriedforward from b261d26
scorecard 85.45% <ø> (ø) Carriedforward from b261d26
theme 88.77% <ø> (+0.24%) ⬆️
translations 5.12% <ø> (ø) Carriedforward from b261d26
x2a 79.20% <ø> (ø) Carriedforward from b261d26

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b261d26...1ce9fd0. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rhdh-qodo-merge

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)

Context used
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 11 rules
✅ Cross-repo context
  Explored: repo: redhat-developer/rhdh (sha: 84072ae9)
  Not relevant to this PR: redhat-developer/rhdh-chart
  Not relevant to this PR: redhat-developer/rhdh-operator
  Not relevant to this PR: redhat-developer/rhdh-local

Grey Divider


Remediation recommended

1. SidebarPage height contract drift 🔗 Cross-repo conflict ≡ Correctness
Description
This PR unconditionally forces BackstageSidebarPage (and its desktop main) to a minHeight
derived from 100vh and uses a flex column root. In redhat-developer/rhdh, the custom
header-aware shell intentionally constrains the available height to calc(100vh - headerHeight), so
the new viewport-based minimum can exceed the allowed space and reintroduce overflow, nested
scrollbars, or clipping when headers are enabled.
Code

workspaces/theme/plugins/theme/src/utils/createComponents.ts[R761-764]

+          minHeight: '100vh',
+          // Let BUI Container's flex: 1 grow into the remaining viewport below
+          // PluginHeader / Header slots (those slots set flex: none).
+          display: 'flex',
Relevance

●● Moderate

They’ve adjusted SidebarPage height/scroll behavior to avoid regressions (e.g., TechDocs scrollbar
fix in PR #3820).

PR-#3820
PR-#2840

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The cited change in createComponents adds an unconditional minHeight: '100vh' to
BackstageSidebarPage and also applies a viewport-derived minHeight to the desktop main. RHDH’s
Root.tsx/RHDHPageWithoutFixHeight documents and implements a layout where SidebarPage is a
direct child of a wrapper that sets maxHeight: calc(100vh - aboveSidebarHeaderHeight) (and only
unsets height on BackstageSidebarPage, not minHeight), meaning a min-height: 100vh on the
SidebarPage root can exceed the constrained region whenever aboveSidebarHeaderHeight is
non-zero, causing overflow and extra/nested scrolling.

workspaces/theme/plugins/theme/src/utils/createComponents.ts[754-803]
workspaces/global-header/packages/app-legacy/src/components/Root/Root.tsx[41-59]
workspaces/global-header/packages/app-legacy/src/components/Root/Root.tsx[61-105]
workspaces/global-header/packages/app-legacy/package.json[52-60]
External repo: redhat-developer/rhdh, packages/app/src/components/Root/Root.tsx [57-93]
External repo: redhat-developer/rhdh, packages/app/src/components/Root/Root.tsx [95-167]
External repo: redhat-developer/rhdh, packages/app/src/components/Root/Root.tsx [541-637]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`@red-hat-developer-hub/backstage-plugin-theme` now applies viewport-based `minHeight` rules (notably `minHeight: '100vh'` on `BackstageSidebarPage`, plus a `main` `minHeight` derived from `100vh`) unconditionally. In the `redhat-developer/rhdh` application shell, the sidebar/page area is intentionally constrained to `calc(100vh - application/global header heights)`, so enforcing a `100vh` minimum can exceed the available height and cause overflow/nested scrollbars or clipping when headers are enabled.

## Issue Context
RHDH’s layout expects `SidebarPage` to flex within a header-aware wrapper (e.g., `RHDHPageWithoutFixHeight` / `Root.tsx` layout) that reduces available height to `calc(100vh - aboveSidebarHeaderHeight)`; `SidebarPage` renders a wrapper `<div>` and the shell applies the max-height constraint to that wrapper. The current change only unsets `height` in some cases, but the new `minHeight` remains in effect, so it can still conflict with the reduced available height.

## Fix Focus Areas
- workspaces/theme/plugins/theme/src/utils/createComponents.ts[726-823]

## Suggested approach
- Add an RHDH-specific escape hatch in the theme overrides so that when `BackstageSidebarPage` is rendered inside the header-aware RHDH shell (e.g., within `components.RHDHPageWithoutFixHeight.styleOverrides.sidebarLayout`/`root`, or via a stable container like `#rhdh-sidebar-layout` / presence of the above-sidebar-header container), the theme does **not** enforce viewport-based minimums.
- Concretely, unset or reduce the new minimums for the `BackstageSidebarPage` subtree in that context (e.g., set `minHeight: 'unset'`/`auto` or `minHeight: 0` on the `BackstageSidebarPage` root and also on its direct `main` and progress children) so the header-aware `maxHeight: calc(100vh - aboveSidebarHeaderHeight)` rules remain authoritative.
- Keep the NFS/regular-shell fix intact for non-header-aware layouts, while preserving correct sizing where the available height is intentionally less than `100vh`.
- Alternatively (with coordination), update `redhat-developer/rhdh` to explicitly override/unset `minHeight` on the `SidebarPage` root (`& > div`) when headers are present.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Brittle theme token tests 🐞 Bug ⚙ Maintainability
Description
The new createComponents tests hard-code #292929 and 1.5rem expectations instead of deriving
them from customDarkTheme(), so legitimate future token changes will cause unrelated test
failures.
Code

workspaces/theme/plugins/theme/src/utils/createComponents.test.ts[R112-116]

+      expect.objectContaining({
+        backgroundColor: '#292929',
+        minHeight: 'calc(100vh - 2 * 1.5rem)',
+        maxHeight: 'calc(100vh - 2 * 1.5rem)',
+      }),
Relevance

●●● Strong

Repo tests typically assert via theme config, not hard-coded hex/rem (see token-based assertions in
PR #2840 tests).

PR-#2840

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The tests compare against hard-coded values even though customDarkTheme() defines these tokens;
changing mainSectionBackgroundColor or pageInset would correctly change the generated styles but
still break the tests.

workspaces/theme/plugins/theme/src/utils/createComponents.test.ts[90-153]
workspaces/theme/plugins/theme/src/darkTheme.ts[49-68]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Tests assert literal values (`#292929`, `1.5rem`, and the resulting `calc(...)` strings) even though they already construct the theme via `customDarkTheme()`. This makes the tests fail on intentional theme token updates.

### Issue Context
The behavior under test is that `createComponents` wires through `rhdh.general.mainSectionBackgroundColor` and `rhdh.general.pageInset` into style overrides.

### Fix Focus Areas
- workspaces/theme/plugins/theme/src/utils/createComponents.test.ts[101-152]

### Proposed fix
In tests, compute expected values from `const palette = customDarkTheme();` and then assert:
- `backgroundColor: palette.rhdh?.general.mainSectionBackgroundColor`
- `minHeight/maxHeight: \`calc(100vh - 2 * ${palette.rhdh?.general.pageInset})\``

This keeps coverage while avoiding duplicated knowledge of token literals.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@rohitratannagar rohitratannagar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have verified the changes on the cluster
/lgtm

Without PR:

Image

With PR:

Screen.Recording.2026-08-04.at.7.01.57.PM.mov

@openshift-ci openshift-ci Bot added the lgtm label Aug 4, 2026
@ciiay
ciiay merged commit 0f401be into redhat-developer:main Aug 4, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants