Skip to content

Change offset and count checks to undefined #141

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.

Sign up for GitHub

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

Open
tomcss wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Change offset and count checks to undefined #141

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/achievement/getAchievementUnlocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ export const getAchievementUnlocks = async (

const queryParams: Record<string, number | string> = { a: achievementId };

if (offset) {
if (offset !== undefined) {
queryParams.o = offset;
}

if (count) {
if (count !== undefined) {
queryParams.c = count;
}
Comment on lines +60 to 66

@greptile-apps greptile-apps Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Null Values Become Query Parameters

JavaScript callers can pass null; this now serializes o=null or c=null instead of omitting the parameter. The API can reject the request.

Suggested change
if (offset !== undefined) {
queryParams.o = offset;
}
if (count) {
if (count !== undefined) {
queryParams.c = count;
}
if (offset != null) {
queryParams.o = offset;
}
if (count != null) {
queryParams.c = count;
}

Context Used: Be extremely concise in whatever you write on GitH... (source)

@wescopeland wescopeland Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure? The args are optionally typed as foo?: number - not nullish.

@greptile-apps greptile-apps Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right. With foo?: number, TypeScript types it as number | undefined — null is not assignable and can't be passed by a well-typed caller. The !== undefined check in the PR is the correct and precise guard here. My suggestion was overly defensive. Good catch.

Shall I remember this as a rule: "don't suggest != null guards for parameters typed as foo?: T — !== undefined is the correct check since null isn't assignable to optional parameters"?

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

@wescopeland wescopeland Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, remember that as a rule.

Also - how many other files need to have !== undefined applied to them for count / offset params for consistency?

@greptile-apps greptile-apps Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rule saved. View rule

Here's the full picture across the codebase:

5 files still using truthy checks (need fixing):

File Pattern
src/comment/getComments.ts if (offset) / if (count)
src/leaderboard/getLeaderboardEntries.ts if (payload?.offset) / if (payload?.count)
src/leaderboard/getUserGameLeaderboards.ts if (payload?.offset) / if (payload?.count)
src/user/getUserWantToPlayList.ts if (payload?.offset) / if (payload?.count)
src/feed/getRecentGameAwards.ts if (payload?.offset) / if (payload?.count)

2 files already correct (no action needed):

  • src/user/getUsersIFollow.ts — uses !== null && !== undefined (redundant but safe; could be simplified to just !== undefined)
  • src/user/getUsersFollowingMe.ts — same as above

So you have 5 files to update for consistency with this PR's fix.

@wescopeland wescopeland Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @tomcss can we propagate this for consistency everywhere else mentioned above


Expand Down
Loading