feat(lock): implement application lock functionality with IPC integration #3115
Conversation
|
|
WalkthroughAdds an application-level screen-lock feature: build entries and HTML shell, renderer and preload scripts, main-process lock orchestration and IPC handlers, store slices and migrations for password/timeout, settings UI and menu triggers, type augmentations, and translations across many locales. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Menu as Menu/Context
participant Timer as Inactivity Timer
participant Main as Main Process
participant Root as Root Window
participant LockView as Lock BrowserView (Renderer)
Note over Main: registerLockIpcHandlers() on app ready
User->>Menu: select "Lock Screen"
Menu->>Main: dispatch MENU_BAR_LOCK_SCREEN_CLICKED
Timer-->>Main: auto-lock trigger (elapsed)
Main->>Root: attach BrowserView overlay (load lockScreen.html)
Root->>LockView: renderer boot (+ i18n)
User->>LockView: submit password
LockView->>Main: ipc invoke 'lock:verify'
Main->>Main: verify (hash/migrate, throttle/backoff)
alt verified
Main-->>LockView: true
LockView->>Main: ipc 'lock:unlock'
Main->>Root: remove overlay, restore state
else failed
Main-->>LockView: false
LockView->>User: show incorrect/unlockFailed
end
sequenceDiagram
autonumber
participant Settings as Settings store
participant Orch as setupScreenLock
participant Timer as Inactivity Timer
participant Window as Root Window events
Settings->>Orch: screenLockTimeoutSeconds updated
alt timeout > 0
Orch->>Timer: start/reset timer
else timeout == 0
Orch->>Timer: clear timer (disabled)
end
Window->>Orch: focus/blur/activity events
Orch->>Timer: clear/reset with blur grace handling
Timer-->>Orch: elapsed -> dispatch MENU_BAR_LOCK_SCREEN_CLICKED
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential review focal points:
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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.
Actionable comments posted: 5
🧹 Nitpick comments (9)
rollup.config.mjs (1)
101-103: Centralize copying UI HTML in the main build configTo avoid duplication and ordering surprises, move copying of src/ui/*.html into the existing copy() plugin in the main (src/main.ts) build config, and remove it here. If lock UI references nested assets (css/js/images), prefer a recursive glob.
Apply within this segment:
- copy({ - targets: [{ src: 'src/ui/*.html', dest: 'app' }], - }),Then update the copy() in the main config (around lines 251-256) to include UI HTML and, if needed, nested assets, e.g.:
copy({ targets: [ { src: 'src/public/*', dest: 'app' }, { src: 'src/ui/*.html', dest: 'app' }, // If UI uses additional assets: // { src: 'src/ui/assets/**/*', dest: 'app/ui/assets' }, { src: 'node_modules/@rocket.chat/icons/dist/*', dest: 'app/icons' }, ], // hook: 'writeBundle', // optional: run after bundles are written })Based on learnings
src/main.ts (1)
85-87: Guard against getRootWindow() rejection to avoid startup abortgetRootWindow() can reject on timing; handle errors to keep startup resilient.
Apply:
- const mainWindow = await getRootWindow(); - lockManager.initialize(mainWindow); + await getRootWindow() + .then((win) => { + lockManager.initialize(win); + }) + .catch((err) => { + // eslint-disable-next-line no-console + console.error('Failed to initialize lock manager (no root window):', err); + });src/lockSettings/main.ts (1)
45-63: Lock Settings window creation — LGTM; minor UX hardening optionalConfig looks good for security. Optionally:
- autoHideMenuBar: true to reduce chrome
- parent + modal to keep it on top of main window
Example:
settingsWindow = new BrowserWindow({ width: 700, height: 700, resizable: false, minimizable: false, maximizable: false, title: 'Lock Settings', + autoHideMenuBar: true, + // parent: someMainWindowRef, + // modal: true, webPreferences: { preload: path.join(__dirname, 'preload.js'), contextIsolation: true, nodeIntegration: false, sandbox: false, }, });src/lockPreload.ts (1)
65-68: Freeze exposed API objectPrevent runtime mutation from renderer.
- if (process.env.NODE_ENV !== 'production') { - (lockAPI as ElectronAPI).forceLock = () => ipcRenderer.invoke('lock:force'); - } + if (process.env.NODE_ENV !== 'production') { + (lockAPI as ElectronAPI).forceLock = () => ipcRenderer.invoke('lock:force'); + } + Object.freeze(lockAPI);src/ui/main/menuBar.ts (2)
517-538: Localize new menu labels and review shortcutsUse i18n for 'Lock Settings' and 'Lock' like other items, and confirm accelerators don’t conflict.
- label: 'Lock Settings', + label: t('menus.lockSettings'), ... - label: 'Lock', + label: t('menus.lockNow'),
772-793: Remove dead/commented lock menu block or add TODOAvoid large commented blocks in production code. Either remove or gate behind a feature flag.
src/ui/lockSettings.html (1)
210-218: Harden password inputs (autocomplete) and UXDisable password managers and autocorrect; consider allowing timeout-only updates without forcing password change.
-<input - type="password" +<input + type="password" id="password" name="password" - placeholder="Enter password (min 4 characters)" + placeholder="Enter password (min 4 characters)" + autocomplete="new-password" + autocapitalize="none" + spellcheck="false" required minlength="4" > ... -<input - type="password" +<input + type="password" id="confirmPassword" name="confirmPassword" placeholder="Re-enter password" + autocomplete="new-password" + autocapitalize="none" + spellcheck="false" required >Optional: accept empty password to keep current (requires IPC/API change).
Also applies to: 221-230
src/types/global.d.ts (1)
5-18: Use proper global augmentationWrap with declare global to avoid interface shadowing issues.
-interface Window { - electronAPI: { - hasLockConfig: () => Promise<boolean>; - getLockConfig: () => Promise<{ timeoutSeconds: number } | null>; - setLockConfig: ( - password: string, - timeoutSeconds: number - ) => Promise<{ success: boolean; error?: string }>; - verifyPassword: (password: string) => Promise<boolean>; - unlockApp: () => Promise<void>; - onLock: (callback: () => void) => () => void; - onUnlock: (callback: () => void) => () => void; - }; -} +declare global { + interface Window { + electronAPI: { + hasLockConfig: () => Promise<boolean>; + getLockConfig: () => Promise<{ timeoutSeconds: number } | null>; + setLockConfig: ( + password: string, + timeoutSeconds: number + ) => Promise<{ success: boolean; error?: string }>; + verifyPassword: (password: string) => Promise<boolean>; + unlockApp: () => Promise<void>; + onLock: (callback: () => void) => () => void; + onUnlock: (callback: () => void) => () => void; + }; + } +}src/lockIPC.ts (1)
17-19: Single source of truth for MIN_TIMEOUT_SECONDSAvoid drift; export from lockManager or a constants module and import here.
📜 Review details
Configuration used: CodeRabbit 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 (14)
rollup.config.mjs(1 hunks)src/lockConfig.ts(1 hunks)src/lockIPC.ts(1 hunks)src/lockManager.ts(1 hunks)src/lockPreload.ts(1 hunks)src/lockRenderer.ts(1 hunks)src/lockSettings/main.ts(1 hunks)src/main.ts(4 hunks)src/preload.ts(2 hunks)src/rootWindow.ts(2 hunks)src/types/global.d.ts(1 hunks)src/ui/lockScreen.html(1 hunks)src/ui/lockSettings.html(1 hunks)src/ui/main/menuBar.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/types/global.d.tssrc/lockSettings/main.tssrc/main.tssrc/ui/main/menuBar.tssrc/lockPreload.tssrc/rootWindow.tssrc/preload.tssrc/lockIPC.tssrc/lockConfig.tssrc/lockManager.tssrc/lockRenderer.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:
rollup.config.mjs
src/main.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
src/main.tsas the Electron main process entry point compiled by Rollup
Files:
src/main.ts
src/rootWindow.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
src/rootWindow.tsas the renderer entry for the main window UI
Files:
src/rootWindow.ts
src/preload.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
src/preload.tsas the preload script bridging main and renderer
Files:
src/preload.ts
🧠 Learnings (3)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
PR: RocketChat/Rocket.Chat.Electron#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/main.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
PR: RocketChat/Rocket.Chat.Electron#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/lockPreload.tssrc/preload.ts
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
PR: RocketChat/Rocket.Chat.Electron#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T19:24:21.364Z
Learning: Applies to src/ipc/**/*.ts : Define and maintain IPC channels in `src/ipc/` for main/renderer state synchronization
Applied to files:
src/lockIPC.ts
🧬 Code graph analysis (7)
src/lockSettings/main.ts (1)
src/lockManager.ts (1)
lockManager(232-232)
src/main.ts (3)
src/lockIPC.ts (1)
setupLockIPC(23-34)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)src/lockManager.ts (1)
lockManager(232-232)
src/ui/main/menuBar.ts (2)
src/lockManager.ts (1)
lockManager(232-232)src/lockSettings/main.ts (1)
openLockSettings(17-69)
src/rootWindow.ts (1)
src/lockRenderer.ts (1)
initializeLockScreen(9-12)
src/preload.ts (1)
src/lockPreload.ts (1)
exposeLockAPI(28-71)
src/lockIPC.ts (2)
src/lockConfig.ts (4)
hasLockConfig(186-194)loadLockConfig(63-104)saveLockConfig(112-128)verifyPassword(135-181)src/lockManager.ts (1)
lockManager(232-232)
src/lockManager.ts (1)
src/lockConfig.ts (1)
loadLockConfig(63-104)
🔇 Additional comments (5)
src/preload.ts (1)
6-6: Expose lock API early in preload — LGTMCalling exposeLockAPI() at preload top-level ensures window.electronAPI is ready before renderer runs.
Confirm that a global type for window.electronAPI (ElectronAPI) is declared (e.g., in src/types/global.d.ts) to satisfy TS strict mode in consumers.
Also applies to: 81-81
src/main.ts (1)
20-22: IPC initialization placement — LGTMInitializing lock IPC early in startup is correct and low risk.
Also applies to: 61-61
src/lockConfig.ts (2)
112-128: Good: scrypt with per-record salt and timingSafeEqualSolid choices; legacy path migrates on success.
Ensure electron-store file permissions are default-safe in your packaging and that no logs ever include plaintext passwords. Based on learnings
63-104: Graceful legacy handling looks correctNull on parse errors, no throws; migration deferred until successful verify.
src/lockPreload.ts (1)
65-71: No duplicate electronAPI exposure detected Only onecontextBridge.exposeInMainWorld('electronAPI', …)call exists (insrc/lockPreload.ts, invoked viaexposeLockAPI()insrc/preload.ts); no merge necessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (12)
src/i18n/tr-TR.i18n.json (1)
111-113: Polish the Turkish phrasing for clarity“Devre dışı bırakmak için 0 ayarlayın.” is understandable, but the more natural Turkish phrasing is “0 olarak ayarlayın.” Consider tweaking the sentence to read more fluidly.
src/i18n/es.i18n.json (1)
427-434: Add the menu label translation too.We render a “Lock Screen” menu item via
menus.lockScreen. English already defines it, but this locale still falls back to English. Please add the Spanish string alongside the other menu entries (e.g.,"lockScreen": "Bloquear pantalla").src/i18n/ru.i18n.json (1)
399-406: Provide the menu translation for completeness.The main menu reads
menus.lockScreen; without a Russian string it stays in English. Please add an entry such as"lockScreen": "Заблокировать экран"next to the othermenuskeys.src/ui/components/SettingsView/features/ScreenLock.tsx (1)
91-105: Prefer Fuselage spacing/tokens over inline styles.Replace raw
style(gap/padding/width) with Fuselage props/tokens (mi,p,w, etc.) for consistency and theme compliance.Also applies to: 116-123
src/lockScreen/LockScreen.tsx (1)
44-74: Optional: use Fuselage components for consistency/theming.Consider replacing raw HTML with Fuselage (
Box,TextInput,Button,Field) for consistent styling and a11y.src/app/main/screenLock.ts (1)
70-81: Confirm behavior: timer only runs while app is unfocused.Current logic starts the inactivity timer only when the root window is not focused. If true inactivity (no input while focused) is desired, you’ll need to start/reset the timer on
before-input-eventwhile focused too (or usepowerMonitoridle signals).src/app/main/app.ts (3)
65-76: Support throttling state; and remove unused listener fieldsAdd counters used by the throttling above, and drop unused moveListener/resizeListener from lockState.
Apply this diff:
let lockWindow: BrowserView | null = null; -// Track original rootWindow state while locked so we can restore it on unlock -const lockState: { - originalBounds?: Electron.Rectangle; - prevResizable?: boolean; - prevMinimizable?: boolean; - prevMaximizable?: boolean; - moveListener?: () => void; - resizeListener?: () => void; -} = {}; +// Throttling state for verify attempts +let lockVerifyAttemptCount = 0; +let lockVerifyLastAttemptTs = 0; + +// Track original rootWindow state while locked so we can restore it on unlock +const lockState: { + originalBounds?: Electron.Rectangle; + prevResizable?: boolean; + prevMinimizable?: boolean; + prevMaximizable?: boolean; + // prevMovable tracked separately via (lockState as any).prevMovable +} = {};
169-176: Remove cleanup for listeners that are never registeredmoveListener/resizeListener are never assigned. Keep only the explicit removals of updateBounds below.
Apply this diff:
- if (lockState.moveListener) { - rootWindow.removeListener('move', lockState.moveListener); - } - if (lockState.resizeListener) { - rootWindow.removeListener('resize', lockState.resizeListener); - }
134-137: 'move' listener is unnecessary; 'setAutoResize' covers size changesMoving the window doesn’t change content bounds; the BrowserView remains aligned at {0,0}. Keeping only resize keeps things simpler.
Apply this diff:
- // keep bounds updated on move/resize - rootWindow.addListener('resize', updateBounds); - rootWindow.addListener('move', updateBounds); + // keep bounds updated on resize + rootWindow.addListener('resize', updateBounds);- // remove the updateBounds listeners we added earlier + // remove the updateBounds listener we added earlier rootWindow.removeListener('resize', updateBounds); - rootWindow.removeListener('move', updateBounds);Also applies to: 176-179
src/lockScreen/lock-screen.tsx (3)
2-2: Type the React rootUse the Root type instead of any.
Apply this diff:
-import { createRoot } from 'react-dom/client'; +import { createRoot } from 'react-dom/client'; +import type { Root } from 'react-dom/client'; @@ -let reactRoot: any = null; +let reactRoot: Root | null = null;As per coding guidelines.
Also applies to: 9-9
13-16: Normalize browser language to base locale before loading resourcesnavigator.language may be 'en-US'. Map to base ('en') and fall back if missing.
Apply this diff:
- const browserLang = (navigator.language || - fallbackLng) as keyof typeof resources; - const lng = browserLang || fallbackLng; + const requested = navigator.language || ''; + const base = requested.split('-')[0] as keyof typeof resources; + const lng = (resources[base] ? base : fallbackLng) as keyof typeof resources; @@ - if (resources[lng]) { + if (resources[lng]) { try { res[lng] = { translation: await resources[lng]() }; } catch (e) { // ignore, will fallback } } @@ - if (!res[fallbackLng]) { + if (!res[fallbackLng]) { res[fallbackLng] = { translation: await resources[fallbackLng]() }; }Also applies to: 21-27, 29-33
49-53: Use a one‑time DOMContentLoaded listener to avoid duplicate wiringSlight cleanup; behavior unchanged.
Apply this diff:
- return new Promise<void>((resolve) => { - document.addEventListener('DOMContentLoaded', () => - start().then(resolve).catch(resolve) - ); - }); + return new Promise<void>((resolve) => { + document.addEventListener( + 'DOMContentLoaded', + () => { start().then(resolve).catch(resolve); }, + { once: true } + ); + });
📜 Review details
Configuration used: CodeRabbit 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 (40)
.editorconfig(1 hunks)rollup.config.mjs(1 hunks)src/app/PersistableValues.ts(2 hunks)src/app/main/app.ts(5 hunks)src/app/main/screenLock.ts(1 hunks)src/app/selectors.ts(1 hunks)src/i18n/ar.i18n.json(1 hunks)src/i18n/de-DE.i18n.json(2 hunks)src/i18n/en.i18n.json(3 hunks)src/i18n/es.i18n.json(2 hunks)src/i18n/fi.i18n.json(2 hunks)src/i18n/fr.i18n.json(2 hunks)src/i18n/hu.i18n.json(2 hunks)src/i18n/it-IT.i18n.json(1 hunks)src/i18n/ja.i18n.json(1 hunks)src/i18n/nb-NO.i18n.json(1 hunks)src/i18n/nn.i18n.json(1 hunks)src/i18n/no.i18n.json(2 hunks)src/i18n/pl.i18n.json(2 hunks)src/i18n/pt-BR.i18n.json(2 hunks)src/i18n/ru.i18n.json(2 hunks)src/i18n/se.i18n.json(1 hunks)src/i18n/sv.i18n.json(2 hunks)src/i18n/tr-TR.i18n.json(2 hunks)src/i18n/uk-UA.i18n.json(2 hunks)src/i18n/zh-CN.i18n.json(2 hunks)src/i18n/zh-TW.i18n.json(2 hunks)src/i18n/zh.i18n.json(1 hunks)src/lockScreen/LockScreen.tsx(1 hunks)src/lockScreen/lock-screen.tsx(1 hunks)src/main.ts(2 hunks)src/public/lockScreen.html(1 hunks)src/store/rootReducer.ts(2 hunks)src/ui/actions.ts(2 hunks)src/ui/components/SettingsView/GeneralTab.tsx(2 hunks)src/ui/components/SettingsView/features/ScreenLock.tsx(1 hunks)src/ui/main/menuBar.ts(2 hunks)src/ui/main/serverView/popupMenu.ts(2 hunks)src/ui/reducers/screenLockPasswordHash.ts(1 hunks)src/ui/reducers/screenLockTimeoutSeconds.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/i18n/nn.i18n.json
- src/public/lockScreen.html
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/PersistableValues.tssrc/ui/main/serverView/popupMenu.tssrc/app/selectors.tssrc/ui/main/menuBar.tssrc/lockScreen/LockScreen.tsxsrc/store/rootReducer.tssrc/ui/components/SettingsView/features/ScreenLock.tsxsrc/ui/reducers/screenLockPasswordHash.tssrc/main.tssrc/ui/actions.tssrc/app/main/screenLock.tssrc/ui/reducers/screenLockTimeoutSeconds.tssrc/app/main/app.tssrc/lockScreen/lock-screen.tsxsrc/ui/components/SettingsView/GeneralTab.tsx
src/store/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Write Redux actions following the Flux Standard Action (FSA) convention
Files:
src/store/rootReducer.ts
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/SettingsView/features/ScreenLock.tsxsrc/ui/components/SettingsView/GeneralTab.tsx
src/main.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
src/main.tsas the Electron main process entry point compiled by Rollup
Files:
src/main.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:
rollup.config.mjs
🧬 Code graph analysis (11)
src/ui/main/serverView/popupMenu.ts (2)
src/store/index.ts (1)
dispatch(38-40)src/ui/actions.ts (1)
MENU_BAR_LOCK_SCREEN_CLICKED(153-153)
src/app/selectors.ts (3)
src/store/rootReducer.ts (1)
RootState(109-109)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/reducers/screenLockPasswordHash.ts (1)
screenLockPasswordHash(6-22)
src/ui/main/menuBar.ts (3)
src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)src/store/index.ts (1)
dispatch(38-40)src/ui/actions.ts (1)
MENU_BAR_LOCK_SCREEN_CLICKED(153-153)
src/ui/components/SettingsView/features/ScreenLock.tsx (4)
src/store/actions.ts (1)
RootAction(44-46)src/store/rootReducer.ts (1)
RootState(109-109)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/actions.ts (2)
SETTINGS_SET_SCREEN_LOCK_TIMEOUT_CHANGED(145-146)SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(147-148)
src/ui/reducers/screenLockPasswordHash.ts (2)
src/ui/actions.ts (1)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(149-150)src/app/actions.ts (1)
APP_SETTINGS_LOADED(6-6)
src/main.ts (1)
src/app/main/screenLock.ts (1)
setupScreenLock(64-193)
src/app/main/screenLock.ts (5)
src/store/index.ts (1)
dispatch(38-40)src/ui/actions.ts (4)
MENU_BAR_LOCK_SCREEN_CLICKED(153-153)WEBVIEW_FOCUS_REQUESTED(68-68)WEBVIEW_MESSAGE_BOX_FOCUSED(70-70)WEBVIEW_USER_LOGGED_IN(84-84)src/store/rootReducer.ts (1)
RootState(109-109)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)
src/ui/reducers/screenLockTimeoutSeconds.ts (2)
src/ui/actions.ts (1)
SETTINGS_SET_SCREEN_LOCK_TIMEOUT_CHANGED(145-146)src/app/actions.ts (1)
APP_SETTINGS_LOADED(6-6)
src/app/main/app.ts (5)
src/app/main/persistence.ts (1)
getPersistedValues(28-29)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)src/utils/browserLauncher.ts (1)
preloadBrowsersList(96-103)src/store/index.ts (3)
listen(88-114)listen(144-157)dispatch(38-40)src/ui/actions.ts (3)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(147-148)SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(149-150)MENU_BAR_LOCK_SCREEN_CLICKED(153-153)
src/lockScreen/lock-screen.tsx (1)
src/i18n/common.ts (1)
fallbackLng(3-3)
src/ui/components/SettingsView/GeneralTab.tsx (1)
src/ui/components/SettingsView/features/ScreenLock.tsx (1)
ScreenLock(26-127)
🔇 Additional comments (17)
src/i18n/hu.i18n.json (1)
272-283: LGTM! Translation structure is correct.The Hungarian translations are properly structured with
screenLockundersettings.optionsandlockScreenas a top-level key. The keys and nesting align with the feature implementation.Also applies to: 427-434
src/i18n/de-DE.i18n.json (1)
250-260: LGTM! German translations are correctly structured.The translations follow the correct structure with
screenLockundersettings.optionsandlockScreenat the top level. All required keys are present.Also applies to: 397-404
src/i18n/fr.i18n.json (1)
248-258: LGTM! French translations are correctly structured.The French localization follows the correct pattern with
screenLocknested undersettings.optionsandlockScreenas a top-level key. All required translation keys are present.Also applies to: 396-403
src/i18n/sv.i18n.json (1)
282-292: LGTM! Swedish translations are correctly structured.The Swedish localization correctly places
screenLockundersettings.optionsandlockScreenat the top level, matching the structure used in other locales. All required keys are present.Also applies to: 452-459
src/ui/actions.ts (2)
144-153: LGTM! Action constants are well-defined.The new screen lock action constants follow the existing naming conventions and include helpful comments distinguishing between plaintext and hashed password actions. The naming is clear and consistent.
287-290: LGTM! Type mappings are correct.The payload type mappings for the new screen lock actions are correctly defined:
numberfor timeout (seconds)stringfor plaintext password inputstring | nullfor hashed password (allowing null to clear)voidfor the lock screen menu actionsrc/i18n/zh-CN.i18n.json (1)
132-142: LGTM!The Chinese (zh-CN) translations for the screen lock feature are properly structured and consistent with the i18n patterns used in other locale files.
Also applies to: 278-285
src/ui/components/SettingsView/GeneralTab.tsx (1)
13-13: LGTM!The ScreenLock component is properly imported and integrated into the general settings tab, following the established pattern for other settings features.
Also applies to: 33-33
src/app/selectors.ts (1)
72-75: LGTM!The new selectors for screen lock state follow the established pattern and correctly map to the types defined in the reducers.
src/main.ts (1)
13-13: LGTM!The screen lock setup is properly initialized after the root window is shown, following the established initialization pattern. The inline comment clearly explains the purpose.
Also applies to: 80-81
src/ui/main/serverView/popupMenu.ts (1)
218-225: Verify the placement of the lock screen item in context menus.The lock screen menu item is added to
createDefaultMenuTemplate, which means it will appear in every context menu across the application (right-click on text fields, images, links, etc.). Typically, application-level actions like "Lock Screen" are reserved for the main menu bar only, not content-specific context menus.Consider whether this is the intended UX. If the lock screen action should only be accessible via the main menu bar and keyboard shortcut, remove it from the context menu and keep it only in
src/ui/main/menuBar.ts.If this placement is intentional, please confirm the reasoning for making the lock screen action available in all context menus.
src/ui/main/menuBar.ts (1)
29-29: LGTM!The lock screen menu item is properly integrated into the application menu with appropriate window visibility and focus handling. The keyboard accelerator (CommandOrControl+L) is well-chosen and follows the established pattern.
Also applies to: 108-121
src/i18n/se.i18n.json (1)
1-28: LGTM!The Swedish (se) translations for the screen lock feature are properly structured and consistent with the i18n patterns used across other locale files.
rollup.config.mjs (1)
111-144: LGTM!The lock screen renderer bundle configuration correctly mirrors the video-call window bundle pattern, using the same plugin chain and external module resolution strategy. The configuration is appropriate for a dedicated renderer bundle.
src/store/rootReducer.ts (1)
41-42: Reducers wired correctly; consider redaction for sensitive state in devtools/logging.The new slices are correctly imported and combined. Since
screenLockPasswordHashis sensitive, ensure Redux devtools/loggers/state dumps sanitize or omit this field.Also applies to: 105-107
src/lockScreen/lock-screen.tsx (1)
1-88: Renderer–main API availability: prefer preload over runtime injectionIf the main process continues injecting window.electronAPI after did‑finish‑load, LockScreen code may run before injection. With the preload proposal in app.ts, this race goes away and the API is available at first paint.
Confirm LockScreen doesn’t access window.electronAPI before did‑finish‑load. If unsure, adopt the preload approach suggested in app.ts to guarantee availability.
src/app/main/app.ts (1)
138-141: Confirm lockScreen.html is packaged to app/lockScreen.html in production
lockScreen.html currently lives at src/public/lockScreen.html; ensure your build config (e.g. electron-builder’sfilesorextraResources) copies it toapp/lockScreen.htmlrelative toapp.getAppPath(), otherwiseloadFilewill fail.
…me code injection; use a preload with contextBridge
…view (or remove it entirely)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/main/app.ts (2)
229-310: LGTM! Proper rate-limiting and origin tracking.The throttling mechanism correctly:
- Identifies callers by sender ID
- Enforces attempt limits per sender
- Applies exponential backoff delays
- Cleans up expired records
Note: Password hashing concern flagged in previous review.
As noted in past review comments (lines 496-510), the SHA-256 hashing (lines 256-259) should be replaced with a proper password hashing algorithm like bcrypt, scrypt, or pbkdf2 for better security against offline attacks.
496-510: Password hashing concern flagged in previous review.As noted in the existing review comment for these lines, SHA-256 is unsuitable for password storage. The previous review recommends using a salted, slow hash function (pbkdf2, scrypt, or argon2) to protect against offline attacks if the storage leaks.
🧹 Nitpick comments (1)
src/lockScreen/preload.ts (1)
1-7: LGTM! Secure preload implementation.The use of
contextBridgewith a minimal IPC surface is the correct approach for the lock screen. This properly isolates the renderer while exposing only the necessary unlock functionality.Optional: Consider more specific return types.
The return types are currently
Promise<any>. For better type safety, consider:contextBridge.exposeInMainWorld('electronAPI', { - verifyPassword: (password: string) => - ipcRenderer.invoke('lock:verify', password), - unlockApp: () => ipcRenderer.invoke('lock:unlock'), + verifyPassword: (password: string): Promise<boolean> => + ipcRenderer.invoke('lock:verify', password), + unlockApp: (): Promise<boolean> => ipcRenderer.invoke('lock:unlock'), });
📜 Review details
Configuration used: CodeRabbit 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 (2)
src/app/main/app.ts(6 hunks)src/lockScreen/preload.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/lockScreen/preload.ts
🧠 Learnings (1)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
PR: RocketChat/Rocket.Chat.Electron#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/lockScreen/preload.ts
🧬 Code graph analysis (1)
src/app/main/app.ts (5)
src/app/main/persistence.ts (1)
getPersistedValues(28-29)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)src/utils/browserLauncher.ts (1)
preloadBrowsersList(96-103)src/store/index.ts (3)
listen(88-114)listen(144-157)dispatch(38-40)src/ui/actions.ts (3)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(147-148)SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(149-150)MENU_BAR_LOCK_SCREEN_CLICKED(153-153)
🔇 Additional comments (5)
src/app/main/app.ts (5)
50-64: LGTM! Solid throttling implementation.The rate-limiting approach is well-designed:
- Per-sender tracking prevents cross-contamination
- Exponential backoff with a reasonable cap (30s)
- Automatic cleanup prevents memory leaks
128-136: LGTM! Secure BrowserView configuration.The BrowserView correctly uses:
nodeIntegration: falsecontextIsolation: true- Preload script at a fixed build path
This properly isolates the lock screen renderer from Node APIs.
Verify the preload path matches your build configuration.
Ensure that
app/lockPreload.jsis correctly output during the build process fromsrc/lockScreen/preload.ts.
312-344: LGTM! Proper origin validation for unlock.The unlock handler correctly validates that the request originates from the lock BrowserView itself (line 315), preventing any other renderer from bypassing the lock.
512-515: LGTM! Straightforward menu integration.The listener correctly triggers the lock overlay when the menu item is clicked.
465-468: LGTM! Proper initialization sequence.Registering the lock IPC handlers after
app.whenReady()ensures Electron's IPC infrastructure is fully available.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/ui/components/SettingsView/features/ScreenLock.tsx (1)
70-77: Stop dispatching plaintext passwords through Redux.Vi sender fortsatt råpassordet i
SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED, så hele Redux‑kjeden (renderer, devtools, IPC logs) ser feltet i klartekst. Med den nye IPC‑strømmen for låselogikk bør renderer kalle den dedikerte preload‑metoden (f.eks.await window.electronAPI.screenLock.setPassword(password)), la main‑prosessen hashe og persistere, og deretter kun dispatch’eSETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHEDmed resultatet (eller ingenting). Vennligst fjern plaintext‑dispatchen helt.
🧹 Nitpick comments (5)
src/app/main/screenLock.ts (2)
64-66: Make setupScreenLock idempotentPrevent duplicate listeners if called twice.
Apply this diff:
export const setupScreenLock = (): void => { + // Idempotency guard + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if ((setupScreenLock as any)._initialized) return; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (setupScreenLock as any)._initialized = true;
25-41: Reset lastStartAt when timer is clearedPrevents the “ignore clears within MIN_CLEAR_IGNORE_MS” window from lingering after a successful clear.
Apply this diff:
if (inactivityTimer) { clearTimeout(inactivityTimer); inactivityTimer = null; } + lastStartAt = 0;src/app/main/app.ts (3)
466-481: Don’t await KDF migration; keep verify snappyThe comment says “Fire-and-forget re-hash,” but it’s awaited. Remove await to return success immediately.
Apply this diff:
- // Fire-and-forget re-hash; do not block unlock - await (async () => { + // Fire-and-forget re-hash; do not block unlock + (async () => { try { const newStored = await hashPlainPassword(String(password)); dispatch({ type: SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED, payload: newStored as unknown as ScreenLockPasswordStored, }); } catch (e) { // Do not prevent unlocking; just log migration failures console.error( 'Error migrating legacy screen lock password to scrypt:', e ); } - })(); + })();
269-276: Remove unused lockState move/resize listener fieldsFields are never assigned; cleanup unused state and related removal code.
Apply this diff:
const lockState: { originalBounds?: Electron.Rectangle; prevResizable?: boolean; prevMinimizable?: boolean; prevMaximizable?: boolean; - moveListener?: () => void; - resizeListener?: () => void; } = {};- // restore window flags and remove listeners + // restore window flags try { - if (lockState.moveListener) { - rootWindow.removeListener('move', lockState.moveListener); - } - if (lockState.resizeListener) { - rootWindow.removeListener('resize', lockState.resizeListener); - } - // remove the updateBounds listeners we added earlier rootWindow.removeListener('resize', updateBounds); rootWindow.removeListener('move', updateBounds); if (typeof lockState.prevResizable === 'boolean') { rootWindow.setResizable(!!lockState.prevResizable); }Also applies to: 359-367
414-524: Optional: fold unlock into verify to reduce surfaceConsider removing 'lock:unlock' and performing unlock directly on successful 'lock:verify'. Simplifies IPC and avoids state drift between calls.
I can provide a patch if you want this change now.
Also applies to: 526-558
📜 Review details
Configuration used: CodeRabbit 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 (14)
src/app/PersistableValues.ts(2 hunks)src/app/main/app.ts(6 hunks)src/app/main/screenLock.ts(1 hunks)src/i18n/it-IT.i18n.json(1 hunks)src/i18n/ja.i18n.json(1 hunks)src/i18n/nb-NO.i18n.json(1 hunks)src/i18n/no.i18n.json(2 hunks)src/i18n/sv.i18n.json(3 hunks)src/i18n/zh-TW.i18n.json(2 hunks)src/i18n/zh.i18n.json(1 hunks)src/lockScreen/LockScreen.tsx(1 hunks)src/ui/actions.ts(3 hunks)src/ui/components/SettingsView/features/ScreenLock.tsx(1 hunks)src/ui/reducers/screenLockPasswordHash.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/i18n/it-IT.i18n.json
- src/i18n/zh-TW.i18n.json
- src/ui/actions.ts
- src/i18n/nb-NO.i18n.json
- src/i18n/sv.i18n.json
- src/lockScreen/LockScreen.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/PersistableValues.tssrc/app/main/app.tssrc/app/main/screenLock.tssrc/ui/components/SettingsView/features/ScreenLock.tsxsrc/ui/reducers/screenLockPasswordHash.ts
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/SettingsView/features/ScreenLock.tsx
🧬 Code graph analysis (4)
src/app/main/app.ts (6)
src/app/main/persistence.ts (1)
getPersistedValues(28-29)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)src/store/index.ts (3)
dispatch(38-40)listen(88-114)listen(144-157)src/ui/actions.ts (3)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(150-151)SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(148-149)MENU_BAR_LOCK_SCREEN_CLICKED(154-154)src/app/PersistableValues.ts (1)
ScreenLockPasswordStored(95-100)src/utils/browserLauncher.ts (1)
preloadBrowsersList(96-103)
src/app/main/screenLock.ts (5)
src/store/index.ts (1)
dispatch(38-40)src/ui/actions.ts (4)
MENU_BAR_LOCK_SCREEN_CLICKED(154-154)WEBVIEW_FOCUS_REQUESTED(69-69)WEBVIEW_MESSAGE_BOX_FOCUSED(71-71)WEBVIEW_USER_LOGGED_IN(85-85)src/store/rootReducer.ts (1)
RootState(109-109)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)
src/ui/components/SettingsView/features/ScreenLock.tsx (5)
src/store/index.ts (1)
dispatch(38-40)src/store/actions.ts (1)
RootAction(44-46)src/store/rootReducer.ts (1)
RootState(109-109)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/actions.ts (2)
SETTINGS_SET_SCREEN_LOCK_TIMEOUT_CHANGED(146-147)SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(148-149)
src/ui/reducers/screenLockPasswordHash.ts (3)
src/app/PersistableValues.ts (1)
ScreenLockPasswordStored(95-100)src/ui/actions.ts (1)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(150-151)src/app/actions.ts (1)
APP_SETTINGS_LOADED(6-6)
🔇 Additional comments (3)
src/i18n/ja.i18n.json (1)
206-230: Translated lock-screen strings look solid.Key structure matches other locales, and the Japanese copy conveys the settings/lock-screen context naturally. 👍
src/app/main/screenLock.ts (1)
135-187: Solid race handling around blur/focus and IPC activityGrace windows, delayed blur start, and focus checks look robust and well thought out. LGTM.
src/app/main/app.ts (1)
526-558: Good: unlock gated to lock BrowserView senderValidates event.sender against lockWindow.webContents before unlocking. This meaningfully closes the bypass.
…d update state management
… for setting and verifying lock password
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/app/actions.ts (1)
13-13: LGTM; add typed action creator for ergonomics (optional)Constant and payload mapping look good. Consider exporting a small creator like setAppIsScreenLocked(locked: boolean) to avoid ad-hoc action shapes.
Also applies to: 24-24
src/lockScreen/LockScreen.tsx (1)
45-49: Minor UX/a11y: submit form, clear error on typing, and add ARIAUse a form submit to handle Enter, clear error as user types, and add aria attributes.
- const onKey = (e: React.KeyboardEvent<HTMLInputElement>) => { - if (e.key === 'Enter') { - tryUnlock(); - } - }; + const onSubmit = (e: React.FormEvent<HTMLFormElement>) => { + e.preventDefault(); + void tryUnlock(); + };- return ( - <div style={{ textAlign: 'center', color: 'white' }}> + return ( + <div style={{ textAlign: 'center', color: 'white' }}> <h2 style={{ margin: 0 }}>{t('lockScreen.title')}</h2> @@ - <div style={{ marginTop: 18 }}> - <input + <form style={{ marginTop: 18 }} onSubmit={onSubmit}> + <input type='password' value={password} - onChange={(e) => setPassword(e.target.value)} - onKeyDown={onKey} + onChange={(e) => { + setError(null); + setPassword(e.target.value); + }} placeholder={t('lockScreen.placeholder')} + aria-label={t('lockScreen.placeholder')} + aria-invalid={Boolean(error)} + aria-describedby={error ? 'lock-error' : undefined} @@ - <button - onClick={tryUnlock} - disabled={busy} + <button + type="submit" + disabled={busy || password.length === 0} style={{ marginLeft: 8, padding: '8px 12px', borderRadius: 4 }} > {t('lockScreen.unlock')} </button> - </div> - {error && <div style={{ color: '#ff6b6b', marginTop: 12 }}>{error}</div>} + </form> + {error && ( + <div id="lock-error" style={{ color: '#ff6b6b', marginTop: 12 }}> + {error} + </div> + )}Also applies to: 51-83
src/ui/components/SettingsView/features/ScreenLock.tsx (1)
54-66: Avoid dispatching Redux actions on every keystroke.The
handleTimeoutInputhandler currently dispatchesSETTINGS_SET_SCREEN_LOCK_TIMEOUT_CHANGEDon every keystroke, which can cause excessive Redux updates and performance issues. The blur handler (handleTimeoutChange) already commits the final value when the user finishes editing.Refactor
handleTimeoutInputto only update local state:const handleTimeoutInput = useCallback( (event: React.ChangeEvent<HTMLInputElement>) => { const raw = event.target.value; setTimeoutValue(raw); - const value = parseInt(raw, 10); - const seconds = Number.isFinite(value) && value >= 0 ? value : 0; - dispatch({ - type: SETTINGS_SET_SCREEN_LOCK_TIMEOUT_CHANGED, - payload: seconds, - }); }, - [dispatch] + [] );The blur handler will still commit the value to Redux when editing is complete.
src/app/main/app.ts (1)
427-429: Type assertion for sender ID is acceptable but consider documenting.The type assertion
(event as any)?.sender?.idis necessary due to incomplete Electron typings, but it assumessender.idexists and is stable across the session.Consider adding a comment documenting the assumption:
- const senderId = (event as any)?.sender?.id - ? String((event as any).sender.id) - : 'unknown'; + // event.sender.id is a stable identifier for the webContents within this session + // but not included in Electron's type definitions + const senderId = (event as any)?.sender?.id + ? String((event as any).sender.id) + : 'unknown';Alternatively, if targeting a specific Electron version, check if the types can be augmented:
declare module 'electron' { interface WebContents { id: number; } }
📜 Review details
Configuration used: CodeRabbit 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 (15)
rollup.config.mjs(1 hunks)src/app/PersistableValues.ts(2 hunks)src/app/actions.ts(2 hunks)src/app/main/app.ts(8 hunks)src/app/main/screenLock.ts(1 hunks)src/app/reducers/isScreenLocked.ts(1 hunks)src/app/selectors.ts(1 hunks)src/i18n/no.i18n.json(2 hunks)src/i18n/zh.i18n.json(1 hunks)src/lockScreen/LockScreen.tsx(1 hunks)src/main.ts(3 hunks)src/preload.ts(3 hunks)src/store/rootReducer.ts(3 hunks)src/types/electron-api.d.ts(1 hunks)src/ui/components/SettingsView/features/ScreenLock.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- rollup.config.mjs
- src/app/selectors.ts
- src/main.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/actions.tssrc/app/reducers/isScreenLocked.tssrc/preload.tssrc/app/main/screenLock.tssrc/ui/components/SettingsView/features/ScreenLock.tsxsrc/types/electron-api.d.tssrc/app/PersistableValues.tssrc/lockScreen/LockScreen.tsxsrc/store/rootReducer.tssrc/app/main/app.ts
src/preload.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
src/preload.tsas the preload script bridging main and renderer
Files:
src/preload.ts
src/ui/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/ui/**/*.tsx: Implement React UI with functional components and hooks
Use Fuselage components (Box, Button, TextInput, Modal, etc.) and import from@rocket.chat/fuselageinstead of raw HTML elements
For Fuselage theming, validate tokens againstTheme.d.tsand only use documented values
Name React component files in PascalCase; non-component files should follow camelCase naming
Files:
src/ui/components/SettingsView/features/ScreenLock.tsx
src/store/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Write Redux actions following the Flux Standard Action (FSA) convention
Files:
src/store/rootReducer.ts
🧠 Learnings (1)
📚 Learning: 2025-09-26T19:24:21.364Z
Learnt from: CR
PR: RocketChat/Rocket.Chat.Electron#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/preload.ts
🧬 Code graph analysis (4)
src/app/reducers/isScreenLocked.ts (1)
src/app/actions.ts (2)
APP_IS_SCREEN_LOCKED_SET(13-13)APP_SETTINGS_LOADED(6-6)
src/app/main/screenLock.ts (5)
src/store/index.ts (1)
dispatch(38-40)src/ui/actions.ts (4)
MENU_BAR_LOCK_SCREEN_CLICKED(154-154)WEBVIEW_FOCUS_REQUESTED(69-69)WEBVIEW_MESSAGE_BOX_FOCUSED(71-71)WEBVIEW_USER_LOGGED_IN(85-85)src/store/rootReducer.ts (1)
RootState(111-111)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)
src/ui/components/SettingsView/features/ScreenLock.tsx (5)
src/store/index.ts (1)
dispatch(38-40)src/store/actions.ts (1)
RootAction(44-46)src/store/rootReducer.ts (1)
RootState(111-111)src/ui/reducers/screenLockTimeoutSeconds.ts (1)
screenLockTimeoutSeconds(6-22)src/ui/actions.ts (1)
SETTINGS_SET_SCREEN_LOCK_TIMEOUT_CHANGED(146-147)
src/app/main/app.ts (6)
src/store/index.ts (4)
select(63-64)dispatch(38-40)listen(88-114)listen(144-157)src/ui/main/rootWindow.ts (1)
getRootWindow(47-52)src/app/actions.ts (1)
APP_IS_SCREEN_LOCKED_SET(13-13)src/ui/actions.ts (3)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(150-151)SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(148-149)MENU_BAR_LOCK_SCREEN_CLICKED(154-154)src/app/PersistableValues.ts (1)
ScreenLockPasswordStored(95-100)src/utils/browserLauncher.ts (1)
preloadBrowsersList(96-103)
🔇 Additional comments (5)
src/app/reducers/isScreenLocked.ts (1)
5-19: LGTMReducer shape and APP_SETTINGS_LOADED bootstrap look correct.
src/i18n/no.i18n.json (2)
281-292: LGTMKeys and phrasing look good; description reflects generic salted hash.
451-459: LGTMLock screen strings are clear and consistent.
src/types/electron-api.d.ts (1)
3-15: LGTMType surface matches preload usage; optional methods support differing contexts.
src/app/main/screenLock.ts (1)
70-81: Clarify "inactivity timeout" vs. unfocused-only behaviorUI strings refer to an “inactivity timeout,” but
src/app/main/screenLock.ts(70–81, 137–156) only starts the lock timer on blur and stops it on focus—there’s no idle tracking while focused. Either:
- Implement true inactivity detection (start/reset the timer on user input/IPCs when focused), retaining blur as an additional trigger.
- Or update all UI copy/locales to clearly state a “timeout when unfocused.”
…locks shortly after blur
# Conflicts: # src/i18n/ar.i18n.json # src/i18n/de-DE.i18n.json # src/i18n/en.i18n.json # src/i18n/es.i18n.json # src/i18n/fi.i18n.json # src/i18n/fr.i18n.json # src/i18n/hu.i18n.json # src/i18n/it-IT.i18n.json # src/i18n/ja.i18n.json # src/i18n/nb-NO.i18n.json # src/i18n/nn.i18n.json # src/i18n/no.i18n.json # src/i18n/pl.i18n.json # src/i18n/pt-BR.i18n.json # src/i18n/ru.i18n.json # src/i18n/se.i18n.json # src/i18n/sv.i18n.json # src/i18n/tr-TR.i18n.json # src/i18n/uk-UA.i18n.json # src/i18n/zh-CN.i18n.json # src/i18n/zh-TW.i18n.json # src/i18n/zh.i18n.json # src/preload.ts # src/ui/actions.ts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit 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 (26)
src/app/main/app.ts(8 hunks)src/i18n/ar.i18n.json(1 hunks)src/i18n/de-DE.i18n.json(2 hunks)src/i18n/en.i18n.json(3 hunks)src/i18n/es.i18n.json(2 hunks)src/i18n/fi.i18n.json(2 hunks)src/i18n/fr.i18n.json(2 hunks)src/i18n/hu.i18n.json(2 hunks)src/i18n/it-IT.i18n.json(1 hunks)src/i18n/ja.i18n.json(1 hunks)src/i18n/nb-NO.i18n.json(1 hunks)src/i18n/nn.i18n.json(1 hunks)src/i18n/no.i18n.json(2 hunks)src/i18n/pl.i18n.json(2 hunks)src/i18n/pt-BR.i18n.json(2 hunks)src/i18n/ru.i18n.json(2 hunks)src/i18n/se.i18n.json(1 hunks)src/i18n/sv.i18n.json(3 hunks)src/i18n/tr-TR.i18n.json(2 hunks)src/i18n/uk-UA.i18n.json(2 hunks)src/i18n/zh-CN.i18n.json(2 hunks)src/i18n/zh-TW.i18n.json(2 hunks)src/i18n/zh.i18n.json(1 hunks)src/preload.ts(3 hunks)src/store/rootReducer.ts(3 hunks)src/ui/actions.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/i18n/ar.i18n.json
🚧 Files skipped from review as they are similar to previous changes (15)
- src/i18n/zh-TW.i18n.json
- src/i18n/es.i18n.json
- src/i18n/pl.i18n.json
- src/i18n/it-IT.i18n.json
- src/i18n/nn.i18n.json
- src/i18n/uk-UA.i18n.json
- src/i18n/fr.i18n.json
- src/ui/actions.ts
- src/i18n/pt-BR.i18n.json
- src/i18n/de-DE.i18n.json
- src/i18n/tr-TR.i18n.json
- src/i18n/sv.i18n.json
- src/i18n/hu.i18n.json
- src/i18n/ja.i18n.json
- src/i18n/no.i18n.json
🧰 Additional context used
📓 Path-based instructions (3)
src/preload.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
src/preload.tsas the preload script bridging main and renderer
Files:
src/preload.ts
**/*.{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/preload.tssrc/app/main/app.tssrc/store/rootReducer.ts
src/store/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Write Redux actions following the Flux Standard Action (FSA) convention
Files:
src/store/rootReducer.ts
🧠 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/preload.ts : Keep `src/preload.ts` as the preload script bridging main and renderer
Applied to files:
src/preload.tssrc/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/ipc/**/*.ts : Define and maintain IPC channels in `src/ipc/` for main/renderer state synchronization
Applied to files:
src/preload.tssrc/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.ts : Keep `src/main.ts` as the Electron main process entry point compiled by Rollup
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 {rollup.config.mjs,electron-builder.json,tsconfig.json,.eslintrc.json,jest.config.js} : Maintain and configure the project via the designated root config files for build, packaging, TypeScript, ESLint, and Jest
Applied to files:
src/app/main/app.ts
🧬 Code graph analysis (1)
src/app/main/app.ts (6)
src/store/index.ts (4)
select(63-64)dispatch(38-40)listen(88-114)listen(144-157)src/ui/main/rootWindow.ts (1)
getRootWindow(47-60)src/app/actions.ts (1)
APP_IS_SCREEN_LOCKED_SET(13-13)src/ui/actions.ts (3)
SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED(154-155)SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGED(152-153)MENU_BAR_LOCK_SCREEN_CLICKED(156-156)src/app/PersistableValues.ts (1)
ScreenLockPasswordStored(95-100)src/utils/browserLauncher.ts (1)
preloadBrowsersList(96-103)
🔇 Additional comments (23)
src/i18n/zh.i18n.json (1)
27-53: Both previous issues have been properly fixed! ✓The corrected structure now matches expectations:
Top-level lockScreen object (lines 46–53): No longer nested under
settings, aligning with other locale files (hu, de-DE, fr, sv). The lock screen UI should now load Chinese translations correctly.Generic password hashing description (line 41): Changed from the SHA-256–specific text to the more accurate and flexible "密码将使用带盐的安全哈希方式存储。" This correctly reflects the implementation's use of salted hashing regardless of the underlying algorithm.
The JSON structure is syntactically valid with proper nesting and formatting.
src/i18n/nb-NO.i18n.json (1)
27-54: ✅ Prior fix correctly applied—lockScreen now at root level.The critical structural issue flagged in the previous review has been properly resolved. The
lockScreenkeys are now correctly positioned at the root level (line 46), as siblings tosettings(line 28), which aligns with the pattern across other locale files. Translation lookups liket('lockScreen.title')will now resolve correctly without falling back to English.The new translation entries are well-organized:
- Settings section (lines 28–45): Covers configuration UI for screen lock timeout and password.
- LockScreen section (lines 46–53): Covers UI strings for the actual lock screen display and unlock flow.
All Norwegian Bokmål translations appear semantically appropriate and complete for the lock feature functionality.
src/i18n/fi.i18n.json (2)
248-258: Screen lock settings translations look good.The Finnish translations for the timeout and password settings are grammatically correct and clearly communicate the intent. The descriptions properly explain that timeout is in seconds and that passwords are SHA-256 hashed, aligning with user expectations for security-sensitive settings.
421-428: Lock screen UI translations are complete, consistent, and properly consumed.Verification confirms all six translation keys are actively used in
src/lockScreen/LockScreen.tsx: title (line 53), description (line 55), placeholder (line 63), unlock (line 78), incorrect (line 36), and unlockFailed (lines 29, 39). The terminology is consistent with settings translations.src/preload.ts (2)
17-27: Type design aligns with security separation.The optional methods in the
Window.electronAPIinterface correctly reflect that different contexts expose different capabilities. Based on the implementation (lines 34-39), this preload only exposessetLockPassword, whileverifyPasswordandunlockAppare exposed in the lock screen's separate preload script (lockPreload.js referenced in app.ts). This follows the principle of least privilege.
34-39: Authorization logic is properly implemented—preload API surface is secure.Verification confirms the
lock:setIPC handler (src/app/main/app.ts, lines 548-554) correctly restricts invocations to only the lock window or root window (Settings UI). The sender's webContents is validated against known allowed windows, with proper error handling for unauthorized attempts.src/i18n/en.i18n.json (1)
244-254: Translation keys are complete and well-structured.The new English translations cover all necessary UI elements for the screen lock feature:
- Settings options for timeout and password configuration
- Menu item for triggering the lock
- Lock screen UI messages including error states
The descriptions clearly communicate the functionality (e.g., SHA-256 hash storage, inactivity-based timeout).
Also applies to: 357-357, 479-486
src/store/rootReducer.ts (1)
6-6: Redux state integration is correct.The three new reducers (
isScreenLocked,screenLockPasswordHash,screenLockTimeoutSeconds) are properly imported and integrated into the root reducer. TheRootStatetype will automatically include these new slices throughReturnType<typeof rootReducer>.Also applies to: 43-44, 108-110
src/i18n/zh-CN.i18n.json (1)
132-142: Chinese translations are complete and parallel English version.The Chinese (Simplified) translations for the screen lock feature are properly structured and cover the same keys as the English version.
Also applies to: 304-311
src/app/main/app.ts (10)
52-66: Throttling configuration is well-designed.The rate-limiting parameters are appropriate:
- 5 attempts per 5-minute window aligns with industry best practices
- Base 500ms backoff with exponential growth (up to 30s in the handler) effectively mitigates brute-force attacks
- Per-sender tracking via
webContents.idprevents a single attacker from exhausting attempts for all users- The
timeoutfield enables automatic cleanup to prevent unbounded memory growth
68-117: Password hashing with scrypt is a significant security improvement.The implementation correctly uses scrypt instead of the previously flagged unsalted SHA-256:
- Memory-hard algorithm (scrypt) provides resistance against GPU/ASIC attacks
- Cost parameters (N=16384, r=8, p=1) meet OWASP recommendations
- Random salt (16 bytes) prevents rainbow table attacks
- Structured storage includes algorithm, hash, salt, and parameters for future algorithm migration
- Async API avoids blocking the main thread during key derivation
This addresses the major security concern from the previous review.
119-250: Password verification provides excellent backward compatibility.The verification logic properly handles multiple password formats:
- New scrypt format: Full parameter extraction and timing-safe comparison
- Legacy formats: pbkdf2, SHA-256 object, and hex string variants
- Timing-safe comparison: Uses
crypto.timingSafeEqualto prevent timing attacks- Graceful degradation: Unknown formats safely return false
This allows seamless migration from older password storage formats while maintaining security for new passwords.
279-412: Lock window implementation correctly addresses previous security concerns.The
showLockWindowfunction has been properly hardened:Security improvements (addressing past review):
- ✅
nodeIntegration: false(line 323)- ✅
contextIsolation: true(line 324)- ✅ Dedicated preload script instead of runtime
executeJavaScriptinjection (line 326)Additional strengths:
- Checks password configuration before showing lock (lines 282-288)
- Preserves window state for seamless unlock UX (lines 300-315)
- BrowserView overlay approach keeps lock within the app window (not above other apps)
- Proper cleanup and state restoration on destroy (lines 358-408)
424-505: lock:verify handler properly implements security hardening.The handler correctly addresses all concerns from the previous review:
Origin validation (lines 427-429):
- ✅ Verifies
event.sender === lockWindow.webContents- Prevents arbitrary renderers from attempting verification
Rate limiting and backoff (lines 431-498):
- ✅ Per-sender attempt tracking via
webContents.id- ✅ Max 5 attempts per 5-minute window
- ✅ Exponential backoff (500ms → 30s)
- ✅ Automatic cleanup via
setTimeoutto prevent memory leaksAdditional security measures:
- Prefers in-memory state to avoid TOCTOU races (lines 448-450)
- Resets counters on successful verification (lines 458-467)
- Safe error handling with consistent false response
508-540: lock:unlock handler correctly restricts access to lock screen.The handler addresses the previous critical security issue:
Origin validation (lines 510-512):
- ✅ Verifies
event.sender === lockWindow.webContents- Prevents arbitrary renderers from unlocking the app
- Only the lock screen itself can trigger unlock
Implementation quality:
- Proper BrowserView cleanup (lines 514-527)
- WebContents destruction with error handling
- State synchronization via Redux dispatch (line 534)
- Root window restoration (lines 529-533)
733-752: Password hashing listener is correct, but note dual code paths.The listener properly handles password changes from the settings UI:
- Receives plaintext password from Redux action
- Asynchronously hashes with scrypt
- Dispatches hashed result for persistence
Note: There are two paths for setting passwords:
- Settings UI →
SETTINGS_SET_SCREEN_LOCK_PASSWORD_CHANGEDaction → this listener → hash → dispatch- Direct IPC call →
lock:sethandler → hash → dispatchBoth paths converge at dispatching
SETTINGS_SET_SCREEN_LOCK_PASSWORD_HASHED, which is appropriate. However, ensure the IPC path (lock:set) has proper authorization as discussed in the previous comment.
755-757: Menu trigger for lock screen is straightforward.The listener correctly responds to the menu bar action by showing the lock overlay. Implementation is simple and appropriate.
704-705: IPC handler registration is properly sequenced.The call to
registerLockIpcHandlers()is correctly placed insetupAppafter the app is ready (as noted in the comment). This ensures IPC handlers are registered before any renderer processes attempt to invoke them, avoiding race conditions.
543-576: Authorization check is adequate; server views are properly isolated. Current password verification gap appears intentional.The concerns in the original review need clarification:
1. Server view isolation (lines 545-554) — VERIFIED ADEQUATE
Server views use separate
WebContentsinstances per server URL (stored in a Map insrc/ui/main/serverView/index.ts), distinct from bothrootWindow.webContentsandlockWindow.webContents. The authorization check correctly restrictslock:setcalls to only these two contexts, preventing server views from invoking the handler regardless of XSS.2. Current password verification — VALID CONCERN, appears intentional
The handler (line 565) hashes and stores the provided password without verifying the existing one. This allows:
- Clearing the password with an empty string (no verification needed)
- Changing the password without providing the current one
The design relies on the Settings UI being trusted (running in
rootWindow.webContents), but if Settings UI is compromised via XSS, an attacker could clear or change the lock password. This is a valid security consideration, though the current architecture intentionally accepts this trade-off.Likely an incorrect or invalid review comment.
src/i18n/ru.i18n.json (3)
251-261: Localization strings for screen lock settings look good.The new
screenLocksettings object is well-structured with clear titles and descriptions. The Russian translations are grammatically sound, and terminology (пароль, блокировка, SHA-256) is used appropriately for a technical settings context.
425-432: Lock screen UI localization is clear and consistent.The
lockScreenobject provides complete UI text for the lock dialog (title, description, placeholder, unlock button, and error messages). The placeholder text matches the settings placeholder for UX consistency, and error messages are user-friendly. Russian translations are appropriate for the context.
1-432: JSON structure is valid and verified.The file parses successfully with no errors.
src/i18n/se.i18n.json (1)
28-45: Swedish translations verified and correctly integrated.All translation keys in
src/i18n/se.i18n.jsonare actively referenced in the codebase. The componentsrc/ui/components/SettingsView/features/ScreenLock.tsxuses all five translation keys (settings.options.screenLock.timeout.title,description,password.title,description, andplaceholder). The JSON structure is valid, and these keys are consistently present across all 22 supported locale files. Backend state management, reducers, and persistence are properly configured.
| "lockScreen": { | ||
| "title": "Skärmen är låst", | ||
| "description": "Lås upp appen för att fortsätta", | ||
| "placeholder": "Ange lösenord", | ||
| "unlock": "Lås upp", | ||
| "incorrect": "Fel lösenord", | ||
| "unlockFailed": "Kunde inte låsa upp" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Lock screen translations are complete and contextually appropriate.
The lock screen UI strings cover all necessary user interactions: title, description, input placeholder, button label, and error messages (incorrect password, unlock failure). Swedish terminology is consistent.
Verify that these translation keys are referenced consistently across all locale files (en, es, pt, de, fr, etc.). Run this to check:
🏁 Script executed:
#!/bin/bash
# Description: Verify lockScreen key structure is present in all locale files
echo "Checking lockScreen structure in all locale files..."
fd -e 'i18n.json' src/i18n/ --exec sh -c '
file="$1"
echo "=== $(basename $file) ==="
jq ".lockScreen? // \"MISSING\"" "$file" 2>/dev/null | head -5
' _ {}
echo -e "\n=== Checking settings.options.screenLock structure ==="
fd -e 'i18n.json' src/i18n/ --exec sh -c '
file="$1"
echo "=== $(basename $file) ==="
jq ".settings.options.screenLock? // \"MISSING\"" "$file" 2>/dev/null | head -5
' _ {}Length of output: 792
🏁 Script executed:
#!/bin/bash
# Description: Verify lockScreen key structure is present in all locale files (corrected)
echo "Checking lockScreen structure in all locale files..."
fd '.i18n.json' 'src/i18n/' --exec sh -c '
file="$1"
echo "=== $(basename $file) ==="
jq ".lockScreen? // \"MISSING\"" "$file" 2>/dev/null
' _ {}
echo -e "\n=== Checking settings.options.screenLock structure ==="
fd '.i18n.json' 'src/i18n/' --exec sh -c '
file="$1"
echo "=== $(basename $file) ==="
jq ".settings.options.screenLock? // \"MISSING\"" "$file" 2>/dev/null
' _ {}Length of output: 13868
Add missing lockScreen translations to Norwegian Nynorsk and Arabic locale files.
Verification across all locale files revealed that nn.i18n.json and ar.i18n.json are missing the lockScreen key entirely, while all other locales (including sv.i18n.json) contain complete lock screen translations. The Swedish translations in sv.i18n.json are consistent and correct, but parity must be restored in the missing locales to avoid incomplete UI rendering for Norwegian Nynorsk and Arabic users.
Files requiring fixes:
src/i18n/nn.i18n.json: AddlockScreenobject with title, description, placeholder, unlock, incorrect, and unlockFailed keyssrc/i18n/ar.i18n.json: AddlockScreenobject with the same structure
🤖 Prompt for AI Agents
In src/i18n/nn.i18n.json and src/i18n/ar.i18n.json (add at the same top-level
position as other locale keys, e.g., near other UI sections), add a new
"lockScreen" object containing the keys title, description, placeholder, unlock,
incorrect, and unlockFailed; populate each with the appropriate Norwegian
Nynorsk and Arabic translations (matching the meaning of the Swedish entries:
"Skärmen är låst" → title, "Lås upp appen för att fortsätta" → description,
"Ange lösenord" → placeholder, "Lås upp" → unlock, "Fel lösenord" → incorrect,
"Kunde inte låsa upp" → unlockFailed) so both locale files have the same
structure as sv.i18n.json and the app can render the lock screen for those
locales.
2 participants
Summary by CodeRabbit
New Features
Chores