Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 8, 2026

TL;DR

Refactored the ContextSwitcher component to improve code organization and fix a naming inconsistency.

What changed?

  • Extracted the inner functionality of ContextSwitcher into a new ContextSwitcherInner component
  • Renamed useContextSwitchMatch to useContextSwitcherMatch for consistency
  • Simplified conditional logic by passing the organization as a prop to the inner component
  • Removed unnecessary type checking and conditional expressions

How to test?

  1. Navigate to pages that use the context switcher
  2. Verify the context switcher appears correctly
  3. Click on the context switcher to open the dropdown
  4. Confirm that all functionality works as expected

Why make this change?

This refactoring improves code readability and maintainability by:

  • Creating a clearer separation of concerns between components
  • Fixing inconsistent naming conventions
  • Simplifying conditional logic that was unnecessarily complex
  • Making the code more predictable by using props instead of accessing context data in multiple places

@railway-app
Copy link

railway-app bot commented Jan 8, 2026

🚅 Deployed to the rivet-pr-3792 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud-prod 🕒 Building (View Logs) Web Jan 8, 2026 at 3:07 pm
website ✅ Success (View Logs) Web Jan 8, 2026 at 3:07 pm
frontend-cloud-staging 🕒 Building (View Logs) Web Jan 8, 2026 at 3:07 pm
frontend-inspector 🕒 Building (View Logs) Web Jan 8, 2026 at 3:06 pm

@jog1t jog1t marked this pull request as ready for review January 8, 2026 15:06
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3792 January 8, 2026 15:06 Destroyed
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 8, 2026

Merge activity

  • Jan 8, 3:06 PM UTC: jog1t added this pull request to the Graphite merge queue.
  • Jan 8, 3:07 PM UTC: CI is running for this pull request on a draft pull request (#3793) due to your merge queue CI optimization settings.
  • Jan 8, 3:08 PM UTC: Merged by the Graphite merge queue via draft PR: #3793.

Copy link
Contributor Author

jog1t commented Jan 8, 2026


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

graphite-app bot pushed a commit that referenced this pull request Jan 8, 2026
…3792)

### TL;DR

Refactored the ContextSwitcher component to improve code organization and fix a naming inconsistency.

### What changed?

- Extracted the inner functionality of `ContextSwitcher` into a new `ContextSwitcherInner` component
- Renamed `useContextSwitchMatch` to `useContextSwitcherMatch` for consistency
- Simplified conditional logic by passing the organization as a prop to the inner component
- Removed unnecessary type checking and conditional expressions

### How to test?

1. Navigate to pages that use the context switcher
2. Verify the context switcher appears correctly
3. Click on the context switcher to open the dropdown
4. Confirm that all functionality works as expected

### Why make this change?

This refactoring improves code readability and maintainability by:
- Creating a clearer separation of concerns between components
- Fixing inconsistent naming conventions
- Simplifying conditional logic that was unnecessarily complex
- Making the code more predictable by using props instead of accessing context data in multiple places
@claude
Copy link

claude bot commented Jan 8, 2026

Pull Request Review

Summary

This PR refactors the ContextSwitcher component to improve code organization and fix a naming inconsistency. The changes are well-executed with one key improvement:

Key improvement: The prefetch query now only runs when on organization routes, preventing unnecessary data fetching on non-org pages.


✅ Strengths

  1. Better separation of concerns: Extracting ContextSwitcherInner creates a clearer component hierarchy
  2. Consistent naming: useContextSwitchMatchuseContextSwitcherMatch aligns with the component name
  3. Simplified conditional logic: Removed complex nested type checking (typeof match === "object" && "organization" in match)
  4. Performance optimization: The usePrefetchInfiniteQuery now only runs when the component is actually rendered (i.e., when on an org route), preventing unnecessary prefetching on non-org pages
  5. Cleaner prop passing: Organization is now explicitly passed as a prop rather than accessed from match context in multiple places

🔍 Code Quality Observations

TypeScript & React Patterns

  • Good: Early return pattern in ContextSwitcher (frontend/src/app/context-switcher.tsx:32-34)
  • Good: Proper TypeScript typing with union types for useContextSwitcherMatch (frontend/src/app/context-switcher.tsx:68-75)
  • Good: Component composition follows React best practices

Logic Correctness

The refactor maintains functional equivalence with a key improvement:

Before:

usePrefetchInfiniteQuery({
  enabled: typeof match === "object" && "organization" in match,
  ...projectsQueryOptions({ organization: /* complex conditional */ })
});

After:

// Only rendered when match is truthy (frontend/src/app/context-switcher.tsx:36)
usePrefetchInfiniteQuery({
  ...projectsQueryOptions({ organization }) // Always has valid org
});

This is actually an improvement because:

  1. The prefetch only runs when the component is mounted (on org routes)
  2. No need for the enabled flag since the component won't exist on non-org pages
  3. Cleaner and more predictable behavior

🎯 Testing Recommendations

Based on the PR description's test plan, I'd suggest verifying:

  1. ✅ Context switcher appears correctly on org routes
  2. ✅ Context switcher does NOT appear on non-org routes
  3. ✅ Dropdown functionality works as expected
  4. ✅ Network tab shows prefetch queries only trigger on org pages (this is the key improvement)
  5. ✅ Navigation between projects/namespaces works correctly
  6. ✅ Breadcrumb display is correct for both project and namespace views

📊 Final Assessment

Code Quality: ⭐⭐⭐⭐⭐ Excellent
Bug Risk: ⭐⭐⭐⭐⭐ Very Low
Performance: ⭐⭐⭐⭐⭐ Improved (reduced unnecessary prefetching)
Maintainability: ⭐⭐⭐⭐⭐ Improved


✨ Recommendation

APPROVE - This is a solid refactoring that improves code quality, maintainability, and performance. The changes are safe, well-structured, and the performance optimization (prefetching only on org routes) is a meaningful improvement.

No blocking issues found. Ready to merge after testing verification.

@graphite-app graphite-app bot closed this Jan 8, 2026
@graphite-app graphite-app bot deleted the 01-08-fix_frontend_prefetch_context_switcher_content_only_on_org_routes branch January 8, 2026 15:08
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 8, 2026

Graphite Automations

"Test" took an action on this PR • (01/08/26)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

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