Skip to content

Conversation

@clodanSPT
Copy link
Contributor

No description provided.

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Aug 19, 2025

PR Code Suggestions ✨

Latest suggestions up to 212d482

CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure UTC consistency

Returning .DateTime yields a local time, which can cause subtle time zone bugs
now that UTC conversion is introduced alongside. Mark the non-UTC method as
clearly local or convert it to UTC for consistency. Prefer returning UtcDateTime
to avoid ambiguity across different environments.

Libraries/SPTarkov.Server.Core/Utils/TimeUtil.cs [150-153]

 public DateTime GetDateTimeFromTimeStamp(long timeStamp)
 {
-    return DateTimeOffset.FromUnixTimeSeconds(timeStamp).DateTime;
+    return DateTimeOffset.FromUnixTimeSeconds(timeStamp).UtcDateTime;
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that GetDateTimeFromTimeStamp returns local time, which is inconsistent with the newly added UTC-based methods and could lead to time zone bugs.

Medium
General
Preserve time-of-day semantics

The modulo applies only to seconds within a day, dropping the date component and
potentially producing a time that doesn’t map to an actual day. Explicitly
construct the time-of-day and avoid implying a full DateTime on 1970-01-01.
Return a DateTime with a neutral date (e.g., today in UTC) plus the computed
time-of-day to prevent downstream date misuse.

Libraries/SPTarkov.Server.Core/Helpers/WeatherHelper.cs [37-39]

-var tarkovTime = timeUtil.GetUtcDateTimeFromTimeStamp(
-    (long)(russiaOffsetSeconds + currentTimestampSeconds * _weatherConfig.Acceleration) % twentyFourHoursSeconds
-);
+var secondsIntoDay = (russiaOffsetSeconds + (long)(currentTimestampSeconds * _weatherConfig.Acceleration)) % twentyFourHoursSeconds;
+if (secondsIntoDay < 0) secondsIntoDay += twentyFourHoursSeconds;
+var baseDate = DateTime.UtcNow.Date;
+var tarkovTime = baseDate.AddSeconds(secondsIntoDay);
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly notes that the calculation results in a DateTime on the Unix epoch date, which can be misleading. Using the current date makes the returned value more robust for downstream consumers.

Low
Incremental [*]
Guard invalid timestamp ranges

Validate the input range and throw a clear exception when the provided seconds
fall outside valid Unix time bounds to prevent ArgumentOutOfRangeException
bubbling from the BCL in unpredictable places

Libraries/SPTarkov.Server.Core/Utils/TimeUtil.cs [155-163]

-/// <summary>
-///     Takes a unix timestamp and converts to its UTC date
-/// </summary>
-/// <param name="timeStamp"></param>
-/// <returns></returns>
 public DateTime GetUtcDateTimeFromTimeStamp(long timeStamp)
 {
+    // Unix epoch seconds valid range for DateTimeOffset
+    const long min = -62135596800; // DateTimeOffset.MinValue.UtcDateTime
+    const long max = 253402300799; // DateTimeOffset.MaxValue.UtcDateTime
+    if (timeStamp < min || timeStamp > max)
+    {
+        throw new ArgumentOutOfRangeException(nameof(timeStamp), $"Timestamp '{timeStamp}' is outside the valid Unix time range.");
+    }
     return DateTimeOffset.FromUnixTimeSeconds(timeStamp).UtcDateTime;
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion improves the robustness of a public utility method by adding input validation, which is good practice, but it is not critical for the current usage within the PR.

Low
  • Update

Previous suggestions

✅ Suggestions up to commit 011efb4
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix seconds-to-milliseconds conversion
Suggestion Impact:The method GetSecondsAsMilliseconds was removed entirely, eliminating the incorrect conversion identified in the suggestion.

code diff:

-    public int GetSecondsAsMilliseconds(int seconds)
-    {
-        return seconds * 60 * 1000;
     }

The conversion from seconds to milliseconds is incorrect (uses 60 instead of
1000). This will produce values 60x too small, breaking timeouts and timers. Fix
the multiplier to 1000 and guard against integer overflow.

Libraries/SPTarkov.Server.Core/Utils/TimeUtil.cs [160-163]

 public int GetSecondsAsMilliseconds(int seconds)
 {
-    return seconds * 60 * 1000;
+    // Prevent overflow and ensure correct conversion
+    checked
+    {
+        return seconds * 1000;
+    }
 }
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug in the GetSecondsAsMilliseconds method where the conversion logic is wrong, which would lead to incorrect time calculations.

High
Normalise modulo to non-negative

Ensure the modulo result is non-negative before converting to a DateTime, as C#
modulo with negatives can yield negative values causing out-of-range timestamps.
Normalise the seconds into the [0, 86400) range.

Libraries/SPTarkov.Server.Core/Helpers/WeatherHelper.cs [37-39]

-var tarkovTime = timeUtil.GetUtcDateTimeFromTimeStamp(
-    (long)(russiaOffsetSeconds + currentTimestampSeconds * _weatherConfig.Acceleration) % twentyFourHoursSeconds
-);
+var accelerated = russiaOffsetSeconds + currentTimestampSeconds * _weatherConfig.Acceleration;
+var mod = accelerated % twentyFourHoursSeconds;
+if (mod < 0) mod += twentyFourHoursSeconds;
+var tarkovTime = timeUtil.GetUtcDateTimeFromTimeStamp((long)mod);
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the modulo operator in C# can return negative values, which would cause an exception, and provides a robust fix to prevent this potential crash.

Medium

@chompDev chompDev merged commit 738c4c4 into develop Aug 19, 2025
3 of 4 checks passed
@chompDev chompDev deleted the raidtimer-fix branch August 19, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants