Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/__tests__/api/doubts-check-duplicate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,42 @@ describe("Doubt check-duplicate API endpoint", () => {
await expect(res.json()).resolves.toMatchObject({ error: "Unauthorized" });
});

it("excludes hidden doubts from candidate query", async () => {
let capturedWhereArg: any = null;
const trackingQuery: any = {
from: () => trackingQuery,
where: (arg: any) => {
capturedWhereArg = arg;
return trackingQuery;
},
orderBy: () => trackingQuery,
limit: () => trackingQuery,
then: (resolve: any) => Promise.resolve(resolve([])),
};
dbSelectMock.mockReturnValue(trackingQuery);
Comment on lines +137 to +148

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The new test only reaches the fallback query: the embedding mock is never configured with a valid 1536-dimensional vector, so findSemanticDuplicates returns an empty result before its database query is executed. A regression removing the hidden predicate from the primary vector-search path would therefore still pass this test. Configure the embedding response and capture/assert the vector query separately. [code quality]

Severity Level: Major ⚠️
- ⚠️ Primary vector-search regressions remain undetected.
- ❌ Hidden doubts could reappear in duplicate results.
- ⚠️ AskDoubt displays returned matches at `src/components/classroom/AskDoubt.tsx:701-710`.

Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/__tests__/api/doubts-check-duplicate.test.ts
**Line:** 137:148
**Comment:**
	*Code Quality: The new test only reaches the fallback query: the embedding mock is never configured with a valid 1536-dimensional vector, so `findSemanticDuplicates` returns an empty result before its database query is executed. A regression removing the hidden predicate from the primary vector-search path would therefore still pass this test. Configure the embedding response and capture/assert the vector query separately.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


const req = new Request("http://localhost/api/doubts/check-duplicate", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
content: "How does photosynthesis convert light into energy?",
}),
});

const res = await POST(req);

expect(res.status).toBe(200);
expect(capturedWhereArg).not.toBeNull();
const hasIsHidden = (expr: any): boolean => {
if (!expr || typeof expr !== "object") return false;
if (expr.name === "isHidden") return true;
if (expr.queryChunks)
return expr.queryChunks.some((c: any) => hasIsHidden(c));
return false;
};
expect(hasIsHidden(capturedWhereArg)).toBe(true);
});

it("succeeds for authenticated classroom duplicate checks", async () => {
const requireMembershipMock = requireMembership as jest.MockedFunction<typeof requireMembership>;
requireMembershipMock.mockResolvedValue({ role: "student" });
Expand Down
51 changes: 51 additions & 0 deletions src/__tests__/api/doubts-similarity.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
process.env.GROQ_API_KEY = "mock-groq-key";

jest.mock("@/lib/ai/groq-client", () => ({
groq: {
embeddings: {
create: jest.fn(),
},
chat: {
completions: {
create: jest.fn(),
},
},
},
}));

import { currentUser } from "@clerk/nextjs/server";
import { NextResponse } from "next/server";

Expand Down Expand Up @@ -170,6 +185,42 @@ describe("Doubt similarity API endpoint", () => {
});
});

it("excludes hidden doubts from candidate query", async () => {
let capturedWhereArg: any = null;
const trackingQuery: any = {
from: () => trackingQuery,
where: (arg: any) => {
capturedWhereArg = arg;
return trackingQuery;
},
orderBy: () => trackingQuery,
limit: () => trackingQuery,
then: (resolve: any) => Promise.resolve(resolve([])),
};
dbSelectMock.mockReturnValue(trackingQuery);
Comment on lines +189 to +200

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The new test also bypasses the primary semantic-search path because the mocked embedding call has no successful vector response and the tracking query resolves to an empty list. Consequently, the assertion verifies only the LLM fallback query and cannot catch hidden doubts returned by findSemanticDuplicates. Configure a valid embedding response and assert both candidate-query paths. [code quality]

Severity Level: Major ⚠️
- ⚠️ Semantic-search regressions remain undetected.
- ❌ Hidden doubts could surface in similarity results.
- ⚠️ Similarity results are rendered by `AskDoubt`.

Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/__tests__/api/doubts-similarity.test.ts
**Line:** 189:200
**Comment:**
	*Code Quality: The new test also bypasses the primary semantic-search path because the mocked embedding call has no successful vector response and the tracking query resolves to an empty list. Consequently, the assertion verifies only the LLM fallback query and cannot catch hidden doubts returned by `findSemanticDuplicates`. Configure a valid embedding response and assert both candidate-query paths.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


const req = new Request("http://localhost/api/doubts/check-similarity", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
content: "How does photosynthesis convert light into energy?",
}),
});

const res = await POST(req);

expect(res.status).toBe(200);
expect(capturedWhereArg).not.toBeNull();
const hasIsHidden = (expr: any): boolean => {
if (!expr || typeof expr !== "object") return false;
if (expr.name === "isHidden") return true;
if (expr.queryChunks)
return expr.queryChunks.some((c: any) => hasIsHidden(c));
return false;
};
expect(hasIsHidden(capturedWhereArg)).toBe(true);
});

it("requires authentication for classroom similarity checks", async () => {
const req = new Request("http://localhost/api/doubts/check-similarity", {
method: "POST",
Expand Down
29 changes: 29 additions & 0 deletions src/__tests__/lib/embeddings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,33 @@ describe("Embeddings & Vector Duplicate Utilities", () => {
const duplicates = await findSemanticDuplicates({ content: "What is momentum?" });
expect(duplicates).toEqual([]);
});

it("findSemanticDuplicates excludes hidden doubts from candidate query", async () => {
const mockVector = new Array(1536).fill(0.1);
(groq.embeddings.create as jest.Mock).mockResolvedValueOnce({
data: [{ embedding: mockVector }],
});

const selectMock = db.select as jest.Mock;
const whereMock = jest.fn().mockReturnThis();
selectMock.mockReturnValue({
from: jest.fn().mockReturnThis(),
where: whereMock,
orderBy: jest.fn().mockReturnThis(),
limit: jest.fn().mockResolvedValue([]),
});

await findSemanticDuplicates({ content: "What is momentum?" });

expect(whereMock).toHaveBeenCalledTimes(1);
const whereArg = whereMock.mock.calls[0][0];
const hasIsHidden = (expr: any): boolean => {
if (!expr || typeof expr !== "object") return false;
if (expr.name === "isHidden") return true;
if (expr.queryChunks)
return expr.queryChunks.some((c: any) => hasIsHidden(c));
return false;
};
expect(hasIsHidden(whereArg)).toBe(true);
Comment on lines +79 to +86

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The test checks only that some AST node is named isHidden; it does not verify that the generated predicate compares the column to false or that hidden rows are excluded from returned results. The assertion would still pass for a malformed or ineffective expression containing the column reference. Execute the query against representative hidden and visible rows, or assert the generated SQL and bound value. [code quality]

Severity Level: Major ⚠️
- ⚠️ Test does not validate the hidden-value comparison.
- ❌ A wrong predicate could expose moderated doubts.
- ⚠️ Vector matches feed both duplicate-check endpoints.

Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/__tests__/lib/embeddings.test.ts
**Line:** 79:86
**Comment:**
	*Code Quality: The test checks only that some AST node is named `isHidden`; it does not verify that the generated predicate compares the column to `false` or that hidden rows are excluded from returned results. The assertion would still pass for a malformed or ineffective expression containing the column reference. Execute the query against representative hidden and visible rows, or assert the generated SQL and bound value.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

});
});
1 change: 1 addition & 0 deletions src/app/api/doubts/check-duplicate/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export async function POST(req: Request) {
: isNull(doubtsTable.classroomId),
eq(doubtsTable.type, "community"),
isNull(doubtsTable.deletedAt),
eq(doubtsTable.isHidden, false),
),
)
.orderBy(desc(doubtsTable.createdAt))
Expand Down
1 change: 1 addition & 0 deletions src/app/api/doubts/check-similarity/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export async function POST(req: Request) {
: isNull(doubtsTable.classroomId),
eq(doubtsTable.type, "community"),
isNull(doubtsTable.deletedAt),
eq(doubtsTable.isHidden, false),
),
)
.orderBy(desc(doubtsTable.createdAt))
Expand Down
1 change: 1 addition & 0 deletions src/lib/ai/embeddings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export async function findSemanticDuplicates(params: {
: isNull(doubtsTable.classroomId),
eq(doubtsTable.type, type),
isNull(doubtsTable.deletedAt),
eq(doubtsTable.isHidden, false),
// exclude null embeddings
sql`${doubtsTable.embedding} IS NOT NULL`,
);
Expand Down
Loading