adding files and folders as well as right-click with firebase integration#200
adding files and folders as well as right-click with firebase integration#200meher0704 wants to merge 12 commits into
Conversation
|
@MeherKhurana is attempting to deploy a commit to the CP Initiative Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
nice! some initial notes:
- it would be great if we could keep the IDE project as simple as possible, even if that means removing features. we don't have a large engineering team, and maintaining a large codebase is challenging. Specifically, do you think file tags are useful / what was the motivation behind adding them?
- The IDE is supposed to be mobile-friendly. Not sure how context menus will interact with that; maybe a button to open a dropdown would be better?
- Permanently deleting files seems scary (and our backend server doesn't support this anyway), what if we just renamed "hide / unhide" to "delete / restore" and have a "recently deleted" page?
- When creating a new file, you also need to specify things like "visibility" / "compiler options" / etc, so I'm not sure if the "right click to make a new file" thing really makes sense -- would it be better to just have the "new file" button make a new file in the current active folder or something?
- Do you think it makes more sense to have the "back" button inside the file explorer or outside?
|
Code Organization & File Management Updates Current Issues 
Planned Improvements
Collaboration Features
Mobile Experience
|
|
I think I agree that we should have some form of a file explorer. To clarify my question above, I was wondering while file tags in particular are needed.
We already have a dedicated creation page: https://ide.usaco.guide/new I have a slight preference against a model.
Personally I think what you have now is fine, but up to you (as long as it doesn't make the code too complicated)
If we are going to do this, can we do this in a separate PR?
Maybe let's talk before you implement these -- in particular, I want to make sure these features are actually needed & won't over-complicate the IDE. thanks again for working on this! |
| * Stored in /users/{uid}/files/{fileID} in Firebase. | ||
| */ | ||
| export type UserFile = { | ||
| type: string; |
There was a problem hiding this comment.
it determines if the user is storing is a file or a folder
There was a problem hiding this comment.
got it. what do you think about storing them separately: /users/{uid}/files/{fileID} and /users/{uid}/folders/{folderID}? or even /users/{uid}/files/folders/{folderID} if you prefer.
That way we can type them differently. In particular, some fields of files are not relevant for folders, like "language."
There was a problem hiding this comment.
well storing separately will make the code significantly more complicated;
i would suggest
export type File = {
kind: "file";
id: string;
name: string;
language?: string;
// File-specific fields
};
export type Folder = {
kind: "folder";
id: string;
name: string;
// Folder-specific fields
};
export type UserItem = File | Folder;
using a discriminated union
so that it doesn't become as complex and still is one file
There was a problem hiding this comment.
sure, I'm ok with that!
|
For the collaboration i will work on it in a separate pr and for now in this pr
|
|
Sounds good, please double check with me before you implement the collaboration features -- in particular, I'm not yet convinced we need them:
We already have shared files, I'm not really sure if shared folders are that important? I also think it's pretty tricky to give folders permissions; then you have to worry about how file permissions and folder permissions interact with each other. Maybe there are more impactful things to do first like #201 ?
Similar sentiment to above -- I'm not sure if the effort of implementing this is worth the benefits
We have a "scribble" pane already that seems to do this?
This seems useful. Note that vscode has a "timeline" feature that we could maybe try to build off of, or maybe we can modify our IDE YJS server to track version history, I'm not really sure. |
|
ok i will finish this then work on #201 |
|
|
|
Seems fine, though personally I might prefer a three-dot dropdown menu on the right with options to rename or delete -- that feels less cluttered than having icons for deleting / renaming each file, since most users won't be looking to do that action. Also, I'm not sure if the "1 item selected" thing is necessary (even though it's super cool!). In particular, when someone clicks a file on mobile, they probably want to open that file for editing, not select it. Bulk file renaming doesn't really make sense and bulk file deleting probably isn't that important. Maybe we can look at the google drive app for inspiration? |
|
i fixed it by just making the tap better so its rrly easy to open on both mobile and desktop; i tried both and determine that this was significantly easier to use; it simplifies the layout |
|
i think this is done. should i add anything? |
thecodingwizard
left a comment
thecodingwizard
left a comment
There was a problem hiding this comment.
Incredible! Some small nits below; feel free to disagree with these suggestions :)
-
No need for x-axis padding in the file manager -- it should be flush with "Real-time Collaborative Online IDE"
-
I think it's strange to have two things that can scroll, the file manager and the website as a whole. Maybe the file manager can just be as tall as needed?
-
Rather than having Py / C++ / etc next to date modified, maybe we can change the icon on the left hand side to either be Python, C++, Java, or a folder?
-
When the three dots dropdown opens, I don't think it's necessary to blur the rest of the page. It feels a bit excessive to me.
-
The New Folder & Recently Deleted icons can probably be de-emphasized (maybe they can be styled similarly to the "Back" button?)
-
I think the right click for context menu is cool, but since there's already a New Folder button, I feel like i'd rather not have the right click for context menu so there's less code we have to maintain.
-
At some screen sizes, the wrapping is a little strange
-
The "no files or folders" screen should probably be centered in the panel, currently it looks like there's more padding at the bottom than at the top
Overall incredible work!
|
pushed those updates; as you add files the file manager will extend; there is only one scroll |
|
For consistency can we use https://www.melt-ui.com/docs/builders/dropdown-menu to handle the three dots dropdown menu instead of capturing the x / y position ourselves? We use Melt UI for the navbar file dropdown menu already. Also, looks like there's some old code related to the context menu that can probably be removed? |
|
pushed those updates and removed unused code |
|
should i add anything else ? |
|
wait why close? |
|
i acc closed mb |
|
im not sure about the python logo its kind of grainy; should i replace it |
|
hmm maybe if there's another one you think is better. will defer to you :) |
|
i think it is done now ? i finished debugging and didn't find any issues |
|
@thecodingwizard is this good to merge? |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const handleDropAction = async (draggedItem: FileItem, targetFolder: FileItem) => { | ||
| if (targetFolder.type === 'folder' && draggedItem.id !== targetFolder.id) { | ||
| await update(ref(database, `users/${firebaseUser.uid}/files/${draggedItem.id}`), { | ||
| parentFolder: targetFolder.id | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔴 Drag-and-drop allows creating circular folder references
When dragging a folder into another folder, handleDropAction only checks that the dragged item is not the same as the target folder (draggedItem.id !== targetFolder.id), but does not check if the target folder is a descendant of the dragged folder.
Click to expand
Mechanism
In Dashboard.svelte:182-188, the handleDropAction function allows moving any folder into any other folder:
const handleDropAction = async (draggedItem: FileItem, targetFolder: FileItem) => {
if (targetFolder.type === 'folder' && draggedItem.id !== targetFolder.id) {
await update(ref(database, `users/${firebaseUser.uid}/files/${draggedItem.id}`), {
parentFolder: targetFolder.id
});
}
};Impact
If a user drags folder A into folder B (which is inside folder A), this creates a circular reference. The goBack function at line 196-205 relies on finding the parent folder, and filesToShow at line 86-92 filters by parentFolder. A circular reference would make both folders and all their contents inaccessible through normal navigation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const moveToTrash = async (item: FileItem) => { | ||
| await update(ref(database, `users/${firebaseUser.uid}/files/${item.id}`), { | ||
| isDeleted: true, | ||
| deletedAt: Date.now() | ||
| }); | ||
| open.set(false); | ||
| }; |
There was a problem hiding this comment.
🔴 Moving folder to trash orphans all files inside the folder
When a folder is moved to trash, files inside the folder are not recursively marked as deleted, causing them to become inaccessible.
Click to expand
Mechanism
The moveToTrash function at Dashboard.svelte:118-124 only marks the single item as deleted:
const moveToTrash = async (item: FileItem) => {
await update(ref(database, `users/${firebaseUser.uid}/files/${item.id}`), {
isDeleted: true,
deletedAt: Date.now()
});
};The filesToShow derived at line 86-92 filters files by:
file.parentFolder === currentFolder && !file.isDeletedfor normal viewfile.isDeletedfor recently deleted view
Actual vs Expected
- Actual: When folder X is deleted, files inside X are NOT marked as deleted. They won't appear in "Recently Deleted" (because
isDeletedis false), and they won't appear in normal view (because theirparentFolderpoints to a deleted folder). - Expected: Child files should either be recursively marked as deleted, or the view logic should account for files whose parent folders are deleted.
Impact
Files inside deleted folders become permanently orphaned and inaccessible to the user.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const restoreItem = async (item: FileItem) => { | ||
| await update(ref(database, `users/${firebaseUser.uid}/files/${item.id}`), { | ||
| isDeleted: false, | ||
| deletedAt: null | ||
| }); | ||
| open.set(false); | ||
| }; |
There was a problem hiding this comment.
🔴 Restoring file may leave it orphaned if parent folder is still deleted
When restoring a file from trash, the code doesn't verify that the file's parentFolder still exists or isn't itself deleted.
Click to expand
Mechanism
The restoreItem function at Dashboard.svelte:126-132 simply sets isDeleted: false:
const restoreItem = async (item: FileItem) => {
await update(ref(database, `users/${firebaseUser.uid}/files/${item.id}`), {
isDeleted: false,
deletedAt: null
});
};If a file was in a folder that was also deleted, restoring the file keeps its original parentFolder reference. But since filesToShow (line 91) filters by file.parentFolder === currentFolder && !file.isDeleted, the restored file won't appear anywhere if its parent folder is still in trash.
Impact
Restored files may remain inaccessible if their parent folder hasn't been restored first, leading to user confusion and apparent data loss.
Recommendation: When restoring a file, check if its parentFolder is deleted. If so, either restore the parent folder chain, or reset parentFolder to null to place the file in the root directory.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
is there a link for this |
5 participants
the files and folder look like
and the file structure looks like