-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Feature/57688 Spike testing primer React components on backlogs #21568
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.
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
df95376 to
7e70042
Compare
| @@ -0,0 +1,64 @@ | |||
| import React, { useState } from 'react'; | |||
| import { Avatar, Button, FormControl, SelectPanel } from '@primer/react'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the problem, remove the unused Avatar import from the import statement while keeping the other used imports (Button, FormControl, SelectPanel). This eliminates the unused symbol without affecting runtime behavior.
Concretely, in frontend/src/react/backlogs/AssigneeSelect.tsx, on the line importing from @primer/react, delete Avatar, from the destructuring import list. No other code changes or new imports are required, as Avatar is not referenced elsewhere.
-
Copy modified line R2
| @@ -1,5 +1,5 @@ | ||
| import React, { useState } from 'react'; | ||
| import { Avatar, Button, FormControl, SelectPanel } from '@primer/react'; | ||
| import { Button, FormControl, SelectPanel } from '@primer/react'; | ||
| import { TriangleDownIcon } from '@primer/octicons-react'; | ||
| import { type ActionListItemInput } from '@primer/react/deprecated'; | ||
| import { User, useAssignableUsers } from './queries'; |
| })); | ||
| }; | ||
|
|
||
| const handleSave = () => { |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, an unused function variable should either be removed if it is truly redundant or be referenced where appropriate if it represents intended behavior. Here, handleSave is clearly meant to be the click handler for the Save (CheckIcon) button in edit mode.
The minimal, behavior-preserving fix is:
- Leave the
handleSavedefinition as-is. - Update the
IconButtonwithCheckIconin the editing branch to useonClick={handleSave}instead ofonClick={() => {}}.
This change is confined to frontend/src/react/backlogs/BacklogHeader.tsx, around the JSX returned when isEditing is true (lines 52–67 in the snippet). No new imports or helper methods are required.
-
Copy modified line R65
| @@ -62,7 +62,7 @@ | ||
| <InlineDateRangeField value={[formValues.startDate, formValues.endDate]} | ||
| onChange={([newstartDate, newEndDate]) => { handleInputChange('startDate', newstartDate); handleInputChange('endDate', newEndDate); }}></InlineDateRangeField> | ||
| </Stack.Item> | ||
| <IconButton icon={CheckIcon} onClick={() => {}} variant="primary" aria-label="Save" /> | ||
| <IconButton icon={CheckIcon} onClick={handleSave} variant="primary" aria-label="Save" /> | ||
| <IconButton icon={UndoIcon} aria-label="Cancel" onClick={handleCancel} /> | ||
| </Stack> | ||
| ); |
| const types = typesQuery.data?._embedded.elements ?? []; | ||
| const statuses = statusesQuery.data?._embedded.elements ?? []; | ||
| const [stories, setStories] = useState<StoryExpanded[]>([]); | ||
| const { mutate, error, isSuccess } = useSubmitForm2(projectIdentifier, backlog.sprint.id); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the problem, we should stop destructuring variables from useSubmitForm2 that are never used. This keeps the code clear and avoids misleading readers into thinking those values are handled.
The best minimal fix is to change the destructuring on line 21 so it only pulls out mutate, which is the only property used in this component. That means editing const { mutate, error, isSuccess } = useSubmitForm2(...) to const { mutate } = useSubmitForm2(...). This change is confined to frontend/src/react/backlogs/BacklogTable.tsx, and no new imports or additional code are needed.
-
Copy modified line R21
| @@ -18,7 +18,7 @@ | ||
| const types = typesQuery.data?._embedded.elements ?? []; | ||
| const statuses = statusesQuery.data?._embedded.elements ?? []; | ||
| const [stories, setStories] = useState<StoryExpanded[]>([]); | ||
| const { mutate, error, isSuccess } = useSubmitForm2(projectIdentifier, backlog.sprint.id); | ||
| const { mutate } = useSubmitForm2(projectIdentifier, backlog.sprint.id); | ||
|
|
||
| useEffect(() => { | ||
| if (!types.length || !statuses.length) return; |
| const types = typesQuery.data?._embedded.elements ?? []; | ||
| const statuses = statusesQuery.data?._embedded.elements ?? []; | ||
| const [stories, setStories] = useState<StoryExpanded[]>([]); | ||
| const { mutate, error, isSuccess } = useSubmitForm2(projectIdentifier, backlog.sprint.id); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the problem, we should stop binding the unused isSuccess property when destructuring the hook result. This keeps the hook call intact while removing the unused local variable, exactly as the rule recommends.
Concretely, in frontend/src/react/backlogs/BacklogTable.tsx, on the line:
const { mutate, error, isSuccess } = useSubmitForm2(projectIdentifier, backlog.sprint.id);we should remove isSuccess from the destructuring, leaving only the actually used properties (mutate and error). No other code changes are needed, and we don’t need any new imports or helper functions.
-
Copy modified line R21
| @@ -18,7 +18,7 @@ | ||
| const types = typesQuery.data?._embedded.elements ?? []; | ||
| const statuses = statusesQuery.data?._embedded.elements ?? []; | ||
| const [stories, setStories] = useState<StoryExpanded[]>([]); | ||
| const { mutate, error, isSuccess } = useSubmitForm2(projectIdentifier, backlog.sprint.id); | ||
| const { mutate, error } = useSubmitForm2(projectIdentifier, backlog.sprint.id); | ||
|
|
||
| useEffect(() => { | ||
| if (!types.length || !statuses.length) return; |
| setStories(expanded.sort((a, b) => a.position - b.position)); | ||
| }, [backlog.stories, types, statuses]); | ||
|
|
||
| const totalPoints = stories |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, to fix an unused-variable issue you either (a) remove the variable and its computation if truly unnecessary, or (b) start using it where it was intended. Since there is no indication in this snippet that totalPoints should be displayed or otherwise used, the minimal, non-behavior-changing fix is to remove its declaration.
Concretely, in frontend/src/react/backlogs/BacklogTable.tsx, delete the lines that compute totalPoints (lines 36–38). No additional imports or definitions are required. This change does not affect any other logic in the component and simply eliminates dead code.
| @@ -33,10 +33,6 @@ | ||
| setStories(expanded.sort((a, b) => a.position - b.position)); | ||
| }, [backlog.stories, types, statuses]); | ||
|
|
||
| const totalPoints = stories | ||
| .map((story) => story.story_points ?? 0) | ||
| .reduce((accum, value) => accum + value, 0); | ||
|
|
||
| const moveItem = useCallback((dragIndex:number, hoverIndex:number) => { | ||
| setStories(prev => { | ||
| const updated = [...prev].sort((a, b) => a.position - b.position); |
| @@ -0,0 +1,233 @@ | |||
| import '@primer/primitives/dist/css/functional/themes/light.css'; | |||
| import { Avatar, BaseStyles, ThemeProvider, Text, IconButton, CounterLabel } from '@primer/react'; | |||
| import React, { useMemo, useState } from 'react'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix an unused import, you remove just that unused symbol from the import statement, keeping the rest unchanged so existing behavior is not affected. Here, we should keep the default React import and useState (since they may be used elsewhere in the omitted code) and remove only useMemo, which CodeQL reports as unused.
Concretely, in frontend/src/react/backlogs/TaskboardContainer.tsx, locate the React import at line 3:
import React, { useMemo, useState } from 'react';Update it to remove useMemo from the named imports:
import React, { useState } from 'react';No other code changes or additional imports are required, and no functionality will change, since useMemo was unused.
-
Copy modified line R3
| @@ -1,6 +1,6 @@ | ||
| import '@primer/primitives/dist/css/functional/themes/light.css'; | ||
| import { Avatar, BaseStyles, ThemeProvider, Text, IconButton, CounterLabel } from '@primer/react'; | ||
| import React, { useMemo, useState } from 'react'; | ||
| import React, { useState } from 'react'; | ||
| import { DndProvider } from 'react-dnd'; | ||
| import { HTML5Backend } from 'react-dnd-html5-backend'; | ||
| import { |
| QueryClient, | ||
| QueryClientProvider, | ||
| } from '@tanstack/react-query'; | ||
| import { Story, Task, Status, useProjectQueries, useTaskboardQueries, User } from './queries'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, to fix an unused import, remove the unused name from the import list, or if the whole import is unused, delete the entire import statement. This avoids confusion and any small overhead in tooling.
Here, we only need to edit the import on line 10 in frontend/src/react/backlogs/TaskboardContainer.tsx. The best fix without altering functionality is to remove User from the destructured import from ./queries, leaving the rest of the imported symbols unchanged. No new methods, imports, or definitions are required; we are only simplifying the import list.
-
Copy modified line R10
| @@ -7,7 +7,7 @@ | ||
| QueryClient, | ||
| QueryClientProvider, | ||
| } from '@tanstack/react-query'; | ||
| import { Story, Task, Status, useProjectQueries, useTaskboardQueries, User } from './queries'; | ||
| import { Story, Task, Status, useProjectQueries, useTaskboardQueries } from './queries'; | ||
| import { I18nProvider } from '../hooks/useI18n'; | ||
| import { useProjectIdentifier } from '../hooks/useProjectIdentifier'; | ||
| import TaskDialog, { TaskDialogFormData } from './TaskDialog'; |
| import { useCreateTask } from './mutations'; | ||
| import { PlusIcon } from '@primer/octicons-react'; | ||
| import styles from './TaskboardContainer.module.css'; | ||
| import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the problem, remove the unused import of PathHelperService from this file. This eliminates dead code and avoids confusion without changing any runtime behavior, since the imported symbol is not referenced.
Concretely, in frontend/src/react/backlogs/TaskboardContainer.tsx, delete the line:
import { PathHelperService } from 'core-app/core/path-helper/path-helper.service';No additional methods, imports, or definitions are required, as nothing in the shown code depends on PathHelperService.
| @@ -14,7 +14,6 @@ | ||
| import { useCreateTask } from './mutations'; | ||
| import { PlusIcon } from '@primer/octicons-react'; | ||
| import styles from './TaskboardContainer.module.css'; | ||
| import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; | ||
| import { Principal } from './Principal'; | ||
|
|
||
| const queryClient = new QueryClient(); |
| import { useMutation, useQueryClient } from '@tanstack/react-query'; | ||
| import { getMetaContent } from 'core-app/core/setup/globals/global-helpers'; | ||
| import { useEffect, useState } from 'react'; | ||
| import { Backlog, Story, Task } from './queries'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix unused imports, we remove only the identifiers that are not used anywhere, while preserving any that are used so we don’t break type checking or code relying on them. In this file, Backlog and Task are not referenced, but Story is used as a type parameter in both useSubmitForm and useSubmitForm2.
Concretely, in frontend/src/react/backlogs/mutations.ts, edit the import on line 4 so that it only imports Story from ./queries. No additional imports or code changes are needed, since nothing else depends on Backlog or Task. This keeps existing functionality intact and resolves the static analysis warning.
-
Copy modified line R4
| @@ -1,7 +1,7 @@ | ||
| import { useMutation, useQueryClient } from '@tanstack/react-query'; | ||
| import { getMetaContent } from 'core-app/core/setup/globals/global-helpers'; | ||
| import { useEffect, useState } from 'react'; | ||
| import { Backlog, Story, Task } from './queries'; | ||
| import { Story } from './queries'; | ||
|
|
||
|
|
||
| function useMetaContent(name:string) { |
| import './backlogs/impediment'; | ||
| import './backlogs/taskboard'; | ||
| import './backlogs/show_main'; | ||
| import { createRoot, Root } from 'react-dom/client'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, unused imports should be removed to keep the code clear and avoid confusion. Here, we should adjust the import from react-dom/client to only bring in what is actually used.
The best fix is to modify the import statement on line 22 so that it only imports createRoot and drops Root. The rest of the file need not change, since the code already calls createRoot(...).render(...) without using the Root type or value. No additional imports, methods, or definitions are needed.
Concretely, in frontend/src/stimulus/controllers/dynamic/backlogs.controller.ts, update line 22 from import { createRoot, Root } from 'react-dom/client'; to import { createRoot } from 'react-dom/client';.
-
Copy modified line R22
| @@ -19,7 +19,7 @@ | ||
| import './backlogs/impediment'; | ||
| import './backlogs/taskboard'; | ||
| import './backlogs/show_main'; | ||
| import { createRoot, Root } from 'react-dom/client'; | ||
| import { createRoot } from 'react-dom/client'; | ||
| import React from 'react'; | ||
| import BacklogsContainer from '../../../react/backlogs/BacklogsContainer'; | ||
| import TaskboardContainer from '../../../react/backlogs/TaskboardContainer'; |
7e70042 to
0a041c8
Compare
Ticket
https://community.openproject.org/wp/57688
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist