Port latest Sheets changes from standalone frappe/sheets#98
Open
AsifMulani1 wants to merge 6 commits into
Open
Port latest Sheets changes from standalone frappe/sheets#98AsifMulani1 wants to merge 6 commits into
AsifMulani1 wants to merge 6 commits into
Conversation
…wport render Port performance work from frappe/sheets since the Phase-5 cutoff: - compact row-major sheet codec (pack/unpack) + compressed sheets_data shipped to the client and decompressed there - clone-free, chunked autosave; faster load path - async pivot/chart aggregation with single-pass + downsampling - lazy-viewport canvas render so switch/load no longer scale with cell count; cheap sheet-add and single-paint switch Backend: cell_codec, storage row-major codec, op-log + versioning save tweaks.
Port the AI Assist feature from frappe/sheets: - backend sheets/ai package (Claude client, sheet context, heuristics, validate) and ai_assist / get_ai_settings / save_ai_settings whitelisted endpoints - Sheets AI Settings single doctype (api key, enabled, model) - AskBar + AISettingsDialog in the editor; www/sheets boot seeds the ai_assist_enabled / ai_assist_can_configure flags (key never reaches client) - add anthropic>=0.40 dependency The model is forced to emit a structured action plan via a tool; it returns formulas/ranges and never computes values (the engine does that client-side).
…ot drill-down Port editor features from frappe/sheets: - column/row-level formatting with cascade (format-scope) and a Frappe-styled color picker replacing the native input - data-validation dropdowns rendered as Sheets-style chips with per-option colors (chip-geometry) - pivot: copy/paste mints a live pivot; double-click drills down to source rows - date-string parsing when applying date/time number formats
Port fixes from frappe/sheets: - clipboard: don't wipe pasted cells when the cut source overlaps the destination; route common edits through op-based undo - repair snapshot-undo, cut+paste, comment & validation history - home: shorter topbar and default to the list view
Contributor
Confidence Score: 4/5This is close, but these issues should be fixed before merging.
frontend/src/apps/sheets/components/SheetEditor/useChartIntegration.js, suite/sheets/doctype/sheet/cell_codec.py
|
| Filename | Overview |
|---|---|
| frontend/src/apps/sheets/components/SheetEditor/useChartIntegration.js | Adds chart data cache invalidation, but structural sheet edits still need to bump it. |
| suite/sheets/doctype/sheet/cell_codec.py | Improves compact row decoding, but still needs container type checks. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/apps/sheets/components/SheetEditor/useChartIntegration.js:43
**Structural edits stay stale** The chart data version is bumped for cell edits and manual refresh, but row and column insert/delete paths shift the source cells without changing this value. A chart over `A1:A10` can keep rendering the old matrix after rows or columns are inserted or deleted until another cell edit or manual refresh invalidates the cache.
### Issue 2 of 2
suite/sheets/doctype/sheet/cell_codec.py:26
**Malformed containers still crash** This guard only handles falsy `rows`; a truthy non-dict like a list or string still reaches `_unpack_rows`, where `.items()` raises. `ai_assist` does not catch that decoder error, so one malformed compact `rows` container can still turn AI Assist into a 500 instead of ignoring the bad stored data.
Reviews (3): Last reviewed commit: "fix: keep charts/pivots fresh on bulk ed..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Replace the inline cyan chart icon in the SheetEditor topbar with the new green (#278F5E) spreadsheet mark, matching the refreshed brand assets.
Addresses code-review feedback on the Sheets port: - Bump chartDataVersion on cell-change callbacks so the overlay matrix cache invalidates on edits (drag/scroll still hit the cache). - Recompute pivots in the incremental onCellsChanged path so paste/fill no longer leave pivot output stale; drop the now-redundant explicit recompute in _doPaste. - Skip malformed rows in cell_codec._unpack_rows so a bad stored row can't 500 AI Assist.
| // move/resize/scroll. The overlay keys its matrix cache on this so dragging a | ||
| // chart over a 100k-row source doesn't re-materialise the matrix every frame, | ||
| // while edits to the source range still refresh the chart. | ||
| const chartDataVersion = ref(0) |
Contributor
There was a problem hiding this comment.
A1:A10 can keep rendering the old matrix after rows or columns are inserted or deleted until another cell edit or manual refresh invalidates the cache.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/apps/sheets/components/SheetEditor/useChartIntegration.js
Line: 43
Comment:
**Structural edits stay stale** The chart data version is bumped for cell edits and manual refresh, but row and column insert/delete paths shift the source cells without changing this value. A chart over `A1:A10` can keep rendering the old matrix after rows or columns are inserted or deleted until another cell edit or manual refresh invalidates the cache.
How can I resolve this? If you propose a fix, please make it concise.| return {} | ||
| if sheet_slice.get("v") != PACK_VERSION: | ||
| return sheet # legacy: already a {cellId: value} map | ||
| return _unpack_rows(sheet.get("rows") or {}) |
Contributor
There was a problem hiding this comment.
rows; a truthy non-dict like a list or string still reaches _unpack_rows, where .items() raises. ai_assist does not catch that decoder error, so one malformed compact rows container can still turn AI Assist into a 500 instead of ignoring the bad stored data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: suite/sheets/doctype/sheet/cell_codec.py
Line: 26
Comment:
**Malformed containers still crash** This guard only handles falsy `rows`; a truthy non-dict like a list or string still reaches `_unpack_rows`, where `.items()` raises. `ai_assist` does not catch that decoder error, so one malformed compact `rows` container can still turn AI Assist into a 500 instead of ignoring the bad stored data.
How can I resolve this? If you propose a fix, please make it concise.
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.
Ports everything
frappe/sheetsgained since the Phase-5 monorepo cutoff(
rename: spreadsheet → sheets, 2026-06-15) intosuite/sheets. ~20substantive commits, grouped here into 4 thematic commits.
Path/namespace adaptation matches the original port:
frontend/src/*→frontend/src/apps/sheets/*(thepages/SheetEditor/dir →components/SheetEditor/),backend
sheets/*→suite/sheets/*(nestedsheets/sheets/doctypeflattened),sheets.api.*/sheets.versioning.*→suite.sheets.*, and the frappe-uibeta.9 design-token renames (
--surface-white→--surface-base, etc.).What's included
sheets_data, clone-freechunked autosave, async pivot/chart aggregation + downsampling, lazy-viewport
canvas render (switch/load no longer scale with cell count).
sheets/aipackage +ai_assist/get_ai_settings/save_ai_settingsendpoints,Sheets AI Settingsdoctype, AskBar +AISettingsDialog, www boot gating. Adds
anthropic>=0.40. The key neverreaches the client; the model returns formulas/ranges, never computed values.
per-option colors, Frappe-styled color picker, pivot copy/paste → live pivot,
pivot drill-down to source rows.
list-first view + shorter topbar.
Not included (deliberate)
sheets/public/sheets/) — gitignored; suite builds its own..test.jsfiles — the original Phase-5 port dropped sheets frontendtests; not reintroduced here. Backend Python tests (e.g.
test_cell_codec.py)are included.
Verification done
Static: adaptation transform proven complete against all 161 ported files, no
leftover un-prefixed
sheets.backend refs, all design tokens resolve, Pythoncompiles, JS syntax checks pass, new-module imports + AI endpoint wiring confirmed.
Verification still needed
A frontend build was not run (full monorepo install). Please build and do
the Phase-6 smoke test per
PORT-NOTES.md: log in atsuite.localhost/sheets,confirm listing/open/edit/persist, charts, version history, validation chips,
color picker, and AI Assist (with a key configured). Run
bench migratetocreate the
Sheets AI Settingsdoctype.