Skip to content

implement threshold list fetch hook & context provider#57922

Merged
nhsiehgit merged 6 commits into
masterfrom
thresholdsList_context
Oct 11, 2023
Merged

implement threshold list fetch hook & context provider#57922
nhsiehgit merged 6 commits into
masterfrom
thresholdsList_context

Conversation

@nhsiehgit

Copy link
Copy Markdown
Contributor

Implements a context provider + hook to fetch release thresholds data

@nhsiehgit nhsiehgit requested a review from a team as a code owner October 11, 2023 18:35
@nhsiehgit nhsiehgit requested review from a team and ryan953 October 11, 2023 18:35
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 11, 2023
ReturnType<typeof useFetchThresholdsListData>
>(EMPTY_THRESHOLDS_LIST_DATA);

export function ThresholdsDataContext({children, ...listDataParams}: ProviderProps) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Context provider is not utilized atm, but will be useful for https://github.com/getsentry/sentry/pull/55886/files

Comment on lines +55 to +57
// NOTE: If release thresholds are not enabled, API will return a 403 not found
// So capture this case and set enabled to false
setFeatureEnabledFlag(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the feature flag not available from the organization object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes - the feature should be available from the org object
This state flag isn't being utilized at all atm 🤷
i may remove it

Comment on lines +19 to +38
export default function useFetchThresholdsListData({
selectedEnvs,
selectedProjects,
}: HookProps) {
const api = useApi();
const organization = useOrganization();

const [isLoading, setIsLoading] = useState<boolean>(true);
const [errors, setErrors] = useState<string | null>();
const [_featureEnabledFlag, setFeatureEnabledFlag] = useState<boolean>(true);
const [thresholds, setThresholds] = useState<Threshold[]>([]);

const fetchThresholds = useCallback(async () => {
// ReleaseThresholdIndexEndpoint.as_view(),
// name="sentry-api-0-organization-release-thresholds",
const path = `/organizations/${organization.id}/releases/thresholds/`;
const query: ThresholdQuery = {};
if (selectedProjects.length) {
query.project = selectedProjects;
} else {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be using useApiQuery unless there's some reason that this can't use it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤔
hmmm. why's that?

Seems like useApiQuery is just another abstraction on top of useApi.

it unnecessarily obfuscates the implementation here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

useApiQuery is a small wrapper on react-query that uses our api client to fetch but it handles loading states, error states, stale time, refetching based on query parameters. basically everything you're doing here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmmm... i see 🤔
Ok that looks useful. will switch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 might be helpful to start by looking at one like this one https://github.com/getsentry/sentry/blob/master/static/app/actionCreators/group.tsx#L518-L545

}

const {
data: thresholds,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

giving this a default can be kinda nice. since the type goes from Threshold[] | null to Threshold[]

Suggested change
data: thresholds,
data: thresholds = [],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually - i'll just straight return the useApiQuery

@nhsiehgit nhsiehgit enabled auto-merge (squash) October 11, 2023 21:43
@nhsiehgit nhsiehgit merged commit e4fec04 into master Oct 11, 2023
@nhsiehgit nhsiehgit deleted the thresholdsList_context branch October 11, 2023 22:00
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants