feat: implement dynamic sitemap generation and remove KV dependencies - #10
3scava1i3r wants to merge 12 commits into
Conversation
- Add dynamic sitemap endpoints (/sitemap.xml, /sitemap.json) - Add plugin-map endpoints (/plugin-map.xml, /plugin-map.json) - Remove KV namespace dependencies and analytics tracking - Implement in-memory caching with 6hr sitemap, 2hr plugin-map cache - Add comprehensive service discovery from GitHub repositories - Remove write-tracker.ts and analytics directory (KV-free architecture) - Add timeout protection (8s) for sitemap generation - Include rich metadata: service types, deployment status, GitHub links
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a core discovery module to probe Deno and Pages deployments, fetch plugin variant manifests with timeouts, and batch-resolve services and plugins. Introduces plugin-map-discovery and site-map-discovery with in-memory TTL caching and forced-refresh. Adds plugin-map-generator and sitemap-generator to produce XML/JSON maps and response helpers. New utilities: memory-cache, static-config, build-pages-url, get-known-services, get-known-plugins, and updated utils re-exports. Worker routes for /sitemap.xml, /sitemap.json, /plugin-map.xml, /plugin-map.json; Env gains GITHUB_TOKEN and REFRESH_SECRET. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
src/utils/get-known-plugins.ts (1)
73-75: Static fallback result is never cached.When the GitHub API fails (or no token is provided), the static list is returned but not written to the cache. This means subsequent calls will repeat the failed API attempt each time instead of serving the fallback from cache. Consider caching the static result too, perhaps with a shorter TTL.
src/worker.ts (1)
264-281:anyreturn types and parameter types lose type safety.
safeSitemapGenerationreturnsPromise<any[]>, and all handlers userequest?: any. Use the proper types (SitemapEntry[],PluginMapEntry[],Request) to get compile-time safety.Proposed fix for safeSitemapGeneration (apply same pattern to safePluginMapGeneration)
+import type { SitemapEntry } from './sitemap-generator' +import type { PluginMapEntry } from './plugin-map-generator' async function safeSitemapGeneration( forceRefresh: boolean, githubToken: string, - request?: any - ): Promise<any[]> { + request?: Request + ): Promise<SitemapEntry[]> { ... - const entries = await Promise.race([sitemapPromise, timeoutPromise]) as any[] + const entries = await Promise.race([sitemapPromise, timeoutPromise]) as SitemapEntry[]Also applies to: 324-340
src/utils/static-config.ts (1)
43-83: Repetitive triple-entry pattern for plugins could be generated.Each plugin has 3 manually maintained entries (base,
-main,-development). A helper generating variants from a base list would halve the line count and eliminate copy-paste drift.Example
const BASE_PLUGINS = [ 'daemon-xp', 'text-conversation-rewards', 'daemon-task-matcher', // ... ] export const STATIC_PLUGINS: PluginConfig[] = BASE_PLUGINS.flatMap(name => [ { name, github: `ubiquity-os-marketplace/${name}` }, { name: `${name}-main`, github: `ubiquity-os-marketplace/${name}/tree/main` }, { name: `${name}-development`, github: `ubiquity-os-marketplace/${name}/tree/development` }, ])src/plugin-map-discovery.ts (1)
46-55:forceRefreshandrequestparams are unused — function namegetCachedPluginMapEntriesis misleading.There's no caching logic here; it always regenerates. The params
forceRefreshandrequestare never read. Either implement caching or rename togetPluginMapEntriesand drop the unused params.src/site-map-discovery.ts (1)
48-57:getCachedSitemapEntries— dead parameters and misleading name.
forceRefreshandrequestare unused. The function name says "cached" but performs no caching. Either wire up the in-memory cache frommemory-cache.ts(as the module docstring on line 3 promises) or simplify this to a plain pass-through with an honest name.src/sitemap-generator.ts (1)
144-155:generatedtimestamp is computed independently of the caller's timestamp.
generateJsonSitemapcallsnew Date().toISOString()internally, whilegenerateJsonPluginMapinplugin-map-generator.tsacceptsgenerationTimestampas a parameter. Consider accepting the timestamp as a parameter here too for consistency and testability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/worker.ts (1)
264-268:Promise<any[]>loses type safety across both safe-generation wrappers.Use
Promise<SitemapEntry[]>andPromise<PluginMapEntry[]>respectively. Also,request?: anyshould berequest?: Request.Also applies to: 324-328
src/site-map-discovery.ts (1)
38-47: ServiceType derivation logic is duplicated withplugin-map-discovery.ts(lines 27-35).Consider extracting a shared helper like
resolvePluginServiceType(variants)to keep both modules in sync.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/utils/memory-cache.ts (3)
7-8: Unbounded cache — no eviction policy.The
Mapgrows without limit. In a long-lived isolate serving many unique cache keys, memory can grow indefinitely. Consider adding a max-size cap with LRU eviction, or at minimum document the assumption that key cardinality is small.
15-28: Async wrappers over synchronousMapoperations.All functions are
asyncbut perform no async work. This adds microtask overhead per call. Fine if you anticipate swapping to an async backend later, but worth a brief inline comment documenting that intent.
86-100:getCacheStatsdoesn't evict expired entries it discovers.It iterates and finds expired keys but leaves them in the map. Opportunistic cleanup here would be cheap.
Proposed fix
for (const [key, item] of GLOBAL_CACHE) { if (now < item.expiresAt) { validKeys.push(key) + } else { + GLOBAL_CACHE.delete(key) } }src/core/discovery.ts (2)
30-50:urlparameter is a dummy — confusing API surface.
discoverServiceTypeaccepts aURLonly so it can be passed tobuildDenoUrl/buildPagesUrl, but the caller at line 136 constructs a throwaway URL with no meaningful path or query. The builders only useurl.pathnameandurl.search, which are always/and""here.Consider accepting just the subdomain and constructing the base URLs internally, or document why the full
URLis needed.
55-78:response.json()can throw on invalid JSON — unhandled inside thetry.If the response body is not valid JSON (e.g., an HTML error page returned with 200 OK),
response.json()throws, which is caught by the outercatch. This works, but the error is silently swallowed. A briefconsole.warnin the catch would help debugging.src/worker.ts (1)
63-63: Unsafe cast of header value toCacheControlValue.Any arbitrary header string is cast to
CacheControlValue. While it's only compared with=== 'refresh', theascast is misleading. A simplestring | nullwould be more honest.Proposed fix
- const cacheControl = request.headers.get('X-Cache-Control') as CacheControlValue + const cacheControl = request.headers.get('X-Cache-Control')src/sitemap-generator.ts (3)
121-128:escapeXmlis duplicated inplugin-map-generator.ts(lines 11-18).Extract to a shared utility (e.g.,
src/utils/escape-xml.ts).
57-62: Past review feedback addressed —-nonecheck moved first.The
plugin-nonedead code issue is fixed. However, note that after line 59, both remaining branches (lines 60–61) return'weekly', making thestartsWith('plugin-')check on line 60 redundant. The function could just be:if (serviceType.endsWith('-none')) return 'monthly' return 'weekly'
20-22:pluginManifesttyped asany.Use
PluginManifestfrom./typesfor type safety. It's already imported in the codebase.Proposed fix
+import type { PluginManifest } from './types' + // in SitemapEntry interface: metadata?: { - pluginManifest?: any + pluginManifest?: PluginManifest githubRepo?: string } // in createSitemapEntry signature: - pluginManifest: any, + pluginManifest: PluginManifest | undefined,src/plugin-map-generator.ts (1)
112-115: GitHub org is hardcoded toubiquity-os-marketplace.If plugins can come from other orgs (e.g.,
ubiquity), this will produce incorrect repo links. Consider deriving the org from the discovery source or making it configurable.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/core/discovery.ts (3)
55-78: Uncaught JSON parse error on malformed response body.If the server returns
200 OKwith non-JSON body (e.g., HTML error page),response.json()throws aSyntaxError. The outercatchhandles it, so this is functionally safe — but logging the error there would help debug manifest discovery issues in production.
102-111: Thin pass-through wrappers add indirection without value.
getAllServicesandgetAllPluginsjust delegate togetKnownServices/getKnownPlugins. Consider calling the utils directly indiscoverAllServices/discoverAllPluginsto reduce indirection.
116-140:discoverServiceTypereceives a synthetic URL — verify build helpers only use the host.Line 128 constructs
new URL(...)purely for discovery. IfbuildDenoUrl/buildPagesUrlappendurl.pathname + url.search, the HEAD check will hit/which is fine. But passing a full URL object when only the subdomain matters is misleading. Consider havingdiscoverServiceTypebuild the URL internally from just the subdomain.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/worker.ts (3)
15-17: Merge duplicate imports from'./types'.Two separate import statements from the same module.
Proposed fix
-import type { PluginMapEntry } from './types' -import { generateXmlPluginMap, generateJsonPluginMap, createXmlPluginMapResponse, createJsonPluginMapResponse } from './plugin-map-generator' -import type { CacheControlValue } from './types' +import type { PluginMapEntry, CacheControlValue } from './types' +import { generateXmlPluginMap, generateJsonPluginMap, createXmlPluginMapResponse, createJsonPluginMapResponse } from './plugin-map-generator'
305-318: Unusedrequestparameter.
request?: anyon line 308 is never used. Remove it to keep the signature honest.Proposed fix
async function handleSitemapXml( forceRefresh: boolean, githubToken: string, - request?: any - ): Promise<Response> { +): Promise<Response> {
277-363:safeSitemapGenerationandsafePluginMapGenerationare near-identical.Consider extracting a generic
withTimeout<T>(fn: () => Promise<T>, ms: number, label: string): Promise<T>helper to DRY this up.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/worker.ts (1)
303-316: Unusedrequest?: anyparameter.
requestis accepted but never read. Remove it to keep the signature honest.async function handleSitemapXml( forceRefresh: boolean, githubToken: string, - request?: any ): Promise<Response> {
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/worker.ts (1)
277-298: Consider extracting the timeout-race pattern into a shared helper.
safeSitemapGenerationandsafePluginMapGenerationare structurally identical — only the inner call differs. A generic wrapper eliminates the duplication:async function withTimeout<T>(fn: () => Promise<T>, label: string, ms = 8000): Promise<T> { let timer: ReturnType<typeof setTimeout> | undefined const timeout = new Promise<never>((_, reject) => { timer = setTimeout(() => reject(new Error(`${label} timeout`)), ms) }) try { return await Promise.race([fn(), timeout]) } finally { clearTimeout(timer) } }Also applies to: 338-359
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/worker.ts (2)
337-358:safePluginMapGenerationis a near-clone ofsafeSitemapGeneration.Extract a generic helper to reduce duplication:
♻️ Suggested refactor
+async function withTimeout<T>( + label: string, + task: Promise<T>, + timeoutMs = 8000, +): Promise<T> { + let timer: ReturnType<typeof setTimeout> | undefined + const timeoutPromise = new Promise<never>((_, reject) => { + timer = setTimeout(() => reject(new Error(`${label} timeout`)), timeoutMs) + }) + try { + return await Promise.race([task, timeoutPromise]) + } finally { + clearTimeout(timer) + } +}Then both call sites become one-liners:
const entries = await withTimeout('Sitemap generation', getCachedSitemapEntries(githubToken, forceRefresh))
303-306: Minor formatting: closing paren is misaligned.async function handleSitemapXml( forceRefresh: boolean, githubToken: string, - ): Promise<Response> { +): Promise<Response> {Same applies to
handleSitemapJsonon line 323.
1 participant
This PR implements the Dynamic Sitemap (Apps & Plugins) feature requested in issue #2 fix for it
🚀 New Features
/sitemap.xmland/sitemap.jsonendpoints available/plugin-map.xmland/plugin-map.json🛠 Architecture Improvements
📊 Rich Metadata
Endpoints Added
GET /sitemap.xml- XML sitemap of all servicesGET /sitemap.json- JSON sitemap with rich metadataGET /plugin-map.xml- XML plugin deployment mapGET /plugin-map.json- JSON plugin deployment mapDiscovery System
Performance Optimizations