review(pose-lib): D-Project — validate before wipe + dedup + file.import/export categories#603
Merged
Merged
Conversation
…eadcrumb category Three review findings on PR #602 (merged). ## Codex P1 — schema-matching file with bad `poses` field wiped library `loadPoseLibrary` removed the in-memory per-entity entry before checking that the `poses` field was actually an array. A file with matching schema but a malformed `poses` value (string, object, missing) would silently drop the user's existing poses and return success — data loss masked as success. Fix: stage parse output in a local `EntityPoses` so the in-memory store stays untouched until the file is fully accepted. Validate `posesV.isArray()` before any mutation. The all-or-nothing replacement (`m_byEntity.insert(entity, staging)`) only fires after a clean parse. ## Codex P2 — duplicate names in file left phantoms in `order` Loading appended every parsed pose name into `order` even on name re-occurrences. `byName` keeps only the last value (hash overwrite), so `order` could grow longer than the actual pose count. A `deletePose("A")` then removed the hash entry but left "A" in `order`, leaving `listPoses` and `hasPose` disagreeing. Fix: guard the `order.append(name)` with a `byName.contains(name)` check so later entries with the same name overwrite the snapshot (last-write-wins, same as a regular `savePose`) without growing `order`. ## CodeRabbit Major — file.import / file.export breadcrumb categories Per the project's Sentry breadcrumb policy in `CLAUDE.md`, file I/O operations use `file.import` / `file.export` not the scene-side category. Switched both `savePoseLibrary` and `loadPoseLibrary` to the right categories. ## 2 new tests - `LoadLibraryWithMissingPosesArrayPreservesInMemoryLibrary` — schema-matching file with `"poses": "oops"` (string instead of array) returns false AND keeps existing in-memory poses intact. - `LoadLibraryDeduplicatesDuplicateNamesInFile` — file with two entries under name "A" produces `listPoses = ["A", "B"]` not `["A", "A", "B"]`; after `deletePose("A")` no phantom remains. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
fernandotonon
added a commit
that referenced
this pull request
May 18, 2026
Codex P2 on PR #604 (merged). The CLI's `--library list` path checked the `schema` string but then did `root.value("poses").toArray()` unconditionally — same silent-empty-on-bad-type bug class as loadPoseLibrary had on PR #602 (fixed in #603), just in the duplicated parse-in-the-CLI path. A schema-matching `.poselib` file with `poses` missing or non-array would report "No poses in library" with exit 0 instead of surfacing the corruption. CI/audit workflows relying on non-zero exit for invalid libraries would miss the failure. Fix: explicit `isArray()` check after the schema match, emit "malformed 'poses' field in <path>" to stderr and return 1. Manual verification: a `{"schema":"qtmesheditor.poselib.v1","poses":"oops"}` file now exits 1 with the expected message instead of 0 with a vacuous result. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.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
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.
Three review findings on PR #602 (merged).
Codex P1 — schema-matching file with bad
poseswiped libraryloadPoseLibraryremoved the in-memory entry before checking thatposeswas actually an array. A file with matching schema but a malformedposesvalue would silently drop the user's existing poses and return success — data loss masked as success.Fix: stage parse output in a local
EntityPosesso the in-memory store stays untouched until the file is fully accepted. ValidateposesV.isArray()before any mutation. All-or-nothing replacement (m_byEntity.insert(entity, staging)) only fires after a clean parse.Codex P2 — duplicate names in file left phantoms in
orderLoading appended every parsed pose name into
ordereven on re-occurrences.byNameonly kept the last value (hash overwrite), soordercould grow longer than the actual pose count. AdeletePose("A")then removed the hash entry but left "A" inorder—listPosesandhasPosedisagreed.Fix: guard
order.append(name)with abyName.contains(name)check so later entries with the same name overwrite (last-write-wins, like a regularsavePose).CodeRabbit Major — file.import / file.export breadcrumb categories
Per the Sentry breadcrumb policy in
CLAUDE.md, file I/O usesfile.import/file.export. Switched both helpers to the right categories.2 new tests
LoadLibraryWithMissingPosesArrayPreservesInMemoryLibraryLoadLibraryDeduplicatesDuplicateNamesInFile🤖 Generated with Claude Code