fix: keep catalog first-page failures retryable - #3457
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@pacocartones is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel. A member of the Team first needs to authorize it. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed August 16, 2026, 12:15 PM ET / 16:15 UTC. ClawSweeper reviewWhat this changesThe PR keeps New, Featured, and Official catalog tabs retryable after a failed first-page request, while retaining Trending’s dedicated unavailable fallback. Merge readinessKeep this PR open for maintainer merge review: current main still turns a rejected non-Trending first page into a terminal state, while this focused patch preserves retry and has no actionable correctness finding. The remaining ambiguous error presentation is separately tracked in #3473. Priority: P2 Review scores
Verification
How this fits togetherThe Skills catalog maps backend page requests into list results, terminal empty states, and retry controls. This patch changes the first-page failure state for non-Trending catalog tabs so users can make another request. flowchart LR
A[Catalog tab] --> B[First page request]
B --> C{Request outcome}
C -->|Success| D[Catalog results]
C -->|Failure| E[List status]
E --> F[Retry control]
F --> G[Second request]
G --> D
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the narrow retry-state repair, then address the distinct error-message state in #3473 without expanding this PR’s failure-recovery boundary. Do we have a high-confidence way to reproduce the issue? Yes—source on current main shows a rejected initial non-Trending request reaches the terminal done state, and the supplied browser proof demonstrates the resulting before/after recovery flow. Is this the best way to solve the issue? Yes—the status-only change preserves the existing Load more retry path and leaves the separate error-message design to #3473. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4117154ecac4. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (51 earlier review cycles; latest 8 shown)
|
ClawHub UI ProofStatus:
Raw proof files: https://github.com/pacocartones/clawhub/tree/qa-artifacts/clawhub-ui-proof/pr-3457/catalog-first-page-retry Both lanes ran real ClawHub apps against the same local Convex deployment. Playwright returned a |
clawsweeper
Bot
added
proof: 📸 screenshot
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Verify pass on 25db30d2 against base faab45ba, returning the favour for the one you ran on #3414.
No blocking findings. Two non-blocking notes at the end.
The state change is exactly one cell of the matrix. Reading the catch block in
src/routes/skills/-useSkillsBrowseModel.ts:292-300 as a table of (catalogTab, pageCursor):
| case | before | after |
|---|---|---|
| trending, first page | done |
done |
| trending, later page | idle |
idle |
| other, first page | done |
idle |
| other, later page | idle |
idle |
catalogTab === "trending" && !pageCursor ? "done" : "idle" moves the third row and nothing else, so
canonical Trending keeps its unavailable / No 24-hour activity yet terminal state and later-page
failures keep the behaviour they already had. The retry itself lands where it should: loadMore
(:517-527) calls fetchPage(listCursor, …) with the listCursor the catch block restored, which is
null on a first-page failure, and the success path replaces rather than appends when cursor is
falsy (:281), so a recovered first page cannot double up.
Test verification, with the negative control. At the PR head:
bunx vitest run src/__tests__/skills-index.test.tsx
Test Files 1 passed (1)
Tests 52 passed (52)
Then restoring only src/routes/skills/-useSkillsBrowseModel.ts from main and leaving the new test
file in place:
Tests 3 failed | 49 passed (52)
× keeps the new first page retryable after a temporary fetch failure
× keeps the featured first page retryable after a temporary fetch failure
× keeps the official first page retryable after a temporary fetch failure
Exactly the three new cases fail and the other 49 still pass, so the tests are bound to the new
behaviour and do not silently re-describe the old one.
Note 1, non-blocking: after a first-page failure the page carries no failure signal at all. The
empty state is gated on listDoneLoading, which src/routes/skills/index.tsx:407 computes as
!isLoadingSkills && !canLoadMore && !isLoadingMore. With the status now idle, canLoadMore is
true, so listDoneLoading is false and -SkillsResults.tsx:235 skips the empty block; the result
list is empty, so the render at :314-334 produces nothing either. What is left on screen is a bare
Load more card. That is strictly better than the false No skills found this fixes, and I would not
widen the diff for it, but a user who hits a 503 now sees an empty page whose only hint that anything
went wrong is a button. If the maintainers want an error line there, it seems like its own change.
Note 2, cosmetic: the vi.stubGlobal("IntersectionObserver", undefined) in the new cases is inert
— canAutoLoad is a hardcoded false at -useSkillsBrowseModel.ts:515, so loadMoreRef is never
attached and no observer is ever constructed. Harmless, and it matches what the neighbouring tests in
the file already do.
clawsweeper
Bot
added
rating: 🦞 diamond lobster
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Re-checked on the re-request. Nothing has changed that would move the verify pass above.
The head is still 25db30d2, the commit I read on August 12 — no new commits, no force-push, no new comments since. The only movement on the PR was ClawSweeper's rating going from platinum hermit to diamond lobster at 08:30 UTC.
What I did re-check is main drift, because that pass was run against base faab45ba and main has since moved to 60b02c09 (fix: stream legacy skill downloads, #3451). That commit touches convex/, server/, public/api/v1/openapi.json, docs/http-api.md, specs/spec.md and two files under src/components/. Restricting the range to the files my reading depended on:
git log --oneline faab45ba..60b02c09 -- \
src/routes/skills/-useSkillsBrowseModel.ts \
src/__tests__/skills-index.test.tsx \
src/routes/skills/index.tsx \
src/routes/skills/-SkillsResults.tsx
returns nothing. So the model, the new test file, and the two render paths behind Note 1 are byte-identical to what I read: the (catalogTab, pageCursor) state table, the loadMore cursor argument, and the listDoneLoading gate all still describe current main. The branch is MERGEABLE against it.
Verify pass stands, no blocking findings, and both notes stay non-blocking.
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Re-checked against ClawSweeper's P1 item, the five failed required checks on 25db30d2. They are one failure, and it lands on the production site rather than in this diff.
Four of the five reds ran nothing. static, unit, packages, types-build and e2e-http are test "$PR_GATES_RESULT" = "success" mirrors of pr-gates (.github/workflows/ci.yml:83-151), so a single red job paints six checks red. In pr-gates the steps those four names refer to all passed:
Static checks success 01:21:13 -> 01:21:39
Unit coverage success 01:21:39 -> 01:28:47
Package checks success 01:28:47 -> 01:29:41
Typecheck and build success 01:29:41 -> 01:30:11
HTTP e2e failure 01:30:11 -> 01:30:14
The one real failure is a live-prod fetch. ci:e2e-http starts with test:e2e:prod-http, which is e2e/prod-http-smoke.e2e.test.ts, and that file fetches CLAWHUB_E2E_SITE defaulting to https://clawhub.ai (prod-http-smoke.e2e.test.ts:22-33). Three of its four tests failed on the same assertion:
✓ serves the home page shell from prod 2521ms
× serves SSR skill html for a public skill page 132ms
× serves the skill og image for the latest published version 1ms
× serves the published SKILL.md file 0ms
AssertionError: expected false to be true
❯ e2e/prod-http-smoke.e2e.test.ts:158:27
❯ e2e/prod-http-smoke.e2e.test.ts:181:20
Line 158 is expect(response.ok).toBe(true) inside fetchSkillDetail(), which GETs /api/v1/skills/gifgrep. That promise is memoized, so the 1ms and 0ms tests are the same rejection re-awaited — one bad response, not three. fetchWithRetry only re-attempts 429 and >= 500 (:100, :104), so what came back was a non-retryable 4xx. The only test that does not touch that endpoint, the home page shell, passed, so the site itself was serving.
Nothing in the diff can reach that, and it is green now. This PR is src/routes/skills/-useSkillsBrowseModel.ts (+2/-2) and src/__tests__/skills-index.test.tsx (+34); e2e/prod-http-smoke.e2e.test.ts is byte-identical between 25db30d2 and main. As of now GET https://clawhub.ai/api/v1/skills/gifgrep returns 200, and a local bun run test:e2e:prod-http gives Test Files 1 passed (1) / Tests 4 passed (4).
Verify pass refreshed against current main. My August 13 re-check was pinned to 60b02c09; main is now 4117154e (36b775a6 and 4117154e since). git log 60b02c09..4117154e restricted to -useSkillsBrowseModel.ts, skills-index.test.tsx, skills/index.tsx and -SkillsResults.tsx returns nothing, and git merge-tree --write-tree main 25db30d2 merges without conflict. On a tree at current main with this PR's two files applied, src/__tests__/skills-index.test.tsx is 52/52. Reverting only -useSkillsBrowseModel.ts to main's copy while keeping the new tests puts exactly the three new cases back to red, each rendering No skills found where the test expects Load more, so the two-line change is still load-bearing and the coverage still binds to it on current main.
Verify pass stands, both earlier notes stay non-blocking. What is left is mechanical: the head is three commits behind main and its check set is from August 12, so a rebase onto 4117154e would replace this stale red with a fresh run. @pacocartones, that one is yours to push.
On ClawSweeper's second P1, the unavailable TruffleHog executable: that is its own gate, not a repo check. Scan for Verified Secrets and CodeQL are both green on 25db30d2.
25db30d to
b23a83c
Compare
|
Thank you for running it in reverse — restoring only the model and watching exactly the three new On the Aug 15 re-check: agreed the red is stale and unreachable from this diff — I've rebased onto Note 1 is right and I don't want to hide it in a thread. The PR moves the first-page-failure Note 2: correct, the stub is inert — Noted on the main drift re-check too — that's the bit most verify passes skip. |
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Re-verified on the rebased head b23a83ca. The local pass that didn't survive your machine today is below, so the fresh CI isn't the only thing standing behind this.
The rebase carries no change. I diffed the PR against each of its bases and compared the two patches:
git diff faab45ba 25db30d2 > old.diff # the head I read on Aug 12
git diff 4117154e b23a83ca > new.diff # the head after the rebase
diff old.diff new.diff # no output, exit 0
Byte-identical. The split into 72dd2784 + b23a83ca is bookkeeping: the second commit puts the try/finally back around the console.error spy where the rebased first commit had dropped it, so the resulting tree matches the one I already reviewed. Nothing new to read.
Local run on b23a83ca (Windows, vitest 4.1.10, bunx vitest run src/__tests__/skills-index.test.tsx):
Test Files 1 passed (1)
Tests 52 passed (52)
Reverse check, redone against the new base. Same method as last time but with 4117154e instead of faab45ba — restore only src/routes/skills/-useSkillsBrowseModel.ts from the base and leave the test file at the PR's version:
x keeps the new first page retryable after a temporary fetch failure
x keeps the featured first page retryable after a temporary fetch failure
x keeps the official first page retryable after a temporary fetch failure
Tests 3 failed | 49 passed (52)
The same three, and still only those three; the other 49 held. The coupling between the model change and the new cases survived the move onto current main, which is the part a rebase can quietly break.
Checks. Everything on b23a83ca is green except the Vercel deploy authorization, which is the same maintainer gate sitting on my #3414 — not something either of us can clear. The five reds from Aug 15 are gone with the rebase, as expected.
Note 1 / #3473. Agreed on the reading and on the scoping. Your write-up there is sharper than my note was: canLoadMore being true in both the empty and the failed case is the actual reason listDoneLoading cannot carry the distinction, and ClawSweeper's recommendation to land it as a follow-up after this merges matches what you proposed in the thread. I'd leave it with you rather than open a competing branch.
Note 2. Accepted. Keeping the stub where the neighbouring cases carry it is right; making three of them different to save three lines would cost more than it saves.
Nothing blocking from me.
2 participants
Summary
Why
The first request used a null cursor. On rejection, the shared error path set the list to
done,which removed Load more and presented the catalog as empty. Returning these non-Trending views
to
idlekeeps the existing retry control available without changing successful pagination orlater-page failure behavior.
Verification
SkillsIndexsuite: 52/52bun run ci:staticbunx tsc --noEmitThe aggregate coverage command did not finish inside a 180-second local timebox, so I am not
claiming that gate; the relevant suites and static gates above completed successfully.
No screenshot is attached because the change concerns a transient rejected-request state and the
regression drives the real rendered controls directly: the retry control remains after failure,
issues a second request, and disappears after recovery.
AI assistance
AI-assisted implementation and review were used. I reviewed the state transition, can maintain the
change, and independently verified retry, pagination, stale-response, double-click, and Trending
fallback behavior.
Real behavior proof
The exact live base (
faab45ba) and PR head (25db30d2) were run as real ClawHub applications inChromium against the same local Convex deployment and seeded repository fixture. Playwright
intercepted the real first
/api/queryrequest with HTTP 503 in both lanes.No skills foundstate with no retry control.Load moreafter the identical failure.Convex request returned 200 and the seeded skill rendered.
Before/after screenshots, metadata and the full methodology are linked in the durable UI proof
comment below. No DOM, application source, HTML or database row was edited to manufacture the UI.