Skip to content

Conversation

@pa-lem
Copy link
Contributor

@pa-lem pa-lem commented Oct 21, 2025

image image

Summary by CodeRabbit

  • New Features

    • Support for an additional validator type
    • Ability to run checks from the UI and view retry status
  • Refactor

    • Moved check/run flow to domain-driven hooks and mutations
    • Replaced direct GraphQL plumbing with typed query/mutation hooks and centralized query keys
    • Improved loading and error handling for checks and validators
  • Style

    • Removed cursor pointer from pie chart container
  • Bug Fixes

    • Fixed failing generator check retries and related loading state
  • Chores

    • Added changelog entry for the fix

@pa-lem pa-lem changed the title Ple fix retry checks ifc 1023 Fix checks retry Oct 21, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Walkthrough

Updated VALIDATIONS_ENUM_MAP to use CheckType and added CoreGeneratorValidator = "GENERATOR". Added single-id GraphQL API helpers: getValidatorsFromApi, getCheckDetailsFromApi, and runCheckFromApi; removed the Handlebars runCheck template. Introduced domain wrappers and React Query hooks: getValidators, getCheckDetails, runCheck, useGetValidatorsQuery, useGetCheckDetails, and useRunCheckMutation, plus new query keys. Migrated UI components (Checks, ChecksSummary, Check) from Apollo GraphQL useQuery/mutation flows to the new domain hooks and mutation hooks. PieChart props renamed and children removed; minor UI adjustments in checks summary.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Fix and refactor checks retry" is related to the actual changeset. The pull request includes a changelog entry stating "Resolved a problem that caused generator checks to fail when retrying requests," and the code changes substantively address this through significant refactoring of the retry mechanism—including migration from Apollo GraphQL mutations to React Query hooks, restructuring of the checks-summary component, and addition of new API and domain layer functions specifically for handling check retries. While the PR encompasses broader architectural changes (such as the Apollo-to-React Query migration and addition of generator validator support), the title accurately captures a real and important aspect of the changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ple-fix-retry-checks-IFC-1023

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the group/frontend Issue related to the frontend (React) label Oct 21, 2025
@pa-lem pa-lem changed the title Fix checks retry Fix and refactor checks retry Oct 21, 2025
@pa-lem pa-lem marked this pull request as ready for review October 22, 2025 07:23
@pa-lem pa-lem requested a review from a team as a code owner October 22, 2025 07:23
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/app/src/entities/diff/checks/checks-summary.tsx (1)

86-95: Disable “Retry all” during mutation and reflect pending state.

Prevents duplicate submissions and keeps UI consistent with per‑chart retry.

-          <Button
-            onClick={() => handleRetry("all")}
-            disabled={!isAuthenticated}
+          <Button
+            onClick={() => handleRetry("all")}
+            disabled={!isAuthenticated || isPending}
             variant="ghost"
             className="gap-1 hover:bg-neutral-200"
           >
             Retry all
-            <Icon icon="mdi:reload" className={classNames(isLoading && "animate-spin")} />
+            <Icon icon="mdi:reload" className={classNames((isPending || isLoading) && "animate-spin")} />
           </Button>
🧹 Nitpick comments (7)
frontend/app/src/shared/components/display/pie-chart.tsx (1)

38-38: Remove unnecessary empty className.

The empty string className adds no value and creates noise in the DOM.

Apply this diff to clean up:

-      <RPieChart width={100} height={60} className="">
+      <RPieChart width={100} height={60}>
frontend/app/src/entities/diff/checks/checks.tsx (1)

34-34: Optional chaining is defensive but unnecessary.

After the isPending check on line 25, validators should always be defined. The optional chaining validators?.map is safe but suggests uncertainty about the data shape.

If validators is always defined after loading, consider removing the optional chaining for clarity:

