3.1.2 beta test7#248
Conversation
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds pre-commit tooling for the monorepo, hardens OpenVINO backend install and shutdown behavior, and downscales remote chat image data URIs before persisting them. ChangesPre-commit Hook Infrastructure
OpenVINO Backend Robustness
Remote Image Downscaling to 1MP
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WebUI/src/assets/js/store/homeAgent.ts (1)
1128-1138: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the post-downscale MIME/ext here, not the inbound one.
downscaleDataUriTo1MP()can re-encode non-JPEG/WebP images as PNG, but this code still stamps theFileUIPartwithimg.mimeand derives the filename extension from the original type. That means the persisted bytes and advertised metadata can diverge for formats like GIF/BMP/AVIF. Parse the MIME fromdownscaledUri(or have the helper return it) before settingmediaType/filename.Suggested fix
try { const dataUri = `data:${img.mime};base64,${img.data_base64}` // Match the regular chat path: cap images at 1MP before persisting so // oversized remote images aren't passed full-resolution to the model. const downscaledUri = await downscaleDataUriTo1MP(dataUri) + const mediaType = downscaledUri.match(/^data:([^;,]+)[;,]/)?.[1] ?? img.mime const aipgUrl = await saveImageToMediaInput(downscaledUri) - const ext = img.mime === 'image/jpeg' ? 'jpg' : img.mime.replace('image/', '') + const ext = mediaType === 'image/jpeg' ? 'jpg' : mediaType.replace('image/', '') parts.push({ type: 'file', - mediaType: img.mime, + mediaType, url: aipgUrl, filename: `${sourceLabel}-${Date.now()}-${i}.${ext}`, })🤖 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 `@WebUI/src/assets/js/store/homeAgent.ts` around lines 1128 - 1138, The FileUIPart metadata in homeAgent’s image persistence path is using the inbound img.mime and derived extension even after downscaleDataUriTo1MP() may re-encode the bytes to a different format. Update the logic in this image handling block to derive the final MIME type and filename extension from downscaledUri (or have downscaleDataUriTo1MP return the post-conversion MIME), then use that value for mediaType and filename so the stored metadata matches the persisted content.
🤖 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 @.pre-commit-config.yaml:
- Around line 29-36: The WebUI pre-commit hooks are still invoking bash via the
lint and prettier entries, which breaks environments without Bash. Update the
hook definitions to use npm’s prefix-based invocation instead of the current
bash -c wrapper in the lint:eslint and format entries so WebUI runs directly
from the repo root and remains Windows-compatible.
In `@WebUI/build/scripts/ensure-precommit-hook.mjs`:
- Around line 71-83: The hook selection logic in ensure-precommit-hook.mjs
currently checks only global pre-commit and uvx, which can skip installation on
a fresh clone. Update the runner resolution in the block that sets runner to
first use the repo-managed build/resources/uv.exe (the uv downloaded by setup),
then fall back to PATH-based pre-commit and uvx. Keep the existing
install-failure messaging path unchanged if none of the runners are available.
In `@WebUI/electron/subprocesses/openVINOBackendService.ts`:
- Around line 1563-1567: The Windows cleanup in killStrayOvmsProcessesWindows is
too broad because taskkill /F /IM ovms.exe /T kills every OVMS process on the
machine. Update this method to target only processes tied to this instance by
locating and terminating PIDs or executables under this.ovmsDir, and keep the
cleanup scoped to the current installation. If that is not feasible, remove the
blanket kill and rely on removeDirWithRetry() to report the lock failure
instead.
- Around line 136-139: The version marker lifecycle in openVINOBackendService is
currently anchored to serviceDir, which can leave installed-version.json behind
after ovmsDir is deleted during reinstall. Update the version marker handling in
the setup/reinstall flow and getInstalledVersion() so the marker is tied to the
extracted OVMS directory instead of a persistent serviceDir path, and ensure
cleanup/removal happens whenever ovmsDir is removed so stale releaseTag/version
data cannot be returned through IPC.
- Around line 1701-1706: `terminateProcessTree()` is exiting after the direct
PID terminates, which can bypass the Windows `taskkill /T /F` cleanup and leave
child OVMS processes running. Update the `terminateProcessTree` flow to continue
through the Windows tree-teardown path even after `proc.kill('SIGTERM')`
succeeds, so `taskkill` still runs when needed. Also, `stopAllModelServers()`
currently only warns on failures and hides them; change it to collect and
propagate stop errors so `stop()` can fail or report an unsuccessful stop
instead of always returning `stopped`, using the existing `stoppers`,
`stopAllModelServers`, and `stop()` entry points.
In `@WebUI/src/lib/utils.ts`:
- Around line 378-390: The fallback branches in the canvas downscale flow are
logging directly with console.error, which bypasses the shared WebUI error
pipeline. Update the affected logic in utils.ts around the canvas context and
canvas.toBlob failure paths to either throw/return a typed silent error to the
caller or report through the existing errors store via useErrors/errors.report.
Keep the existing helper behavior but remove bare console.error calls so these
failures are routed consistently through the shared error handling path.
---
Outside diff comments:
In `@WebUI/src/assets/js/store/homeAgent.ts`:
- Around line 1128-1138: The FileUIPart metadata in homeAgent’s image
persistence path is using the inbound img.mime and derived extension even after
downscaleDataUriTo1MP() may re-encode the bytes to a different format. Update
the logic in this image handling block to derive the final MIME type and
filename extension from downscaledUri (or have downscaleDataUriTo1MP return the
post-conversion MIME), then use that value for mediaType and filename so the
stored metadata matches the persisted content.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6858146f-0c0c-4778-b54f-dba92f2cf103
📒 Files selected for processing (7)
.pre-commit-config.yamlWebUI/build/scripts/ensure-precommit-hook.mjsWebUI/electron/subprocesses/openVINOBackendService.tsWebUI/package.jsonWebUI/src/assets/js/store/homeAgent.tsWebUI/src/lib/utils.tsreadme.md
| entry: bash -c 'cd WebUI && npm run lint:eslint' | ||
| files: ^WebUI/.*\.(ts|tsx|mts|cts|js|jsx|mjs|cjs|vue)$ | ||
| pass_filenames: false | ||
| # Formatter (Prettier) | ||
| - id: prettier | ||
| name: prettier (WebUI) | ||
| language: system | ||
| entry: bash -c 'cd WebUI && npm run format' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant config and package scripts.
git ls-files .pre-commit-config.yaml WebUI/package.json README.md readme.md | sed 's#^`#FILE` #'
printf '\n--- .pre-commit-config.yaml ---\n'
cat -n .pre-commit-config.yaml | sed -n '20,45p'
printf '\n--- WebUI/package.json ---\n'
cat -n WebUI/package.json | sed -n '1,220p'
printf '\n--- README/readme mentions Windows ---\n'
rg -n -i 'windows' README.md readme.md || trueRepository: TNG/AI-Playground
Length of output: 7686
Make the WebUI hooks shell-free. bash -c adds an unnecessary Git Bash dependency here; npm --prefix WebUI run lint:eslint and npm --prefix WebUI run format avoid breaking Windows setups that don’t have bash on PATH.
🤖 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 @.pre-commit-config.yaml around lines 29 - 36, The WebUI pre-commit hooks are
still invoking bash via the lint and prettier entries, which breaks environments
without Bash. Update the hook definitions to use npm’s prefix-based invocation
instead of the current bash -c wrapper in the lint:eslint and format entries so
WebUI runs directly from the repo root and remains Windows-compatible.
| // Prefer a `pre-commit` on PATH; otherwise run it through uv's `uvx`. | ||
| let runner | ||
| if (commandAvailable('pre-commit')) { | ||
| runner = ['pre-commit'] | ||
| } else if (commandAvailable('uvx')) { | ||
| runner = ['uvx', 'pre-commit'] | ||
| } else { | ||
| log('`pre-commit` is not installed and `uvx` is unavailable, so the hook was not installed.') | ||
| log('Install it once with either of:') | ||
| log(' uv tool install pre-commit') | ||
| log(' pipx install pre-commit') | ||
| log('then re-run `npm run setup` (or `pre-commit install`).') | ||
| process.exit(0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files WebUI/build/scripts/ensure-precommit-hook.mjs WebUI/package.json WebUI -co --exclude-standard | sed -n '1,200p'Repository: TNG/AI-Playground
Length of output: 7936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ensure-precommit-hook.mjs ==\n'
cat -n WebUI/build/scripts/ensure-precommit-hook.mjs | sed -n '1,220p'
printf '\n== package.json snippets ==\n'
cat -n WebUI/package.json | sed -n '1,260p'Repository: TNG/AI-Playground
Length of output: 9602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ensure-precommit-hook.mjs (1-240) ==\n'
sed -n '1,240p' WebUI/build/scripts/ensure-precommit-hook.mjs
printf '\n== package.json relevant scripts ==\n'
python3 - <<'PY'
import json
from pathlib import Path
p = Path("WebUI/package.json")
data = json.loads(p.read_text())
for k, v in data.get("scripts", {}).items():
if any(tok in k.lower() for tok in ["setup", "hook", "uv", "precommit", "fetch", "install"]):
print(f"{k}: {v}")
PY
printf '\n== search for uv/pre-commit installation wiring ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\buvx?\b|pre-commit|fetch-external-resources|install-hooks|toolchain' WebUI -g '!WebUI/build/scripts/ensure-precommit-hook.mjs'Repository: TNG/AI-Playground
Length of output: 14736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
find WebUI -maxdepth 4 \( -name '*uv*' -o -name '*precommit*' -o -name '*hook*' \) | sed -n '1,200p'Repository: TNG/AI-Playground
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
pkg = json.loads(pathlib.Path("WebUI/package.json").read_text())
print("scripts:")
for k in sorted(pkg.get("scripts", {})):
if any(s in k.lower() for s in ["setup", "hook", "fetch", "uv", "install"]):
print(f" {k}: {pkg['scripts'][k]}")
PY
printf '\n== files mentioning fetch-external-resources or install-hooks ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'fetch-external-resources|install-hooks|ensure-precommit-hook|uvx|pre-commit' WebUIRepository: TNG/AI-Playground
Length of output: 1710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the installer and setup path
sed -n '1,220p' WebUI/build/scripts/ensure-precommit-hook.mjs
printf '\n----\n'
sed -n '1,220p' WebUI/package.jsonRepository: TNG/AI-Playground
Length of output: 7992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fetch-external-resources.mts ==\n'
sed -n '1,260p' WebUI/build/scripts/fetch-external-resources.mts
printf '\n== uv.ts (top section) ==\n'
sed -n '1,120p' WebUI/electron/subprocesses/uvBasedBackends/uv.ts
printf '\n== aipgRoot.ts (resource names section) ==\n'
sed -n '1,80p' WebUI/electron/aipgRoot.tsRepository: TNG/AI-Playground
Length of output: 15762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fetch-external-resources.mts ==\n'
sed -n '1,260p' WebUI/build/scripts/fetch-external-resources.mts
printf '\n== build-paths.mts ==\n'
sed -n '1,260p' WebUI/build/scripts/build-paths.mts
printf '\n== uv.ts top ==\n'
sed -n '1,80p' WebUI/electron/subprocesses/uvBasedBackends/uv.ts
printf '\n== aipgRoot.ts top ==\n'
sed -n '1,80p' WebUI/electron/aipgRoot.tsRepository: TNG/AI-Playground
Length of output: 19937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== uv resource handling in fetch-external-resources.mts ==\n'
rg -n -A4 -B4 'uvx|uv\.exe|resource.*uv|renameSync|extractAllTo|chmodSync|mv |cpSync|copy' WebUI/build/scripts/fetch-external-resources.mts
printf '\n== PATH wiring / environment setup ==\n'
rg -n -A3 -B3 'PATH=|process\.env\.PATH|append.*PATH|prepend.*PATH|build/resources|uvx\.exe|uv\.exe' WebUIRepository: TNG/AI-Playground
Length of output: 14491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fetch script around the UV resource section ==\n'
sed -n '240,420p' WebUI/build/scripts/fetch-external-resources.mts
printf '\n== any PATH edits involving build/resources ==\n'
rg -n -A3 -B3 'process\.env\.PATH|PATH[^\n]*build/resources|build/resources[^\n]*PATH|prepend.*PATH|append.*PATH' WebUIRepository: TNG/AI-Playground
Length of output: 11066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fetch-external-resources.mts (UV section) ==\n'
sed -n '260,420p' WebUI/build/scripts/fetch-external-resources.mts
printf '\n== search for PATH wiring in setup/install scripts ==\n'
rg -n -A3 -B3 'PATH|build/resources|uvx|uv\.exe|ensure-electron|install-hooks' WebUI/build WebUI/electron WebUI/package.jsonRepository: TNG/AI-Playground
Length of output: 47137
Use the repo-managed uv.exe before the PATH-only fallback. Right now setup downloads uv, but this block still only probes global pre-commit/uvx, so hook installation is skipped on a fresh clone unless those tools are already installed. Resolve build/resources/uv.exe first, then fall back to PATH.
🤖 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 `@WebUI/build/scripts/ensure-precommit-hook.mjs` around lines 71 - 83, The hook
selection logic in ensure-precommit-hook.mjs currently checks only global
pre-commit and uvx, which can skip installation on a fresh clone. Update the
runner resolution in the block that sets runner to first use the repo-managed
build/resources/uv.exe (the uv downloaded by setup), then fall back to
PATH-based pre-commit and uvx. Keep the existing install-failure messaging path
unchanged if none of the runners are available.
| // Records exactly which version/releaseTag was installed. Written at the end of | ||
| // set_up(), read by getInstalledVersion(). This is the only reliable source of | ||
| // the installed releaseTag (the `ovms --version` output does not report it). | ||
| this.versionMarkerPath = path.resolve(path.join(this.serviceDir, 'installed-version.json')) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Tie the version marker lifecycle to the extracted OVMS directory.
The marker lives in serviceDir, but reinstall deletes only ovmsDir. If an update fails after the old ovmsDir is removed, the stale marker can still make getInstalledVersion() report the previous version/releaseTag through the downstream IPC path.
Suggested fix
- this.versionMarkerPath = path.resolve(path.join(this.serviceDir, 'installed-version.json'))
+ this.versionMarkerPath = path.resolve(path.join(this.ovmsDir, 'installed-version.json'))Also applies to: 1611-1617
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcess, spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 136 -
139, The version marker lifecycle in openVINOBackendService is currently
anchored to serviceDir, which can leave installed-version.json behind after
ovmsDir is deleted during reinstall. Update the version marker handling in the
setup/reinstall flow and getInstalledVersion() so the marker is tied to the
extracted OVMS directory instead of a persistent serviceDir path, and ensure
cleanup/removal happens whenever ovmsDir is removed so stale releaseTag/version
data cannot be returned through IPC.
| private async killStrayOvmsProcessesWindows(): Promise<void> { | ||
| if (process.platform !== 'win32') return | ||
| try { | ||
| await execAsync('taskkill /F /IM ovms.exe /T') | ||
| this.appLogger.info('Killed stray ovms.exe processes before extraction', this.name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid killing unrelated ovms.exe processes system-wide.
taskkill /F /IM ovms.exe /T terminates every OVMS instance on the machine, including user-managed or another app’s model server. Scope cleanup to PIDs/executables under this.ovmsDir, or let removeDirWithRetry() surface the lock error instead of using a blanket image-name kill.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcess, spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 1563 -
1567, The Windows cleanup in killStrayOvmsProcessesWindows is too broad because
taskkill /F /IM ovms.exe /T kills every OVMS process on the machine. Update this
method to target only processes tied to this instance by locating and
terminating PIDs or executables under this.ovmsDir, and keep the cleanup scoped
to the current installation. If that is not feasible, remove the blanket kill
and rely on removeDirWithRetry() to report the lock failure instead.
| for (const [label, stopFn] of stoppers) { | ||
| try { | ||
| await stopFn() | ||
| } catch (e) { | ||
| this.appLogger.warn(`Failed to stop OVMS ${label} server: ${e}`, this.name) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='WebUI/electron/subprocesses/openVINOBackendService.ts'
echo '== file size =='
wc -l "$file"
echo
echo '== relevant excerpts around stop helpers =='
sed -n '2060,2165p' "$file"
echo
echo '== search for stopAllModelServers and related stop methods =='
rg -n 'stopAllModelServers|waitForExit|taskkill|SIGTERM|SIGKILL|stoppers|not confirmed exited|Force killing OVMS|stopped' "$file"
echo
echo '== outline =='
ast-grep outline "$file" --view expandedRepository: TNG/AI-Playground
Length of output: 13556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='WebUI/electron/subprocesses/openVINOBackendService.ts'
echo '== stopAllModelServers and stop() =='
sed -n '1688,1730p' "$file"
echo
echo '== stop methods for individual OVMS servers =='
sed -n '2136,2595p' "$file" | sed -n '1,470p'Repository: TNG/AI-Playground
Length of output: 16658
Keep the Windows tree teardown on the taskkill path, and surface stop failures.
WebUI/electron/subprocesses/openVINOBackendService.ts:1693-1720, 2100-2132
terminateProcessTree()returns as soon asproc.kill('SIGTERM')ends the direct PID, which can skiptaskkill /T /Fon Windows and leaveovms.exeworkers alive.stopAllModelServers()swallows stop errors, sostop()can still reportstoppedeven if a server did not fully terminate.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcess, spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 1701 -
1706, `terminateProcessTree()` is exiting after the direct PID terminates, which
can bypass the Windows `taskkill /T /F` cleanup and leave child OVMS processes
running. Update the `terminateProcessTree` flow to continue through the Windows
tree-teardown path even after `proc.kill('SIGTERM')` succeeds, so `taskkill`
still runs when needed. Also, `stopAllModelServers()` currently only warns on
failures and hides them; change it to collect and propagate stop errors so
`stop()` can fail or report an unsuccessful stop instead of always returning
`stopped`, using the existing `stoppers`, `stopAllModelServers`, and `stop()`
entry points.
| if (!ctx) { | ||
| console.error('Failed to get canvas context, using original image') | ||
| resolve(null) | ||
| return | ||
| } | ||
|
|
||
| // If image is already <= 1MP, return original file | ||
| if (currentPixels <= MAX_PIXELS) { | ||
| resolve(file) | ||
| return | ||
| } | ||
| ctx.drawImage(img, 0, 0, newWidth, newHeight) | ||
|
|
||
| // Calculate new dimensions maintaining aspect ratio | ||
| const scale = Math.sqrt(MAX_PIXELS / currentPixels) | ||
| const newWidth = Math.round(img.width * scale) | ||
| const newHeight = Math.round(img.height * scale) | ||
| const { mimeType, quality } = resolveDownscaleEncoding(sourceType) | ||
| canvas.toBlob( | ||
| (blob) => { | ||
| if (!blob) { | ||
| console.error('Failed to create blob from canvas, using original image') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Route these failures through the shared error pipeline.
These new fallback branches add bare console.error() calls under WebUI/src, which bypasses the required useErrors/errors.report(...) flow. Please bubble a typed silent error to the caller or inject/report via the errors store instead of logging directly. As per coding guidelines, "All errors must flow through the errors store (useErrors). Report errors via errors.report(err, overrides?) — never use toast.error() directly or rely on bare console.error()."
Also applies to: 451-458
🤖 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 `@WebUI/src/lib/utils.ts` around lines 378 - 390, The fallback branches in the
canvas downscale flow are logging directly with console.error, which bypasses
the shared WebUI error pipeline. Update the affected logic in utils.ts around
the canvas context and canvas.toBlob failure paths to either throw/return a
typed silent error to the caller or report through the existing errors store via
useErrors/errors.report. Keep the existing helper behavior but remove bare
console.error calls so these failures are routed consistently through the shared
error handling path.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WebUI/electron/subprocesses/openVINOBackendService.ts (1)
1281-1284: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
execFilefor the version probe.this.ovmsExePathis passed through a shell here, so install paths with shell metacharacters can change the command.execFile(this.ovmsExePath, ['--version'], { timeout: 5000, env: ... })avoids shell parsing.Proposed direction
- const result = await execAsync(`"${this.ovmsExePath}" --version`, { + const result = await execFileAsync(this.ovmsExePath, ['--version'], { timeout: 5000, env: this.buildOvmsEnv(extraLibPaths), })🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 1281 - 1284, The version probe in the OVMS startup path is invoking `this.ovmsExePath` through a shell via `execAsync`, which can mis-handle install paths containing shell metacharacters. Update the probe in `openVINOBackendService.ts` to use `execFile` with `this.ovmsExePath` and a `['--version']` argument array, keeping the existing timeout and `this.buildOvmsEnv(extraLibPaths)` env options unchanged.Source: Linters/SAST tools
🤖 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 `@WebUI/src/assets/js/store/backendServices.ts`:
- Around line 419-434: The KV-cache sync watcher in backendServices.ts is
swallowing `updateServiceSettings()` failures with only a local console warning,
leaving renderer state and OVMS settings out of sync. Update the
`watch(openvinoKvCacheU4, ...)` callback to route the caught error through
`useErrors().report(...)` instead of `console.warn`, using an appropriate
warn/silent override for the failed OpenVINO KV cache precision sync. Keep the
existing `updateServiceSettings` call and identify the change near the
`openvinoKvCacheU4` watcher and `effectiveOvmsKvCachePrecision` usage.
In `@WebUI/src/components/BackendOptions.vue`:
- Around line 483-488: The new OpenVINO KV cache toggle is being saved in
BackendOptions.vue, but it is not included in the override bookkeeping used by
hasUserOverride and clearOverride. Update the OpenVINO override detection/reset
logic in those paths to treat backendServices.openvinoKvCacheU4 like the other
OpenVINO settings, so enabling it alone shows Clear Override and clearing
overrides also resets this toggle. Refer to the BackendOptions.vue methods/logic
that handle hasUserOverride and clearOverride.
---
Outside diff comments:
In `@WebUI/electron/subprocesses/openVINOBackendService.ts`:
- Around line 1281-1284: The version probe in the OVMS startup path is invoking
`this.ovmsExePath` through a shell via `execAsync`, which can mis-handle install
paths containing shell metacharacters. Update the probe in
`openVINOBackendService.ts` to use `execFile` with `this.ovmsExePath` and a
`['--version']` argument array, keeping the existing timeout and
`this.buildOvmsEnv(extraLibPaths)` env options 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62d0a8f3-184c-4979-8306-2de02313b146
📒 Files selected for processing (5)
WebUI/electron/subprocesses/openVINOBackendService.tsWebUI/src/assets/i18n/en-US.jsonWebUI/src/assets/js/store/backendServices.tsWebUI/src/components/BackendOptions.vueWebUI/src/env.d.ts
✅ Files skipped from review due to trivial changes (1)
- WebUI/src/env.d.ts
| /** Sync the OpenVINO KV cache precision toggle to the main process so a | ||
| * running OVMS service reloads its chat server with the new precision. */ | ||
| watch( | ||
| openvinoKvCacheU4, | ||
| async () => { | ||
| try { | ||
| await updateServiceSettings({ | ||
| serviceName: 'openvino-backend', | ||
| ovmsKvCachePrecision: effectiveOvmsKvCachePrecision.value, | ||
| }) | ||
| } catch (e) { | ||
| console.warn('Failed to sync OpenVINO KV cache precision to main process:', e) | ||
| } | ||
| }, | ||
| { flush: 'post' }, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Report KV-cache sync failures through the errors store.
If updateServiceSettings() fails here, the renderer still keeps openvinoKvCacheU4 enabled and persisted, but the main process never receives the new ovmsKvCachePrecision. That leaves the UI and OVMS runtime out of sync with only a console.warn for diagnostics. Please route this through useErrors().report(...) (likely as a warn/silent app error) instead of swallowing it locally.
As per coding guidelines, "All errors must flow through the errors store (useErrors). Report errors via errors.report(err, overrides?) — never use toast.error() directly or rely on bare console.error()"
🤖 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 `@WebUI/src/assets/js/store/backendServices.ts` around lines 419 - 434, The
KV-cache sync watcher in backendServices.ts is swallowing
`updateServiceSettings()` failures with only a local console warning, leaving
renderer state and OVMS settings out of sync. Update the
`watch(openvinoKvCacheU4, ...)` callback to route the caught error through
`useErrors().report(...)` instead of `console.warn`, using an appropriate
warn/silent override for the failed OpenVINO KV cache precision sync. Keep the
existing `updateServiceSettings` call and identify the change near the
`openvinoKvCacheU4` watcher and `effectiveOvmsKvCachePrecision` usage.
Source: Coding guidelines
| // Save OpenVINO KV cache precision toggle (not part of version override) | ||
| if (props.backend === 'openvino-backend' && 'ovmsKvCacheU4' in values) { | ||
| backendServices.openvinoKvCacheU4 = !!(values as { ovmsKvCacheU4?: boolean }) | ||
| .ovmsKvCacheU4 | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the new OpenVINO toggle in the reset path.
This persists openvinoKvCacheU4, but hasUserOverride and clearOverride still ignore it. After enabling the checkbox, "Clear Override" will not appear for that change alone, and it will not reset the toggle when clearing other OpenVINO overrides.
Suggested follow-up
const hasUserOverride = computed(
() =>
!!backendServices.versionState[props.backend].uiOverride ||
(props.backend === 'comfyui-backend' && backendServices.comfyUiParameters !== null) ||
- (props.backend === 'llamacpp-backend' && backendServices.llamaCppParameters !== null),
+ (props.backend === 'llamacpp-backend' && backendServices.llamaCppParameters !== null) ||
+ (props.backend === 'openvino-backend' && backendServices.openvinoKvCacheU4),
)
const clearOverride = () => {
backendServices.versionState[props.backend].uiOverride = undefined
if (props.backend === 'comfyui-backend') {
backendServices.comfyUiParameters = null
}
if (props.backend === 'llamacpp-backend') {
backendServices.llamaCppParameters = null
}
+ if (props.backend === 'openvino-backend') {
+ backendServices.openvinoKvCacheU4 = false
+ }
settingsDialogOpen.value = false
menuOpen.value = 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 `@WebUI/src/components/BackendOptions.vue` around lines 483 - 488, The new
OpenVINO KV cache toggle is being saved in BackendOptions.vue, but it is not
included in the override bookkeeping used by hasUserOverride and clearOverride.
Update the OpenVINO override detection/reset logic in those paths to treat
backendServices.openvinoKvCacheU4 like the other OpenVINO settings, so enabling
it alone shows Clear Override and clearing overrides also resets this toggle.
Refer to the BackendOptions.vue methods/logic that handle hasUserOverride and
clearOverride.
2 participants
Description:
Please provide a brief description of the changes made in this pull request.
Related Issue:
If this pull request is related to any existing issue, please mention the issue number here.
Changes Made:
Please list down the specific changes made in this pull request.
Testing Done:
Please describe the testing that has been done to ensure the changes made in this pull request are functioning as expected.
Screenshots:
If applicable, please provide screenshots or GIFs or videos to visually demonstrate the changes made.
Checklist:
Summary by CodeRabbit