Skip to content

First Implementation of Error Boundary and Toast Notification System#32

Open
jayvionthaing wants to merge 1 commit into
mainfrom
feature/error-boundary-and-toasts
Open

First Implementation of Error Boundary and Toast Notification System#32
jayvionthaing wants to merge 1 commit into
mainfrom
feature/error-boundary-and-toasts

Conversation

@jayvionthaing

Copy link
Copy Markdown

Includes a test panel featuring buttons that will display the desired notification (will be removed after initial feedback). I'm not sure if the CI checks are supposed to appear here in the pull request, but I don't see them here. If needed to be done manually, I can do them along with removing the test panel during the feedback and revision process!

…on System. Includes a test panel featuring buttons that will display the desired notification (will be removed after initial feedback).
@Youdahe123

Copy link
Copy Markdown
Contributor

Code Review

Cannot merge — all 4 CI checks are failing, and there are a couple of things that need to be addressed in the PR itself before it's ready.

What's good

  • Error boundaries + toast notifications are the right pattern for surfacing runtime errors without crashing the whole app
  • Including a test panel to demo the toasts during review is a reasonable dev approach

Issues to fix before merge

1. Remove the test panel

You already flagged this — the test panel with demo buttons must be removed before this merges to main. It adds dead UI that real users would see.

2. Lint error in ErrorBoundary.tsx line 10:42

The CI is reporting:

ErrorBoundary.tsx:10:42  error  '_' is defined but never used  @typescript-eslint/no-unused-vars

If the parameter is intentionally unused (e.g. an error handler that ignores the first argument), prefix it with an underscore to signal that:

// Instead of:
(_, errorInfo) => { ... }
// Use:
(_error, errorInfo) => { ... }

3. Pre-existing CI failures (not this PR's fault, but blocking)

There are many pre-existing lint failures across the codebase (no-explicit-any, no-unused-vars, hook violations in ConceptGraph3D.tsx, FlashcardReview.tsx, etc.). These aren't caused by your PR but branch protection requires all checks to pass. Someone needs to clean those up before any PR can merge.

4. Regarding CI not showing up

You mentioned not seeing CI checks on the PR. They are running — you can see them at the bottom of the PR page under "Checks". They show up a minute or two after pushing. All 4 are currently failing (Backend Lint, Backend Tests, Frontend Lint, Frontend Build).

Action items

  1. Remove the test panel
  2. Fix the _ unused variable in ErrorBoundary.tsx
  3. Re-push — the CI will re-run automatically

Once those two things are fixed and CI passes, this is a good addition.

@Youdahe123

Copy link
Copy Markdown
Contributor

Status

CI: All 4 checks failing. No reviewer assigned. Branch is behind main.

This PR needs:

  1. Update branch / rebase on main (it's currently behind)
  2. A human review
  3. Once rebased on main after fix: backend-lint and backend-test CI (#35) #36 and fix: frontend lint tests ci #38 land, CI failures should clear up

Not mergeable until reviewed and CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants