Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe WarehouseS3.vue component adds dynamic field labeling with required indicators, expands validation state management through new computed properties, introduces credential-type-dependent auto-configuration of storage profile flavor, and refines credential/profile emission logic to handle AWS system identity and Cloudflare R2 variations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/WarehouseS3.vue (4)
582-611: Cloudflare R2 credentials are not emitted (sends empty access-key structure).emitNewCredentials handles only access-key and aws-system-identity. Selecting cloudflare-r2 enables Update Credentials but emits the wrong payload. Add an explicit R2 branch.
Apply this diff:
@@ if (credentialType === 'access-key') { @@ - } else if (credentialType === 'aws-system-identity') { + } else if (credentialType === 'aws-system-identity') { @@ - } + } else if (credentialType === 'cloudflare-r2') { + credentials = { + type: 's3' as const, + 'credential-type': 'cloudflare-r2', + 'access-key-id': warehouseObjectData['storage-credential']['access-key-id'], + 'secret-access-key': warehouseObjectData['storage-credential']['secret-access-key'], + token: warehouseObjectData['storage-credential'].token, + 'account-id': warehouseObjectData['storage-credential']['account-id'], + } as unknown as StorageCredential; + }
613-646: emitNewProfile also misses Cloudflare R2 credentials.Same gap as emitNewCredentials; profile updates will carry a wrong credential object.
Apply this diff:
@@ if (credentialType === 'access-key') { @@ - } else if (credentialType === 'aws-system-identity') { + } else if (credentialType === 'aws-system-identity') { @@ - } + } else if (credentialType === 'cloudflare-r2') { + credentials = { + type: 's3' as const, + 'credential-type': 'cloudflare-r2', + 'access-key-id': warehouseObjectData['storage-credential']['access-key-id'], + 'secret-access-key': warehouseObjectData['storage-credential']['secret-access-key'], + token: warehouseObjectData['storage-credential'].token, + 'account-id': warehouseObjectData['storage-credential']['account-id'], + } as unknown as StorageCredential; + }
81-91: Disabled gating misses Cloudflare R2 required fields and flavor selection.
- Update Credentials: allows click on R2 without required fields present.
- Submit/Update Profile: doesn’t block when R2 fields missing or flavor unset.
Apply this diff:
@@ - :disabled=" - warehouseObjectData['storage-credential']['credential-type'] === 'access-key' && - (!warehouseObjectData['storage-credential']['aws-access-key-id'] || - !warehouseObjectData['storage-credential']['aws-secret-access-key']) - " + :disabled=" + (warehouseObjectData['storage-credential']['credential-type'] === 'access-key' && + (!warehouseObjectData['storage-credential']['aws-access-key-id'] || + !warehouseObjectData['storage-credential']['aws-secret-access-key'])) || + (warehouseObjectData['storage-credential']['credential-type'] === 'cloudflare-r2' && + (!warehouseObjectData['storage-credential']['access-key-id'] || + !warehouseObjectData['storage-credential']['secret-access-key'] || + !warehouseObjectData['storage-credential'].token || + !warehouseObjectData['storage-credential']['account-id'])) + " @@ - !warehouseObjectData['storage-profile'].bucket || + !warehouseObjectData['storage-profile'].bucket || + !warehouseObjectData['storage-profile'].flavor || @@ - !warehouseObjectData['storage-profile'].bucket || + !warehouseObjectData['storage-profile'].bucket || + !warehouseObjectData['storage-profile'].flavor ||Also applies to: 309-323, 330-345
396-416: Template references fields not present in S3Credential; TS will fail. Add a local R2 type to the union.The template binds token, access-key-id, secret-access-key, account-id under storage-credential. With S3Credential typing, vue-tsc will complain. Add a CloudflareR2Credential type and widen the union for warehouseObjectData.
Apply this diff:
@@ -import { +import { S3Credential, S3Profile, StorageCredential, StorageProfile, } from '@/gen/management/types.gen'; @@ +type CloudflareR2Credential = { + type: 's3'; + 'credential-type': 'cloudflare-r2'; + 'access-key-id': string; + 'secret-access-key': string; + token: string; + 'account-id': string; +}; @@ - 'storage-credential': S3Credential & { type: string }; + 'storage-credential': (S3Credential | CloudflareR2Credential) & { type: 's3' };If vue-tsc still cannot narrow within v-if, consider a short-term workaround by typing storage-credential as Record<string, unknown> and adding TODO to upstream types. I can help wire a stricter discriminated union in types.gen.
Also applies to: 39-46, 47-56, 58-67, 69-77, 540-568
🧹 Nitpick comments (6)
src/components/WarehouseS3.vue (6)
180-186: Bucket should reject “/”.Add the noSlash rule to bucket input.
Apply this diff:
- :rules="[rules.required]" + :rules="[rules.required, rules.noSlash]"
112-122: Flavor label/placeholder: minor polish.Label shows “S3 Flavor *” even when auto-set/disabled. Optional: use getFieldLabel('S3 Flavor', true) for consistency with other fields.
374-377: Trailing space in “Access Key ”.Cosmetic but visible in the dropdown.
Apply this diff:
- { text: 'Access Key ', value: 'access-key' }, + { text: 'Access Key', value: 'access-key' },
203-211: External ID placeholder looks like an ARN.External ID is a free-form string, not an ARN. Suggest a neutral example.
Apply this diff:
- placeholder="arn:aws:iam::123456789012..." + placeholder="example-external-id-123"
141-144: Guard STS validity seconds.Optional: add min/max to prevent negative or extreme values (e.g., 300–43200).
Apply this diff:
- type="number" + type="number" + min="300" + max="43200"
512-526: Redundant checks in access key validators.areAccessKeysRequired already implies credential-type === 'access-key'. You can drop the duplicate type comparison to simplify.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes