Skip to content

fix: workspace tags in log viewer filter#3284

Merged
jeanfbrito merged 5 commits intodevfrom
fix/log-viewer-server-filter
Apr 1, 2026
Merged

fix: workspace tags in log viewer filter#3284
jeanfbrito merged 5 commits intodevfrom
fix/log-viewer-server-filter

Conversation

@jeanfbrito
Copy link
Copy Markdown
Member

@jeanfbrito jeanfbrito commented Apr 1, 2026

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

  1. Index-based tags vs hostname filter: The get-server-tag IPC handler returned server-1, server-2 etc., but the filter dropdown used hostnames like rocketchat.example.com — they never matched
  2. Missing server context on webview logs: registerWebContentsServer() 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 tag
  3. Context-only matching too narrow: Most main process logs mention server URLs in the message body but don't have a server hostname in the context brackets — the filter only checked context tags

Changes

src/logging/index.ts

  • Import and use getHost() in get-server-tag IPC handler to return hostnames instead of index-based tags
  • Call registerWebContentsServer() during dom-ready so webContents are tagged with server hostname early, before console injection

src/logViewerWindow/logViewerWindow.tsx

  • Simplify context tag extraction to reuse whitespace split
  • Add fallback: match server hostname against the full raw log line when context tags don't match

Test plan

  • TypeScript compilation passes
  • ESLint passes
  • All 213 tests pass across 17 suites
  • Manual verification: log file now contains hostname-based tags (e.g. [rocketchat.jeanbrito.com], [pocrocket.gke-dev.linnovate.net])
  • Verify server filter dropdown shows logs when a server is selected

Summary by CodeRabbit

  • Bug Fixes

    • Logs now show real server hostnames instead of numeric IDs.
    • Server tag removal from log context is more accurate and avoids leftover whitespace.
    • Server filter matching is case-insensitive and more reliable.
  • New Features

    • Sender origins are registered so logs associate with the correct server host.
    • Server selector displays the hostname when no display name is provided.

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.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 1, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bfb4d833-8405-4d59-976c-c83b2c9084ec

📥 Commits

Reviewing files that changed from the base of the PR and between 9a2e821 and 1040ede.

📒 Files selected for processing (2)
  • src/logViewerWindow/logViewerWindow.tsx
  • src/logging/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/logging/index.ts
📜 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)
  • GitHub Check: check (ubuntu-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/fuselage for all UI work and only create custom components when Fuselage doesn't provide what's needed
Check Theme.d.ts for 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/logViewerWindow.tsx
🧠 Learnings (1)
📓 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.
🔇 Additional comments (2)
src/logViewerWindow/logViewerWindow.tsx (2)

167-169: LGTM!

The hostname-based keys align correctly with the serverMapping structure from the IPC handler (context snippet 1), and the fallback to hostname when name is undefined provides a sensible display value.


341-350: Filtering logic looks correct; note fallback could match across similar hostnames.

The case-insensitive exact matching for server tags (tag === serverFilter.toLowerCase()) is appropriate. The raw log fallback on line 350 is intentional per PR objectives for capturing logs that mention a server URL in the message body, but be aware that includes() will match if one hostname is a substring of another (e.g., filtering by chat.example.com would also match raw logs containing api.chat.example.com).

Given the retrieved learning that Rocket.Chat servers are always deployed at distinct origins, this substring collision scenario is unlikely in practice. If this ever becomes a concern, consider using word-boundary matching instead.


Walkthrough

Switches 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

Cohort / File(s) Summary
Context management refactoring
src/logging/context.ts
Removed anonymous server-<n> pool and MAX_SERVER_ID. Added getHost(url) and registerWebContentsServer(webContentsId, serverUrl). getServerContext() now returns a hostname or 'local'/'unknown'. cleanupServerContext simplified; removed getServerMappings.
Logging IPC & registration
src/logging/index.ts
console-log and related IPC handlers now derive host via getHost(...), match origin to configured servers, and call registerWebContentsServer(senderWebContents.id, matchedServer.url) instead of generating server-<n> tags; context construction order adjusted.
Server mapping generation (log viewer IPC)
src/logViewerWindow/ipc.ts
get-server-mapping now uses getHost(server.url) as mapping keys and skips servers without server.url; mapped values remain title with fallback to URL.
Log entry parsing & rendering
src/logViewerWindow/LogEntry.tsx
Replaced regex-based server-<n> extraction with findServerTag(context, serverMapping) that tokenizes context and matches tokens against serverMapping keys; contextWithoutServer now removes the matched token via token filtering.
Viewer filters & matching logic
src/logViewerWindow/logViewerWindow.tsx
Server filter options use serverMapping keys (hostnames) as Select values and display `name

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

type: refactor area: logging area: log-viewer

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title correctly describes the main change: fixing the log viewer server filter by replacing pool-based server IDs with hostname-based tags for stable filtering.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jeanfbrito jeanfbrito changed the title fix: use hostname-based server tags in log viewer filter fix: workspace tags in log viewer filter Apr 1, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between f5be646 and 6571e78.

📒 Files selected for processing (5)
  • src/logViewerWindow/LogEntry.tsx
  • src/logViewerWindow/ipc.ts
  • src/logViewerWindow/logViewerWindow.tsx
  • src/logging/context.ts
  • src/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/fuselage for all UI work and only create custom components when Fuselage doesn't provide what's needed
Check Theme.d.ts for 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.ts
  • src/logViewerWindow/logViewerWindow.tsx
  • src/logViewerWindow/LogEntry.tsx
  • src/logging/index.ts
  • src/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.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6571e78 and d52b332.

📒 Files selected for processing (2)
  • src/logViewerWindow/ipc.ts
  • src/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/fuselage for all UI work and only create custom components when Fuselage doesn't provide what's needed
Check Theme.d.ts for 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 → server association left, delete() on destroyed is enough.

- Rename webContentsToHostname → webContentsToHost to match URL.host usage
- Use token-based removal in LogEntry to avoid partial substring matches
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e583d90 and 9a2e821.

📒 Files selected for processing (2)
  • src/logViewerWindow/LogEntry.tsx
  • src/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/fuselage for all UI work and only create custom components when Fuselage doesn't provide what's needed
Check Theme.d.ts for 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.host is 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).origin alone 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.
@jeanfbrito jeanfbrito merged commit abb4376 into dev Apr 1, 2026
6 checks passed
@jeanfbrito jeanfbrito deleted the fix/log-viewer-server-filter branch April 1, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant