Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request implements dynamic dashboard view routing based on feature flags, replacing the static topology dashboard default with a feature-driven approach. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as App.tsx
participant HomepageRedirect as HomepageRedirect
participant FeatureFlagsContext as FeatureFlagsContext
participant API as View API<br/>(getViewIdByName,<br/>getViewIdByNamespaceAndName)
participant Router as Router
participant Dashboard as Dashboard View/<br/>Health Page
User->>App: Load application
App->>HomepageRedirect: Navigate to "" (default route)
HomepageRedirect->>FeatureFlagsContext: Read DASHBOARD_VIEW_PROPERTY
alt Feature flag value is UUID
HomepageRedirect->>Router: Navigate directly to /views/{uuid}
else Feature flag value contains slash
HomepageRedirect->>API: getViewIdByNamespaceAndName(namespace, name)
API-->>HomepageRedirect: Return view ID
HomepageRedirect->>Router: Navigate to /views/{id}
else Feature flag value is plain name
HomepageRedirect->>API: getViewIdByName(name)
API-->>HomepageRedirect: Return view ID
HomepageRedirect->>Router: Navigate to /views/{id}
else No property or resolution fails
HomepageRedirect->>API: getViewIdByName("mission-control-dashboard")
API-->>HomepageRedirect: Return fallback view ID or undefined
alt Fallback view resolved
HomepageRedirect->>Router: Navigate to /views/{id}
else Fallback fails
HomepageRedirect->>Router: Navigate to /health
end
end
Router->>Dashboard: Render destination view
Dashboard-->>User: Display dashboard or health page
Suggested Reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/App.tsx`:
- Around line 1084-1102: findDashboardView currently looks only in the views
returned from getViewsForSidebar (which filters sidebar=eq.true) so a configured
dashboard with sidebar=false will not be found and thus not added to the sidebar
even though HomepageRedirect still redirects to it; update the logic in
findDashboardView (or the code that calls it) to, when the initial search in the
getViewsForSidebar results returns undefined, perform a secondary fetch for the
specific dashboard view (using the existing helpers getViewIdByName or
getViewIdByNamespaceAndName or a direct getViewById call) and return that result
so the dashboard nav item is added regardless of the sidebar flag, or
alternatively adjust the initial fetch to include sidebar values for the
configured dashboard lookup.
🧹 Nitpick comments (4)
src/components/HomepageRedirect.tsx (2)
19-25: Edge case: values with multiple slashes.
resolveViewIdsplits on"/"with a limit of 2, so for a value like"a/b/c", namespace becomes"a"and name becomes"b", silently dropping"c". If namespace/name values could legitimately contain slashes, consider splitting only on the first occurrence:const idx = value.indexOf("/"); const namespace = value.slice(0, idx); const name = value.slice(idx + 1);If namespace/name values never contain slashes, the current behavior is fine.
27-50: Consider guarding against unloaded feature flags.This component assumes
featureFlagsare already loaded when it renders (currently guaranteed by thefeatureFlagsLoadedguard inIncidentManagerRoutes). If that parent guard is ever removed or bypassed, the component would immediately resolve with an empty flags array and navigate to the fallback/health path before the actual configured view can be determined.A defensive check would make this component self-contained:
🛡️ Suggested defensive guard
export function HomepageRedirect() { - const { featureFlags } = useFeatureFlagsContext(); + const { featureFlags, featureFlagsLoaded } = useFeatureFlagsContext(); + + if (!featureFlagsLoaded) { + return <FullPageSkeletonLoader />; + } const dashboardViewValue = featureFlags.find(src/components/__tests__/HomepageRedirect.unit.test.tsx (1)
89-169: Good test coverage of the main resolution paths.The tests cover UUID, namespace/name, plain name, fallback, and error scenarios well.
One minor gap: there's no test for when namespace/name resolution specifically fails (e.g.,
mockedGetViewIdByNamespaceAndName.mockResolvedValue(undefined)with a"ns/name"flag value). The current tests only cover plain-name lookup failure (line 160). This is low-risk since the fallback path is the same, but adding it would complete the coverage matrix.src/App.tsx (1)
1104-1152: Minor duplication of feature-flag reading logic.The pattern to extract the dashboard view value from feature flags:
featureFlags.find((f) => f.name === DASHBOARD_VIEW_PROPERTY)?.valueis repeated in both
HomepageRedirect.tsxand here. Consider extracting a small utility (e.g.,getDashboardViewProperty(featureFlags)) intodashboardViewConstants.tsto keep the resolution logic centralized.Not blocking — the duplication is small and both usages are near-identical.
| function findDashboardView( | ||
| views: ViewSummary[], | ||
| dashboardViewValue?: string | ||
| ): ViewSummary | undefined { | ||
| if (!dashboardViewValue) { | ||
| return views.find((v) => v.name === FALLBACK_VIEW_NAME); | ||
| } | ||
|
|
||
| if (UUID_REGEX.test(dashboardViewValue)) { | ||
| return views.find((v) => v.id === dashboardViewValue); | ||
| } | ||
|
|
||
| if (dashboardViewValue.includes("/")) { | ||
| const [namespace, name] = dashboardViewValue.split("/", 2); | ||
| return views.find((v) => v.namespace === namespace && v.name === name); | ||
| } | ||
|
|
||
| return views.find((v) => v.name === dashboardViewValue); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find getViewsForSidebar definition
ast-grep --pattern 'export const getViewsForSidebar = $_'Repository: flanksource/flanksource-ui
Length of output: 599
🏁 Script executed:
# Also search for getViewsForSidebar more broadly
rg 'getViewsForSidebar' -A 5 -B 2Repository: flanksource/flanksource-ui
Length of output: 1472
🏁 Script executed:
# Find where findDashboardView is called
rg 'findDashboardView' -B 3 -A 3Repository: flanksource/flanksource-ui
Length of output: 564
🏁 Script executed:
# Find HomepageRedirect and getViewIdByName
rg 'HomepageRedirect|getViewIdByName' -B 3 -A 5Repository: flanksource/flanksource-ui
Length of output: 10534
🏁 Script executed:
rg 'export const getViewIdByNamespaceAndName' -A 8 src/api/services/views.tsRepository: flanksource/flanksource-ui
Length of output: 381
Dashboard view may not appear in sidebar if sidebar flag is false.
findDashboardView searches the views array returned by getViewsForSidebar, which filters on sidebar=eq.true. If the configured dashboard view has sidebar=false, it won't be found here, so no dashboard nav item will be added to the sidebar — even though HomepageRedirect will still redirect to it (since it queries all views via getViewIdByName or getViewIdByNamespaceAndName, both without the sidebar filter).
If the intent is that the dashboard view always appears in the sidebar nav regardless of its sidebar flag, this needs a separate fetch or an adjusted query. If the expectation is that the dashboard view must have sidebar=true, this is fine but worth documenting.
🤖 Prompt for AI Agents
In `@src/App.tsx` around lines 1084 - 1102, findDashboardView currently looks only
in the views returned from getViewsForSidebar (which filters sidebar=eq.true) so
a configured dashboard with sidebar=false will not be found and thus not added
to the sidebar even though HomepageRedirect still redirects to it; update the
logic in findDashboardView (or the code that calls it) to, when the initial
search in the getViewsForSidebar results returns undefined, perform a secondary
fetch for the specific dashboard view (using the existing helpers
getViewIdByName or getViewIdByNamespaceAndName or a direct getViewById call) and
return that result so the dashboard nav item is added regardless of the sidebar
flag, or alternatively adjust the initial fetch to include sidebar values for
the configured dashboard lookup.
2 participants
Fixes: #2848
Summary by CodeRabbit
New Features
Changes