Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't break personal dashboard charts if the flag is called . #8807

Merged
merged 3 commits into from
Nov 20, 2024
Merged

fix: don't break personal dashboard charts if the flag is called . #8807

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19d8737
1-3132: do some initial guarding for malformed features
thomasheartman Nov 20, 2024
4945735
1-3132: filter out known bad names
thomasheartman Nov 20, 2024
d73b0bd
1-3132: update text for metrics error charts
thomasheartman Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions frontend/src/component/personalDashboard/FlagMetricsChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ const EmptyFlagMetricsChart = () => {
const useMetricsEnvironments = (project: string, flagName: string) => {
const [environment, setEnvironment] = useState<string | null>(null);
const { feature } = useFeature(project, flagName);
const activeEnvironments = feature.environments.map((env) => ({
const activeEnvironments = (feature?.environments ?? []).map((env) => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the return type of useFeature lying to us? It would indicate the defensive technique is not needed looking at the useFeature return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. It's both lying and we're abusing it.

First, it's wrapping an http request. This means we might not have the data immediately, but we don't wait. Also, there may not be any data there (we might get an error), but we don't handle that (did someone say union types?).

But second, the useFeature hook returns the deprecated IFeatureToggle interface. If you check that interface, you'll find that environments is always present. But if you compare it to the FeatureSchema which supersedes it, you'll see that environments is optional (yikes!).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should fix the source of truth (the useFeature hook type) so that we don't have to catch those mistakes at runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye. I'll make a follow-up PR to gauge how much work that'd be.

name: env.name,
type: env.type,
}));
Expand Down Expand Up @@ -215,7 +215,7 @@ export const PlaceholderFlagMetricsChartWithWrapper: React.FC<{
);
};

export const FlagMetricsChart: FC<{
const FlagMetricsChartInner: FC<{
flag: { name: string; project: string };
onArchive: () => void;
}> = ({ flag, onArchive }) => {
Expand All @@ -235,7 +235,7 @@ export const FlagMetricsChart: FC<{
return (
<ChartContainer>
<PlaceholderFlagMetricsChart
label={`Couldn't fetch metrics for the current flag. This may be a transient error, or your flag name ("${flag.name}") may be causing issues.`}
label={`Couldn't fetch metrics for the current flag right now. Please try again. Report this if it doesn't resolve itself.`}
/>
</ChartContainer>
);
Expand Down Expand Up @@ -283,6 +283,24 @@ export const FlagMetricsChart: FC<{
);
};

export const FlagMetricsChart: FC<{
flag: { name: string; project: string };
onArchive: () => void;
}> = (props) => {
const breakingNames = ['.', '..'];
if (breakingNames.includes(props.flag.name)) {
return (
<ChartContainer>
<PlaceholderFlagMetricsChart
label={`The current flag name ('${props.flag.name}') is known to cause issues due how it affects URLs. We cannot show you a chart for it.`}
/>
</ChartContainer>
);
}

return <FlagMetricsChartInner {...props} />;
};

ChartJS.register(
annotationPlugin,
CategoryScale,
Expand Down
Loading