Repository navigation
feat: add userId to form controllers, refactor template route structu… - #84
Conversation
…re, and implement OAuth integration tests
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a Bun-based OAuth integration test suite mocking GitHub auth flows, adds prisma disconnect cleanup in test bootstrap, includes ChangesBackend and test changes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as "Test Suite"
participant API as "OAuth Initiation Route"
participant BetterAuth as "Better-Auth Callback"
participant DB as "Database"
Test->>API: initiate social auth
API-->>Test: redirect URL + state + better-auth.state cookie
Test->>BetterAuth: call callback with mocked GitHub responses
BetterAuth->>DB: create/update user
BetterAuth-->>Test: better-auth.session_token cookie
Test->>API: custom OAuth callback (new user)
API-->>Test: redirect to /onboarding
Test->>DB: set username
Test->>API: custom OAuth callback again
API-->>Test: redirect to /dashboard
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/controllers/form.controller.ts (1)
92-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect change, but consider deduplicating the repeated
selectshape.Adding
userId: trueis correct and matches the test expectation thatresponse.data.data.userIdequals the authenticated user's id, and since routes are guarded byrequireAuthpluswhere: { id, userId }filters, no cross-user data is exposed. However, the identical select object is now duplicated three times (createForm,getForm,updateForm); extracting it once avoids the fields drifting out of sync in the future (e.g. Line 129 already has a stray trailing space vs. the other two).♻️ Proposed refactor: extract shared select object
+const formSelect = { + id: true, title: true, description: true, iconSymbol: true, + coverUrl: true, published: true, slug: true, type: true, + requireEmail: true, responseCount: true, viewCount: true, + fields: true, createdAt: true, updatedAt: true, userId: true, +} as const; + export const createForm: RequestHandler = asyncHandler( async (req: Request, res: Response) => { ... const form = await prisma.form.create({ data: { ... }, - select: { - id: true, title: true, description: true, iconSymbol: true, - coverUrl: true, published: true, slug: true, type: true, - requireEmail: true, responseCount: true, viewCount: true, - fields: true, createdAt: true, updatedAt: true, userId: true, - }, + select: formSelect, });Apply the same
select: formSelectsubstitution ingetFormandupdateForm.Also applies to: 125-131, 201-206
🤖 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 `@apps/api/src/controllers/form.controller.ts` around lines 92 - 97, The `select` shape in `createForm`, `getForm`, and `updateForm` is duplicated and should be centralized to keep the returned form fields consistent. Extract the repeated object into a shared `formSelect` constant in `FormController`, then replace each inline `select` with `formSelect` in those methods so future field changes only need to be made in one place.
🤖 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 `@apps/api/src/controllers/template.controller.ts`:
- Around line 289-298: The pagination response shape in getCommunityTemplates is
inconsistent with the sibling template endpoint because it uses meta instead of
pagination. Update the response object in template.controller so
getCommunityTemplates returns the same pagination key name as getTemplateById,
keeping the API contract consistent across both handlers.
In `@packages/api-tests/integration-tests/modules/auth.test.ts`:
- Around line 7-11: Replace the manual globalThis.fetch capture and
__originalFetch flag with bun:test’s native mocking APIs in auth.test.ts. Update
the fetch setup around the existing originalFetch usage to use spyOn(globalThis,
"fetch") with mockImplementation and mockRestore instead of mutating a global
sentinel, and apply the same pattern throughout the affected auth test cases so
mocking/restoration is handled by bun:test rather than module-load ordering.
- Around line 70-91: The test description and request type are inconsistent: the
inline note says a direct GET call, but auth initiation is currently exercised
with client.post on the social sign-in flow. Update the test in auth.test.ts so
it matches the intended behavior by either changing the comment to describe the
POST JSON flow or, if the acceptance criteria require it, switching to the
actual GET-based initiation route and validating the redirect URL there. Use the
existing test block around the sign-in/social request and the redirectUrl/state
assertions to keep the intent clear.
---
Outside diff comments:
In `@apps/api/src/controllers/form.controller.ts`:
- Around line 92-97: The `select` shape in `createForm`, `getForm`, and
`updateForm` is duplicated and should be centralized to keep the returned form
fields consistent. Extract the repeated object into a shared `formSelect`
constant in `FormController`, then replace each inline `select` with
`formSelect` in those methods so future field changes only need to be made in
one place.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33cb3404-29aa-47e7-a649-4a8bb376d587
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignoreapps/api/src/controllers/form.controller.tsapps/api/src/controllers/template.controller.tspackages/api-tests/integration-tests/modules/auth.test.tspackages/api-tests/integration-tests/setup/bootstrap.ts
📜 Review details
🔇 Additional comments (6)
.gitignore (1)
42-43: LGTM!apps/api/src/controllers/template.controller.ts (2)
152-152: LGTM!Also applies to: 499-501
263-284: 🗄️ Data Integrity & IntegrationConfirm no client relies on rating data from
GET /api/v1/templates/community. This response now returns only template summary fields; any UI or integration expectingreviews,avgStars, orreviewCountwill need a separate endpoint or expanded payload.packages/api-tests/integration-tests/modules/auth.test.ts (2)
137-143: 🎯 Functional Correctness | ⚡ Quick winInconsistent header usage vs. earlier requests in the same flow.
Lines 75-79 and 105-109 spoof
Host/X-Forwarded-Host/X-Forwarded-Proto: localhost:3000, but the custom callback requests here omit these headers entirely. If the app route relies on forwarded headers to resolve the correct base URL for the onboarding/dashboard redirect target (as it apparently does for the better-auth callback), omitting them here could produce flaky or environment-dependent redirect assertions.Also applies to: 156-162
1-167: 🩺 Stability & AvailabilityNo change needed.
globalThis.fetchis shared with the in-process test server, so the GitHub calls will hit this mock.packages/api-tests/integration-tests/setup/bootstrap.ts (1)
36-43: LGTM!
…o pagination in template controller response
Basharkhan7776
left a comment
Basharkhan7776
left a comment
There was a problem hiding this comment.
I think it should give me the link on terminal to login in then store token as const use that session for further testing. What do you think?
|
Since these are automated integration tests designed to run in our CI/CD pipeline, they need to run headlessly without human intervention. Pausing the terminal to wait for a manual browser login would cause our CI pipelines ( GitHub Actions) to hang and fail. Instead, I've mocked the external GitHub OAuth HTTP calls and simulated the callback flow. This ensures the tests are fast, deterministic, and can run completely autonomously while still fully exercising our backend's session creation logic. |
|
Okay I understood, but the direct fetching testing is better to analyze the problems and second thing is that if we test any model then we push. In production we have 2 environment 1st is production(main) and development for testing, we won't deploy every PR, we test the stability then go to main. Also issue is about to make a direct call in testing. |
|
I completely agree that testing real, direct calls is the best way to ensure stability before deploying to development/main! Just to clarify how the tests are currently set up:
If the goal of the issue was to create a script for manual QA testing in the terminal (where a developer clicks a link, logs in, and the terminal captures the token), I can definitely write a separate Node CLI script for that! But for the automated CI test suite, we have to keep it headless. Let me know which direction you'd prefer, also if necessary we can create issue for that. |
2 participants
Fix: Index & Auth Route Integration Tests
Description
Closes #50
Added integration tests to ensure the reliability of the index and authentication routing flows.
Changes Made