CodeRabbit review: 7 findings across embed.js, models.py, release.yml, checks.py, utils.py

[Kani999]

Findings from a CodeRabbit CLI 0.7.5 review of the full history:

coderabbit review --base-commit a93eaef

7 findings (3 major, 4 minor) across 36 files. Grouped below by whether they
look worth acting on. Each item is independent — close them off one at a time.

---

Worth fixing

1. embed.js — a late export response can overwrite newer saved content

major · Data Integrity · netbox_drawio/static/netbox_drawio/js/embed.js:62-67 (also 146-163)

The queue check only compares the incoming seq against a payload that is
still queued (if (pending !== null && pending.seq > seq) break;). It does
not compare against payloads already persisted or in flight, because
persist() sets pending = null before the POST.

Sequence that regresses the diagram:

1. stash() runs for seq 1, export requested.
2. stash() runs for seq 2, export requested.
3. The seq 2 export arrives first, is queued, and persist() POSTs it. pending becomes null.
4. The seq 1 export arrives late. pending is null, so the check passes and the older XML/SVG is queued and POSTed.

The server then stores the older diagram.

Proposed fix — track the highest sequence accepted for persistence:

     let stashedXml = null; // latest edit awaiting its SVG export
     let stashSeq = 0; // increments on every save/autosave; tags payloads
     let pending = null; // newest {seq, xml, svg} awaiting POST
+    let acceptedSeq = 0; // highest seq already queued or sent

-                if (pending !== null && pending.seq > seq) {
-                    break; // a newer payload is already queued
-                }
+                if (seq <= acceptedSeq || (pending !== null && pending.seq > seq)) {
+                    break; // an equal or newer payload was already queued or sent
+                }
+                acceptedSeq = seq;
                 // Queue the payload; persist() drains it once any in-flight request settles.
                 pending = { seq: seq, xml: xml, svg: msg.data };
                 persist();

If a POST fails, reset acceptedSeq to the failed payload's seq - 1 so a
retry is still possible.

2. models.py — content_hash is dropped when a caller passes update_fields

major · Data Integrity · netbox_drawio/models.py:61-68

save() recomputes self.content_hash but forwards kwargs unchanged. If any
caller passes update_fields without content_hash (e.g.
save(update_fields=["svg_cache"])), the new hash is computed and then
discarded. The stored content_hash then no longer matches the blobs — and
that value also serves as the SVG endpoint ETag, so clients keep a stale
cached SVG.

     def save(self, *args, **kwargs):
         # Skip re-hashing only when every blob field is deferred and therefore
         # untouched (assignment removes a field from the deferred set). If any
         # blob is loaded it may have changed, so recompute — even though that
         # fetches back a still-deferred sibling.
         if not self.get_deferred_fields().issuperset(BLOB_FIELDS):
             self.content_hash = self._compute_content_hash()
+            update_fields = kwargs.get("update_fields")
+            if update_fields is not None:
+                kwargs["update_fields"] = {*update_fields, "content_hash"}
         super().save(*args, **kwargs)

Worth confirming whether the save view or any bulk path actually uses
update_fields today — if nothing does, this is hardening against a future
caller rather than a live bug.

3. release.yml — version check treats the tag as a regex

minor · Functional Correctness · .github/workflows/release.yml:19

Line 19 treats the tag version as a regular expression, so a tag like v1.0
can match a different source value such as 1-0. The workflow could then
publish a package under a tag that does not exactly match its version.

-        run: grep -q "^__version__ = \"${GITHUB_REF_NAME#v}\"$" netbox_drawio/version.py
+        run: grep -Fqx "__version__ = \"${GITHUB_REF_NAME#v}\"" netbox_drawio/version.py

---

Cheap hardening, low value

4. checks.py — report invalid max_size values during system checks

minor · Stability · netbox_drawio/checks.py:48-55

A non-integer max_size bypasses this check and later raises ValueError in
DiagramSerializer._validate_blob_size(). A negative value also bypasses it and
then rejects every non-empty diagram payload. Return a system-check error when
max_size is not a positive integer.

5. utils.py — require a callable restrict method

minor · Stability · netbox_drawio/utils.py:95-96

Line 95 only checks for an attribute named restrict. A non-callable value
passes the guard, gets registered, and then model.objects.restrict(...) at
netbox_drawio/views.py:65 raises TypeError.

-    if not hasattr(getattr(model, "objects", None), "restrict"):
+    if not callable(getattr(getattr(model, "objects", None), "restrict", None)):
         return "no `objects` manager with restrict()"

Would want a fixture with a non-callable restrict attribute to cover it.

---

Probably decline

6. CHANGELOG.md — soften the privacy guarantee

major · Security & Privacy · CHANGELOG.md:9-10

CodeRabbit argues the embed protocol sends diagram XML to the editor iframe via
postMessage, so code served from drawio_base_url can read that XML — and
that the changelog should therefore say the plugin makes no direct XML request
to the editor host, and require a trusted or self-hosted editor for sensitive
diagrams.

The mechanism is accurate; whether the changelog wording needs changing is a
docs judgement call.

7. test_api.py — pk-substitution comparison can pass for the wrong reason

minor · Functional Correctness · netbox_drawio/tests/test_api.py:181-201

str(forbidden.data).replace(str(self.devices[0].pk), "") replaces every
occurrence of the device pk anywhere in the serialized error. Test-created pks
are small integers, so the digits could also appear inside the diagram pk, a
field name, or an unrelated number — the masked strings could then match when
the errors differ, or differ when they are equivalent.

Suggested replacement:

-        # An existing-but-forbidden pk must be indistinguishable from a missing one
-        self.assertEqual(
-            str(forbidden.data).replace(str(self.devices[0].pk), "<pk>"),
-            str(missing.data).replace("999999999", "<pk>"),
-        )
+        # An existing-but-forbidden pk must be indistinguishable from a missing one
+        self.assertEqual(set(forbidden.data.keys()), set(missing.data.keys()))
+        for field in forbidden.data:
+            self.assertEqual(
+                [e.code for e in forbidden.data[field]],
+                [e.code for e in missing.data[field]],
+            )
+            self.assertNotIn(str(self.devices[0].pk), str(forbidden.data[field]))

The critique is theoretically right, but the test is deterministic under
--keepdb with reset pks, so this is a robustness nit rather than a defect.

---

Findings generated by CodeRabbit; the grouping and recommendations above are a
first pass and need a maintainer call, particularly on 6 and 7.