Resolve tech debt: migrate to React hooks, fix memory leaks, expand test coverage by QilongTang · Pull Request #111 · DynamoDS/SplashScreen

QilongTang · 2026-03-31T19:45:48Z

Summary

This PR addresses a backlog of tech debt identified through a full codebase
audit. The changes span component architecture, test coverage, CI reliability,
and tooling configuration.

Component migration — class → function components

All four components (App, Dynamic, Static, Toast) have been rewritten as
function components using React hooks. This aligns with current React best
practices and makes the codebase easier to onboard contributors into.

Memory leak fixes

Every window.* method assignment (e.g. window.setBarProperties,
window.setLoadingDone, window.showToast) is now registered inside a useEffect
with a cleanup function that deletes the method on unmount. Previously, none
of these were ever cleaned up.

Bug fixes

let checked = false module-level variable in Static.js replaced with useRef
— the old code persisted checkbox state across component unmounts
document.getElementById / getElementsByClassName calls replaced with React
refs, removing fragile DOM queries with no null guards
eslint-disable no-import-assign hack removed — background image placeholder
logic rewritten as a pure expression
FileReader.onerror handler added — file read failures were previously silent
chrome.webview calls in signIn wrapped in try/catch
PropTypes.object replaced with PropTypes.shape({...}) with all label keys
typed; onCheckedChange marked .isRequired
5 broken Playwright assertions (toBeVisible missing ()) fixed — these tests
were passing vacuously

Test coverage

App.test.js: 1 smoke test → 5 behavioural tests (title rendering, phase
switching, setLabels, mount/unmount cleanup)
Dynamic.test.js: new file — 4 tests covering initial render, progress
updates, loading time, and unmount cleanup
Static.test.js: new file — 7 tests covering default labels, checkbox toggle,
loading time, import success/reset, button enable/disable, and unmount
cleanup
Total: 5 tests → 18 tests

CI / tooling

npm ci --force → npm ci in npm-publish.yml (both jobs) — --force was
bypassing peer-dependency validation
test:e2e script restored to package.json (was missing, breaking the CI build
step)
webextensions: true removed from .eslintrc.json — this project is not a
browser extension; the flag was polluting globals with extension APIs
"chrome": "readonly" global added to ESLint so WebView2 usage lints
correctly
no-console: warn rule added — surfaces any future debug logs left in
Duplicate CSS rule block removed from Dynamic.css
react-scripts removed from dependencies (project uses custom webpack; the
only use was the dead eject script, which has also been removed)
Missing devDependencies declared explicitly: @babel/core, @babel/preset-env,
@babel/preset-react, babel-loader, css-loader, style-loader,
terser-webpack-plugin
prop-types added to dependencies (runtime, was imported but undeclared)
html-webpack-plugin upgraded 4 → 5

Agent-friendliness

CLAUDE.md added — documents commands, architecture, WebView2 integration
pattern, window API, conventions, and release workflow for AI-assisted
development
.claude/settings.local.json added to .gitignore

Test plan

npm install — verify no peer dependency errors without --force
npm run build — dev build succeeds
npm run lint:check — no lint errors
npm run test:unit — all 18 tests pass
npm run start + open http://localhost:8080/?debug — Static phase loads
automatically (debug mode), all buttons render correctly
Manual: verify progress bar animates correctly during loading phase

Copilot

Pull request overview

This PR modernizes the splash screen codebase by converting key React components from class-based implementations to function components with hooks, and expands automated test coverage to support the refactor.

Changes:

Refactor App, Static, Dynamic, and Toast to functional components using React hooks and window-API registration via useEffect.
Add new unit tests for App, Static, and Dynamic, and fix Playwright assertions in the E2E test.
Update tooling/config: dependency changes (removing CRA bits, adding Babel/Webpack tooling), tighten CI install behavior (npm ci without --force), and adjust ESLint globals/rules.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/Static.test.js	Adds unit tests for Static behaviors and window APIs.
tests/Dynamic.test.js	Adds unit tests for Dynamic window API and rendering.
tests/App.test.js	Expands App tests around Dynamic/Static switching and window APIs.
tests/e2e.test.js	Fixes Playwright matcher calls (`toBeVisible()`).
src/Toast.js	Refactors Toast to hooks and keeps global show/hide APIs.
src/Static.js	Refactors Static to hooks, ref-based checkbox state, and window APIs.
src/Dynamic.js	Refactors Dynamic to hooks and window API registration.
src/Dynamic.css	Minor styling/formatting adjustments.
src/App.js	Refactors App to hooks and adjusts background image handling and debug mode.
package.json	Updates dependencies/scripts for non-CRA build/test setup.
.github/workflows/npm-publish.yml	Removes `--force` from `npm ci` steps.
.eslintrc.json	Adds `chrome` global and updates lint rules/env.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T19:51:50Z

src/App.js

+          </Row>
+        </Col>
+        <Col className='p-0'>
+          {loadingDone && <span className='close' />}


The close control is rendered as a plain with no click handler, and there’s no close styling in src/App.css. This looks like a functional regression from the previous close button behavior; either reintroduce the close handler (e.g., calling the WebView host close API) and associated styling, or remove the element entirely if closing is no longer supported.

Suggested change

{loadingDone && <span className='close' />}

Copilot · 2026-03-31T19:51:50Z

src/App.js

+              {loadingDone
+                ? <Static
+                  signInStatus={signInStatus}
+                  labels={labels}
+                  onCheckedChange={() => {}}
+                />


Static requires onCheckedChange, but App now passes a no-op (() => {}). If the parent no longer needs this callback, consider making onCheckedChange optional (and/or removing the prop) to avoid a misleading required API; otherwise wire it to real state/behavior so the checkbox change is observable by the rest of the app.

Copilot · 2026-03-31T19:51:50Z

src/Static.js

+  useEffect(() => {
+    window.setImportStatus = (status) => {
+      setImportStatus(status.status);
+      setErrorDescription(status.errorDescription || '');


window.setImportStatus currently overwrites the existing errorDescription with an empty string when status.errorDescription is falsy. This can result in a blank tooltip on error and also discards the default fallback message. Consider only updating errorDescription when a non-empty value is provided (or only when status.status === importStatusEnum.error), and otherwise keep the current/default message.

Suggested change

setErrorDescription(status.errorDescription || '');

if (status.status === importStatusEnum.error) {

const defaultErrorDescription =

'Something went wrong when importing your custom setting file. Please try again or proceed with default settings.';

const hasCustomDescription =

typeof status.errorDescription === 'string' && status.errorDescription.trim().length > 0;

setErrorDescription(hasCustomDescription ? status.errorDescription : defaultErrorDescription);

}

Copilot · 2026-03-31T19:51:51Z

package.json

@@ -33,15 +33,7 @@
    "lic_direct": "npx @adsk/adsk-npm-license-puller --path . --app-name 'splash-screen' --verbose --about-box ./license_output/about-box_direct.html --about-box-type desktop --year $(date \"+%Y\") --paos ./license_output/paos_direct.csv",
    "lic_transitive": "npx @adsk/adsk-npm-license-puller --path . --app-name 'splash-screen' --verbose --about-box ./license_output/about-box_transitive.html --about-box-type desktop --transitive --year $(date \"+%Y\") --paos ./license_output/paos_transitive.csv",
    "generate_license": "npm run lic_direct && npm run lic_transitive",
-    "test:unit": "NODE_ENV=test jest tests/App.test.js tests/Toast.test.js",
-    "test:e2e": "playwright test tests/e2e.test.js",
-    "eject": "react-scripts eject"
-  },
-  "eslintConfig": {
-    "extends": [
-      "react-app",
-      "react-app/jest"
-    ]
+    "test:unit": "NODE_ENV=test jest tests/App.test.js tests/Toast.test.js tests/Dynamic.test.js tests/Static.test.js"
  },


package.json removes react-scripts (which previously provided Jest) but the repo still relies on the jest binary in test:unit, and webpack serve typically requires a direct webpack-dev-server dependency. As-is, a clean install/CI run is likely to fail; add the missing direct devDependencies (Jest + required jsdom/babel-jest tooling, and webpack-dev-server if applicable) and regenerate/commit an updated package-lock.json so npm ci remains reliable.

QilongTang · 2026-03-31T20:11:02Z

@copilot Help me fix this PR so that build pass, tests pass and PR comments with confidence are addressed

Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/aa38d1dc-3c38-4abb-bca5-da15e953d097 Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

…x setImportStatus and onCheckedChange prop Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/aa38d1dc-3c38-4abb-bca5-da15e953d097 Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

Copilot · 2026-03-31T20:17:13Z

@copilot Help me fix this PR so that build pass, tests pass and PR comments with confidence are addressed

Fixed in commit ee6a004. Here's what was addressed:

Build/test failures — Added missing direct devDependencies that were removed when react-scripts was dropped: jest, jest-environment-jsdom, babel-jest, webpack-dev-server, eslint@^8, and eslint-plugin-react. All 18 unit tests now pass and the build succeeds.
onCheckedChange prop — Changed Static.js PropTypes from .isRequired to optional and added a no-op default in defaultProps, since App.js passes () => {} and the callback isn't needed by the parent.
window.setImportStatus bug — Fixed errorDescription to only be updated when status.status === importStatusEnum.error, preserving the default error message when no custom description is provided (instead of overwriting it with an empty string).

QilongTang · 2026-03-31T20:29:40Z

@copilot Address the merge conflict and the build is still failing, try fixing it another time.

…k-dev-server@5 Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/3763e823-c5fe-4305-95f4-9fca9414dfd7 Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

Copilot · 2026-03-31T20:33:56Z

@copilot Address the merge conflict and the build is still failing, try fixing it another time.

Fixed in commit 156ee48. The build failure was caused by a version incompatibility between webpack-cli@4.x and webpack-dev-server@5.x — webpack-cli v4 was passing an internal webpack compilation property (_assetEmittingPreviousFiles) as a dev server option, which webpack-dev-server v5 rejected with a schema validation error. Upgrading webpack-cli to ^5 resolves the incompatibility. Build, lint, and all 18 unit tests pass.

Merges three Dependabot PRs from master: - Bump brace-expansion (#110) - Bump node-forge from 1.3.2 to 1.4.0 (#107) - Bump path-to-regexp from 0.1.12 to 0.1.13 (#108) package-lock.json conflict resolved by taking TechDebts (fully regenerated lock), which subsumes master's transitive changes. Run `npm install` to produce a final lock that incorporates all new direct deps alongside these security bumps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

QilongTang · 2026-04-01T01:05:54Z

@copilot The e2e tests keeps failing, figure out if fix or re-write is required

…t width:0%) Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/81975e31-ed56-48d5-beaf-a16a3a0b841b Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

Copilot · 2026-04-01T01:09:38Z

@copilot The e2e tests keeps failing, figure out if fix or re-write is required

Fixed in commit 4254ca5. The test needed a rewrite for one assertion: .progress-bar-indicator starts at width: 0% (initial state before the WebView2 host calls setBarProperties), so Playwright's toBeVisible() correctly reports it as hidden — there's no real host available in e2e. Changed to toBeAttached(), which verifies the element is rendered in the DOM without requiring non-zero dimensions. All other assertions remain unchanged. The e2e test now passes locally and the webserver starts correctly.

Tech Debt clean up

585a989

QilongTang changed the title ~~Tech Debt clean up~~ Tech Debts Clean Up Mar 31, 2026

QilongTang requested a review from Copilot March 31, 2026 19:46

Copilot started reviewing on behalf of QilongTang March 31, 2026 19:47 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Copilot started work on behalf of QilongTang March 31, 2026 20:11 View session

QilongTang and others added 2 commits March 31, 2026 16:12

update

1aed5ff

Initial plan - no changes yet

795233c

Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/aa38d1dc-3c38-4abb-bca5-da15e953d097 Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

QilongTang changed the title ~~Tech Debts Clean Up~~ Resolve tech debt: migrate to React hooks, fix memory leaks, expand test coverage Mar 31, 2026

Copilot finished work on behalf of QilongTang March 31, 2026 20:19

Copilot started work on behalf of QilongTang March 31, 2026 20:29 View session

Fix e2e test: upgrade webpack-cli to v5 for compatibility with webpac…

156ee48

…k-dev-server@5 Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/3763e823-c5fe-4305-95f4-9fca9414dfd7 Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

Copilot finished work on behalf of QilongTang March 31, 2026 20:34

Copilot started work on behalf of QilongTang April 1, 2026 01:06 View session

Fix e2e test: use toBeAttached() for progress-bar-indicator (starts a…

4254ca5

…t width:0%) Agent-Logs-Url: https://github.com/DynamoDS/SplashScreen/sessions/81975e31-ed56-48d5-beaf-a16a3a0b841b Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>

Copilot finished work on behalf of QilongTang April 1, 2026 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve tech debt: migrate to React hooks, fix memory leaks, expand test coverage #111

Resolve tech debt: migrate to React hooks, fix memory leaks, expand test coverage #111
QilongTang wants to merge 7 commits intomasterfrom
TechDebts

QilongTang commented Mar 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

QilongTang commented Mar 31, 2026

Uh oh!

Copilot AI commented Mar 31, 2026

Uh oh!

QilongTang commented Mar 31, 2026

Uh oh!

Copilot AI commented Mar 31, 2026

Uh oh!

QilongTang commented Apr 1, 2026

Uh oh!

Copilot AI commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-      setErrorDescription(status.errorDescription || '');
+      if (status.status === importStatusEnum.error) {
+        const defaultErrorDescription =
+          'Something went wrong when importing your custom setting file. Please try again or proceed with default settings.';
+        const hasCustomDescription =
+          typeof status.errorDescription === 'string' && status.errorDescription.trim().length > 0;
+        setErrorDescription(hasCustomDescription ? status.errorDescription : defaultErrorDescription);
+      }

Conversation

QilongTang commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Component migration — class → function components

Memory leak fixes

Bug fixes

Test coverage

CI / tooling

Agent-friendliness

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Mar 31, 2026

Uh oh!

Copilot AI commented Mar 31, 2026

Uh oh!

QilongTang commented Mar 31, 2026

Uh oh!

Copilot AI commented Mar 31, 2026

Uh oh!

QilongTang commented Apr 1, 2026

Uh oh!

Copilot AI commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

QilongTang commented Mar 31, 2026 •

edited

Loading