fix: replace empty catch blocks with handling#69
fix: replace empty catch blocks with handling#69abhijeetnardele24-hash wants to merge 1 commit intolinearis-oss:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes remaining silent catch {} blocks across the src/ tree by either documenting intentional fallbacks inline or by surfacing unexpected I/O errors instead of treating them as “not found”.
Changes:
- Replaced empty
catch {}blocks with explicitcatch (_error)plus inline rationale where swallowing is intentional. - Added
isMissingFileError()and used it to distinguish “file missing” from other filesystem errors inFileService. - Updated URL/identifier parsing helpers to treat malformed inputs as expected in permissive parsing paths while avoiding silent swallowing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/utils/linear-service.ts |
Documents the intentional fallback from team key lookup to exact name lookup (but still catches all errors). |
src/utils/identifier-parser.ts |
Makes permissive parsing explicit by returning null with a rationale on parse failures. |
src/utils/file-service.ts |
Adds missing-file detection and surfaces non-ENOENT filesystem errors instead of flattening them to “not found”. |
src/utils/embed-parser.ts |
Documents expected invalid URL inputs when parsing arbitrary markdown content. |
src/commands/documents.ts |
Makes malformed URL handling explicit/documented when extracting Linear document IDs. |
Comments suppressed due to low confidence (1)
src/utils/linear-service.ts:266
- In resolveTeamId(), the catch currently triggers the name-lookup fallback for any error coming from the key lookup (including auth/network/SDK failures), which can mask unexpected failures and change the error the caller sees. Consider only falling back when the key lookup truly results in a not-found condition (e.g., by having executeLinearQuery throw a dedicated NotFound error, or by checking for the specific not-found error it emits) and rethrow other errors.
try {
const team = await executeLinearQuery(
() =>
this.client.teams({
filter: buildEqualityFilter("key", teamKeyOrNameOrId),
first: 1,
}),
"Team",
teamKeyOrNameOrId,
);
return team.id;
} catch (_error) {
// Not every caller passes a team key; fall back to an exact name lookup.
const team = await executeLinearQuery(
() =>
this.client.teams({
filter: buildEqualityFilter("name", teamKeyOrNameOrId),
first: 1,
}),
"Team",
teamKeyOrNameOrId,
);
return team.id;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if file already exists (unless overwrite is enabled) | ||
| if (!options.overwrite) { | ||
| try { | ||
| await access(outputPath); | ||
| return { | ||
| success: false, | ||
| error: | ||
| `File already exists: ${outputPath}. Use --overwrite to replace.`, | ||
| }; | ||
| } catch { | ||
| // File doesn't exist, we can proceed | ||
| } catch (error) { | ||
| if (isMissingFileError(error)) { | ||
| // Missing output path is expected here; we'll create it below. | ||
| } else { | ||
| return { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
downloadFile() now has differentiated handling for access() failures (treat ENOENT as expected, but surface other errors). There are existing unit tests for FileService.uploadFile(), but no unit tests appear to cover downloadFile(); please add tests to lock in the new behavior (e.g., access rejects with ENOENT vs EACCES, and overwrite=false behavior).
|
Fixes #60 |
Summary
Validation
px vitest run tests/unit/documents-url-parsing.test.ts tests/unit/file-service-upload.test.ts
px tsc --noEmit