Skip to content

Fix delete button shown on root SGF collection#3597

Open
teauxfu wants to merge 3 commits into
online-go:mainfrom
teauxfu:fix/sgf-root-collection-delete-button
Open

Fix delete button shown on root SGF collection#3597
teauxfu wants to merge 3 commits into
online-go:mainfrom
teauxfu:fix/sgf-root-collection-delete-button

Conversation

@teauxfu

@teauxfu teauxfu commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #3594. When navigating to the root SGF collection via the breadcrumb, componentDidUpdate was storing the result of parseInt() into collection_id (a string). This caused the root collection check collection_id !== "0" to pass. The fix removes the parseInt() call so collection_id stays a string. As a follow-on, the repeated collection_id !== "0" checks throughout the component could be consolidated into an isRootCollection helper, but I'll leave that style decision to another

@github-actions

Copy link
Copy Markdown

The fix looks correct and well-scoped.

The root cause is clear: componentDidUpdate was the only code path that called parseInt on collection_id, converting it from string to number. All the \!== "0" guards throughout the component use strict equality, so after breadcrumb navigation, 0 \!== "0" always passed — including the delete button visibility check. The constructor correctly initialized collection_id as "0" (string), so the bug only triggered after navigation.

One minor observation: deleteCollection itself has no guard against collection_id === "0", so if the button check were ever bypassed it would POST delete_collections: ["0"] to the backend. The UI guard added in this PR is the right approach, but a defensive early return inside deleteCollection would add a safety net. Low priority given the button is the only call site.

@github-actions

Copy link
Copy Markdown

Code review: no bugs or performance issues found.

The fix is correct: removing parseInt() in componentDidUpdate keeps collection_id as a string, which matches the LibraryPlayerState type and makes all the \!== "0" strict-equality guards work correctly after breadcrumb navigation.

One note on the existing bot comment: it suggests adding a defensive early return in deleteCollection as a future improvement, but this PR already adds exactly that guard (lines 376–378 in the diff). The suggestion is already implemented.

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.

Present delete button when there's no sgf collection to delete

1 participant