fix: View sections in config details page #2766
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughCentralizes view data fetching into a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as SingleView / ConfigDetailsViewPage
participant Hook as useViewData
participant Q as React-Query
participant API as Backend API
participant View as ViewWithSections
UI->>Hook: init(viewId[, configId])
Hook->>Q: start queries (viewResult, display-plugin vars?)
Q->>API: fetch view data / plugin vars
API-->>Q: return data
Q-->>Hook: query results (data/error/loading)
Hook->>Hook: aggregate variables (useAggregatedViewVariables)
Hook-->>UI: return viewResult, isLoading, isFetching, error, aggregatedVariables, currentVariables, handleForceRefresh
UI->>View: render with viewResult + variables
Note over UI,Hook: on user refresh
UI->>Hook: call handleForceRefresh()
Hook->>Q: refetch & invalidate dependent keys
Q->>API: refetch
API-->>Q: fresh data
Q-->>Hook: updated results -> UI re-renders
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
🧹 Nitpick comments (3)
src/pages/config/details/ConfigDetailsViewPage.tsx (2)
21-24: Non-null assertion onviewIdcould mask routing issues.If the route is misconfigured or the user navigates directly without a
viewId, this will passundefined!which becomes"undefined"string when coerced, potentially causing confusing API errors instead of a clear "missing parameter" state.Consider adding a guard or returning early if
viewIdis missing:+ if (!viewId) { + return <div>View ID is required</div>; + } + const { viewResult, isLoading, error, aggregatedVariables, currentVariables, handleForceRefresh } = useViewData({ - viewId: viewId!, + viewId, configId });
35-35: Remove emptyclassNameattribute.The empty string serves no purpose and adds noise.
- <div className=""> + <div>src/pages/views/hooks/useViewData.ts (1)
95-139: The query invalidation afterrefetch()may be redundant.On lines 121-125, you invalidate the same query key that was just refetched. Since
refetch()already updates the cache, this invalidation triggers an additional refetch. If this is intentional (e.g., to force dependent components to re-render), consider adding a comment explaining the rationale.Also, the error handling on lines 100-109 is thorough, but the type cast to
anycould be replaced with a more specific error type if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/pages/config/details/ConfigDetailsViewPage.tsx(1 hunks)src/pages/views/components/SingleView.tsx(3 hunks)src/pages/views/components/ViewWithSections.tsx(1 hunks)src/pages/views/hooks/useViewData.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/pages/views/hooks/useViewData.ts (3)
src/pages/audit-report/types/index.ts (3)
ViewResult(420-435)ViewVariable(382-389)ViewRef(409-412)src/api/services/views.ts (2)
getViewDisplayPluginVariables(140-148)getViewDataById(112-122)src/components/Toast/toast.ts (1)
toastError(15-25)
src/pages/views/components/ViewWithSections.tsx (3)
src/pages/audit-report/types/index.ts (3)
ViewResult(420-435)ViewVariable(382-389)ViewSection(414-418)src/pages/audit-report/components/View/GlobalFiltersForm.tsx (1)
GlobalFiltersForm(65-90)src/pages/views/constants.ts (1)
VIEW_VAR_PREFIX(2-2)
src/pages/config/details/ConfigDetailsViewPage.tsx (3)
src/pages/views/hooks/useViewData.ts (1)
useViewData(30-159)src/ui/Loading/index.tsx (1)
Loading(11-37)src/components/ErrorViewer.tsx (1)
ErrorViewer(68-115)
src/pages/views/components/SingleView.tsx (1)
src/pages/views/hooks/useViewData.ts (1)
useViewData(30-159)
🔇 Additional comments (10)
src/pages/config/details/ConfigDetailsViewPage.tsx (1)
36-58: LGTM!The conditional rendering logic properly handles all states (loading, error, data, and fallback), and the delegation to
ViewWithSectionswithhideVariablesis appropriate for the config details context.src/pages/views/components/ViewWithSections.tsx (3)
1-14: LGTM!Clean imports and well-defined props interface. The optional props (
aggregatedVariables,currentVariables,hideVariables) provide good flexibility for different usage contexts.
25-37: LGTM!The
isAggregatorViewdetermination correctly identifies views that only contain sections without their own content. TheprimaryViewSectionconstruction properly handles the optionalnamespacefield.
42-79: LGTM!The rendering logic correctly handles all cases:
- Global filters are shown only when appropriate
- Primary view section is skipped for aggregator views (which only contain sections)
- Section keys use the unique
namespace:namepatternhideVariablesis properly propagated to childViewSectioncomponents to prevent duplicate filter UIsrc/pages/views/hooks/useViewData.ts (3)
39-43: Consider handling the loading/error state fordisplayPluginVariables.The query for
displayPluginVariablesdoesn't expose its loading or error state. If this query fails or is slow, the main view query will remain disabled (sinceenabled: isDisplayPluginMode ? !!variables : !!viewId), potentially leaving the user in a perpetual loading state without feedback.Consider whether you need to surface loading/error states from this query:
- const { data: displayPluginVariables } = useQuery({ + const { data: displayPluginVariables, isLoading: isLoadingVariables, error: variablesError } = useQuery({ queryKey: ["viewDisplayPluginVariables", viewId, configId], queryFn: () => getViewDisplayPluginVariables(viewId, configId!), enabled: isDisplayPluginMode });
47-66: LGTM!The query configuration is well-structured:
- Distinct query keys for each mode prevent cache collisions
- The
enabledcondition properly gates fetching until dependencies are readystaleTimeof 5 minutes is reasonable for view datakeepPreviousDatais appropriate for v4 of React Query
150-158: LGTM!The return values correctly differentiate between display plugin mode (empty
aggregatedVariables, plugin-derivedcurrentVariables) and standard mode (aggregated variables from all sections).src/pages/views/components/SingleView.tsx (3)
1-6: LGTM!Clean imports. The migration to the new
useViewDatahook andViewWithSectionscomponent simplifies the component significantly while maintaining proper separation of concerns.
23-66: LGTM!The state handling is well-structured:
- Loading spinner only appears on initial load (not during background refetches)
- Error and not-found states provide appropriate feedback
- All states include the refresh action for recovery
68-93: LGTM!The main render correctly passes all necessary props to
ViewWithSections. Notably,hideVariablesis not passed here (unlike inConfigDetailsViewPage), so global filters will be displayed—which is the expected behavior for the standalone view page.
adityathebe
force-pushed
the
fix/view-sections-on-config-page
branch
from
9f2f3a8 to
c5708d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/pages/config/details/ConfigDetailsViewPage.tsx (1)
21-24: Consider guarding against undefinedviewId.The non-null assertion
viewId!could cause issues if the route is accessed without aviewIdparam. WhileuseViewDatahas internalenabledchecks, consider adding an early return or explicit guard for better clarity and to prevent potential edge-case issues.+ if (!viewId) { + return ( + <ConfigDetailsTabs pageTitlePrefix="Config View" activeTabName=""> + <div className="flex flex-1 flex-col items-center justify-center"> + <div className="text-center text-gray-500">View ID is required</div> + </div> + </ConfigDetailsTabs> + ); + } + const { viewResult, isLoading, error, aggregatedVariables, currentVariables, handleForceRefresh } = useViewData({ - viewId: viewId!, + viewId, configId });src/pages/views/components/ViewWithSections.tsx (1)
66-77: Inconsistent key fallback for undefined namespace.Line 59 uses
namespace || "default"for the primary section key, but line 70 usessection.viewRef.namespacedirectly without a fallback. Ifnamespaceisundefined, the key becomes"undefined:name". Consider using a consistent fallback pattern.{viewResult.sections.map((section) => ( <div - key={`${section.viewRef.namespace}:${section.viewRef.name}`} + key={`${section.viewRef.namespace || "default"}:${section.viewRef.name}`} className="mt-4" >src/pages/views/hooks/useViewData.ts (1)
86-94: Avoid unnecessary aggregated variable fetching in display‑plugin modeWhen
configIdis provided, the hook:
- Still calls
useAggregatedViewVariables(allSectionRefs), which will likely spin up per‑section queries.- Then discards
aggregatedVariablesand ignoresaggregatedCurrentVariablesvia:const currentVariables = isDisplayPluginMode ? (variables ?? {}) : aggregatedCurrentVariables; return { ... aggregatedVariables: isDisplayPluginMode ? [] : aggregatedVariables, currentVariables, };So in display‑plugin mode you pay the cost of the extra queries and aggregation but don’t use their results.
You can short‑circuit the aggregation when
isDisplayPluginModeis true:- const { - variables: aggregatedVariables, - currentVariables: aggregatedCurrentVariables - } = useAggregatedViewVariables(allSectionRefs); + const { + variables: aggregatedVariables, + currentVariables: aggregatedCurrentVariables + } = useAggregatedViewVariables( + isDisplayPluginMode ? [] : allSectionRefs + ); @@ - const currentVariables = isDisplayPluginMode - ? (variables ?? {}) - : aggregatedCurrentVariables; + const currentVariables = isDisplayPluginMode + ? (variables ?? {}) + : aggregatedCurrentVariables; @@ - aggregatedVariables: isDisplayPluginMode ? [] : aggregatedVariables, + aggregatedVariables,If there’s a future need to mix plugin variables with aggregated ones, this can be revisited, but for now this keeps network and compute overhead down in config/plugin mode.
Also applies to: 150-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/pages/audit-report/types/index.ts(1 hunks)src/pages/config/details/ConfigDetailsViewPage.tsx(1 hunks)src/pages/views/components/SingleView.tsx(3 hunks)src/pages/views/components/ViewSection.tsx(1 hunks)src/pages/views/components/ViewWithSections.tsx(1 hunks)src/pages/views/hooks/useAggregatedViewVariables.ts(1 hunks)src/pages/views/hooks/useViewData.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/pages/config/details/ConfigDetailsViewPage.tsx (4)
src/pages/views/hooks/useViewData.ts (1)
useViewData(30-159)src/components/Configs/ConfigDetailsTabs.tsx (1)
ConfigDetailsTabs(31-91)src/ui/Loading/index.tsx (1)
Loading(11-37)src/components/ErrorViewer.tsx (1)
ErrorViewer(68-115)
src/pages/views/components/ViewWithSections.tsx (3)
src/pages/audit-report/types/index.ts (3)
ViewResult(430-446)ViewVariable(387-394)ViewSection(424-428)src/pages/audit-report/components/View/GlobalFiltersForm.tsx (1)
GlobalFiltersForm(65-90)src/pages/views/constants.ts (1)
VIEW_VAR_PREFIX(2-2)
src/pages/views/hooks/useViewData.ts (4)
src/pages/audit-report/types/index.ts (3)
ViewResult(430-446)ViewVariable(387-394)ViewRef(419-422)src/api/services/views.ts (2)
getViewDisplayPluginVariables(140-148)getViewDataById(112-122)src/pages/views/hooks/useAggregatedViewVariables.ts (1)
useAggregatedViewVariables(9-53)src/components/Toast/toast.ts (1)
toastError(15-25)
src/pages/views/components/SingleView.tsx (1)
src/pages/views/hooks/useViewData.ts (1)
useViewData(30-159)
src/pages/views/components/ViewSection.tsx (1)
src/api/services/views.ts (1)
getViewDataByNamespace(124-135)
🔇 Additional comments (6)
src/pages/views/components/ViewSection.tsx (1)
36-41: LGTM! Type-safe namespace handling.The fallback to empty string on line 38 ensures
getViewDataByNamespacealways receives astringas expected by its signature, while theenabledcondition on line 39 still correctly prevents the query from running whennamespaceis falsy.src/pages/views/hooks/useAggregatedViewVariables.ts (1)
32-41: LGTM! Consistent namespace handling.The change mirrors the pattern in
ViewSection.tsx, ensuring type-safe string values forgetViewDataByNamespacewhile theenabledcondition correctly prevents queries whennamespaceis falsy.src/pages/config/details/ConfigDetailsViewPage.tsx (1)
35-59: Clean refactoring of the view rendering logic.The conditional rendering structure is clear and handles all states appropriately. The integration with
ViewWithSectionsandErrorViewercomponents follows good component composition patterns.src/pages/views/components/ViewWithSections.tsx (2)
25-29: Verify intended behavior for emptypanels/columnsarrays.The
isAggregatorViewcheck uses!panels && !columns, which treats empty arrays as truthy (not aggregator). If the intent is to also consider empty arrays as "no content," the check should be:const isAggregatorView = viewResult.sections?.length > 0 && (!panels || panels.length === 0) && (!columns || columns.length === 0);
16-41: Well-structured component with clear responsibilities.The component cleanly separates concerns: aggregated variable display at the top level, primary section rendering, and additional sections. The
hideVariablesprop correctly controls filter visibility, and the logic flow is easy to follow.src/pages/views/components/SingleView.tsx (1)
12-22: SingleView’s integration with useViewData and ViewWithSections looks correctThe component cleanly delegates data fetching to
useViewData({ viewId: id }), handles loading/error/not‑found states before destructuringviewResult, and then passesviewResult,aggregatedVariables,currentVariables, andhandleForceRefreshthrough toViewWithSections. This keeps SingleView lean and consistent with the new shared data layer used on the config details page.Also applies to: 68-90
adityathebe
force-pushed
the
fix/view-sections-on-config-page
branch
from
c5708d4 to
6f8976c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/views/hooks/useViewData.ts (1)
30-167: Hook correctly unifies view + display‑plugin queries and fixes prior error/namespace issuesThis hook looks solid:
- Display‑plugin mode now exposes its own loading/fetching/error states and correctly gates the main view query on
variables, so plugin failures surface viaerrorinstead of silently blockingviewResult.allSectionRefsandsectionsToRefreshnormalize namespaces with?? "", which aligns with other view hooks and ensures cache invalidation works even whennamespaceis absent.handleForceRefreshuses a cache‑busting header, invalidates both plugin and main view queries (with mode‑aware keys), and refreshes per‑section"view-result","view-table", and"view-variables"queries via prefix keys.I don’t see functional issues in the current design; this neatly centralizes view data/refresh behavior.
🧹 Nitpick comments (2)
src/pages/views/hooks/useAggregatedViewVariables.ts (1)
25-41: Recheckenabledcondition now that namespace is normalized to an empty string
queryFnalways normalizes the namespace withsection.namespace || "", butenabledstill requires a truthy namespace (!!section.namespace && !!section.name). For refs coming fromuseViewData,namespaceis""when absent, so these queries will never run even though the backend accepts""as the namespace.If you want aggregated variables for namespace-less views/sections, consider relaxing this to only require a name:
- enabled: !!section.namespace && !!section.name, + enabled: !!section.name,Otherwise, it may be worth documenting that aggregation is intentionally disabled for namespace-less refs.
src/pages/views/components/SingleView.tsx (1)
12-91: SingleView’s new data flow is clean; consider whether errors should hide stale dataThe refactor to
useViewDataplusViewWithSectionsis straightforward: loading/error/not‑found states are clearly separated,handleForceRefreshis threaded through consistently, andisFetchingis used to drive the layout’s loading state while data is present.One UX nuance to think about: when a background refetch fails,
errorwill be truthy whileviewResultstill holds previous data (because ofkeepPreviousData: true), but the current logic shows only the error screen. If you’d rather keep showing the last successful result with an inline error banner, you could gate on!viewResultin the error path and otherwise renderViewWithSectionsplus an error indication.As written, though, behavior is consistent and correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/pages/audit-report/types/index.ts(1 hunks)src/pages/config/details/ConfigDetailsViewPage.tsx(1 hunks)src/pages/views/components/SingleView.tsx(3 hunks)src/pages/views/components/ViewSection.tsx(1 hunks)src/pages/views/components/ViewWithSections.tsx(1 hunks)src/pages/views/hooks/useAggregatedViewVariables.ts(1 hunks)src/pages/views/hooks/useViewData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/audit-report/types/index.ts
- src/pages/views/components/ViewSection.tsx
- src/pages/views/components/ViewWithSections.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/config/details/ConfigDetailsViewPage.tsx (4)
src/pages/views/hooks/useViewData.ts (1)
useViewData(30-168)src/components/Configs/ConfigDetailsTabs.tsx (1)
ConfigDetailsTabs(31-91)src/ui/Loading/index.tsx (1)
Loading(11-37)src/components/ErrorViewer.tsx (1)
ErrorViewer(160-187)
src/pages/views/components/SingleView.tsx (1)
src/pages/views/hooks/useViewData.ts (1)
useViewData(30-168)
🔇 Additional comments (1)
src/pages/config/details/ConfigDetailsViewPage.tsx (1)
4-60: NewuseViewData/ViewWithSectionswiring for config views looks consistentThe refactor to use
useViewDataplusViewWithSectionsis coherent: loading/error/empty states are handled cleanly, and passinghandleForceRefreshintoConfigDetailsTabskeeps the refresh behavior wired through without changing its contract (it happily accepts an async function). No functional issues stand out here.
❌ Deploy Preview for clerk-saas-ui failed. Why did it fail? →
|
❌ Deploy Preview for flanksource-demo-stable failed. Why did it fail? →
|
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.