Improve network resilience, monitoring, and social login#528
Merged
Conversation
…awlers making OPTIONS requests
Contributor
|
👋 Hotfix Branch PR Detected! Before merging this Pull Request into This will:
You can trigger the workflow from the 'Actions' tab, selecting the 'Release' workflow, and choosing this |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (6)
📝 WalkthroughWalkthroughThis PR addresses the "fetch failed" socket exhaustion issue in GeoTIFF processing through a multi-layered defense strategy: hybrid loading (small files buffered fully, large files lazy-loaded via HTTP Range), semaphore-based concurrency throttling, abort signals, 10s timeouts, and automatic retries with exponential backoff. Additionally, Sentry instrumentation is added for error tracking and page normalization. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Semaphore
participant getTiff
participant Network as HTTP Network
participant GeoTIFF as GeoTIFF Lib
participant Cache as LRU Cache
Client->>Cache: getTiff(url, signal)
alt Cache Hit
Cache-->>Client: Return cached image
else Cache Miss
Client->>Semaphore: Acquire permit
activate Semaphore
Client->>Network: HEAD request
Network-->>Client: content-length response
alt Size ≤ 2MB
Client->>Network: GET full file
Network-->>Client: arrayBuffer
Client->>GeoTIFF: fromArrayBuffer(buffer)
else Size > 2MB
Client->>GeoTIFF: fromUrl(url, signal)
end
GeoTIFF-->>Client: Image object
Client->>Cache: Store image
Semaphore->>Semaphore: Release permit
deactivate Semaphore
Client-->>Client: Return image
end
Client->>Semaphore: Acquire permit for readRasters
activate Semaphore
Client->>GeoTIFF: readRasters(window, signal)
GeoTIFF-->>Client: Raster data
Semaphore->>Semaphore: Release permit
deactivate Semaphore
Client-->>Client: Return pixel value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-calculator/src/shared/geotiff.ts (2)
102-111:⚠️ Potential issue | 🟠 MajorDon't couple the URL cache to the first caller's signal.
Lines 103-104 reuse the in-flight promise by
urlonly, but the shared promise is created with the first caller'scombinedSignal. If that caller cancels, every other caller awaiting the same TIFF is canceled too. Either keep the shared fetch timeout-only and race each caller with its own signal, or stop deduplicating abortable loads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 102 - 111, The in-flight cache (tiffPromiseCache) currently stores a promise created with a combinedSignal (combining timeoutSignal and the first caller's signal), so if that first caller aborts it cancels all awaiting callers; instead create and cache the shared load promise using only the timeoutSignal (AbortSignal.timeout(DEFAULT_TIMEOUT_MS)) and do not include the caller-provided signal when constructing the cached promise, then for each caller (when returning the cached promise) race or wire that caller's signal against the cached timeout-only promise so that per-call cancellations only affect the caller (i.e., keep the shared fetch promise using timeoutSignal only and handle `signal` per-caller before returning to the caller rather than using `combinedSignal` in the cached promise).
147-151:⚠️ Potential issue | 🟠 MajorPreserve the original abort/fetch failure.
Lines 149-150 wrap every rejection in a new generic
Error, so callers lose the original reason and stack. That breaks the new cancellation contract and makes real network failures harder to classify upstream.Possible fix
} catch (error) { tiffPromiseCache.delete(url) - throw new Error( - `Failed to fetch or parse GeoTIFF from ${url}: ${String(error)}`, - ) + if (combinedSignal.aborted) { + throw combinedSignal.reason + } + throw new Error(`Failed to fetch or parse GeoTIFF from ${url}`, { + cause: error instanceof Error ? error : undefined, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 147 - 151, The catch block for the GeoTIFF fetch currently replaces every rejection with a new generic Error (in the code around tiffPromiseCache.delete(url) and the throw), losing the original error and stack; instead preserve the original rejection by either rethrowing the caught error (throw error) after deleting the cache or, if you need the contextual message, create a new Error that preserves the original as its cause (throw new Error(`Failed to fetch or parse GeoTIFF from ${url}`, { cause: error })), applied in the catch that wraps the fetch/parse logic so callers retain the original abort/network failure details.
🧹 Nitpick comments (1)
fdm-app/app/components/custom/error.tsx (1)
45-46: Normalizepagebefore tagging to avoid high-cardinality metrics.Line 45 uses raw
pageas a metric tag. If it includes IDs/query params, this can explode metric cardinality and reduce metric usability.♻️ Suggested hardening
useEffect(() => { if (clientConfig.analytics.sentry) { + const metricPage = page + .split("?")[0] + .replace(/\/[0-9a-fA-F-]{8,}(?=\/|$)/g, "/:id") + Sentry.withScope((scope) => { scope.setTag("status", status?.toString() ?? "unknown") - scope.setTag("page", page) + scope.setTag("page", metricPage) Sentry.metrics.count("error_block.shown", 1) }) } }, [status, page])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/error.tsx` around lines 45 - 46, The tag call scope.setTag("page", page) uses the raw page string which can contain IDs or query params and cause high-cardinality metrics; before tagging, normalize the page value (e.g., strip query parameters, remove or replace numeric/UUID segments with placeholders, or convert to a canonical route) and use that normalized value in the tag call (e.g., implement a helper normalizePage(page) and change scope.setTag("page", page) to scope.setTag("page", normalizePage(page))). Ensure the normalization logic is deterministic and covers common ID patterns so metrics remain low-cardinality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/perky-geese-clap.md:
- Line 5: Update the release note text to correct the misspelling and phrasing:
replace the incorrect identifier `emailVerfied` with `emailVerified` and change
the phrase "sign in" to the hyphenated form "sign-in", ensuring the sentence
reads clearly (for example: "Set `emailVerified` to true when using social
sign-in").
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 173-179: The semaphore wait can hang cancelled callers because
rasterSemaphore.acquire() is called without an AbortSignal; change the API and
call so acquisitions can be aborted: update rasterSemaphore.acquire to accept an
optional AbortSignal (and immediately reject if signal.aborted), register an
abort listener that removes the waiter from the internal queue and rejects its
promise, and update this call site to pass the current signal (await
rasterSemaphore.acquire(signal)). Also ensure any listener is cleaned up after
acquire resolves/rejects so aborted waiters don't remain in the queue.
- Around line 114-123: The HEAD request using fetchWithRetry (with
combinedSignal) should not cause a hard failure—wrap the HEAD call/response
check in a try/catch (or test response.ok) and treat any error or non-ok as
"unknown size" so execution falls through to the lazy fromUrl path instead of
throwing; remove the throw that references headResponse.status and instead set a
flag or leave size undefined and continue, optionally logging the HEAD failure
for debugging (use fetchWithRetry, combinedSignal, and the fromUrl code paths to
locate the correct places to change).
- Around line 210-212: The readRasters call currently ignores an AbortController
signal so aborting won't stop the geotiff range/decode; update the call to
forward the controller's signal (e.g. replace await image.readRasters({ window
}) with await image.readRasters({ window, signal })) and ensure the surrounding
scope (function parameters or local variables) provides an AbortSignal named
signal (or thread it through from the caller). Also verify TypeScript typings
accept the signal option for image.readRasters and adjust the function signature
to accept/propagate an AbortSignal if necessary.
In `@fdm-core/src/authentication.ts`:
- Line 58: The code currently hardcodes emailVerified: true for social logins;
change it to derive emailVerified from the provider's verification claim
instead: for Google set emailVerified based on the provider's
verified_email/verifiedEmail claim (e.g., profile.verified_email or
profile._json.verified_email), and for Microsoft only set emailVerified if the
Microsoft token/profile explicitly contains an email_verified/verified
claim—otherwise set false (or omit) so the existing invitation auto-acceptance
logic (invitation auto-acceptance at lines 232–239) does not run automatically.
Ensure you update the user creation path where emailVerified is set (the
emailVerified field) to use a conditional based on the provider name and claims.
---
Outside diff comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 102-111: The in-flight cache (tiffPromiseCache) currently stores a
promise created with a combinedSignal (combining timeoutSignal and the first
caller's signal), so if that first caller aborts it cancels all awaiting
callers; instead create and cache the shared load promise using only the
timeoutSignal (AbortSignal.timeout(DEFAULT_TIMEOUT_MS)) and do not include the
caller-provided signal when constructing the cached promise, then for each
caller (when returning the cached promise) race or wire that caller's signal
against the cached timeout-only promise so that per-call cancellations only
affect the caller (i.e., keep the shared fetch promise using timeoutSignal only
and handle `signal` per-caller before returning to the caller rather than using
`combinedSignal` in the cached promise).
- Around line 147-151: The catch block for the GeoTIFF fetch currently replaces
every rejection with a new generic Error (in the code around
tiffPromiseCache.delete(url) and the throw), losing the original error and
stack; instead preserve the original rejection by either rethrowing the caught
error (throw error) after deleting the cache or, if you need the contextual
message, create a new Error that preserves the original as its cause (throw new
Error(`Failed to fetch or parse GeoTIFF from ${url}`, { cause: error })),
applied in the catch that wraps the fetch/parse logic so callers retain the
original abort/network failure details.
---
Nitpick comments:
In `@fdm-app/app/components/custom/error.tsx`:
- Around line 45-46: The tag call scope.setTag("page", page) uses the raw page
string which can contain IDs or query params and cause high-cardinality metrics;
before tagging, normalize the page value (e.g., strip query parameters, remove
or replace numeric/UUID segments with placeholders, or convert to a canonical
route) and use that normalized value in the tag call (e.g., implement a helper
normalizePage(page) and change scope.setTag("page", page) to
scope.setTag("page", normalizePage(page))). Ensure the normalization logic is
deterministic and covers common ID patterns so metrics remain low-cardinality.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a368ca54-092e-4be3-8cd0-2817d26037db
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.changeset/cuddly-lines-dance.md.changeset/empty-cats-build.md.changeset/fix-geotiff-fetch-failed.md.changeset/perky-geese-clap.mdfdm-app/app/components/custom/error.tsxfdm-app/instrument.server.mjsfdm-calculator/src/shared/geotiff.tsfdm-core/src/authentication.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fdm-calculator/src/shared/geotiff.test.ts (2)
283-297: Consider adding a test for abort while waiting in semaphore queue.The current abort test (lines 272-281) tests a pre-aborted signal. Consider adding a test that aborts while a request is waiting in the semaphore queue to fully exercise the
onAborthandler in theSemaphore.acquiremethod.💡 Example test for aborting while queued
it("should reject when aborted while waiting in semaphore queue", async () => { // Fill up the semaphore with long-running operations const blockingPromises = Array.from({ length: 10 }).map(() => getGeoTiffValue("http://example.com/blocking.tif", 50, 50) ) // Start a new request that will be queued const controller = new AbortController() const queuedPromise = getGeoTiffValue( "http://example.com/queued.tif", 50, 50, controller.signal ) // Abort while queued await new Promise(resolve => setTimeout(resolve, 5)) controller.abort(new Error("Aborted while queued")) await expect(queuedPromise).rejects.toThrow("Aborted while queued") await Promise.all(blockingPromises) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.test.ts` around lines 283 - 297, Add a new test that verifies aborting while waiting in the Semaphore queue triggers the onAbort path: start maxConcurrent (10) long-running getGeoTiffValue calls (use mockImage.readRasters to delay), then call getGeoTiffValue for a queued request with an AbortController.signal, wait a few ms and controller.abort(new Error("Aborted while queued")), and assert the queued promise rejects with that error; finally await the blocking promises to let them finish. Reference Semaphore.acquire, getGeoTiffValue, mockImage.readRasters, and AbortController.signal when adding this test.
1-9: Note: Tests usevimocking utilities.These tests use
vi.mock,vi.spyOn, andvi.fnextensively. Based on learnings from the repository, there have been past issues with Vitest's mocking utilities in the FDM project causing problems unrelated to the actual code being tested.If you encounter issues with these tests, consider refactoring to use manual JavaScript mocks instead. However, if the current approach works reliably for
fdm-calculator, this may be acceptable.Based on learnings: "When writing unit tests for the fdm project, avoid using Vitest's mocking utilities (vi) as it has caused problems in the past not related to the actual code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.test.ts` around lines 1 - 9, Tests currently rely on vitest's vi.mock for the "geotiff" module which has caused flaky, unrelated failures in this repo; remove the vi.mock block and replace it with a manual JavaScript mock for the geotiff API (exporting fromArrayBuffer and fromUrl) so the test imports a predictable mock implementation. Create a manual mock module (e.g., __mocks__/geotiff.ts or a local geotiff.mock.ts) that exports the same symbols used by the production code, update the test to import/use that mock instead of calling vi.mock (or configure Vitest to resolve the manual mock), and keep the test assertions targeting getTiff and getGeoTiffValue unchanged. Ensure the mock exports include the functions referenced in the diff (fromArrayBuffer, fromUrl) so getTiff/getGeoTiffValue use the deterministic mock behavior.fdm-calculator/src/shared/geotiff.ts (1)
86-91: Consider consuming response body before retrying on 5xx.When a 5xx error occurs, the response body is not consumed before throwing. In some environments (especially with keep-alive connections), unconsumed response bodies can cause connection issues or resource leaks.
♻️ Optional: Consume response body before retry
// Retry on 5xx server errors if (!response.ok && response.status >= 500 && retries > 0) { + // Consume body to free connection resources + await response.text().catch(() => {}) throw new Error(`Server error: ${response.status}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 86 - 91, The 5xx retry branch throws before consuming the HTTP response body, which can leak connections; in the block that checks "if (!response.ok && response.status >= 500 && retries > 0)" consume the body (e.g., await response.text() or response.arrayBuffer()) or otherwise drain/cancel the stream before throwing so keep-alive connections are freed; update the logic around the response variable in the same function to await body consumption prior to "throw new Error(`Server error: ${response.status}`)" so retries don't leak resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/lib/url-utils.ts`:
- Around line 58-60: The JSDoc for the pathname-normalisation routine (function
normalisePathname) claims dynamic segments are replaced with `:id` but the
implementation returns `:year` and `:centroid`; update the docstring to reflect
the actual placeholders returned by the function (e.g., `:year`, `:centroid`,
etc.) and ensure the descriptive lines referenced (around the current JSDoc
lines) list the same placeholder names as the code to avoid the mismatch.
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 189-192: The large-file branch uses fromUrl(url) which doesn't
receive the AbortSignal, so HTTP range requests keep running after
raceWithSignal aborts; update the fromUrl call in the else branch to pass the
signal (third argument) so the geotiff request is aborted properly (refer to
fromUrl, raceWithSignal, tiff and the existing signal variable).
---
Nitpick comments:
In `@fdm-calculator/src/shared/geotiff.test.ts`:
- Around line 283-297: Add a new test that verifies aborting while waiting in
the Semaphore queue triggers the onAbort path: start maxConcurrent (10)
long-running getGeoTiffValue calls (use mockImage.readRasters to delay), then
call getGeoTiffValue for a queued request with an AbortController.signal, wait a
few ms and controller.abort(new Error("Aborted while queued")), and assert the
queued promise rejects with that error; finally await the blocking promises to
let them finish. Reference Semaphore.acquire, getGeoTiffValue,
mockImage.readRasters, and AbortController.signal when adding this test.
- Around line 1-9: Tests currently rely on vitest's vi.mock for the "geotiff"
module which has caused flaky, unrelated failures in this repo; remove the
vi.mock block and replace it with a manual JavaScript mock for the geotiff API
(exporting fromArrayBuffer and fromUrl) so the test imports a predictable mock
implementation. Create a manual mock module (e.g., __mocks__/geotiff.ts or a
local geotiff.mock.ts) that exports the same symbols used by the production
code, update the test to import/use that mock instead of calling vi.mock (or
configure Vitest to resolve the manual mock), and keep the test assertions
targeting getTiff and getGeoTiffValue unchanged. Ensure the mock exports include
the functions referenced in the diff (fromArrayBuffer, fromUrl) so
getTiff/getGeoTiffValue use the deterministic mock behavior.
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 86-91: The 5xx retry branch throws before consuming the HTTP
response body, which can leak connections; in the block that checks "if
(!response.ok && response.status >= 500 && retries > 0)" consume the body (e.g.,
await response.text() or response.arrayBuffer()) or otherwise drain/cancel the
stream before throwing so keep-alive connections are freed; update the logic
around the response variable in the same function to await body consumption
prior to "throw new Error(`Server error: ${response.status}`)" so retries don't
leak resources.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 712da5ba-4e6c-480b-9abb-7265f2ea8b1f
📒 Files selected for processing (6)
.changeset/perky-geese-clap.mdfdm-app/app/components/custom/error.tsxfdm-app/app/components/custom/navigation-progress.tsxfdm-app/app/lib/url-utils.tsfdm-calculator/src/shared/geotiff.test.tsfdm-calculator/src/shared/geotiff.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/perky-geese-clap.md
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/custom/error.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
fdm-calculator/src/shared/geotiff.ts (2)
40-47: Consider guarding against over-release.If
release()is called more times thanacquire()succeeded (e.g., due to a bug in calling code),activecould go negative, causing the semaphore to allow more thanmaxConcurrentpermits.🛡️ Defensive fix
release(): void { const next = this.queue.shift() if (next) { next() - } else { + } else if (this.active > 0) { this.active-- } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 40 - 47, The release() implementation can drive this.active negative if called more times than acquire(); update release() to guard the semaphore invariant by checking before decrementing: if there is a queued waiter call next() as before, otherwise only decrement this.active when this.active > 0 (and optionally clamp to 0 or log a warning if a spurious release is detected). Refer to the release() and acquire() methods and the this.active and this.queue fields when making the change so the semaphore never allows active < 0 or more than maxConcurrent permits.
265-266: Avoid assuming raster data type.The cast to
Float32Arrayassumes all GeoTIFFs use 32-bit floats, but rasters can use various sample formats (Int16, UInt8, Float64, etc.). While accessing[0]works on any TypedArray, the explicit cast could mislead future maintainers.♻️ Suggested change
// For a single window and band, the result is typically [Float32Array(1)]. - const value = (rasterData[0] as Float32Array)[0] + const band = rasterData[0] as ArrayLike<number> + const value = band[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 265 - 266, The code incorrectly casts rasterData[0] to Float32Array when computing value, which assumes 32-bit float samples; instead, remove the Float32Array cast and treat the band as a generic typed-array/view (e.g., ArrayBufferView | unknown) or index it without a specific cast so different sample formats (Int16, UInt8, Float64, etc.) are supported; update the line using rasterData and value to access rasterData[0][0] (or cast to a generic ArrayBufferView/TypedArray type) rather than casting to Float32Array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 269-276: The current early-return in geotiff.ts uses
Number.isNaN(noDataValue) which causes NaN noData to be mishandled (NaN pixels
always pass); change the condition so noData comparisons handle NaN-by-NaN via
Number.isNaN checks on both sides: treat a pixel as valid only when value !==
undefined AND (noDataValue is null OR (Number.isNaN(noDataValue) ?
!Number.isNaN(value) : value !== noDataValue)); update the return-early branch
(the block that currently checks value, noDataValue and
Number.isNaN(noDataValue)) so that when noDataValue is NaN you filter out pixels
where Number.isNaN(value), and when noDataValue is a real number use strict
equality comparison (value === noDataValue) to decide filtering.
---
Nitpick comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 40-47: The release() implementation can drive this.active negative
if called more times than acquire(); update release() to guard the semaphore
invariant by checking before decrementing: if there is a queued waiter call
next() as before, otherwise only decrement this.active when this.active > 0 (and
optionally clamp to 0 or log a warning if a spurious release is detected). Refer
to the release() and acquire() methods and the this.active and this.queue fields
when making the change so the semaphore never allows active < 0 or more than
maxConcurrent permits.
- Around line 265-266: The code incorrectly casts rasterData[0] to Float32Array
when computing value, which assumes 32-bit float samples; instead, remove the
Float32Array cast and treat the band as a generic typed-array/view (e.g.,
ArrayBufferView | unknown) or index it without a specific cast so different
sample formats (Int16, UInt8, Float64, etc.) are supported; update the line
using rasterData and value to access rasterData[0][0] (or cast to a generic
ArrayBufferView/TypedArray type) rather than casting to Float32Array.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5b3d147-6818-4df1-b753-ea438e52818d
📒 Files selected for processing (3)
fdm-app/app/lib/url-utils.tsfdm-calculator/src/shared/geotiff.test.tsfdm-calculator/src/shared/geotiff.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-calculator/src/shared/geotiff.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/lib/url-utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
fdm-calculator/src/shared/geotiff.ts (2)
56-57: Consider adding cache eviction for long-running servers.The
tiffCachegrows indefinitely as new URLs are processed. For long-running server instances handling many distinct GeoTIFF URLs, this could lead to memory pressure. Consider implementing an LRU eviction policy or a maximum cache size.♻️ Example using a simple LRU approach
+const MAX_CACHE_SIZE = 50 // Adjust based on expected memory budget + const tiffCache = new Map<string, GeoTIFF>() const tiffPromiseCache = new Map<string, Promise<GeoTIFF>>() +function addToCache(url: string, tiff: GeoTIFF): void { + // Simple LRU: delete oldest entry if at capacity + if (tiffCache.size >= MAX_CACHE_SIZE) { + const oldestKey = tiffCache.keys().next().value + if (oldestKey) tiffCache.delete(oldestKey) + } + tiffCache.set(url, tiff) +}Then replace
tiffCache.set(url, tiff)at line 197 withaddToCache(url, tiff).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 56 - 57, The tiffCache and tiffPromiseCache grow unbounded; implement a bounded LRU (or max-size) eviction helper and use it when inserting entries: create an addToCache(key, value) function (and optionally addPromiseToCache for tiffPromiseCache) that stores entries and evicts the least-recently-used or oldest entry when a configured MAX_CACHE_SIZE is exceeded, update all places that currently call tiffCache.set(url, tiff) to call addToCache(url, tiff) and replace direct tiffPromiseCache.set usages with the promise helper so both caches are size-limited, and ensure cache accesses update recency (e.g., move key to front) to preserve LRU semantics for functions that read from tiffCache/tiffPromiseCache.
98-100: Backoff delay doesn't respond to abort signal.If the caller's signal aborts during the
setTimeoutbackoff, the function continues sleeping until the delay completes before the next recursive call detects the abort. This causes up tobackoffms of unnecessary delay.♻️ Proposed fix to honor abort during backoff
if (retries > 0 && !isAbortError) { - await new Promise((resolve) => setTimeout(resolve, backoff)) + await new Promise<void>((resolve, reject) => { + const timer = setTimeout(resolve, backoff) + options.signal?.addEventListener( + "abort", + () => { + clearTimeout(timer) + reject(options.signal!.reason) + }, + { once: true }, + ) + }) return fetchWithRetry(url, options, retries - 1, backoff * 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 98 - 100, The backoff sleep in fetchWithRetry ignores the caller's abort signal and can delay up to backoff ms even after cancellation; replace the simple setTimeout with an abort-aware wait: create a Promise that sets a timeout and also attaches an 'abort' listener to options.signal (or checks options.signal?.aborted) so the promise resolves only when the timeout elapses and rejects or throws immediately if the signal fires, and ensure you clear the timer and remove the listener to avoid leaks before returning to the recursive call in fetchWithRetry (use the same isAbortError/AbortError handling so the function exits immediately on abort instead of sleeping).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 240-246: Replace the manual GDAL_NODATA parsing (which uses
image.fileDirectory.loadValue() and custom type conversion) with the geotiff.js
public helper: call image.getGDALNoData() and assign its result to the existing
noDataValue variable; update any references to noDataValue accordingly and
remove the old loadValue/type-checking logic so you no longer access
image.fileDirectory directly.
---
Nitpick comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 56-57: The tiffCache and tiffPromiseCache grow unbounded;
implement a bounded LRU (or max-size) eviction helper and use it when inserting
entries: create an addToCache(key, value) function (and optionally
addPromiseToCache for tiffPromiseCache) that stores entries and evicts the
least-recently-used or oldest entry when a configured MAX_CACHE_SIZE is
exceeded, update all places that currently call tiffCache.set(url, tiff) to call
addToCache(url, tiff) and replace direct tiffPromiseCache.set usages with the
promise helper so both caches are size-limited, and ensure cache accesses update
recency (e.g., move key to front) to preserve LRU semantics for functions that
read from tiffCache/tiffPromiseCache.
- Around line 98-100: The backoff sleep in fetchWithRetry ignores the caller's
abort signal and can delay up to backoff ms even after cancellation; replace the
simple setTimeout with an abort-aware wait: create a Promise that sets a timeout
and also attaches an 'abort' listener to options.signal (or checks
options.signal?.aborted) so the promise resolves only when the timeout elapses
and rejects or throws immediately if the signal fires, and ensure you clear the
timer and remove the listener to avoid leaks before returning to the recursive
call in fetchWithRetry (use the same isAbortError/AbortError handling so the
function exits immediately on abort instead of sleeping).
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06162198-ca0e-4895-b27c-6a3065d63377
📒 Files selected for processing (1)
fdm-calculator/src/shared/geotiff.ts
Comment on lines
+240
to
+246
| const _noData = await image.fileDirectory.loadValue("GDAL_NODATA") | ||
| const noDataValue = | ||
| typeof _noData === "string" | ||
| ? Number.parseFloat(_noData) | ||
| : _noData != null | ||
| ? Number(_noData) | ||
| : null |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if geotiff.js provides a public API for GDAL_NODATA
# and how loadValue is typically used
# Search for GDAL_NODATA usage in the codebase
rg -n "GDAL_NODATA|getGDALNoData|loadValue" --type ts
# Check geotiff.js type definitions
fd "geotiff" node_modules --type d --exec sh -c 'find {} -name "*.d.ts" | head -5 | xargs cat 2>/dev/null | grep -A5 -E "(getGDALNoData|loadValue|fileDirectory)" | head -30'Repository: nmi-agro/fdm
Length of output: 754
🌐 Web query:
geotiff.js loadValue GDAL_NODATA API documentation
💡 Result:
In geotiff.js (v3+), the ImageFileDirectory object (accessed via image.fileDirectory) provides methods to read TIFF tags, including GDAL_NODATA: - Use image.fileDirectory.getValue('GDAL_NODATA') for synchronous access to small/immediately available tags. Throws if deferred. - Use await image.fileDirectory.loadValue('GDAL_NODATA') for potentially deferred tags (recommended for safety, as GDAL_NODATA can be a string). For convenience, GeoTIFFImage provides a dedicated method: image.getGDALNoData which returns the parsed number value or null. This internally handles the GDAL_NODATA tag parsing, including float precision considerations noted in issues. Example: const tiff = await GeoTIFF.fromUrl('example.tif'); const image = await tiff.getImage; const nodata = image.getGDALNoData; // number|null // or manually: const rawNodata = await image.fileDirectory.loadValue('GDAL_NODATA'); // string like '-3.4e+38' Note: readRasters and resample do not automatically mask nodata values (open issues #257, #448). Manually filter using the nodata value after reading. Official docs: https://geotiffjs.github.io/geotiff.js/ (migration guide covers fileDirectory changes); API: https://geotiffjs.github.io/geotiff.js/module-geotiffimage-GeoTIFFImage.html#getGDALNoData
Citations:
- 1: https://geotiffjs.github.io/geotiff.js/
- 2: https://geotiffjs.github.io/geotiff.js/module-geotiffimage-GeoTIFFImage.html
- 3: https://github.com/geotiffjs/geotiff.js/
- 4: https://github.com/geotiffjs/geotiff.js/blob/master/README.md
- 5: Is there a way to read raster data from a geotiff while excluding nodata? geotiffjs/geotiff.js#257
- 6: Add GeoTIFFImage.getGDALNoData() geotiffjs/geotiff.js#114
🌐 Web query:
geotiff.js fileDirectory loadValue method
💡 Result:
The loadValue method is part of the ImageFileDirectory class (accessed via image.fileDirectory) in geotiff.js version 3.0+, introduced for deferred loading of TIFF tags to improve performance. It asynchronously loads the full value of a potentially deferred tag (e.g., large arrays like 'ColorMap' or 'TileOffsets'), returning a Promise that resolves to the tag's value. Usage example: const colorMap = await image.fileDirectory.loadValue('ColorMap'); const offset = await image.fileDirectory.loadValueIndexed('TileOffsets', 5); In contrast, getValue(tagName) provides synchronous access but throws if the tag is deferred. This change is part of v3 migration: direct property access (e.g., image.fileDirectory.ColorMap) is replaced by these methods. Check hasTag(tagName) before accessing. Official sources: geotiff.js GitHub README and documentation (v3.0.5 as of 2026-03-11).
Citations:
- 1: https://github.com/geotiffjs/geotiff.js/blob/master/README.md
- 2: https://geotiffjs.github.io/geotiff.js/
- 3: https://github.com/geotiffjs/geotiff.js/
- 4: https://www.npmjs.com/package/geotiff
🏁 Script executed:
# Search the codebase for how image object is created and what type it is
rg -n "image\s*=" fdm-calculator/src/shared/geotiff.ts | head -10Repository: nmi-agro/fdm
Length of output: 103
🏁 Script executed:
# Look at the test mocks to understand the expected interface
cat -n fdm-calculator/src/shared/geotiff.test.ts | sed -n '180,270p'Repository: nmi-agro/fdm
Length of output: 4069
Simplify GDAL_NODATA retrieval using the public API method.
The code manually parses the GDAL_NODATA value via image.fileDirectory.loadValue() and type conversion logic (lines 240–246). geotiff.js provides a dedicated convenience method image.getGDALNoData() that returns the parsed value directly (number | null), which internally handles tag parsing and float precision. Replace lines 240–246 with const noDataValue = image.getGDALNoData() to simplify the code and avoid accessing internal APIs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/shared/geotiff.ts` around lines 240 - 246, Replace the
manual GDAL_NODATA parsing (which uses image.fileDirectory.loadValue() and
custom type conversion) with the geotiff.js public helper: call
image.getGDALNoData() and assign its result to the existing noDataValue
variable; update any references to noDataValue accordingly and remove the old
loadValue/type-checking logic so you no longer access image.fileDirectory
directly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fdm-calculator/src/shared/geotiff.ts (1)
93-109: Minor: clean up abort listener when timer fires.When the timer resolves successfully, the abort listener remains attached to the signal. While the impact is minimal (signals are typically short-lived), cleaning it up ensures no dangling references.
♻️ Suggested fix
function abortableDelay(ms: number, signal?: AbortSignal | null): Promise<void> { return new Promise<void>((resolve, reject) => { if (signal?.aborted) { reject(signal.reason) return } - const timer = setTimeout(resolve, ms) - signal?.addEventListener( - "abort", - () => { - clearTimeout(timer) - reject(signal.reason) - }, - { once: true }, - ) + const onAbort = () => { + clearTimeout(timer) + reject(signal!.reason) + } + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort) + resolve() + }, ms) + signal?.addEventListener("abort", onAbort, { once: true }) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 93 - 109, The abortableDelay function leaves the abort listener attached when the timer fires; change the implementation in abortableDelay to store the abort handler in a variable, use that handler when calling signal.addEventListener, and remove that listener inside the timer callback before resolving (and keep the existing clearTimeout/remove logic in the abort handler) so the event listener is cleaned up when the promise resolves or rejects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 93-109: The abortableDelay function leaves the abort listener
attached when the timer fires; change the implementation in abortableDelay to
store the abort handler in a variable, use that handler when calling
signal.addEventListener, and remove that listener inside the timer callback
before resolving (and keep the existing clearTimeout/remove logic in the abort
handler) so the event listener is cleaned up when the promise resolves or
rejects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68296456-ce2b-4cea-bb76-c09a74863547
📒 Files selected for processing (1)
fdm-calculator/src/shared/geotiff.ts
BoraIneviNMI
previously approved these changes
Mar 24, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
I thought that the errors appeared because of internet connection issues but if such semaphore logic was necessary I am fine with it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fdm-calculator/src/shared/geotiff.ts (1)
197-240:⚠️ Potential issue | 🟠 MajorGive the optional
HEADprobe its own timeout budget.Line 199 reuses the same
timeoutSignalfor the preflightHEADand the actualGET/fromUrlload. IfHEADburns the 10s deadline, the fallback branch receives an already-aborted signal and fails immediately, so slow origins still turnHEADinto a hard prerequisite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 197 - 240, The HEAD probe reuses the shared timeoutSignal so if the HEAD consumes the 10s budget the later GET/fromUrl path gets an already-aborted signal; give the HEAD probe its own abort signal instead (e.g., create a separate headTimeoutSignal or headAbortController with its own shorter timeout like HEAD_TIMEOUT_MS) and pass that signal into the HEAD fetchWithRetry call, while keeping the original timeoutSignal for the subsequent response.arrayBuffer()/fromUrl calls (references: the surrounding promise, timeoutSignal, fetchWithRetry for the HEAD request, the sizeBytes branch that calls response.arrayBuffer(), and fromUrl).
🧹 Nitpick comments (2)
fdm-app/app/root.tsx (1)
159-159:<ErrorBoundary error={null} ... />is a no-op here.At Line [159],
erroris hardcoded tonull, and this component returnsnullfor that case (Line [272]), so this render adds no behavior.✂️ Proposed cleanup
- <ErrorBoundary error={null} params={{}} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/root.tsx` at line 159, The ErrorBoundary usage is being passed a hardcoded error={null} which makes it a no-op (ErrorBoundary returns null when error is null); update the render in root.tsx by either removing the ErrorBoundary element entirely or by passing a real error/props when you intend to render an error state—locate the <ErrorBoundary ... /> invocation and delete it if unused, or wire it to the actual error source instead of error={null}.fdm-calculator/src/shared/geotiff.ts (1)
286-309: Move the NoData lookup below the OOB fast path.Line 286 runs before the out-of-bounds return on Lines 300-303, but
noDataValueis only used afterreadRasters()on Line 309. Deferring that lookup keeps outside-of-raster requests as cheap as possible on this hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/shared/geotiff.ts` around lines 286 - 309, The GDAL_NODATA lookup (image.fileDirectory.loadValue("GDAL_NODATA") and the noDataValue calculation) should be moved below the explicit out-of-bounds check so out-of-raster requests return quickly; locate the existing noDataValue creation and instead perform that load and Number conversion after confirming xPx/yPx are inside the TIFF (after the if (xPx < 0 || xPx >= pixelWidth || yPx < 0 || yPx >= pixelHeight) return null) and before you actually use noDataValue (i.e., before/after calling image.readRasters() where noDataValue is needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 156-174: After acquiring rasterSemaphore
(rasterSemaphore.acquire(signal)) but before touching shared tiff loader/cache
or calling getTiff(url, signal), check signal.aborted and if true release the
semaphore (or bail) and throw/reject with signal.reason so you never start or
access shared loaders when already cancelled; similarly, in the miss path where
you would create a new loader (the getTiff or loader constructor code referenced
in getTiff and raceWithSignal), check signal.aborted immediately and avoid
creating or start running the loader if cancelled (release any semaphore and
return/reject), and ensure raceWithSignal does not allow a background loader to
keep running when it rejects by gatekeeping creation with the aborted check.
- Around line 93-110: abortableDelay currently reads signal.reason inside the
onAbort closure while signal is still nullable; capture a non-null reference or
null-guard it after the early-abort check. Specifically, after the initial if
(signal?.aborted) return guard, store const abortSignal = signal! (or const
abortSignal = signal; and handle undefined) and use abortSignal inside onAbort
and when adding/removing the listener (onAbort should reference
abortSignal.reason), or alternatively null-check signal inside onAbort before
accessing .reason to satisfy TS18049.
---
Duplicate comments:
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 197-240: The HEAD probe reuses the shared timeoutSignal so if the
HEAD consumes the 10s budget the later GET/fromUrl path gets an already-aborted
signal; give the HEAD probe its own abort signal instead (e.g., create a
separate headTimeoutSignal or headAbortController with its own shorter timeout
like HEAD_TIMEOUT_MS) and pass that signal into the HEAD fetchWithRetry call,
while keeping the original timeoutSignal for the subsequent
response.arrayBuffer()/fromUrl calls (references: the surrounding promise,
timeoutSignal, fetchWithRetry for the HEAD request, the sizeBytes branch that
calls response.arrayBuffer(), and fromUrl).
---
Nitpick comments:
In `@fdm-app/app/root.tsx`:
- Line 159: The ErrorBoundary usage is being passed a hardcoded error={null}
which makes it a no-op (ErrorBoundary returns null when error is null); update
the render in root.tsx by either removing the ErrorBoundary element entirely or
by passing a real error/props when you intend to render an error state—locate
the <ErrorBoundary ... /> invocation and delete it if unused, or wire it to the
actual error source instead of error={null}.
In `@fdm-calculator/src/shared/geotiff.ts`:
- Around line 286-309: The GDAL_NODATA lookup
(image.fileDirectory.loadValue("GDAL_NODATA") and the noDataValue calculation)
should be moved below the explicit out-of-bounds check so out-of-raster requests
return quickly; locate the existing noDataValue creation and instead perform
that load and Number conversion after confirming xPx/yPx are inside the TIFF
(after the if (xPx < 0 || xPx >= pixelWidth || yPx < 0 || yPx >= pixelHeight)
return null) and before you actually use noDataValue (i.e., before/after calling
image.readRasters() where noDataValue is needed).
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8be3149f-114b-499d-a493-a8f50594fae7
📒 Files selected for processing (3)
.changeset/clear-words-appear.mdfdm-app/app/root.tsxfdm-calculator/src/shared/geotiff.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/clear-words-appear.md
Comment on lines
+156
to
+174
| function raceWithSignal(promise: Promise<GeoTIFF>, signal: AbortSignal): Promise<GeoTIFF> { | ||
| return new Promise<GeoTIFF>((resolve, reject) => { | ||
| if (signal.aborted) { | ||
| reject(signal.reason) | ||
| return | ||
| } | ||
| const onAbort = () => reject(signal.reason) | ||
| signal.addEventListener("abort", onAbort, { once: true }) | ||
| promise.then( | ||
| (value) => { | ||
| signal.removeEventListener("abort", onAbort) | ||
| resolve(value) | ||
| }, | ||
| (err) => { | ||
| signal.removeEventListener("abort", onAbort) | ||
| reject(err) | ||
| }, | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Honor signal.aborted before touching the shared TIFF loader.
If cancellation lands between await rasterSemaphore.acquire(signal) and getTiff(url, signal), Line 189 can still return cached data and Line 197 can still create a new loader. In the miss case raceWithSignal() rejects immediately, so that loader keeps running in the background and a later failure can surface unhandled.
🧯 Small guard that closes the hole
export async function getTiff(url: string, signal?: AbortSignal): Promise<GeoTIFF> {
+ if (signal?.aborted) {
+ throw signal.reason
+ }
+
// Return cached object if it exists
const cached = tiffCacheGet(url)Also applies to: 187-199, 255-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/shared/geotiff.ts` around lines 156 - 174, After acquiring
rasterSemaphore (rasterSemaphore.acquire(signal)) but before touching shared
tiff loader/cache or calling getTiff(url, signal), check signal.aborted and if
true release the semaphore (or bail) and throw/reject with signal.reason so you
never start or access shared loaders when already cancelled; similarly, in the
miss path where you would create a new loader (the getTiff or loader constructor
code referenced in getTiff and raceWithSignal), check signal.aborted immediately
and avoid creating or start running the loader if cancelled (release any
semaphore and return/reject), and ensure raceWithSignal does not allow a
background loader to keep running when it rejects by gatekeeping creation with
the aborted check.
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 by CodeRabbit
Bug Fixes
405 Method Not Allowedresponses from bots/crawlersNew Features
Closes #527