Conversation
…kills UI - Remove Load More button and show all fetched session messages immediately - Auto-fetch remaining clipped messages via $nextTick cascade - Persist promptTemplatesMode and skillsTargetApp across tab switches and page refresh - Polish session browser: solid borders, CSS var unification, refined hover/active states, gradient hot badge - Polish skills/plugins/market panels: glassmorphism modebar, left accent bars, refined action cards - Upgrade all session/action buttons to radius-md, flat surfaces, brand hover accents - Unify timeline nodes and message bubbles with CSS custom properties
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughSession pagination defaults raised and preview rendering changed to render all loaded messages; clipped session details auto-increase limits and re-load. Added a providers-health report (core, CLI/API, Web UI), persisted navigation fields, UI/i18n/computed wiring, and updated tests to match behavior changes. ChangesSession Loading, Providers Health, Navigation, and Dashboard UI
Sequence Diagram(s)sequenceDiagram
participant UI as Web UI
participant API as Server API
participant Core as cli/config-health
participant Provider as External Provider
rect rgba(100,150,240,0.5)
UI->>API: GET /api?action=providers-health&remote=true
API->>Core: buildAllProvidersHealthReport(params)
end
rect rgba(80,200,120,0.5)
Core->>Provider: probe endpoint (parallel)
Provider-->>Core: probe result / error
end
Core-->>API: aggregated providersHealthResult
API-->>UI: providersHealthResult JSON
UI->>UI: store providersHealthResult + render chips/issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
web-ui/partials/index/panel-market.html (1)
135-135: ⚡ Quick winConsider extracting SVG inline styles to CSS classes.
The inline
styleattributes on the SVG icons are repeated across multiple action cards with only minor variations. Extracting to CSS classes would improve maintainability and consistency.♻️ Example CSS class approach
Define a reusable class:
.market-action-icon { width: 14px; height: 14px; vertical-align: -2px; margin-right: 4px; }Then replace inline styles:
<svg class="market-action-icon" viewBox="0 0 20 20" ...>Also applies to: 139-139, 143-143
🤖 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` at line 135, The SVG inline styles used in the market action cards should be extracted into a reusable CSS class (e.g., .market-action-icon) and applied to each SVG to remove duplicated inline style attributes; update the SVG elements inside the span.market-action-title (and the similar SVGs at the other action-card locations referenced) to use the new class instead of the style attribute, and add the corresponding CSS rule in the stylesheet so width/height/vertical-align/margin-right are centralized and consistent.web-ui/partials/index/panel-plugins.html (1)
57-57: ⚡ Quick winConsider extracting inline event handlers and SVG styles for maintainability.
The click handlers now mix state assignment, conditional checks, and function calls in a single expression, and the inline SVG styles are hardcoded. For easier maintenance:
- Extract the click logic to methods (e.g.,
switchToCompose(),switchToManage())- Move SVG styling to CSS classes or extract icons to reusable components
This would make the template more readable and allow icon styling to be updated in one place.
♻️ Example refactor for click handlers
In your Vue methods:
switchToCompose() { this.promptTemplatesMode = 'compose'; if (typeof this.saveNavState === 'function') this.saveNavState(); }, switchToManage() { this.promptTemplatesMode = 'manage'; if (typeof this.saveNavState === 'function') this.saveNavState(); }Then in template:
`@click`="switchToCompose()" `@click`="switchToManage()"For SVG styles, define a CSS class:
.mode-pill-icon { width: 12px; height: 12px; vertical-align: -1px; margin-right: 4px; }Also applies to: 63-63
🤖 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-plugins.html` at line 57, Extract the inline click logic and SVG styles: create component methods switchToCompose() and switchToManage() that set promptTemplatesMode ('compose' / 'manage') and call saveNavState() if it exists, then replace the inline `@click` expressions with those methods; move the inline SVG style into a CSS class (e.g., .mode-pill-icon) or a shared icon component and apply it to the SVG elements so styling is centralized and icons can be reused.web-ui/app.js (1)
225-226: 💤 Low valueSession list batch size doubled—verify viewport fill behavior.
The
sessionListInitialBatchSizeincreased from 20 to 40, andsessionListLoadStepfrom 40 to 80. While this improves the initial render for users with many sessions, ensure that:
- The viewport-fill logic in
scheduleSessionListViewportFill(app.methods.navigation.mjs) still triggers correctly- The initial batch (40) doesn't cause layout shift or scroll position issues
- Users with hundreds of sessions don't experience lag from rendering 40+ session cards at once
🤖 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 225 - 226, You increased sessionListInitialBatchSize to 40 and sessionListLoadStep to 80 which may affect viewport-fill behavior and performance; update and verify scheduleSessionListViewportFill in app.methods.navigation.mjs to ensure it still triggers when scroll/viewport height changes (adjust thresholds if it assumes previous batch sizes), add debounce/throttle to its invocation, and confirm the rendering path that consumes sessionListInitialBatchSize (e.g., the function that maps/creates session cards) uses virtualization/ incremental mounting to avoid layout shift and jank for hundreds of sessions; if necessary, cap initial render count or use requestAnimationFrame batching so rendering 40 items won’t block the main thread.
🤖 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 206-207: The initial message batch
(sessionDetailInitialMessageLimit = 300) plus sessionDetailFetchStep = 300 can
cause large DOM and memory spikes because auto-fetch recursion in
session-helpers.mjs (the incremental fetch loop) pushes many messages into
activeSessionMessages (rendered by panel-sessions.html) at once; reduce
sessionDetailInitialMessageLimit to a smaller safe default (e.g., 100–150), keep
or increase sessionDetailFetchStep for subsequent loads, and/or implement
batching in the consumer (where activeSessionMessages is rendered) so each fetch
appends a limited chunk rather than rendering all messages at once; update the
constants and adjust the auto-fetch loop in session-helpers.mjs and
panel-sessions.html rendering logic (or add a virtualization/batching layer)
accordingly.
In `@web-ui/modules/app.methods.navigation.mjs`:
- Line 693: The change sets this.sessionPreviewVisibleCount directly to total
which forces rendering of all fetched messages and can harm performance for
large sessions; revert to using a capped progressive reveal: initialize
this.sessionPreviewVisibleCount to Math.min(total,
this.sessionPreviewInitialBatchSize) and then incrementally increase it (e.g.,
in the same method that sets the count, use a loop/timer/nextTick to batch-add
more items until it reaches total) so rendering is throttled; update the
assignment at the location that presently does "this.sessionPreviewVisibleCount
= total" and reference sessionPreviewVisibleCount and
sessionPreviewInitialBatchSize when implementing the incremental/increment
logic.
In `@web-ui/session-helpers.mjs`:
- Around line 457-476: The auto-fetch loop triggered when
activeSessionDetailClipped is true currently calls loadActiveSessionDetail on
$nextTick repeatedly, causing rapid sequential API calls; modify the block that
computes nextLimit and calls loadActiveSessionDetail to (1) add a short async
delay (e.g., 100–300ms) before calling loadActiveSessionDetail (use a
sleep/promise or setTimeout) so the recursive fetches are throttled, (2)
introduce a lightweight retry counter or backoff field (e.g.,
sessionDetailAutoFetchRetry) and bail out when it exceeds a small max to avoid
runaway recursion, (3) set a visual flag (e.g., sessionDetailBackgroundLoading)
before the delayed fetch and clear it after loadActiveSessionDetail resolves so
users see background loading, and (4) re-check activeSession,
sessionDetailLoading and activeSessionDetailClipped immediately before invoking
loadActiveSessionDetail to avoid wasted work if the user navigated away; update
places referencing sessionPreviewPendingVisibleCount and
sessionDetailMessageLimit accordingly.
In `@web-ui/styles/plugins-panel.css`:
- Around line 545-549: The .prompt-var-input.is-missing rule uses the animation
shorthand without an animation-name so it never runs; update the rule for
.prompt-var-input.is-missing to either reference a defined `@keyframes` animation
by name (e.g., add/point to a keyframe like shake or pulse and include duration,
timing-function, and name in the animation shorthand) or replace the animation
declaration with a transition (e.g., use transition properties such as
transition: box-shadow var(--transition-normal) ease-in-out) so the intended
visual effect actually occurs.
---
Nitpick comments:
In `@web-ui/app.js`:
- Around line 225-226: You increased sessionListInitialBatchSize to 40 and
sessionListLoadStep to 80 which may affect viewport-fill behavior and
performance; update and verify scheduleSessionListViewportFill in
app.methods.navigation.mjs to ensure it still triggers when scroll/viewport
height changes (adjust thresholds if it assumes previous batch sizes), add
debounce/throttle to its invocation, and confirm the rendering path that
consumes sessionListInitialBatchSize (e.g., the function that maps/creates
session cards) uses virtualization/ incremental mounting to avoid layout shift
and jank for hundreds of sessions; if necessary, cap initial render count or use
requestAnimationFrame batching so rendering 40 items won’t block the main
thread.
In `@web-ui/partials/index/panel-market.html`:
- Line 135: The SVG inline styles used in the market action cards should be
extracted into a reusable CSS class (e.g., .market-action-icon) and applied to
each SVG to remove duplicated inline style attributes; update the SVG elements
inside the span.market-action-title (and the similar SVGs at the other
action-card locations referenced) to use the new class instead of the style
attribute, and add the corresponding CSS rule in the stylesheet so
width/height/vertical-align/margin-right are centralized and consistent.
In `@web-ui/partials/index/panel-plugins.html`:
- Line 57: Extract the inline click logic and SVG styles: create component
methods switchToCompose() and switchToManage() that set promptTemplatesMode
('compose' / 'manage') and call saveNavState() if it exists, then replace the
inline `@click` expressions with those methods; move the inline SVG style into a
CSS class (e.g., .mode-pill-icon) or a shared icon component and apply it to the
SVG elements so styling is centralized and icons can be reused.
🪄 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: 92261f29-674d-4f93-a5c2-a4b1a34555b7
📒 Files selected for processing (14)
tests/e2e/test-web-ui-session-browser.jsweb-ui/app.jsweb-ui/modules/app.methods.navigation.mjsweb-ui/modules/skills.methods.mjsweb-ui/partials/index/panel-market.htmlweb-ui/partials/index/panel-plugins.htmlweb-ui/partials/index/panel-sessions.htmlweb-ui/session-helpers.mjsweb-ui/styles/plugins-panel.cssweb-ui/styles/sessions-list.cssweb-ui/styles/sessions-preview.cssweb-ui/styles/sessions-toolbar-trash.cssweb-ui/styles/skills-list.cssweb-ui/styles/skills-market.css
📜 Review details
🔇 Additional comments (11)
web-ui/styles/skills-list.css (1)
52-56: LGTM!Also applies to: 95-97, 100-103, 107-114
web-ui/styles/skills-market.css (1)
157-172: LGTM!Also applies to: 177-180, 184-186, 190-195, 237-246, 270-279, 284-299, 331-344, 374-395, 399-405, 410-411
web-ui/styles/sessions-list.css (1)
2-5: LGTM!Also applies to: 14-20, 45-46, 141-142, 198-200, 235-239, 249-250, 259-261, 264-267, 270-272, 275-276, 298-303, 313-316, 331-332, 338-339, 342-345, 349-350, 386-390, 407-408, 416-417
web-ui/styles/sessions-preview.css (1)
25-36: LGTM!Also applies to: 102-106, 112-113, 124-127, 138-139, 148-154, 157-160, 163-166, 183-189, 193-195, 205-206, 209-214, 248-249, 269-274
web-ui/styles/sessions-toolbar-trash.css (1)
35-38: LGTM!Also applies to: 53-57, 66-68, 91-102, 105-106, 111-115, 120-121, 130-134, 259-263, 272-277, 325-326
web-ui/styles/plugins-panel.css (1)
3-4: LGTM!Also applies to: 9-12, 27-28, 32-33, 40-49, 52-54, 57-70, 79-80, 83-87, 95-100, 110-111, 122-126, 128-138, 142-151, 154-157, 159-162, 166-167, 180-181, 185-191, 268-278, 281-283, 310-314, 318-320, 330-337, 350-361, 365-367, 402-413, 416-418, 421-433, 458-462, 495-499, 535-539
web-ui/modules/skills.methods.mjs (1)
57-57: LGTM!web-ui/modules/app.methods.navigation.mjs (2)
72-79: LGTM!
106-131: LGTM!web-ui/partials/index/panel-sessions.html (1)
265-265: LGTM!tests/e2e/test-web-ui-session-browser.js (1)
373-379: LGTM!
Comment on lines
+206
to
+207
| sessionDetailInitialMessageLimit: 300, | ||
| sessionDetailFetchStep: 300, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Analyze session message counts and sizes to validate new pagination defaults
echo "=== Session message count distribution ==="
fd -e jsonl -t f . --exec sh -c '
count=$(grep -c "\"type\":\"response_item\"" "$1" 2>/dev/null || echo 0)
if [ "$count" -gt 0 ]; then
echo "$count"
fi
' _ {} | sort -n | awk '
BEGIN { total=0; count=0; }
{
total+=$1; count++;
if ($1 > 300) over300++;
if ($1 > 80) over80++;
}
END {
if (count > 0) {
print "Total sessions analyzed:", count
print "Average messages per session:", int(total/count)
print "Sessions with >80 messages:", over80
print "Sessions with >300 messages:", over300
}
}'
echo ""
echo "=== Large message detection (>10KB) ==="
fd -e jsonl -t f . --exec sh -c '
filename=$(basename "$1")
grep "\"type\":\"response_item\"" "$1" 2>/dev/null | while read line; do
size=${`#line`}
if [ "$size" -gt 10240 ]; then
echo "$filename: message size ${size} bytes"
fi
done
' _ {}Repository: SakuraByteCore/codexmate
Length of output: 152
🏁 Script executed:
# Check if the files mentioned exist and examine the specific line ranges
find . -name "app.js" -path "*/web-ui/*" -type fRepository: SakuraByteCore/codexmate
Length of output: 84
🏁 Script executed:
# Search for session-helpers.mjs
find . -name "session-helpers.mjs" -type fRepository: SakuraByteCore/codexmate
Length of output: 97
🏁 Script executed:
# List the repository structure to understand the layout
git ls-files | head -30Repository: SakuraByteCore/codexmate
Length of output: 726
🏁 Script executed:
# Check app.js size and read lines around 206-207
wc -l ./web-ui/app.js
sed -n '200,215p' ./web-ui/app.jsRepository: SakuraByteCore/codexmate
Length of output: 831
🏁 Script executed:
# Check session-helpers.mjs size and read lines around 457-476
wc -l ./web-ui/session-helpers.mjs
sed -n '450,480p' ./web-ui/session-helpers.mjsRepository: SakuraByteCore/codexmate
Length of output: 1731
🏁 Script executed:
# Search for batching-related code in web-ui
rg -n "batch" ./web-ui/ --type js --type mjs -iRepository: SakuraByteCore/codexmate
Length of output: 96
🏁 Script executed:
# Look at the loadActiveSessionDetail method to understand the impact
rg -n "loadActiveSessionDetail" ./web-ui/session-helpers.mjs -A 30 | head -80Repository: SakuraByteCore/codexmate
Length of output: 3766
🏁 Script executed:
# Check git history for the change from 80 to 300
git log --oneline -n 20 -- ./web-ui/app.jsRepository: SakuraByteCore/codexmate
Length of output: 154
🏁 Script executed:
# Search for batching without mjs file type restriction
rg -n "batch" ./web-ui/ -iRepository: SakuraByteCore/codexmate
Length of output: 6160
🏁 Script executed:
# Look at git diff for the commit that changed these lines
git show 349a50e --statRepository: SakuraByteCore/codexmate
Length of output: 17245
🏁 Script executed:
# Find the actual loadActiveSessionDetail implementation
rg -n "loadActiveSessionDetail\s*\(" ./web-ui/session-helpers.mjs -A 50 | head -120Repository: SakuraByteCore/codexmate
Length of output: 5272
🏁 Script executed:
# Find the message rendering implementation
rg -n "renderActiveSessionDetail|renderSessionPreview|render.*message" ./web-ui/ -i | head -40Repository: SakuraByteCore/codexmate
Length of output: 490
🏁 Script executed:
# Look for the actual rendering/batching code in the computed properties
sed -n '420,480p' ./web-ui/modules/app.computed.session.mjsRepository: SakuraByteCore/codexmate
Length of output: 3077
🏁 Script executed:
# Check how sessionPreviewVisibleCount is managed - is it incremented by auto-fetch?
rg -n "sessionPreviewVisibleCount\|primeSessionPreviewMessageRender" ./web-ui/session-helpers.mjs -A 3 -B 1Repository: SakuraByteCore/codexmate
Length of output: 50
🏁 Script executed:
# Look for the renderSessionPreview or message batch rendering
sed -n '480,550p' ./web-ui/modules/app.computed.session.mjsRepository: SakuraByteCore/codexmate
Length of output: 3785
🏁 Script executed:
# Search more broadly for sessionPreviewVisibleCount
rg -n "sessionPreviewVisibleCount" ./web-ui/ | head -20Repository: SakuraByteCore/codexmate
Length of output: 1155
🏁 Script executed:
# Look at the specific auto-fetch logic more carefully
sed -n '455,480p' ./web-ui/session-helpers.mjsRepository: SakuraByteCore/codexmate
Length of output: 1433
🏁 Script executed:
# Verify the initial rendering batch size
sed -n '225,235p' ./web-ui/app.js | grep -A 2 -B 2 "sessionPreviewInitialBatchSize"Repository: SakuraByteCore/codexmate
Length of output: 308
🏁 Script executed:
# Check if there's any message size filtering or lazy loading in the template
rg -n "activeSessionVisibleMessages" ./web-ui/partials/index/panel-sessions.html -B 2 -A 5Repository: SakuraByteCore/codexmate
Length of output: 685
🏁 Script executed:
# Look at the rendering method for messages in the computed properties
sed -n '435,475p' ./web-ui/modules/app.computed.session.mjsRepository: SakuraByteCore/codexmate
Length of output: 2200
🏁 Script executed:
# Check primeSessionPreviewMessageRender to see the actual rendering logic
rg -n "primeSessionPreviewMessageRender" ./web-ui/ -A 15 | head -60Repository: SakuraByteCore/codexmate
Length of output: 5493
🏁 Script executed:
# Check what happens when sessionDetailLoading completes - does it trigger rendering?
sed -n '430,460p' ./web-ui/session-helpers.mjsRepository: SakuraByteCore/codexmate
Length of output: 1416
🏁 Script executed:
# Check what the rendering actually does - find the template loop for messages
rg -n "activeSessionVisibleMessages" ./web-ui/partials/index/panel-sessions.html -A 10Repository: SakuraByteCore/codexmate
Length of output: 881
🏁 Script executed:
# Look for any virtual scrolling or message virtualization
rg -n "virtual\|infinite\|scroll" ./web-ui/ -i | grep -v "node_modules\|test" | head -20Repository: SakuraByteCore/codexmate
Length of output: 50
🏁 Script executed:
# Look at the full template to see how messages are rendered
sed -n '260,320p' ./web-ui/partials/index/panel-sessions.htmlRepository: SakuraByteCore/codexmate
Length of output: 3748
🏁 Script executed:
# Check if there's any batching in the v-for loop for messages
rg -n "v-for.*activeSessionVisibleMessages|v-for.*message" ./web-ui/partials/index/panel-sessions.html -A 5Repository: SakuraByteCore/codexmate
Length of output: 50
Clarify the impact of increased message loading and pagination strategy.
The sessionDetailInitialMessageLimit was set to 300 (vs. the runtime sessionDetailMessageLimit of 80), and combined with the auto-fetch recursion (lines 457-476 in session-helpers.mjs), a clipped session will trigger multiple fetches of 300 messages each (up to the 1000 cap). The template renders all activeSessionMessages at once without batching (line 265 in panel-sessions.html), so a large session could accumulate 300→600→900→1000 messages in DOM at once.
For sessions with large message payloads, this could cause:
- Significant memory pressure from 300+ large messages in
activeSessionMessages - Slower initial tab load and re-renders after each auto-fetch increment
- Potential rendering jank (v-memo provides some optimization but all messages are still rendered)
Consider whether the 300-message initial batch is justified by your typical session sizes, or if starting smaller (e.g., 100-150) with larger fetch steps would be safer.
🤖 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 206 - 207, The initial message batch
(sessionDetailInitialMessageLimit = 300) plus sessionDetailFetchStep = 300 can
cause large DOM and memory spikes because auto-fetch recursion in
session-helpers.mjs (the incremental fetch loop) pushes many messages into
activeSessionMessages (rendered by panel-sessions.html) at once; reduce
sessionDetailInitialMessageLimit to a smaller safe default (e.g., 100–150), keep
or increase sessionDetailFetchStep for subsequent loads, and/or implement
batching in the consumer (where activeSessionMessages is rendered) so each fetch
appends a limited chunk rather than rendering all messages at once; update the
constants and adjust the auto-fetch loop in session-helpers.mjs and
panel-sessions.html rendering logic (or add a virtualization/batching layer)
accordingly.
| ? Math.max(1, Math.floor(this.sessionPreviewInitialBatchSize)) | ||
| : 40; | ||
| this.sessionPreviewVisibleCount = Math.min(baseSize, total); | ||
| this.sessionPreviewVisibleCount = total; |
Contributor
There was a problem hiding this comment.
Removing message batching may cause rendering performance issues for large sessions.
Previously, sessionPreviewVisibleCount was capped by sessionPreviewInitialBatchSize (12), and users would progressively load more messages. Now it's set directly to total, meaning all loaded messages render immediately.
For a session with 300+ messages (now the default initial load), this could cause:
- Initial render blocking the main thread
- Large DOM tree (300+ message cards)
- Scroll performance degradation
- Memory overhead from Vue tracking hundreds of reactive message elements
The PR summary mentions "display all fetched session messages immediately," but consider whether progressive rendering (even if automatic/invisible) would provide better UX for huge sessions.
Possible mitigation: Virtual scrolling or windowing
If large sessions become a performance bottleneck, consider implementing virtual scrolling (render only visible messages) using a library like vue-virtual-scroller or a custom intersection-observer-based approach. However, this would be a significant refactor.
For now, monitor performance with sessions containing 300+ messages.
🤖 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` at line 693, The change sets
this.sessionPreviewVisibleCount directly to total which forces rendering of all
fetched messages and can harm performance for large sessions; revert to using a
capped progressive reveal: initialize this.sessionPreviewVisibleCount to
Math.min(total, this.sessionPreviewInitialBatchSize) and then incrementally
increase it (e.g., in the same method that sets the count, use a
loop/timer/nextTick to batch-add more items until it reaches total) so rendering
is throttled; update the assignment at the location that presently does
"this.sessionPreviewVisibleCount = total" and reference
sessionPreviewVisibleCount and sessionPreviewInitialBatchSize when implementing
the incremental/increment logic.
Comment on lines
+457
to
+476
| if (this.activeSessionDetailClipped) { | ||
| const autoFetchSession = currentActiveSession; | ||
| this.$nextTick(() => { | ||
| if (this.activeSession !== autoFetchSession) return; | ||
| if (this.sessionDetailLoading) return; | ||
| if (!this.activeSessionDetailClipped) return; | ||
| const currentLimit = Number(this.sessionDetailMessageLimit); | ||
| const fetchStep = Number.isFinite(this.sessionDetailFetchStep) | ||
| ? Math.max(1, Math.floor(this.sessionDetailFetchStep)) | ||
| : 300; | ||
| const limitCap = Number.isFinite(this.sessionDetailMessageLimitCap) | ||
| ? Math.max(1, Math.floor(this.sessionDetailMessageLimitCap)) | ||
| : 1000; | ||
| const nextLimit = Math.min(currentLimit + fetchStep, limitCap); | ||
| if (nextLimit <= currentLimit) return; | ||
| this.sessionDetailMessageLimit = nextLimit; | ||
| this.sessionPreviewPendingVisibleCount = nextLimit; | ||
| void this.loadActiveSessionDetail({ preserveVisibleCount: true }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Auto-fetch recursion may trigger rapid sequential API calls for large clipped sessions.
When activeSessionDetailClipped is true, the code schedules a recursive call to loadActiveSessionDetail on $nextTick. For a session with 1325 messages (from the e2e test), this pattern would trigger:
- Initial load: 300 messages → clipped
- Auto-fetch (nextTick): 600 messages → clipped
- Auto-fetch (nextTick): 900 messages → clipped
- Auto-fetch (nextTick): 1000 messages (cap) → possibly still clipped
This results in 4 rapid sequential API calls with no delay between them. Potential issues:
- API rate limiting or server load
- No visual feedback to the user that background loading is happening (loading spinner turns off before recursion)
- User might navigate away during auto-fetch, wasting resources
- The guards prevent race conditions but don't prevent resource waste
Consider:
- Adding a small delay (e.g., 100-300ms) between auto-fetch attempts
- Setting a visual indicator (e.g., "Loading more messages in background...")
- Adding a max retry counter to prevent infinite loops if the clipped flag doesn't clear
Example: Add delay between auto-fetches
if (this.activeSessionDetailClipped) {
const autoFetchSession = currentActiveSession;
- this.$nextTick(() => {
+ setTimeout(() => {
if (this.activeSession !== autoFetchSession) return;
if (this.sessionDetailLoading) return;
if (!this.activeSessionDetailClipped) return;
const currentLimit = Number(this.sessionDetailMessageLimit);
const fetchStep = Number.isFinite(this.sessionDetailFetchStep)
? Math.max(1, Math.floor(this.sessionDetailFetchStep))
: 300;
const limitCap = Number.isFinite(this.sessionDetailMessageLimitCap)
? Math.max(1, Math.floor(this.sessionDetailMessageLimitCap))
: 1000;
const nextLimit = Math.min(currentLimit + fetchStep, limitCap);
if (nextLimit <= currentLimit) return;
this.sessionDetailMessageLimit = nextLimit;
this.sessionPreviewPendingVisibleCount = nextLimit;
void this.loadActiveSessionDetail({ preserveVisibleCount: true });
- });
+ }, 200);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/session-helpers.mjs` around lines 457 - 476, The auto-fetch loop
triggered when activeSessionDetailClipped is true currently calls
loadActiveSessionDetail on $nextTick repeatedly, causing rapid sequential API
calls; modify the block that computes nextLimit and calls
loadActiveSessionDetail to (1) add a short async delay (e.g., 100–300ms) before
calling loadActiveSessionDetail (use a sleep/promise or setTimeout) so the
recursive fetches are throttled, (2) introduce a lightweight retry counter or
backoff field (e.g., sessionDetailAutoFetchRetry) and bail out when it exceeds a
small max to avoid runaway recursion, (3) set a visual flag (e.g.,
sessionDetailBackgroundLoading) before the delayed fetch and clear it after
loadActiveSessionDetail resolves so users see background loading, and (4)
re-check activeSession, sessionDetailLoading and activeSessionDetailClipped
immediately before invoking loadActiveSessionDetail to avoid wasted work if the
user navigated away; update places referencing sessionPreviewPendingVisibleCount
and sessionDetailMessageLimit accordingly.
Comment on lines
545
to
549
| .prompt-var-input.is-missing { | ||
| border-color: var(--color-error); | ||
| box-shadow: 0 0 0 1px rgba(196, 69, 54, 0.18); | ||
| box-shadow: 0 0 0 2px rgba(196,69,54,0.12); | ||
| animation: var(--transition-normal) ease-in-out; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this selector currently references a named animation
rg -n "prompt-var-input\.is-missing|animation:|@keyframes" web-ui/styles/plugins-panel.cssRepository: SakuraByteCore/codexmate
Length of output: 160
🏁 Script executed:
# Find CSS variable definitions for --transition-normal
rg -n "transition-normal|--transition" web-ui/styles/ -A 1 -B 1 | head -50Repository: SakuraByteCore/codexmate
Length of output: 3175
🏁 Script executed:
# Check if there are any `@keyframes` definitions anywhere in the styles directory
fd "\.css$" web-ui/styles/ | xargs rg -l "@keyframes"Repository: SakuraByteCore/codexmate
Length of output: 189
🏁 Script executed:
# Get full context of the .prompt-var-input.is-missing selector
sed -n '545,550p' web-ui/styles/plugins-panel.cssRepository: SakuraByteCore/codexmate
Length of output: 241
🏁 Script executed:
# Search for any prompt-var related keyframes or animations
rg -i "prompt-var|missing.*pulse" web-ui/styles/ --type cssRepository: SakuraByteCore/codexmate
Length of output: 657
🏁 Script executed:
# Check if this selector is used elsewhere or if there's related animation intent
rg -n "prompt-var-input" web-ui/styles/ --type css -B 2 -A 4Repository: SakuraByteCore/codexmate
Length of output: 680
animation shorthand is missing an animation name
Line 548 sets only timing values without an animation name; the declaration defaults to animation-name: none and won't execute. Add a named keyframes animation or use transition instead.
Suggested fix
.prompt-var-input.is-missing {
border-color: var(--color-error);
box-shadow: 0 0 0 2px rgba(196,69,54,0.12);
- animation: var(--transition-normal) ease-in-out;
+ animation: prompt-var-missing-pulse var(--transition-normal) ease-in-out;
}
+
+@keyframes prompt-var-missing-pulse {
+ 0% { box-shadow: 0 0 0 0 rgba(196,69,54,0.22); }
+ 100% { box-shadow: 0 0 0 2px rgba(196,69,54,0.12); }
+}📝 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
| .prompt-var-input.is-missing { | |
| border-color: var(--color-error); | |
| box-shadow: 0 0 0 1px rgba(196, 69, 54, 0.18); | |
| box-shadow: 0 0 0 2px rgba(196,69,54,0.12); | |
| animation: var(--transition-normal) ease-in-out; | |
| } | |
| .prompt-var-input.is-missing { | |
| border-color: var(--color-error); | |
| box-shadow: 0 0 0 2px rgba(196,69,54,0.12); | |
| animation: prompt-var-missing-pulse var(--transition-normal) ease-in-out; | |
| } | |
| `@keyframes` prompt-var-missing-pulse { | |
| 0% { box-shadow: 0 0 0 0 rgba(196,69,54,0.22); } | |
| 100% { box-shadow: 0 0 0 2px rgba(196,69,54,0.12); } | |
| } |
🤖 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/plugins-panel.css` around lines 545 - 549, The
.prompt-var-input.is-missing rule uses the animation shorthand without an
animation-name so it never runs; update the rule for
.prompt-var-input.is-missing to either reference a defined `@keyframes` animation
by name (e.g., add/point to a keyframe like shake or pulse and include duration,
timing-function, and name in the animation shorthand) or replace the animation
declaration with a transition (e.g., use transition properties such as
transition: box-shadow var(--transition-normal) ease-in-out) so the intended
visual effect actually occurs.
ymkiux
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/config-health.js`:
- Around line 346-360: The current early-return treats a virtual/fallback config
(from readConfigOrVirtualDefault()) as a healthy empty state; instead detect
when readConfigOrVirtualDefault() produced a virtual/default due to parse/read
failure (check properties on the returned status object such as an error field,
isVirtual/isFallback flag, or similar) and, when present, return ok: false (or
propagate the error) with the error details rather than returning ok: true;
update the block that references status, status.config, providerEntries,
currentProvider and readCurrentModels to inspect status for parse/read failure
and include status.error (or equivalent) in the returned response so real config
errors aren’t masked.
- Around line 391-433: The status computation can return "green" even when a
remote probe was requested but skipped/failed; update the providers mapping so
remote probes affect status: inside the entries.map that builds providers,
compute a remoteRequested flag (use the existing outer variable remote) and then
set statusValue = 'red' if entry.remote && !entry.remote.ok, else set
statusValue = 'yellow' if hasConfigIssue OR (remoteRequested && (entry.remote
=== null || entry.issues.length > 0)); otherwise 'green'. Update references in
that block (providers mapping, statusValue, remoteFailed) so providers reflect
remote probe skips/exceptions from runRemoteHealthCheck and caught errors.
- Around line 366-372: The mapping over providerEntries accesses provider
properties directly and can crash if provider is null/non-object; update the
providerEntries.map callback (the entries constant) to first validate or coerce
provider (e.g., skip non-object entries or set const safeProvider = provider &&
typeof provider === 'object' ? provider : {}) before reading base_url,
requires_openai_auth, preferred_auth_method and computing modelName (references:
providerEntries.map, provider, baseUrl, requiresAuth, apiKey, modelName,
currentModels, currentProvider, config.model); ensure you either filter out
malformed entries earlier or use the guarded safeProvider when deriving
baseUrl/requiresAuth/apiKey/modelName so the report doesn't throw on malformed
model_providers.
In `@web-ui/app.js`:
- Around line 574-576: The explicit call to runProvidersHealthCheck({ remote:
true }) duplicates the request already triggered from runHealthCheck()’s finally
path on initial dashboard load; remove this direct invocation (or replace it
with a guard) so only runHealthCheck() triggers providers health on startup.
Specifically, delete or gate the lines that call runProvidersHealthCheck in the
initial-load code path (the block containing "if (typeof
this.runProvidersHealthCheck === 'function') { void
this.runProvidersHealthCheck({ remote: true }); }") and either rely solely on
runHealthCheck() or add a boolean flag (e.g., hasRunInitialProvidersHealthCheck)
checked/set by runHealthCheck to prevent double requests.
In `@web-ui/modules/app.methods.codex-config.mjs`:
- Around line 570-585: Concurrent calls to runProvidersHealthCheck can let older
async responses overwrite newer state; add a request token/sequence gate:
introduce a per-instance counter (e.g. this.providersHealthRequestSeq) that you
increment at the start of runProvidersHealthCheck, capture the current seq in a
local variable, and only update this.providersHealthResult and
this.providersHealthLoading inside the try/catch/finally if the captured seq
matches this.providersHealthRequestSeq (or store latestToken and compare).
Reference runProvidersHealthCheck, providersHealthResult, and
providersHealthLoading when implementing the sequence check so stale responses
are ignored.
In `@web-ui/partials/index/panel-dashboard.html`:
- Around line 125-127: The template accesses providersHealthResult.summary and
providersHealthResult.providers directly which can crash if the API returns a
partial object; update the template to guard these accesses (e.g., change
v-if="providersHealthResult" to v-if="providersHealthResult &&
providersHealthResult.summary" for the summary span and use
v-if="providersHealthResult && providersHealthResult.providers" where providers
are iterated/used, or use optional chaining in bindings like {{
providersHealthResult?.summary?.green }} / {{
providersHealthResult?.summary?.total }}); apply the same guards for the other
occurrences noted (around the elements at 130-131 and 143-145) to prevent
render-time errors.
🪄 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: 6da39d7e-6bb1-4798-8771-7e80b4656b41
📒 Files selected for processing (9)
cli.jscli/config-health.jstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.methods.codex-config.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-dashboard.htmlweb-ui/styles/dashboard.css
📜 Review details
🔇 Additional comments (3)
tests/unit/web-ui-behavior-parity.test.mjs (1)
348-350: LGTM!Also applies to: 377-379, 585-586, 675-677
cli/config-health.js (1)
450-454: LGTM!cli.js (1)
93-93: LGTM!Also applies to: 1750-1756, 10284-10286
Comment on lines
+346
to
+360
| const status = readConfigOrVirtualDefault(); | ||
| const config = status.config || {}; | ||
| const providerEntries = config.model_providers && typeof config.model_providers === 'object' | ||
| ? Object.entries(config.model_providers) | ||
| : []; | ||
| const currentProvider = typeof config.model_provider === 'string' ? config.model_provider.trim() : ''; | ||
| const currentModels = typeof readCurrentModels === 'function' ? readCurrentModels() : {}; | ||
|
|
||
| if (!providerEntries.length) { | ||
| return { | ||
| ok: true, | ||
| currentProvider, | ||
| providers: [], | ||
| summary: { total: 0, green: 0, yellow: 0, red: 0 } | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle virtual config parse/read failures before returning ok: true.
If readConfigOrVirtualDefault() returns a virtual config due to parse/read failure, this path currently returns a healthy empty report. That masks real config errors and can mislead the dashboard.
🤖 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/config-health.js` around lines 346 - 360, The current early-return treats
a virtual/fallback config (from readConfigOrVirtualDefault()) as a healthy empty
state; instead detect when readConfigOrVirtualDefault() produced a
virtual/default due to parse/read failure (check properties on the returned
status object such as an error field, isVirtual/isFallback flag, or similar)
and, when present, return ok: false (or propagate the error) with the error
details rather than returning ok: true; update the block that references status,
status.config, providerEntries, currentProvider and readCurrentModels to inspect
status for parse/read failure and include status.error (or equivalent) in the
returned response so real config errors aren’t masked.
Comment on lines
+366
to
+372
| const entries = providerEntries.map(([name, provider]) => { | ||
| const baseUrl = typeof provider.base_url === 'string' ? provider.base_url.trim() : ''; | ||
| const requiresAuth = provider.requires_openai_auth !== false; | ||
| const apiKey = typeof provider.preferred_auth_method === 'string' | ||
| ? provider.preferred_auth_method.trim() : ''; | ||
| const modelName = currentModels[name] || (name === currentProvider ? (config.model || '').trim() : ''); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard provider shape before property access to avoid runtime crashes.
On Line 367 and adjacent lines, provider is dereferenced without confirming it is an object. A malformed model_providers entry (e.g. null) will throw and fail the report.
Proposed fix
- const entries = providerEntries.map(([name, provider]) => {
- const baseUrl = typeof provider.base_url === 'string' ? provider.base_url.trim() : '';
- const requiresAuth = provider.requires_openai_auth !== false;
- const apiKey = typeof provider.preferred_auth_method === 'string'
- ? provider.preferred_auth_method.trim() : '';
- const modelName = currentModels[name] || (name === currentProvider ? (config.model || '').trim() : '');
+ const entries = providerEntries.map(([name, provider]) => {
+ const providerObj = provider && typeof provider === 'object' ? provider : {};
+ const baseUrl = typeof providerObj.base_url === 'string' ? providerObj.base_url.trim() : '';
+ const requiresAuth = providerObj.requires_openai_auth !== false;
+ const apiKey = typeof providerObj.preferred_auth_method === 'string'
+ ? providerObj.preferred_auth_method.trim() : '';
+ const currentModel = typeof config.model === 'string' ? config.model.trim() : '';
+ const modelName = currentModels[name] || (name === currentProvider ? currentModel : '');
@@
- return { name, provider, baseUrl, modelName, checks, issues };
+ return { name, provider: providerObj, baseUrl, modelName, checks, issues };
});🤖 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/config-health.js` around lines 366 - 372, The mapping over
providerEntries accesses provider properties directly and can crash if provider
is null/non-object; update the providerEntries.map callback (the entries
constant) to first validate or coerce provider (e.g., skip non-object entries or
set const safeProvider = provider && typeof provider === 'object' ? provider :
{}) before reading base_url, requires_openai_auth, preferred_auth_method and
computing modelName (references: providerEntries.map, provider, baseUrl,
requiresAuth, apiKey, modelName, currentModels, currentProvider, config.model);
ensure you either filter out malformed entries earlier or use the guarded
safeProvider when deriving baseUrl/requiresAuth/apiKey/modelName so the report
doesn't throw on malformed model_providers.
Comment on lines
+391
to
+433
| if (remote) { | ||
| const remotePromises = entries.map(async (entry) => { | ||
| if (!entry.checks.baseUrlValid || !entry.modelName) { | ||
| entry.remote = null; | ||
| return; | ||
| } | ||
| try { | ||
| const report = await runRemoteHealthCheck(entry.name, entry.provider, entry.modelName, { | ||
| timeoutMs, | ||
| probeJsonPost: probeJsonPostDep | ||
| }); | ||
| entry.remote = report.remote; | ||
| entry.issues.push(...report.issues); | ||
| } catch (e) { | ||
| entry.remote = null; | ||
| entry.issues.push({ | ||
| code: 'remote-probe-error', | ||
| message: `${entry.name} 远程探测异常: ${e.message}`, | ||
| suggestion: '检查网络连接与 endpoint' | ||
| }); | ||
| } | ||
| }); | ||
| await Promise.allSettled(remotePromises); | ||
| } | ||
|
|
||
| const providers = entries.map((entry) => { | ||
| const hasConfigIssue = !entry.checks.baseUrlValid || !entry.checks.apiKeyPresent; | ||
| const remoteFailed = entry.remote && !entry.remote.ok; | ||
| let statusValue = 'green'; | ||
| if (remoteFailed) { | ||
| statusValue = 'red'; | ||
| } else if (hasConfigIssue) { | ||
| statusValue = 'yellow'; | ||
| } | ||
| return { | ||
| provider: entry.name, | ||
| isCurrent: entry.name === currentProvider, | ||
| status: statusValue, | ||
| checks: entry.checks, | ||
| remote: entry.remote || null, | ||
| issues: entry.issues | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
Don’t classify providers as green when remote mode produced issues.
Current status logic can report green when remote probing was requested but skipped/failed with issues (e.g. missing model or caught probe exception with entry.remote = null). This creates false positives in summary totals.
Proposed fix
if (remote) {
const remotePromises = entries.map(async (entry) => {
- if (!entry.checks.baseUrlValid || !entry.modelName) {
+ if (!entry.checks.baseUrlValid) {
entry.remote = null;
return;
}
+ if (!entry.modelName) {
+ entry.remote = null;
+ entry.issues.push({
+ code: 'remote-skip-model',
+ message: `${entry.name} 无法进行远程探测：未设置模型`,
+ suggestion: '为该 provider 设置模型后重试'
+ });
+ return;
+ }
try {
@@
const providers = entries.map((entry) => {
const hasConfigIssue = !entry.checks.baseUrlValid || !entry.checks.apiKeyPresent;
const remoteFailed = entry.remote && !entry.remote.ok;
+ const hasAnyIssue = entry.issues.length > 0;
let statusValue = 'green';
if (remoteFailed) {
statusValue = 'red';
- } else if (hasConfigIssue) {
+ } else if (hasConfigIssue || hasAnyIssue) {
statusValue = 'yellow';
}🤖 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/config-health.js` around lines 391 - 433, The status computation can
return "green" even when a remote probe was requested but skipped/failed; update
the providers mapping so remote probes affect status: inside the entries.map
that builds providers, compute a remoteRequested flag (use the existing outer
variable remote) and then set statusValue = 'red' if entry.remote &&
!entry.remote.ok, else set statusValue = 'yellow' if hasConfigIssue OR
(remoteRequested && (entry.remote === null || entry.issues.length > 0));
otherwise 'green'. Update references in that block (providers mapping,
statusValue, remoteFailed) so providers reflect remote probe skips/exceptions
from runRemoteHealthCheck and caught errors.
Comment on lines
+574
to
+576
| if (typeof this.runProvidersHealthCheck === 'function') { | ||
| void this.runProvidersHealthCheck({ remote: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid duplicate providers-health requests during initial dashboard load.
On first dashboard load, this explicit call duplicates the request already fired from runHealthCheck()’s finally path (in codex mode), causing redundant API traffic and loading-state churn. Keeping a single trigger path is cleaner.
Suggested minimal fix
- if (typeof this.runProvidersHealthCheck === 'function') {
- void this.runProvidersHealthCheck({ remote: true });
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (typeof this.runProvidersHealthCheck === 'function') { | |
| void this.runProvidersHealthCheck({ remote: true }); | |
| } |
🤖 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 574 - 576, The explicit call to
runProvidersHealthCheck({ remote: true }) duplicates the request already
triggered from runHealthCheck()’s finally path on initial dashboard load; remove
this direct invocation (or replace it with a guard) so only runHealthCheck()
triggers providers health on startup. Specifically, delete or gate the lines
that call runProvidersHealthCheck in the initial-load code path (the block
containing "if (typeof this.runProvidersHealthCheck === 'function') { void
this.runProvidersHealthCheck({ remote: true }); }") and either rely solely on
runHealthCheck() or add a boolean flag (e.g., hasRunInitialProvidersHealthCheck)
checked/set by runHealthCheck to prevent double requests.
Comment on lines
+570
to
585
| async runProvidersHealthCheck(options = {}) { | ||
| this.providersHealthLoading = true; | ||
| try { | ||
| const res = await api('providers-health', { | ||
| remote: options.remote !== false | ||
| }); | ||
| if (res && typeof res === 'object' && !res.error) { | ||
| this.providersHealthResult = res; | ||
| } else { | ||
| this.providersHealthResult = null; | ||
| } | ||
| } catch (e) { | ||
| this.providersHealthResult = null; | ||
| } finally { | ||
| this.providersHealthLoading = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Protect providers-health state from stale async responses.
runProvidersHealthCheck can be called concurrently (manual + auto-trigger), and older responses may overwrite newer results. Add a request token/sequence gate before mutating providersHealthResult and providersHealthLoading.
Suggested fix
async runProvidersHealthCheck(options = {}) {
+ const requestToken = Symbol('providers-health');
+ this._providersHealthRequestToken = requestToken;
this.providersHealthLoading = true;
try {
const res = await api('providers-health', {
remote: options.remote !== false
});
- if (res && typeof res === 'object' && !res.error) {
+ if (this._providersHealthRequestToken !== requestToken) {
+ return;
+ }
+ if (res && typeof res === 'object' && !res.error) {
this.providersHealthResult = res;
} else {
this.providersHealthResult = null;
}
} catch (e) {
- this.providersHealthResult = null;
+ if (this._providersHealthRequestToken === requestToken) {
+ this.providersHealthResult = null;
+ }
} finally {
- this.providersHealthLoading = false;
+ if (this._providersHealthRequestToken === requestToken) {
+ this.providersHealthLoading = false;
+ }
}
},📝 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
| async runProvidersHealthCheck(options = {}) { | |
| this.providersHealthLoading = true; | |
| try { | |
| const res = await api('providers-health', { | |
| remote: options.remote !== false | |
| }); | |
| if (res && typeof res === 'object' && !res.error) { | |
| this.providersHealthResult = res; | |
| } else { | |
| this.providersHealthResult = null; | |
| } | |
| } catch (e) { | |
| this.providersHealthResult = null; | |
| } finally { | |
| this.providersHealthLoading = false; | |
| } | |
| async runProvidersHealthCheck(options = {}) { | |
| const requestToken = Symbol('providers-health'); | |
| this._providersHealthRequestToken = requestToken; | |
| this.providersHealthLoading = true; | |
| try { | |
| const res = await api('providers-health', { | |
| remote: options.remote !== false | |
| }); | |
| if (this._providersHealthRequestToken !== requestToken) { | |
| return; | |
| } | |
| if (res && typeof res === 'object' && !res.error) { | |
| this.providersHealthResult = res; | |
| } else { | |
| this.providersHealthResult = null; | |
| } | |
| } catch (e) { | |
| if (this._providersHealthRequestToken === requestToken) { | |
| this.providersHealthResult = null; | |
| } | |
| } finally { | |
| if (this._providersHealthRequestToken === requestToken) { | |
| this.providersHealthLoading = false; | |
| } | |
| } | |
| } |
🤖 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.codex-config.mjs` around lines 570 - 585,
Concurrent calls to runProvidersHealthCheck can let older async responses
overwrite newer state; add a request token/sequence gate: introduce a
per-instance counter (e.g. this.providersHealthRequestSeq) that you increment at
the start of runProvidersHealthCheck, capture the current seq in a local
variable, and only update this.providersHealthResult and
this.providersHealthLoading inside the try/catch/finally if the captured seq
matches this.providersHealthRequestSeq (or store latestToken and compare).
Reference runProvidersHealthCheck, providersHealthResult, and
providersHealthLoading when implementing the sequence check so stale responses
are ignored.
Comment on lines
+125
to
+127
| <span v-if="providersHealthResult" class="doctor-providers-health-summary"> | ||
| {{ providersHealthResult.summary.green }}/{{ providersHealthResult.summary.total }} | ||
| </span> |
Contributor
There was a problem hiding this comment.
Guard summary/providers access to prevent render-time crashes.
The template assumes providersHealthResult.summary and providersHealthResult.providers always exist. If the API returns a partial object, these direct accesses can throw at render time.
Suggested hardening
- <span v-if="providersHealthResult" class="doctor-providers-health-summary">
- {{ providersHealthResult.summary.green }}/{{ providersHealthResult.summary.total }}
+ <span v-if="providersHealthResult" class="doctor-providers-health-summary">
+ {{ providersHealthSummary.green }}/{{ providersHealthSummary.total }}
</span>
@@
- <div v-if="providersHealthResult" class="doctor-providers-grid">
- <div v-for="entry in providersHealthResult.providers"
+ <div v-if="providersHealthResult && Array.isArray(providersHealthResult.providers)" class="doctor-providers-grid">
+ <div v-for="entry in providersHealthResult.providers"
@@
- <div v-if="providersHealthResult && providersHealthResult.providers.some(e => e.issues && e.issues.length)" class="doctor-provider-issues">
- <div v-for="entry in providersHealthResult.providers.filter(e => e.issues && e.issues.length)"
+ <div v-if="providersHealthResult && Array.isArray(providersHealthResult.providers) && providersHealthResult.providers.some(e => e.issues && e.issues.length)" class="doctor-provider-issues">
+ <div v-for="entry in providersHealthResult.providers.filter(e => e.issues && e.issues.length)"Also applies to: 130-131, 143-145
🤖 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-dashboard.html` around lines 125 - 127, The
template accesses providersHealthResult.summary and
providersHealthResult.providers directly which can crash if the API returns a
partial object; update the template to guard these accesses (e.g., change
v-if="providersHealthResult" to v-if="providersHealthResult &&
providersHealthResult.summary" for the summary span and use
v-if="providersHealthResult && providersHealthResult.providers" where providers
are iterated/used, or use optional chaining in bindings like {{
providersHealthResult?.summary?.green }} / {{
providersHealthResult?.summary?.total }}); apply the same guards for the other
occurrences noted (around the elements at 130-131 and 143-145) to prevent
render-time errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Performance
Style
Bug Fixes
Tests
Chores