Ensure SDK matches live API behaviour and broaden coverage #36
Conversation
*ensure word_fields and translation_fields are included in query serialization *add btoa shim in test setup so token auth works in Node *cover client config, fetcher auth, retry helper, search defaults, and URL utils with new vitest suites Tests: pnpm --filter @quranjs/api test; pnpm --filter @quranjs/api test:coverage
*add tooling/live-smoke.mjs to load env creds, fetch an OAuth token, and probe key production endpoints *surface payload snippets on failures so upstream outages (e.g. search 500s) are easy to spot *reuse shared headers and structured evaluators to keep the smoke run maintainable
|
@basit3407 is attempting to deploy a commit to the Ahmed Riad Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR includes a single logic fix with medium complexity (parameter naming alignment), five homogeneous new test files following consistent patterns, infrastructure setup, and a new QA script. While the test additions are voluminous, their repetitive and straightforward nature moderates review effort; the parameter fix requires verification of correctness against actual usage. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)packages/api/src/lib/url.ts(1 hunks)packages/api/test/client.test.ts(1 hunks)packages/api/test/fetcher.test.ts(1 hunks)packages/api/test/retry.test.ts(1 hunks)packages/api/test/search.test.ts(1 hunks)packages/api/test/setup.ts(1 hunks)packages/api/test/url.test.ts(1 hunks)tooling/live-smoke.mjs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx}: Use explicit types on exported functions
Use strong typing and avoid 'any'
Use camelCase for variables, functions, and methods
Use UPPERCASE for constants and environment variables
Use descriptive function names combining verbs and nouns (e.g., getUserData)
Prefer the 'function' keyword over arrow functions where their differences do not matter
Document functions with JSDoc annotations
Prefer custom interfaces over inline types
Use 'readonly' for immutable properties
If an import is only used as a type, use 'import type' instead of 'import'
When writing JSDocs, only use TypeDoc-compatible tags
Always write JSDocs for all code: classes, functions, methods, fields, types, interfaces
Files:
packages/api/test/setup.tspackages/api/test/retry.test.tspackages/api/test/fetcher.test.tspackages/api/test/client.test.tspackages/api/test/search.test.tspackages/api/test/url.test.tspackages/api/src/lib/url.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use kebab-case for files and directories
Files:
packages/api/test/setup.tspackages/api/test/retry.test.tspackages/api/test/fetcher.test.tspackages/api/test/client.test.tspackages/api/test/search.test.tstooling/live-smoke.mjspackages/api/test/url.test.tspackages/api/src/lib/url.ts
🧬 Code graph analysis (6)
packages/api/test/retry.test.ts (1)
packages/api/src/lib/retry.ts (1)
retry(1-24)
packages/api/test/fetcher.test.ts (1)
packages/api/src/sdk/fetcher.ts (1)
QuranFetcher(22-140)
packages/api/test/client.test.ts (1)
packages/api/src/sdk/fetcher.ts (1)
QuranFetcher(22-140)
packages/api/test/search.test.ts (2)
packages/api/src/sdk/fetcher.ts (1)
QuranFetcher(22-140)packages/api/src/sdk/search.ts (1)
QuranSearch(10-39)
tooling/live-smoke.mjs (1)
packages/api/src/sdk/fetcher.ts (1)
fetch(119-139)
packages/api/test/url.test.ts (1)
packages/api/src/lib/url.ts (2)
removeBeginningSlash(6-8)paramsToString(12-44)
🔇 Additional comments (15)
.gitignore (1)
17-17: LGTM! Essential safeguard for credentials.Adding
.envto.gitignorecorrectly prevents environment files containing API credentials from being committed to version control.packages/api/src/lib/url.ts (1)
10-10: LGTM! Critical bug fix for fields parameter handling.Changing to snake_case keys correctly aligns with the decamelized parameter keys produced on line 15, ensuring the fields-specific processing at line 32 now triggers properly for
word_fieldsandtranslation_fields.tooling/live-smoke.mjs (5)
30-42: LGTM! Robust JSON parsing with helpful error context.The error-resistant implementation correctly handles empty responses and provides detailed diagnostic information on parse failures.
44-78: LGTM! Proper OAuth token flow with validation.The implementation correctly constructs the OAuth request, handles base64 encoding, and validates the token response structure before returning.
89-178: LGTM! Comprehensive endpoint coverage with structured validation.The endpoint definitions provide good coverage across chapters, verses, recitations, search, and audio endpoints, with appropriate payload validation for each.
180-216: LGTM! Thorough test execution with proper error handling.The loop correctly handles both HTTP failures and evaluation failures, tracks failure counts, and provides clear success/failure logging for each endpoint.
218-221: LGTM! Appropriate top-level error handler.The catch block correctly handles unexpected failures in the smoke test execution and ensures a non-zero exit code.
packages/api/test/setup.ts (1)
6-9: LGTM! Essential polyfill for Node.js test compatibility.The conditional
btoapolyfill correctly ensures base64 encoding is available in Node.js test environments where it's not natively supported, aligning with the fetcher's authentication flow.packages/api/test/client.test.ts (1)
1-75: LGTM! Comprehensive client configuration tests.The test suite thoroughly validates:
- Default configuration resolution for base URLs and language
- Config merging that preserves existing defaults while applying updates
- Proper delegation of both
updateConfigandclearCachedTokento the underlying fetcherpackages/api/test/fetcher.test.ts (1)
1-150: LGTM! Thorough fetcher behavior validation.The test suite comprehensively validates:
- Token caching and reuse across multiple requests (lines 37-121)
- Correct URL construction with base URL, path, and decamelized query parameters (lines 97-109)
- Proper request headers including auth token and client ID (lines 111-120)
- Error propagation for non-OK responses (lines 123-149)
packages/api/test/retry.test.ts (1)
1-45: LGTM! Well-structured retry helper tests.The test suite correctly validates:
- Retry behavior with eventual success (lines 6-25)
- Error propagation after retry exhaustion (lines 27-44)
- Proper use of fake timers with cleanup in finally blocks
- Correct invocation counts matching retry configuration
packages/api/test/search.test.ts (1)
1-72: LGTM! Complete search API behavior validation.The test suite thoroughly validates:
- Default page size (30) is applied when not specified (lines 8-39)
- Custom options correctly override defaults while preserving the query parameter (lines 41-71)
- Response mapping from fetcher to search result structure
packages/api/test/url.test.ts (3)
1-6: LGTM! Clean test setup.The imports are well-organized, and the test suite is properly structured with a clear describe block.
7-10: LGTM! Good coverage for path handling.The test validates both branches of the slash removal logic clearly.
12-15: LGTM! Proper edge case coverage.Testing both undefined and empty object inputs ensures the guard clauses work as expected.
| it("serialises complex query parameters correctly", () => { | ||
| const query = paramsToString({ | ||
| language: Language.ENGLISH, | ||
| page: 2, | ||
| perPage: 25, | ||
| words: true, | ||
| translations: [1, 2, 3], | ||
| fields: { | ||
| textUthmani: true, | ||
| codeV1: false, | ||
| }, | ||
| wordFields: { | ||
| textUthmani: true, | ||
| codeV2: true, | ||
| }, | ||
| translationFields: { | ||
| verseKey: true, | ||
| languageName: false, | ||
| }, | ||
| }); | ||
|
|
||
| expect(query.startsWith("?")).toBe(true); | ||
|
|
||
| const searchParams = new URLSearchParams(query.slice(1)); | ||
|
|
||
| expect(searchParams.get("language")).toBe(Language.ENGLISH); | ||
| expect(searchParams.get("page")).toBe("2"); | ||
| expect(searchParams.get("per_page")).toBe("25"); | ||
| expect(searchParams.get("words")).toBe("true"); | ||
| expect(searchParams.get("translations")).toBe("1,2,3"); | ||
| expect(searchParams.get("fields")).toBe("text_uthmani"); | ||
| expect(searchParams.get("word_fields")).toBe("text_uthmani,code_v2"); | ||
| expect(searchParams.get("translation_fields")).toBe("verse_key"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Comprehensive validation of the parameter serialization fix.
This test effectively validates the core fix described in the PR:
- Decamelization works correctly (perPage → per_page, wordFields → word_fields)
- Fields objects are properly filtered (only true values included)
- All parameter types serialize correctly (strings, numbers, booleans, arrays, objects)
The use of URLSearchParams for validation mirrors the implementation approach, which strengthens the test.
Optional: Consider adding edge case tests to further strengthen coverage:
Additional test cases to consider:
- Empty array parameter:
translations: [] - Fields object with all false values:
fields: { textUthmani: false, codeV1: false } - Special characters in string values that require URL encoding
Apply this diff to add an edge case test:
+ it("handles edge cases gracefully", () => {
+ const query = paramsToString({
+ translations: [],
+ fields: { textUthmani: false, codeV1: false },
+ });
+
+ const searchParams = new URLSearchParams(query.slice(1));
+ expect(searchParams.get("translations")).toBe("");
+ expect(searchParams.has("fields")).toBe(false);
+ });
+📝 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.
| it("serialises complex query parameters correctly", () => { | |
| const query = paramsToString({ | |
| language: Language.ENGLISH, | |
| page: 2, | |
| perPage: 25, | |
| words: true, | |
| translations: [1, 2, 3], | |
| fields: { | |
| textUthmani: true, | |
| codeV1: false, | |
| }, | |
| wordFields: { | |
| textUthmani: true, | |
| codeV2: true, | |
| }, | |
| translationFields: { | |
| verseKey: true, | |
| languageName: false, | |
| }, | |
| }); | |
| expect(query.startsWith("?")).toBe(true); | |
| const searchParams = new URLSearchParams(query.slice(1)); | |
| expect(searchParams.get("language")).toBe(Language.ENGLISH); | |
| expect(searchParams.get("page")).toBe("2"); | |
| expect(searchParams.get("per_page")).toBe("25"); | |
| expect(searchParams.get("words")).toBe("true"); | |
| expect(searchParams.get("translations")).toBe("1,2,3"); | |
| expect(searchParams.get("fields")).toBe("text_uthmani"); | |
| expect(searchParams.get("word_fields")).toBe("text_uthmani,code_v2"); | |
| expect(searchParams.get("translation_fields")).toBe("verse_key"); | |
| }); | |
| it("serialises complex query parameters correctly", () => { | |
| const query = paramsToString({ | |
| language: Language.ENGLISH, | |
| page: 2, | |
| perPage: 25, | |
| words: true, | |
| translations: [1, 2, 3], | |
| fields: { | |
| textUthmani: true, | |
| codeV1: false, | |
| }, | |
| wordFields: { | |
| textUthmani: true, | |
| codeV2: true, | |
| }, | |
| translationFields: { | |
| verseKey: true, | |
| languageName: false, | |
| }, | |
| }); | |
| expect(query.startsWith("?")).toBe(true); | |
| const searchParams = new URLSearchParams(query.slice(1)); | |
| expect(searchParams.get("language")).toBe(Language.ENGLISH); | |
| expect(searchParams.get("page")).toBe("2"); | |
| expect(searchParams.get("per_page")).toBe("25"); | |
| expect(searchParams.get("words")).toBe("true"); | |
| expect(searchParams.get("translations")).toBe("1,2,3"); | |
| expect(searchParams.get("fields")).toBe("text_uthmani"); | |
| expect(searchParams.get("word_fields")).toBe("text_uthmani,code_v2"); | |
| expect(searchParams.get("translation_fields")).toBe("verse_key"); | |
| }); | |
| it("handles edge cases gracefully", () => { | |
| const query = paramsToString({ | |
| translations: [], | |
| fields: { textUthmani: false, codeV1: false }, | |
| }); | |
| const searchParams = new URLSearchParams(query.slice(1)); | |
| expect(searchParams.get("translations")).toBe(""); | |
| expect(searchParams.has("fields")).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
In packages/api/test/url.test.ts around lines 17 to 50, add an additional test
case that exercises edge conditions: pass translations: [] to ensure empty
arrays are omitted or serialized as expected, pass fields and wordFields objects
with all false values to confirm they produce no entries, and include a string
parameter containing special characters (e.g., spaces, & and =) to verify proper
URL encoding; construct the query with paramsToString, assert it starts with
"?", parse with URLSearchParams(query.slice(1)), and assert the translations key
is absent or empty per implementation, that fields/word_fields are null/empty,
and that the special-character parameter value equals the correctly encoded
original string when retrieved via URLSearchParams.
| const loadEnv = () => { | ||
| const envPath = resolve(process.cwd(), ".env"); | ||
| if (!existsSync(envPath)) return; | ||
|
|
||
| const lines = readFileSync(envPath, "utf-8").split(/\r?\n/); | ||
| for (const line of lines) { | ||
| if (!line || line.startsWith("#")) continue; | ||
| const [key, ...rest] = line.split("="); | ||
| if (!key) continue; | ||
| if (process.env[key]) continue; | ||
| const value = rest.join("="); | ||
| process.env[key] = value; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider edge cases in .env parsing.
The custom .env loader doesn't handle quoted values or trim whitespace. While functional for basic cases, values like KEY="value" or KEY=value will include the quotes or trailing spaces.
Consider trimming the key and value:
- const [key, ...rest] = line.split("=");
- if (!key) continue;
+ const [rawKey, ...rest] = line.split("=");
+ const key = rawKey?.trim();
+ if (!key) continue;
if (process.env[key]) continue;
- const value = rest.join("=");
+ const value = rest.join("=").trim();
process.env[key] = value;📝 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.
| const loadEnv = () => { | |
| const envPath = resolve(process.cwd(), ".env"); | |
| if (!existsSync(envPath)) return; | |
| const lines = readFileSync(envPath, "utf-8").split(/\r?\n/); | |
| for (const line of lines) { | |
| if (!line || line.startsWith("#")) continue; | |
| const [key, ...rest] = line.split("="); | |
| if (!key) continue; | |
| if (process.env[key]) continue; | |
| const value = rest.join("="); | |
| process.env[key] = value; | |
| } | |
| }; | |
| const loadEnv = () => { | |
| const envPath = resolve(process.cwd(), ".env"); | |
| if (!existsSync(envPath)) return; | |
| const lines = readFileSync(envPath, "utf-8").split(/\r?\n/); | |
| for (const line of lines) { | |
| if (!line || line.startsWith("#")) continue; | |
| const [rawKey, ...rest] = line.split("="); | |
| const key = rawKey?.trim(); | |
| if (!key) continue; | |
| if (process.env[key]) continue; | |
| const value = rest.join("=").trim(); | |
| process.env[key] = value; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In tooling/live-smoke.mjs around lines 4 to 17, the .env loader does not trim
whitespace from keys/values and leaves surrounding quotes in quoted values
(e.g., KEY="value") — update the parser to trim() the key and the joined value,
skip empty keys after trim, remove surrounding matching single or double quotes
from the value (and unescape common escaped quotes inside if present), and
continue to allow = in the value by joining rest; preserve existing behavior of
skipping comments and existing process.env entries.
| if (!clientId || !clientSecret) { | ||
| console.error("Missing QF_CLIENT_ID or QF_CLIENT_SECRET env vars."); | ||
| process.exitCode = 1; | ||
| process.exit(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove explicit process.exit() call.
Since process.exitCode is already set on line 26, the explicit process.exit() on line 27 is unnecessary and prevents any potential cleanup handlers from running.
Apply this diff:
if (!clientId || !clientSecret) {
console.error("Missing QF_CLIENT_ID or QF_CLIENT_SECRET env vars.");
process.exitCode = 1;
- process.exit();
}🤖 Prompt for AI Agents
In tooling/live-smoke.mjs around lines 24 to 28, remove the explicit
process.exit() call; since process.exitCode is already set when env vars are
missing, simply set process.exitCode = 1 after logging the error and let the
process terminate naturally so cleanup handlers can run. Replace the block by
keeping the console.error and process.exitCode assignment and delete the
process.exit() invocation.
1 participant
Tests
Summary by CodeRabbit