review(pose-lib): D-Project — validate before wipe + dedup + file.import/export categories

src/PoseLibrary.cpp

@@ -361,7 +361,7 @@ bool PoseLibrary::savePoseLibrary(Ogre::Entity* entity, const QString& filePath)
     }
     if (!file.commit()) return false;
 
-    SentryReporter::addBreadcrumb("scene.anim.pose",
+    SentryReporter::addBreadcrumb("file.export",
         QStringLiteral("save library to '%1' (%2 poses)")
             .arg(filePath).arg(entIt->order.size()));
     return true;
@@ -382,13 +382,13 @@ bool PoseLibrary::loadPoseLibrary(Ogre::Entity* entity, const QString& filePath)
     const QJsonObject root = doc.object();
     if (root.value("schema").toString() != QString::fromLatin1(kPoseLibSchemaV1))
         return false;
-    const QJsonArray poses = root.value("poses").toArray();
-
-    // Wipe the existing per-entity entry — the file is canonical
-    // for this load. Partial overlay would be confusing UX (which
-    // pose wins on name collision?), so we go with replacement.
-    m_byEntity.remove(entity);
-    auto& store = m_byEntity[entity];
+    // Codex P1 on PR #602: validate the payload shape BEFORE wiping
+    // the in-memory library. A schema-matching file with `poses`
+    // missing or non-array would otherwise silently drop the user's
+    // existing data and return success.
+    const QJsonValue posesV = root.value("poses");
+    if (!posesV.isArray()) return false;
+    const QJsonArray poses = posesV.toArray();
 
     auto readVec3 = [](const QJsonArray& a, const Ogre::Vector3& def) -> Ogre::Vector3 {
         if (a.size() != 3) return def;
@@ -404,7 +404,11 @@ bool PoseLibrary::loadPoseLibrary(Ogre::Entity* entity, const QString& filePath)
                                 static_cast<Ogre::Real>(a[3].toDouble()));
     };
 
+    // Build the new library entry off to the side so any parse
+    // failure leaves the in-memory store untouched.
+    EntityPoses staging;
     for (const QJsonValue& p : poses) {
+        if (!p.isObject()) continue;
         const QJsonObject pObj = p.toObject();
         const QString name = pObj.value("name").toString();
         if (name.isEmpty()) continue;
@@ -421,13 +425,25 @@ bool PoseLibrary::loadPoseLibrary(Ogre::Entity* entity, const QString& filePath)
                                   Ogre::Vector3(1, 1, 1));
             snapshot.insert(it.key(), trs);
         }
-        store.byName.insert(name, snapshot);
-        store.order.append(name);
+        // Codex P2 on PR #602: only append `order` on first sighting
+        // of the name so duplicate entries in the file don't leave
+        // `order` with phantom names that survive a `deletePose`.
+        // Later occurrences of the same name overwrite the snapshot
+        // (last-write-wins) like a regular `savePose` does.
+        const bool isFirstSighting = !staging.byName.contains(name);
+        staging.byName.insert(name, snapshot);
+        if (isFirstSighting) staging.order.append(name);
     }
 
-    SentryReporter::addBreadcrumb("scene.anim.pose",
+    // All-or-nothing replacement: now we know the file parsed
+    // cleanly, swap the per-entity entry. Partial overlay would be
+    // confusing UX (which pose wins on name collision?), so we go
+    // with replacement.
+    m_byEntity.insert(entity, staging);
+
+    SentryReporter::addBreadcrumb("file.import",
         QStringLiteral("load library from '%1' (%2 poses)")
-            .arg(filePath).arg(store.order.size()));
+            .arg(filePath).arg(staging.order.size()));
     emit posesChanged(entity);
     return true;
 }

src/PoseLibrary_test.cpp

@@ -297,6 +297,68 @@ TEST_F(PoseLibrarySceneTest, LoadLibraryRejectsMissingFileAndBadSchema) {
     }
 }
 
+// Codex P1 on PR #602: a schema-matching file with `poses` missing
+// or non-array must NOT wipe the in-memory library. Earlier draft
+// did the wipe before validating the array, silently dropping the
+// user's data on a malformed file.
+TEST_F(PoseLibrarySceneTest, LoadLibraryWithMissingPosesArrayPreservesInMemoryLibrary) {
+    QTemporaryDir tmp;
+    Ogre::Entity* entity = createAnimatedTestEntity("PoseLib_PreserveOnBad");
+    ASSERT_NE(entity, nullptr);
+    auto* lib = PoseLibrary::instance();
+
+    ASSERT_TRUE(lib->savePose(entity, QStringLiteral("Keep")));
+    EXPECT_EQ(lib->listPoses(entity).size(), 1);
+
+    // Schema matches, but `poses` is a string, not an array.
+    const QString p = tmp.path() + "/badposes.poselib";
+    QFile f(p); ASSERT_TRUE(f.open(QIODevice::WriteOnly));
+    f.write("{\"schema\": \"qtmesheditor.poselib.v1\", \"poses\": \"oops\"}");
+    f.close();
+
+    EXPECT_FALSE(lib->loadPoseLibrary(entity, p));
+    // In-memory library must still contain "Keep".
+    EXPECT_EQ(lib->listPoses(entity).size(), 1);
+    EXPECT_TRUE(lib->hasPose(entity, QStringLiteral("Keep")));
+}
+
+// Codex P2 on PR #602: duplicate pose names in the sidecar must not
+// leave `order` with phantom entries that survive a `deletePose`
+// (causing listPoses to show a name that hasPose disagrees about).
+TEST_F(PoseLibrarySceneTest, LoadLibraryDeduplicatesDuplicateNamesInFile) {
+    QTemporaryDir tmp;
+    Ogre::Entity* entity = createAnimatedTestEntity("PoseLib_DupNames");
+    ASSERT_NE(entity, nullptr);
+    auto* lib = PoseLibrary::instance();
+
+    // Hand-write a file with two entries under the same name.
+    const QString p = tmp.path() + "/dup.poselib";
+    QFile f(p); ASSERT_TRUE(f.open(QIODevice::WriteOnly));
+    f.write(R"({
+      "schema": "qtmesheditor.poselib.v1",
+      "poses": [
+        {"name": "A", "bones": {}},
+        {"name": "A", "bones": {}},
+        {"name": "B", "bones": {}}
+      ]
+    })");
+    f.close();
+
+    EXPECT_TRUE(lib->loadPoseLibrary(entity, p));
+    const QStringList names = lib->listPoses(entity);
+    // Should be ["A", "B"] not ["A", "A", "B"].
+    ASSERT_EQ(names.size(), 2);
+    EXPECT_EQ(names[0], QStringLiteral("A"));
+    EXPECT_EQ(names[1], QStringLiteral("B"));
+
+    // After deleting "A", listPoses must not contain a phantom "A".
+    EXPECT_TRUE(lib->deletePose(entity, QStringLiteral("A")));
+    EXPECT_FALSE(lib->hasPose(entity, QStringLiteral("A")));
+    const QStringList afterDelete = lib->listPoses(entity);
+    ASSERT_EQ(afterDelete.size(), 1);
+    EXPECT_EQ(afterDelete[0], QStringLiteral("B"));
+}
+
 TEST_F(PoseLibrarySceneTest, LoadLibraryWipesExistingPosesFirst) {
     QTemporaryDir tmp;
     Ogre::Entity* entity = createAnimatedTestEntity("PoseLib_SidecarWipe");