Repository navigation
Import a file as what it holds, not what it's named - #426
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 25 minutes. View limit details
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. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| final archive = ZipDecoder().decodeBytes(await file.readAsBytes()); | ||
| return _zipImportKind(archive) ?? unrecognised; |
There was a problem hiding this comment.
_importFileKind reads and decodes the whole ZIP, then _importStrategyFile or _importZipArchive immediately repeats that work. Large backups and strategies with many images now pay for an extra full-file read and archive parse. Pass the decoded archive into the selected importer to avoid repeating this work.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
ci: analyze test web
Import told people it worked when their strategy never came in. A file saved with no extension is still an .ica, and an .ica is a zip, so Import Backup and drag and drop read it as a folder archive. The toast said "Imported 1 folder" and the library got an empty folder named
renamed-ica.json. Any other zip did the same: a zip of photos became an empty folder callednotes.txt.#420 stops new exports losing their extension. This PR is for the files already out there. It's based on main and doesn't touch the export paths #420 changes.
What changed
Every way in now reads the file to decide what it is.
_importFileKindlooks inside:mapData, alone or at the top of a zip, is a strategy.icafiles is an archivemapDatais the one field the strategy import can't do without, so the check can't reject a strategy that used to load. A file named.icathat the check can't recognise still goes to the old strategy import and fails the way it always did.Import .ica, Import Backup, a dropped file, the contents of a dropped folder and a file opened with Icarus all go through this now, so Import .ica takes a backup and Import Backup takes an .ica.
Three smaller changes follow from it:
.icafilter, a user could not see the file they were trying to import..ica,.jsonand.zipcome off a name.Bind v1.2with its extension lost used to come in asBind v1.What I checked
Windows release driver builds, main and this branch, one script against both (
dropFilesis a new driver step that sends the messages the native drop target sends). The Open dialogs were the real Windows ones. The script took 148 steps with no failures and left no empty folder in the library.Old files: I imported every distinct real .ica and backup I could find on this machine (46, from the fixtures and earlier test runs) through main and this branch. With their names intact the two builds gave identical results for all 46. With the extension gone, main made an empty folder for all 44
.icafiles. This branch imported 42 and refused 2 with a failure, the same 2 that fail with their names on. Those 2 are hand-edited test files with a number where exports write a string (sortIndex).Targeted tests pass: 92 across folder import, strategy integrity, the version guard, the import summary and library export. The new tests run all three historical fixtures with their extensions removed.
Astra reviewed it and found three things. A strategy saved as
icarus-metadatalooked like a backup, which was my regression, so the strategy check now runs first. Opening a backup with Icarus hid partial failures, so it now shows the summary. The picker tests ignored the filter, so they now assert it. Its third finding, that a backup inside a dropped folder is skipped, is left alone. main does that on purpose, a test pins it, and the user is told it was skipped.What this doesn't cover
importFilesinplatform_policy.dart). That's from reading the code, not from a browser.FileType.anyis a plain file_picker option there, so I expect no difference.The PR appears safe to merge, with a non-blocking improvement to avoid reading large ZIP files twice.
Findings
Summary
Imports now identify strategies and backups by their contents rather than their names.
Checks were limited to source review; no app or tests were run.
Acknowledged exclusions from
SunkenInTime: backups inside dropped folders remain intentionally skipped; extensionless strategies inside user-made ZIPs remain unsupported; legacy exports containing only empty folders now fail instead of creating folders.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Picker, drop, or file opened with Icarus] --> B[Inspect file contents] B --> C{File kind} C -->|Strategy| D[Import one strategy] C -->|Archive| E[Import backup or folder archive] C -->|Unknown| F[Report unsupported file] D --> G[Show import summary] E --> G F --> GReviews (1) · Last reviewed commit: "Check for a strategy before a manifest, ..." · Reviewed by Greptile