Create and Delete documents (not folders yet) in the sidebar #60

andresgutgon · 2024-07-25T11:53:58Z

What

☝️ That

TODO

Add file IMPLEMENT server action
When creating a file reseting tmpFolders zustand store
Implement destroyOrSoftDeleteDocuments (used by both destroy document or destroy folder)
Destroy document server action
DRY File node display in File tree

Next PR

Destroy folder server action
Resizable sidebar
Make the sidebar files tree vertically scrollable. Custom scrollbar styles
Sidebar header with File + | Folder +
Review constraint in document version by commit and path. Now that documents can be soft deleted it will be possible to have same path in 2 different docs

andresgutgon · 2024-07-25T18:47:21Z

packages/core/src/services/documents/create.test.ts

@@ -3,6 +3,9 @@ import { describe, expect, it } from 'vitest'

 import { mergeCommit } from '../commits/merge'
 import { createNewDocument } from './create'
+import useTestDatabase from '$core/tests/useTestDatabase'
+
+useTestDatabase()


I'm not happy with this. I'm not sure the rollback is working on each test. We need to investigate

andresgutgon · 2024-07-25T18:51:40Z

apps/web/src/components/Sidebar/DocumentTree/index.tsx

Ideally we should NOT have apps/web/src/components ever. All the UI lives in packages/web-ui. What's in apps/web is the glue to use server actions + SWR stores. Nothing else. NO TAILWIND allowed in apps/web

andresgutgon · 2024-07-26T13:16:03Z

packages/web-ui/src/sections/Document/Editor/index.tsx

