Fix legacy persona avatar crop parsing#1383
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR extends the avatar crop system to recognize and render legacy crop data ( ChangesLegacy Avatar Crop Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The PR introduces a new union type with moderate complexity: type definitions and parsing are straightforward, but the changes are distributed across multiple layers (shared utilities, UI components, persona editor, feature modules). The logic density is moderate—parsing validation and CSS style computation for legacy crops require careful review, and the persona editor's normalization logic spans several scattered locations. The heterogeneity is moderate: mostly consistent patterns (type union adoption and parsing logic) repeated across files, but each layer serves a distinct role and context. Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/components/ui/AvatarCropWidget.tsx (1)
83-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy crop input is still visually dropped in the widget.
At Line 83 and Line 120, legacy crops are routed to centered default state; combined with preview deriving from
cropPx, legacy framing is never rendered before edits. Imported legacy avatars will still display incorrect framing in this widget.Proposed fix
- const previewCrop: AvatarCrop | null = - imgRect && cropPx - ? { - srcX: cropPx.x / imgRect.w, - srcY: cropPx.y / imgRect.h, - srcWidth: cropPx.size / imgRect.w, - srcHeight: cropPx.size / imgRect.h, - } - : null; + const previewCrop: AvatarCrop | LegacyAvatarCrop | null = + crop && isLegacyAvatarCrop(crop) + ? crop + : imgRect && cropPx + ? { + srcX: cropPx.x / imgRect.w, + srcY: cropPx.y / imgRect.h, + srcWidth: cropPx.size / imgRect.w, + srcHeight: cropPx.size / imgRect.h, + } + : null;Also applies to: 120-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/components/ui/AvatarCropWidget.tsx` around lines 83 - 95, The widget currently treats legacy crops as "no saved crop" and centers the image, so legacy framing never appears; update AvatarCropWidget to handle legacy crop inputs by translating them into the current cropPx instead of falling through to the centered default. Specifically, in the branch where you check isLegacyAvatarCrop(crop) (and the similar block around the 120–132 region), compute cropPx from the legacy crop fields (e.g., srcX, srcY, srcWidth/srcHeight or whatever the legacy shape provides) by converting those normalized/source coordinates into pixel coords using the image rect (w, h), apply clamp identical to the non-legacy path, and call setCropPx(...) so previews derived from cropPx render the legacy framing correctly; consider adding a small helper like legacyCropToPx(crop, w, h) to centralize the conversion and reuse in both locations.
🧹 Nitpick comments (1)
src/shared/lib/avatar-crop.test.ts (1)
21-24: ⚡ Quick winFortify malformed legacy tests for offset field validation.
Heh—good baseline, but one more incision here will harden regression detection: add malformed cases for missing/non-numeric
offsetX/offsetY, since those fields are core to legacy framing.Suggested test additions
it("rejects malformed legacy crops", () => { expect(parseAvatarCropJson('{"zoom":0,"offsetX":12,"offsetY":-8}')).toBeNull(); expect(parseAvatarCropJson('{"zoom":1.2,"offsetX":12,"offsetY":-8,"fullImage":"yes"}')).toBeNull(); + expect(parseAvatarCropJson('{"zoom":1.2,"offsetX":"12","offsetY":-8}')).toBeNull(); + expect(parseAvatarCropJson('{"zoom":1.2,"offsetX":12}')).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/lib/avatar-crop.test.ts` around lines 21 - 24, The test currently validates some malformed legacy crops but misses cases for invalid or missing offset fields; update the "rejects malformed legacy crops" test in src/shared/lib/avatar-crop.test.ts to include additional expects calling parseAvatarCropJson with payloads that omit offsetX, omit offsetY, and use non-numeric offsetX/offsetY (e.g., strings or null), asserting that each returns null so parseAvatarCropJson correctly rejects missing or non-numeric offsetX/offsetY in legacy crop objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/lib/utils.ts`:
- Around line 139-149: The current logic in isLegacyAvatarCrop handling discards
non-identity legacy crops by returning {} when crop.zoom <= 1; instead detect
true identity and only return {} for zoom===1 and offsetX===0 and offsetY===0.
In the block inside isLegacyAvatarCrop (the branch that currently does "if
(crop.zoom <= 1) return {}"), replace that condition with an identity check
(e.g., if (crop.zoom === 1 && crop.offsetX === 0 && crop.offsetY === 0) return
{}), so parseAvatarCropJson-accepted legacy values like {zoom:1, offsetX:12,
offsetY:-8} still produce a transform rather than being discarded.
---
Outside diff comments:
In `@src/shared/components/ui/AvatarCropWidget.tsx`:
- Around line 83-95: The widget currently treats legacy crops as "no saved crop"
and centers the image, so legacy framing never appears; update AvatarCropWidget
to handle legacy crop inputs by translating them into the current cropPx instead
of falling through to the centered default. Specifically, in the branch where
you check isLegacyAvatarCrop(crop) (and the similar block around the 120–132
region), compute cropPx from the legacy crop fields (e.g., srcX, srcY,
srcWidth/srcHeight or whatever the legacy shape provides) by converting those
normalized/source coordinates into pixel coords using the image rect (w, h),
apply clamp identical to the non-legacy path, and call setCropPx(...) so
previews derived from cropPx render the legacy framing correctly; consider
adding a small helper like legacyCropToPx(crop, w, h) to centralize the
conversion and reuse in both locations.
---
Nitpick comments:
In `@src/shared/lib/avatar-crop.test.ts`:
- Around line 21-24: The test currently validates some malformed legacy crops
but misses cases for invalid or missing offset fields; update the "rejects
malformed legacy crops" test in src/shared/lib/avatar-crop.test.ts to include
additional expects calling parseAvatarCropJson with payloads that omit offsetX,
omit offsetY, and use non-numeric offsetX/offsetY (e.g., strings or null),
asserting that each returns null so parseAvatarCropJson correctly rejects
missing or non-numeric offsetX/offsetY in legacy crop objects.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 8d4decb4-0632-4bdf-9f80-0f58b8f4f165
📒 Files selected for processing (9)
src/engine/contracts/types/persona.tssrc/features/catalog/characters/components/CharacterAvatarImage.tsxsrc/features/catalog/personas/components/PersonaEditor.tsxsrc/features/modes/game/components/GameNarration.tsxsrc/features/modes/game/components/GameSurface.tsxsrc/features/runtime/visuals/types.tssrc/shared/components/ui/AvatarCropWidget.tsxsrc/shared/lib/avatar-crop.test.tssrc/shared/lib/utils.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Linked issue
Closes #1364
Why this change
zoom/offsetX/offsetY/fullImage, so imported legacy personas could lose their saved avatar framing.What changed
Refactor impact
Primary owner:
Impact areas reviewed:
Boundary notes:
Pressure points touched:
AvatarCropWidget.Validation
pnpm checkpasses locallypnpm typecheckpasses locallypnpm buildpasses locallypnpm check:architecturepasses locallypnpm check:docspasses locallycargo check --manifest-path src-tauri/Cargo.toml --workspacepasses locallyManual verification notes
node scratch\issue-1364-avatar-crop-proof.mjsbefore the fix.node scratch\issue-1364-avatar-crop-proof.mjs.pnpm exec vitest run src\shared\lib\avatar-crop.test.ts; 1 test file / 4 tests passed.pnpm check; architecture, TypeScript, Rustcargo check, and docs checks passed.node C:\Users\celia\.codex\tools\marinara-proof-gate.mjs scratch\bugfix-verification.json; proof gate passed.zoom/offset crop data to visually confirm framing in the app before ticking manual UI boxes.Docs and release impact
README.mdCONTRIBUTING.mddocs/developer/AGENTS.mdUI evidence
Summary by CodeRabbit
New Features
Tests