-
Notifications
You must be signed in to change notification settings - Fork 48
refactor(cache): separate web-ui-settings cache from payload cache #632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5b34bd0 to
4288324
Compare
Update workspaces/cache/docs/AppCache.md Co-authored-by: PierreDemailly <[email protected]> Update workspaces/cache/docs/AppCache.md Co-authored-by: PierreDemailly <[email protected]> Update workspaces/cache/docs/AppCache.md Co-authored-by: PierreDemailly <[email protected]> Update workspaces/cache/docs/AppCache.md Co-authored-by: PierreDemailly <[email protected]>
fc9e517 to
39b37be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the cache system to separate web UI settings from payload caching by introducing a generic FilePersistanceProvider class. The previous monolithic AppCache class handled both configuration and payload caching, which has now been split into specialized components.
Key changes:
- Introduced
FilePersistanceProvider<T>as a generic file-based caching abstraction using cacache - Moved payload-related caching logic to a dedicated
AppCache.tsfile - Implemented web UI settings caching using
FilePersistanceProviderin the server's config module
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/cache/src/FilePersistanceProvider.ts | New generic file persistence provider with get/set/remove methods for cached data |
| workspaces/cache/src/AppCache.ts | Extracted AppCache class from index.ts, focusing on payload management |
| workspaces/cache/src/index.ts | Refactored to export both AppCache and FilePersistanceProvider via barrel exports |
| workspaces/server/src/config.ts | Refactored to use FilePersistanceProvider for web UI settings instead of AppCache |
| workspaces/server/src/cache.ts | Removed AppConfig export, simplified to only export cache instance |
| workspaces/server/src/endpoints/config.ts | Updated to use WebUISettings type instead of AppConfig |
| workspaces/server/src/index.ts | Added config module export for external access |
| workspaces/server/src/websocket/*.ts | Updated imports to reference AppCache from dist directory |
| workspaces/cache/test/FilePersistanceProvider.test.ts | New comprehensive test suite for FilePersistanceProvider |
| workspaces/server/test/config.test.ts | Updated to use FilePersistanceProvider instead of cacache directly |
| workspaces/server/test/httpServer.test.ts | Updated to use config module methods instead of direct cacache access |
| workspaces/cache/test/index.test.ts | Removed config-related test now covered in server tests |
| workspaces/cache/package.json | Updated test script to run all test files |
| workspaces/cache/docs/*.md | Added documentation for AppCache and FilePersistanceProvider |
| workspaces/cache/README.md | Updated to reference new documentation structure |
| src/commands/cache.js | Added config.setDefault() call when clearing cache |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { match } from "ts-pattern"; | ||
| import type { Logger } from "pino"; | ||
| import type { AppCache } from "@nodesecure/cache"; | ||
| import type { AppCache } from "@nodesecure/cache/dist/AppCache.ts"; |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path references the compiled output directory (/dist/AppCache.ts) instead of using the package's main export. Consider importing from "@nodesecure/cache" directly, which would use the barrel export defined in the package's index.ts. This would make the imports more maintainable and follow standard package consumption patterns.
| import type { AppCache } from "@nodesecure/cache/dist/AppCache.ts"; | |
| import type { AppCache } from "@nodesecure/cache"; |
| // Import Third-party Dependencies | ||
| import * as scanner from "@nodesecure/scanner"; | ||
| import type { PayloadsList, AppCache } from "@nodesecure/cache"; | ||
| import type { PayloadsList, AppCache } from "@nodesecure/cache/dist/AppCache.ts"; |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path references the compiled output directory (/dist/AppCache.ts) instead of using the package's main export. Consider importing from "@nodesecure/cache" directly, which would use the barrel export defined in the package's index.ts. This would make the imports more maintainable and follow standard package consumption patterns.
| import type { PayloadsList, AppCache } from "@nodesecure/cache/dist/AppCache.ts"; | |
| import type { PayloadsList, AppCache } from "@nodesecure/cache"; |
| @@ -1,5 +1,5 @@ | |||
| // Import Third-party Dependencies | |||
| import type { PayloadsList } from "@nodesecure/cache"; | |||
| import type { PayloadsList } from "@nodesecure/cache/dist/AppCache.ts"; | |||
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path references the compiled output directory (/dist/AppCache.ts) instead of using the package's main export. Consider importing from "@nodesecure/cache" directly, which would use the barrel export defined in the package's index.ts. This would make the imports more maintainable and follow standard package consumption patterns.
| import type { PayloadsList } from "@nodesecure/cache/dist/AppCache.ts"; | |
| import type { PayloadsList } from "@nodesecure/cache"; |
| } | ||
| } | ||
| export * from "./AppCache.ts"; | ||
| export * from "./FilePersistanceProvider.ts"; |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "Persistance" should be "Persistence". The filename should be "FilePersistenceProvider.ts" and the export should reference the corrected filename.
| export * from "./FilePersistanceProvider.ts"; | |
| export * from "./FilePersistenceProvider.ts"; |
| // Import Internal Dependencies | ||
| import { FilePersistanceProvider } from "../src/FilePersistanceProvider.ts"; | ||
|
|
||
| describe("FilePersistanceProvider", () => { |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "Persistance" should be "Persistence". Update test suite name to use the corrected class name.
| import { warnings, type WarningName } from "@nodesecure/js-x-ray"; | ||
| import type { Flag } from "@nodesecure/flags"; | ||
| import { | ||
| FilePersistanceProvider |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "Persistance" should be "Persistence". Update the import to use the corrected class name.
| // Import Third-party Dependencies | ||
| import type { WebSocket } from "ws"; | ||
| import type { AppCache, PayloadsList } from "@nodesecure/cache"; | ||
| import type { AppCache, PayloadsList } from "@nodesecure/cache/dist/AppCache.ts"; |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path references the compiled output directory (/dist/AppCache.ts) instead of using the package's main export. Consider importing from "@nodesecure/cache" directly, which would use the barrel export defined in the package's index.ts. This would make the imports more maintainable and follow standard package consumption patterns.
| import type { AppCache, PayloadsList } from "@nodesecure/cache/dist/AppCache.ts"; | |
| import type { AppCache, PayloadsList } from "@nodesecure/cache"; |
| ## Interfaces | ||
|
|
||
| ```ts | ||
| interface BasePersistanceProvider<T> { |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "Persistance" should be "Persistence". Update the interface name in the documentation.
| interface BasePersistanceProvider<T> { | |
| interface BasePersistenceProvider<T> { |
| } | ||
| ``` | ||
| - [AppCache](./docs/AppCache.md) | ||
| - [FilePersistanceProvider](./docs/FilePersistanceProvider.md) |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "Persistance" should be "Persistence". Update the documentation link to use the corrected filename.
| - [FilePersistanceProvider](./docs/FilePersistanceProvider.md) | |
| - [FilePersistenceProvider](./docs/FilePersistenceProvider.md) |
| } | ||
| catch (err: any) { | ||
| logger.error(`[config|get](error: ${err.message})`); | ||
| export function getProvider(): FilePersistanceProvider<WebUISettings> { |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "Persistance" should be "Persistence". Update the return type to use the corrected class name.
No description provided.