feat(dedupe): give the choice of which copy to keep back to the user - #545
Conversation
… into feat/dedupe-selection-and-readout
Four things from the 2026-07-31 batch on #347, all in the Find Duplicates dialog. **Any copy can be ticked.** The first file of every group had a disabled checkbox, so the one copy you could not delete was whichever the scan happened to return first — reported as the dialog insisting that the file actually named " (Copy)" was the keeper. The invariant that was protecting (never delete every copy of something) is now watched rather than enforced by amputation: a group with all of its copies ticked is highlighted and blocks the delete, saying why. Which copy survives is simply the one left unticked, and the KEEP/DELETE badge follows the selection instead of the row index. **The button now says what it does.** "Select All Duplicates" skipped one file per group. It is now "Select all but one per group", and a real Select All sits beside it. What "one" means is a policy the user picks: shortest name (the default — " (copy)", " (1)" and " - Copy" make the derived file the longer name almost every time), smallest, largest, or first found. **Rows carry their own byte size.** The engine reported one size per group, which is true in exact mode and false in fuzzy mode, where the members are not the same file. `DuplicateGroup.file_sizes` now runs parallel to `files` through the engine and the command, each row shows its size and how far it is from the largest in the group, and Sort offers "Byte-size spread". That last one is Ehud's observation made usable: the pair that looked identical differed by a rounding error, the pair that was clearly different by a third. It is an ordering, not a clustering metric — a re-encode moves the byte count a lot and the perceptual hash barely at all, which is exactly the case worth catching. **The hash is readable.** 64 hex digits per row is a wall; the first 32 are 128 bits, past any collision anyone will meet, and the full value stays one hover or one copy away. It also stays the grouping key: there is no reason to weaken the key of an operation that deletes files when the digest is computed in full either way. Also: rows grow a thumbnail of the file, clickable into the app's viewer where AeroImage can edit it — deciding between two near-identical pictures from their names and byte counts alone was guesswork. And the results list gets its scrollbar back. It had none because `html.modal-open` hides every `::-webkit-scrollbar` on the page while a modal is open (WebKitGTK paints native bars above CSS overlays), and the selector matched the modal's own content too. `.modal-scroll` opts it back in at 14px. One correctness fix found on the way: `file_hashes` and `file_sizes` are indexed by position in `files`, and the post-delete rebuild filtered `files` alone — which would have shifted every surviving row's hash and size by the number of copies deleted above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (30)
🚧 Files skipped from review as they are similar to previous changes (29)
📝 WalkthroughWalkthroughThe PR adds per-file duplicate sizes across the backend and frontend. It updates duplicate selection, sorting, previews, deletion confirmation, hash display, modal scrolling, and localization. It also adds scoped thumbnail signatures and remote preview integration. ChangesDuplicate management
Thumbnail workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
…nto feat/dedupe-selection-and-readout # Conflicts: # src/components/DuplicateFinderDialog.tsx
…nto feat/dedupe-selection-and-readout
… into feat/dedupe-selection-and-readout
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/DuplicateFinderDialog.tsx (1)
239-253: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
keepPolicyto thescandependency list.
scanreadskeepPolicyat line 242 but does not list it. The callback identity only changes withscanPath,mode, orappliedThreshold. If the user changes the keep policy and then presses Retry (line 691),scanruns with the policy captured earlier and pre-selects the wrong copies.🐛 Proposed fix
- }, [scanPath, mode, appliedThreshold]); + }, [scanPath, mode, appliedThreshold, keepPolicy]);🤖 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/components/DuplicateFinderDialog.tsx` around lines 239 - 253, Update the dependency array for the scan callback in DuplicateFinderDialog so it includes keepPolicy, ensuring Retry uses the current policy when calling selectionForPolicy. Preserve the existing scanPath, mode, and appliedThreshold dependencies and selection behavior.src/components/ImageThumbnail.tsx (1)
47-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset thumbnail state when the cache key changes.
On a cache miss, both components retain the previous file's
src. They also retain a previous failure state. A recycled component can show the old file thumbnail for the new path. A successful request can remain hidden behind the fallback.Track the loaded cache key with the thumbnail state, or hide the source when it does not match
cacheKey. Clear the failure state before a new request. Clear it again after a successful response. Add regression coverage for changing a mounted component from one uncached key to another.
src/components/ImageThumbnail.tsx#L47-L77: prevent a priorsrcorerrorvalue from rendering for a newcacheKey.src/components/ProviderThumbnail.tsx#L45-L72: prevent a priorsrcorfailedvalue from rendering for a newcacheKey.🤖 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/components/ImageThumbnail.tsx` around lines 47 - 77, Reset thumbnail state when cacheKey changes so ImageThumbnail does not render a previous src or error during a cache miss; clear the failure state before loading and after success, and ensure the loaded source belongs to the current cache key. Apply the equivalent reset and cache-key validation in src/components/ImageThumbnail.tsx lines 47-77 and src/components/ProviderThumbnail.tsx lines 45-72, then add regression coverage for changing a mounted component between uncached keys.
🟡 Minor comments (21)
src/components/DuplicateFinderDialog.tsx-781-788 (1)
781-788: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove
cursor-pointerfrom the row, or give the row a click handler.The row keeps
cursor-pointer, but the rowdivno longer has anonClick. Clicking the row body does nothing. The child buttons still calle.stopPropagation(), which implies a row handler was expected. This file already records the same defect class at lines 599-601.Either wire the row to
toggleFile(filePath)or drop the class.🐛 Proposed fix: make the row toggle the selection
<div key={filePath} + onClick={() => toggleFile(filePath)} className={`flex items-start gap-3 px-3 py-2 cursor-pointer transition-colors ${🤖 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/components/DuplicateFinderDialog.tsx` around lines 781 - 788, Update the duplicate-file row div identified by its key={filePath} and isChecked class logic to add an onClick that calls toggleFile(filePath), preserving the existing child-button stopPropagation behavior so row clicks toggle selection without affecting button actions.src/components/DuplicateFinderDialog.tsx-766-774 (1)
766-774: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFormat negative size deltas instead of reusing
formatBytes(delta).
formatBytesreturns0 Bfor negative input, so a smaller copy currently renders as−0 B. Compute the mismatch once per group, then render the magnitude with an explicit sign.🤖 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/components/DuplicateFinderDialog.tsx` around lines 766 - 774, Update the duplicate-size delta handling around fileSize, largest, and delta in DuplicateFinderDialog so negative differences are not passed directly to formatBytes. Compute the group mismatch once, then format its absolute magnitude and apply an explicit negative sign for smaller copies while preserving the existing null/zero behavior.src/i18n/locales/mk.json-3431-3431 (1)
3431-3431: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the size-spread label.
sortSizeSpreadsorts duplicate groups byMath.max(...sizes) - Math.min(...sizes), so"Разлика во големина"underspecifies the group size range. Use wording such as"Распон на големини"and confirm it with a Macedonian reviewer.🤖 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/i18n/locales/mk.json` at line 3431, Update the Macedonian translation for sortSizeSpread to clearly describe the size range or spread of duplicate groups, using wording such as “Распон на големини” rather than the current ambiguous label, and confirm the phrasing with a Macedonian reviewer.src/i18n/locales/mk.json-3437-3437 (1)
3437-3437: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the retained copy explicit in
selectAllButOne.
selectAllButOnerunssetSelectionForPolicy(groups, keepPolicy), so every group retains one file. Use wording that makes the retention guarantee explicit and matches the delete-action wording.Proposed wording
- "selectAllButOne": "Избери ги сите освен по една од група", + "Избери ги сите освен по една копија од секоја група",🤖 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/i18n/locales/mk.json` at line 3437, Update the selectAllButOne translation value to explicitly state that one file is kept in each group, matching the corresponding delete-action wording while preserving the key and locale structure.src/i18n/locales/nl.json-3437-3439 (1)
3437-3439: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse precise Dutch selection wording.
“Alles op één na” is less direct than “Alles behalve één”. “Gemarkeerde groep” is ambiguous because the UI uses selection terminology. Use “geselecteerde groep” in the deletion warning.
Proposed wording
- "selectAllButOne": "Alles op één na per groep selecteren", + "selectAllButOne": "Alles behalve één per groep selecteren", "everyCopyTicked": "alle kopieën aangevinkt", - "everyCopyTickedHint": "Vink in elke gemarkeerde groep één kopie uit: anders worden alle kopieën verwijderd." + "everyCopyTickedHint": "Vink in elke geselecteerde groep één kopie uit; anders worden alle kopieën verwijderd."🤖 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/i18n/locales/nl.json` around lines 3437 - 3439, Update the Dutch locale strings selectAllButOne and everyCopyTickedHint: replace “Alles op één na” with “Alles behalve één” and change “gemarkeerde groep” to “geselecteerde groep,” preserving the rest of the wording.src/i18n/locales/no.json-3431-3431 (1)
3431-3431: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a spread-specific Norwegian label.
sortSizeSpreadsorts groups by the range between their smallest and largest byte sizes.Størrelsesforskjellcan read as a generic pairwise difference. UseStørrelsesspredningorSpredning i størrelse.🤖 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/i18n/locales/no.json` at line 3431, Update the Norwegian translation value for the sortSizeSpread key to use a spread-specific label, preferably “Størrelsesspredning” or “Spredning i størrelse,” while leaving the key and surrounding translations unchanged.src/i18n/locales/ca.json-3439-3439 (1)
3439-3439: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the deletion warning.
Line 3439 says that deselecting one copy “thus” deletes all copies. This reverses the intended meaning. Use wording such as:
Desmarqueu una còpia de cada grup destacat; altrament s'esborrarien totes les còpies.🤖 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/i18n/locales/ca.json` at line 3439, Update the everyCopyTickedHint Catalan translation so it states that all copies would be deleted if one copy from each highlighted group is not deselected, replacing the current causal wording with the intended warning meaning.src/i18n/locales/da.json-3434-3435 (1)
3434-3435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the size-policy labels explicit.
"Mindste"and"Største"do not identify what the policy compares. The dialog uses these options to choose the retained duplicate before deletion. Use"Mindste fil"and"Største fil"or"Mindste størrelse"and"Største størrelse".Proposed translation update
- "keepSmallest": "Mindste", - "keepLargest": "Største", + "keepSmallest": "Mindste fil", + "keepLargest": "Største fil",🤖 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/i18n/locales/da.json` around lines 3434 - 3435, Update the Danish translation values for the keepSmallest and keepLargest properties in the da.json file to explicitly reference what is being compared. Replace the ambiguous "Mindste" and "Største" with either "Mindste fil" and "Største fil" (to indicate smallest/largest file) or "Mindste størrelse" and "Største størrelse" (to indicate smallest/largest size), ensuring the labels clearly convey that the size-policy is comparing file sizes for duplicate retention decisions.src/i18n/locales/et.json-3430-3430 (1)
3430-3430: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTranslate the fuzzy-cutoff placeholder.
Line 3430 uses the English shorthand
"auto"in the Estonian locale. Replace it with"automaatne"to keep this duplicate control localized.🤖 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/i18n/locales/et.json` at line 3430, Update the fuzzyCutoffPlaceholder value in the Estonian locale from the English shorthand to the localized Estonian string “automaatne”, preserving the existing key and structure.src/i18n/locales/sw.json-3430-3430 (1)
3430-3430: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTranslate the
fuzzyCutoffPlaceholdervalue.
fuzzyCutoffPlaceholderis used as the input placeholder inDuplicateFinderDialog, so"auto"shows English text in the Swahili UI. Replace it with a Swahili label such as"Otomatiki"so the placeholder is localized.Proposed fix
- "fuzzyCutoffPlaceholder": "auto", + "fuzzyCutoffPlaceholder": "Otomatiki",🤖 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/i18n/locales/sw.json` at line 3430, Update the fuzzyCutoffPlaceholder entry in the Swahili locale from the English “auto” value to the localized Swahili label “Otomatiki”, preserving the existing locale structure.src/i18n/locales/eu.json-3430-3430 (1)
3430-3430: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTranslate the fuzzy-cutoff placeholder.
The Basque locale already translates
common.autoas"Automatikoa". The duplicate dialog rendersduplicates.fuzzyCutoffPlaceholder, so"auto"appears untranslated in the input.Proposed fix
- "fuzzyCutoffPlaceholder": "auto", + "fuzzyCutoffPlaceholder": "Automatikoa",🤖 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/i18n/locales/eu.json` at line 3430, Update the fuzzyCutoffPlaceholder value in the Basque locale to use the existing translated “Automatikoa” text, ensuring the duplicate dialog no longer displays the raw “auto” placeholder.src/i18n/locales/fi.json-3437-3437 (1)
3437-3437: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse clearer Finnish for the per-group selection action.
Line 3437 is understandable but less natural than
Valitse kaikki paitsi yksi kustakin ryhmästä. Use that wording to make the per-group scope explicit.Proposed fix
- "selectAllButOne": "Valitse kaikki paitsi yksi ryhmää kohden", + "selectAllButOne": "Valitse kaikki paitsi yksi kustakin ryhmästä",🤖 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/i18n/locales/fi.json` at line 3437, Update the "selectAllButOne" translation value in the Finnish locales file to use the clearer wording that explicitly expresses the per-group scope. Replace the current phrasing with the proposed alternative that better conveys the action applies per group.src/i18n/locales/tl.json-3438-3439 (1)
3438-3439: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse consistent wording for the “all copies selected” warning.
The locale already uses
napilifor selected items. Replacenakatikandtikwith selection wording so the deletion warning is clearer.Proposed fix
- "everyCopyTicked": "lahat ng kopya ay nakatik", - "everyCopyTickedHint": "Alisin ang tik sa isang kopya sa bawat naka-highlight na grupo: kung hindi, mabubura lahat ng kopya." + "everyCopyTicked": "Napili ang lahat ng kopya", + "everyCopyTickedHint": "Alisin ang pagpili sa isang kopya sa bawat naka-highlight na grupo; kung hindi, mabubura ang lahat ng kopya."🤖 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/i18n/locales/tl.json` around lines 3438 - 3439, Update the translations for everyCopyTicked and everyCopyTickedHint to use the locale’s established napili selection wording instead of nakatik and tik, while preserving the warning’s existing meaning about removing one selected copy per highlighted group.src/i18n/locales/hr.json-3430-3432 (1)
3430-3432: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate and clarify the new Croatian labels.
Line 3430 uses the English word
auto. Line 3432 uses awkward Croatian wording. Replace both with clear Croatian text so these controls are fully localized.Proposed fix
- "fuzzyCutoffPlaceholder": "auto", + "fuzzyCutoffPlaceholder": "automatski", - "keepByDefault": "Zadržati prema zadanome", + "keepByDefault": "Zadrži prema zadanim postavkama",🤖 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/i18n/locales/hr.json` around lines 3430 - 3432, Update the Croatian locale entries fuzzyCutoffPlaceholder and keepByDefault: replace the English “auto” with an appropriate Croatian translation and revise the awkward keepByDefault wording to clear, natural Croatian, while leaving sortSizeSpread unchanged.src/i18n/locales/is.json-3430-3430 (1)
3430-3430: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a complete Icelandic label for the default cutoff.
"sjálf"is incomplete and does not mean “default” in this context. Use"sjálfgefið"or the project-approved equivalent.Proposed wording
- "fuzzyCutoffPlaceholder": "sjálf", + "fuzzyCutoffPlaceholder": "sjálfgefið",🤖 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/i18n/locales/is.json` at line 3430, Update the fuzzyCutoffPlaceholder translation in the Icelandic locale to use the complete, project-approved Icelandic label meaning “default,” replacing the incomplete “sjálf” value with “sjálfgefið” or the established equivalent.src/i18n/locales/ja.json-3399-3400 (1)
3399-3400: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSpecify the compared property in the keeper labels.
Line [3399] and Line [3400] use
最小and最大. These labels do not state that the policy compares file size. Use最小サイズand最大サイズto prevent incorrect keeper selection.Proposed wording
- "keepSmallest": "最小", - "keepLargest": "最大", + "keepSmallest": "最小サイズ", + "keepLargest": "最大サイズ",🤖 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/i18n/locales/ja.json` around lines 3399 - 3400, Update the keepSmallest and keepLargest locale labels to explicitly reference file size, using the requested Japanese wording 最小サイズ and 最大サイズ.src/i18n/locales/ja.json-3404-3404 (1)
3404-3404: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIdentify the file that must remain.
Line [3404] describes removing “one selection,” not deselecting one file. State that the user must deselect one file in each group.
Proposed wording
- "everyCopyTickedHint": "強調表示された各グループで 1 つの選択を外してください。このままではすべての複製が削除されます。" + "everyCopyTickedHint": "強調表示された各グループで、1つのファイルの選択を解除してください。このままではグループ内のすべてのファイルが削除されます。"🤖 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/i18n/locales/ja.json` at line 3404, Update the Japanese everyCopyTickedHint translation in the locale resource to explicitly instruct the user to deselect one file in each highlighted group, rather than referring ambiguously to one selection. Preserve the warning that otherwise all copies will be deleted.src/i18n/locales/km.json-3438-3438 (1)
3438-3438: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace
ច្រកចម្លងwithច្បាប់ចម្លង.Line 3438 uses
ច្រកចម្លង, butច្រកmeans “port”. The warning therefore does not describe copies. (learnentry.com)Suggested correction
- "everyCopyTicked": "ច្រកចម្លងទាំងអស់ត្រូវបានធីក", + "everyCopyTicked": "ច្បាប់ចម្លងទាំងអស់ត្រូវបានធីក",🤖 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/i18n/locales/km.json` at line 3438, Update the everyCopyTicked translation value by replacing the incorrect Khmer term ច្រកចម្លង with ច្បាប់ចម្លង, while preserving the rest of the message unchanged.src/components/remoteThumbnails.test.ts-52-60 (1)
52-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the file signature participates in caching.
The test only checks for the shared-cache import and calls to
keyFor()andputThumbnail(). It does not verify thatsignaturereaches the cache key or that reads use the same key.The test will pass if the signature is ignored. Add a behavior-level test with two signatures for one path, or assert the exact key and read/write wiring.
🤖 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/components/remoteThumbnails.test.ts` around lines 52 - 60, Strengthen the caching test around ImageThumbnail and ProviderThumbnail so it verifies that signature is included in the cache key and that cache reads and writes use the same signature-derived key. Add a behavior-level case using two signatures for one path, or assert the exact keyFor/read/write wiring, ensuring the test fails if signature is ignored.src/components/remoteThumbnails.test.ts-62-69 (1)
62-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the call-site assertion complete.
toBeGreaterThanOrEqual(2)does not prove that every call site was inspected. A new third call site withoutsignaturecan leave the test green.The
{0,700}?\/>matcher also skips longer or non-self-closing JSX tags. Enumerate all thumbnail elements with an AST or a complete tag matcher, then assert that every matched element containssignature={signatureOf(...)}.🤖 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/components/remoteThumbnails.test.ts` around lines 62 - 69, The call-site test does not reliably inspect every thumbnail element. Update the test around the thumbnail-mount assertion to enumerate all ImageThumbnail and ProviderThumbnail JSX elements using an AST or complete tag matcher that handles longer and non-self-closing tags, then require every matched element to contain signature={signatureOf(...)} instead of relying on a minimum count.src/components/remoteThumbnails.test.ts-32-35 (1)
32-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBind the assertion to the remote grid and require the remote value.
remoteMountstarts at the first<LargeIconsGrid>and searches a broad slice. A local grid can satisfy the assertion. The match also acceptsisRemote={false}.Match the tag containing
files={sortedRemoteFiles}and assertisRemote={true}.Proposed fix
- const mount = appRaw.slice(appRaw.indexOf('<LargeIconsGrid')); - const remoteMount = mount.slice(0, mount.indexOf('files={sortedRemoteFiles') + 200); - expect(remoteMount, 'the remote Large Icons grid').toMatch(/\bisRemote\b/); + const remoteMount = appRaw.match( + /<LargeIconsGrid\b(?=[^>]*files=\{sortedRemoteFiles\})[^>]*>/, + )?.[0] ?? ''; + expect(remoteMount, 'the remote Large Icons grid').toMatch(/isRemote=\{true\}/);🤖 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/components/remoteThumbnails.test.ts` around lines 32 - 35, Update the remote grid assertion in the test to target the LargeIconsGrid tag containing files={sortedRemoteFiles}, rather than a broad slice beginning at the first grid. Require that same tag to contain the exact isRemote={true} prop so a local grid or false value cannot satisfy the assertion.
🧹 Nitpick comments (4)
src/components/DuplicateFinderDialog.tsx (1)
476-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
sizeSpreadOfto module scope.
sizeSpreadOfis a pure function of its argument, but it is redefined on every render and then called insideuseMemowithout appearing in the dependency list.react-hooks/exhaustive-depsflags this pattern, which can fail thenpm run ci:pre-pushlint gate. Hoisting it next tokeeperOfremoves the dependency question and allows a test to import it.Note the ordering: groups without
file_sizesreturn0and therefore sort first, together with the closest matches. Confirm that is the intended placement for unknown sizes.♻️ Proposed refactor
- /** Largest minus smallest member, in bytes. 0 when sizes are unknown. */ - const sizeSpreadOf = (group: DuplicateGroup): number => { - const sizes = group.file_sizes; - if (!sizes || sizes.length < 2) return 0; - return Math.max(...sizes) - Math.min(...sizes); - }; - const orderedGroups = useMemo(() => {Add the helper at module scope, beside
selectionForPolicy:/** Largest minus smallest member, in bytes. 0 when sizes are unknown. */ export function sizeSpreadOf(group: DuplicateGroup): number { const sizes = group.file_sizes; if (!sizes || sizes.length < 2) return 0; return Math.max(...sizes) - Math.min(...sizes); }Based on learnings from the coding guidelines: "Before pushing any commit, run
npm run ci:pre-push; do not push if it fails or cannot run".🤖 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/components/DuplicateFinderDialog.tsx` around lines 476 - 492, Move the pure sizeSpreadOf helper from inside the component to module scope beside selectionForPolicy or keeperOf, exporting it so tests can import it. Remove the nested definition and keep its existing behavior, including returning 0 for missing or insufficient file_sizes; preserve the current sizeSpread ordering where those groups sort first.Source: Coding guidelines
src/components/dedupeSelection.test.ts (1)
116-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
indexOfbeforesliceto avoid a silent near-empty-string fallback.At Line 126,
dialogRaw.indexOf('const updatedGroups')returns-1if the source no longer contains that exact string.dialogRaw.slice(-1)then silently returns only the last character of the file instead of failing clearly. The two assertions on Lines 127-128 would fail, but the failure message would not point at the real cause.Add an explicit check that the index is not
-1before slicing, so a rename or removal ofupdatedGroupsfails with a clear message.🔧 Proposed fix
it('keeps the parallel arrays parallel after a delete', () => { // `file_hashes` and `file_sizes` are indexed by position in `files`. // Filtering one and not the others shifts every row's hash and size by // the number of copies removed above it. - const after = dialogRaw.slice(dialogRaw.indexOf('const updatedGroups')); + const idx = dialogRaw.indexOf('const updatedGroups'); + expect(idx, 'expected to find `const updatedGroups` in DuplicateFinderDialog.tsx').toBeGreaterThan(-1); + const after = dialogRaw.slice(idx); expect(after.slice(0, 900)).toMatch(/file_hashes: group\.file_hashes \? kept\.map/); expect(after.slice(0, 900)).toMatch(/file_sizes: group\.file_sizes \? kept\.map/); });🤖 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/components/dedupeSelection.test.ts` around lines 116 - 130, In the “keeps the parallel arrays parallel after a delete” test, capture the result of dialogRaw.indexOf('const updatedGroups') and explicitly assert it is not -1 before calling slice. Use the validated index to create after, so removal or renaming of updatedGroups produces a clear test failure instead of slicing from the end.src/components/modalScrollbar.test.ts (1)
23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
indexOfbeforeslice, same pattern as indedupeSelection.test.ts.At Line 25,
dialogRaw.indexOf('{/* Groups list (scrollable) */}')returns-1if that comment is removed or reworded.dialogRaw.slice(-1)then silently returns only the last character, and the assertion at Line 26 fails without pointing at the real cause.See the consolidated comment for the shared fix across both test files.
🤖 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/components/modalScrollbar.test.ts` around lines 23 - 34, Guard the marker lookup in the “the results list keeps its scrollbar (`#347`)” test before slicing dialogRaw: capture the result of dialogRaw.indexOf('{/* Groups list (scrollable) */}') and assert it is non-negative before calling slice. Follow the established pattern in dedupeSelection.test.ts so a missing or changed marker produces a direct test failure instead of slicing from -1.src/i18n/locales/pl.json (1)
3434-3435: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests covering both retention policies.
keeperOf()andselectionForPolicy()already implement both policies, but the current tests only checkshortestName. Add a case that selects one keeper per group usingoldestand another usingnewest.🤖 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/i18n/locales/pl.json` around lines 3434 - 3435, Add tests for both retention policies by exercising keeperOf() and selectionForPolicy(): create cases that select one keeper per group with the oldest policy and with the newest policy, while preserving the existing shortestName coverage.
🤖 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/components/DuplicateFinderDialog.tsx`:
- Around line 935-938: Resolve the conflict between selectAll and the delete
guard in DuplicateFinderDialog: preserve the intended second-confirmation
behavior described near selectAll, allowing deletion when fullyTickedGroups is
populated while requiring an explicit confirmation that identifies the affected
groups; do not leave the Delete button permanently disabled after selectAll.
In `@src/i18n/locales/bn.json`:
- Around line 3434-3435: Update the Bengali keepSmallest and keepLargest
translations in the locale definition to match the bound oldest and newest
retention policies, using the approved Bengali labels for “oldest” and “newest”;
do not change policy values or other locale entries.
In `@src/i18n/locales/cy.json`:
- Around line 3434-3435: Align the keeper-policy localization with the values
handled by DuplicateFinderDialog: replace the keepSmallest and keepLargest
locale keys with oldest and newest and provide translations describing those
policies, or consistently rename the consumer values and selection logic
instead. Ensure the rendered labels accurately identify which file is retained
before deletion.
In `@src/i18n/locales/pt.json`:
- Around line 3434-3435: Rename the duplicate-retention option keys currently
represented by duplicates.keepSmallest and duplicates.keepLargest, including
their implementation/value keys, to size-based names that do not imply
modification-time ordering. Update every reference and all locale translation
files consistently, preserving the mapping where the smallest file_sizes entry
is retained by the former keepSmallest option and the largest by the former
keepLargest option.
In `@src/i18n/locales/tl.json`:
- Line 3432: Update the keepByDefault translation in tl.json to use the existing
“Panatilihin bilang default” term from the nearby translation, preserving the
keeper-policy meaning instead of the current hide-by-default wording.
---
Outside diff comments:
In `@src/components/DuplicateFinderDialog.tsx`:
- Around line 239-253: Update the dependency array for the scan callback in
DuplicateFinderDialog so it includes keepPolicy, ensuring Retry uses the current
policy when calling selectionForPolicy. Preserve the existing scanPath, mode,
and appliedThreshold dependencies and selection behavior.
In `@src/components/ImageThumbnail.tsx`:
- Around line 47-77: Reset thumbnail state when cacheKey changes so
ImageThumbnail does not render a previous src or error during a cache miss;
clear the failure state before loading and after success, and ensure the loaded
source belongs to the current cache key. Apply the equivalent reset and
cache-key validation in src/components/ImageThumbnail.tsx lines 47-77 and
src/components/ProviderThumbnail.tsx lines 45-72, then add regression coverage
for changing a mounted component between uncached keys.
---
Minor comments:
In `@src/components/DuplicateFinderDialog.tsx`:
- Around line 781-788: Update the duplicate-file row div identified by its
key={filePath} and isChecked class logic to add an onClick that calls
toggleFile(filePath), preserving the existing child-button stopPropagation
behavior so row clicks toggle selection without affecting button actions.
- Around line 766-774: Update the duplicate-size delta handling around fileSize,
largest, and delta in DuplicateFinderDialog so negative differences are not
passed directly to formatBytes. Compute the group mismatch once, then format its
absolute magnitude and apply an explicit negative sign for smaller copies while
preserving the existing null/zero behavior.
In `@src/components/remoteThumbnails.test.ts`:
- Around line 52-60: Strengthen the caching test around ImageThumbnail and
ProviderThumbnail so it verifies that signature is included in the cache key and
that cache reads and writes use the same signature-derived key. Add a
behavior-level case using two signatures for one path, or assert the exact
keyFor/read/write wiring, ensuring the test fails if signature is ignored.
- Around line 62-69: The call-site test does not reliably inspect every
thumbnail element. Update the test around the thumbnail-mount assertion to
enumerate all ImageThumbnail and ProviderThumbnail JSX elements using an AST or
complete tag matcher that handles longer and non-self-closing tags, then require
every matched element to contain signature={signatureOf(...)} instead of relying
on a minimum count.
- Around line 32-35: Update the remote grid assertion in the test to target the
LargeIconsGrid tag containing files={sortedRemoteFiles}, rather than a broad
slice beginning at the first grid. Require that same tag to contain the exact
isRemote={true} prop so a local grid or false value cannot satisfy the
assertion.
In `@src/i18n/locales/ca.json`:
- Line 3439: Update the everyCopyTickedHint Catalan translation so it states
that all copies would be deleted if one copy from each highlighted group is not
deselected, replacing the current causal wording with the intended warning
meaning.
In `@src/i18n/locales/da.json`:
- Around line 3434-3435: Update the Danish translation values for the
keepSmallest and keepLargest properties in the da.json file to explicitly
reference what is being compared. Replace the ambiguous "Mindste" and "Største"
with either "Mindste fil" and "Største fil" (to indicate smallest/largest file)
or "Mindste størrelse" and "Største størrelse" (to indicate smallest/largest
size), ensuring the labels clearly convey that the size-policy is comparing file
sizes for duplicate retention decisions.
In `@src/i18n/locales/et.json`:
- Line 3430: Update the fuzzyCutoffPlaceholder value in the Estonian locale from
the English shorthand to the localized Estonian string “automaatne”, preserving
the existing key and structure.
In `@src/i18n/locales/eu.json`:
- Line 3430: Update the fuzzyCutoffPlaceholder value in the Basque locale to use
the existing translated “Automatikoa” text, ensuring the duplicate dialog no
longer displays the raw “auto” placeholder.
In `@src/i18n/locales/fi.json`:
- Line 3437: Update the "selectAllButOne" translation value in the Finnish
locales file to use the clearer wording that explicitly expresses the per-group
scope. Replace the current phrasing with the proposed alternative that better
conveys the action applies per group.
In `@src/i18n/locales/hr.json`:
- Around line 3430-3432: Update the Croatian locale entries
fuzzyCutoffPlaceholder and keepByDefault: replace the English “auto” with an
appropriate Croatian translation and revise the awkward keepByDefault wording to
clear, natural Croatian, while leaving sortSizeSpread unchanged.
In `@src/i18n/locales/is.json`:
- Line 3430: Update the fuzzyCutoffPlaceholder translation in the Icelandic
locale to use the complete, project-approved Icelandic label meaning “default,”
replacing the incomplete “sjálf” value with “sjálfgefið” or the established
equivalent.
In `@src/i18n/locales/ja.json`:
- Around line 3399-3400: Update the keepSmallest and keepLargest locale labels
to explicitly reference file size, using the requested Japanese wording 最小サイズ
and 最大サイズ.
- Line 3404: Update the Japanese everyCopyTickedHint translation in the locale
resource to explicitly instruct the user to deselect one file in each
highlighted group, rather than referring ambiguously to one selection. Preserve
the warning that otherwise all copies will be deleted.
In `@src/i18n/locales/km.json`:
- Line 3438: Update the everyCopyTicked translation value by replacing the
incorrect Khmer term ច្រកចម្លង with ច្បាប់ចម្លង, while preserving the rest of
the message unchanged.
In `@src/i18n/locales/mk.json`:
- Line 3431: Update the Macedonian translation for sortSizeSpread to clearly
describe the size range or spread of duplicate groups, using wording such as
“Распон на големини” rather than the current ambiguous label, and confirm the
phrasing with a Macedonian reviewer.
- Line 3437: Update the selectAllButOne translation value to explicitly state
that one file is kept in each group, matching the corresponding delete-action
wording while preserving the key and locale structure.
In `@src/i18n/locales/nl.json`:
- Around line 3437-3439: Update the Dutch locale strings selectAllButOne and
everyCopyTickedHint: replace “Alles op één na” with “Alles behalve één” and
change “gemarkeerde groep” to “geselecteerde groep,” preserving the rest of the
wording.
In `@src/i18n/locales/no.json`:
- Line 3431: Update the Norwegian translation value for the sortSizeSpread key
to use a spread-specific label, preferably “Størrelsesspredning” or “Spredning i
størrelse,” while leaving the key and surrounding translations unchanged.
In `@src/i18n/locales/sw.json`:
- Line 3430: Update the fuzzyCutoffPlaceholder entry in the Swahili locale from
the English “auto” value to the localized Swahili label “Otomatiki”, preserving
the existing locale structure.
In `@src/i18n/locales/tl.json`:
- Around line 3438-3439: Update the translations for everyCopyTicked and
everyCopyTickedHint to use the locale’s established napili selection wording
instead of nakatik and tik, while preserving the warning’s existing meaning
about removing one selected copy per highlighted group.
---
Nitpick comments:
In `@src/components/dedupeSelection.test.ts`:
- Around line 116-130: In the “keeps the parallel arrays parallel after a
delete” test, capture the result of dialogRaw.indexOf('const updatedGroups') and
explicitly assert it is not -1 before calling slice. Use the validated index to
create after, so removal or renaming of updatedGroups produces a clear test
failure instead of slicing from the end.
In `@src/components/DuplicateFinderDialog.tsx`:
- Around line 476-492: Move the pure sizeSpreadOf helper from inside the
component to module scope beside selectionForPolicy or keeperOf, exporting it so
tests can import it. Remove the nested definition and keep its existing
behavior, including returning 0 for missing or insufficient file_sizes; preserve
the current sizeSpread ordering where those groups sort first.
In `@src/components/modalScrollbar.test.ts`:
- Around line 23-34: Guard the marker lookup in the “the results list keeps its
scrollbar (`#347`)” test before slicing dialogRaw: capture the result of
dialogRaw.indexOf('{/* Groups list (scrollable) */}') and assert it is
non-negative before calling slice. Follow the established pattern in
dedupeSelection.test.ts so a missing or changed marker produces a direct test
failure instead of slicing from -1.
In `@src/i18n/locales/pl.json`:
- Around line 3434-3435: Add tests for both retention policies by exercising
keeperOf() and selectionForPolicy(): create cases that select one keeper per
group with the oldest policy and with the newest policy, while preserving the
existing shortestName coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae4f2340-62d4-4799-839e-f4cb7560efd3
📒 Files selected for processing (62)
src-tauri/src/dedupe/mod.rssrc-tauri/src/filesystem.rssrc/App.tsxsrc/components/DuplicateFinderDialog.tsxsrc/components/ImageThumbnail.tsxsrc/components/LargeIconsGrid.tsxsrc/components/LocalFilePanel.tsxsrc/components/ProviderThumbnail.tsxsrc/components/dedupeSelection.test.tssrc/components/modalScrollbar.test.tssrc/components/remoteThumbnails.test.tssrc/i18n/locales/bg.jsonsrc/i18n/locales/bn.jsonsrc/i18n/locales/ca.jsonsrc/i18n/locales/cs.jsonsrc/i18n/locales/cy.jsonsrc/i18n/locales/da.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/el.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/et.jsonsrc/i18n/locales/eu.jsonsrc/i18n/locales/fi.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/gl.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/hr.jsonsrc/i18n/locales/hu.jsonsrc/i18n/locales/hy.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/is.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ka.jsonsrc/i18n/locales/km.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/lt.jsonsrc/i18n/locales/lv.jsonsrc/i18n/locales/mk.jsonsrc/i18n/locales/ms.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/no.jsonsrc/i18n/locales/pl.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ro.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/sk.jsonsrc/i18n/locales/sl.jsonsrc/i18n/locales/sr.jsonsrc/i18n/locales/sv.jsonsrc/i18n/locales/sw.jsonsrc/i18n/locales/th.jsonsrc/i18n/locales/tl.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/uk.jsonsrc/i18n/locales/vi.jsonsrc/i18n/locales/zh.jsonsrc/styles.csssrc/types/aerofile.tssrc/utils/thumbnailCache.test.tssrc/utils/thumbnailCache.ts
…after size Review of this PR found that `selectAll`, which ticks every copy of every group on purpose, always put the dialog in the state the delete guard refused. The Delete button was hard-disabled whenever any group had all its copies ticked, so the button that had just been added to "do as it says" offered an action that could never be completed, with a tooltip on a dead control as the only feedback. The comment above `selectAll` described the intended behaviour, a deliberate second confirmation; the code implemented neither half. The invariant is still watched, but it now costs a confirmation rather than the action: the delete is allowed, and a selection that would leave a group with no copy is always confirmed, even with `confirmBeforeDelete` off, because that setting is about the routine delete and this is the case that is not routine. The confirmation names the groups rather than counting them, since "3 groups" is not something a user can check against what they ticked. Two more from the same review: `scan` read `keepPolicy` without listing it, so Retry re-selected with the policy captured earlier. Fixed through a ref rather than through the dependency list: the effect that runs `scan` keys off the callback identity on purpose, so that changing the mode or the fuzzy threshold re-scans, and adding `keepPolicy` there would have turned a dropdown into a full filesystem walk. The two size policies were named `oldest` and `newest` while ordering by size, which is what their labels say and what `keeperOf` has always done. The names were the only thing claiming otherwise, and they said it to everyone reading the option list: the same review proposed rewriting four locales to say oldest and newest, which would have made the label disagree with the file the policy keeps. Renamed to `smallest` and `largest`. No locale changes: all 47 already said smallest and largest, correctly. One locale fix that the file itself justifies: Tagalog `keepByDefault` read "Itago bilang default", "Hide by default", in a dialog that deletes files. The same file already carries "Panatilihin" for Keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On the outside-diff finding about All five inline comments answered: three applied, two declined with the reason on the comment itself. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DuplicateFinderDialog.tsx (1)
823-838: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the mtime in the thumbnail cache key for duplicate rows.
keyForstores onlyscope + path + signature, whilesignatureOf(fileSize, null)produces signatures like1024:?. A cache populated for one duplicate row can then match another distinct duplicate with the same byte size, so include a stable mtime if one is available, or disable caching for rows with unknown mtimes.🤖 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/components/DuplicateFinderDialog.tsx` around lines 823 - 838, Update the duplicate-row ImageThumbnail cache inputs in the preview block near isPreviewableImage so the cache key includes each file’s stable mtime alongside fileSize and path. When no mtime is available, disable thumbnail caching for that row rather than using signatureOf(fileSize, null); preserve the existing dedupe cache scope and preview behavior.
🧹 Nitpick comments (2)
src/components/DuplicateFinderDialog.tsx (2)
224-225: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the ref write out of the render body.
keepPolicyRef.current = keepPolicy;runs during render, on every render. React's own guidance states: "Don't read or write ref.current during rendering. This makes your component hard to predict." React Doctor flags this same line for the same reason.In practice this specific write is idempotent (same source, same value), so it is unlikely to cause a visible bug today. But it is a documented anti-pattern, and it is exactly the pattern the "latest ref" write-up recommends doing in an effect instead of inline in render.
🔧 Proposed fix: move the assignment into an effect
const keepPolicyRef = useRef(keepPolicy); - keepPolicyRef.current = keepPolicy; + useEffect(() => { + keepPolicyRef.current = keepPolicy; + }, [keepPolicy]);🤖 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/components/DuplicateFinderDialog.tsx` around lines 224 - 225, Move the keepPolicyRef.current assignment out of the render body and into a useEffect in DuplicateFinderDialog, with keepPolicy as its dependency so the ref stays synchronized after updates. Preserve the existing useRef initialization and all consumers of keepPolicyRef.Source: Linters/SAST tools
493-498: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid spread-based
Math.max/Math.minoverfile_sizes, and stop recomputing the group maximum per row.Two related issues on
group.file_sizes:
Math.max(...sizes)/Math.min(...sizes)(line 497) andMath.max(...group.file_sizes)(line 789) use the spread operator. For arrays beyond roughly 100k-135k elements, this throwsRangeError: Maximum call stack size exceededin V8-based engines, since spread arguments are pushed onto the call stack. A duplicate-finder scan over a large cache or log directory can plausibly produce a group with that many identical-size files.- At line 789,
largestis recomputed with a freshMath.max(...group.file_sizes)call insidegroup.files.map(...), so the whole array is scanned once per file in the group. For a group ofnfiles, that is O(n²) work just to render the rows.Replace the spread calls with a single linear reduction, computed once per group.
🔧 Proposed fix
const sizeSpreadOf = (group: DuplicateGroup): number => { const sizes = group.file_sizes; if (!sizes || sizes.length < 2) return 0; - return Math.max(...sizes) - Math.min(...sizes); + let max = sizes[0], min = sizes[0]; + for (const s of sizes) { + if (s > max) max = s; + if (s < min) min = s; + } + return max - min; };+ {/* compute once per group, before mapping files */} const fileSize = group.file_sizes?.[fileIdx]; - const largest = group.file_sizes?.length ? Math.max(...group.file_sizes) : undefined; + const largest = groupLargest;where
groupLargestis computed once, ahead ofgroup.files.map(...), via afor/reduceloop instead ofMath.max(...group.file_sizes).Also applies to: 780-792
🤖 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/components/DuplicateFinderDialog.tsx` around lines 493 - 498, Update sizeSpreadOf and the group.files.map rendering flow to avoid spread-based Math.max/Math.min calls on file_sizes. Compute the group minimum and maximum with one linear loop or reduction, reuse the resulting maximum as groupLargest for every rendered row, and ensure the existing zero/unknown-size behavior is preserved.
🤖 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.
Outside diff comments:
In `@src/components/DuplicateFinderDialog.tsx`:
- Around line 823-838: Update the duplicate-row ImageThumbnail cache inputs in
the preview block near isPreviewableImage so the cache key includes each file’s
stable mtime alongside fileSize and path. When no mtime is available, disable
thumbnail caching for that row rather than using signatureOf(fileSize, null);
preserve the existing dedupe cache scope and preview behavior.
---
Nitpick comments:
In `@src/components/DuplicateFinderDialog.tsx`:
- Around line 224-225: Move the keepPolicyRef.current assignment out of the
render body and into a useEffect in DuplicateFinderDialog, with keepPolicy as
its dependency so the ref stays synchronized after updates. Preserve the
existing useRef initialization and all consumers of keepPolicyRef.
- Around line 493-498: Update sizeSpreadOf and the group.files.map rendering
flow to avoid spread-based Math.max/Math.min calls on file_sizes. Compute the
group minimum and maximum with one linear loop or reduction, reuse the resulting
maximum as groupLargest for every rendered row, and ensure the existing
zero/unknown-size behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53828dc8-4140-4c49-b32f-00fb1929f54f
📒 Files selected for processing (4)
src/components/DuplicateFinderDialog.test.tssrc/components/DuplicateFinderDialog.tsxsrc/components/dedupeSelection.test.tssrc/i18n/locales/tl.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/locales/tl.json
The merge conflicts were all in locale files, against #538, which restored diacritics and converted sr.json to Cyrillic while this branch was adding nine keys to the same block. Resolved by taking main's value for every shared key and keeping this branch's new keys: cs 3 shared from main, fr 2, sk 4, sr 21, 9 new keys kept in each. Taking either side whole would have silently dropped one of the two pieces of work. Three findings from the second review, all applied: The keep-policy ref was written during render. Moved into an effect: writing a ref while rendering is the documented anti-pattern, and the value is only read from a click handler, which runs after effects have flushed. `Math.max(...sizes)` and `Math.min(...sizes)` push every element onto the call stack, so a scan of a cache or a log directory can hand them a group large enough to throw, and the per-row copy rescanned the whole size list once per file in the group. Replaced with one linear `extentOf` pass, hoisted out of the row map. Pinned, including a 200k-element case that proves the spread form throws where this does not. A duplicate row asked the thumbnail cache to key on `size:?`. Every member of an exact group has the same size and the command reports no mtime, so an edit that kept a file's length would have been served the old preview. It now passes the row's content hash, which the command does report and which is the stronger half of the pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Stacks on #540 and #544 — please merge those first.
Five items from the 2026-07-31 batch on #347, all in the Find Duplicates dialog.
Any copy can be ticked
Both halves were true, and they had the same cause: the first file of each group had a disabled checkbox, so the one copy you could not delete was whichever the scan happened to return first.
The invariant behind that — never delete every copy of something — is real, but it was being enforced by removing the control instead of watching the state. It is now watched: a group whose copies are all ticked is highlighted and blocks the delete, saying why. Which copy survives is simply the one left unticked, and the KEEP/DELETE badge follows the selection rather than the row index.
The button is now labelled Select all but one per group, which is what it always did, and a real Select All sits beside it. What "one" means is a policy you pick: shortest name (the default — " (copy)", " (1)" and " - Copy" make the derived file the longer name almost every time, which is your suggestion), smallest, largest, or first found.
Rows carry their own byte size
The engine reported one size per group. That is true in exact mode and false in fuzzy mode, where the members are not the same file at all.
DuplicateGroup.file_sizesnow runs parallel tofilesthrough the engine and the command; each row shows its own size and how far it is from the largest in the group, and Sort offers Byte-size spread.On the "real dist" part: not as the clustering metric. A re-encode at a different quality moves the byte count a great deal and the perceptual hash barely at all — and that pair, the same picture through a different encoder, is exactly the one worth catching. As an ordering, though, it is genuinely useful and it separates your two cases: the pair that looked identical differed by a rounding error, the pair that was clearly different by a third. So it is a column and a sort, not a distance.
The hash is readable
Displayed at 32 hex digits, which is your 16 bytes, with the full value one hover or one copy away.
Worth being precise about the reasoning, because your instinct here matches what AeroFTP already ships elsewhere: the per-shard checksums in AeroCorrect are BLAKE3 truncated to exactly 16 bytes, and AeroVault v3 chunk IDs are
blake3-keyed-128. So the rule is already in the house.What did not change is the grouping key, which stays the full digest. There is no CPU to save — BLAKE3 computes the whole state regardless,
-lonly changes how much is extracted, as you noted — and no reason to narrow the key of an operation that deletes files. The saving is in readability, which was the real problem with a 64-character string on every row.A thumbnail on every row
Added, clickable into the app's own viewer, which is also the way into AeroImage and the
<>editor. Deciding between two near-identical pictures from their names and byte counts alone was guesswork.The scrollbar
It was not missing by oversight — it was being hidden on purpose, by a rule aimed at something else.
html.modal-openhides every::-webkit-scrollbaron the page while a modal is open, because WebKitGTK paints native scrollbars above CSS z-index overlays and the page behind would otherwise show its bars through the dialog. The selector matched every scrollable element, the modal's own content included..modal-scrollopts a modal's own list back in, at 14px.One correctness fix found on the way
file_hashesandfile_sizesare indexed by position infiles, and the post-delete rebuild filteredfilesalone. Every surviving row's hash and size would have shifted by the number of copies deleted above it.Verification
dedupeSelection.test.tsandmodalScrollbar.test.ts, verified failing on the branch base across 15 of their 16 assertions. Front end: 72 files / 658 tests,tscclean,i18n:validate46/46 clean with the 9 new strings translated in every locale. Rust:cargo fmt --checkclean,cargo checkclean, dedupe suite 8/8.Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests