Open
Conversation
phnagy
force-pushed
the
localized-metadata
branch
from
e8fd3c5 to
b81e3d3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
=======================================
Coverage 75.00% 75.00%
=======================================
Files 25 25
Lines 356 356
=======================================
Hits 267 267
Misses 89 89
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors Package.swift into function-driven manifest builders, converts several StudyDefinition text fields to localized dictionaries, gates Darwin-only components with conditional compilation, updates StudyBundle path handling and validations, adjusts tests/UI to use localized lookups and locale codes, and adds a Linux package test job to GitHub Actions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
phnagy
force-pushed
the
localized-metadata
branch
from
2c8f4ae to
6af51ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
Tests/SpeziStudyTests/Resources/questionnaires/Invalid6+en-UK.json (1)
4-4: Consider aligning fixture filename with payload locale.The file content is now
en-GB, but the fixture name still ends with+en-UK.json. For localization tests, this can be misleading. Consider renaming to+en-GB.json(or add a short test comment if the mismatch is intentional).Also applies to: 14-14
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SpeziStudyTests/Resources/questionnaires/Invalid6`+en-UK.json at line 4, The fixture file Invalid6+en-UK.json has a language field set to "en-GB" causing a mismatch; either rename the fixture to end with +en-GB.json to match the payload or change the "language" value to "en-UK" and add a brief inline comment explaining the intentional mismatch; update any test references that import or load Invalid6+en-UK.json to use the new filename if you rename the file.Package.swift (1)
77-77: Redundant empty array concatenation.The
[] + swiftLintPlugin()pattern is redundant sinceswiftLintPlugin()already returns[Target.PluginUsage]. This pattern appears on lines 77, 94, and 113.♻️ Suggested simplification
- plugins: [] + swiftLintPlugin() + plugins: swiftLintPlugin()Apply similarly to lines 94 and 113.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Package.swift` at line 77, The plugin lists currently use a redundant concatenation pattern "[] + swiftLintPlugin()"; remove the empty array concatenation and replace each occurrence with just swiftLintPlugin() (update the plugin arguments where this appears—the call sites involving swiftLintPlugin() in the Package.swift plugin arrays) so the plugin arrays directly use the returned [Target.PluginUsage] instead of prepending an empty array.Sources/SpeziStudyDefinition/StudyDefinition.swift (1)
171-179: Consider fallback fortimedWalkingTestdisplay title on non-Darwin platforms.The
timedWalkingTestcase returnsnilon non-Darwin platforms becausedisplayTitleusesLocalizedStringResource. While this is technically correct, it means timed walking tests will have no display title on Linux. If Linux support is intended for more than CI/testing, consider providing a plain string fallback.♻️ Optional: Add non-Darwin fallback
case .timedWalkingTest(let component): `#if` canImport(Darwin) String(localized: component.test.displayTitle) `#else` - nil + // Provide a basic fallback for non-Darwin platforms + switch component.test.kind { + case .walking: "Walk Test" + case .running: "Run Test" + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/SpeziStudyDefinition/StudyDefinition.swift` around lines 171 - 179, The timedWalkingTest branch currently yields nil on non-Darwin platforms because it depends on LocalizedStringResource; update the non-Darwin fallback to return a plain String instead of nil (for example derive a simple title from the model such as component.test.title or String(describing: component.test)) so that timedWalkingTest has a usable display title on Linux; modify the conditional around timedWalkingTest (referencing timedWalkingTest and component.test.displayTitle) to return that fallback string when canImport(Darwin) is false.Tests/SpeziStudyTests/StudyBundleTests.swift (1)
51-53: Add cleanup for temporary copied bundle path.Line 51 creates a temp bundle URL for copy/equality checks, but it is not cleaned up. Adding a
deferremoval avoids temp-directory buildup across repeated test runs.♻️ Suggested small cleanup
let bundle3Url = URL.temporaryDirectory.appending(component: "\(UUID().uuidString).\(StudyBundle.fileExtension)", directoryHint: .isDirectory) +defer { + try? FileManager.default.removeItem(at: bundle3Url) +} try bundle1.copy(to: bundle3Url) let bundle3 = try StudyBundle(bundleUrl: bundle3Url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SpeziStudyTests/StudyBundleTests.swift` around lines 51 - 53, Add cleanup for the temporary copied bundle by removing the file at bundle3Url after the test completes: after creating bundle3Url (the temporary URL used with bundle1.copy and the StudyBundle initializer), add a defer that attempts to remove the item at bundle3Url (using FileManager.default.removeItem or try? to swallow errors) so the temp file does not accumulate across test runs.Tests/SpeziStudyTests/StudyBundleValidationTests.swift (1)
49-49: Consider extracting temp study-bundle URL creation into a helper.Line 49, Line 319, and Line 629 repeat the same temp URL construction pattern; a helper would reduce duplication and drift across tests.
Also applies to: 319-319, 629-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SpeziStudyTests/StudyBundleValidationTests.swift` at line 49, Extract the repeated temp URL construction into a single helper function (e.g., makeTempStudyBundleURL() or temporaryStudyBundleURL()) that returns URL(temporaryDirectory).appending(component: "\(UUID().uuidString).\(StudyBundle.fileExtension)", directoryHint: .isDirectory); replace the three inline constructions (currently assigned to tmpUrl at the three test sites) with calls to this helper and update any test teardown or uses accordingly to keep behavior identical.Tests/UITests/TestAppUITests/TestAppUITests.swift (1)
42-42: Scope swipe gestures to the scroll container for stability.Line 42 and Line 56 use app-wide swipes; targeting the specific
Form/scrollable container is usually more deterministic in UI tests.Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/UITests/TestAppUITests/TestAppUITests.swift` at line 42, Replace the app-wide swipe gestures with swipes scoped to the specific scrollable container: locate the Form/scrollView used in TestAppUITests (e.g., reference the Form element or use app.scrollViews.firstMatch) and change the two app.swipeUp() calls to formElement.swipeUp() or app.scrollViews.firstMatch.swipeUp(); if the Form has an accessibility identifier, use app.forms["<identifier>"] or app.otherElements["<identifier>"] to target it for deterministic swiping in TestAppUITests.Tests/SpeziStudyTests/StudyManagerTests.swift (1)
11-11: Consider narrowing the Darwin gate scope.Line 11 and Line 364 gate the full suite; this also excludes tests that look platform-agnostic (for example locale matching/update logic). Splitting Darwin-only tests into a separate gated suite would preserve Linux coverage.
Also applies to: 364-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SpeziStudyTests/StudyManagerTests.swift` at line 11, The top-level Darwin gate currently wrapping the entire StudyManagerTests file is excluding platform-agnostic tests; remove the file-wide `#if` canImport(Darwin) and instead restrict Darwin-only checks to the specific tests that require Apple platforms (e.g., move the Darwin-only assertions into a separate file or wrap only the Apple-specific test methods with `#if` canImport(Darwin)). Update StudyManagerTests by unwrapping platform-agnostic tests (like the locale matching/update tests) so they run on Linux, and place the Darwin-only cases inside a guarded extension or new test file that still uses `#if` canImport(Darwin).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-and-test.yml:
- Around line 74-80: The uploadcoveragereport job's needs array is missing the
Linux test job, so add package_tests_linux to the needs list for the
uploadcoveragereport job to ensure Linux LCOV artifacts (coveragereports_lcov:
SpeziStudy-Package-Linux-*.lcov) are produced before upload; update the
uploadcoveragereport job declaration (the needs field under the
uploadcoveragereport job) to include package_tests_linux alongside package_tests
and ui_tests.
In `@Tests/UITests/TestApp/HomeTab.swift`:
- Line 68: The UI string lookup currently forces .enUS in the AsyncButton title
(see AsyncButton(...) and mockStudyV1.studyDefinition.metadata.title[.enUS])
which can ignore the active locale or be empty; update the lookup to resolve
using the current/preferred locale(s) first and fall back to a safe default
(e.g., Locale.current or a list of preferred locales then any available language
or a hardcoded fallback) before formatting the button label; apply the same
change where .enUS is used at the other occurrence so the title resolves
robustly across locales.
---
Nitpick comments:
In `@Package.swift`:
- Line 77: The plugin lists currently use a redundant concatenation pattern "[]
+ swiftLintPlugin()"; remove the empty array concatenation and replace each
occurrence with just swiftLintPlugin() (update the plugin arguments where this
appears—the call sites involving swiftLintPlugin() in the Package.swift plugin
arrays) so the plugin arrays directly use the returned [Target.PluginUsage]
instead of prepending an empty array.
In `@Sources/SpeziStudyDefinition/StudyDefinition.swift`:
- Around line 171-179: The timedWalkingTest branch currently yields nil on
non-Darwin platforms because it depends on LocalizedStringResource; update the
non-Darwin fallback to return a plain String instead of nil (for example derive
a simple title from the model such as component.test.title or String(describing:
component.test)) so that timedWalkingTest has a usable display title on Linux;
modify the conditional around timedWalkingTest (referencing timedWalkingTest and
component.test.displayTitle) to return that fallback string when
canImport(Darwin) is false.
In `@Tests/SpeziStudyTests/Resources/questionnaires/Invalid6`+en-UK.json:
- Line 4: The fixture file Invalid6+en-UK.json has a language field set to
"en-GB" causing a mismatch; either rename the fixture to end with +en-GB.json to
match the payload or change the "language" value to "en-UK" and add a brief
inline comment explaining the intentional mismatch; update any test references
that import or load Invalid6+en-UK.json to use the new filename if you rename
the file.
In `@Tests/SpeziStudyTests/StudyBundleTests.swift`:
- Around line 51-53: Add cleanup for the temporary copied bundle by removing the
file at bundle3Url after the test completes: after creating bundle3Url (the
temporary URL used with bundle1.copy and the StudyBundle initializer), add a
defer that attempts to remove the item at bundle3Url (using
FileManager.default.removeItem or try? to swallow errors) so the temp file does
not accumulate across test runs.
In `@Tests/SpeziStudyTests/StudyBundleValidationTests.swift`:
- Line 49: Extract the repeated temp URL construction into a single helper
function (e.g., makeTempStudyBundleURL() or temporaryStudyBundleURL()) that
returns URL(temporaryDirectory).appending(component:
"\(UUID().uuidString).\(StudyBundle.fileExtension)", directoryHint:
.isDirectory); replace the three inline constructions (currently assigned to
tmpUrl at the three test sites) with calls to this helper and update any test
teardown or uses accordingly to keep behavior identical.
In `@Tests/SpeziStudyTests/StudyManagerTests.swift`:
- Line 11: The top-level Darwin gate currently wrapping the entire
StudyManagerTests file is excluding platform-agnostic tests; remove the
file-wide `#if` canImport(Darwin) and instead restrict Darwin-only checks to the
specific tests that require Apple platforms (e.g., move the Darwin-only
assertions into a separate file or wrap only the Apple-specific test methods
with `#if` canImport(Darwin)). Update StudyManagerTests by unwrapping
platform-agnostic tests (like the locale matching/update tests) so they run on
Linux, and place the Darwin-only cases inside a guarded extension or new test
file that still uses `#if` canImport(Darwin).
In `@Tests/UITests/TestAppUITests/TestAppUITests.swift`:
- Line 42: Replace the app-wide swipe gestures with swipes scoped to the
specific scrollable container: locate the Form/scrollView used in TestAppUITests
(e.g., reference the Form element or use app.scrollViews.firstMatch) and change
the two app.swipeUp() calls to formElement.swipeUp() or
app.scrollViews.firstMatch.swipeUp(); if the Form has an accessibility
identifier, use app.forms["<identifier>"] or app.otherElements["<identifier>"]
to target it for deterministic swiping in TestAppUITests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/build-and-test.ymlPackage.swiftSources/SpeziStudy/Study Manager/StudyManager.swiftSources/SpeziStudyDefinition/Study Definition Components/CustomActiveTaskComponent.swiftSources/SpeziStudyDefinition/Study Definition Components/TimedWalkingTestComponent.swiftSources/SpeziStudyDefinition/StudyBundle/StudyBundle+Write.swiftSources/SpeziStudyDefinition/StudyBundle/StudyBundle.swiftSources/SpeziStudyDefinition/StudyBundle/Validation/StudyDefinition+Validation.swiftSources/SpeziStudyDefinition/StudyDefinition+Components.swiftSources/SpeziStudyDefinition/StudyDefinition+Metadata.swiftSources/SpeziStudyDefinition/StudyDefinition.swiftTests/SpeziStudyTests/Resources/questionnaires/Invalid5+en-UK.jsonTests/SpeziStudyTests/Resources/questionnaires/Invalid6+en-UK.jsonTests/SpeziStudyTests/Resources/questionnaires/TestSurvey+en-UK.jsonTests/SpeziStudyTests/StudyBundleTests.swiftTests/SpeziStudyTests/StudyBundleValidationTests.swiftTests/SpeziStudyTests/StudyManagerTests.swiftTests/UITests/TestApp/HomeTab.swiftTests/UITests/TestApp/MockStudy.swiftTests/UITests/TestAppUITests/TestAppUITests.swift
lukaskollmer
approved these changes
Mar 3, 2026
Member
lukaskollmer
left a comment
lukaskollmer
left a comment
There was a problem hiding this comment.
very nice thank you @phnagy!
might wanna mention the addition of linux support in the PR title, so that it shows up in the changelog
Comment on lines
+48
to
+51
| #if canImport(Darwin) | ||
| /// A component that prompts the participant to perform a custom Active Task. | ||
| case customActiveTask(CustomActiveTaskComponent) | ||
| #endif |
Member
There was a problem hiding this comment.
why is this Darwin-only? wouldn't that break all studies that use active tasks, when running on linux?
Member
Author
There was a problem hiding this comment.
The issue is that CustomActiveTaskComponent essentially is a composition of two LocalizedStringResources
Comment on lines
+171
to
+175
| #if canImport(Darwin) | ||
| String(localized: component.test.displayTitle) | ||
| #else | ||
| nil | ||
| #endif |
Member
There was a problem hiding this comment.
technically localized strings should actually be available on Linux as well, but i agree that it's ok to not support this for the time being. maybe add a note somewhere so that we can look into adding it at some point in the future
Member
Author
There was a problem hiding this comment.
The issue is that LocalizedStringResource doesn't exist on Linux
| "title": "test survey with non-matching question input bounds", | ||
| "resourceType": "Questionnaire", | ||
| "language": "en-UK", | ||
| "language": "en-GB", |
Member
There was a problem hiding this comment.
what's your motivation for changing these?
Member
Author
There was a problem hiding this comment.
On Apple platforms, Foundation translates the "UK" to "GB". On Linux, this doesn't happen, as UK is not a valid ISO 3166-1 region code. I am not sure if that needs to be considered somewhere else
Member
Author
|
@lukaskollmer we should probably also update the |
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
2 participants
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.
Linux Support & Localized Metadata
♻️ Current situation & Problem
⚙️ Release Notes
📚 Documentation
-/-
✅ Testing
-/-
🏗️ ToDo's
en-UKtoen-GBmapping📝 Code of Conduct & Contributing Guidelines
Summary by CodeRabbit
New Features
Improvements
Tests
Chores