ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

PR Summary by Qodo

Fix copyPadWithoutHistory changeset length, trailing newline drift, and async writes

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

• Fix copyPadWithoutHistory() to pack a total new length (not a delta) and generate a valid
 changeset.
• Prevent copies from gaining an extra newline by explicitly deleting the surplus newline.
• Harden copy/compaction integrity via new regression tests and stricter existing assertions.

graph TD
  A["API/Cleanup (compactPad)"] --> B["Pad.copyPadWithoutHistory"] --> C["Ops + SmartOpAssembler"] --> D["Changeset.pack"] --> E["dstPad.appendRevision"] --> F[("ueberDB / pad revs")]

Loading

The following are alternative approaches to this PR:

1. Rebuild destination from full AText snapshot

• ➕ Avoids manual newline bookkeeping by constructing a canonical AText and deriving changeset from that snapshot.
• ➕ Potentially simpler invariants: destination text length always derives from AText length.
• ➖ Still needs a correct changeset to persist rev1; requires careful integration with existing revision/pool expectations.
• ➖ May be more invasive than a focused fix and risk subtle attribute/author replay differences.

2. Initialize destination pad with a single newline (instead of internal "\n\n")

• ➕ Could remove the need for an explicit '-/1char/1line' op in the copy path.
• ➕ Makes copy semantics more intuitive (less special casing of rev-0).
• ➖ Likely affects broader pad initialization assumptions and may have compatibility impact across code paths.
• ➖ Higher blast radius than correcting the changeset header + trimming the char bank.

3. Add a repair/rehydration tool for already-corrupted pads

• ➕ Provides an operational path to fix historical data so keep-count cleanup and compaction work again.
• ➕ Can be run as a targeted maintenance job rather than requiring manual intervention.
• ➖ Additional scope and operational risk; needs careful detection and auditing to avoid data loss.
• ➖ Not necessary to stop new corruption; better as a follow-up PR.

Recommendation: Keep the PR’s surgical approach (correct total newLen, trim char bank, delete the surplus newline, and await async writes). It fixes the root correctness issue with minimal behavioral surface area, and the added integrity tests meaningfully reduce regression risk. Consider a separate follow-up for repairing already-corrupted pads.

Bug fix (1) +27 / -6

Pad.tsFix changeset packing, newline drift, and async durability in copyPadWithoutHistory() +27/-6

Fix changeset packing, newline drift, and async durability in copyPadWithoutHistory()

• Corrects copyPadWithoutHistory() to pass pack() the total new document length (oldLength + delta) instead of the delta. Adds an explicit delete op to remove the extra newline that otherwise accumulates due to opsFromAText() omitting the final newline, and trims the char bank accordingly. Also awaits saveToDatabase() and dstPad.appendRevision() to ensure failures surface as copy errors rather than unhandled rejections.

src/node/db/Pad.ts

Tests (3) +161 / -10

pad.tsAlign copyPadWithoutHistory HTML assertion with fixed newline behavior +6/-1

Align copyPadWithoutHistory HTML assertion with fixed newline behavior

• Updates the API test to strip a single trailing <br> (matching the direct getHTML test) now that pad copies no longer gain an extra newline. Adds inline commentary documenting the historical bug and why the expectation changed.

src/tests/backend/specs/api/pad.ts

compactPad.tsMake compactPad full-collapse test byte-exact and assert pad.check() +14/-9

Make compactPad full-collapse test byte-exact and assert pad.check()

• Strengthens the full-compaction test to require byte-exact text preservation instead of tolerating trailing whitespace drift caused by repeated copyPadWithoutHistory(). Adds an explicit post-compaction pad.check() to ensure the compacted pad remains valid and cleanable in future keep-count runs.

src/tests/backend/specs/compactPad.ts

copyPadWithoutHistoryIntegrity.tsAdd regression tests for copyPadWithoutHistory changeset integrity and compaction safety +141/-0

Add regression tests for copyPadWithoutHistory changeset integrity and compaction safety

• Introduces a focused backend spec verifying copied text equality, Changeset.checkRep() for revision 1, pad.check() on copies, stability across chained copies, and author attribution preservation. Also covers compactPad full-collapse invariants (text preserved and pad remains checkable/cleanable).

src/tests/backend/specs/copyPadWithoutHistoryIntegrity.ts

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Description

If dstPad.appendRevision() rejects, copyPadWithoutHistory() throws after creating the destination
pad and writing author/group metadata, leaving a partially-created destination pad behind. A retry
with force=false will then fail early with “destinationID already exists”.

Code

src/node/db/Pad.ts[R823-826]

+    // Must be awaited: an un-awaited rejection here (an invalid changeset,
+    // a failed write) surfaces as an unhandled rejection instead of failing
+    // the copy, which is how the length bug above went unnoticed.
+    await dstPad.appendRevision(changeset, authorId);

Evidence

The destination pad and related metadata are created/updated before the awaited appendRevision().
If appendRevision() throws, the function exits early with those side effects still present; on
retry, the existing-pad check will throw unless force=true.

src/node/db/Pad.ts[754-767]
src/node/db/Pad.ts[769-827]
src/node/db/Pad.ts[717-737]
src/node/db/Pad.ts[286-340]
src/node/db/PadManager.ts[109-151]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`copyPadWithoutHistory()` now correctly `await`s `dstPad.appendRevision(...)`, which means failures (db write, hook error, invariant error) will reject the copy. However, the function has already created/persisted the destination pad and updated related metadata (authors + group pad list). If `appendRevision()` fails, there is no rollback, so an incomplete destination pad can remain and a subsequent retry with `force=false` will fail with `destinationID already exists`.

### Issue Context
The destination pad is created via `padManager.getPad(...)` (which initializes and persists rev0). Then author/group side effects are applied before the awaited `appendRevision()`.

### Fix Focus Areas
- src/node/db/Pad.ts[746-843]
- src/node/db/Pad.ts[717-737]

### Suggested fix
- Wrap the destination-pad creation + revision append in a `try { ... } catch (err) { ... }`.
- In the `catch` block, best-effort remove the destination pad that was just created (for example `await dstPad.remove()`), then rethrow the original error.
- Consider moving `copyAuthorInfoToDestinationPad()` and the `group:${destGroupID}.pads` update to *after* the revision append succeeds, to reduce rollback work.
- Add a regression test that forces `appendRevision()` to throw (stub/hook) and asserts the destination pad does not exist afterward (or is retryable without `force=true`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