chore: Removal of @tsd-ui/core dependency by stanislavsemeniuk · Pull Request #354 · securesign/rhtas-console-ui

stanislavsemeniuk · 2026-08-04T11:20:32Z

Type of Change

Testing

All tests pass (npm run test)
No lint errors (npm run lint)
No type errors (TypeScript compiles cleanly)
New/changed UI behavior visually verified in browser
Mock data updated if API types changed

…side rhtas-console

qodo-for-securesign · 2026-08-04T11:22:16Z

PR Summary by Qodo

Remove @tsd-ui/core; inline Theme and LoadingWrapper utilities

✨ Enhancement ⚙️ Configuration changes 🧪 Tests 🕐 40+ Minutes

AI Description

• Drop the @tsd-ui/core dependency from the client bundle.
• Re-home Theme and LoadingWrapper primitives inside rhtas-console components.
• Add an internal createComparator utility and expand unit coverage for sorting behavior.

Diagram

graph TD
  pages["App pages"] --> loadingwrapper["LoadingWrapper"] --> patternfly{{"PatternFly"}}
  pages --> theme["Theme module"] --> patternfly
  localstorage["LocalStorageThemeProvider"] --> theme --> dom["HTML + meta"]
  pages --> sorting["Table sorting"] --> comparator["createComparator"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep @tsd-ui/core and re-export only used symbols

➕ Minimal code added/maintained in this repo
➕ Upstream fixes/improvements come “for free”
➖ Does not achieve dependency removal goal
➖ Still couples release cadence and compatibility to external package

2. Create an internal shared workspace package (e.g., @app/ui-core)

➕ Clean separation from app code; reusable across other consoles
➕ Versionable API boundary for Theme/LoadingWrapper/utils
➖ Adds packaging/release overhead
➖ Likely premature unless multiple repos/apps will share these primitives soon

3. Use a general-purpose utility library for sorting (e.g., lodash orderBy)

➕ Well-tested comparator/sorting behavior out of the box
➖ Introduces (or increases) another dependency footprint
➖ Less control over null placement/locale defaults without extra wrapper code

Recommendation: Given the explicit goal to remove @tsd-ui/core and the small surface area being replaced (Theme, LoadingWrapper, comparator), the current approach—localizing the few required primitives—is the best trade-off. If these primitives start to be shared across multiple apps, consider promoting them into an internal workspace package later to keep app code slimmer and APIs more intentional.

Files changed (20) +389 / -96

Enhancement (3) +136 / -0

DefaultErrorState.tsxAdd default LoadingWrapper error UI +18/-0

Add default LoadingWrapper error UI
• Introduces a PatternFly EmptyState-based default error state used when a fetch error occurs and no custom error renderer is provided.
client/src/app/components/LoadingWrapper/DefaultErrorState.tsx

ThemeSelector.tsxAdd local ThemeSelector UI +92/-0

Add local ThemeSelector UI
• Adds a PatternFly Select-based theme mode picker that updates ThemeContext mode (light/dark/system) with icons and descriptions.
client/src/app/components/Theme/ThemeSelector.tsx

utils.tsAdd Intl.Collator-based createComparator utility +26/-0

Add Intl.Collator-based createComparator utility
• Introduces a configurable comparator factory supporting locale-aware, numeric-aware comparisons with direction and null ordering options.
client/src/app/utils/utils.ts

Refactor (13) +131 / -9

LoadingWrapper.tsxAdd generic LoadingWrapper component +27/-0

Add generic LoadingWrapper component
• Adds a reusable wrapper that renders a spinner while fetching and a default/custom error state on fetch errors, otherwise renders children.
client/src/app/components/LoadingWrapper/LoadingWrapper.tsx

index.tsExport LoadingWrapper barrel +1/-0

Export LoadingWrapper barrel
• Adds a barrel export for the new LoadingWrapper module to standardize imports.
client/src/app/components/LoadingWrapper/index.ts

LocalStorageThemeProvider.tsxSwitch LocalStorageThemeProvider to local Theme module +1/-1

Switch LocalStorageThemeProvider to local Theme module
• Replaces the @tsd-ui/core ThemeProvider/ThemeMode import with the locally-defined Theme module while keeping localStorage persistence behavior.
client/src/app/components/LocalStorageThemeProvider.tsx

ThemeContext.tsxAdd local ThemeContext and ThemeProvider +92/-0

Add local ThemeContext and ThemeProvider
• Implements theme mode handling (system/light/dark), resolves system preference changes, and updates document class + theme-color meta based on active theme.
client/src/app/components/Theme/ThemeContext.tsx

index.tsExport Theme module barrel +2/-0

Export Theme module barrel
• Exports ThemeContext/ThemeProvider and ThemeSelector for consumption from @app/components/Theme.
client/src/app/components/Theme/index.ts

ThemeAwareLogo.tsxUpdate ThemeAwareLogo to use local ThemeContext +1/-1

Update ThemeAwareLogo to use local ThemeContext
• Repoints ThemeContext usage to the local Theme module to remove @tsd-ui/core coupling.
client/src/app/components/ThemeAwareLogo.tsx

getLocalSortDerivedState.tsReplace @tsd-ui/core comparator with local utility +1/-1

Replace @tsd-ui/core comparator with local utility
• Switches sorting derived-state logic to use createComparator from @app/utils/utils.
client/src/app/hooks/TableControls/sorting/getLocalSortDerivedState.ts

about.tsxUpdate About page theme context import +1/-1

Update About page theme context import
• Migrates ThemeContext import from @tsd-ui/core to @app/components/Theme.
client/src/app/layout/about.tsx

header.tsxUpdate Header to use local ThemeSelector +1/-1

Update Header to use local ThemeSelector
• Replaces @tsd-ui/core ThemeSelector usage with the new local ThemeSelector component.
client/src/app/layout/header.tsx

Artifacts.tsxUse local LoadingWrapper in Artifacts page +1/-1

Use local LoadingWrapper in Artifacts page
• Repoints LoadingWrapper import to @app/components/LoadingWrapper to eliminate @tsd-ui/core dependency.
client/src/app/pages/Artifacts/Artifacts.tsx

TrustRoot.tsxUse local LoadingWrapper in TrustRoots page +1/-1

Use local LoadingWrapper in TrustRoots page
• Repoints LoadingWrapper import to @app/components/LoadingWrapper to eliminate @tsd-ui/core dependency.
client/src/app/pages/TrustRoot/TrustRoot.tsx

Overview.tsxUse local LoadingWrapper in TrustRoot Overview +1/-1

Use local LoadingWrapper in TrustRoot Overview
• Repoints LoadingWrapper import to @app/components/LoadingWrapper for local ownership and dependency removal.
client/src/app/pages/TrustRoot/components/Overview.tsx

RootDetails.tsxUse local createComparator in RootDetails +1/-1

Use local createComparator in RootDetails
• Replaces @tsd-ui/core createComparator usage with the locally implemented createComparator from @app/utils/utils.
client/src/app/pages/TrustRoot/components/RootDetails.tsx

Tests (2) +122 / -1

ThemeAwareLogo.test.tsxUpdate ThemeAwareLogo test imports +1/-1

Update ThemeAwareLogo test imports
• Migrates ThemeContext import from @tsd-ui/core to the new local Theme module for tests.
client/src/app/components/ThemeAwareLogo.test.tsx

utils.test.tsAdd unit tests for createComparator +121/-0

Add unit tests for createComparator
• Extends utils test coverage with comprehensive comparator tests across numeric/string comparisons, locale behavior, direction, and null placement.
client/src/app/utils/utils.test.ts

Other (2) +0 / -86

package.jsonRemove @tsd-ui/core dependency +0/-1

Remove @tsd-ui/core dependency
• Drops @tsd-ui/core from the client dependency list to decouple the UI from the external core package.
client/package.json

package-lock.jsonRemove @tsd-ui/core from lockfile +0/-85

Remove @tsd-ui/core from lockfile

• Updates the lockfile to drop @tsd-ui/core and remove its node_modules entry, reflecting the dependency removal.

package-lock.json

codecov-commenter · 2026-08-04T11:23:00Z

Codecov Report

❌ Patch coverage is 69.11765% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.87%. Comparing base (f51765d) to head (5ba510e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
client/src/app/components/Theme/ThemeContext.tsx	57.14%	10 Missing and 2 partials ⚠️
client/src/app/components/Theme/ThemeSelector.tsx	53.84%	6 Missing ⚠️
client/src/app/components/Theme/theme-utils.ts	62.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
- Coverage   76.11%   75.87%   -0.24%     
==========================================
  Files         105      109       +4     
  Lines        1553     1621      +68     
  Branches      474      496      +22     
==========================================
+ Hits         1182     1230      +48     
- Misses        327      343      +16     
- Partials       44       48       +4

Flag	Coverage Δ
e2e	`64.85% <66.66%> (+0.08%)`	⬆️
unit	`60.49% <41.17%> (-0.79%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

qodo-for-securesign · 2026-08-04T11:27:09Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 29 rules

1. Falsy error not shown 🐞 Bug ≡ Correctness

Description

LoadingWrapper only renders the error state when fetchError is truthy, so valid falsy error values
(e.g., "", 0, false) will incorrectly render children instead of an error UI. This breaks the
implied contract of fetchError?: TError | null (non-nullish should mean error).

Code

client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[R23-25]
+  if (props.fetchError) {
+    return props.fetchErrorState ? props.fetchErrorState(props.fetchError) : <DefaultErrorState />;
+  }

Relevance

●●● Strong
Truthiness check on generic error is a concrete bug; team has accepted similar defensive
nullish-guard fixes.
PR-#3

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The component declares fetchError as TError | null but uses a truthiness check, which is not
equivalent to “non-nullish” for generic error types.
client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[7-12]
client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[23-26]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LoadingWrapper` checks `if (props.fetchError)` which ignores falsy-but-present errors and may render `children` when an error should be displayed.

### Issue Context
`fetchError` is typed as `TError | null | undefined`, so the intended behavior is typically “show error UI when non-nullish”.

### Fix Focus Areas
- client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[23-25]

### Suggested fix
Change the condition to a nullish check and ensure the value passed into `fetchErrorState` is non-nullish, e.g.:
- `if (props.fetchError != null) { ... }`
- call `props.fetchErrorState(props.fetchError)` inside that branch.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. LoadingWrapper lacks partial-state 📘 Rule violation ≡ Correctness

Description

LoadingWrapper renders only a spinner whenever isFetching is true, which prevents showing
existing (stale) data during background refetches. This violates the requirement to explicitly
handle partial-data async UI states.

Code

client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[R14-21]

+  if (props.isFetching) {
+    return (
+      props.isFetchingState ?? (
+        <Bullseye>
+          <Spinner />
+        </Bullseye>
+      )
+    );

Relevance

●● Moderate
Partial/stale-data UX is semantic; no clear repo precedent that wrappers must support background
refetch display.
PR-#209

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 463 requires explicit handling of loading, error, empty, and partial-data states.
The added LoadingWrapper unconditionally returns a spinner when isFetching is true, so it cannot
represent the partial-data case where children/data exist while a background fetch is occurring.
Rule 463: Explicitly handle all async data UI states (empty, loading, error, partial)
client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[14-25]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LoadingWrapper` currently replaces its children with a spinner whenever `isFetching` is true. This makes background refetches indistinguishable from initial loading and prevents a partial-data state (show data + subtle refresh indicator).

## Issue Context
Per compliance, async-data UI must explicitly model partial-data states (data present while a refresh is in progress).

## Fix Focus Areas
- client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[14-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Duplicate error empty state~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

DefaultErrorState duplicates the existing ErrorEmptyState component with identical markup/text,
increasing the risk of future divergence and unnecessary maintenance. This duplication was
introduced with the new LoadingWrapper implementation.

Code

client/src/app/components/LoadingWrapper/DefaultErrorState.tsx[R6-9]
+export const DefaultErrorState: React.FC = () => {
+  return (
+    <EmptyState
+      status="danger"

Relevance

●● Moderate
Team often removes redundant/dead code, but refactor to reuse ErrorEmptyState may be deferred as
churn.
PR-#268
PR-#106

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Both components render the same EmptyState configuration and identical body text, so keeping both
creates redundant sources of truth.
client/src/app/components/LoadingWrapper/DefaultErrorState.tsx[6-17]
client/src/app/components/ErrorEmptyState.tsx[6-17]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DefaultErrorState` duplicates `ErrorEmptyState` (same PatternFly EmptyState props and message).

### Issue Context
The duplication can lead to inconsistent changes later (one gets updated, the other does not).

### Fix Focus Areas
- client/src/app/components/LoadingWrapper/DefaultErrorState.tsx[6-16]
- client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[5-6]

### Suggested fix
Prefer a single shared component:
- Remove `DefaultErrorState` and import/reuse `ErrorEmptyState`, OR
- Move the shared component to a common location and reference it from both places.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. Theme change listener missing 🐞 Bug ☼ Reliability

Description

ThemeProvider only subscribes to system theme changes when MediaQueryList.addEventListener exists;
in environments that only support the legacy addListener/removeListener APIs, systemTheme will
never update, leaving mode="system" stuck on the initial theme. This causes incorrect theme
behavior when OS preference changes.

Code

client/src/app/components/Theme/ThemeContext.tsx[R75-84]

+  React.useEffect(() => {
+    const mediaQuery = window.matchMedia("(prefers-color-scheme: dark)");
+    const handleChange = () => {
+      setSystemTheme(getSystemTheme());
+    };
+
+    if (mediaQuery.addEventListener) {
+      mediaQuery.addEventListener("change", handleChange);
+      return () => mediaQuery.removeEventListener("change", handleChange);
+    }

Relevance

● Weak
Very similar matchMedia listener/guard feedback (including system theme listener concerns) was
previously rejected.
PR-#176

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The effect registers a listener only inside the if (mediaQuery.addEventListener) branch and has no
alternative subscription path.
client/src/app/components/Theme/ThemeContext.tsx[75-85]
client/src/app/components/Theme/ThemeContext.tsx[58-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ThemeProvider` listens for OS theme changes only via `mediaQuery.addEventListener('change', ...)`. If that API is unavailable, there is no subscription and the system theme will not update.

### Issue Context
`isDark` depends on `systemTheme` when `mode === 'system'`, so missing subscription leads to stale UI.

### Fix Focus Areas
- client/src/app/components/Theme/ThemeContext.tsx[75-85]

### Suggested fix
Add a compatibility fallback:
- Guard `window.matchMedia` existence.
- If `addEventListener` exists, use it and remove in cleanup.
- Else, use `addListener`/`removeListener` with cleanup.
Example:
```ts
if (!window.matchMedia) return;
const mql = window.matchMedia(...);
if (mql.addEventListener) { ... } else { mql.addListener(handleChange); return () => mql.removeListener(handleChange); }
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. Theme components missing tests 📘 Rule violation ▣ Testability

Description

New theme behavior (ThemeProvider effects and ThemeSelector interactions) is introduced without
any corresponding new/updated tests that exercise it. This increases regression risk for
user-visible behavior changes (theme mode selection and dark-mode application).

Code

client/src/app/components/Theme/ThemeContext.tsx[R62-69]

+  React.useEffect(() => {
+    const htmlElement = document.documentElement;
+    const themeMeta = document.querySelector('meta[name="theme-color"]');
+
+    if (isDark) {
+      htmlElement.classList.add(DARK_MODE_KEY);
+      themeMeta?.setAttribute("content", "#000000");
+    } else {

Relevance

● Weak
Repo has recent precedent rejecting requests to add tests purely for changed/behavior code coverage
in PRs.
PR-#306
PR-#309
PR-#312

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 462 requires tests for behavior-changing code modifications. This PR adds new theme
behavior (e.g., applying/removing the dark-mode class and setting the theme-color meta tag), but
the only touched tests in the diff are unrelated utility comparator tests and an import-path
adjustment in an existing component test, leaving the new theme behaviors untested.
Rule 462: Require tests for all behavior-changing code modifications
client/src/app/components/Theme/ThemeContext.tsx[62-73]
client/src/app/utils/utils.test.ts[647-765]
client/src/app/components/ThemeAwareLogo.test.tsx[1-4]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New behavior-changing theme code was added (theme mode sanitization, system theme detection, DOM class/meta updates, and a new selector UI), but no tests were added to cover these behaviors.

## Issue Context
Compliance requires that behavior-changing modifications include corresponding automated tests that would fail if the behavior were reverted.

## Fix Focus Areas
- client/src/app/components/Theme/ThemeContext.tsx[42-85]
- client/src/app/components/Theme/ThemeSelector.tsx[44-92]
- client/src/app/components/LoadingWrapper/LoadingWrapper.tsx[14-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

chore: removed @tsd-ui/core dependency, added components and utils in…

746877e

…side rhtas-console

stanislavsemeniuk requested review from carlosthe19916 and kahboom August 4, 2026 11:20

stanislavsemeniuk added 4 commits August 4, 2026 13:29

fix: react 19 lint rules

1e6e9d1

fix: extracted vars from component export

9641e97

fix: added theme-utils.ts to Theme exports

878eb54

chore: removed duplication of error state component

5ba510e

carlosthe19916 approved these changes Aug 4, 2026

View reviewed changes

stanislavsemeniuk added this pull request to the merge queue Aug 4, 2026

Merged via the queue into securesign:main with commit 65f165c Aug 4, 2026
15 checks passed

stanislavsemeniuk deleted the chore/tsd-ui-removal branch August 4, 2026 14:17

Navigation Menu

Uh oh!

chore: Removal of @tsd-ui/core dependency - #354

chore: Removal of @tsd-ui/core dependency#354
stanislavsemeniuk merged 5 commits into
securesign:mainfrom
stanislavsemeniuk:chore/tsd-ui-removal

stanislavsemeniuk commented Aug 4, 2026

Uh oh!

qodo-for-securesign Bot commented Aug 4, 2026

Uh oh!

codecov-commenter commented Aug 4, 2026 •

edited

Loading

Uh oh!

qodo-for-securesign Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

stanislavsemeniuk commented Aug 4, 2026

Type of Change

Testing

Uh oh!

qodo-for-securesign Bot commented Aug 4, 2026

PR Summary by Qodo

Uh oh!

codecov-commenter commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qodo-for-securesign Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Aug 4, 2026 •

edited

Loading

qodo-for-securesign Bot commented Aug 4, 2026 •

edited

Loading