Skip to content

🐛 fix(desktop): prevent duplicate IPC handler registration from dynamic imports#11827

Merged
arvinxx merged 2 commits into
nextfrom
fix/desktop-dynamic-import-side-effects
Jan 25, 2026
Merged

🐛 fix(desktop): prevent duplicate IPC handler registration from dynamic imports#11827
arvinxx merged 2 commits into
nextfrom
fix/desktop-dynamic-import-side-effects

Conversation

@Innei

@Innei Innei commented Jan 25, 2026

Copy link
Copy Markdown
Member

Summary

  • Fix duplicate __ELECTRON_LOG__ IPC handler registration caused by Rollup bundling debug package into index.js
  • When dynamic imports in @lobechat/file-loaders loaded, they would require("./index.js") to get shared debug dependency, re-executing side-effect code including electron-log's IPC handler registration
  • Add manualChunks config to isolate debug package into separate chunk (vendor-debug.js)
  • Add @napi-rs/canvas to native modules list for proper externalization

Root Cause

Rollup's default code-splitting strategy merges shared dependencies into the entry chunk (index.js). When dynamic import chunks need these shared deps, they require the entry chunk, causing side-effects (like IPC handler registration) to execute again in CJS format.

Test plan

  • Run desktop app in dev mode
  • Trigger file reading via IPC (e.g., open a local file)
  • Verify no "Attempted to register a second handler for 'ELECTRON_LOG'" error appears

Summary by Sourcery

Adjust desktop Electron bundling to avoid duplicate IPC handler side effects and ensure native dependencies are properly externalized.

Build:

  • Configure Rollup manualChunks in the Electron Vite config to isolate the debug package into a separate vendor chunk and prevent it from being bundled into the main entry file.

Deployment:

  • Add @napi-rs/canvas to the native modules configuration so it is treated as an external native dependency during packaging.

…ic imports

Fix an issue where dynamic imports in file-loaders package would cause
the debug package to be bundled into index.js, leading to side-effect
pollution and duplicate electron-log IPC handler registration.

- Add manualChunks config to isolate debug package into separate chunk
- Add @napi-rs/canvas to native modules for proper externalization
@vercel

vercel Bot commented Jan 25, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
lobehub Ready Ready Preview, Comment Jan 25, 2026 5:26pm

Request Review

@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jan 25, 2026
@sourcery-ai

sourcery-ai Bot commented Jan 25, 2026

Copy link
Copy Markdown
Contributor
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Configures Rollup output in the desktop Electron Vite config to isolate the debug dependency into its own chunk to prevent side-effect re-execution from dynamic imports, and updates the native dependencies configuration to treat @napi-rs/canvas as an external native module.

File-Level Changes

Change Details Files
Configure manual Rollup chunking so the debug package is split out of the main Electron entry bundle to avoid duplicate side-effect execution (including IPC handler registration).
  • Add an output.manualChunks function under build.rollupOptions that returns a dedicated chunk name when bundling files from node_modules/debug
  • Ensure the Rollup config does not otherwise alter existing externalization behavior or sourcemap settings
apps/desktop/electron.vite.config.ts
Update the native modules configuration so @napi-rs/canvas is always treated as a native dependency and externalized correctly in the desktop build.
  • Add @napi-rs/canvas to the nativeModules array, alongside platform-conditional native dependencies
  • Preserve existing platform-specific handling (e.g., node-mac-permissions for Darwin)
apps/desktop/native-deps.config.mjs

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@gru-agent

gru-agent Bot commented Jan 25, 2026

Copy link
Copy Markdown
Contributor

TestGru Assignment

Summary

Link CommitId Status Reason
Detail b65aeb9 🚫 Skipped No files need to be tested {"apps/desktop/electron.vite.config.ts":"File path does not match include patterns.","apps/desktop/native-deps.config.mjs":"File path does not match include patterns."}

History Assignment

Tip

You can @gru-agent and leave your feedback. TestGru will make adjustments based on your input

@dosubot dosubot Bot added the 🐛 Bug label Jan 25, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov

codecov Bot commented Jan 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.64%. Comparing base (d088d60) to head (c9e334e).
⚠️ Report is 18 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff            @@
##             next   #11827    +/-   ##
========================================
  Coverage   73.64%   73.64%            
========================================
  Files        1213     1213            
  Lines       97619    97619            
  Branches    13091    12653   -438     
========================================
  Hits        71896    71896            
  Misses      25632    25632            
  Partials       91       91            
Flag Coverage Δ
app 67.30% <ø> (ø)
database 85.14% <ø> (ø)
packages/agent-runtime 88.57% <ø> (ø)
packages/context-engine 85.52% <ø> (ø)
packages/conversation-flow 92.32% <ø> (ø)
packages/file-loaders 87.04% <ø> (ø)
packages/memory-user-memory 67.12% <ø> (ø)
packages/model-bank 100.00% <ø> (ø)
packages/model-runtime 86.72% <ø> (ø)
packages/prompts 78.32% <ø> (ø)
packages/python-interpreter 92.90% <ø> (ø)
packages/ssrf-safe-fetch 0.00% <ø> (ø)
packages/utils 93.07% <ø> (ø)
packages/web-crawler 95.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Store 67.24% <ø> (ø)
Services 50.30% <ø> (ø)
Server 68.33% <ø> (ø)
Libs 40.22% <ø> (ø)
Utils 93.82% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jan 25, 2026
@arvinxx arvinxx merged commit c3fd2dc into next Jan 25, 2026
50 checks passed
@arvinxx arvinxx deleted the fix/desktop-dynamic-import-side-effects branch January 25, 2026 18:10
@lobehubbot

Copy link
Copy Markdown
Member

❤️ Great PR @Innei ❤️

The growth of project is inseparable from user feedback and contribution, thanks for your contribution! If you are interesting with the lobehub developer community, please join our discord and then dm @arvinxx or @canisminor1990. They will invite you to our private developer channel. We are talking about the lobe-chat development or sharing ai newsletter around the world.

lobehubbot pushed a commit that referenced this pull request Jan 25, 2026
## [Version&nbsp;2.0.0-next.379](v2.0.0-next.378...v2.0.0-next.379)
<sup>Released on **2026-01-25**</sup>

#### ✨ Features

- **utils**: Added `trimBasedOnBatchProbe` for truncating without compromising structured data.

#### 🐛 Bug Fixes

- **desktop**: Prevent duplicate IPC handler registration from dynamic imports.
- **misc**: Fix update memory tools, resolve server version check issue for desktop app, slove the descktop use offical endpoint mcp not use stdio.

<br/>

<details>
<summary><kbd>Improvements and Fixes</kbd></summary>

#### What's improved

