feat(client): align filtering with API usage#26
Conversation
WalkthroughThis PR implements multi-field filtering capabilities by replacing a single-filter model with a per-field query parameter approach. It introduces a two-step filter pipeline (normalize and apply), updates test suites to validate the new behavior, revises documentation, adds SQLite cache dependencies, and removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 99.42% 99.47% +0.05%
==========================================
Files 3 3
Lines 174 192 +18
==========================================
+ Hits 173 191 +18
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/TheSneakerDatabaseClient.e2e.test.ts (2)
48-76: Clarify whyreleaseYeartests are skipped.The
describe.skiplacks a comment explaining why these tests are disabled. If this is due to API limitations or pending backend support, adding a brief comment would help future maintainers understand when to enable them.- describe.skip('using releaseYear', () => { + // TODO: Enable when releaseYear filtering is supported by the API + describe.skip('using releaseYear', () => {
158-161: Consider a soft assertion instead of early return.Returning early when results are empty means no assertion runs. If the API consistently returns empty results, this test would silently pass. Consider using a soft warning with
expectto track this:if (payload.results.length === 0) { console.warn('Combined filter request returned 0 results – RapidAPI may not have matching data.'); - return; + // Still pass but log warning - flaky test detection will surface patterns + expect(payload.results.length).toBeGreaterThanOrEqual(0); + return; // Skip detailed assertions }Alternatively, you could track this as a known-flaky test if the API data is inconsistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.gitignore(1 hunks)README.md(1 hunks)package.json(1 hunks)src/TheSneakerDatabaseClient.e2e.test.ts(5 hunks)src/TheSneakerDatabaseClient.integration.test.ts(9 hunks)src/TheSneakerDatabaseClient.ts(2 hunks)src/interfaces.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Source TypeScript files should be organized insrc/directory withTheSneakerDatabaseClient.tsexposing the SDK,interfaces.tsfor shared types,utils.tsfor helpers, andindex.tsre-exporting public modules
Use Prettier-equivalent formatting with 4-space indent, single quotes, trailing commas, semicolons, and 120-character line wraps in TypeScript files
Use PascalCase for class names in TypeScript
Use camelCase for function names in TypeScript
Use UPPER_SNAKE_CASE for exported constants in TypeScript
Validate payloads with the interfaces module before sending requests in TypeScript client code
Files:
src/TheSneakerDatabaseClient.tssrc/TheSneakerDatabaseClient.e2e.test.tssrc/interfaces.tssrc/TheSneakerDatabaseClient.integration.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.ts: Test files should use the*.test.tsnaming convention and be colocated with their target implementation files
Add unit test specs alongside code using*.test.tswith Bun's built-in test runner andaxios-mock-adapterfor offline testing
Files:
src/TheSneakerDatabaseClient.e2e.test.tssrc/TheSneakerDatabaseClient.integration.test.ts
🧠 Learnings (2)
📚 Learning: 2025-12-17T08:35:10.837Z
Learnt from: CR
Repo: angelxmoreno/sneakerdb-client PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T08:35:10.837Z
Learning: Applies to src/**/*.ts : Source TypeScript files should be organized in `src/` directory with `TheSneakerDatabaseClient.ts` exposing the SDK, `interfaces.ts` for shared types, `utils.ts` for helpers, and `index.ts` re-exporting public modules
Applied to files:
src/TheSneakerDatabaseClient.tssrc/TheSneakerDatabaseClient.e2e.test.ts
📚 Learning: 2025-12-17T08:35:10.837Z
Learnt from: CR
Repo: angelxmoreno/sneakerdb-client PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T08:35:10.837Z
Learning: Applies to **/*.test.ts : Add unit test specs alongside code using `*.test.ts` with Bun's built-in test runner and `axios-mock-adapter` for offline testing
Applied to files:
src/TheSneakerDatabaseClient.e2e.test.tssrc/TheSneakerDatabaseClient.integration.test.ts
🧬 Code graph analysis (3)
src/TheSneakerDatabaseClient.ts (1)
src/interfaces.ts (4)
FilterOption(49-53)ComparableField(45-45)FILTERABLE_FIELDS(43-43)FilterOperator(41-41)
src/TheSneakerDatabaseClient.e2e.test.ts (3)
src/interfaces.ts (2)
FilterOption(49-53)GetSneakersResponse(83-86)src/TheSneakerDatabaseClient.ts (1)
TheSneakerDatabaseClient(29-272)src/index.ts (2)
TheSneakerDatabaseClient(14-14)GetSneakersResponse(4-4)
src/TheSneakerDatabaseClient.integration.test.ts (1)
src/interfaces.ts (4)
GetSneakersOptions(71-81)GetSneakersResponse(83-86)FILTERABLE_FIELDS(43-43)FilterOption(49-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (20)
.gitignore (1)
131-132: LGTM!Adding
testCache.sqliteto.gitignoreis appropriate to prevent test-generated SQLite cache files from being committed. This aligns with the new@keyvhq/sqlitedevDependency.package.json (1)
64-64: LGTM!The
@keyvhq/sqlitedevDependency version^2.1.11correctly matches the@keyvhq/coreversion, ensuring compatibility between the cache packages used in tests.README.md (1)
39-51: LGTM!The documentation updates accurately reflect the new per-field query parameter approach. The example demonstrates both direct parameters (
releaseYear: '2025') and thefiltershelper, with a clear explanation of how the API expects these parameters at the URL level.src/interfaces.ts (2)
43-43: LGTM -estimatedMarketValueremoved from filterable fields.The
FILTERABLE_FIELDSconstant now correctly includes onlyreleaseDate,releaseYear, andretailPrice, aligning with the API's actual filtering capabilities.If this is a breaking change for existing consumers who were filtering by
estimatedMarketValue, consider documenting it in the changelog or release notes.
68-68: LGTM!The union type
FilterOption | FilterOption[]provides flexibility for both single-filter and multi-filter use cases, aligning well with the new per-field filtering approach.src/TheSneakerDatabaseClient.integration.test.ts (6)
58-62: LGTM!The
filterFixturesrecord is type-safe, usingFilterOption['field']as the key type to ensure alignment withFILTERABLE_FIELDS. This keeps test fixtures in sync with the interface definition.
252-288: LGTM!The filter serialization tests thoroughly validate the per-field query parameter approach:
- Verifies filters are sent as individual field params (e.g.,
releaseYear: 'gte:2020')- Confirms the
filtersproperty is not sent in the request- Tests the
eqoperator optimization (value without operator prefix)- Dynamic iteration over
FILTERABLE_FIELDSensures comprehensive coverage
290-302: LGTM!The test correctly validates that omitting the operator defaults to
eqbehavior, serializing the value as a plain string ('200') without an operator prefix.
304-325: LGTM!This test validates the core multi-field filtering capability, ensuring that multiple filters on distinct fields are correctly serialized as separate query parameters (
releaseYear: 'gte:2000',retailPrice: 'lte:400').
327-344: LGTM!This test validates important duplicate filter detection logic. Attempting to apply multiple filters to the same field (e.g., two
releaseYearconstraints) correctly fails validation before making any network request, with a clear error message.
346-361: LGTM!This test validates the conflict detection between explicit query parameters and the
filtersoption. When bothreleaseYear: '2025'andfilters: { field: 'releaseYear', ... }are provided, the client correctly rejects the request with a descriptive error before making any network call.src/TheSneakerDatabaseClient.ts (4)
148-155: LGTM on the filter integration into query params.The integration of
applyFilterParamsintoprepareQueryParamsis clean. The method correctly handles the case where no valid filters exist by returningfalseand deleting thefilterskey.
170-188: Conflict detection logic is sound.The
applyFilterParamsmethod correctly:
- Normalizes filters via
normalizeFilters- Detects conflicts when a filter field already exists as a query parameter
- Serializes each filter to its field key and removes the original
filterskeyOne consideration: the deletion of
normalized.filtersat line 186 is always executed when filters exist, which is the correct behavior since filters are expanded into per-field parameters.
190-234: Thorough validation innormalizeFilters.The method handles:
- Undefined input gracefully (returns empty array)
- Both single filter objects and arrays
- Field presence and validity checks
- Duplicate field detection via
seenFieldsSet- Value presence validation
- Operator validation against allowed set
The error messages are descriptive and actionable.
236-240: Clean operator serialization with sensible default.The
serializeFilterValueWithOperatormethod correctly defaults toeqand only prefixes the operator when it's noteq, which aligns with typical API query parameter conventions.src/TheSneakerDatabaseClient.e2e.test.ts (5)
14-14: Type extension is well-designed.
FilterOptionWithComparecleanly extendsFilterOptionwith acomparefunction for runtime validation of filter results. This enables data-driven testing with per-field comparison logic.
107-145: Well-structured parameterized filter tests.The test design is solid:
- Each filterable field has its own compare function for runtime validation
- Tests verify every returned sneaker satisfies the filter condition
- Diagnostic logging helps debug API discrepancies
Minor observation: The
biome-ignorecomment at line 131-132 is acceptable given the dynamic field access pattern.
147-169: Good coverage for combined filters with graceful handling of empty results.The test correctly:
- Applies multiple filters simultaneously
- Handles zero-result scenarios with a warning rather than failure (appropriate for e2e tests against live APIs)
- Validates each result against all filter criteria
33-34: Empty describe block when e2e is disabled.The empty
describe('TheSneakerDatabaseClient:e2e', () => {});is a valid pattern to avoid test runner complaints about missing tests. This is cleaner than the previousdescribe.skipapproach.
38-42: SQLite cache setup is appropriate for persistent e2e test caching.Using
@keyvhq/sqlitewith a 24-hour TTL is reasonable for e2e tests to reduce API calls across test runs. The namespacetestv1helps with cache versioning.The test cache file
testCache.sqliteis already in.gitignore, preventing accidental commits of test cache files.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Changes
estimatedMarketValuefield is no longer available as a filterable option.✏️ Tip: You can customize this high-level summary in your review settings.