fix(server): keep a deleted server's notifier out of error state - #1520
Conversation
Deleting a server left its `serverProvider` in an error state, and the app logged `Unhandled (FlutterError): Bad state: Server ... not found` twice per delete. `ServerNotifier` is `keepAlive`, so `ref.invalidate` does not dispose it: it runs the dispose listeners and schedules a refresh, and Riverpod refreshes an *active* element. With the server's editor still on screen — the page the delete button sits on — `build` ran again after the record was gone and threw. The same shape sits in the rename and sync-reload paths, which invalidate a removed id too. The dispose listener also read `state` to close the SSH client, which forces that rebuild; the throw came before the `close`, so the connection stayed open. `build` now keeps the state it last had when the record is gone: its readings, marked disconnected. Nothing polls it again — `refresh` walks the servers that still exist — and a restore that reuses the id is picked up by the lookup. The client is closed from a field that every path installing or dropping one keeps in step, so disposal no longer reads `state`. Adds `server_delete_live_notifier_test.dart`: deleting a watched server, and a record dropped by a sync pull, both leave the provider readable. Both cases failed before this change. Refs #1512.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
Suggested reviewers: Priority: ➖ Normal Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the deleted-server provider objective in issue Resolution Handle a missing server ID without throwing when the notifier has no previous state, or otherwise prove that this state cannot occur during the required entry lifecycle. Add the required graceful
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
What
Deleting a server left its
serverProviderin an error state, and the app loggedUnhandled (FlutterError): Bad state: Server ... not foundtwice per delete (#1512).Why
ServerNotifieris@Riverpod(keepAlive: true), soref.invalidatedoes not dispose the element. In Riverpod 3invalidateSelfruns the dispose listeners and then schedules a refresh, and_performRefreshflushes an element that isisActive— one something is still watching. The server editor does exactly that, and it is the page the delete button sits on.So after
_delServerremoved the record,buildran again, found no SPI and threw. The same shape is in the rename path (_updateServer) and inreload()after a sync pull drops a record — both invalidate an id that is already gone.The dispose listener also read
stateto close the SSH client. Reading the state there forces that same rebuild, and the throw came before theclose, so the connection was left open too.How
buildkeeps the state it last had when the record is gone: the readings it already had, markeddisconnected, client dropped. Nothing polls it again (refreshwalks the servers that still exist), and a record restored under the same id is picked up by the lookup on the next rebuild. Abuildfor an id that never existed still throws.state.Tests
test/unit/server/server_delete_live_notifier_test.dartcovers both triggers: a watched server deleted throughdelServer, and a record dropped by areload(a sync pull). Both failed before this change.flutter analyze lib test integration_testis clean.test/unit/serveris green apart from the pre-existing cases that need/bin/shand systemd, which a Windows host cannot run.Fixes #1512.
Summary by CodeRabbit