bugfix(rotation): improve rotation logic #106
Open
BenAuerDev wants to merge 6 commits into
Open
Conversation
Replaced unreliable delta-accumulation drag math with direct center-to-cursor angle calculation. Added degree display during rotation. Removed dead rotationSignal and onRotate state mutation that leaked rotation state when switching tools.
…osition Refactored processRotation to return typed RotationResult instead of Map. rotateImage() now preserves canvas scale factor and centers image after rotation by compensating for trim center offset.
…ing box Rotations are now composed into a single transform using data-orig-* attributes to track original dimensions. Prevents bounding box from growing with repeated rotations. Fixed svgStringToPngBytes canvas drawing and exception catches.
Covers processRotation, trimPngAsBytes, getImageFromBytes, SVG parsing utilities, rotation composition, bounding box stability, and rotation helper methods. 39 tests total.
📝 WalkthroughWalkthroughRotation state was centralized inside RotationWrapper (removing external rotationSignal/onRotate/initialRotation), raster rotation now returns trim offsets, SVG rotation composes prior rotations using preserved original-size attributes, move/resize tool was simplified to rotation-agnostic rendering, and unit tests for raster and SVG rotation utilities were added. Changes
Sequence DiagramsequenceDiagram
participant User
participant RW as RotationWrapper
participant FI as FlitesImage
participant IPU as ImageProcessingUtils
participant SvgU as SvgUtils
User->>RW: Drag (pan) around center to rotate
RW->>RW: compute angle from center, update internal rotation
User->>RW: Release (apply)
RW->>FI: request rotateImage(angle)
alt Raster image
FI->>IPU: rotateInIsolates / processRotation(bytes, angle)
IPU-->>FI: RotationResult { imageBytes, trimOffsetX, trimOffsetY }
else SVG image
FI->>SvgU: rotateAndTrimSvg(svgData, angle)
SvgU-->>FI: rotated SVG bytes (composed with any prior rotation, original-size attrs preserved)
end
FI->>FI: preserve scale & center, apply trim offsets, reset transient rotation, save changes
FI-->>RW: updated image state
RW->>User: UI reflects rotated & re-centered image
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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 (4)
apps/flites/lib/utils/svg_utils.dart (1)
232-234: Regex injection risk is low but worth noting.
getDataAttributeinterpolatesnamedirectly into a regex pattern. The current callers all pass literaldata-orig-*strings which are safe. If this method is ever called with user-controlled input, the unescaped string could break the regex or match unintended content. Consider usingRegExp.escape(name)for defense-in-depth.Defensive fix
static double? getDataAttribute(String attributes, String name) { - final match = RegExp('$name="([^"]*)"').firstMatch(attributes); + final match = RegExp('${RegExp.escape(name)}="([^"]*)"').firstMatch(attributes); return match != null ? double.tryParse(match.group(1)!) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/utils/svg_utils.dart` around lines 232 - 234, The getDataAttribute function interpolates the name parameter directly into a RegExp which can be broken or exploited if name ever comes from user input; update getDataAttribute to escape the name before building the pattern (use RegExp.escape(name)) so the generated regex matches the literal attribute name safely and still extracts the captured value for double.tryParse.apps/flites/lib/utils/image_processing_utils.dart (1)
50-51: Nit: comment about isolate type restrictions is slightly inaccurate.Dart isolates via
compute()can pass many types beyond primitives (includingUint8List,List, custom classes). TheMapapproach works fine, but the comment overstates the constraint. Consider softening to something like "we use a Map for simplicity across the isolate boundary."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/utils/image_processing_utils.dart` around lines 50 - 51, The comment stating "Isolate entry point — accepts and returns Maps because Dart isolates can only pass primitive types across the boundary." overstates Dart's restrictions; replace it with a softer, accurate note such as: "Isolate entry point — we use a Map for simplicity when passing data across the isolate boundary (other types like Uint8List and Lists are supported by compute())." Locate and update the comment above the isolate entry function / compute usage in image_processing_utils.dart so it references the Map choice as a convenience rather than a language limitation.apps/flites/lib/types/flites_image.dart (1)
88-89: Weak unique ID generation.The ID is constructed from
millisecondsSinceEpochplus two small random ints. In a batch-import scenario (e.g., importing multiple files in a loop),DateTime.now().millisecondsSinceEpochcan repeat within the same millisecond, andRandom().nextInt(14000)/Random().nextInt(15000)has a non-trivial collision probability (~1 in 210M per pair). Consider using a UUID package or at least a singleRandom.secure()instance with a wider range.This is pre-existing code, not introduced in this PR, so feel free to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/types/flites_image.dart` around lines 88 - 89, The current ID assignment (the id field set with '${DateTime.now().millisecondsSinceEpoch}-${Random().nextInt(14000)}-${Random().nextInt(15000)}') is weak and can collide; replace it with a proper UUID or a cryptographically strong random generator: either import and use a UUID library (e.g. uuid.v4()) to set the id, or create a single Random.secure() instance and generate a larger bit-width value (e.g. 64+ bits) instead of two small nextInt calls; update the code where id is assigned (the id field/constructor in the FlitesImage class or the method that currently builds this string) to use the chosen approach and remove the current DateTime+small-random concatenation.apps/flites/lib/widgets/rotation/rotation_wrapper.dart (1)
31-37:circleRadiusis computed once ininitStateand won't update ifwidget.rectchanges.If the parent rebuilds this widget with a different
rect(e.g., after resizing the image),circleRadiuswill still reflect the stale value. Consider moving the calculation tobuild()or adding adidUpdateWidgetoverride.Option: use didUpdateWidget
+ `@override` + void didUpdateWidget(covariant RotationWrapper oldWidget) { + super.didUpdateWidget(oldWidget); + if (oldWidget.rect.size != widget.rect.size) { + circleRadius = (longestSideSize(widget.rect.size) / 2) * 1.5; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart` around lines 31 - 37, circleRadius is computed once in initState so it won't update when widget.rect changes; move the computation out of initState or update it in didUpdateWidget: either recompute circleRadius inside build() using longestSideSize(widget.rect.size) / 2 * 1.5, or override didUpdateWidget(oldWidget) to detect changes to widget.rect and recalculate circleRadius there (and call setState if needed) so the value stays in sync with updates to widget.rect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/flites/lib/utils/svg_utils.dart`:
- Around line 155-189: The code uses previouslyRotated to branch but
storedW/storedH remain nullable, causing Size(storedW, storedH) to fail; update
the if to directly check storedW != null && storedH != null (so Dart will
promote them) or explicitly unwrap/coalesce when assigning
originalContentSize—e.g., replace the previouslyRotated branch condition with if
(storedW != null && storedH != null) { originalContentSize = Size(storedW!,
storedH!); ... } or use Size(storedW ?? 0, storedH ?? 0) if a default is
acceptable; adjust references to rotationCenter and other uses accordingly.
In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart`:
- Around line 47-51: The current _rotationDegrees getter uses (rotation * 180 /
pi) % 360 which yields wrong results for large negative rotations because Dart's
% preserves the left operand sign; update the normalization to always map into
(-180, 180] by computing degrees = rotation * 180 / pi and then normalized =
(((degrees + 180) % 360) + 360) % 360 - 180 (then format
normalized.toStringAsFixed(1)°); replace the existing normalization line in
_rotationDegrees with this stable expression.
In `@apps/flites/test/utils/image_rotation_test.dart`:
- Around line 133-149: The test currently redefines longestSideSize locally
instead of importing the production helper; remove the local definition of
longestSideSize from the test file and add an import for rotation_wrapper.dart
so the tests exercise the actual rotation_wrapper.dart::longestSideSize used
elsewhere (e.g., rotate_tool.dart). Ensure the test file imports the same symbol
(longestSideSize) from rotation_wrapper.dart and delete the duplicate function
at the file end.
---
Nitpick comments:
In `@apps/flites/lib/types/flites_image.dart`:
- Around line 88-89: The current ID assignment (the id field set with
'${DateTime.now().millisecondsSinceEpoch}-${Random().nextInt(14000)}-${Random().nextInt(15000)}')
is weak and can collide; replace it with a proper UUID or a cryptographically
strong random generator: either import and use a UUID library (e.g. uuid.v4())
to set the id, or create a single Random.secure() instance and generate a larger
bit-width value (e.g. 64+ bits) instead of two small nextInt calls; update the
code where id is assigned (the id field/constructor in the FlitesImage class or
the method that currently builds this string) to use the chosen approach and
remove the current DateTime+small-random concatenation.
In `@apps/flites/lib/utils/image_processing_utils.dart`:
- Around line 50-51: The comment stating "Isolate entry point — accepts and
returns Maps because Dart isolates can only pass primitive types across the
boundary." overstates Dart's restrictions; replace it with a softer, accurate
note such as: "Isolate entry point — we use a Map for simplicity when passing
data across the isolate boundary (other types like Uint8List and Lists are
supported by compute())." Locate and update the comment above the isolate entry
function / compute usage in image_processing_utils.dart so it references the Map
choice as a convenience rather than a language limitation.
In `@apps/flites/lib/utils/svg_utils.dart`:
- Around line 232-234: The getDataAttribute function interpolates the name
parameter directly into a RegExp which can be broken or exploited if name ever
comes from user input; update getDataAttribute to escape the name before
building the pattern (use RegExp.escape(name)) so the generated regex matches
the literal attribute name safely and still extracts the captured value for
double.tryParse.
In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart`:
- Around line 31-37: circleRadius is computed once in initState so it won't
update when widget.rect changes; move the computation out of initState or update
it in didUpdateWidget: either recompute circleRadius inside build() using
longestSideSize(widget.rect.size) / 2 * 1.5, or override
didUpdateWidget(oldWidget) to detect changes to widget.rect and recalculate
circleRadius there (and call setState if needed) so the value stays in sync with
updates to widget.rect.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/flites/lib/feature_kits/tools/canvas_helpers/image_editor.dartapps/flites/lib/feature_kits/tools/move_resize_tool.dartapps/flites/lib/feature_kits/tools/rotate_tool.dartapps/flites/lib/types/flites_image.dartapps/flites/lib/utils/image_processing_utils.dartapps/flites/lib/utils/svg_utils.dartapps/flites/lib/widgets/rotation/rotation_wrapper.dartapps/flites/test/utils/image_rotation_test.dartapps/flites/test/utils/svg_rotation_test.dart
💤 Files with no reviewable changes (1)
- apps/flites/lib/feature_kits/tools/rotate_tool.dart
…tSideSize test import
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/flites/lib/widgets/rotation/rotation_wrapper.dart (1)
203-205: MovelongestSideSizeout of the widget layer.This helper is now shared outside
RotationWrapper, but it lives in a file that also pulls in widget/app-state dependencies. A small geometry utility would keep non-UI callers from importing the whole widget layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart` around lines 203 - 205, Extract the pure-geometry helper longestSideSize out of rotation_wrapper.dart into a new non-UI utility (e.g., a geometry or math utils file) so callers that need only geometry won’t import widget/app-state code; create a new dart file (e.g., geometry_utils.dart) with the longestSideSize(Size) implementation (keeping it independent of any Flutter widget or app-state imports), update all call sites (including rotate_tool.dart and RotationWrapper) to import the new utility, and remove the helper from rotation_wrapper.dart so the widget layer no longer exposes non-UI utilities.apps/flites/lib/utils/svg_utils.dart (1)
230-234: Accept both quote styles fordata-orig-*parsing.
getAttributesoperates on arbitrary SVG markup, but this helper only matches double-quoted attributes. Supporting single quotes too would make the stored-rotation path as tolerant asgetSvgSizeandgetViewBox.Suggested fix
static double? getDataAttribute(String attributes, String name) { - final match = RegExp('$name="([^"]*)"').firstMatch(attributes); + final escapedName = RegExp.escape(name); + final match = + RegExp('$escapedName="([^"]*)"').firstMatch(attributes) ?? + RegExp("$escapedName='([^']*)'").firstMatch(attributes); return match != null ? double.tryParse(match.group(1)!) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/utils/svg_utils.dart` around lines 230 - 234, getDataAttribute currently only matches double-quoted attributes; change its RegExp to accept both double- and single-quoted attribute values (e.g., match name="..." OR name='...') and then use whichever capture group matched (or combine into one capture by using an alternation) before calling double.tryParse; update the RegExp and the return logic in getDataAttribute so it tolerates single quotes like getSvgSize/getViewBox do.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart`:
- Around line 121-127: The rotation snap occurs because the raw delta
(currentAngle - _dragStartAngle) can jump by ~±2π across the ±π seam in
onPanUpdate; to fix, compute the delta using _angleFromCenter and then normalize
it into the (-π, π] range before applying it: calculate delta = currentAngle -
_dragStartAngle, then while (delta > π) subtract 2π and while (delta < -π) add
2π, and finally set rotation = _rotationAtDragStart + delta (keep references to
onPanUpdate, _angleFromCenter, _rotationAtDragStart, _dragStartAngle, and
rotation).
- Around line 65-67: circleRadius is computed once in initState() from
widget.rect.size causing the gesture/overlay SizedBox to use a stale radius when
the parent supplies a new widget.rect; update the computation to always reflect
current widget.rect by either moving the circleRadius calculation into build()
(so SizedBox uses the latest value) or override didUpdateWidget() in
RotationWrapper to recompute and setState() when oldWidget.rect != widget.rect,
ensuring the SizedBox height/width and any gesture bounds stay in sync with the
rotated content that uses widget.rect directly.
---
Nitpick comments:
In `@apps/flites/lib/utils/svg_utils.dart`:
- Around line 230-234: getDataAttribute currently only matches double-quoted
attributes; change its RegExp to accept both double- and single-quoted attribute
values (e.g., match name="..." OR name='...') and then use whichever capture
group matched (or combine into one capture by using an alternation) before
calling double.tryParse; update the RegExp and the return logic in
getDataAttribute so it tolerates single quotes like getSvgSize/getViewBox do.
In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart`:
- Around line 203-205: Extract the pure-geometry helper longestSideSize out of
rotation_wrapper.dart into a new non-UI utility (e.g., a geometry or math utils
file) so callers that need only geometry won’t import widget/app-state code;
create a new dart file (e.g., geometry_utils.dart) with the
longestSideSize(Size) implementation (keeping it independent of any Flutter
widget or app-state imports), update all call sites (including rotate_tool.dart
and RotationWrapper) to import the new utility, and remove the helper from
rotation_wrapper.dart so the widget layer no longer exposes non-UI utilities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef987445-4ec4-475d-9703-d217efee61fe
📒 Files selected for processing (3)
apps/flites/lib/utils/svg_utils.dartapps/flites/lib/widgets/rotation/rotation_wrapper.dartapps/flites/test/utils/image_rotation_test.dart
- Normalize _rotationDegrees to (-180, 180] - Compute circle radius from current rect (getter) instead of initState - Normalize pan delta to (-π, π] to avoid snap when crossing seam
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/flites/lib/widgets/rotation/rotation_wrapper.dart (1)
180-193:⚠️ Potential issue | 🟠 MajorGuard the async apply path against re-entry and disposal.
The apply button remains enabled while
rotateImageawaits on line 185, allowing rapid taps to enqueue multiple rotation operations. Additionally,setStateruns unconditionally after the await completes, causing an error if the user navigates away during the async operation (setState after dispose).Suggested fix
class _RotationWrapperState extends State<RotationWrapper> { double rotation = 0; double _dragStartAngle = 0; double _rotationAtDragStart = 0; + bool _isApplyingRotation = false; ... IconButton( icon: Icon( Icons.check_circle_outlined, color: context.colors.surfaceContainer, ), - onPressed: () async { - final currentImage = - getFliteImage(SelectedImageState.selectedImageId); - - if (currentImage != null) { - await currentImage.rotateImage(rotation); - } - - setState(() { - rotation = 0; - }); - - toolController.selectedTool = Tool.canvas; + onPressed: _isApplyingRotation + ? null + : () async { + setState(() { + _isApplyingRotation = true; + }); + try { + final currentImage = getFliteImage( + SelectedImageState.selectedImageId, + ); + + if (currentImage != null) { + await currentImage.rotateImage(rotation); + } + + if (!mounted) return; + setState(() { + rotation = 0; + }); + toolController.selectedTool = Tool.canvas; + } finally { + if (mounted) { + setState(() { + _isApplyingRotation = false; + }); + } + } + }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart` around lines 180 - 193, Protect the async apply path by adding a re-entry guard boolean (e.g. _isApplying) checked at the start of the onPressed handler and set true before awaiting SelectedImageState.selectedImageId -> getFliteImage(...).rotateImage(rotation), and set false at the end; also disable the apply button when _isApplying is true. After the await, only call setState and update toolController.selectedTool if the widget is still mounted (check mounted) to avoid setState after dispose; ensure rotation is reset inside the guarded mounted check and that the guard is cleared in finally so the button becomes usable again.
♻️ Duplicate comments (1)
apps/flites/lib/widgets/rotation/rotation_wrapper.dart (1)
42-45:⚠️ Potential issue | 🟡 MinorExact
180°still formats as-180.0°.Line 44 normalizes into
[-180, 180), so bothpiand-pirender as-180.0°. The PR summary says the display should be(-180, 180], so this edge case is still flipped.Suggested fix
String get _rotationDegrees { - final degrees = rotation * 180 / pi; - final normalized = (((degrees + 180) % 360) + 360) % 360 - 180; + var normalized = (rotation * 180 / pi) % 360; + if (normalized <= -180) normalized += 360; + if (normalized > 180) normalized -= 360; return '${normalized.toStringAsFixed(1)}°'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart` around lines 42 - 45, The _rotationDegrees getter currently normalizes degrees into [-180, 180) causing both +pi and -pi to format as "-180.0°"; update the normalization in _rotationDegrees (variables degrees/normalized) to produce the range (-180, 180] instead — e.g., keep the existing normalization but after computing normalized, if normalized == -180.0 then set normalized = 180.0 so the edge case renders as "180.0°".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart`:
- Around line 180-193: Protect the async apply path by adding a re-entry guard
boolean (e.g. _isApplying) checked at the start of the onPressed handler and set
true before awaiting SelectedImageState.selectedImageId ->
getFliteImage(...).rotateImage(rotation), and set false at the end; also disable
the apply button when _isApplying is true. After the await, only call setState
and update toolController.selectedTool if the widget is still mounted (check
mounted) to avoid setState after dispose; ensure rotation is reset inside the
guarded mounted check and that the guard is cleared in finally so the button
becomes usable again.
---
Duplicate comments:
In `@apps/flites/lib/widgets/rotation/rotation_wrapper.dart`:
- Around line 42-45: The _rotationDegrees getter currently normalizes degrees
into [-180, 180) causing both +pi and -pi to format as "-180.0°"; update the
normalization in _rotationDegrees (variables degrees/normalized) to produce the
range (-180, 180] instead — e.g., keep the existing normalization but after
computing normalized, if normalized == -180.0 then set normalized = 180.0 so the
edge case renders as "180.0°".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9589c231-c2bb-4cdc-8506-1701f1bddfee
📒 Files selected for processing (1)
apps/flites/lib/widgets/rotation/rotation_wrapper.dart
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.
Description
Summary
Type of Change
Summary by CodeRabbit
Bug Fixes
Refactor
Tests