Implement etag handling #296
Conversation
WalkthroughThe changes introduce ETag-based HTTP caching for GitHub API requests in the codebase. A new utility module manages ETag storage and conditional requests, and existing API calls are refactored to use this mechanism. Comprehensive tests are added to verify ETag handling, header usage, and cache behavior across repeated requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant API (fetchWorkflowRunIds/fetchWorkflowRunJobSteps)
participant withEtag
participant Octokit
Caller->>API: Call fetchWorkflowRunIds/JobSteps(params)
API->>withEtag: withEtag(endpoint, params, requester)
withEtag->>withEtag: Check ETag cache for (endpoint, params)
alt ETag exists
withEtag->>Octokit: Make request with If-None-Match: ETag
else No ETag
withEtag->>Octokit: Make request without If-None-Match
end
Octokit-->>withEtag: Respond (may be 304 Not Modified or new data with ETag)
alt 304 Not Modified
withEtag->>withEtag: Return cached response
else New or changed data
withEtag->>withEtag: Update cache with new ETag and response
withEtag-->>API: Return new response
end
withEtag-->>API: Return response
API-->>Caller: Return data
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
carlobeltrame
force-pushed
the
conditional-api-requests
branch
from
fc3223c to
7c798e9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
+ Coverage 96.91% 97.04% +0.13%
==========================================
Files 6 7 +1
Lines 583 643 +60
Branches 112 124 +12
==========================================
+ Hits 565 624 +59
- Misses 17 18 +1
Partials 1 1 ☔ 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
🧹 Nitpick comments (2)
src/etags.ts (2)
48-50: Consider cache key generation edge cases.Using
JSON.stringify({ endpoint, params })for cache keys could lead to issues ifparamscontains functions, circular references, or objects with non-deterministic property ordering.Consider a more stable cache key generation strategy:
-function getEtag(endpoint: string, params: object): EtagStoreEntry | undefined { - return etagStore.get(JSON.stringify({ endpoint, params })); -} +function getEtag(endpoint: string, params: object): EtagStoreEntry | undefined { + // Create a stable key by sorting object properties + const stableKey = JSON.stringify({ endpoint, params }, Object.keys({ endpoint, params }).sort()); + return etagStore.get(stableKey); +}
12-12: Consider implementing cache eviction strategy.The global
etagStoreMap could grow indefinitely as it never removes entries. For a long-running application, this could lead to memory issues.Consider adding cache size limits or TTL-based eviction:
+const MAX_CACHE_SIZE = 1000; + const etagStore = new Map<string, EtagStoreEntry>();Then implement LRU eviction in the
rememberEtagfunction or consider using a library likelru-cache.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.mjsis excluded by!**/dist/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)src/api.spec.ts(23 hunks)src/api.ts(3 hunks)src/etags.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/api.ts (1)
src/etags.ts (1)
withEtag(14-41)
🔇 Additional comments (10)
package.json (1)
37-37: LGTM - Appropriate dependency addition.The
@octokit/typesdependency correctly supports the TypeScript types used in the new ETag handling functionality.src/api.ts (3)
5-5: LGTM - Clean ETag integration.The import and usage of
withEtagis well-implemented and maintains backward compatibility.
173-191: ETag caching implementation looks good.The
withEtagwrapper cleanly integrates ETag functionality while preserving the original API logic and error handling.
231-243: Verify caching assumption for fetchWorkflowRunJobSteps.The ETag caching assumes that the same endpoint with same parameters always returns the same data. As mentioned in the PR objectives, there's uncertainty about whether this assumption holds for
fetchWorkflowRunJobSteps.Consider whether additional request context (like timestamp or run status) should be included in the cache key for this endpoint, since workflow run job steps could potentially change even with the same
run_idparameter.#!/bin/bash # Search for any documentation or comments about whether workflow run job steps can change rg -A 10 -B 5 "listJobsForWorkflowRun|job.*step.*change|workflow.*run.*mutable"src/api.spec.ts (6)
23-23: Good addition for test cleanup.The
clearEtagsimport enables proper test isolation by resetting ETag cache between tests.
71-73: Excellent test isolation practice.The
afterEachhook properly clears the ETag cache between tests, preventing test interference.
33-33: Good mock interface extension.Adding the
headersproperty toMockResponseproperly supports ETag testing while maintaining consistency with the existing mock structure.
582-636: Comprehensive ETag caching test.This test thoroughly verifies the ETag functionality:
- First request gets fresh data with ETag
- Second request with same parameters sends
If-None-Matchheader- 304 response handling works correctly
- Proper logging verification
638-692: Good test for ETag isolation.This test correctly verifies that ETags are scoped to request parameters, ensuring different parameters don't incorrectly share cached ETags.
810-866: Excellent test coverage for fetchWorkflowRunJobSteps.Both tests (ETag reuse and parameter isolation) provide comprehensive coverage of the ETag functionality for the job steps endpoint, mirroring the workflow runs tests appropriately.
Also applies to: 868-924
|
Awesome, will review when I can 🫡 |
carlobeltrame
force-pushed
the
conditional-api-requests
branch
from
7c798e9 to
b8c1da8
Compare
carlobeltrame
force-pushed
the
conditional-api-requests
branch
from
b8c1da8 to
8401a12
Compare
src/etags.ts
Outdated
| paramsWithEtag.headers = { | ||
| "If-None-Match": etag, | ||
| ...(params.headers ?? {}), | ||
| } as RequestHeaders; |
There was a problem hiding this comment.
| } as RequestHeaders; | |
| } satisfies RequestHeaders; |
There was a problem hiding this comment.
Wow I didn't know that one. Fixed in e14223d
There was a problem hiding this comment.
Very handy! Another great use case when writing tests and such is doing something like {} satisfies Partial<Foo> as <Foo> which lets you correctly type the keys you want to mock and then satisfy the interface required with the final cast.
|
I think overall this looks really good, I do wonder about usages when there are long-running things, but can cross that bridge if it becomes an issue |
3 participants
This is a first attempt to fix #295.
@Codex- please let me know whether this implementation matches how you imagined the solution. I will manually test it with our setup. If you want, I could also write unit tests for the etag file, but the logic should already be tested in the added api test.
Summary by CodeRabbit
New Features
Tests
Chores