Google Drive integration - #149
Closed
alexsh1410 wants to merge 28 commits into
Closed
Conversation
works only when you have google drive for desktop
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughPhase 1 Google Drive for Desktop integration is fully implemented: a ChangesGoogle Drive for Desktop Integration (Phase 1)
Sequence Diagram(s)sequenceDiagram
participant User
participant GoogleDriveSection as GoogleDriveSection (Settings)
participant StatusAPI as /api/google-drive/status
participant BrowseAPI as /api/google-drive/browse
participant MountsAPI as /api/google-drive/mounts
participant DB as google_drive_mounts (SQLite)
participant FS as Local Filesystem
participant Sidebar as GoogleDriveTreeSection
User->>GoogleDriveSection: Open integrations hub
GoogleDriveSection->>StatusAPI: GET (no-store)
StatusAPI->>FS: detectDriveDesktop()
StatusAPI->>DB: SELECT mounts
StatusAPI-->>GoogleDriveSection: { detected, mountPath, mounts }
User->>GoogleDriveSection: Click "Add folder"
GoogleDriveSection->>BrowseAPI: GET ?path=mountRoot
BrowseAPI->>FS: realpath + listSubdirectories
BrowseAPI-->>GoogleDriveSection: { dirs, resolvedPath }
User->>GoogleDriveSection: Select folder → confirm
GoogleDriveSection->>MountsAPI: POST { absPath, folderName }
MountsAPI->>DB: INSERT google_drive_mounts
MountsAPI-->>GoogleDriveSection: 201 { id, absPath }
GoogleDriveSection->>Sidebar: dispatch cabinet:gdrive-mounts-changed
Sidebar->>Sidebar: Fetch /api/google-drive/tree
Sidebar-->>User: Render Drive folder in sidebar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
src/lib/google-drive/tree-builder.ts (1)
112-128: 💤 Low valueConsider hoisting
typeMapoutside the loop.The
typeMapobject is recreated on every iteration of the file loop. Moving it to module scope (alongsideGOOGLE_NATIVE) would avoid repeated allocations during large directory traversals.♻️ Suggested refactor
+const FILE_TYPE_MAP: Record<string, TreeNode["type"]> = { + ".pdf": "pdf", + ".md": "file", + ".txt": "file", + ".png": "image", + ".jpg": "image", + ".jpeg": "image", + ".gif": "image", + ".webp": "image", + ".svg": "image", + ".docx": "docx", + ".xlsx": "xlsx", + ".xlsm": "xlsx", + ".pptx": "pptx", + ".csv": "csv", + ".ipynb": "notebook", +}; + async function buildDriveNodes( dirPath: string, visited = new Set<string>() ): Promise<TreeNode[]> { // ... earlier code ... - // Classify known file types — all use the same nodePath (abs path with prefix) - const typeMap: Record<string, TreeNode["type"]> = { - ".pdf": "pdf", - // ... etc - }; - - const fileType = typeMap[ext]; + const fileType = FILE_TYPE_MAP[ext];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/google-drive/tree-builder.ts` around lines 112 - 128, The typeMap object is being recreated on every iteration of the file loop, causing unnecessary memory allocations. Move the typeMap constant definition outside the loop to module scope (similar to how GOOGLE_NATIVE is defined), so it is created only once when the module loads. This constant definition should appear before or alongside other module-level constants to maintain consistent code organization.src/app/api/assets/[...path]/route.ts (1)
92-131: 💤 Low valueConsider extracting Range handling into a shared helper.
The Range header parsing and partial content response logic is duplicated between the Drive branch (lines 92-131) and the existing branch (lines 166-205). Extracting this into a reusable helper would reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/assets/`[...path]/route.ts around lines 92 - 131, The Range header parsing and partial content response handling is duplicated between the Drive branch and the existing branch. Extract the range parsing logic (starting with the regex `/^bytes=(\d*)-(\d*)$/` match), the validation checks, the file reading with Buffer.alloc, and the 206 NextResponse creation into a reusable helper function. This helper should accept the request headers, file handle, total file size, and content type as parameters, then return the appropriate NextResponse or null if the range is invalid. Replace both duplicated blocks with calls to this new helper function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cabinet/Gmail.md`:
- Around line 136-145: The imap_password column comment claims the value is
"encrypted App Password" but provides no details on the encryption mechanism,
key management strategy, or storage location for encryption keys. Either add
specific documentation describing the encryption algorithm, key derivation
method, and where keys are stored and rotated, or remove the encryption
guarantee from the comment and replace it with a note that the App Password
storage is unencrypted until encryption is implemented. Ensure the schema
documentation accurately reflects the actual implementation status.
In `@cabinet/GoogleAuth.md`:
- Around line 132-148: The `google_credentials` table schema is designed
exclusively for OAuth tokens with fields like access_token, refresh_token, and
token_expiry, but the documentation mentions service accounts as a supported
authentication option. Service accounts use JSON keys which do not fit this
OAuth-centric schema. You need to either: (1) define a separate table or schema
extension to store service account JSON keys with appropriate metadata fields,
OR (2) update the Drive and Gmail feature descriptions to remove service account
support and keep only the OAuth option. Choose one approach and update the
GoogleAuth.md documentation to ensure the persistence layer design aligns with
the documented authentication options.
In `@cabinet/GoogleDrive.md`:
- Around line 78-114: The Tree integration section currently describes Google
Drive mounts appearing as a separate root section with a collapsible ☁ header
below the local documents, but this conflicts with the actual implementation
which integrates mounts inline within the main tree without a dedicated Drive
section header. Update the Tree integration section to remove references to the
separate ☁ Google Drive root section, the collapsible section header, and the
dedicated section styling, and instead describe how mounts are integrated inline
within the existing file tree alongside local documents while still retaining
the source annotation for styling purposes.
- Around line 68-76: The DDL schema for the google_drive_mounts table in the
documentation is incomplete and will cause schema drift if developers copy it.
Update the CREATE TABLE statement to include the UNIQUE constraint on the
abs_path column and add all DEFAULT values that are specified in the actual
database migration. Ensure the documented schema exactly mirrors what the real
migration creates so that implementers copying this verbatim will have a
matching table structure.
- Around line 188-194: In the GoogleDrive.md file, update all references to the
Google Drive path scheme in the Search indexing, Content resolution, and Context
injection sections from the URL-style `gdrive://<mountId>/<fileId>` format to
match the documented prefix in `src/lib/google-drive/paths.ts`, which defines it
as `gdrive:` without the `://`. Replace all instances of `gdrive://` with
`gdrive:` to ensure consistency across the documentation and prevent
decode/route handling failures.
In `@src/app/api/google-drive/browse/route.ts`:
- Around line 21-42: The code has a path disclosure vulnerability where the
fs.realpath() call on the attacker-controlled rawPath parameter is attempted
before checking if the path is contained within the mount, and different HTTP
status codes (404 vs 403) leak whether host filesystem paths exist. To fix this,
constrain path resolution to only paths that are mount-root-relative before
resolving them. First, normalize and validate the requestedPath as relative to
the realMountPath, then only resolve paths that pass the containment check.
Return a uniform 404 error response for both cases where the path doesn't exist
or is outside the mount boundary, eliminating the information leak that
distinguishes between non-existent paths and out-of-bounds access attempts.
In `@src/app/api/google-drive/mounts/route.ts`:
- Around line 27-43: The validation logic in the try-catch block starting at the
fs.stat call only checks if the provided path exists and is a directory, but
does not verify that the path is actually within the detected Google Drive
Desktop mount root. Add an additional validation check after the isDirectory
check to ensure the absPath is contained within the detected Drive root
directory (you will need to obtain the Drive root path from your configuration
or detection logic). This should occur before the database insertion in the
db.prepare call to prevent arbitrary host directories from being mounted and
exposed through Drive APIs.
In `@src/components/layout/viewer-breadcrumb.tsx`:
- Around line 41-44: The fallback label extraction in the ViewerBreadcrumb
component uses split("/").pop() which only handles forward slashes and will
return the entire Windows path (like C:\...) instead of just the filename when
driveNode is missing. Replace this path splitting logic with a platform-agnostic
basename extraction method (such as importing and using path.basename() from
Node.js) to properly handle both Unix and Windows path separators in the
driveAbsPath fallback expression.
In `@src/components/settings/google-drive-section.tsx`:
- Around line 277-279: The description text within the p element with className
"text-[12px] text-muted-foreground" in the google-drive-section component
incorrectly states that mounted Google Drive files appear in "a separate Google
Drive section," but this PR changes the behavior to display them inline with the
main sidebar tree. Update this description text to accurately reflect that files
now appear inline with the main tree instead of in a separate dedicated section.
- Around line 102-103: The current implementation uses split("/") to extract
folder names from currentPath, which fails on Windows-style paths containing
backslashes. Replace the split("/") approach with a path-separator-agnostic
method that handles both forward slashes and backslashes. Use the
path.basename() utility function from Node.js or modify the split to handle both
"/" and "\" separators. This same fix needs to be applied at the location where
folderName is created (around line 102) and also at lines 116-118 where
breadcrumb text or selected mount names are extracted from paths.
- Around line 237-259: The removeMount function lacks exception handling for
fetch failures such as network errors or timeouts. Currently, if the fetch call
in removeMount throws an exception, it will result in an unhandled promise
rejection with no user feedback. Add a catch block to the existing try-finally
structure that catches fetch exceptions and dispatches a cabinet:toast error
event to inform the user that the operation failed, using a message like "Failed
to remove {name}" or similar appropriate error messaging.
In `@src/components/sidebar/google-drive-tree.tsx`:
- Around line 184-221: The fetchDriveTree callback has sections.length in its
dependency array which creates an infinite re-render cycle when setSections is
called. Remove sections.length from the useCallback dependency array and change
it to an empty array. Move the cache hydration logic (the try-catch block that
reads from localStorage and calls JSON.parse) from inside the fetchDriveTree
function to a useState initializer function for setSections, so the initial
state is populated from cache on component mount rather than on each render.
---
Nitpick comments:
In `@src/app/api/assets/`[...path]/route.ts:
- Around line 92-131: The Range header parsing and partial content response
handling is duplicated between the Drive branch and the existing branch. Extract
the range parsing logic (starting with the regex `/^bytes=(\d*)-(\d*)$/` match),
the validation checks, the file reading with Buffer.alloc, and the 206
NextResponse creation into a reusable helper function. This helper should accept
the request headers, file handle, total file size, and content type as
parameters, then return the appropriate NextResponse or null if the range is
invalid. Replace both duplicated blocks with calls to this new helper function.
In `@src/lib/google-drive/tree-builder.ts`:
- Around line 112-128: The typeMap object is being recreated on every iteration
of the file loop, causing unnecessary memory allocations. Move the typeMap
constant definition outside the loop to module scope (similar to how
GOOGLE_NATIVE is defined), so it is created only once when the module loads.
This constant definition should appear before or alongside other module-level
constants to maintain consistent code organization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 114ab999-8716-4b91-8eda-dacf7aa5a677
📒 Files selected for processing (26)
cabinet/Gmail.mdcabinet/GoogleAuth.mdcabinet/GoogleDrive.mdserver/migrations/002_google_drive_mounts.sqlsrc/app/api/assets/[...path]/route.tssrc/app/api/google-drive/browse/route.tssrc/app/api/google-drive/mounts/[id]/route.tssrc/app/api/google-drive/mounts/route.tssrc/app/api/google-drive/reveal/route.tssrc/app/api/google-drive/serve/route.tssrc/app/api/google-drive/status/route.tssrc/app/api/google-drive/tree/route.tssrc/components/integrations/hub/integration-detail-page.tsxsrc/components/integrations/hub/integrations-hub-page.tsxsrc/components/layout/app-shell.tsxsrc/components/layout/viewer-breadcrumb.tsxsrc/components/settings/google-drive-section.tsxsrc/components/sidebar/google-drive-tree.tsxsrc/components/sidebar/tree-node.tsxsrc/components/sidebar/tree-view.tsxsrc/lib/google-drive/detect-desktop.tssrc/lib/google-drive/paths.tssrc/lib/google-drive/tree-builder.tssrc/lib/integrations/preview-catalog.tssrc/stores/tree-store.tssrc/types/index.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cabinet/GoogleDrive.md (1)
198-200:⚠️ Potential issue | 🟡 MinorUpdate Phase 2 documentation to use canonical
gdrive:format without//.The implementation in
src/lib/google-drive/paths.tsdefines the canonical path format asgdrive:(without//), with an explicit comment stating "No :// to avoid Next.js routing issues." Phase 2 documentation incorrectly usesgdrive://format in three places (lines 198, 200, 250). Phase 1 documentation correctly references thegdrive:format.Update lines 198 and 200 to replace
gdrive://<mountId>/<fileId>andgdrive://with the canonicalgdrive:format. Also review line 250 for the same correction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cabinet/GoogleDrive.md` around lines 198 - 200, The Phase 2 documentation in cabinet/GoogleDrive.md uses the incorrect `gdrive://` format in multiple places (lines 198, 200, and 250), but the canonical implementation in src/lib/google-drive/paths.ts defines the path format as `gdrive:` without the `//` to avoid Next.js routing issues. Replace all instances of `gdrive://<mountId>/<fileId>` with `gdrive:<mountId>/<fileId>` and replace standalone `gdrive://` references with `gdrive:` to align the documentation with the actual canonical format used in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cabinet/GoogleDrive.md`:
- Around line 198-200: The Phase 2 documentation in cabinet/GoogleDrive.md uses
the incorrect `gdrive://` format in multiple places (lines 198, 200, and 250),
but the canonical implementation in src/lib/google-drive/paths.ts defines the
path format as `gdrive:` without the `//` to avoid Next.js routing issues.
Replace all instances of `gdrive://<mountId>/<fileId>` with
`gdrive:<mountId>/<fileId>` and replace standalone `gdrive://` references with
`gdrive:` to align the documentation with the actual canonical format used in
the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb51afdc-a734-4722-bd98-87a40c7fde91
📒 Files selected for processing (3)
cabinet/GoogleDrive.mdsrc/components/layout/viewer-breadcrumb.tsxsrc/components/settings/google-drive-section.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/layout/viewer-breadcrumb.tsx
- src/components/settings/google-drive-section.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/api/google-drive/mounts/route.ts (1)
61-66: 💤 Low valueConsider storing
realAbsPathinstead ofabsPathfor consistency.The containment check validates
realAbsPath, but the INSERT stores the originalabsPath. IfabsPathis a symlink that later changes to point outside the Drive mount, the stored record would reference a now-invalid location. Runtime realpath checks in the browse/tree APIs mitigate actual exposure, but storing the resolved path would be more consistent with the validation.db.prepare( "INSERT INTO google_drive_mounts (id, abs_path, folder_name, enabled, added_at) VALUES (?, ?, ?, 1, datetime('now'))" - ).run(id, absPath, folderName); + ).run(id, realAbsPath, folderName);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/google-drive/mounts/route.ts` around lines 61 - 66, The INSERT statement in the google_drive_mounts route is storing absPath instead of realAbsPath in the abs_path column, but the validation logic checks realAbsPath. To maintain consistency and prevent symlink vulnerabilities, replace the absPath parameter in the db.prepare() call with realAbsPath so that the resolved real path (not the original symlink) is persisted in the database.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/api/google-drive/mounts/route.ts`:
- Around line 61-66: The INSERT statement in the google_drive_mounts route is
storing absPath instead of realAbsPath in the abs_path column, but the
validation logic checks realAbsPath. To maintain consistency and prevent symlink
vulnerabilities, replace the absPath parameter in the db.prepare() call with
realAbsPath so that the resolved real path (not the original symlink) is
persisted in the database.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68321b8a-0e05-492b-9c05-5bb19d088084
📒 Files selected for processing (4)
src/app/api/google-drive/browse/route.tssrc/app/api/google-drive/mounts/route.tssrc/components/settings/google-drive-section.tsxsrc/components/sidebar/google-drive-tree.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/settings/google-drive-section.tsx
- src/components/sidebar/google-drive-tree.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Google Drive: dedicated integration page + inline sidebar mounts
Integration page
The Google Drive card used to open the Google Workspace OAuth suite — which isn't implemented — instead of the working Drive-for-Desktop flow.
Launch gate (preview-catalog.ts): Drive card is now live/clickable instead of greyed-out "Soon".
Routing (integrations-hub-page.tsx): the card opens its own google-drive page instead of the Workspace suite.
Detail page (integration-detail-page.tsx): renders the real Drive-for-Desktop flow (GoogleDriveSection) — detect install, add/remove mounts — with the "OAuth coming soon" note kept.
Left sidebar
Mounts previously sat in a separate section pinned to the bottom of the data tab and didn't refresh on mount.
Inline placement (tree-view.tsx): mounts now render inside SidebarSearch, right after the cabinet files, so they flow inline in both collapsed and expanded states (previously the flex-1 slack pushed them to the bottom). A stopPropagation guard keeps Drive right-clicks from opening the cabinet menu.
Inline styling (google-drive-tree.tsx): dropped the "Google Drive" divider/header; each mount is now a plain collapsible folder row (cloud glyph), expand to see files, click to open in the viewer.
Live refresh (google-drive-tree.tsx + google-drive-section.tsx): mount/unmount fires cabinet:gdrive-mounts-changed, so the sidebar updates immediately instead of waiting out the 60s cache.
Summary by CodeRabbit
Release Notes
Documentation
New Features
Database