Phase 1 (#4662): address remaining Greptile review (P2 + stagger)
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.
This commit is contained in:
1 parent
ef0b4ba37a
commit
42d1fbc728
3 files changed
+33
-12
No files matched your search
+6
-4
@@ -3518,6 +3518,7 @@ function showSessionListSkeleton(){
|
|||||||
const wrap = document.createElement('div');
|
const wrap = document.createElement('div');
|
||||||
wrap.className = 'skeleton-list';
|
wrap.className = 'skeleton-list';
|
||||||
wrap.setAttribute('aria-hidden', 'true');
|
wrap.setAttribute('aria-hidden', 'true');
|
||||||
|
let rowIndex = 0;
|
||||||
for(const group of _SESSION_SKELETON_GROUPS){
|
for(const group of _SESSION_SKELETON_GROUPS){
|
||||||
const label = document.createElement('div');
|
const label = document.createElement('div');
|
||||||
label.className = 'skeleton-group-label';
|
label.className = 'skeleton-group-label';
|
||||||
@@ -3525,6 +3526,11 @@ function showSessionListSkeleton(){
|
|||||||
for(const spec of group.rows){
|
for(const spec of group.rows){
|
||||||
const row = document.createElement('div');
|
const row = document.createElement('div');
|
||||||
row.className = 'skeleton-row';
|
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');
|
const title = document.createElement('div');
|
||||||
title.className = 'skeleton-bar skeleton-title';
|
title.className = 'skeleton-bar skeleton-title';
|
||||||
title.style.width = spec.title + '%';
|
title.style.width = spec.title + '%';
|
||||||
@@ -3554,10 +3560,6 @@ function showSessionListSkeleton(){
|
|||||||
_sessionListSkeletonActive = true;
|
_sessionListSkeletonActive = true;
|
||||||
}
|
}
|
||||||
|
|
||||||
function _clearSessionListSkeletonFlag(){
|
|
||||||
_sessionListSkeletonActive = false;
|
|
||||||
}
|
|
||||||
|
|
||||||
function _isOptimisticFirstTurnSessionRow(s){
|
function _isOptimisticFirstTurnSessionRow(s){
|
||||||
if(!s||!s.session_id||s.archived) return false;
|
if(!s||!s.session_id||s.archived) return false;
|
||||||
const messageCount=Number(s.message_count||0);
|
const messageCount=Number(s.message_count||0);
|
||||||
|
|||||||
+4
-8
@@ -1236,15 +1236,11 @@
|
|||||||
animation:skeletonSheen 1.25s ease-in-out infinite;background-size:200% 100%;}
|
animation:skeletonSheen 1.25s ease-in-out infinite;background-size:200% 100%;}
|
||||||
.skeleton-tree-row .skeleton-name{min-width:0;}
|
.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;}
|
.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,.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 skeletonSheen{0%{background-position:200% 0;}100%{background-position:-200% 0;}}
|
||||||
@keyframes skeletonFadeIn{from{opacity:0;}to{opacity:1;}}
|
@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);}
|
:root{--skeleton-base:color-mix(in srgb,var(--text) 8%,transparent);--skeleton-sheen:color-mix(in srgb,var(--text) 22%,transparent);}
|
||||||
|
|||||||
@@ -137,6 +137,16 @@ const result = {
|
|||||||
rows: list.querySelectorAll('.skeleton-row').length,
|
rows: list.querySelectorAll('.skeleton-row').length,
|
||||||
titles: list.querySelectorAll('.skeleton-title').length,
|
titles: list.querySelectorAll('.skeleton-title').length,
|
||||||
stamps: list.querySelectorAll('.skeleton-stamp').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,
|
skeleton_active_flag: _sessionListSkeletonActive,
|
||||||
list_scrolltop: list.scrollTop,
|
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}"
|
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):
|
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["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"
|
assert outcome["list_scrolltop"] == 0, "skeleton should reset scroll to top"
|
||||||
|
|||||||
Reference in new issue
Block a user