-
Notifications
You must be signed in to change notification settings - Fork 4
635: wip #653
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
base: main
Are you sure you want to change the base?
635: wip #653
Conversation
Title635: wip PR TypeEnhancement, Bug fix Description
Diagram Walkthroughflowchart LR
A["ApiURL enhancements"] -- "generate query keys" --> B["queryKey/_generateKey/prefix"]
B -- "used by" --> C["Leaderboard queries refactor"]
B -- "used by" --> D["Submission query refactor"]
C -- "invalidate via prefix" --> E["React Query cache"]
F["Lobby navigation"] -- "check joinCode before redirect" --> G["Stable routing"]
|
| Relevant files | |||||||
|---|---|---|---|---|---|---|---|
| Bug fix |
| ||||||
| Enhancement |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| const segments = path.split("/").filter((segment) => segment.length > 0); | ||
| return [ | ||
| ...segments, | ||
| { | ||
| method: options.method, | ||
| params: options.params, | ||
| queries: options.queries, | ||
| }, | ||
| ] as unknown as readonly [ | ||
| ...PathSegments<TPath>, | ||
| { method: TMethod; params: TParams; queries: TQueries }, | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Avoid as unknown as casts by preserving segment types when splitting. Use a typed helper to narrow non-empty segments so the returned tuple aligns with PathSegments<TPath> at runtime and compile-time. [possible issue, importance: 4]
| const segments = path.split("/").filter((segment) => segment.length > 0); | |
| return [ | |
| ...segments, | |
| { | |
| method: options.method, | |
| params: options.params, | |
| queries: options.queries, | |
| }, | |
| ] as unknown as readonly [ | |
| ...PathSegments<TPath>, | |
| { method: TMethod; params: TParams; queries: TQueries }, | |
| ]; | |
| const isNonEmpty = (s: string): s is Exclude<string, ""> => s.length > 0; | |
| const segments = path.split("/").filter(isNonEmpty); | |
| return [ | |
| ...segments, | |
| { | |
| method: options.method, | |
| params: options.params, | |
| queries: options.queries, | |
| }, | |
| ]; |
| const { url, method, res, queryKey } = ApiURL.create( | ||
| "/api/leaderboard/current/user/all", | ||
| { | ||
| method: "GET", | ||
| queries: { | ||
| page, | ||
| pageSize, | ||
| filters, | ||
| globalIndex, | ||
| query: debouncedQuery, | ||
| }), | ||
| globalIndex, | ||
| ...Object.typedFromEntries( | ||
| Object.typedEntries(filters).map(([tagEnum, filterEnabled]) => { | ||
| const metadata = ApiUtils.getMetadataByTagEnum(tagEnum); | ||
|
|
||
| return [metadata.apiKey, filterEnabled]; | ||
| }), | ||
| ), | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Guard against undefined filters to prevent runtime errors when calling Object.typedEntries. Default to an empty object or short-circuit the spread to ensure stability. [possible issue, importance: 3]
| const { url, method, res, queryKey } = ApiURL.create( | |
| "/api/leaderboard/current/user/all", | |
| { | |
| method: "GET", | |
| queries: { | |
| page, | |
| pageSize, | |
| filters, | |
| globalIndex, | |
| query: debouncedQuery, | |
| }), | |
| globalIndex, | |
| ...Object.typedFromEntries( | |
| Object.typedEntries(filters).map(([tagEnum, filterEnabled]) => { | |
| const metadata = ApiUtils.getMetadataByTagEnum(tagEnum); | |
| return [metadata.apiKey, filterEnabled]; | |
| }), | |
| ), | |
| }, | |
| }, | |
| ); | |
| const safeFilters = filters ?? {}; | |
| const { url, method, res, queryKey } = ApiURL.create( | |
| "/api/leaderboard/current/user/all", | |
| { | |
| method: "GET", | |
| queries: { | |
| page, | |
| pageSize, | |
| query: debouncedQuery, | |
| globalIndex, | |
| ...Object.typedFromEntries( | |
| Object.typedEntries(safeFilters).map(([tagEnum, filterEnabled]) => { | |
| const metadata = ApiUtils.getMetadataByTagEnum(tagEnum); | |
| return [metadata.apiKey, filterEnabled]; | |
| }), | |
| ), | |
| }, | |
| }, | |
| ); |
| if (data?.success && data.payload.lobby.joinCode) { | ||
| navigate("/duel/current"); | ||
| } else { | ||
| navigate("/duel"); | ||
| } | ||
| }, [data?.success, navigate]); | ||
| }, [data?.payload?.lobby.joinCode, data?.success, navigate]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Optional-chain payload and lobby in the condition to match the dependency array and avoid possible undefined access. This prevents runtime errors when data.success is true but payload or lobby is missing. [possible issue, importance: 6]
| if (data?.success && data.payload.lobby.joinCode) { | |
| navigate("/duel/current"); | |
| } else { | |
| navigate("/duel"); | |
| } | |
| }, [data?.success, navigate]); | |
| }, [data?.payload?.lobby.joinCode, data?.success, navigate]); | |
| useEffect(() => { | |
| if (data?.success && data?.payload?.lobby?.joinCode) { | |
| navigate("/duel/current"); | |
| } else { | |
| navigate("/duel"); | |
| } | |
| }, [data?.payload?.lobby?.joinCode, data?.success, navigate]); |
|
635
Description of changes
Checklist before review
Screenshots
Dev
Staging