Layout switcher#15
Conversation
📝 WalkthroughWalkthroughAdded timeline density presets with persistent state, conditional video thumbnails, resizable upper/timeline panels, layout reset controls, pane-height persistence, automatic sizing, and corresponding HTML/CSS updates. ChangesTimeline layout
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LayoutTools
participant initPanelSplit
participant timelinePanel
participant localStorage
User->>LayoutTools: drag split handle or choose Layout reset
LayoutTools->>initPanelSplit: update or restore panel height
initPanelSplit->>timelinePanel: clamp and apply height
initPanelSplit->>localStorage: persist layout height
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 4
🧹 Nitpick comments (3)
style.css (2)
106-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeprecated
clipproperty in.sr-only.Stylelint flags
clip: rect(0, 0, 0, 0);as deprecated; the modern visually-hidden pattern usesclip-path: inset(50%);instead.🎨 Proposed fix
overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); white-space: nowrap;🤖 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 `@style.css` around lines 106 - 117, Update the `.sr-only` visually hidden style by removing the deprecated `clip` declaration and using the modern `clip-path: inset(50%)` declaration instead, while preserving the existing accessibility and layout-hiding behavior.Source: Linters/SAST tools
566-566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnquoted font name for consistency with Stylelint and the rest of the file.
Stylelint's
font-family-name-quotesflags the quoted"Consolas"here; lines 638 and 1058 already use it unquoted.🎨 Proposed fix
- font: 600 14px/1 "Consolas", monospace; + font: 600 14px/1 Consolas, monospace;🤖 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 `@style.css` at line 566, Update the font shorthand declaration to use the unquoted Consolas font name, matching the existing style at nearby declarations and satisfying Stylelint’s font-family-name-quotes rule; preserve the monospace fallback and other font properties.Source: Linters/SAST tools
app.js (1)
2839-2839: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider debouncing the resize handler.
Every native
resizeevent forces a synchronousgetBoundingClientRect()read plus a style mutation viaclampTimelineHeight()/setTimelineHeight(). Harmless at current scale, but worth debouncing if this becomes a hot path.🤖 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 `@app.js` at line 2839, Debounce the resize listener around clampTimelineHeight so rapid native resize events do not repeatedly trigger synchronous layout reads and style updates. Preserve setting state.dirtyTimeline on resize and ensure the debounced handler still invokes clampTimelineHeight after resizing settles.
🤖 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 `@app.js`:
- Around line 2948-2951: Update the resize handle’s "dblclick" event handler to
call restoreDefaultLayout() instead of resetTimelineHeight(), so double-clicking
restores both the default track size and pane height consistently with the "⊡
Layout" button.
- Around line 2845-2852: Update availableTimelineMax() to subtract the .app
container’s 12px total vertical padding before calculating the timeline maximum,
while preserving the existing topbarH, UPPER_MIN, split, and gaps deductions.
In `@index.html`:
- Around line 103-104: Make the upper timeline separator keyboard-operable and
accessible: in index.html (103-104), update `#splitUpperTimeline` with tabindex
and appropriate separator value attributes or an accessible label; in app.js
(2927-2947), add keyboard handling on handle for vertical arrow resizing through
setTimelineHeight() and persist the result using TL_H_KEY; in style.css (60-83),
add a visible :focus-visible style for .v-split.
In `@style.css`:
- Around line 320-325: Merge the later grid-layout declarations into the
existing `.upper` rule instead of defining a duplicate selector, preserving the
earlier `min-height: 140px` floor while retaining the grid columns, gap, and
other layout properties.
---
Nitpick comments:
In `@app.js`:
- Line 2839: Debounce the resize listener around clampTimelineHeight so rapid
native resize events do not repeatedly trigger synchronous layout reads and
style updates. Preserve setting state.dirtyTimeline on resize and ensure the
debounced handler still invokes clampTimelineHeight after resizing settles.
In `@style.css`:
- Around line 106-117: Update the `.sr-only` visually hidden style by removing
the deprecated `clip` declaration and using the modern `clip-path: inset(50%)`
declaration instead, while preserving the existing accessibility and
layout-hiding behavior.
- Line 566: Update the font shorthand declaration to use the unquoted Consolas
font name, matching the existing style at nearby declarations and satisfying
Stylelint’s font-family-name-quotes rule; preserve the monospace fallback and
other font properties.
🪄 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
Run ID: d2801d9c-49b0-404c-8127-1140322138ed
📒 Files selected for processing (3)
app.jsindex.htmlstyle.css
| function availableTimelineMax() { | ||
| const app = $("app").getBoundingClientRect(); | ||
| const topbar = document.querySelector(".topbar")?.getBoundingClientRect(); | ||
| const topbarH = topbar ? topbar.height : 46; | ||
| const split = 6; | ||
| const gaps = 18; // three 6px flex gaps between four children | ||
| return Math.floor(app.height - topbarH - UPPER_MIN - split - gaps); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
availableTimelineMax() omits .app's own padding, over-allocating space for the timeline pane.
style.css sets .app { padding: 6px; }, so the flex children's content box is app.height - 12px, not app.height. This function only subtracts topbarH, UPPER_MIN, split, and gaps, so the computed max is 12px too large. Combined with the .upper min-height override flagged in style.css (no CSS floor left to catch the overshoot), dragging/fitting the timeline to max will silently squeeze .upper below its intended 140px minimum instead of being clamped.
🐛 Proposed fix
function availableTimelineMax() {
const app = $("app").getBoundingClientRect();
+ const appStyle = getComputedStyle($("app"));
+ const appPad = parseFloat(appStyle.paddingTop) + parseFloat(appStyle.paddingBottom);
const topbar = document.querySelector(".topbar")?.getBoundingClientRect();
const topbarH = topbar ? topbar.height : 46;
const split = 6;
const gaps = 18; // three 6px flex gaps between four children
- return Math.floor(app.height - topbarH - UPPER_MIN - split - gaps);
+ return Math.floor(app.height - appPad - topbarH - UPPER_MIN - split - gaps);
}📝 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.
| function availableTimelineMax() { | |
| const app = $("app").getBoundingClientRect(); | |
| const topbar = document.querySelector(".topbar")?.getBoundingClientRect(); | |
| const topbarH = topbar ? topbar.height : 46; | |
| const split = 6; | |
| const gaps = 18; // three 6px flex gaps between four children | |
| return Math.floor(app.height - topbarH - UPPER_MIN - split - gaps); | |
| } | |
| function availableTimelineMax() { | |
| const app = $("app").getBoundingClientRect(); | |
| const appStyle = getComputedStyle($("app")); | |
| const appPad = parseFloat(appStyle.paddingTop) + parseFloat(appStyle.paddingBottom); | |
| const topbar = document.querySelector(".topbar")?.getBoundingClientRect(); | |
| const topbarH = topbar ? topbar.height : 46; | |
| const split = 6; | |
| const gaps = 18; // three 6px flex gaps between four children | |
| return Math.floor(app.height - appPad - topbarH - UPPER_MIN - split - gaps); | |
| } |
🤖 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 `@app.js` around lines 2845 - 2852, Update availableTimelineMax() to subtract
the .app container’s 12px total vertical padding before calculating the timeline
maximum, while preserving the existing topbarH, UPPER_MIN, split, and gaps
deductions.
| handle.addEventListener("dblclick", (e) => { | ||
| e.preventDefault(); | ||
| resetTimelineHeight(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Double-click only resets the pane height, not the "default layout".
The PR's commit message describes double-click as "Restore the default layout when the resize handle is double-clicked," and restoreDefaultLayout() (used by the "⊡ Layout" button) already implements that semantic — resetting both track size and pane height. This handler calls resetTimelineHeight() alone, so if the user is on the "S"/"M" preset, double-clicking the splitter won't restore the "L" default despite the stated intent.
🐛 Proposed fix
handle.addEventListener("dblclick", (e) => {
e.preventDefault();
- resetTimelineHeight();
+ restoreDefaultLayout();
});📝 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.
| handle.addEventListener("dblclick", (e) => { | |
| e.preventDefault(); | |
| resetTimelineHeight(); | |
| }); | |
| handle.addEventListener("dblclick", (e) => { | |
| e.preventDefault(); | |
| restoreDefaultLayout(); | |
| }); |
🤖 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 `@app.js` around lines 2948 - 2951, Update the resize handle’s "dblclick" event
handler to call restoreDefaultLayout() instead of resetTimelineHeight(), so
double-clicking restores both the default track size and pane height
consistently with the "⊡ Layout" button.
| <div class="v-split" id="splitUpperTimeline" role="separator" aria-orientation="horizontal" title="Drag to resize · double-click to reset"></div> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Timeline/upper split handle is not keyboard-operable. role="separator" on #splitUpperTimeline implies it should support focus and keyboard resizing per ARIA authoring practices, but the markup, JS, and CSS only implement pointer/mouse interaction.
index.html#L103-L104: addtabindex="0",aria-valuemin/aria-valuemax/aria-valuenow(oraria-label) to#splitUpperTimelineso it's focusable and its current size is exposed to assistive tech.app.js#L2927-L2947: add akeydownlistener onhandle(e.g.ArrowUp/ArrowDowncallingsetTimelineHeight()and persisting toTL_H_KEY, mirroring the existing pointer-drag logic).style.css#L60-L83: add a:focus-visiblestyle for.v-splitso keyboard focus is visible once it becomes focusable.
📍 Affects 3 files
index.html#L103-L104(this comment)app.js#L2927-L2947style.css#L60-L83
🤖 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 `@index.html` around lines 103 - 104, Make the upper timeline separator
keyboard-operable and accessible: in index.html (103-104), update
`#splitUpperTimeline` with tabindex and appropriate separator value attributes or
an accessible label; in app.js (2927-2947), add keyboard handling on handle for
vertical arrow resizing through setTimelineHeight() and persist the result using
TL_H_KEY; in style.css (60-83), add a visible :focus-visible style for .v-split.
| .upper { | ||
| display: grid; | ||
| grid-template-columns: 280px minmax(0, 1fr) 280px; | ||
| gap: 6px; | ||
| min-height: 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Duplicate .upper selector overrides the flex-layout min-height: 140px with min-height: 0.
This grid-layout .upper rule and the earlier flex-item .upper rule (lines 48-52) both set min-height on the same selector/specificity; since this block appears later in the file, min-height: 0 wins, silently discarding the 140px floor that's meant to match app.js's UPPER_MIN constant. That removes the CSS safety net that would otherwise stop .upper from shrinking below 140px if the timeline pane is over-allocated (see the related availableTimelineMax() padding bug in app.js).
🐛 Proposed fix (merge into a single rule)
-.upper {
- flex: 1 1 0;
- min-height: 140px;
- min-width: 0;
-}
-...
.upper {
display: grid;
grid-template-columns: 280px minmax(0, 1fr) 280px;
gap: 6px;
- min-height: 0;
+ flex: 1 1 0;
+ min-height: 140px;
+ min-width: 0;
}📝 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.
| .upper { | |
| display: grid; | |
| grid-template-columns: 280px minmax(0, 1fr) 280px; | |
| gap: 6px; | |
| min-height: 0; | |
| } | |
| .upper { | |
| display: grid; | |
| grid-template-columns: 280px minmax(0, 1fr) 280px; | |
| gap: 6px; | |
| flex: 1 1 0; | |
| min-height: 140px; | |
| min-width: 0; | |
| } |
🤖 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 `@style.css` around lines 320 - 325, Merge the later grid-layout declarations
into the existing `.upper` rule instead of defining a duplicate selector,
preserving the earlier `min-height: 140px` floor while retaining the grid
columns, gap, and other layout properties.
2 participants
What does this PR do?
Top toolbar has a track height switcher that allows to switch track height between three predefined values:
Choosing the track height resizes the monitor/timeline panels to show all the tracks without overflowing
Closes #
Type of change
How was it verified?
node --check server.js && node --check app.js && node --check mcp-server.jspassesCLAUDE.md/README.mdif the schema, props, or API changedChecklist
Summary by CodeRabbit
New Features
UI Improvements