feat(sheets): expose cell notes in read_sheet_values#580
Merged
taylorwilsdon merged 4 commits intotaylorwilsdon:mainfrom Mar 15, 2026
Merged
feat(sheets): expose cell notes in read_sheet_values#580taylorwilsdon merged 4 commits intotaylorwilsdon:mainfrom
taylorwilsdon merged 4 commits intotaylorwilsdon:mainfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds support for retrieving and formatting Google Sheets cell notes and consolidates grid metadata fetching (notes and hyperlinks) into a single bounded, single-request helper; exposes an Changes
Sequence DiagramsequenceDiagram
participant User
participant sheets_tools as sheets_tools.read_sheet_values
participant sheets_helpers as sheets_helpers._fetch_grid_metadata
participant GoogleSheetsAPI as Google Sheets API
User->>sheets_tools: call read_sheet_values(..., include_notes=True)
sheets_tools->>sheets_helpers: _fetch_grid_metadata(service, spreadsheet_id, resolved_range, values, include_hyperlinks, include_notes)
sheets_helpers->>GoogleSheetsAPI: spreadsheets.get(includeGridData=True, fields=...)
GoogleSheetsAPI-->>sheets_helpers: spreadsheet grid data (cells, notes, links)
sheets_helpers->>sheets_helpers: _extract_cell_notes_from_grid(...) / format hyperlink section
sheets_helpers-->>sheets_tools: formatted notes_section, hyperlink_section
sheets_tools-->>User: composed output including notes_section
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
|
Appreciate this! |
…ace_mcp into feat/517-cell-notes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
gsheets/sheets_tools.py (2)
267-267: Consider renamingMAX_HYPERLINK_FETCH_CELLSto a more generic name.This constant is now used for both hyperlinks and notes fetching. A name like
MAX_GRID_DATA_FETCH_CELLSwould better reflect its broader usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gsheets/sheets_tools.py` at line 267, The constant MAX_HYPERLINK_FETCH_CELLS is used for both hyperlink and note fetching and should be renamed to a generic identifier like MAX_GRID_DATA_FETCH_CELLS; update the constant definition and all usages (e.g., the conditional in sheets_tools.py that reads "if cell_count <= MAX_HYPERLINK_FETCH_CELLS:") to the new name, adjust any related comments/docstrings and imports/exports so tests and other modules reference MAX_GRID_DATA_FETCH_CELLS consistently, and run a project-wide search/replace to ensure no stale references remain.
255-287: Consider extracting shared fetch logic to reduce duplication.The notes fetching block is nearly identical to the hyperlinks block (lines 220-253). Both compute
_a1_range_for_values, check cell count againstMAX_HYPERLINK_FETCH_CELLS, and have the same error handling structure. A helper function could reduce ~60 lines to ~20.Additionally, when both
include_hyperlinksandinclude_notesareTrue, two separate API calls are made. These could potentially be combined into a singlespreadsheets.getcall with a mergedfieldsselector to halve the API overhead.💡 Example of potential helper extraction
async def _fetch_grid_metadata( service, spreadsheet_id: str, resolved_range: str, values: List[List[object]], *, include_hyperlinks: bool, include_notes: bool, ) -> tuple[str, str]: """Fetch hyperlinks and/or notes in a single API call when possible.""" tight_range = _a1_range_for_values(resolved_range, values) if not tight_range: return "", "" cell_count = _a1_range_cell_count(tight_range) or sum(len(row) for row in values) if cell_count > MAX_HYPERLINK_FETCH_CELLS: return "", "" # Build combined fields selector based on what's requested # ... fetch once and parse both hyperlinks and notes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gsheets/sheets_tools.py` around lines 255 - 287, The notes-fetching logic duplicates the hyperlinks block; extract a helper (e.g., _fetch_grid_metadata) that takes (service, spreadsheet_id, resolved_range, values, include_hyperlinks, include_notes) and: compute tight_range via _a1_range_for_values, bail out/log if None, compute cell_count via _a1_range_cell_count or sum(len(row) for row in values) and bail if over MAX_HYPERLINK_FETCH_CELLS, build a combined fields selector for a single spreadsheets.get call when both include_hyperlinks and include_notes are requested, perform one API call, then parse and return both formatted sections (reusing _format_sheet_hyperlinks_section/_format_sheet_notes_section or equivalent) and propagate/log errors the same way as the current try/except blocks; replace the existing inline blocks with calls to this helper in read_sheet_values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gsheets/sheets_tools.py`:
- Line 267: The constant MAX_HYPERLINK_FETCH_CELLS is used for both hyperlink
and note fetching and should be renamed to a generic identifier like
MAX_GRID_DATA_FETCH_CELLS; update the constant definition and all usages (e.g.,
the conditional in sheets_tools.py that reads "if cell_count <=
MAX_HYPERLINK_FETCH_CELLS:") to the new name, adjust any related
comments/docstrings and imports/exports so tests and other modules reference
MAX_GRID_DATA_FETCH_CELLS consistently, and run a project-wide search/replace to
ensure no stale references remain.
- Around line 255-287: The notes-fetching logic duplicates the hyperlinks block;
extract a helper (e.g., _fetch_grid_metadata) that takes (service,
spreadsheet_id, resolved_range, values, include_hyperlinks, include_notes) and:
compute tight_range via _a1_range_for_values, bail out/log if None, compute
cell_count via _a1_range_cell_count or sum(len(row) for row in values) and bail
if over MAX_HYPERLINK_FETCH_CELLS, build a combined fields selector for a single
spreadsheets.get call when both include_hyperlinks and include_notes are
requested, perform one API call, then parse and return both formatted sections
(reusing _format_sheet_hyperlinks_section/_format_sheet_notes_section or
equivalent) and propagate/log errors the same way as the current try/except
blocks; replace the existing inline blocks with calls to this helper in
read_sheet_values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0f39b47-26a4-4de8-bb00-53db85060d1c
📒 Files selected for processing (2)
gsheets/sheets_helpers.pygsheets/sheets_tools.py
20 tasks
This was referenced Mar 30, 2026
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
2 participants
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
include_notesparameter toread_sheet_valuesto expose Google Sheets cell notesFalsepreserves existing behavior with zero extra API callsinclude_hyperlinksparameterImplementation
include_notes: bool = Falseparameter onread_sheet_valuesTrue, fetches notes viaspreadsheets.getwithincludeGridData=Trueandfieldsscoped to onlynote(minimal payload)sheets_helpers.py:_extract_cell_notes_from_grid()— extracts[{"cell": "A1", "note": "..."}]from grid data_fetch_sheet_notes()— API call with tight range bounds and 5000-cell cap_format_sheet_notes_section()— formats notes into readable output (max 25 entries)Test plan
read_sheet_valueswithoutinclude_notes— verify unchanged behavior, no extra API callinclude_notes=Trueon a sheet with notes — verify notes appear in outputinclude_notes=Trueon a sheet without notes — verify graceful empty responseSummary by CodeRabbit