Quality hardening: real bug fixes + SOLID/DRY refactors + honest coverage + tooling spine#46
Open
alichherawalla wants to merge 336 commits into
Open
Quality hardening: real bug fixes + SOLID/DRY refactors + honest coverage + tooling spine#46alichherawalla wants to merge 336 commits into
alichherawalla wants to merge 336 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 563 files, which is 463 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (22)
📒 Files selected for processing (598)
You can disable this status message by setting the ✨ 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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
PR Summary by QodoQuality hardening: bug fixes, pure-logic refactors, coverage gates, and tooling
AI Description
Diagram
High-Level Assessment
Files changed (66)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipc.ts (1)
170-178: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNotification broadcast unguarded against destroyed
webContents.Every other
sender.send/webContents.sendcall in this file wraps the send in a try/catch (/* window gone */), but this newnotification:new-memorybroadcast doesn't. If a window closes betweengetAllWindows()andsend(), this throws insideinsertMemoryRecordand propagates to its caller.🛡️ Guard the broadcast like the rest of the file
if (memoryId) { BrowserWindow.getAllWindows().forEach(win => { - win.webContents.send('notification:new-memory', { - sessionId: params.sessionId || null, - memoryContent: content.slice(0, 100) + (content.length > 100 ? '...' : '') - }); + try { + win.webContents.send('notification:new-memory', { + sessionId: params.sessionId || null, + memoryContent: content.slice(0, 100) + (content.length > 100 ? '...' : '') + }); + } catch { /* window gone */ } }); }🤖 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 `@src/main/ipc.ts` around lines 170 - 178, Guard the notification:new-memory broadcast in insertMemoryRecord with a per-window try/catch around win.webContents.send, matching the existing “window gone” handling used by other sender.send/webContents.send calls so a destroyed window cannot propagate an exception.src/main/files.ts (1)
21-73: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRoute via
classifyUpload()instead of duplicating its branch logic.
files-classify.tscentralizes the ext→kind mapping inclassifyUpload(), butprocessUploadstill re-implements the same if/else chain against the raw extension arrays (Lines 27, 38, 42, 56, 70) instead of calling the extracted function. This is exactly the duplicated "routing rule" the coding guideline for**/*.{ts,tsx}warns against — two independent decision paths for the same mapping can drift.♻️ Suggested refactor
-import { IMAGE_EXT, AUDIO_EXT, VIDEO_EXT, sanitizeUploadName } from './files-classify'; +import { classifyUpload, sanitizeUploadName } from './files-classify'; export async function processUpload(name: string, bytes: ArrayBuffer | Uint8Array): Promise<ProcessedFile> { - const ext = path.extname(name).slice(1).toLowerCase(); + const kind = classifyUpload(name); const safe = sanitizeUploadName(name); const tmp = path.join(os.tmpdir(), `offgrid-upload-${Date.now()}-${safe}`); await fs.promises.writeFile(tmp, Buffer.from(bytes as ArrayBuffer)); try { - if (IMAGE_EXT.includes(ext)) { + if (kind === 'image') { ... } - if (AUDIO_EXT.includes(ext)) { + if (kind === 'audio') { ... } - if (VIDEO_EXT.includes(ext)) { + if (kind === 'video') { ... } - if (ext === 'pdf') { + if (kind === 'pdf') { ... } - if (ext === 'docx') return { ... }; + if (kind === 'docx') return { ... }; ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/files.ts` around lines 21 - 73, Refactor processUpload to call classifyUpload() for extension-to-kind routing instead of directly checking IMAGE_EXT, AUDIO_EXT, VIDEO_EXT, and individual extensions. Use the returned kind to dispatch the existing image, audio, video, PDF, DOCX, and text processing while preserving their current behavior and cleanup handling; remove the duplicated branch conditions and imports no longer needed.Source: Coding guidelines
🧹 Nitpick comments (8)
src/main/skills-parse.ts (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace literal BOM character in regex with an explicit escape.
The regex embeds a literal BOM (
) character directly in source, which is invisible in most editors/diffs and trips ESLint'sno-irregular-whitespacerule.🔧 Proposed fix
- const fm = /^?---\s*\n([\s\S]*?)\n---\s*\n?([\s\S]*)$/.exec(md); + const fm = /^\uFEFF?---\s*\n([\s\S]*?)\n---\s*\n?([\s\S]*)$/.exec(md);🤖 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 `@src/main/skills-parse.ts` at line 57, Replace the literal BOM character in the frontmatter regex used by the skills parser with the explicit Unicode escape \uFEFF, preserving the optional BOM-matching behavior and avoiding irregular whitespace lint errors.Source: Linters/SAST tools
src/main/licensing/keygen-client.ts (1)
64-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider typing the JSON:API response shapes instead of
any.
toLicense,parseValidateResult,parseActivateResult, andparseMachinesare now exported/reusable parsing helpers but all acceptanyforbody/data, flagged by ESLint (no-explicit-any) at Lines 64, 75, 88, 90, 102, 103. A minimal JSON:API shape type would preserve the parsing flexibility while catching response-shape drift at compile time.🤖 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 `@src/main/licensing/keygen-client.ts` around lines 64 - 112, Replace the explicit any parameters and local arrays in toLicense, parseValidateResult, parseActivateResult, and parseMachines with reusable minimal JSON:API response types, allowing optional attributes, meta, data, and errors fields while preserving current null-safe parsing behavior. Type nested license, machine, validation metadata, and error shapes sufficiently to access the existing properties without disabling no-explicit-any.Source: Linters/SAST tools
src/main/modality-queue/queue.ts (1)
42-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShallow freeze leaves
evictsarray mutable.
Object.freezeonly freezes the top-level object;evictsis a separate array reference that can still be mutated in place (push/pop/sort), silently corrupting the shared singleton descriptor for every futurerun()call. The comment's claim that "a caller can't mutate the shared descriptor" doesn't fully hold.🔒 Deep-freeze the evicts array
-export const CHAT_JOB: QueueRequest = Object.freeze({ tier: 2, label: 'chat', evicts: ['image'] }); -export const IMAGE_JOB: QueueRequest = Object.freeze({ tier: 2, label: 'image', evicts: ['llm'] }); +export const CHAT_JOB: QueueRequest = Object.freeze({ tier: 2, label: 'chat', evicts: Object.freeze(['image']) }); +export const IMAGE_JOB: QueueRequest = Object.freeze({ tier: 2, label: 'image', evicts: Object.freeze(['llm']) });🤖 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 `@src/main/modality-queue/queue.ts` around lines 42 - 43, Deep-freeze the nested evicts arrays in the shared CHAT_JOB and IMAGE_JOB descriptors by freezing each array before freezing its containing object, preserving immutable singleton state for all future run() calls.src/main/ipc.ts (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the dead
_newSummaryparameter.updateMasterMemoryIncrementalnever reads it, so either remove it and update the call site or keep it only as a deliberate compatibility placeholder.🤖 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 `@src/main/ipc.ts` around lines 12 - 16, Remove the unused _newSummary parameter from updateMasterMemoryIncremental and update every call site to invoke it without arguments; alternatively, retain it only if compatibility requires it and document that intentional placeholder.Source: Linters/SAST tools
src/main/tools-parsers.ts (1)
22-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
htmlToTextmisses self-closing<br>/<br/>line breaks.The block-close regex only matches
</br>; real-world HTML almost always uses<br>or<br/>, so those breaks are lost (collapsed into surrounding text) while</p>,</div>, etc. still work.♻️ Proposed fix to also match self-closing br
- .replace(/<\/(p|div|li|h[1-6]|br|tr|section|article)>/gi, '\n'); + .replace(/<\/(p|div|li|h[1-6]|tr|section|article)>/gi, '\n') + .replace(/<br\s*\/?>/gi, '\n');🤖 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 `@src/main/tools-parsers.ts` around lines 22 - 28, Update htmlToText so its tag-to-newline replacement recognizes opening and self-closing br elements, including <br>, <br/>, and whitespace variants, while preserving the existing closing-tag handling for paragraphs and other block elements.src/main/files-classify.ts (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider freezing the exported extension arrays.
These arrays are now the single source of truth consumed by
files.ts,rag-ipc.ts, and tests. As plain mutable arrays, an accidental.push()/.sort()from any importer would silently change classification everywhere they're shared.🛡️ Optional hardening
-export const IMAGE_EXT = ['png', 'jpg', 'jpeg', 'webp', 'gif', 'bmp', 'heic']; -export const AUDIO_EXT = ['mp3', 'wav', 'm4a', 'aac', 'ogg', 'oga', 'opus', 'flac', 'aiff', 'aif']; -export const VIDEO_EXT = ['mp4', 'mov', 'webm', 'mkv', 'avi', 'm4v']; +export const IMAGE_EXT = ['png', 'jpg', 'jpeg', 'webp', 'gif', 'bmp', 'heic'] as const; +export const AUDIO_EXT = ['mp3', 'wav', 'm4a', 'aac', 'ogg', 'oga', 'opus', 'flac', 'aiff', 'aif'] as const; +export const VIDEO_EXT = ['mp4', 'mov', 'webm', 'mkv', 'avi', 'm4v'] as const;🤖 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 `@src/main/files-classify.ts` around lines 7 - 13, Freeze the exported extension arrays IMAGE_EXT, AUDIO_EXT, VIDEO_EXT, and DOC_EXT to prevent importers from mutating shared classification data. Apply Object.freeze (with suitable readonly typing if needed) while preserving their existing values and usages in files.ts, rag-ipc.ts, and tests.src/main/__tests__/prompts.test.ts (1)
17-20: 📐 Maintainability & Code Quality | 🔵 TrivialConsider a real temp-DB collaborator instead of mocking
../databasewholesale.
getSetting/deleteSettingare stubbed out entirely rather than exercised against a real (temporary) SQLite instance, even though the module reads/writes via a DB that can run in-process. This is a defensible boundary call given the file's scope is the pure template engine, but it diverges from the general test-writing preference.As per coding guidelines, "Prefer integration tests with real collaborators such as temporary SQLite databases, temporary user-data directories, real cryptography, or real WASM; mock only boundaries that cannot run in-process."
🤖 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 `@src/main/__tests__/prompts.test.ts` around lines 17 - 20, Replace the wholesale ../database mock in prompts.test.ts with a real temporary SQLite database collaborator, configuring test setup and teardown to create and clean up an isolated database. Exercise getSetting and deleteSetting against that database while preserving the template-engine test behavior, and mock only dependencies that cannot run in-process.Source: Coding guidelines
src/main/bootstrap/__tests__/proEnabled.test.ts (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unnecessary type cast for
vi.stubEnv.Vitest's
stubEnvsignature natively acceptsstring | undefinedfor non-mode env vars, soundefined as unknown as stringis unneeded (and misleading — it lies about the runtime type). Passingundefineddirectly is correct.🧹 Proposed simplification
- vi.stubEnv('OFFGRID_PRO', undefined as unknown as string); + vi.stubEnv('OFFGRID_PRO', undefined);Also applies to: 61-61
🤖 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 `@src/main/bootstrap/__tests__/proEnabled.test.ts` at line 54, Remove the unnecessary `as unknown as string` cast from the `vi.stubEnv` calls in the `proEnabled` tests, including both occurrences, and pass `undefined` directly.
🤖 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 `@docs/GAPS_BACKLOG.md`:
- Around line 8-12: Replace every em dash in docs/GAPS_BACKLOG.md with approved
punctuation such as a comma, colon, semicolon, or parentheses, preserving the
original meaning across the OPEN and RESOLVED sections and all referenced
passages. Search the entire document to ensure no em dash characters remain.
In `@src/main/licensing/__tests__/device-fingerprint.test.ts`:
- Around line 16-18: Address the shared TypeScript lint violations in
device-fingerprint.test.ts: explicitly annotate freshModule and setPlatform
return types, and handle the intentionally unused _key parameter in the electron
mock via a test-file lint override or an appropriate unused-parameter
suppression.
In `@src/main/llm/__tests__/kv-launch-roundtrip.test.ts`:
- Around line 64-74: Add explicit return types to the `freshService` and
`applySettings` helper functions to satisfy
`@typescript-eslint/explicit-function-return-type`; use the inferred promise
types, with `freshService` returning `Promise<LLMService>` and `applySettings`
returning `Promise<void>`.
In `@src/main/llm/stream.ts`:
- Around line 40-118: Remove the abort event listener registered in the stream
request by tracking the handler and centralizing cleanup in the terminal paths.
Update the logic around opts.signal, the timeout callback, non-200 response
handling, response end, request error, and abort callback so cleanup is
performed exactly once before resolving or rejecting, preventing reused signals
from accumulating listeners.
In `@src/main/mcp-parse-data-url.ts`:
- Around line 14-23: Update parseDataUrl() to validate that url.indexOf(',')
returns a valid delimiter before extracting metadata or decoding the payload;
when comma === -1, reject the malformed data URL explicitly. Preserve decoding
behavior for valid URLs while preventing truncated inputs from reaching
decodeURIComponent().
In `@src/main/models/gguf.ts`:
- Around line 40-52: Ensure the file descriptor opened in isValidGgufFile is
always closed, including when readSync throws. Wrap the readSync and header
validation logic in a try/finally block and call closeSync(fd) in the finally
clause, while preserving the existing false return for caught errors.
In `@src/renderer/src/components/ChatDetail.tsx`:
- Line 62: Add an explicit string return type to both inline formatTime
functions in MemoryCardFull and EntityCardFull, updating their signatures to
satisfy the explicit-function-return-type ESLint rule.
In `@src/renderer/src/components/ChatList.tsx`:
- Line 68: Replace the text-green-500 class on the link in ChatList with the
mandated emerald accent styling, using the appropriate dark/light emerald values
(`#34D399` and `#059669`) via the project’s existing theme or Tailwind classes.
- Around line 316-320: Update formatTime to avoid fixed-position slicing of
toLocaleString(), which is locale-dependent. Using the parsed date from
parseSqliteUtc, format the date and time with explicit Intl.DateTimeFormat
options or a dedicated locale-aware formatter, preserving the intended displayed
components across locales.
In `@src/renderer/src/components/MemoryChat.tsx`:
- Around line 811-816: Reuse the existing agenticActive capability check
throughout MemoryChat’s turn-routing logic. Replace the later inline `(toolsOn
|| connectorsOn) && !activeProjectId` condition guarding the tools/connectors
branch with agenticActive, keeping the variable as the single source of truth
for both shouldAutoRouteImage and the agentic path.
- Around line 551-554: Update chooseImageModel to handle rejected promises from
window.api.setActiveModalModel, matching the error-handling pattern used by
other fire-and-forget IPC calls in MemoryChat.tsx (for example, attach a catch
handler). Preserve the optimistic setImgModel update while ensuring failures are
not silently unhandled.
- Around line 911-920: Thread the captured streamed reasoning through every
cancellation/partial-answer persistence path. In the cancel branches that call
addRagMessage with toolCtx or result.context, build or augment the persisted
context using the captured toolReasoning, matching the finalized path’s
buildAssistantContext(toolCtx, { reasoning: toolReasoning }) behavior so stopped
turns retain their reasoning block.
---
Outside diff comments:
In `@src/main/files.ts`:
- Around line 21-73: Refactor processUpload to call classifyUpload() for
extension-to-kind routing instead of directly checking IMAGE_EXT, AUDIO_EXT,
VIDEO_EXT, and individual extensions. Use the returned kind to dispatch the
existing image, audio, video, PDF, DOCX, and text processing while preserving
their current behavior and cleanup handling; remove the duplicated branch
conditions and imports no longer needed.
In `@src/main/ipc.ts`:
- Around line 170-178: Guard the notification:new-memory broadcast in
insertMemoryRecord with a per-window try/catch around win.webContents.send,
matching the existing “window gone” handling used by other
sender.send/webContents.send calls so a destroyed window cannot propagate an
exception.
---
Nitpick comments:
In `@src/main/__tests__/prompts.test.ts`:
- Around line 17-20: Replace the wholesale ../database mock in prompts.test.ts
with a real temporary SQLite database collaborator, configuring test setup and
teardown to create and clean up an isolated database. Exercise getSetting and
deleteSetting against that database while preserving the template-engine test
behavior, and mock only dependencies that cannot run in-process.
In `@src/main/bootstrap/__tests__/proEnabled.test.ts`:
- Line 54: Remove the unnecessary `as unknown as string` cast from the
`vi.stubEnv` calls in the `proEnabled` tests, including both occurrences, and
pass `undefined` directly.
In `@src/main/files-classify.ts`:
- Around line 7-13: Freeze the exported extension arrays IMAGE_EXT, AUDIO_EXT,
VIDEO_EXT, and DOC_EXT to prevent importers from mutating shared classification
data. Apply Object.freeze (with suitable readonly typing if needed) while
preserving their existing values and usages in files.ts, rag-ipc.ts, and tests.
In `@src/main/ipc.ts`:
- Around line 12-16: Remove the unused _newSummary parameter from
updateMasterMemoryIncremental and update every call site to invoke it without
arguments; alternatively, retain it only if compatibility requires it and
document that intentional placeholder.
In `@src/main/licensing/keygen-client.ts`:
- Around line 64-112: Replace the explicit any parameters and local arrays in
toLicense, parseValidateResult, parseActivateResult, and parseMachines with
reusable minimal JSON:API response types, allowing optional attributes, meta,
data, and errors fields while preserving current null-safe parsing behavior.
Type nested license, machine, validation metadata, and error shapes sufficiently
to access the existing properties without disabling no-explicit-any.
In `@src/main/modality-queue/queue.ts`:
- Around line 42-43: Deep-freeze the nested evicts arrays in the shared CHAT_JOB
and IMAGE_JOB descriptors by freezing each array before freezing its containing
object, preserving immutable singleton state for all future run() calls.
In `@src/main/skills-parse.ts`:
- Line 57: Replace the literal BOM character in the frontmatter regex used by
the skills parser with the explicit Unicode escape \uFEFF, preserving the
optional BOM-matching behavior and avoiding irregular whitespace lint errors.
In `@src/main/tools-parsers.ts`:
- Around line 22-28: Update htmlToText so its tag-to-newline replacement
recognizes opening and self-closing br elements, including <br>, <br/>, and
whitespace variants, while preserving the existing closing-tag handling for
paragraphs and other block elements.
🪄 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 Plus
Run ID: d9dbb58c-155f-4d0a-8f8d-ef63e2a1af57
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (138)
.dependency-cruiser.cjs.github/workflows/ci.ymlCLAUDE.mdREADME.mddocs/CODE_AUDIT.mddocs/GAPS_BACKLOG.mdeslint.config.mjsknip.jsonpackage.jsonsonar-project.propertiessrc/components/ui/focus-cards.tsxsrc/hooks/use-outside-click.tsxsrc/main/__tests__/files-classify.test.tssrc/main/__tests__/ipc-query-logic.test.tssrc/main/__tests__/mcp-parse-data-url.test.tssrc/main/__tests__/mime.test.tssrc/main/__tests__/prompts.test.tssrc/main/__tests__/reasoning-persistence.dbtest.tssrc/main/__tests__/search-ranking.test.tssrc/main/__tests__/skills-parse.test.tssrc/main/__tests__/tools-parsers.test.tssrc/main/__tests__/tools-stream.test.tssrc/main/__tests__/tts-logic.test.tssrc/main/__tests__/vectors-predicates.test.tssrc/main/bootstrap/__tests__/hookRegistry.test.tssrc/main/bootstrap/__tests__/proEnabled.test.tssrc/main/files-classify.tssrc/main/files.tssrc/main/imagegen.tssrc/main/imagegen/__tests__/model-filter.test.tssrc/main/imagegen/model-filter.tssrc/main/index.tssrc/main/ipc-query-logic.tssrc/main/ipc.tssrc/main/licensing/__tests__/device-fingerprint.test.tssrc/main/licensing/__tests__/keygen-parse.test.tssrc/main/licensing/__tests__/license-logic.test.tssrc/main/licensing/keygen-client.tssrc/main/licensing/license-service.tssrc/main/llm.tssrc/main/llm/__tests__/kv-launch-roundtrip.test.tssrc/main/llm/__tests__/settings-merge.test.tssrc/main/llm/__tests__/stream.test.tssrc/main/llm/settings-math.tssrc/main/llm/stream.tssrc/main/mcp-parse-data-url.tssrc/main/mcp-server.tssrc/main/media-server.tssrc/main/mime.tssrc/main/modality-queue/__tests__/jobs.test.tssrc/main/modality-queue/queue.tssrc/main/model-server/__tests__/data-url.test.tssrc/main/model-server/data-url.tssrc/main/models-manager.tssrc/main/models/__tests__/gguf.test.tssrc/main/models/gguf.tssrc/main/rag-ipc.tssrc/main/rag/__tests__/rag-prompt.test.tssrc/main/rag/index.tssrc/main/rag/prompt.tssrc/main/search-ranking.tssrc/main/search.tssrc/main/skills-parse.tssrc/main/skills.tssrc/main/tools-parsers.tssrc/main/tools.tssrc/main/tools/__tests__/mcpConnectorToolExtension.test.tssrc/main/tools/mcpConnectorToolExtension.tssrc/main/transcription/__tests__/ffmpeg-decode.test.tssrc/main/transcription/ffmpeg-decode.tssrc/main/transcription/parakeet-cli.tssrc/main/transcription/whisper-cli.tssrc/main/transcription/whisper-server.tssrc/main/tts-logic.tssrc/main/tts.tssrc/main/vectors-predicates.tssrc/main/vectors.tssrc/preload/index.tssrc/renderer/src/__tests__/theme.test.tssrc/renderer/src/components/ArtifactCanvas.tsxsrc/renderer/src/components/ChatDetail.tsxsrc/renderer/src/components/ChatList.tsxsrc/renderer/src/components/ConnectorsScreen.tsxsrc/renderer/src/components/MemoryChat.tsxsrc/renderer/src/components/ModelsScreen.tsxsrc/renderer/src/components/ProjectsScreen.tsxsrc/renderer/src/components/Settings.tsxsrc/renderer/src/components/__tests__/MemoryChat.image.test.tsxsrc/renderer/src/components/__tests__/chatlist-brand.test.tssrc/renderer/src/components/connectorCatalog.tssrc/renderer/src/components/setup/StoragePanel.tsxsrc/renderer/src/components/ui/accordion.tsxsrc/renderer/src/components/ui/background-beams-with-collision.tsxsrc/renderer/src/components/ui/background-gradient-animation.tsxsrc/renderer/src/components/ui/badge.tsxsrc/renderer/src/components/ui/bento-grid.tsxsrc/renderer/src/components/ui/canvas-reveal-effect.tsxsrc/renderer/src/components/ui/card-spotlight.tsxsrc/renderer/src/components/ui/colourful-text.tsxsrc/renderer/src/components/ui/comet-card.tsxsrc/renderer/src/components/ui/flip-words.tsxsrc/renderer/src/components/ui/focus-cards.tsxsrc/renderer/src/components/ui/glare-card.tsxsrc/renderer/src/components/ui/glowing-effect.tsxsrc/renderer/src/components/ui/glowing-stars.tsxsrc/renderer/src/components/ui/hero-highlight.tsxsrc/renderer/src/components/ui/hover-border-gradient.tsxsrc/renderer/src/components/ui/scroll-area.tsxsrc/renderer/src/components/ui/select.tsxsrc/renderer/src/components/ui/separator.tsxsrc/renderer/src/components/ui/sheet.tsxsrc/renderer/src/components/ui/shooting-stars.tsxsrc/renderer/src/components/ui/sparkles.tsxsrc/renderer/src/components/ui/spotlight.tsxsrc/renderer/src/components/ui/stars-background.tsxsrc/renderer/src/components/ui/tabs.tsxsrc/renderer/src/components/ui/text-generate-effect.tsxsrc/renderer/src/components/ui/textarea.tsxsrc/renderer/src/components/ui/wobble-card.tsxsrc/renderer/src/lib/__tests__/artifact-labels.test.tssrc/renderer/src/lib/__tests__/connectorCatalog.test.tssrc/renderer/src/lib/__tests__/image-intent.test.tssrc/renderer/src/lib/__tests__/image-params-wiring.test.tssrc/renderer/src/lib/__tests__/image-params.test.tssrc/renderer/src/lib/__tests__/message-persistence.test.tssrc/renderer/src/lib/__tests__/model-kind-labels.test.tssrc/renderer/src/lib/__tests__/session-id.test.tssrc/renderer/src/lib/__tests__/time.test.tssrc/renderer/src/lib/artifact-labels.tssrc/renderer/src/lib/image-intent.tssrc/renderer/src/lib/image-params.tssrc/renderer/src/lib/message-persistence.tssrc/renderer/src/lib/model-kind-labels.tssrc/renderer/src/lib/session-id.tssrc/renderer/src/lib/time.tstsconfig.node.jsontsconfig.web.jsonvitest.config.ts
💤 Files with no reviewable changes (30)
- src/hooks/use-outside-click.tsx
- src/renderer/src/components/ui/wobble-card.tsx
- src/renderer/src/components/ui/scroll-area.tsx
- src/renderer/src/components/ui/accordion.tsx
- src/renderer/src/components/ui/comet-card.tsx
- src/renderer/src/components/ui/colourful-text.tsx
- src/renderer/src/components/ui/textarea.tsx
- src/renderer/src/components/ui/bento-grid.tsx
- src/renderer/src/components/ui/glare-card.tsx
- src/renderer/src/components/ui/hover-border-gradient.tsx
- src/renderer/src/components/ui/separator.tsx
- src/components/ui/focus-cards.tsx
- src/renderer/src/components/ui/stars-background.tsx
- src/renderer/src/components/ui/flip-words.tsx
- src/renderer/src/components/ui/text-generate-effect.tsx
- src/renderer/src/components/ui/sheet.tsx
- src/renderer/src/components/ui/shooting-stars.tsx
- src/renderer/src/components/ui/sparkles.tsx
- src/renderer/src/components/ui/background-gradient-animation.tsx
- src/renderer/src/components/ui/badge.tsx
- src/renderer/src/components/ui/focus-cards.tsx
- src/renderer/src/components/ui/background-beams-with-collision.tsx
- src/renderer/src/components/ui/spotlight.tsx
- src/renderer/src/components/ui/tabs.tsx
- src/renderer/src/components/ui/hero-highlight.tsx
- src/renderer/src/components/ui/card-spotlight.tsx
- src/renderer/src/components/ui/glowing-stars.tsx
- src/renderer/src/components/ui/glowing-effect.tsx
- src/renderer/src/components/ui/canvas-reveal-effect.tsx
- src/renderer/src/components/ui/select.tsx
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)
.github/workflows/ci.yml (1)
37-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPin the fallback checkout to
main. The fallback step omitsref, so it tracks the repository’s default branch instead of the intendedmain. Addref: mainto keep CI behavior stable.🔧 Proposed fix
with: repository: off-grid-ai/desktop-pro token: ${{ secrets.CI_CROSS_REPO_TOKEN }} path: pro + ref: main🤖 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 @.github/workflows/ci.yml around lines 37 - 41, Update the fallback checkout step using actions/checkout@v4 to include ref: main alongside repository, token, and path, ensuring it always checks out the intended branch rather than the repository default.
🤖 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 @.github/workflows/ci.yml:
- Around line 34-37: Remove continue-on-error from the fallback
actions/checkout@v4 step named “Fall back to pro main if no matching branch,” or
explicitly fail when the pro checkout is unavailable, so failures in the pro
typecheck/coverage path cannot be silently skipped.
- Around line 25-33: Add persist-credentials: false to both actions/checkout
steps for desktop-pro, including the checkout identified by id pro_branch, so
the CI token is not retained in local git configuration.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 37-41: Update the fallback checkout step using actions/checkout@v4
to include ref: main alongside repository, token, and path, ensuring it always
checks out the intended branch rather than the repository default.
🪄 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 Plus
Run ID: 8e6c962d-ded6-461e-8ce3-9f18794fe010
📒 Files selected for processing (1)
.github/workflows/ci.yml
alichherawalla
pushed a commit
that referenced
this pull request
Jul 10, 2026
…ss + FD leak From the PR #46 bot reviews, the confirmed real bugs (several in this PR's own mislabel/drift class): - MIME map omitted bmp/heic (Gitar + Qodo): files-classify's IMAGE_EXT accepts them, so a .bmp/.heic upload was mislabelled via the image/png fallback. Added both; a test now asserts EVERY IMAGE_EXT resolves to a real image/* MIME (imported from the router — test-side DRY — so the map can't drift from the accept-list again). - imageMime (llm/chat-payload, the RAG-chat vision path via decodeImages) still did the old `endsWith('.png') ? png : jpeg` — the exact webp mislabel I fixed in tools.ts but missed here (Qodo). Routed through the shared mimeForExt (image/png fallback); webp→image/webp, gif→image/gif. - gguf.isValidGgufFile FD leak (CodeRabbit, Major): closeSync only ran on success; a readSync throw leaked the descriptor. Wrapped in try/finally. - MemoryChat.chooseImageModel write-through swallowed a rejected setActiveModalModel (CodeRabbit): added .catch+log so a failed persist can't silently re-diverge the composer from the Active-models panel. tsc x3 clean; full suite 2113 pass; coverage 97/92/96/98. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
alichherawalla
pushed a commit
that referenced
this pull request
Jul 10, 2026
…aram updaters More PR #46 review findings (MemoryChat), all in this PR's own drift/correctness class: - Reasoning captured via a setState-updater SIDE EFFECT (Gitar Bug + CodeRabbit): both persist sites read `m.reasoning` out of the setConvMessages updater then used it on the next line. React only runs an updater eagerly on a bail-out, else defers it to render → the read could be undefined → the persisted Thinking block vanished on reload (the exact T1f bug it was meant to fix). Now mirrored into a reasoningByStream ref as it streams (written synchronously in onRagStream) and read deterministically at persist; entry freed after use so the map can't grow unbounded. - agenticActive (Major DRY): the `(toolsOn||connectorsOn)&&!activeProjectId` capability was computed into agenticActive then recomputed inline at the tools/connectors gate. Reuse the variable — one definition. - saveSetting inside the setImgParamStore updater (Quality): updaters must be pure (StrictMode double-invokes → double IPC save). Moved to one effect keyed on the store, gated on prefsLoaded. tsc x3 clean; MemoryChat render test green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/MemoryChat.tsx (1)
463-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew code omits braces on single-line
ifstatements.The functional logic here (
refreshImageModel, theresolveImageParamseffect,setStepsOverride/setSizeOverride) is correct, but the new single-line guards at Lines 472, 488, 541, 564, and 573 (e.g.if (!s) return;,if (!imgModel) return;) omit braces, which conflicts with the plannedcurly: allhygiene rule for this file type.As per coding guidelines,
**/*.{js,ts,tsx}: "The planned ESLint hygiene rules arecurly: all... introduce limits with a ratchet and never lower them."🧹 Example fix
- const s = await window.api.imageGenStatus?.(); - if (!s) return; + const s = await window.api.imageGenStatus?.(); + if (!s) { return; }Also applies to: 533-578
🤖 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 `@src/renderer/src/components/MemoryChat.tsx` around lines 463 - 491, Wrap every single-line if guard in the affected logic with braces to satisfy the planned curly: all rule, including guards in refreshImageModel, the resolveImageParams effect, and setStepsOverride/setSizeOverride. Update conditions such as “if (!s) return;” and “if (!imgModel) return;” while preserving their existing control flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/renderer/src/components/MemoryChat.tsx`:
- Around line 463-491: Wrap every single-line if guard in the affected logic
with braces to satisfy the planned curly: all rule, including guards in
refreshImageModel, the resolveImageParams effect, and
setStepsOverride/setSizeOverride. Update conditions such as “if (!s) return;”
and “if (!imgModel) return;” while preserving their existing control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e39bbe29-7860-464a-9a5f-844e8e44106c
📒 Files selected for processing (7)
src/main/__tests__/mime.test.tssrc/main/llm/__tests__/chat-payload.test.tssrc/main/llm/chat-payload.tssrc/main/mime.tssrc/main/model-server/__tests__/data-url.test.tssrc/main/models/gguf.tssrc/renderer/src/components/MemoryChat.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/mime.ts
- src/main/tests/mime.test.ts
- src/main/models/gguf.ts
alichherawalla
pushed a commit
that referenced
this pull request
Jul 10, 2026
… time format Remaining CodeRabbit findings from PR #46 (all real quick-wins): - streamCompletion leaked an abort listener: opts.signal is reused across the whole tool loop, but the 'abort' handler was only auto-removed via {once:true} when it actually fired — on normal end/error/timeout it stayed attached, so handlers piled up on the shared signal for the loop's lifetime. Added cleanup() (clear timer + removeEventListener) run on every terminal path. - parseDataUrl: guard comma === -1 (truncated data: URL) → return empty bytes instead of slicing garbage out of the metadata; and catch decodeURIComponent URIError on a stray % → fall back to raw bytes instead of bubbling. Tests for both. - ChatList formatTime: replaced fixed-offset substring slicing of toLocaleString() (breaks/truncates in non-US locales) with explicit Intl date+HH:MM fields; added the missing return type. tsc x2 clean; full suite green; coverage 97/92/96/98. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Review findings — triaged + addressedThanks to Gitar, CodeRabbit, Qodo, and SonarCloud. Went through every comment. Fixed all the substantive findings (each with a test + green gate); noted the ones that are intentionally out of scope. Fixed (commits on this branch):
Intentionally out of scope (not defects in this PR):
All fixes: tsc (both configs) clean, full suite green, coverage 97/92/96/98. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/__tests__/MemoryChat.reasoning.test.tsx (1)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor lint nits flagged by static analysis.
observe/unobserve/disconnectstubs andinstallApilack explicit return types, and_ain theaddRagMessagemock (line 38) is unused. NoteargsIgnorePattern: "^_"is not a default in@typescript-eslint/no-unused-vars— it must be explicitly configured, so the underscore convention used elsewhere in this file (e.g._argsat line 57, which is used) won't automatically suppress this one since_ais never referenced.🧹 Proposed cleanup
(globalThis as unknown as { ResizeObserver?: unknown }).ResizeObserver ??= class { - observe() {} - unobserve() {} - disconnect() {} + observe(): void {} + unobserve(): void {} + disconnect(): void {} };- const addRagMessage = vi.fn(async (..._a: AddRagArgs) => {}); + const addRagMessage = vi.fn(async () => {});Also applies to: 36-38
🤖 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 `@src/renderer/src/components/__tests__/MemoryChat.reasoning.test.tsx` around lines 24 - 28, Add explicit return types to the ResizeObserver stub methods observe, unobserve, and disconnect, and to installApi. Remove the unused _a parameter from the addRagMessage mock (or otherwise avoid declaring an unreferenced argument), since the lint configuration does not ignore underscore-prefixed variables by default.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 `@src/main/mcp-parse-data-url.ts`:
- Around line 26-27: Update the metadata check in the data URL parser to
recognize base64 only as a standalone metadata token, not as a substring within
values such as foo=notbase64 or foo=base64; preserve the existing base64
decoding behavior otherwise. Add a regression test covering the exact foo=base64
case and verifying the payload is not decoded as base64.
---
Nitpick comments:
In `@src/renderer/src/components/__tests__/MemoryChat.reasoning.test.tsx`:
- Around line 24-28: Add explicit return types to the ResizeObserver stub
methods observe, unobserve, and disconnect, and to installApi. Remove the unused
_a parameter from the addRagMessage mock (or otherwise avoid declaring an
unreferenced argument), since the lint configuration does not ignore
underscore-prefixed variables by default.
🪄 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 Plus
Run ID: a8aaecfa-6ad7-4498-a806-523b81d8f6af
📒 Files selected for processing (6)
src/main/__tests__/mcp-parse-data-url.test.tssrc/main/llm/stream.tssrc/main/mcp-parse-data-url.tssrc/renderer/src/components/ChatList.tsxsrc/renderer/src/components/MemoryChat.tsxsrc/renderer/src/components/__tests__/MemoryChat.reasoning.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/tests/mcp-parse-data-url.test.ts
- src/renderer/src/components/ChatList.tsx
- src/main/llm/stream.ts
- src/renderer/src/components/MemoryChat.tsx
Code Review ✅ Approved 2 resolved / 2 findingsImplements extensive architectural hardening including automated tooling for dead-code and dependency management while unifying MIME handling and core state persistence. Resolves multiple reported image attachment and reasoning state bugs with no remaining open findings. ✅ 2 resolved✅ Edge Case: HEIC/BMP image attachments fall back to image/png in vision data URL
✅ Bug: Reasoning captured via side effect inside setState updater is unreliable
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
…ed, D26)
autoConfigure passes the setup-vocab kind 'voice' to setActiveModalChoice, but
that function gated on isModalKind, which only accepts the storage vocab
('speech'/'image'/'transcription'). So the 'voice' call returned { success:false }
(swallowed by the try/catch) and Kokoro TTS was never set active — setup claimed
"voice is set up" while the model showed inactive.
Fix: setActiveModalChoice normalizes the kind through modalityForModel — the one
dispatch, now idempotent on 'speech' — so BOTH the setup 'voice' and the
dispatched 'speech' (from activateModel) activate TTS. One normalizer, both
vocabularies (DRY).
Test: active-models.test.ts gains the modalityForKind('speech')==='speech'
idempotency (red on HEAD — no 'speech' case); set-active-modal-choice.test.ts is
a source-contract guard that setActiveModalChoice no longer gates on isModalKind
and normalizes via modalityForModel (red on HEAD — had the isModalKind gate).
Full flow is the on-device check. Unit suite 2138 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The download loop renamed `<file>.part` to its final name (and recorded it installed + activatable) with NO integrity check. A server that closed the connection early, or any short read, left a TRUNCATED file that got promoted — then llama-server died on load with a blank "Chat model Down", the exact no-reason failure CLAUDE.md warns about. (importLocalModel already validated; the download path never did.) Fix: downloadIntegrityError (models/download-verify.ts) gates the promotion — rejects when written < the server-reported content-length, and for a .gguf when the magic header / min size fails. downloadModel throws on it before the rename, so a bad file surfaces as a visible "failed" instead of a silently-installed broken model. Test: download-verify.test.ts over real temp files — truncated / corrupt-magic / too-small are rejected; complete valid GGUF, non-GGUF, and no-length pass. Falsified (neutralizing the check reds the 3 rejection cases). Unit suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Downloading the same model twice (double-click, or Retry while it's already
downloading) set controllers[id] a second time — two writers raced into the same
`<file>.part` (interleaved → corrupt), and the second controller overwrote the
first, so a later Cancel/Clear aborted the wrong download and left an orphan.
Fix: downloadModel no-ops (`{ success:false, error:'already downloading' }`) if a
download for that id is already in flight — checked before any status emit or
controller registration.
Test: download-reentrancy.test.ts — source-contract guard that the check exists
and runs before controllers.set (red on HEAD). On-device check in DEVICE_TEST_LOG.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
D13 (QA claim: cancel-partial finalize strands the activity label) is a FALSE
POSITIVE: the activity/thinking block renders only while message.streaming
(MemoryChat.tsx:1721), and every finalize sets streaming:false, so the leftover
`activity` field is never shown. A test I wrote "passed" only because
streaming:false hid the label — a false-green — so I backed it out rather than
ship theater. Kept the one real find from the investigation: an
aria-label="Stop generating" on the Stop button, which had no accessible name.
D7 similarly dismissed (see DEVICE_TEST_LOG): the /health result IS honored
(setup.ts:306 `return { success: ok }`); the QA agent only read to line 279.
Both recorded honestly in DEVICE_TEST_LOG with the disproving evidence.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
deleteModel cleared a per-modality selection only when `modals[k] === modelId`. But image picks are stored by PRIMARY FILENAME (setActiveModalChoice maps id -> filename for image), so deleting the active image model never matched → the active-modalities pointer dangled at a now-deleted file, and the next image gen failed to load with no clear reason. Fix: modalSelectionMatches (pure, in catalog-logic) matches a stored selection by id OR primary filename; deleteModel passes the entry's primaryFileName so the image modality is cleared too. Test: catalog-logic.test.ts — the matcher matches by filename (the case HEAD's id-only compare missed) and by id, and rejects null/mismatch. Full delete flow is the on-device check. tsc + suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…D23) deleteRagConversation deleted only the rag_conversations row. rag_messages orphaned (FKs are off, so the ON DELETE CASCADE never fired) and the conversation's generated artifacts (keyed by conversationId) lingered in the library forever — still openable, unbounded disk growth. Same missing lifecycle tie D20 fixed for projects. Fix: deleteRagConversation deletes the conversation's rag_messages explicitly; the rag:delete-conversation handler also calls the new deleteArtifactsForConversation (mirrors deleteArtifactsForProject). Test: conversation-delete-cascade.dbtest.ts — real temp SQLite + real artifact files; runs the exact two calls the handler makes, asserts rows + artifact gone. Red when the fixes are reverted (falsified). DB suite 69/69; unit suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this is
The
chore/quality-hardeningbranch: fix the real bugs, land the SOLID/DRY refactorbacklog, bring coverage to an honest floor, and stand up the code-quality tooling spine.
One PR per repo (pro is a separate repo — companion PR in
desktop-pro, same branch name).🐞 Real bugs fixed (each with a regression test)
src/main/mime.ts— fixed a.webpattachmentmislabelled
image/jpeg(vision model may reject) andgif→image/png.classify sets (was omitting gif/bmp/heic/opus/avi the router handles).
pre-decides when the agentic tool path owns the turn.
survives reload, KV-cache choice survives restart.
Refactors (SOLID/DRY — behavior-neutral, tested)
ToolDef.rununiform dispatch (DIP); onemimeForExt;appNameLikeClause(7 rag:chat sites);likeMatch/LIKE_COLUMNS(facet counts == results);epochMsSql; sharedisValidGguf,artifact-kind/model-kind label maps,
CHAT_JOB/IMAGE_JOB,ffmpeg-decode,streamCompletion(one SSE transport),
parseSqliteUtc,parseSessionId; ChatList link cyan→emerald token.Coverage — honest floor, enforced
all:true+ the true testable-surfaceinclude/exclude(native/IPC/spawn shells excluded, eachcovered by e2e/smoke/test:db). Ratchet floor in
vitest.config.ts, blocked on pre-push AND CI.Now 97.1 / 92.1 / 96.0 / 98.1 (stmts/branches/funcs/lines). README carries the badges.
Quality-tooling spine (new)
circular/unresolvable/dev-dep/orphans. 0 violations.
noUnusedLocals/noUnusedParameters/allowUnreachableCode:false.npm run knip) — module-graph dead code; first pass deleted 27 unused template UI files.no-unnecessary-condition(warn-ratchet) — dead-branch detection.Evidence
npx tsc --noEmit— bothtsconfig.node.jsonandtsconfig.web.json: 0 errors.npm run test:coverage— 2110 passed, 1 skipped; floor held (97.1/92.1/96.0/98.1).npm run depcruise— 0 violations.Honest gaps (not blocking this PR)
(regression safety) + structural health; they do NOT verify end-to-end functionality (real model
chat/image-gen, connectors, capture). Recommend the Playwright e2e tour + a live golden-path smoke
before release. Screenshots therefore pending the e2e run.
docs/CODE_AUDIT.md(now a clean "what's left" list): ImageRuntime,imagegen/ipc/database SRP splits, tabler→Phosphor, and the warn-ratchet lint backlog
(no-unnecessary-condition 289, sonarjs-on-pro 295, knip deps/exports). Each lands as its own
verified change — not this PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by Gitar
dependency-cruiserfor architectural boundary enforcement andknipfor dead-code elimination.ipc-query-logic.tsand refined tool dispatch using a uniformToolResultstructure.agent: false) forhttp.requestto preventECONNRESETin multi-round tool loops.userExplicitpin-set for LLM settings to ensure user-selected performance modes and cache configurations persist across restarts.noUncheckedIndexedAccessacross the project, resulting in 14/14 module migrations and associated guard logic.This will update automatically on new commits.