refactor: add control-plane cache store abstraction#564
refactor: add control-plane cache store abstraction#564ColeMurray wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR introduces a small CacheStore abstraction in the control-plane and rewires GitHub installation-token caching and provider construction to depend on it instead of KVNamespace directly. The refactor is behavior-preserving in the reviewed paths, and the cache semantics, error handling, and token TTL logic remain intact.
- PR Title and number:
refactor: add control-plane cache store abstraction(#564) - Author:
@ColeMurray - Files changed count, additions/deletions:
7 files, +51 / -33 - Verification:
npm test -w @open-inspect/control-plane -- src/auth/github-app.test.tspassed (9 tests), andnpm run typecheck -w @open-inspect/control-planepassed.
Critical Issues
None.
Suggestions
- [Testing]
packages/control-plane/src/cache/cache-store.ts:1- This abstraction is now part of the control-plane wiring, but the added test coverage only exercisesgetCachedInstallationTokenwith a fake store. A small focused test around provider wiring orcreateKvCacheStore()would help catch future drift if the abstraction expands beyond installation-token caching.
Nitpicks
None.
Positive Feedback
- The refactor keeps the cache failure behavior unchanged, which is important on auth-critical paths.
- The call-site updates are minimal and consistent, which keeps the new abstraction easy to reason about.
- The renamed local variables in
getCachedInstallationToken()improve clarity by separating persistent cache reads from the in-memory cache.
Questions
None.
Verdict
Approve: Ready to merge, no blocking issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/control-plane/src/session/durable-object.ts (1)
583-589: Optional: hoist thecacheStorewrapper out of the per-call closure.A fresh
createKvCacheStore(this.env.REPOS_CACHE)is allocated every timegetCloneToken()is invoked. The wrapper is stateless (the in-memory token cache and in-flight map live ingithub-app.ts), so the duplication is harmless, but constructing it once and capturing it in the closure is cleaner and matchescreateSourceControlProviderabove.♻️ Hoist wrapper
+ const cacheStore = createKvCacheStore(this.env.REPOS_CACHE); const getCloneToken: () => Promise<string | null> = scmProvider === "gitlab" ? () => Promise.resolve(this.env.GITLAB_ACCESS_TOKEN ?? null) : appConfig - ? () => - getCachedInstallationToken(appConfig, { - cacheStore: createKvCacheStore(this.env.REPOS_CACHE), - }) + ? () => getCachedInstallationToken(appConfig, { cacheStore }) : () => Promise.resolve(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/session/durable-object.ts` around lines 583 - 589, The per-call closure currently constructs a new cache wrapper each time (createKvCacheStore(this.env.REPOS_CACHE)) when returning the getCachedInstallationToken path; hoist that wrapper into a single const (e.g. const reposCacheStore = createKvCacheStore(this.env.REPOS_CACHE)) captured by the outer scope of getCloneToken so the closure reuses reposCacheStore instead of allocating a fresh one on every invocation; update the branch that calls getCachedInstallationToken(appConfig, { cacheStore: ... }) to pass reposCacheStore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/control-plane/src/session/durable-object.ts`:
- Around line 583-589: The per-call closure currently constructs a new cache
wrapper each time (createKvCacheStore(this.env.REPOS_CACHE)) when returning the
getCachedInstallationToken path; hoist that wrapper into a single const (e.g.
const reposCacheStore = createKvCacheStore(this.env.REPOS_CACHE)) captured by
the outer scope of getCloneToken so the closure reuses reposCacheStore instead
of allocating a fresh one on every invocation; update the branch that calls
getCachedInstallationToken(appConfig, { cacheStore: ... }) to pass
reposCacheStore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97097ea7-7f73-48d2-af72-74c8bb18d3a1
📒 Files selected for processing (7)
packages/control-plane/src/auth/github-app.test.tspackages/control-plane/src/auth/github-app.tspackages/control-plane/src/cache/cache-store.tspackages/control-plane/src/routes/shared.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/source-control/providers/github-provider.tspackages/control-plane/src/source-control/providers/types.ts
1 participant
Summary
CacheStoreinterface with a KV-backed implementation.CacheStoreinstead ofKVNamespace.Verification
npm test -w @open-inspect/control-plane -- src/auth/github-app.test.tsnpm run typecheck -w @open-inspect/control-planeCreated with Open-Inspect
Summary by CodeRabbit