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(1-3363): frontend flag type should not return truthy for variant #9293

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(1-3363): frontend flag type should not return truthy for variant #9293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8bec94d
fix(1-3363): frontend flag type should not return truthy for variant
Tymek Feb 11, 2025
60d3df3
fix: useUiFlag type
Tymek Feb 11, 2025
2afc1fb
fix: ts check
Tymek Feb 11, 2025
c066fcc
Merge branch 'main' into 1-3363
Tymek Feb 11, 2025
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
8 changes: 4 additions & 4 deletions frontend/src/component/feedbackNew/FeedbackComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ import type { IToast } from 'interfaces/toast';
import { useTheme } from '@mui/material/styles';
import type { FeedbackData, FeedbackMode } from './FeedbackContext';
import { usePlausibleTracker } from 'hooks/usePlausibleTracker';
import { useUiFlag } from 'hooks/useUiFlag';
import useUserType from './useUserType';
import { BaseModal } from 'component/common/SidebarModal/SidebarModal';
import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';

export const ParentContainer = styled('div')(({ theme }) => ({
position: 'relative',
Expand Down Expand Up @@ -205,12 +205,13 @@ export const FeedbackComponent = ({
const userType = useUserType();
const { trackEvent } = usePlausibleTracker();
const theme = useTheme();
const { uiConfig } = useUiConfig();

const { addFeedback } = useUserFeedbackApi();
const { setHasSubmittedFeedback } = useUserSubmittedFeedback(
feedbackData.category,
);
const feedbackComments = useUiFlag('feedbackComments');
const feedbackComments = uiConfig?.flags?.feedbackComments;

function isProvideFeedbackSchema(data: any): data is ProvideFeedbackSchema {
data.difficultyScore = data.difficultyScore
Expand Down Expand Up @@ -329,8 +330,7 @@ export const FeedbackComponent = ({
</ScoreHelpContainer>
</StyledScoreContainer>

{feedbackComments !== false &&
feedbackComments.enabled &&
{feedbackComments?.enabled &&
feedbackComments.name === 'withoutComments' ? (
<>
<Box>
Expand Down
10 changes: 8 additions & 2 deletions frontend/src/hooks/useUiFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';

type flags = ReturnType<typeof useUiConfig>['uiConfig']['flags'];

export const useUiFlag = <K extends keyof flags>(flag: K) => {
export const useUiFlag = <K extends keyof flags>(flag: K): boolean => {
const { uiConfig } = useUiConfig();
const value = uiConfig?.flags?.[flag];
if (typeof value === 'boolean') {
return value;
} else if (typeof value !== 'undefined') {
console.error(`Flag ${flag} does not return a boolean.`);
}

return uiConfig?.flags?.[flag] || false;
return false;
Comment on lines +5 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

While I think this is fine in theory, I'm not confident about changing it now because we have places where we depend on the contents. Yeh, ts found the one in feedback component, but I don't really trust it to monitor everywhere. I don't see what we gain by changing this function right now. I'd rather look at other options too before changing this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's about 60 places, I can check it manually

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. That's a lot of stuff to do manually. It feels like a high risk, low reward move at this point. Lemme have a think about this. Let's continue the discussion tomorrow?

};
Loading