fix dataset list in group and organization #29
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR refactors dataset list handling from static, pre-fetched props to dynamic server-fetched lists with client-side pagination. Key changes include converting Changes
Sequence DiagramsequenceDiagram
autonumber
participant Browser
participant Page (SSR)
participant searchDatasets API
participant DatasetList Component
participant SWR Cache
participant Pagination UI
Note over Browser,Pagination UI: Server-Side Initial Fetch
Browser->>Page (SSR): Request page
Page (SSR)->>searchDatasets API: Fetch initialDatasets (offset=0, limit=10)
searchDatasets API-->>Page (SSR): Return paginated datasets + count
Page (SSR)-->>Browser: Render page with initialDatasets
Note over Browser,Pagination UI: Client-Side Interaction
Browser->>DatasetList Component: Mount with initialDatasets
DatasetList Component->>SWR Cache: Use initialDatasets as fallbackData
SWR Cache-->>DatasetList Component: Hydrate cache with initial data
DatasetList Component->>Pagination UI: Render with count > limit
Browser->>Pagination UI: Click next page
Pagination UI->>DatasetList Component: Trigger onPageChange(offset=10)
DatasetList Component->>SWR Cache: Fetch datasets at new offset via SWR
SWR Cache->>searchDatasets API: Request datasets (offset=10, limit=10)
searchDatasets API-->>SWR Cache: Return new dataset page
SWR Cache-->>DatasetList Component: Update dataset list
DatasetList Component->>Browser: Re-render with new page
Browser->>Browser: Scroll to top
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
pages/[org]/index.tsx (1)
67-67: Add TypeScript types for function parameters.The function parameters lack type annotations. Adding explicit types improves type safety and IDE support.
Apply this diff:
-export default function OrgPage({ org, initialDatasets }): JSX.Element { +export default function OrgPage({ + org, + initialDatasets +}: { + org: Organization & { activity_stream?: any }; + initialDatasets: any; +}): JSX.Element {Note: Consider defining a more specific type for
initialDatasetsinstead ofany(e.g., the return type ofsearchDatasets).pages/groups/[groupName].tsx (1)
59-59: Add TypeScript types for function parameters.The function parameters lack type annotations. Adding explicit types improves type safety and IDE support.
Apply this diff:
-export default function GroupPage({ group, initialDatasets }): JSX.Element { +export default function GroupPage({ + group, + initialDatasets +}: { + group: Group & { activity_stream?: any }; + initialDatasets: any; +}): JSX.Element {Note: Consider defining a more specific type for
initialDatasetsinstead ofany(e.g., the return type ofsearchDatasets).components/_shared/PaginationOrgGroup.tsx (4)
3-9: Consider exporting PaginationProps interface.The
PaginationPropsinterface is not exported, which may limit reusability and type checking in other parts of the codebase.Apply this diff if the interface needs to be reused elsewhere:
-interface PaginationProps { +export interface PaginationProps {
18-19: Hardcoded page size should match DatasetList limit.The
max = 10is hardcoded here and must match thelimit = 10inDatasetList.tsx(line 18). Consider extracting this as a shared constant to prevent synchronization issues.Create a shared constant file (e.g.,
lib/constants.ts):export const DATASET_PAGE_SIZE = 10;Then import and use it in both files:
+import { DATASET_PAGE_SIZE } from "@/lib/constants"; + - const max = 10; + const max = DATASET_PAGE_SIZE;
28-91: Add accessibility attributes to pagination controls.The pagination buttons lack proper accessibility attributes, which may hinder screen reader users and keyboard navigation.
Consider adding:
aria-labelattributes to Prev/Next buttons describing their actionaria-current="page"to the active page buttondisabledattribute and styling when buttons shouldn't be clickable- Role attributes if needed
Example for the active page button:
<button key={x} className={`${ x === currentPage ? "bg-accent-100 !h-9 !w-9 rounded-[10px] text-black" : "" } px-2 font-semibold`} onClick={() => handlePageClick(x)} + aria-label={`Go to page ${x + 1}`} + aria-current={x === currentPage ? "page" : undefined} style={{ display: x >= subsetOfPages && x < subsetOfPages + max ? "block" : "none", }} > {x + 1} </button>
68-68: Simplify the Next button visibility condition.The condition
count > max * max && (subsetOfPages + max) * max < countis complex and may be hard to maintain. Consider adding a comment or extracting it into a well-named variable.Apply this diff:
- {count > max * max && (subsetOfPages + max) * max < count && ( + {(() => { + const hasMoreThan100Items = count > max * max; + const moreSubsetsExist = (subsetOfPages + max) * max < count; + return hasMoreThan100Items && moreSubsetsExist; + })() && (Or add a comment:
+ {/* Show Next button if: more than 100 items total AND current subset window hasn't reached the end */} {count > max * max && (subsetOfPages + max) * max < count && (components/_shared/DatasetList.tsx (3)
9-13: Replaceanytype for initialDatasets with a specific type.The
initialDatasetsprop is typed asany, which loses type safety. It should match the return type ofsearchDatasets.Define a proper type (you may need to adjust based on the actual return type):
+import { PackageSearchResult } from "@/lib/queries/dataset"; // or wherever the type is defined + interface DatasetListProps { type: "organization" | "group"; name: string; - initialDatasets?: any; + initialDatasets?: PackageSearchResult | null; }
15-18: Extract hardcoded limit as a shared constant.The
limit = 10is hardcoded here and inPaginationOrgGroup.tsx(asmax = 10). Extract this as a shared constant to prevent synchronization issues.Create a shared constant file (e.g.,
lib/constants.ts):export const DATASET_PAGE_SIZE = 10;Then import and use it:
+import { DATASET_PAGE_SIZE } from "@/lib/constants"; + - const limit = 10; + const limit = DATASET_PAGE_SIZE;
24-44: Add error handling for the SWR fetcher function.The fetcher function doesn't handle errors from
searchDatasets. While SWR has built-in error handling, you should consider catching and logging errors for better debugging.Apply this diff:
const { data: searchResults, isValidating } = useSWR( ["entity_package_search", { fq, offset, limit }], async (api, options) => { - return searchDatasets({ - fq: options.fq, - offset: options.offset, - limit: options.limit, - type: "dataset", - query: "", - sort: "metadata_modified desc", - groups: [], - orgs: [], - tags: [], - resFormat: [], - }); + try { + return await searchDatasets({ + fq: options.fq, + offset: options.offset, + limit: options.limit, + type: "dataset", + query: "", + sort: "metadata_modified desc", + groups: [], + orgs: [], + tags: [], + resFormat: [], + }); + } catch (error) { + console.error("Failed to fetch datasets:", error); + throw error; // Re-throw to let SWR handle it + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/_shared/DatasetList.tsx(1 hunks)components/_shared/PaginationOrgGroup.tsx(1 hunks)components/groups/individualPage/GroupInfo.tsx(1 hunks)components/organization/individualPage/OrgInfo.tsx(1 hunks)pages/[org]/index.tsx(3 hunks)pages/groups/[groupName].tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
components/_shared/DatasetList.tsx (2)
lib/queries/dataset.ts (1)
searchDatasets(13-97)components/_shared/PaginationOrgGroup.tsx (1)
Pagination(11-92)
pages/groups/[groupName].tsx (2)
lib/queries/dataset.ts (1)
searchDatasets(13-97)components/_shared/DatasetList.tsx (1)
DatasetList(15-96)
pages/[org]/index.tsx (2)
lib/queries/dataset.ts (1)
searchDatasets(13-97)components/_shared/DatasetList.tsx (1)
DatasetList(15-96)
🔇 Additional comments (13)
components/organization/individualPage/OrgInfo.tsx (1)
44-44: LGTM! Package count source updated correctly.The change from
org.packages?.lengthtoorg.package_count || 0correctly aligns with the new data structure where package counts are provided directly rather than via array length.pages/[org]/index.tsx (4)
13-13: Import added correctly.The
searchDatasetsimport is properly added to support the new server-side initial dataset fetching.
30-30: Correct optimization: datasets fetched separately.Setting
include_datasets: falseis appropriate since datasets are now fetched separately viasearchDatasetsfor better performance and pagination support.
71-73: DatasetList integration looks good.The updated DatasetList usage correctly passes
type="organization", the organization name, andinitialDatasetsfor server-side rendering with client-side pagination.
39-39: <!-- [scratchpad_start]
[task verification_1/1]
[observations]
- searchDatasets function receives fq as-is from options and passes it directly to CKAN API (line 42-44, 66)
- No normalization or sanitization of fq parameter occurs
- Inconsistency confirmed across codebase:
- pages/[org]/index.tsx:39 →
organization:${org._name}(NO parentheses)- pages/groups/[groupName].tsx:32 →
groups:${group._name}(NO parentheses)- components/_shared/DatasetList.tsx:21 →
(organization:${name})(WITH parentheses)- lib/queries/dataset.ts:47 →
organization:(${joinTermsWithOr(...)})(WITH parentheses, multi-value)- Patterns: single-value server-side = no parens; client-side and multi-value = with parens
[analysis]
- Inconsistency is REAL and verifiable
- fq parameters are combined with space joins which is AND logic in Solr/Lucene
- Parentheses generally valid in both forms but affect query parsing/precedence
- No code normalizes these values before CKAN API call
- Potential risk: query behavior could differ based on parenthesization if CKAN is strict
[pending]
- CKAN API documentation or behavior verification not possible without external testing
- Both formats appear to work in Solr/Lucene but consistency is better practice
[actions]
- Identify specific inconsistency locations
- Determine if inconsistency needs fixing or if both are valid
- Frame recommendation clearly
[done]
- Verified inconsistency is real across four locations
- Verified no normalization happens in searchDatasets function
- Both parenthesized and non-parenthesized patterns exist in codebase
</scratchpad_start] -->Fix fq filter syntax inconsistency: use parentheses consistently.
An inconsistency exists in how the fq filter is formatted across the codebase:
pages/[org]/index.tsx:39usesorganization:${org._name}(no parentheses)pages/groups/[groupName].tsx:32usesgroups:${group._name}(no parentheses)components/_shared/DatasetList.tsx:21uses(organization:${name})(with parentheses)lib/queries/dataset.ts:47usesorganization:(${joinTermsWithOr(...)})(with parentheses, multi-value case)The
searchDatasetsfunction passes the fq parameter directly to the CKAN API without normalization. While both formats may be valid in Solr/Lucene syntax, parenthesized filters are used for multi-value queries and client-side components. Standardize all single-value organization and group filters to use parentheses for consistency:(organization:${org._name})and(groups:${group._name}).pages/groups/[groupName].tsx (4)
12-12: Import added correctly.The
searchDatasetsimport properly supports the new server-side initial dataset fetching for groups.
27-27: Correct optimization: datasets fetched separately.Setting
include_datasets: falseis appropriate since datasets are now fetched separately viasearchDatasetsfor better performance and pagination support.
63-65: DatasetList integration looks good.The updated DatasetList usage correctly passes
type="group", the group name, andinitialDatasetsfor server-side rendering with client-side pagination.
32-32: No CKAN API compatibility issues found.CKAN's package_search fq parameter uses Apache Solr filter query syntax, where both
groups:nameand(groups:name)are semantically equivalent for single field filters. The variation in parentheses usage acrosspages/groups/[groupName].tsx(line 32),components/_shared/DatasetList.tsx(line 22), andlib/queries/dataset.ts(line 51) reflects stylistic differences rather than functional problems. Parentheses in Solr are only required when grouping multiple clauses or applying boolean operators, so the single-entity filters work correctly regardless of parentheses.components/_shared/DatasetList.tsx (3)
20-22: fq filter syntax wrapped in parentheses.The fq filter wraps the entire expression in parentheses (e.g.,
(organization:${name})), but the server-side calls inpages/[org]/index.tsx(line 39) andpages/groups/[groupName].tsx(line 32) don't use parentheses. Verify this doesn't cause inconsistent behavior with the CKAN API.See comments in the page files for verification scripts to check fq parameter usage patterns.
59-73: Loading and empty states implemented correctly.The component properly handles loading and empty states, providing good user feedback during data fetching and when no results are found.
75-95: Pagination integration looks good.The Pagination component is properly integrated with smooth scrolling and conditional rendering based on the dataset count.
components/groups/individualPage/GroupInfo.tsx (1)
36-36: LGTM! Package count source updated correctly.The change from
group.packages?.lengthtogroup.package_count || 0correctly aligns with the new data structure where package counts are provided directly rather than via array length.
| } | ||
| }) | ||
| ); | ||
| console.log("Fetched organization:", org); |
There was a problem hiding this comment.
Remove debug console.log before merging.
This console.log statement appears to be debug code and should be removed before merging to production.
Apply this diff:
- console.log("Fetched organization:", org);
-📝 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.
| console.log("Fetched organization:", org); |
🤖 Prompt for AI Agents
In pages/[org]/index.tsx around line 33, remove the debug console.log("Fetched
organization:", org); statement; delete that line from the file and ensure no
other stray console.debug/console.log calls remain in this component (replace
with proper app logger if persistent logging is required).
| let initialDatasets = null | ||
|
|
||
| if (org.package_count) { | ||
| initialDatasets = await searchDatasets({ | ||
| fq: `organization:${org._name}`, | ||
| offset: 0, | ||
| limit: 10, | ||
| type: "dataset", | ||
| query: "", | ||
| sort: "metadata_modified desc", | ||
| groups: [], | ||
| orgs: [], | ||
| tags: [], | ||
| resFormat: [], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for searchDatasets call.
The searchDatasets call lacks error handling. If the API call fails, it could cause the page to error or return unexpected data to the client.
Consider wrapping the call in a try-catch block:
let initialDatasets = null
if (org.package_count) {
- initialDatasets = await searchDatasets({
- fq: `organization:${org._name}`,
- offset: 0,
- limit: 10,
- type: "dataset",
- query: "",
- sort: "metadata_modified desc",
- groups: [],
- orgs: [],
- tags: [],
- resFormat: [],
- });
+ try {
+ initialDatasets = await searchDatasets({
+ fq: `organization:${org._name}`,
+ offset: 0,
+ limit: 10,
+ type: "dataset",
+ query: "",
+ sort: "metadata_modified desc",
+ groups: [],
+ orgs: [],
+ tags: [],
+ resFormat: [],
+ });
+ } catch (error) {
+ console.error("Failed to fetch initial datasets:", error);
+ // initialDatasets remains null, component will handle gracefully
+ }
}📝 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.
| let initialDatasets = null | |
| if (org.package_count) { | |
| initialDatasets = await searchDatasets({ | |
| fq: `organization:${org._name}`, | |
| offset: 0, | |
| limit: 10, | |
| type: "dataset", | |
| query: "", | |
| sort: "metadata_modified desc", | |
| groups: [], | |
| orgs: [], | |
| tags: [], | |
| resFormat: [], | |
| }); | |
| } | |
| let initialDatasets = null | |
| if (org.package_count) { | |
| try { | |
| initialDatasets = await searchDatasets({ | |
| fq: `organization:${org._name}`, | |
| offset: 0, | |
| limit: 10, | |
| type: "dataset", | |
| query: "", | |
| sort: "metadata_modified desc", | |
| groups: [], | |
| orgs: [], | |
| tags: [], | |
| resFormat: [], | |
| }); | |
| } catch (error) { | |
| console.error("Failed to fetch initial datasets:", error); | |
| // initialDatasets remains null, component will handle gracefully | |
| } | |
| } |
🤖 Prompt for AI Agents
In pages/[org]/index.tsx around lines 35 to 50, the call to searchDatasets is
unprotected and can throw, so wrap the await searchDatasets(...) in a try-catch
block; on error log the error with context (e.g., include org._name), set
initialDatasets to a safe fallback (null or an empty array depending on
downstream expectations), and avoid rethrowing to prevent crashing the page (or
rethrow after logging if upstream should handle it).
| let initialDatasets = null; | ||
| if (group.package_count) { | ||
| initialDatasets = await searchDatasets({ | ||
| fq: `groups:${group._name}`, | ||
| offset: 0, | ||
| limit: 10, | ||
| type: "dataset", | ||
| query: "", | ||
| sort: "metadata_modified desc", | ||
| groups: [], | ||
| orgs: [], | ||
| tags: [], | ||
| resFormat: [], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for searchDatasets call.
The searchDatasets call lacks error handling. If the API call fails, it could cause the page to error or return unexpected data to the client.
Consider wrapping the call in a try-catch block:
let initialDatasets = null;
if (group.package_count) {
- initialDatasets = await searchDatasets({
- fq: `groups:${group._name}`,
- offset: 0,
- limit: 10,
- type: "dataset",
- query: "",
- sort: "metadata_modified desc",
- groups: [],
- orgs: [],
- tags: [],
- resFormat: [],
- });
+ try {
+ initialDatasets = await searchDatasets({
+ fq: `groups:${group._name}`,
+ offset: 0,
+ limit: 10,
+ type: "dataset",
+ query: "",
+ sort: "metadata_modified desc",
+ groups: [],
+ orgs: [],
+ tags: [],
+ resFormat: [],
+ });
+ } catch (error) {
+ console.error("Failed to fetch initial datasets:", error);
+ // initialDatasets remains null, component will handle gracefully
+ }
}📝 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.
| let initialDatasets = null; | |
| if (group.package_count) { | |
| initialDatasets = await searchDatasets({ | |
| fq: `groups:${group._name}`, | |
| offset: 0, | |
| limit: 10, | |
| type: "dataset", | |
| query: "", | |
| sort: "metadata_modified desc", | |
| groups: [], | |
| orgs: [], | |
| tags: [], | |
| resFormat: [], | |
| }); | |
| } | |
| let initialDatasets = null; | |
| if (group.package_count) { | |
| try { | |
| initialDatasets = await searchDatasets({ | |
| fq: `groups:${group._name}`, | |
| offset: 0, | |
| limit: 10, | |
| type: "dataset", | |
| query: "", | |
| sort: "metadata_modified desc", | |
| groups: [], | |
| orgs: [], | |
| tags: [], | |
| resFormat: [], | |
| }); | |
| } catch (error) { | |
| console.error("Failed to fetch initial datasets:", error); | |
| // initialDatasets remains null, component will handle gracefully | |
| } | |
| } |
🤖 Prompt for AI Agents
In pages/groups/[groupName].tsx around lines 29 to 43, the call to
searchDatasets has no error handling; wrap the await searchDatasets(...) call in
a try-catch, on error log the error (use existing logger or console.error), and
set initialDatasets to a safe fallback (null or an empty result structure) so
the page can render instead of crashing; optionally rethrow or report to
monitoring only if you want build-time failure.
Summary by CodeRabbit
New Features
Improvements