fix: skip re-export when content unchanged, avoid redundant backups - #74
fix: skip re-export when content unchanged, avoid redundant backups#74cgongac wants to merge 2 commits into
Conversation
Currently every full export (cmd+p "ibook export") re-creates every book
file. With backupWhenExist enabled, each run renames the existing file to
a -bk-{timestamp}.md copy even when content is identical, causing backup
files to pile up after major updates or repeated exports.
Change save() to:
- read existing file content and skip entirely when unchanged
- only back up (or remove) the old file when content actually differs
- still fall back to the old file path when backup is disabled, so
overwriting actually works instead of being swallowed by the
"file already exists" catch
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesFile replacement
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/export.ts`:
- Around line 104-119: Update generate() to await this.save(...) and ensure
callers such as the search flow await generate(). Serialize concurrent save
operations per filePath so each read, rename/remove, and create sequence
completes before another save for the same path begins.
- Around line 110-117: Update save() to preserve the existing export until
replacement succeeds: use Vault.modify() for existing files or create the
replacement at a temporary path before renaming it into place, including backup
behavior. Update generate() to await save(), and serialize concurrent exports so
they cannot overwrite or remove each other’s files.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (isExist) { | ||
| // skip if content is identical, avoid redundant backups (issue: #44, #69) | ||
| const oldContent = await this.plugin.app.vault.adapter.read(filePath); | ||
| if (oldContent === content) { | ||
| return; | ||
| } | ||
| if (this.plugin.settings.backupWhenExist) { | ||
| // backup file if file already exists | ||
| // issue: #44 | ||
| const backupPath = normalizePath(path.join(this.plugin.settings.output, `${fileName}-bk-${Date.now()}.md`)); | ||
| await this.plugin.app.vault.adapter.rename(filePath, backupPath); | ||
| } else { | ||
| // remove old file so create() below can overwrite it | ||
| await this.plugin.app.vault.adapter.remove(filePath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Await save() before allowing another export to start.
This code now performs a multi-step read, rename/remove, and create sequence. However, generate() calls this.save(...) without await at Line 71, and the supplied caller in src/ui/search.ts:51-61 also launches generate() without awaiting it. Two exports for the same filePath can both pass exists() and then race, causing failed replacements or lost updates. Await save() from generate() and serialize concurrent saves for the same path.
Suggested sequencing fix
- this.save(renderData.library.ZTITLE, content);
+ await this.save(renderData.library.ZTITLE, content);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/export.ts` around lines 104 - 119, Update generate() to await
this.save(...) and ensure callers such as the search flow await generate().
Serialize concurrent save operations per filePath so each read, rename/remove,
and create sequence completes before another save for the same path begins.
Address CodeRabbit review:
- await save() from generate() and the search flow so consecutive
exports cannot race on the same filePath
- keep the existing export intact while replacing it: when backup is
enabled, rename to -bk-{timestamp}.md then create the new file; when
backup is disabled, overwrite in place via Vault.modify() instead of
remove-then-create so a failed write never leaves the file missing
|
Thanks for the review! Both issues are addressed in the follow-up commit
Verified locally with |
1 participant
Summary
Currently every full export (
cmd + p→ibook export) re-creates every book file. When thebackupWhenExistsetting is enabled, each run renames the existing file to a-bk-{timestamp}.mdcopy — even when the content is identical. After a major update or repeated exports, this causes backup files to pile up indefinitely.There is also a related bug: when
backupWhenExistis disabled, an existing file is never updated at all —vault.create()throwsfile already exists, which is silently swallowed by the catch, so the old file stays forever.Changes (
src/export.ts—save())If the target file exists, read its content first:
backupWhenExiston → rename old file to-bk-{timestamp}.md, then write the new one (backup only happens on real changes).backupWhenExistoff → remove old file, then write the new one, so overwriting actually works.awaittherename/createcalls so operations are properly sequenced.Effect
-bk-*.mdpile-up on repeated full exports.Test
npm run build(tsc + esbuild) passes.npm run lintpasses (0 errors; pre-existing warnings insrc/typingsonly).Summary by CodeRabbit