From 42d1fbc72896699746700c8e4867f21921b717cf Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Mon, 22 Jun 2026 06:38:06 +0000 Subject: [PATCH] Phase 1 (#4662): address remaining Greptile review (P2 + stagger) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on Nathan's switch-failure workspace-skeleton-clear fix (which already covered Greptile P1). Remaining two: - [P2] Removed dead _clearSessionListSkeletonFlag() — the flag is reset inline in renderSessionListFromCache(); the helper had no call site. - [stagger] The .skeleton-row:nth-child(N) fade-in delays skipped most rows because group-label siblings are interleaved with rows in the DOM (positions shift: rows land at child 2, 4-6, 8-11, so 9/10/11 got no delay). Replaced the CSS :nth-child rules with an inline per-row animationDelay set in JS (rowIndex * 0.025s, capped 0.2s) so every row gets its correct stagger regardless of group structure. Tests: +1 behavioural (every row carries a real inline stagger delay; last row not skipped). 26 pass. --- static/sessions.js | 10 ++++---- static/style.css | 12 ++++------ ...e4662_profile_switch_skeleton_behaviour.py | 23 +++++++++++++++++++ 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/static/sessions.js b/static/sessions.js index 54341cd6..8ec53946 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -3518,6 +3518,7 @@ function showSessionListSkeleton(){ const wrap = document.createElement('div'); wrap.className = 'skeleton-list'; wrap.setAttribute('aria-hidden', 'true'); + let rowIndex = 0; for(const group of _SESSION_SKELETON_GROUPS){ const label = document.createElement('div'); label.className = 'skeleton-group-label'; @@ -3525,6 +3526,11 @@ function showSessionListSkeleton(){ for(const spec of group.rows){ const row = document.createElement('div'); row.className = 'skeleton-row'; + // Stagger the fade-in per row. Set inline (not via CSS :nth-child) because + // group-label siblings are interleaved with rows, so a :nth-child stagger + // would skip most rows. Cap so the longest list doesn't feel laggy. + row.style.animationDelay = Math.min(rowIndex * 0.025, 0.2) + 's'; + rowIndex++; const title = document.createElement('div'); title.className = 'skeleton-bar skeleton-title'; title.style.width = spec.title + '%'; @@ -3554,10 +3560,6 @@ function showSessionListSkeleton(){ _sessionListSkeletonActive = true; } -function _clearSessionListSkeletonFlag(){ - _sessionListSkeletonActive = false; -} - function _isOptimisticFirstTurnSessionRow(s){ if(!s||!s.session_id||s.archived) return false; const messageCount=Number(s.message_count||0); diff --git a/static/style.css b/static/style.css index 3c57dece..66c4fced 100644 --- a/static/style.css +++ b/static/style.css @@ -1236,15 +1236,11 @@ animation:skeletonSheen 1.25s ease-in-out infinite;background-size:200% 100%;} .skeleton-tree-row .skeleton-name{min-width:0;} .skeleton-tree-row .skeleton-size{height:8px;width:22px;flex:0 0 auto;margin-left:auto;opacity:.5;} - /* Gentle entrance so the skeleton itself doesn't pop in harshly */ + /* Gentle entrance so the skeleton itself doesn't pop in harshly. The + per-row stagger delay is set inline in JS (showSessionListSkeleton), + because group-label siblings are interleaved with rows so a CSS + :nth-child stagger would skip most rows. */ .skeleton-row,.skeleton-tree-row,.skeleton-group-label{animation:skeletonFadeIn .22s ease both;} - .skeleton-row:nth-child(2){animation-delay:.025s;} - .skeleton-row:nth-child(3){animation-delay:.05s;} - .skeleton-row:nth-child(4){animation-delay:.075s;} - .skeleton-row:nth-child(5){animation-delay:.1s;} - .skeleton-row:nth-child(6){animation-delay:.125s;} - .skeleton-row:nth-child(7){animation-delay:.15s;} - .skeleton-row:nth-child(8){animation-delay:.175s;} @keyframes skeletonSheen{0%{background-position:200% 0;}100%{background-position:-200% 0;}} @keyframes skeletonFadeIn{from{opacity:0;}to{opacity:1;}} :root{--skeleton-base:color-mix(in srgb,var(--text) 8%,transparent);--skeleton-sheen:color-mix(in srgb,var(--text) 22%,transparent);} diff --git a/tests/test_issue4662_profile_switch_skeleton_behaviour.py b/tests/test_issue4662_profile_switch_skeleton_behaviour.py index a79a4b2f..b8294d4e 100644 --- a/tests/test_issue4662_profile_switch_skeleton_behaviour.py +++ b/tests/test_issue4662_profile_switch_skeleton_behaviour.py @@ -137,6 +137,16 @@ const result = { rows: list.querySelectorAll('.skeleton-row').length, titles: list.querySelectorAll('.skeleton-title').length, stamps: list.querySelectorAll('.skeleton-stamp').length, + // Every row should carry an inline stagger delay (set in JS, not via a + // CSS :nth-child that would skip rows because group labels are interleaved). + rows_with_delay: list.querySelectorAll('.skeleton-row').filter(function(r){ + return r.style && typeof r.style.animationDelay === 'string' && r.style.animationDelay.length > 0; + }).length, + last_row_delay: (function(){ + var rs = list.querySelectorAll('.skeleton-row'); + var last = rs[rs.length - 1]; + return last && last.style ? last.style.animationDelay : null; + })(), skeleton_active_flag: _sessionListSkeletonActive, list_scrolltop: list.scrollTop, @@ -180,6 +190,19 @@ def test_session_skeleton_rows_have_title_and_stamp(outcome): assert outcome["stamps"] == 8, f"every row needs a timestamp bar: {outcome}" +def test_session_skeleton_rows_have_inline_stagger_delay(outcome): + # Greptile: the stagger must be applied inline per row, not via a CSS + # :nth-child rule (group labels are interleaved with rows, so :nth-child + # would skip most rows). Every row carries an animationDelay, and the last + # row's delay is non-empty (capped at 0.2s). + assert outcome["rows_with_delay"] == 8, ( + f"all 8 rows must carry an inline stagger delay: {outcome}" + ) + assert outcome["last_row_delay"] not in (None, "", "0s"), ( + f"the last row must have a real (non-zero) stagger delay, not be skipped: {outcome}" + ) + + def test_session_skeleton_sets_active_flag_and_resets_scroll(outcome): assert outcome["skeleton_active_flag"] is True, "must flag skeleton active for the clear-on-render path" assert outcome["list_scrolltop"] == 0, "skeleton should reset scroll to top"