Skip to content

fix(desktop): log the real error in the app error boundary#1427

Merged
jaylfc merged 1 commit into
devfrom
fix/error-boundary-log-error
Jun 24, 2026
Merged

fix(desktop): log the real error in the app error boundary#1427
jaylfc merged 1 commit into
devfrom
fix/error-boundary-log-error

Conversation

@jaylfc

@jaylfc jaylfc commented Jun 24, 2026

Copy link
Copy Markdown
Owner

Problem

The top-level AppErrorBoundary's componentDidCatch() took no arguments and logged nothing. When an unexpected error hit the generic "Something went wrong" fallback, the actual error and component stack were discarded, so the crash left no console trace and nothing for the observability stack to pick up. (Surfaced while debugging a live 'Something went wrong' crash on the Pi that was undiagnosable for exactly this reason.)

Fix

Log error + info.componentStack in componentDidCatch for the generic mode only. The expected, handled states (waiting for backend reconnect, chunk for stale-deploy reload) stay quiet to avoid noise. No UI/behavior change.

Tests

Added two cases: a generic crash logs the real error + stack; the waiting state does not log. Both AppErrorBoundary suites green (13 passed).

Summary by CodeRabbit

  • Bug Fixes
    • Improved error boundary behavior so real errors are logged with more useful diagnostic details.
    • Prevented unnecessary logging when the app is in an expected waiting state.

componentDidCatch swallowed the error entirely, so a generic 'Something went
wrong' crash left no trace in the console or for the observability stack,
making it undiagnosable. Log the error and component stack for the generic
mode (the expected waiting/chunk states stay quiet). No UI change.
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c568f8c-b2bb-4c4e-bd8b-32fcb6f15a38

📥 Commits

Reviewing files that changed from the base of the PR and between a27ca5b and 353b2f9.

📒 Files selected for processing (2)
  • desktop/src/components/AppErrorBoundary.tsx
  • desktop/src/components/__tests__/AppErrorBoundary.test.tsx

📝 Walkthrough

Walkthrough

componentDidCatch in AppErrorBoundary gains typed (error: Error, info: ErrorInfo) parameters and now calls console.error only when the boundary is in "generic" mode. Two new tests verify that generic errors are logged and that BackendUnavailableError produces no boundary log output.

Changes

AppErrorBoundary conditional error logging

Layer / File(s) Summary
componentDidCatch rewrite and logging tests
desktop/src/components/AppErrorBoundary.tsx, desktop/src/components/__tests__/AppErrorBoundary.test.tsx
ErrorInfo is added to the React import; componentDidCatch now accepts typed (error, info) parameters and calls console.error only when the error mode is "generic". Two new tests assert that generic errors produce a log with the boundary's prefix and that BackendUnavailableError produces no boundary log output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐇 A boundary guards the app with care,
When generic errors fill the air,
It logs a clue for bugs to trace,
But stays quiet in the waiting place.
No noise for errors we expect—
Just tidy logs, with full respect! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: logging the real error from the desktop app error boundary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/error-boundary-log-error

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

Note

Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually.
Learn more about usage limits

Code Review ✅ Approved

Updates AppErrorBoundary to log the original error and component stack during generic failures, ensuring crash traceability. No issues were found.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Important

Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

// is diagnosable from the console and reachable by the observability stack.
// Skip the "waiting"/"chunk" modes -- those are expected, handled states.
if (this.state.mode === "generic") {
console.error("[AppErrorBoundary]", error, info?.componentStack ?? "");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: Defensive info?.componentStack ?? "" is misleading

ErrorInfo from react types componentStack as a required non-optional string (see @types/react ErrorInfo). React never invokes componentDidCatch with a nullish info or with componentStack missing. The optional-chaining + nullish coalesce suggest these are realistic failure modes when they aren't, and they obscure the real contract from future readers. Drop the noise:

Suggested change
console.error("[AppErrorBoundary]", error, info?.componentStack ?? "");
console.error("[AppErrorBoundary]", error, info.componentStack);

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@kilo-code-bot

kilo-code-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0

The change correctly fixes the root cause: a componentDidCatch that dropped both the error and the component stack. Logging only on the generic mode is the right call -- the waiting and chunk paths are deliberate, expected states, and noise from them would dilute the signal on the one path that's actually a crash.

The test additions are well-scoped: one positive case proves the real error reaches the console with the right prefix, and one negative case proves the handled waiting state stays quiet. Using args[0] === "[AppErrorBoundary]" as the filter is robust against React's own error logging, which the existing consoleErr spy silences.

One small wart on the new logging line, captured inline.

Issue Details (click to expand)

WARNING

File Line Issue
desktop/src/components/AppErrorBoundary.tsx 53 info?.componentStack ?? "" is defensive against a state that ErrorInfo's types and React's lifecycle contract both forbid. Misleading to future readers; drop the ?. and ?? "".
Files Reviewed (2 files)
  • desktop/src/components/AppErrorBoundary.tsx - 1 issue
  • desktop/src/components/__tests__/AppErrorBoundary.test.tsx - 0 issues

Fix these issues in Kilo Cloud


Reviewed by minimax-m3 · Input: 33.4K · Output: 2.8K · Cached: 136.1K

@jaylfc jaylfc merged commit e4617ba into dev Jun 24, 2026
10 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in TinyAgentOS Roadmap Jun 24, 2026
jaylfc added a commit that referenced this pull request Jun 24, 2026
jaylfc added a commit that referenced this pull request Jun 24, 2026
…dger failures (#1431)

Folds two gitar findings on the consent-flow backend:
- #1430 Edge-Case (must-fix): request_app_consent did not de-duplicate input
  capabilities, so a repeated cap became a colliding consent option whose value
  the #1428 dedup suffixer rewrote to a non-capability (e.g. 'app.net (2)').
  Now de-duped while preserving order.
- #1429 Quality: _apply_app_grant swallowed all app_grants ledger errors
  silently; now logs a warning with the app + decision id so a broken grant
  write is diagnosable (same lesson as #1427).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant