docs: clarify protocolVersion field to distinguish from API versioning #4500
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.
AnvayKharb
requested review from
TRohit20,
asyncapi-bot-eve and
thulieblack
as code owners
WalkthroughClarified Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as BlogPage
participant S as State (currentPage, filters, posts)
U->>B: Load blog index
B->>S: initialize posts, currentPage = 1
B-->>U: render currentPosts (slice by currentPage)
U->>B: Click Next / Prev
B->>S: update currentPage (bounded)
B-->>U: render new currentPosts
U->>B: Apply / Clear filter
B->>S: update filters, reset currentPage = 1
B-->>U: render filtered currentPosts (page 1)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4500 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 778 778
Branches 144 144
=========================================
Hits 778 778 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4500--asyncapi-website.netlify.app/ |
|
please review @thulieblack @bandantonio |
- 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
AnvayKharb
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
derberg and
sambhavgupta0705
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: 1
🧹 Nitpick comments (2)
pages/blog/index.tsx (2)
75-85: Consider scrolling to top when page changes.The pagination logic and boundary checks are correct. However, when users navigate to a different page, they typically expect to see the top of the new content. Without automatic scrolling, users might remain at the bottom of the page and not realize the content has changed.
Consider adding scroll behavior to the handlers:
const goToNextPage = () => { if (currentPage < totalPages) { setCurrentPage(currentPage + 1); + window.scrollTo({ top: 0, behavior: 'smooth' }); } }; const goToPreviousPage = () => { if (currentPage > 1) { setCurrentPage(currentPage - 1); + window.scrollTo({ top: 0, behavior: 'smooth' }); } };Or scroll to a specific element like the blog list:
const goToNextPage = () => { if (currentPage < totalPages) { setCurrentPage(currentPage + 1); + document.getElementById('main-content')?.scrollIntoView({ behavior: 'smooth' }); } };
41-41: Consider syncing pagination state with URL query parameters.The current page is stored in local state, which means users who refresh the page or share the URL will lose their page position and return to page 1. Syncing with URL query parameters (e.g.,
/blog?page=3) would improve UX by:
- Making page positions shareable
- Supporting browser back/forward navigation
- Allowing users to bookmark specific pages
Example implementation:
// Initialize from URL const [currentPage, setCurrentPage] = useState(() => { const pageParam = router.query.page; return pageParam ? parseInt(pageParam as string, 10) : 1; }); // Update handlers to push to URL const goToNextPage = () => { if (currentPage < totalPages) { const nextPage = currentPage + 1; setCurrentPage(nextPage); router.push({ pathname: router.pathname, query: { ...router.query, page: nextPage } }, undefined, { shallow: true }); } };
📜 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 (2)
next-env.d.ts(1 hunks)pages/blog/index.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- next-env.d.ts
🔇 Additional comments (5)
pages/blog/index.tsx (5)
17-196: This file change appears unrelated to the PR objectives.The PR objectives state this PR is meant to clarify the
protocolVersionfield in AsyncAPI documentation. However, this file implements blog post pagination functionality, which has no connection to the AsyncAPI documentation clarification.Consider separating unrelated changes into different PRs to maintain focus and make reviews more straightforward.
17-18: LGTM!Good practice to define the pagination size as a named constant. The value of 15 posts per page is reasonable for a blog listing.
43-46: LGTM!Good UX decision to reset to page 1 when applying filters. This prevents users from landing on an empty page if the filtered results have fewer pages than their current page number.
63-63: LGTM!Consistent with the filter reset behavior. Ensures users return to the first page when clearing filters.
67-72: LGTM!The pagination calculations are correct and use standard formulas. Edge cases (empty posts, partial pages) are handled properly.
| {totalPages > 1 && ( | ||
| <div className='mt-12 flex items-center justify-center space-x-4'> | ||
| <button | ||
| onClick={goToPreviousPage} | ||
| disabled={currentPage === 1} | ||
| className={`px-6 py-2 rounded-md font-medium transition-all duration-200 ${ | ||
| currentPage === 1 | ||
| ? 'bg-gray-200 text-gray-400 cursor-not-allowed' | ||
| : 'bg-indigo-600 text-white hover:bg-indigo-700 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:ring-offset-2' | ||
| }`} | ||
| > | ||
| Previous | ||
| </button> | ||
|
|
||
| <div className='flex items-center space-x-2'> | ||
| <span className='text-sm text-gray-700'> | ||
| Page {currentPage} of {totalPages} | ||
| </span> | ||
| <span className='text-sm text-gray-500'> | ||
| ({totalPosts} total posts) | ||
| </span> | ||
| </div> | ||
|
|
||
| <button | ||
| onClick={goToNextPage} | ||
| disabled={currentPage === totalPages} | ||
| className={`px-6 py-2 rounded-md font-medium transition-all duration-200 ${ | ||
| currentPage === totalPages | ||
| ? 'bg-gray-200 text-gray-400 cursor-not-allowed' | ||
| : 'bg-indigo-600 text-white hover:bg-indigo-700 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:ring-offset-2' | ||
| }`} | ||
| > | ||
| Next | ||
| </button> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add accessibility attributes for pagination controls.
The pagination UI is missing semantic HTML and ARIA attributes that are essential for screen reader users to navigate effectively. The buttons lack proper labels, and the pagination region lacks semantic structure.
Apply these improvements:
{/* Pagination Controls */}
{totalPages > 1 && (
- <div className='mt-12 flex items-center justify-center space-x-4'>
+ <nav className='mt-12 flex items-center justify-center space-x-4' aria-label='Blog pagination'>
<button
onClick={goToPreviousPage}
disabled={currentPage === 1}
+ aria-label='Go to previous page'
className={`px-6 py-2 rounded-md font-medium transition-all duration-200 ${
currentPage === 1
? 'bg-gray-200 text-gray-400 cursor-not-allowed'
: 'bg-indigo-600 text-white hover:bg-indigo-700 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:ring-offset-2'
}`}
>
Previous
</button>
- <div className='flex items-center space-x-2'>
+ <div className='flex items-center space-x-2' aria-live='polite' aria-atomic='true'>
<span className='text-sm text-gray-700'>
Page {currentPage} of {totalPages}
</span>
<span className='text-sm text-gray-500'>
({totalPosts} total posts)
</span>
</div>
<button
onClick={goToNextPage}
disabled={currentPage === totalPages}
+ aria-label='Go to next page'
className={`px-6 py-2 rounded-md font-medium transition-all duration-200 ${
currentPage === totalPages
? 'bg-gray-200 text-gray-400 cursor-not-allowed'
: 'bg-indigo-600 text-white hover:bg-indigo-700 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:ring-offset-2'
}`}
>
Next
</button>
- </div>
+ </nav>
)}Key improvements:
- Wrap in
<nav>witharia-labelfor semantic structure - Add
aria-labelto buttons for clear purpose - Add
aria-live='polite'to page info so screen readers announce page changes
🤖 Prompt for AI Agents
In pages/blog/index.tsx around lines 160 to 195, the pagination markup needs
accessibility improvements: wrap the entire pagination block in a <nav> with an
appropriate aria-label (e.g., "Pagination") to provide semantic structure; add
explicit aria-labels to the Previous and Next buttons (e.g., "Previous page" and
"Next page") and include aria-disabled or keep disabled attribute for disabled
state; and add aria-live="polite" to the page info container (the "Page X of Y /
(N total posts)" text) so screen readers are notified when the page changes.
Ensure the nav contains the existing controls and that the button labels are
descriptive and updated to reflect disabled state for assistive tech.
2 participants
This fixes #4169
This pull request clarifies the documentation for the
protocolVersionfield in the AsyncAPI server object, making it clear that this field refers specifically to the version of the underlying communication protocol and not to the AsyncAPI specification or the application's API version.Documentation improvements for
protocolVersion:markdown/docs/concepts/asyncapi-document/structure.mdto specify thatprotocolVersionrefers to the underlying communication protocol version, with examples, and clarified its distinction from the AsyncAPI spec or application API version.markdown/docs/reference/specification/v3.0.0.mdto reinforce thatprotocolVersionis about the protocol itself, not the document or API version, and provided guidance to omit the field if the protocol lacks versioning.Summary by CodeRabbit