feat(ambee): add Ambee environmental intelligence plugin - #651
Open
NISHANT-GUPTA1 wants to merge 2 commits into
Open
feat(ambee): add Ambee environmental intelligence plugin#651NISHANT-GUPTA1 wants to merge 2 commits into
NISHANT-GUPTA1 wants to merge 2 commits into
Conversation
Adds the @corsair-dev/ambee plugin covering Ambee's air quality, weather, pollen, wildfire and location-services APIs (18 read-only endpoints) with API-key auth via the x-api-key header.
Contributor
|
@NISHANT-GUPTA1 is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis change adds the ChangesAmbee provider
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin as ambee plugin
participant Endpoint as endpoint group
participant Client as makeAmbeeRequest
participant API as Ambee API
participant Store as persistence helper
Plugin->>Endpoint: invoke typed endpoint
Endpoint->>Client: build request and query
Client->>API: GET with x-api-key
API-->>Client: response payload
Client-->>Endpoint: validated response
Endpoint->>Store: persist supported records
Endpoint-->>Plugin: return result and log completion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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. 🔧 Biome (2.5.6)packages/ambee/endpoints/disasters.test.tsComment |
Contributor
Greptile SummaryThe PR adds an Ambee plugin with 18 environmental and location endpoints, API-key authentication, Zod response schemas, retry/error routing, telemetry, tests, and best-effort persistence.
Confidence Score: 3/5The PR should not merge until pollen inputs are validated at runtime and zero-valued geocode coordinates are preserved during persistence. Ambiguous pollen calls currently query a silently selected location, while valid equator or prime-meridian coordinates are stored as missing; the remaining broad-type documentation issue is non-blocking. Files Needing Attention: packages/ambee/endpoints/pollen.ts, packages/ambee/endpoints/persist.ts, packages/ambee/client.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Corsair as Corsair Endpoint Binding
participant Plugin as Ambee Endpoint
participant API as Ambee API
participant DB as Corsair Database
Caller->>Corsair: Invoke environmental lookup
Corsair->>Plugin: Pass context and input
Plugin->>API: GET with x-api-key and query
API-->>Plugin: Provider response
Plugin->>Plugin: Validate response with Zod
opt Persisted endpoint
Plugin->>DB: Best-effort upsert
end
Plugin-->>Caller: Parsed response
Reviews (1): Last reviewed commit: "feat(ambee): add Ambee environmental int..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+16
to
+18
| return 'place' in input | ||
| ? { place: input.place } | ||
| : { lat: input.lat, lng: input.lng }; |
Contributor
There was a problem hiding this comment.
When a call supplies both place and lat/lng, runtime binding does not apply the registered union schema and pollenLocationQuery silently selects place, causing Ambee to return data for a different location than the supplied coordinates.
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
Knowledge Base Used: The provider-plugin package pattern
Comment on lines
+126
to
+127
| lat: result.lat === null ? undefined : Number(result.lat) || undefined, | ||
| lng: result.lng === null ? undefined : Number(result.lng) || undefined, |
Contributor
There was a problem hiding this comment.
When Ambee returns latitude or longitude as numeric 0 or string "0", the Number(value) || undefined conversion treats the valid coordinate as missing, causing the persisted geocoded-place row to omit data present in the endpoint response.
Suggested change
| lat: result.lat === null ? undefined : Number(result.lat) || undefined, | |
| lng: result.lng === null ? undefined : Number(result.lng) || undefined, | |
| lat: | |
| result.lat === null || | |
| result.lat === undefined || | |
| Number.isNaN(Number(result.lat)) | |
| ? undefined | |
| : Number(result.lat), | |
| lng: | |
| result.lng === null || | |
| result.lng === undefined || | |
| Number.isNaN(Number(result.lng)) | |
| ? undefined | |
| : Number(result.lng), |
Knowledge Base Used: The provider-plugin package pattern
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: plugin files only | ✅ | |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | ✅ | |
| R4 — Demo video / recording | ❌ | Required in "Screenshots / Demos" before a maintainer reviews |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @NISHANT-GUPTA1, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
Rule Used: Every endpoint must validate inputs and outputs wi... (source) Knowledge Base Used: The provider-plugin package pattern
Knowledge Base Used: The provider-plugin package pattern PR requirements (rules)
If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
There was a problem hiding this comment.
Pull request overview
Adds a new @corsair-dev/ambee plugin package that integrates with Ambee’s environmental intelligence APIs (air quality, weather, pollen, wildfire, and geocoding) and registers it in Corsair’s provider registry.
Changes:
- Introduces the full Ambee plugin package (client, endpoints, schemas, persistence helpers, and error handlers).
- Adds comprehensive Jest test coverage for request wiring, schema validation, endpoint behavior, and error routing (plus optional live contract tests).
- Registers
ambeeinpackages/corsair/core/constants.tsand updates the workspace lockfile.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds the new packages/ambee importer and dev dependency resolutions. |
| packages/corsair/core/constants.ts | Registers ambee in provider lists/types for core discovery. |
| packages/ambee/package.json | Declares the new plugin package metadata, scripts, and deps/peers. |
| packages/ambee/tsconfig.json | Adds TS project config for the new plugin package. |
| packages/ambee/tsup.config.ts | Adds build bundling configuration for ESM output. |
| packages/ambee/jest.config.cjs | Adds Jest configuration for plugin test execution. |
| packages/ambee/index.ts | Implements the plugin factory, endpoint tree, schemas/meta, and exports. |
| packages/ambee/client.ts | Adds the shared HTTP request helper, API base, error wrapper, and timestamp normalization. |
| packages/ambee/client.test.ts | Tests request config/header behavior, error wrapping, and timestamp normalization. |
| packages/ambee/error-handlers.ts | Adds error classification and retry/backoff routing for common HTTP statuses. |
| packages/ambee/error-handlers.test.ts | Tests handler routing precedence and retry/no-retry behavior. |
| packages/ambee/endpoints/index.ts | Aggregates endpoint implementations into namespaced endpoint objects. |
| packages/ambee/endpoints/types.ts | Defines Zod input/output schemas and exported endpoint IO types. |
| packages/ambee/endpoints/schema-validation.test.ts | Validates schema constraints and coverage across all endpoints. |
| packages/ambee/endpoints/persist.ts | Adds best-effort persistence helpers for readings and geocode results. |
| packages/ambee/endpoints/air-quality.ts | Implements air quality endpoints (latest/history/forecast) with validation and telemetry. |
| packages/ambee/endpoints/air-quality.test.ts | Tests request paths/query, validation behavior, persistence, and telemetry for air quality. |
| packages/ambee/endpoints/weather.ts | Implements weather endpoints (latest/history/forecast) with validation, persistence, and telemetry. |
| packages/ambee/endpoints/weather.test.ts | Tests request paths/query, persistence behavior, and telemetry for weather. |
| packages/ambee/endpoints/pollen.ts | Implements pollen endpoints with union input handling and query shaping. |
| packages/ambee/endpoints/pollen.test.ts | Tests location union behavior, speciesRisk query omission rules, and forecast path selection. |
| packages/ambee/endpoints/fire.ts | Implements wildfire endpoints (latest/risk by lat/lng or place) with validation and telemetry. |
| packages/ambee/endpoints/fire.test.ts | Tests request paths/query and telemetry for wildfire endpoints. |
| packages/ambee/endpoints/geocode.ts | Implements geocode/reverse-geocode endpoints with persistence and telemetry. |
| packages/ambee/endpoints/geocode.test.ts | Tests request paths/query, persistence keying, coordinate coercion, and telemetry. |
| packages/ambee/schema/index.ts | Declares the plugin’s schema entity map and exports database schemas/types. |
| packages/ambee/schema/database.ts | Defines Zod schemas for persisted entities (air quality, weather, geocode). |
| packages/ambee/schema.test.ts | Tests schema shape/version invariants for plugin schema registration. |
| packages/ambee/api.test.ts | Adds optional live contract tests that self-skip without AMBEE_API_KEY. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/ambee/endpoints/types.ts:327
- Same as above: this field is documented as a three-letter ISO country code, but the schema only enforces
.min(2). Align the validation with the documented/expected format so history requests fail fast with a clear validation error.
countryCode: z
.string()
.min(2)
.describe('Three-letter ISO country code, e.g. "IND" or "USA"'),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+122
to
+128
| for (const [index, result] of results.entries()) { | ||
| await safeUpsert('geocode result', () => | ||
| table.upsertByEntityId(`${query}#${index}`, { | ||
| query, | ||
| lat: result.lat === null ? undefined : Number(result.lat) || undefined, | ||
| lng: result.lng === null ? undefined : Number(result.lng) || undefined, | ||
| city: result.city ?? undefined, |
Comment on lines
+308
to
+312
| countryCode: z | ||
| .string() | ||
| .min(2) | ||
| .describe('Three-letter ISO country code, e.g. "IND" or "USA"'), | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/ambee/client.test.ts (1)
81-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the assertions in the
catchblock.If
makeAmbeeRequeststops throwing, thecatchblock never runs and this test passes without checking anything. Addexpect.assertionsso the test fails in that case.♻️ Proposed change
it('wraps an ApiError in AmbeeAPIError, preserving status and cause', async () => { + expect.assertions(5); const original = apiError(429, 1500); mockRequest.mockRejectedValue(original);🤖 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 `@packages/ambee/client.test.ts` around lines 81 - 98, Add an expect.assertions count at the start of the “wraps an ApiError in AmbeeAPIError, preserving status and cause” test to cover every assertion in the catch block, ensuring the test fails if makeAmbeeRequest resolves instead of throwing.
🤖 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 `@packages/ambee/endpoints/persist.ts`:
- Around line 122-138: Update the coordinate mapping in the safeUpsert call
within the results persistence loop to preserve valid zero values: convert lat
and lng only when the result is non-null and the converted value is finite,
otherwise use undefined. Add a regression test covering string "0" values for
both coordinates and verify they persist as numeric zero.
In `@packages/ambee/endpoints/types.ts`:
- Around line 399-448: Update all branches of PollenGetLatestInputSchema,
PollenGetHistoryInputSchema, and PollenGetForecastInputSchema in
packages/ambee/endpoints/types.ts:399-448 to use strict object validation so
coordinate-and-place inputs are rejected rather than silently stripping unknown
fields. Add a schema-validation assertion in
packages/ambee/endpoints/schema-validation.test.ts:25-41 confirming
pollenGetLatest rejects an input containing lat, lng, and place.
- Around line 306-330: Update the countryCode fields in
AirQualityGetLatestByPostalCodeInputSchema and
AirQualityGetHistoryByPostalCodeInputSchema to require exactly three characters
with .length(3) instead of .min(2), preserving their existing descriptions and
validation behavior otherwise.
In `@packages/ambee/error-handlers.ts`:
- Around line 29-102: Update the match functions for RATE_LIMIT_ERROR,
AUTH_ERROR, VALIDATION_ERROR, and SERVER_ERROR so message-based fallback runs
only when getStatus(error) is absent; when status is present, rely exclusively
on the corresponding status checks. Replace bare numeric substring checks in the
fallback with standalone-token matching for each HTTP code, while preserving the
existing textual checks and handler behavior.
In `@packages/ambee/jest.config.cjs`:
- Around line 46-49: Remove the explicit corsair/core and corsair/http entries
from moduleNameMapper in the Jest configuration, leaving those imports to
resolve through the declared workspace corsair package while preserving the
relative .js mapping.
---
Nitpick comments:
In `@packages/ambee/client.test.ts`:
- Around line 81-98: Add an expect.assertions count at the start of the “wraps
an ApiError in AmbeeAPIError, preserving status and cause” test to cover every
assertion in the catch block, ensuring the test fails if makeAmbeeRequest
resolves instead of throwing.
🪄 Autofix
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 Plus
Run ID: d7925c01-ff34-4832-ba69-093b57840e40
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
packages/ambee/api.test.tspackages/ambee/client.test.tspackages/ambee/client.tspackages/ambee/endpoints/air-quality.test.tspackages/ambee/endpoints/air-quality.tspackages/ambee/endpoints/fire.test.tspackages/ambee/endpoints/fire.tspackages/ambee/endpoints/geocode.test.tspackages/ambee/endpoints/geocode.tspackages/ambee/endpoints/index.tspackages/ambee/endpoints/persist.tspackages/ambee/endpoints/pollen.test.tspackages/ambee/endpoints/pollen.tspackages/ambee/endpoints/schema-validation.test.tspackages/ambee/endpoints/types.tspackages/ambee/endpoints/weather.test.tspackages/ambee/endpoints/weather.tspackages/ambee/error-handlers.test.tspackages/ambee/error-handlers.tspackages/ambee/index.tspackages/ambee/jest.config.cjspackages/ambee/package.jsonpackages/ambee/schema.test.tspackages/ambee/schema/database.tspackages/ambee/schema/index.tspackages/ambee/tsconfig.jsonpackages/ambee/tsup.config.tspackages/corsair/core/constants.ts
Comment on lines
+122
to
+138
| for (const [index, result] of results.entries()) { | ||
| await safeUpsert('geocode result', () => | ||
| table.upsertByEntityId(`${query}#${index}`, { | ||
| query, | ||
| lat: result.lat === null ? undefined : Number(result.lat) || undefined, | ||
| lng: result.lng === null ? undefined : Number(result.lng) || undefined, | ||
| city: result.city ?? undefined, | ||
| state: result.state ?? undefined, | ||
| countryCode: result.countryCode ?? undefined, | ||
| postalCode: | ||
| result.postalCode === null || result.postalCode === undefined | ||
| ? undefined | ||
| : String(result.postalCode), | ||
| placeName: result.placeName ?? undefined, | ||
| formattedAddress: result.formattedAddress ?? undefined, | ||
| fetchedAt: new Date(), | ||
| }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve zero-valued coordinates.
Number(result.lat) || undefined converts a valid coordinate of 0 to undefined. The same defect exists for lng. A result on the equator or prime meridian will persist without its coordinate.
Use a finite-number check that preserves zero. Add a regression test with "0" coordinate values.
Proposed fix
for (const [index, result] of results.entries()) {
+ const lat = Number(result.lat);
+ const lng = Number(result.lng);
+
await safeUpsert('geocode result', () =>
table.upsertByEntityId(`${query}#${index}`, {
query,
- lat: result.lat === null ? undefined : Number(result.lat) || undefined,
- lng: result.lng === null ? undefined : Number(result.lng) || undefined,
+ lat: result.lat === null || !Number.isFinite(lat) ? undefined : lat,
+ lng: result.lng === null || !Number.isFinite(lng) ? undefined : lng,📝 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.
Suggested change
| for (const [index, result] of results.entries()) { | |
| await safeUpsert('geocode result', () => | |
| table.upsertByEntityId(`${query}#${index}`, { | |
| query, | |
| lat: result.lat === null ? undefined : Number(result.lat) || undefined, | |
| lng: result.lng === null ? undefined : Number(result.lng) || undefined, | |
| city: result.city ?? undefined, | |
| state: result.state ?? undefined, | |
| countryCode: result.countryCode ?? undefined, | |
| postalCode: | |
| result.postalCode === null || result.postalCode === undefined | |
| ? undefined | |
| : String(result.postalCode), | |
| placeName: result.placeName ?? undefined, | |
| formattedAddress: result.formattedAddress ?? undefined, | |
| fetchedAt: new Date(), | |
| }), | |
| for (const [index, result] of results.entries()) { | |
| const lat = Number(result.lat); | |
| const lng = Number(result.lng); | |
| await safeUpsert('geocode result', () => | |
| table.upsertByEntityId(`${query}#${index}`, { | |
| query, | |
| lat: result.lat === null || !Number.isFinite(lat) ? undefined : lat, | |
| lng: result.lng === null || !Number.isFinite(lng) ? undefined : lng, | |
| city: result.city ?? undefined, | |
| state: result.state ?? undefined, | |
| countryCode: result.countryCode ?? undefined, | |
| postalCode: | |
| result.postalCode === null || result.postalCode === undefined | |
| ? undefined | |
| : String(result.postalCode), | |
| placeName: result.placeName ?? undefined, | |
| formattedAddress: result.formattedAddress ?? undefined, | |
| fetchedAt: new Date(), | |
| }), |
🤖 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 `@packages/ambee/endpoints/persist.ts` around lines 122 - 138, Update the
coordinate mapping in the safeUpsert call within the results persistence loop to
preserve valid zero values: convert lat and lng only when the result is non-null
and the converted value is finite, otherwise use undefined. Add a regression
test covering string "0" values for both coordinates and verify they persist as
numeric zero.
Comment on lines
+306
to
+330
| export const AirQualityGetLatestByPostalCodeInputSchema = z.object({ | ||
| postalCode: z.string().min(1).describe('Postal code, e.g. "560020"'), | ||
| countryCode: z | ||
| .string() | ||
| .min(2) | ||
| .describe('Three-letter ISO country code, e.g. "IND" or "USA"'), | ||
| }); | ||
| export type AirQualityGetLatestByPostalCodeInput = z.infer< | ||
| typeof AirQualityGetLatestByPostalCodeInputSchema | ||
| >; | ||
|
|
||
| export const AirQualityGetHistoryByLatLngInputSchema = LatLngRangeInputSchema; | ||
| export type AirQualityGetHistoryByLatLngInput = z.infer< | ||
| typeof AirQualityGetHistoryByLatLngInputSchema | ||
| >; | ||
|
|
||
| export const AirQualityGetHistoryByPostalCodeInputSchema = z.object({ | ||
| postalCode: z.string().min(1).describe('Postal code, e.g. "560020"'), | ||
| countryCode: z | ||
| .string() | ||
| .min(2) | ||
| .describe('Three-letter ISO country code, e.g. "IND" or "USA"'), | ||
| from: FromSchema, | ||
| to: ToSchema, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Constrain countryCode to three characters.
The description states a three-letter ISO code, but .min(2) accepts two-letter codes and longer strings. A two-letter code passes local validation and then fails at the provider. Use .length(3) to keep the stated contract and avoid the wasted call.
🔧 Proposed fix
-const CountryCodeSchema = z
- .string()
- .min(2)
- .describe('Three-letter ISO country code, e.g. "IND" or "USA"'); export const AirQualityGetLatestByPostalCodeInputSchema = z.object({
postalCode: z.string().min(1).describe('Postal code, e.g. "560020"'),
countryCode: z
.string()
- .min(2)
+ .length(3)
.describe('Three-letter ISO country code, e.g. "IND" or "USA"'),
}); export const AirQualityGetHistoryByPostalCodeInputSchema = z.object({
postalCode: z.string().min(1).describe('Postal code, e.g. "560020"'),
countryCode: z
.string()
- .min(2)
+ .length(3)
.describe('Three-letter ISO country code, e.g. "IND" or "USA"'),
from: FromSchema,
to: ToSchema,
});The duplicated field is also a good candidate for a shared CountryCodeSchema fragment, like LatSchema and LngSchema.
🤖 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 `@packages/ambee/endpoints/types.ts` around lines 306 - 330, Update the
countryCode fields in AirQualityGetLatestByPostalCodeInputSchema and
AirQualityGetHistoryByPostalCodeInputSchema to require exactly three characters
with .length(3) instead of .min(2), preserving their existing descriptions and
validation behavior otherwise.
Comment on lines
+399
to
+448
| /** | ||
| * Pollen endpoints accept *either* a coordinate pair or a place name — never | ||
| * both. Each input is therefore a union of the two location forms rather than | ||
| * one object with both fields optional, so an ambiguous call is rejected at | ||
| * validation time instead of silently dropping a parameter at request time. | ||
| */ | ||
| export const PollenGetLatestInputSchema = z.union([ | ||
| z.object({ lat: LatSchema, lng: LngSchema, speciesRisk: SpeciesRiskSchema }), | ||
| z.object({ place: PlaceSchema, speciesRisk: SpeciesRiskSchema }), | ||
| ]); | ||
| export type PollenGetLatestInput = z.infer<typeof PollenGetLatestInputSchema>; | ||
|
|
||
| export const PollenGetHistoryInputSchema = z.union([ | ||
| z.object({ | ||
| lat: LatSchema, | ||
| lng: LngSchema, | ||
| from: FromSchema, | ||
| to: ToSchema, | ||
| speciesRisk: SpeciesRiskSchema, | ||
| }), | ||
| z.object({ | ||
| place: PlaceSchema, | ||
| from: FromSchema, | ||
| to: ToSchema, | ||
| speciesRisk: SpeciesRiskSchema, | ||
| }), | ||
| ]); | ||
| export type PollenGetHistoryInput = z.infer<typeof PollenGetHistoryInputSchema>; | ||
|
|
||
| const PollenForecastHoursSchema = z | ||
| .union([z.literal(48), z.literal(120)]) | ||
| .optional() | ||
| .describe('Forecast horizon in hours: 48 (hourly) or 120 (3-hourly)'); | ||
|
|
||
| export const PollenGetForecastInputSchema = z.union([ | ||
| z.object({ | ||
| lat: LatSchema, | ||
| lng: LngSchema, | ||
| hours: PollenForecastHoursSchema, | ||
| speciesRisk: SpeciesRiskSchema, | ||
| }), | ||
| z.object({ | ||
| place: PlaceSchema, | ||
| hours: PollenForecastHoursSchema, | ||
| speciesRisk: SpeciesRiskSchema, | ||
| }), | ||
| ]); | ||
| export type PollenGetForecastInput = z.infer< | ||
| typeof PollenGetForecastInputSchema | ||
| >; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The pollen location unions accept an ambiguous input. The three pollen input unions use z.object() branches. Zod strips unknown keys, so { lat, lng, place } matches the coordinate branch and place is dropped without an error. This is the exact behavior the doc comment says it prevents, and no test currently detects it.
packages/ambee/endpoints/types.ts#L399-L448: change each branch ofPollenGetLatestInputSchema,PollenGetHistoryInputSchema, andPollenGetForecastInputSchemafromz.objecttoz.strictObject.packages/ambee/endpoints/schema-validation.test.ts#L25-L41: add an assertion thatpollenGetLatest.safeParse({ lat, lng, place })fails.
📍 Affects 2 files
packages/ambee/endpoints/types.ts#L399-L448(this comment)packages/ambee/endpoints/schema-validation.test.ts#L25-L41
🤖 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 `@packages/ambee/endpoints/types.ts` around lines 399 - 448, Update all
branches of PollenGetLatestInputSchema, PollenGetHistoryInputSchema, and
PollenGetForecastInputSchema in packages/ambee/endpoints/types.ts:399-448 to use
strict object validation so coordinate-and-place inputs are rejected rather than
silently stripping unknown fields. Add a schema-validation assertion in
packages/ambee/endpoints/schema-validation.test.ts:25-41 confirming
pollenGetLatest rejects an input containing lat, lng, and place.
Comment on lines
+29
to
+102
| RATE_LIMIT_ERROR: { | ||
| match: (error: Error) => { | ||
| if (getStatus(error) === 429) return true; | ||
| const msg = error.message.toLowerCase(); | ||
| return msg.includes('429') || msg.includes('rate limit'); | ||
| }, | ||
| handler: async (error: Error) => ({ | ||
| maxRetries: 3, | ||
| retryStrategy: 'exponential_backoff' as const, | ||
| headersRetryAfterMs: getRetryAfter(error), | ||
| }), | ||
| }, | ||
| AUTH_ERROR: { | ||
| match: (error: Error) => { | ||
| const status = getStatus(error); | ||
| if (status === 401 || status === 403) return true; | ||
| const msg = error.message.toLowerCase(); | ||
| return ( | ||
| msg.includes('401') || | ||
| msg.includes('403') || | ||
| msg.includes('unauthorized') || | ||
| msg.includes('forbidden') || | ||
| msg.includes('invalid api key') | ||
| ); | ||
| }, | ||
| handler: async () => { | ||
| console.error( | ||
| '[AMBEE] Authentication failed — check that the API key is valid and ' + | ||
| 'that the account is subscribed to the requested product.', | ||
| ); | ||
| return { maxRetries: 0 }; | ||
| }, | ||
| }, | ||
| NOT_FOUND_ERROR: { | ||
| // Ambee returns 404 when a location is outside a product's coverage | ||
| // (e.g. wildfire risk outside North America) — retrying cannot help. | ||
| match: (error: Error) => { | ||
| if (getStatus(error) === 404) return true; | ||
| const msg = error.message.toLowerCase(); | ||
| return msg.includes('404') || msg.includes('not found'); | ||
| }, | ||
| handler: async () => { | ||
| console.warn( | ||
| '[AMBEE] No data available for the requested location — it may be ' + | ||
| 'outside this product’s coverage area.', | ||
| ); | ||
| return { maxRetries: 0 }; | ||
| }, | ||
| }, | ||
| VALIDATION_ERROR: { | ||
| match: (error: Error) => { | ||
| const status = getStatus(error); | ||
| if (status === 400 || status === 422) return true; | ||
| const msg = error.message.toLowerCase(); | ||
| return ( | ||
| msg.includes('400') || | ||
| msg.includes('422') || | ||
| msg.includes('unprocessable') | ||
| ); | ||
| }, | ||
| handler: async () => { | ||
| console.warn( | ||
| '[AMBEE] Request rejected — a required parameter is missing or malformed.', | ||
| ); | ||
| return { maxRetries: 0 }; | ||
| }, | ||
| }, | ||
| SERVER_ERROR: { | ||
| match: (error: Error) => { | ||
| const status = getStatus(error); | ||
| if (status !== undefined && status >= 500) return true; | ||
| const msg = error.message.toLowerCase(); | ||
| return msg.includes('500') || msg.includes('internal server error'); | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bare numeric substring matching can route errors to the wrong handler.
Each matcher falls back to error.message.includes('<code>') even when status is present and does not match. Ambee error messages carry the request URL and response body, so digits from a postal code, a coordinate, or a body field can contain "400", "404", "429", or "500". Matchers run in declaration order, so a 503 whose body text contains "429" retries as a rate limit, and a 500 whose message contains "404" is never retried.
Apply the message fallback only when status is absent, and match the code as a standalone token.
🔧 Proposed fix
+/** Matches a bare HTTP status code as a standalone token in a message. */
+function messageHasCode(message: string, ...codes: number[]): boolean {
+ return codes.some((code) => new RegExp(`\\b${code}\\b`).test(message));
+}
+
export const errorHandlers = {
RATE_LIMIT_ERROR: {
match: (error: Error) => {
- if (getStatus(error) === 429) return true;
+ const status = getStatus(error);
+ if (status !== undefined) return status === 429;
const msg = error.message.toLowerCase();
- return msg.includes('429') || msg.includes('rate limit');
+ return messageHasCode(msg, 429) || msg.includes('rate limit');
}, NOT_FOUND_ERROR: {
match: (error: Error) => {
- if (getStatus(error) === 404) return true;
+ const status = getStatus(error);
+ if (status !== undefined) return status === 404;
const msg = error.message.toLowerCase();
- return msg.includes('404') || msg.includes('not found');
+ return messageHasCode(msg, 404) || msg.includes('not found');
},Apply the same pattern to AUTH_ERROR, VALIDATION_ERROR, and SERVER_ERROR.
🤖 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 `@packages/ambee/error-handlers.ts` around lines 29 - 102, Update the match
functions for RATE_LIMIT_ERROR, AUTH_ERROR, VALIDATION_ERROR, and SERVER_ERROR
so message-based fallback runs only when getStatus(error) is absent; when status
is present, rely exclusively on the corresponding status checks. Replace bare
numeric substring checks in the fallback with standalone-token matching for each
HTTP code, while preserving the existing textual checks and handler behavior.
Comment on lines
+46
to
+49
| moduleNameMapper: { | ||
| '^corsair/core$': '<rootDir>/../corsair/core.ts', | ||
| '^corsair/http$': '<rootDir>/../corsair/http.ts', | ||
| '^(\\.\\.?/.*)\\.js$': '$1', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove sibling source mappings for Corsair modules.
The mappings bypass the declared corsair dependency and couple this plugin test suite to packages/corsair internal files. Resolve corsair/core and corsair/http through the workspace package instead.
As per coding guidelines, each plugin should remain self-contained within its own packages/<plugin>/ package, except for its required registration in packages/corsair/core/constants.ts.
🤖 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 `@packages/ambee/jest.config.cjs` around lines 46 - 49, Remove the explicit
corsair/core and corsair/http entries from moduleNameMapper in the Jest
configuration, leaving those imports to resolve through the declared workspace
corsair package while preserving the relative .js mapping.
Source: Coding guidelines
Completes the API surface listed on the Corsair OSS dashboard: air quality by country code, elevation by lat/lng and place, the influenza-like-illness forecast, and the seven paginated natural-disaster endpoints.
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 `@packages/ambee/endpoints/types.ts`:
- Around line 406-411: Expose the optional positive-integer limit for the
country air-quality query: add it to AirQualityGetLatestByCountryCodeInputSchema
in packages/ambee/endpoints/types.ts, forward input.limit in the
latest/by-country-code request in packages/ambee/endpoints/air-quality.ts, and
update packages/ambee/endpoints/air-quality.test.ts to verify a supplied limit
reaches makeAmbeeRequest.
- Around line 627-631: In packages/ambee/endpoints/types.ts lines 627-631,
update EventTypeSchema from an unrestricted optional string to an optional enum
containing exactly TN, EQ, TC, WF, FL, ET, DR, SW, SI, VO, LS, and Misc. In
packages/ambee/endpoints/schema-validation.test.ts lines 148-175, add a
validation case confirming an unsupported eventType is rejected.
🪄 Autofix
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 Plus
Run ID: dd1f68f8-c0dc-4dbe-8d54-920f453999a5
📒 Files selected for processing (12)
packages/ambee/api.test.tspackages/ambee/endpoints/air-quality.test.tspackages/ambee/endpoints/air-quality.tspackages/ambee/endpoints/disasters.test.tspackages/ambee/endpoints/disasters.tspackages/ambee/endpoints/elevation.test.tspackages/ambee/endpoints/elevation.tspackages/ambee/endpoints/ili.tspackages/ambee/endpoints/index.tspackages/ambee/endpoints/schema-validation.test.tspackages/ambee/endpoints/types.tspackages/ambee/index.ts
Comment on lines
+406
to
+411
| export const AirQualityGetLatestByCountryCodeInputSchema = z.object({ | ||
| countryCode: z | ||
| .string() | ||
| .min(2) | ||
| .describe('Three-letter ISO country code, e.g. "IND" or "USA"'), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose and forward the country-query limit.
The provider supports an optional limit for latest/by-country-code. The current endpoint always uses the provider default of one station. (docs.ambeedata.com)
packages/ambee/endpoints/types.ts#L406-L411: add an optional positive integerlimitfield.packages/ambee/endpoints/air-quality.ts#L108-L114: includeinput.limitin the request query.packages/ambee/endpoints/air-quality.test.ts#L183-L200: verify that a suppliedlimitreachesmakeAmbeeRequest.
📍 Affects 3 files
packages/ambee/endpoints/types.ts#L406-L411(this comment)packages/ambee/endpoints/air-quality.ts#L108-L114packages/ambee/endpoints/air-quality.test.ts#L183-L200
🤖 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 `@packages/ambee/endpoints/types.ts` around lines 406 - 411, Expose the
optional positive-integer limit for the country air-quality query: add it to
AirQualityGetLatestByCountryCodeInputSchema in
packages/ambee/endpoints/types.ts, forward input.limit in the
latest/by-country-code request in packages/ambee/endpoints/air-quality.ts, and
update packages/ambee/endpoints/air-quality.test.ts to verify a supplied limit
reaches makeAmbeeRequest.
Comment on lines
+627
to
+631
| const EventTypeSchema = z | ||
| .string() | ||
| .min(1) | ||
| .optional() | ||
| .describe('Restrict results to a single Ambee event-type code'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict eventType to documented disaster codes.
z.string().min(1) accepts values that every disaster endpoint forwards to the provider. Use an optional enum for the documented codes: TN, EQ, TC, WF, FL, ET, DR, SW, SI, VO, LS, and Misc. (docs.ambeedata.com)
packages/ambee/endpoints/types.ts#L627-L631: replace the unrestricted string schema with the optional supported-code enum.packages/ambee/endpoints/schema-validation.test.ts#L148-L175: add a rejected unsupportedeventTypecase.
📍 Affects 2 files
packages/ambee/endpoints/types.ts#L627-L631(this comment)packages/ambee/endpoints/schema-validation.test.ts#L148-L175
🤖 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 `@packages/ambee/endpoints/types.ts` around lines 627 - 631, In
packages/ambee/endpoints/types.ts lines 627-631, update EventTypeSchema from an
unrestricted optional string to an optional enum containing exactly TN, EQ, TC,
WF, FL, ET, DR, SW, SI, VO, LS, and Misc. In
packages/ambee/endpoints/schema-validation.test.ts lines 148-175, add a
validation case confirming an unsupported eventType is rejected.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements the
@corsair-dev/ambeeplugin for Ambee — real-time and historical environmental intelligence (air quality, weather, pollen, wildfire) plus location services.Claimed via the Corsair OSS dashboard (
corsair.dev/oss/ambee).Operations implemented (18)
Air Quality —
docs.ambeedata.com/apis/air-qualityairQuality.getLatestByLatLng—GET /latest/by-lat-lngairQuality.getLatestByCity—GET /latest/by-cityairQuality.getLatestByPostalCode—GET /latest/by-postal-codeairQuality.getHistoryByLatLng—GET /history/by-lat-lngairQuality.getHistoryByPostalCode—GET /history/by-postal-codeairQuality.getForecastByLatLng—GET /forecast/aq/by-lat-lngWeather —
docs.ambeedata.com/apis/weatherweather.getLatest—GET /weather/latest/by-lat-lngweather.getHistory—GET /weather/history/by-lat-lngweather.getForecast—GET /weather/forecast/by-lat-lngPollen (v3) —
docs.ambeedata.com/apis/pollenpollen.getLatest—GET /v3/pollen/latestpollen.getHistory—GET /v3/pollen/historypollen.getForecast—GET /v3/pollen/forecast/48hrsand/120hrsWildfire —
docs.ambeedata.com/apis/firefire.getLatestByLatLng—GET /fire/latest/by-lat-lngfire.getLatestByPlace—GET /fire/latest/by-placefire.getRiskByLatLng—GET /fire/risk/by-lat-lngfire.getRiskByPlace—GET /fire/risk/by-placeLocation services —
docs.ambeedata.com/apis/locationgeocode.byPlace—GET /geocode/by-placegeocode.reverseByLatLng—GET /geocode/reverse/by-lat-lngAuthentication
x-api-keyheader.OpenAPIConfig.TOKENis deliberately left unset — setting it would add anAuthorization: Bearerheader, which Ambee rejects.Architecture
client.ts) — every Ambee product is served fromhttps://api.ambeedata.com, so one base URL and one request helper covers all five products.toAmbeeTimestamp()accepts either Ambee'sYYYY-MM-DD hh:mm:ssformat or any ISO 8601 date and normalises to UTC, so callers (and agents) don't have to know the provider's format.lat/lngvsplacechoice for pollen (a union, so an ambiguous call is rejected at validation time rather than silently dropping a parameter). Outputs validate Ambee's{ message: "success", ... }envelope and the documented payload fields; response objects are.loose()with optional members because Ambee omits fields a location has no coverage for (a station with no SO2 sensor simply omitsSO2), and provider-side additions pass through instead of throwing.AmbeeAPIErrorchains the originatingApiErrorascauseand copiesstatus/statusText/body/retryAfter/rate-limit headers onto itself, soerror-handlers.tsroutes 401/403 (auth), 404 (out-of-coverage location), 400/422 (validation), 429 (rate limit, withRetry-Afterpropagation and exponential backoff) and 5xx (retried) without needinginstanceof ApiErrorchecks.airQualityReadings,weatherObservationsandgeocodedPlacesentities are upserted best-effort, keyed by coordinates rounded to ~11 m so repeated lookups for the same place update one row. Storage failures are logged and swallowed; a missing table (no database configured) is skipped.latest/by-citytakes alimitparameter, which is forwarded.Verification
Test coverage — 59 assertions across 9 suites:
client.test.ts— header/auth wiring, query passthrough,ApiError→AmbeeAPIErrorstatus/cause preservation, timestamp normalisationendpoints/{air-quality,weather,pollen,fire,geocode}.test.ts— every one of the 18 endpoints asserts its path, query shape, parsed result, persistence rows and telemetry eventendpoints/schema-validation.test.ts— input rejection (out-of-range coordinates, missing range bound, invalid fire type, invalid forecast horizon, pollen with neither location form) and output toleranceerror-handlers.test.ts— routing for 400/401/403/404/422/429/5xx and the default bucketapi.test.ts— live contract tests against the real API, run withAMBEE_API_KEY=...; they self-skip without a key and CI excludesapi.test.tsby designChecklist
pnpm lintand all checks passpnpm typecheckand there are no TypeScript errorspnpm buildand all packages build successfullypnpm testand all tests passScreenshots / Demos
To add: recording of the live endpoints running against a real Ambee API key.
Additional Notes
pnpm generate:plugin Ambeeproduces:packages/ambee/**, the single registration edit inpackages/corsair/core/constants.ts, andpnpm-lock.yaml.Summary by CodeRabbit
New Features
Tests