Skip to content

Conversation

@dimaMachina
Copy link
Collaborator

image

@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2025

🦋 Changeset detected

Latest commit: c980d23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@inkeep/agents-manage-ui Patch
@inkeep/agents-manage-api Patch
@inkeep/agents-run-api Patch
@inkeep/agents-cli Patch
@inkeep/agents-core Patch
@inkeep/agents-manage-mcp Patch
@inkeep/agents-sdk Patch
@inkeep/ai-sdk-provider Patch
@inkeep/create-agents Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Dec 19, 2025

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

Project Deployment Review Updated (UTC)
agents-manage-api Error Error Dec 19, 2025 10:58am
agents-manage-ui Ready Ready Preview, Comment Dec 19, 2025 10:58am
agents-run-api Ready Ready Preview, Comment Dec 19, 2025 10:58am
1 Skipped Deployment
Project Deployment Review Updated (UTC)
agents-docs Skipped Skipped Dec 19, 2025 10:58am

@claude
Copy link

claude bot commented Dec 19, 2025

Claude finished @dimaMachina's task —— View job


PR Review: Fix Double Breadcrumbs

Analyzing the double breadcrumb issue and the proposed solution...

Todo List:

  • Examine the screenshot to understand the issue
  • Analyze all changed files to understand the fix approach
  • Review the breadcrumb component changes
  • Evaluate layout and loading state modifications
  • Provide comprehensive feedback and recommendations

Understanding the Issue

The screenshot clearly shows the problem: duplicate "Projects" breadcrumbs appearing in the navigation header on the /projects error page. This happens because:

  1. Previous Architecture: The projects index route had a nested layout structure:

    • projects/(index)/layout.tsx - Added BodyTemplate with empty breadcrumbs []
    • Breadcrumbs component automatically adds "Projects" for non-settings routes
    • FullPageError component adds its own breadcrumbs with "Error"
    • Result: Double "Projects" breadcrumb (one from auto-generation, one from layout)
  2. Root Cause: The Breadcrumbs component has logic that automatically prepends "Projects" to breadcrumbs for non-settings routes (lines 29-33 in breadcrumbs.tsx), but the layout was also wrapping content with BodyTemplate and empty breadcrumbs.

Review of the Solution

The fix takes a layout consolidation approach by removing the intermediate layout and moving breadcrumb responsibility directly to the page/error components:

✅ Positive Aspects:

  1. Clean Architecture: Eliminates the redundant layout layer that was causing the duplication
  2. Consistent Pattern: Both success and error cases now consistently handle their own breadcrumbs through BodyTemplate
  3. DRY Principle: Removes duplicate skeleton code by making loading.tsx reuse the [projectId]/loading.tsx component
  4. Settings Layout Fix: Corrects the TypeScript generic from '/[tenantId]/projects' to '/[tenantId]/settings' (good catch!)

⚠️ Areas for Consideration:

  1. Breadcrumb Logic Complexity: The automatic breadcrumb injection in Breadcrumbs component creates implicit behavior that's hard to reason about. Consider whether this auto-injection pattern is the best long-term approach.

  2. Error State Breadcrumbs: Currently, error pages show "Projects > Error" breadcrumbs. Consider whether this provides good UX - users might expect to see the full navigation context even in error states.

  3. Loading State Consistency: The loading states now properly wrap content in BodyTemplate, but worth verifying this doesn't create layout shift during the loading → content transition.

Architectural Feedback

Pattern Consistency: This fix establishes a pattern where pages are responsible for their own BodyTemplate wrapper rather than relying on layouts. This is generally good for flexibility, but consider documenting this pattern to ensure consistency across the app.

Auto-breadcrumb Logic: The Breadcrumbs component's automatic "Projects" injection (lines 29-41) is a bit of "magic" that could be confusing. Consider either:

  • Making this behavior more explicit through props
  • Or documenting this behavior clearly
  • Or moving this logic to a higher level so it's more visible

Type Safety: The settings layout fix shows good attention to TypeScript correctness. Consider adding runtime validation or more specific typing to prevent similar issues.

Testing Considerations

Since this touches navigation and loading states, ensure testing covers:

  • Error page breadcrumbs render correctly
  • No layout shift between loading and loaded states
  • Breadcrumb navigation links work correctly
  • Different error types (404, 500, etc.) show appropriate breadcrumbs

Minor Code Quality Notes

The changes are clean and well-structured. The code follows existing patterns and maintains good separation of concerns.

Verdict: This is a solid fix that addresses the immediate issue while improving the overall architecture. The consolidation approach is cleaner than trying to work around the auto-injection logic.

Recommendation: ✅ Approve with minor suggestion to consider documenting the breadcrumb auto-injection behavior for future maintainers.

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.

2 participants