[EXAMPLE ONLY DO NOT MERGE] Example of user-defined custom hook in theme#78
[EXAMPLE ONLY DO NOT MERGE] Example of user-defined custom hook in theme#78cwbuecheler wants to merge 1 commit intomainfrom
Conversation
WalkthroughThe changes introduce a theme-based async props customization system. A new Changes
Sequence DiagramsequenceDiagram
participant Component as BarChartDefaultPro
participant Effect as useEffect Hook
participant Theme as Theme System
participant Transform as Props Transformer
participant Render as Component Render
Component->>Component: Mount with original props
Component->>Effect: Trigger useEffect
Effect->>Theme: Call theme.useCustomProps(props, 'BarChartDefaultPro')
Theme->>Transform: Check chartName match and props shape
Transform->>Transform: If matched: augment data with manipulatedValue
Transform-->>Theme: Return transformed props
Theme-->>Effect: Return customProps (transformed or original fallback)
Effect->>Component: setState(customProps)
Component->>Render: Render with customProps
Render-->>Component: Derived values from customProps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 4
🤖 Fix all issues with AI agents
In `@embeddable.theme.ts`:
- Around line 12-18: The current manipulation in manipulatedData uses
row[props.dimension.name] * 2 which will produce NaN because dimensions are
usually strings; change manipulatedData (the data.map callback) to use a numeric
measure instead (e.g., row[props.measure.name] or props.measures[0].name) for
arithmetic or explicitly coerce and validate the value (parseFloat/Number with
isFinite check and fallback) before assigning manipulatedValue, and add a guard
so manipulatedValue is set to a safe default when the source is not a valid
number.
In `@src/components/charts/bars/BarChartDefaultPro/index.tsx`:
- Line 41: The component initializes customProps with the incoming props causing
a brief flash of untransformed data; change the state initialization to null
(useState<BarChartDefaultProProps | null>(null)) and update all references to
handle null (render a loading placeholder or return null) until your async hook
sets customProps via setCustomProps; ensure prop type annotations and any
destructuring that uses customProps are guarded (e.g., in the render path and in
effects) so the component doesn't access fields on null.
- Around line 46-48: The Promise returned by
customPropsHook<BarChartDefaultProProps>(props, 'BarChartDefaultPro') lacks
error handling; add a .catch handler to that chain to catch rejections and call
setCustomProps(props) as a safe fallback (and optionally log the error), so the
component doesn't rely on a rejected Promise or stale state; ensure you modify
the call site where customPropsHook is invoked and touch setCustomProps only in
the success and error handlers.
- Around line 43-52: The current useEffect launching theme.useCustomProps
(customPropsHook) can suffer a race where slower promises overwrite newer state;
modify the effect in BarChartDefaultPro to use a mounted/stale flag (e.g., let
cancelled = false) and flip it in the cleanup so that after awaiting
customPropsHook<BarChartDefaultProProps>(...) you only call
setCustomProps(result || props) when not cancelled; ensure the cleanup sets the
flag to true and that you handle both the async branch and the immediate
setCustomProps(props) branch consistently to prevent stale updates.
🧹 Nitpick comments (3)
src/theme/theme.types.ts (2)
56-60:BaseChartTypesassumes a singledimension, which may not fit all chart types.Some charts (e.g., grouped bar charts) may accept multiple dimensions or use different prop shapes. If this type is meant to be the base constraint for all charts using
useCustomProps, it could be too restrictive. Worth considering whetherdimensionshould beDimension | Dimension[]or if the constraint should be loosened for broader applicability.
68-69: Theuseprefix onuseCustomPropsimplies a React hook, but the signature is a plain async function.In the React ecosystem, the
useprefix conventionally signals a React hook (subject to the Rules of Hooks). Since this is just an async transformation function on the theme object — not a hook — the naming could mislead consumers into thinking it follows hook semantics. Something likecustomizePropsortransformPropswould avoid that confusion.embeddable.theme.ts (1)
9-9: Thetypeof props === 'object' && props !== nullguard is redundant.Since
T extends BaseChartTypes(which is an object type),propswill always be an object when TypeScript types are respected. This check adds noise without practical value for an example. Minor point though — no action needed if you prefer the defensive style.
| const manipulatedData = data.map((row) => { | ||
| // Example manipulation: add a new field to each row based on existing fields | ||
| return { | ||
| ...row, | ||
| manipulatedValue: row[props.dimension.name] * 2, // Just an example manipulation | ||
| }; | ||
| }); |
There was a problem hiding this comment.
row[props.dimension.name] * 2 will likely produce NaN — dimension values are typically strings.
Dimension values (e.g., category names like "Q1", "Electronics") are usually strings, so multiplying by 2 yields NaN. Even as example code, this would silently corrupt the data and confuse anyone using it as a reference. Consider using a measure field for the arithmetic, or a string transformation for the dimension:
Example fix using a measure value instead
const manipulatedData = data.map((row) => {
return {
...row,
- manipulatedValue: row[props.dimension.name] * 2, // Just an example manipulation
+ manipulatedValue: row[props.measures[0]?.name] * 2, // Example: double the first measure's value
};
});📝 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.
| const manipulatedData = data.map((row) => { | |
| // Example manipulation: add a new field to each row based on existing fields | |
| return { | |
| ...row, | |
| manipulatedValue: row[props.dimension.name] * 2, // Just an example manipulation | |
| }; | |
| }); | |
| const manipulatedData = data.map((row) => { | |
| // Example manipulation: add a new field to each row based on existing fields | |
| return { | |
| ...row, | |
| manipulatedValue: row[props.measures[0]?.name] * 2, // Example: double the first measure's value | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In `@embeddable.theme.ts` around lines 12 - 18, The current manipulation in
manipulatedData uses row[props.dimension.name] * 2 which will produce NaN
because dimensions are usually strings; change manipulatedData (the data.map
callback) to use a numeric measure instead (e.g., row[props.measure.name] or
props.measures[0].name) for arithmetic or explicitly coerce and validate the
value (parseFloat/Number with isFinite check and fallback) before assigning
manipulatedValue, and add a guard so manipulatedValue is set to a safe default
when the source is not a valid number.
|
|
||
| The `useCustomProps` hook should take two arguments, the props, and an additional string telling it which chart is being rendered. This allows the hook to be used for multiple charts and manipulate the props differently based on the chart type. | ||
| */ | ||
| const [customProps, setCustomProps] = useState<BarChartDefaultProProps>(props); |
There was a problem hiding this comment.
Initial state mirrors props, so the chart briefly renders with unmodified data before the async hook resolves.
This means users see the original data flash before the manipulated data appears. Depending on how different the transformed data is, this could be a noticeable flicker. If this is a concern, consider initializing customProps to null and showing a loading state until the hook resolves, or keeping the current behavior if the brief flash is acceptable for your use case.
🤖 Prompt for AI Agents
In `@src/components/charts/bars/BarChartDefaultPro/index.tsx` at line 41, The
component initializes customProps with the incoming props causing a brief flash
of untransformed data; change the state initialization to null
(useState<BarChartDefaultProProps | null>(null)) and update all references to
handle null (render a loading placeholder or return null) until your async hook
sets customProps via setCustomProps; ensure prop type annotations and any
destructuring that uses customProps are guarded (e.g., in the render path and in
effects) so the component doesn't access fields on null.
| useEffect(() => { | ||
| const customPropsHook = theme.useCustomProps; | ||
| if (customPropsHook) { | ||
| customPropsHook<BarChartDefaultProProps>(props, 'BarChartDefaultPro').then((result) => | ||
| setCustomProps(result || props), | ||
| ); | ||
| } else { | ||
| setCustomProps(props); | ||
| } | ||
| }, [props, theme.useCustomProps]); |
There was a problem hiding this comment.
Race condition: stale async responses can overwrite newer state.
When props changes rapidly, multiple async useCustomProps calls are fired in parallel. There's no cleanup/cancellation, so a slow response for stale props can resolve after a newer one, overwriting customProps with outdated data. This is a well-known pitfall with async work inside useEffect.
A simple fix is to use a boolean flag that's flipped in the cleanup function:
Proposed fix with staleness guard
useEffect(() => {
+ let stale = false;
const customPropsHook = theme.useCustomProps;
if (customPropsHook) {
- customPropsHook<BarChartDefaultProProps>(props, 'BarChartDefaultPro').then((result) =>
- setCustomProps(result || props),
- );
+ customPropsHook<BarChartDefaultProProps>(props, 'BarChartDefaultPro')
+ .then((result) => {
+ if (!stale) setCustomProps(result || props);
+ })
+ .catch(() => {
+ if (!stale) setCustomProps(props);
+ });
} else {
setCustomProps(props);
}
+ return () => { stale = true; };
}, [props, theme.useCustomProps]);🤖 Prompt for AI Agents
In `@src/components/charts/bars/BarChartDefaultPro/index.tsx` around lines 43 -
52, The current useEffect launching theme.useCustomProps (customPropsHook) can
suffer a race where slower promises overwrite newer state; modify the effect in
BarChartDefaultPro to use a mounted/stale flag (e.g., let cancelled = false) and
flip it in the cleanup so that after awaiting
customPropsHook<BarChartDefaultProProps>(...) you only call
setCustomProps(result || props) when not cancelled; ensure the cleanup sets the
flag to true and that you handle both the async branch and the immediate
setCustomProps(props) branch consistently to prevent stale updates.
| customPropsHook<BarChartDefaultProProps>(props, 'BarChartDefaultPro').then((result) => | ||
| setCustomProps(result || props), | ||
| ); |
There was a problem hiding this comment.
Missing error handling on the Promise — unhandled rejection if useCustomProps throws.
The PR description mentions calling an external API. If that API call fails, the rejected Promise is completely unhandled, which will trigger an unhandled promise rejection. The component would also be stuck showing stale customProps from a previous render. Adding a .catch() that falls back to the original props would keep the chart functional.
🤖 Prompt for AI Agents
In `@src/components/charts/bars/BarChartDefaultPro/index.tsx` around lines 46 -
48, The Promise returned by customPropsHook<BarChartDefaultProProps>(props,
'BarChartDefaultPro') lacks error handling; add a .catch handler to that chain
to catch rejections and call setCustomProps(props) as a safe fallback (and
optionally log the error), so the component doesn't rely on a rejected Promise
or stale state; ensure you modify the call site where customPropsHook is invoked
and touch setCustomProps only in the success and error handlers.
|
I'm not going to address all the CodeRabbit stuff here because this code is not intended to be used. This whole branch can be ditched once it's determined if this is a project we want to undertake. If so, Rui or whomever will be the one to implement it, not me (most likely). It's just a quick example to help explain what we discussed w/ NOQ as a possbility. |
| const theme = defineTheme<Theme>(remarkableTheme, { | ||
| useCustomProps: async <T extends BaseChartTypes>(props: T, chartName: string): Promise<T> => { | ||
| // Example of a custom props hook that manipulates data from the results prop before it is used in the chart. | ||
| if (chartName === 'BarChartDefaultPro' && typeof props === 'object' && props !== null) { |
There was a problem hiding this comment.
with this approach all the charts BarChartDefaultPro will go throught the same process even if they dont need the extra data.
in the ideal scenario:
1 - bar chart that get the id-name from the api and them maps (does the api call)
2 - bar chart that gets the state-name from the api and them maps (does the api call)
3 - bar chart normal (doesnt do the api call because is not needed)
There was a problem hiding this comment.
@mad-raccoon - This example puts the useCustomProps function in the R.Pro repo just to show it off in one place, but it wouldn't actually live there. It'd just be in the customers repo. So only customers that wanted to use the hook would define it. Sorry, that wasn't clear.
There was a problem hiding this comment.
i got it, but this would mean that, if defined, it would apply to all the bar charts, correct?
my concern here is to avoid this for the barcharts that dont need it
There was a problem hiding this comment.
Ah, right - yeah, we may need to add some kind of additional way they can say that a specific chart should use the custom props hook. Because yes if they had five bar charts and only needed one of them to run this functionality, that'd be an issue. It'd run for all five.
There was a problem hiding this comment.
i have 1 idea that might do the trick
let me take some time to test it
2 participants
This shows one way of implementing a custom hook that the user could define on the theme which would allow them access to the props for any given chart, and they could then manipulate those props as they like and return them.
Apologies, the typing for TS compatibility is a hack job, but speed's of the essence in this case! 😁
This is related to an issue NOQ are having where they need to hit an external API, get some stuff from it, and then use that stuff to manipulate
props.results.datareturned byloadDatabefore doing anything in the chart. They have to do this for virtually every chart, with the results from the external API being different for each individual user and dataset, meaning potentially tens of thousands of different results.Summary by CodeRabbit
Release Notes
New Features
Refactor