Feature/google cloud storage#316
Conversation
📝 WalkthroughWalkthroughThis pull request integrates Google Cloud Storage as a new storage provider into Portabase. It adds the ChangesGoogle Cloud Storage Provider
Sequence Diagram(s)sequenceDiagram
participant User
participant ChannelForm as Channel Form
participant Validation as Zod Validation
participant Dispatch as Provider Dispatch
participant GCS as GCS Provider
User->>ChannelForm: select google-cloud-storage
User->>ChannelForm: enter projectId, bucketName, clientEmail, privateKey
ChannelForm->>Validation: validate form data
Validation-->>ChannelForm: validation result
User->>ChannelForm: submit or test connection
ChannelForm->>Dispatch: dispatch ping request
Dispatch->>GCS: pingGoogleCloudStorage(config)
GCS->>GCS: initialize client, verify bucket
GCS->>GCS: write/download/delete ping.txt
GCS-->>Dispatch: success or error
Dispatch-->>ChannelForm: connection status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
scripts/test-gcs.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: Converting circular structure to JSON ... [truncated 446 characters] ... c/dist/eslintrc.cjs:3261:25) src/features/channel/storages/google-cloud-storage/index.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: Converting circular structure to JSON ... [truncated 446 characters] ... c/dist/eslintrc.cjs:3261:25) 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: 3
🤖 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
`@src/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.ts`:
- Around line 4-7: The schema currently allows whitespace-only strings and
accepts any non-empty value for clientEmail; update the validators for
projectId, bucketName, and privateKey to use z.string().trim().min(1, "...") so
whitespace-only values are rejected, and change clientEmail to
z.string().trim().min(1, "...").email("Client email must be a valid email") to
enforce a valid email format; modify the fields projectId, bucketName,
clientEmail, and privateKey in the Google Cloud Storage schema accordingly.
In `@src/features/channel/storages/google-cloud-storage/index.ts`:
- Around line 127-131: The current ping uses a fixed key "ping.txt" which can
overwrite or delete existing objects; change the logic in this block (the
bucket.file(...) usage around file.save, file.download, file.delete) to create
and use a unique temporary object name (e.g. include a timestamp/UUID:
`${BASE_DIR}ping-${Date.now()}-${uuid()}.txt`), write/download/delete that
unique key, and wrap the operations in a try/finally so you only call
file.delete() on the specific temp file you created (and catch/log errors) to
avoid touching any pre-existing objects.
- Around line 57-62: The upload wiring currently only listens to the writable
end (uploadStream.pipe(writeStream).on("finish"/"error")) which misses errors
from the readable; replace that pattern in the upload flow using the pipeline
utility (import from 'stream'/'stream/promises') and await
pipeline(uploadStream, writeStream) so both ends propagate errors reliably. Also
change pingGoogleCloudStorage so it does not use the fixed key "ping.txt" under
possibly-empty BASE_DIR; generate a namespaced/unique ping object name (e.g.,
include BASE_DIR and a UUID or timestamp like `${BASE_DIR}/.ping-<unique>.tmp`)
and only delete that exact generated key to avoid clobbering real customer
objects.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e623b294-b1ac-4a69-84e2-34c3c1d73f30
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.gitignorepackage.jsonsrc/db/migrations/0058_tense_tana_nile.sqlsrc/db/migrations/meta/0058_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema/12_storage-channel.tssrc/features/channel/channel-form.schema.tssrc/features/channel/channels-helpers.tsxsrc/features/channel/channels-storage-helper.tsxsrc/features/channel/storages/google-cloud-storage/google-cloud-storage.form.tsxsrc/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.tssrc/features/channel/storages/google-cloud-storage/index.tssrc/features/channel/storages/google-cloud-storage/types.tssrc/features/channel/storages/index.tssrc/features/storages/storages.types.ts
📜 Review details
🔇 Additional comments (14)
src/features/storages/storages.types.ts (1)
7-7: LGTM!src/db/migrations/meta/_journal.json (1)
410-416: LGTM!src/features/channel/storages/google-cloud-storage/types.ts (1)
1-6: LGTM!src/features/channel/storages/google-cloud-storage/index.ts (1)
13-30: LGTM!Also applies to: 70-113, 147-174
src/features/channel/storages/google-cloud-storage/google-cloud-storage.form.tsx (1)
1-86: LGTM!src/features/channel/channels-helpers.tsx (1)
38-40: LGTM!Also applies to: 102-103
src/features/channel/channels-storage-helper.tsx (1)
10-10: LGTM!src/features/channel/storages/index.ts (1)
16-22: LGTM!Also applies to: 53-60
src/db/schema/12_storage-channel.ts (1)
8-8: LGTM!src/db/migrations/0058_tense_tana_nile.sql (1)
1-1: LGTM!src/db/migrations/meta/0058_snapshot.json (1)
2731-2739: LGTM!.gitignore (1)
31-32: LGTM!package.json (1)
23-23: LGTM!src/features/channel/channel-form.schema.ts (1)
12-12: LGTM!Also applies to: 68-71
| await new Promise<void>((resolve, reject) => { | ||
| uploadStream | ||
| .pipe(writeStream) | ||
| .on("finish", resolve) | ||
| .on("error", reject); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current upload stream wiring and absence/presence of pipeline usage.
rg -n -C3 'uploadStream|pipe\(writeStream\)|on\("finish"|on\("error"' src/features/channel/storages/google-cloud-storage/index.ts
rg -n 'pipeline\(' src/features/channel/storages/google-cloud-storage/index.ts
# Expected:
# - First command shows handlers attached through .pipe(writeStream).
# - Second command returns no matches in current code.Repository: Portabase/portabase
Length of output: 957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/features/channel/storages/google-cloud-storage/index.ts"
# Verify ping key usage and where it's used (upload/copy/delete)
rg -n "ping\.txt|ping" "$FILE"
rg -n "delete\(|remove\(|unlink\(|copy\(|rewrite\(|rename\(|bucket\.file\(" "$FILE"
# Inspect the relevant ping/copy/delete/ping method implementations with context
rg -n -C3 "ping\.txt" "$FILE"
rg -n -C5 "async function ping|ping\(" "$FILE" || true
rg -n -C5 "async function copy|copy\(" "$FILE" || true
rg -n -C5 "async function delete|delete\(|remove\(|destroy\(|unlink\(" "$FILE" || trueRepository: Portabase/portabase
Length of output: 2946
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/features/channel/storages/google-cloud-storage/index.ts"
# Check BASE_DIR definition to reason about ping key uniqueness/clobber risk
rg -n "BASE_DIR" "$FILE"
sed -n '1,220p' "$FILE" | nl -ba | sed -n '1,220p' >/tmp/gcs_top.txt || true
# Inspect upload method body for any additional error handlers on uploadStream/source
rg -n "uploadStream|writeStream|createWriteStream|Readable\.from|\.on\(\"error\"|\.once\(\"error\"" "$FILE"
# Specifically look for any handlers attached to uploadStream itself
rg -n "uploadStream.*on\(\"error\"|uploadStream.*once\(\"error\"|fileStream.*on\(\"error\"|fileStream.*once\(\"error\"" "$FILE"
# Check whether pipeline is used elsewhere in the file
rg -n "pipeline\(" "$FILE" || trueRepository: Portabase/portabase
Length of output: 816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/features/channel/storages/google-cloud-storage/index.ts"
# Show full relevant sections with line numbers (cat -n exists even if nl doesn't)
cat -n "$FILE" | sed -n '1,120p'
cat -n "$FILE" | sed -n '120,220p'Repository: Portabase/portabase
Length of output: 6954
Use pipeline for upload stream wiring; avoid fixed ping.txt clobber risk
- Upload: the promise resolves/rejects only from the writable (
.pipe(writeStream).on("finish"/"error")), so errors from the source/readable (uploadStream) aren’t handled reliably—usepipeline(lines 57-62). - Ping:
BASE_DIRis"", andpingGoogleCloudStoragewrites to a fixed keyping.txtand then deletes it, which can overwrite/delete an actual customer object at the same path (lines 24, 127-131).
Proposed fix (upload)
import {Readable} from "node:stream";
+import {pipeline} from "node:stream/promises";
@@
- await new Promise<void>((resolve, reject) => {
- uploadStream
- .pipe(writeStream)
- .on("finish", resolve)
- .on("error", reject);
- });
+ await pipeline(uploadStream, writeStream);📝 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.
| await new Promise<void>((resolve, reject) => { | |
| uploadStream | |
| .pipe(writeStream) | |
| .on("finish", resolve) | |
| .on("error", reject); | |
| }); | |
| await pipeline(uploadStream, writeStream); |
🤖 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/features/channel/storages/google-cloud-storage/index.ts` around lines 57
- 62, The upload wiring currently only listens to the writable end
(uploadStream.pipe(writeStream).on("finish"/"error")) which misses errors from
the readable; replace that pattern in the upload flow using the pipeline utility
(import from 'stream'/'stream/promises') and await pipeline(uploadStream,
writeStream) so both ends propagate errors reliably. Also change
pingGoogleCloudStorage so it does not use the fixed key "ping.txt" under
possibly-empty BASE_DIR; generate a namespaced/unique ping object name (e.g.,
include BASE_DIR and a UUID or timestamp like `${BASE_DIR}/.ping-<unique>.tmp`)
and only delete that exact generated key to avoid clobbering real customer
objects.
| const key = `${BASE_DIR}ping.txt`; | ||
| const file = bucket.file(key); | ||
| await file.save(Buffer.from("ping")); | ||
| await file.download(); | ||
| await file.delete(); |
There was a problem hiding this comment.
Avoid clobbering real bucket objects during ping (Lines 127-131).
Using a fixed ping.txt key can overwrite and then delete an existing object with that name, causing data loss.
Proposed fix
+import {randomUUID} from "node:crypto";
@@
- const key = `${BASE_DIR}ping.txt`;
+ const key = `${BASE_DIR}__portabase_ping_${randomUUID()}.txt`;
const file = bucket.file(key);
- await file.save(Buffer.from("ping"));
- await file.download();
- await file.delete();
+ try {
+ await file.save(Buffer.from("ping"));
+ await file.download();
+ } finally {
+ await file.delete({ignoreNotFound: true});
+ }📝 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.
| const key = `${BASE_DIR}ping.txt`; | |
| const file = bucket.file(key); | |
| await file.save(Buffer.from("ping")); | |
| await file.download(); | |
| await file.delete(); | |
| const key = `${BASE_DIR}__portabase_ping_${randomUUID()}.txt`; | |
| const file = bucket.file(key); | |
| try { | |
| await file.save(Buffer.from("ping")); | |
| await file.download(); | |
| } finally { | |
| await file.delete({ignoreNotFound: true}); | |
| } |
🤖 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/features/channel/storages/google-cloud-storage/index.ts` around lines 127
- 131, The current ping uses a fixed key "ping.txt" which can overwrite or
delete existing objects; change the logic in this block (the bucket.file(...)
usage around file.save, file.download, file.delete) to create and use a unique
temporary object name (e.g. include a timestamp/UUID:
`${BASE_DIR}ping-${Date.now()}-${uuid()}.txt`), write/download/delete that
unique key, and wrap the operations in a try/finally so you only call
file.delete() on the specific temp file you created (and catch/log errors) to
avoid touching any pre-existing objects.
LuuOW
left a comment
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: code implementation verified for system consistency.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@src/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.ts`:
- Around line 3-10: The GoogleCloudStorageChannelConfigSchema in this file has
duplicate syntax: the export declaration appears twice (once on line 3 and again
on line 4) and the closing brace appears twice (once on line 9 and again on line
10). Remove the first duplicate export const
GoogleCloudStorageChannelConfigSchema declaration and remove the duplicate
closing brace at the end so only one complete schema definition remains with a
single closing brace.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27fa740f-638c-47de-bc80-d5447b58bf92
📒 Files selected for processing (1)
src/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.ts
📜 Review details
🧰 Additional context used
🪛 Biome (2.5.0)
src/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.ts
[error] 4-4: expected : but instead found const
(parse)
[error] 4-4: expected : but instead found GoogleCloudStorageChannelConfigSchema
(parse)
[error] 9-9: expected , but instead found ;
(parse)
|
Hi @hayzie-chu, Did you test with real google cloud storage ? |
|
@hayzie-chu , you could use : https://github.com/fsouza/fake-gcs-server in order to test your integration. services:
gcs:
image: fsouza/fake-gcs-server:latest
container_name: fake-gcs
ports:
- "4443:4443"
command:
- "-scheme"
- "http"
- "-port"
- "4443"
- "-public-host"
- "localhost:4443"
- "-external-url"
- "http://localhost:4443"
volumes:
- ./gcs-data:/data
- ./gcs-storage:/storage |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/test-gcs.ts`:
- Around line 3-4: The seed bucket path in the commented commands on lines 3-4
uses gcs-data/test-bucket, but the emulator is configured with -filesystem-root
/storage mapped to ./gcs-storage, causing a path mismatch. Update the commented
mkdir and echo commands to use the correct path that corresponds to the
emulator's configured filesystem-root directory (./gcs-storage/test-bucket),
ensuring the bucket path used in the ping operation on line 40 matches the
seeded bucket location.
- Line 62: The main() function error handler on line 62 only logs the error
using console.error but does not exit the process with a non-zero exit code,
which may cause command runners to treat the failure as a success. Modify the
catch handler for main() to call process.exit(1) after logging the error so that
the script properly signals failure to the shell and any dependent processes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c2e9733-3e55-4227-b2a4-6347e659d659
📒 Files selected for processing (5)
.gitignoredocker-compose.gcs.ymlscripts/test-gcs.tssrc/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.tssrc/features/channel/storages/google-cloud-storage/index.ts
💤 Files with no reviewable changes (1)
- src/features/channel/storages/google-cloud-storage/google-cloud-storage.schema.ts
📜 Review details
🔇 Additional comments (3)
.gitignore (1)
12-13: LGTM!src/features/channel/storages/google-cloud-storage/index.ts (1)
14-18: LGTM!docker-compose.gcs.yml (1)
1-23: LGTM!
| // mkdir -p gcs-data/test-bucket | ||
| // echo "hello" > gcs-data/test-bucket/seed.txt |
There was a problem hiding this comment.
Seed bucket path does not match the emulator filesystem root.
Line [3]-Line [4] seed gcs-data/test-bucket, but the emulator is configured with -filesystem-root /storage (mapped to ./gcs-storage), so Line [40] can report bucket-missing on ping.
Suggested fix
- // mkdir -p gcs-data/test-bucket
- // echo "hello" > gcs-data/test-bucket/seed.txt
+ // mkdir -p gcs-storage/test-bucket
+ // echo "hello" > gcs-storage/test-bucket/seed.txtAlso applies to: 40-40
🤖 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 `@scripts/test-gcs.ts` around lines 3 - 4, The seed bucket path in the
commented commands on lines 3-4 uses gcs-data/test-bucket, but the emulator is
configured with -filesystem-root /storage mapped to ./gcs-storage, causing a
path mismatch. Update the commented mkdir and echo commands to use the correct
path that corresponds to the emulator's configured filesystem-root directory
(./gcs-storage/test-bucket), ensuring the bucket path used in the ping operation
on line 40 matches the seeded bucket location.
| })); | ||
| } | ||
|
|
||
| main().catch(console.error); |
There was a problem hiding this comment.
Set a non-zero exit code when the script fails.
Line [62] only logs the error; command runners may still treat the run as successful.
Suggested fix
-main().catch(console.error);
+main().catch((err) => {
+ console.error(err);
+ process.exitCode = 1;
+});📝 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.
| main().catch(console.error); | |
| main().catch((err) => { | |
| console.error(err); | |
| process.exitCode = 1; | |
| }); |
🤖 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 `@scripts/test-gcs.ts` at line 62, The main() function error handler on line 62
only logs the error using console.error but does not exit the process with a
non-zero exit code, which may cause command runners to treat the failure as a
success. Modify the catch handler for main() to call process.exit(1) after
logging the error so that the script properly signals failure to the shell and
any dependent processes.
|
Hi @RambokDev, I have recently pushed a new commit with the test scripts and how to run it. I have verified that the flow works with the fake-gcs-server. |
Ok, nice. I’ll take a look at that and come back to you if I have any questions. |
3 participants
Closes #266
Validation Requirements screenshots:
Note: the ping/upload path hasn't been tested against a real GCS bucket yet, but the UI is fully functional
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignoreto ignore additional local data folders and.envfiles.