Fix repo-picker freeze + presets-only settings, Responses runner, Telegram relay#16
Merged
Merged
Conversation
The folder picker was a synchronous Tauri command, which runs on the main (UI) thread. blocking_pick_folder() then blocked that thread waiting for a dialog that needs the same thread's event loop to run -> deadlock/freeze on every repo selection. Make pick_directory async and use the non-blocking pick_folder + a oneshot channel so the dialog runs on the main thread while the handler awaits off it. check_git_repo and which_commands are made async too so their git/PATH probes never stall the UI thread. Adds tokio 'sync' feature for the oneshot channel. Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
A new ResponsesRunner (kind 'http-responses') targets POST /v1/responses (input + instructions -> output_text), distinct from HttpRunner which speaks the older Chat Completions shape. Wired into the runner factory and the RunnerProfile schema/kind union. Mirrors HttpRunner's timeout, cancellation, env-by-reference auth, quota detection, and non-throwing error handling. Includes a 13-case test covering extraction, request shaping, and quota. Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
Replace the provider/model matrix with three first-class runner presets: - codex-cli codex exec --json --model <model> (edits files) - openai-responses OpenAI Responses API for a model id like gpt-5.5 - kiro-cli kiro-cli chat --no-interactive --trust-tools=... (KIRO_API_KEY) The settings page now picks a preset per role (writer=actor, reviewer=critic) with an editable model id (default gpt-5.5 where configurable), shows auth status (command installed / key stored), and keeps raw runner JSON behind the Advanced escape hatch. Adds a configurable Kiro trusted-tools field. Bumps the settings storage key (v2) so legacy provider/model selections fall back to preset defaults. Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
Add a private phone-updates layer that pushes final run status to a single allowlisted Telegram chat and long-polls for replies, treating each as a new goal it submits as a run (reusing the last run's presets + repo). It uses only OUTBOUND HTTPS to api.telegram.org, so the engine stays loopback-only with no inbound exposure. - src/notify/telegram.ts: TelegramRelay (push + control loop, allowlist, backlog skip, backoff) and createTelegramRelayFromEnv (LOOPWRIGHT_TELEGRAM_* or plain TELEGRAM_* secret names). - server: extract submitRun() core shared by the HTTP route and the relay, expose submitRun + lastRunConfig, and add an outbound ServerNotifier hook fired on terminal status (best-effort, never affects a run). - index: build the relay from env, attach the server, start/stop with lifecycle. - The desktop settings page gains a Telegram card (stores token + chat id as secrets, restarts the engine to apply). Includes an 11-case relay test. Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR migrates the desktop settings from a provider/model catalog to runner presets ( ChangesDesktop: Async Tauri Commands and Preset-Based Settings
ResponsesRunner: New
Telegram Relay and Server Programmatic Submission
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over TelegramUser,LoopwrightServer: Inbound goal submission via Telegram
end
participant TelegramUser as Telegram User
participant TelegramAPI as Telegram API
participant TelegramRelay
participant LoopwrightServer
TelegramRelay->>TelegramAPI: getUpdates(offset, long-poll timeout)
TelegramAPI-->>TelegramRelay: message update from allowed chatId
TelegramRelay->>TelegramRelay: handleUpdate — verify chatId, parse goal text
TelegramRelay->>LoopwrightServer: submitRun(goal, lastRun.env, repoDir)
LoopwrightServer-->>TelegramRelay: SubmitResult { ok, sessionId }
TelegramRelay->>TelegramAPI: sendMessage("Run started")
LoopwrightServer-->>TelegramRelay: runFinished(RunFinishedInfo)
TelegramRelay->>TelegramAPI: sendMessage(formatFinalStatus)
TelegramAPI-->>TelegramUser: status message
sequenceDiagram
rect rgba(200, 230, 180, 0.5)
Note over DesktopUI,ResponsesRunner: Preset-driven run execution via ResponsesRunner
end
participant DesktopUI as Desktop UI (main.ts)
participant Settings as settings.ts
participant ResponsesRunner
participant OpenAI as OpenAI /v1/responses
DesktopUI->>Settings: buildRunEnv(settings)
Settings->>Settings: profileFor(openai-responses preset)
Settings-->>DesktopUI: LOOPWRIGHT_RUNNERS env var
DesktopUI->>ResponsesRunner: run(RunRequest)
ResponsesRunner->>OpenAI: POST {baseUrl}/responses (model, input, apiKey)
OpenAI-->>ResponsesRunner: 200 response body
ResponsesRunner->>ResponsesRunner: extractResponsesText(body)
ResponsesRunner-->>DesktopUI: RunResult { text, meta }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/main.ts (1)
352-366:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly mark CLI tools missing after the probe actually succeeds.
If
detectCommands()throws,installedstays{}, butcommandMissing()and the detected-tools badges both treat that as "missing" for every CLI. That turns a probe failure into false onboarding/publishing warnings. Track probe success separately and only surface missing-command states when that probe completed.Also applies to: 369-385, 728-734
🤖 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 `@desktop/src/main.ts` around lines 352 - 366, The issue is that when `detectCommands()` throws an exception, the `installed` variable remains as an empty object, but this is then incorrectly interpreted as "all CLI tools are missing" by `commandMissing()` and the detected-tools badges, causing false warnings. The fix is to track whether the probe succeeded separately (similar to how `knowKeys` tracks success for `listSecretKeys()`). Create a new boolean variable (like `probeSucceeded` or `cliToolsDetected`) that is only set to true when `detectCommands()` completes without throwing. Then update all references to `installed` in `commandMissing()` and the detected-tools badge logic to only treat commands as missing if the probe actually succeeded. Apply this same pattern to the other affected locations at lines 369-385 and 728-734 where similar probe-and-track patterns exist with different data (listSecretKeys, etc.).
🤖 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 `@desktop/src/main.ts`:
- Around line 764-817: The Telegram configuration flow requires both bot token
and chat ID to be properly set, but the current implementation only validates
the token on first save and uses stale hasToken/hasChat values captured once at
initialization. Modify the saveBtn click handler to require both
TELEGRAM_BOT_TOKEN and TELEGRAM_CHAT_ID to be present (either already stored or
newly provided) before allowing save. After successfully calling setSecret and
restartEngine, recompute the hasToken and hasChat flags from the current stored
state (or refetch them) and update tgStatus.textContent based on the actual
current values instead of hardcoding "stored · stored". Apply the same pattern
to the removeBtn handler (which begins around line 820) to ensure that after
removing either secret, the cached flags are refreshed and the status display
reflects the actual stored state.
- Around line 784-817: The saveBtn click event handler directly calls
restartEngine() without checking for active runs, which can silently abort
in-flight operations. Instead of calling restartEngine() directly after the
setSecret calls succeed, route the restart through the same active-run guard
used elsewhere in this file by checking activeRunCount() first and prompting the
user for confirmation if there are active runs before proceeding with the engine
restart. Apply this same guarded restart pattern to all locations in this
section where restartEngine() is invoked.
In `@desktop/src/settings.ts`:
- Line 160: The STORAGE_KEY constant at line 160 in desktop/src/settings.ts was
changed to v2 without providing a migration path for existing users, causing
them to lose their saved settings. In the loadSettings() function at lines
231-245, add a fallback mechanism that first attempts to read settings from the
legacy storage key (the previous version before v2), then if found, applies any
necessary coercion to that legacy payload, and finally saves the migrated
settings under the new STORAGE_KEY before returning. This ensures existing
users' settings are preserved and properly migrated when they upgrade.
In `@src/notify/telegram.ts`:
- Around line 168-171: The offset is being updated before the update is handled,
so if handleUpdate throws an exception, that update is still acknowledged and
won't be retried. Move the offset update statement (currently at the beginning
of the loop in the for loop iterating over updates) to after the handleUpdate
call completes successfully. Specifically, move the line that sets this.offset
to after the if statement that calls handleUpdate, ensuring the offset only
advances when the update has been successfully handled by the handleUpdate
method.
In `@src/runners/responsesRunner.ts`:
- Around line 262-268: The body object construction in responsesRunner.ts
spreads extraBody after defining the required model and input fields, allowing
extraBody to silently override these core request parameters. Reorder the object
properties so that model and input are defined after spreading extraBody,
ensuring the required fields cannot be overridden by configuration values and
the actual prompt and model used match req.prompt and this.profile.model
respectively.
---
Outside diff comments:
In `@desktop/src/main.ts`:
- Around line 352-366: The issue is that when `detectCommands()` throws an
exception, the `installed` variable remains as an empty object, but this is then
incorrectly interpreted as "all CLI tools are missing" by `commandMissing()` and
the detected-tools badges, causing false warnings. The fix is to track whether
the probe succeeded separately (similar to how `knowKeys` tracks success for
`listSecretKeys()`). Create a new boolean variable (like `probeSucceeded` or
`cliToolsDetected`) that is only set to true when `detectCommands()` completes
without throwing. Then update all references to `installed` in
`commandMissing()` and the detected-tools badge logic to only treat commands as
missing if the probe actually succeeded. Apply this same pattern to the other
affected locations at lines 369-385 and 728-734 where similar probe-and-track
patterns exist with different data (listSecretKeys, etc.).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a7f9faf-6609-426a-9c34-edc5ac8d9142
📒 Files selected for processing (12)
desktop/src-tauri/Cargo.tomldesktop/src-tauri/src/repo.rsdesktop/src/main.tsdesktop/src/settings.tssrc/notify/telegram.tssrc/runners/agentRunner.tssrc/runners/responsesRunner.tssrc/runners/runnerFactory.tssrc/server/index.tssrc/server/server.tstest/responsesRunner.test.tstest/telegram.test.ts
- responsesRunner: spread extraBody before model/input so config can't override the core request fields. - telegram: advance the getUpdates offset only after a successful handleUpdate, so a throwing handler leaves the update unacked for retry instead of dropping it. - settings: migrate legacy v1 localStorage on upgrade (provider->preset coercion + carry-over of repo/run/publishing prefs) and resave under v2, instead of silently discarding saved settings. - desktop Telegram card: require BOTH secrets before saving, refresh the cached stored-state from the keychain after save/remove (no optimistic 'stored'), and route the engine restart through a shared active-run guard (restartEngineGuarded) so it warns before aborting in-flight runs.
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.
This pull request was created by @kiro-agent on behalf of @inhaq 👻
Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro Web
Summary
Four related changes that move the app toward "configure once, run from anywhere":
1. Fix the repo-picker freeze (the bug)
pick_directorywas a synchronous Tauri command, so it ran on the main UI thread and calledblocking_pick_folder(). The native dialog needs that same thread's event loop to run, so the thread deadlocked waiting for a dialog that could never appear — the app froze on every repo selection.Fix: make
pick_directoryasyncand use the non-blockingpick_folder+ aoneshotchannel (the dialog runs on the main thread; the handler awaits off it).check_git_repoandwhich_commandsare made async too so their git/PATH probes never stall the UI. Adds the tokiosyncfeature.2. Presets-only settings
Replaces the provider/model matrix with three first-class runner presets:
codex exec --json --model <model>(edits files)gpt-5.5kiro-cli chat --no-interactive --trust-tools=…withKIRO_API_KEYThe settings page picks a preset per role (writer = actor, reviewer = critic) with an editable model id (default
gpt-5.5where configurable), shows auth status (command installed / key stored), and keeps raw runner JSON behind the Advanced escape hatch. Adds a configurable Kiro trusted-tools field.3. OpenAI Responses runner (new transport)
New
ResponsesRunner(kind: "http-responses") targetsPOST /v1/responses(input+instructions→output_text), distinct from the Chat-CompletionsHttpRunner. Wired into the factory and profile schema.4. Telegram notify + control relay (loopback-safe)
A private phone-updates layer: pushes final run status to one allowlisted chat and long-polls for replies, treating each as a new goal it submits as a run (reusing the last run's presets + repo). Outbound-only to api.telegram.org — the engine stays loopback-only with no inbound exposure. Configurable from the desktop settings (token + chat id stored as secrets).
Testing
npm run typecheck(engine) andtsc --noEmit(desktop): cleannpm test: 197 passing (+24 new — Responses runner and Telegram relay)repo.rsvalidated with rustfmtKnown limitations / notes for review
webkit2gtk-4.1), so please build/run the desktop app locally to confirm the picker now opens and the Telegram card works. The Rust change is the canonical Tauri v2 async-picker pattern and parses cleanly under rustfmt.gpt-5.5is set as the default model id but is fully editable in settings — change it without touching code if the id differs.Summary by CodeRabbit