-                <div className='flex flex-row gap-4 w-full items-center'>
+              inputs.map((param, idx) => (
+                <div
+                  key={idx}


Always put your keys in react loops kids

andresgutgon · 2024-07-27T14:50:28Z

apps/web/src/app/(private)/_data-access/index.ts

@@ -54,20 +54,28 @@ export const getDocumentByUuid = cache(
    documentUuid: string
    commit: Commit
  }) => {
-    const workspace = await findWorkspaceFromCommit(commit)


This pattern for finding the workspace on commit can be solved in a more generic way when returning from a repository .find method. Instead of returning just the model we return the model with their relations. At least some of then.

What we're doing now is doing 1 more SQL query to get again the Workspace when we already got it to build the scope for that repository. I think this is doable. I might try in another PR.

andresgutgon · 2024-07-27T14:54:51Z

apps/web/src/app/(private)/_data-access/index.ts

+    if (result.error) {
+      const error = result.error
+      if (error instanceof NotFoundError) {
+        return notFound()


When is a not found we don't raise the error. We use NextJS notFound() method that display the not-found.tsx page

andresgutgon · 2024-07-27T14:57:34Z

apps/web/src/app/api/projects/[projectId]/commits/[commitUuid]/documents/route.ts

@@ -23,7 +23,7 @@ export async function GET(
    const commit = await commitsScope
      .getCommitByUuid({ uuid: commitUuid, project })
      .then((r) => r.unwrap())
-    const documents = await scope.getDocumentsAtCommit(commit)
+    const documents = await scope.getDocumentsAtCommit({ commit })


We had a discussion offline about using or not positional arguments. The conclusion we arrive at is that we hate it. No, I'm joking (kind of).

The conclusion was that we're going to have methods with positional arguments when we're sure this method will never have more arguments. This is an exception because I did this refactor before arriving at that conclusion and I'm sick of having this PR open. Feel free to change this one if you want.

andresgutgon · 2024-07-27T14:58:47Z

apps/web/src/stores/documentVersions.ts

+  const { project } = useCurrentProject()
+  const { commit } = useCurrentCommit()
+  const { mutate, data, ...rest } = useSWR<DocumentVersion[]>(
+    ['documentVersions', project.id, commit.id],


Let's agree on using Arrays for keys in the stores. URL like strings are harder to manipulate

andresgutgon · 2024-07-27T15:00:25Z

packages/core/src/repositories/documentVersionsRepository/getDocumentsAtCommit.test.ts

Huge refactoring of these tests. Please don't touch this file before I merge this PR 🙏

Resolving conflicts was taught

andresgutgon · 2024-07-27T15:01:45Z

packages/core/src/repositories/documentVersionsRepository/getDocumentsAtCommit.test.ts

+      expect(documents[0]!.content).toBe('VERSION_2')
+    })
+
+    it('get docs from version 3', async () => {


Before all these case was in a single test with all the initialization factories. Super hard to understand. Always try to put between 1 and 3 expectations by it block. Ideally one.

andresgutgon · 2024-07-27T15:03:05Z

packages/core/src/repositories/documentVersionsRepository/index.ts

-          eq(documentVersions.commitId, commit.id),
-          eq(documentVersions.documentUuid, documentUuid),
+          eq(this.scope.commitId, commit.id),
+          eq(this.scope.documentUuid, documentUuid),


Now that we use scopes we always have to use the scope for picking the columns of the table. Otherwise it doesn't work

💯 good catch

andresgutgon · 2024-07-27T15:05:13Z

packages/core/src/repositories/documentVersionsRepository/index.ts

@@ -145,6 +144,32 @@ export class DocumentVersionsRepository extends Repository {
    return Result.ok(changedDocuments)
  }

+  private async getAllDocumentsAtCommit({ commit }: { commit: Commit }) {


Returning all documents with deleted documents included is private. The moment we want to show then to for example "Recover from trash" we can implement getAllDeletedDocumentsAtCommit

andresgutgon · 2024-07-27T15:05:47Z

packages/core/src/repositories/documentVersionsRepository/index.ts

+
+    const documentsFromDraft = await this.db
+      .select(getTableColumns(documentVersions))
+      .from(documentVersions)


This is wrong. We should use this.scope. We'll fix in another PR

andresgutgon · 2024-07-27T15:06:57Z

packages/core/src/services/commits/create.ts

+  commit: {
+    projectId: number
+    title?: string
+    mergedAt?: Date


I prefer to be explicit about what means creating an object. This way people using don't include not permitted columns.

andresgutgon · 2024-07-27T15:09:18Z

packages/core/src/services/documents/destroyDocument.ts

+}: {
+  document: DocumentVersion
+  commit: Commit
+  workspaceId: number


This is not ideal as I commented here a commit passed here was found with a repository so the workspace was already there.

There has to be a nice way of having commit.workspace.id so object includes relations already resolved

andresgutgon · 2024-07-27T15:10:43Z

packages/core/src/services/documents/destroyOrSoftDeleteDocuments.ts

+    .where(eq(documentVersions.commitId, commitId))
+}
+
+/**


Thanks to Carlos for helping me with this. A lot is going on here. I tried my best to make as understandable as possible and added tests

andresgutgon · 2024-07-27T15:12:28Z

packages/core/src/services/documents/update.ts

-  document: DocumentVersion
-  path?: string
-  content?: string | null
-  deletedAt?: Date | null


Removed deletedAt from here now that we specific services for deleting documents.

andresgutgon · 2024-07-27T15:14:59Z

packages/web-ui/src/sections/Document/Sidebar/Files/index.tsx

-      {children}
-    </div>
-  )
-})


Now ☝️ a document and a folder share all the presentation layers and they only pass the necessary props. This way design is unified. This was a leftover refactor I had pending from initial file tree implementation

csansoon · 2024-07-29T07:10:41Z

packages/core/src/services/documents/destroyOrSoftDeleteDocuments.test.ts

+    await database
+      .update(documentVersions)
+      .set({
+        resolvedContent: '[CHACHED] Doc 1 (version 1)',


geclos · 2024-07-29T07:15:02Z

apps/web/src/stores/documentVersions.ts

+    [executeDestroyDocument, mutate, data],
+  )
+
+  return { ...rest, documents: data ?? [], createFile, destroyFile, mutate }


minor nitpicks: I'd prefer to keep it consistent between stores and always return data instead of a different-named prop that contains the data for each store. Also no need for the suffix "file" in actions. create and destroy is enough and beautifully concise.

We'll have destroyFolder in this store

geclos · 2024-07-29T07:15:07Z

apps/web/src/stores/documentVersions.ts

+  )
+
+  return { ...rest, documents: data ?? [], createFile, destroyFile, mutate }
+}


geclos · 2024-07-29T07:15:52Z

packages/core/src/repositories/documentVersionsRepository/getDocumentAtCommit.test.ts

+      resolvedContent: 'VERSION_1',
+    })
+  })
+})


csansoon · 2024-07-29T07:16:07Z

packages/core/src/services/documents/destroyOrSoftDeleteDocuments.ts

+    await Promise.all([
+      hardDestroyDocuments({ documents, existingUuids, tx }),
+      createDocumentsAsSoftDeleted({ toBeSoftDeleted, commitId, tx }),
+      updateDocumetsAsSoftDeleted({ toBeSoftDeleted, commitId, tx }),
+      invalidateDocumentsCacheInCommit(commitId, tx),
+    ])


How is it you're calling both createDocumentsAsSoftDeleted and updateDocumentsAsSoftDeleted with the same array?

I see that you're filtering them inside each method, but I think it should be more clear to filter it here instead. The create… and update… services just receive instructions, and if they fail that's responsibility of the caller.

I agree. Also the array can be conditionally computed since some of the methods do not require it.

geclos · 2024-07-29T07:17:40Z

packages/core/src/services/commits/create.ts

  db = database,
 }: {
-  commit: Omit<Partial<Commit>, 'id'>
+  commit: {
+    projectId: number


leave a todo this should be a project instead of projectid

csansoon · 2024-07-29T07:18:12Z

packages/core/src/tests/factories/documents.ts

+export async function markAsSoftDelete(documentUuid: string, tx = database) {
+  return tx
+    .update(documentVersions)
+    .set({ deletedAt: new Date() })
+    .where(eq(documentVersions.documentUuid, documentUuid))
+}
+


Add a commitId, otherwise you're marking it as deleted in the whole file history. A document should not be created being deleted (?)

csansoon · 2024-07-29T07:20:15Z

packages/web-ui/src/sections/Document/Sidebar/Files/NodeHeaderWrapper/index.tsx

+  // Litle trick to focus the input after the component is mounted
+  // We wait some time to focus the input to avoid the focus being stolen
+  // by the click event in the menu item that created this node.
+  useEffect(() => {
+    const timeout = setTimeout(() => {
+      inputRef.current?.focus(), 100
+    })
+
+    return () => {
+      clearTimeout(timeout)
+    }
+  }, [inputRef])


You could probably use 1 or even 0 ms and it should still work, right?

geclos · 2024-07-29T07:23:00Z

packages/core/src/services/documents/destroyOrSoftDeleteDocuments.test.ts

+import { destroyOrSoftDeleteDocuments } from './destroyOrSoftDeleteDocuments'
+
+describe('destroyOrSoftDeleteDocuments', () => {
+  it('hardDestroyDocuments', async () => {


can we make the titles self explanatory? why does it hard destroy in this test case but it creates in the next test case? It would be awesome if i could know the rule by just reading this line

geclos · 2024-07-29T07:24:05Z

packages/core/src/services/documents/destroyOrSoftDeleteDocuments.ts

+ * A document can:
+ *
+ * 1. Not exists in previous commits. In this case, it will be hard deleted
+ * 1. Exists in previous commits and in the commit. It will be updated the `deletedAt` field


geclos

awesome

* One more refactor to remove complexity from external api on Sidebar node wrapper * On input tab create a new child tmp folder under it to make easier build a tree * Address some comments from this PR: #60

andresgutgon added the 🚧 wip Work in progress label Jul 25, 2024

andresgutgon commented Jul 25, 2024

View reviewed changes

andresgutgon force-pushed the feature/add-files-to-sidebar-and-make-resizable branch 3 times, most recently from 38c000d to 7a41443 Compare July 26, 2024 13:14

andresgutgon commented Jul 26, 2024

View reviewed changes

andresgutgon force-pushed the feature/add-files-to-sidebar-and-make-resizable branch from 7a41443 to 0a34227 Compare July 27, 2024 12:08

andresgutgon commented Jul 27, 2024

View reviewed changes

andresgutgon changed the title ~~Save documents in DB from sidebar and make sidebar resizable~~ Create and Delete documents (not folders yet) in the sidebar Jul 27, 2024

andresgutgon removed the 🚧 wip Work in progress label Jul 27, 2024

andresgutgon force-pushed the feature/add-files-to-sidebar-and-make-resizable branch from c4cc0b5 to eea2fb9 Compare July 27, 2024 15:25

andresgutgon requested review from csansoon and geclos July 27, 2024 15:36

Save documents in DB from sidebar and make sidebar resizable

1d03f61

andresgutgon force-pushed the feature/add-files-to-sidebar-and-make-resizable branch from eea2fb9 to 1d03f61 Compare July 27, 2024 16:04

csansoon reviewed Jul 29, 2024

View reviewed changes

geclos reviewed Jul 29, 2024

View reviewed changes

csansoon reviewed Jul 29, 2024

View reviewed changes

geclos reviewed Jul 29, 2024

View reviewed changes

csansoon reviewed Jul 29, 2024

View reviewed changes

geclos reviewed Jul 29, 2024

View reviewed changes

geclos approved these changes Jul 29, 2024

View reviewed changes

andresgutgon merged commit 1477638 into main Jul 29, 2024
2 checks passed

andresgutgon deleted the feature/add-files-to-sidebar-and-make-resizable branch July 29, 2024 07:58

andresgutgon added a commit that referenced this pull request Jul 29, 2024

Address some comments from this PR: #60

c056997

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create and Delete documents (not folders yet) in the sidebar #60

Create and Delete documents (not folders yet) in the sidebar #60

andresgutgon commented Jul 25, 2024 •

edited

Loading

andresgutgon Jul 25, 2024

andresgutgon Jul 25, 2024

andresgutgon Jul 26, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024 •

edited

Loading

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

geclos Jul 29, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

csansoon Jul 29, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

andresgutgon Jul 27, 2024

csansoon Jul 29, 2024

geclos Jul 29, 2024

andresgutgon Jul 29, 2024 •

edited

Loading

geclos Jul 29, 2024

geclos Jul 29, 2024

csansoon Jul 29, 2024

geclos Jul 29, 2024

geclos Jul 29, 2024

csansoon Jul 29, 2024

csansoon Jul 29, 2024

geclos Jul 29, 2024

geclos Jul 29, 2024

geclos left a comment

Create and Delete documents (not folders yet) in the sidebar #60

Create and Delete documents (not folders yet) in the sidebar #60

Conversation

andresgutgon commented Jul 25, 2024 • edited Loading

What

TODO

Next PR

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

andresgutgon Jul 27, 2024 • edited Loading

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

andresgutgon Jul 29, 2024 • edited Loading

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

geclos left a comment

Choose a reason for hiding this comment

andresgutgon commented Jul 25, 2024 •

edited

Loading

andresgutgon Jul 27, 2024 •

edited

Loading

andresgutgon Jul 29, 2024 •

edited

Loading