Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Jan 12, 2026
@claude
Copy link

claude bot commented Jan 12, 2026

Code Review - PR #442

Overall, this is a well-structured PR with good component abstraction. I found a few issues that should be addressed:


🔴 Code Quality and Best Practices

Issue: Tailwind class syntax error in PlayerSaveContent.tsx:104

The className has an invalid Tailwind syntax:

className='bg-gray-700!'

Should be:

className='!bg-gray-700'

The ! important modifier must come before the class name in Tailwind v4.


🟡 Potential Bugs or Issues

Issue 1: TimePeriodPicker.tsx defensive check could hide bugs

Lines 17-21 silently fail if a period isn't found, which shouldn't happen given the component contract:

onChange={(id) => {
  const period = periods.find((p) => p.id === id)
  if (period) {
    onPick(period)
  }
}}

Since onChange only fires with IDs from the options array, the period will always be found. The defensive check masks potential data inconsistency. Consider removing the check or adding error logging if the invariant is violated.

Issue 2: Node hover state added to all nodes unnecessarily

In PlayerSaveContent.tsx:65-73, nodesWithHoverState spreads and modifies every node object whenever hoveredNodeId changes, even when isHovered is already present in the data. This creates unnecessary re-renders for nodes that aren't being hovered. Consider only spreading when the hover state actually changes for a given node.


🔵 Performance Considerations

Issue: Both graph hooks run on every render

Lines 51-53 in PlayerSaveContent.tsx:

const treeGraph = useNodeGraph(save, search, mode === 'tree')
const linearGraph = useLinearNodeGraph(save, search, mode === 'linear')
const { nodes, edges } = mode === 'tree' ? treeGraph : linearGraph

While the enabled parameter prevents the hooks from executing their logic, the hooks themselves still run on every render. The ternary selection is fine, but note that both hooks will re-run when search changes even though only one mode is active. This is acceptable given the current implementation but worth documenting.


Security Concerns

No issues found.

@tudddorrr tudddorrr merged commit 6ee6206 into develop Jan 12, 2026
5 checks passed
@tudddorrr tudddorrr deleted the save-linear-mode branch January 12, 2026 23:10
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.

2 participants