Merged
Conversation
- Change quote style from double to single quotes throughout knowledge base components - Update "Hide Sources" button label to "Hide Configuration" for clarity - Restructure SourceChunksPage layout to use xl:container for consistent spacing - Add controlled page input state with validation on blur and Enter key - Synchronize page input field with pagination controls to prevent state drift - Reset page input to "1" when changing page
deon-sanchez
changed the title
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughTwo UI changes: a label update in the KnowledgeBase upload modal and a refactoring of pagination controls in the Source Chunks page to use controlled input with numeric validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ 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 |
github-actions
bot
added
bug
deon-sanchez
commented
Mar 3, 2026
Comment on lines
+67
to
+94
| setPageInput("1"); | ||
| }; | ||
|
|
||
| const handlePageInputChange = (value: string) => { | ||
| setPageInput(value); | ||
| }; | ||
|
|
||
| const handlePageInputBlur = () => { | ||
| const value = parseInt(pageInput, 10); | ||
| if (!isNaN(value) && value >= 1 && value <= totalPages) { | ||
| setCurrentPage(value); | ||
| setPageInput(String(value)); | ||
| } else { | ||
| setPageInput(String(currentPage)); | ||
| } | ||
| }; | ||
|
|
||
| const handlePageInputKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { | ||
| if (e.key === "Enter") { | ||
| const value = parseInt(pageInput, 10); | ||
| if (!isNaN(value) && value >= 1 && value <= totalPages) { | ||
| setCurrentPage(value); | ||
| setPageInput(String(value)); | ||
| } else { | ||
| setPageInput(String(currentPage)); | ||
| } | ||
| e.currentTarget.blur(); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
address pagination issue with black arrows on safari, we change to text field to only integers and update on enter or unblur
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
bot
added
bug
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (34.28%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12023 +/- ##
==========================================
- Coverage 37.46% 37.43% -0.03%
==========================================
Files 1616 1616
Lines 79041 79060 +19
Branches 11945 11946 +1
==========================================
- Hits 29612 29596 -16
- Misses 47771 47806 +35
Partials 1658 1658
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/pages/MainPage/pages/knowledgePage/sourceChunksPage/SourceChunksPage.tsx (1)
30-37:⚠️ Potential issue | 🟠 MajorReset
pageInputwhen search resets pagination.At Line 35,
currentPageis reset to1, butpageInputis not. This can leave stale input text and cause unintended jumps when the field blurs.Suggested patch
debounceTimer.current = setTimeout(() => { setDebouncedSearch(value); setCurrentPage(1); + setPageInput("1"); }, 300);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/pages/MainPage/pages/knowledgePage/sourceChunksPage/SourceChunksPage.tsx` around lines 30 - 37, handleSearchChange resets pagination by calling setCurrentPage(1) but does not update the page input state, leaving stale text in the page input; inside the debounce timeout (the callback in handleSearchChange) also call the page input updater (e.g., setPageInput("1") or reset pageInput ref) so the visible page input matches the new currentPage; keep the existing debounceTimer clear/set logic and ensure you reference handleSearchChange, setDebouncedSearch, setCurrentPage and the page input state updater (setPageInput or equivalent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/src/pages/MainPage/pages/knowledgePage/sourceChunksPage/SourceChunksPage.tsx`:
- Around line 255-267: The page number input in SourceChunksPage.tsx lacks an
accessible label; add an explicit label (either a <label htmlFor="..."> paired
with the input id or an aria-label on the input) so screen readers announce its
purpose. Update the input element used with handlers handlePageInputChange,
handlePageInputBlur, handlePageInputKeyDown and value pageInput to include
id="page-number" (or similar) and add a matching <label
htmlFor="page-number">Page</label>, or alternatively add aria-label="Page
number" directly on the input; ensure the label text or aria-label mentions
totalPages if desired (e.g., "Page number of N") and that styling/layout still
matches the existing pagination UI.
---
Outside diff comments:
In
`@src/frontend/src/pages/MainPage/pages/knowledgePage/sourceChunksPage/SourceChunksPage.tsx`:
- Around line 30-37: handleSearchChange resets pagination by calling
setCurrentPage(1) but does not update the page input state, leaving stale text
in the page input; inside the debounce timeout (the callback in
handleSearchChange) also call the page input updater (e.g., setPageInput("1") or
reset pageInput ref) so the visible page input matches the new currentPage; keep
the existing debounceTimer clear/set logic and ensure you reference
handleSearchChange, setDebouncedSearch, setCurrentPage and the page input state
updater (setPageInput or equivalent).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/frontend/src/modals/knowledgeBaseUploadModal/KnowledgeBaseUploadModal.tsxsrc/frontend/src/pages/MainPage/pages/knowledgePage/sourceChunksPage/SourceChunksPage.tsx
Comment on lines
+255
to
+267
| <input | ||
| type="number" | ||
| min={1} | ||
| max={totalPages} | ||
| value={pageInput} | ||
| onChange={(e) => | ||
| handlePageInputChange(e.target.value) | ||
| } | ||
| onBlur={handlePageInputBlur} | ||
| onKeyDown={handlePageInputKeyDown} | ||
| className="h-7 w-16 rounded border border-input bg-background px-2 text-center text-sm focus:outline-none focus:ring-1 focus:ring-ring [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-inner-spin-button]:opacity-100 [&::-webkit-inner-spin-button]:[filter:invert(1)] [&::-webkit-outer-spin-button]:appearance-none [&::-webkit-outer-spin-button]:opacity-100 [&::-webkit-outer-spin-button]:[filter:invert(1)]" | ||
| /> | ||
| <span>of {totalPages}</span> |
Contributor
There was a problem hiding this comment.
Add an explicit accessible label for the page input.
The number input has nearby text, but no semantic label association. Add an aria-label (or <label htmlFor> pair) so screen readers announce its purpose reliably.
Suggested patch
<input
type="number"
+ aria-label="Page number"
min={1}
max={totalPages}
value={pageInput}📝 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.
Suggested change
| <input | |
| type="number" | |
| min={1} | |
| max={totalPages} | |
| value={pageInput} | |
| onChange={(e) => | |
| handlePageInputChange(e.target.value) | |
| } | |
| onBlur={handlePageInputBlur} | |
| onKeyDown={handlePageInputKeyDown} | |
| className="h-7 w-16 rounded border border-input bg-background px-2 text-center text-sm focus:outline-none focus:ring-1 focus:ring-ring [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-inner-spin-button]:opacity-100 [&::-webkit-inner-spin-button]:[filter:invert(1)] [&::-webkit-outer-spin-button]:appearance-none [&::-webkit-outer-spin-button]:opacity-100 [&::-webkit-outer-spin-button]:[filter:invert(1)]" | |
| /> | |
| <span>of {totalPages}</span> | |
| <input | |
| type="number" | |
| aria-label="Page number" | |
| min={1} | |
| max={totalPages} | |
| value={pageInput} | |
| onChange={(e) => | |
| handlePageInputChange(e.target.value) | |
| } | |
| onBlur={handlePageInputBlur} | |
| onKeyDown={handlePageInputKeyDown} | |
| className="h-7 w-16 rounded border border-input bg-background px-2 text-center text-sm focus:outline-none focus:ring-1 focus:ring-ring [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-inner-spin-button]:opacity-100 [&::-webkit-inner-spin-button]:[filter:invert(1)] [&::-webkit-outer-spin-button]:appearance-none [&::-webkit-outer-spin-button]:opacity-100 [&::-webkit-outer-spin-button]:[filter:invert(1)]" | |
| /> | |
| <span>of {totalPages}</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/pages/MainPage/pages/knowledgePage/sourceChunksPage/SourceChunksPage.tsx`
around lines 255 - 267, The page number input in SourceChunksPage.tsx lacks an
accessible label; add an explicit label (either a <label htmlFor="..."> paired
with the input id or an aria-label on the input) so screen readers announce its
purpose. Update the input element used with handlers handlePageInputChange,
handlePageInputBlur, handlePageInputKeyDown and value pageInput to include
id="page-number" (or similar) and add a matching <label
htmlFor="page-number">Page</label>, or alternatively add aria-label="Page
number" directly on the input; ensure the label text or aria-label mentions
totalPages if desired (e.g., "Page number of N") and that styling/layout still
matches the existing pagination UI.
Cristhianzl
requested changes
Mar 4, 2026
Member
Cristhianzl
left a comment
Cristhianzl
left a comment
There was a problem hiding this comment.
🔴 CRITICAL: DRY Principle
| Check | Status |
|---|---|
| No duplicate logic | ❌ VIOLATION |
Issue found: handlePageInputBlur and handlePageInputKeyDown contain identical validation logic (lines 76-83 and 87-94 in the diff):
// Duplicated block in BOTH handlers:
const value = parseInt(pageInput, 10);
if (!isNaN(value) && value >= 1 && value <= totalPages) {
setCurrentPage(value);
setPageInput(String(value));
} else {
setPageInput(String(currentPage));
}Recommended fix: Extract to a shared helper:
const commitPageInput = () => {
const value = parseInt(pageInput, 10);
if (!isNaN(value) && value >= 1 && value <= totalPages) {
setCurrentPage(value);
setPageInput(String(value));
} else {
setPageInput(String(currentPage));
}
};
const handlePageInputBlur = () => {
commitPageInput();
};
const handlePageInputKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => {
if (e.key === "Enter") {
commitPageInput();
e.currentTarget.blur();
}
};Result: ❌ FAIL — Must fix before merge
🟢 TESTING
| Check | Status |
|---|---|
| Tests included | ❌ No tests in PR |
| Happy path tests | ❌ Missing |
| Adversarial tests | ❌ Missing |
| Coverage validated | ❌ Not run |
Missing test scenarios:
- Happy path: Type valid page number → press Enter → navigates to page
- Happy path: Type valid page number → blur → navigates to page
- Edge case: Type
0→ blur → reverts to current page - Edge case: Type number > totalPages → blur → reverts to current page
- Edge case: Type non-numeric → blur → reverts to current page
- Edge case: Type negative number → blur → reverts to current page
- Integration: Pagination buttons keep input in sync
Result: ❌ FAIL — New behavior should have tests
Extract page input validation and commit logic from handlePageInputBlur and handlePageInputKeyDown into a shared commitPageInput function to eliminate code duplication.
github-actions
bot
added
bug
github-actions
bot
added
bug
HimavarshaVS
pushed a commit
that referenced
this pull request
Mar 10, 2026
* fix: improve knowledge base UI consistency and pagination handling - Change quote style from double to single quotes throughout knowledge base components - Update "Hide Sources" button label to "Hide Configuration" for clarity - Restructure SourceChunksPage layout to use xl:container for consistent spacing - Add controlled page input state with validation on blur and Enter key - Synchronize page input field with pagination controls to prevent state drift - Reset page input to "1" when changing page * refactor: extract page input commit logic into reusable function Extract page input validation and commit logic from handlePageInputBlur and handlePageInputKeyDown into a shared commitPageInput function to eliminate code duplication.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request enhances the usability and consistency of the
SourceChunksPagepagination controls and improves layout structure for better alignment and responsiveness. It also includes a small wording update in the knowledge base upload modal. The most significant changes are grouped below:Pagination and Input Handling Improvements:
pageInputstate to decouple the pagination input field from the actualcurrentPage, allowing for better user experience and validation. Handlers for input changes, blur, and Enter key were introduced to validate and update the page correctly. [1] [2] [3]pageInputin sync withcurrentPage, ensuring consistent UI updates when navigating between pages. [1] [2] [3]Layout and Structure Enhancements:
xl:containerfor improved alignment and responsiveness across the page, including the header, search bar, chunk list, and pagination controls. [1] [2] [3] [4] [5] [6]Minor Improvements:
Summary by CodeRabbit