refactor(llc, ui)!: adopt stream_core's platform detector and sealed exception family - #2960
refactor(llc, ui)!: adopt stream_core's platform detector and sealed exception family#2960xsahil03x wants to merge 11 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
cb926c1 to
2c2bbbc
Compare
2c2bbbc to
25afe4a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## migrate-core/05-coordinate-listext #2960 +/- ##
=====================================================================
Coverage ? 74.03%
=====================================================================
Files ? 424
Lines ? 28364
Branches ? 0
=====================================================================
Hits ? 20999
Misses ? 7365
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
25afe4a to
580e45e
Compare
38da5ce to
101fd5e
Compare
ae71e1b to
ca73fa4
Compare
ca73fa4 to
a713085
Compare
a713085 to
2625478
Compare
2625478 to
9dde64d
Compare
40d6a8f to
9dd4bd8
Compare
9dd4bd8 to
ded1b03
Compare
9a5b7fc to
f7bbde2
Compare
f7bbde2 to
289348f
Compare
289348f to
58f125c
Compare
58f125c to
d781fdd
Compare
d781fdd to
303d983
Compare
303d983 to
41e423e
Compare
41e423e to
5bf510f
Compare
5bf510f to
7bb9f78
Compare
7bb9f78 to
fd2caed
Compare
`CurrentPlatform` and `PlatformType` were a near-verbatim fork of core's. Deleting ours drops the `js_interop` branch chat needed only because its own stub threw; core's non-`io` fallback answers `web` directly. BREAKING CHANGE: `CurrentPlatform.name` is now `CurrentPlatform.operatingSystem` and reports the same string. Both types are re-exported from this package, so an import of `stream_chat.dart` needs no change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces our error class tree with core's four `StreamException` kinds and deletes `stream_chat_error.dart` outright. Two of its types were scheduled to outlive this phase; neither needed to. Nothing raised `StreamChatNetworkError` once the verb facade threw a `StreamException`, and the socket path maps onto core's kinds without waiting for the new transport. Deleting beats deprecating here: nothing throws the old types, so a deprecated `on StreamChatNetworkError catch` would keep compiling and silently match nothing. Removing them turns that into a compile error. Also fixes five `is` checks on the old type that had gone unreachable — four `scheduleRetry` gates in `channel.dart` and the `sync` 400 recovery in `client.dart`. A failed message was never queued for retry, and an app with a stale `last_sync_at` could not heal. The tests covering them asserted message state rather than the queue, so they passed against dead branches. BREAKING CHANGE: `StreamChatError`, `StreamChatNetworkError`, `StreamChatNetworkErrorType`, `StreamWebSocketError` and `ChatErrorCode` are removed. Catch `StreamChatException` — an alias of core's sealed root — or one of `StreamApiException`, `StreamNetworkException`, `StreamAuthenticationException` or `StreamClientException`. `isRetriable` is now an extension on the alias, and `stream_chat_flutter`'s three attachment-validation errors moved to their own sealed `AttachmentValidationError` family. See migrations/v11-migration.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A live anonymous connect against a real app key succeeds, which was the last open item. Records why that had to be tested rather than argued. Reading the backend source end to end predicted the opposite: `handshake` validates before it authenticates, `ConnectUserDetails.ID` carries `validate:"userID,required"`, and that tag's regex does not match `!anon`. Every link holds on its own and the conclusion was still wrong, on a path the backend has no test for either. Why the request passes is left unexplained rather than guessed at. What does survive is that the server discards the client's id on the anonymous path — it rebuilds its own user — so the switch to `!anon` could not have changed behaviour there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The existing interceptor tests assert each type is present, which a fully reordered pipeline satisfies just as well, and `ApiErrorInterceptor` had no test at all. Order is behaviour here: anything rejecting ahead of `ApiErrorInterceptor` escapes as a raw `DioException`, and anything logging ahead of it logs the transport error rather than the mapped one. Verified the assertion fails on a swap rather than only passing as written. Also corrects the phase doc, which recorded "six verbs, not eight" as done. `fetch` and `request` are still on the facade with no caller in `lib/`; they go when `StreamHttpClient` becomes `@internal` in phase 09, rather than breaking a public class twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 09 assumed 05 would delete `StreamHttpClient`, which would have forced `AttachmentFileUploaderProvider` to be retyped. 05 kept it instead — it is the verb facade — so the typedef compiles unchanged and nothing forces the retype. What was an obligation is now a choice, and the two phases pull opposite ways: leaving the typedef alone keeps 05's `@internal` item open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`melos.yaml` declares `stream_core: ^0.5.0` but `dependencyOverridePaths` resolves it to the sibling checkout, whose pubspec also says 0.5.0 while carrying APIs the published version does not. Every local run has therefore been green against code no consumer can get. Chat already depends on five of them: `debugCurrentPlatformOverride`, `sortedMerge`/`sortedUpsertAt`, `normalizeStringForSort`, `Filter.raw` and element-wise array matching. `Filter.raw` is the sharpest — persistence's filter converter and `PredefinedFilter` both need it. Also drops the rows this migration has since closed, and fixes UPSTREAM's closing advice, which still named `$nor` (dropped) and `debugCurrentPlatformOverride` (landed) as what to push first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three places said the opposite of what this stack does, each true when written and stale a rung or two later. The migration guide carried both halves of a reversed decision in one blockquote: the paragraph saying `StreamChatNetworkError` is deprecated, telling readers "the deprecation warning tells you where", directly above the paragraph saying it is deleted. The first was mine — I appended the replacement without removing what it replaced. A reader upgrading would have searched for warnings that never appear, because the type is gone and the failure is a compile error. Phase 02's doc still said "merging is gated on a core release" and marked the platform detector blocked, in the PR that deletes it, with its own definition-of-done fully ticked. `DEFERRED.md` described the resolution mechanism as `dependencyOverridePaths` against a sibling checkout, which is what it *was* before CI proved it only works on a machine that has one. And the schema version reads `1102`, which is what the database declares. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three corrections against the spec, all in the sealed family this branch adopts. A cancellation is a cancelled request. A send superseded by a later one, and a pending upload dropped because its message was deleted, were `StreamClientException` — the kind whose documented reaction is "report to your crash tracker". They are now `StreamNetworkException` with `isCancelled` set, which is how a cancelled attachment upload was already reported two hundred lines away. Misuse is not in the hierarchy at all. Connecting twice, opening a connection that is already open, `queryChannels` without one, a persistence client that is not set or belongs to another user, and cancelling an upload that never started — all say fix the call rather than handle the failure, so they raise `StateError`. `queryChannels` without a connection is close to the spec's own example. An upload failure said `Failed to upload one or more attachments` and nothing else. It now names which ones and what each reported. A real `cause` is out of reach until `UploadStateFailed` holds more than a string, which is phase 09's model swap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rung that introduced these paragraphs states them as of itself, and they flow up unchanged, so by here three had gone stale. The collision list is down to `AttachmentFile` and `User`: the query DSL, the platform detector, the token and interceptor types and the logger types are all core's by now. Nothing upstream is blocking either — phase 08's changes shipped in stream-core-flutter#181, and the operators an earlier pass called a hard block turned out not to be asks at all. 07's findings do reach its own file by this rung, so the parked note can point at them again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The section still described `sort: null` as coercing to the default — the setter no longer takes null, and `XSort.empty` is what sends none — and told every poll-vote caller to undo a default flip that was reverted before it shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| throw const StreamChatError( | ||
| "Upload request for this Attachment hasn't started yet or maybe " | ||
| 'Already completed', | ||
| throw StateError( |
There was a problem hiding this comment.
I am curious why is this no longer a Stream- scoped error?
Is it because it indicates an invalid use of the API, and not really a client-level error?
Edit: I saw the updated changelog - it answers my question! We would probably need the sweep the other API surface for similar stuff.
There was a problem hiding this comment.
Right on both counts — StateError because it reports a caller mistake (cancelling an upload that never started, or was already cancelled), not something the server said.
On the sweep: it is already done across the stack. There is no throw StreamChatError left anywhere in packages/*/lib, and the split now reads:
| package | StateError |
ArgumentError |
StreamClientException |
|---|---|---|---|
stream_chat |
15 | 11 | 3 |
stream_chat_flutter_core |
1 | 0 | 0 |
stream_chat_flutter |
2 | 4 | 0 |
stream_chat_persistence |
1 | 2 | 0 |
The rule applied was: ArgumentError for a bad argument, StateError for a call made at the wrong time, and StreamClientException only where the SDK itself failed at something it promised to do — the three in stream_chat are all the attachment-upload aggregate (_uploadFailureMessage), which is a real SDK failure rather than misuse. ERROR_LAYER.md is the source for that boundary.
Linear: FLU-
CLA
🎯 Goal
Closes the migration: the platform detector goes, and the old error tree is deleted outright rather than left deprecated.
🛠 Implementation details
CurrentPlatformandPlatformTypebecome core's, re-exported from this package.CurrentPlatform.namebecomes.operatingSystem, reporting the same string. Deleting our copy also drops thejs_interopbranch chat needed only because its own stub threw.StreamChatError,StreamChatNetworkError,StreamChatNetworkErrorTypeandStreamWebSocketErrorare removed, not deprecated. Nothing throws them any more, so a deprecated type would leaveon StreamChatNetworkError catchcompiling while silently matching nothing — a break you ship without noticing. Removing it is a compile error instead.isRetriablebecomes an extension onStreamChatException, so it reads the same on any failure.stream_chat_flutter's three attachment-validation errors move to their own sealedAttachmentValidationErrorfamily. They are returned, never thrown, so typing a refused attachment as a transport failure invited callers to treat it as one.☑️ Verification
melos run analyze,melos run formatandmelos run test:dartpass against the pinnedstream_corecommit this stack resolves from.The pipeline-order test was checked to fail on a swap rather than only to pass as written — moving
ApiErrorInterceptorafter the logging interceptor makes it red.