feat: refine web-ui layout hierarchy with dory-inspired composition#79
feat: refine web-ui layout hierarchy with dory-inspired composition#79
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved the CLI install FAB/modal and status inspector, restructured header and side-rail markup with new docs main tab and relabeled nav items, unified panel header/topbar and status-strip chips, overhauled layout CSS to a 2-column flat rail and hidden scrollbars, added Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Script as reset-main.js
participant Git as Git/Remote
participant FS as Local Workspace
rect rgba(220,220,255,0.5)
Dev->>Script: Run `npm run reset` [optional: -- <PR>]
Script->>Dev: Prompt for PR number if not provided
alt No PR supplied
Script->>Git: fetch origin main
Script->>FS: git reset --hard (discard local changes)
Script->>FS: git clean -fd (remove untracked)
Script->>Git: checkout main
Script->>Git: reset --hard origin/main
Script->>FS: git clean -fd
else PR number supplied
Script->>Git: fetch pull/<n>/head:refs/remotes/pr-snapshot/<n>
Script->>FS: git reset --hard
Script->>FS: git clean -fd
Script->>Git: checkout -B pr-<n> refs/remotes/pr-snapshot/<n>
Script->>Git: reset --hard refs/remotes/pr-snapshot/<n>
Script->>FS: git clean -fd
end
Script->>Script: validateFinalState (branch/ref/diffs/untracked)
Script->>Dev: print status/result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
awsl233777
force-pushed
the
feat/dory-inspired-layout
branch
from
76fb354 to
5dbee84
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web-ui/partials/index/panel-sessions.html (2)
204-205:⚠️ Potential issue | 🟠 MajorInclude
session.matchfields insession-itemv-memodependencies.Line 204's v-memo omits fields rendered on lines 255–259. If search results change while other session data (title, count, timestamp) remains stable, the match badge and snippet will remain stale.
🔧 Proposed fix
- v-memo="[activeSessionExportKey === getSessionExportKey(session), session.messageCount, session.updatedAt, session.title, session.sourceLabel, isSessionPinned(session), sessionsLoading]" + v-memo="[activeSessionExportKey === getSessionExportKey(session), session.messageCount, session.updatedAt, session.title, session.sourceLabel, isSessionPinned(session), sessionsLoading, session.match && session.match.hit, session.match && session.match.count, session.match && session.match.primarySnippet]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/partials/index/panel-sessions.html` around lines 204 - 205, The v-memo dependency array for the session-item is missing the session.match fields, so search-result changes can leave the match badge/snippet stale; update the v-memo (the array currently including activeSessionExportKey === getSessionExportKey(session), session.messageCount, session.updatedAt, session.title, session.sourceLabel, isSessionPinned(session), sessionsLoading) to also include the relevant session.match properties (e.g., session.match and/or session.match.* as used when rendering the badge/snippet) so that changes to match data invalidate the memo and re-render the session item.
349-360:⚠️ Potential issue | 🟠 MajorAdd missing search-related dependencies to
v-memo.The memoization on line 349 excludes
msg.isSearchMatchandmsg.matchSnippet, which are rendered on lines 359–360. When these properties change while other memoized values remain unchanged, Vue will skip the subtree update, preventing the search highlight class and match chip from rendering.- v-memo="[msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole]" + v-memo="[msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole, msg.isSearchMatch, msg.matchSnippet]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/partials/index/panel-sessions.html` around lines 349 - 360, v-memo currently tracks [msg.text, msg.timestamp, msg.roleLabel, msg.normalizedRole] but omits the search-related fields so Vue can skip re-rendering the message subtree when only search state changes; update the v-memo dependency array on the element using getRecordRenderKey/getSessionMessageRefBinder to include msg.isSearchMatch and msg.matchSnippet (e.g., add those two properties to the array) so the session-msg-content class and the match-chip render when search properties change.
🧹 Nitpick comments (1)
web-ui/partials/index/layout-header.html (1)
38-39: Consolidate repeatedmainTabcopy mapping into computed props.These nested ternaries duplicate label/title/subtitle mapping in multiple places, which is easy to drift over time. Reuse shared computed values (e.g., existing
mainLabelText) and add dedicated computed strings for header title/subtitle.Also applies to: 261-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/partials/index/layout-header.html` around lines 38 - 39, The template uses nested ternaries on mainTab to render labels (e.g., the span using mainTab === 'config' ? '配置中心' : ...) which duplicates mapping logic; extract this mapping into shared computed properties (e.g., mainLabelText, mainHeaderTitle, mainHeaderSubtitle) in the component and replace all inline ternaries (including the other occurrences around the file mentioned) with those computed strings; update references in the template to use mainLabelText/mainHeaderTitle/mainHeaderSubtitle so the mapping lives in one place and can be reused/maintained easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/logic.sessions.mjs`:
- Around line 118-126: The loop currently returns only the record key after
finding a matching snippet, which loses which snippet matched; update the logic
inside the loop (where buildSessionMessageSearchText(message) and
normalizedSnippets are used) to capture the actual matching snippet (e.g., the
snippet variable from normalizedSnippets.some or by using
normalizedSnippets.find/snippet) and return both the record key and the matched
snippet (or return an object with { recordKey, matchedSnippet }) instead of just
recordKey so callers no longer must rely on primarySnippet fallbacks.
In `@web-ui/modules/app.methods.session-browser.mjs`:
- Around line 451-454: The returned frozen message only updates matchSnippet and
leaves the existing isSearchMatch flag from normalizeSessionMessage(), so
preview branches keyed on isSearchMatch stay incorrect; update the object
returned in the branch that sets matchSnippet (the code creating
Object.freeze({...message, matchSnippet: match.primarySnippet})) to also set
isSearchMatch (e.g., set isSearchMatch: true when match.primarySnippet is
present) so normalizeSessionMessage()/the jump-to message and its injected
snippet remain in sync.
In `@web-ui/partials/index/layout-header.html`:
- Around line 27-32: The external anchor element with class "hero-link-pill" and
title "打开 GitHub 仓库" uses target="_blank" but only rel="noopener"; update its
rel attribute to include "noreferrer" (e.g., change rel="noopener" to
rel="noopener noreferrer") to prevent referrer leakage when opening the GitHub
link in a new tab.
In `@web-ui/styles/navigation-panels.css`:
- Around line 34-37: The media query override still applies flexbox properties
to .status-strip which now uses CSS Grid; update the responsive rule that
targets .status-strip (in responsive.css) to use grid rules instead—replace
flex-direction/flex-wrap with a grid-template-columns: 1fr (or repeat(1, 1fr))
so chips collapse to a single column at the 720px breakpoint and preserve any
gap setting (gap: 12px) to keep spacing consistent.
---
Outside diff comments:
In `@web-ui/partials/index/panel-sessions.html`:
- Around line 204-205: The v-memo dependency array for the session-item is
missing the session.match fields, so search-result changes can leave the match
badge/snippet stale; update the v-memo (the array currently including
activeSessionExportKey === getSessionExportKey(session), session.messageCount,
session.updatedAt, session.title, session.sourceLabel, isSessionPinned(session),
sessionsLoading) to also include the relevant session.match properties (e.g.,
session.match and/or session.match.* as used when rendering the badge/snippet)
so that changes to match data invalidate the memo and re-render the session
item.
- Around line 349-360: v-memo currently tracks [msg.text, msg.timestamp,
msg.roleLabel, msg.normalizedRole] but omits the search-related fields so Vue
can skip re-rendering the message subtree when only search state changes; update
the v-memo dependency array on the element using
getRecordRenderKey/getSessionMessageRefBinder to include msg.isSearchMatch and
msg.matchSnippet (e.g., add those two properties to the array) so the
session-msg-content class and the match-chip render when search properties
change.
---
Nitpick comments:
In `@web-ui/partials/index/layout-header.html`:
- Around line 38-39: The template uses nested ternaries on mainTab to render
labels (e.g., the span using mainTab === 'config' ? '配置中心' : ...) which
duplicates mapping logic; extract this mapping into shared computed properties
(e.g., mainLabelText, mainHeaderTitle, mainHeaderSubtitle) in the component and
replace all inline ternaries (including the other occurrences around the file
mentioned) with those computed strings; update references in the template to use
mainLabelText/mainHeaderTitle/mainHeaderSubtitle so the mapping lives in one
place and can be reused/maintained easily.
🪄 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: 446cd90c-891d-4d65-b58a-81b53a125e2b
📒 Files selected for processing (12)
tests/unit/session-search-ui.test.mjsweb-ui/logic.sessions.mjsweb-ui/modules/app.computed.session.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/panel-sessions.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/sessions-list.cssweb-ui/styles/sessions-preview.cssweb-ui/styles/titles-cards.css
📜 Review details
🔇 Additional comments (6)
web-ui/styles/sessions-list.css (1)
381-405: Looks good for match-row visual hierarchy.The new match container and count pill are consistent with existing session item styling and keep snippet content bounded.
web-ui/styles/sessions-preview.css (1)
252-268: Match highlight styles are clean and scoped.The added classes integrate well with existing message blocks and keep visual emphasis lightweight.
web-ui/modules/app.computed.session.mjs (1)
34-45: Session match decoration path looks correct.
sortedSessionsList()now consistently exposes normalized match payloads for both pinned and non-pinned branches.Also applies to: 51-54
web-ui/styles/titles-cards.css (1)
5-13: Visual refinements are coherent with the new hierarchy.Title scale, subtitle rhythm, and card surface updates read consistently with the new hero/panel composition.
Also applies to: 25-30, 88-103, 238-238
web-ui/styles/responsive.css (1)
16-31: Responsive breakpoint updates look aligned.The new hero/main-panel adjustments across 1280/960/720/540 breakpoints are coherent and maintain layout continuity.
Also applies to: 49-79, 113-123, 174-188
tests/unit/session-search-ui.test.mjs (1)
18-121: Good targeted test coverage for search-hit UI plumbing.The suite exercises normalization, computed list decoration, and message-level annotation/jump behavior end-to-end for the new feature.
web-ui/logic.sessions.mjs
Outdated
| for (let index = 0; index < messages.length; index += 1) { | ||
| const message = messages[index]; | ||
| const haystack = buildSessionMessageSearchText(message).toLowerCase(); | ||
| if (!haystack) continue; | ||
| if (normalizedSnippets.some((snippet) => haystack.includes(snippet))) { | ||
| const recordKey = message && Number.isInteger(message.recordLineIndex) && message.recordLineIndex >= 0 | ||
| ? `record-${message.recordLineIndex}` | ||
| : `record-fallback-${index}-${message && message.timestamp ? message.timestamp : ''}`; | ||
| return recordKey; |
There was a problem hiding this comment.
Return the matched snippet, not just the key.
This loop throws away which snippet actually matched. When a hit contains multiple snippets, the caller can only fall back to primarySnippet, so the preview can end up showing/highlighting text that is not in the matched message.
Suggested direction
- if (normalizedSnippets.some((snippet) => haystack.includes(snippet))) {
+ const matchedSnippet = normalizedSnippets.find((snippet) => haystack.includes(snippet));
+ if (matchedSnippet) {
const recordKey = message && Number.isInteger(message.recordLineIndex) && message.recordLineIndex >= 0
? `record-${message.recordLineIndex}`
: `record-fallback-${index}-${message && message.timestamp ? message.timestamp : ''}`;
- return recordKey;
+ return { key: recordKey, snippet: matchedSnippet };
}📝 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.
| for (let index = 0; index < messages.length; index += 1) { | |
| const message = messages[index]; | |
| const haystack = buildSessionMessageSearchText(message).toLowerCase(); | |
| if (!haystack) continue; | |
| if (normalizedSnippets.some((snippet) => haystack.includes(snippet))) { | |
| const recordKey = message && Number.isInteger(message.recordLineIndex) && message.recordLineIndex >= 0 | |
| ? `record-${message.recordLineIndex}` | |
| : `record-fallback-${index}-${message && message.timestamp ? message.timestamp : ''}`; | |
| return recordKey; | |
| for (let index = 0; index < messages.length; index += 1) { | |
| const message = messages[index]; | |
| const haystack = buildSessionMessageSearchText(message).toLowerCase(); | |
| if (!haystack) continue; | |
| const matchedSnippet = normalizedSnippets.find((snippet) => haystack.includes(snippet)); | |
| if (matchedSnippet) { | |
| const recordKey = message && Number.isInteger(message.recordLineIndex) && message.recordLineIndex >= 0 | |
| ? `record-${message.recordLineIndex}` | |
| : `record-fallback-${index}-${message && message.timestamp ? message.timestamp : ''}`; | |
| return { key: recordKey, snippet: matchedSnippet }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/logic.sessions.mjs` around lines 118 - 126, The loop currently returns
only the record key after finding a matching snippet, which loses which snippet
matched; update the logic inside the loop (where
buildSessionMessageSearchText(message) and normalizedSnippets are used) to
capture the actual matching snippet (e.g., the snippet variable from
normalizedSnippets.some or by using normalizedSnippets.find/snippet) and return
both the record key and the matched snippet (or return an object with {
recordKey, matchedSnippet }) instead of just recordKey so callers no longer must
rely on primarySnippet fallbacks.
| return Object.freeze({ | ||
| ...message, | ||
| matchSnippet: match.primarySnippet | ||
| }); |
There was a problem hiding this comment.
Keep isSearchMatch in sync with the injected snippet.
Line 453 only updates matchSnippet, so the existing isSearchMatch: false from normalizeSessionMessage() survives. Any preview branch/class keyed off isSearchMatch will stay off for the jumped-to message.
Suggested fix
return Object.freeze({
...message,
- matchSnippet: match.primarySnippet
+ matchSnippet: match.primarySnippet,
+ isSearchMatch: true
});📝 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.
| return Object.freeze({ | |
| ...message, | |
| matchSnippet: match.primarySnippet | |
| }); | |
| return Object.freeze({ | |
| ...message, | |
| matchSnippet: match.primarySnippet, | |
| isSearchMatch: true | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.methods.session-browser.mjs` around lines 451 - 454, The
returned frozen message only updates matchSnippet and leaves the existing
isSearchMatch flag from normalizeSessionMessage(), so preview branches keyed on
isSearchMatch stay incorrect; update the object returned in the branch that sets
matchSnippet (the code creating Object.freeze({...message, matchSnippet:
match.primarySnippet})) to also set isSearchMatch (e.g., set isSearchMatch: true
when match.primarySnippet is present) so normalizeSessionMessage()/the jump-to
message and its injected snippet remain in sync.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 46-50: The current .app-shell grid-template-columns uses two large
fixed tracks (272px and 332px) causing the center to shrink too much on ~1024px
viewports; update the .app-shell rule to make the side columns flexible (e.g.,
use minmax with smaller minimums like minmax(160px,272px) and
minmax(160px,332px) or switch to minmax(200px, 1fr) for sidebars) and/or add a
media query at ~1024px to collapse or stack side panels earlier so the center
panel (.main-panel) retains sufficient width after its padding is applied.
In `@web-ui/styles/titles-cards.css`:
- Around line 24-29: The .subtitle rule sets margin-bottom: 0 but the responsive
media query in responsive.css (the `@media` (max-width: 720px) block) re-applies
margin-bottom: 16px, so mobile keeps the old spacing; update the responsive.css
entry that targets .subtitle inside the max-width: 720px media query to either
remove the margin-bottom override or set margin-bottom: 0 so the compact
subtitle spacing is preserved on phones (look for the .subtitle selector in the
responsive.css media query and change its margin-bottom).
🪄 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: 060a9834-cd05-4d0f-98a9-8d2078d3449e
📒 Files selected for processing (5)
web-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/titles-cards.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/styles/responsive.css
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/partials/index/layout-header.html
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)
cmd/reset-main.js (1)
3-8:⚠️ Potential issue | 🟡 MinorHeader comment is now out of sync with actual steps.
The comment lists a 5-step flow but omits the new "discard local changes" step (step 2). Update it to reflect the current behavior.
📝 Proposed fix
/** * Reset working tree to origin/main: * - fetch origin/main + * - discard local changes * - checkout main * - hard reset to origin/main * - clean untracked files/dirs * - print final status * * Cross-platform: requires Node.js and git in PATH. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/reset-main.js` around lines 3 - 8, Update the top-of-file header comment in the reset script to reflect the actual steps performed by the script: insert the missing "discard local changes" step as step 2 (so the sequence now lists: fetch origin/main, discard local changes, checkout main, hard reset to origin/main, clean untracked files/dirs, print final status) so the header stays accurate with the behavior implemented in this file.
🧹 Nitpick comments (3)
cmd/reset-main.js (1)
40-41: Consider removing redundantgit clean -fd.
git clean -fdis already run at step 2 (line 32), and steps 3–4 (checkout,reset --hard) do not create untracked files. The second clean is effectively a no-op.♻️ Proposed simplification
- console.log('[5/6] Remove untracked files'); - run('git clean -fd'); - - console.log('[6/6] Final status'); + console.log('[5/5] Final status'); run('git status --short --branch');If you keep the second clean for extra defensiveness (e.g., in case of future step additions), that's also fine—just note the intent in a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/reset-main.js` around lines 40 - 41, The second invocation of git clean -fd (the console.log('[5/6] Remove untracked files') and run('git clean -fd')) is redundant since git clean -fd is already run earlier; remove these two lines to simplify the script, or if you want to keep it for defensive reasons, retain run('git clean -fd') but add a clear comment immediately above it explaining the defensive intent (e.g., "keep extra clean for future steps that might create untracked files") so the redundancy is explicit.web-ui/styles/sessions-list.css (1)
187-192: Duplicate rule for.session-preview-scroll::-webkit-scrollbar.This combined selector defines WebKit scrollbar styles for both
.session-listand.session-preview-scroll. However,.session-preview-scroll::-webkit-scrollbaris also separately defined insessions-preview.css(lines 19-23) with identical styles. This duplication creates a maintenance burden—if the styling needs to change, it must be updated in two places.Consider either:
- Removing
.session-preview-scrollfrom this combined selector (keeping it only insessions-preview.css)- Consolidating all scrollbar-hiding rules in a single shared utility file
♻️ Option 1: Remove duplicate selector from this file
-.session-list::-webkit-scrollbar, -.session-preview-scroll::-webkit-scrollbar { +.session-list::-webkit-scrollbar { width: 0; height: 0; display: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/sessions-list.css` around lines 187 - 192, The combined selector currently includes `.session-list::-webkit-scrollbar` and `.session-preview-scroll::-webkit-scrollbar`, but `.session-preview-scroll::-webkit-scrollbar` is already defined (identically) in sessions-preview.css; remove `.session-preview-scroll::-webkit-scrollbar` from this rule in sessions-list.css so only `.session-list::-webkit-scrollbar` remains, or alternatively move both rules into a single shared stylesheet/utility and delete the duplicate in sessions-preview.css—update references to `.session-preview-scroll::-webkit-scrollbar` accordingly to avoid duplicated scrollbar hiding rules.web-ui/styles/layout-shell.css (1)
57-71: Inconsistent use of design tokens vs. fixed values.The
.side-railnow mixes CSS variables (e.g.,var(--spacing-md)on line 59) with hardcoded values forgap,padding,border-radius, andbox-shadow. Consider using design tokens consistently for maintainability, or document why these specific values deviate from the token system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 57 - 71, The .side-rail rule mixes CSS variables and hardcoded values (gap, padding, border-radius, box-shadow, min-height) which breaks token consistency; update .side-rail to use the corresponding design tokens (e.g., replace gap: 16px, padding: 16px 12px, border-radius: 22px, box-shadow and min-height with their token variables) or add a clear comment in the rule explaining why these specific fixed values are required, referencing the selector .side-rail and the properties gap, padding, border-radius, box-shadow, and min-height so reviewers can locate and validate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 322-327: The allowlist logic for sessionsViewMode is reversed:
when parityAgainstHead is true both allowedExtraCurrentKeys and
allowedMissingCurrentKeys should be empty, and when false both should include
'sessionsViewMode'; update the assignments for allowedExtraCurrentKeys and
allowedMissingCurrentKeys (referencing those exact variable names and the symbol
'sessionsViewMode' and flag parityAgainstHead) so that HEAD parity yields empty
arrays and non-HEAD baselines allow the key in both directions.
In `@web-ui/styles/layout-shell.css`:
- Around line 408-411: Add visible hover and keyboard focus styles for
.hero-link-pill to provide interaction feedback and accessibility: implement
:hover, :focus, and preferably :focus-visible rules for .hero-link-pill to
change visual state (e.g., background or border/pill color, text-decoration or
underline, and a clear focus outline), include a smooth transition on the
color/background properties, and ensure the focus styles meet
contrast/visibility requirements so keyboard users can perceive focus on the
link.
---
Outside diff comments:
In `@cmd/reset-main.js`:
- Around line 3-8: Update the top-of-file header comment in the reset script to
reflect the actual steps performed by the script: insert the missing "discard
local changes" step as step 2 (so the sequence now lists: fetch origin/main,
discard local changes, checkout main, hard reset to origin/main, clean untracked
files/dirs, print final status) so the header stays accurate with the behavior
implemented in this file.
---
Nitpick comments:
In `@cmd/reset-main.js`:
- Around line 40-41: The second invocation of git clean -fd (the
console.log('[5/6] Remove untracked files') and run('git clean -fd')) is
redundant since git clean -fd is already run earlier; remove these two lines to
simplify the script, or if you want to keep it for defensive reasons, retain
run('git clean -fd') but add a clear comment immediately above it explaining the
defensive intent (e.g., "keep extra clean for future steps that might create
untracked files") so the redundancy is explicit.
In `@web-ui/styles/layout-shell.css`:
- Around line 57-71: The .side-rail rule mixes CSS variables and hardcoded
values (gap, padding, border-radius, box-shadow, min-height) which breaks token
consistency; update .side-rail to use the corresponding design tokens (e.g.,
replace gap: 16px, padding: 16px 12px, border-radius: 22px, box-shadow and
min-height with their token variables) or add a clear comment in the rule
explaining why these specific fixed values are required, referencing the
selector .side-rail and the properties gap, padding, border-radius, box-shadow,
and min-height so reviewers can locate and validate the change.
In `@web-ui/styles/sessions-list.css`:
- Around line 187-192: The combined selector currently includes
`.session-list::-webkit-scrollbar` and
`.session-preview-scroll::-webkit-scrollbar`, but
`.session-preview-scroll::-webkit-scrollbar` is already defined (identically) in
sessions-preview.css; remove `.session-preview-scroll::-webkit-scrollbar` from
this rule in sessions-list.css so only `.session-list::-webkit-scrollbar`
remains, or alternatively move both rules into a single shared
stylesheet/utility and delete the duplicate in sessions-preview.css—update
references to `.session-preview-scroll::-webkit-scrollbar` accordingly to avoid
duplicated scrollbar hiding rules.
🪄 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: f7c7eaf6-a91e-4979-822e-7a64c4dacb6c
📒 Files selected for processing (9)
cmd/reset-main.jstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/sessions-list.cssweb-ui/styles/sessions-preview.cssweb-ui/styles/skills-list.css
✅ Files skipped from review due to trivial changes (2)
- web-ui/app.js
- web-ui/styles/skills-list.css
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/styles/navigation-panels.css
- web-ui/partials/index/layout-header.html
📜 Review details
🔇 Additional comments (5)
cmd/reset-main.js (1)
30-32: Good addition for robustness.This pre-checkout discard step ensures the script succeeds even when starting from a different branch with uncommitted changes.
web-ui/styles/sessions-list.css (1)
171-185: LGTM on scrollbar hiding for.session-list.The cross-browser scrollbar suppression using
scrollbar-width: none(Firefox),-ms-overflow-style: none(IE/Edge Legacy), and the WebKit pseudo-element is correctly implemented. Theoverflow-y: autois preserved, maintaining scroll functionality.web-ui/styles/sessions-preview.css (1)
6-23: Scrollbar hiding implementation is correct.The cross-browser approach (
scrollbar-width: none,-ms-overflow-style: none, and WebKit pseudo-element) correctly suppresses scrollbar visibility while preserving scroll functionality viaoverflow-y: auto.Accessibility consideration: Completely hidden scrollbars remove visual affordance that content is scrollable. If users report confusion about scrollability, consider adding subtle scroll indicators (e.g., fade gradients at edges, or showing scrollbars on hover/focus).
web-ui/styles/layout-shell.css (2)
46-50: The grid layout breakpoint concern from prior review remains unaddressed.The fixed column tracks still reserve 644px before the center panel gets space. This issue was flagged in the previous review cycle.
337-367: Hero section structure looks well-organized.The new
.hero-surfacewith its gradient background, decorative::afterpseudo-element, and.hero-copyz-index layering creates a clean visual hierarchy. Theoverflow: hiddencorrectly contains the negative-positioned decoration.
| const allowedExtraCurrentKeys = parityAgainstHead ? [] : [ | ||
| 'sessionsViewMode' | ||
| ]; | ||
| const allowedMissingCurrentKeys = parityAgainstHead ? [ | ||
| 'sessionsViewMode' | ||
| ] : []; |
There was a problem hiding this comment.
Fix reversed allowlist handling for sessionsViewMode parity drift.
At Line 325, strict HEAD parity currently allows sessionsViewMode as missing, which can hide a real regression. In HEAD mode both allowlists should be empty; for non-HEAD baselines, drift tolerance should allow this key in both directions.
Proposed fix
const allowedExtraCurrentKeys = parityAgainstHead ? [] : [
'sessionsViewMode'
];
-const allowedMissingCurrentKeys = parityAgainstHead ? [
- 'sessionsViewMode'
-] : [];
+const allowedMissingCurrentKeys = parityAgainstHead ? [] : [
+ 'sessionsViewMode'
+];📝 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.
| const allowedExtraCurrentKeys = parityAgainstHead ? [] : [ | |
| 'sessionsViewMode' | |
| ]; | |
| const allowedMissingCurrentKeys = parityAgainstHead ? [ | |
| 'sessionsViewMode' | |
| ] : []; | |
| const allowedExtraCurrentKeys = parityAgainstHead ? [] : [ | |
| 'sessionsViewMode' | |
| ]; | |
| const allowedMissingCurrentKeys = parityAgainstHead ? [] : [ | |
| 'sessionsViewMode' | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/web-ui-behavior-parity.test.mjs` around lines 322 - 327, The
allowlist logic for sessionsViewMode is reversed: when parityAgainstHead is true
both allowedExtraCurrentKeys and allowedMissingCurrentKeys should be empty, and
when false both should include 'sessionsViewMode'; update the assignments for
allowedExtraCurrentKeys and allowedMissingCurrentKeys (referencing those exact
variable names and the symbol 'sessionsViewMode' and flag parityAgainstHead) so
that HEAD parity yields empty arrays and non-HEAD baselines allow the key in
both directions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web-ui/styles/layout-shell.css (1)
213-230: Give.github-badgean explicit keyboard focus state too.The hero pills now get custom focus treatment, but this link still only defines hover feedback. Matching it with a
:focus-visiblerule would keep keyboard navigation consistent on the rail.♿ One possible follow-up
.github-badge:hover { border-color: rgba(201, 94, 75, 0.5); color: var(--color-text-primary); transform: translateY(-1px); box-shadow: 0 6px 12px rgba(27, 23, 20, 0.08); } + +.github-badge:focus-visible { + outline: 3px solid rgba(201, 94, 75, 0.22); + outline-offset: 2px; + border-color: rgba(201, 94, 75, 0.54); + color: var(--color-text-primary); +}Also applies to: 245-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 213 - 230, Add an explicit keyboard focus state for the .github-badge selector: locate the .github-badge rule and add a matching .github-badge:focus-visible (and optionally .github-badge:focus for broader support) rule that mirrors the hover styling (e.g., visible outline or box-shadow, color/ background changes) while preserving existing border-radius and min-width; ensure the transition and contrast remain so keyboard users get a clear, consistent focus indication with the other hero pills.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 395-406: The hero pill styles (.hero-link-pill and
.hero-summary-pill) lack overflow handling and can expand the row when inner
text is long; add min-width: 0 to both pill containers and apply truncation
rules (overflow: hidden; text-overflow: ellipsis; white-space: nowrap) to the
value text inside the pill (e.g., the inner span or the element that holds the
repo/area label — similar to .github-badge-text) so long labels are clipped
instead of growing the row; update the corresponding rules for both selectors
and any inner span used to render the pill content.
---
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 213-230: Add an explicit keyboard focus state for the
.github-badge selector: locate the .github-badge rule and add a matching
.github-badge:focus-visible (and optionally .github-badge:focus for broader
support) rule that mirrors the hover styling (e.g., visible outline or
box-shadow, color/ background changes) while preserving existing border-radius
and min-width; ensure the transition and contrast remain so keyboard users get a
clear, consistent focus indication with the other hero pills.
🪄 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: 7e6bed6a-9505-44e6-97be-234cce578d05
📒 Files selected for processing (6)
cmd/reset-main.jstests/unit/web-ui-behavior-parity.test.mjsweb-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/responsive.cssweb-ui/styles/sessions-list.css
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/reset-main.js
- tests/unit/web-ui-behavior-parity.test.mjs
- web-ui/styles/sessions-list.css
- web-ui/styles/responsive.css
- web-ui/partials/index/layout-header.html
📜 Review details
🔇 Additional comments (1)
web-ui/styles/layout-shell.css (1)
46-49: Nice fix for the narrow-laptop squeeze.Switching the side tracks to
minmax(...)gives the center panel much more room before the compact breakpoint kicks in. This looks like a solid follow-up to the earlier three-column issue.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
web-ui/styles/responsive.css (1)
127-130: Minor gap inconsistency across breakpoints for.status-strip.The
gapvalue varies across breakpoints:
- Base (navigation-panels.css):
10px- 720px:
12px- 540px (line 198):
var(--spacing-xs)This may be intentional for visual rhythm at different sizes, but if not, consider using a consistent spacing variable throughout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/responsive.css` around lines 127 - 130, The .status-strip gap is inconsistent across breakpoints (10px in navigation-panels.css base, 12px here, and var(--spacing-xs) at 540px); pick a single spacing token (e.g., var(--spacing-xs) or var(--spacing-sm)) and replace the hard-coded 12px in the .status-strip rule and any other occurrences so all breakpoints reference the same CSS variable; update the declaration for .status-strip (and the base rule in navigation-panels.css if needed) to use the chosen variable to ensure consistent spacing across breakpoints.web-ui/styles/titles-cards.css (2)
45-45: Add-webkit-backdrop-filterprefix for Safari compatibility.Same as noted in layout-shell.css, the
backdrop-filterproperty requires the-webkit-vendor prefix for older Safari versions.💅 Suggested fix
.segmented-control { ... + -webkit-backdrop-filter: blur(4px); backdrop-filter: blur(4px); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/titles-cards.css` at line 45, Add the -webkit- vendor-prefixed version of the backdrop-filter property alongside the existing backdrop-filter declaration in titles-cards.css to ensure Safari compatibility; locate the CSS rule that currently contains "backdrop-filter: blur(4px);" and insert a "-webkit-backdrop-filter: blur(4px);" declaration immediately before or after it so older WebKit/Safari engines pick up the blur effect.
12-15: The-webkit-prefixed properties may be unnecessary if gradient text is removed.Setting
-webkit-background-clip: border-boxand-webkit-text-fill-color: currentColorresets gradient text styling. If the previous gradient implementation has been fully removed, these properties can be simplified to justbackground: noneand relying oncoloralone.💅 Simplified version
.main-title { font-size: clamp(30px, 3.2vw, 42px); font-weight: var(--font-weight-display); line-height: 1.02; letter-spacing: -0.04em; margin-bottom: 0; color: var(--color-text-primary); font-family: var(--font-family-display); - background: none; - -webkit-background-clip: border-box; - -webkit-text-fill-color: currentColor; - background-clip: border-box; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/titles-cards.css` around lines 12 - 15, Remove the obsolete WebKit-specific gradient text fallbacks in titles-cards.css: if you've removed gradient-based text, delete the -webkit-background-clip and -webkit-text-fill-color declarations and rely on background: none plus the standard background-clip and color rules; update the block containing background: none, -webkit-background-clip, -webkit-text-fill-color, background-clip to only keep background: none, background-clip: border-box (or remove background-clip if unnecessary) and ensure text color is controlled via color rather than -webkit-text-fill-color.web-ui/styles/layout-shell.css (1)
69-69: Add vendor prefix forbackdrop-filterto ensure Safari compatibility.
backdrop-filterrequires the-webkit-prefix for older Safari versions (pre-Safari 18). The same applies to lines 227 and 45 in the segmented control.💅 Suggested fix
.side-rail { ... + -webkit-backdrop-filter: blur(8px); backdrop-filter: blur(8px); }Apply similarly to
.github-badge(line 227) and.segmented-controlin titles-cards.css (line 45).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` at line 69, Add the WebKit vendor-prefixed property for backdrop-filter to ensure Safari compatibility: for the selector containing backdrop-filter: blur(8px) in layout-shell.css, add -webkit-backdrop-filter: blur(8px) alongside the existing backdrop-filter; do the same for the .github-badge rule and the .segmented-control rule in titles-cards.css so each has both -webkit-backdrop-filter and backdrop-filter with the same value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Line 69: Add the WebKit vendor-prefixed property for backdrop-filter to ensure
Safari compatibility: for the selector containing backdrop-filter: blur(8px) in
layout-shell.css, add -webkit-backdrop-filter: blur(8px) alongside the existing
backdrop-filter; do the same for the .github-badge rule and the
.segmented-control rule in titles-cards.css so each has both
-webkit-backdrop-filter and backdrop-filter with the same value.
In `@web-ui/styles/responsive.css`:
- Around line 127-130: The .status-strip gap is inconsistent across breakpoints
(10px in navigation-panels.css base, 12px here, and var(--spacing-xs) at 540px);
pick a single spacing token (e.g., var(--spacing-xs) or var(--spacing-sm)) and
replace the hard-coded 12px in the .status-strip rule and any other occurrences
so all breakpoints reference the same CSS variable; update the declaration for
.status-strip (and the base rule in navigation-panels.css if needed) to use the
chosen variable to ensure consistent spacing across breakpoints.
In `@web-ui/styles/titles-cards.css`:
- Line 45: Add the -webkit- vendor-prefixed version of the backdrop-filter
property alongside the existing backdrop-filter declaration in titles-cards.css
to ensure Safari compatibility; locate the CSS rule that currently contains
"backdrop-filter: blur(4px);" and insert a "-webkit-backdrop-filter: blur(4px);"
declaration immediately before or after it so older WebKit/Safari engines pick
up the blur effect.
- Around line 12-15: Remove the obsolete WebKit-specific gradient text fallbacks
in titles-cards.css: if you've removed gradient-based text, delete the
-webkit-background-clip and -webkit-text-fill-color declarations and rely on
background: none plus the standard background-clip and color rules; update the
block containing background: none, -webkit-background-clip,
-webkit-text-fill-color, background-clip to only keep background: none,
background-clip: border-box (or remove background-clip if unnecessary) and
ensure text color is controlled via color rather than -webkit-text-fill-color.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32248764-8f44-4982-b529-14cf603bd648
📒 Files selected for processing (5)
web-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/titles-cards.css
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/partials/index/layout-header.html
- web-ui/styles/navigation-panels.css
📜 Review details
🔇 Additional comments (7)
web-ui/styles/layout-shell.css (3)
47-48: Grid columns now use responsiveminmax()ranges.The switch from fixed-width columns to
minmax(188px, 248px)andminmax(220px, 292px)addresses the previous concern about the center panel being squeezed on smaller viewports. This allows the side columns to shrink gracefully before the 960px collapse point.
393-406: Overflow handling for hero pills has been implemented.The
min-width: 0on the pill containers (line 399) and truncation rules on value elements (lines 442-445) address the previous concern about long repo slugs or area labels blowing out the row.
414-426: Hover and focus-visible states are now properly defined.The
:hoverstate (lines 414-418) and combined:focus/:focus-visiblestates (lines 420-426) address the previous accessibility concern. The outline styling provides clear keyboard focus indication.web-ui/styles/responsive.css (2)
117-119: Subtitle margin now consistently set to 0 on mobile.The previous concern about the 720px breakpoint restoring
margin-bottom: 16pxhas been addressed. The subtitle now hasmargin-bottom: 0at this breakpoint, maintaining the tighter header grouping across all viewport sizes.
177-181: Full-width hero pills on narrow screens is a good mobile pattern.Forcing
.hero-link-pilland.hero-summary-pilltowidth: 100%withjustify-content: space-betweenensures proper touch target sizing and readable labels on small devices.web-ui/styles/titles-cards.css (2)
99-101: Usingwill-change: autois preferable to specifying properties.Good change from explicitly listing properties to
will-change: auto. This avoids potential GPU memory overhead from forced layer promotion while still allowing the browser to optimize as needed.
168-182: Card icon styling streamlined with cleaner transitions.The simplified
.card-iconstyling removes unnecessary shadows and gradients while maintaining clear visual hierarchy. The explicit transition properties (background-color,color) are better thantransition: allfor performance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web-ui/styles/layout-shell.css (1)
112-124: Consolidate duplicated active-state rules for side nav items.
Line 112-Line 117andLine 119-Line 124are identical. Merging them will reduce drift risk when active styles change later.♻️ Suggested consolidation
-.side-item.active { - border-color: rgba(201, 94, 75, 0.48); - background: rgba(249, 244, 240, 0.98); - color: var(--color-text-primary); - box-shadow: inset 0 0 0 1px rgba(201, 94, 75, 0.06); -} - -.side-item.nav-intent-active { +.side-item.active, +.side-item.nav-intent-active { border-color: rgba(201, 94, 75, 0.48); background: rgba(249, 244, 240, 0.98); color: var(--color-text-primary); box-shadow: inset 0 0 0 1px rgba(201, 94, 75, 0.06); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 112 - 124, The two identical rules for .side-item.active and .side-item.nav-intent-active should be consolidated to a single CSS rule to avoid duplication; replace the separate blocks by one rule that targets both selectors (e.g., ".side-item.active, .side-item.nav-intent-active") containing the shared declarations (border-color, background, color, box-shadow) and remove the duplicate block so future changes apply to both states at once.web-ui/styles/navigation-panels.css (1)
288-318: Animatebox-shadowin tab state transitions for smoother nav-intent feedback.
Line 297transitions color/background only, whileLine 310andLine 317changebox-shadow. Add it to avoid abrupt visual jumps duringactive/nav-intent-*toggles.💅 Suggested tweak
.top-tab { @@ - transition: border-color var(--transition-fast) var(--ease-smooth), background-color var(--transition-fast) var(--ease-smooth), color var(--transition-fast) var(--ease-smooth); + transition: border-color var(--transition-fast) var(--ease-smooth), + background-color var(--transition-fast) var(--ease-smooth), + color var(--transition-fast) var(--ease-smooth), + box-shadow var(--transition-fast) var(--ease-smooth); box-shadow: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/navigation-panels.css` around lines 288 - 318, The .top-tab transition currently animates border-color, background-color, and color but omits box-shadow, causing abrupt jumps when toggling .top-tab.active and .top-tab.nav-intent-active; update the .top-tab rule (the selector named ".top-tab") to include box-shadow in its transition list so the box-shadow changes in ".top-tab.active" and ".top-tab.nav-intent-active" animate smoothly along with color and background.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles/titles-cards.css`:
- Around line 5-9: The title rule in titles-cards.css sets line-height: 0.99
which is too tight and can clip glyphs on font fallbacks; update the rule that
currently contains "line-height: 0.99" to a safer value (suggest 1.04–1.1, e.g.,
1.05) so larger sizes won't clip ascenders/descenders while leaving the existing
font-size, font-weight, letter-spacing and margin-bottom unchanged.
---
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 112-124: The two identical rules for .side-item.active and
.side-item.nav-intent-active should be consolidated to a single CSS rule to
avoid duplication; replace the separate blocks by one rule that targets both
selectors (e.g., ".side-item.active, .side-item.nav-intent-active") containing
the shared declarations (border-color, background, color, box-shadow) and remove
the duplicate block so future changes apply to both states at once.
In `@web-ui/styles/navigation-panels.css`:
- Around line 288-318: The .top-tab transition currently animates border-color,
background-color, and color but omits box-shadow, causing abrupt jumps when
toggling .top-tab.active and .top-tab.nav-intent-active; update the .top-tab
rule (the selector named ".top-tab") to include box-shadow in its transition
list so the box-shadow changes in ".top-tab.active" and
".top-tab.nav-intent-active" animate smoothly along with color and background.
🪄 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: 141584dc-1a40-4c93-bd02-899ce26ba3a5
📒 Files selected for processing (3)
web-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/titles-cards.css
📜 Review details
🔇 Additional comments (3)
web-ui/styles/layout-shell.css (1)
45-48: Responsive track sizing looks solid.Good move replacing fixed side columns with
minmax(...); this prevents the center column from collapsing too early on laptop widths.web-ui/styles/navigation-panels.css (1)
34-48: Status strip grid update is clean and responsive.The
repeat(auto-fit, minmax(168px, 1fr))update is a good fit for variable chip counts and aligns well with the refreshed panel spacing.web-ui/styles/titles-cards.css (1)
87-110: Card flattening changes are coherent and well-scoped.The updated base/hover/active/icon styles stay visually consistent and keep interaction states clear without heavy elevation effects.
Also applies to: 142-146, 168-188
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/partials/index/layout-header.html`:
- Around line 15-17: Extract the tab label logic into a single computed mapping
(e.g., tabLabelMap or a computed getter like getMainTabLabel) and use that in
both the hero pill and the panel header instead of the inline ternary chain;
specifically reference the mainTab prop/variable and ensure the map explicitly
defines 'settings' and uses the same fallback ('未知') as
web-ui/modules/config-mode.computed.mjs so unexpected values don't fall through
to "设置"/"Settings". Update the template spans (the hero-summary-value and the
panel header) to bind to this computed label, remove duplicated ternaries, and
reuse the existing computed module if possible to avoid copy/drift.
- Around line 19-27: Both the desktop GitHub pill (.hero-link-pill inside
.hero-surface) and the mobile badge (.github-badge-mobile inside .hero-github)
are rendered simultaneously at compact widths; add a breakpoint rule to hide one
when the other is visible. For example, add a media query matching the rule that
shows .hero-surface (e.g., `@media` (min-width: 961px) and
body:not(.force-compact)) and set .hero-github { display: none; } there, or
conversely add `@media` (max-width: 960px) { .hero-surface { display: none; } } so
only one of .hero-surface/.hero-link-pill or .hero-github/.github-badge-mobile
is visible at a given viewport; ensure the rule respects the existing
body.force-compact override.
🪄 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: e4f8033f-c267-4d4a-9664-36bc03f330a2
📒 Files selected for processing (4)
web-ui/partials/index/layout-footer.htmlweb-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/partials/index/layout-footer.html
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/styles/navigation-panels.css
📜 Review details
🔇 Additional comments (3)
web-ui/styles/layout-shell.css (3)
45-48: Good call switching the shell tracks tominmax().This keeps the center panel usable on mid-width screens without forcing an early collapse.
394-405: Nice accessibility polish on.hero-link-pill.The hover and focus states are clear and consistent now.
373-386: Good overflow guard for dynamic pill text.Letting the pill shrink and truncating the value should keep long labels from blowing out the hero row.
Also applies to: 417-425
| <div class="hero-summary-pill"> | ||
| <span class="hero-summary-label">当前区域</span> | ||
| <span class="hero-summary-value">{{ mainTab === 'config' ? '配置中心' : (mainTab === 'sessions' ? '会话浏览' : (mainTab === 'usage' ? 'Usage' : (mainTab === 'market' ? '技能市场' : '设置'))) }}</span> |
There was a problem hiding this comment.
Extract the mainTab copy to one explicit map.
The same branch chain is now duplicated in the hero pill and the panel header, and both fall through to the settings strings for any unexpected value. web-ui/modules/config-mode.computed.mjs:48-56 already treats settings explicitly and falls back to 未知, so reusing one computed map here would avoid misleading copy and future drift.
Also applies to: 270-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/partials/index/layout-header.html` around lines 15 - 17, Extract the
tab label logic into a single computed mapping (e.g., tabLabelMap or a computed
getter like getMainTabLabel) and use that in both the hero pill and the panel
header instead of the inline ternary chain; specifically reference the mainTab
prop/variable and ensure the map explicitly defines 'settings' and uses the same
fallback ('未知') as web-ui/modules/config-mode.computed.mjs so unexpected values
don't fall through to "设置"/"Settings". Update the template spans (the
hero-summary-value and the panel header) to bind to this computed label, remove
duplicated ternaries, and reuse the existing computed module if possible to
avoid copy/drift.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/modules/app.methods.startup-claude.mjs (1)
389-399: The function now writes a marker with no observable effect.The
codexmateStarPromptedmarker is persisted on first run but never triggers any UI messaging or gated behavior. If the intent is to completely remove the star prompt feature, consider whether maintaining this storage write is still necessary, or if the entire method body could be simplified to a no-op (or removed entirely along with its call site at line 77).If the marker is being retained for future use or analytics, a brief comment clarifying its purpose would help maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.methods.startup-claude.mjs` around lines 389 - 399, The maybeShowStarPrompt function currently writes the 'codexmateStarPrompted' key but has no observable effect; either remove the storage write and simplify maybeShowStarPrompt to a no-op (or remove the function and its call site), or keep the write but add a clear comment explaining the retained marker's future/analytics purpose, or implement the missing gating/UI behavior that reads 'codexmateStarPrompted' (so the marker actually prevents/shows the prompt). Locate the function maybeShowStarPrompt and the call site that invokes it, and apply one of these fixes consistently: delete the write and/or function and its invocation, replace body with an explicit no-op and comment, or implement the UI gating that consumes the 'codexmateStarPrompted' key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/modules/app.methods.startup-claude.mjs`:
- Around line 389-399: The maybeShowStarPrompt function currently writes the
'codexmateStarPrompted' key but has no observable effect; either remove the
storage write and simplify maybeShowStarPrompt to a no-op (or remove the
function and its call site), or keep the write but add a clear comment
explaining the retained marker's future/analytics purpose, or implement the
missing gating/UI behavior that reads 'codexmateStarPrompted' (so the marker
actually prevents/shows the prompt). Locate the function maybeShowStarPrompt and
the call site that invokes it, and apply one of these fixes consistently: delete
the write and/or function and its invocation, replace body with an explicit
no-op and comment, or implement the UI gating that consumes the
'codexmateStarPrompted' key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66051a58-cf9d-4bfa-97d3-13d9d6eedb39
📒 Files selected for processing (8)
tests/unit/startup-claude-star-prompt.test.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/partials/index/layout-footer.htmlweb-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/titles-cards.css
💤 Files with no reviewable changes (1)
- web-ui/partials/index/layout-footer.html
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/styles/layout-shell.css
📜 Review details
🔇 Additional comments (9)
tests/unit/startup-claude-star-prompt.test.mjs (3)
59-76: LGTM!The test correctly verifies the silent persistence behavior: when the marker is absent,
setItemis called with the expected key/value, and no messages are pushed to the context.
78-93: LGTM!The test properly verifies that storage write failures are silently swallowed without pushing any error messages to the UI.
95-110: LGTM!Good coverage for the
getItemfailure path. The test correctly asserts that when storage read throws, the function exits silently without attemptingsetItemor pushing messages. ThesetItemmock throwing ensures no unexpected call happens.web-ui/partials/index/layout-header.html (2)
231-233: ConsolidatemainTablabel/title/subtitle mapping into one source.Line 231 to Line 233 still duplicate nested ternary chains and fallback to “Settings”, which can drift from the canonical fallback (
未知) inweb-ui/modules/config-mode.computed.mjs. Please reuse a single computed map/helper for kicker/title/subtitle.
16-22: External link hardening is correctly applied.Line 20 and Line 223 include
rel="noopener noreferrer"with_blank, which is the right security posture.Also applies to: 219-224
web-ui/styles/titles-cards.css (2)
4-16: Typography cleanup is solid.Line 7 uses a safer
line-height: 1.06, and Line 28 keeps subtitle spacing compact as intended. This aligns well with the refined header composition.Also applies to: 24-31
87-110: Card/segment visual simplification looks consistent.The flatter surfaces, explicit transition properties, and lighter active/hover treatments reduce visual noise without changing interaction semantics.
Also applies to: 142-154, 168-188
web-ui/styles/responsive.css (1)
36-53: Breakpoint behavior is coherent with the new shell/header model.Line 37 correctly reveals
.workbench-barfor compact layouts, and Lines 91-93 keep status chips in a predictable single-column stack on smaller screens.Also applies to: 75-93, 126-132
web-ui/styles/navigation-panels.css (1)
34-54: Panel/status and tab styling updates are well-aligned with the new hierarchy.The new grid-based status strip and refined panel header classes match the updated template structure and improve consistency across breakpoints.
Also applies to: 131-159, 197-241
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/styles/navigation-panels.css (1)
197-214: Remove redundantpaddingdeclaration in.top-tabs.Line 202 (
padding: 0;) is fully overridden by Line 213 (padding: 0 8px 2px 2px;). Dropping the earlier one will reduce CSS noise and simplify maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/navigation-panels.css` around lines 197 - 214, The .top-tabs rule contains a redundant padding declaration: remove the earlier "padding: 0;" statement so only the intended "padding: 0 8px 2px 2px;" remains; update the .top-tabs block (the CSS selector .top-tabs) to delete the first padding line to reduce noise and avoid duplicate declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/styles/navigation-panels.css`:
- Around line 197-214: The .top-tabs rule contains a redundant padding
declaration: remove the earlier "padding: 0;" statement so only the intended
"padding: 0 8px 2px 2px;" remains; update the .top-tabs block (the CSS selector
.top-tabs) to delete the first padding line to reduce noise and avoid duplicate
declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd05066d-e8b9-4d97-b161-1589769766b5
📒 Files selected for processing (5)
web-ui/partials/index/layout-header.htmlweb-ui/styles/base-theme.cssweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.css
📜 Review details
🔇 Additional comments (4)
web-ui/partials/index/layout-header.html (1)
200-202: ConsolidatemainTabcopy into one computed mapping.Line 200-Line 202 repeat parallel ternary chains for kicker/title/subtitle. Centralizing this in one computed map will avoid drift and make fallback handling explicit.
web-ui/styles/base-theme.css (1)
137-148: Compact tab-row fallback is well implemented.Line 138-Line 148 cleanly switches compact tabs to horizontal scrolling with cross-browser scrollbar suppression, which should prevent cramped wrapping in forced-compact mode.
web-ui/styles/responsive.css (1)
41-54: Responsive tab/status behavior looks consistent.Line 42-Line 54 correctly moves tabs to a horizontal scroll row and keeps status chips in a controlled 2-column grid at ≤960px, improving small-screen stability.
web-ui/styles/layout-shell.css (1)
99-163: Side-rail nav state styling is a solid cleanup.Line 99-Line 163 gives clear active/nav-intent differentiation while keeping inactive state visually quiet; the accent bar approach improves scanability without adding noise.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-ui/styles/layout-shell.css (1)
216-222: Move this brand subtitle into the markup.Visible copy in
::afteris hard to localize, copy, or test, and assistive tech announces generated text inconsistently.web-ui/partials/index/layout-header.html:80-83already owns the brand block, so this should be a real element there instead of CSS content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 216 - 222, The subtitle text is injected via .brand-block::after which prevents localization, selection, and proper accessibility; instead add a real element (e.g., a <span> with class "brand-subtitle") inside the existing brand block in web-ui/partials/index/layout-header.html next to the brand content, move the visual styles (font-size, line-height, color, max-width) from .brand-block::after into a new .brand-subtitle selector in web-ui/styles/layout-shell.css, and remove the content property and ::after rule so the text is sourced from markup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 171-183: The row jumps because .side-item-meta is switched between
display:none and display:flex; instead always keep its layout space (e.g., keep
display:flex or inline-flex and set a reserved height/min-height for the second
line) and toggle visibility/opacity and pointer-events for the show/hide state
so the element’s box remains stable; update the .side-item-meta rule (replace
display:none) and the .side-item.active/.side-item.nav-intent-active rules (stop
switching display, set visibility:visible; opacity:1; pointer-events:auto when
active and visibility:hidden; opacity:0; pointer-events:none when inactive) to
prevent layout reflow while preserving the
nav-intent-active/.nav-intent-inactive toggles.
In `@web-ui/styles/navigation-panels.css`:
- Around line 65-72: The .status-chip .value flex item is forcing its
min-content width due to white-space: nowrap, causing overflow on narrow panels;
update the .status-chip .value rules to allow shrinking by adding min-width: 0
and enable an ellipsis by adding overflow: hidden and text-overflow: ellipsis
(keep white-space: nowrap if you want a single-line truncated value) so long
values shrink inside the chip instead of overflowing.
---
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 216-222: The subtitle text is injected via .brand-block::after
which prevents localization, selection, and proper accessibility; instead add a
real element (e.g., a <span> with class "brand-subtitle") inside the existing
brand block in web-ui/partials/index/layout-header.html next to the brand
content, move the visual styles (font-size, line-height, color, max-width) from
.brand-block::after into a new .brand-subtitle selector in
web-ui/styles/layout-shell.css, and remove the content property and ::after rule
so the text is sourced from markup.
🪄 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: a0df98a2-39a6-489b-ac73-4c69c24cd2e8
📒 Files selected for processing (4)
web-ui/styles/controls-forms.cssweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/titles-cards.css
| .status-chip .value { | ||
| font-size: var(--font-size-body); | ||
| font-weight: var(--font-weight-secondary); | ||
| font-size: 12px; | ||
| font-weight: 600; | ||
| color: var(--color-text-primary); | ||
| letter-spacing: -0.01em; | ||
| white-space: normal; | ||
| overflow-wrap: anywhere; | ||
| word-break: break-word; | ||
| white-space: nowrap; | ||
| overflow-wrap: normal; | ||
| word-break: normal; |
There was a problem hiding this comment.
Allow long status values to shrink inside the chip.
With white-space: nowrap, this flex item keeps its min-content width. A longer provider/model/path value will overflow the status strip on narrow panels instead of shrinking. Add min-width: 0 plus ellipsis, or allow wrapping.
One simple follow-up
.status-chip .value {
+ min-width: 0;
+ overflow: hidden;
+ text-overflow: ellipsis;
font-size: 12px;
font-weight: 600;
color: var(--color-text-primary);
letter-spacing: -0.01em;
white-space: nowrap;
overflow-wrap: normal;
word-break: normal;
}📝 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.
| .status-chip .value { | |
| font-size: var(--font-size-body); | |
| font-weight: var(--font-weight-secondary); | |
| font-size: 12px; | |
| font-weight: 600; | |
| color: var(--color-text-primary); | |
| letter-spacing: -0.01em; | |
| white-space: normal; | |
| overflow-wrap: anywhere; | |
| word-break: break-word; | |
| white-space: nowrap; | |
| overflow-wrap: normal; | |
| word-break: normal; | |
| .status-chip .value { | |
| min-width: 0; | |
| overflow: hidden; | |
| text-overflow: ellipsis; | |
| font-size: 12px; | |
| font-weight: 600; | |
| color: var(--color-text-primary); | |
| letter-spacing: -0.01em; | |
| white-space: nowrap; | |
| overflow-wrap: normal; | |
| word-break: normal; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/styles/navigation-panels.css` around lines 65 - 72, The .status-chip
.value flex item is forcing its min-content width due to white-space: nowrap,
causing overflow on narrow panels; update the .status-chip .value rules to allow
shrinking by adding min-width: 0 and enable an ellipsis by adding overflow:
hidden and text-overflow: ellipsis (keep white-space: nowrap if you want a
single-line truncated value) so long values shrink inside the chip instead of
overflowing.
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 (1)
web-ui/styles/layout-shell.css (1)
42-50:⚠️ Potential issue | 🟡 MinorStale comment: the layout is now 2-column, not 3-column.
The comment on lines 43-44 references "三栏" (three columns) and "状态检查器" (status inspector), but the implementation is a 2-column grid and the status inspector has been removed. Update the comment to match the current structure.
📝 Suggested fix
/* ============================================ - 布局：三栏（侧栏 + 主区 + 状态检查器） + 布局：双栏（侧栏 + 主区） ============================================ */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 42 - 50, Update the stale header comment above the .app-shell CSS: change the description from "布局：三栏（侧栏 + 主区 + 状态检查器）" to reflect the current two-column layout (e.g., "布局：两栏（侧栏 + 主区）") so the comment matches grid-template-columns and the removed status inspector; modify only the comment text near the .app-shell rule to accurately describe the structure.
🧹 Nitpick comments (1)
web-ui/styles/layout-shell.css (1)
133-145: Consider consolidating duplicate active state rules.
.side-item.activeand.side-item.nav-intent-activehave identical style declarations. While keeping them separate may be intentional for clarity, they could be combined to reduce duplication.♻️ Optional consolidation
-.side-item.active { - border-color: rgba(201, 94, 75, 0.18); - background: rgba(255, 255, 255, 0.82); - color: var(--color-text-primary); - box-shadow: none; -} - -.side-item.nav-intent-active { +.side-item.active, +.side-item.nav-intent-active { border-color: rgba(201, 94, 75, 0.18); background: rgba(255, 255, 255, 0.82); color: var(--color-text-primary); box-shadow: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 133 - 145, The two CSS rules for .side-item.active and .side-item.nav-intent-active are identical; consolidate them by merging into a single rule that targets both selectors (e.g., ".side-item.active, .side-item.nav-intent-active") and remove the duplicate block so the shared declarations (border-color, background, color, box-shadow) are defined only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 216-222: The pseudo-element .brand-block::after contains hardcoded
Chinese text which blocks i18n; remove the literal content from the CSS and make
the subtitle driven by markup (e.g., add a data-subtitle attribute or a child
element in the HTML where the brand block is rendered) and change the CSS to use
content: attr(data-subtitle) (or target the child element) so translators can
update the HTML/strings; update the component that renders the .brand-block to
supply the localized string (and provide a sensible default/fallback) rather
than embedding it in layout-shell.css.
---
Outside diff comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 42-50: Update the stale header comment above the .app-shell CSS:
change the description from "布局：三栏（侧栏 + 主区 + 状态检查器）" to reflect the current
two-column layout (e.g., "布局：两栏（侧栏 + 主区）") so the comment matches
grid-template-columns and the removed status inspector; modify only the comment
text near the .app-shell rule to accurately describe the structure.
---
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 133-145: The two CSS rules for .side-item.active and
.side-item.nav-intent-active are identical; consolidate them by merging into a
single rule that targets both selectors (e.g., ".side-item.active,
.side-item.nav-intent-active") and remove the duplicate block so the shared
declarations (border-color, background, color, box-shadow) are defined only
once.
🪄 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: 6e607720-545b-43cb-b278-f095784204f5
📒 Files selected for processing (3)
tests/unit/compact-layout-ui.test.mjsweb-ui/styles/layout-shell.cssweb-ui/styles/responsive.css
✅ Files skipped from review due to trivial changes (1)
- tests/unit/compact-layout-ui.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/styles/responsive.css
📜 Review details
🔇 Additional comments (6)
web-ui/styles/layout-shell.css (6)
171-183: Side-rail row height instability during nav intent still present.The
display: none/display: flextoggle causes neighboring items to jump during navigation intent updates. Reserve the row height and togglevisibility/opacityinstead.🔧 Suggested fix to stabilize layout
.side-item-meta { font-size: 10px; color: var(--color-text-tertiary); - display: none; + display: flex; + visibility: hidden; + opacity: 0; gap: 5px; flex-wrap: wrap; line-height: 1.35; + min-height: 1.35em; + transition: opacity var(--transition-fast) var(--ease-smooth), + visibility var(--transition-fast) var(--ease-smooth); } .side-item.active .side-item-meta, .side-item.nav-intent-active .side-item-meta { - display: flex; + visibility: visible; + opacity: 1; }
8-25: Cosmetic background adjustments look good.The multi-layer gradient and refined grid overlay with very subtle opacity values provide a cleaner visual foundation without impacting layout behavior.
56-70: Side rail restyling is clean.The flatter translucent background with
backdrop-filter: blur(10px)creates a modern glassmorphism effect. Note thatbackdrop-filtercan impact rendering performance on lower-end devices with complex DOM trees, but for a sticky side rail this should be acceptable.
191-195: Media query for top-tabs visibility is appropriate.The 721px breakpoint correctly hides the mobile
.top-tabson larger viewports when not in compact mode.
263-276: Good interactive states on GitHub badge.The hover and
:focus-visiblestates provide clear visual feedback with appropriate outline offset for accessibility. The subtle box-shadow ring on hover is a nice touch.
358-359: GitHub badge rail hover is appropriately minimal.The border-color change provides sufficient feedback for the in-rail variant.
web-ui/styles/layout-shell.css
Outdated
| .brand-block::after { | ||
| content: "本地配置与会话工作台"; | ||
| font-size: 10px; | ||
| line-height: 1.3; | ||
| color: var(--color-text-muted); | ||
| max-width: 18ch; | ||
| } |
There was a problem hiding this comment.
Hardcoded Chinese text in CSS limits localization.
The ::after content ("本地配置与会话工作台") is not accessible to translation systems. If the app needs i18n support in the future, this subtitle should move to HTML.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/styles/layout-shell.css` around lines 216 - 222, The pseudo-element
.brand-block::after contains hardcoded Chinese text which blocks i18n; remove
the literal content from the CSS and make the subtitle driven by markup (e.g.,
add a data-subtitle attribute or a child element in the HTML where the brand
block is rendered) and change the CSS to use content: attr(data-subtitle) (or
target the child element) so translators can update the HTML/strings; update the
component that renders the .brand-block to supply the localized string (and
provide a sensible default/fallback) rather than embedding it in
layout-shell.css.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/styles/layout-shell.css (1)
22-25: Consider removing or adjusting the background grid opacity.The
opacity: 0.01makes this grid overlay essentially invisible. If the effect is intentional for extremely subtle visual texture, this is fine—otherwise, consider either removing the pseudo-element entirely or increasing the opacity to a perceptible level (e.g.,0.03–0.05).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 22 - 25, The background grid overlay in web-ui/styles/layout-shell.css is effectively invisible due to opacity: 0.01; update the rule that sets opacity (the grid pseudo-element/background layer with background-size: 220px 220px) to a more perceptible value (e.g., 0.03–0.05) or remove the overlay entirely if the texture is not needed; ensure you modify the opacity property on the same selector that contains the two linear-gradient background declarations so the change takes effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 22-25: The background grid overlay in
web-ui/styles/layout-shell.css is effectively invisible due to opacity: 0.01;
update the rule that sets opacity (the grid pseudo-element/background layer with
background-size: 220px 220px) to a more perceptible value (e.g., 0.03–0.05) or
remove the overlay entirely if the texture is not needed; ensure you modify the
opacity property on the same selector that contains the two linear-gradient
background declarations so the change takes effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2f88c92-e8fc-4c3a-be35-be1a4c4fd450
📒 Files selected for processing (2)
web-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/partials/index/layout-header.html
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
web-ui/styles/layout-shell.css
🔇 Additional comments (6)
web-ui/styles/layout-shell.css (6)
43-50: LGTM!The two-column grid layout with flexible sidebar bounds (
minmax(196px, 216px)) is well-structured. The responsive cascade inresponsive.cssproperly adjusts the sidebar at 960px and collapses to single-column at 720px.
56-70: LGTM!The translucent side-rail with
backdrop-filter: blur(10px)provides a clean, modern appearance. Settingmin-height: 0correctly prevents flex children from causing overflow issues.
116-126: LGTM!The left accent bar via
::beforepseudo-element provides a clear visual indicator for the active state. The transition setup enables smooth state changes.
165-182: Previous review concern addressed—LGTM!The
.side-item-metanow usesvisibility: hidden+opacity: 0withmin-height: 1.35eminstead of togglingdisplay. This reserves layout space and prevents row height jumps during.nav-intent-active/.nav-intent-inactiveclass toggles. The transitions are properly configured for smooth state changes.
196-220: LGTM!The
.brand-blockrefactor to flex-column layout with explicit.brand-kickerand.brand-subtitleclasses moves the subtitle text to HTML, which is i18n-friendly. Themax-width: 18chon.brand-subtitleappropriately constrains the text width.
261-274: Good accessibility enhancement—LGTM!The
:focus-visiblestyling withoutline: 3px solid rgba(201, 94, 75, 0.22)andoutline-offset: 2pxprovides clear keyboard focus indication without affecting mouse users. This aligns well with accessibility best practices.
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 (1)
web-ui/styles/controls-forms.css (1)
164-209:⚠️ Potential issue | 🟠 MajorDon't ship
13pxform fields to phones.These controls drop to
13px/36px, so iOS will zoom the page on focus and the touch target stays smaller than the44pxfloor you already use for buttons inresponsive.css. Please bump the mobile breakpoint for these too, then verify on iOS Safari.📱 Suggested mobile override
`@media` (max-width: 540px) { .model-select, .model-input { min-height: 44px; font-size: 16px; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/controls-forms.css` around lines 164 - 209, The .model-select and .model-input controls use 13px/36px which triggers iOS Safari zoom and provides touch targets smaller than 44px; add a mobile media-query override (e.g. max-width: 540px) that increases .model-select and .model-input min-height to 44px and font-size to 16px for touch accessibility, update the CSS to include that media block, and verify the change on iOS Safari.
🧹 Nitpick comments (2)
web-ui/styles/controls-forms.css (1)
251-327: Retint the add/tool hover states to the current palette.The new resting styles are blue/neutral, but hover still uses the older warm RGBA values. These buttons visibly jump back to the previous accent when hovered.
🎨 Small palette-alignment cleanup
.btn-add:hover { border-color: var(--color-brand); color: var(--color-brand); - background: linear-gradient(to bottom, rgba(210, 107, 90, 0.05) 0%, rgba(210, 107, 90, 0.02) 100%); + background: linear-gradient(to bottom, rgba(59, 130, 246, 0.08) 0%, rgba(59, 130, 246, 0.03) 100%); transform: translateY(-1px); } @@ .btn-tool:not(:disabled):hover { border-color: var(--color-brand); color: var(--color-brand); transform: translateY(-1px); - box-shadow: 0 4px 8px rgba(210, 107, 90, 0.12); + box-shadow: 0 4px 8px rgba(59, 130, 246, 0.12); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/controls-forms.css` around lines 251 - 327, Hover styles for .btn-add:hover (including background gradient and border/color) and .btn-tool:not(:disabled):hover still use the old warm RGBA values; update them to use the current palette variables (e.g., var(--color-brand) or the neutral hover variables) so hover states match the new resting blue/neutral theme. Locate .btn-add:hover, .btn-add:hover .icon and .btn-tool:not(:disabled):hover in the diff and replace the warm RGBA gradient and box-shadow colors with the corresponding CSS variables used elsewhere (border-color, color, background, box-shadow) so the hover visuals follow the new palette.web-ui/styles/responsive.css (1)
79-87: Merge the duplicated mobile.top-tabsrule.The later
marginshorthand resets the earliermargin-left/right: -2px, so the final phone spacing is hard to reason about and the edge bleed never applies. Folding this into one block will make the intended mobile layout explicit.♻️ One cleanup that preserves the edge bleed
- .top-tabs { - display: flex !important; - flex-wrap: nowrap; - overflow-x: auto; - overflow-y: hidden; - padding-bottom: 2px; - margin-left: -2px; - margin-right: -2px; - } + .top-tabs { + display: flex !important; + flex-wrap: nowrap; + overflow-x: auto; + overflow-y: hidden; + padding-bottom: 2px; + margin: 8px -2px 10px; + gap: 5px; + } @@ - .top-tabs { - margin: 8px 0 10px; - gap: 5px; - }Also applies to: 113-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/responsive.css` around lines 79 - 87, There are duplicate .top-tabs CSS blocks where a later margin shorthand overrides the earlier margin-left/right:-2px causing the intended edge bleed to be lost; merge the two .top-tabs rules into a single block that preserves display:flex !important, flex-wrap:nowrap, overflow-x:auto, overflow-y:hidden, padding-bottom:2px and explicit margin-left:-2px and margin-right:-2px (do not use a later margin shorthand that resets those), then remove the redundant duplicate block (the one that currently overrides margins) so the mobile spacing is explicit and stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 38-52: The .side-rail is stretching to the grid row height via
min-height: 100vh which makes its content unreachable until the main column
scrolls; update the .side-rail rule to constrain it to the viewport (use height:
100vh or max-height: 100vh instead of min-height) and move scrolling into the
rail by adding overflow: auto (or overflow-y: auto) so internal nav/rail content
scrolls independently; remove or replace min-height: 100vh in the .side-rail
block (and the similar block around lines 70-75) and ensure border, padding and
backdrop styles remain unchanged.
---
Outside diff comments:
In `@web-ui/styles/controls-forms.css`:
- Around line 164-209: The .model-select and .model-input controls use 13px/36px
which triggers iOS Safari zoom and provides touch targets smaller than 44px; add
a mobile media-query override (e.g. max-width: 540px) that increases
.model-select and .model-input min-height to 44px and font-size to 16px for
touch accessibility, update the CSS to include that media block, and verify the
change on iOS Safari.
---
Nitpick comments:
In `@web-ui/styles/controls-forms.css`:
- Around line 251-327: Hover styles for .btn-add:hover (including background
gradient and border/color) and .btn-tool:not(:disabled):hover still use the old
warm RGBA values; update them to use the current palette variables (e.g.,
var(--color-brand) or the neutral hover variables) so hover states match the new
resting blue/neutral theme. Locate .btn-add:hover, .btn-add:hover .icon and
.btn-tool:not(:disabled):hover in the diff and replace the warm RGBA gradient
and box-shadow colors with the corresponding CSS variables used elsewhere
(border-color, color, background, box-shadow) so the hover visuals follow the
new palette.
In `@web-ui/styles/responsive.css`:
- Around line 79-87: There are duplicate .top-tabs CSS blocks where a later
margin shorthand overrides the earlier margin-left/right:-2px causing the
intended edge bleed to be lost; merge the two .top-tabs rules into a single
block that preserves display:flex !important, flex-wrap:nowrap, overflow-x:auto,
overflow-y:hidden, padding-bottom:2px and explicit margin-left:-2px and
margin-right:-2px (do not use a later margin shorthand that resets those), then
remove the redundant duplicate block (the one that currently overrides margins)
so the mobile spacing is explicit and stable.
🪄 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: 6aab45a0-7bca-423e-ab11-71a451e06fc7
📒 Files selected for processing (7)
tests/unit/compact-layout-ui.test.mjsweb-ui/styles/base-theme.cssweb-ui/styles/controls-forms.cssweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/titles-cards.css
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/compact-layout-ui.test.mjs
- web-ui/styles/base-theme.css
- web-ui/styles/navigation-panels.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
web-ui/styles/layout-shell.css
🔇 Additional comments (1)
web-ui/styles/titles-cards.css (1)
4-16: LGTM — the refined title/subtitle metrics look solid.The safer title line-height and zero subtitle gap make the header stack cleaner, and the matching mobile override keeps that spacing consistent.
Also applies to: 24-31
| .side-rail { | ||
| position: sticky; | ||
| top: var(--spacing-md); | ||
| align-self: start; | ||
| top: 0; | ||
| align-self: stretch; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: var(--spacing-sm); | ||
| padding: var(--spacing-md) var(--spacing-sm); | ||
| background: linear-gradient(180deg, rgba(255, 255, 255, 0.95) 0%, rgba(255, 250, 245, 0.9) 100%); | ||
| border: 1px solid rgba(216, 201, 184, 0.65); | ||
| border-radius: var(--radius-xl); | ||
| box-shadow: var(--shadow-card); | ||
| min-height: 420px; | ||
| gap: 0; | ||
| padding: 0; | ||
| background: var(--color-surface); | ||
| border-right: 1px solid var(--color-border); | ||
| border-radius: 0; | ||
| box-shadow: none; | ||
| min-height: 100vh; | ||
| backdrop-filter: none; | ||
| } |
There was a problem hiding this comment.
Keep the sticky rail viewport-bound.
As written, the rail stretches with the full grid row, so on long pages it becomes page-height instead of viewport-height. That makes lower rail content reachable only after scrolling deep into the main panel. Give the rail a fixed viewport height and move overflow into the rail/nav area.
🧭 Minimal fix to keep the rail usable on tall pages
.side-rail {
position: sticky;
top: 0;
- align-self: stretch;
+ align-self: start;
@@
- min-height: 100vh;
+ height: 100vh;
+ overflow-y: auto;
}Also applies to: 70-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/styles/layout-shell.css` around lines 38 - 52, The .side-rail is
stretching to the grid row height via min-height: 100vh which makes its content
unreachable until the main column scrolls; update the .side-rail rule to
constrain it to the viewport (use height: 100vh or max-height: 100vh instead of
min-height) and move scrolling into the rail by adding overflow: auto (or
overflow-y: auto) so internal nav/rail content scrolls independently; remove or
replace min-height: 100vh in the .side-rail block (and the similar block around
lines 70-75) and ensure border, padding and backdrop styles remain unchanged.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web-ui/styles/layout-shell.css (1)
38-52:⚠️ Potential issue | 🟡 MinorSticky rail still uses
min-heightinstead of fixed viewport height.The side-rail uses
min-height: 100vhwithalign-self: stretch, which can cause the rail to stretch beyond the viewport on pages with tall main content. Lower rail items become unreachable without scrolling deep into the main panel.🧭 Suggested fix to keep the rail viewport-bound
.side-rail { position: sticky; top: 0; - align-self: stretch; + align-self: start; display: flex; flex-direction: column; gap: 0; padding: 0; background: var(--color-surface); border-right: 1px solid var(--color-border); border-radius: 0; box-shadow: none; - min-height: 100vh; + height: 100vh; + overflow-y: auto; backdrop-filter: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 38 - 52, The .side-rail should be constrained to the viewport instead of using min-height:100vh; update the .side-rail rule to remove min-height and use height:100vh (or max-height:100vh) and add overflow:auto so the rail stays viewport-bound and its internal content can scroll independently; keep existing positioning (position:sticky; top:0) and remove or adjust align-self: stretch if necessary to avoid conflicting sizing.
🧹 Nitpick comments (4)
web-ui/styles/titles-cards.css (2)
143-151: Same hardcoded brand color pattern in card.active.This uses
rgba(199, 116, 98, 0.18)directly. Consider defining additional opacity variants as tokens (e.g.,--color-brand-subtle-18) if this pattern is intentional, or use existing tokens like--color-brand-light/--color-brand-subtle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/titles-cards.css` around lines 143 - 151, The .card.active rule uses a hardcoded RGBA color (rgba(199, 116, 98, 0.18)); replace that literal with a CSS token to keep design tokens consistent: either use an existing token like --color-brand-light or --color-brand-subtle if appropriate, or add a new token (e.g., --color-brand-subtle-18) to the token definitions and reference that in .card.active and any other places using the same opacity variant; also update .card.active::before if it relies on the same brand variant to ensure consistency across selectors.
68-72: Hardcoded color in box-shadow reduces maintainability.The
box-shadowuses hardcodedrgba(199, 116, 98, 0.12)instead of deriving from the brand token. Consider using a CSS custom property or ensuring this matches the brand color if it changes..segment.active { color: var(--color-brand-dark); background: var(--color-brand-light); - box-shadow: inset 0 0 0 1px rgba(199, 116, 98, 0.12); + box-shadow: inset 0 0 0 1px var(--color-brand-subtle); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/titles-cards.css` around lines 68 - 72, The box-shadow in the .segment.active rule uses a hardcoded rgba(199, 116, 98, 0.12); replace this with a brand-based custom property so it tracks theme changes: add or use a CSS variable (e.g., --color-brand-dark-alpha or compute via color-mix() from --color-brand-dark) and reference that variable in the box-shadow for .segment.active instead of the hardcoded rgba value so the shadow color derives from the brand token.web-ui/styles/layout-shell.css (1)
258-264: Consider using a token for the focus outline color.The focus-visible outline uses a hardcoded
rgba(199, 116, 98, 0.18)while the rest of the file uses design tokens. For consistency, consider usingvar(--color-brand-light)or defining a dedicated focus token..github-badge:focus-visible { - outline: 3px solid rgba(199, 116, 98, 0.18); + outline: 3px solid var(--color-brand-light); outline-offset: 2px; border-color: var(--color-brand);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/layout-shell.css` around lines 258 - 264, Replace the hard-coded RGBA in .github-badge:focus-visible with a design token: use an existing token like var(--color-brand-light) or add a new token (e.g., --color-focus-outline) in the CSS variable definitions, then reference that token for outline color in the .github-badge:focus-visible rule so the focus outline matches the file's token-based theming.web-ui/styles/base-theme.css (1)
29-30: Redundant gradient definition.The
--bg-warm-gradientnow uses the same color at 0% and 100%, making it effectively a solid color rather than a gradient. Consider simplifying to just the background-color or renaming the variable if the gradient behavior is no longer needed.--bg-warm-gradient: - linear-gradient(180deg, `#F7F3EF` 0%, `#F7F3EF` 100%); + `#F7F3EF`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/base-theme.css` around lines 29 - 30, The CSS custom property --bg-warm-gradient is defined as a degenerate gradient (same color at 0% and 100%) so replace it with a simple color variable or rename it to reflect a flat color; update the declaration of --bg-warm-gradient to a single color value (e.g. --bg-warm-color: `#F7F3EF`) and then replace all uses of --bg-warm-gradient in the codebase with the new variable name (or if you keep the name, change its value from linear-gradient(...) to the single hex) so callers like background: var(--bg-warm-gradient) get the correct flat color.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web-ui/styles/layout-shell.css`:
- Around line 38-52: The .side-rail should be constrained to the viewport
instead of using min-height:100vh; update the .side-rail rule to remove
min-height and use height:100vh (or max-height:100vh) and add overflow:auto so
the rail stays viewport-bound and its internal content can scroll independently;
keep existing positioning (position:sticky; top:0) and remove or adjust
align-self: stretch if necessary to avoid conflicting sizing.
---
Nitpick comments:
In `@web-ui/styles/base-theme.css`:
- Around line 29-30: The CSS custom property --bg-warm-gradient is defined as a
degenerate gradient (same color at 0% and 100%) so replace it with a simple
color variable or rename it to reflect a flat color; update the declaration of
--bg-warm-gradient to a single color value (e.g. --bg-warm-color: `#F7F3EF`) and
then replace all uses of --bg-warm-gradient in the codebase with the new
variable name (or if you keep the name, change its value from
linear-gradient(...) to the single hex) so callers like background:
var(--bg-warm-gradient) get the correct flat color.
In `@web-ui/styles/layout-shell.css`:
- Around line 258-264: Replace the hard-coded RGBA in
.github-badge:focus-visible with a design token: use an existing token like
var(--color-brand-light) or add a new token (e.g., --color-focus-outline) in the
CSS variable definitions, then reference that token for outline color in the
.github-badge:focus-visible rule so the focus outline matches the file's
token-based theming.
In `@web-ui/styles/titles-cards.css`:
- Around line 143-151: The .card.active rule uses a hardcoded RGBA color
(rgba(199, 116, 98, 0.18)); replace that literal with a CSS token to keep design
tokens consistent: either use an existing token like --color-brand-light or
--color-brand-subtle if appropriate, or add a new token (e.g.,
--color-brand-subtle-18) to the token definitions and reference that in
.card.active and any other places using the same opacity variant; also update
.card.active::before if it relies on the same brand variant to ensure
consistency across selectors.
- Around line 68-72: The box-shadow in the .segment.active rule uses a hardcoded
rgba(199, 116, 98, 0.12); replace this with a brand-based custom property so it
tracks theme changes: add or use a CSS variable (e.g., --color-brand-dark-alpha
or compute via color-mix() from --color-brand-dark) and reference that variable
in the box-shadow for .segment.active instead of the hardcoded rgba value so the
shadow color derives from the brand token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68f5480d-ea08-4687-9204-85ca665f8684
📒 Files selected for processing (4)
web-ui/styles/base-theme.cssweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/titles-cards.css
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/styles/navigation-panels.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
web-ui/styles/layout-shell.css
🔇 Additional comments (8)
web-ui/styles/base-theme.css (2)
137-148: Good cross-browser scrollbar hiding.The combination of
scrollbar-width: noneand::-webkit-scrollbar { display: none }correctly handles scrollbar hiding across Firefox and WebKit-based browsers.
244-258: Body layout changes align with the new shell architecture.The switch to
justify-content: stretchandalign-items: stretchwithpadding: 0properly supports the full-bleed two-column shell layout.web-ui/styles/layout-shell.css (3)
154-171: Good implementation of stable meta visibility transitions.The use of
visibility/opacitytransitions withmin-height: 1.35emcorrectly reserves layout space while animating the meta text appearance. This prevents the layout jumping that would occur withdisplay: none/flextoggling.
185-209: Clean brand hierarchy implementation.The refactored brand-block with flex layout and separate
.brand-kicker/.brand-subtitleclasses provides clear visual hierarchy while keeping text in the HTML where it can be localized.
219-256: GitHub badge styling aligns with the flattened design.The simplified badge styling with token-based colors and removed shadows is consistent with the PR's visual direction.
web-ui/styles/titles-cards.css (3)
4-16: Main-title changes look good.The
clamp()sizing provides responsive typography, andline-height: 1.15is safe for glyph rendering. The background-clip reset removes any prior gradient text effect correctly.
87-110: Card styling simplification is well-executed.The removal of transforms and shadows in favor of border-color transitions reduces visual complexity. Using
will-change: autois appropriate since transforms are no longer animated on hover.
233-240: Subtitle opacity approach is acceptable.Using
opacity: 0.9for subtle de-emphasis works, though usingvar(--color-text-tertiary)directly without opacity would be more consistent with the token system. This is a minor style choice.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/styles/responsive.css (1)
90-98: Consolidate duplicated.top-tabsrules in the 720px block.Line 96–97 (
margin-left/right) is currently overridden by Line 135 (marginshorthand). Merging into one block will remove dead declarations and reduce future regression risk.♻️ Suggested cleanup (no behavior change)
- .top-tabs { - display: flex !important; - flex-wrap: nowrap; - overflow-x: auto; - overflow-y: hidden; - padding-bottom: 2px; - margin-left: -2px; - margin-right: -2px; - } + .top-tabs { + display: flex !important; + flex-wrap: nowrap; + overflow-x: auto; + overflow-y: hidden; + padding-bottom: 2px; + margin: 8px 0 10px; + gap: 5px; + } @@ - .top-tabs { - margin: 8px 0 10px; - gap: 5px; - }Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/responsive.css` around lines 90 - 98, The .top-tabs style has duplicated margin rules where margin-left/margin-right are overridden later by a margin shorthand; locate both .top-tabs blocks (the base and the 720px media block) and consolidate the duplicated properties into a single declaration (use the margin shorthand once, remove the separate margin-left and margin-right entries) so the later margin shorthand no longer creates dead declarations and behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/styles/responsive.css`:
- Around line 90-98: The .top-tabs style has duplicated margin rules where
margin-left/margin-right are overridden later by a margin shorthand; locate both
.top-tabs blocks (the base and the 720px media block) and consolidate the
duplicated properties into a single declaration (use the margin shorthand once,
remove the separate margin-left and margin-right entries) so the later margin
shorthand no longer creates dead declarations and behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b7b084d-8ef4-474d-98a7-098f81873d96
📒 Files selected for processing (6)
tests/unit/compact-layout-ui.test.mjsweb-ui/partials/index/layout-header.htmlweb-ui/styles/base-theme.cssweb-ui/styles/layout-shell.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/styles/layout-shell.css
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/compact-layout-ui.test.mjs
- web-ui/styles/base-theme.css
- web-ui/styles/navigation-panels.css
- web-ui/partials/index/layout-header.html
📜 Review details
🔇 Additional comments (3)
web-ui/styles/responsive.css (3)
15-27: Responsive 1280px shell/topbar adjustments look consistent.The reduced sidebar width and synchronized
.main-panel/.main-panel-topbarspacing are aligned and preserve the base grid behavior cleanly.
35-55: 960px breakpoint behavior is well-structured.The two-column contraction, tighter panel spacing, and 2-column status grid are coherent and fit the intended hierarchy refinement.
159-191: 540px refinements are directionally good for touch layouts.The tighter top-tabs/main-panel spacing and topbar margin reduction are consistent with the small-screen compaction strategy.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
web-ui/styles/sessions-list.css (2)
89-89: Simplify redundantminmax(116px, 116px)to just116px.Using
minmax(116px, 116px)where both values are identical is equivalent to a fixed116px. Consider simplifying for clarity.♻️ Suggested simplification
- grid-template-columns: repeat(2, minmax(116px, 116px)); + grid-template-columns: repeat(2, 116px);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/sessions-list.css` at line 89, The grid definition uses redundant minmax with identical values; update the grid-template-columns declaration that currently reads "grid-template-columns: repeat(2, minmax(116px, 116px));" to use a fixed width instead, e.g. "grid-template-columns: repeat(2, 116px);", by locating the grid-template-columns rule in sessions list styles and replacing minmax(116px, 116px) with 116px.
187-195: Hidden scrollbars may impact accessibility for some users.Completely hiding scrollbars with
scrollbar-width: noneanddisplay: noneremoves visual affordance for scrollable content. Users who rely on scrollbar visibility to understand content extent may find this confusing. If the design intent is purely aesthetic, consider keeping minimal scrollbar styling on:hoveror:focus-withinto preserve discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/sessions-list.css` around lines 187 - 195, The CSS hides scrollbars completely for .session-list (using scrollbar-width: none, -ms-overflow-style: none and .session-list::-webkit-scrollbar { display: none }), which hurts accessibility; change this to provide a minimal visible scrollbar by removing the hard hide and instead use a subtle, low-opacity scrollbar that becomes more visible on :hover or :focus-within of .session-list (update rules targeting .session-list, .session-list:hover, .session-list:focus-within and .session-list::-webkit-scrollbar to use thin/auto styles and low-opacity colors rather than none, and remove -ms-overflow-style: none and scrollbar-width: none so assistive discoverability is preserved).web-ui/styles/responsive.css (1)
90-98: Consider consolidating duplicate.top-tabsrules within the 720px breakpoint.
.top-tabsis styled twice within the same@media (max-width: 720px)block (lines 90-98 and 134-137). These could be merged into a single rule block for clarity and to avoid potential override confusion.♻️ Suggested consolidation
`@media` (max-width: 720px) { /* ... other rules ... */ .top-tabs { display: flex !important; flex-wrap: nowrap; overflow-x: auto; overflow-y: hidden; padding-bottom: 2px; margin-left: -2px; margin-right: -2px; + margin-top: 8px; + margin-bottom: 10px; + gap: 5px; } - .top-tabs { - margin: 8px 0 10px; - gap: 5px; - } /* ... other rules ... */ }Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles/responsive.css` around lines 90 - 98, There are duplicate .top-tabs rule blocks inside the same `@media` (max-width: 720px) breakpoint; consolidate them into a single .top-tabs rule by merging all properties from both occurrences (preserve display, flex-wrap, overflow-x, overflow-y, padding-bottom, margin-left, margin-right and any other attributes) so no styles are lost or overridden, then remove the redundant block to avoid confusion and potential cascade issues.web-ui/partials/index/panel-docs.html (1)
67-67: Extract repeated action label ternary to a computed property.The ternary chain
installCommandAction === 'update' ? '升级' : (installCommandAction === 'uninstall' ? '卸载' : '安装')appears in multiple templates (here, line 207 and 329 inlayout-header.html, and line 67 in this file). Consider extracting this to a computed property likeinstallCommandActionLabelto avoid drift and simplify maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/partials/index/panel-docs.html` at line 67, The inline ternary for mapping installCommandAction to a Chinese label should be replaced by a single computed property (e.g., installCommandActionLabel) on the component that returns '升级' for 'update', '卸载' for 'uninstall', and '安装' otherwise; add this computed property where installCommandAction is defined, then replace each template occurrence (including the snippet using installCommandAction) with the simpler binding {{ installCommandActionLabel }} so all templates share the same logic and cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/partials/index/panel-docs.html`:
- Line 67: The inline ternary for mapping installCommandAction to a Chinese
label should be replaced by a single computed property (e.g.,
installCommandActionLabel) on the component that returns '升级' for 'update', '卸载'
for 'uninstall', and '安装' otherwise; add this computed property where
installCommandAction is defined, then replace each template occurrence
(including the snippet using installCommandAction) with the simpler binding {{
installCommandActionLabel }} so all templates share the same logic and cannot
drift.
In `@web-ui/styles/responsive.css`:
- Around line 90-98: There are duplicate .top-tabs rule blocks inside the same
`@media` (max-width: 720px) breakpoint; consolidate them into a single .top-tabs
rule by merging all properties from both occurrences (preserve display,
flex-wrap, overflow-x, overflow-y, padding-bottom, margin-left, margin-right and
any other attributes) so no styles are lost or overridden, then remove the
redundant block to avoid confusion and potential cascade issues.
In `@web-ui/styles/sessions-list.css`:
- Line 89: The grid definition uses redundant minmax with identical values;
update the grid-template-columns declaration that currently reads
"grid-template-columns: repeat(2, minmax(116px, 116px));" to use a fixed width
instead, e.g. "grid-template-columns: repeat(2, 116px);", by locating the
grid-template-columns rule in sessions list styles and replacing minmax(116px,
116px) with 116px.
- Around line 187-195: The CSS hides scrollbars completely for .session-list
(using scrollbar-width: none, -ms-overflow-style: none and
.session-list::-webkit-scrollbar { display: none }), which hurts accessibility;
change this to provide a minimal visible scrollbar by removing the hard hide and
instead use a subtle, low-opacity scrollbar that becomes more visible on :hover
or :focus-within of .session-list (update rules targeting .session-list,
.session-list:hover, .session-list:focus-within and
.session-list::-webkit-scrollbar to use thin/auto styles and low-opacity colors
rather than none, and remove -ms-overflow-style: none and scrollbar-width: none
so assistive discoverability is preserved).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ee322e0-af30-45bc-b187-4f9bbafc2155
📒 Files selected for processing (16)
tests/unit/compact-layout-ui.test.mjstests/unit/config-tabs-ui.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/app.methods.install.mjsweb-ui/modules/config-mode.computed.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-docs.htmlweb-ui/styles.cssweb-ui/styles/docs-panel.cssweb-ui/styles/modals-core.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/sessions-list.css
💤 Files with no reviewable changes (2)
- web-ui/partials/index/modals-basic.html
- web-ui/modules/app.methods.install.mjs
✅ Files skipped from review due to trivial changes (5)
- web-ui/index.html
- web-ui/styles.css
- web-ui/modules/config-mode.computed.mjs
- web-ui/styles/modals-core.css
- web-ui/styles/docs-panel.css
🚧 Files skipped from review as they are similar to previous changes (4)
- web-ui/app.js
- tests/unit/web-ui-behavior-parity.test.mjs
- tests/unit/compact-layout-ui.test.mjs
- web-ui/styles/navigation-panels.css
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-docs.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (12)
web-ui/partials/index/panel-docs.html (2)
1-6: Static analysis hint is a false positive for partial templates.The HTMLHint
doctype-firsterror can be ignored here—this file is an HTML partial included via the mainindex.htmland is not meant to be a standalone document.
84-101: LGTM on the command card iteration and copy logic.The
v-foriteration overinstallTargetCardswith a composite key includinginstallCommandActionensures proper re-rendering. The:disabled="!target.command"binding combined with the defensive guard incopyInstallCommandprovides good protection against empty commands.web-ui/partials/index/layout-header.html (5)
234-236: Extract duplicated ternary chains to computed properties.The nested ternary chains for
panel-kicker,main-title, andsubtitle(lines 234-236) duplicate logic that also appears inconfig-mode.computed.mjs(inspectorMainTabLabel). Additionally, theinstallCommandActionlabel ternary is repeated at lines 207, 329 in this file and line 67 inpanel-docs.html.Consider creating computed properties (e.g.,
mainTabKicker,mainTabTitle,mainTabSubtitle,installCommandActionLabel) to centralize these mappings and prevent drift.
66-75: LGTM on the new docs tab button.The docs tab is properly wired with consistent patterns:
id,role="tab",data-main-tab,:tabindex,:aria-selected,aria-controls,:class, and the pointer/click handlers matching other tabs.
195-210: LGTM on the docs side navigation section.The docs section follows the established pattern with proper
role="navigation",aria-label,id,:aria-current, and consistent event handlers.
240-277: LGTM on the config mode status-strip organization.The explicit
<template>blocks per config mode (isProviderConfigMode,configMode === 'claude',configMode === 'openclaw') with appropriate fallback provide clear, maintainable status chip rendering. The|| '未选择'fallbacks ensure a consistent UX when no selection exists.
322-335: LGTM on the docs status-strip.The docs panel status chips correctly display package manager, action, and registry preview, consistent with the panel-docs.html summary strip.
tests/unit/config-tabs-ui.test.mjs (3)
60-88: LGTM on the comprehensive docs tab test coverage.The assertions thoroughly verify the new docs tab wiring end-to-end: both top and side navigation elements, data attributes, event handlers, ARIA attributes, panel ID, and content identifiers (
installTargetCards,installTroubleshootingTips).
247-248: LGTM on modal removal assertions.The
doesNotMatchassertions forinstall-cli-modal-titleandshowInstallModalcorrectly verify the CLI install modal has been removed, aligning with the parity test'sallowedMissingCurrentKeyslist and the modals-basic.html context showing no install modal exists.
438-444: LGTM on updated CSS dimension assertions.The updated assertions correctly match the CSS changes: grid columns at
116px, button heights at38px, and responsivemin-height: 44px.web-ui/styles/responsive.css (2)
106-112: LGTM on the.main-panel-topbarneutralization at 720px.The reset to
position: static,background: transparent, andbackdrop-filter: nonecorrectly follows the established pattern frombody.force-compact(as shown in context snippet 2) and prevents layout bleed from the sticky/blur behavior defined in the base rule.
317-327: LGTM on responsive trash action button sizing.The
height: 44px,min-height: 44px,line-height: 1, andwhite-space: nowrapadjustments ensure consistent touch-friendly sizing at narrow viewports, aligning with the base styles insessions-list.css.
|
@coderabbitai If we merge this PR as-is right now, do you see any likely regression risks, baseline anomalies, or user-visible UX regressions that still need attention? Please focus on silent regressions that CI may miss, layout/interaction regressions, and any changes that could degrade the current user experience after merge. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me scan for dangling references to removed symbols before giving you a final answer. 🏁 Script executed: Length of output: 2816 🏁 Script executed: Length of output: 15434
🔴 High — Silent functional regression risks1. A search across all JS/MJS files turned up zero definitions of these two computed properties. They are actively used in the new 2.
3. A full-codebase search for these class names returned zero matches across all file types. The AI summary describes the sidebar brand area being restructured using these classes. If the CSS defines selectors that no HTML element carries (or vice versa), the brand/workspace area in the non- 🟠 Medium — Layout/interaction regressions4. Navigation dead zone at 721–960 px (tablet viewport) From
At 721–960 px (common iPad landscape / narrow browser window), top tabs are gone and the side-rail is the only navigation. If a user has their browser at ~768–900 px wide (very common for split-screen on desktop or tablet landscape), they lose the top tab strip entirely with no obvious affordance that tabs still exist. Any user who previously navigated via the top tab bar at those widths will find it silently absent. 5. The 6. Hidden scrollbars across all list views
🟡 Low — Worth noting before merge7. The status-strip was restructured to show new chip pairs per 8. The Summary table:
I'd recommend manually verifying items 1–3 before merging, as they are the most likely to produce a silently broken UX that no automated test currently covers. 🧠 Learnings used |
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Style
Behavior
Tests
Docs