fix(linux): add wrapper script to prevent Wayland/X11 crashes#3172
fix(linux): add wrapper script to prevent Wayland/X11 crashes#3172jeanfbrito wants to merge 6 commits intoRocketChat:masterfrom
Conversation
- Updated the Linux display server configuration to automatically detect Wayland sessions and fallback to X11 if necessary, preventing crashes during initialization. - Improved documentation to clarify the automatic detection process and added troubleshooting information for Ubuntu 22.04 LTS users experiencing segfaults. - Introduced unit tests for the platform detection logic to ensure correct behavior across various session types and manual overrides.
- Refactored logging tests in `app.main.spec.ts` to validate JSON structure of log messages for Wayland and X11 sessions. - Removed outdated tests for session type handling and improved assertions for log data properties. - Enhanced clarity in test descriptions to reflect the updated logging behavior during Electron startup.
…ection tests - Eliminated unnecessary assertions for JSON parsing in the logging tests for Wayland and X11 sessions in `app.main.spec.ts`. - Streamlined test cases to focus on validating the structure of log data without redundant error handling, enhancing clarity and maintainability.
|
|
|
Warning Rate limit exceeded@jeanfbrito has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a Linux display-server detection and wrapper flow: a bash wrapper installed at packaging, runtime pre-start detection in the Electron main process that conditionally forces X11 via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Wrapper as build/linux/wrapper.sh
participant OS as OS Env (XDG/WAYLAND/socket)
participant Binary as rocketchat-desktop.bin
participant App as Electron Main (app.ts)
participant Chromium as Chromium/Electron
User->>Wrapper: launch `rocketchat-desktop`
Wrapper->>OS: read XDG_SESSION_TYPE, WAYLAND_DISPLAY, check socket
alt Wrapper decides X11 required
Wrapper->>Binary: exec with `--ozone-platform=x11` + args
else Wrapper allows auto-detect
Wrapper->>Binary: exec with original args
end
Binary->>App: start (pre-Electron initialization)
App->>OS: if Linux & no manual override, read/trim env vars, check socket
alt App forces X11
App->>Chromium: append `--ozone-platform=x11`
else App leaves auto-detect
App->>Chromium: no ozone switch
end
Chromium->>User: initialize UI with selected platform
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…Chat#3154) The app.commandLine.appendSwitch() approach doesn't work because Chromium initializes before Electron JavaScript runs. Fix by package type: - deb/rpm/tar.gz: Wrapper script runs BEFORE binary, detects display server - Snap: Set allowNativeWayland=false to force X11 via electron-builder - Flatpak/AppImage: Use electron-builder launcher with X11 fallback Wrapper script detection logic: - If XDG_SESSION_TYPE != wayland: force X11 - If WAYLAND_DISPLAY is empty: force X11 - If Wayland socket doesn't exist: force X11 - Otherwise: use native Wayland Validated on: - Fedora 42 physical (GTX 1660 Ti) - Wayland native - Ubuntu 22.04 physical (GTX 1660 Ti) - X11 session - Fedora 42 VM (no GPU) - Ubuntu 22.04 VM (no GPU) All 5 test scenarios pass with fix vs SEGFAULT without. Closes RocketChat#3154
jeanfbrito
force-pushed
the
fix-x11-ubuntu2204
branch
from
23d45a1 to
2509acf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/linux-wayland-bug-postmortem.md:
- Around line 299-308: In the "Timeline" table remove or replace the placeholder
dates "2024-XX-XX" with verifiable dates or delete those rows; specifically edit
the table rows containing "2024-XX-XX | Issue #3154 reported" and "2024-XX-XX |
PR #3171 submitted with `app.commandLine.appendSwitch()` fix" so they either
contain confirmed GitHub issue/PR dates or are removed until accurate dates can
be sourced.
In @src/app/main/app.ts:
- Around line 166-223: The code allows Wayland when WAYLAND_DISPLAY is non-empty
but doesn't verify the socket exists, so add a filesystem existence check for
the Wayland socket before treating isWaylandSession as true: compute the socket
path using process.env.XDG_RUNTIME_DIR (fallback to
`/run/user/${process.getuid()}`) joined with normalizedWaylandDisplay, test that
file exists and is a socket, and only set isWaylandSession to true if that check
passes; update the fallback reason handling to return 'socket-not-found' when
the env is set but the socket is missing, and keep the existing logging and
app.commandLine.appendSwitch('ozone-platform','x11') behavior for the X11
branch.
- Around line 169-171: Add a brief inline comment next to the
hasOzonePlatformOverride check explaining why ELECTRON_OZONE_PLATFORM_HINT is
considered an override (e.g., "check for env var override set by wrapper or user
to select Ozone backend on Linux"), so future maintainers understand the intent;
update the comment near the args.some(...) / ELECTRON_OZONE_PLATFORM_HINT usage
and optionally add unit tests covering when
process.env.ELECTRON_OZONE_PLATFORM_HINT is set to ensure behavior is detected.
🧹 Nitpick comments (5)
docs/linux-wayland-bug-postmortem.md (3)
24-35: Add language specifier to fenced code block.The error output block should specify a language for better rendering and to satisfy markdownlint rules.
📝 Suggested fix
-``` +```text ERROR:ui/ozone/platform/wayland/host/wayland_connection.cc:202 Failed to connect to Wayland display: No such file or directory (2)
147-169: Add language specifiers to process timeline diagrams.The process flow diagrams should specify "text" as the language for better rendering and to satisfy markdownlint rules.
📝 Suggested fix
For the first diagram (lines 147-169):
-``` +```text Process Startup Timeline:For the second diagram (lines 178-203):
-``` +```text Process Startup Timeline with Wrapper:Also applies to: 178-203
327-330: Format reference URLs as markdown links.The reference URLs should use markdown link syntax for better presentation and to satisfy markdownlint rules.
🔗 Suggested fix
-- Chromium Ozone Platform: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md -- Electron Command Line Switches: https://www.electronjs.org/docs/latest/api/command-line-switches -- Wayland Protocol: https://wayland.freedesktop.org/ -- XDG Base Directory Specification: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html +- [Chromium Ozone Platform](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md) +- [Electron Command Line Switches](https://www.electronjs.org/docs/latest/api/command-line-switches) +- [Wayland Protocol](https://wayland.freedesktop.org/) +- [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)build/afterPack.js (1)
54-56: Consider moving function definition before usage for clarity.While JavaScript hoists async function declarations (so this code works correctly), defining
setupLinuxWrapperbefore it's called improves code readability and eliminates the static analysis warning.♻️ Suggested refactor
Move the
setupLinuxWrapperfunction definition (lines 59-82) to appear before the mainafterPackfunction (before line 5), or at minimum before it's called (before line 54).src/app/main/app.main.spec.ts (1)
78-132: Consider adding test coverage forELECTRON_OZONE_PLATFORM_HINT.The manual override tests verify command-line arguments (lines 79-114) but don't test the
ELECTRON_OZONE_PLATFORM_HINTenvironment variable path that's checked in the production code (app.ts line 171).📋 Suggested test case
Add a test case to verify the environment variable override:
it('should respect ELECTRON_OZONE_PLATFORM_HINT environment variable', () => { process.env.ELECTRON_OZONE_PLATFORM_HINT = 'x11'; process.env.XDG_SESSION_TYPE = 'wayland'; process.env.WAYLAND_DISPLAY = 'wayland-0'; performElectronStartup(); const ozonePlatformCalls = appendSwitchMock.mock.calls.filter( (call) => call[0] === 'ozone-platform' ); expect(ozonePlatformCalls).toHaveLength(0); // Verify no detection logs are emitted const detectionLogs = consoleLogMock.mock.calls.filter((call) => call[0]?.includes('Wayland') || call[0]?.includes('X11') ); expect(detectionLogs).toHaveLength(0); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
build/afterPack.jsbuild/linux/wrapper.shdocs/linux-display-server.mddocs/linux-wayland-bug-postmortem.mdelectron-builder.jsonsrc/app/main/app.main.spec.tssrc/app/main/app.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/app/main/app.tssrc/app/main/app.main.spec.ts
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
docs/**/*.md: Never invent or estimate metrics/time; only include verifiable numbers in documentation and reports
Structure technical post-mortems with a top section “The Solution That Actually Worked” including Problem, Solution, Result, and PR
Place all documentation in thedocs/directory using Markdown with clear, descriptive filenames and simple language
Before adding features, check and update existing docs; create/extend docs for new or changed features
Files:
docs/linux-display-server.mddocs/linux-wayland-bug-postmortem.md
src/**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Main-process tests must be named with the suffix
.main.spec.tsand run under the Electron runner
Files:
src/app/main/app.main.spec.ts
{rollup.config.mjs,electron-builder.json,tsconfig.json,.eslintrc.json,jest.config.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain and configure the project via the designated root config files for build, packaging, TypeScript, ESLint, and Jest
Files:
electron-builder.json
🧠 Learnings (4)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/main.ts : Keep `src/main.ts` as the Electron main process entry point compiled by Rollup
Applied to files:
build/afterPack.jssrc/app/main/app.tssrc/app/main/app.main.spec.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
Applied to files:
src/app/main/app.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/**/*.main.spec.ts : Main-process tests must be named with the suffix `.main.spec.ts` and run under the Electron runner
Applied to files:
src/app/main/app.main.spec.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/**/!(*.main).spec.ts : Renderer-process tests should be named `*.spec.ts` (without the `.main` qualifier)
Applied to files:
src/app/main/app.main.spec.ts
🧬 Code graph analysis (1)
build/afterPack.js (1)
build/notarize.js (2)
context(5-5)context(29-29)
🪛 GitHub Check: check (ubuntu-latest)
build/afterPack.js
[failure] 55-55:
'setupLinuxWrapper' was used before it was defined
🪛 markdownlint-cli2 (0.18.1)
docs/linux-wayland-bug-postmortem.md
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
327-327: Bare URL used
(MD034, no-bare-urls)
328-328: Bare URL used
(MD034, no-bare-urls)
329-329: Bare URL used
(MD034, no-bare-urls)
330-330: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🔇 Additional comments (7)
electron-builder.json (1)
150-150: LGTM! Snap configuration aligns with wrapper-based approach.Adding
allowNativeWayland: falseensures Snap packages use the X11 fallback behavior, which is consistent with the wrapper script detection logic applied to other package formats (deb, rpm, AppImage, tar.gz).docs/linux-display-server.md (1)
1-146: Excellent documentation! Clear and comprehensive.The updated documentation effectively explains the wrapper-based detection approach, provides helpful troubleshooting steps, and covers common scenarios. The structure is user-friendly and the technical explanations are accurate.
build/linux/wrapper.sh (1)
1-29: LGTM! Robust shell script with proper error handling.The wrapper script correctly implements the detection logic:
- Uses
set -euo pipefailfor strict error handling- Properly resolves paths with
readlink -f- Detection function checks session type, display variable, and socket existence
- Uses
execto replace the shell process efficiently- Correctly handles user arguments with
"$@"The unquoted
$EXTRA_ARGSon line 29 is intentional and correct—it expands to nothing when empty or a single argument when set, which is the desired behavior.build/afterPack.js (1)
59-82: Wrapper installation logic is correct and well-structured.The
setupLinuxWrapperfunction properly:
- Extracts the output directory from context
- Checks if wrapper is already installed (idempotent)
- Renames the binary to
.bin- Copies and makes the wrapper executable
- Provides clear logging throughout
The implementation aligns with the documented wrapper approach and handles the re-run case correctly.
src/app/main/app.ts (1)
166-223: Clarify the relationship with the wrapper script and verify timing assumptions.The PR objectives state that
app.commandLine.appendSwitch('ozone-platform', 'x11')is "ineffective" because "Chromium's display platform initialization occurs before Electron JavaScript." However, this code still uses that approach (line 221).Additionally, the PR objectives indicate the wrapper script (build/linux/wrapper.sh) is the primary solution. It's unclear:
- Whether this TypeScript code is intended as a fallback for development mode, a redundant safety layer, or serves a different purpose
- Whether
performElectronStartup()is called early enough for the command-line switches to take effect, contradicting the PR objectives' statementPlease verify:
- The timing of
performElectronStartup()invocation relative to Chromium's platform initialization- The intended relationship between the wrapper script and this TypeScript detection logic
- Whether this code successfully prevents crashes in development mode (where the wrapper may not be present)
Consider adding comments or documentation explaining when each detection mechanism applies.
src/app/main/app.main.spec.ts (2)
1-469: Excellent test coverage for the platform detection logic.The test suite is well-structured and comprehensive, covering:
- Manual overrides via command-line arguments
- Various Wayland and X11 scenarios
- Normalization of environment variables (whitespace handling)
- Invalid session types
- Logging structure validation (JSON format)
- Platform-specific guards (Linux-only behavior)
- Integration with other command-line switches
The tests follow the naming convention for main-process tests (
.main.spec.tssuffix) and use appropriate mocking strategies.
435-468: Good integration test coverage.The integration tests effectively verify that the platform detection logic doesn't interfere with other command-line switches (autoplay-policy, enable-features) and works correctly alongside PipeWire feature enablement. This is important for ensuring the changes don't have unintended side effects.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @build/afterPack.js:
- Around line 54-82: Move the async function setupLinuxWrapper so it is defined
before afterPack where it's invoked (the call checking
context.electronPlatformName === 'linux'); relocate the entire setupLinuxWrapper
function block above the afterPack function (preserving its name, params, and
internal logic: binaryPath, binaryBinPath, wrapperSrc, wrapperDest,
fs.renameSync, fs.copyFileSync, fs.chmodSync and console logs) so
linters/pipeline no longer flag a function-used-before-defined error.
- Around line 1-3: The import grouping violates ESLint import/order: separate
built-in Node modules from third-party imports with a blank line; update the top
of this file so the third-party import for @electron/fuses (flipFuses,
FuseVersion, FuseV1Options) is in its own group and there is a blank line before
the built-in requires for fs and path (fs, path) to satisfy the rule and resolve
the pipeline failure.
In @docs/linux-wayland-bug-postmortem.md:
- Around line 1-11: Insert a new top section titled "The Solution That Actually
Worked" immediately after the Issue Reference block and before the Executive
Summary, and populate it with four brief subsections: "Problem" summarizing the
Wayland-env-but-no-compositor segfault, "Solution" describing the shell wrapper
approach that sets appropriate command-line flags before the binary runs (rather
than using app.commandLine.appendSwitch()), "Result" stating that the wrapper
prevents the crash and restores stability, and "PR" linking to the related PR
(#3171) and date; keep each subsection concise so readers get a quick overview
before the detailed Executive Summary.
🧹 Nitpick comments (2)
docs/linux-wayland-bug-postmortem.md (1)
24-35: Consider addressing markdown linting issues.The static analysis tool flagged several formatting issues:
- Fenced code blocks at lines 24, 147, and 178 are missing language specifiers
- URLs at lines 327-330 should use proper markdown link syntax
♻️ Suggested fixes
For the error output block:
-``` +```text ERROR:ui/ozone/platform/wayland/host/wayland_connection.cc:202 Failed to connect to Wayland display: No such file or directory (2)For the process timeline diagrams:
-``` +```text Process Startup Timeline:For the references section:
-- Chromium Ozone Platform: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md -- Electron Command Line Switches: https://www.electronjs.org/docs/latest/api/command-line-switches -- Wayland Protocol: https://wayland.freedesktop.org/ -- XDG Base Directory Specification: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html +- [Chromium Ozone Platform](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md) +- [Electron Command Line Switches](https://www.electronjs.org/docs/latest/api/command-line-switches) +- [Wayland Protocol](https://wayland.freedesktop.org/) +- [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)Also applies to: 147-169, 178-203, 327-330
build/afterPack.js (1)
59-82: Wrapper installation logic is correct and idempotent.The implementation properly:
- Checks for existing installation to avoid redundant work
- Renames the binary before installing the wrapper
- Sets correct executable permissions (0o755)
The synchronous fs operations are acceptable for build scripts, though you could consider using async versions (
fs.promises.rename,fs.promises.copyFile,fs.promises.chmod) for consistency since the function is already async.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
build/afterPack.jsbuild/linux/wrapper.shdocs/linux-display-server.mddocs/linux-wayland-bug-postmortem.mdelectron-builder.json
🚧 Files skipped from review as they are similar to previous changes (1)
- build/linux/wrapper.sh
🧰 Additional context used
📓 Path-based instructions (2)
{rollup.config.mjs,electron-builder.json,tsconfig.json,.eslintrc.json,jest.config.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain and configure the project via the designated root config files for build, packaging, TypeScript, ESLint, and Jest
Files:
electron-builder.json
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
docs/**/*.md: Never invent or estimate metrics/time; only include verifiable numbers in documentation and reports
Structure technical post-mortems with a top section “The Solution That Actually Worked” including Problem, Solution, Result, and PR
Place all documentation in thedocs/directory using Markdown with clear, descriptive filenames and simple language
Before adding features, check and update existing docs; create/extend docs for new or changed features
Files:
docs/linux-display-server.mddocs/linux-wayland-bug-postmortem.md
🧠 Learnings (1)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/main.ts : Keep `src/main.ts` as the Electron main process entry point compiled by Rollup
Applied to files:
build/afterPack.js
🧬 Code graph analysis (1)
build/afterPack.js (1)
build/notarize.js (4)
require(1-1)require(2-2)context(5-5)context(29-29)
🪛 GitHub Actions: Validate pull request
build/afterPack.js
[error] 1-1: ESLint: import/order - There should be at least one empty line between import groups.
🪛 GitHub Check: check (macos-latest)
build/afterPack.js
[failure] 55-55:
'setupLinuxWrapper' was used before it was defined
🪛 GitHub Check: check (ubuntu-latest)
build/afterPack.js
[failure] 55-55:
'setupLinuxWrapper' was used before it was defined
🪛 GitHub Check: check (windows-latest)
build/afterPack.js
[failure] 55-55:
'setupLinuxWrapper' was used before it was defined
🪛 markdownlint-cli2 (0.18.1)
docs/linux-wayland-bug-postmortem.md
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
327-327: Bare URL used
(MD034, no-bare-urls)
328-328: Bare URL used
(MD034, no-bare-urls)
329-329: Bare URL used
(MD034, no-bare-urls)
330-330: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
electron-builder.json (1)
150-150: LGTM! Snap configuration correctly disables native Wayland.This aligns with the wrapper-based solution described in the post-mortem documentation, where Snap uses electron-builder's
allowNativeWayland: falsesetting to force X11 mode, while other package formats (deb/rpm/AppImage/tar.gz) rely on the wrapper script.docs/linux-display-server.md (2)
3-42: Excellent documentation of the wrapper-based detection approach.The documentation clearly explains:
- The automatic detection and fallback mechanism
- The three-step detection logic with a helpful decision table
- The technical rationale for why a wrapper script is necessary
- Cross-reference to the detailed post-mortem analysis
This will help users understand the behavior and troubleshoot issues effectively.
68-146: Comprehensive troubleshooting guidance with practical examples.The common scenarios and troubleshooting sections provide:
- Distribution-specific guidance (Ubuntu, Fedora)
- Edge case coverage (SSH sessions, VMs)
- Practical validation commands
- Clear manual override instructions
This addresses the likely support questions that will arise from the wrapper implementation.
docs/linux-wayland-bug-postmortem.md (1)
303-304: Remove or replace placeholder dates in the timeline.The timeline still contains placeholder dates
2024-XX-XXwhich violates the coding guidelines that require only verifiable numbers in documentation. This issue was previously flagged but not yet addressed.As per coding guidelines, either replace these with verifiable dates from the GitHub issue/PR or remove the entries until accurate dates can be confirmed.
⛔ Skipped due to learnings
Learnt from: CR Repo: RocketChat/Rocket.Chat.Electron PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-09-26T19:24:21.364Z Learning: Applies to docs/**/*.md : Never invent or estimate metrics/time; only include verifiable numbers in documentation and reports
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @docs/linux-wayland-bug-postmortem.md:
- Around line 1-11: Add a new top section titled "The Solution That Actually
Worked" immediately after the existing "Issue Reference" block; in that section
include four short subsections labeled Problem, Solution, Result, and PR that
concisely state the root cause (Wayland/Chromium init ordering), the working fix
(shell wrapper that sets proper command-line flags before launching the binary),
the observed result (no more segfaults in environments without a Wayland
compositor), and link the merged PR number. Ensure the section appears before
"Executive Summary" and uses the exact heading "The Solution That Actually
Worked" so reviewers can find it easily.
In @src/app/main/app.main.spec.ts:
- Around line 54-58: There's an extra trailing/whitespace-only line after the
statement "console.log = consoleLogMock;" causing CI to fail; remove that
blank/whitespace line so the next statement "statSyncMock = fs.statSync as
jest.Mock;" immediately follows without extra spaces.
- Around line 1-6: Reorder and reformat the imports so Node built-ins come first
and import groups are separated by a blank line: import 'fs' before importing
'electron', keep the relative import of './app' after those, and ensure there's
an empty line between the grouped imports; also leave the existing
jest.mock('fs') call in place after imports. Locate the import statements
(references: fs, app from 'electron', performElectronStartup from './app') and
update their order and spacing to satisfy ESLint/Prettier.
In @src/app/main/app.ts:
- Around line 213-226: The if condition on the
normalizedSessionType/normalizedWaylandDisplay check is too long on a single
line; refactor by extracting the compound condition into a clearly named boolean
(e.g., isWaylandWithDisplay) declared above the if and then use that boolean in
the if statement so the line length is reduced; locate the block that sets
reason (references: normalizedSessionType, normalizedWaylandDisplay, reason) and
replace the long inline condition with the extracted variable to keep lines
short and readable.
- Around line 188-200: The Prettier error is caused by overly long lines in the
checkWaylandSocket function; break the long conditional and template string into
multiple lines to satisfy line-length rules. In the checkWaylandSocket arrow
function, wrap the if condition that references normalizedSessionType and
normalizedWaylandDisplay across two lines, and split the socketPath/template
literal assignment (using runtimeDir and normalizedWaylandDisplay) so each piece
fits within max line length; preserve existing logic and variable names
(checkWaylandSocket, normalizedSessionType, normalizedWaylandDisplay,
runtimeDir, socketPath) and ensure no other semantics are changed.
🧹 Nitpick comments (2)
docs/linux-wayland-bug-postmortem.md (2)
24-35: Add language specifiers to fenced code blocks.The linter flags code blocks without language specifiers (MD040). For ASCII diagrams and terminal output, use
textorplaintext.♻️ Suggested fixes
### Error Output -``` +```text ERROR:ui/ozone/platform/wayland/host/wayland_connection.cc:202### Why `app.commandLine.appendSwitch()` Doesn't Work -``` +```text Process Startup Timeline:### Why Wrapper Script Works -``` +```text Process Startup Timeline with Wrapper:Also applies to: 147-169, 178-203
323-329: Convert bare URLs to proper markdown links.Linter flags bare URLs (MD034). Using proper markdown link syntax improves consistency.
♻️ Suggested fix
## References -- Chromium Ozone Platform: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md -- Electron Command Line Switches: https://www.electronjs.org/docs/latest/api/command-line-switches -- Wayland Protocol: https://wayland.freedesktop.org/ -- XDG Base Directory Specification: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html +- [Chromium Ozone Platform](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md) +- [Electron Command Line Switches](https://www.electronjs.org/docs/latest/api/command-line-switches) +- [Wayland Protocol](https://wayland.freedesktop.org/) +- [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/linux-wayland-bug-postmortem.mdsrc/app/main/app.main.spec.tssrc/app/main/app.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow existing TypeScript patterns with strict mode enabled
All new code must pass ESLint and TypeScript checks
Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Files:
src/app/main/app.tssrc/app/main/app.main.spec.ts
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
docs/**/*.md: Never invent or estimate metrics/time; only include verifiable numbers in documentation and reports
Structure technical post-mortems with a top section “The Solution That Actually Worked” including Problem, Solution, Result, and PR
Place all documentation in thedocs/directory using Markdown with clear, descriptive filenames and simple language
Before adding features, check and update existing docs; create/extend docs for new or changed features
Files:
docs/linux-wayland-bug-postmortem.md
src/**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Main-process tests must be named with the suffix
.main.spec.tsand run under the Electron runner
Files:
src/app/main/app.main.spec.ts
🧠 Learnings (7)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/main.ts : Keep `src/main.ts` as the Electron main process entry point compiled by Rollup
Applied to files:
src/app/main/app.tssrc/app/main/app.main.spec.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/rootWindow.ts : Keep `src/rootWindow.ts` as the renderer entry for the main window UI
Applied to files:
src/app/main/app.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/**/*.main.spec.ts : Main-process tests must be named with the suffix `.main.spec.ts` and run under the Electron runner
Applied to files:
src/app/main/app.tssrc/app/main/app.main.spec.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/preload.ts : Keep `src/preload.ts` as the preload script bridging main and renderer
Applied to files:
src/app/main/app.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to **/*.{ts,tsx} : Avoid unnecessary comments; only add comments for complex logic or non-obvious decisions
Applied to files:
src/app/main/app.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to docs/**/*.md : Structure technical post-mortems with a top section “The Solution That Actually Worked” including Problem, Solution, Result, and PR
Applied to files:
docs/linux-wayland-bug-postmortem.md
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/**/!(*.main).spec.ts : Renderer-process tests should be named `*.spec.ts` (without the `.main` qualifier)
Applied to files:
src/app/main/app.main.spec.ts
🧬 Code graph analysis (2)
src/app/main/app.ts (1)
workspaces/desktop-release-action/dist/index.js (2)
fs(475-475)fs(2861-2861)
src/app/main/app.main.spec.ts (1)
src/app/main/app.ts (1)
performElectronStartup(85-239)
🪛 GitHub Check: check (macos-latest)
src/app/main/app.ts
[failure] 215-215:
Replace normalizedSessionType·===·'wayland'·&&·normalizedWaylandDisplay·!==·'' with ⏎··········normalizedSessionType·===·'wayland'·&&⏎··········normalizedWaylandDisplay·!==·''⏎········
[failure] 193-193:
Insert ⏎···········
[failure] 189-189:
Replace normalizedSessionType·!==·'wayland'·||·normalizedWaylandDisplay·===·'' with ⏎··········normalizedSessionType·!==·'wayland'·||⏎··········normalizedWaylandDisplay·===·''⏎········
src/app/main/app.main.spec.ts
[failure] 55-55:
Delete ····
[failure] 2-2:
fs import should occur before import of electron
[failure] 1-1:
There should be at least one empty line between import groups
🪛 GitHub Check: check (ubuntu-latest)
src/app/main/app.ts
[failure] 215-215:
Replace normalizedSessionType·===·'wayland'·&&·normalizedWaylandDisplay·!==·'' with ⏎··········normalizedSessionType·===·'wayland'·&&⏎··········normalizedWaylandDisplay·!==·''⏎········
[failure] 193-193:
Insert ⏎···········
[failure] 189-189:
Replace normalizedSessionType·!==·'wayland'·||·normalizedWaylandDisplay·===·'' with ⏎··········normalizedSessionType·!==·'wayland'·||⏎··········normalizedWaylandDisplay·===·''⏎········
src/app/main/app.main.spec.ts
[failure] 55-55:
Delete ····
[failure] 2-2:
fs import should occur before import of electron
[failure] 1-1:
There should be at least one empty line between import groups
🪛 markdownlint-cli2 (0.18.1)
docs/linux-wayland-bug-postmortem.md
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
325-325: Bare URL used
(MD034, no-bare-urls)
326-326: Bare URL used
(MD034, no-bare-urls)
327-327: Bare URL used
(MD034, no-bare-urls)
328-328: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (windows-latest)
🔇 Additional comments (6)
docs/linux-wayland-bug-postmortem.md (1)
299-306: Timeline now contains only verifiable dates.The placeholder dates have been removed, addressing the previous review feedback. The remaining dates are specific and verifiable.
src/app/main/app.main.spec.ts (3)
45-84: Well-structured test setup with proper environment isolation.The
beforeEach/afterEachhooks correctly save and restoreprocess.platform,process.env,process.argv, andconsole.log. The mock setup forfs.statSyncenables testing socket existence scenarios.
213-247: Socket existence tests cover critical failure scenarios.The tests properly verify both cases where the Wayland socket check should fail:
- Socket file doesn't exist (throws ENOENT)
- Path exists but is not a socket (
isSocket()returns false)Both correctly expect the
socket-not-foundreason and X11 forcing.
348-406: Good coverage for environment variable normalization.The tests verify that whitespace is properly trimmed from
XDG_SESSION_TYPEandWAYLAND_DISPLAY, ensuring malformed environment variables don't cause unexpected behavior.src/app/main/app.ts (2)
182-201: Socket existence check properly addresses the crash scenario.The
checkWaylandSocket()function correctly:
- Validates both
XDG_SESSION_TYPEandWAYLAND_DISPLAYare set- Constructs the socket path using
XDG_RUNTIME_DIRwith fallback- Uses
fs.statSync()withisSocket()to verify it's an actual socket- Catches errors (ENOENT, permission issues) and returns
falseThis prevents the segfault scenario from issue #3154 where
WAYLAND_DISPLAYpoints to a non-existent socket.
227-236: Structured logging with reason codes aids debugging.The JSON-formatted log output with
reason,sessionType, andwaylandDisplayfields provides useful diagnostic information when troubleshooting display server issues.
| # Linux Wayland/X11 Display Server Bug - Post-Mortem Analysis | ||
|
|
||
| ## Issue Reference | ||
| - **GitHub Issue**: [#3154](https://github.com/RocketChat/Rocket.Chat.Electron/issues/3154) | ||
| - **Related PR**: [#3171](https://github.com/RocketChat/Rocket.Chat.Electron/pull/3171) | ||
| - **Date**: January 2025 | ||
| - **Severity**: Critical (application crash/segfault) | ||
|
|
||
| ## Executive Summary | ||
|
|
||
| Rocket.Chat Desktop crashes with SEGFAULT when environment variables suggest a Wayland session but no valid Wayland compositor is available. The initial fix attempt using `app.commandLine.appendSwitch()` was **ineffective** because Chromium's display platform initialization occurs before any Electron JavaScript code executes. The solution requires a **shell wrapper script** that detects the display server situation and passes appropriate command-line flags before the binary starts. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add required "The Solution That Actually Worked" section at the top.
Per coding guidelines, structure technical post-mortems with a top section "The Solution That Actually Worked" including Problem, Solution, Result, and PR. This section should be placed immediately after the Issue Reference block.
📝 Suggested structure to add after line 8
- **Severity**: Critical (application crash/segfault)
+## The Solution That Actually Worked
+
+**Problem**: Rocket.Chat Desktop crashes with SEGFAULT when WAYLAND_DISPLAY points to a non-existent Wayland socket. Chromium's display platform initialization occurs before Electron JavaScript code executes.
+
+**Solution**: Shell wrapper script (`build/linux/wrapper.sh`) that detects display server availability before binary execution and forces X11 via `--ozone-platform=x11` when Wayland is unavailable.
+
+**Result**: All tested scenarios (fake Wayland sockets, SSH sessions, misconfigured environments) now gracefully fall back to X11 without crashing. Validated across Ubuntu 22.04, Fedora 42, and all package formats (DEB, RPM, AppImage, tar.gz, Snap).
+
+**PR**: [#3172](https://github.com/RocketChat/Rocket.Chat.Electron/pull/3172)
+
## Executive Summary📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Linux Wayland/X11 Display Server Bug - Post-Mortem Analysis | |
| ## Issue Reference | |
| - **GitHub Issue**: [#3154](https://github.com/RocketChat/Rocket.Chat.Electron/issues/3154) | |
| - **Related PR**: [#3171](https://github.com/RocketChat/Rocket.Chat.Electron/pull/3171) | |
| - **Date**: January 2025 | |
| - **Severity**: Critical (application crash/segfault) | |
| ## Executive Summary | |
| Rocket.Chat Desktop crashes with SEGFAULT when environment variables suggest a Wayland session but no valid Wayland compositor is available. The initial fix attempt using `app.commandLine.appendSwitch()` was **ineffective** because Chromium's display platform initialization occurs before any Electron JavaScript code executes. The solution requires a **shell wrapper script** that detects the display server situation and passes appropriate command-line flags before the binary starts. | |
| # Linux Wayland/X11 Display Server Bug - Post-Mortem Analysis | |
| ## Issue Reference | |
| - **GitHub Issue**: [#3154](https://github.com/RocketChat/Rocket.Chat.Electron/issues/3154) | |
| - **Related PR**: [#3171](https://github.com/RocketChat/Rocket.Chat.Electron/pull/3171) | |
| - **Date**: January 2025 | |
| - **Severity**: Critical (application crash/segfault) | |
| ## The Solution That Actually Worked | |
| **Problem**: Rocket.Chat Desktop crashes with SEGFAULT when WAYLAND_DISPLAY points to a non-existent Wayland socket. Chromium's display platform initialization occurs before Electron JavaScript code executes. | |
| **Solution**: Shell wrapper script (`build/linux/wrapper.sh`) that detects display server availability before binary execution and forces X11 via `--ozone-platform=x11` when Wayland is unavailable. | |
| **Result**: All tested scenarios (fake Wayland sockets, SSH sessions, misconfigured environments) now gracefully fall back to X11 without crashing. Validated across Ubuntu 22.04, Fedora 42, and all package formats (DEB, RPM, AppImage, tar.gz, Snap). | |
| **PR**: [#3172](https://github.com/RocketChat/Rocket.Chat.Electron/pull/3172) | |
| ## Executive Summary | |
| Rocket.Chat Desktop crashes with SEGFAULT when environment variables suggest a Wayland session but no valid Wayland compositor is available. The initial fix attempt using `app.commandLine.appendSwitch()` was **ineffective** because Chromium's display platform initialization occurs before any Electron JavaScript code executes. The solution requires a **shell wrapper script** that detects the display server situation and passes appropriate command-line flags before the binary starts. |
🤖 Prompt for AI Agents
In @docs/linux-wayland-bug-postmortem.md around lines 1 - 11, Add a new top
section titled "The Solution That Actually Worked" immediately after the
existing "Issue Reference" block; in that section include four short subsections
labeled Problem, Solution, Result, and PR that concisely state the root cause
(Wayland/Chromium init ordering), the working fix (shell wrapper that sets
proper command-line flags before launching the binary), the observed result (no
more segfaults in environments without a Wayland compositor), and link the
merged PR number. Ensure the section appears before "Executive Summary" and uses
the exact heading "The Solution That Actually Worked" so reviewers can find it
easily.
| console.log = consoleLogMock; | ||
|
|
||
| statSyncMock = fs.statSync as jest.Mock; | ||
| statSyncMock.mockReturnValue({ isSocket: () => true }); | ||
|
|
There was a problem hiding this comment.
Remove extra whitespace on line 55.
CI is failing due to extra whitespace. Remove the trailing/extra spaces.
🔧 Suggested fix
console.log = consoleLogMock;
-
+
statSyncMock = fs.statSync as jest.Mock;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log = consoleLogMock; | |
| statSyncMock = fs.statSync as jest.Mock; | |
| statSyncMock.mockReturnValue({ isSocket: () => true }); | |
| console.log = consoleLogMock; | |
| statSyncMock = fs.statSync as jest.Mock; | |
| statSyncMock.mockReturnValue({ isSocket: () => true }); |
🧰 Tools
🪛 GitHub Check: check (macos-latest)
[failure] 55-55:
Delete ····
🪛 GitHub Check: check (ubuntu-latest)
[failure] 55-55:
Delete ····
🤖 Prompt for AI Agents
In @src/app/main/app.main.spec.ts around lines 54 - 58, There's an extra
trailing/whitespace-only line after the statement "console.log =
consoleLogMock;" causing CI to fail; remove that blank/whitespace line so the
next statement "statSyncMock = fs.statSync as jest.Mock;" immediately follows
without extra spaces.
- Add fs.statSync check to verify Wayland socket actually exists - Add 'socket-not-found' reason when socket is missing - Remove placeholder dates from timeline in postmortem - Add tests for socket existence checking
jeanfbrito
force-pushed
the
fix-x11-ubuntu2204
branch
from
0c846aa to
01c3111
Compare
|
Closing in favor of PR #3171 which uses the same branch |
2 participants
Summary
Fixes #3154 - Rocket.Chat Desktop crashes (SEGFAULT) on Linux when
WAYLAND_DISPLAYenvironment variable points to a non-existent Wayland socket.Root Cause
The initial PR #3171 used
app.commandLine.appendSwitch('ozone-platform', 'x11')but this doesn't work because Chromium's display platform initialization happens BEFORE any Electron JavaScript executes.Solution
A wrapper script that runs before the binary and detects display server availability:
XDG_SESSION_TYPE != wayland→ force X11WAYLAND_DISPLAYis empty → force X11Files Changed
build/linux/wrapper.sh- Shell wrapper with detection logicbuild/afterPack.js- Installs wrapper during build (renames binary to.bin, installs wrapper)electron-builder.json- AddedallowNativeWayland: falsefor Snapdocs/linux-wayland-bug-postmortem.md- Technical analysisTest Results
Validated on real hardware:
Packages tested: DEB, RPM, AppImage, tar.gz, Snap
Key Validation
Real Wayland session shows
"Using Wayland platform"in logs, confirming wrapper correctly allows native Wayland when socket exists. Fake Wayland forces X11 fallback without crash.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.