Skip to content

feat: enhance error handling in git-push and repo services#1406

Merged
jescalada merged 13 commits intofinos:mainfrom
dcoric:denis-coric/fix-1392-linked
Feb 25, 2026
Merged

feat: enhance error handling in git-push and repo services#1406
jescalada merged 13 commits intofinos:mainfrom
dcoric:denis-coric/fix-1392-linked

Conversation

@dcoric
Copy link
Contributor

@dcoric dcoric commented Feb 13, 2026

Summary

This PR fixes both:

Improve error handling and test coverage for UI services

This PR refactors error handling across the UI service layer and adds comprehensive test coverage. Error handling is now consistent, type-safe, and properly tested.

What's changed

Error handling improvements:

  • Added src/ui/services/errors.ts with reusable error utilities (getServiceError, errorResult, successResult)
  • Refactored git-push.ts and repo.ts services to use the new error handling utilities
  • Updated UI views (PushDetails.tsx, RepoDetails.tsx, etc.) to properly handle and display error states
  • All service functions now return consistent ServiceResult<T> objects with success/failure states

Test coverage (14 → 79 tests):

  • test/ui/errors.test.ts - comprehensive tests for error utility functions (18 tests)
  • test/ui/user.test.ts - tests for user service functions (13 tests)
  • test/ui/git-push.test.ts - expanded coverage for all git-push functions (13 tests)
  • test/ui/repo.test.ts - expanded coverage for all repo functions (21 tests)

Tests cover success paths, HTTP errors (401/403/404/409/500), network failures, and edge cases.

Benefits

  • Consistent error handling - all services use the same pattern
  • Better UX - proper error messages shown to users
  • Type safety - TypeScript ensures correct error handling
  • Test coverage - prevents regressions and documents expected behavior
Screenshot 2026-02-13 at 18 20 59

Addition to this PR - Error Boundary

New file src/ui/components/ErrorBoundary/ErrorBoundary.tsx

ErrorBoundary class component with getDerivedStateFromError (render capture) + componentDidCatch (logging)

Wraps all route content in and catches any render error from any view without taking down the sidebar/navbar

It has 2 modes - dev and production. In dev mode it has option to display stacktrace of the error, in production mode only offers option to reload:

DEV:
Screenshot 2026-02-20 at 17 10 46

PROD:
Screenshot 2026-02-20 at 17 10 58

- Add new test file for errors.ts utility functions (18 tests)
- Expand git-push.test.ts to cover getPush and getPushes functions (13 tests total)
- Expand repo.test.ts to cover getRepos, addRepo, deleteUser, and deleteRepo (21 tests total)
- Add new test file for user.ts service functions (13 tests)
- All tests verify both success and error handling scenarios
- Total test count increased from 14 to 79 tests for UI services
@netlify
Copy link

netlify bot commented Feb 13, 2026

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 2c2b093
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/699e99abbfbf6200081625cb
😎 Deploy Preview https://deploy-preview-1406.git-proxy.preview.finos.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@dcoric dcoric requested a review from fabiovincenzi February 13, 2026 17:40
@dcoric dcoric requested a review from jescalada February 13, 2026 17:40
@dcoric dcoric self-assigned this Feb 13, 2026
@dcoric dcoric requested a review from kriswest February 13, 2026 17:41
@codecov
Copy link

codecov bot commented Feb 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.78%. Comparing base (6713757) to head (2c2b093).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1406   +/-   ##
=======================================
  Coverage   81.78%   81.78%           
=======================================
  Files          67       67           
  Lines        4766     4766           
  Branches      827      827           
=======================================
  Hits         3898     3898           
  Misses        852      852           
  Partials       16       16           

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

Looks good so far, thanks for all the extra UI tests!

What I had in mind when I first made the error handling refactor ticket was some sort of wrapper component (context?) that would catch any errors happening in API calls and forward the error to a toast. I'm aware this would require a lot of extra refactoring, so I might have to clarify the details and we can deal with it in a separate PR.

Note that I haven't tested behavior yet - will complete the review later this week 👍🏼

@kriswest kriswest requested a review from a team as a code owner February 17, 2026 09:12
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Very happy to see this being done (particularly redirects to login on 401s and use of the API's error messages) and would love it to go into 2.0 (as the current situation is awful). I only have a couple of minor comments to add to Juan's.

Copy link
Contributor

@fabiovincenzi fabiovincenzi left a comment

Choose a reason for hiding this comment

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

Looks good to me, and it covers #1402 , thanks @dcoric!

@jescalada jescalada linked an issue Feb 19, 2026 that may be closed by this pull request
@dcoric
Copy link
Contributor Author

dcoric commented Feb 20, 2026

Looks good so far, thanks for all the extra UI tests!

What I had in mind when I first made the error handling refactor ticket was some sort of wrapper component (context?) that would catch any errors happening in API calls and forward the error to a toast. I'm aware this would require a lot of extra refactoring, so I might have to clarify the details and we can deal with it in a separate PR.

Note that I haven't tested behavior yet - will complete the review later this week 👍🏼

I have explored universal API error messaging. It is doable with shared axios instance with a response interceptor + react-toastify (or similar) that would auto-surface API errors as toasts without per-service error handling. I would tackle this in a separate PR (and do a cleanup as well)

Addition to this PR:
I have added an ErrorBoundary component to catch and display UI render crashes gracefully. In development it shows the error message and stack trace; in production it shows a user-friendly message with a retry option.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

I noticed a few errors that are either caught elsewhere or don't make it into the ErrorBoundary:

Non-existent user

image

Non-existent push

This might happen if accessing a link to a push that later got removed from the database somehow:

image

For /dashboard/repo/<repo-id> this is working correctly:

image

Another thing I noticed is that we still get the errors in the console. Not sure if we should keep these for debugging (as they sometimes contain additional context vs. the stack trace) or not.

image

@jescalada
Copy link
Contributor

LGTM after fixing those! 👍🏼

@dcoric
Copy link
Contributor Author

dcoric commented Feb 23, 2026

@jescalada Addressed both issues:

  • Non-existent user/push routes: UserProfile and PushDetails now throw on error instead of rendering <Danger> inline, so the ErrorBoundary catches and displays them properly.
  • Console errors: console.error in ErrorBoundary.componentDidCatch and auth.ts is now gated behind a dev-mode check, no noise in production.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

LGTM!

@kriswest kriswest mentioned this pull request Feb 23, 2026
21 tasks
…inked

# Conflicts:
#	src/ui/services/git-push.ts
#	src/ui/views/PushDetails/PushDetails.tsx
@jescalada jescalada merged commit b937878 into finos:main Feb 25, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

4 participants