* **utils**: Added `trimBasedOnBatchProbe` for truncating without compromising structured data, closes [#11836](#11836) ([6dac3d1](6dac3d1))

#### What's fixed

* **desktop**: Prevent duplicate IPC handler registration from dynamic imports, closes [#11827](#11827) ([c3fd2dc](c3fd2dc))
* **misc**: Fix update memory tools, closes [#11831](#11831) ([cfc03dd](cfc03dd))
* **misc**: Resolve server version check issue for desktop app, closes [#11834](#11834) ([0bd2a59](0bd2a59))
* **misc**: Slove the descktop use offical endpoint mcp not use stdio, closes [#11813](#11813) ([370bf16](370bf16))

</details>

<div align="right">

[![](https://img.shields.io/badge/-BACK_TO_TOP-151515?style=flat-square)](#readme-top)

</div>
@lobehubbot

Copy link
Copy Markdown
Member

🎉 This PR is included in version 2.0.0-next.379 🎉

The release is available on:

Your semantic-release bot 📦🚀

arvinxx added a commit that referenced this pull request May 17, 2026
…eged should be called before app is ready`

Two-part fix for a regression where reading any text/JSON/source file via the
local-system `readFile` tool surfaced an Electron protocol error in the response
content. The error fired *after* `stat()` succeeded (so missing-file ENOENT was
unaffected), making it look like the file couldn't be parsed.

## Root cause

Stack trace (instrumented `read.ts` to capture it):

```
Error: protocol.registerSchemesAsPrivileged should be called before app is ready
    at new App (apps/desktop/dist/main/index.js:105339:21)
    at Module.<anonymous> (apps/desktop/dist/main/index.js:105615:11)
    at Module._compile (...)
```

`Module._compile` on `dist/main/index.js` means the main bundle is being freshly
evaluated as a CJS module — re-running its top-level `var app = new App(); …;
app.bootstrap();` after the real Electron-launched App was already ready.

Triggering chain: agent calls `readFile` → main runs `loadFile(path)` from
`@lobechat/file-loaders` → `getFileLoader('txt')` → `await import('./text')`.
The lazy text-loader chunk back-references the main bundle for the shared util
`detectUtf16NoBom`:

```js
// dist/main/text-Cbmlmtca.js
const require_index = require("./index.js");      // ← re-evaluates main
…
const variant = require_index.detectUtf16NoBom(buffer);
```

Electron's main entry is not in Node's CJS module cache (it's bootstrapped
separately), so this `require("./index.js")` triggers a fresh compile of the
main bundle — re-running `new App()` and `protocol.registerSchemesAsPrivileged`
*after* `app.whenReady()`, which is illegal per Electron's API contract.

Introduced by #14602 (`fix(local-system): guard readFile against binary blobs
and oversized output`): adding `isBinaryContent.ts` made `detectUtf16NoBom`
shared between the main bundle (via `sniffBinaryFile`) and the lazy text chunk,
so rolldown placed it in main and rewrote the text chunk's call as a
`require_index.detectUtf16NoBom`.

Identical class of bug previously fixed for the `debug` package in #11827.

## Fix

1. **`packages/file-loaders/src/loaders/index.ts`** — TextLoader was lazy-imported
   for no real benefit. It's a 10KB module whose only deps are `node:fs/promises`
   and a tiny utf-16 detect util — nothing like the multi-MB parsers (pdfjs-dist,
   xlsx, mammoth) that the lazy pattern was designed for. Make it a static
   import; `getFileLoader('txt')` returns it synchronously. Result: the text
   chunk disappears entirely, removing this back-reference at the source.

2. **`apps/desktop/electron.vite.config.ts`** — defensive `manualChunks` rules
   so any future shared symbol doesn't recreate the same trap:
   - `vendor-file-loaders-utils` for the three small text/binary detection
     utils (`detectUtf16` / `isBinaryContent` / `isTextReadableFile`).
     Explicitly enumerated to avoid catching `parser-utils.ts`, which pulls
     in xmldom/yauzl/concat-stream (≈900KB) and belongs in the docx/pptx
     chunks instead.
   - `vendor-jszip` for JSZip — same root cause for `.docx` reads: the docx
     chunk had `require_index.require_lib()` (JSZip) back-referencing main.
     Both ends now share the vendor chunk; no main re-eval.

Follows the project precedent set by #11827 for `debug`.

## Verification (live Electron via CDP)

Bundle inventory before/after:

| Chunk | Before | After |
| --- | --- | --- |
| `text-*.js` | 9.7KB (back-refs main) | (gone, inlined into main) |
| `vendor-file-loaders-utils-*.js` | n/a | 18KB |
| `vendor-jszip-*.js` | n/a | 899KB |
| `docx-*.js` back-refs | `require_index.require_lib` | none |

End-to-end via `tool.invokeBuiltinTool('lobe-local-system', 'readFile', …)`:

| File | Before | After |
| --- | --- | --- |
| `.md` / `.json` / `.ts` | `Error accessing or processing file: protocol.registerSchemesAsPrivileged should be called before app is ready` | real file content |

`grep -o 'require_index\\.[a-zA-Z_]*' dist/main/*-*.js | sort -u` → empty.

All 61 file-loaders tests pass; all 64 builtin-tool-local-system tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arvinxx added a commit that referenced this pull request May 17, 2026
…o /client (#14888)

* 🐛 fix(local-system): forward all grepContent params + move executor to /client

The local-system executor was reducing the agent's full grepContent params
({pattern, glob, output_mode, -i/-n/-A/-B/-C, multiline, head_limit, type,
scope, ...}) down to {directory, pattern} before handing them to the runtime.
`directory` isn't recognized by the IPC layer (which expects path/scope), so
cwd silently fell back to process.cwd() (= apps/desktop/ in dev), and with
glob/-i/output_mode all stripped grep matched anything containing the pattern
across the whole tree — explaining LOBE-8666's dist/main/index.js +
tsconfig.tsbuildinfo leaks.

Also audited the rest of the executor layer:
- listFiles: forward `limit` (was silently dropped → manifest default of 100
  always won).
- getCommandOutput: forward `filter` (was silently dropped → no regex filter
  ever applied to streamed output).
- runCommand: mirror `run_in_background` → `background` so
  ComputerRuntime.RunCommandState.isBackground reflects reality (the IPC
  handler reads run_in_background directly, so the command itself ran in
  background — only the state field was wrong).

Structure: moved src/executor/ → src/client/executor/ to match the other
builtin-tool packages (task / lobe-agent / knowledge-base) and consolidate
renderer-only code under /client. Dropped the `./executor` package subpath;
consumers now import from `…/client`.

Defensive: also added a resolveSearchPath helper in apps/desktop's
contentSearch module that reads params.scope as a fallback for params.path,
so any non-executor caller (direct IPC, future Gateway path) that passes
`scope` still gets routed correctly instead of falling through to
process.cwd().

Regression coverage:
- grepContent full forwarding (LOBE-8666 case + all optional flags)
- listFiles.limit forwarding
- getCommandOutput.filter forwarding
- runCommand.run_in_background → background mirror
- resolveSearchPath fallback semantics (3 cases in base.test.ts)

Verified end-to-end via Electron CDP — tool.invokeBuiltinTool with the
LOBE-8666 params returns 9 clean .ts matches (no dist/, no .tsbuildinfo);
listFiles {limit:3} returns 3 files (totalCount 10); runCommand
{run_in_background:true} reports state.isBackground=true.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* 🐛 fix(desktop): readFile fails with `protocol.registerSchemesAsPrivileged should be called before app is ready`

Two-part fix for a regression where reading any text/JSON/source file via the
local-system `readFile` tool surfaced an Electron protocol error in the response
content. The error fired *after* `stat()` succeeded (so missing-file ENOENT was
unaffected), making it look like the file couldn't be parsed.

## Root cause

Stack trace (instrumented `read.ts` to capture it):

```
Error: protocol.registerSchemesAsPrivileged should be called before app is ready
    at new App (apps/desktop/dist/main/index.js:105339:21)
    at Module.<anonymous> (apps/desktop/dist/main/index.js:105615:11)
    at Module._compile (...)
```

`Module._compile` on `dist/main/index.js` means the main bundle is being freshly
evaluated as a CJS module — re-running its top-level `var app = new App(); …;
app.bootstrap();` after the real Electron-launched App was already ready.

Triggering chain: agent calls `readFile` → main runs `loadFile(path)` from
`@lobechat/file-loaders` → `getFileLoader('txt')` → `await import('./text')`.
The lazy text-loader chunk back-references the main bundle for the shared util
`detectUtf16NoBom`:

```js
// dist/main/text-Cbmlmtca.js
const require_index = require("./index.js");      // ← re-evaluates main
…
const variant = require_index.detectUtf16NoBom(buffer);
```

Electron's main entry is not in Node's CJS module cache (it's bootstrapped
separately), so this `require("./index.js")` triggers a fresh compile of the
main bundle — re-running `new App()` and `protocol.registerSchemesAsPrivileged`
*after* `app.whenReady()`, which is illegal per Electron's API contract.

Introduced by #14602 (`fix(local-system): guard readFile against binary blobs
and oversized output`): adding `isBinaryContent.ts` made `detectUtf16NoBom`
shared between the main bundle (via `sniffBinaryFile`) and the lazy text chunk,
so rolldown placed it in main and rewrote the text chunk's call as a
`require_index.detectUtf16NoBom`.

Identical class of bug previously fixed for the `debug` package in #11827.

## Fix

1. **`packages/file-loaders/src/loaders/index.ts`** — TextLoader was lazy-imported
   for no real benefit. It's a 10KB module whose only deps are `node:fs/promises`
   and a tiny utf-16 detect util — nothing like the multi-MB parsers (pdfjs-dist,
   xlsx, mammoth) that the lazy pattern was designed for. Make it a static
   import; `getFileLoader('txt')` returns it synchronously. Result: the text
   chunk disappears entirely, removing this back-reference at the source.

2. **`apps/desktop/electron.vite.config.ts`** — defensive `manualChunks` rules
   so any future shared symbol doesn't recreate the same trap:
   - `vendor-file-loaders-utils` for the three small text/binary detection
     utils (`detectUtf16` / `isBinaryContent` / `isTextReadableFile`).
     Explicitly enumerated to avoid catching `parser-utils.ts`, which pulls
     in xmldom/yauzl/concat-stream (≈900KB) and belongs in the docx/pptx
     chunks instead.
   - `vendor-jszip` for JSZip — same root cause for `.docx` reads: the docx
     chunk had `require_index.require_lib()` (JSZip) back-referencing main.
     Both ends now share the vendor chunk; no main re-eval.

Follows the project precedent set by #11827 for `debug`.

## Verification (live Electron via CDP)

Bundle inventory before/after:

| Chunk | Before | After |
| --- | --- | --- |
| `text-*.js` | 9.7KB (back-refs main) | (gone, inlined into main) |
| `vendor-file-loaders-utils-*.js` | n/a | 18KB |
| `vendor-jszip-*.js` | n/a | 899KB |
| `docx-*.js` back-refs | `require_index.require_lib` | none |

End-to-end via `tool.invokeBuiltinTool('lobe-local-system', 'readFile', …)`:

| File | Before | After |
| --- | --- | --- |
| `.md` / `.json` / `.ts` | `Error accessing or processing file: protocol.registerSchemesAsPrivileged should be called before app is ready` | real file content |

`grep -o 'require_index\\.[a-zA-Z_]*' dist/main/*-*.js | sort -u` → empty.

All 61 file-loaders tests pass; all 64 builtin-tool-local-system tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 Bug released on @next size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants