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
3 changes: 2 additions & 1 deletion convex/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ const souls = defineTable({
.index("by_slug", ["slug"])
.index("by_owner", ["ownerUserId"])
.index("by_owner_publisher", ["ownerPublisherId"])
.index("by_updated", ["updatedAt"]);
.index("by_updated", ["updatedAt"])
.index("by_active_updated", ["softDeletedAt", "updatedAt"]);

const skillVersions = defineTable({
skillId: v.id("skills"),
Expand Down
75 changes: 74 additions & 1 deletion convex/souls.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it, vi } from "vitest";
import { getSoulBySlugInternal, insertVersion } from "./souls";
import { getSoulBySlugInternal, insertVersion, list } from "./souls";

type WrappedHandler<TArgs> = {
_handler: (ctx: unknown, args: TArgs) => Promise<unknown>;
Expand All @@ -10,6 +10,7 @@ const insertVersionHandler = (insertVersion as unknown as WrappedHandler<Record<
const getSoulBySlugInternalHandler = (
getSoulBySlugInternal as unknown as WrappedHandler<{ slug: string }>
)._handler;
const listHandler = (list as unknown as WrappedHandler<{ ownerUserId?: string; limit?: number }>)._handler;

describe("souls.insertVersion", () => {
it("throws a soul-specific ownership error for non-owners", async () => {
Expand Down Expand Up @@ -139,3 +140,75 @@ describe("souls.insertVersion", () => {
);
});
});

describe("souls.list", () => {
it("uses the active browse index and only takes the requested limit", async () => {
let requestedIndex: string | null = null;
let requestedSoftDeletedAt: number | undefined;
let requestedLimit: number | null = null;

const result = await listHandler(
{
db: {
query: vi.fn((table: string) => {
if (table !== "souls") throw new Error(`unexpected table ${table}`);
return {
withIndex: (
name: string,
build:
| ((q: { eq: (field: string, value: undefined) => unknown }) => unknown)
| undefined,
) => {
requestedIndex = name;
const q = {
eq: (field: string, value: undefined) => {
if (field !== "softDeletedAt") throw new Error(`unexpected field ${field}`);
requestedSoftDeletedAt = value;
return q;
},
};
build?.(q);
return {
order: () => ({
take: async (limit: number) => {
requestedLimit = limit;
return [
{
_id: "souls:1",
_creationTime: 1,
slug: "demo-soul",
displayName: "Demo Soul",
summary: "A demo soul",
ownerUserId: "users:owner",
ownerPublisherId: undefined,
latestVersionId: undefined,
tags: {},
softDeletedAt: undefined,
stats: { downloads: 1, stars: 2, versions: 3, comments: 4 },
createdAt: 1,
updatedAt: 2,
},
];
},
}),
};
},
};
}),
},
} as never,
{ limit: 7 } as never,
);

expect(requestedIndex).toBe("by_active_updated");
expect(requestedSoftDeletedAt).toBeUndefined();
expect(requestedLimit).toBe(7);
expect(result).toEqual([
expect.objectContaining({
_id: "souls:1",
slug: "demo-soul",
displayName: "Demo Soul",
}),
]);
});
});
5 changes: 2 additions & 3 deletions convex/souls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,10 @@ export const list = query({
}
const entries = await ctx.db
.query("souls")
.withIndex("by_active_updated", (q) => q.eq("softDeletedAt", undefined))
.order("desc")
.take(limit * 5);
.take(limit);
Comment on lines 139 to +143
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Silent sort-order change: _creationTimeupdatedAt

The old scan (no index, .order("desc")) sorted by _creationTime — Convex's default ordering. The new by_active_updated index is ["softDeletedAt", "updatedAt"], so after the equality constraint on softDeletedAt the remaining sort key is updatedAt. The browse listing will now return souls ordered by most-recently-updated rather than most-recently-created, which is a silent behavioral change. The PR description says "keeps the behavior the same," but the sort order is different. If this is intentional (aligning with listPublicPage's by_updated ordering), please call it out explicitly in the description.

Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/souls.ts
Line: 139-143

Comment:
**Silent sort-order change: `_creationTime``updatedAt`**

The old scan (no index, `.order("desc")`) sorted by `_creationTime` — Convex's default ordering. The new `by_active_updated` index is `["softDeletedAt", "updatedAt"]`, so after the equality constraint on `softDeletedAt` the remaining sort key is `updatedAt`. The browse listing will now return souls ordered by most-recently-*updated* rather than most-recently-*created*, which is a silent behavioral change. The PR description says "keeps the behavior the same," but the sort order is different. If this is intentional (aligning with `listPublicPage`'s `by_updated` ordering), please call it out explicitly in the description.

How can I resolve this? If you propose a fix, please make it concise.

return entries
.filter((soul) => !soul.softDeletedAt)
.slice(0, limit)
.map((soul) => toPublicSoul(soul))
.filter((soul): soul is NonNullable<typeof soul> => Boolean(soul));
},
Expand Down
6 changes: 3 additions & 3 deletions src/routes/cli/auth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ export function CliAuth({ navigate = (url: string) => window.location.assign(url
hash.set("token", result.token);
hash.set("registry", registry);
hash.set("state", state);
const callbackUrl = `${redirectUri}#${hash.toString()}`;
const redirectUrl = `${redirectUri}#${hash.toString()}`;
// Render the fallback token before attempting navigation so it is
// always visible if the browser blocks or fails the http:// redirect
// (e.g. ERR_CONNECTION_REFUSED when the CLI server has already shut
// down, or Chrome's HTTPS-first mode interfering with localhost).
flushSync(() => {
setToken(result.token);
setCallbackUrl(callbackUrl);
setCallbackUrl(redirectUrl);
setStatus("Redirecting to CLI…");
});
navigate(callbackUrl);
navigate(redirectUrl);
};

void run().catch((error) => {
Expand Down