Extending blocknote #518
Extending blocknote #518
Conversation
…rsing. Need to extend to work for any file
…und file creation and search.
…ternal and external links.
![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 significantly extends the BlockNote editor with internal/external linking capabilities and semantic file search functionality, along with improved file path management and caching mechanisms.
- Security concern in
electron/main/path/ipcHandlers.ts: The 'find-absolute-path' handler allows resolving arbitrary paths without validation, potentially enabling access outside intended directories src/lib/utils/cache/fileSearchIndex.tslacks error handling for race conditions in async operations and path manipulationsrc/lib/semanticService.tsxneeds improved error handling and content sanitization for semantic searchsrc/lib/utils/editor-state.tshas potential memory leaks due to uncleaned semantic cache entriessrc/lib/blocknote/react/LinkToolbar/components/LinkToolbarContent.tsxcontains an unbounded re-render loop with a 100ms interval
39 file(s) reviewed, 38 comment(s)
Edit PR Review Bot Settings | Greptile
| export function createFileRecursive(filePath: string, content: string, charset?: BufferEncoding): FileInfo | null { | ||
| const dirname = path.dirname(filePath) | ||
|
|
||
| if (!fs.existsSync(dirname)) { | ||
| fs.mkdirSync(dirname, { recursive: true }) | ||
| } | ||
|
|
||
| if (fs.existsSync(filePath)) { | ||
| return | ||
| return null | ||
| } | ||
| const filePathWithExtension = addExtensionToFilenameIfNoExtensionPresent(filePath, markdownExtensions, '.md') | ||
|
|
||
| fs.writeFileSync(filePathWithExtension, content, charset) | ||
| return { | ||
| name: path.basename(filePathWithExtension), | ||
| path: filePathWithExtension, | ||
| relativePath: path.relative(dirname, filePathWithExtension), | ||
| dateModified: new Date(), | ||
| dateCreated: new Date(), | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: The function now returns a FileInfo object with hardcoded dates. Consider using the actual file stats (mtime and birthtime) from fs.statSync() after file creation for accuracy.
| export function createFileRecursive(filePath: string, content: string, charset?: BufferEncoding): FileInfo | null { | |
| const dirname = path.dirname(filePath) | |
| if (!fs.existsSync(dirname)) { | |
| fs.mkdirSync(dirname, { recursive: true }) | |
| } | |
| if (fs.existsSync(filePath)) { | |
| return | |
| return null | |
| } | |
| const filePathWithExtension = addExtensionToFilenameIfNoExtensionPresent(filePath, markdownExtensions, '.md') | |
| fs.writeFileSync(filePathWithExtension, content, charset) | |
| return { | |
| name: path.basename(filePathWithExtension), | |
| path: filePathWithExtension, | |
| relativePath: path.relative(dirname, filePathWithExtension), | |
| dateModified: new Date(), | |
| dateCreated: new Date(), | |
| } | |
| } | |
| export function createFileRecursive(filePath: string, content: string, charset?: BufferEncoding): FileInfo | null { | |
| const dirname = path.dirname(filePath) | |
| if (!fs.existsSync(dirname)) { | |
| fs.mkdirSync(dirname, { recursive: true }) | |
| } | |
| if (fs.existsSync(filePath)) { | |
| return null | |
| } | |
| const filePathWithExtension = addExtensionToFilenameIfNoExtensionPresent(filePath, markdownExtensions, '.md') | |
| fs.writeFileSync(filePathWithExtension, content, charset) | |
| const stats = fs.statSync(filePathWithExtension) | |
| return { | |
| name: path.basename(filePathWithExtension), | |
| path: filePathWithExtension, | |
| relativePath: path.relative(dirname, filePathWithExtension), | |
| dateModified: stats.mtime, | |
| dateCreated: stats.birthtime, | |
| } | |
| } |
| ipcMain.handle('get-file-info', (event, absolutePath: string, parentRelativePath: string): FileInfo => { | ||
| const fileInfo = fs.statSync(absolutePath) | ||
| return { | ||
| name: path.basename(absolutePath), | ||
| path: absolutePath, | ||
| relativePath: parentRelativePath, | ||
| dateModified: fileInfo.mtime, | ||
| dateCreated: fileInfo.birthtime, // Add the birthtime property here | ||
| } | ||
| }) |
There was a problem hiding this comment.
logic: get-file-info doesn't handle errors from fs.statSync which could crash the app. Should use try/catch
| ipcMain.handle('get-file-info', (event, absolutePath: string, parentRelativePath: string): FileInfo => { | |
| const fileInfo = fs.statSync(absolutePath) | |
| return { | |
| name: path.basename(absolutePath), | |
| path: absolutePath, | |
| relativePath: parentRelativePath, | |
| dateModified: fileInfo.mtime, | |
| dateCreated: fileInfo.birthtime, // Add the birthtime property here | |
| } | |
| }) | |
| ipcMain.handle('get-file-info', (event, absolutePath: string, parentRelativePath: string): FileInfo => { | |
| try { | |
| const fileInfo = fs.statSync(absolutePath) | |
| return { | |
| name: path.basename(absolutePath), | |
| path: absolutePath, | |
| relativePath: parentRelativePath, | |
| dateModified: fileInfo.mtime, | |
| dateCreated: fileInfo.birthtime, | |
| } | |
| } catch (error) { | |
| throw new Error(`Failed to get file info: ${error}`) | |
| } | |
| }) |
| export function findFileRecursive(dir: string, filename: string): string | null { | ||
| const files = fs.readdirSync(dir) | ||
| const basename = path.parse(filename).name | ||
|
|
||
| for (const file of files) { | ||
| const fullPath = path.resolve(dir, file) | ||
| const stat = fs.statSync(fullPath) | ||
|
|
||
| if (stat.isDirectory()) { | ||
| const result = findFileRecursive(fullPath, filename) | ||
| if (result) return result | ||
| } else { | ||
| // Check if files match | ||
| const fileName = path.parse(file).name | ||
| if (fileName === basename) { | ||
| return fullPath | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
logic: The recursive file search doesn't handle file extensions, which could lead to incorrect matches when multiple files have the same basename but different extensions.
| const hydrateIndex = async () => { | ||
| const files = await window.fileSystem.getFilesTreeForWindow() | ||
| const flat = flattenFileInfoTree(files).map((f: FileInfo) => ({ | ||
| ...f, | ||
| })) | ||
| useFileSearchIndex.getState().hydrate(flat) | ||
| } | ||
| hydrateIndex() | ||
| }, []) |
There was a problem hiding this comment.
logic: No error handling for file tree fetching and hydration. Could crash if filesystem access fails
| const hydrateIndex = async () => { | |
| const files = await window.fileSystem.getFilesTreeForWindow() | |
| const flat = flattenFileInfoTree(files).map((f: FileInfo) => ({ | |
| ...f, | |
| })) | |
| useFileSearchIndex.getState().hydrate(flat) | |
| } | |
| hydrateIndex() | |
| }, []) | |
| const hydrateIndex = async () => { | |
| try { | |
| const files = await window.fileSystem.getFilesTreeForWindow() | |
| const flat = flattenFileInfoTree(files).map((f: FileInfo) => ({ | |
| ...f, | |
| })) | |
| useFileSearchIndex.getState().hydrate(flat) | |
| } catch (error) { | |
| toast.error('Failed to load file index: ' + error.message) | |
| } | |
| } | |
| hydrateIndex() | |
| }, []) |
| if (this.options.openOnClick) { | ||
| plugins.push( | ||
| clickHandler({ | ||
| openFile: (this.options as any).openFile, |
There was a problem hiding this comment.
style: Unsafe type assertion with (this.options as any).openFile. Consider adding proper type definition.
| await window.fileSystem.deleteFile(file.path) | ||
| set((s) => { | ||
| const newMap = new Map(s.index) | ||
| newMap.delete(fileName) | ||
| return { index: newMap } | ||
| }) |
There was a problem hiding this comment.
style: state update should be wrapped in try/catch in case deleteFile fails
| await window.fileSystem.deleteFile(file.path) | |
| set((s) => { | |
| const newMap = new Map(s.index) | |
| newMap.delete(fileName) | |
| return { index: newMap } | |
| }) | |
| try { | |
| await window.fileSystem.deleteFile(file.path) | |
| set((s) => { | |
| const newMap = new Map(s.index) | |
| newMap.delete(fileName) | |
| return { index: newMap } | |
| }) | |
| } catch (error) { | |
| console.error('Failed to delete file:', error) | |
| } |
| markStale: (filePath: string) => { | ||
| set((state) => { | ||
| const entry = state.semanticCache[filePath] | ||
| if (!entry) return {} |
There was a problem hiding this comment.
logic: Returning empty object here will cause no state update, but the function signature implies state will be updated. Should return the original state instead.
| if (!entry) return {} | |
| if (!entry) return state |
| setFetching: (filePath: string, isFetching: boolean) => { | ||
| set((state) => { | ||
| const entry = state.semanticCache[filePath] | ||
| if (!entry) return {} |
There was a problem hiding this comment.
logic: Same issue as markStale - returning empty object when no entry exists could cause state inconsistencies
| if (!entry) return {} | |
| if (!entry) return state |
| shouldRefetch: (filePath: string, thresholdMs = 30000) => { | ||
| const entry = get().semanticCache[filePath] | ||
| if (!entry) return true | ||
| if (entry.isStale) return true | ||
| const age = Date.now() - entry.lastFetched | ||
| return age > thresholdMs | ||
| }, |
There was a problem hiding this comment.
style: Should check isFetching state to prevent multiple concurrent fetches of the same data
| export const HYPERMEDIA_SCHEME = 'hm' | ||
|
|
||
| export function isHypermediaScheme(url?: string) { | ||
| return !!url?.startsWith(`${HYPERMEDIA_SCHEME}://`) | ||
| return !!url?.startsWith(`${HYPERMEDIA_SCHEME}://`) || !!url?.startsWith(`reor://`) |
There was a problem hiding this comment.
style: Define 'reor://' as a constant like HYPERMEDIA_SCHEME to avoid magic strings and make updates easier to maintain
| export const HYPERMEDIA_SCHEME = 'hm' | |
| export function isHypermediaScheme(url?: string) { | |
| return !!url?.startsWith(`${HYPERMEDIA_SCHEME}://`) | |
| return !!url?.startsWith(`${HYPERMEDIA_SCHEME}://`) || !!url?.startsWith(`reor://`) | |
| export const HYPERMEDIA_SCHEME = 'hm' | |
| export const REOR_SCHEME = 'reor' | |
| export function isHypermediaScheme(url?: string) { | |
| return !!url?.startsWith(`${HYPERMEDIA_SCHEME}://`) || !!url?.startsWith(`${REOR_SCHEME}://`) |
There was a problem hiding this comment.
When typing in [[, there is a black dot/white dot, what is its functionality?
There was a problem hiding this comment.
[[ displays all similar files and then allows you to select one. After that, it creates an inline link towards that file. If there is no files being displayed, that means there is no similar file.
There was a problem hiding this comment.
here I am in the note "Untitled", but it also suggests itself as a similar file. Also, how does it define similar files because what's the relevance between "Untitled"(an empty note) and the welcoming note?
Added