Request caching #1636
Request caching #1636
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a request-scoped promise deduper ( Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1636 +/- ##
==========================================
+ Coverage 9.92% 10.70% +0.78%
==========================================
Files 315 317 +2
Lines 22687 22848 +161
Branches 412 457 +45
==========================================
+ Hits 2251 2447 +196
+ Misses 20436 20401 -35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/app/c/community.tsx (1)
85-98: Fix theog:urlmeta tag—it's stringifying an object, not the base URLThe function returns an object map (
{ VITE_BETA_SITE_URL?: string }), so concatenating it directly produces[object Object]/c/fooinstead of the intended URL. Extract the value first:const { VITE_BETA_SITE_URL } = getPublicEnvVariables(["VITE_BETA_SITE_URL"]); <meta property="og:url" content={`${VITE_BETA_SITE_URL ?? ""}${location.pathname}`} />
🧹 Nitpick comments (4)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
57-95: Request‑scoped Proxy + caching behavior is correct but narrowly scopedThe
getRequestScopedDapper(request)Proxy only wrapsget*methods and delegates them throughgetCachedDapperPromisewith a per‑Requestcache, which is exactly what you want for cross‑clientLoader dedupe. Non‑getmethods are passed through as normal bound functions, so POST‑like operations won’t be cached. Just keep in mind:
- Any new DapperTs methods that should be deduped must keep the
get*naming convention.- Cached promises (including rejections) are reused for the lifetime of the
Request, so if you ever need “retry on same args within a request”, you’d need to evict on rejection.apps/cyberstorm-remix/app/entry.client.tsx (1)
7-25: Dapper priming order is good; double‑checkgetConfigsignature and env failure modeInitializing
sessionToolsand callingprimeClientDapperbeforeSentry.initis a good place to ensure the global Dapper instance has a session‑aware config early. Two things to confirm:
sessionTools.getConfig(publicEnvVariables.VITE_API_URL)—make suregetConfigis meant to accept an optional API host override; otherwise this argument may be ignored or cause a type mismatch.getSessionTools()now runs at module evaluation and will throw if required env vars are missing, which will prevent the app from hydrating at all. If you ever want Sentry + a minimal UI even with broken env, you may want to lazy‑init sessionTools instead.apps/cyberstorm-remix/app/p/tabs/Readme/Readme.tsx (1)
10-39: Loader/clientLoader community shape differs; consider unifying itOn the server loader you
await dapper.getCommunity(...), but on the client loader you returncommunityas a Promise fromgetRequestScopedDapper. Right now the component only logscommunity, so this mismatch isn’t user‑visible, but it does meancommunityis sometimes a plain object and sometimes a Promise.If you plan to actually render from
communityhere (e.g. via<Await>), it’d be safer to make both loaders return the same shape (either both promises or both resolved values) to avoid subtle runtime checks down the line.apps/cyberstorm-remix/cyberstorm/utils/dapperRequestCache.ts (1)
14-75: Per‑request cache + abort cleanup look solid; be explicit about rejected‑promise behaviorThe WeakMap+AbortSignal approach in
getCachegives you nice per‑Request lifetime for cached entries, andrestrictedNames+labelavoid issues with minified/anonymous functions. Two behavioral notes to be aware of:
getCachedDapperPromisekeeps a rejected promise in the cache, so repeated calls with the same method+inputs during a single request will all see the same rejection (no implicit retry).- The global fallback cache is only used when
requestis undefined; currentlygetRequestScopedDapperalways passes arequest, so the global path is effectively unused.If you ever want retries on identical args within the same request, consider evicting entries on rejection, e.g. by attaching a
.catchthat removes the entry before rethrowing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/cyberstorm-remix/app/c/community.tsx(2 hunks)apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/app/p/tabs/Readme/Readme.tsx(3 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperRequestCache.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/dapper-ts/src/index.ts (1)
DapperTs(38-96)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-67)apps/cyberstorm-remix/cyberstorm/utils/dapperRequestCache.ts (1)
getCachedDapperPromise(39-71)
apps/cyberstorm-remix/app/c/community.tsx (3)
apps/cyberstorm-remix/app/p/tabs/Readme/Readme.tsx (1)
clientLoader(32-46)apps/cyberstorm-remix/app/upload/upload.tsx (1)
clientLoader(78-88)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
getRequestScopedDapper(57-96)
apps/cyberstorm-remix/app/p/tabs/Readme/Readme.tsx (4)
apps/cyberstorm-remix/app/c/community.tsx (2)
loader(31-46)clientLoader(48-57)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getPublicEnvVariables(19-48)packages/dapper-ts/src/index.ts (1)
DapperTs(38-96)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
getRequestScopedDapper(57-96)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-67)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
primeClientDapper(28-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (2)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
11-26: Config factory resolution and window‑bound priming look sound
resolveConfigFactory+primeClientDappercorrectly avoid touchingwindowon the server, and centralize all config viacurrentConfigFactory. Thefactoryoverride path inprimeClientDapperalso correctly flows throughresolveConfigFactory()so the provided sessionTools‑based config is what backs the initialDapperTsinstance.apps/cyberstorm-remix/app/c/community.tsx (1)
48-55: clientLoader now correctly participates in per‑request Dapper cachingSwitching
clientLoaderto accept{ params, request }and usinggetRequestScopedDapper(request)meanscommunityloads can now be deduped across client loaders that share the sameRequest. Returning the unresolved Promise and consuming it via<Await resolve={community}>is consistent with the pattern used elsewhere.
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
1d366e2 to
1fd696b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/cyberstorm-remix/app/c/community.tsx (1)
31-46: Consider refactoring the server loader for consistency.The server-side
loader(lines 31-46) still manually constructsDapperTswhileclientLoaderuses the newgetRequestScopedDapperpattern. For maintainability, consider creating a server-side equivalent (e.g.,getServerDapper) or a unified accessor that works in both environments.apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (2)
42-45: Remove commented placeholder code.The commented line in
afterEachserves no purpose and should be removed for cleaner code.Apply this diff:
afterEach(() => { - // Restore window - // globalThis.window = originalWindow; // This might be read-only in some envs });
48-55: Skipped window undefined tests are acceptable for browser environment.Since this module is browser-focused and
windowis always defined in the test environment, it's reasonable to skip these tests. Consider removing the empty test functions and replacing them with a single comment explaining that these scenarios are not applicable in browser mode.Example:
describe("primeClientDapper", () => { - it("does nothing if window is undefined", () => { - // Simulating window undefined is hard in browser env. - // We can skip this test or try to mock it if possible. - // In browser mode, window is always defined. - // We can try to spy on window property access if possible, but 'window' is a global. - // If we are in browser mode, we can't easily delete window. - // So we might skip this test for browser environment. - }); + // Note: Tests for window undefined scenarios are omitted as this module is browser-only it("initializes window.Dapper if it does not exist", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/cyberstorm-remix/app/c/community.tsx(2 hunks)apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperRequestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperRequestCache.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts
- apps/cyberstorm-remix/cyberstorm/utils/dapperRequestCache.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperRequestCache.test.ts (1)
apps/cyberstorm-remix/cyberstorm/utils/dapperRequestCache.ts (2)
clearGlobalDapperCache(73-75)getCachedDapperPromise(39-71)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (2)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
resetDapperSingletonForTest(98-100)primeClientDapper(28-43)getClientDapper(45-55)getRequestScopedDapper(57-96)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)
apps/cyberstorm-remix/app/c/community.tsx (1)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
getRequestScopedDapper(57-96)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-67)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
primeClientDapper(28-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build
- GitHub Check: ESLint
- GitHub Check: Generate visual diffs
🔇 Additional comments (4)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
2-2: LGTM! Import path consolidation.The updated import path correctly references the typeChecks utility module.
apps/cyberstorm-remix/app/c/community.tsx (1)
48-57: LGTM! Request-scoped Dapper simplifies the clientLoader.The refactor to use
getRequestScopedDapper(request)is clean and enables request deduplication as intended. The promise is correctly returned unwrapped for Suspense streaming.apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperRequestCache.test.ts (1)
1-109: LGTM! Comprehensive test coverage for request cache.The test suite covers all critical behaviors:
- Global and per-request caching
- Cache isolation across different requests and inputs
- Restricted name handling with and without labels
- Cache clearing mechanisms including abort signal cleanup
The abort signal test (lines 92-108) is particularly valuable for preventing memory leaks.
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
1-160: LGTM! Solid test coverage for the singleton module.The test suite comprehensively covers:
- Dapper initialization and factory configuration
- Client dapper getter with lazy initialization
- Request-scoped proxy creation and caching
- Proxy behavior for get vs non-get methods
The tests appropriately use mocks and verify the singleton pattern works as expected.
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
1fd696b to
5562cf2
Compare
Oksamies
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
44-50: Cache lookup uses linear search.The current implementation iterates through all cached entries to find a match. For typical use cases with a handful of cached functions per request, this is acceptable. If you anticipate many cached functions per request, consider using a Map keyed by a hash of
funcName + inputsfor O(1) lookups.apps/cyberstorm-remix/app/entry.client.tsx (1)
23-23: Clarify or remove TODO about Sentry.init synchronicity.
Sentry.initis synchronous for core setup in the browser SDK, so errors thrown in the same event loop tick (like your try-catch below) will be captured. If this has been verified, remove the TODO. Otherwise, consider testing or checking Sentry docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts
- apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (2)
getPublicEnvVariables(19-48)getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
initializeClientDapper(28-43)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts (1)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
getCachedRequestPromise(33-61)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (2)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
resetDapperSingletonForTest(98-100)initializeClientDapper(28-43)getClientDapper(45-55)getRequestScopedDapper(57-96)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Generate visual diffs
🔇 Additional comments (4)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
18-27: LGTM - proper abort handling.The abort listener correctly clears the per-request cache while allowing in-flight promises to complete naturally. Good defensive checks for signal existence.
apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts (1)
1-76: Excellent test coverage.The test suite thoroughly validates caching semantics, request isolation, named function enforcement, and abort behavior. Well structured.
apps/cyberstorm-remix/app/entry.client.tsx (1)
78-86: Past review concern is resolved.The initialization now correctly happens after
Sentry.init(line 24), andpublicEnvVariablesis passed togetSessionToolsto avoid redundant fetching. Error handling properly captures exceptions via Sentry.apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
1-147: Comprehensive test suite with good coverage.The tests properly validate singleton initialization, client retrieval, request-scoped proxy behavior, and method interception semantics. Setup/teardown ensures proper isolation. Well done.
|
Updated PR based on the days meeting.
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces request-scoped caching for API calls to improve performance by memoizing responses within a single request lifecycle. The implementation uses a WeakMap-based cache that automatically cleans up when requests are garbage collected and clears on request abort signals.
Key Changes
- Implements a generic request-scoped promise caching utility (
requestCache.ts) - Creates a Dapper singleton pattern with automatic caching for GET methods via Proxy (
dapperSingleton.ts) - Initializes the Dapper client at application entry point with proper error handling
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts |
New utility providing request-scoped promise caching with automatic cleanup on request abort |
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts |
New singleton pattern for DapperTs with request-scoped proxies that cache GET methods |
apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts |
Comprehensive tests for the caching mechanism including abort scenarios |
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts |
Tests for Dapper initialization and proxy-based caching behavior |
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts |
Updated import path to use relative imports and added optional parameter to getSessionTools |
apps/cyberstorm-remix/app/entry.client.tsx |
Initializes Dapper client at application entry with environment configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Show resolved
Hide resolved
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Show resolved
Hide resolved
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
5562cf2 to
7488a24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
59-59: Fix typo in error message."Enviroment" should be "Environment".
- "Enviroment variables did not load correctly, please hard refresh page" + "Environment variables did not load correctly, please hard refresh page"
♻️ Duplicate comments (3)
apps/cyberstorm-remix/app/entry.client.tsx (1)
23-23: Clarify or remove vague TODO.This TODO doesn't provide actionable information.
Sentry.initis synchronous in its setup but integrations may register async handlers. Consider:
- Removing it if the synchronous behavior has been verified
- Making it specific: "TODO: Verify initializeClientDapper errors are captured after Sentry.init completes"
- Linking to a tracking issue
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
105-123: Consider adding promise reference equality test.The test verifies caching behavior by checking call count, but doesn't confirm that both invocations receive the same promise instance. Adding this check would strengthen the test:
const promise1 = dapper.getCommunities(); const promise2 = dapper.getCommunities(); expect(promise1).toBe(promise2);Additionally, consider testing that the same method with different arguments creates separate cache entries (e.g.,
getCommunity("1")vsgetCommunity("2")).apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
57-96: LGTM - Per-request proxy with caching.The proxy implementation correctly:
- Caches proxies per Request using WeakMap (automatic cleanup)
- Returns non-function properties directly
- Binds non-getter methods without interception
- Wraps getter methods (starting with "get") with
getCachedRequestPromiseNote: As mentioned in previous reviews, this assumes all query methods follow the "get" prefix naming convention. Non-conforming query methods won't be cached.
🧹 Nitpick comments (1)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
11-19: Minor: Environment variables refetched in fallback path.When
resolveConfigFactoryis called without a provided factory (line 16), it callsgetSessionTools()which refetches environment variables. Whileentry.client.tsxcorrectly passes pre-fetched env togetSessionTools(publicEnvVariables), this internal fallback path doesn't benefit from that optimization.Given this is initialization code that runs once and only in the fallback case, the performance impact is negligible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts
- apps/cyberstorm-remix/cyberstorm/utils/tests/requestCache.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (2)
getPublicEnvVariables(19-48)getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
initializeClientDapper(28-43)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
resetDapperSingletonForTest(98-100)initializeClientDapper(28-43)getClientDapper(45-55)getRequestScopedDapper(57-96)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
getCachedRequestPromise(41-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (4)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
2-2: LGTM - Improved testability.The relative import path and optional
preFetchedEnvparameter address the previous feedback about redundant environment variable fetching and improve testability.Also applies to: 50-53
apps/cyberstorm-remix/app/entry.client.tsx (1)
78-86: LGTM - Error handling and optimization implemented.The try/catch block properly captures initialization errors and routes them to Sentry. Passing
publicEnvVariablestogetSessionToolsavoids redundant environment variable fetching as addressed in previous review feedback.apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (2)
28-55: LGTM - Browser guards and lazy initialization.The browser runtime guards and lazy initialization pattern are implemented correctly. The separation between explicit initialization (
initializeClientDapper) and lazy initialization (getClientDapper) provides flexibility for different usage patterns.
98-100: LGTM - Test reset function.Resetting only
currentConfigFactoryis sufficient. TherequestScopedProxiesWeakMap doesn't need explicit clearing since Request objects created in tests will be garbage collected, automatically removing their WeakMap entries.
|
Ah looks like copilot review request is different from its auto-scan review, I'll go through those tomorrow |
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts
Outdated
Show resolved
Hide resolved
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Show resolved
Hide resolved
- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose. - Rename `getRequestScopedDapper` to `getDapperForRequest`. - Improve cache management: - Immediately handle aborted requests to prevent race conditions. - Remove rejected promises from cache to allow retries. - Use `WeakMap.delete` for safer cache clearing instead of array splicing. - Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods). - Expand test coverage: - Verify promise reference equality. - Ensure different arguments create distinct cache entries. - Test behavior with aborted signals. - Remove- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose. - Rename `getRequestScopedDapper` to `getDapperForRequest`. - Improve cache management: - Immediately handle aborted requests to prevent race conditions. - Remove rejected promises from cache to allow retries. - Use `WeakMap.delete` for safer cache clearing instead of array splicing. - Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods). - Expand test coverage: - Verify promise reference equality. - Ensure different arguments create distinct cache entries. - Test behavior with aborted signals. - Remove resolved TODO in entry.client.tsx
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
7488a24 to
ffb0b04
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/cyberstorm-remix/app/entry.client.tsx (1)
77-85: Consider whether Dapper init failures should block hydration or surface a user-facing fallbackThe try/catch here ensures env/session/Dapper init errors are captured by Sentry and hydration proceeds, but it also means the app can start without a configured Dapper instance and only fail later when something calls
getClientDapper/getDapperForRequest. If a missing/invalidVITE_API_URL/VITE_COOKIE_DOMAINis considered a fatal misconfiguration, consider either rethrowing afterSentry.captureException(error)or gatingstartTransitionbehind successful initialization so failures are more immediate and predictable.apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (2)
13-37: Abort handling correctly clears per-request cache but still allows new work on aborted RequestsThe updated
getCachelogic looks solid for avoiding stale entries: it clearsrequestScopedCachesimmediately ifrequest.signal.abortedand installs a one-shot abort listener when the cache is first created. Note that for already-aborted requests you still get a fresh, non-persisted cache array, sodeduplicatePromiseForRequestwill happily executefuncbut won’t reuse or retain any entries. If the intent is “no further work at all once a Request is aborted”, you may want an early error or short‑circuit indeduplicatePromiseForRequestinstead of just skipping persistence; otherwise this current behavior is fine and worth documenting as “cache cleanup only, not cancellation.”
71-84: Rejection cleanup is correct but could be documented as part of the cache contractThe pattern of attaching
promise.catchpurely for side effects (removing the failed entry from the WeakMap cache) is correct here and doesn’t interfere with consumers awaiting the originalpromise. It might be useful to briefly document that failed calls are evicted from the cache to allow retries, while successful ones remain cached for the lifetime of theRequest, so callers know what to expect for transient vs permanent failures.apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (2)
64-104: Strengthen test reset by also clearingwindow.Dapperand per-request proxies
resetDapperSingletonForTestonly resetscurrentConfigFactory, butwindow.DapperandrequestScopedProxiescan still carry state between tests. For deterministic test runs, consider extending the reset to also remove the global singleton and per-request proxies, e.g. by deletingwindow.Dapper(when defined) and reinitializingrequestScopedProxies(which would require changing it fromconsttolet). That keeps each test run isolated from any prior Dapper configuration or proxies.
64-75: Make the browser-only nature ofgetDapperForRequestexplicit
getDapperForRequestalways routes throughgetClientDapper, so on the server it will throwgetClientDapper can only run in the browser, even when aRequestis provided. If this function is intended strictly for client loaders, consider either:
- adding a short guard (
if (typeof window === "undefined") throw ...) with an error message that namesgetDapperForRequest, and/or- mentioning in the docstring that it’s browser-only and not for SSR loaders.
This will make misuse easier to diagnose if someone accidentally wires it into a server-side loader.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cyberstorm-remix/cyberstorm/utils/tests/dapperSingleton.test.ts
- apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts
- apps/cyberstorm-remix/cyberstorm/utils/tests/requestCache.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (2)
getPublicEnvVariables(19-48)getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
initializeClientDapper(28-43)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
deduplicatePromiseForRequest(48-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose. - Rename `getRequestScopedDapper` to `getDapperForRequest`. - Improve cache management: - Immediately handle aborted requests to prevent race conditions. - Remove rejected promises from cache to allow retries. - Use `WeakMap.delete` for safer cache clearing instead of array splicing. - Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods). - Expand test coverage: - Verify promise reference equality. - Ensure different arguments create distinct cache entries. - Test behavior with aborted signals. - Remove- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose. - Rename `getRequestScopedDapper` to `getDapperForRequest`. - Improve cache management: - Immediately handle aborted requests to prevent race conditions. - Remove rejected promises from cache to allow retries. - Use `WeakMap.delete` for safer cache clearing instead of array splicing. - Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods). - Expand test coverage: - Verify promise reference equality. - Ensure different arguments create distinct cache entries. - Test behavior with aborted signals. - Remove resolved TODO in entry.client.tsx
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
ffb0b04 to
b2fd318
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (2)
106-124: Consider verifying promise instance equality.The test correctly verifies caching via mock call count. For additional rigor, you could verify that both calls return the same promise instance:
const promise1 = dapper.getCommunities(); const promise2 = dapper.getCommunities(); expect(promise1).toBe(promise2);This would strengthen the guarantee that promise deduplication is working at the reference level, not just the result level.
157-157: Optional: Use a more specific type instead ofany.The mock could use a proper type for better type safety:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - .mockResolvedValue({ identifier: "1", name: "Test Community" } as any); + .mockResolvedValue({ identifier: "1", name: "Test Community" } as { identifier: string; name: string });Given this is a test mock, the current approach is acceptable if you want to keep it simple.
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
56-58: Reconsider thefunc.namevalidation.The check validates
func.name(the function's intrinsic name property) even though thefuncNameparameter is what's actually used as the cache key (line 66). This creates confusion:
- The error message says "Must be named functions" but funcName is explicitly provided
- Legitimate use cases are blocked where developers intentionally provide a custom
funcNamefor anonymous functionsIf the check is meant as a safety guardrail against accidental misuse, consider:
- Removing it entirely since
funcNameis explicit- Or clarifying the error message to explain the actual concern
- Or checking that
funcNameis not empty/generic insteadBased on learnings (anttimaki's past comment), this validation was questioned for this exact reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cyberstorm-remix/app/entry.client.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts (1)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
deduplicatePromiseForRequest(50-95)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
deduplicatePromiseForRequest(50-95)
🪛 GitHub Check: ESLint
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
[failure] 157-157: Disallow the any type
Unexpected any. Specify a different type.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (5)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
37-104: Solid test coverage for initialization and proxy lifecycle.The tests properly verify factory resolution, proxy caching per request, and isolation between different requests.
apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts (1)
1-103: Excellent test coverage addressing previous feedback.The tests comprehensively cover:
- Per-request caching and promise instance equality
- Argument differentiation
- Rejection handling and cache removal
- Request isolation
- Abort signal behavior
All scenarios from previous review comments have been addressed.
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
11-35: LGTM on abort signal handling.The implementation correctly handles pre-aborted signals by returning an empty array that's not persisted in the WeakMap (lines 12-14), effectively preventing caching for aborted requests. The tests in requestCache.test.ts lines 143-161 verify this behavior.
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (2)
28-55: Clean initialization and singleton management.The browser-check guards (lines 29-30, 46-48) appropriately prevent SSR usage, and the lazy initialization pattern with optional factory override provides good flexibility for testing.
64-104: Proxy implementation correctly integrates with request caching.The handler properly:
- Returns the base client Dapper when no request is provided
- Caches proxies per request using WeakMap
- Intercepts only "get" methods (line 83) to avoid caching mutations
- Delegates to
deduplicatePromiseForRequestfor per-request promise deduplicationThe comment on line 82 documents the "get" prefix assumption, addressing the concern from past reviews.
- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose. - Rename `getRequestScopedDapper` to `getDapperForRequest`. - Improve cache management: - Immediately handle aborted requests to prevent race conditions. - Remove rejected promises from cache to allow retries. - Use `WeakMap.delete` for safer cache clearing instead of array splicing. - Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods). - Expand test coverage: - Verify promise reference equality. - Ensure different arguments create distinct cache entries. - Test behavior with aborted signals. - Remove- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose. - Rename `getRequestScopedDapper` to `getDapperForRequest`. - Improve cache management: - Immediately handle aborted requests to prevent race conditions. - Remove rejected promises from cache to allow retries. - Use `WeakMap.delete` for safer cache clearing instead of array splicing. - Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods). - Expand test coverage: - Verify promise reference equality. - Ensure different arguments create distinct cache entries. - Test behavior with aborted signals. - Remove resolved TODO in entry.client.tsx
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
b2fd318 to
9fe7186
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (func.name === "default" || func.name === "anonymous") { | ||
| throw new Error("Must be named functions to support caching."); |
There was a problem hiding this comment.
The validation logic and error message are inconsistent. The code checks if func.name is "default" or "anonymous", but the funcName parameter is what's actually used as the cache key (not func.name). This means:
- An arrow function (with
func.name === "") would pass this check even though it's unnamed - The error message "Must be named functions to support caching" is misleading since you can pass any function as long as you provide a valid
funcNameparameter
Consider either: (a) removing this check entirely since funcName is explicitly provided, or (b) checking if the provided funcName matches these restricted names, with an error message like "funcName cannot be 'default' or 'anonymous' as they may cause cache collisions".
| if (func.name === "default" || func.name === "anonymous") { | |
| throw new Error("Must be named functions to support caching."); | |
| if (funcName === "default" || funcName === "anonymous") { | |
| throw new Error("funcName cannot be 'default' or 'anonymous' as they may cause cache collisions."); |
There was a problem hiding this comment.
I think the my earlier review comment was wrong and the bot is right. As func.name is not part of the cache key, it being anonymous doesn't affect caching. This check should probably be removed. Or alternatively the cache key should include the actual function name (as the bot suggest in a comment below), but I think that defeats the point of the funcName parameter. Either way, don't trust my comments this time, think this through yourself.
There was a problem hiding this comment.
Thought this through myself when I changed it and decided to not make function grab the given functions name. This makes using the reqeustCache more flexible
There was a problem hiding this comment.
Okay, but if we choose to do the caching based on funcName and inputs, then this whole check makes no sense and should be removed.
In usePromise label like funcName was not used, and in there passing two anonymous functions would have had the same cache key even if the functions were completely unrelated. If we go with the more flexible route of using a label here, we can also cache anonymous functions since the caching doesn't depend on the function name. So this check just makes our cache implementation less flexible for no reason.
There was a problem hiding this comment.
And asking this here as posting it in the review's main comment box seems to go unnoticed. Sorry if you answered this at some point already and I missed it.
Did you ever check that the implementation doesn't use an old value from cache when Remix does its revalidation thing?
There was a problem hiding this comment.
Fair, removed the check. And yep, whenever shouldRevalidate of RR returns true during a triggered revalidation, a new request is made for the Route.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
50-59: ValidatefuncNameinstead offunc.namefor caching safetyThe cache key is
funcName, but the guard checksfunc.name. This allows anonymous/default-export functions as long as a “good”funcNameis passed, and also doesn’t prevent empty or auto-generatedfuncNamevalues. It also doesn’t match the intent described in the JSDoc and prior tests.I’d switch the guard to validate
funcNamedirectly:-export function deduplicatePromiseForRequest<Args extends unknown[], Result>( +export function deduplicatePromiseForRequest<Args extends unknown[], Result>( funcName: string, func: (...inputs: Args) => Promise<Result>, inputs: Args, request: Request ): Promise<Result> { - if (func.name === "default" || func.name === "anonymous") { - throw new Error("Must be named functions to support caching."); - } + if (funcName === "" || funcName === "default" || funcName === "anonymous") { + throw new Error( + "funcName must be a stable, non-empty identifier for caching." + ); + }This aligns the validation with the actual cache key and with the tests’ expectations.
🧹 Nitpick comments (3)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
73-88: Avoid capturingrequestin rejection handler to reduce retentionThe rejection cleanup currently closes over
requestand hits the WeakMap again. That’s not necessary and keeps theRequeststrongly reachable via thePromise→catchclosure chain, which can lengthen the lifetime of per-request state.You can instead capture the concrete cache entry and manipulate the already-known
cachearray:- const promise = func(...inputs) as Promise<Result>; - - // Remove from cache if it rejects to allow retrying - promise.catch(() => { - const currentCache = requestScopedCaches.get(request); - if (currentCache) { - const index = currentCache.findIndex( - (entry) => entry.promise === promise - ); - if (index !== -1) { - currentCache.splice(index, 1); - } - } - }); - - cache.push({ - funcName, - inputs, - promise, - }); + const promise = func(...inputs) as Promise<Result>; + + const cacheEntry: CacheEntry = { + funcName, + inputs, + promise, + }; + + cache.push(cacheEntry); + + // Remove from cache if it rejects to allow retrying + promise.catch(() => { + const index = cache.indexOf(cacheEntry); + if (index !== -1) { + cache.splice(index, 1); + } + });This keeps the behavior the same while avoiding extra references back to the
Request.apps/cyberstorm-remix/app/entry.client.tsx (1)
7-10: Session tools + Dapper init ordering looks good; consider explicit failure behaviorPassing the pre-fetched
publicEnvVariablesintogetSessionToolsand running this inside atryafterSentry.initfixes the earlier ordering and duplication issues. The only open design question is what should happen if env/session setup fails: right now you log to Sentry and continue hydrating without Dapper, which may surface as harder-to-diagnose downstream errors.If you want a stricter behavior, you could either:
- Re-throw after
Sentry.captureException(error)to fail fast, or- Gate later Dapper usage on having successfully initialized here and show a user-facing error when not.
Not mandatory, but worth confirming the intended UX.
Also applies to: 14-21, 77-85
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
106-108: Consider clearing request-scoped proxies in the test reset helper
resetDapperSingletonForTestcurrently only resetscurrentConfigFactory. For isolation it’s probably enough, since tests don’t reuseRequestinstances andrequestScopedProxiesis aWeakMap.If you ever add tests that reuse the same
Requestacross cases, you might also want to clear the proxies:-export function resetDapperSingletonForTest() { - currentConfigFactory = undefined; -} +export function resetDapperSingletonForTest() { + currentConfigFactory = undefined; + requestScopedProxies.clear?.(); +}(Or reassign a new
WeakMap.) Optional, but can make test behavior more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cyberstorm-remix/cyberstorm/utils/tests/requestCache.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (2)
getPublicEnvVariables(19-48)getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
initializeClientDapper(28-43)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-66)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
deduplicatePromiseForRequest(50-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (10)
apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
11-35: Abort-aware cache initialization looks solidClearing the WeakMap entry for already-aborted requests and registering a one-shot abort listener on first cache creation gives the cache a clear lifecycle per
Requestand removes the earlier race where pre-aborted signals weren’t handled. No issues here.apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (5)
12-20: Test setup/mocking keeps Dapper state nicely isolatedMocking
getSessionToolsand resettingwindow.DapperplusresetDapperSingletonForTest()inbeforeEachgives each test a clean slate and avoids leaking config across cases.afterEach’svi.restoreAllMocks()rounds this out. No changes needed here.Also applies to: 22-36
37-56: initializeClientDapper tests exercise both factory pathsThese cases validate:
- Default path pulls config from
getSessionTools, and- Custom factory path wires through to
DapperTs.config().That covers the main configuration branches of
initializeClientDapperand should catch regressions in how the factory is resolved.
73-104: Per-request proxy tests cover identity and isolationThe
getDapperForRequesttests that:
- No-request calls return the global client,
- A
Requestyields a distinct proxy, and- The same
Requestreuses the proxy while differentRequestinstances get separate proxies.That matches the intended WeakMap-per-request behavior; looks good.
106-146: Proxy interception behavior is well-coveredThe “intercepts 'get' methods and caches promises” and “does not intercept non-'get' methods” tests clearly pin the contract:
get*methods are deduped,- non-
get*methods likepostTeamCreatebypass caching.Given the separate integration test with
deduplicatePromiseForRequest, this is sufficient coverage for the proxy behavior.
148-179: Nice integration test withdeduplicatePromiseForRequestThis test explicitly proves that:
- A proxied
getCommunity("1")call populates the request cache, and- A manual
deduplicatePromiseForRequest("getCommunity", ...)with matching args hits that cache instead of invoking its own function.That’s a strong end-to-end guarantee that the proxy and the generic cache share the same keying scheme.
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (4)
11-19: Lazy config factory resolution aligns with session tools
resolveConfigFactorydefers togetSessionTools()only once and then caches aConfigFactorythat simply callstools.getConfig(). That keeps the Dapper wiring decoupled from env loading while still centralizing session/context logic inpublicEnvVariables. Looks good.
28-43: initializeClientDapper correctly handles custom factories and browser guardThe browser check at the top prevents accidental SSR usage, and the branching between:
updateDapperConfig(factory)when a factory is provided, andresolveConfigFactory()when not,ensures
currentConfigFactoryis always set beforewindow.Dapperis constructed. The secondresolveConfigFactory()call reuses the cached factory and won’t hitgetSessionTools()again, which is what you want.
64-103: Request-scoped Proxy behavior matches intended caching contractThe proxy handler:
- Returns the raw (bound) method for non-functions or non-
get*methods, avoiding accidental caching of mutating calls.- Wraps
get*methods indeduplicatePromiseForRequest, using the property name as the cache key and the original args array.Given Dapper’s
get*methods return promises, this is a good fit and lines up with the tests that assert per-request deduping and separation across differentRequestinstances.
45-55: I'll help you verify thatgetClientDapperis only called in browser contexts. Let me search the codebase for all usages of this function and related server-side alternatives.
<function_calls>
#!/bin/bashFind all usages of getClientDapper and getDapperForRequest
echo "=== Searching for getClientDapper usages ==="
rg -nP "getClientDapper\s*(" --type ts --type tsx -A 2 -B 2echo ""
echo "=== Searching for getDapperForRequest usages ==="
rg -nP "getDapperForRequest\s*(" --type ts --type tsx -A 2 -B 2
</function_calls>
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
9fe7186 to
fa0754a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anttimaki
left a comment
anttimaki
left a comment
There was a problem hiding this comment.
Did you ever check this one? I remember it being discussed but don't remember if I ever got a confirmation for it.
Important: check that the implementation doesn't use an old value from cache when Remix does its revalidation thing.
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts
Outdated
Show resolved
Hide resolved
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Show resolved
Hide resolved
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Fixed
Show fixed
Hide fixed
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts
Show resolved
Hide resolved
| if (func.name === "default" || func.name === "anonymous") { | ||
| throw new Error("Must be named functions to support caching."); |
There was a problem hiding this comment.
I think the my earlier review comment was wrong and the bot is right. As func.name is not part of the cache key, it being anonymous doesn't affect caching. This check should probably be removed. Or alternatively the cache key should include the actual function name (as the bot suggest in a comment below), but I think that defeats the point of the funcName parameter. Either way, don't trust my comments this time, think this through yourself.
Oksamies
force-pushed
the
11-20-test_implemenation_of_cross-clientloader-request_dedupe
branch
from
fa0754a to
0642cbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/entry.client.tsx (1)
77-85: Tighten failure handling for Dapper init and double‑checkgetConfigusageThis block now correctly initializes Dapper only after Sentry, so errors are captured, but failures are silently swallowed and the app hydrates with Dapper potentially uninitialized. If Dapper is required for core flows, consider either re‑throwing after
Sentry.captureException(error)or guarding downstream usage behind an explicit “Dapper not available” path so you fail fast instead of in obscure places later.Also, please verify that
sessionTools.getConfig(publicEnvVariables.VITE_API_URL)matches the declared signature ofgetConfig. It appears to be used without arguments elsewhere, so passing an API URL only here could be inconsistent or misleading if the parameter is not actually part of the API.
🧹 Nitpick comments (1)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
38-196: Comprehensive coverage of singleton config and per‑request proxy cachingThe tests under
initializeClientDapper,getClientDapper, and especiallygetDapperForRequestdo a good job verifying:
- Config factory handling (including updates without replacing the instance and resolution via mocked
getSessionTools).- That request-scoped proxies are reused per
Requestand distinct across differentRequestobjects.- That only
get*methods are deduped while mutation methods likepostTeamCreatebypass the cache.- That cache entries are shared between the proxy and direct
deduplicatePromiseForRequestcalls (thegetCommunity+ manualFunc test nicely exercises the core dedupe contract).If you ever want to extend coverage, an extra test for the “non-browser” guard (e.g.,
typeof window === "undefined"ingetClientDapper) would be a reasonable future addition, but what’s here already gives strong confidence in the new behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cyberstorm-remix/app/entry.client.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts
- apps/cyberstorm-remix/cyberstorm/utils/tests/requestCache.test.ts
- apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts
- apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/entry.client.tsx (2)
apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (2)
getPublicEnvVariables(19-48)getSessionTools(50-67)apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (1)
initializeClientDapper(28-41)
apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (3)
apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts (3)
initializeClientDapper(28-41)getClientDapper(43-53)getDapperForRequest(62-102)packages/dapper-ts/src/index.ts (1)
DapperTs(41-102)apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts (1)
deduplicatePromiseForRequest(56-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (2)
apps/cyberstorm-remix/app/entry.client.tsx (1)
7-21: Dapper/env wiring after Sentry init looks solidThe added imports plus inclusion of
VITE_COOKIE_DOMAINinpublicEnvVariablescleanly hook Dapper initialization into the existing Sentry-aware client bootstrap. The flow of: load env → init Sentry → later derive session tools/Dapper config is coherent and matches the new singleton design; no issues from my side here.apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts (1)
1-37: Good isolation of Dapper singleton and session tools in test setupMocking
getSessionTools, explicitly deletingwindow.Dapper, and callingresetDapperSingletonForTest()inbeforeEachgive you clean state for every test, andclearAllMocks/restoreAllMockskeep spies localized. This is a solid foundation for reliably testing the singleton behavior.
| if (func.name === "default" || func.name === "anonymous") { | ||
| throw new Error("Must be named functions to support caching."); |
There was a problem hiding this comment.
Okay, but if we choose to do the caching based on funcName and inputs, then this whole check makes no sense and should be removed.
In usePromise label like funcName was not used, and in there passing two anonymous functions would have had the same cache key even if the functions were completely unrelated. If we go with the more flexible route of using a label here, we can also cache anonymous functions since the caching doesn't depend on the function name. So this check just makes our cache implementation less flexible for no reason.
| if (func.name === "default" || func.name === "anonymous") { | ||
| throw new Error("Must be named functions to support caching."); |
There was a problem hiding this comment.
And asking this here as posting it in the review's main comment box seems to go unnoticed. Sorry if you answered this at some point already and I missed it.
Did you ever check that the implementation doesn't use an old value from cache when Remix does its revalidation thing?
No description provided.