feat: setup sse connection to broadcast skin and project updates#5
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughVersion 0.0.2 adds backend skin streaming, lifecycle-aware stream management, a public skin-change stream, example app rebuilds on updates, and release/version metadata updates. ChangesSSE Skin-Change Broadcasting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 1: The changelog version is out of sync with the package version, so
update the entry in CHANGELOG.md to match the version set in pubspec.yaml.
Locate the top release heading in the changelog and change the documented
version from 0.0.2-alpha.1 to 0.0.2 so the release notes align with the
published package.
- Line 1: The first heading in the changelog uses a level-2 heading, which
violates markdownlint MD041; update the top-of-file heading in CHANGELOG.md to
use an H1 instead. Keep the existing version/date text, but change the heading
marker at the start of the file from a second-level heading to the first-level
heading so the document begins with the correct markdown structure.
In `@example/lib/main.dart`:
- Around line 8-10: The example app is hardcoding a real-looking apiKey value in
main, which should not be committed to source. Update the initialization in main
so it reads the key at runtime from a dart-define or environment-based
configuration instead of embedding the secret directly, and replace the literal
with a clearly fake placeholder or fallback. Use the main entrypoint and the
apiKey parameter as the spots to update, and ensure no live credential remains
in the example source.
- Around line 21-28: The `MyAppState` listener is created in `initState()` but
never stored or cancelled, and its callback calls `setState` without checking
whether the widget is still mounted. Capture the
`FlutterSkin.onSkinChanged.listen(...)` result in a `StreamSubscription`, cancel
it in `dispose()`, and guard the callback with `mounted` before calling
`setState`; also add the needed `dart:async` import for the subscription type.
In `@example/lib/widgets/movie_card.dart`:
- Around line 100-102: The movie card description is hardcoded to a Lorem ipsum
placeholder, causing every card to show identical text. Update the Text in
movie_card.dart within the MovieCard widget to use the per-movie description
field from the Movie model (such as description or overview) instead of the
fixed string, and keep the existing layout/maxLines behavior intact; if no such
field exists, verify that the placeholder is intentionally meant for demo use.
In `@lib/remote/fskin_remote_config.dart`:
- Around line 53-55: The cached skin update path in FSkinRemoteConfig is
emitting ThemeData with a null color scheme when skin colors are missing, which
should be treated as no remote theme instead. Update the _cachedConfig branch in
the onSkinChanged flow to skip adding to _skinController unless
_cachedConfig!.skin?.colors is present, matching the behavior of toThemeData(),
so consumers can continue using their own fallback theme.
In `@lib/services/fskin_subscriber.dart`:
- Around line 9-16: The subscriber lifecycle in FskinSubscriber is not
idempotent because listen() only clears _disposed and can leave an existing
subscription, pending retry, or closed _client alive across re-initialization.
Update listen() to cancel any active stream subscription and pending retry Timer
(or guard reconnects with a generation/token check) before calling _connect(),
and make sure retries use the latest apiKey only. In dispose(), fully terminate
the subscriber state by canceling the subscription/retry and preventing any
later reconnect unless a new client is created; adjust the reconnect logic in
_connect() and the retry path around the affected block so stale callbacks
cannot fire after teardown.
- Around line 21-24: The SSE request in fskin_subscriber.dart is building the
stream URL by interpolating apiKey directly into the URI, which can break when
the key contains reserved characters. Update the request creation in the
subscriber logic to use Uri.https with queryParameters instead of Uri.parse
string concatenation, and reference the existing http.Request setup in the same
block so the encoded query is applied consistently. If the backend supports it,
consider moving the key out of the query string and into an auth header for
better handling of sensitive values.
- Around line 26-30: The retry counter is being reset too early in the request
flow inside the subscriber logic around the _client.send() response handling,
which causes failed HTTP responses to be treated as successful stream starts.
Move the _retryCount reset in the response callback so it only happens after the
response status has been validated as a valid SSE stream and _subscription is
created, and keep failed 401/404/500 responses on the retry path. Use the
existing _client.send(), _retryCount, and _subscription handling in
fskin_subscriber.dart to locate the fix.
- Around line 53-59: The retry logic in _retry currently computes seconds with
clamp(), which leaves it as a num and uses linear growth instead of exponential
backoff. Update the delay calculation in FskinSubscriber._retry to produce an
int, then change the retry schedule to exponential backoff (for example, based
on _retryCount) while keeping the existing 2–30 second bounds before passing it
to Duration(seconds: ...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6724452d-23e4-48cd-8605-43979dd41ecc
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdexample/android/app/src/main/AndroidManifest.xmlexample/lib/main.dartexample/lib/widgets/movie_card.dartlib/constants/fskin_constants.dartlib/flutter_skin.dartlib/remote/fskin_remote_config.dartlib/services/fskin_subscriber.dartlib/services/skin_service.dartpubspec.yaml
koukibadr
force-pushed
the
feat/live-skin/setup-sse-connection
branch
5 times, most recently
from
c7fb921 to
3b452f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/flutter_skin.dart (1)
17-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStart the SSE stream during initial initialization.
init()fetches the config but never calls_startStream(), so the persistent SSE connection is not opened until a later resume lifecycle event. Start the stream afterFskinRemoteConfig.init(...)succeeds.Proposed fix
await FskinRemoteConfig.init(apiKey: apiKey); + _startStream(); return _instance!;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/flutter_skin.dart` around lines 17 - 29, The init flow in FlutterSkin.init() fetches remote config but never opens the SSE connection, so the stream should be started immediately after FskinRemoteConfig.init(apiKey: apiKey) completes successfully. Update FlutterSkin.init to invoke _startStream() on the initialized _instance before returning, keeping the existing apiKey setup and lifecycle observer registration intact so the stream is available during first initialization rather than waiting for a resume event.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 7: The changelog entry for FlutterSkin.onSkinChanged has the wrong
emitted payload type; update the wording so it matches the public API contract
exposed by FlutterSkin.onSkinChanged as Stream<ThemeData>, not SkinTokens. Keep
the entry aligned with the actual stream behavior by replacing the type
description in the affected changelog line only.
In `@lib/flutter_skin.dart`:
- Around line 52-54: The resume handler in FlutterSkin is dropping the Future
returned by fetchConfig(), which can lead to unhandled async errors when the app
resumes. Update the AppLifecycleState.resumed path in the lifecycle override to
await FskinRemoteConfig.singleton.fetchConfig() inside a guarded async flow, or
attach catchError, and make sure _startStream() still runs from a finally path
so the stream restarts even if refresh fails.
- Around line 54-57: The lifecycle handling around _startStream and the
AppLifecycleState.paused/resumed branch is reusing a disposed FskinSubscriber,
which leaves _sse backed by a closed http.Client after resume. Update the resume
path to recreate _sse before calling _startStream, and ensure the logic in
listen/send uses a fresh subscriber/client instance rather than the disposed
one.
---
Outside diff comments:
In `@lib/flutter_skin.dart`:
- Around line 17-29: The init flow in FlutterSkin.init() fetches remote config
but never opens the SSE connection, so the stream should be started immediately
after FskinRemoteConfig.init(apiKey: apiKey) completes successfully. Update
FlutterSkin.init to invoke _startStream() on the initialized _instance before
returning, keeping the existing apiKey setup and lifecycle observer registration
intact so the stream is available during first initialization rather than
waiting for a resume event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3d5e4f7-9656-4457-93d1-26e8c998173c
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdexample/android/app/src/main/AndroidManifest.xmlexample/lib/main.dartexample/lib/widgets/movie_card.dartlib/constants/fskin_constants.dartlib/flutter_skin.dartlib/remote/fskin_remote_config.dartlib/services/fskin_subscriber.dartlib/services/skin_service.dartpubspec.yaml
koukibadr
force-pushed
the
feat/live-skin/setup-sse-connection
branch
from
3b452f6 to
39e52cf
Compare
This was referenced Jul 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
FlutterSkin.init()now performs full one-call setup, including initial loading and lifecycle-aware pause/resume behavior.0.0.2.