Issue 812#1099
Open
ramakanth98 wants to merge 4 commits into
Open
Conversation
|
|
📝 WalkthroughWalkthroughThis PR introduces a class-based axios API wrapper ( ChangesAPI Wrapper Introduction
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
src/api/index.js (1)
12-14: ⚡ Quick winConsider validating or normalizing the path parameter.
The constructor doesn't validate that
pathstarts with/. If a caller passes'campaigns'instead of'/campaigns', the resultingbaseUrlwould be malformed (e.g.,/api/v1campaigns). While all current examples use the correct format, adding defensive validation or automatic prepending would prevent future bugs.🛡️ Proposed defensive fix
constructor(path, version) { + const normalizedPath = path.startsWith('/') ? path : `/${path}` - this.baseUrl = version ? `/api/${version}${path}` : `/api${path}` + this.baseUrl = version ? `/api/${version}${normalizedPath}` : `/api${normalizedPath}` }🤖 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/api/index.js` around lines 12 - 14, The constructor currently builds this.baseUrl from the path without ensuring it begins with '/', so passing "campaigns" produces a malformed URL; normalize/validate the path at the start of the constructor (e.g., if path is falsy or not a string throw or default to '/', and if it doesn't startWith('/') prepend '/'), then compute this.baseUrl exactly as before using the normalized path and the version parameter; reference the constructor and this.baseUrl to locate where to add the normalization.
🤖 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 `@docs/superpowers/plans/2026-06-03-api-wrapper.md`:
- Line 184: The expected test summary string "Tests: 7 passed, 7 total" in the
plan should be updated to match the actual tests — change it to "Tests: 8
passed, 8 total"; confirm by comparing the test file
src/api/__tests__/api.test.js which contains 8 tests (2 URL construction tests +
3 get() tests + 3 post() tests) so the plan's expected output string must
reflect 8 total tests.
In `@docs/superpowers/specs/2026-06-03-api-wrapper-design.md`:
- Around line 85-91: The fenced code block showing the file structure is missing
a language identifier; update that fenced block (the triple-backtick block
containing "src/ api/ index.js ...") to include a language token such as
plaintext (e.g., ```plaintext) so the snippet gets proper syntax highlighting
and clarity in the docs.
In `@src/api/__tests__/api.test.js`:
- Around line 1-82: Add unit tests for API.put and API.delete mirroring the
existing get/post suites: create tests that (1) mock axios.put/axios.delete to
resolve with { data: ... } and assert API.put(path, body) and API.delete(path)
return response.data, (2) mock axios.put/axios.delete to reject with { message,
response: { status } } and assert the calls reject with APIError, and (3) assert
the thrown APIError carries the HTTP status (use rejects.toMatchObject({ status
})) — place these new describe/it blocks for API.put and API.delete after the
current post() tests and reuse the existing jest.mock('axios') setup and API
constructor patterns.
---
Nitpick comments:
In `@src/api/index.js`:
- Around line 12-14: The constructor currently builds this.baseUrl from the path
without ensuring it begins with '/', so passing "campaigns" produces a malformed
URL; normalize/validate the path at the start of the constructor (e.g., if path
is falsy or not a string throw or default to '/', and if it doesn't
startWith('/') prepend '/'), then compute this.baseUrl exactly as before using
the normalized path and the version parameter; reference the constructor and
this.baseUrl to locate where to add the normalization.
🪄 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: 0f5ba642-9238-41e1-916d-984266a154cb
📒 Files selected for processing (5)
docs/superpowers/plans/2026-06-03-api-wrapper.mddocs/superpowers/specs/2026-06-03-api-wrapper-design.mdsrc/api/__tests__/api.test.jssrc/api/index.jssrc/router/index.js
💤 Files with no reviewable changes (1)
- src/router/index.js
| npx jest src/api/__tests__/api.test.js --no-coverage | ||
| ``` | ||
|
|
||
| Expected output: `Tests: 7 passed, 7 total` |
There was a problem hiding this comment.
Correct the expected test count.
The plan states "Tests: 7 passed, 7 total", but the actual test file src/api/__tests__/api.test.js contains 8 tests (2 URL construction tests + 3 get() tests + 3 post() tests).
📝 Proposed fix
-Expected output: `Tests: 7 passed, 7 total`
+Expected output: `Tests: 8 passed, 8 total`📝 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
| Expected output: `Tests: 7 passed, 7 total` | |
| Expected output: `Tests: 8 passed, 8 total` |
🤖 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 `@docs/superpowers/plans/2026-06-03-api-wrapper.md` at line 184, The expected
test summary string "Tests: 7 passed, 7 total" in the plan should be updated to
match the actual tests — change it to "Tests: 8 passed, 8 total"; confirm by
comparing the test file src/api/__tests__/api.test.js which contains 8 tests (2
URL construction tests + 3 get() tests + 3 post() tests) so the plan's expected
output string must reflect 8 total tests.
Comment on lines
+85
to
+91
| ``` | ||
| src/ | ||
| api/ | ||
| index.js ← new | ||
| __tests__/ | ||
| api.test.js ← new | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The code block showing the file structure is missing a language identifier. Adding one improves syntax highlighting and clarity.
📝 Proposed fix
-```
+```plaintext
src/
api/
index.js ← new
__tests__/
api.test.js ← new</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/superpowers/specs/2026-06-03-api-wrapper-design.md around lines 85 -
91, The fenced code block showing the file structure is missing a language
identifier; update that fenced block (the triple-backtick block containing "src/
api/ index.js ...") to include a language token such as plaintext (e.g.,
docs.
Comment on lines
+1
to
+82
| import axios from 'axios' | ||
| import { API, APIError } from '../index' | ||
|
|
||
| jest.mock('axios') | ||
|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| describe('API — URL construction', () => { | ||
| test('builds base URL with version', () => { | ||
| const api = new API('/campaigns', 'v1') | ||
| expect(api.baseUrl).toBe('/api/v1/campaigns') | ||
| }) | ||
|
|
||
| test('builds base URL without version', () => { | ||
| const api = new API('/representatives') | ||
| expect(api.baseUrl).toBe('/api/representatives') | ||
| }) | ||
| }) | ||
|
|
||
| describe('API — get()', () => { | ||
| test('returns response.data on success', async () => { | ||
| const mockData = { id: 1, name: 'test' } | ||
| axios.get.mockResolvedValue({ data: mockData }) | ||
|
|
||
| const api = new API('/campaigns') | ||
| const result = await api.get() | ||
| expect(result).toEqual(mockData) | ||
| }) | ||
|
|
||
| test('throws APIError on failure', async () => { | ||
| axios.get.mockRejectedValue({ | ||
| message: 'Not found', | ||
| response: { status: 404 } | ||
| }) | ||
|
|
||
| const api = new API('/campaigns') | ||
| await expect(api.get()).rejects.toThrow(APIError) | ||
| }) | ||
|
|
||
| test('APIError carries HTTP status', async () => { | ||
| axios.get.mockRejectedValue({ | ||
| message: 'Not found', | ||
| response: { status: 404 } | ||
| }) | ||
|
|
||
| const api = new API('/campaigns') | ||
| await expect(api.get()).rejects.toMatchObject({ status: 404 }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('API — post()', () => { | ||
| test('returns response.data on success', async () => { | ||
| const mockData = { letter: '<p>Hello</p>' } | ||
| axios.post.mockResolvedValue({ data: mockData }) | ||
|
|
||
| const api = new API('/letter_templates', 'v1') | ||
| const result = await api.post('/render', { templateId: 1 }) | ||
| expect(result).toEqual(mockData) | ||
| }) | ||
|
|
||
| test('throws APIError on failure', async () => { | ||
| axios.post.mockRejectedValue({ | ||
| message: 'Server error', | ||
| response: { status: 500 } | ||
| }) | ||
|
|
||
| const api = new API('/letter_templates', 'v1') | ||
| await expect(api.post('/render', {})).rejects.toThrow(APIError) | ||
| }) | ||
|
|
||
| test('APIError carries HTTP status', async () => { | ||
| axios.post.mockRejectedValue({ | ||
| message: 'Server error', | ||
| response: { status: 500 } | ||
| }) | ||
|
|
||
| const api = new API('/letter_templates', 'v1') | ||
| await expect(api.post('/render', {})).rejects.toMatchObject({ status: 500 }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Add test coverage for put() and delete() methods.
The test suite covers get() and post() comprehensively, but the put() and delete() methods implemented in src/api/index.js are not tested at all. This creates a significant gap in test coverage for public API methods.
✅ Proposed tests to add
Add these test suites after the existing post() tests:
+describe('API — put()', () => {
+ test('returns response.data on success', async () => {
+ const mockData = { id: 1, updated: true }
+ axios.put.mockResolvedValue({ data: mockData })
+
+ const api = new API('/campaigns')
+ const result = await api.put('/123', { name: 'Updated' })
+ expect(result).toEqual(mockData)
+ })
+
+ test('throws APIError on failure', async () => {
+ axios.put.mockRejectedValue({
+ message: 'Update failed',
+ response: { status: 400 }
+ })
+
+ const api = new API('/campaigns')
+ await expect(api.put('/123', {})).rejects.toThrow(APIError)
+ })
+
+ test('APIError carries HTTP status', async () => {
+ axios.put.mockRejectedValue({
+ message: 'Update failed',
+ response: { status: 400 }
+ })
+
+ const api = new API('/campaigns')
+ await expect(api.put('/123', {})).rejects.toMatchObject({ status: 400 })
+ })
+})
+
+describe('API — delete()', () => {
+ test('returns response.data on success', async () => {
+ const mockData = { deleted: true }
+ axios.delete.mockResolvedValue({ data: mockData })
+
+ const api = new API('/campaigns')
+ const result = await api.delete('/123')
+ expect(result).toEqual(mockData)
+ })
+
+ test('throws APIError on failure', async () => {
+ axios.delete.mockRejectedValue({
+ message: 'Delete failed',
+ response: { status: 403 }
+ })
+
+ const api = new API('/campaigns')
+ await expect(api.delete('/123')).rejects.toThrow(APIError)
+ })
+
+ test('APIError carries HTTP status', async () => {
+ axios.delete.mockRejectedValue({
+ message: 'Delete failed',
+ response: { status: 403 }
+ })
+
+ const api = new API('/campaigns')
+ await expect(api.delete('/123')).rejects.toMatchObject({ status: 403 })
+ })
+})📝 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
| import axios from 'axios' | |
| import { API, APIError } from '../index' | |
| jest.mock('axios') | |
| afterEach(() => { | |
| jest.clearAllMocks() | |
| }) | |
| describe('API — URL construction', () => { | |
| test('builds base URL with version', () => { | |
| const api = new API('/campaigns', 'v1') | |
| expect(api.baseUrl).toBe('/api/v1/campaigns') | |
| }) | |
| test('builds base URL without version', () => { | |
| const api = new API('/representatives') | |
| expect(api.baseUrl).toBe('/api/representatives') | |
| }) | |
| }) | |
| describe('API — get()', () => { | |
| test('returns response.data on success', async () => { | |
| const mockData = { id: 1, name: 'test' } | |
| axios.get.mockResolvedValue({ data: mockData }) | |
| const api = new API('/campaigns') | |
| const result = await api.get() | |
| expect(result).toEqual(mockData) | |
| }) | |
| test('throws APIError on failure', async () => { | |
| axios.get.mockRejectedValue({ | |
| message: 'Not found', | |
| response: { status: 404 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.get()).rejects.toThrow(APIError) | |
| }) | |
| test('APIError carries HTTP status', async () => { | |
| axios.get.mockRejectedValue({ | |
| message: 'Not found', | |
| response: { status: 404 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.get()).rejects.toMatchObject({ status: 404 }) | |
| }) | |
| }) | |
| describe('API — post()', () => { | |
| test('returns response.data on success', async () => { | |
| const mockData = { letter: '<p>Hello</p>' } | |
| axios.post.mockResolvedValue({ data: mockData }) | |
| const api = new API('/letter_templates', 'v1') | |
| const result = await api.post('/render', { templateId: 1 }) | |
| expect(result).toEqual(mockData) | |
| }) | |
| test('throws APIError on failure', async () => { | |
| axios.post.mockRejectedValue({ | |
| message: 'Server error', | |
| response: { status: 500 } | |
| }) | |
| const api = new API('/letter_templates', 'v1') | |
| await expect(api.post('/render', {})).rejects.toThrow(APIError) | |
| }) | |
| test('APIError carries HTTP status', async () => { | |
| axios.post.mockRejectedValue({ | |
| message: 'Server error', | |
| response: { status: 500 } | |
| }) | |
| const api = new API('/letter_templates', 'v1') | |
| await expect(api.post('/render', {})).rejects.toMatchObject({ status: 500 }) | |
| }) | |
| }) | |
| import axios from 'axios' | |
| import { API, APIError } from '../index' | |
| jest.mock('axios') | |
| afterEach(() => { | |
| jest.clearAllMocks() | |
| }) | |
| describe('API — URL construction', () => { | |
| test('builds base URL with version', () => { | |
| const api = new API('/campaigns', 'v1') | |
| expect(api.baseUrl).toBe('/api/v1/campaigns') | |
| }) | |
| test('builds base URL without version', () => { | |
| const api = new API('/representatives') | |
| expect(api.baseUrl).toBe('/api/representatives') | |
| }) | |
| }) | |
| describe('API — get()', () => { | |
| test('returns response.data on success', async () => { | |
| const mockData = { id: 1, name: 'test' } | |
| axios.get.mockResolvedValue({ data: mockData }) | |
| const api = new API('/campaigns') | |
| const result = await api.get() | |
| expect(result).toEqual(mockData) | |
| }) | |
| test('throws APIError on failure', async () => { | |
| axios.get.mockRejectedValue({ | |
| message: 'Not found', | |
| response: { status: 404 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.get()).rejects.toThrow(APIError) | |
| }) | |
| test('APIError carries HTTP status', async () => { | |
| axios.get.mockRejectedValue({ | |
| message: 'Not found', | |
| response: { status: 404 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.get()).rejects.toMatchObject({ status: 404 }) | |
| }) | |
| }) | |
| describe('API — post()', () => { | |
| test('returns response.data on success', async () => { | |
| const mockData = { letter: '<p>Hello</p>' } | |
| axios.post.mockResolvedValue({ data: mockData }) | |
| const api = new API('/letter_templates', 'v1') | |
| const result = await api.post('/render', { templateId: 1 }) | |
| expect(result).toEqual(mockData) | |
| }) | |
| test('throws APIError on failure', async () => { | |
| axios.post.mockRejectedValue({ | |
| message: 'Server error', | |
| response: { status: 500 } | |
| }) | |
| const api = new API('/letter_templates', 'v1') | |
| await expect(api.post('/render', {})).rejects.toThrow(APIError) | |
| }) | |
| test('APIError carries HTTP status', async () => { | |
| axios.post.mockRejectedValue({ | |
| message: 'Server error', | |
| response: { status: 500 } | |
| }) | |
| const api = new API('/letter_templates', 'v1') | |
| await expect(api.post('/render', {})).rejects.toMatchObject({ status: 500 }) | |
| }) | |
| }) | |
| describe('API — put()', () => { | |
| test('returns response.data on success', async () => { | |
| const mockData = { id: 1, updated: true } | |
| axios.put.mockResolvedValue({ data: mockData }) | |
| const api = new API('/campaigns') | |
| const result = await api.put('/123', { name: 'Updated' }) | |
| expect(result).toEqual(mockData) | |
| }) | |
| test('throws APIError on failure', async () => { | |
| axios.put.mockRejectedValue({ | |
| message: 'Update failed', | |
| response: { status: 400 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.put('/123', {})).rejects.toThrow(APIError) | |
| }) | |
| test('APIError carries HTTP status', async () => { | |
| axios.put.mockRejectedValue({ | |
| message: 'Update failed', | |
| response: { status: 400 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.put('/123', {})).rejects.toMatchObject({ status: 400 }) | |
| }) | |
| }) | |
| describe('API — delete()', () => { | |
| test('returns response.data on success', async () => { | |
| const mockData = { deleted: true } | |
| axios.delete.mockResolvedValue({ data: mockData }) | |
| const api = new API('/campaigns') | |
| const result = await api.delete('/123') | |
| expect(result).toEqual(mockData) | |
| }) | |
| test('throws APIError on failure', async () => { | |
| axios.delete.mockRejectedValue({ | |
| message: 'Delete failed', | |
| response: { status: 403 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.delete('/123')).rejects.toThrow(APIError) | |
| }) | |
| test('APIError carries HTTP status', async () => { | |
| axios.delete.mockRejectedValue({ | |
| message: 'Delete failed', | |
| response: { status: 403 } | |
| }) | |
| const api = new API('/campaigns') | |
| await expect(api.delete('/123')).rejects.toMatchObject({ status: 403 }) | |
| }) | |
| }) |
🤖 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/api/__tests__/api.test.js` around lines 1 - 82, Add unit tests for
API.put and API.delete mirroring the existing get/post suites: create tests that
(1) mock axios.put/axios.delete to resolve with { data: ... } and assert
API.put(path, body) and API.delete(path) return response.data, (2) mock
axios.put/axios.delete to reject with { message, response: { status } } and
assert the calls reject with APIError, and (3) assert the thrown APIError
carries the HTTP status (use rejects.toMatchObject({ status })) — place these
new describe/it blocks for API.put and API.delete after the current post() tests
and reuse the existing jest.mock('axios') setup and API constructor patterns.
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
1 participant
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.
Summary
Closes #812
Introduces a common interface for front-end API requests, replacing scattered
axioscalls with a tested, resource-scopedwrapper — following the same class + custom error pattern used in
shared/presenters/payment-presenter.js.src/api/index.jsexportingAPI(takes a resource path and optional version, exposesget/post/put/delete) andAPIError(extendsError, carries HTTPstatus)src/api/__tests__/api.test.jswith 8 Jest unit tests covering URL construction, success/failure forgetandpost, and error status propagationUsage
Test plan
npx jest src/api/__tests__/api.test.js --no-coverage→ 8 passednpm testNotes
This PR lays the foundation. Existing axios call sites (SearchReps, DonateMoney, SignName, etc.) are intentionally left for
follow-up migration — per the issue spec: "Don't worry about moving every api over, just the new stuff."
Summary by CodeRabbit
Documentation
Chores