feat: log viewer, downloads, settings and document viewer as separate windows on a shared native shell - #3444
Open
rodrigok wants to merge 41 commits into
Open
feat: log viewer, downloads, settings and document viewer as separate windows on a shared native shell#3444rodrigok wants to merge 41 commits into
rodrigok wants to merge 41 commits into
Conversation
Rebuilds the log viewer window around a left filters sidebar and a unified toolbar, and makes the window honour the transparency setting. Window: - Samples isTransparentWindowEnabled at creation and applies transparent + sidebar vibrancy on macOS. `transparent` cannot be toggled afterwards, so the sampled value is passed to the renderer in the page query rather than fetched over IPC — an async read would flash an opaque surface first. - Uses the toolbar as the title bar on macOS (hiddenInset) so the window shows one header instead of a native title bar stacked on an in-app one. Traffic light geometry is derived, not guessed, so toolbar content clears the buttons. Filters: - Levels, contexts and servers are faceted checkbox lists with counts. Each count reflects the other filters, so a count is never unreachable. - Context tags are parsed into a list instead of a whitespace-joined string, which also lets contexts be discovered from the file rather than hardcoded. - Selections persist; "empty means everything" so a stored selection stays valid when new levels or tags appear. List: - Entries are paged in as the reader scrolls instead of being capped by an entry-limit control, which read as a filter but was pagination. Copy and Save act on every match, not just the rendered page. - Day headers are virtual list group headers, so the date stays readable at any scroll position. - Multi-line entries fold to their first line with an expand toggle, search matches are highlighted, and each row can be copied on its own. - Metadata tags sit above the message so every message shares one left edge and one width. Adds specs for the parser, the facet toggle and the paging advance. The paging advance in particular must settle once everything is rendered: the virtual list keeps firing endReached while the last row is in view, so an unbounded increment re-renders forever and wedges the renderer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a histogram above the log list showing how the matching entries are spread over the file's time span, oldest on the left, each bar stacked by level so a burst of errors stays visible inside an otherwise busy period. Dragging across the plot selects a time range and filters the list to it; a plain click selects the single slice under the pointer, and the range clears from the chart, from Clear Filters, or with Escape mid-drag. The chart is built from the matches of every filter *except* the time range. Feeding it the range-filtered set would collapse the chart onto the selection and leave no way back to the rest of the span. Facet counts do include the range, so each control still reports what selecting it would yield. Drag listeners are bound imperatively on mousedown rather than in an effect keyed on drag state: an effect only runs after the next render, so a drag fast enough to finish inside one task lost its own mouseup and stayed stuck — which is exactly what a synthesized-event test caught. No chart library. The part worth owning is bucketing log entries by time and level, which is here and covered by tests; the rendering is flex boxes using the existing palette tokens, and a library would have added a second styling system plus hundreds of KB to a secondary window for one histogram. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
WalkthroughThis change adds standalone document viewer, downloads, settings, and log viewer windows. It adds shared window chrome, persisted window state and bounds, IPC handlers, renderer bundles, redesigned interfaces, search and filtering utilities, document saving, and updated integrations. ChangesSecondary windows and shared foundation
Document viewer
Downloads
Log viewer
Settings
About and existing UI integration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Shell, matching the main window's concept: the log area is an inset rounded card with a hairline and a soft shadow, and the toolbar, sidebar and status bar carry no fill of their own. The panel colour lives on the window root, not on each bar. Painting the bars individually left the card's 4px gutter showing a different surface, so the card had a halo and its rounded corners read as a cut-out. Now body, root and every bar resolve to one continuous colour and only the card paints. That colour also has to be *recessed* or the corners look like a hole punched in a lighter surface. `surface-tint` gives that in the light palette — grey behind a white card — but inverts in the dark one, where it is lighter than `surface-light`, so the dark panel is mixed down from the card colour instead. Transparency now applies without reopening the window. `transparent` cannot be toggled after creation, so — exactly as the root window does — the window is always created transparent with a vibrancy material on macOS, and the setting only decides whether the renderer paints an opaque surface over it. The initial value still arrives in the page query so the first paint matches; changes are pushed to the open window afterwards. The window also reopens at launch when it was open at shutdown, showing itself without taking focus from the main window. Only a deliberate close records `false`: the `closed` handler fires on quit as well, so it checks a `before-quit` flag first, or quitting would erase the state it is meant to restore. The sidebar toggle is a filters glyph instead of a burger, and ghost instead of `pressed` — the filled state read as a heavy block wedged against the traffic lights, and the sidebar's own presence already shows whether it is open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Downloads leaves the main window's view stack and becomes a separate window, built on the same shell the log viewer uses. The shell is now a shared module (src/ui/windowChrome) rather than something each window reimplements: window surfaces and the inset card, the macOS title-bar toolbar with its drag region and traffic-light inset, the sticky day header, the status bar and its items, the sidebar filter rows and sections, the facet toggle logic, the link-weight button and the transparency hook. The log viewer was moved onto it, which is most of the churn here. Opaque surfaces now come straight from the main window: `surface-neutral` behind the content and `surface-light` for the content itself, so all three windows read as the same app. The card carries no hairline — the shadow alone lifts it — and the toolbar, sidebar, card gutter and status bar paint nothing, leaving one continuous panel. Downloads window: - Every existing entry point already dispatches the same action, so the main process listens for it once rather than editing each call site; the root window keeps whatever view it was on. - Rows are list rows, not cards: icon, name, and one muted line of server, size and — while a transfer is live — its rate and time left. A finished download's name opens the file, and on macOS there is Quick Look beside it; both resolve the path from main's own state by id, so a renderer cannot ask for an arbitrary file to be opened. - Grouped under sticky day headings, filtered by faceted server/type/status lists with counts, and reopened at launch when it was open at shutdown. Both windows drop their sidebar-hide toggle and their shared "clear filters" footer; each facet section resets itself instead, and the destructive action sits in the status bar beside the count it affects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rodrigok
changed the title
Linux installer download |
macOS installer download |
The old FileIcon drew a fixed white page, which read as a bright block on the dark window. This one is inline SVG, so its fill, outline, fold and label all come from palette neutrals at low alpha — one drawing that works on both themes. Deliberately monochrome. The icon identifies a row; the file name is what the reader is looking for, and colour-coding by type competed with it. Row hover moves to a shared LIST_ROW_CLASS, so the downloads list and the window chrome's filter rows highlight from the same rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Settings becomes the third window on the shared chrome, alongside the log
viewer and downloads: same toolbar, sidebar, nav rows and surface handling,
so transparency and the card treatment follow the other two for free.
Sections are a registry rather than one long page. Appearance is new and
takes the theme and layout settings that were scattered across General;
telephony and video calls split apart, since they were only ever grouped by
both being "calls". The theme and layout options are now picked from
thumbnails, drawn with literal colours rather than palette tokens — these
are the one place in the app that must not follow the current theme, or all
three options would render identically.
Search matches settings, not just section names, and names the matching
settings in the row so it is clear why a section is still listed. Fuzzy
subsequence matching is applied only to short labels: over prose it matched
almost anything ("vibr" hit "Video calls"), so longer text falls back to
substring.
Certificates merge trusted and untrusted into one list with the state shown
per row and a filter field, instead of two lists that had to be compared.
All three windows now remember where they were left. Bounds are saved from
getNormalBounds() so maximising does not overwrite the size to restore to,
debounced because move and resize fire continuously while dragging, and
dropped when they no longer overlap any display — a window restored onto an
unplugged monitor is a window the reader cannot reach.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rodrigok
changed the title
…tions The secondary windows kept their native Windows caption while also drawing an in-app toolbar, so each one showed two headers stacked. They now hide the title bar as the main window already does there, and the toolbar draws the caption buttons into its trailing edge. The glyphs and button styling come from the main window's own controls, so this is not a second set that drifts from it; only the wiring differs. The main window's buttons dispatch redux actions bound to that one window, while these ask the main process to act on whichever window sent the request — one registration serving all three. Maximised state is pushed back per window, so the glyph shows restore even when the change came from a double click on the toolbar rather than from the buttons. The toolbar reserves the buttons' width at its leading edge too, so the title stays centred in the window rather than in what is left of it. Settings opens wider. A full row of theme thumbnails did not fit the old 680px minimum and wrapped to a second row, which reads as a layout accident rather than a choice. The new minimum is derived from the option's real width — 178px, not the thumbnail's 168px, because the selection ring is drawn whether or not an option is selected — through a metrics module the ring, the grid gap and the thumbnail all share, so the three cannot drift. Measured against the built window: 876px wraps, 878px does not, and the minimum leaves the Windows scrollbar its 10px, which the macOS overlay scrollbar does not take. The downloads sidebar shows a placeholder while there is nothing to filter, instead of a search field over an empty column that reads as a rendering failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The About dialog is gone. On macOS the menu item now opens the system About panel — every Mac app has that item in the same place, and its contents come from the bundle, so a hand-built dialog was a worse version of something the OS already provides. Windows and Linux have no such convention, so they get no About item at all. What the dialog actually held moves into settings. Update channel and the logging switches that had their own Developer section now sit in a new Advanced section together with hardware acceleration and error reports — the things a reader reaches for when something is wrong. The section list no longer needs a developer-only flag: Advanced hides its own developer parts. Checking for updates leads General as a single row: the automatic check and the manual one are the same decision from two angles, so the toggle and the button share a field rather than sitting apart. Version and copyright move to the foot of the sidebar, small and unlabelled — worth being able to find and copy, not worth a row of their own. Spacing is one rhythm now, 24px between settings and a hairline where a group genuinely ends. Three fields had been setting their own block margins, which is why the PDF size limit sat closer to its neighbour than anything else did, and four more hand-rolled the Field markup instead of using the shared wrappers. The telephony shortcut was the worst case: it dropped the className FieldGroup passes down, and with it the group's spacing entirely, so its rows had no gap at all. Measured against the built window: every gap within a group is now 24px, every gap across a divider 49px. Sidebar rows all carry the same text and icon colour — the fill behind the selected one already says which is selected, and dimming the rest made the list read as mostly disabled. Keyboard focus draws a highlight ring instead of borrowing that fill, which had left two rows claiming to be current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four conflicts, all of them both sides adding to the same place: the new downloads simulation listener next to this branch's open-file and preview handlers, the downloads-percentage reducer next to the window-open ones, its action next to the secondary-window actions, and master's DownloadsIndicator mock where this branch had removed the About dialog's. Both sides kept in each case. One thing needed wiring rather than merging: master added Downloads percentage to GeneralTab, which this branch no longer renders, so the setting moves to the settings window's General section and its search index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The indicator reads Date.now() twice — once for the mount-time seenAt, once when the button is clicked — and a download counts as unseen only while its endTime sits between the two. The test set endTime to mountTime + 1, so that window was a fraction of a millisecond wide: it passed when mount and render landed in the same tick and failed when they did not. On the Linux and Windows runners they did not, and this is the test failing on master. Date.now() is stubbed instead, mount and click ten seconds apart, so the download is unambiguously unseen at mount and seen after the click. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The viewer was an overlay inside the server pane, which meant reading a document and reading the conversation it came from were mutually exclusive. It becomes the fourth window on the shared chrome instead, so the workspace stays visible behind it and the window can be sized and placed on its own. Every entry point already dispatched SERVER_DOCUMENT_VIEWER_OPEN_URL — the page asking to open a PDF, the main process intercepting a markdown download — so the window listens for that one action and both paths redirect at once; no caller changed. The document still renders in a webview on the originating server's session, which is what lets an authenticated URL resolve at all. One window, reused: a second document replaces the first rather than piling up near-identical windows. Fixes a viewer bug found on the way. PdfContent announced its webview to the main process on `did-attach`, but getWebContentsId() throws until the guest document exists, so the call threw every time and the announcement never arrived — leaving the main process unable to intercept link clicks inside a PDF and route them to the browser. It now announces on `dom-ready`, guarded so navigation inside the viewer does not register the handler twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rodrigok
changed the title
Both viewers get a download button. The bytes are read on the workspace's own session, so an authenticated document saves as the signed-in user rather than as an anonymous request; a blob the server page created is read back through that page's web contents, since a blob URL resolves nowhere else. Markdown gets a source toggle. The text is already fetched to render it, so switching between rendered and source costs no round trip. Also drops a `bg='surface'` from the markdown viewer. There is no such palette token — Fuselage logged "invalid color: surface" on every render and painted nothing — and the window's card already carries the background. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One conflict, in the downloads indicator's seen test: master pinned Date.now() for that test independently, the same fix this branch had made. Master's version is taken verbatim so the file stops diverging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The secondary windows' toolbars were 52px while the main window's tab strip is 40px, so the windows did not line up as the same app. They read the height from one constant now rather than each carrying its own number, and the macOS traffic lights stay centred in it because their position is derived from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One conflict, in the workspace-switcher setting: master dropped the Linux menu-bar coupling that disabled the tabs option, while this branch had replaced the radio buttons with thumbnails. The thumbnails stay and the coupling goes, so the option is offered on every platform as master intends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clicking the visible checkbox did nothing: Fuselage draws it as a label wrapping a real input, so the click reached the row twice — once on its way up from the box, once from the click the label forwards to the input — and the filter toggled straight back to where it started. Clicking the row's label worked, which is why it read as an unreliable checkbox rather than a broken one. The checkbox was meant to be inert, via pointer-events, but that never reached the element Fuselage puts the handler on. It reports its own change now and keeps its clicks to itself; the row handles everywhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unchecking the last option in a facet silently ticked every box again, so the list could never be narrowed to nothing from the sidebar — the one thing a reader tries when they want to see what a filter is actually doing. The cause was one value meaning two things: an empty selection stood for "untouched, so everything", which the facets need in order to keep taking in servers and file types that only show up later. Untouched is `null` now and an empty list means what it says, so all three states are expressible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A window created on macOS while the app is in full screen was given full screen itself, so opening Downloads or Settings from a full-screen workspace replaced it rather than appearing alongside it. `fullscreenable: false` is what refuses that, and it refuses it outright: even an explicit setFullScreen(true) leaves the window windowed. Maximise, minimise and resize are untouched. Several richer approaches were tried first and are not here for good reason. Toggling the workspace collection behaviour around show() left the main window in a full-screen state it could not be brought out of, and making the secondary windows children of the main window turned it black — both worse than the problem. This changes one constructor option and can do neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The panel had its own item component, so a download looked one way in the downloads window and another in the panel a click away. It renders the window's row now and that component is gone. Making the row portable took removing its one tie to the downloads window: a `surfaces` prop used for a single divider colour that is the same palette token in every theme. The server name is a prop instead — worth a line in the window, which filters by it, and only crowding in a panel listing a handful of downloads from the session at hand. Progress no longer changes a row's height. It was an 8px bar in the flow, so every row below jumped 12px the moment a transfer started; it is a line along the row's own bottom edge now. Drawn directly rather than with Fuselage's ProgressBar, whose animated shine is an absolutely positioned pseudo-element with no containing block of its own — it escaped the bar and swept a white band across the whole list. Every row's last action is a cross now, so the button nearest the edge does not move as a download progresses. They remain different actions: cancel while a transfer is live, remove from the list once it is over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test opened the panel with userEvent and then searched the whole tree with a regex. Both got slower when the panel started rendering the full download row, and on the Windows runner the pair crossed the 5s limit. It clicks with fireEvent and asserts against the panel's text instead, which tests the same thing: a transfer that has not moved any bytes shows no size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/components/ServersView/PdfContent.tsx (1)
45-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the
did-finish-loadlistener in cleanup and avoid duplicate registrations.
handleDomReadyadds adid-finish-loadlistener each time the guest web contents ID changes. The cleanup at Line 77 removes only thedom-readylistener. If the guest is replaced, the previousdid-finish-loadlistener stays attached, so the click-interception script is injected more than once per load and the guest accumulates duplicate click handlers.Register
did-finish-loadonce, outsidehandleDomReady, and remove it in the cleanup.🔧 Proposed fix
let announcedWebContentsId: number | null = null; + const handleDidFinishLoad = () => { + webviewElement.executeJavaScript(` + document.addEventListener('click', (event) => { + const anchor = event.target instanceof Element ? event.target.closest('a') : null; + if (anchor && anchor.href) { + try { + const url = new URL(anchor.href, document.baseURI); + if (url.pathname.toLowerCase().endsWith('.pdf')) { + event.preventDefault(); + } + } catch {} + } + }, true); + `); + }; + const handleDomReady: () => void = () => { const webContentsId = webviewElement.getWebContentsId(); if (announcedWebContentsId === webContentsId) return; announcedWebContentsId = webContentsId; dispatch({ type: WEBVIEW_PDF_VIEWER_ATTACHED, payload: { WebContentsId: webContentsId }, }); - - webviewElement.addEventListener('did-finish-load', () => { - ... - }); }; webviewElement.addEventListener('dom-ready', handleDomReady); + webviewElement.addEventListener('did-finish-load', handleDidFinishLoad); return () => { webviewElement.removeEventListener('dom-ready', handleDomReady); + webviewElement.removeEventListener( + 'did-finish-load', + handleDidFinishLoad + ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/ServersView/PdfContent.tsx` around lines 45 - 79, Move the did-finish-load listener registration out of handleDomReady so it is attached only once to webviewElement, while preserving the existing click-interception injection. Update the cleanup returned by the effect to remove both the dom-ready handler and the did-finish-load handler.src/ui/components/TopBar/DownloadsIndicator.tsx (1)
478-519: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore vertical padding at the top and bottom of the panel.
The dialog Box at line 478 sets no padding. The header Box now sets
paddingInlineonly. The footer Box setspaddingBlockStartonly. As a result the title row touches the top edge of the dropdown and the "show all" button touches the bottom edge.🎨 Proposed fix
<Box display='flex' alignItems='center' justifyContent='space-between' paddingInline='x12' + paddingBlock='x12' ><Box display='flex' justifyContent='center' paddingBlockStart='x12' + paddingBlockEnd='x12' paddingInline='x12' >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/TopBar/DownloadsIndicator.tsx` around lines 478 - 519, Restore vertical padding on the dialog panel containing the downloads header, rows, and footer, ensuring spacing appears at both its top and bottom edges. Update the outer dialog Box and preserve the existing horizontal header and footer padding behavior around recentDownloads and handleShowAll.
🟡 Minor comments (14)
src/ui/windowChrome/filters.ts-17-31 (1)
17-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck selected options against
universebefore resetting the facet.A stale selected value can make
next.lengthreachuniverse.lengthbefore every current option is selected. The function then returnsnulland incorrectly enables all options. Use membership of everyuniversevalue instead of comparing lengths.Proposed fix
- if (next.length >= universe.length) { + if (universe.every((value) => next.includes(value))) { return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/windowChrome/filters.ts` around lines 17 - 31, Update toggleFacet so it resets to null only when every value in universe is present in next, rather than comparing next.length with universe.length. Preserve the existing toggle behavior and return next for partial selections, including selections containing stale values.src/ui/windowChrome/NavRow.tsx-47-65 (1)
47-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse button semantics or complete the tab pattern.
The sidebar has a
tablist, butSettingsWindowrenders no mappedtabpanel.NavRowalso lacksaria-controls, rovingtabIndex, and arrow-key navigation. Remove the tab roles and use buttons, or implement the complete tab contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/windowChrome/NavRow.tsx` around lines 47 - 65, Update NavRow to use native button semantics instead of the incomplete tab pattern: remove role="tab", aria-selected, and tab-specific tabIndex usage, while preserving selection styling and click/keyboard activation through the button. Ensure the surrounding tablist/tab-panel contract is not relied upon unless SettingsWindow and NavRow are updated to implement it completely.src/documentViewerWindow/ipc.ts-117-125 (1)
117-125: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
loadFilefailure.
loadFilereturns a promise. If the page fails to load, the rejection is unhandled and the window stays hidden, becauseready-to-shownever fires. Attach acatchand close or report the window in that case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/documentViewerWindow/ipc.ts` around lines 117 - 125, Handle the promise returned by documentViewerWindow.loadFile by attaching rejection handling. On failure, report the loading error and close or otherwise clean up documentViewerWindow so it cannot remain hidden when ready-to-show never fires.src/i18n/en.i18n.json-206-218 (1)
206-218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate MIME labels with inconsistent wording.
The
mimesblock defines two key sets for the same concepts:images/image,videos/video,audios/audio,texts/text, andfiles/application. The wording differs between them.textresolves to "Documents" whiletextsresolves to "Texts".audioresolves to "Audio" whileaudiosresolves to "Audios".Keep only the key set that
src/downloadsWindowreads, and remove the other set. Duplicate keys must also be kept in sync in every other locale file.#!/bin/bash # Determine which MIME label key set the downloads window uses. rg -n -C3 "filters\.mimes|mimes\." src --type=ts --type=tsx🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/en.i18n.json` around lines 206 - 218, The mimes translations duplicate singular and plural key sets with inconsistent labels. Check the key set consumed by the downloads window and retain only that set in the English locale and every other locale, removing the unused counterparts while preserving the selected labels consistently across locales.src/documentViewerWindow/DocumentViewerWindow.tsx-60-67 (1)
60-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSurface the save result and catch the rejection.
saveDocumentreturns{ success, canceled, error }, and the main handler returns{ success: false, error: 'The document viewer is not open' }. This call discards the result, so a failed download gives the user no feedback. The dropped promise also produces an unhandled rejection if the IPC call itself fails.Await the result and show the error, for example through a toast or an inline message in the toolbar.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/documentViewerWindow/DocumentViewerWindow.tsx` around lines 60 - 67, Update handleDownload to await the document-viewer/save-document IPC result, inspect its success and error fields, and surface failures to the user through the existing toast or toolbar message mechanism. Also catch rejected invoke promises so IPC failures are handled without unhandled rejections, while preserving the current download arguments.src/downloadsWindow/ipc.ts-46-50 (1)
46-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the window before focusing it.
If the reader minimizes the downloads window and then clicks the downloads entry point again,
focus()alone does not restore a minimized window on Windows. The window stays minimized and the click appears to do nothing.🐛 Proposed fix
if (downloadsWindow && !downloadsWindow.isDestroyed()) { + if (downloadsWindow.isMinimized()) { + downloadsWindow.restore(); + } + downloadsWindow.show(); downloadsWindow.focus(); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downloadsWindow/ipc.ts` around lines 46 - 50, Update createDownloadsWindow so an existing, non-destroyed downloadsWindow is restored from its minimized state before calling focus(). Preserve the current early return and avoid changing behavior for newly created windows.src/downloadsWindow/DownloadsWindow.tsx-393-394 (1)
393-394: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake each day-run key unique.
A day can occur in more than one group.
groupDownloadsByDaypreserves these separate runs. Line 394 then gives each repeated day the same React key. React can reconcile these sections incorrectly when downloads change.Proposed fix
- {dayGroups.map((group) => ( - <Box key={group.day || 'unknown'}> + {dayGroups.map((group, groupIndex) => ( + <Box key={`${group.day || 'unknown'}-${groupIndex}`}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downloadsWindow/DownloadsWindow.tsx` around lines 393 - 394, Update the dayGroups.map rendering in DownloadsWindow so each group uses a unique React key even when multiple groups share the same group.day value. Incorporate the map index or another run-specific identifier while preserving the existing “unknown” fallback.src/ui/components/SettingsView/features/CheckForUpdates.tsx-87-103 (1)
87-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep an update error visible after a manual check.
When
updateErroris set andnewUpdateVersionis empty, Line 90 sets the no-update message. This replaces the error set by the preceding effect. ClearhasRequestedCheckwithout settingnoUpdatesAvailablewhenupdateErrorexists. AddupdateErrorto this effect dependency list. Add a regression test for a failed manual check.Proposed fix
useEffect(() => { if (!hasRequestedCheck || isCheckingForUpdates) return undefined; + if (updateError) { + setHasRequestedCheck(false); + return undefined; + } + if (!newUpdateVersion) { setHasRequestedCheck(false); setResultMessage(t('dialog.about.noUpdatesAvailable')); @@ - }, [dispatch, hasRequestedCheck, isCheckingForUpdates, newUpdateVersion, t]); + }, [ + dispatch, + hasRequestedCheck, + isCheckingForUpdates, + newUpdateVersion, + t, + updateError, + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/SettingsView/features/CheckForUpdates.tsx` around lines 87 - 103, Update the manual-check effect around hasRequestedCheck so it clears the request without setting noUpdatesAvailable when updateError is present, preserving the existing no-update message path otherwise. Add updateError to the effect dependencies, and add a regression test covering a failed manual check with no newUpdateVersion to ensure the error remains visible.src/settingsWindow/sections.ts-49-116 (1)
49-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter unavailable settings before matching search keys.
SettingsWindow.tsxsearches everysettingKeysentry without checking whether its control is rendered. Platform-specific and developer-only keys can match on unsupported environments and display a section with no matching control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/settingsWindow/sections.ts` around lines 49 - 116, The SettingsWindow search flow should filter each section’s settingKeys to only settings currently available and rendered before matching search terms. Update the logic around the settingKeys search and reuse the existing availability/platform and developer-mode checks so unsupported or hidden controls cannot cause an otherwise empty section to appear.src/ui/components/SettingsView/features/ChromeThumbnailOption.tsx-47-59 (1)
47-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDerive the frame radius from the thumbnail platform instead of hardcoding 11.
Line 97 passes
radius={11}.getRadiiinsrc/ui/components/SettingsView/features/WindowChromeThumbnail.tsx(Line 51-52) returnswindow: 11only fordarwinandwindow: 6forwin32andlinux. On Windows and Linux the thumbnail draws a 6px corner, butFramedraws a 13px ring around it. The ring corner will not follow the thumbnail corner.The same value is now written in two files. Export the radius helper and reuse it.
🎨 Proposed fix to share the platform radius
In
src/ui/components/SettingsView/features/WindowChromeThumbnail.tsx:-const getRadii = (platform: ThumbnailPlatform) => +export const getRadii = (platform: ThumbnailPlatform) => platform === 'darwin' ? { window: 11, card: 8 } : { window: 6, card: 5 }; + +export const resolveThumbnailPlatform = ( + platform?: ThumbnailPlatform +): ThumbnailPlatform => + platform ?? + (process.platform === 'darwin' || process.platform === 'win32' + ? process.platform + : 'linux');In this file:
-import { WindowChromeThumbnail } from './WindowChromeThumbnail'; +import { + WindowChromeThumbnail, + getRadii, + resolveThumbnailPlatform, +} from './WindowChromeThumbnail';- <Frame isSelected={checked} radius={11}> + <Frame + isSelected={checked} + radius={getRadii(resolveThumbnailPlatform(platform)).window} + >Also applies to: 97-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/SettingsView/features/ChromeThumbnailOption.tsx` around lines 47 - 59, Export the existing getRadii helper from WindowChromeThumbnail and import/reuse it in ChromeThumbnailOption when passing the Frame radius, replacing the hardcoded radius={11}; ensure the frame uses the platform-specific window radius (11 on darwin, 6 on win32/linux) so its corner matches the thumbnail.src/settingsWindow/ipc.ts-85-93 (1)
85-93: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClamp partially visible restored bounds before creating the window.
getSavedWindowBoundsrejects bounds with no overlap with current displays, but it does not require full containment or clamp to the work area. After a resolution change, the settings window can still open partly offscreen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/settingsWindow/ipc.ts` around lines 85 - 93, Update the saved-bounds handling around getSavedWindowBounds before constructing settingsWindow so restored settings bounds are clamped fully within the relevant display work area. Preserve the existing centered fallback when no valid saved bounds exist, and ensure the BrowserWindow initialization receives only onscreen bounds.src/logViewerWindow/logViewerWindow.tsx-875-886 (1)
875-886: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the side effects out of the
setAutoScrollupdater.The updater passed to
setAutoScrollcallssetUserHasScrolledand schedules a timeout. React requires state updaters to be pure. React StrictMode double-invokes updaters in development, so the smooth scroll is scheduled twice.autoScrollis already available in scope, so compute the next value directly.♻️ Proposed fix
const handleToggleAutoScroll = useCallback(() => { - setAutoScroll((previous) => { - const next = !previous; - if (next) { - setUserHasScrolled(false); - setTimeout(() => { - virtuosoRef.current?.scrollToIndex({ index: 0, behavior: 'smooth' }); - }, SCROLL_DELAY_MS); - } - return next; - }); - }, [setAutoScroll]); + const next = !autoScroll; + setAutoScroll(next); + if (!next) return; + setUserHasScrolled(false); + setTimeout(() => { + virtuosoRef.current?.scrollToIndex({ index: 0, behavior: 'smooth' }); + }, SCROLL_DELAY_MS); + }, [autoScroll, setAutoScroll]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/logViewerWindow.tsx` around lines 875 - 886, Update handleToggleAutoScroll to compute the next auto-scroll value from the current autoScroll state before calling setAutoScroll, then perform setUserHasScrolled and the delayed virtuosoRef scroll outside the state updater only when enabling auto-scroll. Keep the updater pure and preserve the existing delay and smooth-scroll behavior.src/logViewerWindow/logViewerWindow.tsx-89-98 (1)
89-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParse fallback
YYYY-MM-DDvalues as local dates.
getEntryDaynormally returnstoDateString(), but its invalid-timestamp fallback returnsYYYY-MM-DD.formatDayLabelthen parses that value as UTC, which can display the previous day in negative-offset timezones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/logViewerWindow.tsx` around lines 89 - 98, Update formatDayLabel to detect YYYY-MM-DD fallback strings and construct the date using local year, month, and day components before formatting; preserve the existing parsing behavior for other date strings and return the original value when parsing remains invalid.src/logViewerWindow/LogTimeline.tsx-148-168 (1)
148-168: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the timeline range selectable from the keyboard
The plot selects a two-ended range but exposes one
sliderand handles only mouse events. Implement a valid keyboard-operable range widget with slider values and keyboard behavior, or remove the slider semantics and add a separate keyboard-operable range control. A labelledgroupalone does not provide keyboard access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/LogTimeline.tsx` around lines 148 - 168, Update the timeline accessibility implementation around the Box with ref={plotRef} so keyboard users can select and adjust both range endpoints, either by exposing valid two-thumb slider controls with appropriate values and key handling or by adding a separate keyboard-operable range control. Do not retain a single slider role that only responds to mouse events; ensure each endpoint has an accessible label and keyboard behavior while preserving the existing mouse selection.
🧹 Nitpick comments (16)
src/ui/reducers/isDownloadsWindowOpen.ts (1)
23-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftValidate action payloads at the IPC boundary before removing reducer guards.
The action types are compile-time only, and IPC checks only the FSA
type. A malformed cross-process action can reach these reducers. Keep the guards and test, or add boundary validation first.secondaryWindowStatescan throw whenpayloadis missing because it destructures before checking it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/reducers/isDownloadsWindowOpen.ts` around lines 23 - 31, Retain runtime payload validation for the window-state reducers: in src/ui/reducers/isDownloadsWindowOpen.ts (23-31), src/ui/reducers/isLogViewerWindowOpen.ts (23-31), and src/ui/reducers/isSettingsWindowOpen.ts (23-31), keep the boolean guards and invalid-payload fallback; in src/ui/reducers/secondaryWindowStates.ts (27-30), validate the action payload before destructuring so missing payloads cannot throw; in src/ui/reducers/isLogViewerWindowOpen.spec.ts (28-42), preserve or add coverage for malformed payloads reaching the reducer. If validation is instead moved to the IPC boundary, implement it before removing any reducer guards and cover all listed action paths.Source: Learnings
src/ui/windowChrome/WindowToolbar.tsx (1)
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRename these component files to camelCase.
The new component names are correctly PascalCase. The file names must be camelCase.
src/ui/windowChrome/WindowToolbar.tsx#L1-L13: Rename towindowToolbar.tsxand update imports.src/ui/windowChrome/DayHeader.tsx#L1-L4: Rename todayHeader.tsxand update imports.src/ui/windowChrome/FilterRow.tsx#L1-L5: Rename tofilterRow.tsxand update imports.src/ui/windowChrome/FilterSection.tsx#L1-L4: Rename tofilterSection.tsxand update imports.src/ui/windowChrome/NavRow.tsx#L1-L7: Rename tonavRow.tsxand update imports.src/ui/windowChrome/StatusBar.tsx#L1-L4: Rename tostatusBar.tsxand update imports.As per coding guidelines, “Use camelCase for file names and PascalCase for component names.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/windowChrome/WindowToolbar.tsx` around lines 1 - 13, Rename the component files and update every import reference: src/ui/windowChrome/WindowToolbar.tsx to windowToolbar.tsx, DayHeader.tsx to dayHeader.tsx, FilterRow.tsx to filterRow.tsx, FilterSection.tsx to filterSection.tsx, NavRow.tsx to navRow.tsx, and StatusBar.tsx to statusBar.tsx. Keep the component names PascalCase, including WindowToolbar, DayHeader, FilterRow, FilterSection, NavRow, and StatusBar.Source: Coding guidelines
src/documentViewerWindow/DocumentViewerWindow.tsx (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
documentstate variable.The state variable
documentshadows the globaldocumentobject for the whole component body. Any later DOM call inside this component would silently resolve to the descriptor. UsedocumentDescriptororcurrentDocument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/documentViewerWindow/DocumentViewerWindow.tsx` around lines 38 - 39, Rename the state variable in the DocumentViewerWindow component from document to documentDescriptor (or currentDocument) and update all corresponding references, while keeping setDocument or renaming it consistently. Ensure the global document object remains accessible within the component body.src/documentViewerWindow/saveDocument.ts (1)
94-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the document fetch.
session.fetchhas no timeout. If the workspace does not respond, the save dialog result never resolves and the renderer waits without feedback. Pass anAbortSignalwith a timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/documentViewerWindow/saveDocument.ts` around lines 94 - 102, Update the document fetch in the session.fromPartition(partition).fetch flow to pass an AbortSignal with a timeout, ensuring an unresponsive workspace aborts and the save operation does not wait indefinitely while preserving the existing response validation and buffer conversion.src/i18n/en.i18n.json (1)
183-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
documentViewer.backand the unused in-paneDocumentViewercomponent. The key has no live references outside that component.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/en.i18n.json` around lines 183 - 186, Remove the unused documentViewer.back translation key and delete the in-pane DocumentViewer component that is its only consumer. Preserve any separate, actively used document viewer implementation and unrelated translation entries.src/ui/components/TopBar/DownloadsIndicator.tsx (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
DownloadRowto a shared location.
DownloadsIndicatoris a main-window component. It now imports fromsrc/downloadsWindow, which is a separate renderer bundle. The dependency points from the main window into a secondary-window module.
DownloadRowalready draws its shared pieces fromsrc/ui/windowChrome(LIST_ROW_CLASS,useCopiedFeedback,isDarwin). Placing the row there too would keep the dependency direction consistent and would avoid pullingdownloadsWindowmodules into the main renderer bundle.This is a placement change only. Defer it if the bundles already share these modules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/TopBar/DownloadsIndicator.tsx` at line 8, Move the DownloadRow component from the downloadsWindow module into the shared src/ui/windowChrome location, then update DownloadsIndicator and any other consumers to import it from that shared location while preserving its existing behavior.src/downloadsWindow/DownloadRow.tsx (1)
112-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
runover the download action channels instead of casting.
Parameters<typeof invoke>[0]allows every valid IPC channel, although unknown channel names still fail. The nine download channels all acceptDownload['itemId']; use aDownloadActionChannelunion and callinvoke(channel, itemId)without casts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downloadsWindow/DownloadRow.tsx` around lines 112 - 117, Update the run callback in DownloadRow to accept the DownloadActionChannel union rather than Parameters<typeof invoke>[0]. Remove both casts and call invoke(channel, itemId) directly, preserving the existing itemId dependency and behavior.src/downloadsWindow/downloads-window.tsx (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the entry file to camelCase.
Rename
src/downloadsWindow/downloads-window.tsxtosrc/downloadsWindow/downloadsWindow.tsx. Updaterollup.config.mjsand the script insrc/public/downloads-window.htmlto usedownloadsWindow.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downloadsWindow/downloads-window.tsx` around lines 1 - 11, Rename the entry file from downloads-window.tsx to downloadsWindow.tsx, then update the corresponding Rollup entry configuration and the script reference in downloads-window.html to load downloadsWindow.js.Source: Coding guidelines
src/settingsWindow/SettingsSidebar.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse camelCase file names.
Rename these files and update their imports and entry-point references.
src/settingsWindow/SettingsSidebar.tsx#L1-L1: rename tosettingsSidebar.tsx.src/settingsWindow/SettingsWindow.tsx#L1-L1: rename tosettingsWindow.tsx.src/settingsWindow/sections/CertificateRow.tsx#L1-L1: rename tocertificateRow.tsx.src/settingsWindow/sections/CertificatesSection.tsx#L1-L1: rename tocertificatesSection.tsx.src/settingsWindow/settings-window.tsx#L1-L1: rename to a camelCase entry-point name, such assettingsWindowEntry.tsx.As per coding guidelines, “Use camelCase for file names and PascalCase for component names.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/settingsWindow/SettingsSidebar.tsx` at line 1, Rename src/settingsWindow/SettingsSidebar.tsx#L1-L1 to settingsSidebar.tsx, src/settingsWindow/SettingsWindow.tsx#L1-L1 to settingsWindow.tsx, src/settingsWindow/sections/CertificateRow.tsx#L1-L1 to certificateRow.tsx, src/settingsWindow/sections/CertificatesSection.tsx#L1-L1 to certificatesSection.tsx, and src/settingsWindow/settings-window.tsx#L1-L1 to a camelCase entry point such as settingsWindowEntry.tsx; update all imports and entry-point references while preserving the PascalCase component names.Source: Coding guidelines
src/settingsWindow/sections/AdvancedSection.tsx (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRename new TSX files to camelCase.
These files use PascalCase names. This conflicts with the repository file-name rule. Rename each file and update its imports.
src/settingsWindow/sections/AdvancedSection.tsx#L19-L19: rename the file toadvancedSection.tsx.src/settingsWindow/sections/GeneralSection.tsx#L19-L19: rename the file togeneralSection.tsx.src/settingsWindow/sections/TelephonySection.tsx#L7-L7: rename the file totelephonySection.tsx.src/settingsWindow/sections/VideoCallsSection.tsx#L8-L8: rename the file tovideoCallsSection.tsx.src/ui/components/SettingsView/features/SettingGroupDivider.tsx#L11-L11: rename the file tosettingGroupDivider.tsx.src/ui/components/SettingsView/features/DebugLogging.tsx#L16-L16: rename the file todebugLogging.tsx.src/ui/components/SettingsView/features/DetailedEventsLogging.tsx#L16-L16: rename the file todetailedEventsLogging.tsx.src/ui/components/SettingsView/features/VerboseOutlookLogging.tsx#L16-L16: rename the file toverboseOutlookLogging.tsx.As per coding guidelines, “Use camelCase for file names and PascalCase for component names.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/settingsWindow/sections/AdvancedSection.tsx` at line 19, Rename AdvancedSection.tsx to advancedSection.tsx, GeneralSection.tsx to generalSection.tsx, TelephonySection.tsx to telephonySection.tsx, and VideoCallsSection.tsx to videoCallsSection.tsx; rename SettingGroupDivider.tsx to settingGroupDivider.tsx, DebugLogging.tsx to debugLogging.tsx, DetailedEventsLogging.tsx to detailedEventsLogging.tsx, and VerboseOutlookLogging.tsx to verboseOutlookLogging.tsx. Update every import path referencing these files while preserving the PascalCase component names.Source: Coding guidelines
src/ui/components/SettingsView/features/SettingGroupDivider.tsx (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the typed background color prop.
Fuselage 0.80.0 defines
stroke-extra-lightinTheme.d.tsand resolves it throughBox.backgroundColor. Replace the inline style withbackgroundColor='stroke-extra-light'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/SettingsView/features/SettingGroupDivider.tsx` around lines 14 - 17, Update the divider styling in SettingGroupDivider to remove the inline backgroundColor style and pass the typed `backgroundColor='stroke-extra-light'` prop to the Box component, while preserving the existing one-pixel height.Source: Coding guidelines
src/ui/components/SettingsView/features/WindowChromeThumbnail.tsx (1)
125-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the Windows control positions from
WIDTH.Lines 133 to 135 hardcode x coordinates 130, 144, 157 and 163. These are trailing-edge positions that only hold while
WIDTHequals 168.WIDTHisTHUMBNAIL_WIDTHfromsrc/ui/components/SettingsView/features/thumbnailMetrics.ts, which is shared withsrc/settingsWindow/constants.tsto size the settings window. IfTHUMBNAIL_WIDTHchanges, the macOS traffic lights stay anchored to the leading edge but the Windows controls drift away from the trailing edge.Express the offsets relative to
WIDTH.♻️ Proposed refactor to anchor the controls to the trailing edge
+ // Anchored to the trailing edge so the strip survives a change to WIDTH. + const close = WIDTH - 11; + const maximise = close - 13; + const minimise = maximise - 14; + return ( <g key='controls' stroke={palette.muted} strokeWidth={1.1} fill='none' strokeLinecap='round' > - <path d={`M130 ${cy}h7`} /> - <rect x={144} y={cy - 3} width={6} height={6} rx={1} /> - <path d={`M157 ${cy - 3}l6 6M163 ${cy - 3}l-6 6`} /> + <path d={`M${minimise} ${cy}h7`} /> + <rect x={maximise} y={cy - 3} width={6} height={6} rx={1} /> + <path d={`M${close - 6} ${cy - 3}l6 6M${close} ${cy - 3}l-6 6`} /> </g> );Note:
src/ui/components/SettingsView/features/WindowChromeThumbnail.spec.tsxLine 45 and Line 54 assertpath[d^="M130"]. Update those selectors together with this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/SettingsView/features/WindowChromeThumbnail.tsx` around lines 125 - 137, Update the Windows controls group in WindowChromeThumbnail to derive the x coordinates in the path and rect elements from WIDTH, preserving their current trailing-edge spacing when WIDTH is 168. Replace the hardcoded 130, 144, 157, and 163 positions with WIDTH-relative offsets, and update the WindowChromeThumbnail.spec.tsx selectors that assert the M130 path prefix.src/logViewerWindow/LogEntry.tsx (1)
96-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse an own-property check when matching a tag against
serverMapping.
tag in serverMappingalso matches inheritedObject.prototypekeys. A context tag namedconstructor,toString, orvalueOfresolves to a function, andserverNamethen holds a function. React throws when a function is rendered as a child.Use
Object.prototype.hasOwnProperty.callto restrict the lookup to own keys.🛡️ Proposed guard
- const serverTag = entry.contextTags.find((tag) => tag in serverMapping); + const serverTag = entry.contextTags.find((tag) => + Object.prototype.hasOwnProperty.call(serverMapping, tag) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/LogEntry.tsx` around lines 96 - 105, Update the serverTag lookup inside the useMemo callback to use Object.prototype.hasOwnProperty.call(serverMapping, tag) instead of the inherited-key-inclusive in operator, ensuring only own serverMapping keys are matched and serverName remains renderable text.src/logViewerWindow/__tests__/parseLogs.spec.ts (1)
26-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a tag that contains a space.
The doc comment in
src/logViewerWindow/parseLogs.tsstates thatcontextTagsexists because splitting the joinedcontexton whitespace would break a tag that contains a space. No test covers that case, so a regression to whitespace splitting would pass.Add an assertion with a multi-word tag, for example
[My Workspace].💚 Proposed test
it('keeps every bracketed tag as its own context tag', () => {Add after that test:
it('keeps a tag that contains a space intact', () => { const [entry] = parseLogLines( '[2026-08-07 18:28:38.804] [info] [main] [My Workspace] connected' ); expect(entry.contextTags).toEqual(['main', 'My Workspace']); expect(entry.message).toBe('connected'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/__tests__/parseLogs.spec.ts` around lines 26 - 37, Add a test alongside the existing parseLogLines context-tag test that parses a multi-word tag such as “My Workspace” and asserts contextTags preserves it as one entry while message remains correct. Use the existing parseLogLines test pattern and symbols.src/logViewerWindow/LogViewerSidebar.tsx (1)
116-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the level universe in one place.
This expression repeats the fallback in
handleToggleLevelinsrc/logViewerWindow/logViewerWindow.tsxat Line 543.toggleFacetcollapses a selection tonullwhennext.length >= universe.length, so the rendered universe and the toggle universe must stay identical. If one side changes, "select all" behaviour desynchronizes from the checkbox list.Compute the resolved level list once in the parent and pass it as a single prop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/LogViewerSidebar.tsx` around lines 116 - 127, Resolve the level universe once in the parent component containing the level toggle flow, using availableLevels with LOG_LEVELS as the fallback, and reuse that same list for both FilterRow rendering and handleToggleLevel/toggleFacet. Pass the resolved list to LogViewerSidebar as a single prop, removing the duplicate fallback expression so select-all behavior stays synchronized with the displayed checkboxes.src/logViewerWindow/timeline.ts (1)
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
newestIndexfield and assignment.No production code reads
TimelineBucket.newestIndex; only tests assert it. Remove the obsolete test assertions with the field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/timeline.ts` around lines 10 - 15, Remove the unused TimelineBucket.newestIndex field and its assignment, then delete the test assertions that expect this property. Keep the remaining timeline bucket data and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/documentViewerWindow/ipc.ts`:
- Around line 162-182: Update openDocumentViewerWindow and
startDocumentViewerWindowHandler to serialize concurrent creation through a
module-level pending-creation promise, reusing that promise while
createDocumentViewerWindow is in progress and clearing it after completion.
Ensure the listener handles the returned promise with catch-based error handling
so failures from getRootWindow or BrowserWindow construction do not become
unhandled rejections.
In `@src/documentViewerWindow/saveDocument.ts`:
- Around line 63-77: Update the injected script in the document-saving flow to
avoid the synchronous per-byte conversion and intermediate binary string. Use
native chunked blob conversion, preferably FileReader.readAsDataURL, then
extract the base64 payload and preserve the existing Buffer.from(..., 'base64')
return behavior.
In `@src/downloadsWindow/ipc.ts`:
- Around line 182-187: Update the transparency watcher in the downloads window
setup to send the setting only when running on macOS, matching the initial value
computed with the platform guard. Preserve the existing destroyed-window check
and renderer notification behavior, but ensure non-macOS platforms always
receive an opaque value.
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 229-235: Update the pruning effect around serverFilters and
setServerFilters so that when the filtered selection contains no hosts, it
resets the selection to null instead of persisting an empty array. Preserve the
existing pruned allow-list when one or more configured hosts remain.
In `@src/settingsWindow/ipc.ts`:
- Around line 48-52: Update createSettingsWindow to track its in-flight creation
promise and return the existing promise when creation is already underway,
before the settingsWindow guard and any await such as getRootWindow. Clear the
tracked promise after creation completes, while preserving the existing focus
behavior for an already-created window and ensuring both
SIDE_BAR_SETTINGS_BUTTON_CLICKED and settings-window/open-window reuse the same
creation operation.
In `@src/settingsWindow/sections/GeneralSection.tsx`:
- Line 16: Guard renderer platform access in GeneralSection.tsx at line 16 by
deriving isWin32 from globalThis.process?.platform with the existing false
default; update VideoCallsSection.tsx at lines 13-14 to use
globalThis.process?.platform for the Windows check and globalThis.process?.mas
!== true to preserve the non-MAS default.
In `@src/ui/reducers/currentView.ts`:
- Around line 88-96: Update the SIDE_BAR_DOWNLOADS_BUTTON_CLICKED and
SIDE_BAR_SETTINGS_BUTTON_CLICKED cases in currentView to migrate persisted
legacy “downloads” and “settings” views to a valid root view before preserving
state. In src/ui/reducers/currentView.ts:88-96, apply the normalization while
retaining current behavior for valid views; in
src/ui/reducers/__tests__/stateGroups.spec.ts:125-137, replace obsolete
legacy-state expectations and add migration coverage for APP_SETTINGS_LOADED.
---
Outside diff comments:
In `@src/ui/components/ServersView/PdfContent.tsx`:
- Around line 45-79: Move the did-finish-load listener registration out of
handleDomReady so it is attached only once to webviewElement, while preserving
the existing click-interception injection. Update the cleanup returned by the
effect to remove both the dom-ready handler and the did-finish-load handler.
In `@src/ui/components/TopBar/DownloadsIndicator.tsx`:
- Around line 478-519: Restore vertical padding on the dialog panel containing
the downloads header, rows, and footer, ensuring spacing appears at both its top
and bottom edges. Update the outer dialog Box and preserve the existing
horizontal header and footer padding behavior around recentDownloads and
handleShowAll.
---
Minor comments:
In `@src/documentViewerWindow/DocumentViewerWindow.tsx`:
- Around line 60-67: Update handleDownload to await the
document-viewer/save-document IPC result, inspect its success and error fields,
and surface failures to the user through the existing toast or toolbar message
mechanism. Also catch rejected invoke promises so IPC failures are handled
without unhandled rejections, while preserving the current download arguments.
In `@src/documentViewerWindow/ipc.ts`:
- Around line 117-125: Handle the promise returned by
documentViewerWindow.loadFile by attaching rejection handling. On failure,
report the loading error and close or otherwise clean up documentViewerWindow so
it cannot remain hidden when ready-to-show never fires.
In `@src/downloadsWindow/DownloadsWindow.tsx`:
- Around line 393-394: Update the dayGroups.map rendering in DownloadsWindow so
each group uses a unique React key even when multiple groups share the same
group.day value. Incorporate the map index or another run-specific identifier
while preserving the existing “unknown” fallback.
In `@src/downloadsWindow/ipc.ts`:
- Around line 46-50: Update createDownloadsWindow so an existing, non-destroyed
downloadsWindow is restored from its minimized state before calling focus().
Preserve the current early return and avoid changing behavior for newly created
windows.
In `@src/i18n/en.i18n.json`:
- Around line 206-218: The mimes translations duplicate singular and plural key
sets with inconsistent labels. Check the key set consumed by the downloads
window and retain only that set in the English locale and every other locale,
removing the unused counterparts while preserving the selected labels
consistently across locales.
In `@src/logViewerWindow/LogTimeline.tsx`:
- Around line 148-168: Update the timeline accessibility implementation around
the Box with ref={plotRef} so keyboard users can select and adjust both range
endpoints, either by exposing valid two-thumb slider controls with appropriate
values and key handling or by adding a separate keyboard-operable range control.
Do not retain a single slider role that only responds to mouse events; ensure
each endpoint has an accessible label and keyboard behavior while preserving the
existing mouse selection.
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 875-886: Update handleToggleAutoScroll to compute the next
auto-scroll value from the current autoScroll state before calling
setAutoScroll, then perform setUserHasScrolled and the delayed virtuosoRef
scroll outside the state updater only when enabling auto-scroll. Keep the
updater pure and preserve the existing delay and smooth-scroll behavior.
- Around line 89-98: Update formatDayLabel to detect YYYY-MM-DD fallback strings
and construct the date using local year, month, and day components before
formatting; preserve the existing parsing behavior for other date strings and
return the original value when parsing remains invalid.
In `@src/settingsWindow/ipc.ts`:
- Around line 85-93: Update the saved-bounds handling around
getSavedWindowBounds before constructing settingsWindow so restored settings
bounds are clamped fully within the relevant display work area. Preserve the
existing centered fallback when no valid saved bounds exist, and ensure the
BrowserWindow initialization receives only onscreen bounds.
In `@src/settingsWindow/sections.ts`:
- Around line 49-116: The SettingsWindow search flow should filter each
section’s settingKeys to only settings currently available and rendered before
matching search terms. Update the logic around the settingKeys search and reuse
the existing availability/platform and developer-mode checks so unsupported or
hidden controls cannot cause an otherwise empty section to appear.
In `@src/ui/components/SettingsView/features/CheckForUpdates.tsx`:
- Around line 87-103: Update the manual-check effect around hasRequestedCheck so
it clears the request without setting noUpdatesAvailable when updateError is
present, preserving the existing no-update message path otherwise. Add
updateError to the effect dependencies, and add a regression test covering a
failed manual check with no newUpdateVersion to ensure the error remains
visible.
In `@src/ui/components/SettingsView/features/ChromeThumbnailOption.tsx`:
- Around line 47-59: Export the existing getRadii helper from
WindowChromeThumbnail and import/reuse it in ChromeThumbnailOption when passing
the Frame radius, replacing the hardcoded radius={11}; ensure the frame uses the
platform-specific window radius (11 on darwin, 6 on win32/linux) so its corner
matches the thumbnail.
In `@src/ui/windowChrome/filters.ts`:
- Around line 17-31: Update toggleFacet so it resets to null only when every
value in universe is present in next, rather than comparing next.length with
universe.length. Preserve the existing toggle behavior and return next for
partial selections, including selections containing stale values.
In `@src/ui/windowChrome/NavRow.tsx`:
- Around line 47-65: Update NavRow to use native button semantics instead of the
incomplete tab pattern: remove role="tab", aria-selected, and tab-specific
tabIndex usage, while preserving selection styling and click/keyboard activation
through the button. Ensure the surrounding tablist/tab-panel contract is not
relied upon unless SettingsWindow and NavRow are updated to implement it
completely.
---
Nitpick comments:
In `@src/documentViewerWindow/DocumentViewerWindow.tsx`:
- Around line 38-39: Rename the state variable in the DocumentViewerWindow
component from document to documentDescriptor (or currentDocument) and update
all corresponding references, while keeping setDocument or renaming it
consistently. Ensure the global document object remains accessible within the
component body.
In `@src/documentViewerWindow/saveDocument.ts`:
- Around line 94-102: Update the document fetch in the
session.fromPartition(partition).fetch flow to pass an AbortSignal with a
timeout, ensuring an unresponsive workspace aborts and the save operation does
not wait indefinitely while preserving the existing response validation and
buffer conversion.
In `@src/downloadsWindow/DownloadRow.tsx`:
- Around line 112-117: Update the run callback in DownloadRow to accept the
DownloadActionChannel union rather than Parameters<typeof invoke>[0]. Remove
both casts and call invoke(channel, itemId) directly, preserving the existing
itemId dependency and behavior.
In `@src/downloadsWindow/downloads-window.tsx`:
- Around line 1-11: Rename the entry file from downloads-window.tsx to
downloadsWindow.tsx, then update the corresponding Rollup entry configuration
and the script reference in downloads-window.html to load downloadsWindow.js.
In `@src/i18n/en.i18n.json`:
- Around line 183-186: Remove the unused documentViewer.back translation key and
delete the in-pane DocumentViewer component that is its only consumer. Preserve
any separate, actively used document viewer implementation and unrelated
translation entries.
In `@src/logViewerWindow/__tests__/parseLogs.spec.ts`:
- Around line 26-37: Add a test alongside the existing parseLogLines context-tag
test that parses a multi-word tag such as “My Workspace” and asserts contextTags
preserves it as one entry while message remains correct. Use the existing
parseLogLines test pattern and symbols.
In `@src/logViewerWindow/LogEntry.tsx`:
- Around line 96-105: Update the serverTag lookup inside the useMemo callback to
use Object.prototype.hasOwnProperty.call(serverMapping, tag) instead of the
inherited-key-inclusive in operator, ensuring only own serverMapping keys are
matched and serverName remains renderable text.
In `@src/logViewerWindow/LogViewerSidebar.tsx`:
- Around line 116-127: Resolve the level universe once in the parent component
containing the level toggle flow, using availableLevels with LOG_LEVELS as the
fallback, and reuse that same list for both FilterRow rendering and
handleToggleLevel/toggleFacet. Pass the resolved list to LogViewerSidebar as a
single prop, removing the duplicate fallback expression so select-all behavior
stays synchronized with the displayed checkboxes.
In `@src/logViewerWindow/timeline.ts`:
- Around line 10-15: Remove the unused TimelineBucket.newestIndex field and its
assignment, then delete the test assertions that expect this property. Keep the
remaining timeline bucket data and behavior unchanged.
In `@src/settingsWindow/sections/AdvancedSection.tsx`:
- Line 19: Rename AdvancedSection.tsx to advancedSection.tsx, GeneralSection.tsx
to generalSection.tsx, TelephonySection.tsx to telephonySection.tsx, and
VideoCallsSection.tsx to videoCallsSection.tsx; rename SettingGroupDivider.tsx
to settingGroupDivider.tsx, DebugLogging.tsx to debugLogging.tsx,
DetailedEventsLogging.tsx to detailedEventsLogging.tsx, and
VerboseOutlookLogging.tsx to verboseOutlookLogging.tsx. Update every import path
referencing these files while preserving the PascalCase component names.
In `@src/settingsWindow/SettingsSidebar.tsx`:
- Line 1: Rename src/settingsWindow/SettingsSidebar.tsx#L1-L1 to
settingsSidebar.tsx, src/settingsWindow/SettingsWindow.tsx#L1-L1 to
settingsWindow.tsx, src/settingsWindow/sections/CertificateRow.tsx#L1-L1 to
certificateRow.tsx, src/settingsWindow/sections/CertificatesSection.tsx#L1-L1 to
certificatesSection.tsx, and src/settingsWindow/settings-window.tsx#L1-L1 to a
camelCase entry point such as settingsWindowEntry.tsx; update all imports and
entry-point references while preserving the PascalCase component names.
In `@src/ui/components/SettingsView/features/SettingGroupDivider.tsx`:
- Around line 14-17: Update the divider styling in SettingGroupDivider to remove
the inline backgroundColor style and pass the typed
`backgroundColor='stroke-extra-light'` prop to the Box component, while
preserving the existing one-pixel height.
In `@src/ui/components/SettingsView/features/WindowChromeThumbnail.tsx`:
- Around line 125-137: Update the Windows controls group in
WindowChromeThumbnail to derive the x coordinates in the path and rect elements
from WIDTH, preserving their current trailing-edge spacing when WIDTH is 168.
Replace the hardcoded 130, 144, 157, and 163 positions with WIDTH-relative
offsets, and update the WindowChromeThumbnail.spec.tsx selectors that assert the
M130 path prefix.
In `@src/ui/components/TopBar/DownloadsIndicator.tsx`:
- Line 8: Move the DownloadRow component from the downloadsWindow module into
the shared src/ui/windowChrome location, then update DownloadsIndicator and any
other consumers to import it from that shared location while preserving its
existing behavior.
In `@src/ui/reducers/isDownloadsWindowOpen.ts`:
- Around line 23-31: Retain runtime payload validation for the window-state
reducers: in src/ui/reducers/isDownloadsWindowOpen.ts (23-31),
src/ui/reducers/isLogViewerWindowOpen.ts (23-31), and
src/ui/reducers/isSettingsWindowOpen.ts (23-31), keep the boolean guards and
invalid-payload fallback; in src/ui/reducers/secondaryWindowStates.ts (27-30),
validate the action payload before destructuring so missing payloads cannot
throw; in src/ui/reducers/isLogViewerWindowOpen.spec.ts (28-42), preserve or add
coverage for malformed payloads reaching the reducer. If validation is instead
moved to the IPC boundary, implement it before removing any reducer guards and
cover all listed action paths.
In `@src/ui/windowChrome/WindowToolbar.tsx`:
- Around line 1-13: Rename the component files and update every import
reference: src/ui/windowChrome/WindowToolbar.tsx to windowToolbar.tsx,
DayHeader.tsx to dayHeader.tsx, FilterRow.tsx to filterRow.tsx,
FilterSection.tsx to filterSection.tsx, NavRow.tsx to navRow.tsx, and
StatusBar.tsx to statusBar.tsx. Keep the component names PascalCase, including
WindowToolbar, DayHeader, FilterRow, FilterSection, NavRow, and StatusBar.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Two of these are user-visible. The log viewer's server filter could hide every entry. Making an empty facet selection mean "nothing selected" gave the pruning effect a way to empty the list: when no persisted host survived, it set `[]`. It falls back to untouched now, which is what the effect was there to guarantee. A profile saved before downloads and settings moved into windows still names one of them as the root window's view. Restoring it stranded the reader there, since the sidebar buttons that used to leave those views now open windows. Such a value is ignored on load. The focus-request branch that returned 'downloads' goes too — the downloads notification opens the window through SIDE_BAR_DOWNLOADS_BUTTON_CLICKED and nothing dispatches it. The rest is hardening. Each window guarded on its module variable and then awaited the main window before assigning it, so two opens arriving in that gap each built a window and the second orphaned the first — visible, untracked, unclosable from its own channel. Every caller now passes through one gate holding a single in-flight promise, `restore*` included. The document viewer's listener catches instead of leaving an unhandled rejection. Reading a blob to save it used a per-byte loop and btoa in the reader's own workspace, freezing that window for the length of the file and holding it three times over. FileReader does it natively; checked byte for byte over the full 0-255 range. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/documentViewerWindow/ipc.ts (1)
180-195: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep
pendingCreationactive until the document renderer is ready.
buildDocumentViewerWindowignores theloadFilepromise, so page-load failures do not reach the handler’scatch. The renderer registersDOCUMENT_CHANNELonly after its asynchronous bootstrap. A second open can therefore send its replacement document before the listener exists. AwaitloadFile, add a renderer-ready acknowledgement after registering the listener, and reset or destroy failed windows. Add tests for load failure and back-to-back opens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/documentViewerWindow/ipc.ts` around lines 180 - 195, Update buildDocumentViewerWindow and the document renderer bootstrap so window creation remains pending until loadFile completes and the renderer acknowledges readiness after registering DOCUMENT_CHANNEL. Propagate load failures to startDocumentViewerWindowHandler’s catch, and reset or destroy failed windows before allowing retries. Add coverage for load failure and back-to-back opens to ensure replacement documents wait for renderer readiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/documentViewerWindow/ipc.ts`:
- Around line 180-195: Update buildDocumentViewerWindow and the document
renderer bootstrap so window creation remains pending until loadFile completes
and the renderer acknowledges readiness after registering DOCUMENT_CHANNEL.
Propagate load failures to startDocumentViewerWindowHandler’s catch, and reset
or destroy failed windows before allowing retries. Add coverage for load failure
and back-to-back opens to ensure replacement documents wait for renderer
readiness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15097151-3e8f-44eb-bf46-274567236edd
📒 Files selected for processing (8)
src/documentViewerWindow/ipc.tssrc/documentViewerWindow/saveDocument.tssrc/downloadsWindow/ipc.tssrc/logViewerWindow/ipc.tssrc/logViewerWindow/logViewerWindow.tsxsrc/settingsWindow/ipc.tssrc/ui/reducers/__tests__/stateGroups.spec.tssrc/ui/reducers/currentView.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/settingsWindow/ipc.ts
- src/documentViewerWindow/saveDocument.ts
- src/logViewerWindow/ipc.ts
- src/ui/reducers/currentView.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: build (windows-latest, windows)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/downloadsWindow/ipc.tssrc/ui/reducers/__tests__/stateGroups.spec.tssrc/logViewerWindow/logViewerWindow.tsxsrc/documentViewerWindow/ipc.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/ui/reducers/__tests__/stateGroups.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/ui/reducers/__tests__/stateGroups.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/ui/reducers/__tests__/stateGroups.spec.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
Files:
src/logViewerWindow/logViewerWindow.tsx
🧠 Learnings (4)
📚 Learning: 2026-05-19T20:49:24.859Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat.Electron PR: 3329
File: src/ui/reducers/e2ePdfPreviewSizeLimit.ts:14-16
Timestamp: 2026-05-19T20:49:24.859Z
Learning: In Rocket.Chat.Electron’s reducer files under src/ui/reducers/, reducers should not re-implement validation for action payloads. Assume the caller (UI component or dispatch site) has already validated the action payload and type/shape; reducers should trust the payload and update state directly. If validation is needed, add it at the dispatch site/caller rather than inside the reducer.
Applied to files:
src/ui/reducers/__tests__/stateGroups.spec.ts
📚 Learning: 2026-05-19T20:49:24.859Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat.Electron PR: 3329
File: src/ui/reducers/e2ePdfPreviewSizeLimit.ts:14-16
Timestamp: 2026-05-19T20:49:24.859Z
Learning: In the Rocket.Chat.Electron UI reducers under src/ui/reducers/, do not add/repeat input validation for action payloads inside reducers. Follow the existing codebase pattern: validate the action payload in the caller (e.g., the UI component or dispatch site) before dispatching. Reducers should trust the incoming payload and apply it directly to state. If adding/updating a reducer, ensure the corresponding caller performs the necessary validation (e.g., check numeric constraints like !isNaN(value) && value > 0 before dispatching the action).
Applied to files:
src/ui/reducers/__tests__/stateGroups.spec.ts
📚 Learning: 2026-08-12T14:11:15.452Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/logViewerWindow/logViewerWindow.tsx:229-238
Timestamp: 2026-08-12T14:11:15.452Z
Learning: In `src/logViewerWindow/logViewerWindow.tsx`, `FacetSelection` uses `null` to mean all options are selected and `[]` to mean no options are selected. When pruning persisted server filters removes every host, the code must store `null`, not `[]`, to prevent `matchesServer` from excluding all log entries.
Applied to files:
src/logViewerWindow/logViewerWindow.tsx
📚 Learning: 2026-08-12T14:11:40.244Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/settingsWindow/sections/GeneralSection.tsx:16-16
Timestamp: 2026-08-12T14:11:40.244Z
Learning: In Rocket.Chat.Electron renderer TypeScript/TSX files running with nodeIntegration enabled, direct access to process.platform and process.mas is intentional; do not require optional-chaining fallbacks for these properties. Optional-chaining safeguards should apply to Linux-only APIs such as process.getuid(), process.getgid(), process.geteuid(), and process.getegid(). Adding fallbacks for process.platform or process.mas can silently select an incorrect UI layout and conceal a renderer configuration error.
Applied to files:
src/logViewerWindow/logViewerWindow.tsx
🔇 Additional comments (3)
src/downloadsWindow/ipc.ts (1)
46-47: LGTM!Also applies to: 146-166
src/logViewerWindow/logViewerWindow.tsx (1)
232-237: LGTM!src/ui/reducers/__tests__/stateGroups.spec.ts (1)
57-59: LGTM!Also applies to: 74-96, 141-153
Reopening a secondary window only called focus(), which does not deminiaturize on macOS or restore on Windows. A shared helper now restores, shows, and focuses in every reuse branch.
Await the save-document result and show a dismissible danger Callout on failure (cancellation stays silent). Queue DOCUMENT_CHANNEL sends behind did-finish-load with last-request-wins so rapid opens cannot be lost, and key the content webview on partition:url so a same-URL document from another workspace remounts under its own session.
isFacetSelected now takes the facet universe: values a facet cannot name (like normal downloads' 'All' status) always match, so narrowing the status facet no longer hides every normal download. toggleFacet sanitizes persisted selections against the universe so stale values cannot collapse the selection back to everything.
The check-for-updates result effect now waits for the check to actually start before acting on its settle, instead of consuming the request on stale state. When an update is found the settings window closes so the root window's update panel is visible. Section auto-selection during search is a non-persisted override; only explicit clicks persist.
persistValues used a leading-edge throttle that silently discarded writes within one second, losing pre-quit window-state changes. It now coalesces to a trailing write of the latest values, and before-quit flushes anything still pending.
Load errors now show a danger Callout with retry instead of the "adjust filters" empty state. Saving acknowledges success on the toolbar button, stays silent on cancel, and shows a dismissible Callout on failure.
The plot already advertised role=slider. Arrow/Home/End now move the selected bucket, Shift extends the range, Escape clears it, and aria-value* reports the active bucket. Mouse drag and the clear button reset the keyboard anchor.
Card radius uses --rcx-border-radius-large and the shadow uses --rcx-color-shadow-elevation-1 (Tile's elevation-1 chain) so the shared card follows theme and high-contrast instead of hardcoded rgba and platform px.
The section list advertised a tablist without the tabs contract. Arrow/Home/End now move the selected section, only the current tab is in tab order, and the content panel is linked with aria-controls. Selected labels use fontScale p2b so state is not color-only.
Opaque hover and selected fills now use surface-hover/selected. TextButton keeps an unfilled look but gains a 24px hit target and a focus ring. Copy and save ticks are announced on a polite live region, and save uses its own label instead of "Copied".
The live dot is a StatusBullet, level stripes use bullet/badge fills at 1px, and Display rows are ToggleSwitches so they no longer look like data filters. Row-action fade respects prefers-reduced-motion.
Clear All and certificate remove now ask through the existing Electron dialog pattern before they run. The downloads progress fill uses ProgressBar's info token, and the width animation stops under prefers-reduced-motion.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/logViewerWindow/logViewerWindow.tsx (2)
185-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
timeRangewhen the log source changes.Line 185 states that the range belongs to the current file.
handleOpenLogFileandhandleOpenDefaultLogchange the source but retaintimeRange. If the ranges do not overlap, the new file shows no entries.Clear
timeRangebefore changing the selected log source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/logViewerWindow.tsx` around lines 185 - 187, Reset timeRange to null in both handleOpenLogFile and handleOpenDefaultLog before updating the selected log source, ensuring each newly opened log starts without the previous file’s range filter.
257-338: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDiscard responses from older load requests.
loadLogscan overlap when the user changes files or requests a refresh. An olderread-logsrequest can resolve last and overwritelogEntries,fileInfo, andcurrentLogFilefor the newer source.Use a monotonically increasing request identifier. Guard every response state update, including
setIsLoading(false), against the latest request identifier.parseGenerationRefonly creates distinct entry IDs. It does not prevent stale responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/logViewerWindow.tsx` around lines 257 - 338, The loadLogs callback must discard results from overlapping requests by introducing a monotonically increasing request identifier. Capture the identifier at the start of each loadLogs invocation and guard every response-dependent state update, including setLogEntries, setExpandedEntryIds, setCurrentLogFile, setFileInfo, setLoadError, and setIsLoading(false), so only the latest request can update state; do not use parseGenerationRef for this purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/main/persistence.ts`:
- Around line 76-86: Update the elapsed-time comparison in persistValues to use
an inclusive boundary, so elapsed values equal to THROTTLE_INTERVAL_MS clear
pendingValues and call writeNow synchronously. Add a test covering elapsed ===
THROTTLE_INTERVAL_MS and verify it does not schedule a trailing save.
In `@src/documentViewerWindow/DocumentViewerWindow.tsx`:
- Around line 62-82: Update handleDownload to catch rejected
document-viewer-window/save-document invocations and setSaveError to
t('documentViewer.downloadError') when rejection occurs, while preserving the
existing cancellation and unsuccessful-result handling.
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 1066-1080: Prevent stale log entries from rendering when loadError
is present: update the refresh failure handling around handleRefresh to clear
logEntries, or gate the GroupedVirtuoso and timeline rendering on the absence of
loadError. Preserve the retry callout and ensure failed refreshes do not display
previous entries.
---
Outside diff comments:
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 185-187: Reset timeRange to null in both handleOpenLogFile and
handleOpenDefaultLog before updating the selected log source, ensuring each
newly opened log starts without the previous file’s range filter.
- Around line 257-338: The loadLogs callback must discard results from
overlapping requests by introducing a monotonically increasing request
identifier. Capture the identifier at the start of each loadLogs invocation and
guard every response-dependent state update, including setLogEntries,
setExpandedEntryIds, setCurrentLogFile, setFileInfo, setLoadError, and
setIsLoading(false), so only the latest request can update state; do not use
parseGenerationRef for this purpose.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c939c6eb-d867-4785-ad57-11c8c56bb3d2
📒 Files selected for processing (43)
src/app/main/persistence.main.spec.tssrc/app/main/persistence.tssrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/documentViewerWindow/ipc.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/downloadsWindow/DownloadRow.tsxsrc/downloadsWindow/DownloadsWindow.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/downloadsWindow/ipc.tssrc/downloadsWindow/main/ipc.main.spec.tssrc/i18n/en.i18n.jsonsrc/ipc/channels.tssrc/logViewerWindow/LogEntry.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogViewerToolbar.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/logViewerWindow/appearance.tssrc/logViewerWindow/ipc.tssrc/logViewerWindow/logViewerWindow.tsxsrc/logViewerWindow/styles.tsxsrc/main.tssrc/settingsWindow/SettingsSidebar.tsxsrc/settingsWindow/SettingsWindow.tsxsrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/ipc.tssrc/settingsWindow/main/ipc.main.spec.tssrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/settingsWindow/sections/CertificateRow.tsxsrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsxsrc/ui/components/SettingsView/features/CheckForUpdates.tsxsrc/ui/main/secondaryWindowFocus.tssrc/ui/windowChrome/NavRow.tsxsrc/ui/windowChrome/TextButton.tsxsrc/ui/windowChrome/__tests__/copiedFeedback.spec.tsxsrc/ui/windowChrome/__tests__/filters.spec.tssrc/ui/windowChrome/appearance.tssrc/ui/windowChrome/filters.tssrc/ui/windowChrome/styles.tsxsrc/ui/windowChrome/useCopiedFeedback.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- src/logViewerWindow/LogViewerToolbar.tsx
- src/main.ts
- src/ipc/channels.ts
- src/ui/windowChrome/filters.ts
- src/ui/windowChrome/tests/filters.spec.ts
- src/downloadsWindow/DownloadsWindow.tsx
- src/downloadsWindow/DownloadRow.tsx
- src/ui/windowChrome/styles.tsx
- src/documentViewerWindow/ipc.ts
- src/settingsWindow/SettingsWindow.tsx
- src/ui/components/SettingsView/features/CheckForUpdates.tsx
- src/ui/windowChrome/NavRow.tsx
- src/ui/windowChrome/appearance.ts
- src/logViewerWindow/ipc.ts
- src/logViewerWindow/appearance.ts
- src/settingsWindow/ipc.ts
- src/settingsWindow/sections/CertificateRow.tsx
- src/i18n/en.i18n.json
- src/logViewerWindow/LogEntry.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: build (macos-latest, mac)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/ui/main/secondaryWindowFocus.tssrc/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/settingsWindow/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/ui/windowChrome/TextButton.tsxsrc/ui/windowChrome/__tests__/copiedFeedback.spec.tsxsrc/ui/windowChrome/useCopiedFeedback.tssrc/downloadsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/documentViewerWindow/main/ipc.main.spec.tssrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.tssrc/downloadsWindow/ipc.tssrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsxsrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsxsrc/app/main/persistence.tssrc/logViewerWindow/styles.tsxsrc/logViewerWindow/logViewerWindow.tsx
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/settingsWindow/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/ui/windowChrome/__tests__/copiedFeedback.spec.tsxsrc/downloadsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.tssrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsx
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/settingsWindow/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/downloadsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/settingsWindow/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/ui/windowChrome/__tests__/copiedFeedback.spec.tsxsrc/downloadsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.tssrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
Files:
src/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/ui/windowChrome/TextButton.tsxsrc/ui/windowChrome/__tests__/copiedFeedback.spec.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsxsrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsxsrc/logViewerWindow/styles.tsxsrc/logViewerWindow/logViewerWindow.tsx
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/settingsWindow/main/ipc.main.spec.tssrc/downloadsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/app/main/persistence.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/settingsWindow/main/ipc.main.spec.tssrc/downloadsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/app/main/persistence.main.spec.ts
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: qa/AGENTS.md:0-0
Timestamp: 2026-07-09T13:51:14.404Z
Learning: Applies to qa/** : Classify changed Desktop surfaces by user-visible risk, including Electron main process, protocol handlers, OS default handlers, settings UI, menus, modals, packaging/installers, startup, shortcuts, workspace routing, i18n, and layout.
📚 Learning: 2026-06-26T18:14:15.729Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx:47-55
Timestamp: 2026-06-26T18:14:15.729Z
Learning: In `src/ui/components/SettingsView/features/SettingField.tsx` for the Rocket.Chat Electron App settings panel, full-width selects and inputs are intentional by design: the UXDQA Figma spec calls for controls to stretch to the form column width in the stacked label/description layout, and the macOS panel was verified 1:1 against that spec. Do not flag full-width numeric inputs such as `src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx` as layout regressions in this panel.
Applied to files:
src/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/settingsWindow/SettingsSidebar.tsx
📚 Learning: 2026-08-12T14:11:40.244Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/settingsWindow/sections/GeneralSection.tsx:16-16
Timestamp: 2026-08-12T14:11:40.244Z
Learning: In Rocket.Chat.Electron renderer TypeScript/TSX files running with nodeIntegration enabled, direct access to process.platform and process.mas is intentional; do not require optional-chaining fallbacks for these properties. Optional-chaining safeguards should apply to Linux-only APIs such as process.getuid(), process.getgid(), process.geteuid(), and process.getegid(). Adding fallbacks for process.platform or process.mas can silently select an incorrect UI layout and conceal a renderer configuration error.
Applied to files:
src/settingsWindow/__tests__/SettingsWindow.spec.tsxsrc/settingsWindow/sections/CertificateRow.spec.tsxsrc/logViewerWindow/__tests__/LogTimeline.spec.tsxsrc/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/ui/windowChrome/TextButton.tsxsrc/ui/windowChrome/__tests__/copiedFeedback.spec.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsxsrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsxsrc/logViewerWindow/styles.tsxsrc/logViewerWindow/logViewerWindow.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.spec.ts : Use `*.spec.ts` for renderer process tests.
Applied to files:
src/downloadsWindow/__tests__/DownloadsWindow.spec.tsxsrc/settingsWindow/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/displayControls.spec.tsxsrc/documentViewerWindow/main/ipc.main.spec.tssrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.tssrc/ui/components/SettingsView/features/CheckForUpdates.spec.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.main.spec.ts : Use `*.main.spec.ts` for main process tests.
Applied to files:
src/settingsWindow/main/ipc.main.spec.tssrc/documentViewerWindow/main/ipc.main.spec.tssrc/app/main/persistence.main.spec.ts
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.main.spec.ts : Main-process specs use `*.main.spec.ts`.
Applied to files:
src/settingsWindow/main/ipc.main.spec.tssrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.ts
📚 Learning: 2026-07-09T13:51:14.404Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: qa/AGENTS.md:0-0
Timestamp: 2026-07-09T13:51:14.404Z
Learning: Applies to qa/** : Classify changed Desktop surfaces by user-visible risk, including Electron main process, protocol handlers, OS default handlers, settings UI, menus, modals, packaging/installers, startup, shortcuts, workspace routing, i18n, and layout.
Applied to files:
src/settingsWindow/main/ipc.main.spec.ts
📚 Learning: 2026-07-09T13:51:14.404Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: qa/AGENTS.md:0-0
Timestamp: 2026-07-09T13:51:14.404Z
Learning: Applies to qa/**/scripts/*.mjs : If a script mutates OS state, put the mutation behind an explicit flag and document cleanup in the matching flow.
Applied to files:
src/settingsWindow/main/ipc.main.spec.tssrc/downloadsWindow/ipc.ts
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.spec.{ts,tsx} : Renderer specs use `*.spec.ts` / `*.spec.tsx`.
Applied to files:
src/logViewerWindow/__tests__/displayControls.spec.tsxsrc/settingsWindow/__tests__/SettingsSidebar.spec.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to src/**/*.{spec.ts,spec.tsx} : Renderer test files should be placed in nested module paths such as `src/<module>/<subdir>/*.spec.ts(x)` so Jest discovers them.
Applied to files:
src/logViewerWindow/__tests__/displayControls.spec.tsxsrc/settingsWindow/__tests__/SettingsSidebar.spec.tsxsrc/app/main/persistence.main.spec.ts
📚 Learning: 2026-08-12T14:11:17.209Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/logViewerWindow/logViewerWindow.tsx:229-238
Timestamp: 2026-08-12T14:11:17.209Z
Learning: In `src/logViewerWindow/logViewerWindow.tsx`, `FacetSelection` uses `null` to mean all options are selected and `[]` to mean no options are selected. When pruning persisted server filters removes every host, the code must store `null`, not `[]`, to prevent `matchesServer` from excluding all log entries.
Applied to files:
src/logViewerWindow/__tests__/displayControls.spec.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/logViewerWindow.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.{ts,tsx} : Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Applied to files:
src/ui/windowChrome/TextButton.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsx
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.{ts,tsx} : Use Fuselage components from `rocket.chat/fuselage` for UI work unless the design requires something Fuselage does not provide.
Applied to files:
src/ui/windowChrome/TextButton.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.{ts,tsx} : Import Fuselage components from `rocket.chat/fuselage`.
Applied to files:
src/ui/windowChrome/TextButton.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogStatusBar.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to qa/**/*.md : Describe screen region, relative position, icon shape, nearby UI, visible text, and confirmation state in QA steps.
Applied to files:
src/ui/windowChrome/__tests__/copiedFeedback.spec.tsx
📚 Learning: 2026-07-09T13:51:14.404Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: qa/AGENTS.md:0-0
Timestamp: 2026-07-09T13:51:14.404Z
Learning: Applies to qa/**/flows/*.md : Use the implementation as the source of truth for visible steps; for Rocket.Chat Desktop UI, inspect the React component tree, Fuselage icon names, translation keys, menu action definitions, modal button labels, and platform guards; for browser helpers, inspect the committed HTML; for OS behavior, inspect the branch code/tests that determine the expected prompt, settings button, registry/default-app state, or desktop integration.
Applied to files:
src/documentViewerWindow/DocumentViewerWindow.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.{tsx,jsx} : Use React functional components with hooks.
Applied to files:
src/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsx
📚 Learning: 2026-06-26T18:14:16.585Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/ToggleField.tsx:1-8
Timestamp: 2026-06-26T18:14:16.585Z
Learning: In the App settings UI for `src/ui/components/SettingsView/features/ToggleField.tsx` in Rocket.Chat Electron, the Fuselage three-tier field structure `FieldLabel` / `FieldDescription` / `FieldHint` is intentionally required by the UXDQA spec: `FieldDescription` carries the regular secondary body text, while `FieldHint` is reserved for the smaller dimmer subline such as restart caveats, so they should not be collapsed into a single hint tier.
Applied to files:
src/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogViewerSidebar.tsx
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.{ts,tsx} : Use React functional components with hooks.
Applied to files:
src/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/LogTimeline.tsxsrc/settingsWindow/SettingsSidebar.tsx
📚 Learning: 2026-07-09T13:51:14.404Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: qa/AGENTS.md:0-0
Timestamp: 2026-07-09T13:51:14.404Z
Learning: Applies to qa/**/scripts/*.mjs : Keep exporters deterministic and dependency-light, using Node built-ins plus existing project dependencies only.
Applied to files:
src/app/main/persistence.main.spec.ts
📚 Learning: 2026-08-12T14:11:43.869Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/settingsWindow/sections/GeneralSection.tsx:16-16
Timestamp: 2026-08-12T14:11:43.869Z
Learning: In Rocket.Chat.Electron renderer windows that run with `nodeIntegration: true`, direct `process.platform` and `process.mas` access is intentional. Do not require optional-chaining fallbacks for these properties. The optional-chaining guidance applies to Linux-only process APIs such as `process.getuid()`, `process.getgid()`, `process.geteuid()`, and `process.getegid()`. A fallback for `process.platform` or `process.mas` can silently select an incorrect UI layout and hide a renderer configuration error.
Applied to files:
src/downloadsWindow/ipc.ts
📚 Learning: 2026-06-26T18:14:11.817Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx:47-55
Timestamp: 2026-06-26T18:14:11.817Z
Learning: In the Rocket.Chat Electron App SettingsView features under `src/ui/components/SettingsView/features/`, treat full-width selects/inputs (including full-width numeric inputs) as intentional for the stacked label/description layout. Per the UXDQA Figma spec (and macOS 1:1 verification), reviews should not flag these as layout regressions as long as they match the expected form-column stretching behavior.
Applied to files:
src/ui/components/SettingsView/features/CheckForUpdates.spec.tsx
📚 Learning: 2026-06-26T18:14:13.838Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/ToggleField.tsx:1-8
Timestamp: 2026-06-26T18:14:13.838Z
Learning: In Rocket.Chat Electron App settings field UIs that use the Fuselage three-tier pattern, keep the `FieldLabel` / `FieldDescription` / `FieldHint` structure separate. Use `FieldDescription` for the regular secondary body text, and reserve `FieldHint` for the smaller, dimmer subline content (e.g., restart caveats). Do not collapse `FieldDescription` and `FieldHint` into a single hint tier, as this violates the intended UXDQA spec.
Applied to files:
src/ui/components/SettingsView/features/CheckForUpdates.spec.tsx
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.{ts,tsx} : Check `Theme.d.ts` for valid color tokens before using Fuselage colors.
Applied to files:
src/logViewerWindow/logViewerWindow.tsx
🔇 Additional comments (22)
src/app/main/persistence.main.spec.ts (1)
43-109: LGTM!src/documentViewerWindow/DocumentViewerWindow.tsx (1)
1-60: LGTM!Also applies to: 84-212
src/documentViewerWindow/main/ipc.main.spec.ts (1)
1-324: LGTM!src/downloadsWindow/__tests__/DownloadsWindow.spec.tsx (1)
1-110: LGTM!src/downloadsWindow/ipc.ts (1)
4-5: LGTM!Also applies to: 17-17, 33-33, 158-224
src/downloadsWindow/main/ipc.main.spec.ts (1)
1-179: LGTM!src/settingsWindow/main/ipc.main.spec.ts (1)
1-189: LGTM!src/settingsWindow/sections/CertificateRow.spec.tsx (1)
1-78: LGTM!src/ui/components/SettingsView/features/CheckForUpdates.spec.tsx (1)
1-125: LGTM!src/ui/main/secondaryWindowFocus.ts (1)
1-15: LGTM!src/ui/windowChrome/__tests__/copiedFeedback.spec.tsx (1)
1-57: LGTM!src/logViewerWindow/LogStatusBar.tsx (1)
1-1: LGTM!Also applies to: 20-72
src/logViewerWindow/LogTimeline.tsx (1)
2-12: LGTM!Also applies to: 47-102, 109-150, 152-205, 221-323
src/logViewerWindow/LogViewerSidebar.tsx (1)
1-7: LGTM!Also applies to: 55-242
src/logViewerWindow/__tests__/LogTimeline.spec.tsx (1)
1-110: LGTM!src/logViewerWindow/__tests__/displayControls.spec.tsx (1)
1-126: LGTM!src/logViewerWindow/styles.tsx (1)
9-9: LGTM!Also applies to: 22-74
src/settingsWindow/SettingsSidebar.tsx (1)
3-18: LGTM!Also applies to: 49-96, 127-177
src/settingsWindow/__tests__/SettingsSidebar.spec.tsx (1)
1-228: LGTM!src/settingsWindow/__tests__/SettingsWindow.spec.tsx (1)
1-106: LGTM!src/ui/windowChrome/TextButton.tsx (1)
18-46: LGTM!src/ui/windowChrome/useCopiedFeedback.ts (1)
1-99: LGTM!
elapsed === 1000ms used to schedule a zero-delay trailing write instead of writing now. The comparison is inclusive, with a boundary test.
A thrown save IPC left an unhandled rejection and no Callout. handleDownload now catches and shows the existing download error.
A failed refresh no longer leaves the old timeline and list under the error Callout. Opening another file clears the time range. Overlapping read-logs responses are ignored via a request id.
Sidebar headings and day headers use one SectionLabel. Cmd/Ctrl+F focuses the local search field in logs, downloads, and settings. Day-header blur runs only over a vibrant window, and scrollbar thumbs follow currentColor instead of theme rgba.
Row IconButtons keep Fuselage's glyph chain. The document-viewer title icon matches the other toolbars. File-type labels no longer force weight 700. The certificates list uses stroke and radius tokens instead of a 4px literal frame.
Register the PDF click interceptor once and remove it on cleanup. Restore vertical padding on the title-bar downloads panel. Mime filters share one key set so the old in-window view matches the downloads window.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/logViewerWindow/logViewerWindow.tsx (1)
682-686: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize and invalidate incremental log reads.
checkForUpdatesruns fromsetIntervalbut is asynchronous. Two calls can capture the samepreviousSizeand read the same bytes. Both callbacks can then prepend the same entries. The newparseGenerationRefvalues give duplicate entries different IDs, so the list cannot remove them.A tail response can also apply after
loadLogs()or after a file switch becauseloadRequestIdRefonly guards full loads. Add a single-flight guard and a tail request token. Check the token and selected-file identity before applying the response. Invalidate the token when a full load or file switch starts. Add a delayed-overlap regression test.🐛 Proposed fix
+const tailRequestIdRef = useRef(0); +const tailInFlightRef = useRef(false); const checkForUpdates = useCallback(async () => { if (!isStreaming || !currentLogFile.isDefaultLog) return; + if (tailInFlightRef.current) return; + tailInFlightRef.current = true; + const tailRequestId = ++tailRequestIdRef.current; try { // existing stat and tail reads + if (tailRequestId !== tailRequestIdRef.current) return; // apply newEntries, newSize, and lastModifiedTime + } finally { + tailInFlightRef.current = false; }Also increment
tailRequestIdRefwhenloadLogs()and either file-switch handler starts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logViewerWindow/logViewerWindow.tsx` around lines 682 - 686, Serialize asynchronous checkForUpdates calls with a single-flight guard so overlapping intervals cannot read or prepend the same bytes. Add a tail request token, invalidate it when loadLogs() or either file-switch handler starts, and before applying tailResponse verify the token and selected-file identity still match. Keep parseGenerationRef updates only for responses that pass these checks, and add a delayed-overlap regression test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 682-686: Serialize asynchronous checkForUpdates calls with a
single-flight guard so overlapping intervals cannot read or prepend the same
bytes. Add a tail request token, invalidate it when loadLogs() or either
file-switch handler starts, and before applying tailResponse verify the token
and selected-file identity still match. Keep parseGenerationRef updates only for
responses that pass these checks, and add a delayed-overlap regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2eb50a-2d18-4271-b40d-b811d0c9f835
📒 Files selected for processing (22)
src/app/main/persistence.main.spec.tssrc/app/main/persistence.tssrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/downloadsWindow/DownloadRow.tsxsrc/downloadsWindow/DownloadsSidebar.tsxsrc/downloadsWindow/DownloadsWindow.tsxsrc/downloadsWindow/FileTypeIcon.tsxsrc/i18n/en.i18n.jsonsrc/logViewerWindow/LogEntry.tsxsrc/logViewerWindow/LogViewerSidebar.tsxsrc/logViewerWindow/logViewerWindow.tsxsrc/settingsWindow/SettingsSidebar.tsxsrc/settingsWindow/sections/CertificateRow.tsxsrc/settingsWindow/sections/CertificatesSection.tsxsrc/ui/components/DownloadsManagerView/index.tsxsrc/ui/components/ServersView/PdfContent.tsxsrc/ui/components/TopBar/DownloadsIndicator.tsxsrc/ui/windowChrome/DayHeader.tsxsrc/ui/windowChrome/FilterSection.tsxsrc/ui/windowChrome/SectionLabel.tsxsrc/ui/windowChrome/styles.tsxsrc/ui/windowChrome/useFindShortcut.ts
💤 Files with no reviewable changes (4)
- src/settingsWindow/sections/CertificateRow.tsx
- src/downloadsWindow/FileTypeIcon.tsx
- src/downloadsWindow/DownloadRow.tsx
- src/i18n/en.i18n.json
🚧 Files skipped from review as they are similar to previous changes (12)
- src/ui/windowChrome/FilterSection.tsx
- src/ui/windowChrome/styles.tsx
- src/ui/components/TopBar/DownloadsIndicator.tsx
- src/downloadsWindow/DownloadsWindow.tsx
- src/app/main/persistence.main.spec.ts
- src/settingsWindow/SettingsSidebar.tsx
- src/app/main/persistence.ts
- src/logViewerWindow/LogEntry.tsx
- src/logViewerWindow/LogViewerSidebar.tsx
- src/downloadsWindow/DownloadsSidebar.tsx
- src/settingsWindow/sections/CertificatesSection.tsx
- src/ui/components/ServersView/PdfContent.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: build (ubuntu-latest, linux)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/ui/components/DownloadsManagerView/index.tsxsrc/ui/windowChrome/useFindShortcut.tssrc/ui/windowChrome/DayHeader.tsxsrc/ui/windowChrome/SectionLabel.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/logViewerWindow.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
Files:
src/ui/components/DownloadsManagerView/index.tsxsrc/ui/windowChrome/DayHeader.tsxsrc/ui/windowChrome/SectionLabel.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/logViewerWindow.tsx
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: qa/AGENTS.md:0-0
Timestamp: 2026-07-09T13:51:14.404Z
Learning: Applies to qa/** : Classify changed Desktop surfaces by user-visible risk, including Electron main process, protocol handlers, OS default handlers, settings UI, menus, modals, packaging/installers, startup, shortcuts, workspace routing, i18n, and layout.
📚 Learning: 2026-08-12T14:11:40.244Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/settingsWindow/sections/GeneralSection.tsx:16-16
Timestamp: 2026-08-12T14:11:40.244Z
Learning: In Rocket.Chat.Electron renderer TypeScript/TSX files running with nodeIntegration enabled, direct access to process.platform and process.mas is intentional; do not require optional-chaining fallbacks for these properties. Optional-chaining safeguards should apply to Linux-only APIs such as process.getuid(), process.getgid(), process.geteuid(), and process.getegid(). Adding fallbacks for process.platform or process.mas can silently select an incorrect UI layout and conceal a renderer configuration error.
Applied to files:
src/ui/components/DownloadsManagerView/index.tsxsrc/ui/windowChrome/DayHeader.tsxsrc/ui/windowChrome/SectionLabel.tsxsrc/documentViewerWindow/DocumentViewerWindow.tsxsrc/logViewerWindow/logViewerWindow.tsx
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.{ts,tsx} : Use React functional components with hooks.
Applied to files:
src/ui/windowChrome/useFindShortcut.ts
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.{tsx,jsx} : Use React functional components with hooks.
Applied to files:
src/ui/windowChrome/useFindShortcut.ts
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.{ts,tsx} : Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Applied to files:
src/ui/windowChrome/SectionLabel.tsx
📚 Learning: 2026-06-26T18:14:16.585Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/ToggleField.tsx:1-8
Timestamp: 2026-06-26T18:14:16.585Z
Learning: In the App settings UI for `src/ui/components/SettingsView/features/ToggleField.tsx` in Rocket.Chat Electron, the Fuselage three-tier field structure `FieldLabel` / `FieldDescription` / `FieldHint` is intentionally required by the UXDQA spec: `FieldDescription` carries the regular secondary body text, while `FieldHint` is reserved for the smaller dimmer subline such as restart caveats, so they should not be collapsed into a single hint tier.
Applied to files:
src/ui/windowChrome/SectionLabel.tsx
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.{ts,tsx} : Use Fuselage components from `rocket.chat/fuselage` for UI work unless the design requires something Fuselage does not provide.
Applied to files:
src/ui/windowChrome/SectionLabel.tsx
📚 Learning: 2026-07-10T13:16:09.853Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-07-10T13:16:09.853Z
Learning: Applies to **/*.{ts,tsx} : Import Fuselage components from `rocket.chat/fuselage`.
Applied to files:
src/ui/windowChrome/SectionLabel.tsx
📚 Learning: 2026-08-03T18:47:17.146Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3431
File: src/updates/main.ts:0-0
Timestamp: 2026-08-03T18:47:17.146Z
Learning: In `src/updates/main.ts`, `electron-updater` installation failures from `autoUpdater.quitAndInstall()` normally emit the `error` event through `BaseUpdater.dispatchError`; the `autoUpdater.addListener('error', ...)` handler is the primary update-installation error-reporting path. The local `quitAndInstall` catch is only a backstop for unexpected synchronous throws.
Applied to files:
src/documentViewerWindow/DocumentViewerWindow.tsx
📚 Learning: 2026-03-11T06:38:40.426Z
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
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/documentViewerWindow/DocumentViewerWindow.tsx
📚 Learning: 2026-08-12T14:11:17.209Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat.Electron PR: 3444
File: src/logViewerWindow/logViewerWindow.tsx:229-238
Timestamp: 2026-08-12T14:11:17.209Z
Learning: In `src/logViewerWindow/logViewerWindow.tsx`, `FacetSelection` uses `null` to mean all options are selected and `[]` to mean no options are selected. When pruning persisted server filters removes every host, the code must store `null`, not `[]`, to prevent `matchesServer` from excluding all log entries.
Applied to files:
src/logViewerWindow/logViewerWindow.tsx
📚 Learning: 2026-07-09T13:50:56.290Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-09T13:50:56.290Z
Learning: Applies to **/*.{ts,tsx} : Check `Theme.d.ts` for valid color tokens before using Fuselage colors.
Applied to files:
src/logViewerWindow/logViewerWindow.tsx
🪛 React Doctor (0.9.3)
src/logViewerWindow/logViewerWindow.tsx
[error] 344-344: This resets a loading/busy flag only on the success path: if the awaited call rejects the reset never runs and the flag stays stuck truthy (a spinner that never stops, a button disabled forever). Move the reset into a finally block, or mirror it on every catch, so it clears on rejection too.
A trailing setLoading(false) after an await never runs if the awaited call rejects, so the flag stays stuck truthy; reset it in a finally block (or mirror the reset on every catch) so it clears on both paths.
(no-loading-flag-reset-outside-finally)
🔇 Additional comments (10)
src/documentViewerWindow/DocumentViewerWindow.tsx (1)
24-60: LGTM!Also applies to: 62-94, 96-171, 173-190, 192-224
src/ui/components/DownloadsManagerView/index.tsx (1)
79-83: LGTM!src/ui/windowChrome/DayHeader.tsx (1)
4-11: LGTM!Also applies to: 25-51
src/ui/windowChrome/SectionLabel.tsx (1)
1-18: LGTM!src/ui/windowChrome/useFindShortcut.ts (1)
1-17: LGTM!src/logViewerWindow/logViewerWindow.tsx (5)
138-138: LGTM!Also applies to: 259-260, 273-273, 328-341
773-776: LGTM!
817-818: LGTM!Also applies to: 836-837
873-874: LGTM!Also applies to: 884-884
1055-1062: LGTM!Also applies to: 1113-1128
The 2px bar had copied ProgressBar's status-font-on-info token, a text color used as a fill. It now uses font-info, the accent token for progress fills.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The log viewer, downloads, settings and the document viewer each open as their own window, built on one shared window chrome instead of four separate implementations.
Previously all four lived inside the main window: settings and downloads as full-window takeovers, the log viewer as a bare page, the document viewer as an overlay on top of the server pane. Pulling them out means the reader keeps their workspace visible while reading logs, a PDF, or changing a setting, and each window can be sized and placed independently.
Shared chrome
src/ui/windowChrome/holds what the windows have in common — toolbar, sidebar filter rows and sections, nav rows, status bar, day headers, text buttons, surface resolution, caption buttons and the transparency hook. A window is now mostly its own content plus a sidebar.The toolbar replaces the native title bar on both macOS and Windows: macOS keeps its traffic lights floating over it, Windows hides the caption entirely and the toolbar draws its own minimise/maximise/close from the main window's existing glyphs. Linux keeps its native frame. The buttons act on whichever window sent the request, so one registration serves all four.
Transparency follows the existing setting and applies live, without a restart.
transparentcannot be toggled after a window is created, so — as the root window already does — these windows are always transparent with a vibrancy material on macOS, and the setting decides only whether the renderer paints an opaque surface over it.All four windows remember their position and size, and the log viewer, downloads and settings reopen at launch if they were open at shutdown. Bounds come from
getNormalBounds()so maximising does not overwrite the size to restore to, are debounced becausemove/resizefire continuously while dragging, and are dropped when they no longer overlap any display — a window restored onto an unplugged monitor is a window the reader cannot reach.Log viewer
Downloads
Settings
vibrhit "Video calls"), so longer text falls back to substring.On macOS the About menu item now opens the system About panel; every Mac app has that item in the same place and its contents come from the bundle. Windows and Linux have no such convention, so they get no About item at all.
Document viewer
PDFs and markdown open in their own window rather than an overlay over the server pane. Every entry point already dispatched
SERVER_DOCUMENT_VIEWER_OPEN_URL, so the window listens for that one action and both the page's open request and the intercepted markdown download redirect at once; no caller changed. The document still renders in a webview on the originating server's session, which is what lets an authenticated URL resolve at all. One window, reused — a second document replaces the first.Both formats get a download button. The bytes are read on the workspace's own session, so an authenticated document saves as the signed-in user rather than as an anonymous request; a blob the server page created is read back through that page's web contents, since a blob URL resolves nowhere else.
Markdown also gets a source toggle, for reading a file as written rather than rendered. The text is already fetched to render it, so switching costs no round trip.
These two are the only viewers in the app — PDF and markdown are the formats the workspace preload can open, and both now live here.
Fixes found along the way
PdfContentannounced its webview to the main process ondid-attach, butgetWebContentsId()throws until the guest document exists, so the call threw every time and the announcement never arrived. It now announces ondom-ready.FieldGroup's rhythm — the PDF size limit sat 16px below its neighbour while everything else sat at 24. Worse,TelephonyGlobalShortcutnever accepted theclassNamethatFieldGrouppasses down, so its rows had no gap at all. Seven components that hand-rolledFieldmarkup now use the sharedSettingField/ToggleFieldwrappers. Measured on the built window: every gap within a group is 24px, every gap across a divider 49px.DownloadsIndicatorreadsDate.now()twice and a download counts as unseen only between the two; the test left that window under a millisecond wide, so it passed on the macOS runners and failed on Linux and Windows.Date.now()is stubbed instead. Master landed the same fix independently in the meantime, and its version is what this branch now carries.bg='surface', which is not in the palette — Fuselage logged "invalid color: surface" on every render and painted nothing. The window's card already carries the background, so the prop is gone.Testing
yarn lint,npx tsc --noEmitandyarn testall pass (169 suites, 1907 tests).Unit coverage: log parsing, timeline bucketing, pagination convergence, download grouping, file labels, fuzzy matching, the settings search index, the section registry, per-platform title-bar options, window-open reducers and saved-bounds validation.
Rendering was verified by loading the built bundles in Electron with stubbed IPC and seeded state — measuring computed surface colours, geometry, caption-button placement, field spacing and search results in both themes, and forcing
process.platformto check the Windows chrome — rather than eyeballing screenshots.Not covered by automated tests, and worth a look during review:
blob:document in the viewer window, both displaying it and downloading it: Chromium registers those per origin and partition, so a webview on the same partition should resolve them, but that is worth confirming against a real workspaceNotes
SettingsView,DownloadsManagerView,CertificatesManagerand the in-paneDocumentViewerare now unreachable. They are left in place to keep the diff to the new windows; removing them is a follow-up.The
dialog.about.*translation keys stayed put now that the dialog is gone — renaming them tosettings.*would orphan every existing translation in the other locales.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes