fix(PDiskSpaceDistribution): slot height calculation#3897
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts PDisk space distribution rendering so slot rows use explicit heights based on slot size, with log slots rendered smaller than regular slots.
Changes:
- Replaces the fixed container-height/flex-grow layout with per-slot height calculation.
- Computes the smallest non-log slot total as the scaling baseline.
- Updates PDisk space distribution styles to support auto-height layout and full-height slot bars.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/containers/PDiskPage/PDiskSpaceDistribution/PDiskSpaceDistribution.tsx |
Adds slot height scaling logic and passes the computed baseline into each slot. |
src/containers/PDiskPage/PDiskSpaceDistribution/PDiskSpaceDistribution.scss |
Adjusts layout styles for the new explicit slot-height rendering. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Log slots get half the base height; others scale proportionally to the smallest non-log slot | ||
| const slotHeight = | ||
| item.SlotType === 'log' | ||
| ? BASE_SLOT_HEIGHT / 2 | ||
| : ((Number(item.Total) || 1) / minNonLogTotal) * BASE_SLOT_HEIGHT; |
There was a problem hiding this comment.
item.Total is undefined, null, or 0 for any non-log slot, Number(item.Total) || 1 normalises it to 1. That 1 then wins the minNonLogTotal search, so every other slot with a real capacity — say 100 GB = 107_374_182_400 — computes a height of (107_374_182_400 / 1) × 40 ≈ 4.3 × 10¹²px, effectively freezing the page. Adding a Math.min cap (e.g. 10 × BASE_SLOT_HEIGHT) keeps the layout sensible even when data is incomplete.
| // Log slots get half the base height; others scale proportionally to the smallest non-log slot | |
| const slotHeight = | |
| item.SlotType === 'log' | |
| ? BASE_SLOT_HEIGHT / 2 | |
| : ((Number(item.Total) || 1) / minNonLogTotal) * BASE_SLOT_HEIGHT; | |
| // Log slots get half the base height; others scale proportionally to the smallest non-log slot | |
| const slotHeight = | |
| item.SlotType === 'log' | |
| ? BASE_SLOT_HEIGHT / 2 | |
| : Math.min( | |
| ((Number(item.Total) || 1) / minNonLogTotal) * BASE_SLOT_HEIGHT, | |
| 10 * BASE_SLOT_HEIGHT, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/containers/PDiskPage/PDiskSpaceDistribution/PDiskSpaceDistribution.tsx
Line: 200-204
Comment:
When `item.Total` is `undefined`, `null`, or `0` for any non-log slot, `Number(item.Total) || 1` normalises it to `1`. That `1` then wins the `minNonLogTotal` search, so every other slot with a real capacity — say 100 GB = `107_374_182_400` — computes a height of `(107_374_182_400 / 1) × 40 ≈ 4.3 × 10¹²px`, effectively freezing the page. Adding a `Math.min` cap (e.g. `10 × BASE_SLOT_HEIGHT`) keeps the layout sensible even when data is incomplete.
```suggestion
// Log slots get half the base height; others scale proportionally to the smallest non-log slot
const slotHeight =
item.SlotType === 'log'
? BASE_SLOT_HEIGHT / 2
: Math.min(
((Number(item.Total) || 1) / minNonLogTotal) * BASE_SLOT_HEIGHT,
10 * BASE_SLOT_HEIGHT,
);
```
How can I resolve this? If you propose a fix, please make it concise.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c578b476f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (item.SlotType === 'log') { | ||
| continue; | ||
| } | ||
| const value = Number(item.Total) || 1; |
There was a problem hiding this comment.
When any non-log slot has a missing, zero, or NaN Total, this fallback makes minNonLogTotal equal to 1. The new height calculation then divides real byte-sized slots by that value, so a normal 20 GB empty slot renders as about 800,000,000,000 px tall. This can happen with partial/older VDisk data where SizeLimit is unavailable while ExpectedSlotCount still creates empty slots; the previous flex layout kept the total bar height bounded, but these fixed pixel heights can make the PDisk page unusable.
Useful? React with 👍 / 👎.
#3821
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary 🗑️13
🗑️ Deleted Tests (13)
Bundle Size: ✅
Current: 63.76 MB | Main: 63.76 MB
Diff: +1.27 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information
Greptile Summary
This PR replaces the previous fixed-container-height +
flexGrowapproach for rendering PDisk slot bars with explicit per-slot pixel heights calculated proportionally relative to the smallest non-log slot capacity. Log slots are assigned a fixed half-height (20 px) while data and empty slots scale by(Total / minNonLogTotal) × 40 px.minNonLogTotalis memoised across slot items; the outer container's hard-codedheight/minHeightinline styles are removed in favour ofheight: autoon the bar class, letting the container grow naturally.flex-grow: 1is removed from__pdisk-bar,height: 100%is added to__slotso eachDiskStateProgressBarfills its explicitly-sized wrapper, and the redundant.storage-disk-progress-bar { height: 100% }override is dropped.Confidence Score: 4/5
Safe to merge for normal YDB deployments; one edge case with incomplete slot data could produce extreme heights but is unlikely in production.
The change is straightforward and well-scoped. For the typical case where all non-log slots have valid Total values the new proportional heights work correctly. The only concern is that a slot with a missing or zero Total forces minNonLogTotal down to 1, which would make other slots compute astronomically large pixel heights — a cap on slotHeight would close that gap without changing normal behaviour.
The height formula in PDiskSpaceDistribution.tsx around lines 200–204 is worth a second look for the missing-Total edge case.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[SlotItems] --> B{Any non-log slots?} B -- No --> C[minNonLogTotal = 1] B -- Yes --> D[Find min Total among non-log slots] D --> E{All Totals undefined/zero?} E -- Yes --> C E -- No --> F[minNonLogTotal = smallest real Total] F --> G[For each Slot] C --> G G --> H{SlotType === 'log'?} H -- Yes --> I[height = BASE_SLOT_HEIGHT / 2 = 20px] H -- No --> J["height = (item.Total / minNonLogTotal) × 40px"] J --> K{item.Total undefined/zero?} K -- Yes --> L["effective Total = 1 → height = 1/minNonLogTotal × 40"] K -- No --> M[height proportional to capacity ratio] I --> N[Render slot-wrapper with explicit height] L --> N M --> NPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(PDiskSpaceDistribution): slot height..." | Re-trigger Greptile