fix: guard against empty viewport range in diff editor gutter clip (fixes #316032)#316038
Open
vs-code-engineering[bot] wants to merge 1 commit into
Open
fix: guard against empty viewport range in diff editor gutter clip (fixes #316032)#316038vs-code-engineering[bot] wants to merge 1 commit into
vs-code-engineering[bot] wants to merge 1 commit into
Conversation
…ixes #316032) The guard at line 312 checked that preferredParentRange was non-empty but did not check preferredViewPortRange. When preferredViewPortRange had start === endExclusive (empty range), it passed the truthiness check but OffsetRange.clip() threw 'Invalid clipping range'. Also use the idiomatic .isEmpty property instead of manual start < end comparison for both ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
🔧 Error Fix
Summary
Invalid clipping range: [37, 37)5c896f3a-1b03-ede7-83e9-9035cdc4464dThe diff editor gutter layout code calls
OffsetRange.clip()onpreferredViewPortRangewithout first verifying that the range is non-empty.OffsetRange.tryCreate()returnsundefinedonly whenstart > endExclusive, but returns an empty range (start === endExclusive) as a valid object. The existing guard checked nullability and thatpreferredParentRangewas non-empty, but missed checkingpreferredViewPortRangefor emptiness. WhenpreferredViewPortRangeis empty (e.g.,[37, 37)),clip()throwsBugIndicatingError.Fixes #316032
Recommended reviewer:
@hedietCulprit Commit
This is a long-standing bug, not a recent regression. The incomplete guard has been present since the gutter feature was originally authored by
@hediet. The error appears across multiple versions (1.112.0 through 1.119.0).Code Flow
graph TD A["ResizeObserver callback"] --> B["editorGutter.ts: render()"] B --> C["editorGutter.ts: transaction()"] C --> D["gutterFeature.ts: layout() :313"] D --> E{"preferredViewPortRange non-null?"} E -->|yes| F{"preferredParentRange non-empty?"} F -->|yes| G["preferredViewPortRange.clip() :313"] G --> H["offsetRange.ts: clip() :170"] H --> I{"this.isEmpty?"} I -->|yes - BUG| J["throw BugIndicatingError"]Affected Files
src/vs/editor/browser/widget/diffEditor/features/gutterFeature.ts— missing emptiness check forpreferredViewPortRangesrc/vs/editor/common/core/ranges/offsetRange.ts—clip()correctly throws on empty range (not a bug here)Repro Steps
endExclusive - margin - itemHeightto equalmargin)preferredViewPortRangeis created as an empty range[N, N)clip()is called on the empty range, throwing the errorHow the Fix Works
Chosen approach (
src/vs/editor/browser/widget/diffEditor/features/gutterFeature.ts): Added!preferredViewPortRange.isEmptyto the guard condition at line 312, alongside the existingpreferredParentRangenon-empty check. Also replaced the manualstart < endExclusivecomparison with the idiomatic.isEmptyproperty for both ranges. This fixes at the data producer (the guard that decides whether to callclip()), not the crash site (clip()itself), following the principle of fixing where invalid data flows into the throwing function.Recommended Owner
@hediet— author and maintainer of the diff editor gutter feature code.errors-fix-driver — cycle 11
Trigger: cron_review_comments · Head:
c61f61b81c49f0337860f6a968a0f484c167b130(c61f61b)blocked(awaiting human approval)Push: no (nothing to fix) · Copilot rerequested: no (no push)
Ready gate: PR already marked ready; awaiting human approval from
@hediet.errors-fix-driver — cycle 12
Trigger: cron_review_comments · Head:
c61f61b81c49f0337860f6a968a0f484c167b130(c61f61b)blocked(awaiting human approval)Push: no (nothing to fix) · Copilot rerequested: no (no push)
Ready gate: PR already marked ready; awaiting human approval from
@hediet.errors-fix-driver — cycle 13
Trigger: cron_review_comments · Head:
c61f61b81c49f0337860f6a968a0f484c167b130(c61f61b)blocked(awaiting human approval)Push: no (nothing to fix) · Copilot rerequested: no (no push)
Ready gate: PR already marked ready; awaiting human approval from
@hediet.