Feat/comfyhub redesign#950
Open
MaanilVerma wants to merge 65 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR overhauls the hub site: it adds an Embla-powered featured carousel, rebuilds the site navigation (desktop dropdowns, mobile sheet drawer, GitHub star badge), introduces a ChangesHub Site Redesign: Carousel, Navigation, Browse Toolbar & Detail Page
Sequence Diagram(s)sequenceDiagram
participant Page as workflows/index.astro
participant Featured as getFeatured / featuredPreloadImage
participant Carousel as FeaturedCarousel (client:load)
participant Browse as HubBrowse (client:load)
participant Toolbar as BrowseToolbar
participant Store as useHubStore
participant Grid as WorkflowGrid
participant Facets as useFacets
Page->>Featured: loadSerializedTemplates() → getFeatured(templates)
Featured-->>Page: featured[], featuredLcpImage
Page->>Carousel: templates=featured, labels, locale
Carousel->>Carousel: initEmbla + autoplay plugin
Page->>Browse: templates, facetsConfig, toolbarLabels
Browse->>Grid: filteredTemplates, facetTemplates, facetsConfig, toolbarLabels
Grid->>Toolbar: facetSource, facetsConfig, toolbarLabels
Toolbar->>Store: setTab / cycleSort / toggleBadge
Toolbar->>Facets: facetsByType, isBadgeActive, activeCountForType
Store-->>Grid: activeTab, sortBy → sorted/filtered display
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…description, FAQ accordion, and related grid
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
site/src/components/hub/HubBrowse.vue (1)
63-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle dual mode badges as a union (not first-match wins).
At Line 65 and Line 66, when both
appandnodeGraphare selected, the firstifreturns early and filters to apps only. In rhyme-time: two badges in, one badge out. Treat both-selected as “show all modes.”🔧 Suggested fix
if (modeBadges.length > 0) { + const includeApps = modeBadges.includes('app'); + const includeNodeGraphs = modeBadges.includes('nodeGraph'); result = result.filter((t) => { - if (modeBadges.includes('app')) return t.isApp; - if (modeBadges.includes('nodeGraph')) return !t.isApp; + if (includeApps && includeNodeGraphs) return true; + if (includeApps) return t.isApp; + if (includeNodeGraphs) return !t.isApp; return true; }); }🤖 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 `@site/src/components/hub/HubBrowse.vue` around lines 63 - 67, The filter logic in the modeBadges length check uses early return statements that cause only the first matching condition to apply, so when both 'app' and 'nodeGraph' badges are selected, only one mode is displayed. Modify the conditional logic to treat multiple selected badges as a union by combining the conditions with OR logic instead of separate if statements - return true if (modeBadges includes 'app' AND t.isApp) OR (modeBadges includes 'nodeGraph' AND !t.isApp), ensuring that when both badges are selected, items matching either criterion are included in the results.site/src/components/hub/SearchPopover.vue (1)
549-561:⚠️ Potential issue | 🟠 MajorVerify thumbnail normalization cannot double-prefix local paths.
Lines 549-550 and line 560 always route thumbnails through
thumbnailPath(file), but the function only guards against full HTTP/HTTPS URLs. If upstream data contains an already-prefixed path like/workflows/thumbnails/...orworkflows/thumbnails/..., this produces broken doubled paths.The same vulnerability exists in
ThumbnailDisplay.astro(line 44) with an identicalthumbUrl()helper.Fix: Expand the URL check in both
thumbnailPath()andthumbUrl()to reject paths that already start with/workflows/thumbnails/or contain that segment:export const thumbnailPath = (asset: string) => asset.startsWith('http://') || asset.startsWith('https://') || asset.startsWith('/workflows/thumbnails/') ? asset : `/workflows/thumbnails/${asset}`;🤖 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 `@site/src/components/hub/SearchPopover.vue` around lines 549 - 561, The `thumbnailPath()` function in SearchPopover.vue (used in lines 549-550 and 560) and the identical `thumbUrl()` helper in ThumbnailDisplay.astro (line 44) only guard against full HTTP/HTTPS URLs but fail to prevent double-prefixing when paths already contain the `/workflows/thumbnails/` segment. Expand the URL check in both functions to also reject paths that already start with `/workflows/thumbnails/` by adding an additional condition to the existing startsWith checks, so that upstream data containing already-prefixed paths will be returned as-is instead of being double-prefixed.
🤖 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 `@site/src/components/hub/nav/MobileMenu.vue`:
- Around line 76-89: The watcher for the `open` property has a race condition
where if `open` changes before `await nextTick()` resolves, the stale branch can
still execute the focus and event listener attachment. Guard against stale
callbacks by adding a check after the `await nextTick()` in the isOpen branch to
verify that `isOpen` is still true before proceeding with the focus() call and
addEventListener('keydown', trapFocus) call. This ensures that if the menu was
closed while waiting for the next tick, the stale focus and event handling code
won't execute.
- Line 44: The template ref variable menuRef is initialized with ref<HTMLElement
| undefined>() but should follow the repository's convention of using
ref<HTMLElement | null>(null) for DOM element refs. Update the menuRef
declaration to change the type from HTMLElement | undefined to HTMLElement |
null, and explicitly initialize it with null as the default value to maintain
consistency with other template refs in the codebase like SearchPopover.vue and
HubWorkflowCard.vue.
In `@site/src/components/hub/nav/NavDesktopLink.vue`:
- Around line 48-50: The active navigation state detection in NavDesktopLink.vue
is comparing currentPath (a pathname) directly against href values (which may be
absolute URLs), causing the comparison to fail. Extract the pathname from the
href values before comparing them with currentPath to ensure the string equality
check works correctly. Apply this normalization to all three locations where
this comparison occurs: the ternary operator around line 48-50, the check around
line 66-68, and the condition around line 79-84.
In `@site/src/components/hub/nav/SiteNav.vue`:
- Around line 114-117: The closeMobileMenu function is forcing focus on the
hamburger element every time it is called, and since onNavigate calls
closeMobileMenu on every astro:after-swap route transition, this steals focus
from the user during normal navigation. Remove the hamburgerRef.value?.focus()
call from the closeMobileMenu function to prevent unwanted focus changes during
route transitions. If hamburger focus is needed for keyboard navigation
scenarios, consider conditionally focusing only when the menu is closed via
explicit user interaction rather than during automatic transitions.
- Line 112: The hamburgerRef variable declaration uses HTMLButtonElement |
undefined as the type without initializing the ref, which does not align with
the project convention. Change the hamburgerRef declaration to use
HTMLButtonElement | null as the type and explicitly initialize it with null
value, matching the pattern used in other components like HubWorkflowCard.vue
and SearchPopover.vue for consistency across the codebase.
In `@site/src/composables/useFacets.ts`:
- Around line 52-55: The counting logic in the nested loop where field(t) is
iterated currently counts duplicate values multiple times if they appear in the
same template, causing incorrect rankings. Deduplicate the values returned by
field(t) before counting by converting them to a Set first, then iterate through
the deduplicated Set to increment the counts in the counts map, ensuring each
unique value is counted only once per template regardless of how many times it
appears in that template.
In `@site/src/lib/github.ts`:
- Around line 21-23: The `inflight` cache stores request entries but never
removes them, causing transient failures or null results to be cached
permanently, preventing future retries. After setting the request in the
`inflight` map using `inflight.set(key, request)`, attach a handler to the
request promise that removes the entry from the `inflight` map when the request
settles (using `.finally()` or `.then()` combined with `.catch()`), ensuring
that future calls can retry the request instead of returning the cached failed
result.
In `@site/src/lib/provider-logos.ts`:
- Around line 42-45: The substring matching logic in the loop iterating through
MODEL_TO_LOGO entries is too greedy because it uses includes() to check if the
lowercase input contains the lowercase key, which matches partial substrings
unintentionally (e.g., "Swan AI" matches "Wan"). Replace this loose substring
matching with a more precise approach that checks for exact word boundaries or
exact matches, such as by splitting the input name into words and checking if
any word exactly matches the key, rather than using a simple includes() check.
In `@site/src/lib/structured-data.ts`:
- Around line 23-27: The faqItems mapping in the mainEntity section includes
unsanitized user-provided strings (item.question and item.answer) that flow into
JSON-LD script sinks. These strings could contain `</script>` sequences that
break out of the script tag context and enable markup/script injection. Create
or use a safe serializer function that escapes these potentially dangerous
characters in item.question and item.answer before they are included in the
acceptedAnswer object's text field and the name field to prevent script-breakout
attacks.
In `@site/src/pages/`[locale]/workflows/[slug].astro:
- Around line 226-227: The detailRelated variable assignment at line 226-227
lacks error handling for the listRelatedWorkflows function call. Wrap the
listRelatedWorkflows invocation in a try-catch block to gracefully handle Hub
API failures, returning an empty array in the catch block instead of allowing
the error to propagate and crash the entire detail page. This ensures the detail
page remains functional even when fetching related workflows fails, since this
is a non-critical section.
- Around line 265-267: The JSON-LD payload in the script block with `detailFaq`
and `set:html` is vulnerable to script-breakout XSS because the stringified JSON
is injected directly without escaping. Fix this by escaping the JSON string
before passing it to `set:html`, specifically escaping the `<` character (or
`</` sequence) in the JSON.stringify(detailFaq) result to prevent content from
breaking out of the JSON-LD script block. Replace the direct JSON.stringify call
with a safe serialization that escapes angle brackets or use a JSON
serialization method that produces XSS-safe output.
In `@site/src/styles/global.css`:
- Around line 194-197: The multi-line CSS comment starting with "Compact variant
for small controls" is missing required whitespace before the closing comment
delimiter `*/`. To fix this, add a space before `*/` at the end of the comment
block where it currently reads `breakpoints.*/` so it becomes `breakpoints. */`.
This will satisfy the stylelint whitespace requirement for comment closing
delimiters.
In `@site/tests/unit/provider-logos.test.ts`:
- Around line 20-22: Add regression test cases to the getLogoPath test suite to
prevent false-positive logo matches from short substring collisions. After the
existing test that checks for unknown providers using 'Totally Unknown Co', add
additional test cases that verify inputs like 'Swan AI' and 'Conflux Labs'
return null to ensure these provider names don't accidentally match against
shorter keys like 'Wan' or 'Flux' through substring matching.
In `@site/tests/unit/use-facets.test.ts`:
- Around line 34-40: The test for the useFacets function with the test case
"ranks tags by template count and counts each template once per tag" does not
actually verify the "once per template" behavior because the makeTemplates()
fixture never includes duplicate tags within the same template. Modify the test
fixture passed to useFacets (either by updating makeTemplates() or creating a
new specialized fixture) to include at least one template that contains
duplicate occurrences of the same tag, then add assertions to verify that the
tag count still reflects only one count per template even when duplicates exist
within that template.
---
Outside diff comments:
In `@site/src/components/hub/HubBrowse.vue`:
- Around line 63-67: The filter logic in the modeBadges length check uses early
return statements that cause only the first matching condition to apply, so when
both 'app' and 'nodeGraph' badges are selected, only one mode is displayed.
Modify the conditional logic to treat multiple selected badges as a union by
combining the conditions with OR logic instead of separate if statements -
return true if (modeBadges includes 'app' AND t.isApp) OR (modeBadges includes
'nodeGraph' AND !t.isApp), ensuring that when both badges are selected, items
matching either criterion are included in the results.
In `@site/src/components/hub/SearchPopover.vue`:
- Around line 549-561: The `thumbnailPath()` function in SearchPopover.vue (used
in lines 549-550 and 560) and the identical `thumbUrl()` helper in
ThumbnailDisplay.astro (line 44) only guard against full HTTP/HTTPS URLs but
fail to prevent double-prefixing when paths already contain the
`/workflows/thumbnails/` segment. Expand the URL check in both functions to also
reject paths that already start with `/workflows/thumbnails/` by adding an
additional condition to the existing startsWith checks, so that upstream data
containing already-prefixed paths will be returned as-is instead of being
double-prefixed.
🪄 Autofix (Beta)
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: 2d07f80f-17a8-47b8-a38d-816d4f639fdc
⛔ Files ignored due to path filters (11)
site/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsite/public/brand/comfy-hub-logo.svgis excluded by!**/*.svgsite/public/icons/arrow-right.svgis excluded by!**/*.svgsite/public/icons/arrow-up-right.svgis excluded by!**/*.svgsite/public/icons/breadthumb.svgis excluded by!**/*.svgsite/public/icons/close.svgis excluded by!**/*.svgsite/public/icons/logomark.svgis excluded by!**/*.svgsite/public/icons/node-left.svgis excluded by!**/*.svgsite/public/icons/node-right.svgis excluded by!**/*.svgsite/public/icons/node-union.svgis excluded by!**/*.svgsite/public/icons/social/github.svgis excluded by!**/*.svg
📒 Files selected for processing (55)
site/package.jsonsite/scripts/lib/search/build-index.tssite/src/components/ThumbnailDisplay.astrosite/src/components/hub/BrowseToolbar.vuesite/src/components/hub/FeaturedCarousel.vuesite/src/components/hub/HubBrowse.vuesite/src/components/hub/HubHero.astrosite/src/components/hub/HubNavbar.astrosite/src/components/hub/HubWorkflowCard.vuesite/src/components/hub/MobileFilterDrawer.vuesite/src/components/hub/SearchPopover.vuesite/src/components/hub/WorkflowGrid.vuesite/src/components/hub/nav/GitHubStarBadge.vuesite/src/components/hub/nav/MobileMenu.vuesite/src/components/hub/nav/NavDesktopLink.vuesite/src/components/hub/nav/NodeBadge.vuesite/src/components/hub/nav/SiteNav.vuesite/src/components/template-detail/Breadcrumbs.astrosite/src/components/template-detail/ExpandableText.vuesite/src/components/template-detail/FaqAccordion.vuesite/src/components/template-detail/TemplateDetailPage.astrosite/src/components/ui/button/index.tssite/src/composables/scrollLock.tssite/src/composables/useFacets.tssite/src/composables/useHubStore.tssite/src/config/nav-routes.tssite/src/i18n/ui.tssite/src/lib/featured.tssite/src/lib/github.tssite/src/lib/hub-api.tssite/src/lib/provider-logos.tssite/src/lib/routes.tssite/src/lib/search-config.tssite/src/lib/search.tssite/src/lib/structured-data.tssite/src/lib/toolbar.tssite/src/pages/[locale]/workflows/[slug].astrosite/src/pages/[locale]/workflows/category/[type].astrosite/src/pages/[locale]/workflows/index.astrosite/src/pages/[locale]/workflows/model/[name].astrosite/src/pages/[locale]/workflows/tag/[tag].astrosite/src/pages/workflows/[slug].astrosite/src/pages/workflows/[username].astrosite/src/pages/workflows/category/[type].astrosite/src/pages/workflows/index.astrosite/src/pages/workflows/model/[name].astrosite/src/pages/workflows/tag/[tag].astrosite/src/styles/design-tokens.csssite/src/styles/global.csssite/tests/unit/featured.test.tssite/tests/unit/provider-logos.test.tssite/tests/unit/routes.test.tssite/tests/unit/search-config.test.tssite/tests/unit/use-facets.test.tssite/vitest.config.ts
💤 Files with no reviewable changes (1)
- site/src/components/hub/MobileFilterDrawer.vue
dante01yoon
reviewed
Jun 24, 2026
dante01yoon
left a comment
dante01yoon
left a comment
Contributor
There was a problem hiding this comment.
Strict review — ComfyHub redesign (read-only, not approving per request)
Reviewed at head 37f0634. I focused on the areas the description flags as risky (search, navbar, filter migration, structured-data escaping) and on the design-system alignment raised in the Hub-consolidation RFC thread. Verified against the actual current files (note: the earlier CodeRabbit pass references MobileMenu.vue/SiteNav.vue/NavDesktopLink.vue, which no longer exist — they were refactored into HeaderMain*.vue, so several of those comments are stale).
Primary ask (strategic, from the RFC / Slack thread)
The team agreed the Hub will be migrated into the main comfy.org website with shared design components. To get a soft landing (minimal churn at migration time), this PR should reuse the website's design tokens rather than defining a parallel Comfy palette/typography here. Left as an inline suggestion on design-tokens.css. This is the highest-leverage change to make before merge — happy to pair on which token set is canonical.
Blocking-class issues (inline)
issueSearch abbreviation expansion replaces the literal token, regressing exact-abbreviation title matches (Wan2.1 Alpha T2V,LTX-2 I2Vbecome unfindable byt2v/i2v). —search-config.ts:64issueGitHub stars overridethrowviolates the file's own "returns null on any error" contract and can crash the whole site build on a bad env value, because the only caller is un-guarded. —github.ts:70issueModel-page breadcrumb regressed (Models → /workflows/model/becameWorkflows → /workflows/), contradicting the "consistent across model pages" claim. —workflows/model/[name].astro:69(+ localized variant)
Questions / suggestions (inline)
questionActive-nav highlighting is inert: everynav-routeshref is an absolutehttp(s)URL, soisHrefActivealways returns false. —useCurrentPath.ts:26suggestiona11y: mobile icon-only inactive tabs have no accessible name. —BrowseToolbar.vue:101
What checked out well (verified, no action)
- Search does not drift index/query config (shared
tokenize/SEARCH_FIELDS/STORE_FIELDS), and the index-fetch cache does recover from a failed first fetch (indexPromiseis cleared on error) — both core claims hold. serializeJsonLdForScriptis a correct escaper (</>/&/U+2028/U+2029 →\uXXXX) and every changed inline JSON-LD site (FAQ, breadcrumb, softwareApp) routes through it;listRelatedWorkflowsis wrapped in try/catch →[].useFacetsdedups values per-template (new Set(field(t))) — fixes latent count inflation vs. the old drawer.provider-logos.tsuses word-boundary regex matching — fixes the oldincludes()substring bug (e.g.Swanno longer matcheswan).ExpandableTextkeeps full text in SSR DOM while collapsed (crawler-visible), disconnects itsResizeObserveron unmount, and honorsprefers-reduced-motion;FeaturedCarouseldestroys Embla/autoplay on unmount, handles RTL + 0 slides; scroll-lock ref-counts and restores correctly on unmount.
Non-blocking notes (not inline)
- CI:
Build Siteandbuild-and-test(168 tests) pass.previewfails only on a missingVERCEL_TOKEN(fork PR — secrets unavailable; the build itself completed).SEO Auditfails on a missingdist/in its own job.Lint & Formatis red but dominated by pre-existing parse errors in files this PR doesn't touch (BaseLayout.astroarguments/is:inline, and the[username].astroline-15 HTML comment that is unchanged frommain); 0 new ESLint errors are introduced. Worth confirming none of these gate the merge. - Pre-existing / out of scope: the dual-mode badge union bug at
HubBrowse.vue:63-67(selecting bothapp+nodeGraphshows only apps) that CodeRabbit flagged is byte-for-byte unchanged frommain— not introduced here, but a good follow-up. - Escaping scope gap: the co-located TechArticle/SoftwareApplication graph (same
data.title/data.description) still serializes raw via the unchangedSEOHead.astroJSON.stringify, so the "title/description-derived JSON-LD is escaped" invariant isn't fully true post-PR. Pre-existing path, but consider migrating SEOHead to the new escaper too. - nit Dead code:
mobileDrawerOpen/toggleMobileDrawer/closeMobileDrawerinuseHubStore.tshave no consumers afterMobileFilterDrawer.vuedeletion.formatStarCountrenders1000Kin the 999.5k–1M window.
Not approving — strict review only, as requested.
…ma/workflow_templates into feat/comfyhub-redesign
…gration Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dante01yoon
approved these changes
Jun 24, 2026
dante01yoon
left a comment
dante01yoon
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed at head 13fba4f (read-only). Strong, well-documented redesign. I re-verified the substantive CodeRabbit findings against the current head and they are all resolved, and I confirmed the red CI checks are fork-PR infrastructure failures rather than code problems. Findings below are non-blocking nits/suggestions, so approving.
CI red checks are infra, not code — worth noting so they aren't mistaken for blockers:
Lint & Format: ESLint and Prettier both passed; the job only failed at the Comment on PR step with403 Resource not accessible by integration(read-only token on a fork PR).SEO Audit: Sitemap Validation and SEO Audit both passed; same 403 on the comment step.preview: the Vercel deploy step needsVERCEL_TOKEN, which isn't exposed to fork PRs.- The real quality gates —
Build Site,build-and-test, CodeRabbit, Socket Security, CLA — are green.
CodeRabbit findings re-verified as resolved on this head:
- JSON-LD XSS: every workflow route page now serializes via
serializeJsonLdForScript(escapes</>/&/U+2028-9); the only remaining rawJSON.stringifyis in unchangedSEOHead.astro, out of scope. github.tsinflight cache now clears via.finally(() => inflight.delete(key))— no permanent caching of failures.provider-logos.tsuses word-boundary regex, soSwan AIno longer matchesWan.useFacets.tsdedups per-template values withnew Set(field(t))before counting.listRelatedWorkflowsalready wraps its fetch in try/catch returning[], so the detail page can't crash on a Hub API failure.- The nav was rewritten (
MobileMenu.vue/NavDesktopLink.vue/SiteNav.vue->HeaderMain*/NavColumn), so CodeRabbit's nav comments are stale/moot. - The dual-mode (
app+nodeGraph) first-match-wins filter CodeRabbit flagged is unchanged pre-existing code and currently unreachable (the discovery panel that offers a second mode chip hides once any badge is active), so it isn't introduced here.
Verified: all t('...') keys referenced by new code resolve in ui.ts (a missing key would render the literal key); FAQ JSON-LD and the visible FaqAccordion share the same faqItems source; the breadcrumb JSON-LD mirrors the visible Breadcrumbs.astro trail in label and order.
Not run: static review + CI log analysis only — I did not run the app or re-run the build/tests locally, and did not verify visual/Figma fidelity.
dante01yoon
reviewed
Jun 24, 2026
dante01yoon
left a comment
dante01yoon
left a comment
Contributor
There was a problem hiding this comment.
Re-review at 13fba4f — all 6 prior findings verified resolved ✅
Thanks for the fast turnaround, @MaanilVerma. I re-reviewed the follow-up commits since my last pass (37f0634..13fba4f) and adversarially verified each fix (rebuilt the MiniSearch index with the current shared config to test queries empirically, ran the unit suites, and grepped for dangling references/tokens). Every prior finding is genuinely resolved:
| Prior finding | Fix | Verified |
|---|---|---|
issue search abbrev regression |
searchWorkflows now searches the literal query first, then appends the expansion (deduped); expandQuery returns null when nothing expands |
✅ t2v→ returns Wan2.1 Alpha T2V literal and the expansion, literal ranked first; new test asserts ordering; 58/58 pass |
issue GitHub override throw |
console.warn + return undefined (falls through to live fetch) |
✅ every invalid input returns instead of throwing; new github.test.ts 4/4 pass; contract upheld |
issue Models breadcrumb |
section override restores Models → /workflows/model/ in both default + localized pages |
✅ localized for all 11 locales, no raw-key leak (see one non-blocking note below) |
question inert active-nav |
useCurrentPath.ts deleted, all consumers cleaned |
✅ zero dangling refs; NavigationMenuTrigger.active is optional so the contract holds |
suggestion design tokens |
--text-2xs/3xs, --color-site-dropdown, ppformula-text-center tokens added; text-[10px]/[12px] removed |
✅ no dangling token refs; 0.625rem=10px exact (no visual regression); good step toward the migration soft-landing |
suggestion tab a11y |
:aria-label="labels[tab.labelKey]" on TabsTrigger |
✅ localized name now always present |
Not approving (review-only, as before). Two small non-blocking follow-ups inline — neither blocks merge.
Verification notes
- Build/lint sanity sweep of the follow-up commits found no dangling imports, unused-var lint failures, or undefined token classes.
- The named search templates (
Wan2.1 Alpha T2V,LTX-2 I2V) are only findable if present in the hub-API search index (it indexes hub workflows, nottemplates/index.json) — the fix is correct for any indexed doc carrying a literal abbreviation regardless.
| { name: modelName }, | ||
| { | ||
| name: t('labels.models', locale), | ||
| item: absoluteUrl(localizeUrl('/workflows/model/', locale)), |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking, follow-up to my earlier breadcrumb finding): restoring the Models crumb is exactly right and resolves the regression. One thing I missed in the original review — this crumb's item URL /workflows/model/ doesn't resolve to a page: there's only [name].astro here (no index.astro) and no redirect, so the bare path is a 404. Note this is not introduced by this PR — main pointed the same crumb at /workflows/model/ too, so it's a pre-existing latent issue, and there's no user-facing broken link (only the JSON-LD BreadcrumbList, no visible trail on model pages).
Still, Google expects breadcrumb item URLs to resolve, so while we're touching this line it'd be cheap to either drop the item (schema.org allows a name-only crumb — same as the leaf crumbs on the category/tag pages) or add a /workflows/model/ index. Same line in the localized variant [locale]/workflows/model/[name].astro:75.
Author
There was a problem hiding this comment.
@dante01yoon This is actually addressed in a follow-up PR that comes right after this one is merged (and I don't think we autodeploy). In that follow-up PR, we've introduced /models as its own page along with several SEO-related updates, so this is already taken care of there.
Contributor
|
🚀 Preview deployed for this PR (head https://workflow-templates-h3x07jaod-comfyui.vercel.app Deployed manually by a maintainer as a one-off preview. Note: built without the preview API env vars, so data-driven sections may render empty — the comfyhub redesign UI itself is fully viewable. Going forward, fork PRs will get automatic previews once a maintainer adds the |
Contributor
|
Verify actual CI passed - failures happen due to token issue |
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
2 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
End-to-end visual redesign of the ComfyHub site to the new Figma direction — a real marketing navbar, a much stronger search, a single browse toolbar replacing the sidebar/drawer filters, a featured carousel, and reworked cards and detail page. SEO and i18n are preserved across all 11 locales; the whole hub now reads as one consistent unit instead of a basic filtered grid.
Figma Link
Changes
What
flux_image-to-videois found by "flux"/"image"/"video"; common abbreviations expand at query time ("txt2img" → text to image, "i2v" → image to video, "cn" → controlnet); fuzziness scales with term length so short queries stay tight and long typos still match; multi-word queries match all terms first and only fall back to any-term when that's empty. Real usage counts now feed ranking, so popular workflows surface.<form role="search">, Enter activates the highlighted (or first) result with no page reload, and a live region announces the result count.Breaking
None. Protected infra is restyled around, never removed — SEOHead/HreflangTags, canonical
{name}-{shareId}links,getCloudCtaUrl()/UTM params, and<Analytics />are all intact, and the new JSON-LD matches what's rendered.Review Focus
Testing
Search is the part most likely to regress silently, so it has the most coverage; the rest is verified by the type/lint gate plus manual passes against the Figma.
tests/unit/search-config.test.ts— tokenizer sub-parts, synonym expansion, length-aware fuzzy thresholds, and the AND→OR fallback (including the title-over-description ranking).tests/unit/use-facets.test.ts— facet derivation and dedup from the template set.tests/unit/featured.test.ts/tests/unit/provider-logos.test.ts/tests/unit/routes.test.ts— usage-ranked selection, word-boundary logo matching, and canonical slug composition.pnpm run check(0 errors),pnpm test(168 passing),pnpm run lint,pnpm run buildall green.pnpm run dev): searchtxt2img/i2v/cnandflux; block the firstsearch-index.jsonrequest once and search again to confirm it recovers; open a desktop dropdown and the mobile sheet drill-down; scroll a card's tag row via the chevrons; spot-check a/zh/…and the RTL/ar/…page for i18n + dir.Screen Recording
Screen.Recording.2026-06-23.at.8.38.49.PM.mov