diff --git a/.github/workflows/pages.yml b/.github/workflows/pages.yml index 2fc91f60..d818ab5a 100644 --- a/.github/workflows/pages.yml +++ b/.github/workflows/pages.yml @@ -2,17 +2,23 @@ name: Deploy Website (GitHub Pages) # Brand website (website/, VitePress). Decoupled from the v* release pipeline: # build on every PR touching the site/docs (as a check), deploy on push to master. +# CHANGELOG.md / CHANGELOG.zh-CN.md are included because the site changelog is synced from them (see +# website/scripts/sync-docs.mjs) — a release merges the changelog to master and must rebuild the site. on: push: branches: [master] paths: - 'website/**' - 'docs/**' + - 'CHANGELOG.md' + - 'CHANGELOG.zh-CN.md' - '.github/workflows/pages.yml' pull_request: paths: - 'website/**' - 'docs/**' + - 'CHANGELOG.md' + - 'CHANGELOG.zh-CN.md' - '.github/workflows/pages.yml' workflow_dispatch: diff --git a/CHANGELOG.md b/CHANGELOG.md index a487e0b3..13338a97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,33 @@ All notable changes to this project are recorded here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and the versioning follows [Semantic Versioning](https://semver.org/). +## [0.12.0] - 2026-09-09 + +### ✨ Added + +- A comment can now be handed to the agent to investigate: click the reference button on any comment and ask about it, and the agent gets the comment, its author and the code it points at, then checks the claim against the actual code. Its answer can be turned into a draft reply to that comment in one click, editable before it goes out. +- The code editor is now Monaco 0.56.0 (from 0.55.1), which also drops the outdated sanitizer copy behind most of the open dependency advisories. +- The bundled review engine is now pr-agent 0.45.0 (from 0.39.0). Two long-standing local fixes are no longer needed — a single-line file change is rendered correctly upstream now, and a binary file no longer has to be worked around — and its YAML handling is more tolerant of imperfect model output. +- Mentions now render as a pill instead of blending into the surrounding text, so it is obvious at a glance when someone is named — on the activity page, in the inline diff comments, in drafts, and in the PR description alike. +- Proxy settings now take a list of **direct connections**: hosts that bypass the proxy and connect straight out, so an internal code platform, its git remote, or a self-hosted model stays reachable while everything else still goes through the proxy. Uses the familiar `NO_PROXY` syntax (a domain covers its subdomains), and applies to every outbound path at once — REST, git and the LLM call. +- A review that fails because the model is unavailable now says so and tells you what to do — with a local CLI provider (claude / codex) the model comes from that CLI's own configuration, so it has to be changed there. + +### 🔧 Fixed + +- A generated PR description no longer ends with a blank card — an invisible marker the review engine now stamps into its output was being treated as a section of its own. +- Diagrams in a generated PR description are laid out left-to-right again, instead of turning tall and top-down once they get past a few steps and pushing the rest of the result off screen. +- The commit divider in the review timeline now stays where the commit boundary actually is, instead of sliding further down every time a new message appears below it. +- The file list in a generated PR description now shows the real number of added and removed lines per file, instead of `+-1/--1`. +- Links in that file list now open the file instead of pointing at a non-existent line, so clicking through works. +- A PR description ending in a git merge tail (`# Conflicts:` and the file lines under it) no longer breaks up the generated description — those lines are part of the quoted description, not headings of their own. +- A part of the interface that loads on demand — the diff editor, a comment's inline code context — no longer takes the whole app down with it when it fails to load; the failure now stays inside that pane. If it failed because the app was updated or rebuilt while the window was open, it says so and offers to reload, which is the only thing that actually helps in that case. +- A failed review now shows the provider's actual error instead of only "all fallback models failed" — the real cause (an unavailable model, an expired login, an exhausted quota) was previously swallowed and never reached the run card. +- A local CLI provider that exits successfully but returns an empty reply is now reported as a failure naming that cause, rather than as an unexplained LLM failure. +- A merged PR now leaves the list on its own shortly after you merge it, instead of lingering until the next periodic sync — the remote takes a few seconds to actually mark it merged, and the app now waits for that rather than refreshing too early and finding nothing changed. +- Approving a PR now updates whether it can be merged, so the merge button appears as soon as your approval satisfies the last requirement — previously it stayed hidden until the next periodic sync, because the remote recomputes mergeability only after the approval returns. +- The window no longer goes permanently black when the interface fails: a crash now shows what went wrong with a retry / reload, a failure during startup shows a recovery screen, and a renderer that dies outright is reloaded automatically. Crash details are also written to the application log, which they previously never were. +- Switching to another PR while a merge is still in flight no longer clears the PR you just opened. + ## [0.11.2] - 2026-07-28 > Highlights of this release: @@ -514,6 +541,7 @@ and the versioning follows [Semantic Versioning](https://semver.org/). License: [Apache-2.0](LICENSE). The package bundles third-party components (pr-agent, Electron, etc.), each distributed under its own license, see [NOTICE](NOTICE). [Unreleased]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.1...HEAD +[0.12.0]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.2...v0.12.0 [0.11.2]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.1...v0.11.2 [0.11.1]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.0...v0.11.1 [0.11.0]: https://github.com/huhamhire/code-meeseeks/compare/v0.10.0...v0.11.0 diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index 7b3a085e..63fdb30c 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -5,6 +5,33 @@ 本项目所有重要变更记录于此。格式参考 [Keep a Changelog](https://keepachangelog.com/zh-CN/1.1.0/)， 版本号遵循 [语义化版本](https://semver.org/lang/zh-CN/)。 +## [0.12.0] - 2026-09-09 + +### ✨ 新增 + +- 现在可以把评论交给 Agent 调查：在任意评论上点击引用按钮再提问，Agent 会拿到评论原文、作者及其指向的代码，对照真实代码核查该说法是否成立。得到的结论可一键转成针对该评论的回复草稿，发出前仍可编辑。 +- 代码编辑器升级至 Monaco 0.56.0（原 0.55.1），同时替换掉了此前多数依赖安全告警所指向的过时清理库副本。 +- 内置评审引擎升级至 pr-agent 0.45.0（原 0.39.0）。两处长期存在的本地修补不再需要——单行文件变更在上游已渲染正确，二进制文件也无需再绕开——其 YAML 解析对不规范的模型输出也更宽容。 +- @提及 改为胶囊标签展示，不再淹没在正文里，一眼即可看出点到了谁——活动页、内联 diff 评论、草稿与 PR 描述一致生效。 +- 代理设置新增**直连地址**列表：列出的地址跳过代理直接连接，内网代码平台、它的 git 远端或自建模型服务因此保持可达，其余流量照常走代理。沿用通行的 `NO_PROXY` 写法（填域名同时覆盖子域），并对所有出站路径一并生效——REST、git 与 LLM 调用。 +- 因模型不可用而失败的评审现在会明确说明，并给出处理方式——使用本地 CLI 供应商（claude / codex）时，模型来自该 CLI 自身的配置，需要在那里更换。 + +### 🔧 修复 + +- 生成的 PR 描述末尾不再出现空白卡片——评审引擎新加入输出的一个不可见标记此前被当成了独立分块。 +- 生成的 PR 描述中的图表恢复为从左至右布局，不再在步骤稍多时变成纵向长图、把其余内容挤出屏幕。 +- 评审时间线中的提交分割线现在固定在提交边界的真实位置，不再因为下方出现新消息而一路下移。 +- 生成的 PR 描述中，文件清单现在显示每个文件真实的增删行数，不再是 `+-1/--1`。 +- 该清单中的链接现在指向文件本身，而非一个不存在的行号，点击可正常跳转。 +- PR 描述末尾带有 git 合并残留（`# Conflicts:` 及其下的文件行）时，生成的描述不再被切碎——那些行属于被引用的描述正文，而不是标题。 +- 按需加载的界面部分——diff 编辑器、评论中的内联代码上下文——加载失败时不再拖垮整个应用，失败被限制在该区域内。若失败原因是窗口开着时应用被更新或重新构建，会明确说明并提供重新加载，那也是这种情况下唯一有效的操作。 +- 评审失败时现在会展示供应商返回的真实错误，而不再只有一句「所有备选模型均调用失败」——真正的原因（模型不可用、登录过期、额度耗尽）此前被吞掉，从未出现在运行卡片上。 +- 本地 CLI 供应商正常退出却返回空回复时，现在会作为失败上报并指明该原因，而不再表现为一次无从解释的 LLM 调用失败。 +- 合并 PR 后，该 PR 会在稍后自动从列表中消失，而不再滞留到下一次周期同步——远端需要几秒才真正标记为已合并，应用现在会等待这一刻，而不是过早刷新、结果什么都没变。 +- 批准 PR 后会重新判断其是否可合并，当你的批准满足最后一项要求时合并按钮即刻出现——此前它会一直隐藏到下一次周期同步，因为远端要在批准返回之后才重新计算可合并性。 +- 界面出错时窗口不再永久黑屏：渲染崩溃会显示出错内容并提供重试／重新加载，启动阶段失败会显示恢复界面，渲染进程整个崩溃则自动重新加载。崩溃详情同时写入应用日志——此前从不记录。 +- 合并请求仍在进行时切换到其他 PR，不再清空你刚打开的那个 PR。 + ## [0.11.2] - 2026-07-28 > 本次发布要点： @@ -514,6 +541,7 @@ 许可证：[Apache-2.0](LICENSE)。打包内含第三方组件（pr-agent、Electron 等），各按其许可证分发，见 [NOTICE](NOTICE)。 [Unreleased]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.1...HEAD +[0.12.0]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.2...v0.12.0 [0.11.2]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.1...v0.11.2 [0.11.1]: https://github.com/huhamhire/code-meeseeks/compare/v0.11.0...v0.11.1 [0.11.0]: https://github.com/huhamhire/code-meeseeks/compare/v0.10.0...v0.11.0 diff --git a/apps/desktop/package.json b/apps/desktop/package.json index e5e8f328..74370c9c 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "@meebox/desktop", - "version": "0.11.2", + "version": "0.12.0", "private": true, "description": "meebox Electron desktop app", "author": { @@ -81,7 +81,7 @@ "i18next": "^26.3.1", "i18next-resources-to-backend": "^1.2.1", "mermaid": "^11.4.0", - "monaco-editor": "^0.55.0", + "monaco-editor": "^0.56.0", "pino": "^9.5.0", "pino-roll": "^3.0.0", "react": "^19.2.0", diff --git a/apps/desktop/scripts/pragent-runtime.json b/apps/desktop/scripts/pragent-runtime.json index 0216b6ef..99f495d7 100644 --- a/apps/desktop/scripts/pragent-runtime.json +++ b/apps/desktop/scripts/pragent-runtime.json @@ -8,6 +8,6 @@ }, "prAgent": { "_comment": "嵌入式运行时安装的 pr-agent 版本（pinned）；须与 sitecustomize.py 的 _EXPECTED_PRAGENT_VERSION 对齐，升级时两处同步", - "version": "0.39.0" + "version": "0.45.0" } } diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py index 9a57b442..061d8203 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py @@ -10,7 +10,6 @@ applied it silently degrades, never letting a shim exception block the flow. """ from .patches.describe_assessment import patch as _patch_describe_assessment -from .patches.git_patch_processing import patch as _patch_git_patch_processing from .patches.litellm_handler import patch as _patch_litellm_handler from .patches.load_yaml import patch as _patch_load_yaml from .patches.local_git_provider import patch as _patch_local_git_provider @@ -18,9 +17,8 @@ def apply() -> None: - # local_git_provider two patches merged into one patch_fn (registering multiple finders for the - # same module shadows each other; only the meta_path[0] one takes effect): binary-safe - # get_diff_files + get_line_link anchor. + # local_git_provider: inject get_line_link (the structured /review anchor) + get_repo_file_content. Registered as a + # single patch_fn because multiple finders on one module shadow each other (only meta_path[0] takes effect). _register_post_import( "pr_agent.git_providers.local_git_provider", _patch_local_git_provider, @@ -40,11 +38,4 @@ def apply() -> None: "pr_agent.tools.pr_description", _patch_describe_assessment, ) - # git patch processing: fix the phantom "unchanged" line on single-line hunks (omitted hunk size defaulted to 0 - # instead of 1), which made single-line file changes look like they contained both the old and new value. Self- - # disabling once upstream fixes it — see patches/git_patch_processing.py. - _register_post_import( - "pr_agent.algo.git_patch_processing", - _patch_git_patch_processing, - ) _debug("meebox shim loaded") diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py index 8d4c3e09..caa0e91b 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py @@ -11,7 +11,7 @@ import sys from ..runtime import _debug, strip_cache_break -from ..usage import _emit_usage_tokens +from ..usage import _emit_llm_error, _emit_usage_tokens from .specs import _CLI_SPECS @@ -88,12 +88,18 @@ async def run_cli_chat(bin_name, system, user) -> str: except Exception as exc: # noqa: BLE001 raise RuntimeError(f"failed to start CLI '{bin_name}': {exc}") from exc out, err = await proc.communicate(prompt.encode("utf-8")) + out_text = (out or b"").decode("utf-8", "replace") if proc.returncode != 0: - raise RuntimeError( - f"CLI '{bin_name}' exit code {proc.returncode}: " - f"{(err or b'').decode('utf-8', 'replace')[:500]}" - ) - text, usage = spec["parser"]((out or b"").decode("utf-8", "replace")) + # Error source: some CLIs report the failure only on **stdout** (codex writes turn.failed / error events into its + # JSONL stream and leaves stderr empty), which would otherwise surface as an empty reason and lose the real cause + # (auth / model unavailable / quota). When the spec provides an extractor, take stdout's verdict first and fall + # back to stderr; either way keep the raise contract (pr-agent turns it into "all fallback models failed"). + extractor = spec.get("error_extractor") + detail = (extractor(out_text) if extractor else None) or (err or b"").decode("utf-8", "replace") + detail = (detail or "")[:500] + _emit_llm_error(bin_name, detail) + raise RuntimeError(f"CLI '{bin_name}' exit code {proc.returncode}: {detail}") + text, usage = spec["parser"](out_text) if usage: # prompt_tokens ≈ total input-side size, output_tokens ≈ completion (input/output_tokens share the same names across both). # The cache fields differ in convention between the two: @@ -115,6 +121,18 @@ async def run_cli_chat(bin_name, system, user) -> str: cache_read_tokens=cache_read if isinstance(cache_read, int) else None, turns=turns if isinstance(turns, int) else None, ) + # Empty reply on a zero exit code: the CLI ran to completion but produced no assistant message (codex does this when a turn + # dies on a tool/feature failure, e.g. Code Mode failing closed because its host binary is missing). Downstream this became + # an empty prompt into pr-agent's load_yaml → the opaque "all fallback models failed"; raise here instead, carrying any error + # event the stream did report, so the run card names the actual cause. Usage is emitted first, keeping the token count truthful. + if not (text or "").strip(): + extractor = spec.get("error_extractor") + detail = (extractor(out_text) if extractor else None) or "" + msg = f"CLI '{bin_name}' returned an empty reply" + ( + f": {detail[:500]}" if detail else " (no error reported in its output)" + ) + _emit_llm_error(bin_name, msg) + raise RuntimeError(msg) return text diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py index 5dc7b27c..7e504720 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py @@ -71,3 +71,45 @@ def _parse_codex_output(stdout): if isinstance(usage, dict) and turns: usage["num_turns"] = turns return (text if isinstance(text, str) else ""), usage + + +def _extract_codex_error(stdout): + """Pull the failure cause out of the `codex exec --json` event stream, returning a message string or None. + + codex reports failures on **stdout** (the JSONL stream) and leaves stderr empty, so without this the caller would + raise with an empty detail and the real cause (auth, model 404, quota ...) would never reach the logs. Precedence: + - `turn.failed` → error.message is the terminal verdict, most accurate; + - otherwise the last `type=="error"` event (the intermediate retry notices carry the same upstream message); + - otherwise the last `item.completed` whose item.type=="error" (startup-stage errors, e.g. an unavailable feature host). + """ + import json + + turn_failed = None + last_error = None + last_item_error = None + for line in (stdout or "").splitlines(): + line = line.strip() + if not line: + continue + try: + ev = json.loads(line) + except Exception: # noqa: BLE001 - skip non-JSON lines (logs, etc.) + continue + if not isinstance(ev, dict): + continue + etype = ev.get("type") + if etype == "turn.failed": + err = ev.get("error") + if isinstance(err, dict) and isinstance(err.get("message"), str): + turn_failed = err["message"] + elif etype == "error": + msg = ev.get("message") + if isinstance(msg, str): + last_error = msg + elif etype == "item.completed": + item = ev.get("item") + if isinstance(item, dict) and item.get("type") == "error": + msg = item.get("message") + if isinstance(msg, str): + last_item_error = msg + return turn_failed or last_error or last_item_error diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py index 8145fd61..9e0ef3af 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py @@ -1,6 +1,6 @@ """Spec table for adapted local CLI commands: argv flags (the prompt always goes via stdin) + output parser + billing env to strip. Registering one entry here is enough for a new command; the renderer-side whitelist validation must stay in sync (see LlmProfileForm.validateProfile).""" -from .parsers import _parse_claude_output, _parse_codex_output +from .parsers import _extract_codex_error, _parse_claude_output, _parse_codex_output # `low_effort_flags`: argv to append for the low-effort tier (only enabled by the Agent orchestration channel via MEEBOX_CLI_REASONING, # see install.py). Commands with a trailing `-` (stdin) insert these flags before the `-`, keeping `-` last. @@ -30,6 +30,9 @@ ], "low_effort_flags": ["-c", "model_reasoning_effort=low"], "parser": _parse_codex_output, + # codex writes its failures into the stdout event stream and leaves stderr empty, so a non-zero exit needs + # the cause extracted from stdout (see install.py); commands without this key just fall back to stderr. + "error_extractor": _extract_codex_error, "strip_env": ("OPENAI_API_KEY", "CODEX_API_KEY"), }, } diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py deleted file mode 100644 index cfe29630..00000000 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py +++ /dev/null @@ -1,88 +0,0 @@ -"""git_patch_processing patch (version-guarded, self-disabling): fix phantom "unchanged" line on single-line hunks. - -=============================================================================================================== -MAINTENANCE NOTE — remove this shim once pr-agent fixes the bug upstream. ---------------------------------------------------------------------------------------------------------------- -Bug: pr-agent renders a single-line file change (e.g. VERSION `1.5.2` -> `1.5.3`, a hash/checksum file, any -hunk whose OLD side is exactly one line) so the model sees the OLD value as a still-present, unchanged line — -it then wrongly reports "the file now contains both the old and new value". - -Root cause (in pr_agent/algo/git_patch_processing.py): - 1. git omits the hunk line-count when it is 1, so a single-line change emits `@@ -1 +1 @@` - (which per the unified-diff spec means `@@ -1,1 +1,1 @@`). - 2. `extract_hunk_headers` coerces every missing regex group to 0, so the omitted size becomes size1=0 - (it should be 1). - 3. `process_patch_lines`' trailing-context slice `file_original_lines[start1 + size1 - 1 : ...]` then reads - from `1 + 0 - 1 = 0` — i.e. re-reads the CHANGED line itself — and appends it as a phantom unchanged - context line, which `decouple_and_convert_to_hunks_with_lines_numbers` places into `__new hunk__`. - -Fix: default an OMITTED hunk size to 1 (spec-correct) instead of 0, keeping an EXPLICIT 0 (e.g. `@@ -0,0 +1 @@` -new-file case) as 0. This makes the trailing-context slice start after the hunk, so no phantom line is emitted. - -Upstream status (re-checked at pr-agent 0.39.0): still present — `extract_hunk_headers` is byte-identical to -0.36.0 and still coerces the omitted size to 0; no upstream issue/PR addresses it (PRs #2322 / #2330 guard a -DIFFERENT None — a fully malformed `@@` line where `match` itself is None — not the size default). - -Removal / upgrade checklist: - * This patch is BOTH version-guarded (only applies to the pinned _EXPECTED_PRAGENT_VERSION) AND self-disabling - (it probes the live `extract_hunk_headers`; if upstream already returns size1=1 for `@@ -1 +1 @@`, it does - nothing). So on a pr-agent bump, re-verify: if the probe reports the bug is gone, DELETE this file and its - registration in `../__init__` apply(); if it persists, bump _EXPECTED_PRAGENT_VERSION and keep it. - * Reproduce quickly against the vendored runtime with `MEEBOX_SHIM_DEBUG=1` (the probe logs which branch it took). -=============================================================================================================== -""" -from ..runtime import _EXPECTED_PRAGENT_VERSION, _debug, _pragent_version, _warn - - -def patch(module) -> None: - # Version guard: this reimplementation mirrors the pinned pr-agent's extract_hunk_headers structure; on a - # version mismatch skip rather than risk a wrong reimplementation (safe degradation, consistent with the - # other shim patches). The warning is the "pay attention on upgrade" signal. - installed = _pragent_version() - if installed != _EXPECTED_PRAGENT_VERSION: - _warn( - f"pr-agent {installed} does not match the {_EXPECTED_PRAGENT_VERSION} that the meebox patch is " - "adapted for; git_patch_processing patch skipped (single-line-hunk phantom-line fix disabled). If " - "this is an intentional upgrade, sync runtime.py's _EXPECTED_PRAGENT_VERSION + pragent-runtime.json, " - "re-verify whether upstream has fixed the omitted-hunk-size bug, and remove this shim if so." - ) - return - - orig_extract = module.extract_hunk_headers - re_hunk_header = module.RE_HUNK_HEADER - - # Self-disable if upstream already fixed it: git omits the size for `@@ -1 +1 @@`, so a correct implementation - # yields size1 == 1. If the live function already returns a non-zero size here, the bug is gone — do nothing. - probe = re_hunk_header.match("@@ -1 +1 @@") - try: - _, probe_size1, _, _, _ = orig_extract(probe) - except Exception as exc: # noqa: BLE001 - unexpected upstream signature change → skip, don't crash the run - _debug(f"git_patch_processing: extract_hunk_headers probe failed ({exc}); patch skipped") - return - if probe_size1 != 0: - _debug( - "git_patch_processing: upstream already defaults an omitted hunk size correctly " - "(probe size1=%r); phantom-line shim skipped — safe to delete this file" % probe_size1 - ) - return - - def extract_hunk_headers(match): - # Faithful mirror of upstream extract_hunk_headers, with ONE correction: an omitted hunk line-count - # (regex group is None) defaults to 1 per the unified-diff spec — git drops ",N" only when N == 1 — - # instead of upstream's blanket 0. An EXPLICIT 0 (present in the text, e.g. the `@@ -0,0 +1 @@` new-file - # case) is untouched because its group is "0", not None. Only the two size groups (indices 1 and 3) get - # the size default; any other missing group keeps the original 0 fallback. - res = list(match.groups()) - for i in range(len(res)): - if res[i] is None: - res[i] = 1 if i in (1, 3) else 0 - try: - start1, size1, start2, size2 = map(int, res[:4]) - except (ValueError, TypeError): # '@@ -0,0 +1 @@' case (mirrors upstream's bare except) - start1, size1, size2 = map(int, res[:3]) - start2 = 0 - section_header = res[4] - return section_header, size1, size2, start1, start2 - - module.extract_hunk_headers = extract_hunk_headers - _debug("git_patch_processing: applied single-line-hunk phantom-line fix (omitted hunk size -> 1)") diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py index 62d153ac..9c88511f 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py @@ -1,72 +1,23 @@ -"""LocalGitProvider patch (version-guarded): binary-safe get_diff_files + get_line_link anchor + repo-context file fetch.""" +"""LocalGitProvider patch (version-guarded): get_line_link anchor + dirty-tolerant repo prep + repo-context file fetch.""" from ..runtime import _EXPECTED_PRAGENT_VERSION, _pragent_version, _warn def patch(module) -> None: - """LocalGitProvider.get_diff_files blindly .decode('utf-8') on every diff file, and on a binary - file (images / build artifacts / UTF-16 etc., e.g. starting with 0xff) throws UnicodeDecodeError, crashing the whole review. - Replace with a binary-safe version: files that fail to decode are skipped (review doesn't handle binaries), the rest of the logic identical to upstream.""" - # version guard: only patch the pinned pr-agent version; on mismatch skip the whole group (including get_line_link). + """Inject the methods LocalGitProvider does not implement: the structured /review anchor (get_line_link), + a dirty-tolerant _prepare_repo, labels, and repo-context file reads. + + Binary-safe get_diff_files used to live here too; pr-agent 0.45.0 fixed it upstream (it now skips a file that + fails to decode, with a warning), so that override is gone — one less thing to keep in step with upstream. + """ + # version guard: only patch the pinned pr-agent version; on mismatch skip the whole group. installed = _pragent_version() if installed != _EXPECTED_PRAGENT_VERSION: _warn( f"pr-agent {installed} does not match the {_EXPECTED_PRAGENT_VERSION} that the meebox patch is adapted for; " - "patches skipped (/review line-number anchoring and binary-safe diff disabled). If this is an intentional upgrade, sync " + "patches skipped (/review line-number anchoring disabled). If this is an intentional upgrade, sync " "runtime.py's _EXPECTED_PRAGENT_VERSION + pragent-runtime.json and re-verify." ) return - from pr_agent.algo.types import EDIT_TYPE, FilePatchInfo - - def get_diff_files(self): - diffs = self.repo.head.commit.diff( - self.repo.merge_base(self.repo.head, self.repo.branches[self.target_branch_name]), - create_patch=True, - R=True, - ) - diff_files = [] - for diff_item in diffs: - try: - original_file_content_str = ( - diff_item.a_blob.data_stream.read().decode("utf-8") - if diff_item.a_blob is not None - else "" - ) - new_file_content_str = ( - diff_item.b_blob.data_stream.read().decode("utf-8") - if diff_item.b_blob is not None - else "" - ) - patch_str = diff_item.diff.decode("utf-8") - except (UnicodeDecodeError, ValueError): - # binary file can't be utf-8 decoded → skip this file - continue - edit_type = EDIT_TYPE.MODIFIED - if diff_item.new_file: - edit_type = EDIT_TYPE.ADDED - elif diff_item.deleted_file: - edit_type = EDIT_TYPE.DELETED - elif diff_item.renamed_file: - edit_type = EDIT_TYPE.RENAMED - diff_files.append( - FilePatchInfo( - original_file_content_str, - new_file_content_str, - patch_str, - # a deleted file's b_path is None → FilePatchInfo.filename=None, and downstream - # set_file_languages / extract_relevant_lines_str's filename.rsplit/strip - # would crash, and one crash interrupts the whole review's line-snippet extraction (even findings for non-deleted files lose - # code snippets). Fall back to a_path to guarantee filename is never None. - diff_item.b_path or diff_item.a_path, - edit_type=edit_type, - old_filename=None - if diff_item.a_path == diff_item.b_path - else diff_item.a_path, - ) - ) - self.diff_files = diff_files - return diff_files - - module.LocalGitProvider.get_diff_files = get_diff_files # _prepare_repo: upstream throws "repository is not in a clean state" when repo.is_dirty(). For CLI-mode # /ask worktrees we sanitize as needed — truncating the repo's own agent instruction files (CLAUDE.md / AGENTS.md / .cursor rules @@ -87,18 +38,51 @@ def _prepare_repo(self): # and parse-output derives the structured anchor from the link (same source as real providers, not dependent on the model self-reporting a marker). from urllib.parse import quote + def _line_no(value): + """A usable 1-based line number, or 0 for "no specific line".""" + try: + n = int(value) + except (TypeError, ValueError): + return 0 + return n if n > 0 else 0 + def get_line_link(self, relevant_file, relevant_line_start, relevant_line_end=None): f = quote((relevant_file or "").lstrip("/"), safe="/") if not f: return "" - if not relevant_line_start: + # `-1` is upstream's "whole file, no particular line" (pr_description passes it for every File Walkthrough + # row). It is truthy, so a plain falsiness check let it through and produced `#L-1` — a fragment the app then + # parsed into a bogus anchor at line -1. Anything that is not a positive line number yields a file-level link. + start = _line_no(relevant_line_start) + if not start: return f"meebox:///{f}" - if relevant_line_end and relevant_line_end != relevant_line_start: - return f"meebox:///{f}#L{relevant_line_start}-L{relevant_line_end}" - return f"meebox:///{f}#L{relevant_line_start}" + end = _line_no(relevant_line_end) + if end and end != start: + return f"meebox:///{f}#L{start}-L{end}" + return f"meebox:///{f}#L{start}" module.LocalGitProvider.get_line_link = get_line_link + # num_plus_lines / num_minus_lines: FilePatchInfo defaults both to -1, and only the real platform providers fill + # them in — LocalGitProvider does not, so /describe's File Walkthrough rendered every row as "+-1/--1". Wrap + # rather than reimplement get_diff_files: upstream owns how the diff is produced (0.45.0 also made it binary-safe), + # and this only backfills two derived counters, computed the same way the platform providers do. + _orig_get_diff_files = module.LocalGitProvider.get_diff_files + + def get_diff_files(self): + files = _orig_get_diff_files(self) + for f in files or []: + if getattr(f, "num_plus_lines", -1) >= 0: + continue # already counted (a future upstream that fills them in wins) + lines = (getattr(f, "patch", None) or "").splitlines() + # Count the same way the platform providers do: any line starting with +/-, which includes the + # `+++`/`---` file headers. Kept identical rather than "corrected" so the numbers agree across providers. + f.num_plus_lines = len([ln for ln in lines if ln.startswith("+")]) + f.num_minus_lines = len([ln for ln in lines if ln.startswith("-")]) + return files + + module.LocalGitProvider.get_diff_files = get_diff_files + # uniformly enable GFM: LocalGitProvider defaults to False for 'gfm_markdown', causing /describe's # enable_pr_diagram (on by default in configuration.toml) to be gated off by `enable and is_supported(gfm_markdown)`, # not producing the mermaid architecture diagram; /review etc. also take the non-GFM simplified branch. Here we make gfm_markdown diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py index 5e5ea899..998e4ccb 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py @@ -15,7 +15,7 @@ # skipped (safe degradation, preferring to apply fewer patches over applying wrong ones). # When upgrading pr-agent: sync this constant + prAgent.version in scripts/pragent-runtime.json # (the assemble script verifies the two match and extracts this constant from this file), and re-verify patch behavior. -_EXPECTED_PRAGENT_VERSION = "0.39.0" +_EXPECTED_PRAGENT_VERSION = "0.45.0" # System context "cache break" marker: assembleSystemContext (TS, packages/agent/src/assemble.ts) inserts this string # (along with the --- separators on both sides) between the **globally stable prefix** (SOUL/AGENTS/tool directory/memory/user diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py index bccf7e6b..74b45105 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py @@ -1,5 +1,10 @@ """Real token usage collection: emitted to stderr as a sentinel line `@@MEEBOX_USAGE@@ {json}`, which the main process onLine -accumulates from (see apps/desktop/src/main/ipc.ts). Takes only tokens, not cost. Fault-tolerant throughout.""" +accumulates from (see apps/desktop/src/main/ipc.ts). Takes only tokens, not cost. Fault-tolerant throughout. + +The same stderr sentinel channel also carries `@@MEEBOX_LLM_ERROR@@ {json}` (see _emit_llm_error): pr-agent's +retry_with_fallback_models logs the underlying exception into loguru's `artifact=` field, which the default format never +prints, so a failing LLM call reaches stdout as nothing but "Failed to generate prediction with any model". The sentinel +carries the real cause out of band, past that lossy log line.""" import sys from .runtime import _debug @@ -67,3 +72,22 @@ def _emit_usage_tokens( print(f"@@MEEBOX_USAGE@@ {json.dumps(rec)}", file=sys.stderr, flush=True) except Exception as exc: # noqa: BLE001 _debug(f"emit cli usage failed (ignored): {exc}") + + +def _emit_llm_error(cli, message) -> None: + """Emit the real cause of a failed LLM call to stderr as `@@MEEBOX_LLM_ERROR@@ {json}`, right before raising. + + Emitting is purely additive — the exception is still raised and pr-agent still runs its fallback retry; this only + keeps the cause from being swallowed by that retry's log line, so the run card can show what actually went wrong + (and classify it, e.g. an unavailable model) instead of the generic "all fallback models failed". + """ + try: + import json + + print( + f"@@MEEBOX_LLM_ERROR@@ {json.dumps({'cli': cli, 'message': message})}", + file=sys.stderr, + flush=True, + ) + except Exception as exc: # noqa: BLE001 + _debug(f"emit llm error failed (ignored): {exc}") diff --git a/apps/desktop/src/main/bootstrap/window-manager.ts b/apps/desktop/src/main/bootstrap/window-manager.ts index 3cbb56da..f1d94c6c 100644 --- a/apps/desktop/src/main/bootstrap/window-manager.ts +++ b/apps/desktop/src/main/bootstrap/window-manager.ts @@ -18,6 +18,12 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url)); const DEFAULT_SIZE = { width: 1280, height: 800 }; const MIN_SIZE = { width: 960, height: 600 }; +/** + * How many times a window may auto-reload after a renderer crash / load failure before giving up. Bounded so a page + * that dies on load can't spin in a reload loop; the counter resets on any successful load (see installCrashRecovery). + */ +const MAX_CRASH_RELOADS = 3; + // Colors of the system window-control buttons on the right of the self-drawn title bar (Windows // titleBarOverlay): same color as the .app-titlebar background (--bg-app), so the seam is invisible; // the symbol takes the primary text color. Dark/light sets, following the effective theme @@ -204,12 +210,67 @@ export class WindowManager { return { action: 'deny' }; }); + this.installCrashRecovery(win); + if (process.env.ELECTRON_RENDERER_URL) { void win.loadURL(process.env.ELECTRON_RENDERER_URL); } else { void win.loadFile(path.join(__dirname, '../renderer/index.html')); } } + + /** + * Recover the window from renderer-level failures that the React error boundary cannot see. + * + * A boundary only catches errors thrown while rendering; if the renderer **process** dies (OOM, GPU fault, a native + * crash inside Monaco) or the document fails to load, no JavaScript survives to report it — the window is simply left + * showing its background colour, permanently, since nothing was watching. These listeners are that watcher: log what + * happened (the only record there would otherwise be) and reload the window so the user gets a working UI back + * instead of a dead frame. + * + * Reloads are capped: a page that crashes on load would otherwise reload forever, burning CPU and hiding the fault. + * After the cap the window is left as-is with an error in the log, and the user can restart the app. + */ + private installCrashRecovery(win: BrowserWindow): void { + let reloads = 0; + const reload = (reason: string): void => { + if (win.isDestroyed()) return; + if (reloads >= MAX_CRASH_RELOADS) { + this.logger.error( + { reason, reloads }, + 'renderer failed repeatedly; not reloading again (restart the app)', + ); + return; + } + reloads += 1; + this.logger.warn({ reason, attempt: reloads }, 'reloading the renderer to recover'); + win.webContents.reload(); + }; + // A successful load means the previous failure (if any) is behind us; reset the budget so an unrelated crash + // hours later still gets its full set of retries. + win.webContents.on('did-finish-load', () => { + reloads = 0; + }); + win.webContents.on('render-process-gone', (_evt, details) => { + this.logger.error({ details }, 'renderer process gone'); + // 'clean-exit' is a normal teardown (window closing), not a crash to recover from. + if (details.reason !== 'clean-exit') reload(`render-process-gone:${details.reason}`); + }); + win.webContents.on('did-fail-load', (_evt, errorCode, errorDescription, validatedURL) => { + // -3 is ERR_ABORTED, which a superseded navigation raises normally; nothing failed for the user. + if (errorCode === -3) return; + this.logger.error({ errorCode, errorDescription, validatedURL }, 'renderer failed to load'); + reload(`did-fail-load:${errorCode}`); + }); + // Not recovered automatically: an unresponsive renderer is usually a long synchronous task that finishes on its + // own, and reloading would throw away the user's in-flight state. Logged so a hang leaves a trace. + win.webContents.on('unresponsive', () => { + this.logger.warn('renderer became unresponsive'); + }); + win.webContents.on('responsive', () => { + this.logger.info('renderer became responsive again'); + }); + } } /** Loads the window state (missing/corrupt → empty object, falling back to default size) and constructs the WindowManager. */ diff --git a/apps/desktop/src/main/controllers/config.ts b/apps/desktop/src/main/controllers/config.ts index 54f9da13..9e9fc30c 100644 --- a/apps/desktop/src/main/controllers/config.ts +++ b/apps/desktop/src/main/controllers/config.ts @@ -1,6 +1,6 @@ import { randomBytes } from 'node:crypto'; import { nativeTheme } from 'electron'; -import { editorThemeNativeSource } from '@meebox/shared'; +import { editorThemeNativeSource, normalizeNoProxy } from '@meebox/shared'; import { writeConfig } from '@meebox/config'; import { buildDraftAdapter } from '../adapters.js'; import { setMainLanguage } from '../i18n/index.js'; @@ -156,12 +156,16 @@ export const setConnections: IpcController<'config:setConnections'> = async (_ev */ export const setProxy: IpcController<'config:setProxy'> = async (_event, req) => { const { bootstrap, logger, reconfigureConnections } = getContext(); - const next = { ...bootstrap.config, proxy: req.proxy }; + // Normalize the bypass list on the way in, not in the form: whichever way the config arrives — this IPC, or a + // hand-edited config.yaml — what lands on disk is one canonical comma-separated line, so the value the user reads + // back is the value actually matched against. + const proxy = { ...req.proxy, no_proxy: normalizeNoProxy(req.proxy.no_proxy) }; + const next = { ...bootstrap.config, proxy }; await writeConfig(bootstrap.paths.configFile, next); - bootstrap.config.proxy = req.proxy; + bootstrap.config.proxy = proxy; await reconfigureConnections(); logger.info( - { enabled: req.proxy.enabled, host: req.proxy.host, port: req.proxy.port }, + { enabled: proxy.enabled, host: proxy.host, port: proxy.port, noProxy: proxy.no_proxy }, 'proxy config updated (hot-reloaded)', ); }; diff --git a/apps/desktop/src/main/controllers/pr.ts b/apps/desktop/src/main/controllers/pr.ts index 106c6467..176ff52f 100644 --- a/apps/desktop/src/main/controllers/pr.ts +++ b/apps/desktop/src/main/controllers/pr.ts @@ -31,6 +31,11 @@ import { } from '@meebox/shared'; import { annotateOwnership } from '../services/comments.js'; import { getContext } from '../services/context.js'; +import { + confirmMergeSettled, + confirmMergeabilityAfterReview, + refreshSinglePr, +} from '../services/pr-post-action.js'; import type { IpcController } from './types.js'; /* @@ -322,56 +327,8 @@ export const refreshPrs: IpcController<'prs:refresh'> = () => getContext().polle * the mirror has the new commits so the diff renders the new code. Returns the updated PR; the renderer then reloads the * list locally (no network poll of other PRs). */ -export const refreshOnePr: IpcController<'prs:refreshOne'> = async (_event, req) => { - const ctx = getContext(); - const existing = await ctx.pr.findPrOrThrow(req.localId); - const adapter = ctx.pr.adapterForOrThrow(existing); - // Remote fetch of just this PR. 403/404 normalize to error codes (matching openPrByUrl); other errors bubble up. - let fresh; - try { - fresh = await adapter.prs.getSinglePullRequest( - { projectKey: existing.repo.projectKey, repoSlug: existing.repo.repoSlug }, - existing.remoteId, - ); - } catch (err) { - const status = (err as { status?: number } | null)?.status; - if (status === 403) throw new AppError(ERROR_CODES.PR_FORBIDDEN, undefined, 'forbidden'); - if (status === 404) throw new AppError(ERROR_CODES.PR_NOT_FOUND, undefined, 'not found'); - throw err; - } - // localStatus mirrors the remote current user's reviewer status (remote authoritative, same mapping as the poll); - // when the current user is unknown (ping incomplete) keep the recorded status rather than downgrading to pending. - const me = adapter.connection.getCurrentUser(); - const mineStatus = me ? fresh.reviewers.find((r) => r.name === me.name)?.status : undefined; - const localStatus = !me - ? existing.localStatus - : mineStatus === 'approved' - ? 'approved' - : mineStatus === 'needsWork' - ? 'needs_work' - : 'pending'; - const stored: StoredPullRequest = { - ...fresh, - localId: existing.localId, - platform: existing.platform, - connectionId: existing.connectionId, - localStatus, - // Preserve local-only bookkeeping (a single-PR refresh isn't a discovery pass). - discoveryFilters: existing.discoveryFilters, - discoveredAt: existing.discoveredAt, - lastSeenAt: new Date().toISOString(), - }; - await writePrMeta(await ctx.pr.storeForPr(req.localId), req.localId, stored); - await ctx.pr.invalidateCommentsCache(req.localId); - if (fresh.sourceRef.sha !== existing.sourceRef.sha) { - try { - await ctx.pr.ensureMirrorReadyForPr(stored); - } catch { - /* non-fatal: the diff view self-heals / surfaces a readable error if the mirror still lacks the sha */ - } - } - return stored; -}; +export const refreshOnePr: IpcController<'prs:refreshOne'> = (_event, req) => + refreshSinglePr(getContext(), req.localId, { invalidateComments: true }); /** * The Poller's most recent completion time (used for startup initialization). @@ -382,6 +339,11 @@ export const getLastSync: IpcController<'prs:lastSync'> = () => ({ /** * Set review status: write remote first (on failure the frontend is unchanged), and persist locally after the remote is OK. + * + * A verdict can change the remote's mergeability verdict (an approval satisfying the last required rule makes the PR + * mergeable), but the remote recomputes that asynchronously — the value returned here is still the pre-verdict one. So + * kick off a background re-check that lands the new canMerge, rather than leaving the merge button a poll interval + * behind reality (see services/pr-post-action.ts). */ export const setPrStatus: IpcController<'prs:setLocalStatus'> = async (_event, req) => { const ctx = getContext(); @@ -398,7 +360,9 @@ export const setPrStatus: IpcController<'prs:setLocalStatus'> = async (_event, r pr.remoteId, remoteStatus, ); - return setLocalStatus(ctx.stateStore, req.localId, req.status); + const updated = await setLocalStatus(ctx.stateStore, req.localId, req.status); + confirmMergeabilityAfterReview(ctx, req.localId, pr.mergeStatus.canMerge); + return updated; }; /** @@ -408,7 +372,12 @@ export const markRead: IpcController<'prs:markRead'> = (_event, req) => markPrRead(getContext().stateStore, req.localId); /** - * Merge a PR; do not persist locally here, relying on renderer refresh → poll soft-delete to finish, to avoid local and remote disagreeing. + * Merge a PR; do not persist locally here, relying on refresh → poll soft-delete to finish, to avoid local and remote disagreeing. + * + * The remote does not settle synchronously: for a moment after the merge is accepted the PR still reports open and still + * comes back in the discovery list, so an immediate refresh would show it sitting in the list untouched. A background + * re-check confirms it actually left the open state and then archives it through a poll tick (see + * services/pr-post-action.ts); it is deliberately not awaited, so the caller isn't blocked on the remote settling. */ export const mergePr: IpcController<'prs:merge'> = async (_event, req) => { const ctx = getContext(); @@ -418,6 +387,7 @@ export const mergePr: IpcController<'prs:merge'> = async (_event, req) => { { projectKey: pr.repo.projectKey, repoSlug: pr.repo.repoSlug }, pr.remoteId, ); + confirmMergeSettled(ctx, req.localId); }; /** diff --git a/apps/desktop/src/main/services/pr-agent/run-executor.ts b/apps/desktop/src/main/services/pr-agent/run-executor.ts index 398e0234..7701eb8f 100644 --- a/apps/desktop/src/main/services/pr-agent/run-executor.ts +++ b/apps/desktop/src/main/services/pr-agent/run-executor.ts @@ -15,6 +15,7 @@ import { } from '@meebox/pr-agent-bridge'; import { addFindingClosure, + classifyLlmFailure, dropPendingFindingDrafts, finishReviewRun, parseReviewOutput, @@ -37,7 +38,8 @@ import { accumulateUsageSentinel, finalizeUsage, newUsageAcc, - stripUsageSentinels, + parseLlmErrorSentinel, + stripShimSentinels, } from './usage.js'; import { neutralizeWorktreeInstructions } from './worktree-sanitize.js'; @@ -171,12 +173,18 @@ export class RunExecutor { durationMs: Date.now() - t0, exitCode: result.exitCode, stdout, - stderr: stripUsageSentinels(result.stderr), + stderr: stripShimSentinels(result.stderr), tokenUsage, }; if (parsed.llmFailure) { + // pr-agent's fallback retry only logs "Failed to generate prediction with any model" to stdout and hides the real + // exception in loguru's artifact field, so prefer the shim's stderr sentinel when present — it carries the actual + // provider/CLI error, which is also the only text worth classifying into an actionable hint. + const sentinel = parseLlmErrorSentinel(result.stderr); + const message = sentinel?.message ?? parsed.llmFailure.message; + const errorHint = classifyLlmFailure(message); this.ctx.logger.warn( - { runId, reason: parsed.llmFailure.message }, + { runId, reason: message, cli: sentinel?.cli, hint: errorHint }, 'pragent exit 0 but LLM call failed; marking run as failed', ); // Failed runs get no structured collection — findings set empty, UI shows only raw output (no chatpane finding card). @@ -184,7 +192,8 @@ export class RunExecutor { ...base, status: 'failed', errorReason: 'llm-error', - errorMessage: parsed.llmFailure.message, + errorMessage: message, + ...(errorHint ? { errorHint } : {}), findings: [], }; } @@ -225,7 +234,7 @@ export class RunExecutor { errorReason: err.reason, errorMessage: err.message, stdout: err.result.stdout, - stderr: stripUsageSentinels(err.result.stderr), + stderr: stripShimSentinels(err.result.stderr), findings: [], tokenUsage, }; diff --git a/apps/desktop/src/main/services/pr-agent/usage.ts b/apps/desktop/src/main/services/pr-agent/usage.ts index 33ba4076..922fac06 100644 --- a/apps/desktop/src/main/services/pr-agent/usage.ts +++ b/apps/desktop/src/main/services/pr-agent/usage.ts @@ -73,13 +73,51 @@ export function finalizeUsage(acc: UsageAcc): TokenUsage | undefined { } /** - * Strip usage sentinel lines from stderr before persistence: onLine already intercepts them in real time without forwarding, but exec internally + * Strip shim sentinel lines from stderr before persistence: onLine already intercepts them in real time without forwarding, but exec internally * accumulates all stderr into result.stderr (including sentinels), so clear these noise lines before persisting. */ -export function stripUsageSentinels(stderr: string | undefined): string | undefined { +export function stripShimSentinels(stderr: string | undefined): string | undefined { if (!stderr) return stderr; return stderr .split('\n') - .filter((l) => !l.includes(USAGE_SENTINEL)) + .filter((l) => !l.includes(USAGE_SENTINEL) && !l.includes(LLM_ERROR_SENTINEL)) .join('\n'); } + +/** + * LLM-error sentinel-line prefix (kept consistent with the shim's usage._emit_llm_error). Shares the stderr sentinel + * channel with usage: pr-agent's fallback retry logs the underlying exception into loguru's `artifact=` field, which the + * default format drops, so the real cause never reaches stdout — the shim routes it out of band through this line. + */ +export const LLM_ERROR_SENTINEL = '@@MEEBOX_LLM_ERROR@@'; + +/** + * Pull the LLM-call cause out of stderr sentinel lines, returning the **last** one (a run may retry several times; the last + * is what actually sank the run). Bad JSON / a missing message is ignored — this is a diagnostic enrichment, never a + * reason to fail differently. + */ +export function parseLlmErrorSentinel( + stderr: string | undefined, +): { cli?: string; message: string } | undefined { + if (!stderr) return undefined; + let found: { cli?: string; message: string } | undefined; + for (const line of stderr.split('\n')) { + const i = line.indexOf(LLM_ERROR_SENTINEL); + if (i < 0) continue; + try { + const rec = JSON.parse(line.slice(i + LLM_ERROR_SENTINEL.length).trim()) as { + cli?: unknown; + message?: unknown; + }; + if (typeof rec.message === 'string' && rec.message.trim()) { + found = { + cli: typeof rec.cli === 'string' ? rec.cli : undefined, + message: rec.message.trim(), + }; + } + } catch { + // Malformed sentinel → skip, keep any earlier one. + } + } + return found; +} diff --git a/apps/desktop/src/main/services/pr-post-action.ts b/apps/desktop/src/main/services/pr-post-action.ts new file mode 100644 index 00000000..a1c6ebd5 --- /dev/null +++ b/apps/desktop/src/main/services/pr-post-action.ts @@ -0,0 +1,187 @@ +import { writePrMeta } from '@meebox/poller'; +import { AppError, ERROR_CODES, type StoredPullRequest } from '@meebox/shared'; +import type { ServiceContext } from './context.js'; + +/** + * Post-action remote reconciliation: after a write action (merge / review verdict) the platform does **not** settle + * synchronously — the call returns as soon as the request is accepted, while the state the UI reads is recomputed + * asynchronously on the remote and lands some seconds later: + * + * - **merge**: the PR keeps reporting `state: 'open'` and keeps appearing in the discovery list for a moment, so a + * refresh fired the instant the merge returns still shows the PR sitting in the list as if nothing happened; + * - **review verdict**: `mergeStatus.canMerge` is a server-side verdict over approvals / builds / branch protection. + * An approval that satisfies the last required rule flips it to true — but only once the remote has recomputed it, + * which is after the approve call has already returned. + * + * A single immediate refresh therefore reads pre-action state and looks like nothing happened; leaving it to the + * periodic poll means waiting a whole interval. So both actions kick off a **backoff re-check** here: refresh the one + * PR on a growing delay until the expected change shows up (or the attempts run out, where the periodic poll remains + * the backstop), and broadcast `prs:changed` whenever the state actually moved so the renderer reloads. + * + * Living in main rather than in the renderer keeps every entry point covered by one implementation: the merge / approve + * buttons, the chat `/merge` `/approve` commands, and the CLI's review write actions all route through the same + * controllers (see services/api-server/routes/pr.ts). + */ + +/** + * Delays before each re-check attempt, in ms. The first is short because the common case is a remote that has already + * settled by the time the action returns, and that attempt sets the latency the user actually perceives; the rest grow + * to cover a slow remote without hammering it. The tail (~15s total) is deliberately shorter than a poll interval — + * past that, the periodic poll is the backstop and a background loop adds nothing. + */ +const RECHECK_DELAYS_MS = [400, 1_200, 3_500, 10_000] as const; + +/** + * Confirmation loops already running, keyed by `:`, so repeated clicks don't stack duplicate loops. + * Keyed by kind as well as PR: approving and then merging the same PR are two independent settlements, and a merge + * confirmation must not be dropped just because the verdict's loop is still winding down. + */ +const inFlight = new Set(); + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Re-fetch one PR from the remote and persist it, returning the updated record (null when the PR is gone locally). + * + * Shared by the `prs:refreshOne` handler and the confirmation loops below. `invalidateComments` is what separates the + * two callers: a user-driven refresh wants comments re-fetched as well, while a background confirmation only cares + * about PR metadata — invalidating on every attempt would make the open comment / diff panes re-fetch several times + * over for a state the user never asked about. + */ +export async function refreshSinglePr( + ctx: ServiceContext, + localId: string, + opts: { invalidateComments: boolean }, +): Promise { + const existing = await ctx.pr.findPrOrThrow(localId); + const adapter = ctx.pr.adapterForOrThrow(existing); + // Remote fetch of just this PR. 403/404 normalize to error codes (matching openPrByUrl); other errors bubble up. + let fresh; + try { + fresh = await adapter.prs.getSinglePullRequest( + { projectKey: existing.repo.projectKey, repoSlug: existing.repo.repoSlug }, + existing.remoteId, + ); + } catch (err) { + const status = (err as { status?: number } | null)?.status; + if (status === 403) throw new AppError(ERROR_CODES.PR_FORBIDDEN, undefined, 'forbidden'); + if (status === 404) throw new AppError(ERROR_CODES.PR_NOT_FOUND, undefined, 'not found'); + throw err; + } + // localStatus mirrors the remote current user's reviewer status (remote authoritative, same mapping as the poll); + // when the current user is unknown (ping incomplete) keep the recorded status rather than downgrading to pending. + const me = adapter.connection.getCurrentUser(); + const mineStatus = me ? fresh.reviewers.find((r) => r.name === me.name)?.status : undefined; + const localStatus = !me + ? existing.localStatus + : mineStatus === 'approved' + ? 'approved' + : mineStatus === 'needsWork' + ? 'needs_work' + : 'pending'; + const stored: StoredPullRequest = { + ...fresh, + localId: existing.localId, + platform: existing.platform, + connectionId: existing.connectionId, + localStatus, + // Preserve local-only bookkeeping (a single-PR refresh isn't a discovery pass). + discoveryFilters: existing.discoveryFilters, + discoveredAt: existing.discoveredAt, + lastSeenAt: new Date().toISOString(), + }; + await writePrMeta(await ctx.pr.storeForPr(localId), localId, stored); + if (opts.invalidateComments) await ctx.pr.invalidateCommentsCache(localId); + if (fresh.sourceRef.sha !== existing.sourceRef.sha) { + try { + await ctx.pr.ensureMirrorReadyForPr(stored); + } catch { + /* non-fatal: the diff view self-heals / surfaces a readable error if the mirror still lacks the sha */ + } + } + return stored; +} + +/** + * Run the backoff re-check loop for one PR: refresh, hand the result to `settled`, and stop as soon as it reports the + * expected change (or the attempts run out). Errors on an individual attempt are swallowed and retried — a transient + * remote hiccup shouldn't abort the confirmation, and a PR that vanished locally (archived by a concurrent poll) ends + * the loop, since there is nothing left to confirm. + */ +async function recheckUntilSettled( + ctx: ServiceContext, + localId: string, + what: 'merge' | 'review-verdict', + settled: (pr: StoredPullRequest) => boolean, +): Promise { + const key = `${what}:${localId}`; + if (inFlight.has(key)) return; + inFlight.add(key); + try { + for (const delay of RECHECK_DELAYS_MS) { + await sleep(delay); + let pr: StoredPullRequest; + try { + pr = await refreshSinglePr(ctx, localId, { invalidateComments: false }); + } catch (err) { + if (err instanceof AppError && err.code === ERROR_CODES.PR_NOT_FOUND) return; + ctx.logger.debug({ err, localId, what }, 'post-action recheck attempt failed; retrying'); + continue; + } + if (!settled(pr)) { + // Not the change we're waiting for, but the attempt has still persisted fresh remote state (reviewer verdicts, + // vetoes, head sha); leaving that on disk unannounced would show a list disagreeing with what was just stored. + // Reloading is a local read, so the extra broadcast costs nothing. + ctx.broadcast('prs:changed', { localId }); + continue; + } + // The remote has settled. The list filters on archivedAt, not on PR state, so a merged PR only disappears once it + // is archived — archive this one directly rather than running a full poll tick for it: a tick would fetch every + // connection's discovery lists before the PR the user just merged could leave, and all of that is latency the user + // watches. The departure a poll infers from absence is already established here by the confirmed remote state. + if (what === 'merge') { + try { + await ctx.poller.archivePullRequest(localId); + } catch (err) { + ctx.logger.warn({ err, localId }, 'archiving the merged PR failed; the periodic poll will catch up'); + } + } + ctx.logger.info({ localId, what }, 'post-action remote state settled'); + ctx.broadcast('prs:changed', { localId }); + return; + } + ctx.logger.debug( + { localId, what }, + 'post-action remote state did not settle within the recheck window; leaving it to the periodic poll', + ); + } finally { + inFlight.delete(key); + } +} + +/** + * After a merge is accepted: confirm the PR actually left the open state, then archive it through a poll tick so it + * disappears from the list. Fire-and-forget — the merge IPC returns immediately and the button must not stay busy for + * the length of the confirmation. + */ +export function confirmMergeSettled(ctx: ServiceContext, localId: string): void { + void recheckUntilSettled(ctx, localId, 'merge', (pr) => pr.state !== 'open'); +} + +/** + * After a review verdict is written: confirm whether it changed the remote's mergeability verdict, so the merge button + * appears (or disappears) without waiting for the periodic poll. `before` is the canMerge value observed at the moment + * the verdict was written; any move away from it is the settlement we're waiting for. + */ +export function confirmMergeabilityAfterReview( + ctx: ServiceContext, + localId: string, + before: boolean, +): void { + void recheckUntilSettled( + ctx, + localId, + 'review-verdict', + (pr) => pr.mergeStatus.canMerge !== before, + ); +} diff --git a/apps/desktop/src/main/utils/proxy.ts b/apps/desktop/src/main/utils/proxy.ts index d28e8c69..194298dd 100644 --- a/apps/desktop/src/main/utils/proxy.ts +++ b/apps/desktop/src/main/utils/proxy.ts @@ -4,15 +4,33 @@ // - shouldBypass: whether loopback/local goes direct (② decides at the call site whether to attach a dispatcher) // Phase one is HTTP proxy only; when enabled=false all forms yield "empty/direct connection", so call sites need not each check the switch. import { ProxyAgent, type Dispatcher } from 'undici'; -import { ERROR_CODES, errorCodeMessage, type ProxyConfig } from '@meebox/shared'; +import { + ERROR_CODES, + LOOPBACK_NO_PROXY, + errorCodeMessage, + matchesNoProxy, + normalizeNoProxy, + type ProxyConfig, +} from '@meebox/shared'; -// loopback / local: always direct connection, never through the proxy. The env path relies on NO_PROXY, the dispatcher path on shouldBypass. -const NO_PROXY = 'localhost,127.0.0.1,::1'; +/** + * Effective bypass rules = the built-in loopback set + whatever the user configured. Loopback is prepended rather than + * left to the user: a local model or local service must never be sent through a proxy, and that guarantee should not + * depend on the user having typed it. + */ +function effectiveNoProxy(proxy: ProxyConfig): string { + return normalizeNoProxy(`${LOOPBACK_NO_PROXY},${proxy.no_proxy ?? ''}`); +} -/** loopback / local host → true (should go direct connection, not through the proxy). */ -export function shouldBypass(host: string): boolean { - const h = host.toLowerCase().replace(/^\[|\]$/g, ''); // strip IPv6 literal brackets - return h === 'localhost' || h.endsWith('.localhost') || h === '127.0.0.1' || h === '::1'; +/** + * Host should egress directly (loopback, or covered by the user's `no_proxy`) rather than through the proxy. + * + * The env path hands the same rules to subprocesses via `NO_PROXY` and lets their libraries apply them; this is the + * in-process equivalent for the dispatcher path, which is why both derive from `effectiveNoProxy` — the two egress + * classes must not disagree about the same config. + */ +export function shouldBypass(proxy: ProxyConfig, host: string): boolean { + return matchesNoProxy(host, effectiveNoProxy(proxy)); } /** Build a standard proxy URL: `://[user:pass@]host:port`. undefined when disabled / no host. */ @@ -32,6 +50,7 @@ export function proxyUrl(proxy: ProxyConfig): string | undefined { export function buildProxyEnv(proxy: ProxyConfig): Record { const url = proxyUrl(proxy); if (!url) return {}; + const bypass = effectiveNoProxy(proxy); return { HTTP_PROXY: url, http_proxy: url, @@ -39,8 +58,8 @@ export function buildProxyEnv(proxy: ProxyConfig): Record { https_proxy: url, ALL_PROXY: url, all_proxy: url, - NO_PROXY, - no_proxy: NO_PROXY, + NO_PROXY: bypass, + no_proxy: bypass, }; } @@ -85,14 +104,15 @@ export async function testProxyConnectivity( /** * Build a "proxy-aware" fetch for a target host, to inject into BitbucketClient's opts.fetch. - * host hits loopback/local → returns undefined (the call site uses the default global fetch for a direct connection). - * Otherwise returns a fetch wrapper carrying the dispatcher. Also returns undefined when the proxy is disabled. + * host hits loopback/local or the configured bypass list → returns undefined (the call site uses the default global + * fetch for a direct connection). Otherwise returns a fetch wrapper carrying the dispatcher. Also returns undefined + * when the proxy is disabled. */ export function proxyFetchForHost( proxy: ProxyConfig, host: string, ): ((input: string, init?: RequestInit) => Promise) | undefined { - if (shouldBypass(host)) return undefined; + if (shouldBypass(proxy, host)) return undefined; const dispatcher = buildProxyDispatcher(proxy); if (!dispatcher) return undefined; return (input, init) => diff --git a/apps/desktop/src/renderer/src/App.scss b/apps/desktop/src/renderer/src/App.scss index 801115a7..b8dc3f11 100644 --- a/apps/desktop/src/renderer/src/App.scss +++ b/apps/desktop/src/renderer/src/App.scss @@ -29,6 +29,7 @@ @use './styles/features/pr-info'; @use './styles/features/chat'; @use './styles/features/drafts-panel'; +@use './styles/common/crash'; @use './styles/common/modal'; @use './styles/features/pr'; // pr 簇（含 publish-review，须在 common/modal 之后） @use './styles/features/settings/forms'; diff --git a/apps/desktop/src/renderer/src/boot-guard.ts b/apps/desktop/src/renderer/src/boot-guard.ts new file mode 100644 index 00000000..26c3d435 --- /dev/null +++ b/apps/desktop/src/renderer/src/boot-guard.ts @@ -0,0 +1,84 @@ +/** + * Last-resort guard against a blank window, sitting below React entirely. + * + * The root ErrorBoundary covers errors thrown *while rendering*, and main covers the renderer process dying. Between + * those two lies a gap neither can see: a module that throws while it initializes — i18n, theme, the app bundle itself + * — takes down the entry module before `createRoot().render()` ever runs. No React means no boundary; the window just + * stays on its background colour forever, which is the black screen users report and cannot recover from. + * + * So this module (imported first, so its side effect is armed before anything else can throw) watches for `#root` + * staying empty and paints a plain-DOM recovery screen if it does. No React, no i18n, no stylesheet — every one of + * those is a thing that could be the failure being reported, so the screen depends on none of them. + */ + +/** How long to wait for React's first commit before assuming boot failed. Generous: a cold start on a slow machine + * pulls a sizable bundle, and a false positive would replace a working (if slow) boot with an error screen. */ +const BOOT_TIMEOUT_MS = 15_000; + +/** Fixed English copy: i18n may itself be the module that failed, and English is the app's fallback language anyway. */ +const COPY = { + title: 'Code Meeseeks failed to start', + hint: 'The interface could not be loaded. Reloading usually resolves it; if it keeps happening, the details below and the application log (meebox.log) identify the cause.', + reload: 'Reload', +}; + +function rootIsEmpty(): boolean { + const root = document.getElementById('root'); + return !root || root.childElementCount === 0; +} + +/** Paint the recovery screen, once — later failures must not stack copies of it on top of each other. */ +function showRecovery(detail: string): void { + if (document.getElementById('boot-guard-screen')) return; + if (!rootIsEmpty()) return; // The app did render after all; leave the working UI alone. + const host = document.createElement('div'); + host.id = 'boot-guard-screen'; + // Inline styles on purpose: the stylesheet is part of the app bundle that may have failed to load. + host.setAttribute( + 'style', + 'position:fixed;inset:0;z-index:2147483647;display:flex;align-items:center;justify-content:center;' + + 'padding:16px;background:#1e1e1e;color:#ccc;font:13px/1.5 system-ui,sans-serif;', + ); + const card = document.createElement('div'); + card.setAttribute('style', 'max-width:560px;width:100%'); + const title = document.createElement('h1'); + title.setAttribute('style', 'margin:0 0 8px;font-size:15px;font-weight:600;color:#eee'); + title.textContent = COPY.title; + const hint = document.createElement('p'); + hint.setAttribute('style', 'margin:0 0 12px;color:#999'); + hint.textContent = COPY.hint; + const pre = document.createElement('pre'); + pre.setAttribute( + 'style', + 'margin:0 0 16px;padding:8px;max-height:220px;overflow:auto;background:#181818;' + + 'border:1px solid #333;border-radius:4px;white-space:pre-wrap;word-break:break-word;font-size:12px;color:#999', + ); + pre.textContent = detail; + const button = document.createElement('button'); + button.type = 'button'; + button.setAttribute( + 'style', + 'padding:4px 12px;background:#0e639c;color:#fff;border:none;border-radius:3px;cursor:pointer;font:inherit', + ); + button.textContent = COPY.reload; + button.addEventListener('click', () => window.location.reload()); + card.append(title, hint, pre, button); + host.append(card); + document.body.append(host); +} + +// An uncaught error while the app is still blank means boot failed — no need to wait out the timeout. Once the app has +// rendered, the same errors are the app's own business (the root boundary and the log relay handle them), so +// rootIsEmpty() inside showRecovery keeps this from hijacking a working window. +window.addEventListener('error', (e: ErrorEvent) => { + showRecovery(e.error instanceof Error ? (e.error.stack ?? e.error.message) : e.message); +}); +window.addEventListener('unhandledrejection', (e: PromiseRejectionEvent) => { + const reason: unknown = e.reason; + showRecovery(reason instanceof Error ? (reason.stack ?? reason.message) : String(reason)); +}); + +// Backstop for a silent failure — a module that never resolves, an import that hangs — where nothing throws at all. +setTimeout(() => { + showRecovery(`The interface did not render within ${BOOT_TIMEOUT_MS / 1000}s of startup.`); +}, BOOT_TIMEOUT_MS); diff --git a/apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx b/apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx new file mode 100644 index 00000000..401ce60f --- /dev/null +++ b/apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx @@ -0,0 +1,43 @@ +import { useTranslation } from 'react-i18next'; +import { isChunkLoadError } from './LazyBoundary'; + +/** + * Full-window fallback for a crash in the app's root subtree. + * + * Without a boundary at the root, any error thrown while rendering unmounts the whole tree and leaves an empty `#root` + * — which reads as a permanently black window, with no way back short of restarting the app. This screen is what the + * user gets instead: what broke, and a way out. + * + * The way out depends on the failure. `onRetry` re-renders the subtree, which is enough when the crash came from + * transient state (a stale record read during an in-flight update). A **stale chunk** is the exception: the page holds + * a hashed module URL that no longer exists (the app was rebuilt or updated while this window stayed open), so + * re-rendering re-requests the same dead URL and fails identically — only a reload recovers. Offering "retry" there + * would be offering a button that cannot work, so that case leads with reload and explains why. + */ +export function AppCrashScreen({ err, onRetry }: { err: Error; onRetry: () => void }) { + const { t } = useTranslation(); + const stale = isChunkLoadError(err); + return ( +
+
+

{stale ? t('crash.staleTitle') : t('crash.title')}

+

{stale ? t('crash.staleHint') : t('crash.hint')}

+ 
{err.message || String(err)}
+
+ + {!stale && ( + + )} +
+
+
+ ); +} diff --git a/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx b/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx index 9488882e..956756ce 100644 --- a/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx +++ b/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx @@ -1,4 +1,5 @@ import { Component, type ErrorInfo, type ReactNode } from 'react'; +import { invoke } from '../../api'; interface ErrorBoundaryProps { children: ReactNode; @@ -27,11 +28,18 @@ export class ErrorBoundary extends Component { + /* the log relay must never be the thing that breaks the fallback UI */ + }); } reset = (): void => { diff --git a/apps/desktop/src/renderer/src/components/common/LazyBoundary.tsx b/apps/desktop/src/renderer/src/components/common/LazyBoundary.tsx new file mode 100644 index 00000000..29d6c104 --- /dev/null +++ b/apps/desktop/src/renderer/src/components/common/LazyBoundary.tsx @@ -0,0 +1,75 @@ +import { Suspense, type ReactNode } from 'react'; +import { useTranslation } from 'react-i18next'; +import { ErrorBoundary } from './ErrorBoundary'; + +/** + * Whether an error is a dynamic-import failure — the chunk behind a `lazy()` could not be fetched. + * + * Worth distinguishing because the remedy is the opposite of the usual one: the page is holding a hashed chunk URL that + * no longer exists on disk (the app was rebuilt or updated while this window stayed open), so **retrying re-requests the + * same dead URL and fails again**. Only a reload — which re-reads the entry and picks up current hashes — recovers. + * + * The message differs per engine, hence matching several forms rather than one. + */ +export function isChunkLoadError(err: Error): boolean { + const msg = err.message.toLowerCase(); + return ( + msg.includes('failed to fetch dynamically imported module') || // Chromium (Electron) + msg.includes('error loading dynamically imported module') || // Firefox + msg.includes('importing a module script failed') || // Safari + msg.includes('unable to preload') // Vite's preload helper + ); +} + +/** + * Suspense + an error boundary around a `lazy()` subtree. + * + * `Suspense` alone only covers the *pending* half of a lazy import: if the chunk fails to load, the promise rejects and + * the error propagates to the nearest boundary. With no boundary in between it reaches the root one and takes the whole + * app down — a Monaco snippet failing to load should not cost the user the comment thread around it. So each lazy + * subtree gets its own boundary, and the failure stays inside the pane that could not load. + */ +export function LazyBoundary({ + label, + loading, + children, +}: { + /** Names the failing region in logs (see ErrorBoundary). */ + label: string; + /** Rendered while the chunk is in flight. */ + loading: ReactNode; + children: ReactNode; +}) { + const { t } = useTranslation(); + return ( + ( +
+

{t('lazyLoad.failed')}

+

+ {isChunkLoadError(err) ? t('lazyLoad.staleHint') : t('lazyLoad.genericHint')} +

+
+ {/* Retry is offered only when it can actually work: for a stale chunk it would re-request the same dead + URL, so that case leads with reload instead. */} + {!isChunkLoadError(err) && ( + + )} + +
+
+ )} + > + {children} + + ); +} diff --git a/apps/desktop/src/renderer/src/components/common/index.ts b/apps/desktop/src/renderer/src/components/common/index.ts index d49478f4..f9ab8137 100644 --- a/apps/desktop/src/renderer/src/components/common/index.ts +++ b/apps/desktop/src/renderer/src/components/common/index.ts @@ -2,10 +2,12 @@ // Cross-domain consumers (features/* · layout/* · App etc.) import via this barrel; common's internal modules reference each other // (markdownMermaid → MermaidDiagram, Modal → icons, ConfirmModal → Modal) via relative paths, // not through this barrel, to avoid circular dependencies. +export * from './AppCrashScreen'; export * from './Avatar'; export * from './BitbucketImage'; export * from './ConfirmModal'; export * from './ErrorBoundary'; +export * from './LazyBoundary'; export * from './LlmProviderIcon'; export * from './Loading'; export * from './MermaidDiagram'; diff --git a/apps/desktop/src/renderer/src/components/features/chat/ChatPane.tsx b/apps/desktop/src/renderer/src/components/features/chat/ChatPane.tsx index 8fcd5b69..725ae07e 100644 --- a/apps/desktop/src/renderer/src/components/features/chat/ChatPane.tsx +++ b/apps/desktop/src/renderer/src/components/features/chat/ChatPane.tsx @@ -12,8 +12,15 @@ import type { import { invoke } from '../../../api'; import { ChatIcon, TrashIcon, ConfirmModal, PaneLoading } from '../../common'; import { useChatRunStore } from '../../../stores/chat-run-store'; -import { useDraftsForPr } from '../../../stores/drafts-store'; +import { draftsStore, useDraftsForPr } from '../../../stores/drafts-store'; import { useFindingClosuresForPr } from '../../../stores/finding-closures-store'; +import { + commentReferenceStore, + commentReferenceLabel, + formatCommentReference, + useAnsweringComment, + useCommentReference, +} from '../../../stores/comment-reference-store'; import { formatReferencedContext, selectionStore, @@ -27,6 +34,7 @@ import { useChatTimeline } from './hooks/useChatTimeline'; import { AgentStepRow, ThinkingLive } from './components/AgentStep'; import { ChatEmpty } from './components/ChatEmpty'; import { CommitDivider } from './components/CommitDivider'; +import { computeCommitDividers } from './utils/commit-dividers'; import { ChatInputBar } from './components/ChatInputBar'; import { ConversationMessage } from './components/ConversationMessage'; import { PlanPanel } from './components/PlanPanel'; @@ -159,6 +167,7 @@ export function ChatPane({ // On PR switch, clear the reference state and reset the detached state to avoid cross-PR residue. useEffect(() => { setRefFinding(null); + commentReferenceStore.reset(); setScopeDetached(false); }, [prLocalId]); // On switching to another commit (or clearing the view scope), reset the detached state: the newly selected commit becomes the implicit scope again. @@ -171,10 +180,23 @@ export function ChatPane({ // Diff selection (belonging to the current PR): used for the input bar's "N lines selected" badge + carrying the selected code as implicit context into a question. const { selection: diffSelection, ignored: selectionIgnored } = useDiffSelection(prLocalId); - // When not ignored, format the selection into a reference string; shared by /ask and natural-language questions. Ignored / no selection → undefined (this message carries no reference). + // Comment referenced from a comment surface (activity panel / inline diff zone), carried as implicit context so the + // question can be about the comment without restating it. + const commentRef = useCommentReference(prLocalId); + // Implicit context for this message: the diff selection and the referenced comment are independent sources and + // compose — asking "is this comment right about the code I selected?" needs both. Each block is self-describing, so + // concatenating them needs no framing. Undefined when neither is present (the message carries no reference). const referencedContext = - diffSelection && !selectionIgnored ? formatReferencedContext(diffSelection) : undefined; + [ + diffSelection && !selectionIgnored ? formatReferencedContext(diffSelection) : null, + commentRef ? formatCommentReference(commentRef) : null, + ] + .filter(Boolean) + .join('\n\n') || undefined; + // The comment this round is answering (set when the question carried a reference). Its presence is what puts the + // "use as reply" action on the answer. + const answeringComment = useAnsweringComment(prLocalId); // The effective scope for this PR's chat commands: follows the commit selected in the Diff view, unless the user has detached (scopeDetached). // Only one scope may be in effect at a time — when a Diff selection exists it takes precedence (finer-grained), the commit scope is suspended and its chip // is hidden too (see commitScopeChip), auto-restored after the selection is cleared. @@ -244,26 +266,51 @@ export function ChatPane({ prLocalId, }); - // Commit dividers: mark every point in the run timeline where the reviewed commit changes, so the boundary persists - // rather than vanishing once the new code is reviewed. Two cases: - // - between two consecutive runs whose headSha differs → a divider *before* the newer run (a durable boundary - // between the old-commit runs above and the new-commit runs below); - // - a trailing divider at the bottom when the current PR head has advanced past the last run's commit (covers a new - // commit that hasn't been reviewed yet — including while a run against it is still in flight). - // The timeline is ascending by start time; only runs that recorded a headSha participate (pre-feature runs are skipped). - const commitDividers = useMemo(() => { - const before = new Map(); // timeline entry.key → the newer headSha to render a divider before it - let prevSha: string | undefined; - for (const entry of timeline) { - const sha = entry.run?.headSha; - if (!sha) continue; - if (prevSha && sha !== prevSha) before.set(entry.key, sha); - prevSha = sha; + // Commit dividers: every point in the timeline where the reviewed commit changes (see computeCommitDividers, which + // also explains why the trailing boundary anchors after the last run rather than at the end of the pane). + const commitDividers = useMemo( + () => computeCommitDividers(timeline, pr?.sourceRef.sha), + [timeline, pr?.sourceRef.sha], + ); + + // Key of the last assistant message in the timeline: the answer the action belongs on. Anchored to the last one + // rather than to a message id because a conversation message has no stable id of its own here. + const lastAnswerKey = useMemo(() => { + for (let i = timeline.length - 1; i >= 0; i -= 1) { + const m = timeline[i]?.message; + if (m && m.role !== 'user') return timeline[i]!.key; } - const head = pr?.sourceRef.sha; - const bottom = head && prevSha && head !== prevSha ? head : null; - return { before, bottom }; - }, [timeline, pr?.sourceRef.sha]); + return null; + }, [timeline]); + /** Turn the agent's answer into a draft reply to the comment that was asked about, then release the association. */ + const createReplyDraftFromAnswer = (body: string): void => { + if (!answeringComment || !prLocalId) return; + void invoke('drafts:create', { + localId: prLocalId, + draft: { + body, + status: 'pending', + replyTo: { parentCommentId: answeringComment.commentId }, + ...(answeringComment.anchor?.line != null + ? { + anchor: { + path: answeringComment.anchor.path, + startLine: answeringComment.anchor.line, + endLine: answeringComment.anchor.line, + }, + } + : {}), + } as Parameters>[1]['draft'], + }) + .then(() => { + void draftsStore.refresh(prLocalId); + commentReferenceStore.clearAnswering(); + }) + .catch((e: unknown) => { + console.error('create reply draft from answer failed', e); + }); + }; + // Commit messages for divider tooltips: fetch the PR's commits (main-cached; keyed on head sha so it refreshes when // the head advances) into a sha → message map. Empty until loaded / on failure (the tooltip falls back to the short sha). @@ -448,12 +495,26 @@ export function ChatPane({ ) : entry.step ? ( ) : entry.message ? ( - + { + createReplyDraftFromAnswer(entry.message!.content); + }, + } + : null + } + /> ) : null, )} - {/* Bottom commit divider: the PR head advanced past the last run's commit and no run against it exists yet - (a new commit not reviewed yet, including while a run against it is still in flight). Once such a run - completes, the boundary instead renders between the old and new runs above (see commitDividers.before). */} + {/* Trailing commit divider, for the one case with no entry to precede: the head advanced past the last run's + commit and nothing has landed after that run yet. As soon as anything does — a message, a step, a queued + run — the boundary moves into `before` and anchors there, so it stays put instead of being pushed down by + each new bubble (see computeCommitDividers). */} {commitDividers.bottom && ( { if (refFinding) { @@ -524,6 +592,7 @@ export function ChatPane({ return; } void actions.handleAgentAsk(q, referencedContext); + commentReferenceStore.handOff(); }} onCancel={hasMyActive || agentRunningHere ? actions.handleStopAll : undefined} onSetReviewStatus={onSetReviewStatus} @@ -549,6 +618,18 @@ export function ChatPane({ } : null } + // Referenced comment: chip showing ": " + clear. Attached from a comment's reference button; + // carried as implicit context with the next question, and released on send (see onSend). + commentChip={ + commentRef + ? { + label: commentReferenceLabel(commentRef), + onClear: () => { + commentReferenceStore.clear(); + }, + } + : null + } // Single-commit scope chip: shown when a commit is selected in the view (the selected state comes from the view); click to toggle enabled/disabled — // when disabled (scopeDetached) commands revert to the full PR and the chip greys out; switching to another commit or switching PR resets it to enabled. // Only one scope at a time: when a Diff selection exists it yields to the selection chip (hides this chip), auto-restored after the selection is cleared. diff --git a/apps/desktop/src/renderer/src/components/features/chat/components/ChatInputBar.tsx b/apps/desktop/src/renderer/src/components/features/chat/components/ChatInputBar.tsx index 06d5979a..839296bb 100644 --- a/apps/desktop/src/renderer/src/components/features/chat/components/ChatInputBar.tsx +++ b/apps/desktop/src/renderer/src/components/features/chat/components/ChatInputBar.tsx @@ -12,6 +12,7 @@ import { FileTreeIcon, SendIcon, StopIcon, + ShareIcon, } from '../../../common'; import { useChatInput } from '../hooks/useChatInput'; import { useTextareaAutosizeDrag } from '../hooks/useTextareaAutosizeDrag'; @@ -55,6 +56,8 @@ interface ChatInputBarProps { onToggleSelection: () => void; /** Re-review reference chip: shows "re-review " + clear when a finding is referenced; null = not rendered. */ referenceChip?: { label: string; onClear: () => void } | null; + /** Referenced-comment chip: shows ": " + clear when a comment is referenced; null = not rendered. */ + commentChip?: { label: string; onClear: () => void } | null; /** * Single-commit scope chip: follows the commit selected in the Diff view, showing "short SHA · subject". Shown whenever there is * a selection, click to **toggle enable/disable** (disabling does not remove the chip, this session's commands revert to the whole @@ -89,6 +92,7 @@ export function ChatInputBar({ selectionIgnored, onToggleSelection, referenceChip, + commentChip, commitScopeChip, }: ChatInputBarProps) { const { t } = useTranslation(); @@ -287,6 +291,27 @@ export function ChatInputBar({ )} + {/* Referenced comment chip: the comment is carried as implicit context for this question. Its own chip rather + than sharing the re-review one — that reference changes what the run *does* (produces a verdict), while + this only adds context, and the two can be attached at once. */} + {commentChip && ( + <> +