fix: workspace tags in log viewer filter#3284
Conversation
The log viewer server filter showed no results because the server-N IDs in log entries (assigned from a webContents pool) didn't match the IDs in the filter dropdown (derived from Redux array index). Replace the anonymous pool-based server-N system with hostname-based tags. Log entries now use the server hostname directly (e.g., [open.rocket.chat] instead of [server-2]), and the filter dropdown uses the same hostname as key. This makes the mapping deterministic and stable across app reloads.
|
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 Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (2)
WalkthroughSwitches server identification from numeric anonymous IDs to hostname-based keys: webContents are registered with hostnames derived from server URLs at log time; log entries and viewer components extract, match, and filter server tags using those hostname keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Main
participant Context
participant LogViewer
Renderer->>Main: send "console-log" (sender URL + message)
Main->>Main: parse sender origin
Main->>Main: compare origin vs configured server URLs
alt match found
Main->>Context: registerWebContentsServer(webContentsId, matchedServer.url)
end
Main->>Main: build context string (may include hostname)
Main->>LogViewer: emit/log entry with context
LogViewer->>LogViewer: findServerTag(context, serverMapping)
LogViewer->>LogViewer: render entry / apply hostname filter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labelstype: refactor area: logging area: log-viewer 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/logging/index.ts`:
- Around line 423-445: The matching currently compares only origins and loses
configured pathnames (causing https://host/team-a and /team-b to collide);
update the match in the block that builds matchedServer (use selectFunction,
servers, matchedServer, senderWebContents.getURL(), registerWebContentsServer)
to compare a normalized base URL that includes pathname (e.g., origin +
pathname, normalized to always include a trailing slash) rather than new
URL(...).origin alone: parse senderWebContents.getURL() into origin+pathname,
parse each s.url into origin+pathname, normalize both (ensure trailing slash)
and then compare those normalized base URLs to find matchedServer before calling
registerWebContentsServer.
In `@src/logViewerWindow/ipc.ts`:
- Around line 494-497: The current mapping uses getHostname(server.url) as the
key which drops scheme/port and causes collisions (e.g., localhost:3000 vs
localhost:4000); change the key to a stable normalized identifier derived from
the full URL/origin (for example use new URL(server.url).origin or a normalized
server.url string) inside the servers.forEach block and keep
getHostname(server.url) only as the display label; update the assignment to set
mapping[normalizedOrigin] = server.title || server.url || hostname so entries
are unique while the hostname remains the visible label.
In `@src/logViewerWindow/LogEntry.tsx`:
- Around line 86-90: The current code uses replace(tag, '') which removes the
first substring match and can mutate the wrong token; instead split
entry.context into tokens, filter out the exact token equal to tag, then join
tokens back into a string and normalize spaces to produce ctxWithout. Update the
logic around ctxWithout (the code referencing tag and entry.context in
LogEntry.tsx) to perform token-based removal (compare tokens for exact equality
to tag), then collapse multiple spaces and trim the result.
🪄 Autofix (Beta)
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
Run ID: 92995f9a-0b2d-4438-b87e-82612820d035
📒 Files selected for processing (5)
src/logViewerWindow/LogEntry.tsxsrc/logViewerWindow/ipc.tssrc/logViewerWindow/logViewerWindow.tsxsrc/logging/context.tssrc/logging/index.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/logViewerWindow/ipc.tssrc/logViewerWindow/logViewerWindow.tsxsrc/logViewerWindow/LogEntry.tsxsrc/logging/index.tssrc/logging/context.ts
Servers on the same hostname but different ports (e.g., :3000 vs :4000) would collide when using hostname alone. URL.host includes the port when non-default, making each server key unique.
Rocket.Chat servers are always identified by host, not subpaths. Revert to origin-based matching and keep getHost for the server tag.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/logging/context.ts`:
- Around line 3-4: The map webContentsToHostname is misleading because it stores
URL.host (host with port) not URL.hostname; rename the symbol to
webContentsToHost (or, if you prefer keeping the name, update its declaration
comment to state it stores the full host including port) and update all
references to webContentsToHost across the module (look for usages of
webContentsToHostname) so callers remain consistent; ensure the top-of-file
comment reflects "Maps webContentsId → host (includes port)" if you choose the
comment route.
🪄 Autofix (Beta)
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
Run ID: ab4ba98c-4630-494d-add0-311aa079d512
📒 Files selected for processing (2)
src/logViewerWindow/ipc.tssrc/logging/context.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/logViewerWindow/ipc.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/logging/context.ts
🔇 Additional comments (2)
src/logging/context.ts (2)
78-84: Lookup-before-fallback is the right simplification.Registered server context wins first, then non-server windows fall back to
local/unknown. That keeps tagging deterministic without reintroducing pool state.
240-243: Cleanup matches the reduced state model.With only a single
webContentsId → serverassociation left,delete()ondestroyedis enough.
- Rename webContentsToHostname → webContentsToHost to match URL.host usage - Use token-based removal in LogEntry to avoid partial substring matches
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/logging/context.ts`:
- Around line 68-73: registerWebContentsServer currently only sets
webContentsToHost for matched servers, leaving stale mappings when a WebContents
navigates to an unmatched origin; modify
registerWebContentsServer(webContentsId, serverUrl) to clear the mapping when
serverUrl is falsy or doesn't match (e.g., call
webContentsToHost.delete(webContentsId) when matched server is null/undefined)
and ensure cleanupServerContext remains for destroy cleanup; also update the
caller that invokes registerWebContentsServer to pass matchedServer?.url on
every navigation/event so unmatched navigations explicitly clear the stale entry
instead of leaving the previous host mapping.
🪄 Autofix (Beta)
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
Run ID: 4b669d93-a2c9-48fa-ae0a-e7ecffe43b5f
📒 Files selected for processing (2)
src/logViewerWindow/LogEntry.tsxsrc/logging/context.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/logViewerWindow/LogEntry.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/logging/context.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3284
File: src/logging/index.ts:423-445
Timestamp: 2026-04-01T15:41:54.991Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, Rocket.Chat server instances are always deployed at distinct origins (unique hostname or port combinations). Multiple servers sharing the same hostname but differing only by path (e.g., `https://host/team-a` vs `https://host/team-b`) is not a real-world deployment scenario. Therefore, comparing `new URL(s.url).origin` alone is sufficient for matching a webContents URL to its configured server in `src/logging/index.ts`, and path-based collision concerns do not apply.
📚 Learning: 2026-04-01T15:41:54.991Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3284
File: src/logging/index.ts:423-445
Timestamp: 2026-04-01T15:41:54.991Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, Rocket.Chat server instances are always deployed at distinct origins (unique hostname or port combinations). Multiple servers sharing the same hostname but differing only by path (e.g., `https://host/team-a` vs `https://host/team-b`) is not a real-world deployment scenario. Therefore, comparing `new URL(s.url).origin` alone is sufficient for matching a webContents URL to its configured server in `src/logging/index.ts`, and path-based collision concerns do not apply.
Applied to files:
src/logging/context.ts
🔇 Additional comments (1)
src/logging/context.ts (1)
55-63:URL.hostis the right key here.This keeps the tag stable while still distinguishing servers that differ by non-default port, which matches how this app identifies real server instances.
Based on learnings: “Rocket.Chat server instances are always deployed at distinct origins (unique hostname or port combinations) … comparing
new URL(s.url).originalone is sufficient … and path-based collision concerns do not apply.”
Three issues with the server filter: - get-server-tag IPC returned index-based tags (server-1) instead of hostnames, causing a mismatch with the filter dropdown which uses hostnames from getHost() - registerWebContentsServer was not called during dom-ready, so logs from webContents whose console injection failed had no server tag - Filter only checked context tags, missing logs that mention the server URL in the message body Now uses getHost() for server tags, registers server context early in dom-ready, and falls back to matching against the full raw log line.
Summary
Fixes the log viewer server filter to correctly match logs by server hostname. Previously, selecting a server in the filter dropdown showed zero results due to multiple mismatches in how server tags were generated and compared.
Problems Fixed
get-server-tagIPC handler returnedserver-1,server-2etc., but the filter dropdown used hostnames likerocketchat.example.com— they never matchedregisterWebContentsServer()was only called via the IPC console-log handler, but the console injection into webviews fails (Cannot find module 'electron'), so most webview logs never got a server tagChanges
src/logging/index.tsgetHost()inget-server-tagIPC handler to return hostnames instead of index-based tagsregisterWebContentsServer()duringdom-readyso webContents are tagged with server hostname early, before console injectionsrc/logViewerWindow/logViewerWindow.tsxTest plan
[rocketchat.jeanbrito.com],[pocrocket.gke-dev.linnovate.net])Summary by CodeRabbit
Bug Fixes
New Features