Fix/set month overflow - #287
Vishalsahani156 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
@Vishalsahani156 is attempting to deploy a commit to the vernu's projects Team on Vercel. A member of the Team first needs to authorize it. |
Sorry, something went wrong.
WalkthroughThe SMS worker now uses WorkManager retry attempts, SMS-specific unique work names, and malformed-JSON retry handling. Billing usage calculations now use a fixed 30-day window, with regression tests for month-end and ordinary dates. ChangesSMS status worker
Billing usage window
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: vernu 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
Comment @coderabbitai help to get the list of available commands. Warning ⚠️ This pull request has been flagged as potential spam (vandalism) by CodeRabbit slop detection and should be reviewed carefully. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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 `@android/app/src/main/java/com/vernu/sms/workers/SMSStatusUpdateWorker.kt`: - Around line 78-80: Move the Gson parsing of smsDtoJson into its own guarded block before the existing processing flow, catching JsonSyntaxException and handling a null or empty parsed value by returning Result.failure(). Ensure malformed or empty persisted JSON cannot escape doWork, while leaving the existing status-update handling unchanged for valid data. In `@api/src/billing/billing.service.ts`: - Around line 59-62: Update getMonthlyWindowStart to subtract a fixed 30-day duration in milliseconds from the current timestamp instead of using local-calendar setDate arithmetic. Add coverage in a DST-observing timezone verifying the resulting window is exactly 30×24 hours, while preserving its use by all quota queries.
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: f3fd1587-a5a8-4f05-ba88-3f753d102c94
📥 CommitsReviewing files that changed from the base of the PR and between bf0197e and 74a0e9f.
📒 Files selected for processing (3)- android/app/src/main/java/com/vernu/sms/workers/SMSStatusUpdateWorker.kt
- api/src/billing/billing.service.spec.ts
- api/src/billing/billing.service.ts
Commit Status: Vercel: Vercel
Conclusion: failure
Authorization required to deploy.
api/src/billing/billing.service.spec.ts (1)android/app/src/main/java/com/vernu/sms/workers/SMSStatusUpdateWorker.kt (5)1-39: 📐 Maintainability & Code Quality
Verify coverage for the removed billing flows.
This change removes cancellation, revocation, and checkout validation tests. Restore or relocate those tests if equivalent coverage does not exist elsewhere. The supplied context shows no replacement coverage for these billing paths.
10-10: LGTM!
Also applies to: 22-27
40-42: 🗄️ Data Integrity & Integration
Preserve ordering across SMS status transitions.
The supplied SMSStatusReceiver.updateSMSStatus caller enqueues each status update. Because Line 40 includes smsDTO.status, two statuses for one SMS use different unique-work names. ExistingWorkPolicy.KEEP only suppresses pending work with the same name, so these transitions are not serialized and can reach updateSMSStatus out of order. WorkManager defines unique work per name. (developer.android.com)
Confirm that the API rejects stale statuses or applies a monotonic version. Otherwise key the work by SMS ID only and serialize transitions. Add a test for rapid status changes.
59-62: 🎯 Functional Correctness
Clarify whether MAX_RETRIES counts retries or executions.
Line 59 allows counts 0 through 4 to call the API and fails at count 5. If MAX_RETRIES means five retries after the initial call, this stops one execution early. If it means five total executions, rename it to MAX_ATTEMPTS. WorkManager exposes a nonnegative current attempt count, and Result.retry() schedules another run. (developer.android.com)
Confirm the intended semantics and test counts 0, 4, and 5.
78-80: 🗄️ Data Integrity & Integration
Verify that response-parse retries are idempotent.
updateSMSStatus(...).execute() performs the outbound write before this catch handles response parsing. If the server commits the update but returns malformed JSON, Result.retry() resends the same request. WorkManager defines Result.retry() as another execution of transiently failed work. (developer.android.com)
Confirm that the endpoint is idempotent for repeated SMS status values or uses an idempotency key or server-side duplicate guard. Otherwise do not automatically retry an ambiguous response.
78-80: 🎯 Functional Correctness
Verify strictness for malformed JSON handling.
Gson 2.9.0 enables lenient parsing during fromJson, so some non-standard JSON can be accepted without raising JsonSyntaxException. If the retry requirement covers those response bodies, use strict parsing or validate the decoded response. Verify the ApiManager converter and add representative malformed-body tests. (github.com)
Sorry, something went wrong.
| } catch (e: JsonSyntaxException) { | ||
| Log.e(TAG, "Malformed response body: ${e.message}") | ||
| Result.retry() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -eu
file="android/app/src/main/java/com/vernu/sms/workers/SMSStatusUpdateWorker.kt"
printf '%s\n' '--- target file ---'
cat -n "$file" | sed -n '1,125p'
printf '%s\n' '--- related symbols and persisted-key usage ---'
rg -n -C 3 'SMSStatusUpdateWorker|KEY_SMS_DTO|smsDtoJson|SMSDTO|JsonSyntaxException|Result\.retry\(\)|Result\.failure\(\)' android/app/src/main
printf '%s\n' '--- Gson declarations and dependency version ---'
rg -n -C 2 'gson|Gson\(' android --glob '!*build*'Repository: vernu/textbee
Length of output: 42587
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- dependency files ---'
git ls-files android | rg '(^|/)(build\.gradle(\.kts)?|gradle\.properties|libs\.versions\.toml)$' | xargs -r -n1 sh -c 'echo "--- $0"; rg -n -C 2 "gson|retrofit|converter-gson" "$0"'
printf '%s\n' '--- SMSDTO ---'
cat -n android/app/src/main/java/com/vernu/sms/dtos/SMSDTO.kt
printf '%s\n' '--- local Gson artifacts ---'
find "$HOME/.gradle" "$HOME/.m2" -type f \( -iname 'gson-*.jar' -o -iname 'converter-gson-*.jar' \) -print 2>/dev/null | head -20Repository: vernu/textbee
Length of output: 603
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- SMSDTO ---'
cat -n android/app/src/main/java/com/vernu/sms/dtos/SMSDTO.kt
printf '%s\n' '--- local Gson artifacts ---'
find "$HOME/.gradle" "$HOME/.m2" -type f \( -iname 'gson-*.jar' -o -iname 'converter-gson-*.jar' \) -print 2>/dev/null | head -20 || true
printf '%s\n' '--- exact dependency context ---'
sed -n '88,108p' android/app/build.gradleRepository: vernu/textbee
Length of output: 1792
🌐 Web query:
Gson 2.9.0 Gson.fromJson(String, Class) empty string null JsonSyntaxException invalid JSON official source
💡 Result:
In Gson 2.9.0, the Gson.fromJson(String, Class) method intentionally returns null when the input string is empty or null, rather than throwing a JsonSyntaxException [1]. This behavior is a long-standing design choice preserved for backwards compatibility with versions of Gson prior to 1.5 [2]. When Gson detects an empty document during parsing, it catches an EOFException and returns null instead of treating the empty input as invalid syntax [2]. Because this behavior is hardcoded into the core Gson parsing logic, it cannot be overridden by standard mechanisms like custom JsonDeserializers, as those methods are often not invoked when the input is empty [2][3]. If you require an exception to be thrown for empty input, you can use the following workarounds: 1. Use the TypeAdapter API: Directly using the TypeAdapter for a given class will throw an EOFException (or a derived exception) when encountering an empty string, rather than returning null [4][5]. Example: gson.getAdapter(MyClass.class).fromJson(new StringReader("")); // Throws IOException/EOFException 2. Manual Validation: Check if the string is empty or blank before passing it to Gson.fromJson [2]. 3. Custom JsonReader: You can wrap your input in a custom JsonReader that catches EOFException and rethrows it as a JsonSyntaxException, which you can then pass to the Gson.fromJson(JsonReader, Type) method [2].
Citations:
- 1: https://javadoc.io/static/com.google.code.gson/gson/2.9.0/com.google.gson/com/google/gson/Gson.html
- 2: https://stackoverflow.com/questions/42396634/prevent-gson-from-returning-null-when-deserializing-empty-strings
- 3: Attempting to parse an empty string returns null instead of throwing JsonSyntaxException google/gson#457
- 4: fromJson returns null for empty string google/gson#540
- 5: https://stackoverflow.com/questions/39898428/parsing-an-empty-string-with-gson-returns-null-instead-of-throwing-jsonsyntaxexc
Catch malformed queued JSON at the parse site.
Gson().fromJson(...) runs before the try block. Malformed JSON can escape from doWork, and empty input returns null. Parse smsDtoJson separately and return Result.failure() for malformed or empty persisted data.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/com/vernu/sms/workers/SMSStatusUpdateWorker.kt` around lines 78 - 80, Move the Gson parsing of smsDtoJson into its own guarded block before the existing processing flow, catching JsonSyntaxException and handling a null or empty parsed value by returning Result.failure(). Ensure malformed or empty persisted JSON cannot escape doWork, while leaving the existing status-update handling unchanged for valid data.
Sorry, something went wrong.
| private getMonthlyWindowStart(): Date { | ||
| const start = new Date() | ||
| start.setDate(start.getDate() - 30) | ||
| return start |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target implementation and usages ---'
sed -n '45,75p' api/src/billing/billing.service.ts
rg -n -C 3 'getMonthlyWindowStart|setDate|30.*day|monthly' api/src/billing api/test api/tests 2>/dev/null || true
printf '%s\n' '--- related tests and timezone configuration ---'
rg -n -i -C 3 'BillingService|canPerformAction|timezone|TZ|DST|SMS' api --glob '*.{test,spec}.{ts,js}' --glob '*.{json,yml,yaml,ts,js}' 2>/dev/null | head -300 || trueRepository: vernu/textbee
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- existing regression test ---'
sed -n '1,55p' api/src/billing/billing.service.spec.ts
printf '%s\n' '--- exact helper call sites ---'
rg -n 'this\.getMonthlyWindowStart\(\)' api/src/billing/billing.service.ts
printf '%s\n' '--- local-date versus elapsed-time behavior ---'
TZ=America/New_York node - <<'JS'
const DAY_MS = 24 * 60 * 60 * 1000
for (const iso of [
'2024-03-31T12:00:00.000Z',
'2024-11-03T12:00:00.000Z',
'2024-06-15T12:00:00.000Z',
]) {
const now = new Date(iso)
const calendar = new Date(now)
calendar.setDate(calendar.getDate() - 30)
const elapsed = new Date(now.getTime() - 30 * DAY_MS)
console.log(JSON.stringify({
timezone: process.env.TZ,
now: now.toISOString(),
calendarStart: calendar.toISOString(),
calendarSpanHours: (now - calendar) / (60 * 60 * 1000),
elapsedStart: elapsed.toISOString(),
elapsedSpanHours: (now - elapsed) / (60 * 60 * 1000),
}))
}
JS
printf '%s\n' '--- timezone-related deployment configuration ---'
rg -n -i '(^|[^A-Za-z])(TZ|timezone|time zone)([^A-Za-z]|$)' . --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!*.lock' | head -100 || trueRepository: vernu/textbee
Length of output: 5361
Use elapsed-time arithmetic for the fixed 30-day window.
setDate() uses local calendar days. Across daylight-saving transitions, the window can span 719 or 721 hours. All three quota queries use this helper. Add a test in a DST-observing timezone.
Proposed fix private getMonthlyWindowStart(): Date {
const start = new Date()
- start.setDate(start.getDate() - 30)
+ start.setTime(start.getTime() - 30 * 24 * 60 * 60 * 1000)
return start
}‼️ 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.
| private getMonthlyWindowStart(): Date { | |
| const start = new Date() | |
| start.setDate(start.getDate() - 30) | |
| return start | |
| private getMonthlyWindowStart(): Date { | |
| const start = new Date() | |
| start.setTime(start.getTime() - 30 * 24 * 60 * 60 * 1000) | |
| return start |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/billing/billing.service.ts` around lines 59 - 62, Update getMonthlyWindowStart to subtract a fixed 30-day duration in milliseconds from the current timestamp instead of using local-calendar setDate arithmetic. Add coverage in a DST-observing timezone verifying the resulting window is exactly 30×24 hours, while preserving its use by all quota queries.
Sorry, something went wrong.
fix(billing): stop monthly quota window from shrinking on long months (#283)
Added one private helper getMonthlyWindowStart() that subtracts 30 days via
setDate, which can't overflow the way setMonth did.
Replaced the identical expression at all 3 sites (enforcement +
two display paths) with this.getMonthlyWindowStart().
Tests
Added billing.service.spec.ts: pins system time to Mar 31 (the old overflow
day) and asserts the window is always a full 30 days; plus a normal-day case.
PASS src/billing/billing.service.spec.ts
gives a full 30-day window on Mar 31 (the old overflow day)
gives a 30-day window on a normal day too
Summary by CodeRabbit