Add CodeMirror support to Rules tab with syntax highlighting, line number, search and replace...#559
Add CodeMirror support to Rules tab with syntax highlighting, line number, search and replace...#559gunir wants to merge 8 commits intoZenPrivacy:masterfrom
Conversation
This PR add CodeMirror support to improve performance, improve rule editing experience Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
Update index.css - Add CodeMirror Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
WalkthroughReplaces the textarea-based Rules editor with a CodeMirror 6 editor, adds adblock syntax highlighting, search, line numbers, oneDark theme, debounced GetRules/SetRules persistence, a Format action preserving comment lines, readOnly toggling tied to proxy state, and CSS/layout updates with an initial loading state. Changes
Sequence DiagramsequenceDiagram
participant User
participant RulesComp as Rules Component
participant CM as CodeMirror
participant API as GetRules/SetRules
participant State as App State
User->>RulesComp: mount
RulesComp->>API: fetch initial rules
API-->>RulesComp: return rules
RulesComp->>CM: initialize with rules
RulesComp->>State: subscribe isProxyRunning
User->>CM: edit
CM-->>RulesComp: onChange event
RulesComp->>RulesComp: debounce update
RulesComp->>API: SetRules (persist)
API-->>RulesComp: ack
State->>RulesComp: isProxyRunning changes
RulesComp->>CM: update readOnly (Compartment)
User->>RulesComp: click Format
RulesComp->>RulesComp: dedupe & sort (preserve !comments)
RulesComp->>CM: apply formatted content
RulesComp->>API: SetRules (persist)
API-->>RulesComp: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/src/Rules/index.css (1)
42-59: Consider using CSS variables instead of hardcoded colors.The CodeMirror styling uses hardcoded hex colors and multiple
!importantdeclarations. This approach:
- Tightly couples the editor to the One Dark theme
- Makes it difficult to support theme switching or design system updates
- Could cause maintenance issues with excessive
!importantusageConsider defining CSS variables for these colors at the root level and referencing them here for better maintainability and theme flexibility.
🔎 Proposed refactor using CSS variables
Define variables (e.g., in a root/theme file):
:root { --editor-bg: #282c34; --editor-gutter-bg: #21252b; --editor-border: #333; --editor-line-number: #606873; --editor-text: #abb2bf; --editor-input-bg: #21252b; --editor-search-border: #181a1f; }Then update this file:
/* CodeMirror UI Enhancements */ -.cm-editor { height: 100% !important; } -.cm-gutters { background-color: #21252b !important; border-right: 1px solid #333 !important; } -.cm-lineNumbers .cm-gutterElement { padding: 0 8px !important; color: #606873 !important; } +.cm-editor { height: 100%; } +.cm-gutters { background-color: var(--editor-gutter-bg); border-right: 1px solid var(--editor-border); } +.cm-lineNumbers .cm-gutterElement { padding: 0 8px; color: var(--editor-line-number); } /* Search Panel Styling to match One Dark */ .cm-search { - background: #282c34 !important; - color: #abb2bf !important; - border-bottom: 1px solid #181a1f !important; - padding: 8px !important; + background: var(--editor-bg); + color: var(--editor-text); + border-bottom: 1px solid var(--editor-search-border); + padding: 8px; } .cm-search input { - background: #21252b !important; - color: white !important; - border: 1px solid #181a1f !important; + background: var(--editor-input-bg); + color: white; + border: 1px solid var(--editor-search-border); border-radius: 3px; }frontend/src/Rules/index.tsx (1)
31-50: LGTM - Basic adblock syntax highlighting is functional.The custom highlighter correctly identifies comments (lines starting with
!), exception rules (@@), and domain anchors (||). The implementation is straightforward and appropriate for the PR's goals.For future enhancement, consider adding patterns for other common adblock syntax elements like
|(pipe),^(separator),$(options), and wildcards (*) to provide more comprehensive highlighting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/Rules/index.css(1 hunks)frontend/src/Rules/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/Rules/index.tsx (3)
frontend/src/context/ProxyStateContext.tsx (1)
useProxyState(30-36)frontend/wailsjs/go/cfg/Config.js (2)
GetRules(49-51)SetRules(109-111)frontend/wailsjs/runtime/runtime.js (1)
BrowserOpenURL(176-178)
🔇 Additional comments (11)
frontend/src/Rules/index.css (4)
3-9: LGTM - Container layout properly configured for CodeMirror.The changes to full height/width with overflow control align well with the CodeMirror integration requirements mentioned in the PR objectives.
11-17: LGTM - Header flex layout is well structured.The flex-shrink: 0 is a good defensive measure to prevent the header from collapsing when the editor content grows.
19-31: LGTM - Hint and tooltip styling is appropriate.The monospace font for the hint text aligns well with the editor's aesthetic.
33-40: LGTM - Editor container properly configured.The overflow: hidden is particularly important for controlling CodeMirror's scroll behavior within the Wails webview, as mentioned in the PR objectives.
frontend/src/Rules/index.tsx (7)
1-27: LGTM - Imports are well-organized and comprehensive.The CodeMirror imports cover all necessary modules for the editor functionality described in the PR objectives.
52-65: LGTM - Component state and initialization are well-structured.The use of
initialRuleswith null initial state enables proper loading state handling. The Compartment memoization is appropriate for managing dynamic readonly state.
67-71: LGTM - Debounced persistence is properly implemented.The 500ms debounce is a good balance between responsiveness and backend load. The processing logic correctly filters out empty lines.
73-97: LGTM - Format logic is correct and efficient.The implementation properly:
- Deduplicates using Set
- Preserves comments at the top
- Sorts rules alphabetically with localeCompare
- Updates the editor in a single transaction
132-138: LGTM - Readonly state synchronization is correctly implemented.The Compartment pattern is the proper approach for dynamically reconfiguring editor state in CodeMirror 6.
163-173: LGTM - Tooltip and editor container properly configured.The tooltip correctly disables when the proxy is not running, and the editor container ref is properly managed.
99-130: The editor initialization effect is properly structured with correct CodeMirror 6 configuration.The useEffect correctly:
- Guards against duplicate initialization with
editorRef.current && !viewRef.currentchecks- Configures comprehensive CodeMirror extensions including search with the correct
top: trueparameter- Integrates the update listener to persist filter changes via
setFilters- Cleans up the editor view on unmount
The single
[initialRules]dependency is appropriate since the effect should run only once after initial data load.
anfragment
left a comment
anfragment
left a comment
There was a problem hiding this comment.
Please see the failing builds and the frontend linter's output (automatically fixable with npm run lint -- --fix).
gunir
left a comment
gunir
left a comment
There was a problem hiding this comment.
I see, I'll look into it later!
.rules--loading {
display: flex;
align-items: center;
justify-content: center;
height: 100%;
width: 100%;
}
Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/Rules/index.tsx (1)
140-142: Loading state CSS class issue already flagged.This issue was already raised in previous review comments, and you acknowledged it. The
.rules--loadingCSS class needs to be added tofrontend/src/Rules/index.css.
🧹 Nitpick comments (1)
frontend/src/Rules/index.tsx (1)
34-50: Consider optimizing the tokenizer for better performance.The catch-all case at line 42 uses
stream.next()which advances only one character at a time. For long sequences of regular characters (not comments,@@, or||), this calls the tokenizer once per character. Consider consuming all non-special characters in a single call:if (stream.match(/^[^!@|]+/)) return null;This would reduce the number of tokenizer invocations for typical adblock rules.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/Rules/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/Rules/index.tsx (3)
frontend/src/context/ProxyStateContext.tsx (1)
useProxyState(30-36)frontend/wailsjs/go/cfg/Config.js (2)
GetRules(49-51)SetRules(109-111)frontend/wailsjs/runtime/runtime.js (1)
BrowserOpenURL(176-178)
🔇 Additional comments (5)
frontend/src/Rules/index.tsx (5)
73-97: Format logic looks solid.The deduplication and sorting logic correctly:
- Removes duplicates using a Set
- Preserves comment lines (starting with
!) at the top- Sorts remaining rules alphabetically
- Dispatches the changes to replace the entire document
99-130: Editor initialization and lifecycle management are correct.The effect properly:
- Waits for initial rules to load before creating the editor
- Initializes CodeMirror with appropriate extensions (line numbers, search, syntax highlighting, etc.)
- Wires up the debounced persistence via
updateListener- Cleans up by destroying the view on unmount
The dependency array
[initialRules]is intentionally narrow since other captured values (setFilters,readOnlyCompartment) are stable references, andisProxyRunningchanges are handled by a dedicated effect.
132-138: ReadOnly state management is correct.The effect properly reconfigures the editor's readonly state in response to
isProxyRunningchanges. The inclusion ofreadOnlyCompartmentin the dependency array is technically redundant (it's stable fromuseMemo), but harmless.
144-175: UI rendering structure is well organized.The component structure is clear:
- Header with Help and Format buttons using
ButtonGroup- Search hint for discoverability
- Tooltip that explains readonly state when proxy is running
- Editor container properly ref'd and styled with conditional disabled class
110-110: Thetop: trueparameter is not a valid option for thesearch()function.The search() function's only documented configuration option is
createPanel, which is a callback function that returns an object with{ dom, top: true }. Thetopproperty controls panel positioning and belongs in the return value of the createPanel callback, not as a direct parameter to search().The current code
search({ top: true })either has no effect or may cause unexpected behavior. Either remove the parameter to use the default search panel, or implement a propercreatePanelcallback if custom panel positioning is needed.Likely an incorrect or invalid review comment.
Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/Rules/index.tsx (1)
142-144: Missing CSS class for loading state.As noted in previous reviews, the
.rules--loadingclass is not defined in the CSS file. Please add the CSS rule to properly style the loading spinner.
🧹 Nitpick comments (1)
frontend/src/Rules/index.tsx (1)
97-131: RemoveisProxyRunningfrom the effect dependencies.Including
isProxyRunningin the dependencies causes the editor to be destroyed and recreated whenever the proxy state changes. This is unnecessary because the read-only state is already handled separately via compartment reconfiguration in the effect at lines 133-140. Removing it will prevent unnecessary editor recreation and improve performance.🔎 Proposed fix
return () => { if (viewRef.current) { viewRef.current.destroy(); viewRef.current = null; } }; - }, [initialRules, isProxyRunning, readOnlyCompartment, setFilters]); + }, [initialRules, readOnlyCompartment, setFilters]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/Rules/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/Rules/index.tsx (3)
frontend/src/context/ProxyStateContext.tsx (1)
useProxyState(30-36)frontend/wailsjs/go/cfg/Config.js (2)
GetRules(49-51)SetRules(109-111)frontend/wailsjs/runtime/runtime.js (1)
BrowserOpenURL(176-178)
🔇 Additional comments (2)
frontend/src/Rules/index.tsx (2)
71-95: LGTM! Format logic is well-implemented.The deduplication and sorting logic correctly handles comment preservation and uses locale-aware comparison for internationalized sorting.
133-140: LGTM! Efficient read-only state management.Using compartment reconfiguration to update the read-only state is the correct approach and avoids unnecessary editor recreation.
Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/Rules/index.tsx (2)
148-150: The missing.rules--loadingCSS class was already flagged in a previous review. Please ensure it's added toindex.cssas discussed.
169-169: The hardcoded search hint was already flagged in a previous review for translation. Additionally, consider detecting the platform to show the correct modifier key (Cmd on macOS, Ctrl on Windows/Linux) for better UX.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/Rules/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/Rules/index.tsx (3)
frontend/src/context/ProxyStateContext.tsx (1)
useProxyState(30-36)frontend/wailsjs/go/cfg/Config.js (2)
GetRules(49-51)SetRules(109-111)frontend/wailsjs/runtime/runtime.js (1)
BrowserOpenURL(176-178)
🔇 Additional comments (3)
frontend/src/Rules/index.tsx (3)
139-146: LGTM!The compartment reconfiguration pattern is the correct approach for updating CodeMirror state dynamically without recreating the editor.
24-43: Nice adblock syntax highlighter implementation.The StreamLanguage definition cleanly handles comment lines, exception markers (
@@), and domain anchors (||). The tokenTable mapping to lezer highlight tags integrates well with the oneDark theme.
77-101: Format logic is well-implemented.The deduplication via
Set, comment-first sorting, and use oflocaleComparefor stable ordering are all correct. The null check onviewRef.currentis a good defensive practice.
Signed-off-by: Gunir <134402102+gunir@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/Rules/index.tsx (1)
66-75: LGTM! Error handling in place.The debounced save callback now includes proper error handling (lines 67-74) that logs failures. The comment on line 73 suggests adding a user-facing toast notification, which would improve UX but is not critical since the error is logged.
If you'd like, I can help implement a toast notification to surface save failures to users.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/Rules/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/Rules/index.tsx (3)
frontend/src/context/ProxyStateContext.tsx (1)
useProxyState(30-36)frontend/wailsjs/go/cfg/Config.js (2)
GetRules(49-51)SetRules(109-111)frontend/wailsjs/runtime/runtime.js (1)
BrowserOpenURL(176-178)
🔇 Additional comments (4)
frontend/src/Rules/index.tsx (4)
1-21: LGTM! Clean CodeMirror 6 integration setup.All imports are correctly structured for the CodeMirror 6 editor integration. The BlueprintJS components, React hooks, and CodeMirror modules are properly imported.
24-43: LGTM! Well-implemented custom syntax highlighter.The adblock language definition correctly uses CodeMirror's
StreamLanguage.definepattern with proper token parsing and Lezer tag mapping. The comment detection (lines starting with "!"), keyword highlighting ("@@"), and operator highlighting ("||") follow the correct stream-based parsing approach.
46-51: LGTM! Proper state and ref initialization.The state setup correctly uses
useMemoto create a stableCompartmentinstance (line 51), which is the recommended pattern for dynamic CodeMirror configuration. All refs and state are properly typed.
54-64: LGTM! Error handling properly implemented.The fetch effect now includes proper error handling (lines 56-62) that prevents infinite loading by falling back to an empty editor if
GetRules()fails. This correctly addresses the issue raised in previous reviews.
anfragment
left a comment
anfragment
left a comment
There was a problem hiding this comment.
@gunir Please take a look at the remaining linting and build errors.
The PR should also include the updated package.json and package-lock.json.
2 participants
What does this PR do?
CodeMirror is a versatile, modern text editor component built specifically for the web, designed to handle complex editing tasks that go far beyond the capabilities of a standard HTML <textarea>. While a plain textarea is part of the basic browser toolkit, CodeMirror 6 (the version we implemented) is a complete rewrite focused on modularity, accessibility, and high performance.
Here is a comparison between CodeMirror and the previous plain textarea used in your Vite/Wails project:
Performance and Rendering
Editing Features
Layout and Feel
How did you verify your code works?
Check this image:
What are the relevant issues?
No, just improvement