feat: siren support - #1
Conversation
# Conflicts: # .gitignore # package-lock.json # package.json # src/client.ts # yarn.lock
📝 WalkthroughWalkthroughAdds API-key Siren REST support with metric, region, and metric-list operations; integrates it into ChangesSiren REST integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant DClimateClient
participant SirenClient
participant SirenAPI
Application->>DClimateClient: construct with Siren options
DClimateClient->>SirenClient: initialize configured client
Application->>DClimateClient: access client.siren
DClimateClient-->>Application: return SirenClient
Application->>SirenClient: getMetricData(query)
SirenClient->>SirenAPI: send authenticated request
SirenAPI-->>SirenClient: return metric response
SirenClient-->>Application: return normalized data points
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 6
🧹 Nitpick comments (4)
src/client.ts (1)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused STAC cache fields.
These fields are declared but never used in the class. As noted by the comment in
getStacCatalog, client-level caching was abandoned in favor of a module-level cache, making these fields dead code.♻️ Proposed refactor
- private stacCatalog?: StacCatalog; - private stacCatalogTimestamp?: number; - private stacCacheTtl: number = 3600000; // 1 hour🤖 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/client.ts` around lines 45 - 47, Remove the unused stacCatalog, stacCatalogTimestamp, and stacCacheTtl fields from the client class. Keep the existing getStacCatalog implementation and module-level caching unchanged.src/errors.ts (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
X402NotInstalledErroris never thrown.No call site constructs or throws this error anywhere in the provided Siren code — it's dead code given
siren-client.tsimports@x402/fetch/@x402/evmstatically rather than lazily. See consolidated comment for root cause.🤖 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/errors.ts` at line 20, Remove the unused X402NotInstalledError class from errors.ts, since no Siren call site constructs or throws it and the static `@x402` imports do not require this error.src/siren/siren-client.ts (2)
210-216: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on any Siren
fetchcall.Every
fetch/x402-wrapped-fetch call in this file (getMetricData,listRegions,listMetrics) has noAbortController/timeout. A hung Siren API leaves the caller's await blocked indefinitely.♻️ Suggested pattern (apply per call site)
function withTimeout(ms: number) { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), ms); return { signal: controller.signal, cancel: () => clearTimeout(timer) }; }Also applies to: 230-233, 249-255, 270-273, 288-291
🤖 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/siren/siren-client.ts` around lines 210 - 216, Add timeout handling to every Siren request in getMetricData, listRegions, and listMetrics, including direct fetch and x402-wrapped-fetch call sites. Create an AbortController with the established timeout duration, pass its signal to each request, and clear the timer after completion so hung requests abort instead of awaiting indefinitely.
209-209: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnescaped path segments in Siren request URLs.
query.regionId,query.metric, andthis.auth.accountIdare interpolated directly into the URL path with noencodeURIComponent. Any special character (spaces,/,..) breaks the request structure or lets a caller redirect the request to an unintended path on the same authenticated host.🛡️ Proposed fix (example for one site — apply to all 5)
- const url = `${this.baseUrl}/metric-data-multiple/${this.auth.accountId}/${query.regionId}/${query.metric}/${startDate}/${endDate}`; + const url = `${this.baseUrl}/metric-data-multiple/${encodeURIComponent(this.auth.accountId)}/${encodeURIComponent(query.regionId)}/${encodeURIComponent(query.metric)}/${startDate}/${endDate}`;Also applies to: 229-229, 248-248, 269-269, 287-287
🤖 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/siren/siren-client.ts` at line 209, Update every Siren request URL construction site, including the flow containing metric-data-multiple and the four additional affected sites, to wrap each dynamic path segment—this.auth.accountId, query.regionId, query.metric, and the corresponding date or other interpolated values—in encodeURIComponent before interpolation. Preserve the existing endpoint structure while ensuring no caller-controlled value can alter path boundaries.
🤖 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 `@package.json`:
- Around line 55-59: Move `@x402/core`, `@x402/evm`, and `@x402/fetch` out of regular
dependencies and into the project’s optional dependency configuration,
preserving their existing versions so consumers are not required to install x402
tooling by default.
- Around line 60-66: Move viem from devDependencies to dependencies in
package.json so the runtime import used by createEip1193Signer and
src/siren/helpers.ts is available to package consumers. Keep the existing viem
version unchanged and remove its duplicate devDependency entry.
In `@README.md`:
- Line 89: Update the README example around client.listMetrics() to use
SirenClient, which exposes listMetrics(), or add a corresponding listMetrics
forwarder to DClimateClient. Ensure the documented client API matches the
implementation while preserving the example’s intended behavior.
In `@src/siren/helpers.ts`:
- Line 9: Update the package manifest to list viem as a production dependency
rather than only a development dependency, since the exported helpers module
imports encodeFunctionData and Abi at runtime. Preserve the existing viem
version and remove any duplicate devDependency entry.
In `@src/siren/siren-client.ts`:
- Around line 5-6: Replace the static `@x402/fetch` and `@x402/evm` imports in
siren-client.ts with optional runtime loading, preserving the documented
zero-hard-dependency behavior when those packages are unavailable. Update the
affected client initialization and signer usage to resolve these modules only
when x402 payment functionality is requested, using the existing symbols
wrapFetchWithPayment, x402Client, ExactEvmScheme, and toClientEvmSigner.
- Around line 109-116: Update getEnv to access the environment through a
type-safe globalThis lookup instead of directly referencing process, avoiding
reliance on Node ambient types while preserving undefined behavior in browsers
and when the environment is unavailable.
---
Nitpick comments:
In `@src/client.ts`:
- Around line 45-47: Remove the unused stacCatalog, stacCatalogTimestamp, and
stacCacheTtl fields from the client class. Keep the existing getStacCatalog
implementation and module-level caching unchanged.
In `@src/errors.ts`:
- Line 20: Remove the unused X402NotInstalledError class from errors.ts, since
no Siren call site constructs or throws it and the static `@x402` imports do not
require this error.
In `@src/siren/siren-client.ts`:
- Around line 210-216: Add timeout handling to every Siren request in
getMetricData, listRegions, and listMetrics, including direct fetch and
x402-wrapped-fetch call sites. Create an AbortController with the established
timeout duration, pass its signal to each request, and clear the timer after
completion so hung requests abort instead of awaiting indefinitely.
- Line 209: Update every Siren request URL construction site, including the flow
containing metric-data-multiple and the four additional affected sites, to wrap
each dynamic path segment—this.auth.accountId, query.regionId, query.metric, and
the corresponding date or other interpolated values—in encodeURIComponent before
interpolation. Preserve the existing endpoint structure while ensuring no
caller-controlled value can alter path boundaries.
🪄 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: ef7b1c26-3444-4ee0-bbbc-e055148fa719
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
.gitignoreREADME.mdexamples/siren-metric-data.tspackage.jsonscripts/test-x402-siren.tssrc/client.tssrc/constants.tssrc/errors.tssrc/index.tssrc/siren/helpers.tssrc/siren/index.tssrc/siren/siren-client.tssrc/siren/types.tssrc/siren/x402-types.d.tssrc/types.tstests/siren.test.tstsconfig.json
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
|
|
||
| return { | ||
| get address(): `0x${string}` { | ||
| if (!cachedAddress) { |
There was a problem hiding this comment.
HIGH
getX402Fetch() passes this signer to toClientEvmSigner(), whose adapter immediately reads signer.address. Because the address is only populated inside the later signTypedData() call, the documented browser-wallet flow always throws before its first request. Request the account before returning the signer (for example, make this factory async) or require the address as input. See the x402 adapter.
|
|
||
| async signTypedData(params) { | ||
| const address = await getAddress(); | ||
| const typedData = JSON.stringify({ |
There was a problem hiding this comment.
HIGH
The x402 EIP-3009 scheme supplies bigint fields such as value, validAfter, and validBefore, so native JSON.stringify() throws Do not know how to serialize a BigInt before invoking the wallet. Serialize typed data with bigint support, converting integer fields to RPC-compatible strings. See x402's EIP-3009 payload construction.
| }, | ||
| }); | ||
|
|
||
| const metrics = await client.listMetrics(); // returns string[] of available metric names |
There was a problem hiding this comment.
MEDIUM
DClimateClient does not define listMetrics()—only the separately exported SirenClient does—so this primary example fails TypeScript compilation and throws at runtime in JavaScript. Add a forwarding method to DClimateClient or change the documented API.
| ); | ||
| } | ||
| const data: SirenRegionsResponse = await response.json(); | ||
| return data.items; |
There was a problem hiding this comment.
MEDIUM
SirenRegionsResponse is paginated (limit, offset, and total), but both auth branches discard that metadata and return only the first items page. Accounts or global region lists larger than the server page size are silently truncated; fetch subsequent offsets or expose pagination parameters.
| "globals": "17.6.0", | ||
| "typescript": "^5.4.0", | ||
| "typescript-eslint": "8.61.0", | ||
| "viem": "^2.46.2", |
There was a problem hiding this comment.
MEDIUM
src/siren/helpers.ts imports viem in published runtime code, but viem is declared only as a dev dependency. Strict dependency resolvers such as Yarn PnP can therefore fail to load the package. Declare it as a runtime dependency or peer dependency.
| * Siren REST API client supporting API-key and x402 payment authentication. | ||
| */ | ||
|
|
||
| import { wrapFetchWithPayment, x402Client } from "@x402/fetch"; |
There was a problem hiding this comment.
MEDIUM
These top-level x402 imports are reached through the main DClimateClient module, so existing dataset-only and API-key consumers must load or bundle the EVM/payment stack despite the README calling it optional. Move the x402 implementation behind a dynamic x402-only path or a separate entry point.
| } | ||
|
|
||
| return Object.entries(timeSeries).map(([date, value]) => { | ||
| const numericValue = Number(value); |
There was a problem hiding this comment.
MEDIUM
Number(value) converts missing or invalid API values such as null, "", and false to 0, silently turning absent climate measurements into real zero readings. Explicitly accept numbers or validated numeric strings and reject null/boolean/empty values; the list-response parser has the same issue.
TheGreatAlgo
changed the title
| accountId?: string; | ||
| } | ||
|
|
||
| export type SirenAuth = SirenApiKeyAuth; |
There was a problem hiding this comment.
HIGH
x402 authentication is not implemented. SirenAuth only accepts API-key auth, while SirenClient unconditionally requires apiKey and accountId. The documented/tested { type: "x402", signer, ... } configuration therefore fails type checking and throws at construction. Add the x402 auth variant and payment-aware request implementation/exports, or remove the advertised x402 API.
| }, | ||
| }); | ||
|
|
||
| const metrics = await client.listMetrics(); // returns string[] of available metric names |
There was a problem hiding this comment.
MEDIUM
This documented call does not exist on DClimateClient. The facade only adds getMetricData() and listRegions(); listMetrics() exists solely on SirenClient, so this example fails to compile/run. Add a delegating DClimateClient.listMetrics() with the same configuration guard.
| } | ||
| const data: SirenRegionsResponse = await response.json(); | ||
| return data.items; | ||
| } |
There was a problem hiding this comment.
MEDIUM
listRegions() silently returns only the first page. SirenRegionsResponse exposes limit, offset, and total, but this discards that metadata without requesting subsequent offsets. Accounts exceeding the server page size will receive an incomplete region list; iterate until total is reached or expose pagination explicitly.
|
|
||
| return Object.entries(timeSeries).map(([date, value]) => { | ||
| const numericValue = Number(value); | ||
| if (!Number.isFinite(numericValue)) { |
There was a problem hiding this comment.
MEDIUM
Missing metric values are converted into real zeros. Number(null) and Number("") both produce 0 and pass the finite check, corrupting time series instead of rejecting or preserving missing data. Validate the original value's type/content before conversion; apply the same correction to the array-response parser.
| ); | ||
| } | ||
| const data: SirenRegionsResponse = await response.json(); | ||
| return data.items; |
There was a problem hiding this comment.
MEDIUM
listRegions() discards limit, offset, and total, so accounts exceeding the server's page size silently receive an incomplete region list. Fetch subsequent offsets until total is reached, or expose pagination explicitly.
| const startDate = formatDate(query.startDate); | ||
| const endDate = formatDate(query.endDate); | ||
|
|
||
| const url = `${this.baseUrl}/metric-data-multiple/${this.auth.accountId}/${query.regionId}/${query.metric}/${startDate}/${endDate}`; |
There was a problem hiding this comment.
LOW
Encode every dynamic path segment before interpolation. The public query type accepts arbitrary strings, so /, ?, or # in regionId, metric, or dates can alter the requested route; apply encodeURIComponent here and to accountId in the regions URL.
There was a problem hiding this comment.
Codex Automated Review
Found one actionable issue.
No inline issues were posted.
Findings that could not be placed inline:
package-lock.json:4[low]package-lock.jsondeclares version0.6.0, while the unchangedpackage.jsonremains at0.5.11. npm publishing usespackage.json, so this feature would still publish as0.5.11and future installs may rewrite the lockfile. Updatepackage.jsontoo, or revert these lockfile version changes.
| console.log(`Date range: ${startDate.toISOString().split("T")[0]} to ${endDate.toISOString().split("T")[0]}`); | ||
|
|
||
| const data = await client.getMetricData({ | ||
| // Example of region id tied to a specific account |
There was a problem hiding this comment.
MEDIUM
DClimateClient has no getMetricData() method; it is exposed only through client.siren. The advertised command therefore throws before making a request. Call client.siren.getMetricData(...).
| const data = await client.getMetricData({ | ||
| // Example of region id tied to a specific account | ||
| regionId: "4c59966e-8653-4534-a640-5b0e9be3de81", | ||
| metric: "average_precip", |
There was a problem hiding this comment.
LOW
The request combines the user's account credentials with a region ID explicitly tied to another specific account, so the runnable example will fail for most users. Select a region from client.siren.listRegions() or accept the region ID as input.
| */ | ||
| async listMetrics(): Promise<string[]> { | ||
| const url = `${this.baseUrl}/metrics`; | ||
| const response = await fetch(url, { |
There was a problem hiding this comment.
LOW
For browser consumers using the default cross-origin API, Content-Type: application/json makes this bodyless public GET require a CORS preflight, adding an unnecessary request and potentially failing if the header is not allowed. Omit the header or use the safelisted Accept header.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/siren-metric-data.ts (1)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify date instantiation.
The inline comment
2025-12-31does not match the UTC date for1767225600, which evaluates to2026-01-01. Additionally, using Unix timestamps and multiplying by 1000 is unnecessarily complex for a straightforward example script. Consider instantiating theDateobjects directly using ISO strings for better clarity.♻️ Proposed refactor
- // Convert unix timestamps to dates - const startDate = new Date(1767225600 * 1000); // 2025-12-31 - const endDate = new Date(1798761599 * 1000); // 2026-12-31 + // Define the date range + const startDate = new Date("2026-01-01T00:00:00Z"); + const endDate = new Date("2026-12-31T23:59:59Z");🤖 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 `@examples/siren-metric-data.ts` around lines 40 - 43, Update the startDate and endDate instantiation in the example to use clear ISO date strings instead of Unix timestamps multiplied by 1000, and ensure the values and comments represent the intended UTC dates accurately.
🤖 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 `@examples/siren-metric-data.ts`:
- Around line 40-43: Update the startDate and endDate instantiation in the
example to use clear ISO date strings instead of Unix timestamps multiplied by
1000, and ensure the values and comments represent the intended UTC dates
accurately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2eb57a8b-d22a-476e-8dc9-98b084492439
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
README.mdexamples/siren-metric-data.tssrc/client.tssrc/errors.tssrc/index.tssrc/siren/index.tssrc/siren/siren-client.tssrc/siren/types.tssrc/types.tstests/siren.test.ts
💤 Files with no reviewable changes (3)
- src/siren/index.ts
- src/index.ts
- src/errors.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
| const MAX_PAGES = 10_000; | ||
|
|
||
| for (let page = 0; page < MAX_PAGES; page++) { | ||
| const url = `${this.baseUrl}/custom-regions/${accountId}/custom?limit=${pageSize}&offset=${offset}`; |
There was a problem hiding this comment.
MEDIUM
custom is a commodity filter, not a wildcard. Regions tagged cocoa, corn, etc. are omitted, and accounts without access to the custom commodity receive 403, so listRegions() does not list the account's available regions. Accept/iterate commodity codes or use the region-discovery endpoint.
|
|
||
| function formatDate(date: string | Date): string { | ||
| if (typeof date === "string") return date; | ||
| return date.toISOString().split("T")[0]; |
There was a problem hiding this comment.
MEDIUM
Serializing Date inputs through UTC can shift a local calendar date backward. For example, new Date(2025, 5, 1) in UTC+10 becomes 2025-05-31, querying the wrong day. Format local date components or require/document UTC date strings.
| if (regions.length === 0) { | ||
| throw new Error("No Siren regions available for this account."); | ||
| } | ||
| const region = regions.find((r) => r.id === requestedRegionId) ?? regions[0]; |
There was a problem hiding this comment.
LOW
When a supplied REGION_ID is not found, this silently fetches data for regions[0], producing results for the wrong geography. Only use the fallback when no ID was provided; otherwise throw when the requested ID is absent.
1 participant
Summary by CodeRabbit
New Features
DClimateClientand public client exports.Documentation
Tests