fix: resolve search ranking showing N/A on admin page#256
Conversation
The /nodes/search endpoint doesn't include search_ranking in its response,
causing all rankings to display as "N/A". Added a NodeSearchRankingCell
component that fetches each node's details via GET /nodes/{nodeId} (which
returns search_ranking for authenticated admin users) to display the actual
ranking values.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Fixes the admin “Search Ranking Management” page showing N/A by rendering rankings via per-node detail fetches, working around /nodes/search not returning search_ranking.
Changes:
- Added
NodeSearchRankingCellthat callsuseGetNode(nodeId)to displaynode.search_rankingper row. - Updated the table to use the new cell instead of
node.search_rankingfrom the search response. - Reformatted
pages/admin/search-ranking.tsxto match Prettier config (single quotes, no semicolons).
Comments suppressed due to low confidence (2)
pages/admin/search-ranking.tsx:162
Node.idis optional in the generated API types, but this row uses it for both the Reactkeyand the detailLinkURL. If a node ever comes back without anid, this will produce an invalid/nodes/undefinedlink and a non-unique/unstable key. Consider guarding the row rendering (or link) whennode.idis missing and using a guaranteed-stable key fallback.
<div>
<Button size="xs" color="blue" onClick={() => handleEditRanking(node)}>
<MdEdit className="mr-1" /> {t("Edit")}
</Button>
</div>
</li>
))}
{/* Empty state */}
{(!data?.nodes || data.nodes.length === 0) && (
<li className="py-4 text-center text-gray-400">{t("No nodes found")}</li>
pages/admin/search-ranking.tsx:171
NodeSearchRankingCellis invoked withnodeId={node.id || ''}. Whennode.idis absent this silently disables the query (becauseuseGetNodeisenabled: !!nodeId) and will always displayN/A, which can mask data issues. Prefer handling the missing-id case explicitly (e.g., renderN/Awithout instantiating the query, and avoid using an empty-string id that still creates a/nodes/query key).
currentPage={page}
totalPages={data?.totalPages || 1}
onPageChange={handlePageChange}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { data: node, isLoading } = useGetNode(nodeId, { | ||
| staleTime: 5 * 60 * 1000, | ||
| refetchOnWindowFocus: false, |
There was a problem hiding this comment.
useGetNode's second argument is params (querystring), not React Query options. Passing { staleTime, refetchOnWindowFocus } here will be sent as request params and the caching options won't apply. Move these under the 3rd argument as options: { query: { staleTime, refetchOnWindowFocus } } (and keep params as undefined unless needed).
| const { data: node, isLoading } = useGetNode(nodeId, { | |
| staleTime: 5 * 60 * 1000, | |
| refetchOnWindowFocus: false, | |
| const { data: node, isLoading } = useGetNode(nodeId, undefined, { | |
| query: { | |
| staleTime: 5 * 60 * 1000, | |
| refetchOnWindowFocus: false, | |
| }, |
There was a problem hiding this comment.
Fixed in 7e31cd5. Moved staleTime and refetchOnWindowFocus to the 3rd argument under options.query, and passed undefined for params. Also added enabled: !!nodeId to prevent queries with empty strings.
| const { data: node, isLoading } = useGetNode(nodeId, { | ||
| staleTime: 5 * 60 * 1000, | ||
| refetchOnWindowFocus: false, | ||
| }); | ||
| if (isLoading) return <Spinner size="xs" />; |
There was a problem hiding this comment.
This cell treats any useGetNode failure the same as “no ranking” (it will render N/A whenever the request errors, since node is undefined and isLoading is false). Consider handling isError/error separately (e.g., render an explicit error state or retry affordance) so network/auth issues aren’t silently masked as missing data.
| const { data: node, isLoading } = useGetNode(nodeId, { | |
| staleTime: 5 * 60 * 1000, | |
| refetchOnWindowFocus: false, | |
| }); | |
| if (isLoading) return <Spinner size="xs" />; | |
| const { data: node, isLoading, isError } = useGetNode(nodeId, { | |
| staleTime: 5 * 60 * 1000, | |
| refetchOnWindowFocus: false, | |
| }); | |
| if (isLoading) return <Spinner size="xs" />; | |
| if (isError) return <>Error</>; |
There was a problem hiding this comment.
Fixed in 7e31cd5. Added isError handling that shows an explicit "Error" state instead of silently displaying N/A.
| <div className="text-gray-300">{formatDownloadCount(node.downloads || 0)}</div> | ||
| <div className="text-gray-300"> | ||
| {node.search_ranking !== undefined ? node.search_ranking : t("N/A")} | ||
| <NodeSearchRankingCell nodeId={node.id || ""} /> | ||
| </div> |
There was a problem hiding this comment.
Rendering NodeSearchRankingCell for each row introduces an N+1 pattern (up to 24 extra GET /nodes/{id} requests per page load/search/pagination). This can significantly slow the admin page and increase API load; consider adding search_ranking to /nodes/search for admin responses, or introducing a batch endpoint / query that fetches rankings for the current page in one request.
There was a problem hiding this comment.
Acknowledged the N+1 concern. The ideal fix is to extend /nodes/search to include search_ranking for admin responses on the backend. This approach is a workaround since the frontend can't modify the API response. Added staleTime: 5min and refetchOnWindowFocus: false (now correctly placed in query options per 7e31cd5) to mitigate repeated refetches.
| <div className="text-gray-300">{formatDownloadCount(node.downloads || 0)}</div> | ||
| <div className="text-gray-300"> | ||
| {node.search_ranking !== undefined ? node.search_ranking : t("N/A")} | ||
| <NodeSearchRankingCell nodeId={node.id || ""} /> |
There was a problem hiding this comment.
node.id is optional in the generated Node type; passing node.id || "" means the query will be disabled when id is missing and the cell will silently show N/A. Prefer guarding before rendering (e.g., only render the cell when node.id is truthy) so missing IDs are handled explicitly.
There was a problem hiding this comment.
Fixed in 7e31cd5. Now guarding node.id before rendering NodeSearchRankingCell — renders t("N/A") directly when id is missing, avoiding the empty-string query key issue.
Previously only triggered on PRs targeting dev/staging/main, which meant PRs to feature branches skipped lint, format, and build checks. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y-Org/registry-web into fix/admin-search-ranking-na
- Move staleTime/refetchOnWindowFocus to 3rd arg (options.query) instead of 2nd arg (params) - Add isError handling to show explicit error state instead of silent N/A - Guard node.id before rendering NodeSearchRankingCell to handle missing IDs explicitly - Add enabled: !!nodeId to prevent queries with empty string Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
/nodes/searchAPI endpoint doesn't returnsearch_rankingin its response, causing all rankings to display as "N/A" on the admin search ranking pageNodeSearchRankingCellcomponent that fetches each node's details viaGET /nodes/{nodeId}(which returnssearch_rankingfor authenticated admin users) to display actual ranking valuesTest plan
/admin/search-ranking🤖 Generated with Claude Code