Refresh Sheets brand logo (green spreadsheet mark)#99
Open
AsifMulani1 wants to merge 5 commits into
Open
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
Replace the cyan chart-style icon with the new green (#278F5E) spreadsheet mark across all sheets brand assets: - sheets-icon / logo / app-logos/sheets (app icon) - sheets-icon-dark (darker-green variant) - sheets-mark (glyph only, currentColor) - sheets-favicon (browser tab) - sheets-lockup (icon + "Sheets · by Frappe" wordmark) - the inline mobile-blocker mark in pages/SheetEditor.vue The in-editor topbar mark (components/SheetEditor/index.vue) is refreshed in the standalone-port PR, which rewrites that file.
Contributor
Confidence Score: 4/5The AI settings path needs fixes before merging.
suite/sheets/api.py
|
| Filename | Overview |
|---|---|
| suite/sheets/api.py | Adds compressed sheet loading and AI Assist endpoints; the AI settings methods need permission and state-handling fixes. |
| frontend/src/apps/sheets/canvas/index.js | Adds lazy value lookup and validation-chip hit testing; the inspected production caller wires the needed lazy callbacks. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
suite/sheets/api.py:435-437
**Blank Model Stays Cached**
When the settings dialog submits an empty model, this guard skips the assignment and leaves the old model in the singleton. The admin sees the save succeed, but later AI requests still use the stale model instead of reverting to the default.
```suggestion
doc.enabled = 1 if int(enabled or 0) else 0
doc.model = (model or "").strip() or DEFAULT_AI_MODEL
```
### Issue 2 of 3
suite/sheets/api.py:430-443
**Settings Save Bypasses Permissions**
This whitelisted writer skips the `Sheets AI Settings` DocType permission check and saves with `ignore_permissions=True` after only checking the role name. A user who reaches this method with that role string can change the site-wide AI key, enabled flag, and model without the singleton's write permission being enforced.
### Issue 3 of 3
suite/sheets/api.py:418-425
**Settings Read Ignores Doctype Access**
`Sheets AI Settings` is readable only by System Manager in the DocType permissions, but this whitelisted method returns the stored model and whether a key exists without checking read access. Any logged-in caller can learn those site-level settings even though the singleton itself is not readable to them.
Reviews (1): Last reviewed commit: "feat(sheets): refresh brand logo — green..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+435
to
+437
| doc.enabled = 1 if int(enabled or 0) else 0 | ||
| if model: | ||
| doc.model = model |
Contributor
There was a problem hiding this comment.
When the settings dialog submits an empty model, this guard skips the assignment and leaves the old model in the singleton. The admin sees the save succeed, but later AI requests still use the stale model instead of reverting to the default.
Suggested change
| doc.enabled = 1 if int(enabled or 0) else 0 | |
| if model: | |
| doc.model = model | |
| doc.enabled = 1 if int(enabled or 0) else 0 | |
| doc.model = (model or "").strip() or DEFAULT_AI_MODEL |
Prompt To Fix With AI
This is a comment left during a code review.
Path: suite/sheets/api.py
Line: 435-437
Comment:
**Blank Model Stays Cached**
When the settings dialog submits an empty model, this guard skips the assignment and leaves the old model in the singleton. The admin sees the save succeed, but later AI requests still use the stale model instead of reverting to the default.
```suggestion
doc.enabled = 1 if int(enabled or 0) else 0
doc.model = (model or "").strip() or DEFAULT_AI_MODEL
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+430
to
+443
| def save_ai_settings(api_key: str = "", enabled: int = 0, model: str = "") -> dict: | ||
| # Write is gated to System Manager — this is org-level config, not per-sheet. | ||
| if "System Manager" not in frappe.get_roles(): | ||
| frappe.throw("Not permitted to change AI settings", frappe.PermissionError) | ||
| doc = frappe.get_doc(AI_SETTINGS) | ||
| doc.enabled = 1 if int(enabled or 0) else 0 | ||
| if model: | ||
| doc.model = model | ||
| # An empty api_key means "leave the existing key untouched" — the panel | ||
| # never receives the real key back, so it submits "" unless the admin | ||
| # deliberately types a new one. | ||
| if api_key: | ||
| doc.api_key = api_key | ||
| doc.save(ignore_permissions=True) |
Contributor
There was a problem hiding this comment.
This whitelisted writer skips the Sheets AI Settings DocType permission check and saves with ignore_permissions=True after only checking the role name. A user who reaches this method with that role string can change the site-wide AI key, enabled flag, and model without the singleton's write permission being enforced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: suite/sheets/api.py
Line: 430-443
Comment:
**Settings Save Bypasses Permissions**
This whitelisted writer skips the `Sheets AI Settings` DocType permission check and saves with `ignore_permissions=True` after only checking the role name. A user who reaches this method with that role string can change the site-wide AI key, enabled flag, and model without the singleton's write permission being enforced.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+418
to
+425
| # Read is ungated: the response only reveals whether AI is available | ||
| # (enabled + a key is configured), never the key itself, so any logged-in | ||
| # user can decide whether to show the "Ask" entry point. | ||
| doc = frappe.get_cached_doc(AI_SETTINGS) | ||
| return { | ||
| "enabled": bool(doc.enabled), | ||
| "model": doc.model or DEFAULT_AI_MODEL, | ||
| "keyIsSet": bool(_ai_key(doc)), |
Contributor
There was a problem hiding this comment.
Sheets AI Settings is readable only by System Manager in the DocType permissions, but this whitelisted method returns the stored model and whether a key exists without checking read access. Any logged-in caller can learn those site-level settings even though the singleton itself is not readable to them.
Prompt To Fix With AI
This is a comment left during a code review.
Path: suite/sheets/api.py
Line: 418-425
Comment:
**Settings Read Ignores Doctype Access**
`Sheets AI Settings` is readable only by System Manager in the DocType permissions, but this whitelisted method returns the stored model and whether a key exists without checking read access. Any logged-in caller can learn those site-level settings even though the singleton itself is not readable to them.
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.
Replaces the old cyan (#0E7490) chart-style icon with the new green
(#278F5E) spreadsheet mark across all Sheets brand assets.
Assets updated
sheets-icon.svg,logo.svg,app-logos/sheets.svg— the app iconsheets-icon-dark.svg— darker-green variant for dark surfacessheets-mark.svg— glyph only (currentColor, no background)sheets-favicon.svg— browser tabsheets-lockup.svg— icon + "Sheets / by Frappe" wordmark, recoloured to the new greenpages/SheetEditor.vueAll SVGs share one source path set (118×118 viewBox) so every size stays pixel-consistent.
Note
The in-editor topbar brand mark lives in
components/SheetEditor/index.vue,which is rewritten by the standalone-port PR (#98) — its inline icon is refreshed
there to avoid a merge conflict on that file.
Non-logo
#0E7490occurrences (conditional-format data-bar / colour-scaledefaults, pivot-tab accent) are intentionally left unchanged.