Convert to BlockNote, Image support, Video Support, tamagui theme, white and black theme #503
Conversation
|
I will clean the code up, just want suggestions! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a major architectural change, converting the editor from TipTap to BlockNote while adding image/video support and implementing theme switching using Tamagui.
- Critical security concern in
/electron/main/filesystem/storage/MediaStore.ts- potential path traversal vulnerability in file operations and lack of file size limits - Unsafe implementation in
/src/lib/blocknote/core/extensions/LinkMenu/LinkMenuPlugin.ts- direct use of user-provided URLs without sanitization in link handling - Empty configuration files
.tamagui/tamagui-components.config.cjsand.tamagui/tamagui.config.jsoncould break theme functionality - Commented out code and @ts-ignore directives throughout BlockNote implementation suggest incomplete migration
- Memory leak potential in
/src/components/Editor/types/Video/video.tsxfrom unremoved event listeners and unsafe type assertions
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
293 file(s) reviewed, 456 comment(s)
Edit PR Review Bot Settings | Greptile
| 'jsx-a11y/no-static-element-interactions': 'off', | ||
| 'jsx-a11y/click-events-have-key-events': 'off', |
There was a problem hiding this comment.
style: Disabling these accessibility rules may make the app less accessible to keyboard and screen reader users. Consider enabling them and properly handling keyboard interactions.
| @@ -48,7 +48,7 @@ jobs: | |||
| CSC_LINK: ${{ secrets.CSC_LINK }} | |||
| CSC_KEY_PASSWORD: ${{ secrets.CSC_KEY_PASSWORD }} | |||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |||
| NODE_OPTIONS: "--max-old-space-size=8192" | |||
| NODE_OPTIONS: '--max-old-space-size=8192' | |||
There was a problem hiding this comment.
style: Consider moving NODE_OPTIONS to a shared environment variable at the job level since it's used multiple times with the same value
| } |
There was a problem hiding this comment.
style: Files should end with a newline character to follow POSIX standards and avoid potential issues with text processing tools. Consider adding back the trailing newline.
| } | |
| } |
| var conf = { | ||
| themes, | ||
| defaultFont: "body", | ||
| animations, | ||
| shouldAddPrefersColorThemes: true, | ||
| themeClassNameOnRoot: true, | ||
| shorthands, | ||
| fonts: { | ||
| heading: headingFont, | ||
| body: bodyFont, | ||
| mono: monoFont, | ||
| editorBody | ||
| }, | ||
| tokens: (0, import_web4.createTokens)({ | ||
| color, | ||
| radius, | ||
| zIndex, | ||
| space, | ||
| size, | ||
| opacity: { | ||
| low: 0.4, | ||
| medium: 0.6, | ||
| high: 0.8, | ||
| full: 1 | ||
| } | ||
| }), | ||
| media, | ||
| settings: { | ||
| webContainerType: "inherit" | ||
| } | ||
| }; |
There was a problem hiding this comment.
style: Consider extracting opacity values into a separate constants file for better maintainability
| "hsl(0, 0%, 56.1%)", | ||
| "hsl(0, 0%, 52.3%)", | ||
| "hsl(0, 0%, 43.5%)", | ||
| "hsl(0, 0%, 92.0%)", |
There was a problem hiding this comment.
logic: Duplicate value 'hsl(0, 0%, 92.0%)' in light_customGray palette may cause confusion with gray12
| */ | ||
| defaultOpened={true} | ||
| trigger={'hover'} | ||
| closeDelay={10000000} |
There was a problem hiding this comment.
logic: Using 10000000 as closeDelay is a fragile solution. Consider implementing proper menu state management or using Infinity
| let index = 0 | ||
|
|
||
| const groups = _.groupBy(props.filteredItems, (i) => i.group) | ||
| // const showNostr = trpc.experiments.get.useQuery().data?.nostr |
There was a problem hiding this comment.
style: Remove commented out code related to Nostr if it's no longer needed
| setTimeout(() => { | ||
| scroller.current = document.getElementById('scroll-page-wrapper') | ||
| }, 100) | ||
| }, []) |
There was a problem hiding this comment.
style: Using setTimeout to access DOM element is fragile. Consider using a MutationObserver or effect cleanup to handle scroll-page-wrapper availability.
| useEffect(() => { | ||
| // Updates whether the item is selected with the keyboard (triggered on selectedIndex prop change). | ||
| updateSelection() | ||
|
|
||
| // if ( | ||
| // isSelected() && | ||
| // itemRef.current && | ||
| // itemRef.current.getBoundingClientRect().left > MIN_LEFT_MARGIN //TODO: Kinda hacky, fix | ||
| // // This check is needed because initially the menu is initialized somewhere above outside the screen (with left = 1) | ||
| // // scrollIntoView() is called before the menu is set in the right place, and without the check would scroll to the top of the page every time | ||
| // ) { | ||
| // itemRef.current.scrollIntoView({ | ||
| // behavior: 'smooth', | ||
| // block: 'nearest', | ||
| // }) | ||
| // } | ||
| }) |
There was a problem hiding this comment.
logic: Missing dependency array causing unnecessary re-renders on every render cycle
| useEffect(() => { | |
| // Updates whether the item is selected with the keyboard (triggered on selectedIndex prop change). | |
| updateSelection() | |
| // if ( | |
| // isSelected() && | |
| // itemRef.current && | |
| // itemRef.current.getBoundingClientRect().left > MIN_LEFT_MARGIN //TODO: Kinda hacky, fix | |
| // // This check is needed because initially the menu is initialized somewhere above outside the screen (with left = 1) | |
| // // scrollIntoView() is called before the menu is set in the right place, and without the check would scroll to the top of the page every time | |
| // ) { | |
| // itemRef.current.scrollIntoView({ | |
| // behavior: 'smooth', | |
| // block: 'nearest', | |
| // }) | |
| // } | |
| }) | |
| useEffect(() => { | |
| // Updates whether the item is selected with the keyboard (triggered on selectedIndex prop change). | |
| updateSelection() | |
| // if ( | |
| // isSelected() && | |
| // itemRef.current && | |
| // itemRef.current.getBoundingClientRect().left > MIN_LEFT_MARGIN //TODO: Kinda hacky, fix | |
| // // This check is needed because initially the menu is initialized somewhere above outside the screen (with left = 1) | |
| // // scrollIntoView() is called before the menu is set in the right place, and without the check would scroll to the top of the page every time | |
| // ) { | |
| // itemRef.current.scrollIntoView({ | |
| // behavior: 'smooth', | |
| // block: 'nearest', | |
| // }) | |
| // } | |
| }, [props.isSelected]) |
|
|
||
| return () => newRect as DOMRect | ||
| }, | ||
| [referencePos.current], // eslint-disable-line |
There was a problem hiding this comment.
logic: Dependency array includes referencePos.current which may cause unnecessary rerenders. Should use referencePos instead.
| [referencePos.current], // eslint-disable-line | |
| [referencePos], // Track the ref itself, not its current value |
Changes Overview
This PR introduces several significant updates to the editor functionality and UI:
Converted
useEditorfrom TipTap to Custom BlockNoteSchemaAdded Theme Switching in Bottom Left Corner Using Tamagui
Added Image and Video Support
Why This PR is a Single Unit
I understand that these changes could have been broken into smaller, more focused PRs. However, many of these updates were interdependent and built on top of each other, making it challenging to separate them without causing inconsistencies or breaking functionality. This PR serves as a comprehensive update for testing and gathering feedback.
Next Steps
Acknowledgments
Special thanks to the Mintter open-source project for their work on BlockNoteSchema, which greatly influenced this implementation. Their contributions to the community are invaluable!