fix(database): surface typeorm errors in ConnectionService.update so UI can show editfailed#2046
Conversation
…UI can show editfailed
There was a problem hiding this comment.
Pull request overview
This PR improves user-visible error handling when updating an existing connection in the local database by ensuring ConnectionService.update() returns undefined on TypeORM failures (instead of rejecting), allowing existing UI “edit failed” handling to run.
Changes:
- Wrap
ConnectionService.update()in atry/catch, log the underlying error, and returnundefinedon failure. - Update the bulk-import path (
importOneConnection) to explicitly fail fast whenupdate()returnsundefined, preserving prior import failure semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ysfscream
left a comment
There was a problem hiding this comment.
Thanks for the fix. The approach looks good, but one caller still needs the same failure handling: ConnectionInfo.vue currently only emits when connectionService.update() returns a result, and still fails silently when it returns undefined. Could you add the same editfailed feedback there as in ConnectionForm?
|
Thanks for the fix. The approach looks good. One non-blocking follow-up: |
PR Checklist
If you have any questions, you can refer to the Contributing Guide
What is the current behavior?
When
ConnectionService.update(id, data)is called from the connection edit form (src/views/connections/ConnectionForm.vue:739) or the connection settings dialog (src/views/connections/ConnectionInfo.vue:192), and the underlying typeormconnectionRepository.save()(or the cascadingwillService.save()) throws — for example because:Will Messagefield exceeds a CHECK constraint after a recent migration,parentcollection was concurrently deleted,— the
QueryFailedErrorpropagates past the Vue async handler unhandled. The UI's existing failure path atConnectionForm.vue:757:never fires (because
reswas never assigned — the await rejected). The user clicks Save, nothing visibly happens, and the error only appears in the renderer devtools console (which most users don't open).Issue Number
No existing issue — happy to file one if preferred.
What is the new behavior?
ConnectionService.update()now wraps its body in atry { ... } catch (error) { console.error(...); return undefined }block, mirroring the existing pattern insrc/database/services/DashboardService.ts:159-174(updateOrders) andsrc/database/services/WidgetService.ts:120-128(updateOrders).When the typeorm
save()throws,update()now returnsundefined. The existingif (!(res && res.id))check inConnectionForm.vue:757then fires thecommon.editfailedtoast (already wired up, already i18n'd in all supported locales). The error is also logged toconsole.errorfor renderer devtools / log file capture.ConnectionService.importOneConnectionat line 213 (the third caller ofupdate(), used by the bulk-import flow) previously relied onupdate()throwing to halt the import viaimport()'s try/catch wrapper (line 270-293). Now thatupdate()returnsundefinedon failure instead, the import path needs an explicit guard to preserve that signal — otherwiseimport()would silently mark a partially-failed import as'ok'andImportData.vue:403-414would show a misleading success toast. Added a 3-lineif (!updated) throw new Error(...)guard at line 213 to keep the import-path failure semantics unchanged.Does this PR introduce a breaking change?
update()'s return type was alreadyPromise<ConnectionModel | undefined>by inference (becausethis.get(id)returnsConnectionModel | undefined). All call sites already handleundefined. The PR makes the return type explicit and adds theundefinedpath on caught error — no caller behaviour changes for the happy path.Specific Instructions
tests/unit/covers Vue components andutils/). Adding one would require introducing test scaffolding fortypedi+typeormimport resolution that webpack 4 currently can't compile (typeorm pulls inmongodbwhose modern-JS doesn't parse). Happy to add a test in a follow-up PR if you can point at how you'd like service-layer tests structured.yarn run electron:serve), open the renderer devtools, then inConnectionService.update's body, temporarily injectthrow new Error('test')before thesave()call. Without the fix: clicking Save in the connection edit form fails silently with only a console error. With the fix: the existingcommon.editfailedtoast appears at the top of the window.create()(line 374 — same shape of issue, separate follow-up PR), the migrations folder, or the other service files (CollectionService,MessageService, etc. have similar issues — also follow-up scope per "one violation site per PR").DashboardService.updateOrdersandWidgetService.updateOrdersexactly: same try/catch shape, sameconsole.error('Error <verb>:', error)style, same falsy-return convention. The same in-repo neighbours are the strongest argument that this is the project's preferred handling pattern fordatabase/services/files.Other information
The explicit return-type annotation (
Promise<ConnectionModel | undefined>) is a no-op for callers —this.get(id)already returnsConnectionModel | undefined, so the inferred type was the same. Making it explicit communicates intent: "this can fail, you must check," matching the explicit return type onConnectionService.create()at line 374.