From 9bb237f4eb6f1cde252e98340ab809354ad7677b Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 18 Feb 2026 23:04:00 -0600 Subject: [PATCH] Add ChangeBatcher Encoding Audit Plan - Introduced a new audit plan for ChangeBatcher encoding refactors, detailing changes in `encodeService.lua` and `encodeInstance.lua`, including the addition of a new encoding service and modifications to duplicate handling. - Documented findings related to code quality, test gaps, and documentation improvements, with specific tasks completed to enhance clarity and maintainability. - Updated related test files to ensure comprehensive coverage of the new encoding logic and property handling. These changes aim to improve the overall structure and documentation of the encoding process, ensuring better maintainability and understanding of the system. --- ...ngebatcher_encoding_audit_bade61c8.plan.md | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 .cursor/plans/changebatcher_encoding_audit_bade61c8.plan.md 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