chore: finding a good open governance model for AsyncAPI #4505
Conversation
Add explicit clarification that `protocolVersion` refers to the version of the underlying communication protocol (e.g., AMQP 0.9.1, MQTT 3.1.1), not the AsyncAPI specification version or application API version. Updates documentation in: - markdown/docs/reference/specification/v3.0.0.md - markdown/docs/concepts/asyncapi-document/structure.md Closes ambiguity between protocol versioning and API versioning.
- Implement client-side pagination with 15 posts per page - Add Previous/Next navigation controls with proper disabled states - Reset pagination when filtering or clearing filters - Maintain responsive design consistent with existing blog layout - Display current page info and total post count - Improve performance by only rendering visible posts
- Add pagination with Previous/Next buttons - Display maximum 15 posts per page - Use React state to track current page number - Reset pagination when filters are applied - Fix infinite re-render issue with useMemo optimization - Maintain responsive design and existing functionality
AnvayKharb
requested review from
Mayaleeeee,
TRohit20,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
sambhavgupta0705 and
thulieblack
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughDocumentation updates clarify the Changes
Sequence DiagramsequenceDiagram
actor User
participant BlogPage as Blog Page
participant State as React State
participant Memo as useMemo
User->>BlogPage: Load page / Change filter
Note over BlogPage: Component mounts
BlogPage->>State: Initialize currentPage = 1, isClient = false
BlogPage->>Memo: Sort navItems by date & featured
Memo-->>BlogPage: sortedPosts
rect rgb(200, 220, 255)
Note over BlogPage: useEffect activation
BlogPage->>State: Set isClient = true
end
alt User clicks filter
BlogPage->>State: Reset currentPage to 1
BlogPage->>State: Update posts from filtered results
else User clicks "Previous/Next"
BlogPage->>State: Update currentPage
else User clears filters
BlogPage->>State: Reset currentPage to 1
end
rect rgb(200, 255, 200)
Note over BlogPage: Compute pagination
BlogPage->>Memo: Calculate currentPosts slice<br/>[startIndex...endIndex]
Memo-->>BlogPage: currentPosts
end
alt isClient = true
BlogPage-->>User: Render currentPosts + Pagination controls
else isClient = false
BlogPage-->>User: Render placeholder / empty
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
asyncapi-bot
requested review from
CBID2,
bandantonio,
devilkiller-ag and
vishvamsinh28
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4505--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pages/blog/index.tsx (1)
27-29: Refactor: Thepostsstate and its update logic appear redundant.The
useEffectat lines 47-51 copiessortedPostsinto thepostsstate, but this creates an unnecessary extra render cycle. Consider usingsortedPostsdirectly throughout the component instead of maintaining a separatepostsstate. This would simplify the code and improve performance.Apply this refactor:
- const [posts, setPosts] = useState<IBlogPost[]>([]); const [isClient, setIsClient] = useState(false); const [currentPage, setCurrentPage] = useState(1); + const [filteredPosts, setFilteredPosts] = useState<IBlogPost[]>([]); // Memoize sorted posts to prevent unnecessary re-renders const sortedPosts = useMemo(() => { if (!navItems || navItems.length === 0) return []; return [...navItems].sort((i1: IBlogPost, i2: IBlogPost) => { const i1Date = new Date(i1.date); const i2Date = new Date(i2.date); if (i1.featured && !i2.featured) return -1; if (!i1.featured && i2.featured) return 1; return i2Date.getTime() - i1Date.getTime(); }); }, [navItems]); - // Update posts when sortedPosts changes - useEffect(() => { - if (sortedPosts.length > 0) { - setPosts(sortedPosts); - } - }, [sortedPosts]); + // Use filteredPosts if filtering is active, otherwise use sortedPosts + const posts = filteredPosts.length > 0 || Object.keys(router.query).length > 0 + ? filteredPosts + : sortedPosts; const onFilter = (data: IBlogPost[]) => { - setPosts(data); + setFilteredPosts(data); setCurrentPage(1); };Also applies to: 31-51
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
markdown/docs/concepts/asyncapi-document/structure.md(1 hunks)markdown/docs/reference/specification/v3.0.0.md(1 hunks)next-env.d.ts(1 hunks)pages/blog/index.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (7)
next-env.d.ts (1)
1-6: LGTM!This is the standard Next.js ambient TypeScript declarations file. The structure and content are correct for Next.js projects.
markdown/docs/reference/specification/v3.0.0.md (1)
413-413: LGTM! Excellent clarification.The expanded description of the
protocolVersionfield clearly distinguishes between the underlying protocol version, the AsyncAPI specification version, and the application API version. The concrete examples (AMQP 0.9.1, MQTT 3.1.1, HTTP 2.0, Kafka 1.0.0) make this distinction much clearer for users.markdown/docs/concepts/asyncapi-document/structure.md (1)
72-72: LGTM! Consistent documentation improvement.This clarification matches the changes in the specification document, maintaining consistency across the documentation. The examples effectively illustrate the distinction between protocol version, specification version, and application API version.
pages/blog/index.tsx (4)
2-2: LGTM! Proper imports and configuration.The addition of
useMemoand thePOSTS_PER_PAGEconstant (set to 15) align with the stated objectives for configurable pagination.Also applies to: 17-18
53-56: LGTM! Proper pagination reset on filter changes.Resetting
currentPageto 1 when filters are applied or cleared is the correct behavior to prevent displaying empty results pages.Also applies to: 69-74
77-95: LGTM! Correct pagination logic.The pagination calculations and navigation handlers are implemented correctly with proper boundary checks.
169-205: Verify actual pagination requirements and consistency with existing codebase patterns.The review raises valid architectural concerns about pagination, but some claims need clarification:
Verified:
- The codebase establishes a pattern for UI state: the Filter component uses
router.push()with query parameters andshallow: truerouting (seecomponents/navigation/Filter.tsx:76). Pagination should follow this same pattern for consistency.- Current pagination uses pure client-side state, making pages non-bookmarkable and non-crawlable—inconsistent with the Filter pattern.
Unverified:
- The review claims "PR objectives explicitly require" query parameters, rel links, sitemap updates, and ISR support, but these objectives aren't documented in the codebase or SEO guide (
markdown/docs/community/011-styleguide/seo-guide.mdcovers headings, URLs, internal links, and images, but not pagination-specific requirements).Action needed:
Confirm the actual PR requirements. If query-parameter pagination is needed, align it with the Filter component's established pattern. However, verify whether rel links, sitemap updates, and ISR are actual hard requirements or architectural preferences.
| {currentPosts.map((post, index) => ( | ||
| <BlogPostItem key={index} post={post} /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use stable identifiers for list keys instead of array index.
Using index as the key for BlogPostItem can cause issues with React's reconciliation when posts are filtered or reordered. Use a stable unique identifier from the post object instead (e.g., post.slug, post.id, or post.title).
- {currentPosts.map((post, index) => (
- <BlogPostItem key={index} post={post} />
+ {currentPosts.map((post) => (
+ <BlogPostItem key={post.slug || post.title} post={post} />
))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {currentPosts.map((post, index) => ( | |
| <BlogPostItem key={index} post={post} /> | |
| ))} | |
| {currentPosts.map((post) => ( | |
| <BlogPostItem key={post.slug || post.title} post={post} /> | |
| ))} |
🤖 Prompt for AI Agents
In pages/blog/index.tsx around lines 164 to 166, the list items use the array
index as the React key which breaks reconciliation when posts are reordered or
filtered; change the key to a stable unique identifier from the post object
(e.g., post.slug or post.id) by passing that property as the key to BlogPostItem
instead of index so keys remain consistent across renders.
2 participants
It Fixes: #4499
This pull request introduces pagination to the blog index page and improves the sorting and filtering logic for blog posts. Additionally, it clarifies documentation for the
protocolVersionfield in AsyncAPI server objects. Below are the most important changes grouped by theme:Blog Index Page Improvements:-
useMemofor performance, and ensured posts are sorted by featured status and date. Filtering now resets pagination to the first page.POSTS_PER_PAGE) to configure the number of posts shown per page.AsyncAPI Documentation Clarification:
protocolVersionfield refers specifically to the version of the underlying communication protocol, not the AsyncAPI spec or application API version.Summary by CodeRabbit
New Features
Documentation