fix(mobile): lock canvas#40
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a per-session in-memory canvas-lock cache, a mobile "Lock canvas" toolbar toggle (persisted to session options), and mobile two-finger gesture changes that consult the cache to skip scale/pan when canvas-lock is active. Changes
Sequence Diagram(s)sequenceDiagram
participant Toolbar as Toolbar (UI)
participant Session as Flutter Session / Options
participant Cache as CanvasLock Cache (utils)
participant Gesture as Touch Gesture Detector
participant Canvas as Remote Canvas
Toolbar->>Session: read kLockCanvasOptionKey
Session-->>Toolbar: option value ("Y"/"N"/null)
Toolbar->>Cache: setCachedCanvasLockValue(sessionKey, enabled)
Note right of Cache: store bool + increment revision
Gesture->>Cache: setInitialCachedCanvasLockValue(sessionKey, enabled, expectedRev)
Cache-->>Gesture: ok / no-op
Gesture->>Gesture: two-finger scale update (mobile)
Gesture->>Cache: cachedCanvasLockValue(sessionKey) / hasCachedCanvasLockValue
alt locked == true or (loading && !hasCachedCanvasLockValue)
Gesture->>Canvas: update internal _scale and return (skip scale/pan)
else locked == false
Gesture->>Canvas: perform scale/pan updates (ffi.canvasModel.updateScale, pan)
end
Gesture->>Cache: clearCachedCanvasLockValue(sessionKey) on dispose
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter/lib/common/widgets/remote_input.dart`:
- Around line 459-464: The current code fetches the canvas lock on every scale
gesture start (using _canvasLockScaleGestureToken, _canvasLockedForScaleGesture,
_canvasLockLoadingForScaleGesture and _loadCanvasLockForScaleGesture), causing
UI stalls; instead cache the lock state by reading sessionGetFlutterOption once
(e.g., in initState) into _canvasLockedForScaleGesture and set
_canvasLockLoadingForScaleGesture=false, and update that cached value when the
toolbar toggle changes or by subscribing to an option change stream/callback so
_loadCanvasLockForScaleGesture is only used for initial load or explicit
refresh; keep the token/race logic only if you still perform async refreshes
during gestures and ensure widget.isCamera checks remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebc34e71-e39a-4eb1-8ecb-a02460139602
📒 Files selected for processing (3)
flutter/lib/common/widgets/remote_input.dartflutter/lib/common/widgets/toolbar.dartflutter/lib/utils/canvas_lock.dart
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
4fee209 to
c277ba8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds a mobile-only “Lock canvas” option to prevent two-finger scale gestures from panning/zooming the remote canvas, improving UI interaction on touch devices.
Changes:
- Introduces a small utility for persisting/parsing the lock-canvas option and caching its value per session.
- Adds a “Lock canvas” toggle to the mobile display toggles menu (default connections).
- Updates mobile two-finger scale handling to no-op (for pan/zoom) when the lock is enabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| flutter/lib/utils/canvas_lock.dart | Adds option key/constants and an in-memory per-session cache for the canvas-lock setting. |
| flutter/lib/common/widgets/toolbar.dart | Adds a mobile “Lock canvas” toggle wired to session flutter options and the local cache. |
| flutter/lib/common/widgets/remote_input.dart | Loads the option on init and blocks mobile two-finger pan/zoom when the cached lock is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
c277ba8 to
b9eb128
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter/lib/common/widgets/remote_input.dart`:
- Around line 121-149: Add a one-time init guard so gestures cannot act until
the persisted canvas-lock state is loaded: introduce a private bool (e.g.
_canvasLockLoaded = false) on the widget, set it to true at the end of
_loadCachedCanvasLock() after calling setInitialCachedCanvasLockValue(sessionId,
...), and trigger setState if necessary; then update any gesture checks that
currently call cachedCanvasLockValue(sessionId) (the pinch/pan/zoom handlers
that use that value) to also require _canvasLockLoaded before allowing gestures
(or treat gestures as locked until _canvasLockLoaded is true). Ensure the flag
lifecycle is handled consistently (initState starts load, dispose cleanup
unchanged).
In `@flutter/lib/common/widgets/toolbar.dart`:
- Around line 609-616: The handler currently writes the new lock state to the
local cache via setCachedCanvasLockValue(sessionId, value) before persisting
with bind.sessionSetFlutterOption(sessionId: sessionId, k: kLockCanvasOptionKey,
v: canvasLockValue(value)), which can cause cache/backend divergence on failure;
change the logic to record the previous cached value, then attempt the async
persist first (or if keeping current order, catch errors from
bind.sessionSetFlutterOption) and on any failure restore the previous value by
calling setCachedCanvasLockValue(sessionId, previousValue) and log or surface
the error; reference the onChanged closure, setCachedCanvasLockValue,
bind.sessionSetFlutterOption, canvasLockValue and sessionId to locate and update
the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bce4afaa-8b02-40d9-a547-912ccb5eabdd
📒 Files selected for processing (3)
flutter/lib/common/widgets/remote_input.dartflutter/lib/common/widgets/toolbar.dartflutter/lib/utils/canvas_lock.dart
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
b9eb128 to
4def7cf
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
e389dd7 to
e728976
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
e728976 to
5f5ba35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter/lib/common/widgets/toolbar.dart`:
- Around line 615-632: The current onChanged handler can race: add a
monotonically incrementing operation id (e.g., int lockWriteOp = 0) in the same
scope as committedLockValue and increment it at the start of each onChanged
call, capture the id in the async closure, then when the async
bind.sessionSetFlutterOption completes (either success or catch) only update
committedLockValue or revert the cached value if the captured id equals the
latest seen id; for failures ignore stale completions whose captured id is less
than the latest id. Reference symbols: TToggleMenu's onChanged closure,
committedLockValue, setCachedCanvasLockValue(sessionKey, ...),
bind.sessionSetFlutterOption(sessionId, kLockCanvasOptionKey,
canvasLockValue(value)), sessionKey and sessionId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8ec6fba-8beb-4a94-b472-4db3e05a1367
📒 Files selected for processing (3)
flutter/lib/common/widgets/remote_input.dartflutter/lib/common/widgets/toolbar.dartflutter/lib/utils/canvas_lock.dart
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
5f5ba35 to
36d2b94
Compare
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
36d2b94 to
d5a93cc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
d5a93cc to
da37057
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flutter/lib/common/widgets/toolbar.dart`:
- Around line 609-612: You read the persisted lock value into initialLockValue
(via bind.sessionGetFlutterOption + isCanvasLockEnabled) but never update the
in-memory/cache copy remote_input.dart relies on, so the gesture enforcement can
stay stale; after successfully resolving lockValue into initialLockValue, write
the same persisted value back into the session option cache (the same cache API
remote_input.dart uses) using the sessionId and kLockCanvasOptionKey so the
cached state and persisted state are in sync (e.g., call the session cache
setter used elsewhere with sessionId, kLockCanvasOptionKey and lockValue
immediately before leaving the try block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d76c7f4b-87c6-41db-8acb-42fe17c3d6e6
📒 Files selected for processing (4)
flutter/lib/common/widgets/remote_input.dartflutter/lib/common/widgets/toolbar.dartflutter/lib/mobile/pages/view_camera_page.dartflutter/lib/utils/canvas_lock.dart
Signed-off-by: fufesou <linlong1266@gmail.com>
fufesou
force-pushed
the
fix/mobile-lock-canvas
branch
from
da37057 to
e9d2ad4
Compare
Summary by CodeRabbit
New Features
Bug Fixes