refactor(web-ui): UI polish, static URL, skills redesign, and v0.0.35 bump#170
Merged
Conversation
Contributor
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 16 minutes and 53 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughWave-based usage UI with enriched session data, server/client /web-ui canonicalization, skills market/menu with single-item import, provider-disable UX and styles, source-bundle template precompilation, and many unit/E2E test updates; package version bumped. ChangesUsage UI and data
Web UI routing and normalization
Sessions URL & share sync
Skills market and import
Provider toggles and disabled-state UI
Source bundling
Tests (unit & E2E)
Accessibility & misc UI
CLI / server changes
Package manifest
Estimated code review effort: Possibly related PRs:
Suggested reviewers:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (2)
27-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI will fail:
package.jsonis missing scripts referenced by.github/workflows/ci.yml
.github/workflows/ci.ymlrunsnpm run ci:install,npm run ci:lint, andnpm run ci:test, butpackage.jsonno longer definesci:install,ci:lint, orci:testin itsscriptssection (so these steps will fail immediately).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 27 - 35, Add the missing CI scripts so the workflow steps succeed: define "ci:install" (e.g., "npm ci" or the project's install command), "ci:lint" (e.g., run your linter like "eslint ." or delegate to an existing "lint" script), and "ci:test" (e.g., "npm test" or the project's test command) in the package.json "scripts" object; ensure the names exactly match ci:install, ci:lint, and ci:test so .github/workflows/ci.yml can run them, and adjust the specific commands to match the repo's existing lint/test toolchain if different.
27-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRestore the CI/test script contract in
package.json
tests/unit/ci-workflow-contract.test.mjsasserts exactpackage.jsonscriptsentries for:
ci:install:node tools/ci/run-check.js installci:lint:node tools/ci/run-check.js lintci:test:node tools/ci/run-check.js testtest:npm run test:unit && npm run test:e2etest:ci:node tools/ci/run-check.js alltest:unit:node tests/unit/run.mjstest:e2e:node tests/e2e/run.jsNone of these scripts are present in the current
package.json, so this unit test will fail and contradict any “version change only / Tests: N/A” claim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 27 - 35, Restore the missing npm script entries in package.json so they match the CI/test contract checked by tests/unit/ci-workflow-contract.test.mjs: add script keys "ci:install", "ci:lint", "ci:test", "test", "test:ci", "test:unit", and "test:e2e" with the exact command strings required by the test (e.g., "ci:install" -> node tools/ci/run-check.js install, "ci:lint" -> node tools/ci/run-check.js lint, "ci:test" -> node tools/ci/run-check.js test, "test" -> npm run test:unit && npm run test:e2e, "test:ci" -> node tools/ci/run-check.js all, "test:unit" -> node tests/unit/run.mjs, "test:e2e" -> node tests/e2e/run.js) so the scripts keys (ci:install, ci:lint, ci:test, test, test:ci, test:unit, test:e2e) exactly match the expected strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 3: The package.json version was bumped to "0.0.34" but package-lock.json
still lists "0.1.0"; regenerate the lockfile so both agree by running a lockfile
update (e.g., run npm install or npm install --package-lock-only) which will
update packages[""].version and the top-level version in package-lock.json to
"0.0.34", then add and commit the regenerated package-lock.json alongside
package.json.
---
Outside diff comments:
In `@package.json`:
- Around line 27-35: Add the missing CI scripts so the workflow steps succeed:
define "ci:install" (e.g., "npm ci" or the project's install command), "ci:lint"
(e.g., run your linter like "eslint ." or delegate to an existing "lint"
script), and "ci:test" (e.g., "npm test" or the project's test command) in the
package.json "scripts" object; ensure the names exactly match ci:install,
ci:lint, and ci:test so .github/workflows/ci.yml can run them, and adjust the
specific commands to match the repo's existing lint/test toolchain if different.
- Around line 27-35: Restore the missing npm script entries in package.json so
they match the CI/test contract checked by
tests/unit/ci-workflow-contract.test.mjs: add script keys "ci:install",
"ci:lint", "ci:test", "test", "test:ci", "test:unit", and "test:e2e" with the
exact command strings required by the test (e.g., "ci:install" -> node
tools/ci/run-check.js install, "ci:lint" -> node tools/ci/run-check.js lint,
"ci:test" -> node tools/ci/run-check.js test, "test" -> npm run test:unit && npm
run test:e2e, "test:ci" -> node tools/ci/run-check.js all, "test:unit" -> node
tests/unit/run.mjs, "test:e2e" -> node tests/e2e/run.js) so the scripts keys
(ci:install, ci:lint, ci:test, test, test:ci, test:unit, test:e2e) exactly match
the expected strings.
🪄 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: 73bbea5c-ac4b-4fc2-a2a1-099b9279b9af
📒 Files selected for processing (1)
package.json
📜 Review details
🔇 Additional comments (1)
package.json (1)
27-35: ⚡ Quick winPR objectives claim "version change only" but scripts were removed.
The PR description states "N/A — version change only" for tests, but the AI summary and the annotated changes show that multiple scripts have been removed from the
scriptssection, including:
- CI-related scripts (
ci:install,ci:lint,ci:test)- Test-related scripts (
test,test:ci,test:unit,test:e2e)- Other development scripts
This is not a "version change only" PR. The removal of these scripts has significant implications for CI workflows and test execution.
- Merge current session bar with hero metrics for unified focus - Replace bar chart with SVG bezier curve wave visualization - Add automatic delta calculation (7d vs prior 7d, 30d vs prior 30d) - Redesign empty states with animated SVG illustrations - Simplify list views with bullet-point compact style - Enhance color system with delta indicators and heatmap levels
ymkiux
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/modules/app.computed.session.mjs`:
- Around line 608-610: The logic currently computes prevKey (and similarly
deltaKey at the other location) even when sessionsUsageTimeRange === 'all',
causing compare text to appear incorrectly; update the code that sets prevKey
(using baseMs, rangeDays, dayMs) to first check that sessionsUsageTimeRange !==
'all' (and that baseMs is finite) and only then compute the date string,
otherwise set prevKey to '' (apply the same guard to the other occurrence around
deltaKey at the later block). Ensure you reference the existing variables
prevKey, baseMs, rangeDays, dayMs and sessionsUsageTimeRange so the compute is
skipped when range === 'all'.
- Around line 717-726: usageHeroDelta and usageHeroDeltaClass compute previous
tokens from this.sessionsUsageList while current tokens are taken from
sessionUsageCharts.summary (filtered to supported sources), causing inconsistent
deltas; update the calculations so both current and previous use the same
dataset and filters — either switch the previous-period aggregation in the loop
to use sessionUsageCharts.summary (apply the same supported-source filter and
time-window logic) or apply the exact supported-source filter and aggregation
logic used by sessionUsageCharts.summary to this.sessionsUsageList; adjust the
logic in the methods/blocks named usageHeroDelta and usageHeroDeltaClass (also
mirror the change where similar logic appears around the other block at the
754-763 region) so both sides of the delta are computed from the identical
filtered dataset and token-aggregation routine.
In `@web-ui/partials/index/panel-usage.html`:
- Around line 95-99: Replace the non-interactive <span> elements used for
session wave labels with semantic interactive elements (e.g., <button
type="button">) so keyboard users can operate them; keep the same Vue bindings
and class names (sessionUsageWave.labels, :key="label.key", :class="{ active:
sessionsUsageSelectedDay === label.key }",
`@click`="selectSessionsUsageDay(label.key)", usage-wave-label) and add any
necessary accessible attributes (e.g., aria-pressed or aria-label) as
appropriate; make the same change for the other identical blocks referenced (the
occurrences around the other ranges mentioned).
- Around line 58-60: Replace the hardcoded English labels ("tokens", "API",
"total", "msgs") with your i18n translation calls so the UI uses localization
keys instead of literal strings; update the template expressions that build
usage labels (for example where usageCurrentSessionStats.tokenLabel,
usageCurrentSessionStats.apiDurationLabel,
usageCurrentSessionStats.totalDurationLabel, and any
usageCurrentSessionStats.msgs label are rendered) to use the project's
translation helper (e.g., $t or t) and corresponding keys (like "usage.tokens",
"usage.api", "usage.total", "usage.msgs"); apply the same change to the other
occurrences mentioned (lines around 106-109, 147, 162) so all literal labels are
replaced by translation lookups.
In `@web-ui/styles/sessions-usage.css`:
- Around line 303-305: The CSS selector .usage-card-hourly-heatmap doesn't match
the template's class .usage-hourly-heatmap so the overflow rule is never
applied; update the style rule to target .usage-hourly-heatmap (or alternatively
change the template class to .usage-card-hourly-heatmap) so the overflow-x: auto
style takes effect—look for the selector in the CSS file and align it with the
template class name (.usage-hourly-heatmap).
🪄 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: cfa3fabf-df01-400e-b94d-48311659d7a4
📒 Files selected for processing (5)
web-ui/logic.sessions.mjsweb-ui/modules/app.computed.session.mjsweb-ui/partials/index/panel-usage.htmlweb-ui/styles/base-theme.cssweb-ui/styles/sessions-usage.css
📜 Review details
🔇 Additional comments (2)
web-ui/logic.sessions.mjs (1)
552-553: LGTM!Also applies to: 559-560
web-ui/styles/base-theme.css (1)
29-37: LGTM!
Comment on lines
+608
to
610
| const prevKey = Number.isFinite(baseMs) | ||
| ? new Date(baseMs - (rangeDays * dayMs)).toISOString().slice(0, 10) | ||
| : ''; |
Contributor
There was a problem hiding this comment.
Selected-day “prev/delta” should be gated when range is all.
For sessionsUsageTimeRange === 'all', prevKey is still generated (using 7-day offset), so compare text appears even when compare mode is not applicable.
Suggested fix
+ const compareEnabled = this.sessionsUsageTimeRange !== 'all';
const prevKey = Number.isFinite(baseMs)
- ? new Date(baseMs - (rangeDays * dayMs)).toISOString().slice(0, 10)
+ && compareEnabled
+ ? new Date(baseMs - (rangeDays * dayMs)).toISOString().slice(0, 10)
: '';Also applies to: 675-676
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.computed.session.mjs` around lines 608 - 610, The logic
currently computes prevKey (and similarly deltaKey at the other location) even
when sessionsUsageTimeRange === 'all', causing compare text to appear
incorrectly; update the code that sets prevKey (using baseMs, rangeDays, dayMs)
to first check that sessionsUsageTimeRange !== 'all' (and that baseMs is finite)
and only then compute the date string, otherwise set prevKey to '' (apply the
same guard to the other occurrence around deltaKey at the later block). Ensure
you reference the existing variables prevKey, baseMs, rangeDays, dayMs and
sessionsUsageTimeRange so the compute is skipped when range === 'all'.
Comment on lines
+717
to
+726
| for (const session of (Array.isArray(this.sessionsUsageList) ? this.sessionsUsageList : [])) { | ||
| if (!session || typeof session !== 'object') continue; | ||
| const updatedAtMs = Date.parse(session.updatedAt || ''); | ||
| if (!Number.isFinite(updatedAtMs)) continue; | ||
| if (updatedAtMs >= prevStartMs && updatedAtMs < prevEndMs) { | ||
| const sessionTokens = Number.isFinite(Number(session.totalTokens)) | ||
| ? Math.max(0, Math.floor(Number(session.totalTokens))) | ||
| : 0; | ||
| prevTokens += sessionTokens; | ||
| } |
Contributor
There was a problem hiding this comment.
Delta is computed from a different dataset than the headline total.
usageHeroDelta/usageHeroDeltaClass derive previous tokens from this.sessionsUsageList, while current tokens come from sessionUsageCharts.summary (filtered to supported sources). This can skew delta direction and percentage.
Suggested fix
- for (const session of (Array.isArray(this.sessionsUsageList) ? this.sessionsUsageList : [])) {
+ const comparableSessions = this.sessionUsageCharts && Array.isArray(this.sessionUsageCharts.filteredSessions)
+ ? this.sessionUsageCharts.filteredSessions
+ : (Array.isArray(this.sessionsUsageList) ? this.sessionsUsageList : []);
+ for (const session of comparableSessions) {Also applies to: 754-763
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.computed.session.mjs` around lines 717 - 726,
usageHeroDelta and usageHeroDeltaClass compute previous tokens from
this.sessionsUsageList while current tokens are taken from
sessionUsageCharts.summary (filtered to supported sources), causing inconsistent
deltas; update the calculations so both current and previous use the same
dataset and filters — either switch the previous-period aggregation in the loop
to use sessionUsageCharts.summary (apply the same supported-source filter and
time-window logic) or apply the exact supported-source filter and aggregation
logic used by sessionUsageCharts.summary to this.sessionsUsageList; adjust the
logic in the methods/blocks named usageHeroDelta and usageHeroDeltaClass (also
mirror the change where similar logic appears around the other block at the
754-763 region) so both sides of the delta are computed from the identical
filtered dataset and token-aggregation routine.
Comment on lines
+58
to
+60
| <span class="usage-hero-active-stat">{{ usageCurrentSessionStats.tokenLabel }} tokens</span> | ||
| <span class="usage-hero-active-stat">{{ usageCurrentSessionStats.apiDurationLabel }} API</span> | ||
| <span class="usage-hero-active-stat">{{ usageCurrentSessionStats.totalDurationLabel }} total</span> |
Contributor
There was a problem hiding this comment.
New hardcoded English labels bypass i18n.
Strings like tokens, API, total, and msgs should use translation keys to avoid mixed-language UI.
Also applies to: 106-109, 147-147, 162-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/partials/index/panel-usage.html` around lines 58 - 60, Replace the
hardcoded English labels ("tokens", "API", "total", "msgs") with your i18n
translation calls so the UI uses localization keys instead of literal strings;
update the template expressions that build usage labels (for example where
usageCurrentSessionStats.tokenLabel, usageCurrentSessionStats.apiDurationLabel,
usageCurrentSessionStats.totalDurationLabel, and any
usageCurrentSessionStats.msgs label are rendered) to use the project's
translation helper (e.g., $t or t) and corresponding keys (like "usage.tokens",
"usage.api", "usage.total", "usage.msgs"); apply the same change to the other
occurrences mentioned (lines around 106-109, 147, 162) so all literal labels are
replaced by translation lookups.
Comment on lines
+95
to
+99
| <span v-for="label in sessionUsageWave.labels" :key="label.key" | ||
| class="usage-wave-label" | ||
| :class="{ active: sessionsUsageSelectedDay === label.key }" | ||
| @click="selectSessionsUsageDay(label.key)"> | ||
| {{ label.text }} |
Contributor
There was a problem hiding this comment.
Clickable non-button elements break keyboard accessibility.
span/div elements with @click here are not keyboard-operable by default. Please switch to semantic <button type="button"> (or add tabindex + key handlers) for these interactive items.
Also applies to: 143-149, 158-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/partials/index/panel-usage.html` around lines 95 - 99, Replace the
non-interactive <span> elements used for session wave labels with semantic
interactive elements (e.g., <button type="button">) so keyboard users can
operate them; keep the same Vue bindings and class names
(sessionUsageWave.labels, :key="label.key", :class="{ active:
sessionsUsageSelectedDay === label.key }",
`@click`="selectSessionsUsageDay(label.key)", usage-wave-label) and add any
necessary accessible attributes (e.g., aria-pressed or aria-label) as
appropriate; make the same change for the other identical blocks referenced (the
occurrences around the other ranges mentioned).
Comment on lines
303
to
305
| .usage-card-hourly-heatmap { | ||
| overflow-x: auto; | ||
| } |
Contributor
There was a problem hiding this comment.
Hourly heatmap container selector doesn’t match template class.
CSS targets .usage-card-hourly-heatmap, but the template uses .usage-hourly-heatmap. The overflow behavior won’t apply.
Suggested fix
-.usage-card-hourly-heatmap {
+.usage-hourly-heatmap {
overflow-x: auto;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .usage-card-hourly-heatmap { | |
| overflow-x: auto; | |
| } | |
| .usage-hourly-heatmap { | |
| overflow-x: auto; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/styles/sessions-usage.css` around lines 303 - 305, The CSS selector
.usage-card-hourly-heatmap doesn't match the template's class
.usage-hourly-heatmap so the overflow rule is never applied; update the style
rule to target .usage-hourly-heatmap (or alternatively change the template class
to .usage-card-hourly-heatmap) so the overflow-x: auto style takes effect—look
for the selector in the CSS file and align it with the template class name
(.usage-hourly-heatmap).
- Server: handle /web-ui/ requests by returning index.html - Server: explicitly serve index.html for /web-ui/index.html requests - Client: normalize URL on mount to fix /web-ui/web-ui/ duplicates - Client: redirect /web-ui/ and /web-ui/index.html to /web-ui Fixes the issue where refreshing in sessions tab caused URL to become /web-ui/web-ui/index.html
- Use do-while loop to iteratively remove all /web-ui/web-ui/ patterns - Handle edge cases like /web-ui/web-ui/web-ui/index.html - Add additional check for /web-ui/web-ui/ prefix pattern
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/app.js`:
- Around line 434-438: The normalization only handles '/web-ui/web-ui/' with a
trailing slash, leaving '/web-ui/web-ui' unmodified; update the logic around
url.pathname (the replace and startsWith checks) to also detect the exact
'/web-ui/web-ui' case (or broaden the regex to match both with and without
trailing slash) and rewrite it to '/web-ui' so both '/web-ui/web-ui' and
'/web-ui/web-ui/' are canonicalized to '/web-ui'.
🪄 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: dc579630-149a-4e25-b86f-b8d8d2ccbeda
📒 Files selected for processing (1)
web-ui/app.js
- Add test-web-ui-url-routing.js E2E test for server routing - Add test-web-ui-url-normalization.mjs unit test (24 cases) - Update test-web-ui-assets.js expectations for /web-ui/ fix - Fix shouldReplace check logic in app.js (use originalPathname)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/test-web-ui-url-routing.js`:
- Around line 198-208: The for-loop over htmlResponses uses a const loop counter
which causes a runtime error when incrementing i; change the declaration in the
loop header from const to let (for example in the loop that reads "for (const i
= 0; i < htmlResponses.length; i++)") so i is mutable, leaving the assertions
against htmlResponses[i], response.body.includes, and the error messages
unchanged.
🪄 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: cad651be-3bb4-452b-b0e8-4b8c75efb327
📒 Files selected for processing (4)
tests/e2e/test-web-ui-assets.jstests/e2e/test-web-ui-url-routing.jstests/unit/test-web-ui-url-normalization.mjsweb-ui/app.js
📜 Review details
🔇 Additional comments (7)
web-ui/app.js (2)
430-444: Approved with note: past review comment still applies.The fix to capture
originalPathnameat line 430 and use it for the comparison at line 442 correctly ensuresshouldReplaceis set when the pathname has changed from its initial value.However, the past review comment regarding
/web-ui/web-ui(without trailing slash) not being normalized remains valid. The current regex and startsWith checks only handle paths with trailing slashes like/web-ui/web-ui/.
422-429: LGTM!Also applies to: 445-459
tests/e2e/test-web-ui-assets.js (1)
66-75: LGTM!tests/e2e/test-web-ui-url-routing.js (1)
1-197: LGTM!tests/unit/test-web-ui-url-normalization.mjs (3)
10-44: LGTM!
49-187: LGTM!
189-249: LGTM!
Comment on lines
+198
to
+208
| for (const i = 0; i < htmlResponses.length; i++) { | ||
| const response = htmlResponses[i]; | ||
| assert( | ||
| response.body.includes('src="/web-ui/app.js"'), | ||
| `HTML response ${i} should use absolute path for app.js` | ||
| ); | ||
| assert( | ||
| !response.body.includes('src="web-ui/app.js"'), | ||
| `HTML response ${i} should not use relative path for app.js` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix syntax error: loop counter must use let, not const.
The loop declares i as const but then increments it, which will throw a runtime error.
🐛 Proposed fix
- for (const i = 0; i < htmlResponses.length; i++) {
+ for (let i = 0; i < htmlResponses.length; i++) {
const response = htmlResponses[i];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const i = 0; i < htmlResponses.length; i++) { | |
| const response = htmlResponses[i]; | |
| assert( | |
| response.body.includes('src="/web-ui/app.js"'), | |
| `HTML response ${i} should use absolute path for app.js` | |
| ); | |
| assert( | |
| !response.body.includes('src="web-ui/app.js"'), | |
| `HTML response ${i} should not use relative path for app.js` | |
| ); | |
| } | |
| for (let i = 0; i < htmlResponses.length; i++) { | |
| const response = htmlResponses[i]; | |
| assert( | |
| response.body.includes('src="/web-ui/app.js"'), | |
| `HTML response ${i} should use absolute path for app.js` | |
| ); | |
| assert( | |
| !response.body.includes('src="web-ui/app.js"'), | |
| `HTML response ${i} should not use relative path for app.js` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test-web-ui-url-routing.js` around lines 198 - 208, The for-loop
over htmlResponses uses a const loop counter which causes a runtime error when
incrementing i; change the declaration in the loop header from const to let (for
example in the loop that reads "for (const i = 0; i < htmlResponses.length;
i++)") so i is mutable, leaving the assertions against htmlResponses[i],
response.body.includes, and the error messages unchanged.
- Server: /web-ui, /web-ui/, /web-ui/index.html now return 404 - Root path / is the only HTML entry point - Client: auto-redirect from /web-ui/* to / with hash/params preserved - Assets still served at /web-ui/app.js, /web-ui/styles.css, etc. - Simplified routing logic, removed redundant path handling
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/test-web-ui-url-routing.js (1)
194-201:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix logic error: these responses are 404, not HTML.
Lines 66-76 assert that
webUiNoSlashandwebUiWithSlashhave status code 404. Per the server implementation (cli.js:11424), 404 responses return plain text "Not Found", not HTML. These assertions will fail because the response body does not contain'id="app"'.🐛 Proposed fix: remove invalid HTML content checks on 404 responses
- // 验证 /web-ui 和 /web-ui/ 返回相同内容（除了可能的细微差异） - assert( - webUiNoSlash.body.includes('id="app"'), - '/web-ui response should contain Vue app' - ); - assert( - webUiWithSlash.body.includes('id="app"'), - '/web-ui/ response should contain Vue app' - );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test-web-ui-url-routing.js` around lines 194 - 201, The assertions that check HTML content on 404 responses are incorrect—remove the two asserts that test webUiNoSlash.body.includes('id="app"') and webUiWithSlash.body.includes('id="app"') and either omit body checks entirely or replace them with a plain-text 404 check (e.g., assert the body contains 'Not Found') so the tests align with the server behavior for the webUiNoSlash and webUiWithSlash responses.
♻️ Duplicate comments (2)
web-ui/app.js (1)
428-433:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe regex still doesn't handle
/web-ui/web-uiwithout a trailing slash.The pattern
/\/+web-ui\/+web-ui\/+/grequires at least one slash after the secondweb-ui, so a path like/web-ui/web-ui(no trailing slash) remains unchanged and won't be normalized to/web-ui.🔧 Suggested fix
Match zero or more trailing slashes to handle both cases:
do { prevPathname = pathname; - pathname = pathname.replace(/\/+web-ui\/+web-ui\/+/g, '/web-ui/'); + pathname = pathname.replace(/\/+web-ui\/+web-ui\/*/g, '/web-ui/'); } while (pathname !== prevPathname);Alternatively, use a lookahead to preserve exact trailing-slash behavior:
- pathname = pathname.replace(/\/+web-ui\/+web-ui\/+/g, '/web-ui/'); + pathname = pathname.replace(/\/+web-ui\/+web-ui(?=\/|$)/g, '/web-ui');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/app.js` around lines 428 - 433, The current loop that normalizes repeated "/web-ui/web-ui" uses pathname.replace(/\/+web-ui\/+web-ui\/+/g, '/web-ui/'), which misses cases like "/web-ui/web-ui" (no trailing slash); update the replace call on pathname (the code referencing prevPathname and pathname) to use a pattern that allows zero or more trailing slashes or a lookahead (e.g., match "/+web-ui/+web-ui/*" or "/+web-ui/+web-ui(?=\/|$)"), and replace with the normalized "/web-ui" (or "/web-ui/" if you want to keep a trailing slash consistently) so the do/while loop cleans both "/web-ui/web-ui" and "/web-ui/web-ui/" forms.tests/e2e/test-web-ui-url-routing.js (1)
204-215:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix two issues: loop counter must use
let, and HTML checks on 404 responses.
- The loop counter
iis declared asconstbut incremented, which will throw a runtime error.- The
htmlResponsesarray includeswebUiNoSlash,webUiWithSlash, andwebUiIndexHtml, which are all 404 responses (asserted at lines 67-82). These 404 responses return plain text "Not Found", not HTML, so checking for'src="/web-ui/app.js"'will fail.🐛 Proposed fix: use `let` and remove invalid HTML checks
- // 验证所有 HTML 响应都指向正确的静态资源路径 - const htmlResponses = [webUiNoSlash, webUiWithSlash, webUiIndexHtml]; - for (const i = 0; i < htmlResponses.length; i++) { - const response = htmlResponses[i]; - assert( - response.body.includes('src="/web-ui/app.js"'), - `HTML response ${i} should use absolute path for app.js` - ); - assert( - !response.body.includes('src="web-ui/app.js"'), - `HTML response ${i} should not use relative path for app.js` - ); - }Replace with checks on actual HTML responses:
+ // 验证实际的 HTML 响应（根路径）指向正确的静态资源路径 + assert( + rootPath.body.includes('src="/web-ui/app.js"'), + 'Root HTML response should use absolute path for app.js' + ); + assert( + !rootPath.body.includes('src="web-ui/app.js"'), + 'Root HTML response should not use relative path for app.js' + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test-web-ui-url-routing.js` around lines 204 - 215, Change the loop counter declaration from const to let for the for-loop that iterates htmlResponses (variable names: htmlResponses and i). Remove the invalid HTML assertions against the known-404 responses webUiNoSlash, webUiWithSlash, and webUiIndexHtml; instead assert their status is 404 and that response.body includes the plain-text "Not Found". If you still need to test HTML asset paths, iterate only over actual HTML responses (not webUiNoSlash/webUiWithSlash/webUiIndexHtml) or filter htmlResponses to exclude 404 entries before performing the 'src="/web-ui/app.js"' checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/e2e/test-web-ui-url-routing.js`:
- Around line 194-201: The assertions that check HTML content on 404 responses
are incorrect—remove the two asserts that test
webUiNoSlash.body.includes('id="app"') and
webUiWithSlash.body.includes('id="app"') and either omit body checks entirely or
replace them with a plain-text 404 check (e.g., assert the body contains 'Not
Found') so the tests align with the server behavior for the webUiNoSlash and
webUiWithSlash responses.
---
Duplicate comments:
In `@tests/e2e/test-web-ui-url-routing.js`:
- Around line 204-215: Change the loop counter declaration from const to let for
the for-loop that iterates htmlResponses (variable names: htmlResponses and i).
Remove the invalid HTML assertions against the known-404 responses webUiNoSlash,
webUiWithSlash, and webUiIndexHtml; instead assert their status is 404 and that
response.body includes the plain-text "Not Found". If you still need to test
HTML asset paths, iterate only over actual HTML responses (not
webUiNoSlash/webUiWithSlash/webUiIndexHtml) or filter htmlResponses to exclude
404 entries before performing the 'src="/web-ui/app.js"' checks.
In `@web-ui/app.js`:
- Around line 428-433: The current loop that normalizes repeated
"/web-ui/web-ui" uses pathname.replace(/\/+web-ui\/+web-ui\/+/g, '/web-ui/'),
which misses cases like "/web-ui/web-ui" (no trailing slash); update the replace
call on pathname (the code referencing prevPathname and pathname) to use a
pattern that allows zero or more trailing slashes or a lookahead (e.g., match
"/+web-ui/+web-ui/*" or "/+web-ui/+web-ui(?=\/|$)"), and replace with the
normalized "/web-ui" (or "/web-ui/" if you want to keep a trailing slash
consistently) so the do/while loop cleans both "/web-ui/web-ui" and
"/web-ui/web-ui/" forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4e90178-56a8-4919-bb3e-12e8dac438b8
📒 Files selected for processing (5)
cli.jstests/e2e/test-web-ui-assets.jstests/e2e/test-web-ui-url-routing.jstests/unit/test-web-ui-url-normalization.mjsweb-ui/app.js
📜 Review details
🔇 Additional comments (13)
cli.js (2)
11422-11425: LGTM!
11524-11535: LGTM!tests/e2e/test-web-ui-assets.js (1)
47-53: LGTM!tests/e2e/test-web-ui-url-routing.js (5)
1-30: LGTM!
32-43: LGTM!
44-94: LGTM!
96-135: LGTM!
137-189: LGTM!tests/unit/test-web-ui-url-normalization.mjs (5)
1-9: LGTM!
10-35: LGTM!
40-169: LGTM!
174-217: LGTM!
219-231: LGTM!
- filter out 'local' provider from displayProvidersList when configMode is codex/claude - prevents users from selecting local provider in these apps due to existing issues - other modes (terminal) remain unaffected
- local provider card shows as disabled (50% opacity, non-clickable) in codex mode - claude-local bridge card shows as disabled in claude mode - adds CSS .card.disabled style with pointer-events: none - cards remain visible but cannot be selected
- fix operator precedence issue in click handlers - use proper ternary expressions with correct grouping - prevents white screen error on config load
- isLocalProviderDisabled is a computed returning boolean, not a function - should be accessed as property without () in template - fixes white screen error on config load
- display name changed from 'claude-local' to 'local' - adds system badge like codex provider readonly indicator - internal id 'claude-local' remains unchanged for compatibility
- removes '勾选参与负载均衡的提供商' subtitle - keeps card layout cleaner with just title and system badge
- Remove cluttered overview section with 6 stat cards - Consolidate header with inline target switcher - Single-column flow layout for better mobile experience - Add count badges directly in panel headers - Add quick action buttons in import panel - Add individual import button for each skill - Remove help panel (content available elsewhere) - Remove root path display (moved to tooltip) - Preserve all functionality while reducing visual noise Design principles: - Progressive disclosure: scroll to discover - Large touch targets: 56px min height - Visual hierarchy: clear content grouping - Cleaner information architecture
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-ui/modules/skills.methods.mjs (1)
488-488: ⚡ Quick winAlign single-import in-flight guards with other mutating actions.
This guard omits
skillsDeleting,skillsScanningImports, andskillsExporting, so this method can overlap with other mutations if called outside the disabled UI path.Proposed fix
- if (this.skillsImporting || this.skillsZipImporting) return; + if ( + this.skillsDeleting + || this.skillsScanningImports + || this.skillsImporting + || this.skillsZipImporting + || this.skillsExporting + ) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/skills.methods.mjs` at line 488, The single-import guard currently checks only skillsImporting and skillsZipImporting, allowing this method to run concurrently with other mutations; update the condition (the if that reads "if (this.skillsImporting || this.skillsZipImporting) return;") to also include this.skillsDeleting, this.skillsScanningImports, and this.skillsExporting so the method returns whenever any skills mutating flag is set (or replace with a single helper like isMutatingSkills that ORs those flags and use it here).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/modules/skills.methods.mjs`:
- Around line 492-495: The single-import branch in importSelectedSkills() uses
the wrong payload key ("imports") for the 'import-skills' API call; change the
payload to match the bulk-import contract by sending items: [skill] (i.e., use
items instead of imports) while keeping targetApp: this.skillsTargetApp so the
backend reads the item correctly and the single-skill import is not a no-op.
In `@web-ui/partials/index/panel-market.html`:
- Around line 97-99: The import button (button with class "skills-flow-add" that
calls importSingleSkill(skill)) is icon-only and lacks an accessible name; add
an explicit accessible label by setting an aria-label or adding visually-hidden
text that includes the skill context (e.g., dynamic aria-label using skill.name)
and ensure the :disabled and `@click` behavior remains unchanged; update the
button element to include a descriptive attribute like :aria-label="`Import
skill ${skill.name}`" or insert a <span class="sr-only">Import {{ skill.name
}}</span> inside the button so screen readers announce the action.
---
Nitpick comments:
In `@web-ui/modules/skills.methods.mjs`:
- Line 488: The single-import guard currently checks only skillsImporting and
skillsZipImporting, allowing this method to run concurrently with other
mutations; update the condition (the if that reads "if (this.skillsImporting ||
this.skillsZipImporting) return;") to also include this.skillsDeleting,
this.skillsScanningImports, and this.skillsExporting so the method returns
whenever any skills mutating flag is set (or replace with a single helper like
isMutatingSkills that ORs those flags and use it here).
🪄 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: 433d5d46-78a8-4294-9155-71c84652fe02
📒 Files selected for processing (5)
web-ui/modules/skills.methods.mjsweb-ui/partials/index/panel-config-claude.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-market.htmlweb-ui/styles/skills-market.css
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-config-claude.html
[error] 109-109: Special characters must be escaped : [ > ].
(spec-char-escape)
🔇 Additional comments (3)
web-ui/partials/index/panel-config-claude.html (1)
103-114: LGTM!web-ui/partials/index/panel-config-codex.html (1)
123-123: LGTM!web-ui/styles/skills-market.css (1)
1-294: LGTM!
Comment on lines
+492
to
+495
| const res = await api('import-skills', { | ||
| targetApp: this.skillsTargetApp, | ||
| imports: [skill] | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the same import-skills payload contract as bulk import.
importSelectedSkills() sends items, but this path sends imports. If the backend only reads items, single import silently fails or no-ops.
Proposed fix
- const res = await api('import-skills', {
- targetApp: this.skillsTargetApp,
- imports: [skill]
- });
+ const res = await api('import-skills', {
+ targetApp: this.skillsTargetApp,
+ items: [{
+ name: skill && skill.name,
+ sourceApp: skill && skill.sourceApp
+ }]
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/skills.methods.mjs` around lines 492 - 495, The single-import
branch in importSelectedSkills() uses the wrong payload key ("imports") for the
'import-skills' API call; change the payload to match the bulk-import contract
by sending items: [skill] (i.e., use items instead of imports) while keeping
targetApp: this.skillsTargetApp so the backend reads the item correctly and the
single-skill import is not a no-op.
Comment on lines
+97
to
+99
| <button type="button" class="skills-flow-add" @click="importSingleSkill(skill)" :disabled="loading || !!initError || skillsMarketBusy"> | ||
| <svg viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.8" width="16" height="16"><path d="M10 4v6m3-3l-3 3-3-3"/></svg> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the icon-only import button.
This action button has no text/ARIA label, so screen readers won’t announce its purpose.
Proposed fix
- <button type="button" class="skills-flow-add" `@click`="importSingleSkill(skill)" :disabled="loading || !!initError || skillsMarketBusy">
+ <button
+ type="button"
+ class="skills-flow-add"
+ `@click`="importSingleSkill(skill)"
+ :disabled="loading || !!initError || skillsMarketBusy"
+ :aria-label="`${t('market.action.importSingle')}: ${skill.displayName || skill.name}`"
+ :title="`${t('market.action.importSingle')}: ${skill.displayName || skill.name}`">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button type="button" class="skills-flow-add" @click="importSingleSkill(skill)" :disabled="loading || !!initError || skillsMarketBusy"> | |
| <svg viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.8" width="16" height="16"><path d="M10 4v6m3-3l-3 3-3-3"/></svg> | |
| </button> | |
| <button | |
| type="button" | |
| class="skills-flow-add" | |
| `@click`="importSingleSkill(skill)" | |
| :disabled="loading || !!initError || skillsMarketBusy" | |
| :aria-label="`${t('market.action.importSingle')}: ${skill.displayName || skill.name}`" | |
| :title="`${t('market.action.importSingle')}: ${skill.displayName || skill.name}`"> | |
| <svg viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.8" width="16" height="16"><path d="M10 4v6m3-3l-3 3-3-3"/></svg> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/partials/index/panel-market.html` around lines 97 - 99, The import
button (button with class "skills-flow-add" that calls importSingleSkill(skill))
is icon-only and lacks an accessible name; add an explicit accessible label by
setting an aria-label or adding visually-hidden text that includes the skill
context (e.g., dynamic aria-label using skill.name) and ensure the :disabled and
`@click` behavior remains unchanged; update the button element to include a
descriptive attribute like :aria-label="`Import skill ${skill.name}`" or insert
a <span class="sr-only">Import {{ skill.name }}</span> inside the button so
screen readers announce the action.
- Remove all URL routing, URL stays as pure '/' - Delete query parameters and hash from browser address bar - Remove history.pushState/replaceState operations for tab switching - Remove session filter URL synchronization - Share links still work (generate URL with params for clipboard) - When opening share link, params are applied then URL cleaned - All state managed via memory + localStorage Changes: - app.js: Clean URL on mount, remove resource path fix logic - app.methods.navigation.mjs: Remove URL operations in switchMainTab - sessions-filters-url.mjs: Empty syncSessionsFilterUrl, fix share URL base - app.methods.session-browser.mjs: Clean URL after applying URL state
ymkiux
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-ui/modules/sessions-filters-url.mjs (1)
75-78: 💤 Low valueConsider removing no-op function and its call sites.
syncSessionsFilterUrlis now an intentional no-op, but it's still called 6 times acrossapp.methods.session-browser.mjs(lines 481, 506, 512, 518, 576, 587). Calling an empty function creates visual noise and may confuse future maintainers.♻️ Refactor options
Option 1: Remove the function and all call sites.
Option 2: Keep the function as a no-op stub for potential future URL sync requirements (current approach).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/sessions-filters-url.mjs` around lines 75 - 78, Remove the no-op exported function syncSessionsFilterUrl and delete every invocation of syncSessionsFilterUrl across the codebase (there are ~6 call sites in the session-browser methods), and also remove its export/imports; alternatively, if you want to keep a placeholder for future use, leave a single minimal stub named syncSessionsFilterUrl with a clear comment and remove the redundant calls so callers no longer invoke it; ensure imports are updated so there are no unused imports or undefined references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 3: The package.json "version" field was incorrectly set to 0.0.35 while
the PR intention is to revert to 0.0.34; open package.json, change the "version"
property value from "0.0.35" to "0.0.34" (ensure the "version" key is updated),
and then check and update any related metadata (lockfile or CI npm/publish
config) to remain consistent with that version.
In `@web-ui/app.js`:
- Around line 426-434: The redirect code for pathname checks currently clears
url.search and url.hash before redirecting, which discards query/hash; update
the flow so you first parse and apply query params and hash to the app state
(reuse the existing restoreSessionFilterCache logic or call the same routine
used at/near restoreSessionFilterCache) and only then perform a redirect that
preserves or cleans the URL as intended; specifically, in the block that handles
pathname === '/web-ui'... use the code that extracts window.location.search and
window.location.hash, pass them into the same state-restoration function (or
invoke restoreSessionFilterCache) to apply filters/share links, and afterward
call window.location.replace with a URL that either retains the original
search/hash or cleans them according to the PR objective.
---
Nitpick comments:
In `@web-ui/modules/sessions-filters-url.mjs`:
- Around line 75-78: Remove the no-op exported function syncSessionsFilterUrl
and delete every invocation of syncSessionsFilterUrl across the codebase (there
are ~6 call sites in the session-browser methods), and also remove its
export/imports; alternatively, if you want to keep a placeholder for future use,
leave a single minimal stub named syncSessionsFilterUrl with a clear comment and
remove the redundant calls so callers no longer invoke it; ensure imports are
updated so there are no unused imports or undefined references.
🪄 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: 28bd0039-5d23-48c9-9492-b08548019394
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonweb-ui/app.jsweb-ui/modules/app.methods.navigation.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/modules/sessions-filters-url.mjs
📜 Review details
🔇 Additional comments (4)
package.json (1)
3-3: ⚡ Quick winRegenerate package-lock.json to match package.json version.
As flagged in previous reviews, if
package-lock.jsonexists in this repository, it must be regenerated after the version change to ensure consistency.Run the following script to check if
package-lock.jsonneeds updating:#!/bin/bash # Check if package-lock.json exists and verify version consistency if [ -f package-lock.json ]; then echo "=== package.json version ===" jq -r '.version' package.json echo -e "\n=== package-lock.json version ===" jq -r '.version' package-lock.json echo -e "\n=== package-lock.json packages root version ===" jq -r '.packages[""].version' package-lock.json else echo "package-lock.json not found" fiweb-ui/modules/app.methods.session-browser.mjs (1)
267-273: LGTM!web-ui/modules/app.methods.navigation.mjs (1)
422-422: LGTM!web-ui/modules/sessions-filters-url.mjs (1)
60-63: LGTM!
| { | ||
| "name": "codexmate", | ||
| "version": "0.1.0", | ||
| "version": "0.0.35", |
Contributor
There was a problem hiding this comment.
Version conflicts with PR objectives.
The PR description states: "Revert package.json version from 0.0.36 to 0.0.34; npm versions 0.0.35 and 0.0.36 are unpublished." However, this change sets the version to 0.0.35, which contradicts the stated goal and may conflict with the unpublished version.
🔧 Proposed fix
- "version": "0.0.35",
+ "version": "0.0.34",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "version": "0.0.35", | |
| "version": "0.0.34", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 3, The package.json "version" field was incorrectly set
to 0.0.35 while the PR intention is to revert to 0.0.34; open package.json,
change the "version" property value from "0.0.35" to "0.0.34" (ensure the
"version" key is updated), and then check and update any related metadata
(lockfile or CI npm/publish config) to remain consistent with that version.
Comment on lines
+426
to
+434
| if (pathname === '/web-ui' || pathname === '/web-ui/' || pathname === '/web-ui/index.html') { | ||
| const url = new URL(window.location.href); | ||
| url.pathname = '/'; | ||
| // 移除查询参数和 hash，保持 URL 纯净 | ||
| url.search = ''; | ||
| url.hash = ''; | ||
| window.location.replace(url.toString()); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Redirect discards query parameters and hash, conflicting with PR objectives.
The PR description states: "auto-redirect /web-ui/* → / while preserving hash and query params." However, lines 430-431 explicitly clear search and hash before redirecting, which would lose information from URLs like /web-ui?tab=sessions#section.
If the design intent is to apply params before cleaning (as mentioned in the PR for "share links"), the current logic should:
- Read and apply params/hash to view model state (similar to
restoreSessionFilterCachewhich already does this at line 516) - Then redirect and clean the URL
🔍 Proposed approach
const pathname = window.location.pathname;
if (pathname === '/web-ui' || pathname === '/web-ui/' || pathname === '/web-ui/index.html') {
const url = new URL(window.location.href);
+ // Don't clear params yet - let restoreSessionFilterCache handle them first
+ // and then clear them after state is extracted
url.pathname = '/';
- // 移除查询参数和 hash，保持 URL 纯净
- url.search = '';
- url.hash = '';
window.location.replace(url.toString());
return;
}Then ensure lines 436-441 still clean up after restoreSessionFilterCache runs (line 516).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/app.js` around lines 426 - 434, The redirect code for pathname checks
currently clears url.search and url.hash before redirecting, which discards
query/hash; update the flow so you first parse and apply query params and hash
to the app state (reuse the existing restoreSessionFilterCache logic or call the
same routine used at/near restoreSessionFilterCache) and only then perform a
redirect that preserves or cleans the URL as intended; specifically, in the
block that handles pathname === '/web-ui'... use the code that extracts
window.location.search and window.location.hash, pass them into the same
state-restoration function (or invoke restoreSessionFilterCache) to apply
filters/share links, and afterward call window.location.replace with a URL that
either retains the original search/hash or cleans them according to the PR
objective.
- Fix sessionUsageDaily -> sessionUsageHourlyHeatmap assertion - Remove obsolete loadSkillsMarketOverview and market-action-grid checks - Update skills-target-chip selector (was market-target-chip) - Fix provider card disabled state and tabindex assertions - Remove market.pill importableDirect/importMissing checks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/config-tabs-ui.test.mjs (1)
250-250: ⚡ Quick winProvider-card keyboard coverage dropped Space-key behavior in this assertion.
Please keep Space-key handling asserted alongside Enter for this
role="button"path, so disabled/local logic doesn’t regress keyboard accessibility coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/config-tabs-ui.test.mjs` at line 250, The test's regex for the provider card button only asserts Enter key handling and omits Space key coverage; update the assertion to include Space-key handling the same way as Enter by matching an additional `@keydown.space.self.prevent` pattern that follows the same ternary check "(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)", ensuring the disabled/local logic and keyboard accessibility for role="button" remains covered; look for symbols switchProvider, isLocalProviderDisabled, displayCurrentProvider, and provider.name to locate the provider-card assertion to extend.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 614-617: The test failure is due to missing new state fields from
the data-key allowlists; update the relevant allowedExtraCurrentKeys arrays in
web-ui-behavior-parity.test.mjs to include the new keys:
isLocalProviderDisabled, sessionUsageWave, sessionsUsageSelectedDay,
usageHeroDelta, usageHeroDeltaClass, usageHeroMainValue, and usageHeroSubLabel
so the parity checks accept the expanded UI state; locate the
allowedExtraCurrentKeys definitions referenced for the affected components
(search for allowedExtraCurrentKeys and the entries around 'pasteAgentsContent',
'importSingleSkill', 'openSkillsMenu', 'isClaudeLocalBridgeDisabled') and append
these new keys to the appropriate allowlist arrays.
---
Nitpick comments:
In `@tests/unit/config-tabs-ui.test.mjs`:
- Line 250: The test's regex for the provider card button only asserts Enter key
handling and omits Space key coverage; update the assertion to include Space-key
handling the same way as Enter by matching an additional
`@keydown.space.self.prevent` pattern that follows the same ternary check
"(provider.name === 'local' && isLocalProviderDisabled) ? null :
switchProvider(provider.name)", ensuring the disabled/local logic and keyboard
accessibility for role="button" remains covered; look for symbols
switchProvider, isLocalProviderDisabled, displayCurrentProvider, and
provider.name to locate the provider-card assertion to extend.
🪄 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: e0d9821a-972d-4895-8a72-04254603b58a
📒 Files selected for processing (4)
package.jsontests/unit/config-tabs-ui.test.mjstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjs
✅ Files skipped from review due to trivial changes (1)
- package.json
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: ci / 0_ci (22).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 740-740: Assertion failed (deepStrictEqual) in bundled app skeleton data key drift vs parity baseline. Expected [] but received keys: ['isLocalProviderDisabled','sessionUsageWave','sessionsUsageSelectedDay','usageHeroDelta','usageHeroDeltaClass','usageHeroMainValue','usageHeroSubLabel'].
🪛 GitHub Actions: ci / 1_ci (18).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 740-740: Unit test failed (web UI behavior parity). AssertionError: Expected bundled app skeleton data keys to exactly match parity baseline. deepStrictEqual failed. Actual keys include: 'isLocalProviderDisabled', 'sessionUsageWave', 'sessionsUsageSelectedDay', 'usageHeroDelta', 'usageHeroDeltaClass', 'usageHeroMainValue', 'usageHeroSubLabel'; expected: []
🪛 GitHub Actions: ci / 2_ci (20).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 740-740: Unit test failed: AssertionError: Expected values to be strictly deep-equal (deepStrictEqual). Differences: actual keys ['isLocalProviderDisabled','sessionUsageWave','sessionsUsageSelectedDay','usageHeroDelta','usageHeroDeltaClass','usageHeroMainValue','usageHeroSubLabel'] vs expected [].
🪛 GitHub Actions: ci / ci (18)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 740-740: Unit test failed (TAP). captured bundled app skeleton only exposes expected data key drift versus parity baseline. AssertionError: Expected values to be strictly deep-equal; actual keys included ['isLocalProviderDisabled','sessionUsageWave','sessionsUsageSelectedDay','usageHeroDelta','usageHeroDeltaClass','usageHeroMainValue','usageHeroSubLabel'] but expected [] (deepStrictEqual).
🪛 GitHub Actions: ci / ci (20)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 740-740: Test failed: 'captured bundled app skeleton only exposes expected data key drift versus parity baseline' assertion error. Expected deepStrictEqual to [] but actual contained keys: isLocalProviderDisabled, sessionUsageWave, sessionsUsageSelectedDay, usageHeroDelta, usageHeroDeltaClass, usageHeroMainValue, usageHeroSubLabel.
🪛 GitHub Actions: ci / ci (22)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 740-740: Unit test failed: 'captured bundled app skeleton only exposes expected data key drift versus parity baseline' (AssertionError). Expected strictly deep-equal to [], but received keys: ['isLocalProviderDisabled','sessionUsageWave','sessionsUsageSelectedDay','usageHeroDelta','usageHeroDeltaClass','usageHeroMainValue','usageHeroSubLabel']
🔇 Additional comments (1)
tests/unit/web-run-host.test.mjs (1)
746-758: LGTM!Also applies to: 774-791
- isLocalProviderDisabled (data key) - sessionUsageWave, sessionsUsageSelectedDay, usageHeroDelta, usageHeroDeltaClass (computed keys)
- usageHeroMainValue - usageHeroSubLabel
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/web-ui-behavior-parity.test.mjs (1)
412-452:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Incomplete allowlist update causes CI failure—add missing
usageHeroMainValueandusageHeroSubLabel.The allowlist was partially updated but is missing two keys that the current bundle exposes. CI fails because
usageHeroMainValueandusageHeroSubLabelare not inallowedExtraCurrentKeys. The past review comment identified seven keys to add; this change added only five.🔧 Add the missing keys
allowedExtraCurrentKeys.push( 'lang', 'configTemplateContext', 'configTemplateDiffVisible', 'configTemplateDiffLoading', 'configTemplateDiffError', 'configTemplateDiffLines', 'configTemplateDiffStats', 'configTemplateDiffHasChangesValue', 'configTemplateDiffFingerprint', '_configTemplateDiffPreviewRequestToken', 'configTemplateDiffConfirmEnabled', 'healthCheckBatchTotal', 'healthCheckBatchDone', 'healthCheckBatchFailed', 'showHealthCheckModal', 'showCodexBridgePoolModal', 'showClaudeBridgePoolModal', 'pluginsActiveId', 'pluginsLoading', 'pluginsError', 'promptTemplatesListRaw', 'promptTemplatesLoadedOnce', 'promptTemplatesKeyword', 'promptTemplateSelectedId', 'promptTemplateDraftRaw', 'promptTemplateVarValuesRaw', 'promptTemplatesMode', 'promptComposerCommand', 'promptComposerPickerVisible', 'promptComposerPickerKeyword', 'promptComposerSelectedTemplateId', 'promptComposerVarValuesRaw', 'sessionConverting', 'sessionImportingNative', 'sessionSortMode', 'sessionsUsageCompareEnabled', 'sessionsUsageSelectedDayKey', 'currentModels', - 'isLocalProviderDisabled' + 'isLocalProviderDisabled', + 'usageHeroMainValue', + 'usageHeroSubLabel' );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/web-ui-behavior-parity.test.mjs` around lines 412 - 452, The test's allowlist update omitted two keys causing CI failures: add "usageHeroMainValue" and "usageHeroSubLabel" to the allowedExtraCurrentKeys push in the web-ui-behavior-parity test (the array pushed in tests/unit/web-ui-behavior-parity.test.mjs); locate the allowedExtraCurrentKeys.push(...) invocation and append those two string keys alongside the existing entries so the test reflects the current bundle output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 412-452: The test's allowlist update omitted two keys causing CI
failures: add "usageHeroMainValue" and "usageHeroSubLabel" to the
allowedExtraCurrentKeys push in the web-ui-behavior-parity test (the array
pushed in tests/unit/web-ui-behavior-parity.test.mjs); locate the
allowedExtraCurrentKeys.push(...) invocation and append those two string keys
alongside the existing entries so the test reflects the current bundle output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 047fd19c-6c51-4c2a-b172-8eb35ff09219
📒 Files selected for processing (1)
tests/unit/web-ui-behavior-parity.test.mjs
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: ci / 0_ci (20).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 745-745: Unit test failed: AssertionError (ERR_ASSERTION) in web-ui behavior parity. Expected values to be strictly deep-equal; actual returned unexpected key drift array ['isLocalProviderDisabled','usageHeroMainValue','usageHeroSubLabel'] vs expected [].
🪛 GitHub Actions: ci / 1_ci (18).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 745-745: Unit test failed (web-ui behavior parity). AssertionError: Expected values to be strictly deep-equal. Data key drift vs parity baseline was detected. Actual keys: ['isLocalProviderDisabled','usageHeroMainValue','usageHeroSubLabel']; expected: []
🪛 GitHub Actions: ci / 2_ci (22).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 745-745: Unit test failed (assertion). AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: actual ['isLocalProviderDisabled','usageHeroMainValue','usageHeroSubLabel'] vs expected [] (deepStrictEqual).
🪛 GitHub Actions: ci / ci (18)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 745-745: Unit test failed (web UI behavior parity). AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal. Expected: [] but actual drift keys: ['isLocalProviderDisabled','usageHeroMainValue','usageHeroSubLabel'].
🪛 GitHub Actions: ci / ci (20)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 745-745: AssertionError in 'captured bundled app skeleton only exposes expected data key drift versus parity baseline'. Expected no data key drift, but actual drift keys were: ['isLocalProviderDisabled', 'usageHeroMainValue', 'usageHeroSubLabel']. (deepStrictEqual)
🪛 GitHub Actions: ci / ci (22)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 745-745: Unit test failed: captured bundled app skeleton only exposes expected data key drift versus parity baseline. AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal. Actual drift keys: ['isLocalProviderDisabled','usageHeroMainValue','usageHeroSubLabel']; expected: [].
Move isLocalProviderDisabled from data keys to computed keys as it is defined as a computed property in app.computed.dashboard.mjs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/web-ui-behavior-parity.test.mjs (1)
451-451:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win'isLocalProviderDisabled' is a computed property and must be in the computed allowlist, not the data allowlist.
CI is failing because 'isLocalProviderDisabled' is categorized as a data key (line 451) but is actually defined as a computed property in
web-ui/modules/app.computed.dashboard.mjs. This causes the data key drift assertion at line 458 to fail withactual: ['isLocalProviderDisabled'], expected: [].🔧 Proposed fix
Remove 'isLocalProviderDisabled' from the data allowlist:
'sessionsUsageCompareEnabled', 'sessionsUsageSelectedDayKey', 'currentModels', - 'isLocalProviderDisabled' );Then add it to the computed allowlist around line 719:
'displayProviderUrl', 'isTransformProvider', + 'isLocalProviderDisabled', 'activeProviderModel',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/web-ui-behavior-parity.test.mjs` at line 451, Remove 'isLocalProviderDisabled' from the data allowlist in tests/unit/web-ui-behavior-parity.test.mjs and add it to the computed allowlist instead; specifically locate the array containing data allowlist entries (which currently includes 'isLocalProviderDisabled') and delete that entry, then locate the computed allowlist array (near where other computed keys are listed) and add 'isLocalProviderDisabled' so the test reflects its actual definition in web-ui/modules/app.computed.dashboard.mjs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Line 451: Remove 'isLocalProviderDisabled' from the data allowlist in
tests/unit/web-ui-behavior-parity.test.mjs and add it to the computed allowlist
instead; specifically locate the array containing data allowlist entries (which
currently includes 'isLocalProviderDisabled') and delete that entry, then locate
the computed allowlist array (near where other computed keys are listed) and add
'isLocalProviderDisabled' so the test reflects its actual definition in
web-ui/modules/app.computed.dashboard.mjs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6555d93e-6d53-49cf-8bc3-0c4688166115
📒 Files selected for processing (1)
tests/unit/web-ui-behavior-parity.test.mjs
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: ci / 0_ci (20).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 747-747: Test failed: Web UI behavior parity assertion mismatch. Expected no data key drift versus parity baseline, but actual contained ['isLocalProviderDisabled']. (deepStrictEqual)
🪛 GitHub Actions: ci / 1_ci (22).txt
tests/unit/web-ui-behavior-parity.test.mjs
[error] 747-747: Unit test failed: bundled app skeleton exposed unexpected data key drift vs parity baseline. AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal. Actual: ['isLocalProviderDisabled'], Expected: [].
🪛 GitHub Actions: ci / ci (20)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 747-747: Assertion failed in web UI behavior parity test: captured bundled app skeleton exposed unexpected data key. AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal (actual ['isLocalProviderDisabled'] vs expected []).
🪛 GitHub Actions: ci / ci (22)
tests/unit/web-ui-behavior-parity.test.mjs
[error] 747-747: Unit test failed (AssertionError). expected bundled app skeleton to match parity baseline with no unexpected data key drift, but actual contains unexpected key: 'isLocalProviderDisabled'. Assertion operator: deepStrictEqual.
🔇 Additional comments (1)
tests/unit/web-ui-behavior-parity.test.mjs (1)
709-710: LGTM!
Collaborator
|
本轮更新已处理会话浏览 tab 相关的静态 URL 回退问题：
验证：
剩余风险：目前没有发现新的阻塞；这轮主要覆盖会话 tab 切入/切出时的 legacy URL 污染路径。 @coderabbitai review |
Contributor
|
我现在开始对本次 PR 进行全面审查。 ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/config-tabs-ui.test.mjs`:
- Line 66: The test's regex uses [\s\S]* which can span across elements; update
the assertion in tests/unit/config-tabs-ui.test.mjs so the pattern only matches
a single start-tag by replacing the [\s\S]* portion with a pattern that confines
matches to the tag's attributes (e.g. use [^>]* between attribute checks).
Target the assertion line that references v-if="taskOrchestrationTabEnabled",
class="top-tab" and id="tab-orchestration" so all three attributes must appear
within the same element's opening tag.
In `@web-ui/modules/app.methods.navigation.mjs`:
- Around line 67-73: The collapse loop that normalizes duplicate "/web-ui"
segments fails to catch "/web-ui/web-ui" without a trailing slash; update the
replace regex in the do/while (the pathname = pathname.replace(...) call) to
match the second "/web-ui" followed by either one or more slashes OR
end-of-string (e.g. use a pattern that permits (?:\/+|$) after the second
web-ui) so the loop collapses both "/web-ui/web-ui" and "/web-ui/web-ui/"
variants into a single "/web-ui/" and then the existing subsequent check can
canonicalize to "/".
🪄 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: ef26a964-3da0-423f-b113-1453805d1e29
📒 Files selected for processing (5)
tests/e2e/test-web-ui-session-tab.jstests/unit/config-tabs-ui.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.methods.navigation.mjsweb-ui/partials/index/layout-header.html
✅ Files skipped from review due to trivial changes (1)
- web-ui/partials/index/layout-header.html
📜 Review details
🔇 Additional comments (3)
web-ui/modules/app.methods.navigation.mjs (1)
167-167: LGTM!Also applies to: 442-442
tests/e2e/test-web-ui-session-tab.js (1)
22-40: LGTM!Also applies to: 111-113, 141-144, 176-176
tests/unit/web-ui-behavior-parity.test.mjs (1)
450-450: LGTM!Also applies to: 725-726
Comment on lines
+67
to
+73
| do { | ||
| previousPathname = pathname; | ||
| pathname = pathname.replace(/\/+web-ui\/+web-ui\/+/, '/web-ui/'); | ||
| } while (pathname !== previousPathname); | ||
| if (pathname === '/web-ui' || pathname === '/web-ui/' || pathname === '/web-ui/index.html') { | ||
| pathname = '/'; | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize /web-ui/web-ui without trailing slash too.
The current collapse pattern only matches when there is a trailing slash after the second web-ui, so /web-ui/web-ui can bypass canonicalization.
Suggested patch
- pathname = pathname.replace(/\/+web-ui\/+web-ui\/+/, '/web-ui/');
+ pathname = pathname.replace(/\/+web-ui\/+web-ui(?=\/|$)/, '/web-ui');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.methods.navigation.mjs` around lines 67 - 73, The collapse
loop that normalizes duplicate "/web-ui" segments fails to catch
"/web-ui/web-ui" without a trailing slash; update the replace regex in the
do/while (the pathname = pathname.replace(...) call) to match the second
"/web-ui" followed by either one or more slashes OR end-of-string (e.g. use a
pattern that permits (?:\/+|$) after the second web-ui) so the loop collapses
both "/web-ui/web-ui" and "/web-ui/web-ui/" variants into a single "/web-ui/"
and then the existing subsequent check can canonicalize to "/".
Collaborator
|
Fixed the Web UI URL/layout regression on this PR. Changes in latest commit
Validation performed locally:
@coderabbitai re-review |
Contributor
|
我现在开始对最新提交进行全面审查。 ✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli.js (2)
10647-10664:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't require
CODEXMATE_HTTP_TOKENfor remote webhooks.
/hooks/*now hits the shared non-loopback auth gate beforehandleAutomationHook(), so valid GitHub/GitLab deliveries will start failing with401/403unlessCODEXMATE_HTTP_TOKENis also configured. That breaks remote automation even when the webhook secret is correct.🔧 Suggested fix
if ( requestPath === '/api' || requestPath.startsWith('/api/import-') || requestPath.startsWith('/hooks/') || requestPath.startsWith('/download/') ) { const remoteAddr = req && req.socket ? req.socket.remoteAddress : ''; const isLoopback = !remoteAddr || isLoopbackRemoteAddress(remoteAddr); if (!isLoopback) { const rateLimitKey = (remoteAddr || 'unknown') + ':' + requestPath; if (!checkRateLimit(rateLimitKey)) { writeJsonResponse(res, 429, { error: 'Rate limit exceeded' }, { 'Retry-After': '60' }); return; } - const auth = assertRequestAuthorized(req, res); - if (!auth.ok) { - return; + if (!requestPath.startsWith('/hooks/')) { + const auth = assertRequestAuthorized(req, res); + if (!auth.ok) { + return; + } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 10647 - 10664, The current auth gate requires CODEXMATE_HTTP_TOKEN for non-loopback requests and incorrectly forces /hooks/* through assertRequestAuthorized; change the logic so requests with requestPath starting with '/hooks/' are not forced through assertRequestAuthorized here (allow them to proceed to handleAutomationHook which validates webhook secrets). Concretely, modify the conditional around assertRequestAuthorized in the block that checks isLoopback so that if requestPath.startsWith('/hooks/') it skips calling assertRequestAuthorized (or early-returns only on rate limit) and does not require CODEXMATE_HTTP_TOKEN; keep rate limiting and loopback checks intact and leave handleAutomationHook to perform its own signature validation.
11414-11418:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy
/web-uientrypoints still 404 instead of canonicalizing.
/web-ui/reaches the asset branch and returns404whenrelativePath === '', and/web-uifalls through to the final non-root404. If the goal is to collapse legacy Web UI entry URLs back to/, both variants need the same redirect as/web-ui/index.html.🔧 Suggested fix
- } else if (requestPath.startsWith('/web-ui/')) { + } else if (requestPath === '/web-ui' || requestPath === '/web-ui/' || requestPath.startsWith('/web-ui/')) { + if (requestPath === '/web-ui' || requestPath === '/web-ui/') { + const rawUrl = typeof req.url === 'string' ? req.url : ''; + const queryIndex = rawUrl.indexOf('?'); + const query = queryIndex >= 0 ? rawUrl.slice(queryIndex) : ''; + res.writeHead(302, { + 'Location': `/${query}`, + 'Content-Type': 'text/plain; charset=utf-8', + 'Cache-Control': 'no-store, max-age=0' + }); + res.end('Found'); + return; + } // Skip the /web-ui/ directory itself, which is handled above const normalized = path.normalize(requestPath).replace(/^([\\.\\/])+/, ''); const filePath = path.join(__dirname, normalized); @@ - if (relativePath === '' || relativePath === 'index.html') { - res.writeHead(404, { 'Content-Type': 'text/plain; charset=utf-8' }); - res.end('Not Found'); + if (relativePath === '' || relativePath === 'index.html') { + const rawUrl = typeof req.url === 'string' ? req.url : ''; + const queryIndex = rawUrl.indexOf('?'); + const query = queryIndex >= 0 ? rawUrl.slice(queryIndex) : ''; + res.writeHead(302, { + 'Location': `/${query}`, + 'Content-Type': 'text/plain; charset=utf-8', + 'Cache-Control': 'no-store, max-age=0' + }); + res.end('Found'); return; }Also applies to: 11525-11539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 11414 - 11418, The handler currently returns 404 when relativePath === '' or 'index.html' causing both /web-ui/ and /web-ui/index.html (and the bare /web-ui fall-through) to 404; change the logic so that when relativePath === '' or relativePath === 'index.html' you send an HTTP redirect (301/302) to '/' instead of writing 404 (update the branch that checks relativePath and replace the res.writeHead/res.end 404 with a redirect response), and ensure the same canonicalization is applied to the code path that previously let bare '/web-ui' fall through to the final non-root 404 so all legacy /web-ui variants are redirected to '/'.
🧹 Nitpick comments (1)
tests/e2e/test-web-ui-assets.js (1)
47-50: ⚡ Quick winFix stale inline comment to match the redirect contract.
Line 47 says
/web-ui/index.htmlreturns 404, but Lines 49-50 assert a 302 redirect. Please update the comment to avoid confusion during failures.✏️ Suggested patch
- // /web-ui/index.html now returns 404 (only root path serves HTML) + // /web-ui/index.html now redirects to / (only root path serves HTML)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test-web-ui-assets.js` around lines 47 - 50, Update the stale inline comment above the getText call: the comment that currently says "/web-ui/index.html now returns 404 (only root path serves HTML)" is incorrect given the assertions on bundledIndex.statusCode === 302 and bundledIndex.headers.location === '/'; change the comment to state that /web-ui/index.html redirects (302) to the canonical root URL so it matches the behavior being asserted around the getText call and the bundledIndex checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/styles/responsive.css`:
- Around line 89-95: The .container rule uses fixed 100vh which can be clipped
on mobile; update both height and min-height to prefer dynamic viewport units by
adding a 100dvh fallback order: keep the existing 100vh then add 100dvh after it
(e.g. height: 100vh; height: 100dvh; and min-height: 100vh; min-height: 100dvh;)
so browsers that understand 100dvh use it while older browsers fall back to
100vh.
---
Outside diff comments:
In `@cli.js`:
- Around line 10647-10664: The current auth gate requires CODEXMATE_HTTP_TOKEN
for non-loopback requests and incorrectly forces /hooks/* through
assertRequestAuthorized; change the logic so requests with requestPath starting
with '/hooks/' are not forced through assertRequestAuthorized here (allow them
to proceed to handleAutomationHook which validates webhook secrets). Concretely,
modify the conditional around assertRequestAuthorized in the block that checks
isLoopback so that if requestPath.startsWith('/hooks/') it skips calling
assertRequestAuthorized (or early-returns only on rate limit) and does not
require CODEXMATE_HTTP_TOKEN; keep rate limiting and loopback checks intact and
leave handleAutomationHook to perform its own signature validation.
- Around line 11414-11418: The handler currently returns 404 when relativePath
=== '' or 'index.html' causing both /web-ui/ and /web-ui/index.html (and the
bare /web-ui fall-through) to 404; change the logic so that when relativePath
=== '' or relativePath === 'index.html' you send an HTTP redirect (301/302) to
'/' instead of writing 404 (update the branch that checks relativePath and
replace the res.writeHead/res.end 404 with a redirect response), and ensure the
same canonicalization is applied to the code path that previously let bare
'/web-ui' fall through to the final non-root 404 so all legacy /web-ui variants
are redirected to '/'.
---
Nitpick comments:
In `@tests/e2e/test-web-ui-assets.js`:
- Around line 47-50: Update the stale inline comment above the getText call: the
comment that currently says "/web-ui/index.html now returns 404 (only root path
serves HTML)" is incorrect given the assertions on bundledIndex.statusCode ===
302 and bundledIndex.headers.location === '/'; change the comment to state that
/web-ui/index.html redirects (302) to the canonical root URL so it matches the
behavior being asserted around the getText call and the bundledIndex checks.
🪄 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: 1e0a89c3-f79a-40ef-99ff-11e3660eb192
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
cli.jspackage.jsontests/e2e/test-web-ui-assets.jstests/unit/compact-layout-ui.test.mjstests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-source-bundle.test.mjstests/unit/web-ui-url-canonicalization.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/app.methods.session-trash.mjsweb-ui/modules/sessions-filters-url.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/panel-settings.htmlweb-ui/res/vue.runtime.global.prod.jsweb-ui/source-bundle.cjsweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.css
📜 Review details
🔇 Additional comments (24)
web-ui/index.html (1)
9-9: LGTM!tests/unit/run.mjs (1)
16-16: LGTM!tests/unit/compact-layout-ui.test.mjs (1)
42-48: LGTM!Also applies to: 78-78
web-ui/partials/index/layout-header.html (2)
3-3: LGTM!Also applies to: 13-13, 23-23, 34-34, 44-44, 54-54, 64-64, 74-74, 84-84
482-487: LGTM!tests/unit/web-ui-source-bundle.test.mjs (1)
26-27: LGTM!web-ui/styles/navigation-panels.css (1)
41-53: LGTM!tests/unit/web-ui-url-canonicalization.test.mjs (1)
1-82: LGTM!web-ui/modules/app.methods.session-trash.mjs (1)
221-249: LGTM!package.json (1)
49-49: LGTM!web-ui/app.js (1)
10-13: LGTM!Also applies to: 31-31, 674-680
tests/unit/web-ui-behavior-parity.test.mjs (1)
518-518: LGTM!web-ui/source-bundle.cjs (1)
10-13: LGTM!Also applies to: 65-111, 267-267, 279-280
web-ui/partials/index/panel-settings.html (1)
22-23: LGTM!Also applies to: 32-33
tests/unit/web-run-host.test.mjs (1)
720-720: LGTM!Also applies to: 732-732, 774-790
cli.js (1)
10061-10065: LGTM!Also applies to: 10624-10624, 11393-11403, 11425-11428, 11519-11522
tests/unit/config-tabs-ui.test.mjs (2)
68-68: The overly permissive[\s\S]*regex pattern that can span across elements was already flagged in a previous review with a suggested fix.
37-38: LGTM!Also applies to: 61-62
web-ui/modules/sessions-filters-url.mjs (6)
66-75: LGTM!
77-87: LGTM!
89-115: LGTM!
117-132: LGTM!
134-137: LGTM!
58-64: Clarify legacy path coverage:/web-ui/web-ui/is handled outsidecanonicalizeWebUiUrl
canonicalizeWebUiUrlinweb-ui/modules/sessions-filters-url.mjsonly rewritespathname === '/web-ui/index.html'to/(used for sessions-filter URL/history canonicalization).- The duplicated legacy segments like
/web-ui/web-ui/are normalized elsewhere:canonicalizeWebUiRuntimeUrlinweb-ui/modules/app.methods.navigation.mjscollapses repeated/web-ui/.../web-ui/...and redirects/web-ui//web-ui///web-ui/index.htmlto/(covered bytests/unit/test-web-ui-url-normalization.mjscases).
Comment on lines
+89
to
+95
| .container { | ||
| display: flex; | ||
| flex-direction: column; | ||
| height: 100vh; | ||
| min-height: 100vh; | ||
| overflow: hidden; | ||
| } |
Contributor
There was a problem hiding this comment.
Use dynamic viewport units to avoid mobile viewport clipping.
Line 92 and Line 93 rely on 100vh, which can truncate usable height on mobile browsers with dynamic address bars. Add 100dvh fallback to keep the panel reliably scrollable.
📱 Suggested patch
.container {
display: flex;
flex-direction: column;
height: 100vh;
min-height: 100vh;
+ height: 100dvh;
+ min-height: 100dvh;
overflow: hidden;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .container { | |
| display: flex; | |
| flex-direction: column; | |
| height: 100vh; | |
| min-height: 100vh; | |
| overflow: hidden; | |
| } | |
| .container { | |
| display: flex; | |
| flex-direction: column; | |
| height: 100vh; | |
| min-height: 100vh; | |
| height: 100dvh; | |
| min-height: 100dvh; | |
| overflow: hidden; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/styles/responsive.css` around lines 89 - 95, The .container rule uses
fixed 100vh which can be clipped on mobile; update both height and min-height to
prefer dynamic viewport units by adding a 100dvh fallback order: keep the
existing 100vh then add 100dvh after it (e.g. height: 100vh; height: 100dvh; and
min-height: 100vh; min-height: 100dvh;) so browsers that understand 100dvh use
it while older browsers fall back to 100vh.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
11393-11418:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedirect all legacy Web UI entry paths, not just
/web-ui/index.html.Line 11415 still returns
404for/web-ui/, and Lines 11525-11539 do the same for/web-ui. Normalized duplicates like/web-ui/web-ui/index.htmlalso fall through to404, so legacy bookmarks/share URLs still won't canonicalize back to/.Proposed fix
- } else if (requestPath === '/web-ui/index.html') { + } else if (requestPath === '/web-ui' || requestPath === '/web-ui/' || requestPath === '/web-ui/index.html') { const rawUrl = typeof req.url === 'string' ? req.url : ''; const queryIndex = rawUrl.indexOf('?'); const query = queryIndex >= 0 ? rawUrl.slice(queryIndex) : ''; res.writeHead(302, { 'Location': `/${query}`, @@ - // Empty relativePath means direct /web-ui/ access - return 404 - if (relativePath === '' || relativePath === 'index.html') { - res.writeHead(404, { 'Content-Type': 'text/plain; charset=utf-8' }); - res.end('Not Found'); + if ( + relativePath === '' + || relativePath === 'index.html' + || relativePath === 'web-ui' + || relativePath === 'web-ui/index.html' + ) { + const rawUrl = typeof req.url === 'string' ? req.url : ''; + const queryIndex = rawUrl.indexOf('?'); + const query = queryIndex >= 0 ? rawUrl.slice(queryIndex) : ''; + res.writeHead(302, { + 'Location': `/${query}`, + 'Content-Type': 'text/plain; charset=utf-8', + 'Cache-Control': 'no-store, max-age=0' + }); + res.end('Found'); return; }Also applies to: 11525-11539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 11393 - 11418, The handler that checks requestPath and computes normalized/filePath/relativePath currently returns 404 for legacy entry URLs like '/web-ui', '/web-ui/', '/web-ui/index.html', and nested variants (e.g., '/web-ui/web-ui/index.html'); update the conditional that now tests if relativePath === '' || relativePath === 'index.html' to instead treat those cases as redirects back to '/' preserving the original query: use requestPath/rawUrl/query to send a 302 Location of `/${query}` for requestPath '/web-ui' or '/web-ui/' and when relativePath is '' or 'index.html' or startsWith('web-ui/index.html') (or otherwise indicates a legacy entry), rather than returning 404; keep the security check using isPathInside(filePath, webDir) and other logic intact.
♻️ Duplicate comments (12)
web-ui/modules/skills.methods.mjs (1)
492-495:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
importSingleSkillpayload with the existingimport-skillscontract.This path sends
imports, while the existing import flow usesitems. That mismatch can cause single-skill import to no-op depending on backend parsing.Suggested fix
- const res = await api('import-skills', { - targetApp: this.skillsTargetApp, - imports: [skill] - }); + const res = await api('import-skills', { + targetApp: this.skillsTargetApp, + items: [{ + name: skill && skill.name, + sourceApp: skill && skill.sourceApp + }] + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/skills.methods.mjs` around lines 492 - 495, The importSingleSkill function currently calls api('import-skills', { targetApp: this.skillsTargetApp, imports: [skill] }) using the wrong payload key; change the payload to match the existing import-skills contract by sending items: [skill] (and preserve targetApp) so the backend receives the same shape as the bulk import flow. Locate importSingleSkill and the api('import-skills', ...) invocation and replace the imports property with items to align with the existing contract.web-ui/modules/app.computed.session.mjs (2)
717-726:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same filtered dataset for current and previous hero delta.
Current value is from
sessionUsageCharts.summary, but previous-period aggregation here reads rawsessionsUsageList. This can produce incorrect delta direction/percentage.Also applies to: 754-763
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/app.computed.session.mjs` around lines 717 - 726, The previous-period aggregation is iterating raw this.sessionsUsageList while the current value uses the filtered dataset sessionUsageCharts.summary, causing incorrect deltas; change the loop(s) that compute prevTokens (and the similar block at 754-763) to iterate over the same filtered array (sessionUsageCharts.summary or the shared filtered variable used for current calculations) and keep the same validation (typeof session check, Date.parse on session.updatedAt, Number(session.totalTokens) handling) so both current and previous hero values are computed from the identical data source.
608-610:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate prev/delta fields when range is
all.At Line 608–610,
prevKeyis still produced forall, so compare text can render when comparison should be disabled.Also applies to: 675-676
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/app.computed.session.mjs` around lines 608 - 610, The computed is still producing prev/delta values when the selected range is "all"; update the prevKey calculation and the other delta/prev field generation to return empty/disabled values whenever range === 'all' (or equivalent sentinel) instead of computing with rangeDays/dayMs. Concretely, wrap the existing prevKey logic (references: prevKey, rangeDays, dayMs) with a guard like if (range === 'all') return '' (or set delta-related outputs to null/''), and apply the same gate to the other occurrence noted (the block producing the delta at the second location) so compare text cannot render for the "all" range.web-ui/app.js (1)
425-444:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefer URL query/hash cleanup until after filter-state hydration.
At Line 437–443,
search/hashare removed beforerestoreSessionFilterCache()runs on Line 518. That can drop share-link/session params before they are applied.Also applies to: 518-518
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/app.js` around lines 425 - 444, The URL search/hash cleanup runs too early and can strip share-link/session params before restoreSessionFilterCache() consumes them; leave the initial /web-ui -> / redirect logic (location.replace) intact but remove or defer the second cleanup block that calls window.history.replaceState('', ...) and clears url.search/url.hash, and instead perform that cleanup only after restoreSessionFilterCache() has completed (i.e., move the cleanup to execute after the restoreSessionFilterCache() invocation). Ensure you reference the pathname-check/redirect block and the cleanup currently using window.history.replaceState and restoreSessionFilterCache() so reviewers can locate and adjust the code.web-ui/modules/app.methods.navigation.mjs (1)
69-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
/web-ui/web-uiwithout trailing slash in canonicalization.Line 69 only matches the duplicated segment when a trailing slash exists.
/web-ui/web-uistill slips through.Suggested patch
- pathname = pathname.replace(/\/+web-ui\/+web-ui\/+/, '/web-ui/'); + pathname = pathname.replace(/\/+web-ui\/+web-ui(?=\/|$)/, '/web-ui');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/app.methods.navigation.mjs` around lines 69 - 70, The current canonicalization replace call on the pathname variable (inside the do/while loop) only removes duplicated "/web-ui/web-ui/" when a trailing slash exists; update the regex used in the pathname.replace(...) so it also matches the case without a trailing slash (e.g., "/web-ui/web-ui") by making the trailing slash optional or by matching one or more consecutive "/web-ui" segments, and ensure the replacement normalizes to a single "/web-ui/" (maintaining the existing loop and comparison to previousPathname).web-ui/styles/sessions-usage.css (1)
303-305:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix hourly heatmap selector mismatch.
The selector at Line 303 doesn’t match the template class, so
overflow-x: autowon’t apply.Suggested patch
-.usage-card-hourly-heatmap { +.usage-hourly-heatmap { overflow-x: auto; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/styles/sessions-usage.css` around lines 303 - 305, The CSS rule targets .usage-card-hourly-heatmap but the template uses a different class name, so overflow-x: auto never applies; locate the actual class used in the template (e.g., the element rendering the hourly heatmap) and either rename the template class to usage-card-hourly-heatmap or update the selector in web-ui/styles/sessions-usage.css (replace .usage-card-hourly-heatmap) to exactly match the template's class (or add the template class variant alongside .usage-card-hourly-heatmap) so the overflow-x: auto rule is applied to the correct element.web-ui/partials/index/panel-usage.html (2)
58-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove new literal labels to i18n keys.
Hardcoded text (
tokens,API,total,msgs) bypasses localization and causes mixed-language UI.Also applies to: 106-109, 147-147, 162-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/partials/index/panel-usage.html` around lines 58 - 60, The template currently uses hardcoded labels ("tokens", "API", "total", "msgs") in spans like usageCurrentSessionStats.tokenLabel, usageCurrentSessionStats.apiDurationLabel and usageCurrentSessionStats.totalDurationLabel; replace those literals with i18n keys by calling the project's translation helper (e.g., $t or i18n filter) in the template and update the locale JSON/YAML to add keys (e.g., usage.tokens, usage.api, usage.total, usage.msgs); ensure you change the occurrences in this block and the other reported locations (lines around 106-109, 147, 162) so all UI text is localized and no hardcoded strings remain.
95-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse semantic buttons for click targets in Usage panel.
Lines 95–100 and 143–164 use clickable
span/divelements, which are not keyboard-operable by default. Convert these to<button type="button">(or add full keyboard semantics).Also applies to: 143-149, 158-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/partials/index/panel-usage.html` around lines 95 - 100, Replace the non-interactive span/div click targets in the Usage panel with semantic <button type="button"> elements (or add full keyboard semantics) so they are keyboard-operable; specifically change the v-for span that calls selectSessionsUsageDay(label.key) and uses sessionsUsageSelectedDay to a button preserving :key, class="usage-wave-label", :class binding, and the `@click` handler, and do the same for the other click targets referenced (the blocks around lines 143–149 and 158–164) ensuring any visual classes are kept, tabindex/role removed, and aria-pressed or aria-current is added where appropriate to reflect selection for accessibility.tests/unit/config-tabs-ui.test.mjs (1)
68-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten orchestration tab regex to a single start-tag.
Line 68 uses a cross-element pattern and can false-pass. Constrain the match to one
<button ...>opening tag with attribute lookaheads.Suggested patch
- assert.match(html, /v-if="taskOrchestrationTabEnabled"[^>]*class="top-tab"[\s\S]*id="tab-orchestration"/); + assert.match( + html, + /<button(?=[^>]*v-if="taskOrchestrationTabEnabled")(?=[^>]*class="top-tab")(?=[^>]*id="tab-orchestration")[^>]*>/ + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/config-tabs-ui.test.mjs` at line 68, The test's regex allows matching across multiple elements and can false-pass; update the assertion that inspects the html variable (the assert.match call) to require a single opening <button> start-tag by using a regex with a <button\b start, negative/positive lookaheads that check for v-if="taskOrchestrationTabEnabled", class="top-tab", and id="tab-orchestration" inside the same tag, and then match until the first closing '>' so only one start-tag is matched.web-ui/styles/responsive.css (1)
92-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer dynamic viewport units for mobile shell height.
Line 92–93 should include
100dvhafter100vhto avoid viewport clipping on mobile browsers with dynamic bars.Suggested patch
.container { display: flex; flex-direction: column; height: 100vh; + height: 100dvh; min-height: 100vh; + min-height: 100dvh; overflow: hidden; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/styles/responsive.css` around lines 92 - 93, Update the CSS declarations that set the shell height (the height and min-height properties) to include dynamic viewport units for mobile: keep the existing 100vh values but add 100dvh as a fallback/alternative (i.e., specify both units so browsers that support dynamic viewport height use 100dvh to avoid clipping by mobile UI bars while older browsers fall back to 100vh).tests/e2e/test-web-ui-url-routing.js (1)
205-205:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse a mutable loop counter in the
forloop.Line 205 declares
iasconstand then increments it, which throws at runtime and breaks this E2E test.Proposed fix
- for (const i = 0; i < htmlResponses.length; i++) { + for (let i = 0; i < htmlResponses.length; i++) {#!/bin/bash # Verify there is no incrementing for-loop with a const counter in this file. # Expected result: no output. rg -nP 'for\s*\(\s*const\s+\w+\s*=\s*[^;]+;\s*[^;]+;\s*\w+\+\+\s*\)' tests/e2e/test-web-ui-url-routing.js🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test-web-ui-url-routing.js` at line 205, The for-loop declares the loop counter as const which prevents incrementing; change the loop counter declaration from const to let in the for statement (the loop starting "for (const i = 0; i < htmlResponses.length; i++) {") so i is mutable, and scan the file for any other incrementing for-loops that mistakenly use const.web-ui/partials/index/panel-market.html (1)
97-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImport icon button still has no accessible name.
Line 97 remains an icon-only control without an accessible label.
Proposed fix
-<button type="button" class="skills-flow-add" `@click`="importSingleSkill(skill)" :disabled="loading || !!initError || skillsMarketBusy"> +<button + type="button" + class="skills-flow-add" + `@click`="importSingleSkill(skill)" + :disabled="loading || !!initError || skillsMarketBusy" + :aria-label="`${t('market.action.importSingle')}: ${skill.displayName || skill.name}`" + :title="`${t('market.action.importSingle')}: ${skill.displayName || skill.name}`">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/partials/index/panel-market.html` around lines 97 - 99, The icon-only import button lacks an accessible name; update the button with a clear accessible label (e.g., add aria-label="Import skill" or include visually-hidden text) so screen readers can describe its action; modify the element with class "skills-flow-add" and handler importSingleSkill(skill) to include the aria attribute or hidden label while keeping the existing :disabled binding (loading, initError, skillsMarketBusy) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/test-web-ui-url-routing.js`:
- Around line 92-93: The test's content-type assertion is too strict (only
matches application/javascript); update the assertion that checks
appJs.headers['content-type'] so it accepts both application/javascript and
text/javascript (and still handles optional charset), e.g. change the regex used
with String(appJs.headers['content-type'] || '') to allow either "application"
or "text" before "/javascript" so the test won't falsely fail when responses use
text/javascript.
In `@tests/unit/test-web-ui-url-normalization.mjs`:
- Around line 222-223: Currently runTests() is invoked unconditionally on
import; change this to only run when the module is executed directly by adding
an ESM guard using import.meta (e.g. compare process.argv[1] to
fileURLToPath(import.meta.url) from 'url'), call await runTests() inside that
guard, and set process.exitCode = 1 when runTests() returns a falsy/failed
result so CLI failures propagate a non-zero exit code; reference the runTests()
invocation and update surrounding top-level invocation to be conditional.
In `@web-ui/partials/index/panel-config-claude.html`:
- Line 103: The disabled local bridge card lacks ARIA disabled semantics for
screen readers; update the card element (the div with role="button" that uses
isClaudeLocalBridgeDisabled(), applyClaudeLocalBridge(), and
currentClaudeConfig) to include an :aria-disabled binding that reflects
isClaudeLocalBridgeDisabled() (e.g., aria-disabled="true" when disabled and
"false" otherwise) while keeping the existing tabindex and click/keydown guards
so assistive tech announces the control as disabled.
In `@web-ui/partials/index/panel-config-codex.html`:
- Line 123: The card acting as a button should expose its disabled state to
assistive tech: update the element rendered for displayProvidersList (the card
that uses displayCurrentProvider, isLocalProviderDisabled and switchProvider) to
add aria-disabled="true" when (provider.name === 'local' &&
isLocalProviderDisabled) and aria-disabled="false" or omit when enabled; keep
the current tabindex logic (set to -1 when disabled) and retain the
role="button" and existing click/keydown guards so the UI remains
non-interactive when disabled.
In `@web-ui/partials/index/panel-market.html`:
- Around line 44-47: The icon-only action button that calls refreshSkillsList({
silent: false }) (and the other icon-only button around lines 71-74) lacks an
accessible name; add an explicit accessible label by adding aria-label and title
attributes (e.g. aria-label="Refresh skills list", title="Refresh skills list")
or a localized equivalent (use your i18n helper like $t(...) if available), or
include a visually-hidden <span> with the action text inside the button so
screen readers can announce it; ensure the attributes/text are present on the
<button> elements with `@click`="refreshSkillsList({ silent: false })" and the
other icon-only button(s).
---
Outside diff comments:
In `@cli.js`:
- Around line 11393-11418: The handler that checks requestPath and computes
normalized/filePath/relativePath currently returns 404 for legacy entry URLs
like '/web-ui', '/web-ui/', '/web-ui/index.html', and nested variants (e.g.,
'/web-ui/web-ui/index.html'); update the conditional that now tests if
relativePath === '' || relativePath === 'index.html' to instead treat those
cases as redirects back to '/' preserving the original query: use
requestPath/rawUrl/query to send a 302 Location of `/${query}` for requestPath
'/web-ui' or '/web-ui/' and when relativePath is '' or 'index.html' or
startsWith('web-ui/index.html') (or otherwise indicates a legacy entry), rather
than returning 404; keep the security check using isPathInside(filePath, webDir)
and other logic intact.
---
Duplicate comments:
In `@tests/e2e/test-web-ui-url-routing.js`:
- Line 205: The for-loop declares the loop counter as const which prevents
incrementing; change the loop counter declaration from const to let in the for
statement (the loop starting "for (const i = 0; i < htmlResponses.length; i++)
{") so i is mutable, and scan the file for any other incrementing for-loops that
mistakenly use const.
In `@tests/unit/config-tabs-ui.test.mjs`:
- Line 68: The test's regex allows matching across multiple elements and can
false-pass; update the assertion that inspects the html variable (the
assert.match call) to require a single opening <button> start-tag by using a
regex with a <button\b start, negative/positive lookaheads that check for
v-if="taskOrchestrationTabEnabled", class="top-tab", and id="tab-orchestration"
inside the same tag, and then match until the first closing '>' so only one
start-tag is matched.
In `@web-ui/app.js`:
- Around line 425-444: The URL search/hash cleanup runs too early and can strip
share-link/session params before restoreSessionFilterCache() consumes them;
leave the initial /web-ui -> / redirect logic (location.replace) intact but
remove or defer the second cleanup block that calls
window.history.replaceState('', ...) and clears url.search/url.hash, and instead
perform that cleanup only after restoreSessionFilterCache() has completed (i.e.,
move the cleanup to execute after the restoreSessionFilterCache() invocation).
Ensure you reference the pathname-check/redirect block and the cleanup currently
using window.history.replaceState and restoreSessionFilterCache() so reviewers
can locate and adjust the code.
In `@web-ui/modules/app.computed.session.mjs`:
- Around line 717-726: The previous-period aggregation is iterating raw
this.sessionsUsageList while the current value uses the filtered dataset
sessionUsageCharts.summary, causing incorrect deltas; change the loop(s) that
compute prevTokens (and the similar block at 754-763) to iterate over the same
filtered array (sessionUsageCharts.summary or the shared filtered variable used
for current calculations) and keep the same validation (typeof session check,
Date.parse on session.updatedAt, Number(session.totalTokens) handling) so both
current and previous hero values are computed from the identical data source.
- Around line 608-610: The computed is still producing prev/delta values when
the selected range is "all"; update the prevKey calculation and the other
delta/prev field generation to return empty/disabled values whenever range ===
'all' (or equivalent sentinel) instead of computing with rangeDays/dayMs.
Concretely, wrap the existing prevKey logic (references: prevKey, rangeDays,
dayMs) with a guard like if (range === 'all') return '' (or set delta-related
outputs to null/''), and apply the same gate to the other occurrence noted (the
block producing the delta at the second location) so compare text cannot render
for the "all" range.
In `@web-ui/modules/app.methods.navigation.mjs`:
- Around line 69-70: The current canonicalization replace call on the pathname
variable (inside the do/while loop) only removes duplicated "/web-ui/web-ui/"
when a trailing slash exists; update the regex used in the pathname.replace(...)
so it also matches the case without a trailing slash (e.g., "/web-ui/web-ui") by
making the trailing slash optional or by matching one or more consecutive
"/web-ui" segments, and ensure the replacement normalizes to a single "/web-ui/"
(maintaining the existing loop and comparison to previousPathname).
In `@web-ui/modules/skills.methods.mjs`:
- Around line 492-495: The importSingleSkill function currently calls
api('import-skills', { targetApp: this.skillsTargetApp, imports: [skill] })
using the wrong payload key; change the payload to match the existing
import-skills contract by sending items: [skill] (and preserve targetApp) so the
backend receives the same shape as the bulk import flow. Locate
importSingleSkill and the api('import-skills', ...) invocation and replace the
imports property with items to align with the existing contract.
In `@web-ui/partials/index/panel-market.html`:
- Around line 97-99: The icon-only import button lacks an accessible name;
update the button with a clear accessible label (e.g., add aria-label="Import
skill" or include visually-hidden text) so screen readers can describe its
action; modify the element with class "skills-flow-add" and handler
importSingleSkill(skill) to include the aria attribute or hidden label while
keeping the existing :disabled binding (loading, initError, skillsMarketBusy)
intact.
In `@web-ui/partials/index/panel-usage.html`:
- Around line 58-60: The template currently uses hardcoded labels ("tokens",
"API", "total", "msgs") in spans like usageCurrentSessionStats.tokenLabel,
usageCurrentSessionStats.apiDurationLabel and
usageCurrentSessionStats.totalDurationLabel; replace those literals with i18n
keys by calling the project's translation helper (e.g., $t or i18n filter) in
the template and update the locale JSON/YAML to add keys (e.g., usage.tokens,
usage.api, usage.total, usage.msgs); ensure you change the occurrences in this
block and the other reported locations (lines around 106-109, 147, 162) so all
UI text is localized and no hardcoded strings remain.
- Around line 95-100: Replace the non-interactive span/div click targets in the
Usage panel with semantic <button type="button"> elements (or add full keyboard
semantics) so they are keyboard-operable; specifically change the v-for span
that calls selectSessionsUsageDay(label.key) and uses sessionsUsageSelectedDay
to a button preserving :key, class="usage-wave-label", :class binding, and the
`@click` handler, and do the same for the other click targets referenced (the
blocks around lines 143–149 and 158–164) ensuring any visual classes are kept,
tabindex/role removed, and aria-pressed or aria-current is added where
appropriate to reflect selection for accessibility.
In `@web-ui/styles/responsive.css`:
- Around line 92-93: Update the CSS declarations that set the shell height (the
height and min-height properties) to include dynamic viewport units for mobile:
keep the existing 100vh values but add 100dvh as a fallback/alternative (i.e.,
specify both units so browsers that support dynamic viewport height use 100dvh
to avoid clipping by mobile UI bars while older browsers fall back to 100vh).
In `@web-ui/styles/sessions-usage.css`:
- Around line 303-305: The CSS rule targets .usage-card-hourly-heatmap but the
template uses a different class name, so overflow-x: auto never applies; locate
the actual class used in the template (e.g., the element rendering the hourly
heatmap) and either rename the template class to usage-card-hourly-heatmap or
update the selector in web-ui/styles/sessions-usage.css (replace
.usage-card-hourly-heatmap) to exactly match the template's class (or add the
template class variant alongside .usage-card-hourly-heatmap) so the overflow-x:
auto rule is applied to the correct element.
🪄 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: f74a2fe7-4c8a-4063-a91e-abf7e11daefe
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
cli.jspackage.jsontests/e2e/test-web-ui-assets.jstests/e2e/test-web-ui-session-tab.jstests/e2e/test-web-ui-url-routing.jstests/unit/compact-layout-ui.test.mjstests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/test-web-ui-url-normalization.mjstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-source-bundle.test.mjstests/unit/web-ui-url-canonicalization.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/logic.sessions.mjsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.computed.session.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.navigation.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/modules/app.methods.session-trash.mjsweb-ui/modules/sessions-filters-url.mjsweb-ui/modules/skills.methods.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-market.htmlweb-ui/partials/index/panel-settings.htmlweb-ui/partials/index/panel-usage.htmlweb-ui/res/vue.runtime.global.prod.jsweb-ui/source-bundle.cjsweb-ui/styles/base-theme.cssweb-ui/styles/navigation-panels.cssweb-ui/styles/responsive.cssweb-ui/styles/sessions-usage.cssweb-ui/styles/skills-market.cssweb-ui/styles/titles-cards.css
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-config-claude.html
[error] 109-109: Special characters must be escaped : [ > ].
(spec-char-escape)
🔇 Additional comments (23)
tests/unit/web-ui-source-bundle.test.mjs (1)
26-27: LGTM!package.json (1)
3-3: LGTM!Also applies to: 49-49
web-ui/partials/index/panel-settings.html (1)
22-23: LGTM!Also applies to: 32-33
web-ui/styles/navigation-panels.css (1)
41-45: LGTM!Also applies to: 51-52
web-ui/partials/index/layout-header.html (1)
3-94: LGTM!Also applies to: 482-487
web-ui/index.html (1)
9-9: ⚡ Quick winVue runtime-only switch is safe here (precompiled render is used)
web-ui/app.jsassignsappOptions.render = window.__CODEXMATE_WEB_UI_RENDER__(when present) beforecreateApp(...).mount('#app'), so the UI doesn’t rely on the Vue template compiler.- The served
/web-ui/app.jsis expected to includewindow.__CODEXMATE_WEB_UI_RENDER__(tests/e2e/test-web-ui-assets.js), aligning with the runtime-only build.web-ui/modules/app.methods.claude-config.mjs (1)
269-271: LGTM!tests/unit/compact-layout-ui.test.mjs (1)
42-48: LGTM!Also applies to: 78-78
tests/unit/run.mjs (1)
16-16: LGTM!web-ui/styles/titles-cards.css (1)
157-169: LGTM!web-ui/modules/app.methods.session-trash.mjs (1)
221-249: LGTM!web-ui/modules/app.methods.session-browser.mjs (1)
267-273: LGTM!tests/e2e/test-web-ui-assets.js (1)
47-59: LGTM!Also applies to: 68-69
web-ui/styles/base-theme.css (1)
29-37: LGTM!tests/unit/web-ui-behavior-parity.test.mjs (1)
518-518: LGTM!Also applies to: 615-619, 705-710, 726-727
web-ui/modules/app.computed.dashboard.mjs (1)
95-97: LGTM!tests/unit/web-run-host.test.mjs (1)
720-720: LGTM!Also applies to: 732-732, 746-758, 774-791
web-ui/source-bundle.cjs (1)
10-13: LGTM!Also applies to: 65-112, 267-267, 279-280
web-ui/logic.sessions.mjs (1)
552-553: LGTM!Also applies to: 559-560
tests/e2e/test-web-ui-session-tab.js (1)
22-40: LGTM!Also applies to: 111-114, 141-144, 176-176
tests/unit/web-ui-url-canonicalization.test.mjs (1)
1-83: LGTM!web-ui/styles/skills-market.css (1)
1-293: LGTM!cli.js (1)
10061-10065: LGTM!Also applies to: 10621-10625, 10652-10663, 11425-11428, 11455-11458, 11519-11522
Comment on lines
+92
to
+93
| /^application\/javascript\b/.test(String(appJs.headers['content-type'] || '')), | ||
| '/web-ui/app.js should return javascript content type' |
Contributor
There was a problem hiding this comment.
Loosen JavaScript content-type assertion to avoid false negatives.
Line 92 only accepts application/javascript, but valid responses may use text/javascript.
Proposed fix
- /^application\/javascript\b/.test(String(appJs.headers['content-type'] || '')),
+ /^(application|text)\/javascript\b/.test(String(appJs.headers['content-type'] || '')),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /^application\/javascript\b/.test(String(appJs.headers['content-type'] || '')), | |
| '/web-ui/app.js should return javascript content type' | |
| /^(application|text)\/javascript\b/.test(String(appJs.headers['content-type'] || '')), | |
| '/web-ui/app.js should return javascript content type' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test-web-ui-url-routing.js` around lines 92 - 93, The test's
content-type assertion is too strict (only matches application/javascript);
update the assertion that checks appJs.headers['content-type'] so it accepts
both application/javascript and text/javascript (and still handles optional
charset), e.g. change the regex used with String(appJs.headers['content-type']
|| '') to allow either "application" or "text" before "/javascript" so the test
won't falsely fail when responses use text/javascript.
Comment on lines
+222
to
+223
| runTests(); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard direct execution and propagate failure exit code.
Line 222 always runs tests on import, and failures only return false, so direct CLI execution can still exit with code 0. Gate execution to direct runs and set process.exitCode on failure.
Proposed fix
+import { fileURLToPath } from 'url';
+
...
-runTests();
+if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) {
+ const ok = runTests();
+ if (!ok) process.exitCode = 1;
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test-web-ui-url-normalization.mjs` around lines 222 - 223,
Currently runTests() is invoked unconditionally on import; change this to only
run when the module is executed directly by adding an ESM guard using
import.meta (e.g. compare process.argv[1] to fileURLToPath(import.meta.url) from
'url'), call await runTests() inside that guard, and set process.exitCode = 1
when runTests() returns a falsy/failed result so CLI failures propagate a
non-zero exit code; reference the runTests() invocation and update surrounding
top-level invocation to be conditional.
|
|
||
| <div class="card-list"> | ||
| <div :class="['card', { active: currentClaudeConfig === 'claude-local' }]" @click="applyClaudeLocalBridge()" @keydown.enter.self.prevent="applyClaudeLocalBridge()" @keydown.space.self.prevent="applyClaudeLocalBridge()" tabindex="0" role="button" :aria-current="currentClaudeConfig === 'claude-local' ? 'true' : null"> | ||
| <div :class="['card', { active: currentClaudeConfig === 'claude-local', disabled: isClaudeLocalBridgeDisabled() }]" @click="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" @keydown.enter.self.prevent="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" @keydown.space.self.prevent="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" :tabindex="isClaudeLocalBridgeDisabled() ? -1 : 0" role="button" :aria-current="currentClaudeConfig === 'claude-local' ? 'true' : null"> |
Contributor
There was a problem hiding this comment.
Add aria-disabled on disabled local bridge card.
Line 103 blocks activation and removes focus, but the control still lacks disabled semantics for screen readers.
Suggested patch
-<div :class="['card', { active: currentClaudeConfig === 'claude-local', disabled: isClaudeLocalBridgeDisabled() }]" `@click`="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" `@keydown.enter.self.prevent`="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" `@keydown.space.self.prevent`="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" :tabindex="isClaudeLocalBridgeDisabled() ? -1 : 0" role="button" :aria-current="currentClaudeConfig === 'claude-local' ? 'true' : null">
+<div :class="['card', { active: currentClaudeConfig === 'claude-local', disabled: isClaudeLocalBridgeDisabled() }]" `@click`="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" `@keydown.enter.self.prevent`="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" `@keydown.space.self.prevent`="isClaudeLocalBridgeDisabled() ? null : applyClaudeLocalBridge()" :tabindex="isClaudeLocalBridgeDisabled() ? -1 : 0" role="button" :aria-disabled="isClaudeLocalBridgeDisabled() ? 'true' : null" :aria-current="currentClaudeConfig === 'claude-local' ? 'true' : null">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/partials/index/panel-config-claude.html` at line 103, The disabled
local bridge card lacks ARIA disabled semantics for screen readers; update the
card element (the div with role="button" that uses
isClaudeLocalBridgeDisabled(), applyClaudeLocalBridge(), and
currentClaudeConfig) to include an :aria-disabled binding that reflects
isClaudeLocalBridgeDisabled() (e.g., aria-disabled="true" when disabled and
"false" otherwise) while keeping the existing tabindex and click/keydown guards
so assistive tech announces the control as disabled.
|
|
||
| <div v-if="!loading && !initError" class="card-list"> | ||
| <div v-for="provider in displayProvidersList" :key="provider.name" :class="['card', { active: displayCurrentProvider === provider.name }]" @click="switchProvider(provider.name)" @keydown.enter.self.prevent="switchProvider(provider.name)" @keydown.space.self.prevent="switchProvider(provider.name)" tabindex="0" role="button" :aria-current="displayCurrentProvider === provider.name ? 'true' : null"> | ||
| <div v-for="provider in displayProvidersList" :key="provider.name" :class="['card', { active: displayCurrentProvider === provider.name, disabled: provider.name === 'local' && isLocalProviderDisabled }]" @click="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" @keydown.enter.self.prevent="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" @keydown.space.self.prevent="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" :tabindex="provider.name === 'local' && isLocalProviderDisabled ? -1 : 0" role="button" :aria-current="displayCurrentProvider === provider.name ? 'true' : null"> |
Contributor
There was a problem hiding this comment.
Expose disabled card state with aria-disabled.
Line 123 disables interaction/focus, but the button-like card does not announce disabled semantics to assistive tech.
Suggested patch
-<div v-for="provider in displayProvidersList" :key="provider.name" :class="['card', { active: displayCurrentProvider === provider.name, disabled: provider.name === 'local' && isLocalProviderDisabled }]" `@click`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" `@keydown.enter.self.prevent`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" `@keydown.space.self.prevent`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" :tabindex="provider.name === 'local' && isLocalProviderDisabled ? -1 : 0" role="button" :aria-current="displayCurrentProvider === provider.name ? 'true' : null">
+<div v-for="provider in displayProvidersList" :key="provider.name" :class="['card', { active: displayCurrentProvider === provider.name, disabled: provider.name === 'local' && isLocalProviderDisabled }]" `@click`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" `@keydown.enter.self.prevent`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" `@keydown.space.self.prevent`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" :tabindex="provider.name === 'local' && isLocalProviderDisabled ? -1 : 0" role="button" :aria-disabled="provider.name === 'local' && isLocalProviderDisabled ? 'true' : null" :aria-current="displayCurrentProvider === provider.name ? 'true' : null">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div v-for="provider in displayProvidersList" :key="provider.name" :class="['card', { active: displayCurrentProvider === provider.name, disabled: provider.name === 'local' && isLocalProviderDisabled }]" @click="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" @keydown.enter.self.prevent="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" @keydown.space.self.prevent="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" :tabindex="provider.name === 'local' && isLocalProviderDisabled ? -1 : 0" role="button" :aria-current="displayCurrentProvider === provider.name ? 'true' : null"> | |
| <div v-for="provider in displayProvidersList" :key="provider.name" :class="['card', { active: displayCurrentProvider === provider.name, disabled: provider.name === 'local' && isLocalProviderDisabled }]" `@click`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" `@keydown.enter.self.prevent`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" `@keydown.space.self.prevent`="(provider.name === 'local' && isLocalProviderDisabled) ? null : switchProvider(provider.name)" :tabindex="provider.name === 'local' && isLocalProviderDisabled ? -1 : 0" role="button" :aria-disabled="provider.name === 'local' && isLocalProviderDisabled ? 'true' : null" :aria-current="displayCurrentProvider === provider.name ? 'true' : null"> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/partials/index/panel-config-codex.html` at line 123, The card acting
as a button should expose its disabled state to assistive tech: update the
element rendered for displayProvidersList (the card that uses
displayCurrentProvider, isLocalProviderDisabled and switchProvider) to add
aria-disabled="true" when (provider.name === 'local' && isLocalProviderDisabled)
and aria-disabled="false" or omit when enabled; keep the current tabindex logic
(set to -1 when disabled) and retain the role="button" and existing
click/keydown guards so the UI remains non-interactive when disabled.
Comment on lines
+44
to
+47
| <button type="button" class="btn-mini" @click="refreshSkillsList({ silent: false })" :disabled="loading || !!initError || skillsMarketBusy"> | ||
| <svg v-if="!skillsLoading" viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.8" width="14" height="14"><path d="M17 1l4 4-4 4"/><path d="M3 11V9a4 4 0 014-4h14"/><path d="M3 19l-4-4 4-4"/><path d="M17 9v2a4 4 0 01-4 4H3"/></svg> | ||
| <svg v-else viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.8" width="14" height="14" class="spin"><path d="M10 2v4m0 8v4M2 10h4m8 0h4m-3.5-6.5l2.5 2.5M4 4l2.5 2.5M16 16l-2.5-2.5M4 16l2.5-2.5"/></svg> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add accessible names to icon-only action buttons.
Line 44 and Line 71 use icon-only buttons without accessible names, so assistive tech cannot announce their actions.
Proposed fix
-<button type="button" class="btn-mini" `@click`="refreshSkillsList({ silent: false })" :disabled="loading || !!initError || skillsMarketBusy">
+<button
+ type="button"
+ class="btn-mini"
+ `@click`="refreshSkillsList({ silent: false })"
+ :disabled="loading || !!initError || skillsMarketBusy"
+ aria-label="Refresh installed skills"
+ title="Refresh installed skills">
...
-<button type="button" class="btn-mini" `@click`="scanImportableSkills({ silent: false })" :disabled="loading || !!initError || skillsMarketBusy">
+<button
+ type="button"
+ class="btn-mini"
+ `@click`="scanImportableSkills({ silent: false })"
+ :disabled="loading || !!initError || skillsMarketBusy"
+ aria-label="Scan importable skills"
+ title="Scan importable skills">Also applies to: 71-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/partials/index/panel-market.html` around lines 44 - 47, The icon-only
action button that calls refreshSkillsList({ silent: false }) (and the other
icon-only button around lines 71-74) lacks an accessible name; add an explicit
accessible label by adding aria-label and title attributes (e.g.
aria-label="Refresh skills list", title="Refresh skills list") or a localized
equivalent (use your i18n helper like $t(...) if available), or include a
visually-hidden <span> with the action text inside the button so screen readers
can announce it; ensure the attributes/text are present on the <button> elements
with `@click`="refreshSkillsList({ silent: false })" and the other icon-only
button(s).
Collaborator
|
Follow-up fix pushed in
Validation performed after this fix:
@coderabbitai re-review |
Contributor
|
529 unit + E2E 全通过 + URL guard probe 继续生效，整体看起来很扎实。我现在开始对最新提交进行全面审查。 (=^･ω･^=)🐇 ✅ Actions performedFull review triggered. |
Collaborator
|
Follow-up fix pushed in Root cause:
Fix:
Validation performed:
@coderabbitai re-review |
Contributor
|
530 个单元测试全通过 + E2E 通过，整体看起来很扎实。我现在开始对最新提交进行全面审查。 (=^･ω･^=)🐇 ✅ Actions performedFull review triggered. |
ymkiux
added a commit
that referenced
this pull request
May 24, 2026
… bump (#170) * chore: revert version to 0.0.34 after npm unpublish * refactor(web-ui): redesign Usage tab with "River of Time" aesthetic - Merge current session bar with hero metrics for unified focus - Replace bar chart with SVG bezier curve wave visualization - Add automatic delta calculation (7d vs prior 7d, 30d vs prior 30d) - Redesign empty states with animated SVG illustrations - Simplify list views with bullet-point compact style - Enhance color system with delta indicators and heatmap levels * fix(web-ui): resolve /web-ui/ 404 and duplicate path issues - Server: handle /web-ui/ requests by returning index.html - Server: explicitly serve index.html for /web-ui/index.html requests - Client: normalize URL on mount to fix /web-ui/web-ui/ duplicates - Client: redirect /web-ui/ and /web-ui/index.html to /web-ui Fixes the issue where refreshing in sessions tab caused URL to become /web-ui/web-ui/index.html * fix(web-ui): strengthen URL normalization to handle nested duplicates - Use do-while loop to iteratively remove all /web-ui/web-ui/ patterns - Handle edge cases like /web-ui/web-ui/web-ui/index.html - Add additional check for /web-ui/web-ui/ prefix pattern * test(web-ui): add URL routing and normalization tests - Add test-web-ui-url-routing.js E2E test for server routing - Add test-web-ui-url-normalization.mjs unit test (24 cases) - Update test-web-ui-assets.js expectations for /web-ui/ fix - Fix shouldReplace check logic in app.js (use originalPathname) * refactor(web-ui): remove /web-ui route, use / as single entry point - Server: /web-ui, /web-ui/, /web-ui/index.html now return 404 - Root path / is the only HTML entry point - Client: auto-redirect from /web-ui/* to / with hash/params preserved - Assets still served at /web-ui/app.js, /web-ui/styles.css, etc. - Simplified routing logic, removed redundant path handling * feat(web): hide local provider in codex/claude config mode - filter out 'local' provider from displayProvidersList when configMode is codex/claude - prevents users from selecting local provider in these apps due to existing issues - other modes (terminal) remain unaffected * feat(web): disable local provider in codex/claude config mode - local provider card shows as disabled (50% opacity, non-clickable) in codex mode - claude-local bridge card shows as disabled in claude mode - adds CSS .card.disabled style with pointer-events: none - cards remain visible but cannot be selected * fix(web): correct disabled provider click logic - fix operator precedence issue in click handlers - use proper ternary expressions with correct grouping - prevents white screen error on config load * fix(web): remove parentheses from computed property - isLocalProviderDisabled is a computed returning boolean, not a function - should be accessed as property without () in template - fixes white screen error on config load * feat(web): rename claude-local to local with system badge - display name changed from 'claude-local' to 'local' - adds system badge like codex provider readonly indicator - internal id 'claude-local' remains unchanged for compatibility * feat(web): remove local provider subtitle hint - removes '勾选参与负载均衡的提供商' subtitle - keeps card layout cleaner with just title and system badge * refactor(web): redesign skills tab to minimalist single-column layout - Remove cluttered overview section with 6 stat cards - Consolidate header with inline target switcher - Single-column flow layout for better mobile experience - Add count badges directly in panel headers - Add quick action buttons in import panel - Add individual import button for each skill - Remove help panel (content available elsewhere) - Remove root path display (moved to tooltip) - Preserve all functionality while reducing visual noise Design principles: - Progressive disclosure: scroll to discover - Large touch targets: 56px min height - Visual hierarchy: clear content grouping - Cleaner information architecture * refactor(web): implement static URL, always stay at / - Remove all URL routing, URL stays as pure '/' - Delete query parameters and hash from browser address bar - Remove history.pushState/replaceState operations for tab switching - Remove session filter URL synchronization - Share links still work (generate URL with params for clipboard) - When opening share link, params are applied then URL cleaned - All state managed via memory + localStorage Changes: - app.js: Clean URL on mount, remove resource path fix logic - app.methods.navigation.mjs: Remove URL operations in switchMainTab - sessions-filters-url.mjs: Empty syncSessionsFilterUrl, fix share URL base - app.methods.session-browser.mjs: Clean URL after applying URL state * chore: bump version to 0.0.35 * fix(ci): restore missing CI scripts in package.json * test: update unit tests to match new web-ui structure - Fix sessionUsageDaily -> sessionUsageHourlyHeatmap assertion - Remove obsolete loadSkillsMarketOverview and market-action-grid checks - Update skills-target-chip selector (was market-target-chip) - Fix provider card disabled state and tabindex assertions - Remove market.pill importableDirect/importMissing checks * test: add new keys to parity baseline allowlists - isLocalProviderDisabled (data key) - sessionUsageWave, sessionsUsageSelectedDay, usageHeroDelta, usageHeroDeltaClass (computed keys) * test: add more usage hero keys to parity baseline - usageHeroMainValue - usageHeroSubLabel * test: fix isLocalProviderDisabled key type Move isLocalProviderDisabled from data keys to computed keys as it is defined as a computed property in app.computed.dashboard.mjs * fix(web-ui): canonicalize session tab navigation URL * fix: stabilize web ui navigation and canonical urls * fix: avoid runtime vue compiler dependency * fix: preserve sessions tab render on navigation --------- Co-authored-by: ymkiux <ymkiux@users.noreply.github.com> Co-authored-by: awsl233777 <awsl233777@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/, all state via localStorageTests
Summary by CodeRabbit
New Features
UI / Styling
Behavior
Tests
Chores