fix: make Sheets formulas quota-resilient - #31
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe add-on adds document-scoped caching, request blocking, cache-miss locking, batched latest-price retrieval, tier-based TTLs, and worksheet-readable errors. It adds ChangesOilPrice Sheets add-on 1.3.0
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Sheet
participant OILPRICE_TABLE
participant DocumentCache
participant OilPriceAPI
Sheet->>OILPRICE_TABLE: submit commodity range
OILPRICE_TABLE->>DocumentCache: read shared latest-price entries
OILPRICE_TABLE->>OilPriceAPI: request missing codes in one batch
OilPriceAPI-->>OILPRICE_TABLE: return latest-price records
OILPRICE_TABLE->>DocumentCache: store validated records
OILPRICE_TABLE-->>Sheet: spill worksheet rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Code.gs (1)
676-727: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded Apps Script service access in the cache and lock helpers. Both helper groups obtain and use Apps Script service handles without exception guards, while
putCachedValue_,getDocumentProperties_, andgetActiveSpreadsheetId_are guarded. A throw fromCacheServiceorLockServiceescapes into every formula path, including the post-successclearRequestBlocks_call that would discard a valid API response.
Code.gs#L676-L727: wrapcacheStore_, thecache.getcall ingetCachedValue_, and thecache.removecall inremoveCachedValue_in try/catch, and returnnullor no-op on failure.Code.gs#L729-L755: wrapLockService.getDocumentLock()in try/catch and returnnullon failure; also guardtryLockandreleaseLockso a lock-service failure degrades to an unlocked load instead of a formula error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 676 - 727, In Code.gs lines 676-727, guard CacheService access in cacheStore_, getCachedValue_, and removeCachedValue_: catch service, get, or remove failures and return null or no-op without propagating exceptions; ensure callers handle an unavailable cache. In Code.gs lines 729-755, guard LockService.getDocumentLock(), tryLock, and releaseLock so failures return null or proceed as an unlocked load rather than reaching formula paths as errors.
🧹 Nitpick comments (10)
Code.gs (5)
1343-1370: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKey the history cache by the endpoint bucket, not by the raw day count.
The request path depends only on the bucket.
days = 8anddays = 15both request/prices/past_month, buthistory_${code}_${requestedDays}stores them under different keys. Each distinct day value therefore triggers its own API call for identical data.Compute
endpointbefore the cache read and use it in the key.♻️ Proposed key change
- const cacheKey = `history_${code}_${requestedDays}`; - const cached = getCachedValue_(cacheKey, CACHE_TTL_SECONDS.history, 'document'); - if (cached) return cached; - let endpoint = 'past_year'; if (requestedDays <= 1) endpoint = 'past_day'; else if (requestedDays <= 7) endpoint = 'past_week'; else if (requestedDays <= 30) endpoint = 'past_month'; + + const cacheKey = `history_${code}_${endpoint}`; + const cached = getCachedValue_(cacheKey, CACHE_TTL_SECONDS.history, 'document'); + if (cached) return cached;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 1343 - 1370, In the history-fetching flow, compute the endpoint bucket before reading the cache, then key cacheKey with code and endpoint rather than requestedDays. Keep the existing endpoint selection in the history function and ensure both getCachedValue_ and withCacheMissLock_ use the same bucket-based key.
345-361: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the batch cache APIs for block keys.
cachedRequestBlock_performs three separategetcalls, andclearRequestBlocks_performs three separateremovecalls on every successful request.CacheServiceprovidesgetAllandremoveAll, which perform one round trip each.Note that
getAllreturns raw strings, so the envelope parsing and age check ingetCachedValue_must be reused or factored out.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 345 - 361, Update cachedRequestBlock_ and clearRequestBlocks_ to use CacheService getAll and removeAll with the three keys in one operation each. Reuse or factor the envelope parsing and age validation from getCachedValue_ so raw getAll values receive identical handling, and preserve the existing key-priority order and null behavior.
841-867: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the batch tolerant of individual bad records.
validatePriceRecord_runs inside.map(), so one malformed record aborts the whole batch before any record is cached.unresolvedthen turns the entire table into a two-cell error. The valid records are discarded, so the next recalculation repeats the same request. That increases request volume, which the PR aims to reduce.Validate each record independently, cache the valid ones, and report the failed codes per row.
♻️ Proposed per-record handling
- const returned = extractPriceRecords_(body, 'prices').map((record) => - validatePriceRecord_(record, 'Price record') - ); const missingSet = new Set(missingCodes); - for (const record of returned) { - if (!missingSet.has(record.code)) continue; - records.set(record.code, record); - putCachedValue_(`latest_${record.code}`, record, latestCacheTtl_(), 'document'); + for (const raw of extractPriceRecords_(body, 'prices')) { + let record; + try { + record = validatePriceRecord_(raw, 'Price record'); + } catch (error) { + continue; + } + if (!missingSet.has(record.code)) continue; + records.set(record.code, record); + putCachedValue_(`latest_${record.code}`, record, latestCacheTtl_(), 'document'); }
OILPRICE_TABLEcan then emit an error cell for each unresolved code instead of failing the whole spill.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 841 - 867, Update the latest-record processing in the batch flow around extractPriceRecords_ and validatePriceRecord_ to validate each returned record independently instead of using a failing .map() call. Cache every valid record, collect invalid or rejected record codes, and ensure unresolved combines missing and failed codes so the resulting error reports failures per code while preserving valid records for the table output.
326-343: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a path-derived prefix to hashed cache keys.
stableCacheHash_produces a 32-bit value. Two different request paths can map to one key. A collision then serves one endpoint's cached payload to another endpoint's formula, and the failure is silent.Apps Script cache keys accept long values, so include a sanitized, truncated path next to the hash.
♻️ Proposed key construction
function stableCacheHash_(value) { let hash = 2166136261; const text = String(value || ''); for (let index = 0; index < text.length; index += 1) { hash ^= text.charCodeAt(index); hash = Math.imul(hash, 16777619); } - return (hash >>> 0).toString(36); + const digest = (hash >>> 0).toString(36); + const slug = text.replace(/[^A-Za-z0-9_-]+/g, '_').slice(0, 96); + return `${slug}_${digest}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 326 - 343, Update requestBlockKeys_ to include a sanitized, truncated path-derived prefix alongside each stableCacheHash_ value for endpoint and request cache keys. Ensure the prefix is safe for Apps Script cache keys and retain the hash to keep keys bounded and unique enough, while leaving the global key unchanged.
312-324: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the cache generation per execution.
namespacedCacheKey_callscacheGeneration_on every cache get, put, and remove. Each call performs a PropertiesService read, and the fallback path also callsSpreadsheetApp.getActiveSpreadsheet()and a second PropertiesService read. A single request path runscachedRequestBlock_(3 gets) andclearRequestBlocks_(3 removes), so one formula can produce many property reads. PropertiesService calls are slow and quota-limited.The generation cannot change inside one execution, so cache it in a module-level variable.
♻️ Proposed memoization
+let cachedGeneration_ = null; + function cacheGeneration_() { + if (cachedGeneration_) return cachedGeneration_; const documentProperties = getDocumentProperties_(); const documentGeneration = documentProperties ? documentProperties.getProperty(CACHE_GENERATION_PROPERTY) : null; - if (documentGeneration) return documentGeneration; + if (documentGeneration) { + cachedGeneration_ = documentGeneration; + return cachedGeneration_; + } const spreadsheetId = getActiveSpreadsheetId_(); - if (!spreadsheetId) return 'legacy'; - return PropertiesService.getUserProperties().getProperty( + if (!spreadsheetId) return 'legacy'; + cachedGeneration_ = PropertiesService.getUserProperties().getProperty( `${CACHE_GENERATION_PROPERTY}:${spreadsheetId}` ) || 'legacy'; + return cachedGeneration_; }
saveApiKeyanddeleteApiKeymust resetcachedGeneration_tonullafter they write the properties.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 312 - 324, Memoize the result of cacheGeneration_ in a module-level cachedGeneration_ variable so repeated namespacedCacheKey_ calls reuse one generation per execution. Return the cached value when available, and ensure saveApiKey and deleteApiKey reset cachedGeneration_ to null after updating properties so subsequent calls observe the new generation.test/runtime.test.js (5)
572-576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the latest cache write exists before reading it.
If the cache key format changes,
findreturnsundefinedand line 575 throws aTypeError. The test then reports a crash instead of a clear TTL mismatch.♻️ Proposed guard
const latestPut = harness.cachePuts.find((entry) => entry.key.includes("latest_WTI_USD"), ); + assert.ok(latestPut, "expected a latest-price cache write"); assert.equal(latestPut.scope, "document");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtime.test.js` around lines 572 - 576, Guard the latestPut lookup in the cache TTL test before accessing its properties, asserting that a matching cache write exists with a clear failure message. Keep the existing scope and ttlSeconds assertions unchanged after the presence check.
65-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe cache double does not enforce
ttlSeconds.
getreturns any stored value regardless of the recorded TTL. A regression that writes a zero or very short TTL still passes every cache-hit test, because onlycachePutsrecords the TTL. Consider storing an expiry timestamp and returningnullafter it passes. This keeps the double closer toCacheServicebehavior.♻️ Optional: honor TTL in the cache double
- const cacheStore = (scope, values) => ({ - get: (key) => values().get(key) || null, - put: (key, value, ttlSeconds) => { - values().set(key, value); - cachePuts.push({ scope, key, ttlSeconds }); - }, - remove: (key) => values().delete(key), - }); + const cacheExpiry = new Map(); + const cacheStore = (scope, values) => ({ + get: (key) => { + const expiresAt = cacheExpiry.get(key); + if (typeof expiresAt === "number" && Date.now() >= expiresAt) { + values().delete(key); + return null; + } + return values().get(key) || null; + }, + put: (key, value, ttlSeconds) => { + values().set(key, value); + cacheExpiry.set(key, Date.now() + ttlSeconds * 1000); + cachePuts.push({ scope, key, ttlSeconds }); + }, + remove: (key) => { + cacheExpiry.delete(key); + return values().delete(key); + }, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtime.test.js` around lines 65 - 72, Update the cacheStore test double’s get and put methods to enforce ttlSeconds by recording an expiry timestamp for each stored value and returning null once it has expired. Preserve the existing cachePuts tracking and remove behavior while ensuring zero and short TTL values are honored.
954-960: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelecting the first cache key is brittle.
The document cache can hold several entries, for example a cached value plus request-block or tier state.
[...activeCache.keys()][0]then ages an arbitrary entry, and the stale-cache assertion can pass or fail for an unrelated reason. Select the entry by a key substring that identifies the intended value.♻️ Proposed selection by key substring
const activeCache = harness.documentCache.size ? harness.documentCache : harness.userCache; - const cacheKey = [...activeCache.keys()][0]; + const cacheKey = [...activeCache.keys()].find((key) => key.includes("latest_WTI_USD")); + assert.ok(cacheKey, "expected a cached latest-price entry"); const envelope = JSON.parse(activeCache.get(cacheKey));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtime.test.js` around lines 954 - 960, Update the cache-entry selection in the test setup around activeCache so it finds the intended cached value by matching the key substring that identifies that value, rather than taking the first key. Continue aging and updating only the matched entry before the stale-cache assertion.
84-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe lock double ignores ownership and timeout arguments.
getDocumentLock()returns a new object on each call, butreleaseLockclears the sharedlockHeldflag without checking which caller acquired the lock.tryLockalso ignores its timeout argument. IfCode.gsever nests lock scopes or releases a lock it did not acquire, the harness reports success while production would behave differently. Consider tracking an owner token per returned lock object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtime.test.js` around lines 84 - 95, Update the LockService double’s getDocumentLock, tryLock, and releaseLock behavior to track ownership per returned lock object and honor the tryLock timeout argument. Ensure only the lock instance that successfully acquired the shared lock can release it, while preserving failure when the lock is unavailable or already held.
493-514: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe batch fixture uses the history helper.
historyBodybuilds{ status, data: { prices } }. The latest-batch path consumes the same shape, so the test works, but the helper name states the wrong endpoint family. Add alatestBatchBodyalias to keep the fixture intent clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtime.test.js` around lines 493 - 514, Add a latestBatchBody alias for the shared history response fixture helper, then use it in the batch fixture queued by harness.queue. Preserve the existing response shape and test behavior while making the helper name reflect the latest-batch endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Code.gs`:
- Around line 145-148: Update the dialog copy near ADDON_VERSION to distinguish
the runtime release candidate version from the published Marketplace listing
version 1.2.2. Keep the dynamic runtime version display, but explicitly label
the Marketplace availability claim with its separate published version so
readers do not infer that ADDON_VERSION is already listed.
In `@docs/index.html`:
- Line 9: Update the publication-status badge and availability paragraph to
remove rejection/remediation claims, state that runtime 1.2.2 is public, and
identify runtime 1.3.0 as a release candidate. Apply the same correction to both
status references on the page. Extend the existing public-claims validation to
include this page so contradictory availability claims are detected.
---
Outside diff comments:
In `@Code.gs`:
- Around line 676-727: In Code.gs lines 676-727, guard CacheService access in
cacheStore_, getCachedValue_, and removeCachedValue_: catch service, get, or
remove failures and return null or no-op without propagating exceptions; ensure
callers handle an unavailable cache. In Code.gs lines 729-755, guard
LockService.getDocumentLock(), tryLock, and releaseLock so failures return null
or proceed as an unlocked load rather than reaching formula paths as errors.
---
Nitpick comments:
In `@Code.gs`:
- Around line 1343-1370: In the history-fetching flow, compute the endpoint
bucket before reading the cache, then key cacheKey with code and endpoint rather
than requestedDays. Keep the existing endpoint selection in the history function
and ensure both getCachedValue_ and withCacheMissLock_ use the same bucket-based
key.
- Around line 345-361: Update cachedRequestBlock_ and clearRequestBlocks_ to use
CacheService getAll and removeAll with the three keys in one operation each.
Reuse or factor the envelope parsing and age validation from getCachedValue_ so
raw getAll values receive identical handling, and preserve the existing
key-priority order and null behavior.
- Around line 841-867: Update the latest-record processing in the batch flow
around extractPriceRecords_ and validatePriceRecord_ to validate each returned
record independently instead of using a failing .map() call. Cache every valid
record, collect invalid or rejected record codes, and ensure unresolved combines
missing and failed codes so the resulting error reports failures per code while
preserving valid records for the table output.
- Around line 326-343: Update requestBlockKeys_ to include a sanitized,
truncated path-derived prefix alongside each stableCacheHash_ value for endpoint
and request cache keys. Ensure the prefix is safe for Apps Script cache keys and
retain the hash to keep keys bounded and unique enough, while leaving the global
key unchanged.
- Around line 312-324: Memoize the result of cacheGeneration_ in a module-level
cachedGeneration_ variable so repeated namespacedCacheKey_ calls reuse one
generation per execution. Return the cached value when available, and ensure
saveApiKey and deleteApiKey reset cachedGeneration_ to null after updating
properties so subsequent calls observe the new generation.
In `@test/runtime.test.js`:
- Around line 572-576: Guard the latestPut lookup in the cache TTL test before
accessing its properties, asserting that a matching cache write exists with a
clear failure message. Keep the existing scope and ttlSeconds assertions
unchanged after the presence check.
- Around line 65-72: Update the cacheStore test double’s get and put methods to
enforce ttlSeconds by recording an expiry timestamp for each stored value and
returning null once it has expired. Preserve the existing cachePuts tracking and
remove behavior while ensuring zero and short TTL values are honored.
- Around line 954-960: Update the cache-entry selection in the test setup around
activeCache so it finds the intended cached value by matching the key substring
that identifies that value, rather than taking the first key. Continue aging and
updating only the matched entry before the stale-cache assertion.
- Around line 84-95: Update the LockService double’s getDocumentLock, tryLock,
and releaseLock behavior to track ownership per returned lock object and honor
the tryLock timeout argument. Ensure only the lock instance that successfully
acquired the shared lock can release it, while preserving failure when the lock
is unavailable or already held.
- Around line 493-514: Add a latestBatchBody alias for the shared history
response fixture helper, then use it in the batch fixture queued by
harness.queue. Preserve the existing response shape and test behavior while
making the helper name reflect the latest-batch endpoint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87bf01e0-b469-4057-876e-cecdab4480b4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
Code.gsDEPLOYMENT_GUIDE.mdMARKETPLACE_LISTING.mdOAUTH_VERIFICATION.mdREADME.mdSidebar.htmldocs/index.htmlpackage.jsontest/public-claims.test.jstest/runtime.test.jstest/validate_code.js
1 participant
Customer impact
OILPRICE_TABLE(range)to batch up to 25 latest codes in one requestRed-green evidence
npm run validatepasses all 66 tests, package/asset/portfolio validation, and secret scanRelease discipline
This PR does not claim 1.3.0 is published. Deployment still requires an immutable Apps Script version, installed-user smoke, App Configuration selection, and a public-listing smoke. The add-on does not infer or display a numeric plan quota; it follows API status and reset headers.
Closes OilpriceAPI/oilpriceapi-api#5774
Closes OilpriceAPI/oilpriceapi-api#5775
Closes OilpriceAPI/oilpriceapi-api#5776
Closes OilpriceAPI/oilpriceapi-api#5777
Summary by CodeRabbit
New Features
OILPRICE_TABLEspreadsheet function for source-aware latest-price results.Bug Fixes
Documentation