Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explorer): tables viewer pagination #3426

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

karooolis
Copy link
Contributor

@karooolis karooolis commented Jan 8, 2025

Paginate explorer's table by using SQL's offset/limit clauses.

pagination.mov

Copy link

changeset-bot bot commented Jan 8, 2025

🦋 Changeset detected

Latest commit: ecb1118

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@latticexyz/explorer Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/dev-tools Patch
@latticexyz/entrykit Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/paymaster Patch
@latticexyz/protocol-parser Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/stash Patch
@latticexyz/store-consumer Patch
@latticexyz/store-indexer Patch
@latticexyz/store-sync Patch
@latticexyz/store Patch
@latticexyz/utils Patch
vite-plugin-mud Patch
@latticexyz/world-module-callwithsignature Patch
@latticexyz/world-module-erc20 Patch
@latticexyz/world-module-metadata Patch
@latticexyz/world-modules Patch
@latticexyz/world Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@karooolis karooolis changed the title feat(explorer): paginate by sql offset/limit feat(explorer): paginate by sql offset/limit (wip) Jan 8, 2025
@karooolis karooolis force-pushed the kumpis/paginate-by-offset-limit branch from 8256f6b to 506bbfd Compare February 6, 2025 15:02
@karooolis karooolis force-pushed the kumpis/paginate-by-offset-limit branch from 2fd7c66 to d5e63db Compare February 6, 2025 15:43
@karooolis karooolis marked this pull request as ready for review February 7, 2025 12:55
@karooolis karooolis requested review from alvrs and holic as code owners February 7, 2025 12:55
@karooolis karooolis force-pushed the kumpis/paginate-by-offset-limit branch from f1d2931 to c84b9e2 Compare February 7, 2025 12:55
const [pagination, setPagination] = useState<PaginationState>({
pageIndex: page,
pageSize,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the extra state here when the state already lives in the url? why not just use query as source of truth instead of carrying it around twicE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I didn't like the URL query string it produced but actually it probably doesn't matter that much, and doing it like that is def nicer code, adjusted.

Copy link
Member

@holic holic Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

share more? what was the before/after? I didn't run this myself so I can't tell (and the URL isn't shown in video)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it looks like this &pagination=%7B%22pageSize%22:10,%22pageIndex%22:4%7D and before it was &page=2&pageSize=10. But I suppose it's a minor difference given most of the rest of the URL query is quite unreadable anyhow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh okay, I much prefer the nice/user friendly params

setPage(newPageIndex);
setPageSize(newPageSize);

const decodedQuery = decodeURIComponent(query);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the thing we use for query params doesn't give it back to use decoded already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answered below

};

export function getLimitOffset(query: string) {
const decodedQuery = decodeURIComponent(query);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as above, I am confused why this isn't already decoded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is but if we don't decodeURIComponent and encodeURIComponent, it loses new line characters inside the query. Found this to work reliably.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, a good time to put this into a reusable hook, added useSQLQueryState.ts

Comment on lines +5 to +11
return useQueryState(
"pagination",
parseAsJson<PaginationState>().withDefault({
pageIndex: 0,
pageSize: defaultPageSize,
}),
);
Copy link
Member

@holic holic Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this make sense to get nicer query params?

Suggested change
return useQueryState(
"pagination",
parseAsJson<PaginationState>().withDefault({
pageIndex: 0,
pageSize: defaultPageSize,
}),
);
const [page, setPage] = useQueryState("page", parseAsInteger().withDefault(0));
const [pageSize, setPageSize] = useQueryState("pageSize", parseAsInteger().withDefault(defaultPageSize));
return { page, setPage, pageSize, setPageSize };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants