Feat/comfyhub redesign#950
Open
MaanilVerma wants to merge 83 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 |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Author
|
Knit Picks Updated (It start from this Commit ID ) What we fixed:
Under the hood:
|
…tag, rename TagScroller to TagRow
…ma/workflow_templates into feat/comfyhub-redesign
MaanilVerma
added
the
preview
Contributor
|
🚀 Preview deployed: https://workflow-templates-9xscoj11g-comfyui.vercel.app Manual prebuilt Vercel deploy of this PR's head ( |
dante01yoon
approved these changes
Jun 25, 2026
dante01yoon
left a comment
dante01yoon
left a comment
Contributor
There was a problem hiding this comment.
Re-review of the Knit-Picks round since my prior approval (13fba4f1 → 0d6ec856). The incremental changes are clean polish — TagScroller→TagRow fit+overflow, stacked model logos, ButtonPill reveal try-now, sticky in-body search, RTL/reduced-motion fixes, restored field boost, focus-ring fix.
Verified locally on the PR head:
astro check: 0 errors / 0 warnings.- vitest: 213 passing (both changed suites —
search-config+30,provider-logos+29 — green). - No dangling refs after the renames/moves:
TagScroller, the oldtag-pillstestid, and the localCreatorEntryexport are all gone, withCreatorEntrynow sourced fromhub-apiand consumed only bySearchPopover. - FeaturedCarousel "fully clickable" is sound: a real full-slide
<a class="absolute inset-0 z-10">sits behind the now-pointer-events-noneoverlay, and the creator/tag children re-enablepointer-events-autowith@click.stop. - ButtonPill
revealself-hover works (group/button-pillis on the cva base) and the card path usesgroup/pill-trigger.
Note on red checks: Lint & Format, SEO Audit, and preview fail only at their final "Comment on PR"/deploy step with 403 Resource not accessible by integration (fork-PR token), not on real checks — ESLint ✅, Prettier ✅, Sitemap ✅, SEO Audit ✅ all passed. Matches the maintainer note.
Approving. Two non-blocking nits inline.
dante01yoon
approved these changes
Jun 25, 2026
dante01yoon
left a comment
dante01yoon
left a comment
Contributor
There was a problem hiding this comment.
Re-review of the latest round since my prior approval (0d6ec856 → 727d5c61). 5 files; both of my earlier nits are resolved and the navbar wordmark fix verified.
Nit follow-ups (both addressed):
buttonPillLabelVariantsremoved and inlined into ButtonPill.vue. The inline class set is byte-for-byte equivalent to the oldbuttonPillLabelVariants()default (open: false) output — same six utilities, only reordered — so the reveal animation is unchanged. No leftover references anywhere.page.waitForNavigation()→page.waitForURL(/\/workflows\/tag\/.+\//)using the same regex as the assertion below.astro checkdeprecation hints dropped 5 → 4, confirming the ts(6387) flag is gone.
Wordmark (SVG rebuild + HeaderMain sizing) — rendered to verify:
- The SVG renders cleanly as "Comfy HUB" with no doubled/overlapping glyphs (I'd flagged a possible overlap between the original path and the new
scale(0.6115)group — render confirms it's a single clean lockup). - Intrinsic 240×48 → at
h-10(40px) the image is 200px wide; new containermax-w-56(224px) fits it without clipping, where the oldw-44(176px) was cutting the HUB badge. Sizing math and the rendered in-context capture both confirm no clip.
Validation on 727d5c61: astro check 0 errors / 0 warnings; vitest 213 passing; CI ESLint ✅ / Prettier ✅ (Lint & Format / SEO Audit / preview still red only on the fork-token 403 Resource not accessible by integration comment/deploy step — not real failures).
No new findings. Approving.
…, drop dead variant
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