fix(metadata-view): Use internal pagination #4263
Conversation
WalkthroughAdds marker-based pagination to metadata view in ContentExplorer, deriving next/prev markers from state and wiring them via metadataViewProps. Retains Footer offset-based pagination, gated to avoid duplicate controls when metadata view v2 is active. Binds marker-based page change handler to update current page and refresh. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CE as ContentExplorer
participant MV as Metadata View
participant API as Items API
rect rgba(200,230,255,0.3)
note over CE,MV: Marker-based pagination (Metadata View v2)
U->>MV: Click Next/Prev (marker)
MV->>CE: onMarkerBasedPageChange(marker)
CE->>CE: Update currentPageNumber, markers
CE->>API: Fetch items using marker
API-->>CE: Items + next/prev markers
CE-->>MV: Render page with paginationProps (type='marker')
end
rect rgba(230,230,230,0.3)
note over U,CE: Offset-based pagination (Footer)
U->>CE: Click Next/Prev (offset)
CE->>API: Fetch items with offset
API-->>CE: Items + hasNext/hasPrevious
CE-->>U: Render Footer Pagination
end
note over CE: Footer hidden when MV v2 active to avoid duplicate controls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/elements/content-explorer/ContentExplorer.tsx (3)
1579-1587: Clamp and integerize marker pagination, fool!Without clamping/flooring,
currentPageNumbercan go negative or fractional (e.g., 3.7), leading to bad array indexing and flaky pagination. I pity the fool who lets page -1 render!Apply:
- markerBasedPaginate = (newOffset: number) => { - const { currentPageNumber } = this.state; - this.setState( - { - currentPageNumber: currentPageNumber + newOffset, // newOffset could be negative - }, - this.refreshCollection, - ); - }; + markerBasedPaginate = (newOffset: number) => { + this.setState( + ({ currentPageNumber }) => { + // Ensure non-negative, integer page index + const nextPage = Math.max(0, Math.floor(currentPageNumber + newOffset)); + return { currentPageNumber: nextPage }; + }, + this.refreshCollection, + ); + };
457-465: Stop mutating state directly, sucker.
markers[currentPageNumber] = ...mutates state in place. That’s a React anti-pattern and can cause sneaky bugs. Clone and set viasetState.Here’s a safe pattern for this section:
// Preserve the marker as part of the original query immutably let nextMarkers = markers; if (currentPageNumber === 0) { const cloned = [...markers]; cloned[currentPageNumber] = metadataQueryClone.marker; nextMarkers = cloned; } // ... then when resetting view/loading: this.setState({ searchQuery: '', currentCollection: this.currentUnloadedCollection(), view: VIEW_METADATA, markers: nextMarkers, });
947-956: Reset markers when the query/sort/filters change, fool.Changing sort or filters while keeping
currentPageNumber/markersstale will request the wrong page. Reset to page 0 and clear markers whenever metadata query inputs change.Apply conceptually in
sort()andfilterMetadata():- this.setState({ sortBy, sortDirection }, this.refreshCollection); + this.setState({ sortBy, sortDirection, currentPageNumber: 0, markers: [] }, this.refreshCollection); - this.setState({ metadataFilters: fields }, this.refreshCollection); + this.setState({ metadataFilters: fields, currentPageNumber: 0, markers: [] }, this.refreshCollection);Also applies to: 1750-1752
🧹 Nitpick comments (4)
src/elements/content-explorer/ContentExplorer.tsx (2)
1619-1622: DRY up marker flags and align page-base, sucka.You compute
hasNextMarker/hasPrevMarkerhere and again in render. Extract a helper to avoid divergence. Also verify 0-based vs 1-based page semantics match product expectations (state initializes at 0, buthasPrevforces true at page 1).Apply:
- const { currentPageNumber, markers, selectedItemIds } = this.state; - const hasNextMarker: boolean = !!markers[currentPageNumber + 1]; - const hasPrevMarker: boolean = currentPageNumber === 1 || !!markers[currentPageNumber - 1]; + const { selectedItemIds } = this.state; + const { hasNextMarker, hasPrevMarker } = this.getMarkerFlags();Add this helper method in the class (outside the shown range):
private getMarkerFlags() { const { currentPageNumber, markers } = this.state; return { hasNextMarker: !!markers[currentPageNumber + 1], hasPrevMarker: currentPageNumber === 1 || !!markers[currentPageNumber - 1], }; }
1840-1842: Name it the same everywhere.You compute
hasPreviousMarkerthen pass it ashasPrevMarker. Keep one name, fool, so future you doesn’t get confused.Suggested tweak (outside this hunk): rename
hasPreviousMarkertohasPrevMarkerin render and its usage for consistency.Also applies to: 1919-1921
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (2)
687-739: These tests ain’t testing behavior—only math.This suite checks arithmetic on locals, not the component’s state or props. Add behavior-driven tests (via Pagination callbacks) and align with integer clamping and non-negative pages.
Apply to make expectations integer-based and clamped:
- // Test decimal offset handling - currentPageNumber = 2; - newOffset = 1.7; - expectedPageNumber = currentPageNumber + newOffset; - expect(expectedPageNumber).toBe(3.7); + // Decimal offset should floor to int + currentPageNumber = 2; + newOffset = 1.7; + expectedPageNumber = Math.max(0, Math.floor(currentPageNumber + newOffset)); + expect(expectedPageNumber).toBe(3); - // Test edge cases - currentPageNumber = 0; - newOffset = 1; - expectedPageNumber = currentPageNumber + newOffset; - expect(expectedPageNumber).toBe(1); + // Edge: don’t go negative + currentPageNumber = 0; + newOffset = -1; + expectedPageNumber = Math.max(0, Math.floor(currentPageNumber + newOffset)); + expect(expectedPageNumber).toBe(0);Bonus: render CE in metadata view V2, grab the table’s pagination control, call its next/prev handlers, and assert
currentPageNumbervia a public prop/state probe or by verifying new fetches. I can sketch that if you want.
768-804: Sparse markers: good edge cases—add negative/overflow too.You covered null/undefined nicely. Add cases for:
- prev at page 0 (should be false),
- next beyond last known marker (should be false).
Happy to add those test cases for you, sucka.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-explorer/ContentExplorer.tsx (2)
1637-1643: Props look right—good hookup.Passing
type: 'marker'withonMarkerBasedPageChangeis the right contract for metadata view pagination. Solid wiring, chump.
1916-1929: Verify Pagination component supports atypeprop
I pity the fool who can’t find its implementation—ensurePaginationactually accepts and handles atypeprop before applying the refactor.src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
740-767: Prev-marker on first page: verify intent, fool.You assume “first page always has prev marker” (true for page 1 via special-case). Component initializes page at 0. Confirm the product wants prev enabled on page 1 and disabled on page 0.
If keeping this contract, add a test for page 0: prev should be disabled.
greg-in-a-box
force-pushed
the
pagination-mdv
branch
from
4e4a7b4 to
20268dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-explorer/ContentExplorer.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (3)
715-720: Ain’t no such thing as decimal pages.Pagination offsets must be integers. Kill the fractional case; it’s bogus.
- // Test decimal offset handling - currentPageNumber = 2; - newOffset = 1.7; - expectedPageNumber = currentPageNumber + newOffset; - expect(expectedPageNumber).toBe(3.7);Likely an incorrect or invalid review comment.
680-797: No duplicate “Marker-based pagination” suites found, fool!
740-749: Ignore off-by-one warning—tests match implementation
I pity the fool who misread it: tests simulatemarkers[currentPageNumber + 1]andmarkers[currentPageNumber - 1]exactly as implemented ingetMetadataViewProps. No change required.Likely an incorrect or invalid review comment.
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
greg-in-a-box
force-pushed
the
pagination-mdv
branch
from
20268dc to
aab0031
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/elements/content-explorer/ContentExplorer.tsx (2)
1952-1965: Fix Footer gating — don’t hide pagination in non-metadata views, fool!Current condition hides the Footer whenever the metadataViewV2 feature flag is on, even for Folder/Search/Recents. That nukes offset pagination where it’s still needed. Gate it only when actually in metadata view.
- {!isErrorView && !isMetadataViewV2Feature && ( + {!isErrorView && (view !== VIEW_METADATA || !isMetadataViewV2Feature) && ( <Footer> <Pagination hasNextMarker={hasNextMarker} - hasPrevMarker={hasPreviousMarker} + hasPrevMarker={hasPreviousMarker} isSmall={isSmall} offset={offset} onOffsetChange={this.paginate} pageSize={currentPageSize} totalCount={totalCount} - onMarkerBasedPageChange={this.markerBasedPaginate} + onMarkerBasedPageChange={view === VIEW_METADATA ? this.markerBasedPaginate : undefined} /> </Footer> )}
950-959: Reset marker pagination on sort/filter — stale markers ain’t welcome here!When sort or metadata filters change under marker-based pagination, you must reset
currentPageNumberto 0 and clearmarkers. Otherwise you’ll reuse stale markers and fetch the wrong page. I pity the fool who debugs that!@@ sort = (sortBy: SortBy | Key, sortDirection: SortDirection) => { const { currentCollection: { id }, view, }: State = this.state; if (id || view === VIEW_METADATA) { - this.setState({ sortBy, sortDirection }, this.refreshCollection); + this.setState( + { + sortBy, + sortDirection, + ...(view === VIEW_METADATA && { currentPageNumber: 0, markers: [] }), + }, + this.refreshCollection, + ); } }; @@ - filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { metadataFilters: fields, currentPageNumber: 0, markers: [] }, + this.refreshCollection, + ); + };Also applies to: 1786-1788
🧹 Nitpick comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1656-1659: Prev/next flags — DRY it up, sucka!You compute marker nav flags here and again in render. Centralize into a small helper to keep logic identical and readable.
Example helper (add near other utils in this class):
private getMarkerNavState = (page: number, markers: Array<string | null | undefined>) => ({ hasNextMarker: !!markers[page + 1], hasPrevMarker: page === 1 || !!markers[page - 1], });Then use
const { hasNextMarker, hasPrevMarker } = this.getMarkerNavState(currentPageNumber, markers);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/elements/content-explorer/ContentExplorer.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1671-1676: Verify paginationProps keys match MetadataViewProps, fool!
In getMetadataViewProps (ContentExplorer.tsx 1671–1676), confirm that MetadataView’s paginationProps acceptsonMarkerBasedPageChange,hasNextMarker,hasPrevMarker, and atype: 'marker'field per @box/metadata-view’s API.
3 participants
before
after
Summary by CodeRabbit