Migrate Node and update all frontend dependencies #693
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pyanderson
added
enhancement
dependencies
Reviewer's GuideThis PR migrates the frontend to Node.js v22 and pnpm, refreshes all runtime and dev dependencies, introduces a unified lint/format setup, refactors the API client layer to use openapi-ts (Axios plugin), and normalizes code style and CI/build/test configurations across the project. Sequence diagram for updated usersLoader API interactionsequenceDiagram
participant UsersPage
participant usersLoader
participant API
UsersPage->>usersLoader: Request user list (with limit/offset)
usersLoader->>API: Call usersList({query, throwOnError})
API-->>usersLoader: Respond with { data: PaginatedUserList }
usersLoader-->>UsersPage: Return data.results
Note over usersLoader,API: On 401/403 error, usersLoader redirects to login
Class diagram for refactored API client usageclassDiagram
class RestService {
<<deprecated>>
+restRestCheckRetrieve()
+usersList()
}
class restRestCheckRetrieve {
<<function>>
}
class usersList {
<<function>>
+queryLimit: number
+queryOffset: number
+throwOnError: boolean
}
RestService --|> restRestCheckRetrieve : replaced by
RestService --|> usersList : replaced by
class PaginatedUserList {
+results: User[]
+count: number
}
class User {
+id: number
+username: string
+...other fields
}
PaginatedUserList "1" -- "*" User : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was
linked to
issues
Oct 17, 2025
pyanderson
changed the title
pyanderson
changed the title
fjsj
approved these changes
Oct 20, 2025
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The catch block in usersLoader checks error.status on AxiosError, but AxiosError stores the HTTP status on error.response.status—update the code to use error.response?.status.
- You’ve introduced the '@' import alias in webpack and Jest but didn’t add matching compilerOptions.paths in tsconfig.json, so the TypeScript compiler won’t resolve those modules correctly—please sync your tsconfig.json paths with your alias.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The catch block in usersLoader checks error.status on AxiosError, but AxiosError stores the HTTP status on error.response.status—update the code to use error.response?.status.
- You’ve introduced the '@' import alias in webpack and Jest but didn’t add matching compilerOptions.paths in tsconfig.json, so the TypeScript compiler won’t resolve those modules correctly—please sync your tsconfig.json paths with your alias.
## Individual Comments
### Comment 1
<location> `frontend/js/loaders/users.ts:17` </location>
<code_context>
- error instanceof ApiError &&
- (error?.status === 401 || error?.status === 403)
- ) {
+ if (error instanceof AxiosError && (error?.status === 401 || error?.status === 403)) {
const url = new URL(request.url);
const next = url.pathname + url.search + url.hash;
</code_context>
<issue_to_address>
**issue (bug_risk):** Using error?.status on AxiosError may not work as AxiosError uses 'response.status'.
Update the condition to use 'error.response?.status' instead for accurate status code checking.
</issue_to_address>
### Comment 2
<location> `frontend/js/loaders/users.ts:20` </location>
<code_context>
+ if (error instanceof AxiosError && (error?.status === 401 || error?.status === 403)) {
const url = new URL(request.url);
const next = url.pathname + url.search + url.hash;
throw redirectDocument(`/admin/login/?next=${encodeURIComponent(next)}`);
</code_context>
<issue_to_address>
**suggestion:** Redirecting on 401/403 is appropriate, but other error types are not handled.
Please add handling for other error types, such as network failures and server errors (e.g., 500s), to improve user feedback and resilience.
</issue_to_address>
### Comment 3
<location> `frontend/js/pages/__tests__/Home.spec.tsx:13-17` </location>
<code_context>
+ restRestCheckRetrieve: jest.fn(),
}));
-describe("Home", () => {
+describe('Home', () => {
beforeEach(() => {
- (RestService.restRestCheckRetrieve as jest.Mock).mockResolvedValue({
- message: "Test Result",
+ (restRestCheckRetrieve as jest.Mock).mockResolvedValue({
+ data: {
+ message: 'Test Result',
+ },
});
});
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for error handling in the Home page.
Please add a test that simulates an API error and checks that the Home page displays an appropriate error message or handles the failure without crashing.
Suggested implementation:
```typescript
describe('Home', () => {
beforeEach(() => {
(restRestCheckRetrieve as jest.Mock).mockResolvedValue({
data: {
message: 'Test Result',
},
});
});
it('displays an error message when the API call fails', async () => {
(restRestCheckRetrieve as jest.Mock).mockRejectedValue(new Error('API Error'));
// Render the Home component
const { findByText } = render(<Home />);
// Assert that the error message is displayed
expect(await findByText(/error/i)).toBeInTheDocument();
});
```
- If your Home component does not currently display an error message, you will need to update it to do so when the API call fails (e.g., by setting an error state and rendering an error message).
- You may need to import `render` and any other required testing utilities from your test library (e.g., `@testing-library/react`).
</issue_to_address>
### Comment 4
<location> `eslint.config.mjs:18` </location>
<code_context>
+import unicornPlugin from "eslint-plugin-unicorn";
+import jestPlugin from "eslint-plugin-jest";
+
+export default defineConfig([
+ // Base JS rules, with browser + node globals
+ {
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating the multiple config overrides into a single, top-level ESLint config using 'extends' and 'env' for improved maintainability.
Here’s one way to collapse all of that ceremony into a single, maintainable config that still gives you all the same rules/plugins/globals:
```js
// .eslintrc.js
import { defineConfig } from "eslint/config";
const COMMON_ENV = {
browser: true,
node: true,
jest: true,
};
export default defineConfig({
root: true,
env: COMMON_ENV,
ignorePatterns: ["node_modules/", "dist/", "build/", "frontend/js/api/", "openapi-ts.config.ts"],
parser: "@typescript-eslint/parser",
parserOptions: {
project: "./tsconfig.json",
tsconfigRootDir: __dirname,
ecmaVersion: "latest",
sourceType: "module",
ecmaFeatures: { jsx: true },
},
extends: [
"eslint:recommended",
"plugin:@typescript-eslint/recommended", // TS rules
"plugin:react/recommended", // React rules
"plugin:react-hooks/recommended",
"plugin:import/recommended",
"plugin:jsx-a11y/recommended",
"plugin:promise/recommended",
"plugin:sonarjs/recommended",
"plugin:unicorn/recommended",
"plugin:jest/recommended",
],
settings: {
react: { version: "detect" },
},
rules: {
// TS tweaks
"no-undef": "off",
"@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }],
// React tweaks
"react/prop-types": "off",
"react/react-in-jsx-scope": "off",
// import/order
"import/order": [
"warn",
{
groups: [
"builtin",
"external",
"internal",
"parent",
"sibling",
"index",
"object",
"type",
],
"newlines-between": "always",
alphabetize: { order: "asc", caseInsensitive: true },
},
],
// other
"unicorn/prefer-module": "off",
},
});
```
What changed?
1. Switched to a single top-level config instead of four almost-identical overrides.
2. Replaced all those `import`+`globals` spreads with a simple `env: { browser, node, jest }`.
3. Dropped manual plugin registrations and rule-by-rule copies in favor of `extends: […]` bundles (TypeScript, React, import, jsx-a11y, promise, sonarjs, unicorn, jest).
Everything you had is still in place, but now it’s under 50 lines instead of 200+.
</issue_to_address>
### Comment 5
<location> `frontend/js/pages/Home.tsx:12-14` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Migrates the frontend to Node.js v22, updates all runtime and dev dependencies, standardizes linting and formatting, unifies code configuration, and adjusts build/CI pipelines.
Also includes a small refactor of the API client layer.
Main changes:
Motivation and Context
Screenshots (if appropriate):
N/A
Steps to reproduce (if appropriate):
Types of changes
Checklist:
Summary by Sourcery
Migrate the frontend to Node 22, bump all dependencies, standardize linting and formatting, unify code style, update build and CI configurations, and refactor API client usage.
Enhancements:
Build:
CI:
Tests:
Chores:
Summary by Sourcery
Migrate frontend to Node.js v22, refresh all dependencies, unify lint/format configs and code style, refactor API client to use openapi-ts/axios plugin, and update build and CI pipelines accordingly.
Enhancements:
Build:
CI:
Tests: