Naming a folder 'root' causes issues

[meiamsome]

Nature of issue?

• Found a bug

Details about the bug:

This is a related issue to #842, but requires a more in-depth refactoring to fix.

• Steps to reproduce this bug:

1. Create a folder called root. The folder is created, but shows up as blank in the sidebar due to the special behavior for a directory based on this name.

You can also cause a crash by doing the following:

1. Create a folder named something that isn't 'root', and add a file to it.
2. Rename the folder to 'root' and save the project.
3. When you next load the project, it will crash the front-end. (Example sketch: https://editor.p5js.org/meiamsome/sketches/cyNfPGeDu )

[catarak]

thanks for reporting! i figured this would come up eventually. in creating the tree structure for the files/folders, for some reason i came up with this solutions, which down the line i realized wasn't perfect, but so be it. my proposed solution is

• change the root folder to be denoted by the fileType attribute
• change change the code that looks for file.name === 'root' to file.fileType === 'root'

how to handle existing sketches with the old data structure? i see two options

• run a data migration on the database (there's currently 430k entries so this could take a while)
• run a check in js—if there is no file with file.fileType === 'root', look for file.name === 'root'

[meiamsome]

@catarak That does sound like a reasonable suggestion, but might I suggest the following refactor instead:

Add to the project schema a rootFileId field. This field would be used for looking up the root folder and so it would not have anything special about it in the files structure. That would mean that it is less likely that someone will try and create multiple files of type 'root' or any similar shenanigans.

In a similar vein, I would also like to propose that the project schema gains a selectedFileId field and the files themselves lose the isSelected attribute. This would be also help to prevent situations where zero or more than one files have the attribute attached to them which would be an invalid state. (#687)

These both obviously still have the same problem of having to run a migration, so that part of the conversation remains the same either way, but while we're suggesting one migration I thought I'd throw in a suggestion that we can do both together.

It seems to me at least that the concept of the currently selected file and the root folder belong on the project and not the files themselves? Would be good to hear your thoughts

[catarak]

given that these are unique per project, i agree, it makes sense to put them at a project level. and i think we'd figure out a way to make the data migration as painless as possible.

also, with respect to the selected file attribute—i've wondered if this should be persistent and saved server-side or if it's fine to just select the "main" javascript file (determining this is non-trivial) when a user opens a sketch. do you have any thoughts?

[meiamsome]

@catarak I definitely agree that it is a bit weird to keep that state on the server when it's clearly a user/editor preference kind of thing, but I think it makes sense as a way to remove the non-trivial operation you describe. I would definitely be for creating an issue where we could rethink that property from a wider perspective. It might even be worth storing the currently selected file in something like session-storage so that once you have visited someone's sketch, reloading it won't take you to a different file, and then use the project's selectedFileId as a fallback. I can't think of a good way to determine what should be the default file in a project generally so I would agree that this is a hard problem.

Regarding how to make these migrations, I think I like the 'check in JS' as you suggest. I propose the following more concrete steps:

1. Add mongoose virtuals for the new keys, and to generate them on the fly from the files array
2. Work on porting over the behaviour of the front end whilst both methods of accessing that data are valid
3. Move the update behaviour over to a virutal setter
4. Convert the field on the Project model into a concrete field, defaulted with the getter logic
5. At some point in the future, we can run a script or something to load & resave all the projects thus filling in the property in all cases
6. Apply a migration that will delete the isSelected attribute on all files.

5 and 6 are optional and we could just not do them

[catarak]

this sounds good to me! maybe for now it makes sense to create the rootFileId and selectedFileId for each project in this way (since the workflow is basically the same), and then open another ticket that's more generally rethinking the "selected file".