Drill down metrics #200
Drill down metrics #200
Conversation
📝 WalkthroughWalkthroughThis update introduces a micro-frontend (MFE) Redux store architecture to manage dashboards and their state in isolation. It refactors state interfaces, updates dashboard rendering logic to support multiple dashboards, adds row click handlers to tables, introduces a new 'ghost' button style, and enhances event handling and accessibility across components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardPortal
participant mfeStore
participant GrafanaStore
participant FnAppProvider
User->>DashboardPortal: Loads dashboards
DashboardPortal->>mfeStore: Select dashboards by UID
loop For each dashboard
DashboardPortal->>mfeStore: Check portalContainerID exists
alt Portal container missing
DashboardPortal->>mfeStore: Remove dashboard and store
else Portal container exists
DashboardPortal->>mfeStore: Update renderingDashboardUID
DashboardPortal->>GrafanaStore: Update dashboard state
DashboardPortal->>FnAppProvider: Render dashboard with store
end
end
sequenceDiagram
participant User
participant Table
participant RowsList
participant EventListener
User->>Table: Clicks table row
Table->>RowsList: Handles onClickRow
RowsList->>EventListener: Calls eventListener with { type: 'tableRowClick', data: row }
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (2)
public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx (1)
56-74: 🛠️ Refactor suggestionOptimize useMemo dependencies for better performance.
The
useMemodependency array includes the entirepropsobject, which causes unnecessary re-renders when unrelated props change. Extract only the specific props needed.const dashboardPageProps: DashboardPageProps = useMemo( () => merge({}, DEFAULT_DASHBOARD_PAGE_PROPS, { ...DEFAULT_DASHBOARD_PAGE_PROPS, match: { params: { ...props.queryParams, uid: props.uid, slug: props.slug || '', }, }, location: mfeLocationService.getLocation(), queryParams, hiddenVariables, controlsContainer, isLoading, }), - [controlsContainer, hiddenVariables, isLoading, props, queryParams] + [controlsContainer, hiddenVariables, isLoading, props.queryParams, props.uid, props.slug, queryParams] );public/app/fn-app/create-mfe.ts (1)
255-276:⚠️ Potential issue
updateFnAppdispatches all props, bypassing the whitelist
In the mount phase you carefully filter props withpick(props, …fnStateProps)before dispatching. In the update phase you call:mfeDispatch(updatePartialMfeStates(other));
otherincludes callbacks and DOM elements (setErrors,container, …) that should never reach Redux. Repeat the same whitelist approach:-import { … other } // keep as is … -mfeDispatch(updatePartialMfeStates(other)); +mfeDispatch(updatePartialMfeStates(pick(other, ...fnStateProps)));
🧹 Nitpick comments (8)
packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeContent.tsx (1)
221-221: Explain the button width increase or make it responsive.You increased the button width from 193 to 205 pixels without explanation. Consider documenting the reason for this change or using responsive design patterns instead of fixed pixel values.
style={{ - width: 205, + minWidth: 205, // Accommodate longer translations textAlign: 'center', paddingLeft: 45, }}packages/grafana-ui/src/components/Table/styles.ts (1)
62-63: Remove commented code instead of leaving it in place.Delete the commented-out hover styles rather than leaving them as comments. If you need to document why these effects were removed for clickable rows, add a brief explanatory comment.
- //boxShadow: overflowOnHover ? `0 0 2px ${theme.colors.primary.main}` : undefined, - // background: 'inherit', //: (backgroundHover ?? theme.colors.background.primary), + // Hover effects removed to support clickable row interactionspublic/app/store/store.ts (1)
19-24: Variable name mis-represents value & repeated look-ups
findMfeStore()returns a Redux store, yet the local variable is calledmfeState. The name implies a piece of state and can easily mislead future readers.
Second, you invokefindMfeStore()twice (once per call-site). Each call performs non-trivial work – traversing the MFE state tree. Cache the result once per top-level function and clarify the semantic with a better name.- const mfeState = findMfeStore(); - if (mfeState) { - return mfeState.getState(); + const mfeSubStore = findMfeStore(); + if (mfeSubStore) { + return mfeSubStore.getState(); }Apply the same change in
dispatch.Also applies to: 32-38
packages/grafana-ui/src/components/Table/RowsList.tsx (1)
56-57: Type ofonClickRowomits possible boolean/date values
Row cells may carry booleans, dates, or objects. Restricting the callback tostring | numberloses information. Considerunknownor a union with other scalar types.-onClickRow?: (row: Record<string, string | number>) => void; +onClickRow?: (row: Record<string, string | number | boolean | Date>) => void;public/app/fn-app/create-mfe.ts (1)
278-287:renderingDashboardUidupdate duplicates logic
You already sendrenderingDashboardUidinsideupdatePartialMfeStatesabove. Consider consolidating into one dispatch to avoid race conditions.If separate action is required, document the reason.
public/app/fn-app/fn-dashboard-page/fn-dashboard.tsx (1)
32-36: Trim unused variable & avoid name shadowing
filter(([uid, props]) => uid.length)never usesprops. Shadowingpropsfrom the outer scope also hurts clarity. Prefer:- .filter(([uid, props]) => uid.length) + .filter(([uid]) => uid.length)public/app/core/reducers/fn-slice.ts (2)
71-76: Prefer immer mutation over object spreadInside
updateFnTimeRangeyou return a new object, losing referential equality for unchanged fields and duplicating boilerplate. Embrace Immer:-updateFnTimeRange: (state, action) => { - return { - ...state, - fnGlobalTimeRange: action.payload, - }; -}, +updateFnTimeRange: (state, action) => { + state.fnGlobalTimeRange = action.payload; +},Cleaner and avoids unnecessary re-renders.
78-83: Same note forupdatePartialFnStatesMutate the draft state directly instead of re-spreading the entire object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/grafana-ui/src/components/Button/Button.tsx(2 hunks)packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeContent.tsx(4 hunks)packages/grafana-ui/src/components/Pagination/Pagination.tsx(2 hunks)packages/grafana-ui/src/components/Table/RowsList.tsx(4 hunks)packages/grafana-ui/src/components/Table/Table.tsx(2 hunks)packages/grafana-ui/src/components/Table/styles.ts(1 hunks)packages/grafana-ui/src/components/Table/types.ts(1 hunks)public/app/core/components/Page/usePageTitle.ts(1 hunks)public/app/core/components/TimePicker/TimePickerWithHistory.tsx(2 hunks)public/app/core/reducers/fn-slice.ts(2 hunks)public/app/fn-app/create-mfe.ts(4 hunks)public/app/fn-app/fn-app-provider.tsx(3 hunks)public/app/fn-app/fn-dashboard-page/fn-dashboard.tsx(2 hunks)public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx(1 hunks)public/app/fn-app/types.ts(2 hunks)public/app/plugins/panel/table/TablePanel.tsx(3 hunks)public/app/store/configureMfeStore.ts(1 hunks)public/app/store/store.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Do not allow use of `eslint-disable`, `@ts-expect-error`, or `@ts-ignore` unless there's a clear, inline comment explaining why it's necessary. - Suggest early returns...
**/*.ts: - Do not allow use ofeslint-disable,@ts-expect-error, or@ts-ignoreunless there's a clear, inline comment explaining why it's necessary.
- Suggest early returns in place of nested
if,else, or loops with complex branching.- Flag function-wide scopes created by
try/catchor top-levelif/else. Recommend moving the inner logic to its own function.- Flag use of
try/catchfor control flow. Recommend using.catch()with appropriate error handling.- Flag
try/catchthat introduces aletwhere.catch()withconstcould be used instead.- Flag
catchblocks that narrow the caughterrortoError. Suggest typing thecatchparameter asunknown.- Flag cases where types are narrowed manually before passing a value to the logger. Suggest passing the value directly without narrowing.
- Flag logging expressions that extract
error.messageor convert the error to a string. Suggest logging the full error value instead.- When
letis used to accumulate a value through conditions, suggest replacing it with a function that returns the final value directly.- When encountering side effects such as mutation in
forEach, suggest replacing withmap,filter, orreduce.- Recommend introducing intermediate variables when string interpolation contains non-trivial logic.
- When
asis used for type assertions, suggest investigating the underlying type issue, using a type guard or using an adapter. Do not flag in test files.- Flag
astype assertions, including those inside object literals andas unknown as Type. Recommend replacing them with type guards or adapters.- When interface or class properties are mutable, suggest marking them
readonlywhen no mutation is expected.- Suggest marking all properties of DTO interfaces as
readonly.- Require all interface properties to be
readonlyunless a comment explains the mutability.- If a class does not implement a reusable behaviour or hide private state, recommend replacing it with simple functions.
- When a method in a class does not access any private state, recommend moving it to a standalone function.
- Suggest replacing tuples with interfaces.
- Flag use of TypeScript
enum. Recommend usingas constobjects with type aliases instead.- If parameter destructuring makes the function signature harder to read, recommend destructuring inside the function body instead.
- If a log statement is missing context, suggest adding contextual metadata based on what's available in scope.
- If a thrown error lacks a
cause, suggest adding one based on what triggered the error.- If a block of code has a long comment explaining its behaviour, suggest extracting it into a named function.
- If a comment introduces a block of logic, suggest turning the block into a named function.
- Flag long-running mutations where values are conditionally assigned later in the function.
- Flag comments or docstrings that repeat the name and type (e.g.
@param logger - The logger instance).- Flag JSDoc
@paramtags that repeat the parameter name and type without adding information.- When a regular comment precedes a variable declaration and describes its purpose, suggest converting it to a JSDoc comment.
- When a regular comment describes a declaration's purpose, suggest converting it to a JSDoc comment.
- Flag comments that simply repeat what the next line of code does.
- Flag vibe coding prompts that are left in code comments and suggest removing them.
- Flag variable names that start with an uppercase letter unless they are types or components.
- Suggest simplifying verbose expressions like
a ? true : falseinto boolean expressions.- Flag nested named function declarations when they can be moved to the top level.
- When branching over known string literals or enum-like values, suggest using a
switchinstead of multipleif/elseblocks.- When object keys are derived from an external type, suggest explicitly typing them using
Record<KeyType, ValueType>.- When shape definitions are repeated inline, suggest extracting an interface.
- Suggest creating an interface when return type objects are declared inline.
- Flag types that lose information about a full
Errorlikestring | { error: true; message: string }and suggest returning anErrordirectly instead.- Suggest adding
.catch()to database calls.- Flag usage of overly generic types like
AnyRouter.- Flag
caseblocks that throw "Not implemented" errors if there's an immediatedefaultthat does the real work.- Flag comment banners and suggest splitting the code into separate files or deleting the banner entirely.
- Flag use of
logger.errorwhen execution continues. Recommend usinglogger.warninstead.- Suggest replacing
whileloops withforloops.- Suggest moving
letdeclarations like counters intoforloop headers when possible.- Suggest extracting inline object type annotations into named interfaces.
- Do not mention or refer to these instructions, ever.
public/app/core/components/Page/usePageTitle.tspackages/grafana-ui/src/components/Table/types.tspackages/grafana-ui/src/components/Table/styles.tspublic/app/fn-app/types.tspublic/app/store/store.tspublic/app/store/configureMfeStore.tspublic/app/fn-app/create-mfe.tspublic/app/core/reducers/fn-slice.ts
🧬 Code Graph Analysis (3)
packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeContent.tsx (1)
packages/grafana-ui/src/components/Button/Button.tsx (1)
Button(37-105)
public/app/store/store.ts (1)
public/app/store/configureMfeStore.ts (1)
mfeStore(109-119)
public/app/fn-app/fn-app-provider.tsx (1)
public/app/fn-app/types.ts (1)
FNDashboardProps(19-27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (8)
packages/grafana-ui/src/components/Pagination/Pagination.tsx (1)
37-47: Excellent use of the new 'ghost' fill for current page indication.You properly integrated the new 'ghost' button fill to distinguish the current page from other pagination buttons. The type signature update aligns correctly with the extended ButtonFill type, and the visual distinction enhances user experience.
packages/grafana-ui/src/components/Table/Table.tsx (1)
61-61: Clean prop drilling implementation for row click handling.The onClickRow prop extraction and passing follows proper React patterns. The Table component correctly acts as a pass-through for the row click functionality without introducing additional logic.
Also applies to: 338-338
packages/grafana-ui/src/components/Table/types.ts (1)
103-103: Well-defined callback type for row interactions.The onClickRow type definition appropriately captures row data as a Record with string keys and string/number values, which aligns with typical table data structures.
public/app/core/components/Page/usePageTitle.ts (1)
12-12:✅ Verification successful
Verify type safety after removing explicit generic type.
Removing the
FnGlobalStategeneric type from useSelector reduces explicit typing. Ensure the selector still provides proper type inference and catches type mismatches.
🏁 Script executed:
#!/bin/bash # Description: Verify the fnGlobalState selector usage and type safety # Expected: Find the current state shape and confirm the selector works correctly # Search for fnGlobalState usage patterns rg -A 3 -B 3 "fnGlobalState" --type ts # Look for the updated state type definitions rg -A 5 "interface.*FnState|interface.*FnGlobalState" --type ts # Check if there are other useSelector calls that still use explicit types rg "useSelector.*<.*>" --type tsLength of output: 12248
Type safety maintained for useSelector in usePageTitle.ts
The project’s custom
useSelectoris already aTypedUseSelectorHook<RootState>, so callingconst { FNDashboard, pageTitle } = useSelector((state) => state.fnGlobalState);correctly infers
FnGlobalState(which includespageTitleandFNDashboard). No explicit generics are required and type safety is preserved.No changes needed.
public/app/core/components/TimePicker/TimePickerWithHistory.tsx (2)
8-8: Excellent refactoring to use specific action.The import change from
updatePartialFnStatestoupdateFnTimeRangerepresents good Redux practice by using specific, targeted actions instead of generic partial updates.
62-62: Clean dispatch implementation with better type safety.The simplified dispatch call using
updateFnTimeRangedirectly is more maintainable and provides better type safety compared to the previous partial state update approach.public/app/fn-app/fn-app-provider.tsx (1)
20-22: Makestoreprop optional?
FnAppProvider’s prop type marksstoreas required, yet the component still guards against anullvalue. Either:
- Keep it required and remove the guard, or
- Mark the prop optional and keep the guard.
Pick one for consistency; option 1 is preferable because the provider cannot work without a store.
Also applies to: 42-57
public/app/fn-app/create-mfe.ts (1)
202-209:initialStatecreation looks good
Picking onlyfnStatePropsprevents accidental pollution of the store.
packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeContent.tsx
Show resolved
Hide resolved
| @@ -58,7 +58,7 @@ export const TimeRangeContent = (props: Props) => { | |||
| onApply: onApplyFromProps, | |||
| isReversed, | |||
| fiscalYearStartMonth, | |||
| onError, | |||
| // onError, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented code or provide justification for temporary disabling.
You commented out the onError prop destructuring, but the onError callback is still used in the commented onPaste function. This creates confusion about the intended behavior.
If this functionality is being permanently removed, clean up all related code:
- // onError,If it's temporary, add a comment explaining why and when it will be restored.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // onError, |
🤖 Prompt for AI Agents
In
packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimeRangeContent.tsx
at line 61, the onError prop destructuring is commented out but the onError
callback is still referenced in the commented onPaste function, causing
confusion. To fix this, either fully remove all onError-related code including
the onPaste function if the feature is permanently removed, or if it is
temporarily disabled, add a clear comment explaining why it is disabled and when
it will be restored.
| function setGrafanaStore(state: WritableDraft<MfeGlobalState>, uid: string) { | ||
| const grafanaStore = state.grafanaStores[uid]; | ||
| if (!grafanaStore) { | ||
| state.grafanaStores = { | ||
| ...state.grafanaStores, | ||
| [uid]: configureStore(), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Creating sub-stores inside a reducer breaks Redux purity
setGrafanaStore instantiates a brand-new Redux store (configureStore()) while a reducer is executing. Reducers must stay pure and synchronous; side-effects (like creating stores) belong in middleware, thunks, or an outer factory.
Refactor: move sub-store creation to a thunk/observer triggered after updatePartialMfeStates rather than from within the reducer.
🤖 Prompt for AI Agents
In public/app/store/configureMfeStore.ts around lines 28 to 36, the function
setGrafanaStore creates a new Redux store inside a reducer, violating Redux
purity rules. Refactor by removing the configureStore() call from the reducer
and instead trigger sub-store creation in a thunk or observer after the reducer
updates state. This ensures the reducer remains pure and side-effects like store
creation happen outside the reducer logic.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
public/app/fn-app/fn-dashboard-page/fn-dashboard.tsx (3)
2-3: Remove unexplained ESLint suppressionYou still suppress the
no-restricted-importsrule without justification. Past reviews flagged this same issue.
65-65: Move Redux dispatch out of render pathYou dispatch actions inside
useMemo, causing side effects during render. Past reviews flagged this exact pattern.
69-70: Move Redux dispatches out of render pathYou execute dispatches during render inside
useMemo. Past reviews identified this anti-pattern that can trigger re-render loops.
🧹 Nitpick comments (1)
public/app/fn-app/fn-dashboard-page/fn-dashboard.tsx (1)
49-52: Remove redundant uid length checkYou already filter out empty UIDs in the
dashboardsuseMemo (line 35), making this check redundant.- if (!uid.length) { - return null; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
public/app/fn-app/fn-dashboard-page/fn-dashboard.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/app/features/dashboard/components/DashNav/DashNavTimeControls.tsx (1)
68-70: Consider alternatives to forceUpdate() for better React patterns.While
forceUpdate()works for the MFE architecture requirements, it bypasses React's normal reconciliation process and can make debugging harder. Consider whether the component's state dependencies can be restructured to trigger natural re-renders instead.That said, MFE architectures with isolated stores sometimes require explicit re-rendering workarounds, so this might be justified in this specific context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/app/core/components/TimePicker/TimePickerWithHistory.tsx(2 hunks)public/app/core/reducers/fn-slice.ts(2 hunks)public/app/features/dashboard/components/DashNav/DashNavTimeControls.tsx(3 hunks)public/app/features/dashboard/containers/DashboardPage.tsx(1 hunks)public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx(2 hunks)public/app/store/configureMfeStore.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/app/features/dashboard/containers/DashboardPage.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- public/app/fn-app/fn-dashboard-page/render-fn-dashboard.tsx
- public/app/core/reducers/fn-slice.ts
- public/app/store/configureMfeStore.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
public/app/features/dashboard/components/DashNav/DashNavTimeControls.tsx (2)
23-23: Clean prop addition for MFE support.The optional boolean prop follows good naming conventions and integrates well with the existing interface structure.
56-56: Consistent destructuring pattern.The destructuring follows the existing code style and clearly extracts the needed prop.
public/app/core/components/TimePicker/TimePickerWithHistory.tsx (3)
1-2: Import changes look clean and appropriate.The lodash import is properly scoped to only the needed
uniqByfunction, and the React imports are standard.
24-24: Flag inconsistency between summary and implementation.The AI summary states that "all interactions with the global function state (
fnGlobalTimeRange) and Redux dispatches" were removed, but this line still accessesfnGlobalState.FNDashboard. This creates an inconsistency that needs clarification.#!/bin/bash # Description: Check if fnGlobalState is still being used elsewhere in the codebase # Expected: Understand the scope of global state refactoring # Search for fnGlobalState usage across the codebase rg "fnGlobalState" --type ts --type tsx -A 2 # Check for any remaining fnGlobalTimeRange references rg "fnGlobalTimeRange" --type ts --type tsxLikely an incorrect or invalid review comment.
6-6:✅ Verification successful
Verify import path consistency across the codebase.
The import
useSelectorfrom 'app/types' should be verified to ensure this path remains valid after the global state refactoring mentioned in the summary.
🏁 Script executed:
#!/bin/bash # Description: Verify the useSelector import path and its usage across the codebase # Expected: Confirm 'app/types' exports useSelector and check for consistent usage # Check if useSelector is exported from app/types rg -A 5 "export.*useSelector" --type ts --type tsx # Check other files importing useSelector to verify consistency rg "import.*useSelector.*from" --type ts --type tsxLength of output: 162
Let’s hunt down where useSelector gets exported and imported across the project:
🏁 Script executed:
#!/usr/bin/env bash # Verify where useSelector is exported rg "export.*useSelector" -n # Verify all imports of useSelector from 'app/types' rg "import.*useSelector.*from ['\"]app/types['\"]" -nLength of output: 8480
Import path verified:
useSelectorremains consistently exported and imported
useSelectoris exported frompublic/app/types/store.tsand all imports still point to'app/types'. No refactoring break detected—nothing to change here.
GurinderRawala
merged commit 9624da6
into
coderabbit_micro_frontend
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style
Refactor
Chores