fix: V1 import no longer fails on graphs with 1000+ same-named attachments - #960
Open
maccman wants to merge 4 commits into
Open
fix: V1 import no longer fails on graphs with 1000+ same-named attachments#960maccman wants to merge 4 commits into
maccman wants to merge 4 commits into
Conversation
…ments Every image pasted into Reflect V1 uploads as image.png, so a multi-year graph can hold thousands of distinct attachments sharing one filename. The import's collision probing only tried sequential suffixes (image-2.png … image-1000.png) and aborted the whole import once the cap was exhausted: "no free asset name after 1000 probes for image.png". Collision probing (both the import planner and the upload/import intake path) now switches to content-digest names (image-3f9ab2c1.png) after eight sequential probes. Digest candidates are effectively unique per distinct file, so dense stems resolve in a couple of probes, and they are deterministic, so re-importing the same export re-derives the same names and reuses the files already on disk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughAsset collision handling now tries the desired name and readable numeric suffixes before using deterministic SHA-256-derived names. Asset persistence and V1 import planning share this logic. Byte comparisons now use incremental reads. ChangesAsset name collision handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ImportPlanner
participant NameCandidates
participant AssetStorage
participant ByteComparator
ImportPlanner->>NameCandidates: request next asset name
NameCandidates-->>ImportPlanner: return sequential or digest-based candidate
ImportPlanner->>AssetStorage: check candidate availability
AssetStorage->>ByteComparator: compare existing and staged bytes
ByteComparator-->>ImportPlanner: report identical or different content
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/fs/assets.rs (1)
40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment on
SEQUENTIAL_NAME_PROBEShas a broken sentence."while a stem the graph is dense with jumps to digests instead of marching toward the probe cap" reads as missing a word (e.g., "whose graph is dense" or similar). Worth tightening for future readers trying to understand the switch-point rationale.
✏️ Suggested rewording
-/// Sequential `-2`-style probes before candidates switch to content-digest -/// names. Small on purpose: everyday collisions stay readable, while a stem -/// the graph is dense with jumps to digests instead of marching toward the -/// probe cap — a V1 import can carry thousands of pastes all named -/// `image.png`. +/// Sequential `-2`-style probes before candidates switch to content-digest +/// names. Small on purpose: everyday collisions stay readable, while a stem +/// whose graph is dense with reuse jumps to digests instead of marching +/// toward the probe cap — a V1 import can carry thousands of pastes all +/// named `image.png`.🤖 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 `@apps/desktop/src-tauri/src/fs/assets.rs` around lines 40 - 48, Rewrite the doc comment for SEQUENTIAL_NAME_PROBES to fix the missing-word sentence and clearly explain that stems with many collisions switch to content-digest candidates instead of approaching MAX_NAME_PROBES; leave the constants and surrounding behavior unchanged.
🤖 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.
Nitpick comments:
In `@apps/desktop/src-tauri/src/fs/assets.rs`:
- Around line 40-48: Rewrite the doc comment for SEQUENTIAL_NAME_PROBES to fix
the missing-word sentence and clearly explain that stems with many collisions
switch to content-digest candidates instead of approaching MAX_NAME_PROBES;
leave the constants and surrounding behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9153fcc-673f-4ad9-88b9-e0e75fddd4e2
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/fs/assets.rsapps/desktop/src-tauri/src/fs/import_assets.rs
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Do not merge this until I've done more investigation. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes asset import/persist failures in graphs with extreme filename collisions (e.g. thousands of distinct image.png attachments) by switching from long sequential suffix probing to deterministic content-digest-based names after a small readable suffix window.
Changes:
- Introduces a shared
NameCandidatesiterator to generate collision-resistant asset name candidates, including an SHA-256 prefix fallback after-2…-8probes. - Updates V1 import asset planning (
plan_asset_name) and upload/file-import persistence (persist_unique) to use the shared candidate generator while preserving reuse/idempotency behavior. - Adds tests covering digest-name fallback for both the import planning and persistence paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/desktop/src-tauri/src/fs/assets.rs | Adds NameCandidates and updates persist_unique to fall back to digest-based names after a small sequential window; includes new persistence test. |
| apps/desktop/src-tauri/src/fs/import_assets.rs | Switches plan_asset_name to use NameCandidates and adds a test ensuring deterministic digest naming and reuse behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1dfe7f1. Configure here.
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 `@apps/desktop/src-tauri/src/fs/import_assets.rs`:
- Around line 529-534: Update plan_asset_name so each digest candidate is
checked for exact-byte reuse before deciding it is occupied, preserving
immediate reuse when bytes match. After sequential probing completes, invoke
legacy_numbered_reuse_name once regardless of whether all digest candidates were
occupied, and only then select or report exhaustion for a free digest candidate.
- Around line 585-595: Update same_file_bytes to compare files using read_exact
on equal fixed-size buffer slices, iterating until existing_meta.len() bytes are
consumed. Do not return false merely because individual reads have different
lengths; compare each fully read chunk and preserve the true result only after
all bytes match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc8cd10a-d7d1-4a7a-9e84-c65e42126be5
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/fs/assets.rsapps/desktop/src-tauri/src/fs/import_assets.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/fs/assets.rs
Comment on lines
+529
to
+534
| if probe_count > super::assets::SEQUENTIAL_NAME_PROBES { | ||
| if let Some(name) = legacy_numbered_reuse_name(assets_dir, desired, staged, taken)? | ||
| { | ||
| return Ok(PlannedAssetName { name, reuse: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run the legacy reuse scan after checking the first digest candidate.
The legacy scan runs only when a digest candidate is free. If all digest candidates exist with different bytes, plan_asset_name returns the exhaustion error even when a legacy numbered file has identical bytes.
Check the current digest candidate for exact-byte reuse first. Then run legacy_numbered_reuse_name once after sequential probing, before selecting a free digest candidate.
Proposed fix
let mut candidates = super::assets::NameCandidates::new(desired, staged.to_path_buf());
let mut probe_count = 0;
+ let mut legacy_reuse_checked = false;
while let Some(candidate) = candidates.next()? {
probe_count += 1;
if taken.contains(&candidate) {
continue;
}
let target = assets_dir.join(&candidate);
+ if target.is_file() && same_file_bytes(&target, staged)? {
+ return Ok(PlannedAssetName {
+ name: candidate,
+ reuse: true,
+ });
+ }
+ if !legacy_reuse_checked
+ && probe_count > super::assets::SEQUENTIAL_NAME_PROBES
+ {
+ legacy_reuse_checked = true;
+ if let Some(name) = legacy_numbered_reuse_name(assets_dir, desired, staged, taken)? {
+ return Ok(PlannedAssetName { name, reuse: true });
+ }
+ }
if !target.exists() && !super::io::file_occupied(&target) {
- if probe_count > super::assets::SEQUENTIAL_NAME_PROBES {
- if let Some(name) = legacy_numbered_reuse_name(assets_dir, desired, staged, taken)?
- {
- return Ok(PlannedAssetName { name, reuse: true });
- }
- }
return Ok(PlannedAssetName {
name: candidate,
reuse: false,
});
}
- if target.is_file() && same_file_bytes(&target, staged)? {
- return Ok(PlannedAssetName {
- name: candidate,
- reuse: true,
- });
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if probe_count > super::assets::SEQUENTIAL_NAME_PROBES { | |
| if let Some(name) = legacy_numbered_reuse_name(assets_dir, desired, staged, taken)? | |
| { | |
| return Ok(PlannedAssetName { name, reuse: true }); | |
| } | |
| } | |
| let mut candidates = super::assets::NameCandidates::new(desired, staged.to_path_buf()); | |
| let mut probe_count = 0; | |
| let mut legacy_reuse_checked = false; | |
| while let Some(candidate) = candidates.next()? { | |
| probe_count += 1; | |
| if taken.contains(&candidate) { | |
| continue; | |
| } | |
| let target = assets_dir.join(&candidate); | |
| if target.is_file() && same_file_bytes(&target, staged)? { | |
| return Ok(PlannedAssetName { | |
| name: candidate, | |
| reuse: true, | |
| }); | |
| } | |
| if !legacy_reuse_checked | |
| && probe_count > super::assets::SEQUENTIAL_NAME_PROBES | |
| { | |
| legacy_reuse_checked = true; | |
| if let Some(name) = legacy_numbered_reuse_name(assets_dir, desired, staged, taken)? { | |
| return Ok(PlannedAssetName { name, reuse: true }); | |
| } | |
| } | |
| if !target.exists() && !super::io::file_occupied(&target) { | |
| return Ok(PlannedAssetName { | |
| name: candidate, | |
| reuse: false, | |
| }); | |
| } | |
| } |
🤖 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 `@apps/desktop/src-tauri/src/fs/import_assets.rs` around lines 529 - 534,
Update plan_asset_name so each digest candidate is checked for exact-byte reuse
before deciding it is occupied, preserving immediate reuse when bytes match.
After sequential probing completes, invoke legacy_numbered_reuse_name once
regardless of whether all digest candidates were occupied, and only then select
or report exhaustion for a free digest candidate.
Comment on lines
+585
to
+595
| loop { | ||
| let existing_read = existing_file.read(&mut existing_buffer)?; | ||
| let staged_read = staged_file.read(&mut staged_buffer)?; | ||
| if existing_read != staged_read { | ||
| return Ok(false); | ||
| } | ||
| if existing_read == 0 { | ||
| return Ok(true); | ||
| } | ||
| if existing_buffer[..existing_read] != staged_buffer[..staged_read] { | ||
| return Ok(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -fsSL https://doc.rust-lang.org/std/io/trait.Read.html |
rg -n -C 2 'smaller than the length of the buffer|shorter'Repository: team-reflect/reflect-open
Length of output: 163
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target function context ---'
sed -n '530,620p' apps/desktop/src-tauri/src/fs/import_assets.rs
printf '%s\n' '--- related symbols and call sites ---'
rg -n -C 3 'same_file_bytes|existing_meta|existing_buffer|staged_buffer|asset reuse|reuse' apps/desktop/src-tauri/src/fs/import_assets.rs
printf '%s\n' '--- Read::read documentation available locally ---'
rustc --print sysroot 2>/dev/null || true
rg -n -C 3 'smaller than the length of the buffer|short read|shorter than' "$(rustc --print sysroot 2>/dev/null)/share/doc/rust/html/std/io/trait.Read.html" 2>/dev/null || trueRepository: team-reflect/reflect-open
Length of output: 9042
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Rust Read contract ---'
curl -fsSL https://doc.rust-lang.org/std/io/trait.Read.html |
rg -n -C 4 'It is not an error|fewer bytes|less than|short read|number of bytes' |
head -n 80
printf '%s\n' '--- read_exact contract ---'
curl -fsSL https://doc.rust-lang.org/std/io/trait.Read.html |
rg -n -C 4 'read_exact|UnexpectedEof' |
head -n 80
printf '%s\n' '--- relevant tests ---'
sed -n '740,885p' apps/desktop/src-tauri/src/fs/import_assets.rsRepository: team-reflect/reflect-open
Length of output: 33925
Do not treat different short-read lengths as different file contents.
Read::read permits short reads before EOF. Two reads from identical files can return different lengths, causing same_file_bytes to return false before comparing all bytes.
Use read_exact on equal fixed-size slices until existing_meta.len() is consumed.
🤖 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 `@apps/desktop/src-tauri/src/fs/import_assets.rs` around lines 585 - 595,
Update same_file_bytes to compare files using read_exact on equal fixed-size
buffer slices, iterating until existing_meta.len() bytes are consumed. Do not
return false merely because individual reads have different lengths; compare
each fully read chunk and preserve the true result only after all bytes match.
Contributor
|
@maccman is this one ready for review? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A user importing a multi-year Reflect V1 graph hit a hard import failure:
Every image pasted into Reflect V1 uploads under the clipboard's generic filename,
image.png, and Firebase serves that name back viaContent-Disposition. The import names downloaded attachments after that original filename, and its collision probing only tried sequential suffixes:image.png,image-2.png, …image-1000.png. A graph with more than 1000 distinct pasted screenshots exhausts the cap on the 1001st one, andplan_asset_nameaborts the entire import — deterministically, so retries and restarts can never help (which is exactly what the reporter saw).The same 1000-probe cliff exists in
persist_unique, the intake path behind in-app paste/drop uploads and file-picker imports, so a graph dense with one filename would eventually break ordinary pasting too.Before → After
image.png,image-2.png, …image.pngattachmentsimage-<8-hex-sha256>.pngafter the readable suffixes fill upChanges
fs/assets.rs— newNameCandidatesprober shared by both intake paths. Candidate sequence: the desired name, then readable-2…-8suffixes (SEQUENTIAL_NAME_PROBES = 8), then names carrying the first 8 hex chars of the file's sha256 (image-3f9ab2c1.png,image-3f9ab2c1-2.png, …). The digest is computed lazily — the file is only read once sequential probing runs dry — and cached per sequence.MAX_NAME_PROBES = 1000remains as a backstop, but digest candidates are effectively unique per distinct file, so real graphs resolve in a couple of probes.persist_uniquenow iteratesNameCandidates; itspersist_noclobberclaim semantics (collision check and claim, race-free) are unchanged.fs/import_assets.rs—plan_asset_namedrops its private copy of the sequential loop and cap and iterates the sharedNameCandidatesinstead. Determinism matters here: planning the same bytes again re-derives the same digest name, which is how a re-import finds the file it wrote last time and marks itreuseinstead of duplicating it. Thetaken-set check, on-disk existence/file_occupiedchecks, and identical-bytes reuse logic are unchanged.Digest names are content-derived, not random, on purpose: import retries and re-imports stay idempotent, and two distinct files whose 8-hex prefixes ever collided would simply fall through to the
-2variant of the digest name.Tests
plan_switches_to_digest_names_when_sequential_probes_exhaust(import_assets.rs) — withimage.pngthroughimage-8.pngall taken, the plan lands on an 8-hex digest name; the same bytes re-plan to the same name withreuse: true; different bytes land beside it under their own digest.persist_switches_to_digest_names_when_sequential_probes_exhaust(assets.rs) — the upload path falls over to a digest name once the sequential window is dense on disk, and the persisted bytes are intact.plan_reuses_identical_bytes_and_probes_conflicts,persist_probes_numbered_suffixes_on_collision, …) pass unchanged, pinning that behavior below the 8-suffix window is identical to before.Verification
cargo test --libinapps/desktop/src-tauri: 330 passed, 0 failed.cargo clippy --lib: clean.Risk / Rollout
🤖 Generated with Claude Code
Note
Low Risk
Scoped to new asset filenames after heavy collisions; first eight suffixes and noclobber/reuse semantics are unchanged, with no migration of existing files.
Overview
Fixes hard import/upload failures when a graph has more than ~1000 distinct files sharing one stem (e.g. thousands of V1
image.pngpastes). Collision handling no longer stops atimage-1000.png; after eight readable-2…-8suffixes it uses deterministic names from the first 8 hex chars of the file’s SHA256 (image-3f9ab2c1.png, with-2variants if needed).fs/assets.rsintroduces sharedNameCandidates(lazy hash,SEQUENTIAL_NAME_PROBES = 8) and wirespersist_unique(paste/drop and file-picker intake) through it.fs/import_assets.rsdrops its duplicate probe loop;plan_asset_nameuses the same prober, addslegacy_numbered_reuse_nameso re-imports can still reuse olderimage-9.png-style files after the digest switch, andsame_file_bytescompares large files incrementally instead of loading them whole.Reviewed by Cursor Bugbot for commit 3be517a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
-2through-8) for initial filename conflicts.