Skip to content

Add player self service delete page#485

Merged
tudddorrr merged 3 commits intodevelopfrom
self-service-deletion
Mar 5, 2026
Merged

Add player self service delete page#485
tudddorrr merged 3 commits intodevelopfrom
self-service-deletion

Conversation

@tudddorrr
Copy link
Copy Markdown
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Mar 4, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Mar 4, 2026

PR #485 Review: Add player self service delete page

Overall, this is a well-structured feature with good test coverage and clean component decomposition. A few items worth addressing:


Code Quality

Footer.tsx - path check is too broad

The check location.pathname.startsWith('/manage/') hides the footer for any future route under /manage/, not just the self-service delete page. As new self-service routes are added, they will silently lose the footer without an explicit decision being made. A more sustainable approach would be to handle this at the Router level (rendering the footer conditionally based on matched routes), or at minimum check against the specific selfServiceRoutes constants rather than a loose prefix.


useGameFromToken.ts - fetcher recreated on every render

The fetcher function is defined inside the hook body and recreated on every render. SWR does not use the fetcher reference as part of its cache key so this does not cause re-fetching, but it is unnecessarily wasteful. Since the fetcher has no closure dependencies, it should be defined at module scope outside the hook.


Minor

DeletePlayerLogin / DeletePlayerVerify - isLoading not reset on success

In both components, the catch block calls setLoading(false), but the success path does not. Because the parent transitions the step on success (unmounting these components), there is no visible impact in React 18. However, the asymmetry with the error path is confusing to read. Calling setLoading(false) before invoking the success callback would make the pattern consistent.


Security: No issues found.

Potential Bugs: No issues found.

Performance: No issues beyond the fetcher note above.

@tudddorrr tudddorrr force-pushed the self-service-deletion branch 2 times, most recently from eecf44d to 2ce0f5f Compare March 5, 2026 00:00
@tudddorrr tudddorrr force-pushed the self-service-deletion branch from 2ce0f5f to 6fd8d0e Compare March 5, 2026 00:06
@tudddorrr tudddorrr force-pushed the self-service-deletion branch 3 times, most recently from c4e1d5b to ad86876 Compare March 5, 2026 21:37
@tudddorrr tudddorrr force-pushed the self-service-deletion branch from ad86876 to 603b2c8 Compare March 5, 2026 21:42
@tudddorrr tudddorrr merged commit 3373fc5 into develop Mar 5, 2026
5 checks passed
@tudddorrr tudddorrr deleted the self-service-deletion branch March 5, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant