Conversation
|
📝 WalkthroughWalkthroughThe API overview page and its components were refactored to move all data fetching and navigation logic to the client side. Server-side data fetching and the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApisOverviewPage
participant ApiListClient
participant trpc.api.overview.query.useInfiniteQuery
User->>ApisOverviewPage: Loads API overview page
ApisOverviewPage->>ApiListClient: Renders component
ApiListClient->>trpc.api.overview.query.useInfiniteQuery: Fetch API overview data (paginated)
trpc.api.overview.query.useInfiniteQuery-->>ApiListClient: Returns pages of API data
ApiListClient->>User: Shows loading skeletons, API list, or empty state
User->>ApiListClient: Clicks "Load more"
ApiListClient->>trpc.api.overview.query.useInfiniteQuery: Fetch next page
trpc.api.overview.query.useInfiniteQuery-->>ApiListClient: Returns next page
ApiListClient->>User: Updates API list display
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx(1 hunks)apps/dashboard/app/(app)/apis/_components/api-list-grid.tsx(2 hunks)apps/dashboard/app/(app)/apis/_components/skeleton.tsx(1 hunks)apps/dashboard/app/(app)/apis/navigation.tsx(0 hunks)apps/dashboard/app/(app)/apis/page.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/apis/navigation.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (2)
apps/dashboard/app/(app)/apis/_components/api-list-grid.tsx (1)
1-68: I couldn’t locate any references using the--typeflags. Let’s try a straight grep across all files:#!/bin/bash # Search for any usage or import of ApiListGrid in the repo rg -n "ApiListGrid"apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (1)
51-54: Redirect on query error may be wrong destination
router.push("/new")routes to the global/new, which seems unrelated to APIs and might break contextual navigation.
Should this be/apis?new=true(to open the create-API modal) or another path?
|
@mcstepp so should James need to see this? |
|
I mean review |
|
Why would I need to review this? |
|
production stuff should be review by 2 people |
|
2 person reviews don't go into our flow until SOC2 auditing starts... And then it will be enforced in GH |
|
aaaah shit sry |
|
🙂 |
|
I mean you can still see it don't hold yourself 🤣 |
|
LGTM :blob-thumbs-up: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (1)
83-85: Counter display is misleading during searchWhen searching, showing "Showing X of Y APIs" is confusing because it implies pagination when search results are displayed in full. The counter should be adapted for search context.
-<div className="text-center text-sm text-accent-11"> - Showing {apiList.length} of {apisData?.pages[0]?.total || 0} APIs -</div> +<div className="text-center text-sm text-accent-11"> + {isSearching + ? `${apiList.length} results found` + : `Showing ${apiList.length} of ${apisData?.pages[0]?.total || 0} APIs` + } +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx(1 hunks)
🔇 Additional comments (3)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (3)
69-69: Good fix: skeleton count now matches fetch limitThe skeleton count now uses
DEFAULT_LIMITinstead of a hardcoded value, ensuring the loading state matches the expected data size and preventing visual jumps.
87-87: Load more button correctly respects search stateThe "Load more" button now properly checks
!isSearchingcondition, preventing pagination during search operations as intended.
50-54: ```shell
#!/bin/bashPreview the contents of the top of the /new page to verify its purpose
head -n 100 apps/dashboard/app/new/page.tsx
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| useEffect(() => { | ||
| setApiList(allApis); | ||
| }, [allApis]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider potential state synchronization conflicts
The useEffect synchronizes allApis (server data) to apiList (local state), but search operations modify apiList directly. If new server data arrives while searching, the search results would be overwritten.
While this may not be a practical issue due to search disabling pagination, consider a more explicit state management pattern to avoid potential conflicts.
// Consider using derived state instead of synchronization:
-const [apiList, setApiList] = useState(allApis);
-
-useEffect(() => {
- setApiList(allApis);
-}, [allApis]);
+const [searchResults, setSearchResults] = useState<typeof allApis>([]);
+const [isSearching, setIsSearching] = useState(false);
+
+const displayedApis = isSearching ? searchResults : allApis;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/dashboard/app/(app)/apis/_components/api-list-client.tsx around lines 46
to 48, the useEffect hook directly sets apiList state from allApis, but since
apiList is also modified by search operations, incoming updates to allApis can
overwrite search results. To fix this, separate the source data (allApis) from
the filtered or displayed data (apiList) by maintaining distinct states or use
memoized derived state for search results, ensuring that updates to allApis do
not overwrite user-driven changes like search filtering.
3 participants
What does this PR do?
This PR removes RSC from /apis, and uses the same tRPC endpoint that we use to fetch side bar API list. So we are getting rid of one redundant db call.
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor
Chores