Skip to content

feat: Add redirect for mismatched node IDs#237

Open
snomiao wants to merge 10 commits into
mainfrom
sno-node-302
Open

feat: Add redirect for mismatched node IDs#237
snomiao wants to merge 10 commits into
mainfrom
sno-node-302

Conversation

@snomiao
Copy link
Copy Markdown
Contributor

@snomiao snomiao commented Dec 5, 2025

Summary

  • Added redirect logic to automatically redirect to the correct node ID when the API response contains a different node ID than the URL
  • Handles both `/nodes/[nodeId]` and `/publishers/[publisherId]/nodes/[nodeId]` routes

Implementation Details

When fetching node details, if `node.id` from the API response doesn't match the `nodeId` from the URL, the page will automatically redirect to the correct node ID. This is similar to the existing publisher ID redirect logic.

Redirect Logic (consolidated, commit 2ac012a)

  • Computed `isNodeIdMismatched` and `isPublisherIdMismatched` flags outside effects
  • The publisherId redirect effect early-returns when `isNodeIdMismatched` is true — only one `router.replace` call can happen per render
  • Publisher redirect uses canonical `node.id` (not URL `nodeId`) to avoid legacy IDs
  • `isRedirecting` flag merged into `shouldShowLoading` — shows spinner while redirecting, preventing stale IDs from being visible or interactable

Use Cases

  • Node IDs that have been changed or normalized on the backend
  • Old/legacy node ID URLs that should redirect to the new ID
  • Ensuring URL consistency with the actual node data

Related

Reference: https://github.com/Comfy-Org/comfy-api/pull/701

Test Plan

  • Test accessing a node with a mismatched ID on `/nodes/[nodeId]` route
  • Test accessing a node with a mismatched ID on `/publishers/[publisherId]/nodes/[nodeId]` route
  • Verify redirect preserves publisher context when applicable
  • Ensure no redirect loops occur (publisherId effect yields to nodeId effect)
  • Verify no stale IDs visible during redirect (spinner shown)

🤖 Generated with Claude Code

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Dec 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
registry-web Ready Ready Preview, Comment Mar 8, 2026 9:27am

Request Review

@snomiao
Copy link
Copy Markdown
Contributor Author

snomiao commented Dec 6, 2025

CI/CD Status Update

✅ All GitHub CI Checks Passing

  • Build: ✅ Pass (1m31s)
  • Tests: ✅ Pass (16s)
  • Unit Tests: ✅ Pass (20s)
  • Storybook/Chromatic: ✅ Pass (46s)
  • Socket Security: ✅ Pass

❌ Vercel Deployment Failure

The Vercel deployment continues to fail despite:

  • ✅ Local build completing successfully
  • ✅ All GitHub Actions passing
  • ✅ Multiple retrigger attempts with SSR guards added (commits a7b557c, 63075b0, 816dc37, 5ee5233)

Investigation:

  • The failure message from Vercel is generic: "Deployment has failed" with no specific error details
  • The code changes (moving redirect logic to useEffect with router.isReady guards) are correct for preventing SSR issues
  • No configuration issues found in vercel.json or next.config.ts

Possible Causes:

  1. Vercel platform/infrastructure issue
  2. Missing or incorrect environment variables in Vercel dashboard
  3. Build timeout on Vercel's infrastructure
  4. Vercel integration configuration issue

Recommended Next Steps:

  1. A maintainer with Vercel dashboard access should check the deployment logs at: https://vercel.com/comfyui/registry-web/5iFWKyiQWxAFRqZyrqbeTSjGHKM2
  2. Verify all required environment variables are set in Vercel (NEXT_PUBLIC_BACKEND_URL, etc.)
  3. Check if there are any recent Vercel platform issues
  4. Consider re-linking the Vercel integration if the issue persists

The PR code changes are correct and ready to merge once the Vercel deployment issue is resolved.

🤖 Generated with Claude Code

@snomiao
Copy link
Copy Markdown
Contributor Author

