Conversation
Update AppEnv type to include SecretsStoreSecret alongside R2Bucket and Fetcher, allowing workers with [[secrets_store_secrets]] bindings to be fully typed. Add example configuration to template and dev wrangler.toml, and extend test coverage to verify secret bindings are not enumerated as R2 buckets. https://claude.ai/code/session_018pheiK3TL8nFuSnC9HvpPY
|
Review these changes at https://app.gitnotebooks.com/Cybersoulja/R2-Explorer/pull/4 |
Reviewer's GuideAdds Cloudflare Secrets Store binding support by broadening the AppEnv type to include SecretsStoreSecret, updating wrangler.toml templates with commented secrets_store_secrets examples, wiring a mock Secrets Store binding into the test environment, and asserting that secrets are not treated as R2 buckets, with a changeset to release the update. Class diagram for updated AppEnv type with SecretsStoreSecret supportclassDiagram
class AppEnv {
ASSETS : Fetcher
key_string_map : R2Bucket | SecretsStoreSecret | Fetcher | string
}
class R2Bucket {
}
class SecretsStoreSecret {
}
class Fetcher {
}
AppEnv --> R2Bucket : dynamic_bindings
AppEnv --> SecretsStoreSecret : dynamic_bindings
AppEnv --> Fetcher : dynamic_bindings
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Code Review
This pull request adds support for Cloudflare Workers Secrets Store bindings, updating the AppEnv type and providing configuration examples. The review suggests including string in the AppEnv index signature to accommodate standard environment variables. Additionally, it recommends enhancing integration tests by verifying that ASSETS are excluded from the bucket list and correcting the MY_SECRET mock to better reflect the runtime behavior of secret bindings.
| expect.objectContaining({ name: "NON_R2_BINDING" }), | ||
| expect.objectContaining({ name: "MY_SECRET" }), | ||
| ]), |
There was a problem hiding this comment.
Since ASSETS is also a non-R2 binding (a Fetcher), it should be explicitly verified as excluded from the buckets list to ensure the discovery logic correctly filters out all non-bucket bindings, including the ones explicitly defined in AppEnv.
expect.objectContaining({ name: "NON_R2_BINDING" }),
expect.objectContaining({ name: "MY_SECRET" }),
expect.objectContaining({ name: "ASSETS" }),| }, | ||
| bindings: { | ||
| NON_R2_BINDING: { type: "var", value: "some_value" }, // For testing non-bucket bindings | ||
| MY_SECRET: { type: "var", value: "mock-secret-value" }, // Mock SecretsStoreSecret binding |
There was a problem hiding this comment.
The mock for MY_SECRET uses type: "var", which results in a string at runtime in the test environment. However, a real SecretsStoreSecret binding is an object with a get() method. This discrepancy means the test in server.test.ts is verifying that a string is excluded from the bucket list, but it doesn't guarantee that a real object-based secret binding would be excluded if the discovery logic is not robust enough to distinguish between different object types (e.g., checking for R2-specific methods like list or put).
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Expanding the
AppEnvindex signature toR2Bucket | SecretsStoreSecret | Fetcher | stringweakens type safety; consider restricting it to the resource-like types you actually iterate over (e.g.R2Bucket | SecretsStoreSecret | Fetcher) and modeling simple string vars separately so that accidental use of non-bucket bindings is caught at compile time. - Given that
AppEnvalready declaresASSETS: Fetcher, double-check whetherFetcherreally needs to be part of the index signature as well, or if you can narrow the index type to just bucket-like bindings to better reflect how the environment is used in the worker.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Expanding the `AppEnv` index signature to `R2Bucket | SecretsStoreSecret | Fetcher | string` weakens type safety; consider restricting it to the resource-like types you actually iterate over (e.g. `R2Bucket | SecretsStoreSecret | Fetcher`) and modeling simple string vars separately so that accidental use of non-bucket bindings is caught at compile time.
- Given that `AppEnv` already declares `ASSETS: Fetcher`, double-check whether `Fetcher` really needs to be part of the index signature as well, or if you can narrow the index type to just bucket-like bindings to better reflect how the environment is used in the worker.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
$(cat <<'EOF'
Summary
AppEnvtype to includeSecretsStoreSecretin the index signature alongsideR2BucketandFetcher, enabling full TypeScript support for workers that declare[[secrets_store_secrets]]bindings[[secrets_store_secrets]]examples to bothtemplate/wrangler.tomlandpackages/worker/dev/wrangler.tomlfor user referencebindings.d.ts,vitest.config.mts) with a mockMY_SECRETbinding and add an assertion inserver.test.tsverifying secrets are not enumerated as R2 bucketsTest plan
pnpm lint— passes with no errorspnpm testinpackages/worker—server.test.tsconfirmsMY_SECRETis absent from the buckets listtsc --noEmit) with the updatedAppEnvtypehttps://claude.ai/code/session_018pheiK3TL8nFuSnC9HvpPY
EOF
)
Summary by Sourcery
Add support for Cloudflare Workers Secrets Store bindings and ensure they are correctly typed and excluded from R2 bucket enumeration.
New Features:
Tests: