diff --git a/.cursor/plans/changebatcher_encoding_audit_bade61c8.plan.md b/.cursor/plans/changebatcher_encoding_audit_bade61c8.plan.md new file mode 100644 index 00000000..3194ecbd --- /dev/null +++ b/.cursor/plans/changebatcher_encoding_audit_bade61c8.plan.md @@ -0,0 +1,211 @@ +--- +name: ChangeBatcher Encoding Audit +overview: Audit of the ChangeBatcher encoding refactors covering encodeService.lua (new), encodeInstance.lua (modified), and related test changes across 4 files (132 insertions, 63 deletions). +todos: + - id: fix-header-comment + content: "Fix stale header comment in encodeInstance.lua (Finding 1): update DUPLICATE HANDLING section to say duplicates are encoded and delegated to server" + status: completed + - id: extract-shared-filter + content: Extract shared property encoding filter from encodeService.lua and encodeInstance.lua into a common helper to eliminate duplication (Finding 2) + status: completed + - id: add-encodeService-tests + content: Create encodeService.spec.lua with tests for property encoding, Ref carrier logic, Attributes/Tags, EXCLUDE_PROPERTIES, and empty services (Finding 3) + status: completed + - id: document-clocktime-exclude + content: Add comment to EXCLUDE_PROPERTIES in encodeService.lua explaining why Lighting.ClockTime is excluded (Finding 4) + status: completed + - id: fix-servicechunk-doc + content: Fix misleading doc comment on ServiceChunk in src/web/interface.rs to correctly describe refs as 1-based carrier indices (Finding 5) + status: completed +isProject: false +--- + +# ChangeBatcher Encoding Refactors -- Audit Report + +## Scope + +4 files changed: +132 / -63 across 13 commits. + + +| File | Change Type | Lines | +| -------------------------------- | --------------- | -------------------------------- | +| `encodeService.lua` | New (108 lines) | +108 | +| `encodeInstance.lua` | Modified | +24 / -53 | +| `encodeInstance.spec.lua` | Modified | +3 / -4 | +| `encodeInstance.stress.spec.lua` | Modified | -6 (removed skippedCount checks) | + + +--- + +## Architecture Summary + +```mermaid +flowchart TD + subgraph TwoWaySync [Two-Way Sync Path] + InstanceMap["InstanceMap signals"] --> ChangeBatcher + ChangeBatcher --> createPatchSet + createPatchSet --> encodePatchUpdate + encodePatchUpdate --> encodeProperty + end + + subgraph LiveSyncback [Live Syncback Path] + performSyncback["App:performSyncback()"] --> encodeService + encodeService --> encodeProperty + performSyncback --> encodeInstance + encodeInstance --> encodeProperty + end + + encodeProperty --> RbxDom + encodeService --> propertyFilter + encodeInstance --> propertyFilter + encodePatchUpdate --> propertyFilter +``` + + + +`encodeService` is new and serves the live syncback feature. `encodeInstance` was refactored to delegate duplicate handling to the server. + +--- + +## Finding 1: Stale Header Comment (Bug) + +**Severity: Moderate** | **File:** `[encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua)` lines 1-17 + +The module header comment claims: + +``` +DUPLICATE HANDLING: +- Instances with duplicate-named siblings at ANY level of the path are skipped +``` + +But the code was changed to do the opposite -- duplicates are now **encoded** (not skipped), with a debug log, and the server handles dedup. The header directly contradicts the code behavior. + +**Fix:** Update the header to reflect the new behavior. + +--- + +## Finding 2: Duplicated Encoding Filter Logic (Code Quality) + +**Severity: Moderate (small refactor candidate)** | **Files:** `[encodeService.lua](plugin/src/ChangeBatcher/encodeService.lua)`, `[encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua)` + +The property encoding filter is copy-pasted across both files with only minor differences: + + +| Logic | `encodeService.lua` | `encodeInstance.lua` | +| --------------------------------------------- | ------------------- | -------------------- | +| SKIP_PROPERTIES (Parent, Name, Archivable) | Lines 7-11 | Lines 34-38 | +| Skip Attributes/Tags (handled separately) | Line 33 | Lines 217-219 | +| Scriptability check (ReadWrite, Read, Custom) | Lines 37-39 | Lines 224-226 | +| DoesNotSerialize check | Line 40 | Line 227 | +| UNENCODABLE_DATA_TYPES filter | Line 45 | Line 233 | +| Attributes encoding (pcall + descriptor) | Lines 70-81 | Lines 121-138 | +| Tags encoding (pcall + descriptor) | Lines 83-94 | Lines 141-157 | + + +If either filter is updated (e.g., adding a new skip property, changing the scriptability check), the other must be updated identically or a divergence bug is introduced. + +**Fix:** Extract a shared `iterEncodableProperties(className, callback)` helper that handles the filter logic once. Both `encodeService` and `encodeInstance` would call it. The Attributes/Tags encoding blocks could also be extracted to a shared `encodeAttributesAndTags(instance, properties)` function (the `encodeAttributes`/`encodeTags` functions in `encodeInstance.lua` already do this but aren't exported). + +--- + +## Finding 3: No Tests for `encodeService.lua` (Test Gap) + +**Severity: Moderate** | **File:** `encodeService.lua` (new, 108 lines, no spec file) + +Every other encoding module has a `.spec.lua` test: + +- `encodeInstance.spec.lua` (exists) +- `encodeInstance.stress.spec.lua` (exists) +- `encodePatchUpdate.spec.lua` (exists) +- `encodeProperty.spec.lua` (exists) +- `**encodeService.spec.lua` -- MISSING** + +`encodeService` handles Ref property encoding via ObjectValue carriers (a tricky serialization path), service-level property filtering, and the carrier index calculation. These all warrant test coverage. + +**Fix:** Add `encodeService.spec.lua` covering: + +- Basic property encoding for a service (e.g., Lighting) +- Ref property carrier creation and index correctness +- Attributes/Tags encoding on services +- EXCLUDE_PROPERTIES filtering (Lighting.ClockTime) +- Empty service (no properties, no children) + +--- + +## Finding 4: `EXCLUDE_PROPERTIES` Undocumented (Documentation) + +**Severity: Low** | **File:** `[encodeService.lua](plugin/src/ChangeBatcher/encodeService.lua)` lines 13-15 + +```lua +local EXCLUDE_PROPERTIES = { + Lighting = { ClockTime = true }, +} +``` + +`ClockTime` (Float32) and `TimeOfDay` (String) are linked properties on Lighting -- they represent the same value in different formats, and both have `Serialization: "Serializes"`. Excluding `ClockTime` avoids sending a redundant/conflicting representation alongside `TimeOfDay`. This is a reasonable design choice, but there is no comment explaining the reasoning. If someone sees `TimeOfDay` behaving oddly, they won't know `ClockTime` was intentionally excluded or why. + +**Fix:** Add a comment explaining the linked-property reason. + +--- + +## Finding 5: Misleading `ServiceChunk` Doc Comment (Documentation) + +**Severity: Low** | **File:** `[src/web/interface.rs](src/web/interface.rs)` lines 30-42 + +The doc comment on `ServiceChunk` states: + +> `refs` maps property names to 1-based indices into this combined range (0 = nil). Indices <= child_count point directly to a child instance. Indices > child_count point to an ObjectValue carrier. + +But the server code in `[serve.rs](src/cli/serve.rs)` lines 198-204 treats `refs` values as **1-based indices into the carrier slice only** (not the combined range): + +```rust +let carriers = &service_range[carrier_start..carrier_end]; +for (prop_name, &idx) in &chunk.refs { + if idx == 0 || (idx as usize) > carriers.len() { continue; } + ref_entries.push((prop_name.clone(), carriers[idx as usize - 1])); +} +``` + +And the plugin sets `refs[propertyName] = #refTargets` (1-based into refTargets only). The "indices into combined range" and "indices <= child_count point to a child" parts of the comment are incorrect -- carriers never point to children. + +**Fix:** Update the doc comment to say `refs` maps property names to 1-based indices into the carrier slice. + +--- + +## Positive Changes (No Action Needed) + +### P1: Module-Level Mutable State Removed (encodeInstance.lua) + +The old code had `local skippedDuplicateCount = 0` at module scope, mutated across calls and reset inside a wrapper function. This was a shared mutable state pattern that risked stale counts if the wrapper wasn't used. The new code removes this entirely -- clean improvement. + +### P2: Duplicate Handling Delegated to Server + +Previously, `encodeInstance` skipped any instance whose path contained duplicate-named siblings, silently losing data. Now all instances are encoded and sent to the server, which handles filesystem name collisions via the dedup suffix system (`~2`, `~3`, etc.). This is the correct design -- the server has the filesystem context needed to resolve collisions, while the plugin should send complete data. + +### P3: `Custom` Scriptability Added + +The filter now includes `propertyMeta.scriptability == "Custom"` alongside `ReadWrite` and `Read`. The reflection database has 9 `Custom` scriptability properties. After accounting for those handled separately (Attributes, Tags, Source, Contents), this adds coverage for `Scale` (NumberValue), `WorldPivotData` (Model), `PropertiesSerialize` (StyleSheet), and `MaterialColors` (Workspace) -- all of which serialize and were previously silently dropped. + +### P4: Simplified Wrapper Function + +The `encodeInstanceWithDuplicateTracking` wrapper was replaced with a simple `return function(instance, parentId)` that calls `encodeInstance(instance, parentId, false)`. Tests updated to match (no more second `skippedCount` return value). + +### P5: Tests Updated Correctly + +Both `encodeInstance.spec.lua` and `encodeInstance.stress.spec.lua` are updated to assert that duplicates ARE included (e.g., `expect(#encoded.children).to.equal(3)` instead of `1`). All `skippedCount` assertions removed. + +--- + +## Summary + + +| # | Finding | Severity | Type | +| --- | ------------------------------------------ | -------- | --------------------- | +| 1 | Stale header comment in encodeInstance.lua | Moderate | Bug (misleading docs) | +| 2 | Duplicated encoding filter logic | Moderate | Code quality | +| 3 | No tests for encodeService.lua | Moderate | Test gap | +| 4 | EXCLUDE_PROPERTIES undocumented | Low | Documentation | +| 5 | Misleading ServiceChunk doc comment | Low | Documentation | + + +No critical or round-trip-identity issues found. The encoding logic is correct, the server-side consumption matches, and the refactors improve the codebase. The main action items are updating stale comments, adding a test file, and extracting the duplicated filter logic. \ No newline at end of file