feat: Implement Dynamic Flexible Grid Layout Engine (#3345) - #3361
feat: Implement Dynamic Flexible Grid Layout Engine (#3345)#3361knoxiboy wants to merge 1 commit into
Conversation
github-actions
Bot
added
area:core
📝 WalkthroughWalkthroughAdded ChangesGrid layout engine
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🧹 Nitpick comments (1)
packages/core/test/gridLayout.test.ts (1)
1-2: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the public export in this test.
Line 2 imports the implementation module directly. A broken
src/index.tsexport could still pass this suite. Import from../src, or add a separate smoke test through the public entry point.The PR objective defines
src/index.tsas the public export.Suggested import change
-import { GridLayoutManager } from '../src/layout/gridLayout'; +import { GridLayoutManager } from '../src';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/gridLayout.test.ts` around lines 1 - 2, Update the GridLayoutManager import in gridLayout.test.ts to use the public ../src entry point instead of the implementation module, so the test exercises the src/index.ts export while preserving the existing test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/layout/gridLayout.ts`:
- Around line 19-33: Update parseTrackSpec to use complete-token numeric
parsing, preserve explicit 0fr, and default only the bare fr token to value 1;
malformed or invalid factors must not be accepted from numeric prefixes. In the
track allocation logic that accumulates totalFr, validate each public TrackSpec
value and ignore or safely handle negative and non-finite fr values before
adding them to totalFr.
- Around line 61-68: Update the second-pass fractional allocation in the grid
layout calculation so integer rounding distributes all leftover units among the
`fr` tracks, ensuring their sizes sum to `remainingSize` (for example, three 1fr
tracks over 10 units must total 10). Add a test covering fractional tracks and
assert that the computed sizes sum to the available remaining space.
In `@packages/core/test/gridLayout.test.ts`:
- Around line 22-26: Update computeTrackSizes and its related fr-allocation
logic to handle non-divisible totals without leaving uncovered cells; for a
total of 100 with three 1fr tracks, ensure the implementation follows an
explicit remainder policy and the resulting sizes cover all 100 cells. Add a
regression test alongside the existing GridLayoutManager track-size tests for
this case.
---
Nitpick comments:
In `@packages/core/test/gridLayout.test.ts`:
- Around line 1-2: Update the GridLayoutManager import in gridLayout.test.ts to
use the public ../src entry point instead of the implementation module, so the
test exercises the src/index.ts export while preserving the existing test
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dabcaec0-9c52-4c26-911e-2ecc8aabba3b
📒 Files selected for processing (2)
packages/core/src/layout/gridLayout.tspackages/core/test/gridLayout.test.ts
| static parseTrackSpec(specStr: string): TrackSpec[] { | ||
| const tokens = specStr.trim().split(/\s+/).filter(Boolean); | ||
| return tokens.map((token) => { | ||
| if (token === 'auto') return { value: 0, unit: 'auto' }; | ||
| if (token.endsWith('px')) { | ||
| return { value: parseFloat(token.slice(0, -2)) || 0, unit: 'px' }; | ||
| } | ||
| if (token.endsWith('%')) { | ||
| return { value: parseFloat(token.slice(0, -1)) || 0, unit: 'percent' }; | ||
| } | ||
| if (token.endsWith('fr')) { | ||
| return { value: parseFloat(token.slice(0, -2)) || 1, unit: 'fr' }; | ||
| } | ||
| const num = parseFloat(token); | ||
| return isNaN(num) ? { value: 0, unit: 'auto' } : { value: num, unit: 'px' }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve zero fr values and reject invalid factors.
parseTrackSpec('0fr') currently returns { value: 1, unit: 'fr' } because 0 || 1 is truthy fallback logic. This changes an explicit zero-flex track into a flexible track.
parseFloat also accepts numeric prefixes for malformed tokens. Since TrackSpec is public, callers can bypass the parser and pass negative or non-finite fr values. The allocation formula can then produce negative or NaN track sizes.
Use complete-token numeric parsing. Treat only the bare fr token as the default value 1. Validate TrackSpec.value before adding it to totalFr.
Proposed fix
- if (token.endsWith('fr')) {
- return { value: parseFloat(token.slice(0, -2)) || 1, unit: 'fr' };
- }
+ if (token === 'fr') return { value: 1, unit: 'fr' };
+ if (token.endsWith('fr')) {
+ const value = Number(token.slice(0, -2));
+ if (!Number.isFinite(value) || value < 0) {
+ return { value: 0, unit: 'auto' };
+ }
+ return { value, unit: 'fr' };
+ }
...
} else if (track.unit === 'fr') {
+ if (!Number.isFinite(track.value) || track.value < 0) {
+ throw new RangeError('fr track values must be finite and non-negative');
+ }
totalFr += track.value;Also applies to: 56-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/layout/gridLayout.ts` around lines 19 - 33, Update
parseTrackSpec to use complete-token numeric parsing, preserve explicit 0fr, and
default only the bare fr token to value 1; malformed or invalid factors must not
be accepted from numeric prefixes. In the track allocation logic that
accumulates totalFr, validate each public TrackSpec value and ignore or safely
handle negative and non-finite fr values before adding them to totalFr.
| // Second pass: allocate fr units from remaining size | ||
| if (totalFr > 0 && remainingSize > 0) { | ||
| tracks.forEach((track, i) => { | ||
| if (track.unit === 'fr') { | ||
| sizes[i] = Math.floor((remainingSize * track.value) / totalFr); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distribute the fractional rounding remainder.
Independent Math.floor calls can leave part of the available space unused. For example, three 1fr tracks in a size of 10 return [3, 3, 3]. The cell rectangles then cover only 9 cells.
Allocate the remainder to one fractional track or use a largest-remainder distribution. Add a test that verifies the fractional sizes sum to the remaining space.
Proposed fix
+ const frIndexes = tracks.flatMap((track, index) =>
+ track.unit === 'fr' && track.value > 0 ? [index] : [],
+ );
+ let allocatedFrSize = 0;
tracks.forEach((track, i) => {
if (track.unit === 'fr') {
- sizes[i] = Math.floor((remainingSize * track.value) / totalFr);
+ sizes[i] =
+ i === frIndexes[frIndexes.length - 1]
+ ? remainingSize - allocatedFrSize
+ : Math.floor((remainingSize * track.value) / totalFr);
+ allocatedFrSize += sizes[i];
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Second pass: allocate fr units from remaining size | |
| if (totalFr > 0 && remainingSize > 0) { | |
| tracks.forEach((track, i) => { | |
| if (track.unit === 'fr') { | |
| sizes[i] = Math.floor((remainingSize * track.value) / totalFr); | |
| } | |
| }); | |
| } | |
| // Second pass: allocate fr units from remaining size | |
| if (totalFr > 0 && remainingSize > 0) { | |
| const frIndexes = tracks.flatMap((track, index) => | |
| track.unit === 'fr' && track.value > 0 ? [index] : [], | |
| ); | |
| let allocatedFrSize = 0; | |
| tracks.forEach((track, i) => { | |
| if (track.unit === 'fr') { | |
| sizes[i] = | |
| i === frIndexes[frIndexes.length - 1] | |
| ? remainingSize - allocatedFrSize | |
| : Math.floor((remainingSize * track.value) / totalFr); | |
| allocatedFrSize += sizes[i]; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/layout/gridLayout.ts` around lines 61 - 68, Update the
second-pass fractional allocation in the grid layout calculation so integer
rounding distributes all leftover units among the `fr` tracks, ensuring their
sizes sum to `remainingSize` (for example, three 1fr tracks over 10 units must
total 10). Add a test covering fractional tracks and assert that the computed
sizes sum to the available remaining space.
| it('should compute equal track sizes for multiple fr tracks', () => { | ||
| const tracks = GridLayoutManager.parseTrackSpec('1fr 2fr'); | ||
| const sizes = GridLayoutManager.computeTrackSizes(90, tracks); | ||
| expect(sizes).toEqual([30, 60]); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover non-divisible fr allocations.
Lines 22-26 use a divisible total, so the test does not detect rounding gaps. For a total size of 100 with 1fr 1fr 1fr, computeTrackSizes returns [33, 33, 33], and the tracks cover only 99 cells.
Add a regression case and distribute the remainder, or define an explicit remainder policy, before relying on this test for arbitrary terminal sizes.
Suggested regression test
+ it('should fill the container when fr tracks require rounding', () => {
+ const tracks = GridLayoutManager.parseTrackSpec('1fr 1fr 1fr');
+ const sizes = GridLayoutManager.computeTrackSizes(100, tracks);
+
+ expect(sizes.reduce((total, size) => total + size, 0)).toBe(100);
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/gridLayout.test.ts` around lines 22 - 26, Update
computeTrackSizes and its related fr-allocation logic to handle non-divisible
totals without leaving uncovered cells; for a total of 100 with three 1fr
tracks, ensure the implementation follows an explicit remainder policy and the
resulting sizes cover all 100 cells. Add a regression test alongside the
existing GridLayoutManager track-size tests for this case.
Description
Implements a dynamic flexible grid layout calculation engine supporting fractional (
fr), percentage, and fixed unit tracks to dynamically calculate terminal UI widget bounding boxes.Related Issue
Closes #3345
Which package(s)?
@termuijs/core
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/knoxiboyScreenshots / Recordings (UI changes)
Calculates dynamic grid cell dimensions across fixed and fractional tracks.
Notes for the Reviewer
Zero external dependencies; integrates cleanly into the core layout pipeline.
Summary by CodeRabbit
New Features
Tests