feat(templates): make the starter-template picker content-editable and version-correct - #1383
Open
MaanilVerma wants to merge 14 commits into
Open
feat(templates): make the starter-template picker content-editable and version-correct#1383MaanilVerma wants to merge 14 commits into
MaanilVerma wants to merge 14 commits into
Conversation
`net.request` has no deadline of its own, so a stalled socket could hang every awaiting caller indefinitely. - generalise fetchJSONOnce into fetchOnce(url, cached, cacheKey, parse, timeoutMs); fetchJSON and fetchText share one machine - fetchText inherits the ETag cache, R2-mirror retry, and last-cached fallback, so an offline boot with a warm cache still resolves - namespace the persisted cache key by parser (json:/text:) so a URL whose parser changes between releases cannot 304 a string into a JSON caller - drop legacy un-prefixed keys on load rather than let them hold capacity
- add getOpsFlagPayload, a sibling of getOpsFlag for flags whose value is a JSON payload; same consent-bypass rationale and fail-to-undefined posture - collapse makeOpsFlag and the new makeOpsFlagPayload onto one factory with the fetcher injected, so the in-flight/cached/reset plumbing exists once - both public signatures are unchanged; the kill-switch path stays typed around FeatureFlagValue The fetcher is passed as a closure so the telemetry export resolves at call time, not import time — partial ./telemetry mocks would otherwise throw.
The picker read the live `main` index while the user's ComfyUI ships a pinned `comfyui-workflow-templates` package, and the two drift: v0.28.2 pins 0.11.12 (562 templates) while 0.11.31 has 587. So the picker could offer a card the install cannot open — `video_minimax_h3_t2v`, the auto-selected Video pick, does not exist on v0.28.2. That pin is the only compatibility map that exists; the template index carries no version field and its schema is additionalProperties: false. Resolves a ComfyUI tag through requirements.txt to the pinned package version, then to a versioned index URL. Every failure returns null and falls back to `main`, so an unresolvable pin is exactly today's behaviour.
Lets the content team change which cards the post-install picker offers without a code change or a release. Reads through the ops-flag path, which bypasses the consent gate — the picker renders while consent is still 'undecided', so a value routed through experiments.ts would never arrive. Layers, each a fallback for the one above: flag payload -> disk cache -> baked-in CURATED_TEMPLATES. Parser is default-deny. `id` reaches a filesystem path and a fetch URL, so it is pattern-checked and length-capped; unknown fields are ignored for forward compat; a bad optional field drops only itself. Invalid entries drop individually, so one bad row cannot empty the picker. A known id filed under the wrong modality is rejected outright — it would otherwise fork one card across two tabs. The payload arrives from PostHog as an escaped JSON string rather than an object, so the parser accepts both forms.
Sources the card list from the remote manifest, hydrates it against the index the target ComfyUI actually ships, and guarantees the picker's shape. - honour requiresCustomNodes and includeOnDistributions, which the frontend already filters on; desktop could otherwise offer a card it then hides - draw substitutes only from starter categories, so a Node Basics tutorial or Utility graph never becomes a first-run card - never substitute with a baked-in id still owed its own slot; taking one consumes a slot instead of filling it and leaves the tab short - thumbnails resolve against the pinned asset base, so a preview matches the index entry it was hydrated from - terminal top-up makes 4-per-modality unconditional: it relaxes index membership, then runnability, rather than ship a short tab - identity is never relaxed; two cards sharing an id collide on FieldOption.value, the picker's v-for key and selection identity - an all-API tab gets no recommendation, so the wizard offers skip rather than auto-selecting a card that spends credits
- fetch the flag at boot beside initCloudFreeRuns, on the same pre-consent path; never awaited, so it cannot slow startup - resolve the target ComfyUI version for the picker: an explicit version pick, else the channel's resolved tag, else the latest stable tag The last fallback matters because the thumbnail warm-up calls getFieldOptions with no selections. Without it the warm-up prefetched main-based URLs while the picker rendered pinned ones, so every prefetch missed and the index was fetched twice per cold start. Also pass the pinned asset base when resolving a template's models, so a workflow fetched over the network is the same revision as the card the user picked.
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe app now initializes starter-template manifests asynchronously, supports timeout-aware JSON and text fetching, resolves version-pinned template assets, validates remote manifests, and builds catalogs with compatibility filtering, substitutions, backfill, caching, and recommendation rules. ChangesStarter template catalog
Sequence Diagram(s)sequenceDiagram
participant AppStartup
participant StarterTemplateManifest
participant PostHog
participant TemplateCatalog
participant TemplatePin
participant TemplateIndex
participant TemplateModels
AppStartup->>StarterTemplateManifest: initialize with installation identifier
StarterTemplateManifest->>PostHog: fetch starter-template payload
TemplateCatalog->>StarterTemplateManifest: load validated templates
TemplateCatalog->>TemplatePin: resolve ComfyUI template package version
TemplatePin->>TemplateIndex: fetch versioned requirements and index
TemplateIndex-->>TemplateCatalog: return catalog entries and asset base
TemplateCatalog->>TemplateModels: resolve workflows with asset base
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
MaanilVerma
changed the title
Collaborator
Author
|
@Kosinkadink Can you please review this and see if I missed any edge cases ? And @deepme987 you as well. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/lib/fetch.ts (1)
158-177: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMove the cache write inside the settle guard.
A timed-out request still runs its
response/endhandler when the body arrives later. Lines 168-173 call_cacheSetoutsidefinish, so the entry is written and persisted after the caller already received the timeout rejection. In tests this can also write after_resetCacheForTestand make a later assertion depend on timing. Put the write insidefinishso one settle decision covers both the promise and the cache. A tardy response should not sneak a souvenir into the cache.🐛 Proposed fix
- if (cacheKey) { - const etag = _headerString(response.headers['etag']) - if (etag) { - _cacheSet(cacheKey, { etag, data: parsed }) - } - } - finish(() => resolve(parsed)) + finish(() => { + if (cacheKey) { + const etag = _headerString(response.headers['etag']) + if (etag) { + _cacheSet(cacheKey, { etag, data: parsed }) + } + } + resolve(parsed) + })As per coding guidelines: "Treat flaky tests as unacceptable; tests must be deterministic and reliable."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/lib/fetch.ts` around lines 158 - 177, The cache update in the response end handler currently runs outside the settle guard, allowing timed-out requests to write stale entries later. Move the _cacheSet call and its ETag/cacheKey checks into the callback passed to finish, while preserving parsed-data resolution and ensuring a tardy response performs neither promise settlement nor cache mutation.Source: Coding guidelines
src/main/sources/standalone/templateModels.ts (1)
56-82: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject substitutable template names before
loadTemplateJson.
templateCatalog.tscan marklocation.entry.nameas a substitute id, but it does not checkTEMPLATE_ID_PATTERN; only the baked-in and PostHog-derived ids are already vetted. SinceloadTemplateJsonuses that id in both a filesystempath.joinand a URL, reject names outside alphanumerics and_.-early so path traversal tries cannot walk outsidetempltes/. Add the guard there and importTEMPLATE_ID_PATTERNfrom./curatedTemplates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/sources/standalone/templateModels.ts` around lines 56 - 82, Validate templateId at the start of loadTemplateJson using TEMPLATE_ID_PATTERN imported from ./curatedTemplates, returning null for names outside the allowed alphanumeric, underscore, dot, and hyphen characters. Apply this guard before any filesystem or remote URL construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/lib/fetch.test.ts`:
- Around line 233-238: Update the late-timeout test around fetchText to use
Vitest fake timers, advance timers so the setImmediate response completion and
timeout are deterministic, and remove the real 30 ms wait. Preserve the
assertion that the request resolves with “body” even after the timeout would
otherwise become eligible.
In `@src/main/sources/standalone/curatedTemplates.test.ts`:
- Around line 41-71: Merge the duplicated curated-template invariants from the
describe block into the existing manifest validation block, preserving the
stronger assertions for four entries per modality, exactly one non-API
recommended entry, and snapshot fields. Remove the redundant tests while
retaining the unique-IDs test as the new invariant, anchored to
CURATED_TEMPLATES and TEMPLATE_MODALITY_ORDER.
In `@src/main/sources/standalone/index.ts`:
- Around line 569-578: Update loadTemplateCatalog to return both the catalog
templates and its resolved assetBase, then in the surrounding flow reuse that
returned assetBase when calling resolveTemplateModels. Remove the separate
resolveTemplatePackageVersion and templateAssetBaseFor resolution so thumbnails
and workflow JSON use the same resolved revision.
- Around line 562-569: Update the comfyVersion selection logic near
targetComfyVersion to accept selections.comfyVersion.value only when the release
channel is stable, mirroring the isStable guards in the variant branch and
buildInstallation. Preserve the existing tag-format validation and fallback
order, ensuring stale values from the latest channel are ignored before loading
the template catalog.
In `@src/main/sources/standalone/starterTemplateManifest.test.ts`:
- Around line 228-232: Add two tests adjacent to the existing snapshot
preservation test: verify parseStarterTemplateManifest preserves
snapshotOverrides: true when a valid snapshot is present, and omits or drops
snapshotOverrides when no snapshot is provided. Use the existing manifest
fixtures and assertions to cover both valid and invalid combinations.
In `@src/main/sources/standalone/starterTemplateManifest.ts`:
- Around line 140-154: Make the disk fallback rollback-safe by adding an age
limit or explicit invalidation tied to the disabled/absent result of
desktop_starter_templates, so stale persisted manifests eventually return to
CURATED_TEMPLATES. Update the flag-loading flow and fromDisk/persist handling to
distinguish an absent or disabled flag from a remote fetch failure, preserving
disk fallback only for failures. Document the operator rollback procedure and
ensure the cache metadata or deletion behavior is applied consistently.
In `@src/main/sources/standalone/templateCatalog.test.ts`:
- Around line 238-250: Update the test case around loadTemplateCatalog to assert
unconditionally that the compatible substitute live_local_image appears before
first.id, without falling back to image.length when first.id is absent. Also
explicitly assert that first.id is not present in image, preserving the
deterministic expectation that the missing card is excluded.
In `@src/main/sources/standalone/templateCatalog.ts`:
- Around line 390-424: Thread the existing now timestamp from
loadTemplateCatalogUncached through backfill into both fillFromBakedIn calls,
and apply isWithinWindow directly to each CURATED_TEMPLATES entry before adding
it. Ensure future and expired baked-in templates remain excluded in both strict
and terminal passes, and document the terminal-pass window behavior in the
fillFromBakedIn/backfill comment.
In `@src/main/sources/standalone/templateCatalogResolution.test.ts`:
- Around line 408-418: Strengthen the positive test around loadTemplateCatalog
so it explicitly asserts that the allowlisted Use Cases entry use_case_ok is
present in the resulting catalog, matching the named-ID checks in the negative
test. Keep the existing expectFourByFour assertion, but add a direct assertion
against the catalog contents so the test fails if Use Cases substitution is
removed.
In `@src/main/sources/standalone/templateModels.test.ts`:
- Around line 131-145: The test for resolveTemplateModels must also verify that
the pinned asset fetch is the only network request. Extend the existing
assertion around fetchJSON to require exactly one call, preserving the current
pinned URL and argument checks.
In `@src/main/sources/standalone/templatePin.test.ts`:
- Around line 70-81: The test assertion in treats a bare version and a
v-prefixed tag identically currently verifies the first fetchText call, so it
does not validate normalization of the bare tag. Update the fetchText URL
assertion to target the most recent call after resolving bare, while preserving
the existing comparison of resolved versions.
---
Outside diff comments:
In `@src/main/lib/fetch.ts`:
- Around line 158-177: The cache update in the response end handler currently
runs outside the settle guard, allowing timed-out requests to write stale
entries later. Move the _cacheSet call and its ETag/cacheKey checks into the
callback passed to finish, while preserving parsed-data resolution and ensuring
a tardy response performs neither promise settlement nor cache mutation.
In `@src/main/sources/standalone/templateModels.ts`:
- Around line 56-82: Validate templateId at the start of loadTemplateJson using
TEMPLATE_ID_PATTERN imported from ./curatedTemplates, returning null for names
outside the allowed alphanumeric, underscore, dot, and hyphen characters. Apply
this guard before any filesystem or remote URL construction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a8760e8-9013-419f-9edb-0c4c3f6d4917
📒 Files selected for processing (17)
src/main/index.tssrc/main/lib/fetch.test.tssrc/main/lib/fetch.tssrc/main/lib/opsFlag.tssrc/main/lib/telemetry.tssrc/main/sources/standalone/curatedTemplates.test.tssrc/main/sources/standalone/curatedTemplates.tssrc/main/sources/standalone/index.tssrc/main/sources/standalone/starterTemplateManifest.test.tssrc/main/sources/standalone/starterTemplateManifest.tssrc/main/sources/standalone/templateCatalog.test.tssrc/main/sources/standalone/templateCatalog.tssrc/main/sources/standalone/templateCatalogResolution.test.tssrc/main/sources/standalone/templateModels.test.tssrc/main/sources/standalone/templateModels.tssrc/main/sources/standalone/templatePin.test.tssrc/main/sources/standalone/templatePin.ts
… the picker - Backfill re-added baked-in cards the availability window had retired or not yet staged, so content could never actually pull or schedule a card. The window now applies in both backfill passes, and a window the payload sets on a baked-in id wins over the static entry. - The template step read comfyVersion without the stable-channel guard its two sibling branches use, so a tag left over from a channel toggle could pin the picker to a version the install never runs. - loadTemplateCatalog returns the asset base it resolved, so thumbnails and workflow JSON can no longer land on different revisions. - The disk cache now carries a write stamp and expires, so deleting the PostHog flag rolls the picker back instead of serving a withdrawn payload forever. Tests: tighten assertions that could not fail (ordering bound, Use Cases substitution, pin normalization targeting the wrong call, missing fallback call-count), drop a duplicated invariant block, cover snapshotOverrides, and replace a real-timer race with fake timers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/sources/standalone/templateCatalog.ts`:
- Line 273: Remove the duplicate declarations of catalogCache in the module,
retaining a single Map<string, { at: number; value: TemplateCatalog }>
declaration in the shared scope so the module compiles without changing cache
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0789ae4-6ccc-47ae-8eb5-8d7d06547162
📒 Files selected for processing (11)
src/main/lib/fetch.test.tssrc/main/sources/standalone/curatedTemplates.test.tssrc/main/sources/standalone/index.test.tssrc/main/sources/standalone/index.tssrc/main/sources/standalone/starterTemplateManifest.test.tssrc/main/sources/standalone/starterTemplateManifest.tssrc/main/sources/standalone/templateCatalog.test.tssrc/main/sources/standalone/templateCatalog.tssrc/main/sources/standalone/templateCatalogResolution.test.tssrc/main/sources/standalone/templateModels.test.tssrc/main/sources/standalone/templatePin.test.ts
MaanilVerma
changed the title
FeedbackModal renders an iframe pointing at the live typeform URL, and happy-dom fetches iframe sources for real. The request outlived the test, so environment teardown aborted it and the rejection surfaced as an unhandled "window is not defined" error attributed to whichever file was running at the time. Intermittent, and misleading about where the problem was. Tests assert on the iframe's src attribute, never on loaded frame content, so disabling iframe page loading costs no coverage and takes the suite off the network.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vitest.config.ts`:
- Around line 20-22: Update the happyDOM settings in environmentOptions to
replace the deprecated disableIframePageLoading key with
navigation.disableChildFrameNavigation set to true, preserving the existing
child-frame navigation behavior for happy-dom 20.8.9.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 510fec22-3e97-476b-bc00-d45259d6266a
📒 Files selected for processing (1)
vitest.config.ts
Merging main brought in Comfy-Org#1386, which swapped the video API-node starter from Seedance 2.0 to 2.5. That touched only curatedTemplates.ts, so nothing conflicted, but three tests hardcoded ids that were really just "the other video cards" or "some baked-in image id" and broke on content they never meant to pin. Derive them from CURATED_TEMPLATES so a content swap cannot fail a test that is not about that content. The one place an id is genuinely load-bearing (video_minimax_h3_t2v as the v0.28.2 compatibility case) still names it.
…into feat/dynamic-starter-templates
`disableIframePageLoading` is deprecated in happy-dom 20.8.9 in favour of `navigation.disableChildFrameNavigation`. The replacement blocks at the navigation validator rather than erroring inside the iframe element, so it also drops the "Iframe page loading is disabled" console error the old flag logged on every run.
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The content team can now change which templates the post-install picker offers by editing a PostHog flag, with no code change and no app release. Along the way this fixes a live bug where the picker could offer a template the user's ComfyUI cannot actually open.
Changes
What
mainwhile the user's ComfyUI installs a pinned template package, and those drift. ComfyUI v0.28.2 pins 562 templates, current pins 587. The Video tab's auto-selected card does not exist in the older set, so those users were offered a card that cannot open. The picker now reads the index at the version the target install actually ships.imagetype also covers Utility, LLM, and Node Basics, so a tutorial graph could previously be promoted into the Image tab.net.requesthas no deadline of its own, so a stalled socket could hang the install wizard indefinitely.Breaking
None. With no flag configured the picker behaves exactly as it does today. Deeplinks, telemetry, and the install wizard's field contract are unchanged, and the four-per-tab shape is preserved on every path.
Review Focus
The crux is the invariant: exactly four cards in each of the four tabs, always. That is what most of the resolution logic exists to protect, and it is worth reviewing as a whole rather than per hunk.
templateCatalog.tsis the file to open.Two decisions worth a second opinion:
FieldOption.value, which the picker uses as both itsv-forkey and its selection identity, and would give the express-install path two competing defaults. When a payload forces the choice, a three-card tab is preferred over a duplicated card. This reverses an earlier approach in the branch, and the reasoning is recorded in the code so it is not undone by accident.requirements.txtpin is the only compatibility map that exists, so the code derives from that rather than asking a content editor to maintain a number nothing can validate.Deliberately out of scope: no mid-session refresh, since the picker must not mutate under the user; no per-user targeting; no CMS, since the PostHog flag editor is the authoring surface.
githubStars.tsstill hand-rolls its own fetch and was left alone as unrelated surface.Testing
The invariant is the thing that can break in ways a per-function test will not catch, so the suite asserts it as a shared post-condition across every degradation path rather than testing each helper in isolation. Fixtures are built from the real index shape, including cases with no substitute candidates, since a fixture that always supplies spares hides exactly the failure this feature introduces. All network is mocked and time is controlled, so nothing here is wall-clock dependent.
Edge cases covered:
Test files:
starterTemplateManifest.test.ts(37): payload parsing including the escaped-string form, per-entry default-deny validation, disk cache re-validation, and an assertion that reading the flag captures no telemetry event.templateCatalogResolution.test.ts(45): the four-by-four invariant across every failure combination, the version gate, upstream compatibility signals, substitution quality, and adversarial payloads that are valid per entry but hostile in aggregate.templatePin.test.ts(16): tag to pinned package version resolution, caching, and fail-open on every error path.fetch.test.ts(17): text fetching shares the ETag cache and retry layer, request timeouts fire and abort the socket, and a cached JSON body is never served to a text caller.templateCatalog.test.ts(27),curatedTemplates.test.ts(22),templateModels.test.ts(15): hydration, the baked-in list's four-per-tab shape, and pinned workflow resolution.Gate:
typecheck,lint, andformat:checkclean. Full suite 3838 passed, 1 skipped (pre-existing), 227 files.Beyond unit tests, the resolution algorithm was replayed against real
workflow_templatesindex data at three pinned versions plus the live index, covering twelve failure scenarios. All held at four per tab. The specific regression verifies end to end: on ComfyUI v0.28.2 the Video tab now offers a template that install actually ships, while a current install still gets the intended recommendation.Manual verification: install with no flag configured and confirm the picker is unchanged, then set a payload and confirm the named cards appear after a full app restart (flags are read once at boot, so a reload will not pick it up).