refactor gdrive #327
refactor gdrive #327
Conversation
WalkthroughThis pull request introduces a new Google Drive integration for the project. A dedicated JSON configuration file provides metadata for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Application/User
participant Connect as "Connect Function (index.ts)"
participant Gateway as "GDriveGateway"
participant API as "Google Drive API"
Client->>Connect: call connect(database, auth, url, name)
Connect->>Gateway: register protocol & instantiate gateway
Gateway->>API: start(uri) for initialization
Client->>Gateway: perform operations (put/get/delete)
Gateway->>API: forward CRUD requests
API-->>Gateway: return operation results
Gateway-->>Client: respond with data/status
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/drive/index.ts (3)
24-25: Consider improving the default authentication token.Using "yourtoken" as the default authentication token parameter value might lead to accidental use in production environments if developers forget to provide a real token. Consider using an empty string as the default value to encourage proper token provision.
- auth = "yourtoken", + auth = "",
39-44: Consider adding error handling for connection failures.The connection establishment code doesn't have any error handling for potential failures when connecting to Google Drive. Consider adding a try/catch block to handle connection failures gracefully.
return connectionCache.get(urlObj.toString()).once(() => { makeKeyBagUrlExtractable(sthis); - const connection = connectionFactory(sthis, urlObj); - connection.connect_X(blockstore); - return connection; + try { + const connection = connectionFactory(sthis, urlObj); + connection.connect_X(blockstore); + return connection; + } catch (error) { + console.error("Failed to connect to Google Drive:", error); + throw new Error(`Failed to connect to Google Drive: ${error.message}`); + } });
21-21: Consider adding API documentation for theconnectfunction.Adding JSDoc comments for the exported connect function would improve developer experience by providing documentation directly in the IDE.
+/** + * Connect a Fireproof database to Google Drive + * @param db - The Fireproof database to connect + * @param auth - Google Drive authentication token + * @param url - Google Drive API URL + * @param remoteDbName - Name of the remote database + * @returns A connection to Google Drive + */ export const connect: ConnectFunction = (src/drive/drive-gateway.test.ts (1)
7-19: Consider adding more test cases to improve coverage.While the basic smoke test is good, consider adding more specific test cases such as:
- Testing with invalid authentication tokens
- Testing with different database names
- Testing error conditions
This would ensure the Google Drive gateway is thoroughly tested.
src/drive/drive-gateway.ts (3)
25-26: Implement or remove thedestroymethod.
Currently, this method throws an error and remains unimplemented. Either provide a concrete implementation or remove it if destruction logic is not needed.Would you like me to propose a possible implementation or open an issue to track this TODO?
179-207: Validate exponential backoff logic insubscribe.
This polling-based subscription will keep doubling interval until it hitsmaxIntervalon unchanged data. This is fine for limited use. If you expect high concurrency or real-time updates, consider using webhooks or a streaming solution.
300-306: Remove or clarify commented-out code.
These lines appear unused and may confuse future maintainers. If they are no longer required, remove them; otherwise, include a reason for keeping them commented.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package-drive.json(1 hunks)package.json(2 hunks)src/drive/drive-gateway.test.ts(1 hunks)src/drive/drive-gateway.ts(1 hunks)src/drive/index.ts(1 hunks)tsup.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package-drive.json
🔇 Additional comments (7)
src/drive/index.ts (1)
29-31: LGTM! Good error handling for required database name.The check for database name presence is important for proper functioning of the Google Drive connection.
src/drive/drive-gateway.test.ts (1)
18-18: LGTM! Good cleanup with unregistering the protocol.Properly unregistering the protocol after the test is complete is a good practice to avoid side effects in other tests.
package.json (1)
114-114: LGTM! Good addition of browser testing dependencies.The added dependencies (@vitest/browser and webdriverio) are appropriate for testing browser interactions with Google Drive.
Also verify that there's a corresponding update to package.json for the @fireproof/drive package:
#!/bin/bash # Check if there's a package-drive.json file for the new @fireproof/drive package ls -la package-drive.json || echo "package-drive.json not found"Also applies to: 135-135
tsup.config.ts (1)
154-174: LGTM! Bundle configuration follows existing patterns.The bundle configuration for @fireproof/drive follows the same pattern as other library bundles in the file, making it consistent with the project's structure.
src/drive/drive-gateway.ts (3)
104-109: Check behavior for 204 or empty response bodies.
Callingawait response.json()on DELETE might fail if the API returns a204 No Contentor an empty body. Consider verifyingresponse.okand handling empty JSON gracefully, for example:const response = await fetch(url + fileId, { method: "DELETE", headers: headers, }); - return await response.json(); + if (response.ok && response.status !== 204) { + return await response.json(); + } + return {};
296-296: Remove sensitive data from log statements.
Line 296 logs the auth token. Obfuscate or do not log tokens by default.
[security]- return this.logger.Error().Any({ auth, fileName }).Err(err).Msg("Fetch Error").ResultError(); + return this.logger.Error().Any({ auth: "<redacted>", fileName }).Err(err).Msg("Fetch Error").ResultError();
308-324: No major concerns with protocol registration logic.
Code successfully registers theGDriveGatewayunder the"gdrive:"protocol. The approach follows best practices by encapsulating default URIs and returning a gateway instance.
| }); | ||
| return await response.json(); | ||
| } catch (err) { | ||
| return this.logger.Error().Url(url).Any("init", auth).Err(err).Msg("Could not delete").ResultError(); |
There was a problem hiding this comment.
Avoid logging auth tokens in error messages.
Line 110 includes Any("init", auth) in the log, which may reveal bearer tokens. Consider obfuscating or removing sensitive information from logs to prevent accidental exposure.
[security]
- return this.logger.Error().Url(url).Any("init", auth).Err(err).Msg("Could not delete").ResultError();
+ return this.logger.Error().Url(url).Any("init", "<redacted>").Err(err).Msg("Could not delete").ResultError();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return this.logger.Error().Url(url).Any("init", auth).Err(err).Msg("Could not delete").ResultError(); | |
| return this.logger.Error().Url(url).Any("init", "<redacted>").Err(err).Msg("Could not delete").ResultError(); |
| if (!index) { | ||
| name += `-${index}`; | ||
| } |
There was a problem hiding this comment.
Likely reversed condition for file indexing.
The code appends -undefined to the filename when no index is given. Instead, you probably want to append the index only if it exists, e.g.:
-if (!index) {
- name += `-${index}`;
+if (index) {
+ name += `-${index}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!index) { | |
| name += `-${index}`; | |
| } | |
| if (index) { | |
| name += `-${index}`; | |
| } |
| let headers; | ||
| const url = BuildURI.from(this.params.driveURL); | ||
| headers = { | ||
| Authorization: `Bearer ${auth}`, | ||
| "Content-Type": "application/json", | ||
| }; | ||
| if (type == "meta") { | ||
| response = await fetch(url + fileId, { | ||
| method: "GET", | ||
| headers, | ||
| }); | ||
| return Result.Ok(new Uint8Array(await response.arrayBuffer())); | ||
| } else { | ||
| headers = { | ||
| Authorization: `Bearer ${auth}`, | ||
| }; | ||
| response = await fetch(url.appendRelative(fileId).setParam("alt", "media").toString(), { | ||
| method: "GET", | ||
| headers, | ||
| }); | ||
| return Result.Ok(new Uint8Array(await response.arrayBuffer())); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return errors if the fetch fails in #get.
There's no check for response.ok before returning Result.Ok(...). A 404 or other non-2xx status could silently fail, returning malformed data. Consider:
if (!response.ok) {
return this.logger.Error().Any({ status: response.status }).Msg("GET failed").ResultError();
}
return Result.Ok(new Uint8Array(await response.arrayBuffer()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let headers; | |
| const url = BuildURI.from(this.params.driveURL); | |
| headers = { | |
| Authorization: `Bearer ${auth}`, | |
| "Content-Type": "application/json", | |
| }; | |
| if (type == "meta") { | |
| response = await fetch(url + fileId, { | |
| method: "GET", | |
| headers, | |
| }); | |
| return Result.Ok(new Uint8Array(await response.arrayBuffer())); | |
| } else { | |
| headers = { | |
| Authorization: `Bearer ${auth}`, | |
| }; | |
| response = await fetch(url.appendRelative(fileId).setParam("alt", "media").toString(), { | |
| method: "GET", | |
| headers, | |
| }); | |
| return Result.Ok(new Uint8Array(await response.arrayBuffer())); | |
| } | |
| let headers; | |
| const url = BuildURI.from(this.params.driveURL); | |
| headers = { | |
| Authorization: `Bearer ${auth}`, | |
| "Content-Type": "application/json", | |
| }; | |
| if (type == "meta") { | |
| response = await fetch(url + fileId, { | |
| method: "GET", | |
| headers, | |
| }); | |
| if (!response.ok) { | |
| return this.logger.Error().Any({ status: response.status }).Msg("GET failed").ResultError(); | |
| } | |
| return Result.Ok(new Uint8Array(await response.arrayBuffer())); | |
| } else { | |
| headers = { | |
| Authorization: `Bearer ${auth}`, | |
| }; | |
| response = await fetch(url.appendRelative(fileId).setParam("alt", "media").toString(), { | |
| method: "GET", | |
| headers, | |
| }); | |
| if (!response.ok) { | |
| return this.logger.Error().Any({ status: response.status }).Msg("GET failed").ResultError(); | |
| } | |
| return Result.Ok(new Uint8Array(await response.arrayBuffer())); | |
| } |
Summary by CodeRabbit