fix: handle case of empty giftcard list#33
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds HTML-generation hooks to data sources and persists merchant/brand state for that output; introduces an ChangesHTML generation + offline sync
Sequence Diagram(s)sequenceDiagram
participant CLI
participant SyncProcessor
participant DataSource
participant LocalFS
participant GCS
CLI->>SyncProcessor: start(args, include -offline?)
SyncProcessor->>SyncProcessor: check isRunning guard
alt offlineMode == false
SyncProcessor->>GCS: acquire lock / check remote DB / compare checksums
else offlineMode == true
SyncProcessor-->>SyncProcessor: skip lock & remote compare
end
SyncProcessor->>DataSource: importData() (CTX, PiggyCards)
DataSource->>DataSource: fetch & persist merchants/brands & locations/giftcards
DataSource->>LocalFS: generateHtmlFile() -> write dated HTML
alt offlineMode == false
SyncProcessor->>GCS: upload HTML file(s), DBs, delete lock
else offlineMode == true
SyncProcessor-->>GCS: skip uploads & lock removal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[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 (1)
src/main/kotlin/org/dash/mobile/explore/sync/SyncProcessor.kt (1)
224-225: Log that offline mode skips previous-run comparisons.When
offlineModeis enabled,previousLocationsFileis alwaysnull, so new/removed merchant detection is silently omitted from the report. A notice here would make offline output easier to interpret.Small observability improvement
- val previousLocationsFile = if (!offlineMode) gcManager.downloadMostRecentLocationsDb() else null + val previousLocationsFile = if (!offlineMode) { + gcManager.downloadMostRecentLocationsDb() + } else { + logger.notice("Offline mode: skipping previous locations database comparison") + null + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/org/dash/mobile/explore/sync/SyncProcessor.kt` around lines 224 - 225, The code silently skips previous-run comparisons when offlineMode is true; update SyncProcessor to log this event: where previousLocationsFile is set (the line using offlineMode and gcManager.downloadMostRecentLocationsDb()), add a clear info/debug log when offlineMode is true indicating "offline mode enabled — skipping previous locations DB comparison, new/removed merchant detection omitted" so users can understand why comparisons are not performed; use the existing logger instance in the class (or create one if none) and keep the message concise and consistent with other logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/org/dash/mobile/explore/sync/MainApp.kt`:
- Line 36: The help text for OFFLINE_ARG currently says it only skips
publishing; update the println in MainApp.kt that prints "$OFFLINE_ARG - offline
mode: skip all publishing to Google Cloud Storage" to clearly state that offline
mode disables all Google Cloud Storage interactions (including acquiring locks
and comparing against the previous database) rather than only skipping
publishing; edit the string emitted where OFFLINE_ARG is used so it mentions
locks and previous-database comparison.
In `@src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt`:
- Around line 59-65: The Retrofit interface method getAllMerchants only accepts
perPage and page, but the caller is passing apiKey, apiSecret, pageSize,
currentPageIndex positionally; update the call to invoke getAllMerchants using
named params perPage = pageSize, page = currentPageIndex and remove the
extraneous apiKey/apiSecret positional arguments (or if the API requires those,
reintroduce them as `@Header` parameters in getAllMerchants by uncommenting/adding
apiKey and apiSecret into the getAllMerchants signature). Ensure you reference
the getAllMerchants method and the caller variables pageSize/currentPageIndex
when making the change.
---
Nitpick comments:
In `@src/main/kotlin/org/dash/mobile/explore/sync/SyncProcessor.kt`:
- Around line 224-225: The code silently skips previous-run comparisons when
offlineMode is true; update SyncProcessor to log this event: where
previousLocationsFile is set (the line using offlineMode and
gcManager.downloadMostRecentLocationsDb()), add a clear info/debug log when
offlineMode is true indicating "offline mode enabled — skipping previous
locations DB comparison, new/removed merchant detection omitted" so users can
understand why comparisons are not performed; use the existing logger instance
in the class (or create one if none) and keep the message concise and consistent
with other logs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 538f0788-1a92-43b3-a21e-147cb863b91b
📒 Files selected for processing (5)
src/main/kotlin/org/dash/mobile/explore/sync/MainApp.ktsrc/main/kotlin/org/dash/mobile/explore/sync/SyncProcessor.ktsrc/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.ktsrc/main/kotlin/org/dash/mobile/explore/sync/process/DataSource.ktsrc/main/kotlin/org/dash/mobile/explore/sync/process/PiggyCardsDataSource.kt
| println("$QUIET_ARG - quiet mode: no notifications are pushed to Slack") | ||
| println("$PROD_ARG - production mode: use production data sources/destinations") | ||
| println("$DEBUG_ARG - output to CSV files for unit tests, BODY logging") | ||
| println("$OFFLINE_ARG - offline mode: skip all publishing to Google Cloud Storage") |
There was a problem hiding this comment.
Clarify the -offline help text.
SyncProcessor skips all GCS interactions in offline mode, including locks and previous database comparison, not just publishing.
Suggested wording
- println("$OFFLINE_ARG - offline mode: skip all publishing to Google Cloud Storage")
+ println("$OFFLINE_ARG - offline mode: skip all Google Cloud Storage operations")📝 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.
| println("$OFFLINE_ARG - offline mode: skip all publishing to Google Cloud Storage") | |
| println("$OFFLINE_ARG - offline mode: skip all Google Cloud Storage operations") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/org/dash/mobile/explore/sync/MainApp.kt` at line 36, The help
text for OFFLINE_ARG currently says it only skips publishing; update the println
in MainApp.kt that prints "$OFFLINE_ARG - offline mode: skip all publishing to
Google Cloud Storage" to clearly state that offline mode disables all Google
Cloud Storage interactions (including acquiring locks and comparing against the
previous database) rather than only skipping publishing; edit the string emitted
where OFFLINE_ARG is used so it mentions locks and previous-database comparison.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt (1)
171-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
getAllMerchantLocationsin try/catch like the merchants loop.The merchants fetch above (lines 122-163) handles
IOException/HttpException, but the locations call here is unguarded. A transient network error or 5xx will bubble up through theflow { ... }and abort the import, leavingdataSourceReportunset and downstream consumers without a graceful empty/partial result — directly relevant to "handle case of empty giftcard list".Consider catching the same exceptions and treating it as an empty location set (or rethrowing only for non-recoverable cases) so the rest of the pipeline can finish.
🛡️ Suggested handling
- val locationResponse = apiService.getAllMerchantLocations(apiKey, apiSecret) + val locationResponse = try { + apiService.getAllMerchantLocations(apiKey, apiSecret) + } catch (ex: IOException) { + logger.error("Failed to fetch CTX locations: ${ex.message}", ex) + JsonArray() + } catch (ex: HttpException) { + logger.error("CTX locations HTTP error: ${ex.message}", ex) + JsonArray() + } val invalidLocations = linkedMapOf<String, JsonObject>()🤖 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 `@src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt` around lines 171 - 176, The getAllMerchantLocations call is not exception-safe; wrap the apiService.getAllMerchantLocations(apiKey, apiSecret) invocation in a try/catch mirroring the merchants loop (catch IOException and HttpException), log the error, and on those recoverable failures treat locations as empty (e.g., set locationResponse to an empty JsonArray/JsonObject-equivalent or skip processing) so the flow in CTXSpendDataSource continues and dataSourceReport downstream still gets produced; reference the getAllMerchantLocations call, locationResponse, invalidLocations, dataSourceReport, and the same exception handling used around the merchants fetch.
🤖 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 `@src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt`:
- Line 373: The read of m["enabled"].asString can NPE or throw on JsonNull and
semantically this field is boolean elsewhere; in CTXSpendDataSource replace that
line with a null/JsonNull-safe boolean read (mirror the savingsPercentage
pattern) and then convert to string only if escapeJson expects a string: e.g.
compute enabledBool = m["enabled"]?.takeIf { it !is JsonNull }?.asBoolean ?:
false and then set enabled = escapeJson(enabledBool.toString()); update the
reference to m["enabled"] and the variable enabled accordingly.
- Around line 549-555: The current escapeJson function is incomplete and unsafe
for inlining JSON into <script> blocks; replace the hand-rolled escaping by
serializing values with Gson (e.g., use Gson/GsonBuilder and gson.toJson on
merchant/location objects) and then make the JSON script-safe by escaping
closing script sequences (e.g., replace "</" with "<\\/") before inlining;
remove or stop using escapeJson and instead emit Gson-produced JSON (or
JsonObject/JsonPrimitive) via a helper like toJsScriptSafe that calls
gson.toJson(value).replace("</", "<\\/") so control characters and
HTML-sensitive sequences are handled correctly.
---
Outside diff comments:
In `@src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt`:
- Around line 171-176: The getAllMerchantLocations call is not exception-safe;
wrap the apiService.getAllMerchantLocations(apiKey, apiSecret) invocation in a
try/catch mirroring the merchants loop (catch IOException and HttpException),
log the error, and on those recoverable failures treat locations as empty (e.g.,
set locationResponse to an empty JsonArray/JsonObject-equivalent or skip
processing) so the flow in CTXSpendDataSource continues and dataSourceReport
downstream still gets produced; reference the getAllMerchantLocations call,
locationResponse, invalidLocations, dataSourceReport, and the same exception
handling used around the merchants fetch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d04c4184-47ec-48a7-9460-144290b466fc
📒 Files selected for processing (3)
.gitignoresrc/main/kotlin/org/dash/mobile/explore/sync/SyncProcessor.ktsrc/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/org/dash/mobile/explore/sync/SyncProcessor.kt
| val redeemType = escapeJson(m["redeemType"]?.asString ?: "") | ||
| val denomType = escapeJson(m["denominationsType"]?.asString ?: "") | ||
| val type = escapeJson(m["type"]?.asString ?: "") | ||
| val enabled = escapeJson(m["enabled"].asString ?: "") |
There was a problem hiding this comment.
Potential NPE/UnsupportedOperationException on m["enabled"].asString.
Two problems on this line:
JsonObject.get("enabled")returns a Kotlin platform type — if the key is missing it returnsnull, and.asStringwill then throw NPE before?: ""can apply.- If the value is
JsonNull,JsonElement.asStringthrowsUnsupportedOperationException.
Note also that elsewhere enabled is treated as a Boolean (line 146 uses .asBoolean), so reading it as a string is semantically odd — and the safer null/JsonNull-aware pattern is already used at line 369 for savingsPercentage. Mirror that here.
🛡️ Proposed fix
- val enabled = escapeJson(m["enabled"].asString ?: "")
+ val enabled = m["enabled"]
+ ?.takeIf { !it.isJsonNull }
+ ?.asBoolean
+ ?.toString()
+ ?: ""🤖 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 `@src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt`
at line 373, The read of m["enabled"].asString can NPE or throw on JsonNull and
semantically this field is boolean elsewhere; in CTXSpendDataSource replace that
line with a null/JsonNull-safe boolean read (mirror the savingsPercentage
pattern) and then convert to string only if escapeJson expects a string: e.g.
compute enabledBool = m["enabled"]?.takeIf { it !is JsonNull }?.asBoolean ?:
false and then set enabled = escapeJson(enabledBool.toString()); update the
reference to m["enabled"] and the variable enabled accordingly.
| private fun escapeJson(str: String): String { | ||
| return str.replace("\\", "\\\\") | ||
| .replace("\"", "\\\"") | ||
| .replace("\n", "\\n") | ||
| .replace("\r", "\\r") | ||
| .replace("\t", "\\t") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the recommended way to safely embed JSON inside an HTML <script> tag, and which characters (e.g. </, U+2028) need extra escaping beyond standard JSON?
💡 Result:
Recommended way (safe HTML + JavaScript handling) 1) Put the JSON as text inside a non-executing <script> block (type="application/json"), and ensure the embedded JSON cannot terminate the script tag when the HTML parser scans it. Example pattern: <script id="my-json" type="application/json">{ "foo": "bar" }</script> Then read it with DOM textContent and parse it: const data = JSON.parse(document.getElementById('my-json').textContent); This avoids directly executing JSON as JavaScript, and using a fixed element boundary means you only have to escape the characters that could affect HTML parsing / JavaScript string parsing. [1][2] 2) When generating the JSON string server-side for inclusion inside <script>...</script>, escape the characters that could break out of the script element during HTML parsing. In practice, “escape < and >” is the key requirement; multiple sources recommend replacing “<” with \u003c and “>” with \u003e (often also &, ', etc. depending on your templating/attribute context). [3][4][5][6] Which characters need extra escaping beyond standard JSON? A) HTML-breaking sequences: < and > (most important) - Any literal < inside the script contents risks creating markup/closing-tag opportunities depending on surrounding text, so it must be escaped (commonly as \u003c). [3][5][4] - Similarly, escaping > as \u003e is commonly included as part of “safe for <script> tags”. [3][7] B) JavaScript string literal line terminators: U+2028 and U+2029 - JSON itself allows U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR) unescaped in the decoded string, but JavaScript treats them as line terminators in string literals, which can break parsing if they appear unescaped in generated JavaScript source. [8][9] - Therefore, when your JSON is embedded into a JavaScript string literal (directly in JS source), you must ensure U+2028/U+2029 are escaped (e.g., as \u2028 / \u2029). [10][8] - When you store the JSON as raw text in <script type="application/json"> and read via textContent, you typically avoid JavaScript string-literal concerns because you are not interpreting it as a JS string literal; still, escaping these characters is prudent if your pipeline might re-emit JSON inside JS string syntax. [8][9] C) Closing-tag / comment-like delimiters: </script> and depending on technique - The security requirement is “prevent the data from containing the closing </script>”. A common recommendation is to escape “<” (and sometimes also “>”) so that sequences like </script> cannot appear. [3] - Some older approaches also recommend escaping comment delimiters (e.g., -->) if you’re using HTML/JS comment hacks, but the modern, recommended approach above (type=application/json + textContent) avoids needing the comment hack. [1][3] D) What does “standard JSON escaping” cover? - Standard JSON escaping handles things like quotation marks and backslashes inside JSON strings, but it does not automatically protect against HTML parsing of the surrounding <script> element; that’s why you still must escape < (and often >) for HTML/script context. [3][7][5] Practical guidance by language/runtime (PHP examples) - PHP json_encode provides flags that correspond to the common safe escaping needs. For embedding JSON into script tags, sources recommend JSON_HEX_TAG (escapes < and > as \u003c/\u003e) and often also handle other contexts with JSON_HEX_* flags. [11][6][12] - Additionally, escaping U+2028/U+2029 may depend on whether the encoder outputs line terminators unescaped; discussion of U+2028/U+2029 breaking JSONP/JS parsing indicates the need to escape them to \u2028/\u2029 when they would otherwise appear literally. [10][8] Bottom line - Escape at least: “<” (→ \u003c) (and typically also “>” → \u003e) for safe embedding inside <script> contents. [3][5] - If your JSON appears inside a JavaScript string literal in page source, also ensure: U+2028 and U+2029 are escaped (→ \u2028 / \u2029). [8][10] - Prefer <script type="application/json"> + textContent + JSON.parse to keep JSON from being interpreted as executable JavaScript. [1][2]
Citations:
- 1: https://news.ycombinator.com/item?id=44842442
- 2: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Safely_inserting_external_content_into_a_page
- 3: https://stackoverflow.com/questions/45622468/secure-way-of-embedding-json-in-a-script-tag
- 4: https://sirre.al/2025/08/06/safe-json-in-script-tags-how-not-to-break-a-site/
- 5: https://html.spec.whatwg.org/scripting.html
- 6: https://stackoverflow.com/questions/25362056/proper-way-to-escape-json-data-in-php-without-using-js-comment-hack
- 7: https://stackoverflow.com/questions/39193510/how-to-insert-arbitrary-json-in-htmls-script-tag
- 8: UTF-8 chars U+2028 and U+2029 are not escaped - lead to JSON parse error in JavaScript FasterXML/jackson-databind#1485
- 9: https://groups.google.com/g/Google-Web-Toolkit/c/svJogNhhr-I
- 10: https://stackoverflow.com/questions/14179377/how-to-replace-escape-u2028-or-u2029-characters-in-php-to-stop-my-jsonp-api-br
- 11: https://secure.php.net/json_encode
- 12: Flag any echos inside of
<script>tags ifwp_json_encode()not used and ensure JSON is encoded with the best flags WordPress/WordPress-Coding-Standards#63
🏁 Script executed:
# First, locate the file
find . -name "CTXSpendDataSource.kt" -type fRepository: dashpay/explore-dash-sync
Length of output: 146
🏁 Script executed:
# Check the content around the escapeJson function and the script tag context
if [ -f "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt" ]; then
echo "=== Lines 549-555 (escapeJson function) ==="
sed -n '549,555p' "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt"
echo ""
echo "=== Lines 455-460 (script block context) ==="
sed -n '455,460p' "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt"
echo ""
echo "=== Lines 440-475 (broader context) ==="
sed -n '440,475p' "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt"
fiRepository: dashpay/explore-dash-sync
Length of output: 1752
🏁 Script executed:
# Search for usages of escapeJson in the file
rg "escapeJson" "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt" -n -B 2 -A 2Repository: dashpay/explore-dash-sync
Length of output: 2553
🏁 Script executed:
# Check if Gson is already imported or used elsewhere in the file
rg "import.*Gson|Gson\(" "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt" -nRepository: dashpay/explore-dash-sync
Length of output: 162
🏁 Script executed:
# Check for data validation or sanitization before escapeJson is called
rg "merchants|locations" "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt" -n -B 2 -A 2 | head -100Repository: dashpay/explore-dash-sync
Length of output: 4600
🏁 Script executed:
# Search for comments or documentation about data sources and trust levels
rg "generated|report|trusted|validate|sanitize" "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt" -n -iRepository: dashpay/explore-dash-sync
Length of output: 479
🏁 Script executed:
# Check what data sources feed into merchants/locations
sed -n '300,380p' "src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt"Repository: dashpay/explore-dash-sync
Length of output: 3634
escapeJson is incomplete and vulnerable to script tag breakouts; use Gson instead.
The hand-rolled escape misses control characters (\b, \f, \u0000–\u001F) and critical HTML context characters. Because the JSON is embedded directly in <script> blocks (line 454), any merchant or location string containing </script> will terminate the script tag prematurely, and <!-- (or even just <) can break or alter script execution. The data originates from external APIs (@GET("merchants"), @GET("dcg/locations") at lines 59–68) with no sanitization before escaping, making this a real vulnerability.
Two practical mitigations:
- Serialize via
Gson/JsonPrimitiveinstead of hand-concatenating JSON strings; Gson handles all control characters correctly. - When JSON must be inlined in
<script>, escape<to\u003c(or specifically</to<\/) so the closing tag cannot appear verbatim.
♻️ Sketch
private val gson = GsonBuilder().disableHtmlEscaping().create()
private fun toJsScriptSafe(value: Any?): String =
gson.toJson(value).replace("</", "<\\/")Then build merchant/location objects as JsonObject/Map/List and emit with toJsScriptSafe(...) rather than hand-concatenated strings like "\"key\": \"$value\"".
🤖 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 `@src/main/kotlin/org/dash/mobile/explore/sync/process/CTXSpendDataSource.kt`
around lines 549 - 555, The current escapeJson function is incomplete and unsafe
for inlining JSON into <script> blocks; replace the hand-rolled escaping by
serializing values with Gson (e.g., use Gson/GsonBuilder and gson.toJson on
merchant/location objects) and then make the JSON script-safe by escaping
closing script sequences (e.g., replace "</" with "<\\/") before inlining;
remove or stop using escapeJson and instead emit Gson-produced JSON (or
JsonObject/JsonPrimitive) via a helper like toJsScriptSafe that calls
gson.toJson(value).replace("</", "<\\/") so control characters and
HTML-sensitive sequences are handled correctly.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements