feat: current statement highlighting and large document safety caps#442
Merged
feat: current statement highlighting and large document safety caps#442
Conversation
- Highlight the background of the SQL statement under cursor using EmphasisManager (outline with fill). Debounced at 150ms with generation counter to prevent stale updates. - Skip highlighting for single statements (no semicolons), multi-cursor, or documents >5MB. - Add maxHighlightableLength (5MB) guard to Highlighter — documents exceeding this are shown as plain text. - Cap highlight chunks to 2 per cycle (8192 chars) for documents >50KB to keep the editor responsive on large SQL dumps. - Add currentStatementHighlight theme color (light: #F0F4FA, dark: #1A2332).
… scanner - CurrentStatementHighlighterTests: 9 tests for statement detection logic (multi-statement, single statement, strings, comments, edge cases) - ThemeDefinitionTests: 7 tests for currentStatementHighlight color (defaults, round-trip, backward compatibility) - SQLStatementScannerLocatedTests: 13 tests for locatedStatementAtCursor (offsets, comments, backticks, large input, edge cases) - Fix pre-existing TabDiskActorTests compile error (save() now throws)
|
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:
📝 WalkthroughWalkthroughChanges introduce document size safety caps for syntax highlighting (skip >5MB documents, throttle >50KB), extend the theme system with a new currentStatementHighlight color field, update async test methods to propagate errors, and add comprehensive test suites for SQL statement scanning and theme validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
- Clear emphasis immediately on text change (before debounced update) to avoid drawing a stale range that extends beyond the document - Validate NSMaxRange(stmtRange) <= docLength before adding emphasis
EmphasisManager creates a CATextLayer on top with black foreground color, which overwrites syntax highlighting and hides the cursor. Switch to NSTextStorage.addAttribute(.backgroundColor) which draws behind text, preserving syntax colors and cursor visibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
TableProTests/Core/Storage/TabDiskActorTests.swift (1)
61-64: Minor: Test methods markedthrowsbut contain no throwing calls.These test methods are declared
async throwsbut only call non-throwing APIs (load(),clear(),loadLastQuery()). This is harmless in Swift Testing but slightly imprecise. Consider keeping them as justasyncfor clarity, or leave as-is for consistency across the test suite.Also applies to: 103-105, 174-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Core/Storage/TabDiskActorTests.swift` around lines 61 - 64, Several test methods (e.g., loadReturnsNilForUnknown) are declared "async throws" but only call non-throwing APIs like actor.load(), actor.clear(), and actor.loadLastQuery(); change their signatures to just "async" (remove "throws") to reflect they don't throw and update any other test methods in the same test class that only call those non-throwing methods the same way.TableProTests/Core/Utilities/SQLStatementScannerLocatedTests.swift (1)
142-157: Consider adding a test to verify the 10k character highlight range cap.The
largeInputtest verifies that scanning doesn't crash on large inputs, which is good. However, the coding guidelines indicate that regex/highlight ranges should be capped at 10k characters for SQL dumps. Consider adding a test that verifies the statement scanner or highlighter properly limits the returned range for very long single statements.Based on learnings: "Cap regex/highlight ranges at 10k characters for SQL dumps which can have single lines with millions of characters."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Core/Utilities/SQLStatementScannerLocatedTests.swift` around lines 142 - 157, Add a test that asserts the scanner enforces the 10k character cap: in or alongside the existing largeInput test, create a very long single-statement SQL string (>10_000 chars), call SQLStatementScanner.locatedStatementAtCursor(in:cursorPosition:), then verify the returned located object enforces the cap by asserting located.sql.count (or the highlighted range length represented by located) is <= 10_000 and that located.offset stays within the expected window (>= 0 and < nsSQL.length). Use the same symbols from the diff (largeInput, SQLStatementScanner.locatedStatementAtCursor, located.sql, located.offset) so the test checks the 10k highlight/range limit for huge single-line statements.TablePro/Views/Editor/CurrentStatementHighlighter.swift (1)
88-113: Consider capping the highlight range to protect against extremely long single statements.The 5MB document cap (line 16) prevents highlighting huge documents, but a single SQL statement could still be very long (e.g., a multi-megabyte
INSERTwith many values). Per coding guidelines, highlight ranges should be capped at 10k characters for SQL dumps which can have single lines with millions of characters.Consider adding a check to skip highlighting or truncate the range when
stmtNS.lengthexceeds a threshold:🛡️ Suggested guard
let stmtNS = located.sql as NSString + // Skip highlighting extremely long statements to avoid performance issues + guard stmtNS.length <= 10_000 else { + clearHighlight() + return + } let stmtRange = NSRange(location: located.offset, length: stmtNS.length)Based on learnings: "Cap regex/highlight ranges at 10k characters for SQL dumps which can have single lines with millions of characters."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Editor/CurrentStatementHighlighter.swift` around lines 88 - 113, The highlight can blow up for extremely long single statements; inside the block after computing stmtNS and stmtRange (from SQLStatementScanner.locatedStatementAtCursor) add a guard to cap the range length (e.g., maxLen = 10_000): if stmtNS.length > maxLen either skip highlighting (call clearHighlight/return) or set stmtRange.length = maxLen and adjust emphasis range accordingly; ensure you compare/update lastHighlightedRange using the capped/truncated range and then create Emphasis with that capped range before calling textView.emphasisManager?.removeEmphases(for: Self.groupId) and addEmphases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Views/Editor/CurrentStatementHighlighter.swift`:
- Around line 88-113: The highlight can blow up for extremely long single
statements; inside the block after computing stmtNS and stmtRange (from
SQLStatementScanner.locatedStatementAtCursor) add a guard to cap the range
length (e.g., maxLen = 10_000): if stmtNS.length > maxLen either skip
highlighting (call clearHighlight/return) or set stmtRange.length = maxLen and
adjust emphasis range accordingly; ensure you compare/update
lastHighlightedRange using the capped/truncated range and then create Emphasis
with that capped range before calling
textView.emphasisManager?.removeEmphases(for: Self.groupId) and addEmphases.
In `@TableProTests/Core/Storage/TabDiskActorTests.swift`:
- Around line 61-64: Several test methods (e.g., loadReturnsNilForUnknown) are
declared "async throws" but only call non-throwing APIs like actor.load(),
actor.clear(), and actor.loadLastQuery(); change their signatures to just
"async" (remove "throws") to reflect they don't throw and update any other test
methods in the same test class that only call those non-throwing methods the
same way.
In `@TableProTests/Core/Utilities/SQLStatementScannerLocatedTests.swift`:
- Around line 142-157: Add a test that asserts the scanner enforces the 10k
character cap: in or alongside the existing largeInput test, create a very long
single-statement SQL string (>10_000 chars), call
SQLStatementScanner.locatedStatementAtCursor(in:cursorPosition:), then verify
the returned located object enforces the cap by asserting located.sql.count (or
the highlighted range length represented by located) is <= 10_000 and that
located.offset stays within the expected window (>= 0 and < nsSQL.length). Use
the same symbols from the diff (largeInput,
SQLStatementScanner.locatedStatementAtCursor, located.sql, located.offset) so
the test checks the 10k highlight/range limit for huge single-line statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 926c4196-2811-43ea-8721-abd4d36b2594
📒 Files selected for processing (11)
CHANGELOG.mdLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/Highlighting/HighlightProviding/HighlightProviderState.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/Highlighting/Highlighter.swiftTablePro/Theme/ResolvedThemeColors.swiftTablePro/Theme/ThemeDefinition.swiftTablePro/Views/Editor/CurrentStatementHighlighter.swiftTablePro/Views/Editor/SQLEditorCoordinator.swiftTableProTests/Core/Storage/TabDiskActorTests.swiftTableProTests/Core/Utilities/SQLStatementScannerLocatedTests.swiftTableProTests/Theme/ThemeDefinitionTests.swiftTableProTests/Views/Editor/CurrentStatementHighlighterTests.swift
…storage attribute NSTextStorage .backgroundColor conflicts with the syntax highlighting pipeline (Highlighter.setAttributes overwrites it) and stacks visually with the editor's selectedLineBackgroundColor. Using a CALayer with zPosition: -1 draws behind text, preserving syntax colors, cursor, and current line highlight independently.
The editor's selectedLineBackgroundColor stacks with our statement background, creating a double-highlight on the cursor line. Save and clear the line highlight color when statement highlighting is active, restore it when inactive (single statement or cleared).
# Conflicts: # CHANGELOG.md
…caps) The statement highlighting fought CodeEditSourceEditor's rendering pipeline at every turn — EmphasisManager overwrites text colors, NSTextStorage attributes conflict with the highlighter, CALayer stacks with current-line highlight. The feature needs native CESS support (TextSelectionManager background ranges) to work correctly. Kept: large document safety caps (5MB skip, 50KB throttle), theme color definition, scanner located tests, theme definition tests.
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
1 participant
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.
Summary
Current statement highlighting
EmphasisManagerwith.outline(color:, fill: true)style — same system as find panel and bracket matching;), multi-cursor, empty doc, or document >5MBcurrentStatementHighlighttheme color (light:#F0F4FA, dark:#1A2332)Large document safety caps
Highlighter.maxHighlightableLength(5MB) — documents exceeding this are shown as plain textHighlightProviderStatecaps to 2 chunks per cycle (8192 chars) for documents >50KBTests
CurrentStatementHighlighterTests(9 tests): multi-statement, strings, comments, edge casesThemeDefinitionTests(7 tests): defaults, Codable round-trip, backward compatibilitySQLStatementScannerLocatedTests(13 tests): offsets, comments, backticks, large inputTabDiskActorTestscompile error (save()now throws)Test plan
SELECT 1; SELECT 2; SELECT 3;— background highlight follows cursor;) — no highlightSummary by CodeRabbit
New Features
Improvements