Log JS errors to dotnet #2003
Log JS errors to dotnet #2003
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces a JS-invokable .NET logger service (JsInvokableLogger) across backend and frontend: DI registration, provider wiring, TypeGen updates, generated TS enums/interfaces, service registry mapping and accessor, global error handling refactor to use the logger, and a minor proxy guard change in the DotNet service provider. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
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. Comment |
github-actions
bot
added
the
💻 FW Lite
C# Unit Tests130 tests 130 ✅ 19s ⏱️ Results for commit 1e0122e. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
frontend/viewer/src/lib/services/service-provider-dotnet.ts (2)
66-70: Reduce noisy debug logs in production.Consider gating console.debug behind a dev guard to avoid log noise in production.
- console.debug(`[Dotnet Proxy] Calling ${serviceName} method ${dotnetMethodName}`, args); + if (import.meta?.env?.DEV) console.debug(`[Dotnet Proxy] Calling ${serviceName} method ${dotnetMethodName}`, args); ... - console.debug(`[Dotnet Proxy] ${serviceName} method ${dotnetMethodName} returned`, result); + if (import.meta?.env?.DEV) console.debug(`[Dotnet Proxy] ${serviceName} method ${dotnetMethodName} returned`, result);
60-63: Nit: comment wording.Minor wording tweak for clarity: “RuneD resource” → “Runed resource” or name the lib explicitly if that’s intended.
backend/FwLite/FwLiteShared/Services/JsInvokableLogger.cs (1)
8-13: Consider structured logging and basic scrubbing.Current API accepts a single message string. If practical later, pass structured fields (message, stack, url, userAgent) and log with named properties; also scrub sensitive query params before logging.
Would you like me to sketch a backward-compatible DTO and JS usage that preserves this method but adds an optional LogContext payload?
frontend/viewer/src/lib/services/service-provider.ts (1)
43-54: Provide a safe no-op default for the logger (optional).Like JsEventListener, consider a default to avoid throw when backend isn’t connected (storybook/SSR).
private services: Partial<LexboxServiceRegistry> = { [DotnetService.JsEventListener]: { nextEventAsync(): Promise<IFwEvent> { console.warn('using default JsEvent listener which does nothing'); return new Promise<IFwEvent>(() => {}); }, lastEvent(): Promise<IFwEvent | null> { console.warn('using default JsEvent listener which does nothing'); return Promise.resolve(null); } } + , + [DotnetService.JsInvokableLogger]: { + async log() { + // default no-op logger for environments without a backend + } + } as IJsInvokableLogger };frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts (1)
19-21: Small consistency nit: trailing comma.Consider keeping a trailing comma after the last enum member to reduce diff noise on future insertions (if your TypeGen allows it).
frontend/viewer/src/lib/errors/global-errors.ts (2)
92-104: Enrich log payload with minimal context (URL, UA, timestamp) and better cause handlingImproves diagnostics without affecting UX.
Apply this diff:
function getErrorString(event: UnifiedErrorEvent) { const details = [`Message: ${event.message}`]; if (event.at) details.push(`at ${event.at}`); if (event.error instanceof Error) { const error: Error = event.error; if (error.stack) details.push(`Stack: ${error.stack}`); - if (error.cause) details.push(`Cause: ${tryStringify(error.cause)}`); + if (error.cause) { + if (error.cause instanceof Error) { + details.push(`Cause: ${error.cause.stack ?? error.cause.message}`); + } else { + details.push(`Cause: ${tryStringify(error.cause)}`); + } + } } else if (event.error) { details.push(`Error: ${tryStringify(event.error)}`); } - return details.join('\n'); + // minimal environment context + try { + details.push(`URL: ${location.href}`); + details.push(`UserAgent: ${navigator.userAgent}`); + details.push(`Time: ${new Date().toISOString()}`); + } catch { /* ignore */ } + return details.join('\n'); }
106-112: MaketryStringifyalways return a stringPrevents accidental “undefined” literals and improves readability of logs.
Apply this diff:
-function tryStringify(value: unknown): string | undefined { +function tryStringify(value: unknown): string { try { - return JSON.stringify(value); + const s = JSON.stringify(value); + return s ?? String(value); } catch { return '(failed-to-stringify)'; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs(1 hunks)backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs(3 hunks)backend/FwLite/FwLiteShared/Services/JsInvokableLogger.cs(1 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IJsInvokableLogger.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/Microsoft/Extensions/Logging/LogLevel.ts(1 hunks)frontend/viewer/src/lib/errors/global-errors.ts(2 hunks)frontend/viewer/src/lib/services/js-invokable-logger.ts(1 hunks)frontend/viewer/src/lib/services/service-provider-dotnet.ts(1 hunks)frontend/viewer/src/lib/services/service-provider.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-22T09:19:37.386Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Applied to files:
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs
🧬 Code graph analysis (9)
backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs (1)
backend/FwLite/FwLiteShared/Services/JsInvokableLogger.cs (1)
JsInvokableLogger(6-14)
backend/FwLite/FwLiteShared/Services/JsInvokableLogger.cs (2)
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (1)
Task(68-83)backend/FwLite/FwLiteShared/Auth/LoggerAdapter.cs (2)
Log(26-29)LogLevel(8-20)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
backend/FwLite/FwLiteShared/Auth/LoggerAdapter.cs (1)
LogLevel(8-20)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IJsInvokableLogger.ts (1)
backend/FwLite/FwLiteShared/Auth/LoggerAdapter.cs (1)
LogLevel(8-20)
frontend/viewer/src/lib/services/js-invokable-logger.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IJsInvokableLogger.ts (1)
IJsInvokableLogger(8-11)
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (1)
backend/FwLite/FwLiteShared/Services/JsInvokableLogger.cs (1)
JsInvokableLogger(6-14)
frontend/viewer/src/lib/services/service-provider.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IJsInvokableLogger.ts (1)
IJsInvokableLogger(8-11)
frontend/viewer/src/lib/errors/global-errors.ts (3)
frontend/viewer/src/lib/notifications/notifications.ts (2)
AppNotification(39-97)error(52-67)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IJsInvokableLogger.ts (1)
IJsInvokableLogger(8-11)frontend/viewer/src/lib/services/js-invokable-logger.ts (1)
useJsInvokableLogger(4-6)
frontend/viewer/src/lib/dotnet-types/generated-types/Microsoft/Extensions/Logging/LogLevel.ts (1)
backend/FwLite/FwLiteShared/Auth/LoggerAdapter.cs (1)
LogLevel(8-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build API / publish-api
- GitHub Check: Build UI / publish-ui
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: frontend
- GitHub Check: Analyze (csharp)
🔇 Additional comments (14)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts (1)
19-21: Enum member addition aligns with backend service exposure.JsInvokableLogger is correctly added after JsEventListener; generated string values match backend enum names.
frontend/viewer/src/lib/services/service-provider-dotnet.ts (1)
62-64: Good guard to prevent the proxy from being treated as a Promise.Returning undefined for "then" avoids accidental thenable detection on proxied .NET objects.
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (2)
29-29: Required import for LogLevel export.Adding Microsoft.Extensions.Logging is necessary for the new enum export.
175-177: Exporting LogLevel as numeric enum is correct.UseString(false) matches .NET numeric values (Trace=0 … None=6) and aligns with generated TS.
backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs (1)
39-41: Scoped lifetime is appropriate for JS-invokable services.Per-circuit scope prevents cross-circuit leakage and matches JsEventListener.
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (4)
37-38: Expose JsInvokableLogger to the front end.Included in ExportedServices so it’s delivered to JS—good.
55-56: Service type mapping added.Mapping DotnetService.JsInvokableLogger → JsInvokableLogger is correct.
112-114: Backend enum stays in sync with generated TS.New members appended; JSON string enum converter keeps names stable for interop.
68-83: Ensure DotNetObjectReference returned by SetService is disposedRepo search found only the method definition (backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs:68–83) and no callers; verify every caller disposes the returned IDisposable, or make SetService/Provider own/track/dispose the DotNetObjectReference to avoid leaks across page reloads.
frontend/viewer/src/lib/services/service-provider.ts (3)
20-20: Type import added for logger interface.Keeps registry strongly typed.
37-38: Registry mapping for JsInvokableLogger added.Front-end can now resolve the logger via DotnetService key.
66-75: Prefer tryGet when accessing optional services.useJsInvokableLogger already returns ServiceProvider.tryGetService(DotnetService.JsInvokableLogger). The global error handler defines tryGetLogger (with retries) but there is an unguarded call to await logger.log(...); confirm frontend/viewer/src/lib/errors/global-errors.ts guards that call (check for undefined / return early) before invoking logger.log.
frontend/viewer/src/lib/dotnet-types/generated-types/Microsoft/Extensions/Logging/LogLevel.ts (1)
6-14: Numeric enum values match .NET’s LogLevel.Trace=0 … None=6 matches backend; generation looks correct.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IJsInvokableLogger.ts (1)
6-11: Generated interface looks correct and matches intended surfaceType-only import, enum usage, and Promise-returning
logare consistent with the rest of the PR.
myieye
force-pushed
the
log-js-error-to-dotnet
branch
from
d0d9a24 to
1e0122e
Compare
The big win here is in the PR title: currently JS errors don't get further than the browser console.
This PR changes that so that our error handler also forwards global uncaught errors to our dotnet logging.
App errors and the log file:
I also made our notifications and copy-to-clipboard more robust.
Currently we only show (and copy-to-clipboard) a stack trace for dotnet exceptions.
Now we also do that for js errors as well.
Storybook notifications and the copied-to-clipboard text:
One problem is that dotnet errors turn into JS errors and so now we're logging them as JS error to dotnet a second time.
I'm not sure how to confidently avoid that. I'd rather have duplicate dotnet error logs than accidentally lose a JS error log.
UPDATE: Actually logging them again from JS actually has some subtle advantages: