Skip to content

feat(hydrogen): fetchCustomPages for sitemap support#445

Merged
paul-phan merged 3 commits into
mainfrom
feat/443-fetch-custom-pages
Apr 22, 2026
Merged

feat(hydrogen): fetchCustomPages for sitemap support#445
paul-phan merged 3 commits into
mainfrom
feat/443-fetch-custom-pages

Conversation

@paul-phan
Copy link
Copy Markdown
Member

Summary

  • Add CustomPageEntry type for sitemap entries (handle, locale, path, lastModified, priority, changeFrequency)
  • Add fetchCustomPages() method on WeaverseClient that wraps the new public builder endpoint (/api/public/v1/projects/:projectId/custom-pages)
  • Method paginates automatically following nextCursor, caps at 100 iterations, never throws
  • Returns accumulated entries on partial failure; logs warnings via console.warn

Test plan

  • 8 unit tests (success, empty, network error, 404, locale param, partial failure, pagination cap, cap warning)
  • Integration with pilot theme sitemap-weaverse.xml.ts (separate PR)
  • Builder public endpoint already merged (PR #2226)

TDD red phase — 8 test cases covering success, empty, error
handling, locale filtering, pagination, and cap behavior.
Public method for sitemap consumers. Paginates automatically
following nextCursor, caps at 100 iterations, never throws.
Returns accumulated entries on partial failure.
@claude
Copy link
Copy Markdown

claude Bot commented Apr 22, 2026

Claude finished @paul-phan's task in 55s —— View job


Code Review

  • Gather PR context (files, diffs, conventions)
  • Review types.ts changes
  • Review weaverse-client.ts changes
  • Review test coverage
  • Check for breaking changes and API safety
  • Post final review

Code Review Summary

Overall: This is a well-implemented feature that adds sitemap support to the Weaverse Hydrogen SDK. The code is clean, follows project conventions, and has comprehensive test coverage. One minor TypeScript issue should be addressed.

Issues

🟡 packages/hydrogen/src/weaverse-client.ts:788 — Using any for error type violates project guidelines (AGENTS.md line 117: "No any — use unknown with type guards")

Suggestions

💬 packages/hydrogen/src/weaverse-client.ts:784 — Could use Array.isArray(result.data) for extra type safety, though current approach is fine given the typed fetch response

💬 packages/hydrogen/src/weaverse-client.ts:790 — String concatenation in template literal could be simplified: ${status ? (HTTP ${status}) : ''}, but current approach is clear

What's Good

  • Clean API design: The fetchCustomPages() method has a simple, intuitive interface with sensible defaults
  • Robust error handling: Never throws, returns partial results on failure, provides clear console warnings
  • Comprehensive test coverage: 8 tests covering success, empty, errors, pagination, and edge cases
  • Type safety: Proper TypeScript types for CustomPageEntry interface with appropriate optional fields
  • Defensive programming: Uses result.data || [] to handle missing data gracefully
  • Pagination safety: 100-iteration cap prevents infinite loops with appropriate warning
  • No breaking changes: This is a pure addition to the API, doesn't modify existing functionality
  • Follows conventions: Uses let correctly, single quotes, proper async/await patterns, AAA test pattern

Agent Fix Prompts

Copy any prompt below and send it to your coding agent to fix the issue.

🟡 Replace any with unknown for error type

In packages/hydrogen/src/weaverse-client.ts at line 788: Change `catch (err: any)` to `catch (err: unknown)` to comply with project guidelines (AGENTS.md line 117). Then update the error handling code (lines 789-795) to properly narrow the type before accessing `err?.status` and `err?.message`. Use a type guard to check if `err` is an object with these properties before accessing them.

@paul-phan paul-phan merged commit fe0a2ab into main Apr 22, 2026
6 of 8 checks passed
@paul-phan paul-phan deleted the feat/443-fetch-custom-pages branch April 22, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant