test: setup unit test for flutter_skin main class and CI check on PRs#8
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds an apiKey format validation constant, converts FskinRemoteConfig's onSkinChanged and projectConfig from static to instance getters, and introduces a mutable static remoteConfig on FlutterSkin with test hooks (resetInstance, setRemoteConfig). Init now validates apiKey; theme getter guards against uninitialized state. Tests, mocks, mockito dependency, and CI pipeline changes are included. ChangesTestable remote config and key validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant FlutterSkin
participant FskinConstants
participant FskinRemoteConfig
App->>FlutterSkin: init(apiKey)
FlutterSkin->>FskinConstants: validate apiKey against keyRegex
FskinConstants-->>FlutterSkin: valid/invalid
FlutterSkin->>FskinRemoteConfig: remoteConfig = singleton
App->>FlutterSkin: theme
FlutterSkin->>FlutterSkin: check _instance != null
FlutterSkin->>FskinRemoteConfig: remoteConfig.projectConfig
FskinRemoteConfig-->>FlutterSkin: ProjectConfig
FlutterSkin-->>App: ThemeData
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/remote/fskin_remote_config.dart (1)
20-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInstance getter
projectConfigshould usethis._cachedConfig, not the static_instance.Now that
projectConfigis an instance getter, it should return the instance's own_cachedConfig. The guardif (_instance == null)and the return_instance!._cachedConfigare leftovers from when this was a static getter. In production this works only becauseremoteConfigis the singleton, but it's incorrect for any non-singleton instance and is a design inconsistency.♻️ Proposed refactor
ProjectConfig? get projectConfig { - if (_instance == null) { - throw Exception( - 'FskinRemoteConfig must be initialized with FskinRemoteConfig.init()', - ); - } - return _instance!._cachedConfig; + return _cachedConfig; }🤖 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/remote/fskin_remote_config.dart` around lines 20 - 27, The instance getter projectConfig in FskinRemoteConfig still relies on the static _instance singleton check and return, which is leftover static behavior. Update projectConfig to validate and return the current instance’s own _cachedConfig via this, so the getter works correctly for any FskinRemoteConfig instance rather than only the singleton remoteConfig..github/workflows/pr_verification_pipeline.yml (1)
5-67: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a top-level
permissions:block andpersist-credentials: false.The workflow has no
permissions:block, so jobs run with the repo's default (broad) token permissions. These jobs only need read access to source code — none require write scopes. Additionally,actions/checkoutpersists a GITHUB_TOKEN credential in.git/configby default; since no subsequent step pushes or authenticates, setpersist-credentials: falseon all checkout steps.🔒️ Proposed fix
+permissions: + contents: read + jobs: dart_format_and_analyze:And on every checkout step:
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/pr_verification_pipeline.yml around lines 5 - 67, Add a top-level permissions block to the workflow so the jobs use least-privilege read-only access instead of the default broad token scope. Also update every actions/checkout step in the workflow to set persist-credentials to false, since none of the jobs need to keep a GitHub token in .git/config. Locate the workflow jobs dart_format_and_analyze, unit-test, and dry-run-upload and apply the checkout change consistently across each one.Source: Linters/SAST tools
🤖 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 @.github/workflows/pr_verification_pipeline.yml:
- Line 40: The downstream jobs in the GitHub Actions workflow still use
actions/checkout@v2, which is too old for current runners. Update the checkout
step in the unit-test and dry-run-upload jobs in pr_verification_pipeline.yml to
match the existing dart_format_and_analyze job by using actions/checkout@v4,
keeping the same checkout step structure but with the newer action version.
- Around line 8-15: The workflow defines a matrix value flutter-version in the
pr_verification_pipeline job, but the subosito/flutter-action step never
consumes it. Update the flutter-action configuration to use the matrix
flutter-version value in the with block for the flutter-version input (instead
of relying only on channel: stable) so the job runs the pinned version
consistently.
In `@lib/flutter_skin.dart`:
- Around line 22-29: resetInstance() only clears _instance and leaves
remoteConfig pointing at a prior test double, which can leak state across tests.
Update FlutterSkin.resetInstance() so it also resets remoteConfig to the default
singleton behavior, and make sure it does so safely even when
FskinRemoteConfig._instance has not been initialized yet. Use resetInstance(),
setRemoteConfig(), and remoteConfig in FlutterSkin to locate the change, and
ensure init(), onSkinChanged, fetchConfig, and projectConfig will observe the
real config after a reset.
- Line 35: The key validation in FlutterSkin.init is recompiling the same
pattern on every call by creating RegExp(FskinConstants.keyRegex) inline. Move
that regex into a static final field on FlutterSkin (or a nearby shared
constant) and reuse it in init() so the validation uses the precompiled RegExp
instead of rebuilding it each time.
In `@test/flutter_skin/flutter_skin_init_test.dart`:
- Around line 14-71: The FlutterSkin initialization tests are sharing static
state and still triggering real network access. Add test isolation around
FlutterSkin._instance and FlutterSkin.remoteConfig by resetting them in
setUp/tearDown using FlutterSkin.resetInstance() and restoring the default
remote config between tests. Also update FlutterSkin.init (and any direct
FskinRemoteConfig.init path it uses) so initialization goes through the
injectable remoteConfig or a mocked HTTP layer, allowing FlutterSkin.init,
FlutterSkin.singleton, and setRemoteConfig(MockFskinRemoteConfig()) to run
hermetically without live requests.
---
Outside diff comments:
In @.github/workflows/pr_verification_pipeline.yml:
- Around line 5-67: Add a top-level permissions block to the workflow so the
jobs use least-privilege read-only access instead of the default broad token
scope. Also update every actions/checkout step in the workflow to set
persist-credentials to false, since none of the jobs need to keep a GitHub token
in .git/config. Locate the workflow jobs dart_format_and_analyze, unit-test, and
dry-run-upload and apply the checkout change consistently across each one.
In `@lib/remote/fskin_remote_config.dart`:
- Around line 20-27: The instance getter projectConfig in FskinRemoteConfig
still relies on the static _instance singleton check and return, which is
leftover static behavior. Update projectConfig to validate and return the
current instance’s own _cachedConfig via this, so the getter works correctly for
any FskinRemoteConfig instance rather than only the singleton remoteConfig.
🪄 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: 23690c9d-6952-4285-bb44-3f8e0fbed819
📒 Files selected for processing (10)
.github/workflows/pr_verification_pipeline.ymllib/constants/fskin_constants.dartlib/flutter_skin.dartlib/remote/fskin_remote_config.dartlib/services/skin_service.dartpubspec.yamltest/flutter_skin/flutter_skin_init_test.darttest/flutter_skin/initialization_scenario_test.darttest/flutter_skin_test.darttest/mocks/skin_model_mocks.dart
💤 Files with no reviewable changes (1)
- test/flutter_skin_test.dart
koukibadr
force-pushed
the
test/setup-testing-and-ci-strategy
branch
from
377b6b6 to
46a4aaf
Compare
koukibadr
force-pushed
the
test/setup-testing-and-ci-strategy
branch
from
46a4aaf to
374feb4
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests