Fix/attachment width word wrapping#3256
Fix/attachment width word wrapping#3256Ram-sah19 wants to merge 175 commits intoRocketChat:masterfrom
Conversation
* video call window opens and load the video call in a webview
WalkthroughThis PR downgrades Electron from 40.0.0 to ~34.0.2 and electron-builder from 26.0.3 to 25.1.8, removes build configuration hooks and snapcraft parameters, cleans up internationalization files, injects custom CSS to override attachment width constraints, adds external URL handling via IPC, enhances notification attention flashing, and implements renderer process crash recovery with cache clearing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/main/rootWindow.ts (1)
521-561: Good crash recovery implementation with escalating strategy.The crash handler appropriately:
- Prevents duplicate listener registration
- Uses escalating recovery (clear cache after 3+ failures)
- Resets counter on successful load
One consideration: there's no upper bound on crash attempts. If a persistent issue causes repeated crashes, this could loop indefinitely. Consider adding a maximum attempt threshold (e.g., after 10 crashes, show an error dialog and stop retrying).
💡 Optional: Add maximum crash recovery attempts
browserWindow.webContents.on( 'render-process-gone', async (_event, details) => { console.error('Renderer process crashed:', details.reason); consecutiveCrashCount++; + + const MAX_RECOVERY_ATTEMPTS = 10; + if (consecutiveCrashCount > MAX_RECOVERY_ATTEMPTS) { + console.error('Maximum recovery attempts reached. Manual intervention required.'); + return; + } + console.log( `Crash count: ${consecutiveCrashCount}. Attempting recovery...` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/main/rootWindow.ts` around lines 521 - 561, Add a maximum crash-recovery threshold to prevent infinite retry loops: introduce a constant like MAX_CRASH_ATTEMPTS (e.g., 10) and, inside the render-process-gone handler where consecutiveCrashCount is incremented, check if consecutiveCrashCount >= MAX_CRASH_ATTEMPTS and if so stop further recovery attempts (e.g., do not clear cache/reload), remove or ignore the crash handler (using crashHandlerRegistered or removeListener on browserWindow.webContents), and surface a user-visible error (e.g., show an error dialog) explaining that automatic recovery has been disabled; keep the existing reset logic in did-finish-load to resume normal behavior if the page loads successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/main/rootWindow.ts`:
- Around line 521-561: Add a maximum crash-recovery threshold to prevent
infinite retry loops: introduce a constant like MAX_CRASH_ATTEMPTS (e.g., 10)
and, inside the render-process-gone handler where consecutiveCrashCount is
incremented, check if consecutiveCrashCount >= MAX_CRASH_ATTEMPTS and if so stop
further recovery attempts (e.g., do not clear cache/reload), remove or ignore
the crash handler (using crashHandlerRegistered or removeListener on
browserWindow.webContents), and surface a user-visible error (e.g., show an
error dialog) explaining that automatic recovery has been disabled; keep the
existing reset logic in did-finish-load to resume normal behavior if the page
loads successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 284503ed-41a5-4f3b-9b7a-d427c0abe6a4
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonworkspaces/desktop-release-action/dist/index.jsis excluded by!**/dist/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.github/workflows/build-release.ymlelectron-builder.jsonpackage.jsonsrc/i18n/de-DE.i18n.jsonsrc/i18n/fr.i18n.jsonsrc/i18n/hu.i18n.jsonsrc/i18n/ru.i18n.jsonsrc/injected.tssrc/main.tssrc/notifications/main.tssrc/ui/main/rootWindow.ts
💤 Files with no reviewable changes (2)
- .github/workflows/build-release.yml
- electron-builder.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/main.tssrc/injected.tssrc/ui/main/rootWindow.tssrc/notifications/main.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:34.676Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Use `*.spec.ts` file naming for renderer process tests and `*.main.spec.ts` for main process tests
Applied to files:
src/main.ts
📚 Learning: 2026-03-11T06:38:34.676Z
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:34.676Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
Applied to files:
src/injected.tspackage.json
🪛 ESLint
src/injected.ts
[error] 206-206: Replace '[Rocket.Chat·Desktop]·Custom·CSS·injected·for·attachment·width·fix' with ⏎····'[Rocket.Chat·Desktop]·Custom·CSS·injected·for·attachment·width·fix'⏎··
(prettier/prettier)
[error] 604-606: Replace ⏎······Boolean⏎···· with Boolean
(prettier/prettier)
🔇 Additional comments (11)
src/i18n/ru.i18n.json (1)
437-437: LGTM!End-of-file newline addition is a standard formatting improvement with no semantic changes.
src/i18n/de-DE.i18n.json (1)
435-435: LGTM!End-of-file newline addition is a standard formatting improvement with no semantic changes.
src/i18n/fr.i18n.json (1)
434-434: LGTM!End-of-file newline addition is a standard formatting improvement with no semantic changes.
src/main.ts (1)
148-171: Well-structured external URL handler with proper validation.The handler correctly validates URLs, checks protocol allowlists, and routes to appropriate openers. One minor optimization opportunity:
The dynamic import of
isProtocolAllowed(line 158) runs on every IPC call. Consider hoisting it to module scope or caching the import result if this handler is called frequently.src/notifications/main.ts (2)
89-106: LGTM!Good UX enhancement expanding attention drawing to both 'text' and 'voice' notification types. The idempotent
stopAttentioncalls on close/dismiss are safe and ensure consistent cleanup regardless of notification type.
250-252: LGTM!Consistent with the close handler - idempotent stopAttention ensures proper cleanup.
src/ui/main/rootWindow.ts (1)
47-48: LGTM!Module-scoped state for crash handling is appropriately isolated.
src/injected.ts (2)
195-206: This CSS injection directly addresses the PR objective.The approach of injecting CSS to override the hardcoded 360px width constraint on
.rcx-attachment__contentis appropriate for the Electron desktop client. Using!importantensures the override takes precedence over Fuselage's inline or compiled styles.Minor: ESLint/Prettier flagged a formatting issue on line 206 - the log string could be wrapped.
592-618: Improved polling mechanism that self-terminates.The change from
setIntervalto self-reschedulingsetTimeoutis an improvement - it stops polling once all features are initialized rather than running indefinitely. The version-based handling ofbackgroundSettingsis a good approach for server version compatibility.Minor: ESLint/Prettier flagged formatting on lines 604-606 where
Booleancould be on the same line.src/i18n/hu.i18n.json (2)
294-296: LGTM.This is a safe copy-only change; the key path and translation contract stay unchanged.
395-402: LGTM.No issue here; the
sidebar.item.reloadClearingCacheentry remains structurally consistent with the rest of the locale file.
5 participants
Description
Fixes #3113
Text format attachments were hardcoded to a fixed
360pxwidth, causing poor word wrapping and formatting issues for webhook messages (e.g. Grafana alerts). This prevented long URLs and formatted text from using the full available horizontal space.Changes Made
360pxwidth constraint on text format attachments100%of available horizontal spaceType of Change
How to Test
Screenshots
Related Issue
Closes #3113
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Chores