Repository navigation
Conversation
- Add TerminalOutputFilterChain for managing output filters - Implement OSC response filter (filters color queries, DA, CPR) - Filter applied before display only (scrollback preserves raw data) - Fixes display of unwanted sequences like '11;rgb:1a1a/1a1a/1a1a1R'
📝 WalkthroughWalkthroughThis PR introduces a terminal output filter chain system that registers filters during app startup and applies them to terminal output before display. Specifically, it registers an OSC response filter that strips terminal query/response escape sequences, integrated into the session data handling pipeline. Changes
Sequence DiagramsequenceDiagram
participant App as Application<br/>Startup
participant Main as Main Process<br/>(index.ts)
participant Chain as TerminalOutputFilterChain
participant OSCFilter as OSC Response<br/>Filter
participant Session as Terminal<br/>Session
participant Display as Display/Batcher
App->>Main: Application starts
Main->>Main: registerDefaultFilters()
Main->>OSCFilter: Create oscResponseFilter
OSCFilter->>Chain: register(oscResponseFilter)
Chain->>Chain: Store filter by id
Note over Chain: Filter chain ready
Session->>Session: Terminal data received
Session->>Chain: apply(rawData)
Chain->>OSCFilter: filter(data)
OSCFilter->>OSCFilter: Strip OSC sequences<br/>via regex
OSCFilter-->>Chain: Filtered data
Chain-->>Session: Result
Session->>Display: Write filtered data<br/>to batcher
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
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: 2
🧹 Nitpick comments (3)
apps/desktop/src/main/index.ts (1)
17-20: Good placement for filter registration.The
registerDefaultFilters()call is correctly positioned at the module level after Sentry initialization but before app startup. The synchronous registration ensures filters are ready before any terminal sessions are created.Consider wrapping the call in a try-catch block to prevent startup failures if filter registration encounters issues:
🔎 Optional defensive error handling
-registerDefaultFilters(); +try { + registerDefaultFilters(); +} catch (error) { + console.error("[main] Failed to register terminal output filters:", error); + // App can continue without filters, but log the failure +}This follows the pattern already established in the codebase (lines 229-234) for optional initialization steps that shouldn't block app startup.
apps/desktop/src/main/lib/terminal-output-filter.ts (2)
14-19: Add logging for duplicate filter registration.The method silently returns when a duplicate filter ID is detected, which could hide configuration issues. According to the coding guidelines, use prefixed console logging with context.
🔎 Proposed fix to add logging
register(filter: TerminalOutputFilter): void { if (this.filters.some((f) => f.id === filter.id)) { + console.warn(`[terminal-output-filter/register] Duplicate filter ID "${filter.id}" - skipping registration`); return; } this.filters.push(filter); + console.debug(`[terminal-output-filter/register] Registered filter: ${filter.id}`); }
21-28: Consider adding logging for unregister operations.While the boolean return value indicates success, adding debug logging would help troubleshoot filter lifecycle issues, consistent with the logging pattern suggested for
register().🔎 Proposed addition
unregister(filterId: string): boolean { const index = this.filters.findIndex((f) => f.id === filterId); if (index === -1) { + console.debug(`[terminal-output-filter/unregister] Filter not found: ${filterId}`); return false; } this.filters.splice(index, 1); + console.debug(`[terminal-output-filter/unregister] Unregistered filter: ${filterId}`); return true; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/terminal-output-filter.test.tsapps/desktop/src/main/lib/terminal-output-filter.tsapps/desktop/src/main/lib/terminal-output-filters/index.tsapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.tsapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.tsapps/desktop/src/main/lib/terminal/session.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.tsapps/desktop/src/main/lib/terminal-output-filter.test.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/terminal-output-filters/index.tsapps/desktop/src/main/lib/terminal-output-filter.tsapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.tsapps/desktop/src/main/lib/terminal/session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.tsapps/desktop/src/main/lib/terminal-output-filter.test.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/terminal-output-filters/index.tsapps/desktop/src/main/lib/terminal-output-filter.tsapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.tsapps/desktop/src/main/lib/terminal/session.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with implementation files using .test.ts or .test.tsx suffix
Files:
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.tsapps/desktop/src/main/lib/terminal-output-filter.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.tsapps/desktop/src/main/lib/terminal-output-filter.test.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/terminal-output-filters/index.tsapps/desktop/src/main/lib/terminal-output-filter.tsapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.tsapps/desktop/src/main/lib/terminal/session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.tsapps/desktop/src/main/lib/terminal-output-filter.test.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/terminal-output-filters/index.tsapps/desktop/src/main/lib/terminal-output-filter.tsapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.tsapps/desktop/src/main/lib/terminal/session.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (4)
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/main/index.ts
🧬 Code graph analysis (6)
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.ts (2)
apps/desktop/src/main/lib/terminal-output-filters/index.ts (1)
oscResponseFilter(8-8)apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.ts (1)
oscResponseFilter(22-33)
apps/desktop/src/main/lib/terminal-output-filter.test.ts (1)
apps/desktop/src/main/lib/terminal-output-filter.ts (2)
TerminalOutputFilterChain(11-48)TerminalOutputFilter(5-9)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/terminal-output-filters/index.ts (1)
registerDefaultFilters(4-6)
apps/desktop/src/main/lib/terminal-output-filters/index.ts (2)
apps/desktop/src/main/lib/terminal-output-filter.ts (1)
terminalOutputFilterChain(50-50)apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.ts (1)
oscResponseFilter(22-33)
apps/desktop/src/main/lib/terminal-output-filter.ts (1)
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.ts (1)
filter(26-32)
apps/desktop/src/main/lib/terminal/session.ts (1)
apps/desktop/src/main/lib/terminal-output-filter.ts (1)
terminalOutputFilterChain(50-50)
🪛 ast-grep (0.40.3)
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.ts
[warning] 6-9: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
${ESC}\\]1[0-2];rgb:[0-9a-fA-F]{4}/[0-9a-fA-F]{4}/[0-9a-fA-F]{4}(?:${BEL}|${ESC}\\\\),
"g",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 12-12: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${ESC}\\[\\?[0-9;]*c, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 15-15: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${ESC}\\[[0-9]+;[0-9]+R, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (12)
apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.ts (1)
1-85: LGTM! Comprehensive test coverage for OSC response filtering.The test suite is well-structured and covers all the key scenarios:
- OSC color responses with different terminators (BEL and ST)
- Partial OSC sequences without proper framing
- Device Attributes (DA1/DA2) responses
- Cursor Position Reports (CPR)
- Mixed content scenarios
- Preservation of valid ANSI color codes and escape sequences
The test organization using nested
describeblocks provides clear documentation of the filter's behavior, and the use of module-level constants (ESC, BEL) improves readability.apps/desktop/src/main/lib/terminal-output-filter.test.ts (1)
1-125: LGTM! Thorough test coverage for TerminalOutputFilterChain.The test suite comprehensively validates the filter chain's behavior:
- Registration and deduplication logic
- Unregistration with proper return values
- Filter application order and composition
- Early termination optimization when data becomes empty (lines 79-99)
- Empty chain pass-through behavior
The use of
beforeEachensures proper test isolation, and the test cases cover both happy paths and edge cases effectively.apps/desktop/src/main/lib/terminal-output-filters/index.ts (1)
1-8: LGTM! Clean and extensible filter registration pattern.The module provides a clear API for registering default filters and makes
oscResponseFilteravailable for external use. The implementation is straightforward and follows a pattern that makes it easy to add additional default filters in the future by simply adding moreregister()calls inregisterDefaultFilters().apps/desktop/src/main/lib/terminal/session.ts (2)
10-10: Correct import for filter chain integration.The import of
terminalOutputFilterChainenables the filtering functionality while maintaining proper module boundaries.
172-173: Well-designed data flow that preserves raw scrollback while filtering display output.The implementation correctly applies filtering to the original PTY data (
data) before display, while preserving unfiltered data (dataToStore) for scrollback and port scanning. This ensures:
- Scrollback integrity: Raw terminal output is preserved in
session.scrollbackandhistoryWriterfor accurate recovery- Clean display: Terminal query responses (OSC, DA, CPR) are filtered out before reaching xterm via
dataBatcher- Port scanning accuracy: Port detection operates on unfiltered data to catch all output patterns
- Clear sequence handling: Clear scrollback sequences are handled independently and don't interfere with display filtering
The separation of concerns between storage (
dataToStore) and display (dataToDisplay) is exactly what the PR objectives describe.apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.ts (4)
3-4: LGTM: Clean constant definitions.The ESC and BEL constants improve readability and maintainability of the regex patterns.
22-33: LGTM: Clean filter implementation.The filter method correctly chains replacements to remove OSC, DA, CPR, and partial OSC sequences. The implementation is straightforward and matches the documented behavior.
Note: While the coding guidelines suggest prefixed console logging, logging every filter invocation would be excessive. Consider adding logging at the registration level instead (in the index.ts or during startup).
6-20: The(?:1R)?suffix in PARTIAL_OSC_COLOR is intentional and correct. The test case "should filter partial OSC response with trailing 1R" at line 34-37 explicitly validates filtering of sequences like"11;rgb:1a1a/1a1a/1a1a1R". The1Rrepresents corrupted terminal output where CPR response data (which ends with the standardRterminator) becomes interleaved with partial OSC color sequences—a legitimate edge case for defensive filtering.
6-16: Use alias import from tsconfig.json instead of relative path.The ReDoS concerns are indeed low risk with static string constants and controlled PTY input. However, the import on line 1 violates coding guidelines: it should use the alias defined in
apps/desktop/tsconfig.json("*": ["src/*"]) instead of the relative path. Changeimport type { TerminalOutputFilter } from "../terminal-output-filter";toimport type { TerminalOutputFilter } from "main/lib/terminal-output-filter";Likely an incorrect or invalid review comment.
apps/desktop/src/main/lib/terminal-output-filter.ts (3)
1-9: LGTM: Well-designed interface.The
TerminalOutputFilterinterface provides a clean, extensible contract with readonly properties preventing accidental mutation.
41-47: LGTM: Clean accessor and lifecycle methods.Both
getRegisteredFilters()andclear()provide essential lifecycle management with straightforward implementations.
50-50: LGTM: Appropriate singleton pattern.Exporting a singleton instance enables centralized filter chain management across the application, which aligns with the design goals stated in the PR objectives.
| apply(data: string): string { | ||
| let result = data; | ||
| for (const filter of this.filters) { | ||
| result = filter.filter(result); | ||
| if (result.length === 0) { | ||
| break; | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent filter chain failures.
If any filter throws an exception, the entire chain fails silently with no indication of which filter caused the problem. According to the coding guidelines, errors should never be swallowed silently.
🔎 Proposed fix with error handling
apply(data: string): string {
let result = data;
for (const filter of this.filters) {
- result = filter.filter(result);
+ try {
+ result = filter.filter(result);
+ } catch (error) {
+ console.error(`[terminal-output-filter/apply] Filter "${filter.id}" threw error:`, error);
+ // Continue with other filters or rethrow depending on desired behavior
+ // For now, continue to make the chain resilient
+ }
if (result.length === 0) {
break;
}
}
return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| apply(data: string): string { | |
| let result = data; | |
| for (const filter of this.filters) { | |
| result = filter.filter(result); | |
| if (result.length === 0) { | |
| break; | |
| } | |
| } | |
| return result; | |
| } | |
| apply(data: string): string { | |
| let result = data; | |
| for (const filter of this.filters) { | |
| try { | |
| result = filter.filter(result); | |
| } catch (error) { | |
| console.error(`[terminal-output-filter/apply] Filter "${filter.id}" threw error:`, error); | |
| // Continue with other filters or rethrow depending on desired behavior | |
| // For now, continue to make the chain resilient | |
| } | |
| if (result.length === 0) { | |
| break; | |
| } | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/terminal-output-filter.ts around lines 30 to 39,
wrap each call to filter.filter(result) in a try/catch so a thrown exception
from one filter is not swallowed silently; in the catch log/report the error
with identifying info (filter index and filter.constructor.name or a provided
name) and the error stack, then skip the failing filter and continue the chain
using the previous result (do not let one filter break the whole pipeline).
| if (result.length === 0) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Early break on empty string may prevent valid filter compositions.
The optimization that breaks when result.length === 0 assumes no filter needs to process or restore content from an empty string. This could prevent filter compositions where:
- Filter A removes all content temporarily
- Filter B needs to add content back or process the empty state
Consider whether this early exit aligns with your filtering requirements, or if filters should always run regardless of intermediate results.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/terminal-output-filter.ts around lines 34 to 36,
the loop currently breaks when result.length === 0 which prevents later filters
from running and blocks compositions where a later filter might reintroduce or
process content; remove the early break so the loop continues applying remaining
filters even on an empty string (or, if you need performance control, replace
the unconditional break with a per-filter skip flag or check so only filters
that opt out are skipped while others still run).
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Summary
TerminalOutputFilterChainmechanism for filtering terminal output before displayWhy / Context
Terminal emulators respond to certain escape sequences with response strings. For example, when a shell or program queries the background color with
\e]11;?\a, xterm.js responds with\e]11;rgb:RRRR/GGGG/BBBB\e\. These responses were being echoed back to the display, showing noise like11;rgb:1a1a/1a1a/1a1a1Rin the terminal output.How It Works
terminal-output-filter.ts): DefinesTerminalOutputFilterinterface andTerminalOutputFilterChainclassterminal-output-filters/osc-response-filter.ts): Filters:session.ts): Applies filter chain before writing todataBatcher(display), not before writing to scrollbackData flow:
Manual QA Checklist
11;rgb:1a1a/1a1a/1a1a1Rno longer appear in terminal outputlsoutput)Testing
bun run typecheck✅bun test apps/desktop/src/main/lib/terminal-output-filter.test.ts✅ (22 tests)bun test apps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.ts✅Extending
To add a new filter:
Register in
terminal-output-filters/index.ts:New Files
apps/desktop/src/main/lib/terminal-output-filter.ts- Core filter mechanismapps/desktop/src/main/lib/terminal-output-filter.test.ts- Testsapps/desktop/src/main/lib/terminal-output-filters/index.ts- Registrationapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.ts- OSC filterapps/desktop/src/main/lib/terminal-output-filters/osc-response-filter.test.ts- OSC filter testsSummary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.