fix: use indexed query for soul list to avoid over-fetching#1531
fix: use indexed query for soul list to avoid over-fetching#1531goulonghui wants to merge 1 commit intoopenclaw:mainfrom
Conversation
The souls.list query previously used take(limit * 5) followed by JS filtering on softDeletedAt, reading up to 5× more documents than needed. - For the ownerUserId branch: use Convex .filter() on the by_owner index to skip deleted rows before truncating. - For the browse branch: use the by_active_updated compound index with eq(softDeletedAt, undefined) to skip deleted rows at the index level. This reduces document reads and bandwidth, especially as the souls table grows.
|
Someone is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29fb68acc3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // index level instead of reading 5× documents and filtering in JS. | ||
| const entries = await ctx.db | ||
| .query("souls") | ||
| .withIndex("by_active_updated", (q) => q.eq("softDeletedAt", undefined)) |
There was a problem hiding this comment.
souls with an existing index
The new browse path in souls.list now calls withIndex("by_active_updated", ...), but the souls schema only defines by_slug, by_owner, by_owner_publisher, and by_updated (see convex/schema.ts). In environments where this commit is deployed as-is, any souls.list call without ownerUserId will fail at runtime with an unknown-index error instead of returning results.
Useful? React with 👍 / 👎.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| .map((soul) => toPublicSoul(soul)) | ||
| .filter((soul): soul is NonNullable<typeof soul> => Boolean(soul)); | ||
| } | ||
| // Use the by_active_updated index to skip soft-deleted rows at the |
There was a problem hiding this comment.
The souls.ts query uses .withIndex("by_active_updated", ...) but the souls table schema is missing this index definition, which will cause a Convex runtime error.
Greptile SummaryThis PR optimizes the
Confidence Score: 1/5Not safe to merge — the browse path references a non-existent index on the souls table, which will fail TypeScript type-checking and block deployment. The browse branch uses withIndex("by_active_updated") which does not exist on the souls table in schema.ts. This will cause a type error on npx convex deploy and break the list query for all browse (non-owner) callers. The ownerUserId branch improvement is sound, but the missing schema index is a blocking issue that must be resolved first. convex/souls.ts lines 144–146 and convex/schema.ts (souls table index definitions at lines 384–387) Prompt To Fix All With AIThis is a comment left during a code review.
Path: convex/souls.ts
Line: 144-146
Comment:
**Missing `by_active_updated` index on `souls` table**
The browse path references `.withIndex("by_active_updated", ...)`, but this index does not exist on the `souls` table. Looking at `convex/schema.ts`, the `souls` table (defined around line 366) only declares four indexes: `by_slug`, `by_owner`, `by_owner_publisher`, and `by_updated`. The `by_active_updated` compound index (`["softDeletedAt", "updatedAt"]`) is defined on `skillSearchDigest`, `packages`, and `packageSearchDigest` — not on `souls`.
Because Convex generates typed index accessors at build time, this will fail TypeScript type-checking and `npx convex deploy` will reject the code entirely. The `by_active_updated` index needs to be added to the `souls` table in `convex/schema.ts`:
```typescript
// convex/schema.ts — add to the souls defineTable chain
const souls = defineTable({ ... })
.index("by_slug", ["slug"])
.index("by_owner", ["ownerUserId"])
.index("by_owner_publisher", ["ownerPublisherId"])
.index("by_updated", ["updatedAt"])
.index("by_active_updated", ["softDeletedAt", "updatedAt"]); // add this
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: use indexed query for soul list to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .query("souls") | ||
| .withIndex("by_active_updated", (q) => q.eq("softDeletedAt", undefined)) | ||
| .order("desc") |
There was a problem hiding this comment.
by_active_updated index on souls table
The browse path references .withIndex("by_active_updated", ...), but this index does not exist on the souls table. Looking at convex/schema.ts, the souls table (defined around line 366) only declares four indexes: by_slug, by_owner, by_owner_publisher, and by_updated. The by_active_updated compound index (["softDeletedAt", "updatedAt"]) is defined on skillSearchDigest, packages, and packageSearchDigest — not on souls.
Because Convex generates typed index accessors at build time, this will fail TypeScript type-checking and npx convex deploy will reject the code entirely. The by_active_updated index needs to be added to the souls table in convex/schema.ts:
// convex/schema.ts — add to the souls defineTable chain
const souls = defineTable({ ... })
.index("by_slug", ["slug"])
.index("by_owner", ["ownerUserId"])
.index("by_owner_publisher", ["ownerPublisherId"])
.index("by_updated", ["updatedAt"])
.index("by_active_updated", ["softDeletedAt", "updatedAt"]); // add thisPrompt To Fix With AI
This is a comment left during a code review.
Path: convex/souls.ts
Line: 144-146
Comment:
**Missing `by_active_updated` index on `souls` table**
The browse path references `.withIndex("by_active_updated", ...)`, but this index does not exist on the `souls` table. Looking at `convex/schema.ts`, the `souls` table (defined around line 366) only declares four indexes: `by_slug`, `by_owner`, `by_owner_publisher`, and `by_updated`. The `by_active_updated` compound index (`["softDeletedAt", "updatedAt"]`) is defined on `skillSearchDigest`, `packages`, and `packageSearchDigest` — not on `souls`.
Because Convex generates typed index accessors at build time, this will fail TypeScript type-checking and `npx convex deploy` will reject the code entirely. The `by_active_updated` index needs to be added to the `souls` table in `convex/schema.ts`:
```typescript
// convex/schema.ts — add to the souls defineTable chain
const souls = defineTable({ ... })
.index("by_slug", ["slug"])
.index("by_owner", ["ownerUserId"])
.index("by_owner_publisher", ["ownerPublisherId"])
.index("by_updated", ["updatedAt"])
.index("by_active_updated", ["softDeletedAt", "updatedAt"]); // add this
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
The souls.list query previously used take(limit * 5) followed by JS filtering on softDeletedAt, reading up to 5× more documents than needed.
This reduces document reads and bandwidth, especially as the souls table grows.