Repository navigation
fix(export): give each exported document its own path - #2093
Conversation
When two documents in one folder share a title, the second gets a _2 suffix. The suffix was not checked, so if another document was already titled "Notes_2", both were written to Notes_2.md: the ZIP held two entries with that name and the manifest listed the path twice. Import reads the last entry for both, so one document never arrived. Keep counting until the path is free. Also apply ruff format to the file, which the backend-lint hook checks for every changed file.
|
@MohammadHijjawi97 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: MODSetter/SurfSense/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe export service now checks successive filename candidates until it finds an unused path. A new unit test covers duplicate document titles, including a title with a numeric suffix, and verifies unique manifest paths and matching document contents. ChangesDocument export paths
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change prevents colliding document paths in account exports and adds focused regression coverage. No actionable merge-blocking risk is identified; merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
What
When the exporter adds a
_2,_3suffix to a clashing title, it now keeps counting until the path is free, instead of taking the first suffix without checking it. Adds a unit test forbuild_account_export_zip. The file is also run throughruff format, since the backend-lint hook checks every changed file.Why
A suffixed name can already belong to another document. With
Notes,Notes_2andNotesin one folder, the third was written toNotes_2.mdas well:The manifest then listed the same path for two documents. Import opens members by name, so both entries read the last one, and the dedup key drops the second: one document is silently missing after the migration.
How to test
The new test fails before the change and passes after.
ruff check,ruff format --checkand bandit are clean on the changed files. Locally I ran these tests with--noconftestand a small stub forapp.config, because the root conftest loads an embedding model at import time.High-level PR Summary
This PR fixes a bug in the document export service where files with duplicate names (e.g.,
Notes,Notes_2,Notes) could overwrite each other during export. The fix ensures that when generating suffixed filenames for duplicates, the system keeps incrementing the suffix counter until it finds an unused path, preventing silent data loss during export/import cycles. The changes include the core fix in the export logic, formatting updates viaruff format, and a new unit test to verify the behavior.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_backend/tests/unit/services/test_export_document_paths.pysurfsense_backend/app/services/export_service.pySummary by CodeRabbit