Resolve name conflicts in packages #3596

fiskus · 2023-07-20T13:39:38Z

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:

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

…-conflicts

…nto resolve-name-conflicts

codecov · 2023-07-20T13:41:16Z

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%)`	⬇️
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%)`	⬆️

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

…-conflicts

Co-authored-by: Alexei Mochalov <[email protected]>

fiskus · 2023-08-03T09:44:07Z

I deployed it to searchminimal

nl0

First, I'd like to point out that this feature is essential and we were missing it for so long. However, I feel like the current implementation is not yet ready for the prime time.

Here's some UX issues / suggestions that are (probably) easy to address:
1. In the file rename dialog enter triggers package push -- probably should trigger only rename instead.
2. In the file rename dialog "submit" button label seems too generic, "rename" or smth would be more appropriate.
3. 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).
4. 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.
5. When there are conflicts, "files should match schema" error message is shown, which is confusing and doesn't reflect reality.
6. Auto-renaming is sometimes buggy and leads to smth like file (conflict 1) (conflict 1).ext instead of file (conflict 2).ext.
7. (Nice-to-have, tangential) Consider adding a tooltip describing the state of an entry (e.g. added from s3, removed, renamed from $key, etc. showing the physical key may be useful as well).
8. (Nice-to-have, tangential) Adding an option to toggle deleted files visibility may be useful to reduce visual clutter, tho it feels orthogonal and somewhat out of scope.
My main complaint, tho, is I feel like we're stretching the model too far and the abstractions start leaking -- this makes the logic harder to follow and less predictable, ultimately leading to unexpected errors and difficulties maintaining this code and developing it further. So my advice is to adjust the model to accommodate the new requirements and make it more consistent. Some suggestions:
1. Use composition: contain / reference File objects instead of mutating (extending) them directly.
2. "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".

nl0 · 2023-08-03T12:59:14Z

catalog/app/containers/Bucket/PackageDialog/EditFileMeta.tsx

