Repository navigation
[VSDK-770] - Identify Flutter Commons signaling - #49
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16aaf5955f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
P1 #2 (session_manager.dart): persist the decorated telnyxConfig (with Commons SDK identity) for later push reconnections instead of the caller-supplied undecorated config. Previously, after a direct credential or token login, connectWithPushMetadata() reconnected with the undecorated config and lost mobileSdkName/mobileSdkVersion on the wire. P1 #3 (sdk_identity.dart / config_helper.dart / release.sh / tests): remove the hard-coded '0.2.0' literal scattered across the package and source commonsSdkVersion from one release-controlled location. - sdk_identity.dart is now the single source of truth for both commonsSdkName and commonsSdkVersion, with file-level dartdoc describing the contract. - config_helper.dart imports commonsSdkName / commonsSdkVersion instead of repeating the literals. - scripts/release/release.sh bump now updates both pubspec.yaml and sdk_identity.dart in a single edit, so the next release identifies itself correctly on the wire. - A new commonsSdkVersionPubspecSync drift-guard test reads pubspec.yaml from disk and fails the suite if commonsSdkVersion diverges from pubspec.yaml version. Verified end-to-end: bump 0.3.0 updates both files and the test passes; manually setting commonsSdkVersion to 0.9.9 fails the test with a clear actionable message. P1 #1 (restore hosted telnyx_webrtc dependency in pubspec.yaml) remains blocked on the upstream VSDK-769 / telnyx_webrtc release that exposes Config.mobile_sdk_name on pub.dev. Will be addressed in a follow-up once the upstream release ships; pubspec.yaml still points at the vsdk-769-sdk-identity-override Git ref for now. flutter analyze: no new issues introduced. flutter test: 20/20 passing (3 existing identity tests + drift-guard).
|
[afk-bot] Addressed P1 review items #2 and #3 from Codex (2026-10-13T10:09:42Z) in new commit P1 #2 — Store decorated config for push connections (FIXED)
P1 #3 — Derive signaled version from package release (FIXED)
P1 #1 — Restore hosted
P1 #1 will be addressed in a follow-up commit once Validation
Branch pushed: |
The previous ref pointed at the feature branch `vsdk-769-sdk-identity-override` (now merged). After PR #348 was merged into `main` on 2026-10-14T05:57:59Z, that branch is fragile: auto-deletion or a force-push would silently break this Commons package's resolution. Pin the dep to the immutable merge commit `b3c79292a193b2794c55c5cc88c13f9a68947811` on `team-telnyx/flutter-voice-sdk` (`main`). The merge commit includes `Config.mobile_sdk_name` and `Config.mobile_sdk_version` and the wrapper override hook in `version_utils.dart`, so the existing Commons identity plumbing (sdk_identity.dart, config_helper.dart, session_manager.dart) and the mobile_sdk_identity_test.dart suite keep working without churn. This is NOT a fix for Codex P1 #1 ("restore a hosted dependency before publishing"). That requires a hosted `telnyx_webrtc` release (>= 4.5.1 / 4.6.0) exposing the identity override on pub.dev; the upstream release is gated on a maintainer `workflow_dispatch` of `release-01-create-pull-request.yml` and the bot has no signal yet that such a release is in flight. The next bot tick after a release ships will swap this Git dep for a hosted `^4.5.1` (or newer) constraint. Verified locally: - `flutter pub get` → resolves `telnyx_webrtc` from git @ b3c792 in packages/telnyx_webrtc (was 4.4.0, now 4.5.0 with the VSDK-769 identity fields present) - `flutter analyze` → 30 issues (all pre-existing: example-app Firebase config, `avoid_print`, prefer-const in example/, the `invalid_dependency` warning for this same Git dep that we knowingly carry, plus two deprecated_member_use in session_manager.dart unchanged by this PR) - `flutter test` → 20 tests pass (16 base + 4 new mobile SDK identity tests covering credential/token/stored/decorated configs and the commonsSdkVersionPubspecSync drift guard)
|
Reviewed the remaining hosted-dependency finding against the current ecosystem state. The push/reconnect and release-version findings are fixed on this branch. The hosted dependency cannot be corrected yet: VSDK-769 is merged to flutter-voice-sdk main at b3c79292, but the latest published telnyx_webrtc release remains 4.5.0 and does not contain the wrapper-identity API. Replacing the pinned Git dependency with ^4.5.0 would compile against an incompatible API; publishing Flutter Commons must therefore wait for the next hosted telnyx_webrtc release, then this PR should switch to that released constraint before merge/release. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ebe593812
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
[afk-bot] Proposed fix for Codex review P1 on Proposed diff (
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
wojciechowskiradek
left a comment
wojciechowskiradek
left a comment
There was a problem hiding this comment.
Review
What this PR does
Implements VSDK-770: passes flutter_commons identity (name + version) through credential and token configs, preserving it for stored-config and push/reconnect paths. Depends on Flutter core PR #348 (VSDK-769).
Design assessment
Good approach — single source of truth via sdk_identity.dart with shared constants and decorator functions (withCommonsCredentialIdentity / withCommonsTokenIdentity). The stored config is now the decorated version, fixing the push reconnect identity gap that Codex flagged.
Positive
sdk_identity.dartcentralizes identity literals — one place to change name/version_storedConfig = telnyxConfig(decorated) fixes the push reconnect path dropping identityConfigHelper.getConfig()also applies identity to restored configs from SharedPreferences- Tests cover direct credential/token identity, stored credential restore, and stored token restore
- Comments explain the decorated-config rationale well (references Codex P1 #2)
Issues to address before merge
-
Git dependency in pubspec.yaml (Codex P1 #1) —
telnyx_webrtcis now a git dependency pinned to commitb3c79292. This will fail pub.dev publication (Git deps are not allowed for published packages). The comment acknowledges this and says it's deferred until upstream ships a release. This PR must NOT be published to pub.dev until the git dep is replaced with a hosted constraint. Consider keeping this PR as draft until the upstream release lands. -
Hardcoded version in sdk_identity.dart (Codex P1 #3) —
commonsSdkVersion = '0.2.0'is hardcoded. The release script (release.sh) bumpspubspec.yamlbut not this file, so every release will signal0.2.0. Either derive the version from pubspec at runtime (const String commonsSdkVersion = ...from package_info_plus), or addsdk_identity.dartto the release script's bump targets. -
Release script add-paths exclusion (Codex P1 #4) — the release workflow's
create-pull-requeststep only commitspubspec.yamlandCHANGELOG.md. If issue #2 is fixed by having the release script editsdk_identity.dart, that file won't be included in the release PR. The workflow needs updating to also includelib/src/internal/sdk_identity.dart. -
release.shonly adds a blank line — the diff toscripts/release/release.shadds just a blank line (+at line 147). If the intent was to also bump the identity version here, that's missing. If it's just formatting, it's noise.
Non-blocking observations
-
Community post version derivation — the iOS PR (#420) pulls version from
Bundle.infoDictionaryand React Native (#145) frompackage.json. Flutter could usepackage_info_plusto read version at runtime, avoiding the hardcoded constant entirely. This would also resolve issues #2 and #3. -
effectiveSdkName/effectiveSdkVersion— tests use these accessors which must be from the core SDK (VSDK-769). Not visible in this diff, assumed correct from the core PR.
Verdict
Approve with conditions — the implementation is solid and the stored-config fix is important. However:
telnyx_webrtc 4.5.1 is the first hosted release that ships the top-level Verto mobile_sdk_name / mobile_sdk_version identity (Flutter core #350, merged 2026-10-15). Replace the temporary Git pin against fix/vsdk-769-wire-identity-v2 with the hosted range so consumers get the wire-contract fix without dragging in the unreleased main branch. Pinned to ^4.5.1 so consumers opt into future major versions explicitly rather than being forced onto them.
Summary
webrtc-flutter-commonsand package version0.2.0through credential and token configs.Verification
dart formatpassed on changed Dart files.git diff --checkpassed.Dependency
Blocked on Flutter core PR #350 and a subsequent hosted
telnyx_webrtcrelease containing that fix. Before merging this PR, replace the temporary Git dependency with the compatible hosted package version and re-run validation.No release is included.