feat(desktop): add session list density modes - #68124
Conversation
Bryntly
left a comment
There was a problem hiding this comment.
Security review completed. No secrets leakage, XSS vectors, or injection vulnerabilities were found. The use of localStorage for the new density setting correctly sanitizes incoming values via the densityCodec, which ensures only safe, predefined literal strings are accepted. The changes are safe.
Bryntly
left a comment
There was a problem hiding this comment.
Overall, the implementation for session list density is very clean. The UI integration, virtualization cache invalidation, and density-aware layout look rock solid.
I have one minor suggestion for apps/desktop/src/app/chat/sidebar/session-row-details.ts:
Currently, countLabel will format zero counts (e.g., 0 messages, 0 tool calls). Since most sessions won't have tool calls and some may not have messages immediately, this can lead to unnecessary visual clutter in the sidebar.
Consider returning null when the count is falsy/zero to omit these labels entirely:
const countLabel = (count: number, singular: string, plural = `${singular}s`) =>
count ? `${count} ${count === 1 ? singular : plural}` : nullOther than that, great use of the existing persistentAtom and correct fallback behavior when the title matches the preview. Code looks great!
Bryntly
left a comment
There was a problem hiding this comment.
Test Analyzer Review
I've reviewed the test coverage, quality, completeness, and correctness for PR #68124.
🔴 Critical Issues
- Missing i18n Support in Metadata Formatting: The test
it('formats deterministic metadata without ambiguous call wording')insession-row-details.test.tsasserts hardcoded English strings like"26 messages · 8 tool calls". The underlyingsessionRowDetailsfunction hardcodes the words "message(s)" and "tool call(s)" without accepting localized translations. Since the PR updateszh.ts,ja.ts, etc. for density settings, these metadata strings must also be localized. The tests should be updated to verify correct translation formatting.
🟡 Important Issues
- Missing Edge Case Tests in
session-row-details.test.ts:- The implementation uses
session.title?.trim()andsession.git_branch?.trim()to handle whitespace-only strings, but the tests only assertnullvalues and not whitespace-only strings (e.g.title: " "). - The behavior of
session.previewwhen it contains only whitespace isn't explicitly covered.
- The implementation uses
🟢 Strengths
session-list-density.test.tscorrectly isolates thepersistentAtominitialization by utilizingvi.resetModules()and importing the module dynamically. This is a clean approach to test module-level state execution.- Good assertions for fallback behavior (e.g., when
localStoragecontains an invalid value like"tiny", it correctly falls back to"comfortable"). - Density-aware virtual row estimation logic is concisely and effectively tested.
💡 Suggestions
- Consider adding tests to ensure that
message_count: 0ortool_call_count: 0are handled appropriately (e.g. omitting the label entirely if they shouldn't be displayed).
Bryntly
left a comment
There was a problem hiding this comment.
Type Analyzer Review
I've reviewed PR #68124 for type design, invariants, and type safety, particularly around session-list-density.ts and types.ts.
🟡 Important Issues
- Duplicated Type Definition: In
apps/desktop/src/app/chat/sidebar/session-row-details.ts,sessionRowEstimatere-declares the density type inline:(density: 'compact' | 'comfortable' | 'detailed'). This duplicates the single source of truth for the type. It should instead import and useSessionListDensityfrom@/store/session-list-density.
💡 Suggestions
- Const Assertion Strictness: In
apps/desktop/src/app/settings/appearance-settings.tsx,sessionDensityOptionsusesas const. While TypeScript successfully infers this and typechecks it properly via the genericSegmentedControl, consider usingsatisfies readonly { id: SessionListDensity; label: string }[](and importingSessionListDensity). This enforces that all existing and future density options inSessionListDensityare explicitly defined, similar to howembedOptionsworks directly below it.
🟢 Strengths
- Type Safety in Local Storage Codec: The
densityCodeccorrectly checksraw === 'compact' || raw === 'detailed' ? raw : 'comfortable'. Because the fallback is'comfortable', the condition seamlessly handles both garbage data and the valid'comfortable'literal without issue. - No
anyAbuse: The PR successfully introduces typed state management withpersistentAtomand UI options without a single use ofany, ensuring proper end-to-end type safety for the new setting.
|
Feedback incorporated in
I left zero-count labels ( |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused renderer-local implementation. The feature is still absent on current main, but two items need attention during salvage.
Problems
apps/desktop/src/app/chat/sidebar/session-row-details.ts:12hardcodes English count labels. Desktop already localizes count grammar (apps/desktop/src/i18n/en.ts:870versusapps/desktop/src/i18n/ja.ts:905), so Comfortable/Detailed metadata would remain English in Japanese and other locales.- Current main now interleaves date-divider rows in
apps/desktop/src/app/chat/sidebar/virtual-session-list.tsx:92-100; its list input isSidebarListRow[]at line 51. The density estimate must be merged without applying session-row sizing to divider rows or losing their measurement path.
Suggested changes
- Route message/tool-call labels through i18n and cover a non-English locale.
- Preserve the current divider branch and use row-kind-aware estimates when carrying the virtualizer change forward.
Automated hermes-sweeper review.
| const countLabel = (count: number, singular: string, plural = `${singular}s`) => | ||
| `${count} ${count === 1 ? singular : plural}` | ||
|
|
||
| const modelLabel = (model: null | string) => model?.split('/').pop()?.trim() || null |
There was a problem hiding this comment.
This helper hardcodes English metadata labels, so Japanese/Chinese Desktop users would see English message(s) and tool call(s) strings. Please route these through the i18n layer (for example, formatter callbacks passed from the row) and add locale coverage.
There was a problem hiding this comment.
@teknium1 thanks for the review. Both issues are addressed (force-pushed to PR branch).
Issue 1 — Hardcoded English labels: Routed through i18n. sessionRowDetails now accepts SessionRowFormatters with messageCount and toolCallCount callbacks. Added translations for all four locales (en/ja/zh/zh-hant). Verified with 8 targeted tests including Japanese locale coverage.
Issue 2 — Date-divider rows: Rebased onto current main (c92e2c0). estimateSize is now row-kind-aware — dividers return 28px regardless of density, session rows use sessionRowEstimate(density).
Cross-vendor review (Gemini 3.6 Flash + GPT-OSS 120B, full diff): both ACCEPTABLE. GPT-OSS caught unused imports from the rebase (fixed in a17dfb0).
Verification: ESLint clean, 11/11 targeted tests pass, MERGEABLE.
Replace hardcoded English 'message'/'tool call' strings in sessionRowDetails with locale-aware formatters (messageCount, toolCallCount) from the sidebar i18n section. Japanese users now see 件のメッセージ/件のツール呼び出し instead of raw English text. - Add messageCount/toolCallCount formatters to Sidebar i18n (en, ja, zh, zh-hant) - Accept SessionRowFormatters in sessionRowDetails instead of hardcoded English - Drop countLabel helper (now superseded by i18n formatters) - Omit zero-count labels so the sidebar stays clean (matches reviewer feedback) - Add locale coverage test (Japanese formatters produce correct metadata) - 8/8 targeted tests pass
b592a5d to
df74571
Compare
Rebase merged main's openSession() call path, leaving , openSessionTile, canOpenSessionWindow, openSessionInNewWindow, SessionDotState, and sessionDotState as dead imports. GPT-OSS review caught this. ESLint clean, 8/8 tests pass.
SummaryOne PR, #68124, addresses #68119 by implementing Desktop-local Compact, Comfortable, and Detailed session-row modes with deterministic metadata, initial-request previews, persisted presentation state, and density-aware virtualization. Related pull requests
Suggested consolidationKeep #68124 open with a salvage path: preserve its focused renderer-local preference, localized deterministic row details, preview behavior, and row-kind-aware virtualization. This follows the contributor keep_open review rather than overriding it; its two documented concerns are addressed in the supplied diff, so the updated branch should be validated against current main and any newly surfaced integration issues resolved. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I68119(["issue #68119 (open)"])
P68124["PR #68124 (open)"]
P68124 -->|best fix| I68119
class I68119 open
class P68124 open
class P68124 best
class P68124 target
click I68119 "https://github.com/NousResearch/hermes-agent/issues/68119"
click P68124 "https://github.com/NousResearch/hermes-agent/pull/68124"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 4 kB of issue/PR text, 7 kB of discussion (8 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
- merge main (date-divider rows in virtual-session-list, terminal font settings, reactions) into feat/desktop-session-density - keep session-density i18n keys alongside terminal-font keys in all locales - fix perfectionist/sort-imports in session-row.tsx: parent import (../session-status-dot) before sibling imports - density estimate is row-kind aware: dividers stay at 28px, session rows use sessionRowEstimate(density)
Squashed from #68124, resolved against current main (inbox cards, marquee titles, actions cluster). Adds a Settings → Appearance 'Session list density' preference — compact (default, unchanged), comfortable (+deterministic metadata line), detailed (+initial-request preview) — with density-aware virtualizer estimates. Closes #68119 (cherry picked from commits 93bc194..568f78a)
|
Thanks for this fix! It was salvaged into #86771 (cherry-picked onto current main with your authorship preserved in the commit history) and is now merged. Closing since the work has landed. |
Summary
Test plan
npx vitest run --project ui src/store/session-list-density.test.ts src/app/chat/sidebar/session-row-details.test.ts src/app/chat/sidebar/session-row-state.test.ts— 13 passed--project uisuite — 398 files / 3499 tests passedtsc --noEmit— cleannpm run build— succeeded, including renderer and Electron bundlesNotes
SessionInfometadata and deterministicsession.previewvalue.Closes #68119