Desktop release (umbrella): meetings STT + capture grounding + remaining todos#60
Merged
Merged
Conversation
Meeting/voice surfaces need to show which speech-to-text engine + model would run. Add a pure transcriptionProvenance(engine, active, entries) label builder + a live getActiveTranscriptionInfo() reading the same active-model source of truth the transcription path uses (engineForActiveModel + effectiveEngine — no re-derivation). Exposed via transcription:active-info IPC + preload getTranscriptionInfo(). Tests: 6 unit cases for transcriptionProvenance (engine labels, id/filename match, whisper-resident labeling, built-in default, unknown-model fallback). E2E asserts the meeting detail renders 'Whisper · built-in · on device' in the built app (screenshot e2e/screenshots/meeting-transcription-provenance.png).
The provenance label alone wasn't enough — the user needs to easily SEE and CHANGE the
whisper/STT model. Add a pure transcriptionModelOptions(active, installed) (built-in default +
installed transcription models, active flagged) and surface it in the transcription:active-info
IPC. Switching still goes through the existing setActiveModalModel('transcription') — one source
of truth, no parallel selection path.
Tests: 6 unit cases for transcriptionModelOptions (built-in default, id/filename active match,
name fallback). E2E asserts the meeting detail exposes the 'Transcription model' picker + screenshot
(e2e/screenshots/meeting-transcription-picker.png).
…y judge bench-capture.mjs drops the dead OCR leg (production grounds on the Accessibility tree, fetched live) and measures the image path only - the lever that scales with frame count. Adds: comma-list batch sweep (--batch 3,5,7,9), --batch-only to reuse a known single baseline, and --judge (LLM-as-judge scoring accuracy/specificity/grounding of each summary against the current frame). Used to settle the 'how many frames per vision call' question: text-summary context beats stuffing raw frames into one call.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe PR adds transcription provenance APIs, GGUF-trained context limits, LLM generation and teardown updates, renderer workflow fixes, and a vision-only capture benchmark with single-frame, batch, judging, and scaling measurements. ChangesTranscription provenance flow
GGUF context ceiling
LLM generation and lifecycle
Renderer workflow behavior
Vision capture benchmark
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant offGridApi
participant IPC
participant getActiveTranscriptionInfo
participant transcriptionModelOptions
offGridApi->>IPC: invoke transcription:active-info
IPC->>getActiveTranscriptionInfo: read active engine and catalog
IPC->>transcriptionModelOptions: build installed picker options
transcriptionModelOptions-->>IPC: return options
IPC-->>offGridApi: return active info and options
sequenceDiagram
participant benchCapture
participant tempJpegs
participant localChatCompletions
participant judgeSummary
benchCapture->>tempJpegs: create downscaled frame JPEGs
benchCapture->>localChatCompletions: submit single or batch vision request
localChatCompletions-->>benchCapture: return summary and token usage
benchCapture->>judgeSummary: evaluate summary against last frame
judgeSummary->>localChatCompletions: submit rubric request
localChatCompletions-->>judgeSummary: return rubric scores
benchCapture->>tempJpegs: remove temporary JPEGs
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
alichherawalla
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/meeting-transcription.spec.ts (1)
50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWait for a semantic meeting-detail state instead of CSS and sleeps.
[class*="cursor-pointer"]can select an unrelated element, and the fixed delays do not establish that meeting detail loaded. Target the seeded meeting with an accessible locator, then await a detail heading or landmark before asserting the picker.As per coding guidelines, E2E tests must use DOM-driven assertions.
🤖 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 `@e2e/meeting-transcription.spec.ts` around lines 50 - 57, Replace the CSS-based firstMeeting lookup and fixed waitForTimeout calls with a semantic locator for the seeded meeting, using its accessible name or role. After opening it, wait for and assert the meeting-detail heading or landmark before checking the transcription picker, while preserving the existing navigation 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.
Inline comments:
In `@e2e/meeting-transcription.spec.ts`:
- Line 36: Update the button selector in the meeting transcription test to match
the approved product name, replacing the “Start using Off Grid” alternative in
the selector used by the test with “Start using Off Grid AI Desktop.”
In `@scripts/bench-capture.mjs`:
- Around line 187-212: Guard all three downscale call sites so a per-frame
failure is handled without aborting the benchmark: in scripts/bench-capture.mjs
lines 187-212, move downscale inside the existing per-frame try/catch and
log/skip the frame; in lines 175-181, wrap the warmup downscale in the same
error-handling pattern as visionSingle; and in lines 214-248, protect the window
map while preserving cleanup of images already created before a failure.
In `@src/main/transcription/select.ts`:
- Line 153: Update the model selection flow so the returned engine, modelId, and
modelLabel all describe the model that will actually run after fallback; when
Parakeet falls back to Whisper, normalize the model identity to the built-in
Whisper model rather than retaining the unavailable active id. Ensure the IPC
picker can identify the resolved model as installed, add a regression case
covering this fallback, and replace the `·` label separator with plain ASCII
punctuation.
---
Nitpick comments:
In `@e2e/meeting-transcription.spec.ts`:
- Around line 50-57: Replace the CSS-based firstMeeting lookup and fixed
waitForTimeout calls with a semantic locator for the seeded meeting, using its
accessible name or role. After opening it, wait for and assert the
meeting-detail heading or landmark before checking the transcription picker,
while preserving the existing navigation flow.
🪄 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: a112a660-5d5d-4845-b449-578b39c9802c
⛔ Files ignored due to path filters (1)
e2e/screenshots/meeting-transcription-picker.pngis excluded by!**/*.png
📒 Files selected for processing (6)
e2e/meeting-transcription.spec.tsscripts/bench-capture.mjssrc/main/ipc.tssrc/main/transcription/__tests__/select.test.tssrc/main/transcription/select.tssrc/preload/index.ts
An image job already survives a MemoryChat remount (main owns it; the reattach effect re-reads imageGenJobStatus), but reattach only restored imageGenConv/imgProgress - NOT generatingConvs, which is the render gate for the in-flight progress panel. So on a remount mid-generation the job kept running but the panel stayed invisible: the user's 'it eventually generated, the UI just didn't show it'. - observe() now mirrors the SAME state the live-gen path sets: markGenerating(convId, true) while running, false when done - so a reattached run shows the whole in-flight UI, not just the internal owner. - Preload exposes meetingRetranscribeStatus (pairs with the pro-side re-transcribe status). Test: MemoryChat.image reattach regression - mount with main reporting a running job for the open conversation renders the live 'Step 3/20' panel; removing the markGenerating call makes it red (verified).
… a fixed cap Context was ceilinged by an arbitrary constant (DEFAULT_CTX_SIZE) with no awareness of what the model was actually trained for. Running llama.cpp above the trained window needs RoPE scaling and otherwise degrades — the 'above 16k it breaks, back to LM Studio' symptom. Now we read the model's trained window from GGUF metadata and use it as the ceiling (like LM Studio): a user can run right up to the model's own max, and we never silently exceed it. - models/gguf-metadata.ts: pure GGUF KV-block reader for <arch>.context_length (+ a GgufFs-injected file reader that reads only a multi-MB prefix, short-circuiting before the huge tokenizer array). - model-sizing.capContextToModel: pure ceiling rule (min(requested, trainedMax); no cap when unknown). - llm.ts: safeCtxSize now caps to the trained window (memoized per model path) BEFORE the RAM clamp; getSettings exposes modelMaxCtx; modelMaxContext() surfaces it. - SettingsPanel: the context picker is bounded by the model's max (offering the exact max), with a hint that distinguishes a model cap from a RAM clamp (pure contextWindowOptions/contextWindowHint). Tests: gguf-metadata unit (UINT32/UINT64 ctx, short-circuit before tokenizer array, reorder, missing key, non-gguf, v1) + real-temp-file reader; capContextToModel + ctx-options/hint units; llm-context-ceiling integration over a REAL GGUF on disk — trained 8192 pulls the 16384 default down to 8192, a 131072-trained model is NOT capped below its max, unreadable → null (delete-the-impl verified: removing the cap makes the 8192 case fail).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/llm.ts`:
- Around line 201-203: Update safeCtxSize so the result from computeSafeCtx()
cannot exceed the already model-capped requested value, preserving the trained
context ceiling for models such as 1024-token GGUFs. Add an integration
regression covering a 1024-token GGUF and asserting the safe context remains at
or below 1024 tokens, including the relevant safety-floor path.
In `@src/renderer/src/lib/ctx-options.ts`:
- Around line 37-38: Replace the em dash in the user-facing return string within
the model context-cap logic with plain ASCII punctuation, such as a period or
comma. Preserve the existing message content and interpolation.
🪄 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: 7434a8d7-20af-4ec1-b72e-d4546181aebe
📒 Files selected for processing (12)
src/main/__tests__/llm-context-ceiling.integration.test.tssrc/main/__tests__/model-sizing.test.tssrc/main/llm.tssrc/main/model-sizing.tssrc/main/models/__tests__/gguf-metadata.test.tssrc/main/models/gguf-metadata.tssrc/preload/index.tssrc/renderer/src/components/MemoryChat.tsxsrc/renderer/src/components/SettingsPanel.tsxsrc/renderer/src/components/__tests__/MemoryChat.image.test.tsxsrc/renderer/src/lib/__tests__/ctx-options.test.tssrc/renderer/src/lib/ctx-options.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/preload/index.ts
… force-quit
The bundled llama-server can hang on shutdown (GGML/Metal abort), and stop() sent a single
SIGTERM then immediately nulled the handle — so a wedged engine kept holding :8439, which is why
the app 'couldn't be unloaded without a force-quit/reboot' and blocked LM Studio.
- llm/engine-teardown.ts: pure terminateEngine — SIGTERM, wait a grace window, escalate to SIGKILL
if still alive; reports graceful/forced/stuck. Injected effects → unit-testable, plus real-OS-
process tests (incl. a process that TRAPS SIGTERM → forced).
- llm.unload(): awaitable teardown using terminateEngine + waitForProcExit, then reaps any of OUR
llama-server still on the port (never another live app's). Returns { outcome, portFree }.
- Wired: llm:unload IPC + preload unloadLlmEngine; a 'Free engine' button in System Health (with a
freed / still-held message); and app before-quit now awaits unload so ports free on exit.
Tests: engine-teardown unit (4 escalation outcomes + exact signal sequence) + real-process
(graceful vs SIGTERM-trap→forced); HealthPanel.free-engine UI (both result branches, fake api);
HealthPanel.integration end-to-end frees the real fixture engine (isReady()→false) — that one needs
:8439 free so it's CI-verified (a running dev app owns the port locally).
…a 2048 cap Max OUTPUT tokens — not context — was the real limiter: every reply was capped at maxTokens=2048 (~1500 words) regardless of the 128K/256K window, so long answers were truncated. Default is now 'auto': generate until the model's natural stop (EOS) or the context is exhausted. - shared/llm-defaults: MAX_TOKENS_AUTO (0 sentinel) + DEFAULT_MAX_TOKENS — one source for main + UI. - gen-params.maxTokensForWire: maps the resolved cap to the wire value — a positive number is a hard cap; auto (<=0) becomes n_predict = -1. All three chat payloads route through it; the returned metadata keeps the logical value (0=auto), not the wire -1. - llm.ts default maxTokens = MAX_TOKENS_AUTO. - Settings: 'Max output' is now a select — 'Auto (until the model stops)' (default) + 2K..32K hard caps — with a hint explaining auto vs a fixed cap. Tests: gen-params — maxTokensForWire (auto/0/-1 → -1, positive passthrough), resolveMaxTokens auto passthrough, and source guards (default is auto; all 3 payloads wire-mapped, no raw resolveMaxTokens in a payload). HealthPanel.free-engine unused-import cleanup.
…the max-output setting Root cause of the 'All memory' essay getting cut off mid-sentence at ~870 words: the agentic tool loop hardcoded maxTokens: 1024 on BOTH the streaming round that produces the final answer and the forced-final generation. A per-call value wins in resolveMaxTokens, so this 1024 overrode the user's setting AND the new auto default — the log showed the essay generation stop at exactly 1024 predicted tokens (finish=length). Removed both hardcodes so the answer inherits the user's Max-output setting (auto by default → until EOS / window fills). Tool-selection rounds stay short on their own (they emit a call), so no artificial cap is needed there. Tests (tools-loop.dbtest, real toolChat + real LLMService over the fake socket): the first-round request carries the user's setting (5000) not 1024; and with the auto default the wire max_tokens is -1. Delete-the-impl: re-adding maxTokens:1024 makes both fail.
…olls up While a response streamed, scrolling up snapped back to the bottom on the next token. The follow effect used behavior:'smooth', so each token started an animation that held scrollTop near the bottom; the next token then re-measured as 'near bottom' and re-scrolled — a feedback loop the user couldn't escape by scrolling. - scroll-follow.ts (pure): shouldFollowBottom(metrics) — follow only within a small slack of the bottom; a real scroll-up stops it. - MemoryChat: a followBottomRef driven by the container's onScroll captures the USER'S intent (not a mid-animation position); the stream effect scrolls only when it's set, and INSTANTLY (block:'end', no smooth) so there's no animation to fight. Tests: scroll-follow unit — pinned/near-bottom follow, scrolled-up does not, custom threshold.
…g answers survive 'LLM request timed out' at ~5 min: the stream used a single total-duration timer (set once, cleared only on completion). Harmless while output was capped at 1024 (fast), but once we uncapped output a legitimately long/slow answer ran past 300s and was killed mid-stream. Now the timer is re-armed on every received chunk, so a stream that keeps making progress never times out; only a genuinely stalled generation (no data for opts.timeoutMs) does. Tests (real local SSE server): a stream that drips for longer than timeoutMs with each gap under it now COMPLETES (delete-the-impl: a total timer fails this); a stream that stalls mid-response still times out.
…e-limit test The max-output control became a <select> (auto default), so the response-limit integration test's query for the old input[type=range][max=32768] found nothing. Added aria-label to both the Context window and Max output selects (accessible + testable) and pointed the test at select[aria-label= 'Max output']. Fixes the pre-push test failure from the max-output change.
The composer's left chip group (memory scope / model / Thinking / Image) was a non-wrapping flex, so at narrow widths the Image chip clipped off the right edge of the input box. Added flex-wrap so the chips flow onto a second line instead of overflowing.
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 `@src/main/llm.ts`:
- Around line 1059-1091: Serialize unload with in-flight initialization by
coordinating it with the initialization path around init() and _doInit(). Ensure
unload waits for or cancels any pending initialization before snapshotting
this.server, so no engine can spawn after teardown and portFree is reported only
after initialization has settled.
In `@src/main/llm/engine-teardown.ts`:
- Around line 39-47: Recheck process liveness in the teardown flow after the
SIGTERM wait and before sending SIGKILL, returning a graceful outcome without
escalation when fx.isAlive() is false; after the SIGKILL wait, use fx.isAlive()
again before returning stuck. In src/main/llm/engine-teardown.ts#L39-L47 update
the escalation logic, and in
src/main/llm/__tests__/engine-teardown.test.ts#L37-L46 add a fake where SIGTERM
times out but isAlive() becomes false, asserting no SIGKILL and a graceful
result.
In `@src/renderer/src/components/MemoryChat.tsx`:
- Around line 707-712: Reset followBottomRef to true whenever the active
conversation changes, using the conversation identity/state already available in
MemoryChat. Keep the existing onScrollFollow behavior unchanged so subsequent
user scrolling still updates the ref.
In `@src/renderer/src/components/SettingsPanel.tsx`:
- Around line 287-306: Update the Max output control in SettingsPanel: replace
focus:border-green-500 with the approved emerald focus token, such as
focus:border-emerald-400, and replace the em dash in the Auto hint with ASCII
punctuation while preserving the hint’s meaning.
In `@src/renderer/src/components/setup/HealthPanel.tsx`:
- Around line 81-85: Update the HealthPanel status messages in setFreeMsg and
the related lines around the other status strings to use only ASCII punctuation,
replacing em dashes and Unicode ellipses. Remove the rounded-xl class from the
affected panel styling so the desktop surface remains flat and sharp.
In `@src/renderer/src/lib/__tests__/scroll-follow.test.ts`:
- Around line 24-28: Add regression assertions in the custom-threshold test for
shouldFollowBottom at exactly the configured FOLLOW_THRESHOLD_PX and one pixel
beyond it, verifying equality follows and the next pixel does not. Preserve the
existing below/above threshold coverage.
🪄 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: 95ecd69a-db64-4924-9301-26e21ede870e
📒 Files selected for processing (21)
src/main/__tests__/tools-loop.dbtest.tssrc/main/index.tssrc/main/ipc.tssrc/main/llm.tssrc/main/llm/__tests__/engine-teardown.test.tssrc/main/llm/__tests__/gen-params.test.tssrc/main/llm/__tests__/stream.test.tssrc/main/llm/engine-teardown.tssrc/main/llm/gen-params.tssrc/main/llm/stream.tssrc/main/tools.tssrc/preload/index.tssrc/renderer/src/components/MemoryChat.tsxsrc/renderer/src/components/SettingsPanel.tsxsrc/renderer/src/components/__tests__/MemoryChat.response-limit.integration.test.tsxsrc/renderer/src/components/setup/HealthPanel.tsxsrc/renderer/src/components/setup/__tests__/HealthPanel.free-engine.test.tsxsrc/renderer/src/components/setup/__tests__/HealthPanel.integration.test.tsxsrc/renderer/src/lib/__tests__/scroll-follow.test.tssrc/renderer/src/lib/scroll-follow.tssrc/shared/llm-defaults.ts
The ratchet floor had climbed to ~95/89.9/93/96, which turned brittle against CI's legitimately- skipped ambient journeys (macOS-helper / native-dep tests skip on Linux CI) — a 0.1-0.3% swing flipped the gate red even though all tests pass. 85% is the standard stated in CLAUDE.md and sits comfortably below current measured coverage (~95/90/96) while still blocking real regressions. Maintainer's call.
- llm.safeCtxSize: re-cap to the trained window AFTER the RAM clamp — computeSafeCtx's 2048 floor
could exceed a sub-2048-trained model's context (CodeRabbit: RAM floor > trained ceiling). Test:
a 1024-trained model now effectiveContextSize()===1024, not 2048.
- llm.unload: serialize with in-flight init — await a pending initPromise and re-terminate whatever
it spawns, re-asserting intentionalStop each round, so a spawn racing the unload can't survive.
- engine-teardown: recheck isAlive() before escalating to SIGKILL (a wait can race a clean exit);
don't mislabel 'forced'. Test added.
- MemoryChat: reset followBottomRef when the conversation changes (a scroll-up in one chat no
longer stops the next chat's stream from auto-following).
- Brand copy: em dash → ' - ' in the context-window hint; ASCII copy in HealthPanel's free-engine
messages ('Freeing...', ' - ').
- e2e: use the full product name 'Off Grid AI Desktop' in the selector.
- scroll-follow: cover the exact == threshold boundary.
Deferred (replied on-thread, logged as gaps): bench-capture.mjs downscale resilience (dev-only
tool), transcription provenance label on a rare cross-family engine fallback.
…, provenance fallback)
…Cloud reliability) SonarCloud flagged testing this.initPromise (a Promise) for truthiness in unload's loop — a Promise is always truthy, so it was really a null check. Make it explicit (pending === null). No behavior change; clears the 'C Reliability Rating on New Code' quality-gate condition.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
e2e/meeting-transcription.spec.ts (3)
49-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the test name with its actual coverage.
This test verifies that the picker is visible and contains Whisper, but it never changes the selected model. Rename it to indicate view-only coverage, or perform a selection and assert the new value.
🤖 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 `@e2e/meeting-transcription.spec.ts` around lines 49 - 64, Rename the test around the visible symbol `meeting detail exposes the STT model picker (view + change)` to indicate view-only coverage, since the test only checks visibility and the Whisper option. Leave the existing assertions and screenshot behavior unchanged.
53-56: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not swallow meeting-row click failures.
The empty
catchconverts a navigation failure into an unrelated picker timeout, while the visibility branch allows the test to continue without opening a meeting. Wait for the row and let the click fail at its source.🤖 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 `@e2e/meeting-transcription.spec.ts` around lines 53 - 56, Update the firstMeeting interaction in the meeting transcription test to wait for the meeting row and click it without swallowing errors. Remove the visibility guard and empty catch so failures from firstMeeting.click() propagate directly instead of allowing the test to continue without opening a meeting.
18-20: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard teardown when the pro suite is skipped.
When
PRO_PRESENTis false,beforeAllnever assignsuserDataDir, butafterAllstill runs and callsfs.rmSync(undefined, ...), which can fail the skipped suite. Use a top-leveltest.skip(...)guard or keepuserDataDiroptional and only remove it when initialization completed.🤖 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 `@e2e/meeting-transcription.spec.ts` around lines 18 - 20, Guard teardown for the skipped pro suite by applying a top-level test.skip guard, or by making userDataDir optional and having afterAll remove it only after initialization completes. Update the beforeAll/afterAll lifecycle around userDataDir while preserving cleanup for initialized suites.src/main/llm.ts (1)
114-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard non-streaming Auto max-tokens on the mutex-serialized
chat()path.
chat()defaultsmaxTokens = MAX_TOKENS_AUTO, andmaxTokensForWiresends that as-1for llama.cpp unlimited generation.postCompletionOncestill uses a single hardtimeoutMstimer that is not reset on response progress, so a long Auto-lengthchat()request can holdthis.chatMutexfor most/all of the 300s wall-clock timeout while other callers wait. Add idle-timed reset semantics topostCompletionOncelike the streaming path, or require explicit short bounds for Auto calls that should remain extraction/short-response style.🤖 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/llm.ts` around lines 114 - 116, Guard the non-streaming chat() path against unbounded MAX_TOKENS_AUTO requests: update postCompletionOnce and its mutex-serialized caller to either reset the timeout on response progress like the streaming path or enforce an explicit short max-token bound for Auto calls. Preserve unlimited generation only where appropriate and ensure chatMutex is not held by a long-running Auto request under a single fixed wall-clock timeout.
🧹 Nitpick comments (1)
e2e/meeting-transcription.spec.ts (1)
50-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace fixed sleeps with state-based waits.
The
waitForTimeoutcalls introduce unnecessary timing dependence. Wait for the meeting row/detail or picker to become visible instead, so slow CI does not race and fast runs do not idle.As per coding guidelines, E2E tests must use DOM-driven assertions.
🤖 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 `@e2e/meeting-transcription.spec.ts` around lines 50 - 57, Replace the fixed waitForTimeout calls in the meeting navigation flow with DOM-driven waits tied to visible UI state. After clicking the Meetings button, wait for the first meeting row represented by firstMeeting to become visible before clicking it, then wait for the transcription detail or picker element to become visible before continuing. Preserve the existing conditional handling for absent meetings without introducing timing-based delays.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.
Inline comments:
In `@docs/GAPS_BACKLOG.md`:
- Around line 105-106: Update the “Deferred from desktop-pro PR `#32` review
(Gitar)” heading in GAPS_BACKLOG.md to use the canonical product name “Off Grid
AI Desktop” instead of “desktop-pro”, preserving the rest of the heading.
---
Outside diff comments:
In `@e2e/meeting-transcription.spec.ts`:
- Around line 49-64: Rename the test around the visible symbol `meeting detail
exposes the STT model picker (view + change)` to indicate view-only coverage,
since the test only checks visibility and the Whisper option. Leave the existing
assertions and screenshot behavior unchanged.
- Around line 53-56: Update the firstMeeting interaction in the meeting
transcription test to wait for the meeting row and click it without swallowing
errors. Remove the visibility guard and empty catch so failures from
firstMeeting.click() propagate directly instead of allowing the test to continue
without opening a meeting.
- Around line 18-20: Guard teardown for the skipped pro suite by applying a
top-level test.skip guard, or by making userDataDir optional and having afterAll
remove it only after initialization completes. Update the beforeAll/afterAll
lifecycle around userDataDir while preserving cleanup for initialized suites.
In `@src/main/llm.ts`:
- Around line 114-116: Guard the non-streaming chat() path against unbounded
MAX_TOKENS_AUTO requests: update postCompletionOnce and its mutex-serialized
caller to either reset the timeout on response progress like the streaming path
or enforce an explicit short max-token bound for Auto calls. Preserve unlimited
generation only where appropriate and ensure chatMutex is not held by a
long-running Auto request under a single fixed wall-clock timeout.
---
Nitpick comments:
In `@e2e/meeting-transcription.spec.ts`:
- Around line 50-57: Replace the fixed waitForTimeout calls in the meeting
navigation flow with DOM-driven waits tied to visible UI state. After clicking
the Meetings button, wait for the first meeting row represented by firstMeeting
to become visible before clicking it, then wait for the transcription detail or
picker element to become visible before continuing. Preserve the existing
conditional handling for absent meetings without introducing timing-based
delays.
🪄 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: a67a0b2a-7970-4549-b89f-ee15ef0f4f7d
📒 Files selected for processing (11)
docs/GAPS_BACKLOG.mde2e/meeting-transcription.spec.tssrc/main/__tests__/llm-context-ceiling.integration.test.tssrc/main/llm.tssrc/main/llm/__tests__/engine-teardown.test.tssrc/main/llm/engine-teardown.tssrc/renderer/src/components/MemoryChat.tsxsrc/renderer/src/components/setup/HealthPanel.tsxsrc/renderer/src/lib/__tests__/scroll-follow.test.tssrc/renderer/src/lib/ctx-options.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/src/lib/tests/scroll-follow.test.ts
- src/renderer/src/components/setup/HealthPanel.tsx
- src/main/llm/tests/engine-teardown.test.ts
- src/renderer/src/lib/ctx-options.ts
- src/main/tests/llm-context-ceiling.integration.test.ts
- src/main/llm/engine-teardown.ts
- src/renderer/src/components/MemoryChat.tsx
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.
Desktop release — single umbrella PR (core / OGAD)
THE release PR for core. All work landed on one branch (
feat/meetings-and-capture-grounding). Pro-side companion: desktop-pro#32. Every item is green on code · unit · integration; UI behaviours verified live in the running app.Landed
generatingConvs), not just the owner; preload exposesmeetingRetranscribeStatusbench-capture.mjs; ground with prior summaries as text, not raw-frame batches<arch>.context_lengthfrom GGUF; cap to it (like LM Studio) before the RAM clamp; Settings picker bounded by the model maxterminateEngine(SIGTERM→SIGKILL), awaitablellm.unload(), "Free engine" button in System Health, before-quit teardown — frees the model port without a force-quitn_predict = -1, until EOS / window fills) instead of a fixed 2048; Settings "Max output" select (Auto + hard caps)maxTokens: 1024(the essay cut-off root cause); it inherits the settingonScroll-driven follow flag + instant scroll; the view no longer snaps back to the bottom while you readStill to land on this branch
Removed from this release: remote model list/load via an /api/list endpoint.
Evidence
New unit/integration coverage this release: gguf-metadata + capContextToModel + ctx-options (context ceiling), engine-teardown (real-process SIGTERM-trap → forced), gen-params maxTokensForWire + tools-loop max_tokens inheritance, stream idle-timeout (drip completes / stall times out), scroll-follow, image-gen reattach, transcription select.
tsc --noEmitclean (node + web).Version stays 0.0.40; say the word to bump to 0.0.41 for the cut.
Summary by CodeRabbit