Adds Redux store by cguedes · Pull Request #30 · refstudio/refstudio

cguedes · 2023-05-24T10:59:53Z

This PR adds redux using redux toolkit to simplify the application data flow and closes issue #33

Slices

With this change, two slices were created:

selection - store for the selected text in the editor
openedFiles - store for the opened files, and the selected file.

UI Component Changes

With the addition of the redux store, the following components were changed/added:

TipTapEditor - to dispatch selection changes (debounced)
AIView - use the store instead of props
FoldersView - use the store instead of props
CenterPaneView - rewrite to display different types of center views depending on the selected file
OpenedFilesView - added to display and manage the selected files. It allow file selection and close

Current UI

hammer · 2023-05-24T17:28:42Z

Nice work! In general I prefer to keep PRs restricted to a single coherent set of changes, so in the future please break out the startup scripts, TailwindCSS, and git hook changes separately if it's not too much trouble.

cguedes · 2023-05-24T18:19:02Z

Nice work! In general I prefer to keep PRs restricted to a single coherent set of changes, so in the future please break out the startup scripts, TailwindCSS, and git hook changes separately if it's not too much trouble.

You are right. I will do my best to scope these changes in separate PRs.

- Adds first slice for the selected text - got rid of onSelectionChange Still not convinced on where to store the slice files. Under `features` (like they have in the docs/examples) or `store` or `redux` (keeping all the redux store in a single place)

- Created OpenedFilesView (tab panes) - Cleanup App, FoldersView and CenterPane

cguedes · 2023-05-25T13:15:39Z

@hammer @sergioramos I've cleaned up this PR to only include the code related to the redux-store (for the #33).

Created issues and PRs for all the other features implemented before here.

danvk

It was interesting to learn about Redux Toolkit in the process of reviewing this PR! I left a lot of comments, but at a high level:

I do think it's a good idea to have some sort of state management tool for truly global application state.
That tool may or may not be RKT, as per our discussion of Recoil earlier today.
Even with a state management library, we should try to:
1. Use built-in state management (useState) for state that's local to a component.
2. Keep low-level components presentational (store agnostic) for better encapsulation / separation of concerns.
3. Avoid multiple sources of truth (i.e. state in the global store and in local state).

danvk · 2023-05-25T15:20:11Z

-import { EDITOR_EXTENSIONS, INITIAL_CONTENT } from './TipTapEditorConfigs';
+import { EDITOR_EXTENSIONS } from './TipTapEditorConfigs';
+
+export function TipTapEditor({ editorRef, editorContent }: EditorProps) {


I prefer to keep Props type declarations in the same place as their components. They're always tightly coupled so there's no point in separating them.

I agree. We did this to suppport multiple editor implementation(i.e. Lexical) and we wanted to keep a common interface, but we can drop that for now.

danvk · 2023-05-25T15:21:37Z

-      },
-    }));
-  }, [editorContent, onSelectionChange]);
+    setEditor(


This is existing code, but will this create a new text editor any time we want to change the text in it? I imagine that would be expensive?

I would have expected this effect to (conceptually) look more like:

useEffect(() => { editor.setText(editorContent); }, [editorContent]);

We need to refactor this code (interaction with TipTap), but unfortunately their API to set/reset content isn't straightforward. CC @sehyod

Yes, unfortunately, using setText leads to unwanted behaviours, especially regarding the history, because there is no easy way to clear the history (cf ueberdosis/tiptap#491).
I think at some point we will have in-memory editors anyway, corresponding to open tabs, so the create operation would only be called once per file.

danvk · 2023-05-25T15:24:18Z

+    dispatch(setTextSelection(debouncedSelection));
+  }, [dispatch, debouncedSelection]);

  useEffect(() => {


Existing code, but suggest adding a one line comment explaining what this is doing.

We moved that code already to the receiving party and it's now more simple to understand.
Also, this is only "sample interaction code" between the editor and the AI Panel.

danvk · 2023-05-25T15:27:29Z

Is there value in having nested directories under features? What would go in openedFiles other than openedFilesSlice.ts? I'd suggest flattening the structure until there's a reason to nest.

I wasn't sure we would have more files for each feature. But agree we should start with a simpler tree.

danvk · 2023-05-25T15:29:41Z

+
+export interface FileEntryInfo {
+  entry: FileEntry;
+  type: FileEntryType;


This would typically be modeled as a tagged union since some combinations (type=PDF and isDirectory=true) don't make sense. This may be fine for now, but if we ever start adding properties that are only relevant for one type, this should be changed.

Exactly. I wasn't sure if this was the correct approach.
I wanted to create a wrapper over the file entry with additional properties (ex: type) but wasn't sure how "complex" the TS should be.

danvk · 2023-05-25T16:27:03Z

-}
-
-const FileTree = ({ files, root, ...props }: FileTreeBaseProps) => {
+function FileTree({ files, root = false }: { files: FileEntry[]; root?: boolean }) {


Keep FileTree and FileTreeNode as pure presentational components that don't know about the global store. FoldersView can tie these to the store. This will be helpful for maintaining some separation / abstraction.

Sure, I think that we would build more code but in the end it pays off as we can test the UI component in isolation from the store, and we can also replace UI elements (ex: TreeNode) by libraries.

Keep low-level components presentational (store agnostic) for better encapsulation / separation of concerns.

danvk · 2023-05-25T16:29:27Z


+  function handleOnClick(file: FileEntry): void {
+    dispatch(
+      openFile({


Now that you have a FileEntryInfo type, would it be simpler for FileTree to use that instead of muxing between FileEntry and FileEntryInfo?

I see, that's in line with the change of refactoring the component to be more isolated.

danvk · 2023-05-25T16:31:38Z

+  const dispatch = useDispatch();
+  const openedFiles = useAppSelector((state) => state.openedFiles);
+
+  if (openedFiles.files.length === 0) return null;


Is there a reason to return null in this case rather than an empty <div>?

We just want to prevent rendering given that the component don't need a wrapper div/styles.

https://react.dev/learn/conditional-rendering#conditionally-returning-nothing-with-null

danvk · 2023-05-25T16:32:25Z

+          className={cx(
+            'flex flex-wrap items-center justify-between',
+            'cursor-pointer select-none whitespace-nowrap',
+            'px-2 py-1', //


what's going on with the // here? (also in a few other places)

This is for legibility. That would make prettier wrap the array in multiple lines, even with fewer statements.
Also, the idea is to break (using cx) a long list of rules into simpler rules/lines. This also makes diff simpler.

danvk · 2023-05-25T17:09:46Z

+            dispatch(selectFile(fileInfo));
+          }}
+          className={cx(
+            'flex flex-wrap items-center justify-between',


With the caveat that I haven't used TailwindCSS before so it's hard for me to judge whether this is normal… the logic of the component really feels like it's getting drowned out by all these class names. Would it make sense to factor out a meaningfully-named component that applies most of these styles? https://tailwindcss.com/docs/reusing-styles#extracting-components-and-partials

Sure. The focus of this PR was adding the state management (Redux) so I didn't added much ❤️ into that code. For the IDE issue I will fix this.

cguedes · 2023-06-09T13:39:00Z

Closing this as we decided not to use Redux for this project. See #33 for more details.

cguedes requested a review from sehyod May 24, 2023 12:32

sehyod previously approved these changes May 24, 2023

View reviewed changes

Comment thread src/views/AIView.tsx Outdated

Comment thread src-tauri/src/main.rs Outdated

Comment thread src/views/CenterPaneView.tsx Outdated

cguedes dismissed sehyod’s stale review via df4cb5b May 24, 2023 12:58

cguedes marked this pull request as ready for review May 24, 2023 15:59

cguedes requested a review from sehyod May 24, 2023 15:59

sehyod previously approved these changes May 24, 2023

View reviewed changes

Comment thread src/views/CenterPaneView.tsx

This was referenced May 24, 2023

Improve TipTap editor content replacement #32

Closed

[Discussion] Application State Management #33

Closed

cguedes added 2 commits May 25, 2023 14:11

Adds Redux store

4e7a36c

- Adds first slice for the selected text - got rid of onSelectionChange Still not convinced on where to store the slice files. Under `features` (like they have in the docs/examples) or `store` or `redux` (keeping all the redux store in a single place)

Add openedFiles slice (redux)

1a1cf94

- Created OpenedFilesView (tab panes) - Cleanup App, FoldersView and CenterPane

cguedes dismissed sehyod’s stale review via 1a1cf94 May 25, 2023 13:12

cguedes force-pushed the cguedes/redux-store branch from 64e7bdd to 1a1cf94 Compare May 25, 2023 13:12

Fix tailwind rule

9047d16

cguedes requested a review from sehyod May 25, 2023 13:15

danvk self-requested a review May 25, 2023 13:58

danvk reviewed May 25, 2023

View reviewed changes

sergioramos marked this pull request as draft May 31, 2023 09:30

cguedes closed this Jun 9, 2023

Uh oh!

Conversation

cguedes commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hammer commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cguedes commented May 24, 2023

Uh oh!

cguedes commented May 25, 2023

Uh oh!

danvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cguedes commented Jun 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cguedes commented May 24, 2023 •

edited

Loading

hammer commented May 24, 2023 •

edited

Loading