Set up OneSecStanfordStudy package#1
Conversation
|
Warning Review limit reached
Next review available in: 29 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds the Sequence Diagram(s)sequenceDiagram
participant User
participant StudyButton
participant StudySurveySheet
participant WebView
participant OneSecStanfordStudy
participant BulkHealthExporter
User->>StudyButton: tap Initiate Flow
StudyButton->>StudySurveySheet: present sheet
StudySurveySheet->>WebView: load surveyUrl
WebView-->>StudySurveySheet: didNavigate
StudySurveySheet->>OneSecStanfordStudy: updateState(.active)
StudySurveySheet->>OneSecStanfordStudy: triggerHealthExport(forceSessionReset: true)
OneSecStanfordStudy->>BulkHealthExporter: start export session
🚥 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 review |
✅ Action performedReview finished.
|
|
@LambdaDigamma & @lukaskollmer This should simplify our setup by a lot; would be great to get your input here 🚀 |
There was a problem hiding this comment.
Pull request overview
This PR establishes OneSecStanfordStudy as a standalone Swift package (iOS 15 package compatibility with active integration on iOS 17+), including a consolidated UI test app, unit tests, and a reusable-workflow-based CI setup.
Changes:
- Adds the
OneSecStanfordStudySwift package target, public integration entry points, and core implementation (survey sheet + WebView integration + HealthKit bulk export plumbing). - Adds unit tests plus a consolidated Xcode-based UI test app (scheme, test plan, resources).
- Adds repo scaffolding (README, licensing/REUSE metadata, SwiftLint config) and CI via SchmiedmayerLab reusable workflows.
Reviewed changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UITests/UITests.xcodeproj/xcshareddata/xcschemes/TestApp.xcscheme | Adds shared scheme for running the UI test app and UI tests. |
| Tests/UITests/UITests.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist | Xcode workspace check metadata for the UI tests project. |
| Tests/UITests/UITests.xcodeproj/project.xcworkspace/contents.xcworkspacedata | Workspace definition for the UI tests project. |
| Tests/UITests/UITests.xcodeproj/project.pbxproj | UI test app + UI test target project configuration. |
| Tests/UITests/TestAppUITests/TestAppUITests.swift | UI tests validating wrapper behavior + WebView alert/confirm hooks. |
| Tests/UITests/TestApp/WebViewTests/WebViewAlertAndConfirmTestButton.swift | UI surface to open the sheet with a local HTML page for alert/confirm testing. |
| Tests/UITests/TestApp/TestAppDelegate.swift | Initializes the package in an app-delegate context with HealthKit export config. |
| Tests/UITests/TestApp/TestApp.swift | SwiftUI app entrypoint applying the .oneSecStanfordStudy() modifier. |
| Tests/UITests/TestApp/TestApp.entitlements | Enables HealthKit entitlement for the UI test app. |
| Tests/UITests/TestApp/StudyButton.swift | UI entry to start the survey flow in the UI test app. |
| Tests/UITests/TestApp/Resources/alert-confirm-test.html | Local HTML resource used by UI tests to trigger alert() / confirm(). |
| Tests/UITests/TestApp/ContentView.swift | Root UI for iOS 17+ integration testing in the UI test app. |
| Tests/UITests/TestApp/Assets.xcassets/Contents.json | Asset catalog metadata for the UI test app. |
| Tests/UITests/TestApp/Assets.xcassets/AppIcon.appiconset/Contents.json | App icon asset metadata for the UI test app. |
| Tests/UITests/TestApp/Assets.xcassets/AccentColor.colorset/Contents.json | Accent color asset metadata for the UI test app. |
| Tests/UITests/TestApp.xctestplan | Xcode test plan (including coverage configuration) for the UI test setup. |
| Tests/OneSecStanfordStudyTests/OneSecStanfordStudyTests.swift | Unit tests for HealthExportConfiguration and AnyAsyncSequence behavior. |
| Sources/OneSecStanfordStudy/WebView.swift | SwiftUI WKWebView wrapper with navigation hooks and JS alert/confirm bridging. |
| Sources/OneSecStanfordStudy/StudySurveySheet.swift | Survey sheet UI, navigation handling, and export initiation trigger points. |
| Sources/OneSecStanfordStudy/OneSecStanfordStudyModule.swift | Public module interface/base class + HealthExportConfiguration type. |
| Sources/OneSecStanfordStudy/OneSecStanfordStudy.swift | Concrete module implementation integrating Spezi runtime and bulk export logic. |
| Sources/OneSecStanfordStudy/Integration.swift | Public initialization function and root SwiftUI modifier for apps. |
| Sources/OneSecStanfordStudy/HKSampleToFHIRProcessor.swift | Batch processor converting HealthKit samples to FHIR and writing compressed output. |
| Sources/OneSecStanfordStudy/AnyAsyncSequence.swift | Type erasure for AsyncSequence/Iterator (including pre-iOS18 “never throws” wrapper). |
| REUSE.toml | REUSE licensing annotations for generated/config assets and Xcode project files. |
| README.md | Installation/usage/testing documentation for the package. |
| Package.swift | SwiftPM package definition, platforms, targets, and Spezi dependency pinning. |
| LICENSES/MIT.txt | MIT license text under LICENSES directory. |
| LICENSE.md | Root MIT license file for the repository/package. |
| CONTRIBUTORS.md | Contributors list and attribution to upstream repositories. |
| .swiftlint.yml | SwiftLint configuration and exclusions. |
| .spi.yml | Swift Package Index documentation build configuration. |
| .gitignore | Git ignore rules for SwiftPM/Xcode artifacts. |
| .github/workflows/ci.yml | CI workflow using SchmiedmayerLab reusable Swift package workflows. |
Files not reviewed (1)
- Tests/UITests/UITests.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let session = try await healthExportSession() | ||
| let stream = try session.start(retryFailedBatches: true) |
There was a problem hiding this comment.
@lukaskollmer & @LambdaDigamma as far as I can see doesn't have to be written as probably managed outside of this package? If so, probably good to document and/or move this write into the package.
| if isCompleted { | ||
| healthExportConfig.didEndExport() | ||
| } |
There was a problem hiding this comment.
Same here as above in the other comment @lukaskollmer & @LambdaDigamma.
![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
🧹 Nitpick comments (6)
Sources/OneSecStanfordStudy/AnyAsyncSequence.swift (1)
86-89: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider
as!overas?for the element cast innext().The
as? Elementcast returnsnilon failure, which would silently terminate iteration (appearing as end-of-sequence). Given the initializer constraints guaranteeS.Element == Element/I.Element == Element, the cast should never fail. Usingas!would surface a programmer error immediately rather than silently truncating the sequence.Optional: fail fast on cast failure
`@inlinable` public mutating func next() async throws -> Element? { - try await base.next() as? Element + try await base.next() as! Element? }🤖 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 `@Sources/OneSecStanfordStudy/AnyAsyncSequence.swift` around lines 86 - 89, The `AnyAsyncSequence.next()` implementation is using a conditional cast that can silently return nil and end iteration early; update the cast in `next()` to a force cast since the generic constraints on `AnyAsyncSequence`/its initializer already guarantee `S.Element == Element` and `I.Element == Element`. Keep the change localized to `next()` so any unexpected mismatch fails fast instead of being treated as end-of-sequence.Tests/OneSecStanfordStudyTests/OneSecStanfordStudyTests.swift (1)
14-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for the throwing and iOS 18
next(isolation:)paths.The current tests only cover the
Failure == Never(non-throwing) path. The throwing path (Failure == any Error) and the iOS 18next(isolation:)method are untested. Adding a test that wraps a throwingAsyncSequenceand verifies error propagation would improve confidence in the type-erasure correctness.🧪 Suggested test for the throwing path
`@Test` func anyAsyncSequencePropagatesErrors() async throws { struct TestError: Error, Equatable {} let stream = AsyncThrowingStream<Int, Error> { continuation in continuation.yield(1) continuation.finish(throwing: TestError()) } let sequence = AnyAsyncSequence<Int, any Error>(stream) var values: [Int] = [] var caughtError: Error? do { for try await value in sequence { values.append(value) } } catch { caughtError = error } `#expect`(values == [1]) `#expect`(caughtError is TestError) }🤖 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 `@Tests/OneSecStanfordStudyTests/OneSecStanfordStudyTests.swift` around lines 14 - 52, Add tests that cover the missing AnyAsyncSequence paths: the throwing Failure == any Error case and the iOS 18 next(isolation:) behavior. Extend OneSecStanfordStudyTests with a test that wraps a throwing AsyncSequence using AnyAsyncSequence<Int, any Error>, verifies yielded values are preserved, and asserts the thrown error propagates correctly. Also add a test that exercises iteration through the newer isolation-aware next(isolation:) path so the type erasure is validated beyond the current unsafelyAssumingDoesntThrow initializer..github/workflows/ci.yml (1)
22-27: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider scoping down workflow-level write permissions and
secrets: inherit.Static analysis flags
issues: writeandpull-requests: writeas overly broad at the workflow level, andsecrets: inheritunconditionally passes all secrets to the reusable coverage workflow. If the reusable workflows don't require these permissions or all secrets, consider moving write permissions to only the jobs that need them and passing specific secrets explicitly (e.g.,secrets: CODECOV_TOKEN) instead of inheriting everything. If the reusableSchmiedmayerLab/.githubworkflows do require these, this is acceptable as-is.Also applies to: 86-86
🤖 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/ci.yml around lines 22 - 27, The workflow-level permissions and secret forwarding are broader than necessary; review the CI workflow’s permissions block and the reusable workflow calls to scope them down. Move `issues: write` and `pull-requests: write` to only the jobs that actually need them, and replace `secrets: inherit` with explicit secret passing (for example, only `CODECOV_TOKEN`) unless the `SchmiedmayerLab/.github` reusable workflows genuinely require full inheritance.Source: Linters/SAST tools
Package.swift (1)
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
traits: []is redundant andExistentialAnyis duplicated across targets.
traits: []is the default for.package(url:branch:traits:)and can be omitted. TheswiftSettingsblock is identical between the target and testTarget — consider whether a shared array would reduce duplication, though for two targets this is minor.📝 Proposed cleanup
.package(url: "https://github.com/SchmiedmayerLab/Spezi.git", branch: "oldiOSVersion")Also applies to: 45-48
🤖 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 `@Package.swift` around lines 35 - 38, Remove the redundant empty traits argument from the package dependency declaration and simplify the duplicated Swift settings setup. In Package.swift, keep the ExistentialAny/InternalImportsByDefault settings in a shared reusable array (or equivalent shared constant) and reference it from both the target and testTarget declarations so the duplicated swiftSettings blocks are centralized; use the package/dependency declaration symbols and the target definitions as the place to update.CONTRIBUTORS.md (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix heading style inconsistency flagged by markdownlint.
The title uses setext style (
====) while## Attributionsuses atx style. markdownlint (MD003) expects consistent setext headings throughout.📝 Proposed fix
-## Attributions +Attributions +-----------🤖 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 `@CONTRIBUTORS.md` at line 19, The heading style is inconsistent in the Attributions section, which triggers markdownlint MD003. Update the existing Attributions heading to match the document’s setext heading style used elsewhere, and keep the heading formatting consistent throughout CONTRIBUTORS.md.Source: Linters/SAST tools
Tests/UITests/UITests.xcodeproj/project.pbxproj (1)
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemplate leftover names:
remoteInfoandproductNamestill say "Example".The
PBXContainerItemProxyat line 20 hasremoteInfo = "Example"and the native targets at lines 141 and 162 haveproductName = "Example"/"ExampleUITests". SincePRODUCT_NAMEis set to$(TARGET_NAME)in build settings, the product name override is cosmetic, butremoteInfois used by Xcode for dependency resolution display and should match the actual target nameTestApp.♻️ Proposed cleanup
2F6D13AD28F5F386007C25D6 /* PBXContainerItemProxy */ = { isa = PBXContainerItemProxy; containerPortal = 2F6D138A28F5F384007C25D6 /* Project object */; proxyType = 1; remoteGlobalIDString = 2F6D139128F5F384007C25D6; - remoteInfo = Example; + remoteInfo = TestApp; };name = TestApp; packageProductDependencies = ( 809757DB2E61D2AD009B747D /* OneSecStanfordStudy */, ); - productName = Example; + productName = TestApp; productReference = 2F6D139228F5F384007C25D6 /* TestApp.app */;name = TestAppUITests; - productName = ExampleUITests; + productName = TestAppUITests; productReference = 2F6D13AC28F5F386007C25D6 /* TestAppUITests.xctest */;Also applies to: 141-141, 162-162
🤖 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 `@Tests/UITests/UITests.xcodeproj/project.pbxproj` around lines 15 - 21, The project file still contains template placeholder names, so update the PBXContainerItemProxy entry’s remoteInfo to match the real target name TestApp and clean up the native target productName overrides in the same project file so they are consistent with the actual UITests target naming. Use the PBXContainerItemProxy section plus the target entries for productName to locate the stale "Example" and "ExampleUITests" values and replace them with the correct names.
🤖 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 `@LICENSE.md`:
- Line 3: The copyright year in LICENSE.md is inconsistent with LICENSES/MIT.txt
and the repo’s SPDX headers. Update the LICENSE.md notice to match the same year
used elsewhere (or, if 2022 is intended, add a brief note clarifying that it
preserves the original upstream copyright) so the LICENSE.md header is
consistent with the project’s license files.
In `@Sources/OneSecStanfordStudy/OneSecStanfordStudy.swift`:
- Line 72: The OneSecStanfordStudy state load is silently falling back to
.available when localStorage.load(.oneSecStanfordStudyState) throws, which can
overwrite a completed or unavailable study state. Update the initialization path
in OneSecStanfordStudy to avoid defaulting to .available on load failure, and
instead preserve the existing .unavailable-safe behavior or surface/log the
error before deciding the state. Use the updateState call and the
localStorage.load(.oneSecStanfordStudyState) access as the key locations to
adjust.
- Around line 71-87: The resume path in configure() is dead because
didInitiateBulkExport is only read and never written. Update the bulk export
initiation flow in OneSecStanfordStudy/BulkHealthExporter-related code so that
when the export is first started (before or during
triggerHealthExport(forceSessionReset:)), localStorage.store(true, for:
.didInitiateBulkExport) is saved, and keep the existing configure() check that
reloads state and resumes via healthExportSession() and
triggerHealthExport(forceSessionReset: false).
- Around line 132-143: _guard against duplicate completion handling in
_trackCompletion(of:)_: the current withObservationTracking/onChange recursion
can re-enter after session.state leaves and returns to .completed, causing
healthExportConfig.didEndExport() to fire multiple times. Add a one-time
completion guard in OneSecStanfordStudy (for example, a private boolean or
similar state) and check it before calling didEndExport(), then set it when the
export completion is first observed so subsequent _trackCompletion recursions
become no-ops.
In `@Sources/OneSecStanfordStudy/StudySurveySheet.swift`:
- Around line 142-149: The `initiateHealthExport()` error path in
`StudySurveySheet` only logs failures and leaves the participant in `.active`
without any visible feedback. Update the `catch` block to surface a user-facing
failure state in the survey flow, such as presenting an alert or setting an
error state that the UI can react to, using the existing
`oneSecStanfordStudy`/`logger` flow to locate the handling point.
- Around line 154-164: The helper methods pageContainsField(named:) and
pageContainsElement(withId:) currently interpolate variableName and id directly
into JavaScript, which is unsafe and fragile. Update these checks to pass the
values as arguments instead of embedding them in the JS string, ideally by using
WKWebView.callAsyncJavaScript(_:arguments:) through WebViewProxy.wkWebView. If
access control blocks that path, adjust the wkWebView visibility so the safe
parameterized call can be used from the page-check helpers.
In `@Sources/OneSecStanfordStudy/WebView.swift`:
- Around line 47-58: The onAlert/onConfirm handlers in WebView are overwriting
the existing `@State` alert/confirmation while a previous withCheckedContinuation
is still pending, which can orphan the first continuation and hang the JS
bridge. Update the alert/confirmation handling in WebView to either queue
pending requests or add a runtime guard before assigning a new
alert/confirmation that resumes or rejects the existing continuation first; do
not rely on the assert alone since it is debug-only.
- Around line 79-88: The confirmation alert binding in WebView’s alert setup is
clearing the wrong state variable in its `set` closure. Update the closure to
clear `self.confirmation` (not `self.alert`) when `isPresented` becomes false,
so the confirmation flow is dismissed correctly and its continuation can be
resumed from the confirmation handling path.
In `@Tests/UITests/TestApp/ContentView.swift`:
- Around line 23-27: The Task launched from the “Trigger Health Export” Button
in ContentView currently lets triggerHealthExport(forceSessionReset:) throw
without handling, which can cause an unhandled task warning. Update that Task
body to use explicit do-catch error handling, and mirror the existing
TestAppDelegate.handleHealthExportDidStart pattern by catching failures and
routing them to a deliberate test-time failure path (for example, fatalError or
equivalent) so errors are surfaced consistently during testing.
In `@Tests/UITests/TestApp/Resources/alert-confirm-test.html`:
- Line 73: Fix the malformed table header markup in the alert-confirm test HTML
by replacing the invalid closing tag in the “Confirm” row so the `<th
scope="row">` element is properly closed. Update the `alert-confirm-test.html`
content around the table row used by the UI tests to use the correct `</th>`
closing tag, ensuring the table structure remains valid for `WKWebView` parsing
and element lookup.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 22-27: The workflow-level permissions and secret forwarding are
broader than necessary; review the CI workflow’s permissions block and the
reusable workflow calls to scope them down. Move `issues: write` and
`pull-requests: write` to only the jobs that actually need them, and replace
`secrets: inherit` with explicit secret passing (for example, only
`CODECOV_TOKEN`) unless the `SchmiedmayerLab/.github` reusable workflows
genuinely require full inheritance.
In `@CONTRIBUTORS.md`:
- Line 19: The heading style is inconsistent in the Attributions section, which
triggers markdownlint MD003. Update the existing Attributions heading to match
the document’s setext heading style used elsewhere, and keep the heading
formatting consistent throughout CONTRIBUTORS.md.
In `@Package.swift`:
- Around line 35-38: Remove the redundant empty traits argument from the package
dependency declaration and simplify the duplicated Swift settings setup. In
Package.swift, keep the ExistentialAny/InternalImportsByDefault settings in a
shared reusable array (or equivalent shared constant) and reference it from both
the target and testTarget declarations so the duplicated swiftSettings blocks
are centralized; use the package/dependency declaration symbols and the target
definitions as the place to update.
In `@Sources/OneSecStanfordStudy/AnyAsyncSequence.swift`:
- Around line 86-89: The `AnyAsyncSequence.next()` implementation is using a
conditional cast that can silently return nil and end iteration early; update
the cast in `next()` to a force cast since the generic constraints on
`AnyAsyncSequence`/its initializer already guarantee `S.Element == Element` and
`I.Element == Element`. Keep the change localized to `next()` so any unexpected
mismatch fails fast instead of being treated as end-of-sequence.
In `@Tests/OneSecStanfordStudyTests/OneSecStanfordStudyTests.swift`:
- Around line 14-52: Add tests that cover the missing AnyAsyncSequence paths:
the throwing Failure == any Error case and the iOS 18 next(isolation:) behavior.
Extend OneSecStanfordStudyTests with a test that wraps a throwing AsyncSequence
using AnyAsyncSequence<Int, any Error>, verifies yielded values are preserved,
and asserts the thrown error propagates correctly. Also add a test that
exercises iteration through the newer isolation-aware next(isolation:) path so
the type erasure is validated beyond the current unsafelyAssumingDoesntThrow
initializer.
In `@Tests/UITests/UITests.xcodeproj/project.pbxproj`:
- Around line 15-21: The project file still contains template placeholder names,
so update the PBXContainerItemProxy entry’s remoteInfo to match the real target
name TestApp and clean up the native target productName overrides in the same
project file so they are consistent with the actual UITests target naming. Use
the PBXContainerItemProxy section plus the target entries for productName to
locate the stale "Example" and "ExampleUITests" values and replace them with the
correct names.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 7065fbf6-3654-4e9e-ab36-12e389b07692
📒 Files selected for processing (34)
.github/workflows/ci.yml.gitignore.spi.yml.swiftlint.ymlCONTRIBUTORS.mdLICENSE.mdLICENSES/MIT.txtPackage.swiftREADME.mdREUSE.tomlSources/OneSecStanfordStudy/AnyAsyncSequence.swiftSources/OneSecStanfordStudy/HKSampleToFHIRProcessor.swiftSources/OneSecStanfordStudy/Integration.swiftSources/OneSecStanfordStudy/OneSecStanfordStudy.swiftSources/OneSecStanfordStudy/OneSecStanfordStudyModule.swiftSources/OneSecStanfordStudy/StudySurveySheet.swiftSources/OneSecStanfordStudy/WebView.swiftTests/OneSecStanfordStudyTests/OneSecStanfordStudyTests.swiftTests/UITests/TestApp.xctestplanTests/UITests/TestApp/Assets.xcassets/AccentColor.colorset/Contents.jsonTests/UITests/TestApp/Assets.xcassets/AppIcon.appiconset/Contents.jsonTests/UITests/TestApp/Assets.xcassets/Contents.jsonTests/UITests/TestApp/ContentView.swiftTests/UITests/TestApp/Resources/alert-confirm-test.htmlTests/UITests/TestApp/StudyButton.swiftTests/UITests/TestApp/TestApp.entitlementsTests/UITests/TestApp/TestApp.swiftTests/UITests/TestApp/TestAppDelegate.swiftTests/UITests/TestApp/WebViewTests/WebViewAlertAndConfirmTestButton.swiftTests/UITests/TestAppUITests/TestAppUITests.swiftTests/UITests/UITests.xcodeproj/project.pbxprojTests/UITests/UITests.xcodeproj/project.xcworkspace/contents.xcworkspacedataTests/UITests/UITests.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plistTests/UITests/UITests.xcodeproj/xcshareddata/xcschemes/TestApp.xcscheme
| } | ||
|
|
||
| func configure() { | ||
| updateState((try? localStorage.load(.oneSecStanfordStudyState)) ?? .available) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Defaulting to .available on storage error could reset a completed study state.
If localStorage.load(.oneSecStanfordStudyState) throws, the state silently resets to .available, which could re-enroll a participant who already completed or was marked unavailable. Consider logging the error and preserving the current .unavailable default, or surfacing the failure.
💡 Suggested improvement
- updateState((try? localStorage.load(.oneSecStanfordStudyState)) ?? .available)
+ do {
+ let restored = try localStorage.load(.oneSecStanfordStudyState)
+ updateState(restored)
+ } catch {
+ logger.error("Failed to load study state: \(error). Defaulting to .unavailable.")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updateState((try? localStorage.load(.oneSecStanfordStudyState)) ?? .available) | |
| do { | |
| let restored = try localStorage.load(.oneSecStanfordStudyState) | |
| updateState(restored) | |
| } catch { | |
| logger.error("Failed to load study state: \(error). Defaulting to .unavailable.") | |
| } |
🤖 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 `@Sources/OneSecStanfordStudy/OneSecStanfordStudy.swift` at line 72, The
OneSecStanfordStudy state load is silently falling back to .available when
localStorage.load(.oneSecStanfordStudyState) throws, which can overwrite a
completed or unavailable study state. Update the initialization path in
OneSecStanfordStudy to avoid defaulting to .available on load failure, and
instead preserve the existing .unavailable-safe behavior or surface/log the
error before deciding the state. Use the updateState call and the
localStorage.load(.oneSecStanfordStudyState) access as the key locations to
adjust.
There was a problem hiding this comment.
@lukaskollmer & @LambdaDigamma Probably intentional as you want this to be explicit written and set to .available?
There was a problem hiding this comment.
@PSchmiedmayer That's a reasonable possibility — if .available is meant to be the explicit, intentional default whenever no prior state can be loaded (e.g., first launch, or storage cleared), then this behavior would be by design rather than a bug. The main risk I flagged is specifically the case where a decode/load failure (as opposed to "no value has ever been stored") is treated the same as "no prior state," which could mask a genuine storage/corruption issue and silently reset an already-.completed or .unavailable participant back to .available.
I'll wait for confirmation from @lukaskollmer and @LambdaDigamma on whether that distinction matters here, or whether treating all load failures as .available is indeed the intended behavior for this study.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
31-31: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider pinning the reusable workflow to a commit SHA.
Pinning to
@v0.2is better than tracking@main, but tags are mutable and can be moved. For supply-chain hardening, pin to the immutable commit SHA that thev0.2tag currently points to. This is a common convention for third-party GitHub Actions and reusable workflows.🤖 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/ci.yml at line 31, The reusable workflow reference in the CI workflow is still using a mutable tag, so update the workflow call in the `uses` entry to point at the immutable commit SHA for the current `v0.2` revision instead of the tag. Keep the existing reusable workflow target (`SchmiedmayerLab/.github/.github/workflows/swift-package-ci.yml`) and replace the version selector with the pinned SHA so the workflow stays reproducible and supply-chain hardened.
32-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid unconditionally inheriting all secrets.
secrets: inheritpasses every repository secret to the called workflow. If the reusable workflow is ever compromised or develops a vulnerability, all secrets are exposed. Prefer passing only the specific secrets the called workflow needs.If the reusable workflow doesn't require any secrets, remove the
secrets: inheritline entirely. Otherwise, replace it with explicit secret mappings (e.g.,secrets: { SOME_SECRET: ${{ secrets.SOME_SECRET }} }).zizmor also flagged this as a
secrets-inheritwarning.🔒 Suggested change (if no secrets are needed)
uses: SchmiedmayerLab/.github/.github/workflows/swift-package-ci.yml@v0.2 - secrets: inherit🤖 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/ci.yml at line 32, The reusable workflow call is inheriting every repository secret via secrets: inherit, which is too broad. In the workflow dispatch section in ci.yml, update the call site to either remove the secrets entry entirely if the called workflow needs no secrets, or replace it with explicit secret mappings for only the required names. Use the workflow call configuration around the reusable workflow invocation to keep the secret scope minimal.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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 31: The reusable workflow reference in the CI workflow is still using a
mutable tag, so update the workflow call in the `uses` entry to point at the
immutable commit SHA for the current `v0.2` revision instead of the tag. Keep
the existing reusable workflow target
(`SchmiedmayerLab/.github/.github/workflows/swift-package-ci.yml`) and replace
the version selector with the pinned SHA so the workflow stays reproducible and
supply-chain hardened.
- Line 32: The reusable workflow call is inheriting every repository secret via
secrets: inherit, which is too broad. In the workflow dispatch section in
ci.yml, update the call site to either remove the secrets entry entirely if the
called workflow needs no secrets, or replace it with explicit secret mappings
for only the required names. Use the workflow call configuration around the
reusable workflow invocation to keep the secret scope minimal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d81196e5-ea7a-42f0-8530-69350a147c9a
📒 Files selected for processing (13)
.github/workflows/ci.ymlCONTRIBUTORS.mdLICENSE.mdPackage.swiftREADME.mdSources/OneSecStanfordStudy/OneSecStanfordStudy.swiftSources/OneSecStanfordStudy/StudySurveySheet.swiftSources/OneSecStanfordStudy/WebView.swiftTests/OneSecStanfordStudyTests/OneSecStanfordStudyTests.swiftTests/UITests/TestApp/ContentView.swiftTests/UITests/TestApp/Resources/alert-confirm-test.htmlTests/UITests/TestAppUITests/TestAppUITests.swiftTests/UITests/UITests.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (1)
- Tests/UITests/TestAppUITests/TestAppUITests.swift
✅ Files skipped from review due to trivial changes (4)
- CONTRIBUTORS.md
- LICENSE.md
- README.md
- Tests/UITests/UITests.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (5)
- Package.swift
- Tests/UITests/TestApp/ContentView.swift
- Sources/OneSecStanfordStudy/WebView.swift
- Sources/OneSecStanfordStudy/OneSecStanfordStudy.swift
- Sources/OneSecStanfordStudy/StudySurveySheet.swift
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Set up OneSecStanfordStudy package
♻️ Current situation & Problem
This PR creates the standalone
OneSecStanfordStudySwift package from the consolidated OneSec study integration setup prepared during the Spezi monorepo work.The previous two-package setup used separate interface and implementation packages to work around deployment target constraints. This package depends on the Spezi monorepo setup directly and uses the Spezi feature branch for now.
⚙️ Release Notes
OneSecStanfordStudySwift package with iOS 15 package compatibility and active integration on iOS 17+.SchmiedmayerLab/SpezibrancholdiOSVersionuntil the monorepo changes are merged and tagged.📚 Documentation
The README documents installation, the temporary Spezi branch dependency, the later
0.xtagged release dependency, basic usage, testing, contributing, license, and contributor provenance.✅ Testing
Local preflight before opening the PR:
swiftlint lint --strict --quiet --no-cachereuse lintswift package dump-packageCI is expected to run the SchmiedmayerLab reusable Swift package setup, test, static-analysis, and coverage workflows.
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: