-
Notifications
You must be signed in to change notification settings - Fork 3
fix: fixes survey placement and other cosmetic issues #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaced days-between calculation to clamp at zero. SurveyManager: added guard for unknown action classes, clamped display-percentage to [0,100] with random draw, added logs/early exits for display restrictions, switched recontact check to seconds→days, and use survey.delay/survey.id when delaying. Added Placement enum and ProjectOverwrites struct; Survey gains projectOverwrites. WebView/FormbricksViewModel now prefers survey-level overwrites for placement, darkOverlay, languages, and styling. PresentSurveyManager.present gained an optional completion. UpdateQueue: main-thread timer cleanup, snapshotting of userId/attributes, and new cleanup(). Tests and mock JSON updated. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Sources/FormbricksSDK/Extension/Calendar+DaysBetween.swift(1 hunks)Sources/FormbricksSDK/Manager/SurveyManager.swift(3 hunks)Sources/FormbricksSDK/Model/Environment/Survey.swift(2 hunks)Sources/FormbricksSDK/WebView/FormbricksViewModel.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/FormbricksSDK/Manager/SurveyManager.swift (1)
Sources/FormbricksSDK/Logger/Logger.swift (2)
error(37-39)info(29-31)
Sources/FormbricksSDK/WebView/FormbricksViewModel.swift (1)
Sources/FormbricksSDK/Model/Environment/EnvironmentResponse.swift (1)
getSurveyJson(15-22)
🔇 Additional comments (5)
Sources/FormbricksSDK/WebView/FormbricksViewModel.swift (5)
94-96: LGTM: localmatchedSurveyandprojectimprove readability.
No issues spotted with the optional chaining and early binding.
102-103: Confirm default branding behavior.
Settingdata["isBrandingEnabled"] = project.inAppSurveyBranding ?? truemeans branding is enabled when the project flag is nil. Is that the intended product default? If not, consider defaulting tofalse.
110-110: LGTM: darkOverlay override with project fallback.
The nil-coalescing to the project value is correct and straightforward.
120-121: Confirm backend sets allowStyleOverwrite in project styling JSON.
FormbricksViewModel correctly usesproject.styling?.allowStyleOverwrite ?? falseto gate custom survey styling; ensure the API includes and enables this flag when intended.
104-108: Incorrect nil-coalescing of optional String
project.placementis declared asString?, somatchedSurvey?.projectOverwrites?.placement?.rawValue ?? project.placementwon’t compile. You must either provide a non-optional fallback or unwrap the optional twice, for example:
let placementValue = matchedSurvey?.projectOverwrites?.placement?.rawValue ?? project.placement ?? "defaultPlacement" data["placement"] = placementValueLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Sources/FormbricksSDK/Manager/PresentSurveyManager.swift(2 hunks)Sources/FormbricksSDK/Manager/SurveyManager.swift(4 hunks)Sources/FormbricksSDK/WebView/FormbricksViewModel.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/FormbricksSDK/Manager/SurveyManager.swift (2)
Sources/FormbricksSDK/Logger/Logger.swift (2)
error(37-39)info(29-31)Sources/FormbricksSDK/Manager/PresentSurveyManager.swift (1)
present(16-35)
Sources/FormbricksSDK/WebView/FormbricksViewModel.swift (1)
Sources/FormbricksSDK/Model/Environment/EnvironmentResponse.swift (1)
getSurveyJson(15-22)
🔇 Additional comments (2)
Sources/FormbricksSDK/WebView/FormbricksViewModel.swift (1)
112-124: Multi-language handling looks correct now.Wrapping the coalesced count before comparison fixes the precedence issue and keeps the override logic clear. The rest of the override selection reads well.
Sources/FormbricksSDK/Manager/SurveyManager.swift (1)
298-301: Clamp negative elapsed time before converting to days.If
lastDisplayedAtends up in the future (e.g., clock skew),timeIntervalSincebecomes negative, sodaysBetweenis negative and the survey stays filtered out indefinitely. Clamp the elapsed seconds (or days) to zero before comparing so recontact semantics stay non-negative.- let secondsElapsed = Date().timeIntervalSince(lastDisplayedAt) - let daysBetween = Int(secondsElapsed / 86_400) + let secondsElapsed = max(0, Date().timeIntervalSince(lastDisplayedAt)) + let daysBetween = Int(secondsElapsed / 86_400) return daysBetween >= recontactDays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Tests/FormbricksSDKTests/FormbricksSDKTests.swift(2 hunks)Tests/FormbricksSDKTests/Mock/Environment.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/FormbricksSDKTests/FormbricksSDKTests.swift (3)
Sources/FormbricksSDK/Helpers/ConfigBuilder.swift (3)
setLogLevel(56-59)service(61-64)build(67-69)Sources/FormbricksSDK/Formbricks.swift (1)
setup(41-95)Sources/FormbricksSDK/Manager/SurveyManager.swift (1)
refreshEnvironmentIfNeeded(141-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-analyze
🔇 Additional comments (1)
Tests/FormbricksSDKTests/FormbricksSDKTests.swift (1)
245-247: Initializer update acknowledgedExplicit
projectOverwrites: nilkeeps the constructor in sync with the new model and documents the fallback path. Consider adding a test that removes survey overwrites and asserts fallback to project-level placement/darkOverlay.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Tests/FormbricksSDKTests/CalendarDaysBetweenTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/FormbricksSDKTests/CalendarDaysBetweenTests.swift (1)
Sources/FormbricksSDK/Extension/Calendar+DaysBetween.swift (1)
numberOfDaysBetween(5-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-analyze


Fixes several issues with the sdk: #34 #33 #32