feat: view sections #2739
feat: view sections #2739
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@adityathebe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughAdds support for multi-section views: API helpers for namespace-based view retrieval, new types for section references, components and layout for per-section rendering, a hook and util to aggregate variables across sections, and refactors single-view rendering to orchestrate sections and unified variable handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SingleView
participant Hook as useAggregatedViewVariables
participant API as Views API
participant ViewSection
participant View
User->>SingleView: Open view page (with sections)
SingleView->>Hook: Request aggregated variables (all section refs)
par Parallel per-section variable fetches
Hook->>API: getViewDataByNamespace(sectionA, currentVariables?)
Hook->>API: getViewDataByNamespace(sectionB, currentVariables?)
Hook->>API: getViewDataByNamespace(sectionN, currentVariables?)
end
API-->>Hook: ViewResult[] (per-section variables)
Hook-->>SingleView: aggregatedVariables, currentVariables
SingleView->>View: Render main View with aggregatedVariables (if shown)
SingleView->>ViewSection: Render each ViewSection (pass currentVariables)
ViewSection->>API: getViewDataByNamespace(namespace,name,currentVariables)
API-->>ViewSection: ViewResult (section data)
ViewSection->>View: Render nested View (hideVariables true)
View-->>User: Rendered content
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (8)
src/api/services/views.ts (1)
101-129: Namespace-based view fetch is correct; consider URL-encoding and DRYing with getViewDataByIdThe implementation mirrors
getViewDataByIdand looks functionally correct. Two small hardening/refactor suggestions:
- To be robust against special characters in
namespace/name, it’s safer to URL-encode them when building the path:- const response = await fetch(`/api/view/${namespace}/${name}`, { + const response = await fetch( + `/api/view/${encodeURIComponent(namespace)}/${encodeURIComponent(name)}`, + { method: "POST", credentials: "include", headers: { "Content-Type": "application/json", ...headers }, body: JSON.stringify(body) - }); + } + );
- The body/error-handling logic is now duplicated with
getViewDataById; consider extracting a small internal helper (e.g.,fetchView(path, variables, headers)) to keep behavior consistent in one place.src/pages/audit-report/types/index.ts (1)
368-377: ViewRef/ViewSection typing is sound; reuse ViewRef instead of redefining elsewhereThe new
ViewRef,ViewSection, andViewResult.sections?: ViewSection[]types line up well with the new/api/view/{namespace}/{name}behavior and theViewSectionUI component.One follow-up to keep things consistent:
src/pages/views/hooks/useAggregatedViewVariables.tsdefines its ownViewRefinterface with the same shape. It would be cleaner to import and reuse this exportedViewRefhere instead of duplicating the type, so any future changes to the contract happen in a single place.Also applies to: 393-394
src/pages/views/utils/aggregateVariables.ts (1)
7-33: Clarify merge semantics when the same variable key appears in multiple viewsThe aggregation logic is clean and efficient, but its conflict behavior is worth calling out:
- When a key appears multiple times, you union the
optionsarrays, which is great.- For all other fields (
type,label,default,value, etc.), the first occurrence “wins” and later ones are ignored.If that “first writer wins” rule is intentional, it might be good to document it explicitly in the comment so future changes don’t accidentally assume some other merge strategy. If it’s not intentional, consider either:
- Detecting and logging/handling mismatches between variable definitions for the same key, or
- Defining a consistent resolution rule (e.g., prefer non-empty labels/defaults, or prefer a particular namespace).
src/pages/views/hooks/useAggregatedViewVariables.ts (1)
8-56: Reuse the shared ViewRef type instead of redefining it locallyThe hook logic around
usePrefixedSearchParams,useQueries, andaggregateVariableslooks good for aggregating variables across sections.Given that
ViewRefis already exported fromsrc/pages/audit-report/types/index.ts, it would be better to import and use that here instead of redefining a localinterface ViewRef. That keeps section/view references strongly typed from a single source of truth and avoids subtle divergence if the backend contract ever changes.For example:
-import { VIEW_VAR_PREFIX } from "../constants"; - -interface ViewRef { - namespace: string; - name: string; -} +import { VIEW_VAR_PREFIX } from "../constants"; +import type { ViewRef } from "../../audit-report/types";src/pages/views/components/ViewSection.tsx (1)
15-83: Section rendering is correct; consider avoiding double-fetching when combined with aggregated variablesThe per-section fetch and rendering flow here looks solid: variables come from prefixed params, the query is keyed by namespace/name plus variables, and loading/error/success UI is well-handled.
One thing to be aware of in the broader design:
useAggregatedViewVariablesalso callsgetViewDataByNamespacefor each section (to collectvariables), andViewSectionthen calls the same endpoint again to render the actual content. For views with several sections this means each section will be fetched at least twice.If this becomes noticeable under load, a couple of options to reduce duplication would be:
- Have
useAggregatedViewVariablesreturn the fullViewResults (or the underlying queries) soViewSectioncan reuse the same data instead of refetching, or- Introduce a lighter-weight endpoint for “variables only” and reserve full
ViewResultcalls for the actual section rendering.Not urgent for correctness, but worth considering if you see extra traffic or latency from sectional views.
src/pages/views/components/SingleView.tsx (3)
44-60: Minor redundancy in namespace fallback.Line 49 uses
viewResult.namespace || ""but line 45 already guards against falsy namespace values, so the|| ""fallback is never reached. Consider simplifying:const refs = [ - { namespace: viewResult.namespace || "", name: viewResult.name } + { namespace: viewResult.namespace, name: viewResult.name } ];
90-112: Consider combining invalidation calls for efficiency.The three
Promise.allblocks run sequentially, but since these invalidations are independent, they can be combined into a single parallel operation:- await Promise.all( - sectionsToRefresh.map((section) => - queryClient.invalidateQueries({ - queryKey: ["view-result", section.namespace, section.name] - }) - ) - ); - - await Promise.all( - sectionsToRefresh.map((section) => - queryClient.invalidateQueries({ - queryKey: ["view-table", section.namespace, section.name] - }) - ) - ); - - await Promise.all( - sectionsToRefresh.map((section) => - queryClient.invalidateQueries({ - queryKey: ["view-variables", section.namespace, section.name] - }) - ) - ); + await Promise.all( + sectionsToRefresh.flatMap((section) => [ + queryClient.invalidateQueries({ + queryKey: ["view-result", section.namespace, section.name] + }), + queryClient.invalidateQueries({ + queryKey: ["view-table", section.namespace, section.name] + }), + queryClient.invalidateQueries({ + queryKey: ["view-variables", section.namespace, section.name] + }) + ]) + );
171-179: Consider documenting the CSS issue.The comment mentions "CSS issues that I couldn't solve" but doesn't explain what the issues are. This could make future maintenance harder. Consider either:
- Adding a brief description of the CSS issue
- Creating a follow-up issue to properly fix it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/api/services/views.ts(1 hunks)src/pages/audit-report/components/View/View.tsx(6 hunks)src/pages/audit-report/types/index.ts(2 hunks)src/pages/views/components/SingleView.tsx(4 hunks)src/pages/views/components/ViewLayout.tsx(1 hunks)src/pages/views/components/ViewSection.tsx(1 hunks)src/pages/views/constants.ts(1 hunks)src/pages/views/hooks/useAggregatedViewVariables.ts(1 hunks)src/pages/views/utils/aggregateVariables.ts(1 hunks)src/ui/MRTDataTable/MRTDataTable.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/api/services/views.ts (1)
src/pages/audit-report/types/index.ts (1)
ViewResult(379-394)
src/pages/views/utils/aggregateVariables.ts (1)
src/pages/audit-report/types/index.ts (1)
ViewVariable(351-358)
src/pages/views/hooks/useAggregatedViewVariables.ts (5)
src/pages/audit-report/types/index.ts (1)
ViewRef(368-371)src/hooks/usePrefixedSearchParams.ts (1)
usePrefixedSearchParams(14-70)src/pages/views/constants.ts (1)
VIEW_VAR_PREFIX(2-2)src/api/services/views.ts (1)
getViewDataByNamespace(101-129)src/pages/views/utils/aggregateVariables.ts (1)
aggregateVariables(7-33)
src/pages/audit-report/components/View/View.tsx (5)
src/pages/views/constants.ts (1)
VIEW_VAR_PREFIX(2-2)src/pages/audit-report/components/View/GlobalFiltersForm.tsx (1)
GlobalFiltersForm(65-90)src/pages/audit-report/components/View/ViewTableFilterForm.tsx (1)
ViewTableFilterForm(55-80)src/pages/audit-report/components/ViewColumnDropdown.tsx (1)
ViewColumnDropdown(13-67)src/pages/audit-report/components/View/panels/utils.ts (1)
formatDisplayLabel(127-132)
src/pages/views/components/ViewLayout.tsx (3)
src/ui/Head.tsx (1)
Head(8-18)src/ui/Layout/SearchLayout.tsx (1)
SearchLayout(23-77)src/ui/BreadcrumbNav/index.tsx (2)
BreadcrumbNav(10-64)BreadcrumbRoot(70-90)
src/pages/views/components/SingleView.tsx (5)
src/api/services/views.ts (1)
getViewDataById(72-99)src/pages/views/hooks/useAggregatedViewVariables.ts (1)
useAggregatedViewVariables(13-57)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/audit-report/types/index.ts (1)
ViewSection(373-377)
🔇 Additional comments (9)
src/ui/MRTDataTable/MRTDataTable.tsx (1)
123-135: Flex basis change to"1 1 auto"looks goodSwitching the paper and table container flex from
"1 1 0"to"1 1 auto"should make them size more naturally within flex layouts without introducing functional issues, assuming no callers relied on the old zero-basis behavior.src/pages/views/constants.ts (1)
1-2: VIEW_VAR_PREFIX constant is clear and well-placedCentralizing
"viewvar"inVIEW_VAR_PREFIXis a good move for keeping view-variable URL handling consistent across hooks/components.src/pages/views/components/SingleView.tsx (2)
1-11: LGTM!The imports are well-organized, bringing in the new aggregated variable hook, section components, and the shared constant. The consolidation of
VIEW_VAR_PREFIXinto a constants file is a good practice.
212-231: LGTM!The section rendering logic is well-structured. Proper keys are used to ensure React can efficiently reconcile the list, and
hideVariablesis correctly passed to prevent duplicate filter display since aggregated variables are rendered at the top level.src/pages/audit-report/components/View/View.tsx (5)
25-42: LGTM!The new
hideVariablesprop is properly typed as optional boolean, and the import ofVIEW_VAR_PREFIXfrom the shared constants file ensures consistency across components.
161-167: Verify intended behavior for undefined/null titles.The condition
title !== ""will render the title block whentitleisundefinedornull, showingundefined/nullin the UI. If the intent is to only show the title when it's a non-empty string, consider:- {title !== "" && ( + {title && (If the empty string specifically means "hide title" while undefined means "show default", the current logic is correct but could benefit from a comment explaining this distinction.
197-248: LGTM!The
ViewTableFilterFormcorrectly wraps the filterable column dropdowns while the display mode toggle appropriately uses URL params for state management. The form structure is clean and the conditional rendering forhasCardModeis well-implemented.
271-280: LGTM!The
min-h-0 flex-1wrapper is a proper CSS fix for allowing the data table to be scrollable within a flex container. Themin-h-0overrides the defaultmin-height: autothat would prevent the flex item from shrinking.
183-195: LGTM!The grid layout with
auto-fitandminmaxprovides responsive panel sizing. The inline styles are acceptable here since the values are dynamic and specific to this component's needs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/services/views.ts (1)
91-96: Consider handling non-JSON error responses.If the server returns a non-JSON error response (e.g., HTML error page from a proxy),
response.json()will throw, masking the original HTTP error. Consider adding a fallback.if (!response.ok) { - const errorData = await response.json(); - throw new Error( - errorData.error || `HTTP ${response.status}: ${response.statusText}` - ); + let errorMessage = `HTTP ${response.status}: ${response.statusText}`; + try { + const errorData = await response.json(); + if (errorData.error) { + errorMessage = errorData.error; + } + } catch { + // Response wasn't JSON, use default HTTP error message + } + throw new Error(errorMessage); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/services/views.ts(2 hunks)src/pages/views/components/SingleView.tsx(4 hunks)src/pages/views/components/ViewLayout.tsx(1 hunks)src/pages/views/hooks/useAggregatedViewVariables.ts(1 hunks)src/pages/views/utils/aggregateVariables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/views/components/ViewLayout.tsx
- src/pages/views/hooks/useAggregatedViewVariables.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/views/utils/aggregateVariables.ts (1)
src/pages/audit-report/types/index.ts (1)
ViewVariable(351-358)
src/api/services/views.ts (1)
src/pages/audit-report/types/index.ts (1)
ViewResult(379-394)
🔇 Additional comments (7)
src/pages/views/utils/aggregateVariables.ts (1)
1-34: LGTM! Clean implementation for variable aggregation.The deduplication logic using Map is appropriate, and merging options via Set union is correct. The first-definition-wins strategy for other fields (label, default) is well-documented in the comment.
src/api/services/views.ts (1)
101-124: Good refactoring with proper path encoding.The extraction of
fetchViewDataeliminates duplication, and both public functions correctly encode their path segments to handle special characters in IDs, namespaces, and names.src/pages/views/components/SingleView.tsx (5)
30-41: Good query configuration for metadata-only fetch.Passing
undefinedfor variables and usingkeepPreviousData: trueis appropriate for this orchestrating component where per-section queries handle their own data fetching.
43-63: Proper hook placement and memoization.Calling hooks before early returns satisfies React's rules, and the
useMemodependencies correctly capture all values that affect the computedallSectionRefs.
79-104: Query invalidation now properly includes the main view.The previous concern about query key mismatch has been addressed. Line 90 now invalidates
["view-result", id]for the main metadata query, and the subsequent invalidations handle all section-specific queries.
212-223: Clean section rendering with proper key strategy.Each section has a unique key based on its namespace and name, and
hideVariablescorrectly prevents duplicate filter UI since aggregated variables are rendered once at the top.
189-202: Appropriate conditional rendering for aggregated variables.The global filters are correctly rendered once at the top when variables exist, avoiding duplication across sections. The separator provides clear visual distinction.
adityathebe
force-pushed
the
feat/view-sections
branch
from
d18a5ae to
a261b3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/views/components/SingleView.tsx (1)
163-171: Avoid passing an empty‑string namespace intoViewSectionUsing
namespace: namespace || ""still constructs aViewRefwith an invalid namespace whenviewResult.namespaceis missing, andViewSectionwill then operate on that bogus value (likely resulting in failed namespace‑based fetches). It would be safer to guard and only build/render the primary section when bothnamespaceandnameare present:- const primaryViewSection = { - title: "", - viewRef: { - namespace: namespace || "", - name: name - } - }; + const hasViewRef = !!namespace && !!name; + const primaryViewSection = hasViewRef + ? { + title: "", + viewRef: { + namespace, + name + } + } + : null;and below:
- <div> - <ViewSection - key={`${namespace || "default"}:${name}`} - section={primaryViewSection} - hideVariables - /> - </div> + {primaryViewSection && ( + <div> + <ViewSection + key={`${namespace}:${name}`} + section={primaryViewSection} + hideVariables + /> + </div> + )}This aligns with the earlier review concern about not passing an unsafe namespace through to
ViewSection.
🧹 Nitpick comments (2)
src/pages/views/components/SingleView.tsx (2)
43-63: Section ref collection and variable aggregation look correct (small optional tidy‑up)The
allSectionRefsmemo plususeAggregatedViewVariables(allSectionRefs)wiring is sound and ensures the main view and all sections contribute to the aggregated variable set. If you want to tighten it up a bit, you could buildrefsin a more declarative way:- const allSectionRefs = useMemo<ViewRef[]>(() => { - if (!viewResult?.namespace || !viewResult?.name) { - return []; - } - const refs = [{ namespace: viewResult.namespace, name: viewResult.name }]; - if (viewResult?.sections) { - viewResult.sections.forEach((section) => { - refs.push({ - namespace: section.viewRef.namespace, - name: section.viewRef.name - }); - }); - } - return refs; - }, [viewResult?.namespace, viewResult?.name, viewResult?.sections]); + const allSectionRefs = useMemo<ViewRef[]>(() => { + if (!viewResult?.namespace || !viewResult?.name) { + return []; + } + const sectionRefs = + viewResult.sections?.map((section) => section.viewRef) ?? []; + return [{ namespace: viewResult.namespace, name: viewResult.name }, ...sectionRefs]; + }, [viewResult?.namespace, viewResult?.name, viewResult?.sections]);
65-104: Force refresh may use stale section refs and likely double‑fetches metadataTwo things to consider here:
sectionsToRefreshprefersallSectionRefs, which is the snapshot from the render when the button was clicked; afterawait refetch()you already have freshresult.data, so you might want to derive sections from that first and only fall back toallSectionRefsifresult.datais missing, to better reflect any changes in sections:- const sectionsToRefresh = - allSectionRefs.length > 0 && - allSectionRefs[0].namespace && - allSectionRefs[0].name - ? allSectionRefs - : result.data?.namespace && result.data.name - ? [{ namespace: result.data.namespace, name: result.data.name }] - : []; + const sectionsToRefresh = + result.data?.namespace && result.data.name + ? [ + { namespace: result.data.namespace, name: result.data.name }, + ...(result.data.sections?.map((s) => s.viewRef) ?? []) + ] + : allSectionRefs;
- Calling
await refetch()and theninvalidateQueries({ queryKey: ["view-result", id] })means the main metadata query will typically be fetched twice per refresh. You could rely solely oninvalidateQueries(withforceRefreshRefset during that call) or drop the extra invalidation for["view-result", id]to avoid the duplicate network hit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/api/services/views.ts(2 hunks)src/pages/views/components/SingleView.tsx(4 hunks)src/pages/views/hooks/useAggregatedViewVariables.ts(1 hunks)src/pages/views/utils/aggregateVariables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/views/hooks/useAggregatedViewVariables.ts
- src/api/services/views.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/views/utils/aggregateVariables.ts (1)
src/pages/audit-report/types/index.ts (1)
ViewVariable(351-358)
src/pages/views/components/SingleView.tsx (5)
src/api/services/views.ts (1)
getViewDataById(101-111)src/pages/audit-report/types/index.ts (2)
ViewRef(368-371)ViewSection(373-377)src/pages/views/hooks/useAggregatedViewVariables.ts (1)
useAggregatedViewVariables(9-53)src/pages/audit-report/components/View/GlobalFiltersForm.tsx (1)
GlobalFiltersForm(65-90)src/pages/views/constants.ts (1)
VIEW_VAR_PREFIX(2-2)
🔇 Additional comments (1)
src/pages/views/utils/aggregateVariables.ts (1)
1-34: Aggregation helper correctly deduplicates and merges optionsThe
aggregateVariablesimplementation cleanly dedupes bykey, unionsoptions, and avoids mutating inputs while preserving the first definition’s other fields; this matches the intended behavior for cross‑section aggregation.
& Make section React keys unique across namespaces to avoid collisions * Remove or wire up unused viewHash helper to avoid dead code
…useAggregatedViewVariables caching/query keys)
adityathebe
force-pushed
the
feat/view-sections
branch
from
a261b3e to
3656139
Compare
❌ Deploy Preview for clerk-saas-ui failed. Why did it fail? →
|
❌ Deploy Preview for flanksource-demo-stable failed. Why did it fail? →
|
resolves: flanksource/mission-control#2545
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.