fix(api): restore v1 skills list compatibility#961
fix(api): restore v1 skills list compatibility#961Alix-007 wants to merge 4 commits intoopenclaw:mainfrom
Conversation
|
@Alix-007 is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR restores Key observations:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: convex/skills.ts
Line: 2955-2968
Comment:
**No fallback when `trending_non_suspicious` leaderboard doesn't exist yet**
When `nonSuspiciousOnly: true`, `getTrendingEntries` queries the `trending_non_suspicious` leaderboard exclusively. If that leaderboard hasn't been populated yet (e.g., before the first scheduled run of the leaderboard job, or if the job has only ever populated `trending`), it returns `[]`, so the handler sends back an empty items list.
This is a cold-start regression: callers asking for safe trending results get nothing even though a `trending` leaderboard exists that could be filtered at runtime. The PR description says it's "restoring" the trending behavior, but this edge case regresses to empty-page behavior.
A safer approach would fall back to the `trending` leaderboard and apply runtime filtering when the specialized leaderboard is absent:
```typescript
async function getTrendingEntries(
ctx: QueryCtx,
limit: number,
args?: { nonSuspiciousOnly?: boolean },
) {
if (args?.nonSuspiciousOnly) {
const nonSuspicious = await ctx.db
.query('skillLeaderboards')
.withIndex('by_kind', (q) => q.eq('kind', TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND))
.order('desc')
.take(1)
if (nonSuspicious[0]) return nonSuspicious[0].items.slice(0, limit)
// Fall back to the general leaderboard; the caller already filters per-skill.
}
const latest = await ctx.db
.query('skillLeaderboards')
.withIndex('by_kind', (q) => q.eq('kind', TRENDING_LEADERBOARD_KIND))
.order('desc')
.take(1)
return latest[0] ? latest[0].items.slice(0, limit) : []
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: convex/skills.listPublicPage.test.ts
Line: 155-188
Comment:
**No test coverage for stat-sorted paths**
`makeCtx` only wires up `by_updated` — calling the handler with `sort: 'downloads'`, `'stars'`, `'installsCurrent'`, or `'installsAllTime'` would throw `unexpected index` from the mock. The four `sortToIndex` branches and their pagination / filtering logic have zero test coverage in this suite.
Consider adding at least one parameterised case for a stat-sorted path (e.g. `sort: 'downloads'`) to guard against a wrong index name or a logic divergence from the `updated` path.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 30a4e76 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
convex/skills.ts
Outdated
| const kind = args?.nonSuspiciousOnly | ||
| ? TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND | ||
| : TRENDING_LEADERBOARD_KIND | ||
| const latest = await ctx.db | ||
| .query('skillLeaderboards') | ||
| .withIndex('by_kind', (q) => q.eq('kind', kind)) | ||
| .order('desc') | ||
| .take(1) | ||
|
|
||
| if (latest[0]) { | ||
| return latest[0].items.slice(0, limit) | ||
| } | ||
|
|
||
| return [] |
There was a problem hiding this comment.
trending_non_suspicious leaderboard doesn't exist yet
When nonSuspiciousOnly: true, getTrendingEntries queries the trending_non_suspicious leaderboard exclusively. If that leaderboard hasn't been populated yet (e.g., before the first scheduled run of the leaderboard job, or if the job has only ever populated trending), it returns [], so the handler sends back an empty items list.
This is a cold-start regression: callers asking for safe trending results get nothing even though a trending leaderboard exists that could be filtered at runtime. The PR description says it's "restoring" the trending behavior, but this edge case regresses to empty-page behavior.
A safer approach would fall back to the trending leaderboard and apply runtime filtering when the specialized leaderboard is absent:
async function getTrendingEntries(
ctx: QueryCtx,
limit: number,
args?: { nonSuspiciousOnly?: boolean },
) {
if (args?.nonSuspiciousOnly) {
const nonSuspicious = await ctx.db
.query('skillLeaderboards')
.withIndex('by_kind', (q) => q.eq('kind', TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND))
.order('desc')
.take(1)
if (nonSuspicious[0]) return nonSuspicious[0].items.slice(0, limit)
// Fall back to the general leaderboard; the caller already filters per-skill.
}
const latest = await ctx.db
.query('skillLeaderboards')
.withIndex('by_kind', (q) => q.eq('kind', TRENDING_LEADERBOARD_KIND))
.order('desc')
.take(1)
return latest[0] ? latest[0].items.slice(0, limit) : []
}Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.ts
Line: 2955-2968
Comment:
**No fallback when `trending_non_suspicious` leaderboard doesn't exist yet**
When `nonSuspiciousOnly: true`, `getTrendingEntries` queries the `trending_non_suspicious` leaderboard exclusively. If that leaderboard hasn't been populated yet (e.g., before the first scheduled run of the leaderboard job, or if the job has only ever populated `trending`), it returns `[]`, so the handler sends back an empty items list.
This is a cold-start regression: callers asking for safe trending results get nothing even though a `trending` leaderboard exists that could be filtered at runtime. The PR description says it's "restoring" the trending behavior, but this edge case regresses to empty-page behavior.
A safer approach would fall back to the `trending` leaderboard and apply runtime filtering when the specialized leaderboard is absent:
```typescript
async function getTrendingEntries(
ctx: QueryCtx,
limit: number,
args?: { nonSuspiciousOnly?: boolean },
) {
if (args?.nonSuspiciousOnly) {
const nonSuspicious = await ctx.db
.query('skillLeaderboards')
.withIndex('by_kind', (q) => q.eq('kind', TRENDING_NON_SUSPICIOUS_LEADERBOARD_KIND))
.order('desc')
.take(1)
if (nonSuspicious[0]) return nonSuspicious[0].items.slice(0, limit)
// Fall back to the general leaderboard; the caller already filters per-skill.
}
const latest = await ctx.db
.query('skillLeaderboards')
.withIndex('by_kind', (q) => q.eq('kind', TRENDING_LEADERBOARD_KIND))
.order('desc')
.take(1)
return latest[0] ? latest[0].items.slice(0, limit) : []
}
```
How can I resolve this? If you propose a fix, please make it concise.| function makeCtx({ | ||
| by_updated, | ||
| users, | ||
| versions, | ||
| }: { | ||
| by_updated: ReturnType<typeof vi.fn> | ||
| users: Array<ReturnType<typeof makeUser>> | ||
| versions: Array<ReturnType<typeof makeVersion>> | ||
| }) { | ||
| const userMap = new Map(users.map((user) => [user._id, user])) | ||
| const versionMap = new Map(versions.map((version) => [version._id, version])) | ||
| return { | ||
| db: { | ||
| query: vi.fn((table: string) => { | ||
| if (table !== 'skills') throw new Error(`unexpected table ${table}`) | ||
| return { | ||
| withIndex: vi.fn((index: string, _builder: unknown) => { | ||
| if (index !== 'by_updated') throw new Error(`unexpected index ${index}`) | ||
| return { | ||
| order: vi.fn((dir: string) => { | ||
| if (dir !== 'desc') throw new Error(`unexpected order ${dir}`) | ||
| return { paginate: by_updated } | ||
| }), | ||
| } | ||
| }), | ||
| } | ||
| }), | ||
| get: vi.fn(async (id: string) => { | ||
| if (id.startsWith('users:')) return userMap.get(id) ?? null | ||
| if (id.startsWith('skillVersions:')) return versionMap.get(id) ?? null | ||
| return null | ||
| }), | ||
| }, | ||
| } |
There was a problem hiding this comment.
makeCtx only wires up by_updated — calling the handler with sort: 'downloads', 'stars', 'installsCurrent', or 'installsAllTime' would throw unexpected index from the mock. The four sortToIndex branches and their pagination / filtering logic have zero test coverage in this suite.
Consider adding at least one parameterised case for a stat-sorted path (e.g. sort: 'downloads') to guard against a wrong index name or a logic divergence from the updated path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.listPublicPage.test.ts
Line: 155-188
Comment:
**No test coverage for stat-sorted paths**
`makeCtx` only wires up `by_updated` — calling the handler with `sort: 'downloads'`, `'stars'`, `'installsCurrent'`, or `'installsAllTime'` would throw `unexpected index` from the mock. The four `sortToIndex` branches and their pagination / filtering logic have zero test coverage in this suite.
Consider adding at least one parameterised case for a stat-sorted path (e.g. `sort: 'downloads'`) to guard against a wrong index name or a logic divergence from the `updated` path.
How can I resolve this? If you propose a fix, please make it concise.![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: 30a4e76801
ℹ️ 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".
convex/skills.ts
Outdated
| .take(1) | ||
|
|
||
| if (latest[0]) { | ||
| return latest[0].items.slice(0, limit) |
There was a problem hiding this comment.
The trending path slices cached entries to limit inside getTrendingEntries, but listPublicPage then drops entries that are soft-deleted, suspicious, or otherwise non-public. That means valid lower-ranked entries are never considered, so callers can get underfilled (or empty) pages even when enough eligible trending skills exist; for example, limit=1 returns nothing if rank #1 was deleted after cache generation while rank #2 is still valid.
Useful? React with 👍 / 👎.
convex/skills.ts
Outdated
| return latest[0].items.slice(0, limit) | ||
| } | ||
|
|
||
| return [] |
There was a problem hiding this comment.
When nonSuspiciousOnly is enabled, getTrendingEntries queries only trending_non_suspicious and returns an empty list if that cache row is absent, even if the regular trending cache exists. Since leaderboard rebuild writes trending and trending_non_suspicious in separate mutations (convex/leaderboards.ts), a partial failure can leave non-suspicious API requests empty until the next successful rebuild instead of falling back to filtering the main trending entries.
Useful? React with 👍 / 👎.
|
Addressing the review notes in the latest head:
Static check in this environment: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d71ca17b0a
ℹ️ 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".
| .order('desc') | ||
| .take(1) | ||
|
|
||
| return latest[0] ? latest[0].items.slice(0, limit) : [] |
There was a problem hiding this comment.
getTrendingEntries slices the cached leaderboard to limit before listPublicPage filters out non-public/suspicious skills, so lower-ranked valid skills are never considered. In the fallback path (nonSuspiciousOnly=true with only trending cache present), a leaderboard like [suspicious, clean] and limit=1 returns an empty page because only the first entry is fetched and then discarded; this underfills v1 API responses even when enough eligible skills exist.
Useful? React with 👍 / 👎.
|
Closing this to keep the active queue on items I can actually unblock. The code changes are in place, but this PR is currently gated by Vercel team authorization on the repository side, so I am redirecting effort to unblocked issues instead. |
|
Is there any plan to merge the fix from PR #961? |
2 participants
Summary
skills.listPublicPageas a compatibility shim for HTTP API v1 callersupdated, stat-sorted, andtrendingbehaviors instead of returning an empty pageTesting
git diff --checkvitestin this environment becausebun/repo dependencies are not installed locallyCloses #958