-        {validators?.map((item: any) => (
+        {validators.map((item: any) => (
frontend/app/src/entities/diff/api/get-validators-from-api.ts (3)

5-7: Prefer a list variable over an inline array literal.

Using [$id] is valid GraphQL, but passing a dedicated list variable is clearer and easier to reuse.

Proposed:

-  query GET_CORE_VALIDATORS($id: ID!) {
-    CoreValidator(proposed_change__ids: [$id]) {
+  query GET_CORE_VALIDATORS($ids: [ID!]!) {
+    CoreValidator(proposed_change__ids: $ids) {

And:

-    variables: { id: proposedChangeId },
+    variables: { ids: [proposedChangeId] },

5-6: Align names: constant vs operation.

const GET_VALIDATORS wraps query GET_CORE_VALIDATORS. Consider renaming one to match the other for grep-ability.


43-54: Add typed response/variables (if codegen is available).

Return a typed result (e.g., generated GET_CORE_VALIDATORSQuery) to avoid any downstream. If using GraphQL Code Generator, import the typed document and set the generic on query<Resp, Vars>.

frontend/app/src/entities/diff/checks/checks-summary.tsx (2)

103-103: Avoid passing a boolean to onClick.

Pass undefined when disabled to satisfy expected handler type and avoid accidental booleans.

-                <PieChart data={data} onClick={() => canRetry(data) && handleRetry(kind)}>
+                <PieChart data={data} onClick={canRetry(data) ? () => handleRetry(kind) : undefined}>

105-111: Improve retry overlay accessibility.

Make the hover‑only control keyboard‑accessible with semantics; optional but recommended.

-                    <div className="invisible absolute group-hover:visible">
+                    <div
+                      className="invisible absolute group-hover:visible"
+                      role="button"
+                      tabIndex={0}
+                      aria-label={`Retry ${schemaKindLabel[kind]?.replace("Validator", "").trim()} checks`}
+                      onKeyDown={(e) => {
+                        if ((e.key === "Enter" || e.key === " ") && canRetry(data)) handleRetry(kind);
+                      }}
+                    >
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a30f870 and 6702994.

📒 Files selected for processing (12)
  • frontend/app/src/config/constants.ts (1 hunks)
  • frontend/app/src/entities/diff/api/get-validators-from-api.ts (2 hunks)
  • frontend/app/src/entities/diff/api/run-check-from-api.ts (1 hunks)
  • frontend/app/src/entities/diff/api/runCheck.ts (0 hunks)
  • frontend/app/src/entities/diff/checks/checks-summary.tsx (4 hunks)
  • frontend/app/src/entities/diff/checks/checks.tsx (1 hunks)
  • frontend/app/src/entities/diff/domain/diff.query-keys.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-validators.query.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-validators.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/run-check.mutation.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/run-check.ts (1 hunks)
  • frontend/app/src/shared/components/display/pie-chart.tsx (1 hunks)
💤 Files with no reviewable changes (1)
  • frontend/app/src/entities/diff/api/runCheck.ts
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/app/src/entities/diff/domain/get-validators.query.ts (3)
frontend/app/src/entities/diff/api/get-validators-from-api.ts (1)
  • GetValidatorsFromApiParams (43-45)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (1)
  • proposedChangeValidatorsKeys (17-23)
frontend/app/src/entities/diff/domain/get-validators.ts (1)
  • getValidators (6-10)
frontend/app/src/entities/diff/domain/get-validators.ts (1)
frontend/app/src/entities/diff/api/get-validators-from-api.ts (2)
  • GetValidatorsFromApiParams (43-45)
  • getValidatorsFromApi (47-54)
frontend/app/src/entities/diff/domain/run-check.mutation.ts (3)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (1)
  • runCheckMutationKeys (13-15)
frontend/app/src/entities/diff/api/run-check-from-api.ts (1)
  • UpdateCheckFromApiParams (18-21)
frontend/app/src/entities/diff/domain/run-check.ts (1)
  • runCheck (5-7)
frontend/app/src/entities/diff/domain/run-check.ts (1)
frontend/app/src/entities/diff/api/run-check-from-api.ts (2)
  • UpdateCheckFromApiParams (18-21)
  • runCheckFromApi (23-31)
frontend/app/src/entities/diff/checks/checks-summary.tsx (4)
frontend/app/src/entities/diff/domain/run-check.mutation.ts (1)
  • useRunCheckMutation (7-12)
frontend/app/src/shared/api/rest/client.ts (1)
  • queryClient (11-17)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (1)
  • proposedChangeValidatorsKeys (17-23)
frontend/app/src/shared/components/buttons/retry.tsx (1)
  • Retry (12-44)
frontend/app/src/entities/diff/checks/checks.tsx (4)
frontend/app/src/entities/diff/domain/get-validators.query.ts (1)
  • useGetValidatorsQuery (7-14)
frontend/app/src/shared/components/errors/error-screen.tsx (1)
  • ErrorScreen (15-26)
frontend/app/src/entities/diff/checks/checks-summary.tsx (1)
  • ChecksSummary (33-124)
frontend/app/src/entities/diff/checks/validator.tsx (1)
  • Validator (66-133)
🔇 Additional comments (7)
frontend/app/src/config/constants.ts (1)

121-121: LGTM! Validator mapping added correctly.

The addition of CoreGeneratorValidator: "GENERATOR" to the enum map is consistent with existing validator mappings and aligns with the CoreGeneratorValidator type already present in MENU_EXCLUDELIST (line 77).

frontend/app/src/entities/diff/domain/get-validators.query.ts (1)

1-14: LGTM!

The query hook follows React Query best practices with proper query key scoping and type-safe queryOptions usage.

frontend/app/src/entities/diff/domain/run-check.mutation.ts (1)

1-12: LGTM!

The mutation hook is well-structured and follows React Query conventions. Error handling is appropriately delegated to consumers.

frontend/app/src/entities/diff/domain/get-validators.ts (1)

1-10: LGTM!

The function safely handles edge cases with optional chaining and provides a sensible default empty array.

frontend/app/src/entities/diff/domain/run-check.ts (1)

1-7: LGTM!

The domain wrapper appropriately abstracts the API layer and provides a clean interface with a semantic type alias.

frontend/app/src/entities/diff/checks/checks.tsx (2)

10-19: Excellent refactor from imperative to declarative pattern.

Removing forwardRef and using useGetValidatorsQuery simplifies the component and aligns with React Query best practices.


25-27: Good loading state handling.

The early return with LoadingIndicator improves UX and prevents rendering issues.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (2)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (2)

22-23: Add as const assertion for type safety.

The proposedChangeValidatorsKeys.all array should use as const to match the pattern of other query keys in this file (lines 4, 10, 14, 19) and ensure proper type inference.

Apply this diff:

 export const proposedChangeValidatorsKeys = {
-  all: ["proposed-change-validators"],
+  all: ["proposed-change-validators"] as const,
   allWithinProposedChange: (proposedChangeId: string) => [

24-27: Add as const assertion for type safety.

The allWithinProposedChange function should return a readonly tuple by adding as const to match the pattern on line 6.

Apply this diff:

   allWithinProposedChange: (proposedChangeId: string) => [
     ...proposedChangeValidatorsKeys.all,
     proposedChangeId,
-  ],
+  ] as const,
 };
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6702994 and aff1d28.

📒 Files selected for processing (7)
  • changelog/+checks.fixed.md (1 hunks)
  • frontend/app/src/entities/diff/api/get-check-details-from-api.ts (2 hunks)
  • frontend/app/src/entities/diff/checks/check.tsx (3 hunks)
  • frontend/app/src/entities/diff/domain/diff.query-keys.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-check-details.query.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-check-details.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-validators.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • changelog/+checks.fixed.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/app/src/entities/diff/domain/get-validators.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/app/src/entities/diff/domain/get-check-details.query.ts (3)
frontend/app/src/entities/diff/api/get-check-details-from-api.ts (1)
  • GetCheckDetailsFromApiParams (69-71)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (1)
  • getCheckQueryKeys (13-16)
frontend/app/src/entities/diff/domain/get-check-details.ts (1)
  • getCheckDetails (6-12)
frontend/app/src/entities/diff/domain/get-check-details.ts (1)
frontend/app/src/entities/diff/api/get-check-details-from-api.ts (2)
  • GetCheckDetailsFromApiParams (69-71)
  • getCheckDetailsFromApi (73-80)
frontend/app/src/entities/diff/checks/check.tsx (3)
frontend/app/src/entities/diff/domain/get-check-details.query.ts (1)
  • useGetCheckDetailsQuery (7-14)
frontend/app/src/shared/components/errors/error-screen.tsx (1)
  • ErrorScreen (15-26)
frontend/app/src/shared/components/display/date-display.tsx (1)
  • DateDisplay (17-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: coverall-report
  • GitHub Check: documentation
🔇 Additional comments (5)
frontend/app/src/entities/diff/domain/get-check-details.query.ts (1)

7-14: LGTM!

The hook is well-structured and follows React Query best practices. The use of queryOptions ensures proper type inference.

frontend/app/src/entities/diff/api/get-check-details-from-api.ts (2)

5-67: LGTM!

The GraphQL query is comprehensive and covers all necessary fields including type-specific fragments for CoreDataCheck, CoreSchemaCheck, CoreFileCheck, and CoreArtifactCheck.


69-80: LGTM!

The interface and function implementation are clean and follow established patterns in the codebase.

frontend/app/src/entities/diff/checks/check.tsx (2)

77-89: LGTM!

The migration from Apollo's useQuery to the domain-specific useGetCheckDetailsQuery hook is well-executed. Error and loading states are properly handled with dedicated components, improving user experience.


137-142: LGTM!

The data access patterns correctly use optional chaining and provide appropriate fallbacks (display_label when name is unavailable, conditional rendering for created_at).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

♻️ Duplicate comments (2)
frontend/app/src/shared/components/display/pie-chart.tsx (1)

32-32: Restore click affordance and add keyboard/a11y semantics when clickable.

Bring back the pointer cursor and make the div operable via Enter/Space only when onClick exists.

-    <div className={"relative"} onClick={handleClick}>
+    <div
+      className={`relative ${onClick ? "cursor-pointer" : ""}`}
+      onClick={handleClick}
+      role={onClick ? "button" : undefined}
+      tabIndex={onClick ? 0 : undefined}
+      onKeyDown={(e) => {
+        if (!onClick) return;
+        if (e.key === "Enter" || e.key === " ") {
+          e.preventDefault();
+          onClick();
+        }
+      }}
+    >
frontend/app/src/entities/diff/checks/checks-summary.tsx (1)

51-66: Harden handleRetry: remove non-null assertions, add guards, reentrancy block, and onError toast.

Unsafe ! on proposedChangeId and VALIDATIONS_ENUM_MAP[validator], no auth/ID checks, no onError, and re-entrancy not blocked. Also async is unused. Apply:

-  const handleRetry = async (validator: string) => {
-    mutate(
-      {
-        proposedChangeId: proposedChangeId!,
-        checkType: VALIDATIONS_ENUM_MAP[validator]!,
-      },
-      {
-        onSuccess: () => {
-          queryClient.invalidateQueries({
-            queryKey: proposedChangeValidatorsKeys.allWithinProposedChange(proposedChangeId!),
-          });
-          toast(<Alert type={ALERT_TYPES.SUCCESS} message="Checks are running" />);
-        },
-      }
-    );
-  };
+  const handleRetry = (validator: keyof typeof VALIDATIONS_ENUM_MAP) => {
+    if (isPending) return;
+    if (!isAuthenticated) {
+      toast(<Alert type={ALERT_TYPES.ERROR} message="You must be signed in to retry checks" />);
+      return;
+    }
+    const pcId = proposedChangeId;
+    if (!pcId) {
+      toast(<Alert type={ALERT_TYPES.ERROR} message="Missing proposed change id" />);
+      return;
+    }
+    const checkType = VALIDATIONS_ENUM_MAP[validator];
+    if (!checkType) {
+      toast(<Alert type={ALERT_TYPES.ERROR} message={`Unknown validator: ${String(validator)}`} />);
+      return;
+    }
+    mutate(
+      { proposedChangeId: pcId, checkType },
+      {
+        onSuccess: () => {
+          queryClient.invalidateQueries({
+            queryKey: proposedChangeValidatorsKeys.allWithinProposedChange(pcId),
+          });
+          toast(<Alert type={ALERT_TYPES.SUCCESS} message="Checks are running" />);
+        },
+        onError: () => {
+          toast(<Alert type={ALERT_TYPES.ERROR} message="Failed to start checks" />);
+        },
+      }
+    );
+  };
🧹 Nitpick comments (4)
frontend/app/src/shared/components/display/pie-chart.tsx (3)

3-6: Tighten prop types (avoid any[] and Function).

Stronger typing improves safety and editor help.

-type PieChartProps = {
-  data: any[];
-  onClick?: Function;
-};
+type SliceDatum = { name: string; value: number; className?: string };
+type PieChartProps = {
+  data: SliceDatum[];
+  onClick?: () => void;
+};

22-24: Minor: inline destructuring is fine; consider typing handlers for clarity.
No behavior change, but adding explicit types helps future edits.

-export const PieChart = (props: PieChartProps) => {
-  const { data, onClick } = props;
+export const PieChart = ({ data, onClick }: PieChartProps) => {
+  const handleClick: () => void = () => {
+    if (!onClick) return;
+    onClick();
+  };

1-1: Alias Recharts Tooltip to avoid confusion with app UI Tooltip.

Project also exports a Tooltip component (frontend/app/src/shared/components/ui/tooltip.tsx). Alias the Recharts one to make intent obvious.

-import { Cell, Pie, PieChart as RPieChart, Tooltip } from "recharts";
+import { Cell, Pie, PieChart as RPieChart, Tooltip as RechartsTooltip } from "recharts";
...
-        <Tooltip content={renderCustomizedTooltip} />
+        <RechartsTooltip content={renderCustomizedTooltip} />

If both tooltips appear in this module in future, this avoids naming collisions. Based on relevant UI tooltip file.

Also applies to: 47-47

frontend/app/src/entities/diff/checks/checks-summary.tsx (1)

104-116: A11y: reveal controls on keyboard focus, not only hover.

Add focus styles so keyboard users can access Retry; today it's hover-only.

-                    className={classNames(
-                      "text-xs",
-                      canRetry(data) && "absolute text-xs group-hover:invisible"
-                    )}
+                    className={classNames(
+                      "text-xs",
+                      canRetry(data) && "absolute text-xs group-hover:invisible group-focus-within:invisible"
+                    )}
...
-                    <div className="invisible absolute group-hover:visible">
+                    <div className="invisible absolute group-hover:visible group-focus-within:visible">

Consider updating Retry to render a native with tabIndex, role, and keyboard handlers. I can draft that if helpful.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d05267 and 6b4c274.

📒 Files selected for processing (2)
  • frontend/app/src/entities/diff/checks/checks-summary.tsx (5 hunks)
  • frontend/app/src/shared/components/display/pie-chart.tsx (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/src/shared/components/display/pie-chart.tsx (1)
frontend/app/src/shared/components/ui/tooltip.tsx (1)
  • Tooltip (11-40)
frontend/app/src/entities/diff/checks/checks-summary.tsx (10)
frontend/app/src/entities/schema/stores/schema.atom.ts (1)
  • genericSchemasAtom (12-12)
frontend/app/src/entities/authentication/ui/useAuth.tsx (1)
  • useAuth (173-175)
frontend/app/src/entities/diff/domain/run-check.mutation.ts (1)
  • useRunCheckMutation (7-12)
frontend/app/src/config/constants.ts (1)
  • VALIDATIONS_ENUM_MAP (118-126)
frontend/app/src/shared/api/rest/client.ts (1)
  • queryClient (11-17)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (1)
  • proposedChangeValidatorsKeys (22-26)
frontend/app/src/shared/components/ui/alert.tsx (1)
  • Alert (31-142)
frontend/app/src/shared/components/buttons/retry.tsx (1)
  • Retry (12-44)
frontend/app/src/shared/components/display/pie-chart.tsx (1)
  • PieChart (22-51)
frontend/app/src/shared/utils/common.ts (1)
  • classNames (5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: E2E-testing-version-upgrade / From 1.3.0
  • GitHub Check: E2E-testing-playwright
  • GitHub Check: E2E-testing-invoke-demo-start
  • GitHub Check: documentation
🔇 Additional comments (3)
frontend/app/src/entities/diff/checks/checks-summary.tsx (3)

11-11: Wiring looks good.

Imports of queryClient, mutation hook, and query keys + use of isPending are appropriate.

Also applies to: 20-20, 25-25, 39-39


102-102: Decoupling chart click from retry is good.

Cleaner separation; Retry is now explicit.


11-11: Single shared QueryClient instance confirmed.

Verification shows both QueryClientProvider instances (in app.tsx and render.tsx) import and use the same queryClient exported from @/shared/api/rest/client. The import in checks-summary.tsx references the identical instance—invalidation will fire correctly.

@@ -0,0 +1 @@
Fix check retry request and loading state No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

changelog is not by user oriented. We write it for them

@pa-lem pa-lem requested a review from bilalabbad October 23, 2025 12:11
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (3)
frontend/app/src/entities/diff/checks/checks-summary.tsx (3)

26-29: Tighten prop types (avoid any[]).
Use the minimal readonly shape you actually need (__typename).

-type ChecksSummaryProps = {
-  validators: any[];
-  isLoading: boolean;
-};
+type ChecksSummaryProps = {
+  validators: ReadonlyArray<{ __typename: string }>;
+  isLoading: boolean;
+};

50-64: Harden mutation: remove non‑null assertions, add guards and onError.
Prevent runtime throws and surface failures to users.

-  const handleRetry = async (validator: string) => {
-    mutate(
-      {
-        proposedChangeId: proposedChangeId!,
-        checkType: VALIDATIONS_ENUM_MAP[validator]!,
-      },
-      {
-        onSuccess: () => {
-          queryClient.invalidateQueries({
-            queryKey: proposedChangeValidatorsKeys.allWithinProposedChange(proposedChangeId!),
-          });
-          toast(<Alert type={ALERT_TYPES.SUCCESS} message="Checks are running" />);
-        },
-      }
-    );
-  };
+  const handleRetry = async (validator: string) => {
+    if (isPending) return;
+    const pcId = proposedChangeId;
+    if (!pcId) {
+      toast(<Alert type={ALERT_TYPES.ERROR} message="Missing proposed change id" />);
+      return;
+    }
+    const checkType = VALIDATIONS_ENUM_MAP[validator as keyof typeof VALIDATIONS_ENUM_MAP];
+    if (!checkType) {
+      toast(<Alert type={ALERT_TYPES.ERROR} message={`Unknown validator: ${validator}`} />);
+      return;
+    }
+    mutate(
+      { proposedChangeId: pcId, checkType },
+      {
+        onSuccess: () => {
+          queryClient.invalidateQueries({
+            queryKey: proposedChangeValidatorsKeys.allWithinProposedChange(pcId),
+          });
+          toast(<Alert type={ALERT_TYPES.SUCCESS} message="Checks are running" />);
+        },
+        onError: () => {
+          toast(<Alert type={ALERT_TYPES.ERROR} message="Failed to start checks" />);
+        },
+      }
+    );
+  };

113-121: Per‑validator Retry: gate by auth; compute in‑progress from stats.
data.inProgress doesn’t exist; also block unauthenticated clicks.

                   {canRetry(data) && (
                     <div className="invisible absolute group-hover:visible">
                       <Retry
-                        isLoading={isPending || isLoading || !!data.inProgress}
-                        isDisabled={!canRetry(data)}
-                        onClick={() => canRetry(data) && handleRetry(kind)}
+                        isLoading={
+                          isPending ||
+                          isLoading ||
+                          data.some((s: any) => s.name === CHECKS_LABEL.IN_PROGRESS && !!(s as any).value)
+                        }
+                        isDisabled={!isAuthenticated || !canRetry(data)}
+                        onClick={() => isAuthenticated && canRetry(data) && handleRetry(kind)}
                       />
                     </div>
                   )}
🧹 Nitpick comments (2)
frontend/app/src/entities/diff/domain/run-check.mutation.ts (1)

5-9: Type the mutation and allow options pass-through for better DX.

Annotate generics so mutate() is correctly typed, and optionally accept options to extend handlers.

-import { useMutation } from "@tanstack/react-query";
+import { useMutation, type UseMutationOptions } from "@tanstack/react-query";
+import type { CheckType } from "@/shared/api/graphql/generated/graphql";

 import { runCheck } from "@/entities/diff/domain/run-check";

-export function useRunCheckMutation() {
-  return useMutation({
-    mutationFn: runCheck,
-  });
-}
+type Vars = { proposedChangeId: string; checkType: CheckType };
+export function useRunCheckMutation(
+  options?: UseMutationOptions<void, unknown, Vars, unknown>
+) {
+  return useMutation<void, unknown, Vars>({
+    mutationFn: runCheck,
+    ...options,
+  });
+}
frontend/app/src/entities/diff/checks/checks.tsx (1)

10-19: Optional: guard missing route param instead of non‑null assertion.

Avoid proposedChangeId!; render an error if absent.

 export const Checks = () => {
   const { proposedChangeId } = useParams();

-  const {
+  if (!proposedChangeId) {
+    return <ErrorScreen message="Missing proposed change id." />;
+  }
+
+  const {
     isPending,
     error,
     data: validators,
   } = useGetValidatorsQuery({
-    proposedChangeId: proposedChangeId!,
+    proposedChangeId,
   });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 53d073c and ac4182d.

📒 Files selected for processing (16)
  • changelog/+checks.fixed.md (1 hunks)
  • frontend/app/src/config/constants.ts (2 hunks)
  • frontend/app/src/entities/diff/api/get-check-details-from-api.ts (2 hunks)
  • frontend/app/src/entities/diff/api/get-validators-from-api.ts (1 hunks)
  • frontend/app/src/entities/diff/api/run-check-from-api.ts (1 hunks)
  • frontend/app/src/entities/diff/checks/check.tsx (3 hunks)
  • frontend/app/src/entities/diff/checks/checks-summary.tsx (5 hunks)
  • frontend/app/src/entities/diff/checks/checks.tsx (1 hunks)
  • frontend/app/src/entities/diff/checks/validator-details.tsx (0 hunks)
  • frontend/app/src/entities/diff/domain/diff.query-keys.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-check-details.query.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-check-details.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-validators.query.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/get-validators.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/run-check.mutation.ts (1 hunks)
  • frontend/app/src/entities/diff/domain/run-check.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • frontend/app/src/entities/diff/checks/validator-details.tsx
✅ Files skipped from review due to trivial changes (1)
  • changelog/+checks.fixed.md
🚧 Files skipped from review as they are similar to previous changes (7)
  • frontend/app/src/entities/diff/domain/get-validators.query.ts
  • frontend/app/src/entities/diff/domain/diff.query-keys.ts
  • frontend/app/src/entities/diff/api/get-check-details-from-api.ts
  • frontend/app/src/config/constants.ts
  • frontend/app/src/entities/diff/api/get-validators-from-api.ts
  • frontend/app/src/entities/diff/domain/get-check-details.ts
  • frontend/app/src/entities/diff/domain/get-check-details.query.ts
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/app/src/entities/diff/domain/run-check.mutation.ts (1)
frontend/app/src/entities/diff/domain/run-check.ts (1)
  • runCheck (11-13)
frontend/app/src/entities/diff/domain/get-validators.ts (1)
frontend/app/src/entities/diff/api/get-validators-from-api.ts (1)
  • getValidatorsFromApi (49-54)
frontend/app/src/entities/diff/domain/run-check.ts (1)
frontend/app/src/entities/diff/api/run-check-from-api.ts (1)
  • runCheckFromApi (24-32)
frontend/app/src/entities/diff/checks/checks-summary.tsx (9)
frontend/app/src/entities/schema/stores/schema.atom.ts (1)
  • genericSchemasAtom (12-12)
frontend/app/src/entities/authentication/ui/useAuth.tsx (1)
  • useAuth (173-175)
frontend/app/src/entities/diff/domain/run-check.mutation.ts (1)
  • useRunCheckMutation (5-9)
frontend/app/src/config/constants.ts (1)
  • VALIDATIONS_ENUM_MAP (120-128)
frontend/app/src/shared/api/rest/client.ts (1)
  • queryClient (11-17)
frontend/app/src/entities/diff/domain/diff.query-keys.ts (1)
  • proposedChangeValidatorsKeys (18-22)
frontend/app/src/shared/components/buttons/retry.tsx (1)
  • Retry (12-44)
frontend/app/src/shared/components/display/pie-chart.tsx (1)
  • PieChart (22-51)
frontend/app/src/shared/utils/common.ts (1)
  • classNames (5-7)
frontend/app/src/entities/diff/checks/checks.tsx (4)
frontend/app/src/entities/diff/domain/get-validators.query.ts (1)
  • useGetValidatorsQuery (6-13)
frontend/app/src/shared/components/errors/error-screen.tsx (1)
  • ErrorScreen (15-26)
frontend/app/src/entities/diff/checks/checks-summary.tsx (1)
  • ChecksSummary (31-130)
frontend/app/src/entities/diff/checks/validator.tsx (1)
  • Validator (66-133)
frontend/app/src/entities/diff/checks/check.tsx (3)
frontend/app/src/entities/diff/domain/get-check-details.query.ts (1)
  • useGetCheckDetails (6-13)
frontend/app/src/shared/components/errors/error-screen.tsx (1)
  • ErrorScreen (15-26)
frontend/app/src/shared/components/display/date-display.tsx (1)
  • DateDisplay (17-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: E2E-testing-version-upgrade / From 1.3.0
  • GitHub Check: E2E-testing-invoke-demo-start
  • GitHub Check: frontend-tests
  • GitHub Check: E2E-testing-playwright
  • GitHub Check: documentation
  • GitHub Check: validate-generated-documentation
  • GitHub Check: backend-validate-generated
🔇 Additional comments (2)
frontend/app/src/entities/diff/api/run-check-from-api.ts (1)

9-20: LGTM — mutation wiring and types look consistent.
Using generated types and passing variables explicitly keeps this safe and maintainable.

Also applies to: 22-32

frontend/app/src/entities/diff/domain/run-check.ts (1)

5-13: LGTM — type-safe domain wrapper.
Clear boundary and correct use of generated CheckType.

@pa-lem pa-lem merged commit 6364396 into stable Oct 25, 2025
35 checks passed
@pa-lem pa-lem deleted the ple-fix-retry-checks-IFC-1023 branch October 25, 2025 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/frontend Issue related to the frontend (React)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants