Store undo history as serialized patches by nabeya11 · Pull Request #359 · seqsense/pcdeditor

nabeya11 · 2026-08-22T10:50:11Z

Problem

Every edit clones the whole point cloud in the WASM heap and pushes another full copy to the JS heap as undo history. For large PCDs the history alone keeps several times the file size resident, and since the WASM linear memory never shrinks, the per-edit clone turns the peak into permanent residency.

Changes

Groundwork for patch-based undo history; only the history mechanism is replaced.

The history uniformly stores serialized bytes of inverse-edit patches.
- Serialization keeps the history unaware of patch internals (more types to come).
- The old split storage — a Go-side header list plus raw bytes on the JS heap — is unified into a single self-contained byte sequence.
This PR implements only replacePatch (a whole-cloud snapshot); every edit records it, so behavior and memory characteristics stay the same. Follow-up PRs implement per-operation patches and switch edits over.
pop() is replaced by squashLatest(), which merges the two newest entries into one undo step.
The non-js stub (historyDummy) is replaced by a real implementation (historyMem), making undo behavior testable with plain go test. Added tests that apply random edit sequences, undo them all, and assert byte-exact restoration.

Replace the snapshot undo history (raw copies of the whole cloud plus a Go-side header list) with a patch-based history: each edit pushes a patch reverting it, serialized and stored uniformly on the JS heap. For now every edit type uses replacePatch, a whole-cloud snapshot, so behavior and memory characteristics are unchanged while the pipeline (push, serialized storage, undo by revert) is in place. Follow-ups replace the snapshot fallback with cheap per-operation patches and compress what remains. The non-js history stub becomes a real implementation (historyMem), making undo behavior testable with plain go test; randomized round-trip tests assert byte-exact restoration. Undo depth semantics of max_history are unchanged (N entries = N undos). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

codecov · 2026-08-22T10:51:21Z

Codecov Report

❌ Patch coverage is 83.51648% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.93%. Comparing base (836d903) to head (04661d1).

Files with missing lines	Patch %	Lines
patch.go	82.45%	11 Missing and 9 partials ⚠️
editor.go	86.48%	5 Missing ⚠️
undo.go	86.66%	3 Missing and 1 partial ⚠️
command.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
+ Coverage   39.34%   45.93%   +6.58%     
==========================================
  Files           8        9       +1     
  Lines        1426     1585     +159     
==========================================
+ Hits          561      728     +167     
+ Misses        829      808      -21     
- Partials       36       49      +13

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR refactors the editor undo history mechanism to store serialized “inverse edit” patches (currently implemented as a whole-cloud replacePatch) instead of storing full point cloud copies, laying groundwork for future per-operation patches and improved memory behavior (especially in WASM/JS).

Changes:

Introduces a patch serialization format (patch.go) and uses it to store undo history as packed patch bytes.
Replaces the previous undo history API with push(patch), undo(current), and squashLatest() and updates call sites accordingly.
Adds Go (non-js) in-memory history implementation plus new tests validating undo round-trips and history constraints.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
undo.go	Replaces dummy history with `historyMem` storing packed patch chunks for non-js builds.
undo_js.go	Refactors JS history to store packed patch bytes in JS `Uint8Array` chunks and adds `squashLatest()`.
patch.go	Adds patch interface plus `replacePatch` encoding/decoding and chunk reversion logic.
patch_test.go	Adds unit tests for `replacePatch` revert and encode/decode round-trip.
history_test.go	Adds randomized editor edit/undo round-trip tests, max history depth tests, and squash tests.
editor.go	Updates history interface and editor operations to record `replacePatch` snapshots on edits.
command.go	Replaces the previous `pop()` usage with `squashLatest()` for voxel-filter undo grouping.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

A failed revert used to discard the entry; a later undo would then apply an older patch to a state it was not recorded against. Keep the history intact and block undo at the broken entry instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

buf.Bytes() retains the grown capacity of the buffer, which can be nearly twice the content size and is held long-term by historyMem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

historyMem exists only in the non-js build; go vet for GOOS=js compiles test files too and failed on the reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Bound nFields by the minimal encoded field size so corrupted counts fail before allocating, and rewrite the viewpoint bound in the same multiplication-free form as the other guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Pushing a replacePatch serialized the whole cloud into a Go buffer before copying it to the JS heap, transiently holding extra full-size copies in the WASM linear memory, which never shrinks. Split the patch wire form into a head and a raw payload (encodeHead/payload) and copy both straight into one Uint8Array, restoring the memory behavior of the previous direct-copy implementation for snapshots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

at-wat · 2026-08-26T03:09:10Z

Randomized tests are good for fuzzing but not good for defining the desired behavior.
Basic tests are better to be written with explicit small examples.

at-wat · 2026-08-26T03:17:20Z

+const (
+	patchTypeLabel = iota + 1
+	patchTypeDelete
+	patchTypeAppend
+	patchTypeReplace
+)


Should be typed:

Suggested change

const (

patchTypeLabel = iota + 1

patchTypeDelete

patchTypeAppend

patchTypeReplace

)

type patchType byte

const (

patchTypeLabel patchType = iota + 1

patchTypeDelete

patchTypeAppend

patchTypeReplace

)

at-wat · 2026-08-26T03:24:37Z

+	// pp may be mutated; use the returned cloud
+	revert(pp *pc.PointCloud) (*pc.PointCloud, error)
+	// The wire form is the head followed by the raw payload
+	encodeHead(buf *bytes.Buffer)


Comments should explain the functions, not partial internal behavior

at-wat · 2026-08-26T03:32:14Z

+	buf.WriteByte(patchTypeReplace)
+	writeUint32(buf, math.Float32bits(p.header.Version))
+	writeUint32(buf, uint32(len(p.header.Fields)))
+	for i := range p.header.Fields {
+		writeString(buf, p.header.Fields[i])
+		writeUint32(buf, uint32(p.header.Size[i]))
+		writeString(buf, p.header.Type[i])
+		writeUint32(buf, uint32(p.header.Count[i]))
+	}
+	writeUint32(buf, uint32(p.header.Width))
+	writeUint32(buf, uint32(p.header.Height))
+	writeUint32(buf, uint32(len(p.header.Viewpoint)))
+	for _, v := range p.header.Viewpoint {
+		writeUint32(buf, math.Float32bits(v))
+	}
+	writeUint32(buf, uint32(len(p.data)))


Writing custom encoder/decoder for internal storage format makes it unnecessarily complex.
Consider using stdlib's encoding/glob or something others in stdlib.

at-wat · 2026-08-26T03:39:27Z

+type replacePatch struct {
+	header pc.PointCloudHeader
+	data   []byte
+}


Patch usually means a thing converting original state to desired state. So, the name replacePatch evokes having the desired state, however this seems having the original state.
Field names or interface/struct names are better to be fixed to explain what is stored.

at-wat · 2026-08-26T04:25:35Z

+	entry := h.entries[n-1]
+	chunks := make([][]byte, len(entry))
+	for i, c := range entry {
+		b := make([]byte, c.Get("byteLength").Int())
+		js.CopyBytesToGo(b, c)
+		chunks[i] = b
+	}
+	out, err := revertChunks(pp, chunks)


Copying all patches to Go memory and apply them later increases peak memory usage.
Better to copy and apply one by one.

at-wat · 2026-08-26T04:32:54Z

+	// pp may be mutated; use the returned cloud
+	revert(pp *pc.PointCloud) (*pc.PointCloud, error)
+	// The wire form is the head followed by the raw payload
+	encodeHead(buf *bytes.Buffer)


It doesn't need bytes.Buffer and io.Writer is enough

at-wat · 2026-08-26T04:45:19Z

+	return nil, nil, errUnknownPatchType
+}
+
+func revertChunks(pp *pc.PointCloud, chunks [][]byte) (*pc.PointCloud, error) {


This reverts pointcloud not chunks.

Also, chunk has almost no meaning. Actual meaning of the input is something like encoded/binary patch.

at-wat · 2026-08-26T04:56:44Z

This tests only test-only code.

To make some history handling logic testable,

abstract low layer history data storage (JS memory for product and Go memory for testing) like:
type historyEntryStorage interface { Store(...[]byte) historyEntry } type historyEntry interface { Load() []byte }

unify history class with history data storage interface

at-wat · 2026-08-26T05:01:01Z

+			e.push(&replacePatch{
+				header: e.pp.PointCloudHeader.Clone(),
+				data:   e.pp.Data,
+			})


Better to have

func newReplacePatch(*pc.PointCloud) patch

nabeya11 requested a lite review from Copilot August 22, 2026 10:50

Copilot started reviewing on behalf of nabeya11 August 22, 2026 10:50 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread undo.go

Comment thread undo_js.go

nabeya11 requested a lite review from Copilot August 22, 2026 12:14

Copilot started reviewing on behalf of nabeya11 August 22, 2026 12:14 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread undo.go

Comment thread undo_js.go

Comment thread patch.go

nabeya11 and others added 2 commits August 22, 2026 21:29

Copy packed patches into exact-sized slices

42832bc

buf.Bytes() retains the grown capacity of the buffer, which can be nearly twice the content size and is held long-term by historyMem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

nabeya11 requested a lite review from Copilot August 22, 2026 12:35

Copilot started reviewing on behalf of nabeya11 August 22, 2026 12:35 View session

Move the historyMem test into a non-js test file

8750452

historyMem exists only in the non-js build; go vet for GOOS=js compiles test files too and failed on the reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI reviewed Aug 22, 2026

View reviewed changes

Comment thread patch.go

Comment thread undo_js.go

Comment thread patch.go

nabeya11 and others added 2 commits August 22, 2026 21:52

nabeya11 requested a lite review from Copilot August 22, 2026 13:16

Copilot started reviewing on behalf of nabeya11 August 22, 2026 13:17 View session

Copilot AI reviewed Aug 22, 2026

View reviewed changes

nabeya11 requested a review from at-wat August 24, 2026 01:28

nabeya11 marked this pull request as ready for review August 24, 2026 01:28

at-wat reviewed Aug 26, 2026

View reviewed changes

Uh oh!

Conversation

nabeya11 commented Aug 22, 2026

Problem

Changes

Uh oh!

codecov Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 22, 2026 •

edited

Loading