feat(view): add sequential run numbering and sortable table headers t… - #270
feat(view): add sequential run numbering and sortable table headers t…#270Adityakk9031 wants to merge 2 commits into
Conversation
jeqcho
left a comment
There was a problem hiding this comment.
Thanks for picking this up! This lands right on the index wishlist from #247, and the execution shows care: run numbers carry a data-val for correct numeric sorting, the When header starts in the sort-desc state that matches the rendered order, the empty-state colspan and the listener-count test were both updated, and there's a focused new test for the numbering and headers. CI is fully green. A few things to address before merge:
- Sorting by
When(andLog) is effectively a no-op. The comparator doesparseFloat(aVal), andparseFloat("2026-07-29T12:00:00+00:00")returns2026— so every same-year timestamp compares equal and the stable sort leaves the order unchanged. Same problem for log filenames that start with a date stamp. Suggested fix: only take the numeric path when the whole value is numeric, e.g.const isNum = v => /^-?\d+(\.\d+)?$/.test(v);and uselocaleCompareotherwise — ISO 8601 strings sort correctly lexicographically. A quick manual check (clickWhen, confirm the order actually flips with two same-year entries) would have caught this; worth adding to your verification list alongside the pytest run. Closes #247will close the whole issue prematurely. #247 also covers the synced multicam player, transcript rail, media serving, and collision-proof run identity. Please change the PR description to something likePart of #247/Addresses the index portion of #247.- The new
IndexEntry.run_numberfield is dead code. Neither_index_entrynor_unreadable_index_entryincli.pyever sets it, so theentry.run_number is not Nonebranch in_rowis unreachable in practice. Simplest is to drop the field and the fallback entirely and just pass the computed number to_row. (While there, theid(entry)map can go too: enumerate the descending sort directly, e.g.run_num = len(entries) - i.) - Edge case: unreadable logs grab the lowest run numbers.
_unreadable_index_entrycan producecreated="", which sorts before every ISO timestamp, so a corrupt file becomes#1and shifts every real run's number. Worth deciding how unreadable entries should participate in numbering (or at least sorting them last). - Test name now contradicts its assertion.
test_delegated_row_click_listener_is_present_onceassertscount == 2after this change. Please rename it (e.g...._click_listeners_are_row_delegation_and_header_sort) or split the assertion so the intent stays readable. - Nice-to-have: set
aria-sort="ascending"/"descending"on the active header (and remove it from the others) when sorting — the visual▲/▼is already there, this makes it available to screen readers too.
One broader design flag, just so it's on your radar: #247 ultimately wants run identity to be stable and collision-proof, whereas render-time renumbering shifts whenever a log is deleted (a saved filter like #3 would then match a different run). The fixes above stand either way — but the numbering scheme may evolve as the rest of #247 lands.
Happy to take another look once these are in — the overall shape is right and points 2, 3, and 5 are quick fixes; point 1 is the only real behavior bug.
|
Thanks for coming back to this, @Adityakk9031! I took another look after your latest push. Unfortunately, the new commit (
One important heads-up before you invest more time here: PR #272 ("view: multicam player with synced transcript rail, run numbering, sortable index") is now open and covers this PR's entire scope as part of the larger #247 work — sequential run numbering, sortable headers with persisted state, and run-number filtering, plus the collision-proof run identity that makes the numbering stable. If #272 merges, this PR would be redundant. It may be worth holding off on further changes here until that lands — and thank you again for the contribution and for engaging with #247; the index was a genuinely useful place to start. |
|
Update, @Adityakk9031 — good news for this PR: #272 has been closed rather than merged. The larger view work was split into focused pieces, and the index slice this PR covers is now tracked by #354 ("view: index UX from #272 — run numbering, sortable headers, richer filter"). Since this PR only touches To take it forward:
One boundary note: the collision-proof run-identity work is tracked separately in #355 and is out of scope here — no need to touch it. Happy to re-review as soon as the six points are addressed. Thanks for your patience through the #272 shuffle — the timing was unlucky, but your instinct to start with the index was right, and it's now exactly the piece that's still needed. |
Closes #247.
Enhancements
#1,#2, …) by creation order in the directory log index.▲/▼).#1,#2).Verification
ruff check .— Passed.ruff format --check .— Passed.pytest tests/test_html_index.py tests/test_html_view.py— 94 passed.