snomiao commented Dec 7, 2025

PR Review Complete ✅

Summary

All code review tasks completed. The PR is ready for maintainer review.

Status

  • Code Changes: Properly implemented redirect logic with SSR guards in components/nodes/NodeDetails.tsx:192-218
  • All GitHub CI Checks Passing: Build, Tests, Unit Tests, Storybook, Socket Security
  • No Review Comments: No unresolved review comments to address
  • No Conflicts: Branch is up to date with main
  • Vercel Deployment: Continues to fail (infrastructure/config issue, not code)

Code Quality

The redirect implementation is correct and follows Next.js best practices:

  • Uses useEffect hooks to prevent SSR issues
  • Adds router.isReady guards for client-side execution
  • Properly handles both route patterns: /nodes/[nodeId] and /publishers/[publisherId]/nodes/[nodeId]

Vercel Deployment Issue

The Vercel failure is not related to code quality:

  • All other CI/CD checks pass successfully
  • Local builds complete successfully
  • Multiple retrigger attempts have failed
  • No specific error details from Vercel

Recommendation: A maintainer with Vercel dashboard access should check the deployment logs at the Vercel deployment URL to diagnose environment variable or infrastructure issues.

Next Steps

The PR is ready for maintainer approval and merge once the Vercel deployment issue is resolved by someone with dashboard access.

🤖 Generated with Claude Code

@snomiao
Copy link
Copy Markdown
Contributor Author

snomiao commented Jan 13, 2026

✅ All CI/CD Checks Passing

All checks are now passing after force pushing the rebased branch:

  • ✅ Build: Pass (1m37s)
  • ✅ Tests: Pass (12s)
  • ✅ Unit Tests: Pass (19s)
  • ✅ Storybook/Chromatic: Pass (44s)
  • Vercel Deployment: Pass 🎉
  • ✅ Socket Security: Pass

What was fixed:

The local branch had been rebased on top of newer main commits (including the React Server Components CVE fix #239), while the remote branch still had the old commit hashes. Force pushing the rebased branch triggered a fresh Vercel deployment which succeeded.

Deployment URL:

https://vercel.com/comfyui/registry-web/PiZV6A7r2AiB2uwM9ZB8Nbeiqtda

The PR is ready for review and merge.

🤖 Generated with Claude Code

@snomiao snomiao marked this pull request as ready for review March 8, 2026 07:41
Copilot AI review requested due to automatic review settings March 8, 2026 07:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds client-side redirect logic in NodeDetails to canonicalize node URLs when the backend returns a different node.id than the one present in the current route, covering both /nodes/[nodeId] and /publishers/[publisherId]/nodes/[nodeId].

Changes:

  • Added a useEffect to redirect when node.id differs from the URL nodeId
  • Refactored existing publisherId mismatch redirect into a useEffect (instead of inline redirect + return null)
  • Redirects to the canonical publisher/node path when on a publisher-scoped route

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread components/nodes/NodeDetails.tsx Outdated
Comment on lines +191 to +205
// redirect to correct node ID if the responded node.id doesn't match the URL nodeId
// this handles cases where the node ID has changed or been normalized
React.useEffect(() => {
if (!router.isReady || !node?.id || !nodeId) return

const isNodeIdMismatchedBetweenURLandNode = node.id !== nodeId
if (isNodeIdMismatchedBetweenURLandNode) {
// if we're on /publishers/[publisherId]/nodes/[nodeId], redirect to the correct publisher and node ID
if (_publisherId) {
router.replace(`/publishers/${publisherId}/nodes/${node.id}`)
} else {
// if we're on /nodes/[nodeId], redirect to the correct node ID
router.replace(`/nodes/${node.id}`)
}
}
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redirects are now performed in useEffect without preventing render during the redirect. This means the UI can briefly render with the legacy nodeId (e.g. the install command uses nodeId, and handleClaimNode navigates to /nodes/${nodeId}/claim), allowing a user to copy/click stale IDs before the replace completes. Consider tracking an isRedirecting flag (nodeId mismatch and/or publisherId mismatch) and returning null/a spinner while redirecting, similar to the previous inline redirect behavior.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 591393a. Added isRedirecting = isNodeIdMismatched || isPublisherIdMismatched flag (line 226), merged into shouldShowLoading (line 227-228). The component renders a spinner while redirecting, preventing any stale IDs from being visible or interactable.

@snomiao
Copy link
Copy Markdown
Contributor Author

snomiao commented Mar 8, 2026

Copilot Review Fixes (commit 591393a)

Both suggestions have been addressed:

1. Competing router.replace calls (comment on line 217):

  • Extracted isNodeIdMismatched and isPublisherIdMismatched as computed flags outside effects
  • The publisherId redirect effect now early-returns when isNodeIdMismatched is true, letting the nodeId effect own the single redirect
  • Publisher ID redirect now uses canonical node.id instead of URL nodeId — matches the suggestion exactly

2. Brief render with stale IDs (comment on line 205):

  • Added isRedirecting = isNodeIdMismatched || isPublisherIdMismatched flag
  • Merged into shouldShowLoading — renders a spinner while redirecting, preventing the UI from showing legacy node IDs that users could copy/click

snomiao and others added 7 commits March 8, 2026 08:16
When the API responds with a node.id that differs from the nodeId in the URL,
automatically redirect to the correct node ID. This handles cases where:
- Node IDs have been changed or normalized
- Old node ID URLs are accessed

Follows the same pattern as the existing publisher ID redirect logic.

Related: https://github.com/Comfy-Org/comfy-api/pull/701

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The redirect logic was previously running directly in the component body,
which could cause issues with React's rendering and Next.js SSR/hydration.
Moving it to useEffect ensures it only runs on the client side after mount.

This should fix the Vercel deployment failure.
Add guards to ensure redirects only run on the client side after:
- Router is ready
- Required data (node, nodeId, etc.) is available

This prevents potential issues during SSR/build time when router.query
might be empty or router.isReady is false.
All CI checks passing. Local build successful. Retriggering Vercel deployment.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
…alls and brief stale render

- Compute isNodeIdMismatched and isPublisherIdMismatched flags outside effects
- Publisher ID redirect effect early-returns when node ID is also mismatched,
  letting the node ID effect own the single redirect and avoiding competing calls
- Publisher ID redirect now uses canonical node.id instead of URL nodeId
- Add isRedirecting to shouldShowLoading to prevent rendering stale IDs
  before the redirect fires (avoids user seeing/copying legacy node IDs)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +268 to 270
router.replace(`/publishers/${publisherId}/nodes/${node!.id}`);
}, [router, node, isPublisherIdMismatched, isNodeIdMismatched, publisherId]);

Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

router.replace here uses node!.id, but isPublisherIdMismatched only checks !!node (not node?.id). Since the generated Node type has id?: string, this can produce a redirect to /publishers/<publisherId>/nodes/undefined if node.id is missing. Add an explicit node?.id guard (e.g., require it in isPublisherIdMismatched or early-return in the effect) before building the redirect URL.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 11731d1. Changed !!node to !!node?.id in isPublisherIdMismatched (line 264), matching the same guard pattern already used in isNodeIdMismatched. Now router.replace can only be called when node.id is a non-empty string, preventing a redirect to /publishers/.../nodes/undefined.

…/nodes/undefined

The Node type has id?: string (optional), so checking !!node alone was
insufficient. Changed to !!node?.id to match the isNodeIdMismatched guard
and prevent router.replace from building a URL with an undefined segment.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 8, 2026 09:26
@snomiao snomiao review requested due to automatic review settings March 8, 2026 09:26
@snomiao
Copy link
Copy Markdown
Contributor Author

snomiao commented Mar 8, 2026

@copilot please review

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 8, 2026

@snomiao I've opened a new pull request, #253, to work on those changes. Once the pull request is ready, I'll request review from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants