fix: resolve engine bugs, store desync, export formatting, and ESLint warnings#16
Merged
Conversation
… warnings - Fix pivot table multi-value aggregation bug in SpreadsheetEngine - Fix Zustand store and HyperFormula engine desynchronization on row/col structural edits - Fix column index resolution bug for empty target columns in agent executor - Fix multiline CSV export double-quoting per RFC 4180 - Add activeSheetId validation in normalizeImportedWorkbook - Resolve 48 ESLint warnings and React hook dependency issues across 25 files - Add release audit findings documentation in RELEASE_AUDIT_FINDINGS.md Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Reviewer's GuideEngine fixes for pivot aggregation, HyperFormula/Zustand sync, column resolution, CSV export, and workbook import safety, plus extensive ESLint/react-hook cleanup and a new release audit doc. Sequence diagram for HyperFormula engine resync on structural sheet changessequenceDiagram
actor User
participant SpreadsheetGrid
participant Store
participant SpreadsheetEngine
User->>SpreadsheetGrid: clickInsertRowButton()
SpreadsheetGrid->>Store: insertRow(afterRow)
Store->>Store: updateSheetCells()
Store->>SpreadsheetEngine: loadWorkbook(workbook)
User->>SpreadsheetGrid: clickDeleteColumnButton()
SpreadsheetGrid->>Store: deleteColumn(col)
Store->>Store: updateSheetCells()
Store->>SpreadsheetEngine: loadWorkbook(workbook)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Calling
engine.loadWorkbook(get().workbook)after every structural change inuseStoreis duplicated across several actions and may be quite expensive; consider extracting a helper (e.g.resyncEngine()) and/or batching updates so the engine is reloaded once per logical operation. - The updated
resolveColumnIndexnow ignores the actual sheet width and uses a hardcodedindex < 1000bound while still computingheaderRowandmaxColthat are unused; consider either reusing the real max column when validating the target or removing the unused computation and making the limit a documented constant.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Calling `engine.loadWorkbook(get().workbook)` after every structural change in `useStore` is duplicated across several actions and may be quite expensive; consider extracting a helper (e.g. `resyncEngine()`) and/or batching updates so the engine is reloaded once per logical operation.
- The updated `resolveColumnIndex` now ignores the actual sheet width and uses a hardcoded `index < 1000` bound while still computing `headerRow` and `maxCol` that are unused; consider either reusing the real max column when validating the target or removing the unused computation and making the limit a documented constant.
## Individual Comments
### Comment 1
<location path="RELEASE_AUDIT_FINDINGS.md" line_range="82-85" />
<code_context>
+ - Set `LLM_PROVIDER_ORDER=openrouter,groq,ollama` in production environment variables.
+
+### 2. Ollama Local GPU Setup (Optional / Self-Hosted)
+- **Description:** Local LLM execution requires a running Ollama server (`http://127.0.0.1:11434`) with the `smartsht` model loaded.
+- **Action Required:**
+ - On your host/GPU server, run `npm run model:setup` (or `node server/scripts/setup-model.mjs`) to pull and build the Ollama model.
</code_context>
<issue_to_address>
**question (typo):** Possible typo or inconsistency in the Ollama model name (`smartsht`).
Elsewhere the project and repo use `smartsh!t` / `smartshit`, so `smartsht` looks like it may be missing an "i". If `smartsht` is the correct Ollama model name, consider adding a brief note to clarify this and prevent confusion.
```suggestion
### 2. Ollama Local GPU Setup (Optional / Self-Hosted)
- **Description:** Local LLM execution requires a running Ollama server (`http://127.0.0.1:11434`) with the `smartshit` model loaded (Ollama model name uses `smartshit` without the `!`).
- **Action Required:**
- On your host/GPU server, run `npm run model:setup` (or `node server/scripts/setup-model.mjs`) to pull and build the Ollama `smartshit` model.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+82
to
+85
| ### 2. Ollama Local GPU Setup (Optional / Self-Hosted) | ||
| - **Description:** Local LLM execution requires a running Ollama server (`http://127.0.0.1:11434`) with the `smartsht` model loaded. | ||
| - **Action Required:** | ||
| - On your host/GPU server, run `npm run model:setup` (or `node server/scripts/setup-model.mjs`) to pull and build the Ollama model. |
There was a problem hiding this comment.
question (typo): Possible typo or inconsistency in the Ollama model name (smartsht).
Elsewhere the project and repo use smartsh!t / smartshit, so smartsht looks like it may be missing an "i". If smartsht is the correct Ollama model name, consider adding a brief note to clarify this and prevent confusion.
Suggested change
| ### 2. Ollama Local GPU Setup (Optional / Self-Hosted) | |
| - **Description:** Local LLM execution requires a running Ollama server (`http://127.0.0.1:11434`) with the `smartsht` model loaded. | |
| - **Action Required:** | |
| - On your host/GPU server, run `npm run model:setup` (or `node server/scripts/setup-model.mjs`) to pull and build the Ollama model. | |
| ### 2. Ollama Local GPU Setup (Optional / Self-Hosted) | |
| - **Description:** Local LLM execution requires a running Ollama server (`http://127.0.0.1:11434`) with the `smartshit` model loaded (Ollama model name uses `smartshit` without the `!`). | |
| - **Action Required:** | |
| - On your host/GPU server, run `npm run model:setup` (or `node server/scripts/setup-model.mjs`) to pull and build the Ollama `smartshit` model. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes
1. Calculation & Engine Fixes
src/engine/spreadsheet.ts): Keyed aggregations by individual value field index (rowKey||colKey||vfIdx), fixing value mixing when multiple aggregate fields exist in a pivot table.src/store/useStore.ts): Triggeredget().engine.loadWorkbook(...)after structural row/col additions, deletions, or range deletions so formula evaluation stays in sync.src/agent/executor.ts): FixedresolveColumnIndexbounds check so valid target column letters on sparse sheets (e.g. columnDwhen max data is in columnA) are properly resolved.src/io/xlsx.ts): Added newline detection (\n,\r) to CSV escaping condition per RFC 4180.src/io/workbookJson.ts): AddedactiveSheetIdvalidation innormalizeImportedWorkbook.2. Linter & React Hook Cleanups
App.tsx,SpreadsheetGrid.tsx,FindReplaceDialog.tsx, and unused imports/variables).npm run lintnow completes with 0 warnings and 0 errors.3. Release Readiness
RELEASE_AUDIT_FINDINGS.mddocumenting pre-launch audit findings and production environment checklist (OpenRouter/Groq API keys, Clerk JWT, Stripe, PostgreSQL/S3).npm run release:check:v1release checklist gates pass 100%.Summary by Sourcery
Fix spreadsheet engine, store synchronization, and CSV export issues while cleaning up lint warnings and adding release audit documentation.
Bug Fixes:
Enhancements:
Documentation: