Conversation
…thetic - Reduce logo from 38px to 28px with 6px border radius - Simplify title: 18px/700 → 13px/500 with gradient text - Remove subtitle entirely for cleaner layout - Version number now fades in on hover only - Replace warm border with subtle 0.5px black alpha - Add inner glow and radial hover effect on logo - Refine padding and spacing for breathing room
Contributor
📝 WalkthroughWalkthroughThe PR enhances the header brand display with a hover-triggered app-version reveal and refactors session timestamp handling for consistent i18n formatting. It updates styling, adds relative-time translation keys, threads computed labels through session logic and UI layers, and strengthens CLI session discovery by always supplementing indexed sessions with recent unindexed files. ChangesBrand Header Hover Version Display
Session Timestamps and i18n Formatting
Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web-ui/styles/layout-shell.css (1)
496-504: 💤 Low valueConsider slightly longer transition duration for smoother fade.
The version fade animation uses a 0.2s duration, which may feel abrupt for users. A slightly longer duration (e.g., 0.3s or 0.35s) or a small entrance delay could create a more polished feel.
✨ Optional refinement
.brand-version-fade-enter-active, .brand-version-fade-leave-active { - transition: opacity 0.2s var(--ease-smooth); + transition: opacity 0.3s var(--ease-smooth); }Or add a slight delay on entrance only:
.brand-version-fade-enter-active { - transition: opacity 0.2s var(--ease-smooth); + transition: opacity 0.3s var(--ease-smooth) 0.05s; } .brand-version-fade-leave-active { - transition: opacity 0.2s var(--ease-smooth); + transition: opacity 0.25s var(--ease-smooth); }🤖 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 `@web-ui/styles/layout-shell.css` around lines 496 - 504, Update the transition duration for the version fade to be slightly longer and smoother: change the transition on the .brand-version-fade-enter-active and .brand-version-fade-leave-active rules from 0.2s to something like 0.3s or 0.35s (or optionally 0.3s with a small entrance delay) so the .brand-version-fade-enter-from/.brand-version-fade-leave-to opacity transition feels less abrupt.
🤖 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 `@web-ui/partials/index/layout-header.html`:
- Around line 121-125: The version is only shown on mouse hover via the
brand-block mouse handlers and brandHovered state, which blocks keyboard, touch,
and screen-reader access; make the element keyboard-accessible by adding
tabindex="0" to the div.brand-block and wire `@focus`="brandHovered = true" and
`@blur`="brandHovered = false" alongside the existing `@mouseenter/`@mouseleave
handlers (keep the existing brandHovered boolean and the v-if on
appVersion/brand-version-fade), and add an ARIA enhancement such as wrapping the
version span with an aria-live="polite" region or making the version
always-visible in a lower-contrast style so screen readers and touch users can
perceive it; also ensure focus-visible CSS exists for the brand-block to show
keyboard focus.
In `@web-ui/styles/layout-shell.css`:
- Around line 477-487: The .brand-kicker gradient makes text invisible in
browsers that don't support background-clip; restore a solid fallback by keeping
the color: `#1d1d1f` declaration before applying the gradient and ensure the color
property appears earlier than the
-webkit-background-clip/-webkit-text-fill-color rules; then apply the
gradient/background-clip and finally set -webkit-text-fill-color: transparent so
unsupported browsers use the earlier color.
- Around line 441-443: Update the unit test assertion in
compact-layout-ui.test.mjs that checks the .brand-logo dimensions: change the
expected width and height from "38px" to "28px" so it matches the updated CSS
(.brand-logo width/height: 28px). Locate the assertion referencing ".brand-logo"
or the variables in the test (e.g., the selector or snapshot that expects 38px)
and update both width and height expectations to "28px".
---
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 496-504: Update the transition duration for the version fade to be
slightly longer and smoother: change the transition on the
.brand-version-fade-enter-active and .brand-version-fade-leave-active rules from
0.2s to something like 0.3s or 0.35s (or optionally 0.3s with a small entrance
delay) so the .brand-version-fade-enter-from/.brand-version-fade-leave-to
opacity transition feels less abrupt.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a368c7c-3390-4923-b694-a1935f011fdd
📒 Files selected for processing (3)
web-ui/app.jsweb-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.css
📜 Review details
🔇 Additional comments (2)
web-ui/app.js (1)
34-34: LGTM!web-ui/styles/layout-shell.css (1)
429-431: LGTM!Also applies to: 437-467, 472-494
Comment on lines
+121
to
+125
| <div class="brand-block" @mouseenter="brandHovered = true" @mouseleave="brandHovered = false"> | ||
| <div class="brand-head"> | ||
| <img class="brand-logo" src="/res/logo-pack.webp" alt="Codex Mate logo"> | ||
| <div class="brand-copy"> | ||
| <div class="brand-kicker">Codex Mate <span v-if="appVersion" class="brand-version">v{{ appVersion }}</span></div> | ||
| <div class="brand-kicker">Codex Mate<transition name="brand-version-fade"><span v-if="appVersion && brandHovered" class="brand-version"> v{{ appVersion }}</span></transition></div> |
Contributor
There was a problem hiding this comment.
Hover-only version display creates accessibility barriers.
The version number now requires hover to display, which creates significant accessibility issues:
- Keyboard users: Cannot trigger hover on the
div.brand-blockwith keyboard alone—there's no focusable element and no keyboard event handlers. - Touch device users: Hover interactions are unreliable or non-functional on mobile/tablet devices.
- Screen reader users: The dynamic appearance of content is not announced, and users may never discover the version information exists.
Consider one of these alternatives:
- Add keyboard support: Make the brand block focusable with
tabindex="0"and add@focus/@blurhandlers alongside the mouse events. - Add ARIA live region: Wrap the version in an ARIA live region so screen readers announce when it appears.
- Rethink the design: Show version by default in a less prominent style, or provide an alternative always-visible location for version info (e.g., in Settings).
♿ Proposed fix to add keyboard support
- <div class="brand-block" `@mouseenter`="brandHovered = true" `@mouseleave`="brandHovered = false">
+ <div class="brand-block" tabindex="0" `@mouseenter`="brandHovered = true" `@mouseleave`="brandHovered = false" `@focus`="brandHovered = true" `@blur`="brandHovered = false" role="region" aria-label="Application branding">
<div class="brand-head">
<img class="brand-logo" src="/res/logo-pack.webp" alt="Codex Mate logo">
<div class="brand-copy">
- <div class="brand-kicker">Codex Mate<transition name="brand-version-fade"><span v-if="appVersion && brandHovered" class="brand-version"> v{{ appVersion }}</span></transition></div>
+ <div class="brand-kicker">Codex Mate<transition name="brand-version-fade"><span v-if="appVersion && brandHovered" class="brand-version" role="status" aria-live="polite"> v{{ appVersion }}</span></transition></div>
</div>Note: You'll also need to add focus-visible styles in the CSS to indicate keyboard focus.
📝 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.
Suggested change
| <div class="brand-block" @mouseenter="brandHovered = true" @mouseleave="brandHovered = false"> | |
| <div class="brand-head"> | |
| <img class="brand-logo" src="/res/logo-pack.webp" alt="Codex Mate logo"> | |
| <div class="brand-copy"> | |
| <div class="brand-kicker">Codex Mate <span v-if="appVersion" class="brand-version">v{{ appVersion }}</span></div> | |
| <div class="brand-kicker">Codex Mate<transition name="brand-version-fade"><span v-if="appVersion && brandHovered" class="brand-version"> v{{ appVersion }}</span></transition></div> | |
| <div class="brand-block" tabindex="0" `@mouseenter`="brandHovered = true" `@mouseleave`="brandHovered = false" `@focus`="brandHovered = true" `@blur`="brandHovered = false" role="region" aria-label="Application branding"> | |
| <div class="brand-head"> | |
| <img class="brand-logo" src="/res/logo-pack.webp" alt="Codex Mate logo"> | |
| <div class="brand-copy"> | |
| <div class="brand-kicker">Codex Mate<transition name="brand-version-fade"><span v-if="appVersion && brandHovered" class="brand-version" role="status" aria-live="polite"> v{{ appVersion }}</span></transition></div> |
🤖 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 `@web-ui/partials/index/layout-header.html` around lines 121 - 125, The version
is only shown on mouse hover via the brand-block mouse handlers and brandHovered
state, which blocks keyboard, touch, and screen-reader access; make the element
keyboard-accessible by adding tabindex="0" to the div.brand-block and wire
`@focus`="brandHovered = true" and `@blur`="brandHovered = false" alongside the
existing `@mouseenter/`@mouseleave handlers (keep the existing brandHovered
boolean and the v-if on appVersion/brand-version-fade), and add an ARIA
enhancement such as wrapping the version span with an aria-live="polite" region
or making the version always-visible in a lower-contrast style so screen readers
and touch users can perceive it; also ensure focus-visible CSS exists for the
brand-block to show keyboard focus.
Comment on lines
+441
to
+443
| width: 28px; | ||
| height: 28px; | ||
| border-radius: 6px; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check test file for logo dimension assertions
rg -n -A2 -B2 'brand-logo.*38px' tests/Repository: SakuraByteCore/codexmate
Length of output: 738
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the CSS around the referenced lines
echo "---- web-ui/styles/layout-shell.css: around 430-460 ----"
nl -ba web-ui/styles/layout-shell.css | sed -n '420,470p'
# 2) Find all .brand-logo blocks and any width/height assertions involving 38px/28px
echo "---- occurrences in layout-shell.css ----"
rg -n "\.brand-logo" -S web-ui/styles/layout-shell.css
echo "---- width/height values near brand-logo in layout-shell.css ----"
rg -n -S "brand-logo|width:\s*(38|28)px|height:\s*(38|28)px" web-ui/styles/layout-shell.css -n || true
# 3) Search all tests for explicit 38x38 expectations
echo "---- tests with brand-logo 38px expectations ----"
rg -n -A3 -B3 "brand-logo.*38px|\.brand-logo\\s*\\{[\\s\\S]*width:\\s*38px|height:\\s*38px" tests/ || true
# 4) Search all tests for 28px expectations
echo "---- tests with brand-logo 28px expectations ----"
rg -n -A3 -B3 "brand-logo.*28px|\.brand-logo\\s*\\{[\\s\\S]*width:\\s*28px|height:\\s*28px" tests/ || trueRepository: SakuraByteCore/codexmate
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the CSS around the referenced lines
echo "---- web-ui/styles/layout-shell.css: around 430-470 ----"
nl -ba web-ui/styles/layout-shell.css | sed -n '420,470p'
# 2) Find all .brand-logo blocks and any width/height values near them
echo "---- occurrences in layout-shell.css ----"
rg -n "\.brand-logo" -S web-ui/styles/layout-shell.css
echo "---- width/height values in layout-shell.css (38/28) ----"
rg -n -S "width:\s*(38|28)px|height:\s*(38|28)px" web-ui/styles/layout-shell.css || true
# 3) Search all tests for explicit 38x38 expectations
echo "---- tests with brand-logo 38px expectations ----"
rg -n -A3 -B3 "brand-logo.*38px|\.brand-logo\\s*\\{[\\s\\S]*width:\\s*38px|height:\\s*38px" tests/ || true
# 4) Search all tests for 28x28 expectations
echo "---- tests with brand-logo 28px expectations ----"
rg -n -A3 -B3 "brand-logo.*28px|\.brand-logo\\s*\\{[\\s\\S]*width:\\s*28px|height:\\s*28px" tests/ || trueRepository: SakuraByteCore/codexmate
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect CSS around the referenced lines (use cat -n since nl isn't available)
echo "---- web-ui/styles/layout-shell.css: around 430-470 ----"
cat -n web-ui/styles/layout-shell.css | sed -n '420,470p'
# 2) Find all .brand-logo blocks and any 38/28px width/height occurrences in that file
echo "---- occurrences in layout-shell.css ----"
rg -n "\\.brand-logo" -S web-ui/styles/layout-shell.css || true
echo "---- width/height values in layout-shell.css (38/28) ----"
rg -n -S "width:\\s*(38|28)px|height:\\s*(38|28)px" web-ui/styles/layout-shell.css || true
# 3) Search all tests for explicit 38x38 expectations for .brand-logo
echo "---- tests with brand-logo 38px expectations ----"
rg -n -A3 -B3 "brand-logo.*38px|\\.brand-logo\\s*\\{[\\s\\S]*width:\\s*38px|\\.brand-logo\\s*\\{[\\s\\S]*height:\\s*38px" tests/ || true
# 4) Search all tests for explicit 28x28 expectations for .brand-logo
echo "---- tests with brand-logo 28px expectations ----"
rg -n -A3 -B3 "brand-logo.*28px|\\.brand-logo\\s*\\{[\\s\\S]*width:\\s*28px|\\.brand-logo\\s*\\{[\\s\\S]*height:\\s*28px" tests/ || trueRepository: SakuraByteCore/codexmate
Length of output: 3079
Fix unit test for updated .brand-logo dimensions
web-ui/styles/layout-shell.css sets .brand-logo to width/height: 28px, but tests/unit/compact-layout-ui.test.mjs still asserts width: 38px; height: 38px for .brand-logo, which will fail in CI. Update the test expectation to 28px.
🤖 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 `@web-ui/styles/layout-shell.css` around lines 441 - 443, Update the unit test
assertion in compact-layout-ui.test.mjs that checks the .brand-logo dimensions:
change the expected width and height from "38px" to "28px" so it matches the
updated CSS (.brand-logo width/height: 28px). Locate the assertion referencing
".brand-logo" or the variables in the test (e.g., the selector or snapshot that
expects 38px) and update both width and height expectations to "28px".
Comment on lines
+477
to
487
| font-size: 13px; | ||
| line-height: 1.2; | ||
| font-family: -apple-system, BlinkMacSystemFont, "SF Pro Text", "Segoe UI", system-ui, sans-serif; | ||
| color: #1d1d1f; | ||
| letter-spacing: -0.01em; | ||
| font-weight: 500; | ||
| background: linear-gradient(180deg, #1d1d1f 0%, #2d2d2f 100%); | ||
| -webkit-background-clip: text; | ||
| -webkit-text-fill-color: transparent; | ||
| background-clip: text; | ||
| } |
Contributor
There was a problem hiding this comment.
Add fallback color for gradient text.
The .brand-kicker uses gradient text via -webkit-background-clip: text and -webkit-text-fill-color: transparent, which may not be supported in all browsers. Without a fallback, text becomes invisible in unsupported browsers.
🎨 Proposed fix to add fallback color
Keep the color declaration before applying the gradient effect. Browsers that don't support background-clip will use the solid color:
.brand-kicker {
font-size: 13px;
line-height: 1.2;
font-family: -apple-system, BlinkMacSystemFont, "SF Pro Text", "Segoe UI", system-ui, sans-serif;
color: `#1d1d1f`;
letter-spacing: -0.01em;
font-weight: 500;
background: linear-gradient(180deg, `#1d1d1f` 0%, `#2d2d2f` 100%);
-webkit-background-clip: text;
- -webkit-text-fill-color: transparent;
background-clip: text;
+ -webkit-text-fill-color: transparent;
}Note: Moving -webkit-text-fill-color after background-clip ensures the color: #1d1d1f`` acts as a fallback.
🤖 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 `@web-ui/styles/layout-shell.css` around lines 477 - 487, The .brand-kicker
gradient makes text invisible in browsers that don't support background-clip;
restore a solid fallback by keeping the color: `#1d1d1f` declaration before
applying the gradient and ensure the color property appears earlier than the
-webkit-background-clip/-webkit-text-fill-color rules; then apply the
gradient/background-clip and finally set -webkit-text-fill-color: transparent so
unsupported browsers use the earlier color.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 170-171: The hover-only reveal uses _ctx.brandHovered set by
onMouseenter/onMouseleave; add keyboard support by making the brand container
focusable (add tabindex="0") and toggle the same state on focus/blur (add
onFocus: $event => (_ctx.brandHovered = true) and onBlur: $event =>
(_ctx.brandHovered = false)) so the version render that checks _ctx.brandHovered
also becomes visible to keyboard users; update the same handlers wherever
onMouseenter/onMouseleave are used for the brand (and the corresponding version
render logic) so focus/blur mirrors hover behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53b319e6-80ca-4617-9785-45f62557d6f2
📒 Files selected for processing (1)
web-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (1)
web-ui/res/web-ui-render.precompiled.js (1)
2-2: LGTM!
Comment on lines
+170
to
+171
| onMouseenter: $event => (_ctx.brandHovered = true), | ||
| onMouseleave: $event => (_ctx.brandHovered = false) |
Contributor
There was a problem hiding this comment.
Add keyboard-accessible reveal path for the version label.
At Line 170 and Line 171, visibility is controlled only by mouse hover, and at Line 184 the version render is gated by that hover state. Keyboard users won’t trigger this path, so the version can remain hidden.
Please also toggle the same state on focus/blur (and make the brand container focusable) so keyboard navigation gets equivalent behavior.
Also applies to: 184-189
🤖 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 `@web-ui/res/web-ui-render.precompiled.js` around lines 170 - 171, The
hover-only reveal uses _ctx.brandHovered set by onMouseenter/onMouseleave; add
keyboard support by making the brand container focusable (add tabindex="0") and
toggle the same state on focus/blur (add onFocus: $event => (_ctx.brandHovered =
true) and onBlur: $event => (_ctx.brandHovered = false)) so the version render
that checks _ctx.brandHovered also becomes visible to keyboard users; update the
same handlers wherever onMouseenter/onMouseleave are used for the brand (and the
corresponding version render logic) so focus/blur mirrors hover behavior.
- Add translation keys for relative time (zh/en/ja) - Modify formatSessionTimelineTimestamp to support relative time - Pass t function to buildUsageHeatmap and buildSessionTimelineNodes - Add updatedAtLabel field to session list items - Update panel-sessions.html to use formatted time labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/res/web-ui-render.precompiled.js (1)
2356-2358:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
updatedAtLabelin the memo key to prevent stale session time text.At Line 2356, row memoization tracks
session.updatedAt, but Line 2458 renderssession.updatedAtLabelfirst. IfupdatedAtLabelchanges withoutupdatedAtchanging, the row may not rerender.♻️ Suggested fix
- const _memo = ([_ctx.activeSessionExportKey === _ctx.getSessionExportKey(session), session.messageCount, session.updatedAt, session.title, session.sourceLabel, session.cwd, _ctx.isSessionPinned(session), _ctx.sessionsLoading, session.match && session.match.count]) + const _memo = ([_ctx.activeSessionExportKey === _ctx.getSessionExportKey(session), session.messageCount, session.updatedAtLabel, session.updatedAt, session.title, session.sourceLabel, session.cwd, _ctx.isSessionPinned(session), _ctx.sessionsLoading, session.match && session.match.count])🤖 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 `@web-ui/res/web-ui-render.precompiled.js` around lines 2356 - 2358, The memoization array _memo should include session.updatedAtLabel so changes to the rendered time string trigger rerender; update the _memo declaration (the one currently listing session.updatedAt) to also include session.updatedAtLabel, and update the cache key construction (the expression assigning _cached.key === session.source + '-' + session.sessionId + '-' + session.filePath) to append '-' + session.updatedAtLabel so the cached entry invalidates when the label changes; make these changes where _memo and the _cached.key comparison are defined.
🤖 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 `@cli.js`:
- Around line 4882-4888: The code always calls
collectRecentJsonlFiles(claudeProjectsDir, { returnCount: scanCount,
maxFilesScanned, ignoreSubPath: ... }) even when sessions.length already meets
targetCount, causing unnecessary I/O; change the logic around seenFilePaths and
fallbackFiles so you only invoke collectRecentJsonlFiles when sessions.length <
targetCount (compute needed = targetCount - sessions.length and pass that as
returnCount instead of unconditional scanCount), and keep using seenFilePaths to
dedupe returned files; update any variable names used (scanCount/needed) so
collectRecentJsonlFiles is skipped when not needed.
In `@web-ui/logic.sessions.mjs`:
- Line 619: The function buildUsageChartGroups(sessions = [], options = {}) uses
the undeclared variable t when calling formatSessionTimelineTimestamp, causing a
ReferenceError; fix it by adding a t parameter to buildUsageChartGroups (e.g.,
buildUsageChartGroups(sessions = [], options = {}, t)) and update all call sites
(notably the caller in app.computed.session.mjs that invokes
buildUsageChartGroups(this.sessionsUsageList, { range:
this.sessionsUsageTimeRange })) to pass the translation/context t through;
ensure formatSessionTimelineTimestamp calls inside buildUsageChartGroups (and
any other places using t) use the newly added parameter.
---
Outside diff comments:
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 2356-2358: The memoization array _memo should include
session.updatedAtLabel so changes to the rendered time string trigger rerender;
update the _memo declaration (the one currently listing session.updatedAt) to
also include session.updatedAtLabel, and update the cache key construction (the
expression assigning _cached.key === session.source + '-' + session.sessionId +
'-' + session.filePath) to append '-' + session.updatedAtLabel so the cached
entry invalidates when the label changes; make these changes where _memo and the
_cached.key comparison are defined.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e187b1a3-c588-4ed9-9fa9-ffaa80f6236c
📒 Files selected for processing (8)
cli.jstests/unit/web-ui-behavior-parity.test.mjsweb-ui/logic.sessions.mjsweb-ui/modules/app.computed.session.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-sessions.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/session-helpers.mjs
✅ Files skipped from review due to trivial changes (1)
- web-ui/modules/i18n.dict.mjs
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: ci / 0_ci (18).txt
web-ui/logic.sessions.mjs
[error] 619-619: Test failures due to ReferenceError: t is not defined in buildUsageChartGroups.
🪛 GitHub Actions: ci / 1_ci (20).txt
web-ui/logic.sessions.mjs
[error] 619-619: Unit tests failed with ReferenceError: t is not defined at buildUsageChartGroups.
🪛 GitHub Actions: ci / 2_ci (22).txt
web-ui/logic.sessions.mjs
[error] 619-619: Unit test failure: ReferenceError: t is not defined at buildUsageChartGroups.
🪛 GitHub Actions: ci / ci (18)
web-ui/logic.sessions.mjs
[error] 619-619: Unit tests failed in buildUsageChartGroups due to ReferenceError: t is not defined at buildUsageChartGroups (file:///home/runner/work/codexmate/codexmate/web-ui/logic.sessions.mjs:619:85).
🪛 GitHub Actions: ci / ci (20)
web-ui/logic.sessions.mjs
[error] 619-619: Unit tests failed due to ReferenceError: t is not defined in buildUsageChartGroups. (at buildUsageChartGroups in web-ui/logic.sessions.mjs:619:85)
🪛 GitHub Actions: ci / ci (22)
web-ui/logic.sessions.mjs
[error] 619-619: ReferenceError: t is not defined in buildUsageChartGroups (at web-ui/logic.sessions.mjs:619:85).
🔇 Additional comments (6)
web-ui/res/web-ui-render.precompiled.js (1)
2516-2516: LGTM!web-ui/logic.sessions.mjs (1)
178-245: LGTM!Also applies to: 265-265, 757-757, 772-772, 816-817
web-ui/modules/app.computed.session.mjs (1)
244-244: LGTM!Also applies to: 283-283
web-ui/partials/index/panel-sessions.html (1)
196-196: LGTM!Also applies to: 215-215
web-ui/session-helpers.mjs (1)
1-1: LGTM!Also applies to: 269-274
tests/unit/web-ui-behavior-parity.test.mjs (1)
328-328: LGTM!Also applies to: 358-358
Comment on lines
+4882
to
+4888
| // 补充扫描未索引的 .jsonl 文件（包括 sessions-index.json 中遗漏的会话） | ||
| const seenFilePaths = new Set(sessions.map((item) => item.filePath).filter(Boolean)); | ||
| const fallbackFiles = collectRecentJsonlFiles(claudeProjectsDir, { | ||
| returnCount: scanCount, | ||
| maxFilesScanned, | ||
| ignoreSubPath: `${path.sep}subagents${path.sep}` | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid unconditional fallback directory scan when targetCount is already met.
This now always traverses claudeProjectsDir, even if sessions.length has already reached targetCount. On large trees, that adds avoidable synchronous I/O and slows session list responses.
⚡ Suggested fix
- // 补充扫描未索引的 .jsonl 文件（包括 sessions-index.json 中遗漏的会话）
- const seenFilePaths = new Set(sessions.map((item) => item.filePath).filter(Boolean));
- const fallbackFiles = collectRecentJsonlFiles(claudeProjectsDir, {
- returnCount: scanCount,
- maxFilesScanned,
- ignoreSubPath: `${path.sep}subagents${path.sep}`
- });
- for (const filePath of fallbackFiles) {
+ // 补充扫描未索引的 .jsonl 文件（包括 sessions-index.json 中遗漏的会话）
+ if (sessions.length < targetCount) {
+ const seenFilePaths = new Set(sessions.map((item) => item.filePath).filter(Boolean));
+ const fallbackFiles = collectRecentJsonlFiles(claudeProjectsDir, {
+ returnCount: scanCount,
+ maxFilesScanned,
+ ignoreSubPath: `${path.sep}subagents${path.sep}`
+ });
+ for (const filePath of fallbackFiles) {
if (seenFilePaths.has(filePath)) continue;
const summary = parseClaudeSessionSummary(filePath, {
summaryReadBytes,
titleReadBytes
- });
- if (summary) {
- sessions.push(attachSessionNativeStatus({
- ...summary,
- derived: isDerivedSessionFile(filePath)
- }));
- seenFilePaths.add(filePath);
- }
+ });
+ if (summary) {
+ sessions.push(attachSessionNativeStatus({
+ ...summary,
+ derived: isDerivedSessionFile(filePath)
+ }));
+ seenFilePaths.add(filePath);
+ }
- if (sessions.length >= targetCount) {
- break;
+ if (sessions.length >= targetCount) {
+ break;
+ }
}
}🤖 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 `@cli.js` around lines 4882 - 4888, The code always calls
collectRecentJsonlFiles(claudeProjectsDir, { returnCount: scanCount,
maxFilesScanned, ignoreSubPath: ... }) even when sessions.length already meets
targetCount, causing unnecessary I/O; change the logic around seenFilePaths and
fallbackFiles so you only invoke collectRecentJsonlFiles when sessions.length <
targetCount (compute needed = targetCount - sessions.length and pass that as
returnCount instead of unconditional scanCount), and keep using seenFilePaths to
dedupe returned files; update any variable names used (scanCount/needed) so
collectRecentJsonlFiles is skipped when not needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@web-ui/session-helpers.mjs`:
- Around line 272-276: The catch path for session.updatedAtLabel can re-throw
because it re-uses potentially malformed session.updatedAt; update the code
around formatSessionTimelineTimestamp and session.updatedAtLabel to first
coerce/validate session.updatedAt into a safe local value (e.g., const
safeUpdatedAt = typeof session?.updatedAt === 'string' ? session.updatedAt : '')
and use that safeUpdatedAt when calling formatSessionTimelineTimestamp and when
assigning fallback, so both the try and catch use the validated value and
loadSessions cannot crash on bad session shapes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86265de7-ceb7-42c2-88c6-151e16c84fc9
📒 Files selected for processing (1)
web-ui/session-helpers.mjs
Comment on lines
+272
to
+276
| try { | ||
| session.updatedAtLabel = formatSessionTimelineTimestamp(session.updatedAt || '', t); | ||
| } catch (e) { | ||
| session.updatedAtLabel = session.updatedAt || ''; | ||
| } |
Contributor
There was a problem hiding this comment.
Fallback path can throw again for malformed session entries.
The catch block re-reads session.updatedAt (Line 275), so null/non-object entries still crash loadSessions. Guard the session shape once and reuse a safe value.
Proposed fix
if (typeof t === 'function') {
for (const session of this.sessionsList) {
+ const rawUpdatedAt = (session && typeof session === 'object' && typeof session.updatedAt === 'string')
+ ? session.updatedAt
+ : '';
try {
- session.updatedAtLabel = formatSessionTimelineTimestamp(session.updatedAt || '', t);
+ if (session && typeof session === 'object') {
+ session.updatedAtLabel = formatSessionTimelineTimestamp(rawUpdatedAt, t);
+ }
} catch (e) {
- session.updatedAtLabel = session.updatedAt || '';
+ if (session && typeof session === 'object') {
+ session.updatedAtLabel = rawUpdatedAt;
+ }
}
}
}📝 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.
Suggested change
| try { | |
| session.updatedAtLabel = formatSessionTimelineTimestamp(session.updatedAt || '', t); | |
| } catch (e) { | |
| session.updatedAtLabel = session.updatedAt || ''; | |
| } | |
| if (typeof t === 'function') { | |
| for (const session of this.sessionsList) { | |
| const rawUpdatedAt = (session && typeof session === 'object' && typeof session.updatedAt === 'string') | |
| ? session.updatedAt | |
| : ''; | |
| try { | |
| if (session && typeof session === 'object') { | |
| session.updatedAtLabel = formatSessionTimelineTimestamp(rawUpdatedAt, t); | |
| } | |
| } catch (e) { | |
| if (session && typeof session === 'object') { | |
| session.updatedAtLabel = rawUpdatedAt; | |
| } | |
| } | |
| } | |
| } |
🤖 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 `@web-ui/session-helpers.mjs` around lines 272 - 276, The catch path for
session.updatedAtLabel can re-throw because it re-uses potentially malformed
session.updatedAt; update the code around formatSessionTimelineTimestamp and
session.updatedAtLabel to first coerce/validate session.updatedAt into a safe
local value (e.g., const safeUpdatedAt = typeof session?.updatedAt === 'string'
? session.updatedAt : '') and use that safeUpdatedAt when calling
formatSessionTimelineTimestamp and when assigning fallback, so both the try and
catch use the validated value and loadSessions cannot crash on bad session
shapes.
- Remove relative time logic (just now, N minutes ago, etc.) - Use fixed format: YYYY-MM-DD HH:mm - Remove i18n keys for relative time - Update tests to match new format - Net code reduction: 91 lines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/session-helpers.mjs (1)
269-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard session shape before reading
updatedAt.Line 270 dereferences
session.updatedAtwithout validating each entry. A malformedlist-sessionsitem (e.g.,null) can throw and fail the entire load path.Proposed fix
- this.sessionsList = Array.isArray(res.sessions) ? res.sessions : []; - for (const session of this.sessionsList) { - session.updatedAtLabel = formatSessionTimelineTimestamp(session.updatedAt || ''); - } + const rawSessions = Array.isArray(res.sessions) ? res.sessions : []; + this.sessionsList = rawSessions.filter((session) => session && typeof session === 'object'); + for (const session of this.sessionsList) { + const rawUpdatedAt = typeof session.updatedAt === 'string' ? session.updatedAt : ''; + session.updatedAtLabel = formatSessionTimelineTimestamp(rawUpdatedAt); + }🤖 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 `@web-ui/session-helpers.mjs` around lines 269 - 270, Loop over this.sessionsList may contain non-object entries; guard the session shape before dereferencing session.updatedAt by checking that session is truthy and an object (e.g., if (session && typeof session === 'object')), then call formatSessionTimelineTimestamp(session.updatedAt || '') and assign to session.updatedAtLabel; otherwise set a safe default (empty string or null) or skip assignment. Update the loop handling around sessionsList/updatedAtLabel/formatSessionTimelineTimestamp to avoid throwing on null/undefined list items.
🤖 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 `@web-ui/session-helpers.mjs`:
- Around line 269-270: The code sets session.updatedAtLabel once when iterating
this.sessionsList using formatSessionTimelineTimestamp, but
loadActiveSessionDetail later mutates activeSession.updatedAt leaving
updatedAtLabel stale; update loadActiveSessionDetail to recompute the label
immediately after changing activeSession.updatedAt (e.g., set
activeSession.updatedAtLabel =
formatSessionTimelineTimestamp(activeSession.updatedAt || '')), or convert
updatedAtLabel into a derived/computed getter, referencing sessionsList,
updatedAtLabel, formatSessionTimelineTimestamp, loadActiveSessionDetail and
activeSession so the UI always reflects the latest updatedAt.
---
Duplicate comments:
In `@web-ui/session-helpers.mjs`:
- Around line 269-270: Loop over this.sessionsList may contain non-object
entries; guard the session shape before dereferencing session.updatedAt by
checking that session is truthy and an object (e.g., if (session && typeof
session === 'object')), then call
formatSessionTimelineTimestamp(session.updatedAt || '') and assign to
session.updatedAtLabel; otherwise set a safe default (empty string or null) or
skip assignment. Update the loop handling around
sessionsList/updatedAtLabel/formatSessionTimelineTimestamp to avoid throwing on
null/undefined list items.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc850feb-12e5-496d-8f9f-18217fe88865
📒 Files selected for processing (4)
tests/unit/web-ui-logic.test.mjsweb-ui/logic.sessions.mjsweb-ui/modules/i18n.dict.mjsweb-ui/session-helpers.mjs
💤 Files with no reviewable changes (1)
- web-ui/modules/i18n.dict.mjs
📜 Review details
🔇 Additional comments (3)
web-ui/logic.sessions.mjs (1)
178-186: LGTM!Also applies to: 206-206, 399-399, 560-560, 586-586, 698-699, 713-714, 757-758
tests/unit/web-ui-logic.test.mjs (1)
959-964: LGTM!Also applies to: 981-983
web-ui/session-helpers.mjs (1)
1-1: LGTM!
Comment on lines
+269
to
+270
| for (const session of this.sessionsList) { | ||
| session.updatedAtLabel = formatSessionTimelineTimestamp(session.updatedAt || ''); |
Contributor
There was a problem hiding this comment.
Keep updatedAtLabel in sync when updatedAt changes later.
updatedAtLabel is computed at Line 270, but loadActiveSessionDetail can later overwrite activeSession.updatedAt; that leaves a stale label in the UI.
Proposed follow-up in loadActiveSessionDetail
if (res.updatedAt) {
this.activeSession.updatedAt = res.updatedAt;
+ this.activeSession.updatedAtLabel = formatSessionTimelineTimestamp(
+ typeof res.updatedAt === 'string' ? res.updatedAt : ''
+ );
}🤖 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 `@web-ui/session-helpers.mjs` around lines 269 - 270, The code sets
session.updatedAtLabel once when iterating this.sessionsList using
formatSessionTimelineTimestamp, but loadActiveSessionDetail later mutates
activeSession.updatedAt leaving updatedAtLabel stale; update
loadActiveSessionDetail to recompute the label immediately after changing
activeSession.updatedAt (e.g., set activeSession.updatedAtLabel =
formatSessionTimelineTimestamp(activeSession.updatedAt || '')), or convert
updatedAtLabel into a derived/computed getter, referencing sessionsList,
updatedAtLabel, formatSessionTimelineTimestamp, loadActiveSessionDetail and
activeSession so the UI always reflects the latest updatedAt.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Style
Tests