Update dependencies and refactor duplicate code patterns#43
Update dependencies and refactor duplicate code patterns#43
Conversation
Co-authored-by: JohnDeved <24187269+JohnDeved@users.noreply.github.com>
Deploying bills-pc with
|
| Latest commit: |
b445e65
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://615e4be9.bills-pc.pages.dev |
| Branch Preview URL: | https://copilot-update-dependencies.bills-pc.pages.dev |
Co-authored-by: JohnDeved <24187269+JohnDeved@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
Pull Request Overview
This pull request updates multiple dependencies and makes minor code improvements to handle TypeScript and build tooling updates. The changes focus on:
- Upgrading to TypeScript 5.9, ES2023, and major versions of various dependencies (React 19.2, Vite 7.2, Vitest 4.0, Playwright 1.56, etc.)
- Adding Uint8Array support to the parser for improved memory handling
- Creating a shared
usePopoverSidehook to reduce code duplication - Updating to use
toSorted()instead ofsort()for immutability - Refactoring variable names and adding documentation in utility files
- Modernizing test mocks to use class syntax
- Minor code cleanup (removing exports, unused imports)
Reviewed Changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Major dependency updates across the stack |
| tsconfig.app.json | Upgraded lib target from ES2022 to ES2023 |
| src/vite-env.d.ts | Removed empty export statement |
| src/stores/useSaveFileStore.ts | Added Uint8Array support and SharedArrayBuffer safety |
| src/stores/useHistoryStore.ts | Simplified byte array handling |
| src/lib/utils.ts | Improved documentation and variable naming |
| src/lib/parser/core/PokemonSaveParser.ts | Added Uint8Array support and error cause chaining |
| src/lib/parser/tests/cli-event-driven.test.ts | Modernized mock classes |
| src/hooks/usePopoverSide.ts | New shared hook for popover positioning |
| src/components/pokemon/traits/AbilityTab.tsx | Use toSorted() for immutability |
| src/components/pokemon/PokemonNatureCombobox.tsx | Extract to usePopoverSide hook |
| src/components/pokemon/PokemonMoveButton.tsx | Use toSorted() and type annotations |
| src/components/pokemon/PokemonItemCombobox.tsx | Extract to usePopoverSide hook |
| const { buffer } = new Uint8Array(input) | ||
| this.saveData = new Uint8Array(buffer) |
There was a problem hiding this comment.
The logic for handling Uint8Array is incorrect. Line 120 creates a new Uint8Array from input, which doesn't create a copy if the input is already a Uint8Array - it just returns the same underlying buffer. This doesn't avoid SharedArrayBuffer as the comment claims. Instead, use this.saveData = new Uint8Array(input) directly to create a copy, or if you need to ensure a non-shared buffer, use this.saveData = new Uint8Array(input.buffer.slice(input.byteOffset, input.byteOffset + input.byteLength)).
| const { buffer } = new Uint8Array(input) | |
| this.saveData = new Uint8Array(buffer) | |
| this.saveData = new Uint8Array(input.buffer.slice(input.byteOffset, input.byteOffset + input.byteLength)) |
There was a problem hiding this comment.
Fixed in commit 0278836. Changed Uint8Array handling to properly create a new ArrayBuffer copy using new ArrayBuffer() + set(), which guarantees a non-shared buffer and avoids SharedArrayBuffer type issues.
| } else if (input instanceof Uint8Array) { | ||
| // If already a Uint8Array, create a proper ArrayBuffer copy (avoiding SharedArrayBuffer) | ||
| const { buffer } = new Uint8Array(input) | ||
| this.saveData = new Uint8Array(buffer) | ||
| } else { | ||
| buffer = input as ArrayBuffer | ||
| this.saveData = new Uint8Array(buffer) |
There was a problem hiding this comment.
There's a missing assignment to buffer variable in the Uint8Array branch (lines 118-121) before reaching line 127 where it may be referenced. However, the code continues after line 121 without returning, which means if buffer is used later it will be undefined in this branch. This creates inconsistent state handling compared to other branches.
There was a problem hiding this comment.
Fixed in commit 0278836. The Uint8Array branch now properly assigns to the buffer variable before creating saveData, ensuring consistent state handling across all branches.
There was a problem hiding this comment.
💡 Codex Review
bills-pc/src/lib/parser/core/PokemonSaveParser.ts
Lines 98 to 104 in 58e66a6
When a user loads a save via File (including FileSystemFileHandle), loadInputData now reads the file into a local buffer but never assigns the result to this.saveData. Subsequent logic (config detection, parsing, reconstruction) relies on this.saveData being populated and will now fail with "Save data and config not loaded" for every normal file parse. The Uint8Array and ArrayBuffer branches still set this.saveData; the File branch needs the same assignment.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Co-authored-by: JohnDeved <24187269+JohnDeved@users.noreply.github.com>
All issues addressed in commit 0278836:
All tests pass (17 files, 162 tests), build succeeds, and linter passes. |
|
@copilot check typescript and lint issues |
Co-authored-by: JohnDeved <24187269+JohnDeved@users.noreply.github.com>
✅ No TypeScript or lint issues found. Verified in commit b445e65: TypeScript: Build passes with 0 errors Also completed ox-standard setup:
|
Completed Work ✅
Dependency Updates (DONE)
Code Review Fixes (DONE)
ox-standard Configuration (DONE)
Code Duplication Removal (DONE)
usePopoverSidehookCode Refactoring (DONE)
Testing & Verification ✅
Changes in This Commit
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.