Resolve name conflicts in packages

[fiskus]

• Added "Rename" button and prompt for a new file name
• Added new properties to files:
  • conflict is this file conflicted with some other file
  • changed what was changed in this file
• Removed unused FilesSelector

// TODO:

• [x] get rid of @ts-expect-error
• [x] re-use clodeDomFile for adding/editing meta

[codecov[bot]]

Codecov Report

Merging #3596 (e33f96e) into master (34aa05f) will decrease coverage by 0.09%. The diff coverage is 2.67%.

@@            Coverage Diff             @@
##           master    #3596      +/-   ##
==========================================
- Coverage   36.14%   36.05%   -0.09%     
==========================================
  Files         676      677       +1     
  Lines       29673    29754      +81     
  Branches     4321     4340      +19     
==========================================
+ Hits        10724    10727       +3     
- Misses      17812    17883      +71     
- Partials     1137     1144       +7     

Flag	Coverage Δ
api-python	91.35% <ø> (ø)
catalog	9.81% <2.67%> (-0.03%)	:arrow_down:
lambda	86.03% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
catalog/app/components/Dialog/Prompt.tsx	0.00% <0.00%> (ø)
catalog/app/components/FileEditor/CreateFile.tsx	0.00% <0.00%> (ø)
...p/containers/Bucket/PackageDialog/EditFileMeta.tsx	0.00% <0.00%> (ø)
...p/containers/Bucket/PackageDialog/EditFileName.tsx	0.00% <0.00%> (ø)
...app/containers/Bucket/PackageDialog/FilesInput.tsx	0.00% <0.00%> (ø)
...iners/Bucket/PackageDialog/PackageCreationForm.tsx	0.00% <0.00%> (ø)
.../containers/Bucket/PackageDialog/PackageDialog.tsx	26.07% <27.27%> (+0.24%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[fiskus]

I deployed it to searchminimal

[fiskus]

Instead of treating conflicts as errors, maybe it would make more sense to just show warnings when auto-renaming files, and don't block submit.

I think it just unacceptable to submit files with auto-renaming files. It's unexpected for user, that he added a file with one name, but it was submitted with another name. Warning is easy to miss.

Instead of always succeeding, I think it's better to check if the name is already taken and disallow submitting to prevent automatic rename (which is kinda confusing when it happens).

Yes, good idea and probably easy. First, I interpreted it differently, and it could take a week :)

Auto-renaming is sometimes buggy and leads to smth like file (conflict 1) (conflict 1).ext instead of file (conflict 2).ext.

Ok, but I couldn't reproduce, I'll try to find it in code

nice to have: tooltip

Good idea, I'll implement it in standalone PR

nice to have: hide deleted

Hmm, could be useful, but I never felt I need this

[fiskus]

On model

Yes, I agree, I tried to fit into model something that it wasn't designed for. I have two excuses and two solutions.

I wanted to do a minimal change, to have better chances to fit into horizon myself and to fit review into horizon. And I wanted to do a feature separately from refactoring. It's a large separate topic why it's a good idea to make it work first, then make it good (but of course, only if we don't forget and invest time in making good).

I suggest merging this (after addressing UX issues and bugs) and work on refactoring in a separate PR. Also, there is a spec on refactoring file types https://github.com/quiltdata/specs/pull/53, it, for example, has the same proposal to contain file in a separate property. I'll split the spec into three specs as noted in description

[fiskus]

• fixed form submit on rename file
• "Submit" → "Rename"
• "files should match schema" → "There are files with identical names. Rename or remove them please"

---

• treat conflict errors as warnings -- skipped
• nice-to-haves -- TODO
• model adjustments -- TODO

---

• Check if user renames to existing file -- providing data to the editing component would be too hacky, and moving editing component up looks a good thing (also, for performance), but it will take more time
• "file (conflict 1) (conflict 1).ext" -- couldn't find this bug. Though, it can happen when you rename "file (conflict 2).ext" to "file (conflict 1).ext" (not to "file.ext"), manually

[nl0]

I wanted to do a minimal change, to have better chances to fit into horizon myself and to fit review into horizon

well, in the end it did not fit, so i don't think we should rush and try to fit it anyways in some half-baked form

It's a large separate topic why it's a good idea to make it work first, then make it good (but of course, only if we don't forget and invest time in making good).

it's ok (in my book) to make it work and then make it good before shipping.

i can't agree that breaking things, shipping them, and then fixing them is a valid approach.

i'm kinda ok with expanding the model and postponing a proper refactoring, but i'm not ok with breaking the model, so i still feel like this requires addressing:

"renamed" state implementation is inconsistent with the model, since the model only has the notion of existing / added / deleted entries. Instead of tracking this as "changed" prop, maybe on first rename we should (1) "add" an entry with the new logical key and set "renamedFrom" (or smth like that) prop on it pointing to the original logical key; and (2) mark the original entry as deleted -- this feels more in-line with our current state model and UX. The added entry with "renamedFrom" prop may be rendered with the "revert" action instead of "remove".

since the current approach is too brittle and has edge cases that aren't handled

[nl0]

I wanted to do a minimal change, to have better chances to fit into horizon myself and to fit review into horizon

well, in the end it did not fit, so i don't think we should rush and try to fit it anyways in some half-baked form

i realize that i am accountable for this as well to some extent (as a reviewer).

@drernie @akarve you can override and approve this PR if you think it's more important to get this out the door sooner and backlog the polish / refactoring, but i'm convinced that we should not compromise on quality. scope hammering is not compromising on quality, it's compromising on, well, scope, and this feature didn't make it.

[nl0]

• fixed form submit on rename file

👍

* Check if user renames to existing file -- providing data to the editing component would be too hacky, and moving editing component up looks a good thing (also, for performance), but it will take more time

just pass a callback to check if the file exists in the manifest?

* `"file (conflict 1) (conflict 1).ext"` -- couldn't find this bug. Though, it can happen when you rename `"file (conflict 2).ext"` to `"file (conflict 1).ext"` (not to `"file.ext"`), manually

not sure how i managed that lol

[drernie]

Is this obsoleted by @fiskus' work on Selection's last Horizon?

[fiskus]

No, it's still relevant. Alexei couldn't approve this due to his disagreement on tradeoffs I made in design. To reduce tradeoffs and make more suitable design for the tasks, I need to refactor this code first. Also, this unification of types can help me refactor https://github.com/quiltdata/quilt/pull/3681