+  const color = React.useMemo(() => {
+    if (!disabled && state !== 'invalid' && !R.isNil(value) && !R.isEmpty(value)) {
+      return 'primary'
+    }


what's the reason for implicitly returning undefined instead of explicit 'inherit'?

Just saved a couple of symbols, and done this consistently with EditFileName.tsx

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

…-conflicts

fiskus · 2023-08-03T15:20:20Z

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 · 2023-08-03T15:36:36Z

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 · 2023-08-04T11:16:16Z

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

catalog/app/components/Dialog/Prompt.tsx

Co-authored-by: Alexei Mochalov <[email protected]>

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

…nto resolve-name-conflicts

nl0 · 2023-08-04T11:45:05Z

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 · 2023-08-04T11:50:49Z

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 · 2023-08-04T11:59:01Z

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

nl0 · 2023-08-04T12:00:19Z

catalog/app/components/Dialog/Prompt.tsx

@@ -25,10 +29,11 @@ function Dialog({
  const error = React.useMemo(() => validate(value), [validate, value])
  const handleChange = React.useCallback((event) => setValue(event.target.value), [])
  const handleSubmit = React.useCallback(
-    (event) => {
+    (event: React.FormEvent) => {
+      // event.stopPropagation()


pls uncomment or remove

nl0 · 2023-08-04T12:02:04Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

+
+function resolveName(
+  name: string,
+  obj: Record<string, any>,


this argument could probably have a more descriptive name

It's here for keys only, so it doesn't matter what kind of object is here. Just some object with keys

nl0 · 2023-08-04T12:04:38Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

+  const newName = resolveNameConflictRudely(name, attempt)
+  if (!obj[newName]) return newName
+
+  return resolveName(name, obj, attempt + 1)


i doubt that would be an issue in practice, but in theory this can lead to error by exceeding the recursion depth (since js doesn't have a tail-call optimization, unfortunately), so if we are being pedantic, this function should be rewritten using a loop instead of recursion

Do you mean you don't want to use recursion in JS in general, or in this particular case?

i don't "don't want" to, it's just not safe (may lead to an exception) if the depth is unbounded, and here it well may be, since we don't control what files (and how many) a user adds.
idk, maybe it's kinda ok in practice in this particular case (source):

The maximal recursion depth is limited by JavaScript engine. We can rely on it being 10000, some engines allow more, but 100000 is probably out of limit for the majority of them. There are automatic optimizations that help alleviate this (“tail calls optimizations”), but they are not yet supported everywhere and work only in simple cases.

but in general, in js recursion shouldn't be used for possibly unbounded iterative processes (tho it's ok to use it to traverse recursive data structures, ofc)

Ok, but IMO it's an overkill to sacrifice elegance of recursion to cover edge case with 10000 duplicate names. Also, we already have a limit on number of files: 10000

nl0 · 2023-08-04T12:05:00Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

@@ -86,32 +109,108 @@ export type FilesAction = tagged.InstanceOf<typeof FilesAction>

 export type LocalFile = FileWithPath & FileWithHash

+// TODO: queue/array


what does this comment mean?

Use a list of objects instead of a single one. So, we can use infinite "revert"

nl0 · 2023-08-04T12:10:18Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

+    ...mainItems,
+    [resolvedName]: setKeyValue(
+      'conflict',
+      resolvedName === path ? undefined : path,


if i read it correctly, this won't unset the prop on a File instance, because undefined is ignored (see line 160). or am i missing smth?

It creates a copy of a file, first without 'conflict' (at cloning step), and then 'conflict' is ignoring (at setting step)

Don't invest time in it, since in the next iteration I will not do cloning :)

nl0 · 2023-08-04T12:12:04Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

+) {
+  const resolvedName = resolveName(
+    path,
+    itemsToCheck ? { ...mainItems, ...itemsToCheck } : mainItems,


{...undefined} works just fine, so you can skip the check

nl0 · 2023-08-04T12:13:24Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

+function addFile<T extends AnyFile, M extends Record<string, AnyFile>>(
+  path: string,
+  file: T,
+  mainItems: M,


why do you need this type var here? you're not using it. or is it needed for the return type?

Yes, I can skip it now. Earlier types were stricter, but I gave up

nl0 · 2023-08-04T12:23:45Z

catalog/app/containers/Bucket/PackageDialog/PackageCreationForm.tsx

@@ -625,6 +624,8 @@ function PackageCreationForm({
                      validate={validateFiles as FF.FieldValidator<$TSFixMe>}
                      validateFields={['files']}
                      errors={{
+                        conflict:
+                          'There are files with identical names. Rename or remove them please',


Suggested change

'There are files with identical names. Rename or remove them please',

'There are entries with conflicting logical keys. Rename or remove them please',

not sure if this is more user-friendly for less-tech-savvy people, but it seems more correct

I'm not sure if they know what logical key is, but I agree with the rest

nl0 · 2023-08-04T12:25:11Z

catalog/app/containers/Bucket/PackageDialog/PackageDialog.tsx

  logical_key: string
  size: number
  meta?: JsonRecord
 }

+export class EntryNameError extends BaseError {
+  static data: ValidationEntry


i don't quite get it why this is needed (or at least why it's static)

This is used for showing in which file there was an error. Under the FilesInput
static is a mistyping copypasta, thanks

nl0 · 2023-08-04T14:00:10Z

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx

+function resolveName(
+  name: string,
+  obj: Record<string, any>,
+  attempt: number = 1,
+): string {
+  if (!obj[name]) return name
+
+  const newName = resolveNameConflictRudely(name, attempt)
+  if (!obj[newName]) return newName
+
+  return resolveName(name, obj, attempt + 1)


i'm not sure what sacrifice do you mean:

Suggested change

function resolveName(

name: string,

obj: Record<string, any>,

attempt: number = 1,

): string {

if (!obj[name]) return name

const newName = resolveNameConflictRudely(name, attempt)

if (!obj[newName]) return newName

return resolveName(name, obj, attempt + 1)

function resolveName(name: string, obj: Record<string, any>): string {

let newName = name, attempt = 1

while (newName in obj) newName = resolveNameConflictRudely(name, attempt++)

return newName

drernie · 2023-08-21T21:23:32Z

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

fiskus · 2023-08-22T10:28:49Z

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 #3681

fiskus and others added 26 commits July 13, 2023 12:44

remve unused component

1124c00

edit filename

34f7fe1

remove optionality

2e19f06

adjust naming

b20e54f

wip: revert changes in existing files

f9b38d9

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

aa20c85

…-conflicts

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

9b6f97e

…-conflicts

wip

79773c0

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

9f84d33

…-conflicts

fix types

7356520

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

a318282

…-conflicts

resolve multiple names as conflicts

be2cd1a

move to standalone functions

a581a52

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

c446e36

…-conflicts

resolve conflicts after renaming

a99cc45

fix nested names

f1a0ce2

merge with master

b6615c1

lint

5ee3400

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

8df1656

…-conflicts

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

da7b69d

…-conflicts

fix checking existing entries

d673570

Merge branch 'resolve-name-conflicts' of github.com:quiltdata/quilt i…

b3af6dc

…nto resolve-name-conflicts

show invalid state for directory

50c8cc9

polish renaming

88bf4bb

rename functions

0c8d356

revert existing

9d60a4a

fiskus added 3 commits July 24, 2023 13:18

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

9ad2545

…-conflicts

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

1a930b9

…-conflicts

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

eb6fc9b

…-conflicts

fiskus and others added 5 commits August 2, 2023 16:20

fix cloning existing values

6986725

fix comparison

aaf7212

Apply suggestions from code review

7cfa0ee

Co-authored-by: Alexei Mochalov <[email protected]>

revert unnessesary change

e5f840f

use undefined to highlight lack of value

4a4335a

fiskus requested a review from nl0 August 3, 2023 09:44

nl0 reviewed Aug 3, 2023

View reviewed changes

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:quiltdata/quilt into resolve-name…

b2547f3

…-conflicts

fiskus added 4 commits August 4, 2023 12:26

support for customized button labels

c2561eb

stop propagation on file rename

3e2ccc4

show correct message on error

676b372

less types

cbe9fd8

nl0 reviewed Aug 4, 2023

View reviewed changes

catalog/app/components/Dialog/Prompt.tsx Outdated Show resolved Hide resolved

Update catalog/app/components/Dialog/Prompt.tsx

549c584

Co-authored-by: Alexei Mochalov <[email protected]>

nl0 reviewed Aug 4, 2023

View reviewed changes

catalog/app/containers/Bucket/PackageDialog/FilesInput.tsx Outdated Show resolved Hide resolved

fiskus added 2 commits August 4, 2023 14:28

move ternary to arg

d8e7ae1

Merge branch 'resolve-name-conflicts' of github.com:quiltdata/quilt i…

e33f96e

…nto resolve-name-conflicts

nl0 reviewed Aug 4, 2023

View reviewed changes

fiskus marked this pull request as draft August 11, 2023 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve name conflicts in packages #3596

Resolve name conflicts in packages #3596

fiskus commented Jul 20, 2023 •

edited

Loading

codecov bot commented Jul 20, 2023 •

edited

Loading

fiskus commented Aug 3, 2023

nl0 left a comment

nl0 Aug 3, 2023

fiskus Aug 3, 2023

fiskus commented Aug 3, 2023 •

edited

Loading

fiskus commented Aug 3, 2023

fiskus commented Aug 4, 2023

nl0 commented Aug 4, 2023

nl0 commented Aug 4, 2023

nl0 commented Aug 4, 2023

nl0 Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

fiskus Aug 4, 2023

nl0 Aug 4, 2023

drernie commented Aug 21, 2023

fiskus commented Aug 22, 2023 •

edited

Loading

		@@ -86,32 +109,108 @@ export type FilesAction = tagged.InstanceOf<typeof FilesAction>

		export type LocalFile = FileWithPath & FileWithHash

		// TODO: queue/array

	'There are files with identical names. Rename or remove them please',
	'There are entries with conflicting logical keys. Rename or remove them please',

Resolve name conflicts in packages #3596

Are you sure you want to change the base?

Resolve name conflicts in packages #3596

Conversation

fiskus commented Jul 20, 2023 • edited Loading

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

fiskus commented Aug 3, 2023

nl0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiskus commented Aug 3, 2023 • edited Loading

fiskus commented Aug 3, 2023

fiskus commented Aug 4, 2023

nl0 commented Aug 4, 2023

nl0 commented Aug 4, 2023

nl0 commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drernie commented Aug 21, 2023

fiskus commented Aug 22, 2023 • edited Loading

fiskus commented Jul 20, 2023 •

edited

Loading

codecov bot commented Jul 20, 2023 •

edited

Loading

fiskus commented Aug 3, 2023 •

edited

Loading

fiskus commented Aug 22, 2023 •

edited

Loading