feat(camera): add wheel intent resolver and fix layer DPR observation#302
Open
draedful wants to merge 8 commits into
Open
feat(camera): add wheel intent resolver and fix layer DPR observation#302draedful wants to merge 8 commits into
draedful wants to merge 8 commits into
Conversation
Replace device detection with pan/zoom intent rules keyed on integer PIXEL deltas for trackpad and fractional deltas for mouse, with tests and updated docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Apply integer deltaMode=0 trackpad heuristics on all platforms, document deltaMode rules, and add debug signal plus tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Classify trackpad vs mouse from deltaMode and delta shape only; drop OS priors and related public API. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename slow integer trackpad rule id, expand test coverage, align camera docs with resolver behavior, and remove enableWheelIntentDebug from the story. Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer's GuideRefactors camera wheel handling to use a new intent-based resolver (pan vs zoom) instead of device detection, adds a configurable wheel-intent hook on graph settings, updates docs/Storybook/e2e tests, and fixes DPR observation usage in layers. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
wheelIntent.tsresolver is quite large and stateful; consider extracting the rule evaluation (I1–I5) into a smaller strategy table or clearly separated helper functions to make future tuning or bugfixes to specific rules less error-prone. TResolveWheelIntentandcreateWheelIntentResolveraccept adprargument that is currently unused in the implementation; if DPR is not needed yet, consider omitting it from the public signature (or at least marking it as reserved) to avoid implying it has an effect today.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `wheelIntent.ts` resolver is quite large and stateful; consider extracting the rule evaluation (I1–I5) into a smaller strategy table or clearly separated helper functions to make future tuning or bugfixes to specific rules less error-prone.
- `TResolveWheelIntent` and `createWheelIntentResolver` accept a `dpr` argument that is currently unused in the implementation; if DPR is not needed yet, consider omitting it from the public signature (or at least marking it as reserved) to avoid implying it has an effect today.
## Individual Comments
### Comment 1
<location path="src/utils/functions/wheelIntent.ts" line_range="401" />
<code_context>
+ * LINE/PAGE mode (`deltaMode !== 0`) is never trackpad — always mouse (I4).
+ * See `docs/system/wheel-intent.md` for rationale.
+ */
+export function createWheelIntentResolver(): TResolveWheelIntent {
+ let lastIntent: EWheelIntent = EWheelIntent.Zoom;
+ let lastTimestamp: number | null = null;
+ let mouseWheelBurstUntil: number | null = null;
+
+ const markMouseWheelBurst = (now: number): void => {
+ mouseWheelBurstUntil = now + MOUSE_WHEEL_BURST_MS;
+ };
+
+ const isInMouseWheelBurst = (now: number): boolean => mouseWheelBurstUntil !== null && now <= mouseWheelBurstUntil;
+
+ return (e: WheelEvent, dpr: number, mouseWheelBehavior: TMouseWheelBehavior): EWheelIntent => {
+ const now = performance.now();
+ const timeSince = lastTimestamp !== null ? now - lastTimestamp : Number.POSITIVE_INFINITY;
</code_context>
<issue_to_address>
**suggestion:** The `dpr` argument is currently unused in the resolver implementation.
`dpr` is passed into the resolver but never used. If it’s intended for future heuristics, consider either actually integrating it now or renaming to `_dpr` / removing it so it doesn’t mislead callers into thinking DPI affects the intent classification or trigger unused-parameter lint warnings.
```suggestion
// Note: `dpr` is currently unused but kept for future heuristics and to satisfy TResolveWheelIntent.
return (e: WheelEvent, _dpr: number, mouseWheelBehavior: TMouseWheelBehavior): EWheelIntent => {
```
</issue_to_address>
### Comment 2
<location path="src/utils/functions/wheelIntent.ts" line_range="175" />
<code_context>
+ * Converts a wheel delta value to approximate pixel units.
+ * WheelEvent.deltaMode: 0 = PIXEL, 1 = LINE, 2 = PAGE.
+ */
+function normalizeWheelDelta(delta: number, deltaMode: number): number {
+ if (deltaMode === WheelEvent.DOM_DELTA_LINE) return delta * LINE_TO_PIXEL_APPROX;
+ if (deltaMode === WheelEvent.DOM_DELTA_PAGE) return delta * PAGE_TO_PIXEL_APPROX;
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a per-event WheelContext object so all normalization and derived flags are computed once and reused across helpers and the resolver.
You’re doing a lot of work per event that’s scattered across tiny helpers, and many of them re-run `normalizeWheelDelta` and re-derive the same booleans. You can keep all behavior and rules as-is but centralize the per-event “context” so the decision tree is easier to follow and normalization happens once.
### 1. Create a per-event context
Instead of passing `WheelEvent` into every helper and normalizing inside each one, derive a context once:
```ts
type WheelContext = {
e: WheelEvent;
normX: number;
normY: number;
absX: number;
absY: number;
hasFractionalDelta: boolean;
isPixelDeltaMode: boolean;
isSmallDelta: boolean;
isVerticalOnly: boolean;
};
function createWheelContext(e: WheelEvent): WheelContext {
const normX = normalizeWheelDelta(e.deltaX, e.deltaMode);
const normY = normalizeWheelDelta(e.deltaY, e.deltaMode);
const absX = Math.abs(normX);
const absY = Math.abs(normY);
const isPixel = e.deltaMode === WheelEvent.DOM_DELTA_PIXEL;
const hasFractional =
isPixel && (!Number.isInteger(e.deltaX) || !Number.isInteger(e.deltaY));
const isSmall =
absX < SMALL_DELTA_THRESHOLD && absY < SMALL_DELTA_THRESHOLD;
const isVerticalOnly = absX < MIN_HORIZONTAL_SCROLL_ABS;
return {
e,
normX,
normY,
absX,
absY,
hasFractionalDelta: hasFractional,
isPixelDeltaMode: isPixel,
isSmallDelta: isSmall,
isVerticalOnly,
};
}
```
### 2. Rewrite helpers to use `WheelContext`
Helpers become simpler, avoid repeated normalization, and the intent logic is easier to audit:
```ts
function isClassicMouseWheelStep(ctx: WheelContext): boolean {
const { e, absX, absY, isPixelDeltaMode, hasFractionalDelta } = ctx;
if (absX >= 0.5) return false;
if (
e.deltaMode === WheelEvent.DOM_DELTA_LINE ||
e.deltaMode === WheelEvent.DOM_DELTA_PAGE
) {
return true;
}
if (absY < MOUSE_WHEEL_DISCRETE_MIN_PX) return false;
if (isPixelDeltaMode && !hasFractionalDelta) return false;
return true;
}
function isDominantAxisLargeWheel(ctx: WheelContext): boolean {
const { absX, absY, e } = ctx;
return (
(absX >= SMALL_DELTA_THRESHOLD && Math.abs(e.deltaY) < 0.5) ||
(absY >= SMALL_DELTA_THRESHOLD && Math.abs(e.deltaX) < 0.5)
);
}
function isDiagonalScroll(ctx: WheelContext): boolean {
const { e, absX, absY } = ctx;
if (e.shiftKey || absX <= DIAGONAL_MIN_ABS || absY <= DIAGONAL_MIN_ABS) {
return false;
}
const minAxis = Math.min(absX, absY);
const maxAxis = Math.max(absX, absY);
return minAxis / maxAxis >= DIAGONAL_AXIS_MIN_RATIO;
}
```
You can similarly adapt `isTrackpadLikeRapidSmall`, `isSlowFractionalMouseWheelStep`, `isPredominantHorizontalScroll`, `isPinchZoomWheelEvent`, etc., to accept `WheelContext`.
### 3. Use `WheelContext` in the resolver and debug emitter
In `createWheelIntentResolver`, compute the context once and feed it to both signals and debug logging:
```ts
return (e: WheelEvent, dpr: number, mouseWheelBehavior: TMouseWheelBehavior): EWheelIntent => {
const now = performance.now();
const timeSince = lastTimestamp !== null ? now - lastTimestamp : Number.POSITIVE_INFINITY;
lastTimestamp = now;
const isRapidStream = timeSince < RAPID_STREAM_MS;
const inMouseWheelBurst = isInMouseWheelBurst(now);
const mouseWheelBurstRemainingMs =
mouseWheelBurstUntil !== null ? Math.max(0, mouseWheelBurstUntil - now) : null;
const ctx = createWheelContext(e);
const { hasFractionalDelta, isSmallDelta, isVerticalOnly, isPixelDeltaMode, normX, normY } = ctx;
const lastIntentBefore = lastIntent;
const signals: TWheelIntentDebugEntry["signals"] = {
isPinchZoom: isPinchZoomWheelEvent(ctx),
isDiagonalScroll: isDiagonalScroll(ctx),
isPredominantHorizontalScroll: isPredominantHorizontalScroll(ctx),
isClassicMouseWheelStep: isClassicMouseWheelStep(ctx),
isDominantAxisLargeWheel: isDominantAxisLargeWheel(ctx),
isVerticalOnly,
hasFractionalDelta,
isSmallDelta,
isPixelDeltaMode,
};
// ... existing rule chain, but use ctx instead of e where possible ...
if (getWheelIntentDebugLogger() !== null) {
emitDebugEntry(
ctx, // change signature to accept ctx
dpr,
mouseWheelBehavior,
timeSince,
isRapidStream,
inMouseWheelBurst,
mouseWheelBurstRemainingMs,
lastIntentBefore,
signals,
rule,
intent
);
}
return intent;
};
```
And adjust `emitDebugEntry` to take `WheelContext` instead of recomputing normalized deltas:
```ts
function emitDebugEntry(
ctx: WheelContext,
dpr: number,
mouseWheelBehavior: TMouseWheelBehavior,
timeSinceLastWheel: number,
isRapidStream: boolean,
isInMouseWheelBurst: boolean,
mouseWheelBurstRemainingMs: number | null,
lastIntentBefore: EWheelIntent,
signals: TWheelIntentDebugEntry["signals"],
rule: string,
result: EWheelIntent
): void {
const debugLogger = getWheelIntentDebugLogger();
if (debugLogger === null) return;
const { e, normX, normY } = ctx;
debugLogger({
mouseWheelBehavior,
dpr,
input: {
deltaX: e.deltaX,
deltaY: e.deltaY,
deltaMode: e.deltaMode,
deltaModeLabel: deltaModeLabel(e.deltaMode),
ctrlKey: e.ctrlKey,
metaKey: e.metaKey,
shiftKey: e.shiftKey,
altKey: e.altKey,
},
normalized: {
deltaX: normX,
deltaY: normY,
diagonalAxisRatio: formatDiagonalAxisRatio(Math.abs(normX), Math.abs(normY)),
},
session: {
timeSinceLastMs: timeSinceLastWheel === Number.POSITIVE_INFINITY ? -1 : timeSinceLastWheel,
isRapidStream,
isInMouseWheelBurst,
mouseWheelBurstRemainingMs,
lastIntentBefore,
},
signals,
rule,
result,
});
}
```
This keeps all rules, constants, and behavior intact but:
- Eliminates repeated `normalizeWheelDelta` calls across helpers and debug logic.
- Makes each helper a simple predicate over `WheelContext`, which matches the conceptual “event context” the reviewer suggested.
- Makes the decision tree in `createWheelIntentResolver` easier to read because each branch is phrased in terms of precomputed, named properties.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Preview is ready. |
Compute normalized wheel signals once per event, prefix unused dpr with underscore, and fix Prettier export formatting. Co-authored-by: Cursor <cursoragent@cursor.com>
The resolver never used device pixel ratio, so drop it from TResolveWheelIntent, settings helpers, Camera, tests, and docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Playwright mouse.wheel emits integer PIXEL deltas that resolveWheelIntent treats as trackpad pan; LINE mode matches mechanical mouse zoom (I4). Co-authored-by: Cursor <cursoragent@cursor.com>
draedful
changed the title
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
2 participants
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
isTrackpadDetectorwithwheelIntent.tsresolver that classifies wheel input as pan or zoom intent from gesture shape (deltaMode, integer vs fractional PIXEL deltas, timing).resolveWheelIntenton graph settings;MOUSE_WHEEL_BEHAVIORapplies to mouse I4 rules only, integer trackpad scroll always pans.observeDPR/LayersServiceand add docs, unit tests, and updated e2e coverage.Test plan
npm test -- wheelIntent.test.ts(21 tests)MOUSE_WHEEL_BEHAVIOR: zoom→ zoomMOUSE_WHEEL_BEHAVIOR: scroll→ panPINCH_ZOOM_SPEEDMade with Cursor
Summary by Sourcery
Route camera wheel handling through an intent-based resolver and fix DPR handling for layers.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: