Skip to content

Conversation

@marcodejongh
Copy link
Owner

Summary

This PR adds support for two new filtering options in the climb search results page: filtering by holds and filtering for only tall climbs.

Key Changes

  • Added onlyTallClimbs filter parameter to the search query object
  • Added holdsFilter parameter that converts from the internal LitUpHoldsMap format to the GraphQL-expected Record<string, 'ANY' | 'NOT'> format
    • Strips the hold_ prefix from hold keys during conversion
    • Extracts the state property from each hold filter value
  • Updated the default search detection logic to account for the new filters, ensuring searches with these filters applied are not treated as default searches

Implementation Details

  • The holdsFilter conversion handles the impedance mismatch between the frontend's internal hold representation and the GraphQL API's expected format
  • Both new filters are optional and default to undefined when not provided
  • The default search check now validates that both new filters are either absent or empty before considering a search as "default"

https://claude.ai/code/session_01VQ7Ej4DDYEudhK7D89uBbn

@vercel
Copy link

vercel bot commented Feb 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
boardsesh Building Building Preview, Comment Feb 1, 2026 8:20am

Request Review

@claude
Copy link

claude bot commented Feb 1, 2026

Claude Review

Ready to merge - Minor issues noted below, but nothing blocking.

Issues

  1. Type safety concern (line 215): The value.state from LitUpHoldsMap can be any HoldState ('OFF' | 'STARTING' | 'FINISH' | 'HAND' | 'FOOT' | 'ANY' | 'NOT'), but the shared-schema type holdsFilter claims to only accept 'ANY' | 'NOT'. While the backend code shows it actually handles more states, the type definitions are inconsistent. Consider either:

    • Adding validation to filter out invalid states, or
    • Updating the shared-schema types to reflect reality
  2. Missing type cast (line 215): value.state is typed as HoldState but assigned to Record<string, 'ANY' | 'NOT'>. TypeScript should catch this, but the Object.fromEntries return type loses specificity. This works at runtime but could mask future type errors.

Tests

No tests cover this new server-side rendering code path. Consider adding integration tests for the holds filter conversion logic.

@marcodejongh marcodejongh force-pushed the claude/fix-infinite-scroll-duplicates-WkBow branch from 84eb17e to d471bec Compare February 1, 2026 08:10
@claude
Copy link

claude bot commented Feb 1, 2026

Claude Review

Ready to merge - No significant issues found.

The implementation correctly converts the holdsFilter from the internal LitUpHoldsMap format to the GraphQL-expected Record<string, 'ANY' | 'NOT'> format in both locations, with proper filtering for valid states. The default search detection is appropriately updated.

@marcodejongh marcodejongh force-pushed the claude/fix-infinite-scroll-duplicates-WkBow branch from d471bec to 7369619 Compare February 1, 2026 08:14
@claude
Copy link

claude bot commented Feb 1, 2026

Claude Review

⚠️ Needs attention - Type mismatch between frontend and backend for holdsFilter.

Issues Found

  1. Type inconsistency will cause runtime validation failures (packages/shared-schema/src/types.ts:148, packages/backend/src/validation/schemas.ts:185)

    • The PR changes holdsFilter type from Record<string, 'ANY' | 'NOT'> to Record<string, HoldState> in the shared types
    • However, the backend validation schema still only accepts 'ANY' | 'NOT': z.record(z.enum(['ANY', 'NOT']))
    • The backend query layer (packages/backend/src/db/queries/climbs/create-climb-filters.ts:23) also expects only 'ANY' | 'NOT'
    • If the frontend sends any other HoldState values ('STARTING', 'HAND', 'FOOT', 'FINISH', 'OFF'), the backend will reject the request
  2. Missing tests - No tests added for the new filter functionality to verify the conversion logic works correctly.

SSR was missing these filters in the searchInput, causing a mismatch
between server-rendered and client-side data. When users had filters
like onlyTallClimbs=true active, the SSR would return unfiltered climbs
while the client would fetch filtered results, causing duplicates to
appear during infinite scroll as the data sources differed.

Also updated all type definitions to accept all HoldState values for
holdsFilter (not just 'ANY' | 'NOT'), preparing for future UI
implementations of other hold state filters:
- packages/shared-schema/src/types.ts
- packages/web/app/lib/graphql/operations/climb-search.ts
- packages/backend/src/validation/schemas.ts
- packages/backend/src/db/queries/climbs/create-climb-filters.ts

https://claude.ai/code/session_01VQ7Ej4DDYEudhK7D89uBbn
@marcodejongh marcodejongh force-pushed the claude/fix-infinite-scroll-duplicates-WkBow branch from 7369619 to 319ecb8 Compare February 1, 2026 08:20
@claude
Copy link

claude bot commented Feb 1, 2026

Claude Review

Ready to merge - Minor issue noted below, but nothing blocking.

Issues

  • Duplicate prefix stripping: The frontend strips hold_ prefix at packages/web/app/[board_name]/[layout_id]/[size_id]/[set_ids]/[angle]/list/page.tsx:214, but the backend also strips it at packages/backend/src/db/queries/climbs/create-climb-filters.ts:57. This double-stripping is currently harmless (second replace() is a no-op) but could cause bugs if the prefix format changes. Consider removing one.

  • Unused HoldState import: packages/web/app/lib/graphql/operations/climb-search.ts:2 imports HoldState but it's only used in a type annotation that could infer from the shared type.

@marcodejongh marcodejongh merged commit eeb02a6 into main Feb 1, 2026
5 of 8 checks passed
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.

3 participants