Add Sentry error tracking for environment load/transform #258
Conversation
|
|
WalkthroughAdded Sentry integration and tightened error handling in local-sync FS utilities. writeContent now emits Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/renderer/actions/local-sync/fs-utils.ts:
- Around line 324-337: The Sentry error message "Environment write error" in the
Sentry.captureEvent call inside writeContent is misleading because writeContent
handles multiple file types; update the message to a generic, contextual string
(e.g., "File write error" or include fileType.type) and ensure the event still
includes fileType.type and resource.path in tags/extra for context; locate the
Sentry.captureEvent invocation in writeContent and replace only the hardcoded
"Environment write error" text with a generic descriptive message that may
incorporate fileType.type.
- Around line 1171-1173: The falsy check using "!variable.id" incorrectly treats
0 as missing; update the check in the validation logic to only treat undefined
or null as missing (e.g., use "variable.id == null" or "variable.id ===
undefined") before pushing "id" into missingFields so that id: 0 is considered
valid; modify the block that currently reads "if (!variable.id) {
missingFields.push('id'); }" accordingly.
- Around line 1097-1112: The Sentry event message "Environment not found" is
misleading; update the Sentry.captureEvent call (around parsedFileResult
handling) to use a neutral, accurate message that includes the actual error
message and code from parsedFileResult.error (e.g., "Environment load failed")
and add the error code into the top-level message or a dedicated extra field so
the event reflects the real failure type rather than assuming "not found".
- Line 1116: The check that sets isGlobal uses a hardcoded '/' separator: const
isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); — replace this with a
cross-platform check by using Node's path utilities (e.g.,
path.basename(file.path) === GLOBAL_ENV_FILE or path.sep-aware comparison) so
that GLOBAL_ENV_FILE is detected correctly on Windows and POSIX; update the code
referencing file.path and GLOBAL_ENV_FILE (and import path if needed)
accordingly.
🧹 Nitpick comments (2)
src/renderer/actions/local-sync/fs-utils.ts (2)
1196-1211: Consider privacy implications of logging variable names.Variable names (keys) logged in
missingFieldsPerVariablemay contain sensitive identifiers (e.g.,USER_EMAIL,API_SECRET,DB_PASSWORD). While not as severe as logging values, this could still reveal information about the application's secrets structure.Consider logging only the count of affected variables rather than their names, or sanitizing the keys before sending to Sentry.
♻️ Privacy-conscious alternative
Sentry.captureEvent({ message: `Environment variables with missing fields: ${Object.keys(missingFieldsPerVariable).length}`, level: "warning", tags: { fileType: "environment", errorType: "invalid_variable_structure", }, extra: { totalVariables: Object.keys(variables).length, variablesWithIssues: Object.keys(missingFieldsPerVariable).length, - missingFields: missingFieldsPerVariable, + // Aggregate missing field types without exposing variable names + missingFieldTypes: [...new Set(Object.values(missingFieldsPerVariable).flat())], }, });
1162-1169: Document the fallback to(variable as any).value.The type cast
(variable as any).valuesuggests handling a legacy or alternative data structure. Consider adding a brief comment explaining why this fallback exists for future maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/actions/local-sync/fs-utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/actions/local-sync/fs-utils.ts (5)
src/renderer/actions/local-sync/common-utils.ts (3)
parseRaw(85-108)getIdFromPath(127-130)createFileSystemError(212-233)src/renderer/actions/local-sync/file-types/file-types.ts (1)
EnvironmentRecordFileType(25-35)src/renderer/actions/local-sync/constants.ts (1)
GLOBAL_ENV_FILE(12-12)src/renderer/actions/local-sync/types.ts (2)
Environment(108-114)EnvironmentVariableValue(91-96)src/renderer/actions/local-sync/schemas.ts (1)
EnvironmentRecord(190-193)
🔇 Additional comments (1)
src/renderer/actions/local-sync/fs-utils.ts (1)
2-2: LGTM!Standard Sentry import for browser-based error tracking.
| if (parsedFileResult.type === "error") { | ||
| Sentry.captureEvent({ | ||
| message: `Environment not found: ${parsedFileResult.error.message}`, | ||
| level: "error", | ||
| tags: { | ||
| fileType: "environment", | ||
| errorType: "environment_load_failed", | ||
| }, | ||
| extra: { | ||
| path: file.path, | ||
| error: parsedFileResult.error.message, | ||
| errorCode: parsedFileResult.error.code, | ||
| }, | ||
| }); | ||
|
|
||
| const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); | ||
| const environment: Environment = { | ||
| type: "environment", | ||
| id: getIdFromPath(file.path), | ||
| name: content.name, | ||
| variables: content.variables, | ||
| isGlobal, | ||
| }; | ||
| return parsedFileResult; |
There was a problem hiding this comment.
Misleading Sentry message - "not found" vs actual error type.
The message "Environment not found" is inaccurate. The error could be a validation failure, parse error, or permission issue - not necessarily a missing file. The actual error code is available via parsedContentResult.error.code but the message doesn't reflect it.
✏️ Suggested fix
Sentry.captureEvent({
- message: `Environment not found: ${parsedFileResult.error.message}`,
+ message: `Environment load failed: ${parsedFileResult.error.message}`,
level: "error",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (parsedFileResult.type === "error") { | |
| Sentry.captureEvent({ | |
| message: `Environment not found: ${parsedFileResult.error.message}`, | |
| level: "error", | |
| tags: { | |
| fileType: "environment", | |
| errorType: "environment_load_failed", | |
| }, | |
| extra: { | |
| path: file.path, | |
| error: parsedFileResult.error.message, | |
| errorCode: parsedFileResult.error.code, | |
| }, | |
| }); | |
| const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); | |
| const environment: Environment = { | |
| type: "environment", | |
| id: getIdFromPath(file.path), | |
| name: content.name, | |
| variables: content.variables, | |
| isGlobal, | |
| }; | |
| return parsedFileResult; | |
| if (parsedFileResult.type === "error") { | |
| Sentry.captureEvent({ | |
| message: `Environment load failed: ${parsedFileResult.error.message}`, | |
| level: "error", | |
| tags: { | |
| fileType: "environment", | |
| errorType: "environment_load_failed", | |
| }, | |
| extra: { | |
| path: file.path, | |
| error: parsedFileResult.error.message, | |
| errorCode: parsedFileResult.error.code, | |
| }, | |
| }); | |
| return parsedFileResult; |
🤖 Prompt for AI Agents
In @src/renderer/actions/local-sync/fs-utils.ts around lines 1097 - 1112, The
Sentry event message "Environment not found" is misleading; update the
Sentry.captureEvent call (around parsedFileResult handling) to use a neutral,
accurate message that includes the actual error message and code from
parsedFileResult.error (e.g., "Environment load failed") and add the error code
into the top-level message or a dedicated extra field so the event reflects the
real failure type rather than assuming "not found".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/renderer/actions/local-sync/fs-utils.ts:
- Around line 1203-1205: The Sentry message uses the wrong function name; update
the message string passed to Sentry.captureEvent so it references
parseToEnvironmentEntity() instead of parseToEnvironmentEntityEnvironment();
locate the Sentry.captureEvent call in fs-utils.ts (the block that logs
variables with missing fields) and change the template literal to use the
correct function name while preserving the rest of the message and the level
"warning".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/actions/local-sync/fs-utils.ts
🔇 Additional comments (8)
src/renderer/actions/local-sync/fs-utils.ts (8)
2-2: LGTM!The Sentry import is correctly placed and follows standard module import patterns.
279-300: LGTM!The validation failure telemetry properly avoids logging sensitive content by using metadata (
contentMeta) instead. The generic message is appropriate for the multi-purposewriteContentfunction.
328-343: LGTM!The write exception telemetry uses a generic message appropriate for all file types and includes useful debugging context (path, error, stack) without leaking sensitive content.
1134-1149: LGTM!The exception handler provides comprehensive telemetry with path, error message, and stack trace, enabling effective debugging of unexpected failures during environment loading.
1167-1174: LGTM!The value resolution chain (
localValue → syncValue → legacy value) correctly preserves user-entered values as described in the PR objectives.
1190-1198: LGTM!The secret type coercion to string storage and
isSecretflag preservation aligns with the PR objective of maintaining secrets while ensuring proper type storage.
1201-1216: LGTM!Aggregating missing-field diagnostics into a single Sentry warning event per transformation reduces telemetry noise while retaining sufficient context for debugging. Good pattern for batch diagnostics.
1219-1233: LGTM!The exception handler correctly captures transform failures with error context and re-throws to preserve the error propagation contract. This ensures callers are notified while telemetry is captured.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/renderer/actions/local-sync/fs-utils.ts:
- Line 1104: The error text in parseFileToEnv() is misleading ("Environment not
found"); update the error log/throw to reflect the actual failure by using
parsedFileResult.error.message and parsedFileResult.error.code and avoid
asserting "not found" — e.g., change the message produced where parsedFileResult
is handled to something like "parseFileToEnv() - Error parsing environment:
[code] message" (use parsedFileResult.error.code and
parsedFileResult.error.message) so the log/exception reports the real error type
and details.
- Line 1204: The log message uses the incorrect function name
"parseToEnvironmentEntityEnvironment()"—update the string to
"parseToEnvironmentEntity()" in the message template where it is constructed
(the line that builds message: `... variables with missing fields:
${Object.keys(missingFieldsPerVariable).length}`), ensuring the descriptive text
matches the actual function name parseToEnvironmentEntity.
- Line 1121: The check for GLOBAL_ENV_FILE uses a hardcoded forward slash and
fails on Windows; replace the endsWith check with a cross-platform basename
comparison: use path.basename(file.path) === GLOBAL_ENV_FILE (ensure the Node
'path' module is imported where fs-utils.ts defines isGlobal and file.path) so
the filename is compared reliably across platforms.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/actions/local-sync/fs-utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/actions/local-sync/fs-utils.ts (5)
src/renderer/actions/local-sync/common-utils.ts (3)
parseRaw(85-108)getIdFromPath(127-130)createFileSystemError(212-233)src/renderer/actions/local-sync/file-types/file-types.ts (1)
EnvironmentRecordFileType(25-35)src/renderer/actions/local-sync/constants.ts (1)
GLOBAL_ENV_FILE(12-12)src/renderer/actions/local-sync/types.ts (2)
Environment(108-114)EnvironmentVariableValue(91-96)src/renderer/actions/local-sync/schemas.ts (1)
EnvironmentRecord(190-193)
🔇 Additional comments (4)
src/renderer/actions/local-sync/fs-utils.ts (4)
2-2: LGTM - Sentry integration for error tracking.The Sentry import is appropriate for adding observability to environment load and transform operations.
279-300: LGTM - Validation failure tracking without sensitive data.Good work avoiding sensitive data leakage by using
contentMeta(contentLength or key count) instead of raw content. The generic error message is appropriate for this multi-purpose function.
329-342: LGTM - Generic error message for write exceptions.The error tracking appropriately captures write failures with context while using a generic message suitable for all file types handled by
writeContent.
1167-1198: LGTM - Well-designed value resolution and preservation logic.The value resolution cascade (localValue → syncValue → legacy value) properly preserves user-entered environment values. The Secret type mapping to String with an
isSecretflag is a clean approach for storage. The fix at line 1176 correctly handlesid: 0by checking forundefinedornullinstead of using a falsy check.
| const { content } = parsedFileResult; | ||
| if (parsedFileResult.type === "error") { | ||
| Sentry.captureEvent({ | ||
| message: `parseFileToEnv() - Environment not found: ${parsedFileResult.error.message}`, |
There was a problem hiding this comment.
Misleading error message - "not found" vs actual error type.
The message "Environment not found" is inaccurate. The error could be a validation failure, parse error, or permission issue - not necessarily a missing file. The actual error is available in parsedFileResult.error.message and parsedFileResult.error.code.
✏️ Suggested fix
Sentry.captureEvent({
- message: `parseFileToEnv() - Environment not found: ${parsedFileResult.error.message}`,
+ message: `parseFileToEnv() - Environment load failed: ${parsedFileResult.error.message}`,
level: "error",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message: `parseFileToEnv() - Environment not found: ${parsedFileResult.error.message}`, | |
| message: `parseFileToEnv() - Environment load failed: ${parsedFileResult.error.message}`, |
🤖 Prompt for AI Agents
In @src/renderer/actions/local-sync/fs-utils.ts at line 1104, The error text in
parseFileToEnv() is misleading ("Environment not found"); update the error
log/throw to reflect the actual failure by using parsedFileResult.error.message
and parsedFileResult.error.code and avoid asserting "not found" — e.g., change
the message produced where parsedFileResult is handled to something like
"parseFileToEnv() - Error parsing environment: [code] message" (use
parsedFileResult.error.code and parsedFileResult.error.message) so the
log/exception reports the real error type and details.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/renderer/actions/local-sync/fs-utils.ts:
- Line 1121: The code uses a hardcoded forward slash in the isGlobal check
(const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`)), which breaks on
Windows; replace this with a cross-platform check such as comparing the basename
(e.g., path.basename(file.path) === GLOBAL_ENV_FILE) or using path.sep (e.g.,
file.path.endsWith(path.sep + GLOBAL_ENV_FILE)) so the GLOBAL_ENV_FILE detection
works on all OSes; update the isGlobal assignment to use path.basename or
path.sep accordingly (path is already imported).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/actions/local-sync/fs-utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/actions/local-sync/fs-utils.ts (5)
src/renderer/actions/local-sync/common-utils.ts (3)
parseRaw(85-108)getIdFromPath(127-130)createFileSystemError(212-233)src/renderer/actions/local-sync/file-types/file-types.ts (1)
EnvironmentRecordFileType(25-35)src/renderer/actions/local-sync/constants.ts (1)
GLOBAL_ENV_FILE(12-12)src/renderer/actions/local-sync/types.ts (2)
Environment(108-114)EnvironmentVariableValue(91-96)src/renderer/actions/local-sync/schemas.ts (1)
EnvironmentRecord(190-193)
🔇 Additional comments (10)
src/renderer/actions/local-sync/fs-utils.ts (10)
2-2: LGTM - Sentry integration added.The Sentry import is properly added to enable error tracking throughout the file.
279-300: LGTM - Validation failure tracking without sensitive data.The Sentry event correctly captures validation failures while avoiding logging raw content. Using
contentMeta(string length or key count) provides useful debugging context without exposing potentially sensitive data.
329-342: LGTM - Write exception tracking with proper context.The write exception event correctly uses a generic "File write error" message suitable for all file types handled by
writeContent. The stack trace and error details provide adequate debugging information.
1096-1118: LGTM - Comprehensive error tracking for environment loading.The error handling structure properly captures both parse failures and exceptions with appropriate Sentry events. The
environment_load_failedevent includes the error code for better diagnostics.
1134-1149: LGTM - Exception handling with proper telemetry.The catch block appropriately captures unexpected exceptions during environment loading with full stack traces for debugging while maintaining the existing error flow.
1167-1174: LGTM - Value resolution preserves user data correctly.The value resolution logic properly cascades through
localValue→syncValue→ legacyvalue, ensuring user-entered data is preserved as described in the PR objectives.
1176-1184: LGTM - Correct null/undefined checks for required fields.The validation correctly uses explicit
undefinedandnullchecks for theidfield, avoiding the falsy trap whereid: 0would be incorrectly flagged as missing.
1190-1198: LGTM - Secret type handling preserves security flag.The transformation correctly converts
Secrettype toStringfor storage while preserving the secret nature via theisSecretflag, as outlined in the PR objectives.
1201-1216: LGTM - Aggregated diagnostics reduce noise.The single warning event aggregates all missing-field issues with detailed per-variable context, achieving the noise reduction goal mentioned in the PR objectives while retaining full diagnostic information.
1219-1233: LGTM - Exception handling with proper telemetry.The catch block appropriately captures unexpected transformation errors and rethrows them to maintain existing error flow while adding observability.
| content: environment, | ||
| }; | ||
| const { content } = parsedFileResult; | ||
| const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); |
There was a problem hiding this comment.
Cross-platform path separator issue persists.
This issue was flagged in a previous review but remains unaddressed. Using a hardcoded forward slash / will fail on Windows where the path separator is \.
🐛 Recommended fix
- const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`);
+ const isGlobal = path.basename(file.path) === GLOBAL_ENV_FILE;Note: The path module is already imported at line 61.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isGlobal = file.path.endsWith(`/${GLOBAL_ENV_FILE}`); | |
| const isGlobal = path.basename(file.path) === GLOBAL_ENV_FILE; |
🤖 Prompt for AI Agents
In @src/renderer/actions/local-sync/fs-utils.ts at line 1121, The code uses a
hardcoded forward slash in the isGlobal check (const isGlobal =
file.path.endsWith(`/${GLOBAL_ENV_FILE}`)), which breaks on Windows; replace
this with a cross-platform check such as comparing the basename (e.g.,
path.basename(file.path) === GLOBAL_ENV_FILE) or using path.sep (e.g.,
file.path.endsWith(path.sep + GLOBAL_ENV_FILE)) so the GLOBAL_ENV_FILE detection
works on all OSes; update the isGlobal assignment to use path.basename or
path.sep accordingly (path is already imported).
Added Sentry instrumentation around environment read/transform so load failures and transform exceptions surface with path and error metadata instead of silent drops fs-utils.ts:1090-1152.
Preserved user-entered env values by resolving localValue → syncValue → legacy value before writing; keeps secrets flagged and coerces secret types to string storage fs-utils.ts:1153-1189.
Aggregated missing-field diagnostics per variable and emit a single Sentry warning describing count and fields missing, reducing noise while keeping context fs-utils.ts:1189-1206.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.