Skip to content

Desktop: Fixes #15189: Fix Rich Text Editor fails to load in secondary windows #15190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
personalizedrefrigerator wants to merge 3 commits into
base: dev
Choose a base branch
from
+40 −6
Open

Desktop: Fixes #15189: Fix Rich Text Editor fails to load in secondary windows #15190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8db180
Desktop: Fixes #15189: Revert #15035
personalizedrefrigerator Apr 23, 2026
1a58102
Fix crash when closing windows
personalizedrefrigerator Apr 23, 2026
b279cb4
Commenting (for consistency)
personalizedrefrigerator Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/app-desktop/ElectronAppWrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ export default class ElectronAppWrapper {
action: 'allow',
overrideBrowserWindowOptions: {
webPreferences: {
nodeIntegration: false,
preload: resolve(__dirname, './utils/window/secondaryWindowPreload.js'),
},
},
Comment on lines 413 to 418
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check whether contextIsolation is configured anywhere for secondary windows
# and confirm the preload script's assignment pattern.
rg -nP -C3 '\bcontextIsolation\b'
echo '---'
fd -t f secondaryWindowPreload.js --exec cat {}
echo '---'
rg -nP -C2 '\belectronWindow\b'

Repository: laurent22/joplin

Length of output: 4161

Add explicit contextIsolation: false to secondary window configuration.

Disabling nodeIntegration is correct, but the preload script pattern requires explicit attention: secondaryWindowPreload.js directly assigns to window.electronWindow without using contextBridge.exposeInMainWorld. This only works if contextIsolation is false.

The configuration does not explicitly set contextIsolation for these secondary windows. Whilst the main window sets contextIsolation: false (line 273), secondary windows (lines 413–418) lack this setting. They may inherit it from the opener or use Electron's default (which varies by version and has shifted to true in newer versions). To ensure this code works reliably across platforms and Electron versions, explicitly set contextIsolation: false in the secondary window's webPreferences:

webPreferences: {
    nodeIntegration: false,
    contextIsolation: false,
    preload: resolve(__dirname, './utils/window/secondaryWindowPreload.js'),
},

Alternatively, refactor the preload to use contextBridge.exposeInMainWorld('electronWindow', {...}) and update NewWindowOrIFrame.tsx accordingly.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/app-desktop/ElectronAppWrapper.ts` around lines 413 - 418, The
secondary window options in overrideBrowserWindowOptions omit contextIsolation
and rely on defaults, but secondaryWindowPreload.js assigns directly to
window.electronWindow (which requires contextIsolation: false); update the
webPreferences for the secondary window in overrideBrowserWindowOptions to
explicitly set contextIsolation: false (or alternatively refactor
secondaryWindowPreload.js to use contextBridge.exposeInMainWorld and update
NewWindowOrIFrame.tsx to consume that API) so the preload script works reliably
across Electron versions.

Copy link
Copy Markdown
Collaborator Author

@personalizedrefrigerator personalizedrefrigerator Apr 23, 2026

Choose a reason for hiding this comment

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

Out-of-scope (no such parameter was present previously).

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

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

@personalizedrefrigerator, understood — since contextIsolation was not present in the secondary window configuration prior to this PR, flagging it here is indeed out of scope. Apologies for the noise!

(╯°□°）╯

🧠 Learnings used 
Learnt from: personalizedrefrigerator
Repo: laurent22/joplin PR: 14882
File: packages/app-desktop/tools/electronRebuild.js:28-28
Timestamp: 2026-03-30T18:14:58.234Z
Learning: For Electron upgrades in Joplin's `packages/app-desktop/tools/electronRebuild.js`, the `--force-abi` value passed to `electron-rebuild` must match the Node.js `NODE_MODULE_VERSION` (i.e., `process.versions.modules`) for the target Electron version — NOT the Chromium version bundled with that Electron release. These are distinct versioning schemes. The official mapping can be found at: https://github.com/nodejs/node/blob/main/doc/abi_version_registry.json and https://github.com/electron/node-abi/blob/main/abi_registry.json.

Learnt from: Arunava07M
Repo: laurent22/joplin PR: 14673
File: packages/app-desktop/gui/NoteEditor/utils/contextMenu.ts:259-265
Timestamp: 2026-03-10T10:25:43.482Z
Learning: In the Joplin Desktop app (packages/app-desktop/gui/NoteEditor/utils/contextMenu.ts), Electron's context menu `isEditable` param (which maps to `options.isReadOnly`) reports `false` for `<a>` (link) elements even when they are inside an actively editable TinyMCE editor, because `isEditable` reflects the element type, not the surrounding document's writability. Therefore, `linkCut`/`linkCopy` context menu items for links must NOT guard with `!options.isReadOnly`, as doing so would hide Cut/Copy even for links in fully editable notes.

Learnt from: personalizedrefrigerator
Repo: laurent22/joplin PR: 15127
File: packages/lib/services/interop/InteropService_Importer_OneNote.ts:38-42
Timestamp: 2026-04-16T16:54:45.745Z
Learning: In `packages/lib/shim.ts`, `shim.electronBridge()` throws `new Error('Not implemented: electronBridge')` by default when the bridge is not initialized — it does NOT return null/undefined. Optional chaining on its return value provides no protection. The correct guard is `shim.isElectron()`, which ensures the bridge is properly set up before calling it.

Learnt from: Rygaa
Repo: laurent22/joplin PR: 15026
File: packages/htmlpack/packToWriter.ts:178-200
Timestamp: 2026-04-15T02:09:18.869Z
Learning: In PR `#15026` for laurent22/joplin, the OOM bug (`#13903`) when printing/exporting HTML with large attachments occurred during the Node.js export process in `InteropService_Exporter_Html.ts`, not in the browser renderer. The fix uses `streamFileDataUri` to read large files in 3 MB chunks (`chunkBytes = 3 * 1024 * 1024`; chunk size must be divisible by 3 to avoid base64 padding mid-stream) and `writeChunk` which calls `appendFile` to flush each chunk to a temp file on disk immediately, so the full file content is never in memory at once. The standalone `htmlpack/index.ts` CLI still buffers chunks in a `string[]` but that path is not used by Joplin Desktop export/print.

Learnt from: mrjo118
Repo: laurent22/joplin PR: 15122
File: packages/app-mobile/components/screens/encryption-config.tsx:161-169
Timestamp: 2026-04-16T11:29:53.970Z
Learning: In the Joplin codebase, after the fix referenced in PR `#15122`, master keys are never successfully loaded if the password is incorrect. This means the encryption UI in `packages/app-mobile/components/screens/encryption-config.tsx` does not need an additional explicit `checkMasterKeyPassword` call before `toggleAndSetupEncryption` when `props.masterKeys.length > 0`, because the system-wide protection already prevents use of an incorrect password.

Learnt from: personalizedrefrigerator
Repo: laurent22/joplin PR: 14392
File: packages/server/src/utils/requestUtils.ts:84-93
Timestamp: 2026-02-19T23:59:04.938Z
Learning: In all TypeScript files across the repository, prefer template literals (backticks) for string construction instead of string concatenation with +. Use ${…} placeholders where needed to interpolate variables, which improves readability and reduces errors from accidental type coercion.

Learnt from: personalizedrefrigerator
Repo: laurent22/joplin PR: 14591
File: packages/app-desktop/gui/NoteList/utils/UseAutoScroll.test.ts:8-8
Timestamp: 2026-03-05T21:58:10.960Z
Learning: In TypeScript type and interface signatures (e.g., type Props = { makeItemIndexVisible: (index: number) => void }), parameter names are part of the contract and should not be treated as unused variables. The no-unused-vars rule can report false positives on type-level parameters; do not rename them with underscores to appease linting. If you encounter such reports, adjust lint config to apply to type declarations appropriately (and avoid masking the parameter names) rather than altering the type signatures.

Learnt from: personalizedrefrigerator
Repo: laurent22/joplin PR: 14847
File: packages/app-mobile/components/voiceTyping/AudioRecordingBanner.tsx:95-102
Timestamp: 2026-03-20T15:45:28.564Z
Learning: When using the `expo-audio` package (not `expo-av`) and calling `setAudioModeAsync`, do not flag `allowsBackgroundRecording` as an invalid `AudioMode` property. In `expo-audio`, `allowsBackgroundRecording` is an accepted `AudioMode` field that controls whether audio recording/playback continues when the app is in the background (Expo SDK v54+ audio docs).

Expand Down
6 changes: 6 additions & 0 deletions packages/app-desktop/gui/NewWindowOrIFrame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ const useDocument = (
setDoc(iframeElement?.contentWindow?.document);
} else if (mode === WindowMode.NewWindow) {
openedWindow = window.open('about:blank');

// Required to support TinyMCE:
openedWindow.document.open();
openedWindow.document.write('<!DOCTYPE html><html><head></head><body></body></html>');
openedWindow.document.close();

setDoc(openedWindow.document);
Comment on lines 46 to 53
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Minor: no null-check on window.open result.

window.open('about:blank') can return null (e.g. if the browser/Electron refuses the popup). In that case openedWindow.document.open() will throw, leaving the effect mid-setup and preventing onClose from firing on unmount (the cleanup guards with openedWindow?.closed, so the onClose path is skipped too).

This was already fragile before the change (setDoc(openedWindow.document) would have thrown on the same condition), but since you're explicitly expanding the initialisation here, it's a good opportunity to add a guard:

🛡️ Suggested guard 
 } else if (mode === WindowMode.NewWindow) {
 	openedWindow = window.open('about:blank');
+	if (!openedWindow) {
+		// Popup was blocked or could not be created — nothing more to set up.
+		return () => {};
+	}

 	// Required to support TinyMCE:
 	openedWindow.document.open();
 	openedWindow.document.write('<!DOCTYPE html><html><head></head><body></body></html>');
 	openedWindow.document.close();
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/app-desktop/gui/NewWindowOrIFrame.tsx` around lines 46 - 53, The
code uses the result of window.open('about:blank') without checking for null
(openedWindow) which can throw when the popup is blocked; update the
NewWindowOrIFrame.tsx effect to check if openedWindow is null before calling
openedWindow.document.open()/write()/close() or setDoc, and handle the
popup-blocked case by invoking the same onClose cleanup path (or early-return
from the effect) so the cleanup guards (openedWindow?.closed) still work;
specifically, add a null-check around the openedWindow accesses (the variable
named openedWindow and the setDoc call) and ensure onClose is called or the
effect exits cleanly when window.open returns null.


// .onbeforeunload and .onclose events don't seem to fire when closed by a user -- rely on polling
Expand Down
19 changes: 16 additions & 3 deletions packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useContextMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import { menuItems } from '../../../utils/contextMenu';
import isItemId from '@joplin/lib/models/utils/isItemId';
import { extractResourceUrls } from '@joplin/lib/urlUtils';
import { WindowIdContext } from '../../../../NewWindowOrIFrame';
import Logger from '@joplin/utils/Logger';

const logger = Logger.create('useContextMenu');

export type ResourceMarkupType = 'image' | 'file';

Expand Down Expand Up @@ -355,11 +358,21 @@ const useContextMenu = (props: ContextMenuProps) => {

// Prepend the event listener so that it gets called before
// the listener that shows the default menu.
targetWindow.webContents.prependListener('context-menu', onContextMenu);
try {
targetWindow.webContents.prependListener('context-menu', onContextMenu);
} catch (error) {
logger.warn('Error registering menu', error);
}

return () => {
if (!targetWindow.isDestroyed()) {
targetWindow.webContents.off('context-menu', onContextMenu);
try {
if (!targetWindow.isDestroyed()) {
targetWindow.webContents.off('context-menu', onContextMenu);
}
} catch (error) {
// This can happen if the window closes after the isDestroyed check, but before webContents.off
// finishes running.
logger.warn('Error removing menu listener', error);
}
};
}, [
Expand Down
20 changes: 17 additions & 3 deletions packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/utils/useContextMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import { _ } from '@joplin/lib/locale';
import type { MenuItem as MenuItemType } from 'electron';
import isItemId from '@joplin/lib/models/utils/isItemId';
import { WindowIdContext } from '../../../../NewWindowOrIFrame';
import Logger from '@joplin/utils/Logger';

const logger = Logger.create('useContextMenu');

const Menu = bridge().Menu;
const MenuItem = bridge().MenuItem;
Expand Down Expand Up @@ -157,13 +160,24 @@ export default function(editor: Editor, plugins: PluginStates, dispatch: Dispatc
}
};

targetWindow.webContents.prependListener('context-menu', onElectronContextMenu);
try {
targetWindow.webContents.prependListener('context-menu', onElectronContextMenu);
} catch (error) {
logger.error('Failed to register context menu', error);
}
editor.on('contextmenu', onBrowserContextMenu);

return () => {
editor.off('contextmenu', onBrowserContextMenu);
if (!targetWindow.isDestroyed() && targetWindow?.webContents?.off) {
targetWindow.webContents.off('context-menu', onElectronContextMenu);

try {
if (!targetWindow.isDestroyed() && targetWindow?.webContents?.off) {
targetWindow.webContents.off('context-menu', onElectronContextMenu);
}
} catch (error) {
// This can happen if the window closes after the isDestroyed check, but before webContents.off
// finishes running.
logger.error('Error removing context menu listener', error);
}
};
}, [editor, plugins, dispatch, htmlToMd, mdToHtml, editDialog, windowId]);
Expand Down
Loading