fix(web): sessions, CSP, and settings improvements#168
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds robust Claude session timestamp handling and tests; gates private-host webhooks via CODEXMATE_ALLOW_PRIVATE_WEBHOOK; tweaks web server CSP and applies API rate limits only to non-loopback requests; refactors settings panel into General/Data tabs, updates styles, and cleans navigation URLs. ChangesSession timestamp reliability improvements
Server security & webhook
Settings UI & navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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: 1
🧹 Nitpick comments (1)
tests/unit/session-usage-backend.test.mjs (1)
792-807: ⚡ Quick winStrengthen coverage for the stale index
updatedAtpath.This fixture currently proves the missing-timestamp path. Add an explicitly stale index
updatedAtso the test also locks in overwrite behavior from JSONL timestamps.Proposed test tweak
fs.writeFileSync(path.join(projectDir, 'sessions-index.json'), JSON.stringify({ originalPath: '/repo/project-a', entries: [{ sessionId: 'today-session', fullPath: sessionPath, + updatedAt: '2026-04-11T22:00:00.000Z', created: '2026-04-12T00:59:00.000Z', firstPrompt: 'today claude code', messageCount: 2 }] }));🤖 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 `@tests/unit/session-usage-backend.test.mjs` around lines 792 - 807, The test fixture currently omits an explicit updatedAt in the sessions-index.json entry so it only exercises the "missing timestamp" path; modify the JSON written to include an explicit (stale) updatedAt value (e.g., earlier than the JSONL message timestamps) for the entry with sessionId 'today-session' so listClaudeSessions(...) will detect and be overwritten by the JSONL timestamps; update the sessions-index.json entry that contains fullPath: sessionPath and firstPrompt: 'today claude code' to include this stale updatedAt and keep the existing assertions against result[0].updatedAt and result[0].source to ensure overwrite behavior is locked in.
🤖 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 4817-4819: The tail-scan branch updates updatedAt from
record.timestamp but never backfills createdAt when it is still unset; modify
the block in which record.timestamp is processed so that if createdAt is
falsy/empty it is initialized using the same logic as updatedAt (i.e., call
updateLatestIso(createdAt, record.timestamp)) while keeping the existing
updateLatestIso(updatedAt, record.timestamp) call; locate the code handling
record.timestamp (referencing variables createdAt, updatedAt and the function
updateLatestIso) and add the createdAt assignment before or alongside the
updatedAt update.
---
Nitpick comments:
In `@tests/unit/session-usage-backend.test.mjs`:
- Around line 792-807: The test fixture currently omits an explicit updatedAt in
the sessions-index.json entry so it only exercises the "missing timestamp" path;
modify the JSON written to include an explicit (stale) updatedAt value (e.g.,
earlier than the JSONL message timestamps) for the entry with sessionId
'today-session' so listClaudeSessions(...) will detect and be overwritten by the
JSONL timestamps; update the sessions-index.json entry that contains fullPath:
sessionPath and firstPrompt: 'today claude code' to include this stale updatedAt
and keep the existing assertions against result[0].updatedAt and
result[0].source to ensure overwrite behavior is locked in.
🪄 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: 1f1fc186-b31f-4c86-b154-93e2e3776ad7
📒 Files selected for processing (2)
cli.jstests/unit/session-usage-backend.test.mjs
📜 Review details
🔇 Additional comments (1)
cli.js (1)
4238-4240: LGTM!Also applies to: 4748-4749, 4781-4786
Collaborator
Author
|
Updated PR 168 with a fix for the Web UI white-screen/load failure. What changed:
Validation performed:
|
Collaborator
Author
|
Follow-up update pushed for the remaining CodeRabbit timestamp feedback. What changed:
Validation performed:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli.js (1)
10648-10678: ⚡ Quick winReuse the shared auth helpers in this gate.
This branch now maintains its own loopback detection and token parsing even though
isLoopbackRemoteAddress()andassertRequestAuthorized()already define that policy. Keeping two copies here is likely to drift, and then throttling/auth decisions stop matching for the same client address.♻️ Proposed cleanup
const remoteAddr = req && req.socket ? req.socket.remoteAddress : ''; - const isLoopback = !remoteAddr - || remoteAddr === '127.0.0.1' - || remoteAddr === '::1' - || remoteAddr === '::ffff:127.0.0.1'; + const isLoopback = !remoteAddr || isLoopbackRemoteAddress(remoteAddr); if (!isLoopback) { const rateLimitKey = (remoteAddr || 'unknown') + ':' + requestPath; if (!checkRateLimit(rateLimitKey)) { res.writeHead(429, { 'Content-Type': 'application/json; charset=utf-8', 'Retry-After': '60' }); res.end(JSON.stringify({ error: 'Rate limit exceeded' })); return; } - const expected = typeof process.env.CODEXMATE_HTTP_TOKEN === 'string' - ? process.env.CODEXMATE_HTTP_TOKEN.trim() - : ''; - if (!expected) { - sendJson(403, { - error: 'Remote access is disabled (set CODEXMATE_HTTP_TOKEN or use --host 127.0.0.1)' - }); - return; - } - const headers = req && req.headers && typeof req.headers === 'object' ? req.headers : {}; - const rawAuth = typeof headers.authorization === 'string' ? headers.authorization.trim() : ''; - const match = rawAuth ? rawAuth.match(/^bearer\s+(.+)$/i) : null; - const actual = match && match[1] - ? match[1].trim() - : (rawAuth ? rawAuth : (typeof headers['x-codexmate-token'] === 'string' ? String(headers['x-codexmate-token']).trim() : '')); - if (!actual || !safeTimingEqual(actual, expected)) { - sendJson(401, { error: 'Unauthorized' }); + const auth = assertRequestAuthorized(req, res); + if (!auth.ok) { return; } }🤖 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 10648 - 10678, The gate duplicates loopback detection and token parsing; replace the manual logic with the shared helpers: call isLoopbackRemoteAddress(req) to determine loopback and use assertRequestAuthorized(req, requestPath, res) (or its underlying authorization check) to validate tokens and handle 401/403 responses so behavior stays consistent; maintain existing rate-limiting by computing rateLimitKey from req.socket.remoteAddress and requestPath and call checkRateLimit(rateLimitKey) as before (returning the 429 response via sendJson if limit exceeded), removing the duplicated remoteAddr/isLoopback/token-parsing/safeTimingEqual blocks and relying on the centralized functions instead.
🤖 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`:
- Line 10620: The Content-Security-Policy header currently adds 'unsafe-eval' to
permit the full Vue runtime, which weakens CSP; revert the UI bundle to the
precompiled/runtime-only Vue build and remove 'unsafe-eval' from the header
value in the place that sets the 'Content-Security-Policy' string so the header
becomes e.g. "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src
'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self' ws: wss:";
ensure the server serves the prebuilt runtime-only bundle (replace any reference
to the full Vue runtime with the runtime-only bundle path used by the build
system) so pages work without needing 'unsafe-eval'.
In `@web-ui/partials/index/panel-settings.html`:
- Around line 14-31: Add keyboard navigation to the tab buttons so arrow keys
move focus and switch tabs: add an `@keydown` handler to each tab button
(alongside the existing `@click`) that calls a new method like
onSettingsTabKeydown(event, 'general'|'data'), and implement
onSettingsTabKeydown to listen for ArrowLeft/ArrowUp and ArrowRight/ArrowDown
(optionally Home/End) to compute the next tab id, call
onSettingsTabClick(nextTab) to update settingsTab, then programmatically focus
the target button element (e.g., document.getElementById('settings-tab-data') or
'settings-tab-general'); keep the existing tabindex logic (inactive tabs stay
tabindex="-1") because programmatic focus will still work after switching.
---
Nitpick comments:
In `@cli.js`:
- Around line 10648-10678: The gate duplicates loopback detection and token
parsing; replace the manual logic with the shared helpers: call
isLoopbackRemoteAddress(req) to determine loopback and use
assertRequestAuthorized(req, requestPath, res) (or its underlying authorization
check) to validate tokens and handle 401/403 responses so behavior stays
consistent; maintain existing rate-limiting by computing rateLimitKey from
req.socket.remoteAddress and requestPath and call checkRateLimit(rateLimitKey)
as before (returning the 429 response via sendJson if limit exceeded), removing
the duplicated remoteAddr/isLoopback/token-parsing/safeTimingEqual blocks and
relying on the centralized functions instead.
🪄 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: ec1a7cd6-69ef-4e58-94bf-aa013cc6e4d0
📒 Files selected for processing (5)
cli.jslib/cli-webhook.jstests/e2e/run.jstests/unit/web-run-host.test.mjsweb-ui/partials/index/panel-settings.html
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/run.js
- tests/unit/web-run-host.test.mjs
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-settings.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (2)
web-ui/partials/index/panel-settings.html (1)
1-13: LGTM!Also applies to: 32-158
lib/cli-webhook.js (1)
81-85: LGTM!
| 'X-Content-Type-Options': 'nosniff', | ||
| 'X-Frame-Options': 'DENY', | ||
| 'Content-Security-Policy': "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self' ws: wss:" | ||
| 'Content-Security-Policy': "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self' ws: wss:" |
Contributor
There was a problem hiding this comment.
Avoid making 'unsafe-eval' the permanent CSP fix.
This restores the current Vue runtime path, but it also removes a major CSP protection for every HTML page this server returns. Because codexmate run can listen on non-loopback hosts, any future DOM-XSS bug becomes immediately script-executable. Please switch the Web UI bundle back to a precompiled/runtime-only path and keep the stricter production CSP.
🤖 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` at line 10620, The Content-Security-Policy header currently adds
'unsafe-eval' to permit the full Vue runtime, which weakens CSP; revert the UI
bundle to the precompiled/runtime-only Vue build and remove 'unsafe-eval' from
the header value in the place that sets the 'Content-Security-Policy' string so
the header becomes e.g. "default-src 'self'; script-src 'self' 'unsafe-inline';
style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self' ws:
wss:"; ensure the server serves the prebuilt runtime-only bundle (replace any
reference to the full Vue runtime with the runtime-only bundle path used by the
build system) so pages work without needing 'unsafe-eval'.
Comment on lines
+14
to
+31
| <button | ||
| id="settings-tab-general" | ||
| type="button" | ||
| role="tab" | ||
| aria-controls="settings-panel-general" | ||
| :aria-selected="settingsTab === 'general'" | ||
| :tabindex="settingsTab === 'general' ? 0 : -1" | ||
| :class="['segmented-option', { active: settingsTab === 'general' }]" | ||
| @click="onSettingsTabClick('general')">{{ t('settings.tab.general') }}</button> | ||
| <button | ||
| id="settings-tab-data" | ||
| type="button" | ||
| role="tab" | ||
| aria-controls="settings-panel-data" | ||
| :aria-selected="settingsTab === 'data'" | ||
| :tabindex="settingsTab === 'data' ? 0 : -1" | ||
| :class="['segmented-option', { active: settingsTab === 'data' }]" | ||
| @click="onSettingsTabClick('data')">{{ t('settings.tab.data') }}</button> |
Contributor
There was a problem hiding this comment.
Keyboard users cannot switch to the inactive settings tab.
The inactive tab is removed from tab order (tabindex="-1"), but there is no arrow-key handler to move focus between tabs. With only @click, keyboard-only users get stuck on the active tab.
Suggested minimal fix
- :tabindex="settingsTab === 'general' ? 0 : -1"
+ tabindex="0"
...
- :tabindex="settingsTab === 'data' ? 0 : -1"
+ tabindex="0"🤖 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/panel-settings.html` around lines 14 - 31, Add keyboard
navigation to the tab buttons so arrow keys move focus and switch tabs: add an
`@keydown` handler to each tab button (alongside the existing `@click`) that calls a
new method like onSettingsTabKeydown(event, 'general'|'data'), and implement
onSettingsTabKeydown to listen for ArrowLeft/ArrowUp and ArrowRight/ArrowDown
(optionally Home/End) to compute the next tab id, call
onSettingsTabClick(nextTab) to update settingsTab, then programmatically focus
the target button element (e.g., document.getElementById('settings-tab-data') or
'settings-tab-general'); keep the existing tabindex logic (inactive tabs stay
tabindex="-1") because programmatic focus will still work after switching.
ymkiux
force-pushed
the
fix/claude-today-session-browser
branch
from
39c1d5c to
42f46e7
Compare
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/styles/settings-panel.css`:
- Around line 375-378: The CSS rule .settings-tab-header .segmented-control uses
an invalid value justify-content: stretch; update that selector to remove the
invalid property (or replace it with a valid value like
flex-start/space-between/center if specific horizontal alignment is required).
Because the segmented options already use flex: 1, removing justify-content
entirely will let the children fill available space naturally; adjust the
.settings-tab-header .segmented-control rule accordingly.
🪄 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: e141faee-6da0-42d8-a50b-9a707a4fcbe1
📒 Files selected for processing (2)
web-ui/modules/app.methods.navigation.mjsweb-ui/styles/settings-panel.css
📜 Review details
🔇 Additional comments (12)
web-ui/styles/settings-panel.css (11)
1-4: LGTM!
6-15: LGTM!
17-52: LGTM!
54-109: LGTM!
111-141: LGTM!
143-192: LGTM!
194-237: LGTM!
239-305: LGTM!
306-328: LGTM!
330-367: LGTM!
380-415: LGTM!web-ui/modules/app.methods.navigation.mjs (1)
422-435: LGTM!
ymkiux
force-pushed
the
fix/claude-today-session-browser
branch
2 times, most recently
from
bb93bbd to
6149300
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/styles/settings-panel.css (1)
397-401: ⚡ Quick winClarify flexbox main-axis intent: replace
justify-content: stretchwithflex-start
justify-content: stretchis valid, but in flexbox it behaves likeflex-start(so it won’t actually stretch items along the main axis). Switching toflex-startavoids confusion for the mobile action containers.🔧 Proposed fix
.settings-card-action, .settings-card-actions { width: 100%; - justify-content: stretch; + justify-content: flex-start; }🤖 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/settings-panel.css` around lines 397 - 401, Replace the ambiguous flexbox rule on .settings-card-action and .settings-card-actions by changing justify-content from "stretch" to "flex-start": locate the CSS selectors .settings-card-action and .settings-card-actions and update the justify-content property to flex-start so the main-axis alignment is explicit and behaves correctly on mobile action containers.
🤖 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.
Nitpick comments:
In `@web-ui/styles/settings-panel.css`:
- Around line 397-401: Replace the ambiguous flexbox rule on
.settings-card-action and .settings-card-actions by changing justify-content
from "stretch" to "flex-start": locate the CSS selectors .settings-card-action
and .settings-card-actions and update the justify-content property to flex-start
so the main-axis alignment is explicit and behaves correctly on mobile action
containers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddb016c7-0d4a-48b9-a8fa-7b726debd2d2
📒 Files selected for processing (2)
web-ui/modules/app.methods.navigation.mjsweb-ui/styles/settings-panel.css
📜 Review details
🔇 Additional comments (2)
web-ui/styles/settings-panel.css (1)
375-378:justify-content: stretchremains in the segmented control mobile rule.Already reported previously; this is still present at Line 377.
web-ui/modules/app.methods.navigation.mjs (1)
422-435: LGTM!
ymkiux
force-pushed
the
fix/claude-today-session-browser
branch
from
6149300 to
f506280
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web-ui/styles/settings-panel.css (2)
397-401:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvalid CSS property value.
Line 400 uses
justify-content: stretch, which is invalid. Thestretchvalue is only valid foralign-itemsandalign-content, notjustify-content.Valid
justify-contentvalues include:flex-start,flex-end,center,space-between,space-around,space-evenly.Since the action buttons already use
flex: 1(line 404), they will naturally fill the container. You can either remove this property or use a valid alignment value if specific horizontal distribution is needed.🔧 Proposed fix
.settings-card-action, .settings-card-actions { width: 100%; - justify-content: stretch; }🤖 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/settings-panel.css` around lines 397 - 401, The CSS rule for .settings-card-action and .settings-card-actions uses an invalid value "justify-content: stretch"; update this by removing that property (since .settings-card-action children already have flex: 1) or replace it with a valid value such as "justify-content: space-between" or "justify-content: center" depending on desired horizontal distribution; edit the selector .settings-card-action, .settings-card-actions to remove or change the invalid property accordingly.
256-328:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a visible focus indicator for keyboard navigation.
The
.settings-card-actionbuttons lack a:focusor:focus-visiblestate. When users navigate with the keyboard, focused buttons are not visually distinguished, which fails WCAG 2.4.7 (Focus Visible).♿ Proposed fix to add focus state
.settings-card-action:disabled { opacity: 0.5; cursor: not-allowed; } + +.settings-card-action:focus-visible { + outline: 2px solid var(--color-brand); + outline-offset: 2px; +} .settings-card-action--active {🤖 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/settings-panel.css` around lines 256 - 328, Add a visible keyboard focus style: update the .settings-card-action selector set to include a :focus-visible (and optionally :focus) rule that applies a clear high-contrast outline or ring and visible box-shadow (for example using outline: 2px solid var(--color-brand) or box-shadow: 0 0 0 3px rgba(...)) so focused buttons are distinct; also add matching :focus-visible rules for .settings-card-action--active and .settings-card-action--danger to ensure active/danger variants show an accessible focus indicator without breaking their background/border colors.
♻️ Duplicate comments (1)
web-ui/styles/settings-panel.css (1)
375-378:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvalid CSS property value (duplicate).
Line 377 still uses
justify-content: stretch, which is invalid. As noted in a previous review,stretchis only valid foralign-itemsandalign-content, notjustify-content.Since the segmented options already have
flex: 1(line 381), removing this line will allow them to fill the available space naturally.🔧 Proposed fix
.settings-tab-header .segmented-control { width: 100%; - justify-content: stretch; }🤖 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/settings-panel.css` around lines 375 - 378, Remove the invalid CSS rule `justify-content: stretch` from the `.settings-tab-header .segmented-control` block (it’s not a valid value for justify-content); rely on the existing `flex: 1` on the segmented options to make them fill available space instead, so edit the `.settings-tab-header .segmented-control` rule to drop the `justify-content: stretch` declaration.
🤖 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/styles/settings-panel.css`:
- Around line 157-186: The toggle lacks a visible keyboard focus state; add
focus styles so keyboard users see when the control is focused by styling either
the input:focus-visible + .toggle-track and/or .toggle-track:focus-within
selectors to show a clear focus indicator (e.g., a high-contrast outline or ring
and a subtle box-shadow), and ensure the ::after knob remains visible when
focused; update the existing .settings-toggle-row .toggle-track and
.settings-toggle-row .toggle-track::after related rules to include these focus
selectors so the focus ring appears consistently without breaking the checked
transform (input:checked + .toggle-track and input:checked +
.toggle-track::after).
---
Outside diff comments:
In `@web-ui/styles/settings-panel.css`:
- Around line 397-401: The CSS rule for .settings-card-action and
.settings-card-actions uses an invalid value "justify-content: stretch"; update
this by removing that property (since .settings-card-action children already
have flex: 1) or replace it with a valid value such as "justify-content:
space-between" or "justify-content: center" depending on desired horizontal
distribution; edit the selector .settings-card-action, .settings-card-actions to
remove or change the invalid property accordingly.
- Around line 256-328: Add a visible keyboard focus style: update the
.settings-card-action selector set to include a :focus-visible (and optionally
:focus) rule that applies a clear high-contrast outline or ring and visible
box-shadow (for example using outline: 2px solid var(--color-brand) or
box-shadow: 0 0 0 3px rgba(...)) so focused buttons are distinct; also add
matching :focus-visible rules for .settings-card-action--active and
.settings-card-action--danger to ensure active/danger variants show an
accessible focus indicator without breaking their background/border colors.
---
Duplicate comments:
In `@web-ui/styles/settings-panel.css`:
- Around line 375-378: Remove the invalid CSS rule `justify-content: stretch`
from the `.settings-tab-header .segmented-control` block (it’s not a valid value
for justify-content); rely on the existing `flex: 1` on the segmented options to
make them fill available space instead, so edit the `.settings-tab-header
.segmented-control` rule to drop the `justify-content: stretch` declaration.
🪄 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: 552db908-768e-4cf6-9098-878767964ee3
📒 Files selected for processing (2)
web-ui/modules/app.methods.navigation.mjsweb-ui/styles/settings-panel.css
📜 Review details
🔇 Additional comments (9)
web-ui/styles/settings-panel.css (8)
1-4: LGTM!
6-15: LGTM!
17-52: LGTM!
54-100: LGTM!
102-141: LGTM!
239-245: LGTM!
330-367: LGTM!
408-414: LGTM!web-ui/modules/app.methods.navigation.mjs (1)
422-435: LGTM!
ymkiux
force-pushed
the
fix/claude-today-session-browser
branch
3 times, most recently
from
35ebf82 to
4993709
Compare
ymkiux
force-pushed
the
fix/claude-today-session-browser
branch
from
4993709 to
43e375e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/styles/settings-panel.css (1)
375-378:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
justify-content: stretchis invalid in flexbox.This was previously flagged and appears to be reintroduced;
stretchis not a validjustify-contentvalue.Suggested fix
.settings-tab-header .segmented-control { width: 100%; - justify-content: stretch; }🤖 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/settings-panel.css` around lines 375 - 378, The CSS uses an invalid flexbox value `justify-content: stretch` in the selector `.settings-tab-header .segmented-control`; replace it with a valid property depending on intent—if you wanted cross-axis stretching, remove `justify-content` and add `align-items: stretch`, otherwise use a valid main-axis value such as `justify-content: space-between` (or `flex-start`/`center`) to distribute items; update the `.settings-tab-header .segmented-control` rule accordingly.
🤖 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/styles/settings-panel.css`:
- Around line 153-155: The rule targeting .settings-toggle-row
input[type="checkbox"] currently uses display: none which removes keyboard focus
and breaks accessibility; replace it with a visually-hidden pattern that keeps
the input in the DOM and focusable (e.g., visually-hide via positioning, size
and clipping while preserving pointer/focus) so screen readers and keyboard
users can interact with the checkbox; update the CSS for .settings-toggle-row
input[type="checkbox"] to use the visually-hidden approach instead of
display:none and ensure any custom visual toggle uses aria-checked/for
attributes to reflect the hidden input's state.
---
Duplicate comments:
In `@web-ui/styles/settings-panel.css`:
- Around line 375-378: The CSS uses an invalid flexbox value `justify-content:
stretch` in the selector `.settings-tab-header .segmented-control`; replace it
with a valid property depending on intent—if you wanted cross-axis stretching,
remove `justify-content` and add `align-items: stretch`, otherwise use a valid
main-axis value such as `justify-content: space-between` (or
`flex-start`/`center`) to distribute items; update the `.settings-tab-header
.segmented-control` rule accordingly.
🪄 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: 514b3dd8-bfc6-481d-b854-6121e2541674
📒 Files selected for processing (2)
web-ui/modules/app.methods.navigation.mjsweb-ui/styles/settings-panel.css
📜 Review details
🔇 Additional comments (1)
web-ui/modules/app.methods.navigation.mjs (1)
422-435: LGTM!
Comment on lines
+153
to
155
| .settings-toggle-row input[type="checkbox"] { | ||
| display: none; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the checkbox focusable for keyboard users.
Using display: none on the checkbox removes native focus/keyboard interaction for the control. Use a visually-hidden pattern instead so the input remains accessible.
Suggested fix
.settings-toggle-row input[type="checkbox"] {
- display: none;
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ margin: -1px;
+ padding: 0;
+ border: 0;
+ overflow: hidden;
+ clip: rect(0 0 0 0);
+ clip-path: inset(50%);
+ white-space: nowrap;
}📝 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
| .settings-toggle-row input[type="checkbox"] { | |
| display: none; | |
| } | |
| .settings-toggle-row input[type="checkbox"] { | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| margin: -1px; | |
| padding: 0; | |
| border: 0; | |
| overflow: hidden; | |
| clip: rect(0 0 0 0); | |
| clip-path: inset(50%); | |
| white-space: nowrap; | |
| } |
🤖 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/settings-panel.css` around lines 153 - 155, The rule targeting
.settings-toggle-row input[type="checkbox"] currently uses display: none which
removes keyboard focus and breaks accessibility; replace it with a
visually-hidden pattern that keeps the input in the DOM and focusable (e.g.,
visually-hide via positioning, size and clipping while preserving pointer/focus)
so screen readers and keyboard users can interact with the checkbox; update the
CSS for .settings-toggle-row input[type="checkbox"] to use the visually-hidden
approach instead of display:none and ensure any custom visual toggle uses
aria-checked/for attributes to reflect the hidden input's state.
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
2 participants
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
Technical Changes
web-ui/modules/sessions.mjs: Keep timestamps fresh, improve fallbackweb-ui/modules/app.methods.navigation.mjs: URL cleanup inswitchMainTab()web-ui/styles/settings-panel.css: Minimalist design improvementsVersion
🤖 Generated with Claude Code