[feat]: Dynamically use deconfig based on the request context #973
Conversation
Signed-off-by: Marcos Candeia <[email protected]>
|
Warning Rate limit exceeded@mcandeia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds request-bound decofile resolution via RequestContext.current and a new decofile getter that parses headers/URL params and builds Deconfig URIs (token-aware); introduces team-aware DecoContext; adds Deconfig.build and token-conditional requests; implements pooled DecofileProvider caching with idle disposal and FS disposal support; binds request contexts at runtime. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Runtime as Deco.handler
participant ReqBinder as RequestContextBinder
participant PooledProv as PooledDecofileProvider
participant Pool as ProviderPool
participant Fallback as FallbackProvider
participant Deconfig as Deconfig (builder)
Client->>Runtime: HTTP request
Runtime->>ReqBinder: RequestContext.bind(request) (sets current)
Note right of ReqBinder #ddd: decofile getter reads headers/query<br/>x-deco-server, x-deco-branch, x-deco-team, x-deco-token
ReqBinder->>Deconfig: Deconfig.build(...) (when deconfig URI needed)
Runtime->>PooledProv: state()/revision()/set()/notify()
alt decofile present
PooledProv->>Pool: get(decofilePath)
Pool-->>PooledProv: provider (cached/new)
PooledProv->>PooledProv: delegate calls to pooled provider
else no decofile
PooledProv->>Fallback: delegate to fallback provider
end
sequenceDiagram
autonumber
participant Factory as getProvider()
participant Deconfig as Deconfig
participant Endpoint as EndpointProvider
participant Pooled as PooledDecofileProvider
participant Local as LocalStorageProvider
Factory->>Deconfig: canParse(decofilePath)?
alt deconfig URI
Factory->>Endpoint: load deconfig-backed provider (may call Deconfig.build)
alt pooled=true
Factory->>Pooled: wrap Endpoint with PooledDecofileProvider
Pooled-->>Factory: provider
else pooled=false
Endpoint-->>Factory: provider
end
else not deconfig
Factory->>Local: load blocks/local provider
Local-->>Factory: provider
end
Factory-->>Factory: compose(providers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/decofile/provider.ts (1)
129-143: Guard against composing zero providersWhen
decofilePathresolves toundefined(no env var) and the blocks folder is missing, andUSE_LOCAL_STORAGEisn’t set, theprovidersarray stays empty. Passing an empty array tocomposetriggersArray.prototype.reduce’s “Reduce of empty array with no initial value” error at runtime, breaking initialization. Add an explicit fallback before callingcompose.Apply this diff:
if (Deno.env.has("USE_LOCAL_STORAGE")) { providers.push(newFsProvider()); } - return compose( - ...providers, - ); + if (providers.length === 0) { + return newFsProvider(); + } + + if (providers.length === 1) { + return providers[0]; + } + + return compose(...providers);
🧹 Nitpick comments (1)
runtime/mod.ts (1)
127-129: Bind the request’s abort signal too.As implemented, we only stash
current, soRequestContext.signal(which readsContext.active().request?.signal) stays empty and downstream code can’t observe cancellations. Please also carryreq.signalwhen binding.- return (req, bindings) => { - return RequestContext.bind({ current: req }, handler)(req, bindings); + return (req, bindings) => { + return RequestContext.bind({ current: req, signal: req.signal }, handler)( + req, + bindings, + ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deco.ts(6 hunks)engine/decofile/constants.ts(1 hunks)engine/decofile/deconfig.ts(2 hunks)engine/decofile/fsFolder.ts(1 hunks)engine/decofile/pool.ts(1 hunks)engine/decofile/provider.ts(3 hunks)engine/manifest/manifest.ts(3 hunks)runtime/mod.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
runtime/mod.ts (2)
engine/manifest/manifest.ts (1)
teamNameFromEnv(107-107)deco.ts (2)
RequestContext(28-33)RequestContext(143-223)
deco.ts (1)
engine/decofile/deconfig.ts (1)
Deconfig(3-30)
engine/decofile/pool.ts (1)
engine/decofile/provider.ts (5)
DecofileProvider(21-28)getProvider(106-143)OnChangeCallback(17-17)ReadOptions(18-20)Decofile(16-16)
engine/decofile/provider.ts (4)
engine/decofile/fs.ts (1)
newFsProvider(100-106)engine/decofile/deconfig.ts (1)
Deconfig(3-30)engine/decofile/pool.ts (1)
PooledDecofileProvider(130-218)engine/decofile/fetcher.ts (1)
fromEndpoint(211-242)
engine/manifest/manifest.ts (2)
engine/decofile/constants.ts (1)
ENV_TEAM_NAME(3-3)deco.ts (2)
DecoContext(48-70)Context(116-138)
🔇 Additional comments (1)
engine/decofile/constants.ts (1)
3-3: New team env key aligns with manifest usage.Matches the manifest helper and keeps naming consistent.
| const active = Context.active(); | ||
| return Context.bind({ | ||
| ...active, | ||
| request: { ...active.request, ...request }, | ||
| }, f); | ||
| }, |
There was a problem hiding this comment.
Prevent crashes when no request context exists yet.
On Line 166 we spread active.request, but the default context starts with request === undefined. Spreading undefined throws TypeError: Cannot convert undefined or null to object, so the very first request will fail. Guard the spread.
Apply this diff:
- request: { ...active.request, ...request },
+ request: { ...(active.request ?? {}), ...request },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const active = Context.active(); | |
| return Context.bind({ | |
| ...active, | |
| request: { ...active.request, ...request }, | |
| }, f); | |
| }, | |
| const active = Context.active(); | |
| return Context.bind({ | |
| ...active, | |
| request: { ...(active.request ?? {}), ...request }, | |
| }, f); | |
| }, |
🤖 Prompt for AI Agents
In deco.ts around lines 163 to 168, the code spreads active.request which can be
undefined and causes a TypeError on first request; change the merge to guard the
spread by using a safe default for active.request (e.g., treat it as an empty
object when undefined) so the returned context sets request to a merged object
of the default and incoming request; update the line that constructs request to
merge ...(active.request ?? {}) with ...request (or equivalent) to avoid
spreading undefined.
| build: ( | ||
| project: string, | ||
| branch: string, | ||
| token?: string, | ||
| server?: string | null, | ||
| ) => { | ||
| return `deconfig://${server ?? "api.decocms.com"}/${project}@${branch}:${ | ||
| token ?? "" | ||
| }`; | ||
| }, |
There was a problem hiding this comment.
Normalize the server before building the URI
If server already includes a scheme (e.g. https://api.decocms.com or http://localhost:8000), the current build routine produces deconfig://https://api.decocms.com/...; parseUri will then prepend another scheme, yielding https://https://api.decocms.com/... and every subsequent fetch fails. Please strip any leading protocol (and trailing slash) before composing the URI.
- return `deconfig://${server ?? "api.decocms.com"}/${project}@${branch}:${token ?? ""}`;
+ const host = (server ?? "api.decocms.com")
+ .replace(/^https?:\/\//, "")
+ .replace(/\/+$/, "");
+ return `deconfig://${host}/${project}@${branch}:${token ?? ""}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| build: ( | |
| project: string, | |
| branch: string, | |
| token?: string, | |
| server?: string | null, | |
| ) => { | |
| return `deconfig://${server ?? "api.decocms.com"}/${project}@${branch}:${ | |
| token ?? "" | |
| }`; | |
| }, | |
| build: ( | |
| project: string, | |
| branch: string, | |
| token?: string, | |
| server?: string | null, | |
| ) => { | |
| const host = (server ?? "api.decocms.com") | |
| .replace(/^https?:\/\//, "") | |
| .replace(/\/+$/, ""); | |
| return `deconfig://${host}/${project}@${branch}:${token ?? ""}`; | |
| }, |
🤖 Prompt for AI Agents
In engine/decofile/deconfig.ts around lines 7 to 16, the build routine blindly
includes the provided server which can already contain a scheme or trailing
slash causing invalid URIs; normalize the server first by defaulting to
"api.decocms.com" when missing, then strip any leading protocol (e.g. "http://",
"https://", or other "<scheme>://") and any trailing slash (use a simple regex
replace like server.replace(/^[a-z]+:\/\//i, "").replace(/\/$/, "")), and
finally compose the deconfig:// URI with that cleaned server value.
| dispose: () => { | ||
| fs[Symbol.dispose]?.(); | ||
| }, |
There was a problem hiding this comment.
Dispose must actually tear down the watcher
We now create and pool multiple folder providers, but this dispose hook only calls fs[Symbol.dispose]?.(). Neither denoFs nor the deconfig-backed Fs currently implement that symbol, so the watchFs loop started above never stops—events and callbacks keep flowing even after the pool evicts the provider, which defeats disposal and leaks resources. Please retain a reference to the watcher you start (e.g. grab it in outer scope) and explicitly close it on dispose (watcher.close?.(), watcher[Symbol.dispose]?.(), or watcher.return?.()), then call any FS-level dispose. Otherwise the pool cannot reclaim idle providers.
🤖 Prompt for AI Agents
In engine/decofile/fsFolder.ts around lines 203 to 205, the dispose hook only
calls fs[Symbol.dispose]?.() but never stops the watchFs loop; retain the
watcher started earlier in an outer-scope variable, assign the iterator/object
returned from watchFs to that variable, and in dispose first close the watcher
by calling watcher.close?.(), watcher[Symbol.dispose]?.(), or watcher.return?.()
(whichever exists) to stop the loop and free resources, then call
fs[Symbol.dispose]?.() afterwards so the provider is fully torn down.
| team?: string, | ||
| ): Promise<DecoContext<T>> => { | ||
| const currentContext = Context.active<T>(); | ||
| const ctx: DecoContext<T> = { | ||
| ...currentContext, | ||
| team, | ||
| site: site ?? currentContext.site, |
There was a problem hiding this comment.
Preserve the existing team when the caller omits it.
Line 420 assigns team directly from the optional argument, so any caller that still calls newContext(...) without the new parameter will wipe out the team we already resolved (and downstream request-bound deconfig resolution stops working). Keep the current value unless a new one is provided.
Apply this diff:
- team,
+ team: team ?? currentContext.team,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| team?: string, | |
| ): Promise<DecoContext<T>> => { | |
| const currentContext = Context.active<T>(); | |
| const ctx: DecoContext<T> = { | |
| ...currentContext, | |
| team, | |
| site: site ?? currentContext.site, | |
| team?: string, | |
| ): Promise<DecoContext<T>> => { | |
| const currentContext = Context.active<T>(); | |
| const ctx: DecoContext<T> = { | |
| ...currentContext, | |
| team: team ?? currentContext.team, | |
| site: site ?? currentContext.site, |
🤖 Prompt for AI Agents
In engine/manifest/manifest.ts around lines 415 to 421, the code unconditionally
assigns the newContext.team from the optional parameter which overwrites the
previously resolved team when callers omit the argument; change the assignment
to preserve the existing team by using the existing context's team when the
parameter is undefined (i.e., set team to the provided value only if present,
otherwise keep currentContext.team).
Signed-off-by: Marcos Candeia <[email protected]>
Signed-off-by: Marcos Candeia <[email protected]>
Signed-off-by: Marcos Candeia <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/decofile/deconfig.ts (1)
112-147: Propagate the resolved token to headers and watcher URL
tokenalready falls back toDECO_DECONFIG_TOKEN, but the Authorization header and watcher query string still readoptions.token. With only the env var set, we silently drop auth and even emitauth-token=undefined. Use the resolvedtokeneverywhere and only append the param when it exists.- const token = options.token ?? Deno.env.get("DECO_DECONFIG_TOKEN"); + const token = options.token ?? Deno.env.get("DECO_DECONFIG_TOKEN"); @@ - ...(token ? { Authorization: `Bearer ${options.token}` } : {}), + ...(token ? { Authorization: `Bearer ${token}` } : {}), @@ - url.searchParams.set("auth-token", options.token); + if (token) { + url.searchParams.set("auth-token", token); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engine/decofile/deconfig.ts(4 hunks)engine/decofile/fsFolder.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engine/decofile/fsFolder.ts (4)
deco.ts (2)
decofile(179-213)Context(116-138)engine/decofile/realtime.ts (1)
VersionedDecofile(38-41)engine/decofile/provider.ts (2)
Decofile(16-16)ReadOptions(18-20)engine/decofile/pool.ts (1)
state(168-171)
🔇 Additional comments (2)
engine/decofile/fsFolder.ts (1)
145-213: Retain and dispose the watcher on provider teardownWe still spawn
fs.watchFsper provider but never stop it, so pooled providers leak watchers/events after eviction—exactly the issue flagged earlier. Keep a shared reference to the watcher and close/return it insidedispose()before delegating to the underlying FS.- (async () => { - const limiter = new Mutex(); - const watcher = fs.watchFs(fullPath); + (async () => { + const limiter = new Mutex(); + watcher = fs.watchFs(fullPath); @@ - for await (const event of watcher) { + for await (const event of watcher) { @@ - dispose: () => { - fs[Symbol.dispose]?.(); + dispose: () => { + watcher?.return?.(); + watcher?.close?.(); + watcher = null; + fs[Symbol.dispose]?.();(Remember to declare
let watcher: ReturnType<Fs["watchFs"]> | null = null;alongsidedecofile.)engine/decofile/deconfig.ts (1)
13-15: Normalize the deconfig host before composing the URL
Deconfig.build()still dropsserverin verbatim. Passing something likehttps://api.decocms.comyieldsdeconfig://https://api.decocms.com/..., whichparseUri()turns intohttps://https://api.decocms.com/..., breaking every downstream fetch. Strip the scheme (and trailing slashes) before composing the URI.- return `deconfig://${server ?? "api.decocms.com"}/${project}@${branch}:${token ?? ""}`; + const host = (server ?? "api.decocms.com") + .replace(/^[a-z0-9+.-]+:\/\//i, "") + .replace(/\/+$/, ""); + return `deconfig://${host}/${project}@${branch}:${token ?? ""}`;
| return decofile ??= fs.exists(fullPath, { | ||
| isDirectory: true, | ||
| isReadable: true, | ||
| }).then( | ||
| async () => { | ||
| const decofile: Decofile = {}; | ||
| const files = fs.readDir(fullPath); | ||
| const promises: Promise<unknown>[] = []; | ||
| for await (const file of files) { | ||
| promises.push( | ||
| fs.readTextFile(join(fullPath, file)) | ||
| .then(JSON.parse) | ||
| .catch(() => null); | ||
| changedBlocks[parseBlockId(basename(filePath))] = content; | ||
| }), | ||
| ); | ||
| decofile = Promise.resolve({ | ||
| state: { ...prevState, ...changedBlocks }, | ||
| revision: `${Date.now()}`, | ||
| }); | ||
| for (const cb of onChangeCbs) { | ||
| cb(); | ||
| .then((content) => { | ||
| if (content !== null) { | ||
| decofile[parseBlockId(file)] = content; | ||
| } | ||
| }) | ||
| .catch(() => null), | ||
| ); | ||
| } | ||
| }, 300); | ||
| for await (const event of watcher) { | ||
| filesChangedBatch.push(...event.paths); | ||
| updateState(); | ||
| } | ||
| })(); | ||
| await Promise.all(promises); | ||
| return { | ||
| state: decofile, | ||
| revision: Context.active().isPreview | ||
| ? `${Date.now()}` | ||
| : Context.active().deploymentId ?? `${Date.now()}`, | ||
| }; | ||
| }, | ||
| ).then((result) => { |
There was a problem hiding this comment.
Check fs.exists result before reading the folder
Fs.exists returns a boolean, yet we drop the value and proceed to readDir/watchFs regardless. When the directory is missing or unreadable, we fall into a cascade of ENOENTs after partially initializing the provider. Validate the result up front and bail explicitly.
- return decofile ??= fs.exists(fullPath, {
- isDirectory: true,
- isReadable: true,
- }).then(
- async () => {
+ return decofile ??= fs.exists(fullPath, {
+ isDirectory: true,
+ isReadable: true,
+ }).then(
+ async (isPresent) => {
+ if (!isPresent) {
+ throw new Deno.errors.NotFound(
+ `${fullPath} is missing or not accessible`,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return decofile ??= fs.exists(fullPath, { | |
| isDirectory: true, | |
| isReadable: true, | |
| }).then( | |
| async () => { | |
| const decofile: Decofile = {}; | |
| const files = fs.readDir(fullPath); | |
| const promises: Promise<unknown>[] = []; | |
| for await (const file of files) { | |
| promises.push( | |
| fs.readTextFile(join(fullPath, file)) | |
| .then(JSON.parse) | |
| .catch(() => null); | |
| changedBlocks[parseBlockId(basename(filePath))] = content; | |
| }), | |
| ); | |
| decofile = Promise.resolve({ | |
| state: { ...prevState, ...changedBlocks }, | |
| revision: `${Date.now()}`, | |
| }); | |
| for (const cb of onChangeCbs) { | |
| cb(); | |
| .then((content) => { | |
| if (content !== null) { | |
| decofile[parseBlockId(file)] = content; | |
| } | |
| }) | |
| .catch(() => null), | |
| ); | |
| } | |
| }, 300); | |
| for await (const event of watcher) { | |
| filesChangedBatch.push(...event.paths); | |
| updateState(); | |
| } | |
| })(); | |
| await Promise.all(promises); | |
| return { | |
| state: decofile, | |
| revision: Context.active().isPreview | |
| ? `${Date.now()}` | |
| : Context.active().deploymentId ?? `${Date.now()}`, | |
| }; | |
| }, | |
| ).then((result) => { | |
| return decofile ??= fs.exists(fullPath, { | |
| isDirectory: true, | |
| isReadable: true, | |
| }).then( | |
| async (isPresent) => { | |
| if (!isPresent) { | |
| throw new Deno.errors.NotFound( | |
| `${fullPath} is missing or not accessible`, | |
| ); | |
| } | |
| const decofile: Decofile = {}; | |
| const files = fs.readDir(fullPath); | |
| const promises: Promise<unknown>[] = []; | |
| for await (const file of files) { | |
| promises.push( | |
| fs.readTextFile(join(fullPath, file)) | |
| .then(JSON.parse) | |
| .then((content) => { | |
| if (content !== null) { | |
| decofile[parseBlockId(file)] = content; | |
| } | |
| }) | |
| .catch(() => null), | |
| ); | |
| } | |
| await Promise.all(promises); | |
| return { | |
| state: decofile, | |
| revision: Context.active().isPreview | |
| ? `${Date.now()}` | |
| : Context.active().deploymentId ?? `${Date.now()}`, | |
| }; | |
| }, | |
| ).then((result) => { |
🤖 Prompt for AI Agents
In engine/decofile/fsFolder.ts around lines 116-144, the code calls
fs.exists(...) but ignores the boolean result and proceeds to read the
directory; change the logic to first await the boolean from fs.exists(fullPath,
{isDirectory:true,isReadable:true}) and if it resolves to false bail immediately
(return undefined or the provider’s empty/absent value) instead of continuing to
readDir/watchFs; only when exists returns true proceed to readDir, parse files,
collect promises and build the decofile state and revision as currently
implemented.
| const changedBlocks: Decofile = {}; | ||
| await Promise.all( | ||
| copied.map(async (filePath) => { | ||
| const content = await fs.readTextFile(filePath) | ||
| .then(JSON.parse) | ||
| .catch(() => null); | ||
| changedBlocks[parseBlockId(basename(filePath))] = content; | ||
| }), | ||
| ); | ||
| decofile = Promise.resolve({ | ||
| state: { ...prevState, ...changedBlocks }, | ||
| revision: `${Date.now()}`, | ||
| }); |
There was a problem hiding this comment.
Remove stale blocks when files disappear or fail to parse
During incremental updates we still assign null into changedBlocks, so deleted/broken files linger in the cached state forever. Align with the initial load: drop the key when content can’t be read instead of keeping a null placeholder.
- await Promise.all(
- copied.map(async (filePath) => {
- const content = await fs.readTextFile(filePath)
- .then(JSON.parse)
- .catch(() => null);
- changedBlocks[parseBlockId(basename(filePath))] = content;
- }),
- );
- decofile = Promise.resolve({
- state: { ...prevState, ...changedBlocks },
+ await Promise.all(
+ copied.map(async (filePath) => {
+ const blockId = parseBlockId(basename(filePath));
+ const content = await fs.readTextFile(filePath)
+ .then(JSON.parse)
+ .catch(() => null);
+ if (content === null) {
+ delete prevState[blockId];
+ return;
+ }
+ changedBlocks[blockId] = content;
+ }),
+ );
+ decofile = Promise.resolve({
+ state: { ...prevState, ...changedBlocks },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const changedBlocks: Decofile = {}; | |
| await Promise.all( | |
| copied.map(async (filePath) => { | |
| const content = await fs.readTextFile(filePath) | |
| .then(JSON.parse) | |
| .catch(() => null); | |
| changedBlocks[parseBlockId(basename(filePath))] = content; | |
| }), | |
| ); | |
| decofile = Promise.resolve({ | |
| state: { ...prevState, ...changedBlocks }, | |
| revision: `${Date.now()}`, | |
| }); | |
| const changedBlocks: Decofile = {}; | |
| await Promise.all( | |
| copied.map(async (filePath) => { | |
| const blockId = parseBlockId(basename(filePath)); | |
| const content = await fs.readTextFile(filePath) | |
| .then(JSON.parse) | |
| .catch(() => null); | |
| if (content === null) { | |
| // drop this block from the previous state when file is gone or unparseable | |
| delete prevState[blockId]; | |
| return; | |
| } | |
| changedBlocks[blockId] = content; | |
| }), | |
| ); | |
| decofile = Promise.resolve({ | |
| state: { ...prevState, ...changedBlocks }, | |
| revision: `${Date.now()}`, | |
| }); |
🤖 Prompt for AI Agents
In engine/decofile/fsFolder.ts around lines 162 to 174, the current incremental
update sets changedBlocks[key] = null for files that are missing or fail to
parse, which leaves stale keys in the merged state; instead, only add entries to
changedBlocks when content is non-null and track missing/broken keys separately,
then after merging prevState with changedBlocks remove those tracked keys from
the resulting state (so deleted or unparsable files are dropped rather than
stored as null); implement by not assigning null into changedBlocks, collecting
removedKeys, creating the merged state, and deleting each removed key from that
merged object before resolving decofile.
Signed-off-by: Marcos Candeia <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deno.json(1 hunks)dev/deno.json(1 hunks)engine/decofile/deconfig.ts(4 hunks)scripts/deno.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- scripts/deno.json
- dev/deno.json
🔇 Additional comments (2)
deno.json (1)
3-3: Version bump looks consistentNo concerns with moving to 1.127.0-beta.1 here.
engine/decofile/deconfig.ts (1)
7-15: Normalize server host before composing the deconfig URIIf the caller passes
https://api.decocms.com(or any host with scheme/trailing slash), Line 13 buildsdeconfig://https://api.decocms.com/..., which later parses intohttps://https://api.decocms.com/...and breaks every fetch. Please strip the scheme and trailing slashes before composing the URI.) => { - return `deconfig://${server ?? "api.decocms.com"}/${project}@${branch}:${ - token ?? "" - }`; + const host = (server ?? "api.decocms.com") + .replace(/^[a-z]+:\/\//i, "") + .replace(/\/+$/, ""); + return `deconfig://${host}/${project}@${branch}:${token ?? ""}`; },
| const token = options.token ?? Deno.env.get("DECO_DECONFIG_TOKEN"); | ||
| const callTool = async (tool: string, args: Record<string, unknown>) => { | ||
| const response = await fetch( | ||
| `${options.projectUrl}/i:deconfig-management/tools/call/${tool}`, | ||
| `${options.projectUrl}/tools/call/${tool}`, | ||
| { | ||
| method: "POST", | ||
| body: JSON.stringify(args), | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${options.token}`, | ||
| "x-deco-branch": options.branch ?? "main", | ||
| ...(token ? { Authorization: `Bearer ${options.token}` } : {}), | ||
| }, |
There was a problem hiding this comment.
Use the resolved token for all outbound requests
Line 111 resolves token, but Lines 121 and 145 still reference options.token. When options.token is absent yet DECO_DECONFIG_TOKEN is set, you end up sending Authorization: Bearer undefined and auth-token=undefined, so every authenticated call fails. Use the resolved token consistently (and skip the param when none is available).
- const token = options.token ?? Deno.env.get("DECO_DECONFIG_TOKEN");
+ const token =
+ options.token?.trim() || Deno.env.get("DECO_DECONFIG_TOKEN")?.trim();
...
- ...(token ? { Authorization: `Bearer ${options.token}` } : {}),
+ ...(token ? { Authorization: `Bearer ${token}` } : {}),
...
- url.searchParams.set("auth-token", options.token);
+ if (token) {
+ url.searchParams.set("auth-token", token);
+ } else {
+ url.searchParams.delete("auth-token");
+ }Also applies to: 145-146
🤖 Prompt for AI Agents
In engine/decofile/deconfig.ts around lines 111 to 122 (and also apply to
145-146), the code resolves token into the local variable `token` but continues
to reference `options.token` when building Authorization header and query
params; replace uses of `options.token` with the resolved `token` variable and
only add the Authorization header or auth-token query parameter when `token` is
truthy so we never send "Bearer undefined" or "auth-token=undefined".
Signed-off-by: Marcos Candeia <[email protected]>
Summary by CodeRabbit
New Features
Refactor
Chore