refactor: extract API business logic into dedicated service modules#569
Merged
knoxiboy merged 21 commits intoJul 24, 2026
Merged
Conversation
This commit extracts complex query building, LLM orchestration, and moderation logic out of route handlers and into testable, dependency-injected services. Implemented using TDD Red-Green-Refactor.
|
CodeAnt AI is reviewing your PR. |
|
@Mohammedsami001 is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts complex route business logic into dedicated service modules (AI solver, analytics, doubts), extends ApiError with optional codes and a ServiceError class, adds DB tables/columns for badges/karma/confusion/drift, thins routes to auth/validation/delegate, and adds/updates unit tests and a service worker. ChangesService Layer Extraction and Database Enhancements
Sequence Diagram(s)sequenceDiagram
participant Client
participant AskAIRoute as ask-ai route
participant generateAISolution
participant callGroqWithFallback
participant Database
Client->>AskAIRoute: POST /api/ask-ai
AskAIRoute->>AskAIRoute: requireAuth(), validate payload
AskAIRoute->>generateAISolution: generateAISolution(db, params)
generateAISolution->>callGroqWithFallback: call vision or text models
callGroqWithFallback-->>generateAISolution: reply with SUBJECT header
generateAISolution->>Database: insert doubt and reply records
generateAISolution-->>AskAIRoute: {reply, subject}
AskAIRoute-->>Client: 200 JSON response
sequenceDiagram
participant Client
participant AnalyticsRoute as analytics route
participant getDashboardAnalytics
participant Database
Client->>AnalyticsRoute: GET /api/analytics?classroomId=N
AnalyticsRoute->>AnalyticsRoute: requireAuth()
AnalyticsRoute->>getDashboardAnalytics: getDashboardAnalytics(db, {email, classroomId})
getDashboardAnalytics->>getDashboardAnalytics: checkUserBlock, verify membership
getDashboardAnalytics->>Database: execute parallel analytics queries
Database-->>getDashboardAnalytics: aggregated results
getDashboardAnalytics->>getDashboardAnalytics: transform weak topics, fetch settings
getDashboardAnalytics-->>AnalyticsRoute: aggregated analytics object
AnalyticsRoute-->>Client: 200 JSON response
sequenceDiagram
participant Client
participant DoubtsRoute as doubts route
participant getDoubts
participant createDoubt
participant Database
Client->>DoubtsRoute: GET /api/doubts?subject=Math&page=1
DoubtsRoute->>getDoubts: getDoubts(db, {email, subject, page, ...})
getDoubts->>Database: Drizzle query with filters, joins, pagination
Database-->>getDoubts: filtered doubt list
getDoubts->>getDoubts: enrich with likes, bookmarks, tags
getDoubts-->>DoubtsRoute: doubt array
DoubtsRoute-->>Client: 200 JSON response
Client->>DoubtsRoute: POST /api/doubts {content, subject, classroomId}
DoubtsRoute->>createDoubt: createDoubt(db, {email, content, subject, ...})
createDoubt->>createDoubt: checkUserBlock, moderate, categorize
createDoubt->>Database: insert doubt, persist tags, trigger notifications
createDoubt-->>DoubtsRoute: created doubt with tags
DoubtsRoute-->>Client: 201 JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
gssoc'26
|
@coderabbitai review |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
src/lib/errors.ts (1)
3-8: 💤 Low valueInconsistent parameter order vs.
ApiError/createApiErroris a footgun.
ServiceErrororders its optional args as(code?, details?)whileApiErrorandcreateApiErroruse(details?, code?). Thesuper(...)call correctly remaps them, so there's no bug today, but the divergence is easy to mix up when callers move between the two APIs. Consider aligning the orders (or documenting the intentional difference) to reduce the chance of swappedcode/detailsarguments later.🤖 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/lib/errors.ts` around lines 3 - 8, ServiceError's constructor parameter order (code?, details?) differs from ApiError and createApiError (details?, code?), which is confusing; update ServiceError to use the same optional argument order as ApiError/createApiError by changing its signature to accept details?: unknown then code?: string and call super(statusCode, message, details, code) so callers are consistent with ApiError and createApiError (references: ServiceError, ApiError, createApiError).src/services/analytics.service.ts (1)
3-3: ⚡ Quick winUnused import:
isNull.
isNullis imported but not used in this file. Remove it to reduce noise.♻️ Suggested fix
-import { desc, sql, and, isNull, eq, count, countDistinct, ne, inArray } from 'drizzle-orm'; +import { desc, sql, and, eq, count, countDistinct, ne, inArray } from 'drizzle-orm';🤖 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/services/analytics.service.ts` at line 3, The import list in analytics.service.ts currently includes an unused symbol isNull; remove isNull from the named imports on the import line that imports from 'drizzle-orm' (the line containing desc, sql, and, isNull, eq, count, countDistinct, ne, inArray) so the file only imports symbols actually used, then save and run linters/formatters to ensure the import list is clean.src/services/doubt.service.ts (2)
199-204: ⚡ Quick winMove imports to the top of the file.
These imports are placed mid-file after the
getDoubtsfunction. While functionally valid in ESM, this is unconventional and reduces readability. Consolidate all imports at the top.♻️ Suggested refactor
Move these imports to join the others at lines 1-12:
import { ServiceError } from "`@/lib/errors`"; import { bookmarksTable, ... } from "`@/configs/schema`"; import { and, eq, inArray, isNull, or, not, sql, SQL, ilike, desc, getTableColumns } from "drizzle-orm"; +import { categorizeDoubt } from "`@/lib/ai/categorizer`"; +import { moderateContent, handleModerationViolation } from "`@/lib/moderation`"; +import { checkUserBlock } from "`@/lib/auth-utils`"; +import { createClassroomDoubtNotifications } from "`@/lib/notifications/service`"; +import { inngest } from "`@/inngest/client`"; export interface GetDoubtsParams {Then remove lines 199-204.
🤖 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/services/doubt.service.ts` around lines 199 - 204, Move the mid-file import block (categorizeDoubt, moderateContent, handleModerationViolation, checkUserBlock, createClassroomDoubtNotifications, inngest) to the top alongside the existing imports: consolidate these symbols into the initial import section and delete the duplicate import lines currently after getDoubts; ensure no name collisions or unused imports remain and run a quick build/linter to confirm import order and formatting are ok.
307-314: 💤 Low valueTitle-casing may produce unexpected results for acronyms/abbreviations.
Line 309 title-cases the display name (e.g.,
"css"→"Css","api"→"Api"). Consider whether preserving user-provided casing for the display name is preferable, while still normalizing the lookup key.💡 Alternative: preserve original casing
If you want to preserve user casing, store the first occurrence's original form:
- name: name.replace(/\b\w/g, (char) => char.toUpperCase()), + name: tags.find(t => t.trim().replace(/\s+/g, " ").toLowerCase() === name) || name,🤖 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/services/doubt.service.ts` around lines 307 - 314, The current tagsToInsert entry title-cases the tag display via name.replace(...), which mangles acronyms (e.g., "API" → "Api"); update the logic in the block that pushes into tagsToInsert so the display name preserves the original user-provided casing (use the raw name for the display field) while still writing a normalized lookup key (e.g., set normalizedName to a trimmed lowercased form like name.toLowerCase().trim()); adjust references to the name/display fields accordingly in the surrounding code that reads tags (e.g., where normalizedName is used for deduplication/lookups) to ensure uniqueness is based on normalizedName and not the display text.src/services/ai-solver.service.ts (3)
146-151: ⚡ Quick winBlock-check logic is duplicated; consider using the shared utility.
Other services (
analytics.service.ts,doubt.service.ts) usecheckUserBlock(email)from@/lib/auth-utils. This inline query duplicates that logic and risks divergence (e.g., the utility also handles query errors gracefully).For consistency, consider delegating to the shared helper.
♻️ Suggested refactor
+import { checkUserBlock } from '`@/lib/auth-utils`'; ... - // Check blocked status - const [dbUser] = await db.select().from(usersTable).where(eq(usersTable.email, email)); - if (dbUser?.blockedUntil && new Date(dbUser.blockedUntil) > new Date()) { - const unlockDate = new Date(dbUser.blockedUntil).toDateString(); - throw new ServiceError(403, `Your account is temporarily blocked due to safety violations. Access will be restored on ${unlockDate}.`); - } + const { isBlocked, dbUser } = await checkUserBlock(email); + if (isBlocked) { + const unlockDate = new Date(dbUser?.blockedUntil).toDateString(); + throw new ServiceError(403, `Your account is temporarily blocked due to safety violations. Access will be restored on ${unlockDate}.`); + }Note: This would require
checkUserBlockto accept an injecteddbparameter to maintain the DI pattern, or you could keep the inline approach if you want full DI control in this service.🤖 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/services/ai-solver.service.ts` around lines 146 - 151, The inline block-check in ai-solver.service.ts duplicates logic in the shared utility; replace the manual query that reads usersTable and throws ServiceError with a call to the shared checkUserBlock helper from "`@/lib/auth-utils`". If checkUserBlock currently does not accept a DB instance, update the helper to accept an injected db parameter (e.g., checkUserBlock(email, db)) and update other callers (analytics.service.ts, doubt.service.ts) to pass the db, then call checkUserBlock(email, db) here instead of repeating the query/throw logic so behavior and error handling remain consistent.
9-11: 💤 Low valueConsider failing fast when
GROQ_API_KEYis missing.Falling back to
'dummy_key'will allow the service to initialize but cause cryptic authentication failures at runtime. Consider throwing an error at startup or emitting a warning log so misconfiguration is surfaced early.💡 Suggested improvement
const groq = new Groq({ - apiKey: process.env.GROQ_API_KEY || 'dummy_key', + apiKey: process.env.GROQ_API_KEY, }); + +if (!process.env.GROQ_API_KEY) { + console.warn('GROQ_API_KEY is not set - AI solver requests will fail'); +}🤖 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/services/ai-solver.service.ts` around lines 9 - 11, The code currently instantiates Groq with a fallback 'dummy_key', which masks missing configuration and leads to runtime auth failures; update the startup logic to read process.env.GROQ_API_KEY and if it's undefined or empty, throw an Error (or call process.exit after logging) so the service fails fast instead of using 'dummy_key', and then pass the validated API key into the Groq constructor (references: Groq, groq).
360-362: 💤 Low valueSilently swallowing persistence errors may hide data-loss bugs.
The
try/catcharound doubt/reply persistence logs the error but does not propagate it. If persistence fails, the client receives a successful response with the AI reply, but no record is saved. Consider whether this silent degradation is intentional; if not, you may want to surface a warning in the response or rethrow.🤖 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/services/ai-solver.service.ts` around lines 360 - 362, The catch block that logs "Failed to fully persist AI doubt and solution turn 1:" is silently swallowing persistence failures; update that catch in src/services/ai-solver.service.ts so the persistence error is surfaced: either rethrow the error (include dbErr) so upstream can return an error, or add a warning/metadata to the AI response (e.g., persisted: false and error message) so the client is informed; ensure the change is made where the persistence logic for the AI doubt/reply runs (the try/catch handling dbErr) and include the dbErr details in the propagated error/warning.src/__tests__/services/analytics.service.test.ts (1)
2-2: ⚡ Quick winUnused import:
authUtils.The namespace import
* as authUtilsis not referenced after the mock setup. Remove it.♻️ Suggested fix
import { getDashboardAnalytics } from "`@/services/analytics.service`"; -import * as authUtils from "`@/lib/auth-utils`";🤖 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/__tests__/services/analytics.service.test.ts` at line 2, Remove the unused namespace import authUtils from the test file; the import line "import * as authUtils from '`@/lib/auth-utils`'" is not referenced after mocks, so delete that import and keep any existing jest.mock or mock setup that targets "`@/lib/auth-utils`" intact (no other changes to test logic required).src/app/api/doubts/route.ts (1)
4-5: ⚡ Quick winUnused imports:
membershipsTable,eq,and.These imports are no longer used after moving the membership checks to the service layer.
-import { membershipsTable } from "`@/configs/schema`"; -import { eq, and } from "drizzle-orm";🤖 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/app/api/doubts/route.ts` around lines 4 - 5, Remove the now-unused imports from the route file: delete membershipsTable, eq, and, which are no longer referenced after moving membership checks to the service layer; specifically remove the named imports membershipsTable from "`@/configs/schema`" and eq, and from "drizzle-orm" in src/app/api/doubts/route.ts so the file only imports what it actually uses.src/app/api/ask-ai/route.ts (1)
50-55: 💤 Low valueRedundant
error.codeassignment.
buildErrorResponsealready extracts and includescodefrom errors viasanitizeError. The manual assignment on line 54 is redundant.Suggested simplification
const { status, body } = buildErrorResponse(error); - - // Ensure code is passed to client if ServiceError threw it - if (error.code) { - body.code = error.code; - } return NextResponse.json(body, { status });🤖 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/app/api/ask-ai/route.ts` around lines 50 - 55, The manual assignment that copies error.code into body.code is redundant because buildErrorResponse (via sanitizeError) already includes the error code; remove the conditional block that checks error.code and assigns body.code (the if (error.code) { body.code = error.code; } lines) from route.ts so the handler relies solely on buildErrorResponse to populate the response code and avoid duplicate logic.
🤖 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 `@src/__tests__/services/doubt.service.test.ts`:
- Around line 1-2: The test imports getDoubts which causes a transitive import
of the real DB module "`@/configs/db`" and that triggers a real DB init leading to
"query" undefined; fix by adding a Jest mock for "`@/configs/db`" at the top of
the test file before importing getDoubts (e.g., jest.mock('`@/configs/db`', () =>
({ query: jest.fn(), /* any other exported db helpers */ }))) so the service
uses the mocked DB during tests; ensure the mock is declared above the existing
import of getDoubts in this test file.
- Around line 19-27: The test fails because getDoubts checks classroom
membership when params.classroomId is present and your mock DB returns an empty
array for the memberships query; either remove classroomId from params to
exercise the public/community path, or update the mock so the membership query
returns a valid record. Specifically, adjust the mock for
db.select().from(membershipsTable).where(...) used by getDoubts to return a
non-empty membership (e.g., a membership object with the expected classroomId
and email) before the rest of the query chain is evaluated so the membership
check passes.
In `@src/app/api/analytics/route.ts`:
- Around line 10-11: The parsed classroomId may be NaN for invalid inputs (e.g.,
classroomIdStr = "abc"); update the route handler around
classroomIdStr/classroomId to validate the parseInt result: after parsing, check
Number.isNaN(classroomId) (or Number.isInteger) and if it is invalid, return a
400 Bad Request (or explicitly set classroomId = null before calling the
service) instead of passing NaN into the service/SQL layer; adjust the code that
calls the analytics service to only pass a validated integer or null.
In `@src/app/api/doubts/route.ts`:
- Around line 22-25: The page and limit parsing (pageStr, limitStr -> parseInt
to page and limit) can yield NaN for invalid inputs; update the logic in
src/app/api/doubts/route.ts so that after calling parseInt you validate the
results and fall back to safe defaults (page = 1, limit = 20) when parseInt
returns NaN or produces non-positive values; implement this check for both page
and limit before passing them to the service so only valid integers are used.
In `@src/services/ai-solver.service.ts`:
- Around line 321-333: The insert truncates the base64 payload via
imageBase64?.slice(0, 500) in the db.insert(doubtsTable) call (newDoubt), which
corrupts images; remove the slice and either store the full imageBase64 value or
set imageUrl to null for oversized payloads, or better: upload the image to
object storage and store the returned URL in imageUrl (and ensure the doubts
table column can accept the chosen length/type).
In `@src/services/analytics.service.ts`:
- Around line 228-231: The spread of engagement[0] can throw if engagement is an
empty array; update the assembly of the engagement object by defensively using a
default when engagement[0] is missing (e.g., use engagement[0] || {} or check
engagement.length) so the spread won't fail and ensure totalReplies uses
totalReplies[0]?.count || 0 as before; modify the code around the engagement
creation in analytics.service.ts to apply this guard for the engagement
variable.
---
Nitpick comments:
In `@src/__tests__/services/analytics.service.test.ts`:
- Line 2: Remove the unused namespace import authUtils from the test file; the
import line "import * as authUtils from '`@/lib/auth-utils`'" is not referenced
after mocks, so delete that import and keep any existing jest.mock or mock setup
that targets "`@/lib/auth-utils`" intact (no other changes to test logic
required).
In `@src/app/api/ask-ai/route.ts`:
- Around line 50-55: The manual assignment that copies error.code into body.code
is redundant because buildErrorResponse (via sanitizeError) already includes the
error code; remove the conditional block that checks error.code and assigns
body.code (the if (error.code) { body.code = error.code; } lines) from route.ts
so the handler relies solely on buildErrorResponse to populate the response code
and avoid duplicate logic.
In `@src/app/api/doubts/route.ts`:
- Around line 4-5: Remove the now-unused imports from the route file: delete
membershipsTable, eq, and, which are no longer referenced after moving
membership checks to the service layer; specifically remove the named imports
membershipsTable from "`@/configs/schema`" and eq, and from "drizzle-orm" in
src/app/api/doubts/route.ts so the file only imports what it actually uses.
In `@src/lib/errors.ts`:
- Around line 3-8: ServiceError's constructor parameter order (code?, details?)
differs from ApiError and createApiError (details?, code?), which is confusing;
update ServiceError to use the same optional argument order as
ApiError/createApiError by changing its signature to accept details?: unknown
then code?: string and call super(statusCode, message, details, code) so callers
are consistent with ApiError and createApiError (references: ServiceError,
ApiError, createApiError).
In `@src/services/ai-solver.service.ts`:
- Around line 146-151: The inline block-check in ai-solver.service.ts duplicates
logic in the shared utility; replace the manual query that reads usersTable and
throws ServiceError with a call to the shared checkUserBlock helper from
"`@/lib/auth-utils`". If checkUserBlock currently does not accept a DB instance,
update the helper to accept an injected db parameter (e.g.,
checkUserBlock(email, db)) and update other callers (analytics.service.ts,
doubt.service.ts) to pass the db, then call checkUserBlock(email, db) here
instead of repeating the query/throw logic so behavior and error handling remain
consistent.
- Around line 9-11: The code currently instantiates Groq with a fallback
'dummy_key', which masks missing configuration and leads to runtime auth
failures; update the startup logic to read process.env.GROQ_API_KEY and if it's
undefined or empty, throw an Error (or call process.exit after logging) so the
service fails fast instead of using 'dummy_key', and then pass the validated API
key into the Groq constructor (references: Groq, groq).
- Around line 360-362: The catch block that logs "Failed to fully persist AI
doubt and solution turn 1:" is silently swallowing persistence failures; update
that catch in src/services/ai-solver.service.ts so the persistence error is
surfaced: either rethrow the error (include dbErr) so upstream can return an
error, or add a warning/metadata to the AI response (e.g., persisted: false and
error message) so the client is informed; ensure the change is made where the
persistence logic for the AI doubt/reply runs (the try/catch handling dbErr) and
include the dbErr details in the propagated error/warning.
In `@src/services/analytics.service.ts`:
- Line 3: The import list in analytics.service.ts currently includes an unused
symbol isNull; remove isNull from the named imports on the import line that
imports from 'drizzle-orm' (the line containing desc, sql, and, isNull, eq,
count, countDistinct, ne, inArray) so the file only imports symbols actually
used, then save and run linters/formatters to ensure the import list is clean.
In `@src/services/doubt.service.ts`:
- Around line 199-204: Move the mid-file import block (categorizeDoubt,
moderateContent, handleModerationViolation, checkUserBlock,
createClassroomDoubtNotifications, inngest) to the top alongside the existing
imports: consolidate these symbols into the initial import section and delete
the duplicate import lines currently after getDoubts; ensure no name collisions
or unused imports remain and run a quick build/linter to confirm import order
and formatting are ok.
- Around line 307-314: The current tagsToInsert entry title-cases the tag
display via name.replace(...), which mangles acronyms (e.g., "API" → "Api");
update the logic in the block that pushes into tagsToInsert so the display name
preserves the original user-provided casing (use the raw name for the display
field) while still writing a normalized lookup key (e.g., set normalizedName to
a trimmed lowercased form like name.toLowerCase().trim()); adjust references to
the name/display fields accordingly in the surrounding code that reads tags
(e.g., where normalizedName is used for deduplication/lookups) to ensure
uniqueness is based on normalizedName and not the display text.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4225f93a-b6e8-4ebe-9178-1b6a4cac8e7c
📒 Files selected for processing (13)
.gitignoresrc/__tests__/services/ai-solver.service.test.tssrc/__tests__/services/analytics.service.test.tssrc/__tests__/services/doubt.service.test.tssrc/app/api/analytics/route.tssrc/app/api/ask-ai/route.tssrc/app/api/doubts/route.tssrc/lib/error-handler.tssrc/lib/errors.tssrc/services/ai-solver.service.tssrc/services/analytics.service.tssrc/services/doubt.service.tssrc/services/room.service.ts
💤 Files with no reviewable changes (1)
- .gitignore
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/__tests__/api/doubts.test.ts (1)
104-115: 💤 Low valueConsider completing the mock sorting logic to match the service contract.
The service (as shown in the relevant code snippets) always sorts by
isPinneddescending first, then bylikesorcountdepending on mode, and finally bycreatedAtdescending for "newest" mode. The mock currently only sorts bylikesorcountand omitsisPinnedandcreatedAtsorting.While this doesn't affect current tests (all mock doubts have
isPinned: falseand no test verifiescreatedAtordering), adding complete sorting would make the mock more robust for future tests.📋 Optional: Complete mock sorting logic
then: (resolve: any) => { let internalData = [...data]; if (mockFilterMode === 'unsolved') { internalData = internalData.filter((d: any) => d.isSolved === 'unsolved'); } + // Always sort by isPinned first (matches service behavior) + internalData.sort((a: any, b: any) => (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0)); + if (mockSortMode === 'popular') { - internalData.sort((a: any, b: any) => b.likes - a.likes); + internalData = internalData.sort((a: any, b: any) => + (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0) || b.likes - a.likes || + new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() + ); } else if (mockSortMode === 'most-replied') { - internalData.sort((a: any, b: any) => b.count - a.count); + internalData = internalData.sort((a: any, b: any) => + (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0) || b.count - a.count || + new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() + ); + } else { + // "newest" mode: sort by createdAt descending + internalData = internalData.sort((a: any, b: any) => + (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0) || + new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() + ); } return Promise.resolve(resolve(internalData)); },🤖 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/__tests__/api/doubts.test.ts` around lines 104 - 115, The mock sorting in the Promise resolver is incomplete: update the logic inside the then resolver (where internalData, mockFilterMode, and mockSortMode are used) to first sort by isPinned descending, then by likes (when mockSortMode === 'popular') or by count (when mockSortMode === 'most-replied'), and when mockSortMode === 'newest' sort by createdAt descending; ensure the comparator chains these criteria so pinned items come first, then the selected secondary key, then createdAt as a tie-breaker to match the service contract.src/__tests__/api/rooms-members.test.ts (1)
93-104: ⚡ Quick winKeep asserting the pagination contract in these route tests.
These expectations now validate only
json.members, but the page still depends onjson.pagination.totalPages. A regression in the route’s pagination metadata would pass both tests as written.Suggested assertion to keep coverage on the response contract
expect(json.members).toEqual([ // ... ]); +expect(json.pagination).toEqual({ + total: 2, + page: 1, + limit: 20, + totalPages: 1, +});Also applies to: 137-148
🤖 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/__tests__/api/rooms-members.test.ts` around lines 93 - 104, The test currently asserts only json.members but must also assert the pagination contract; update the assertions in the blocks that reference json.members (the one containing displayName Student_1/Student_2 and the similar block at lines 137-148) to include checks on json.pagination.totalPages (and optionally json.pagination.page or json.pagination.totalItems) so the route's pagination metadata is verified along with the members array.
🤖 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 `@src/app/api/rooms/members/route.ts`:
- Around line 76-88: The response currently replaces the members shape for
non-privileged callers (creating displayName and exposing member.id) which
breaks consumers expecting { userEmail, role, joinedAt }; update the mapping in
the safeMembers branch (where canViewMemberEmails is false) to preserve the
original keys by returning { userEmail, role, joinedAt } and set userEmail to a
pseudonym (e.g. 'Student' or 'Member' or a deterministic anonymized string)
instead of exposing member.id or adding displayName; keep the privileged branch
that strips email intact and return the JSON with members: safeMembers as
before.
---
Nitpick comments:
In `@src/__tests__/api/doubts.test.ts`:
- Around line 104-115: The mock sorting in the Promise resolver is incomplete:
update the logic inside the then resolver (where internalData, mockFilterMode,
and mockSortMode are used) to first sort by isPinned descending, then by likes
(when mockSortMode === 'popular') or by count (when mockSortMode ===
'most-replied'), and when mockSortMode === 'newest' sort by createdAt
descending; ensure the comparator chains these criteria so pinned items come
first, then the selected secondary key, then createdAt as a tie-breaker to match
the service contract.
In `@src/__tests__/api/rooms-members.test.ts`:
- Around line 93-104: The test currently asserts only json.members but must also
assert the pagination contract; update the assertions in the blocks that
reference json.members (the one containing displayName Student_1/Student_2 and
the similar block at lines 137-148) to include checks on
json.pagination.totalPages (and optionally json.pagination.page or
json.pagination.totalItems) so the route's pagination metadata is verified along
with the members array.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7cde2ba3-96e1-440d-b298-1d7b4a076619
📒 Files selected for processing (4)
src/__tests__/api/doubts.test.tssrc/__tests__/api/rooms-members.test.tssrc/app/api/rooms/members/route.tssrc/configs/db.tsx
✅ Files skipped from review due to trivial changes (1)
- src/configs/db.tsx
Contributor
Author
|
Hey @knoxiboy 👋 I've resolved all the failing test suites in the CI pipeline! The issues were related to some missing global database mocks during the execution of the isolated unit tests for our new services. Everything is fully passing now (100% green). The architecture refactor to extract the API business logic into the service layer is complete and stable. It's ready for your final review and merge! |
github-actions
Bot
removed
quality:exceptional
github-actions
Bot
added
gssoc'26
github-actions
Bot
added
merge-conflict
knoxiboy
added
level:advanced
github-actions
Bot
added
merge-conflict
github-actions
Bot
removed
backend
17 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
2 participants
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.
User description
Description
This PR introduces a major architectural refactor to the backend by implementing a dedicated Service Layer. Previously, our API route handlers (
src/app/api/...) were "fat" and responsible for everything: parsing requests, validating payloads, executing complex SQL queries, handling third-party integrations (like Groq), and formatting responses. This made the business logic impossible to unit test without mocking the entire Next.js request lifecycle.By extracting the core logic into dedicated, modular services, the route handlers are now "thin" wrappers that simply manage HTTP concerns, while the services handle the heavy lifting.
🏗️ Architectural Changes
dbclient as a parameter (executeService(db, params)). This decouples the business logic from the global database configuration, allowing us to easily inject mock databases during unit testing.ServiceErrorclass insrc/lib/errors.tsthat extends the globalApiError.buildErrorResponseutility insrc/lib/error-handler.tsto support passing customcodestrings (e.g.,IMAGE_QUALITY_LOW) to the frontend for precise client-side error handling.🧩 Service Modules Extracted
1. Doubt Service (
src/services/doubt.service.ts)src/app/api/doubts/route.tsgetDoubts) and submitting new doubts (createDoubt) was completely decoupled from the route. The service now handles data normalization, authorization checks, and safe Drizzle ORM queries independently.2. Analytics Service (
src/services/analytics.service.ts)src/app/api/analytics/route.tsgetDashboardAnalytics.weakTopicsand performance trends based on historical doubt resolution data is now safely encapsulated and fully testable.3. AI-Solver Service (
src/services/ai-solver.service.ts)src/app/api/ask-ai/route.tsgenerateAISolution.callGroqWithFallbackorchestrator, which ensures high availability by rolling over to fallback models if the primary vision/text models timeout or rate-limit.checkPedagogicalDrift), which guarantees AI responses match the target grade level of the student's classroom.🧪 Testing
src/__tests__/services/).Related Issue
Closes #538
Type of Change
How Has This Been Tested?
npm run devai-solver.service.test.ts,analytics.service.test.ts,doubt.service.test.ts)Checklist
npm run dev)anytypes)mainSummary by CodeRabbit
New Features
Improvements
CodeAnt-AI Description
Split API logic into services and return clearer AI and access errors
What Changed
Impact
✅ Clearer AI failure messages✅ Fewer confusing classroom access errors✅ Better handling of blocked accounts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.