Conversation
🦋 Changeset detectedLatest commit: 6160a7e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe mobile app adds maturity-based card statements, localized statement views, payment-history navigation, breakdowns, and guarded PDF downloads for web and native platforms. ChangesCard statement experience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pay
participant PaymentHistory
participant HistorySheet
participant Statement
participant StatementAPI
participant FileSharing
Pay->>PaymentHistory: display past maturities
PaymentHistory->>Pay: return selected maturity
Pay->>HistorySheet: open payment history
HistorySheet->>Statement: navigate with maturity
Statement->>StatementAPI: fetch statement activity
Statement->>FileSharing: download and share PDF
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 393538b7-cc1b-4824-8e1b-55d6bc881ef2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/curvy-jobs-sort.mdapp.config.tspackage.jsonsrc/app/(main)/statement/_layout.tsxsrc/app/(main)/statement/index.tsxsrc/components/pay/Breakdown.tsxsrc/components/pay/HistorySheet.tsxsrc/components/pay/Pay.tsxsrc/components/pay/PaymentHistory.tsxsrc/components/pay/PaymentSheet.tsxsrc/components/pay/StatementActions.tsxsrc/components/shared/ModalSheet.tsxsrc/components/statement/Statement.tsxsrc/i18n/es.jsonsrc/i18n/pt.jsonsrc/utils/server.tssrc/utils/statement.tssrc/utils/useStatement.tssrc/utils/useStatements.ts
| const [downloading, setDownloading] = useState(false); | ||
|
|
||
| function download() { | ||
| if (downloading) return; | ||
| setDownloading(true); | ||
| downloadStatement(maturity, `account-statement-${maturity}.pdf`) | ||
| .catch(reportError) | ||
| .finally(() => { | ||
| setDownloading(false); | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The statement download logic is duplicated in two components. Both copies hold the same downloading state, build the same filename, call downloadStatement, catch with reportError, and reset in finally.
src/components/pay/StatementActions.tsx#L24-L34: replace the local state anddownload()with a shared hook, for exampleuseDownloadStatement(maturity).src/components/statement/Statement.tsx#L52-L60: remove the local state anddownload()and use the same hook, keeping the existingmaturity === undefinedguard inside the call site.
📍 Affects 2 files
src/components/pay/StatementActions.tsx#L24-L34(this comment)src/components/statement/Statement.tsx#L52-L60
Source: Coding guidelines
| const parameter = useLocalSearchParams().maturity; | ||
| const raw = Array.isArray(parameter) ? parameter[0] : parameter; | ||
| const maturity = raw && /^\d+$/.test(raw) ? Number(raw) : undefined; | ||
| const { data, isLoading } = useStatement(maturity); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Handle an invalid maturity parameter explicitly.
If maturity is missing or not numeric, useStatement receives undefined, the query is skipped, and the screen renders "No statement for this period". The user cannot distinguish a broken link from an empty period. Render an error state, or redirect back, when raw is present but does not match /^\d+$/.
| "{{currency}} via {{methods}}": "{{currency}} vía {{methods}}", | ||
| "{{discount}} off": "{{discount}} off", | ||
| "{{network}} deposit address": "Dirección de depósito de {{network}}", | ||
| "{{percent}} late fee": "{{percent}} de interés", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use late-payment fee terminology in both translations.
Both values translate late fee as interest. This changes the financial meaning.
src/i18n/es.json#L20-L20: replace the interest term with a late-payment fee term.src/i18n/pt.json#L20-L20: replace the interest term with a late-payment fee term.
📍 Affects 2 files
src/i18n/es.json#L20-L20(this comment)src/i18n/pt.json#L20-L20
| const key = item.timestamp.slice(0, 10); | ||
| const dates = card.dates.get(key) ?? { label: item.timestamp, rows: [] }; | ||
| dates.rows.push(...lines.map((line) => ({ merchant: item.merchant.name, ...line }))); | ||
| card.dates.set(key, dates); | ||
| cards.set(item.cardId, card); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Row identity is lost in group(), so the statement list has unstable keys. group() builds each Row from merchant, installment index, and amount, and drops the activity item id. The consumer then has no stable key.
src/utils/statement.ts#L30-L34: include the source item id in each pushed row, for example{ id: item.id, merchant: item.merchant.name, ...line }, and addid: stringto theRowtype.src/components/statement/Statement.tsx#L145-L146: replace the composite key withkey={row.id}, or with`${row.id}-${row.current}`when one purchase yields several installment rows.
📍 Affects 2 files
src/utils/statement.ts#L30-L34(this comment)src/components/statement/Statement.tsx#L145-L146
| const url = pdf(bytes); | ||
| const anchor = document.createElement("a"); | ||
| anchor.href = url; | ||
| anchor.download = filename; | ||
| document.body.append(anchor); | ||
| anchor.click(); | ||
| anchor.remove(); | ||
| URL.revokeObjectURL(url); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Defer URL.revokeObjectURL so the web download is not cancelled.
The code revokes the object URL in the same task as anchor.click(). Some browsers, notably Safari, start the download asynchronously and then fail because the blob URL is already revoked. Revoke the URL after the current task.
🛠️ Proposed fix
anchor.click();
anchor.remove();
- URL.revokeObjectURL(url);
+ setTimeout(() => {
+ URL.revokeObjectURL(url);
+ }, 0);📝 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.
| const url = pdf(bytes); | |
| const anchor = document.createElement("a"); | |
| anchor.href = url; | |
| anchor.download = filename; | |
| document.body.append(anchor); | |
| anchor.click(); | |
| anchor.remove(); | |
| URL.revokeObjectURL(url); | |
| const url = pdf(bytes); | |
| const anchor = document.createElement("a"); | |
| anchor.href = url; | |
| anchor.download = filename; | |
| document.body.append(anchor); | |
| anchor.click(); | |
| anchor.remove(); | |
| setTimeout(() => { | |
| URL.revokeObjectURL(url); | |
| }, 0); |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/pay/Pay.tsx (1)
188-191: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep payment history available after all positions are settled.
Line 188 renders
PaymentHistoryonly whenhasPaymentsis true. Line 191 also removesHistorySheetin the empty state.hasPaymentsonly represents active positions, butuseStatements()can still return historical maturities. Render both components outside this conditional, or render payment history withEmpty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b1997f1-d9df-46e4-9a91-796544dbf19f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/curvy-jobs-sort.mdapp.config.tspackage.jsonsrc/app/(main)/statement/_layout.tsxsrc/app/(main)/statement/index.tsxsrc/components/pay/Breakdown.tsxsrc/components/pay/HistorySheet.tsxsrc/components/pay/Pay.tsxsrc/components/pay/PaymentHistory.tsxsrc/components/pay/PaymentSheet.tsxsrc/components/pay/StatementActions.tsxsrc/components/shared/ModalSheet.tsxsrc/components/statement/Statement.tsxsrc/i18n/es.jsonsrc/i18n/pt.jsonsrc/utils/server.tssrc/utils/statement.tssrc/utils/useStatement.tssrc/utils/useStatements.ts
| async function share(bytes: Uint8Array, filename: string) { | ||
| const file = new File(Paths.cache, filename); | ||
| if (file.exists) file.delete(); | ||
| file.write(bytes); | ||
| if (!(await isAvailableAsync())) throw new Error("sharing unavailable"); | ||
| await shareAsync(file.uri, { mimeType: "application/pdf", UTI: "com.adobe.pdf", dialogTitle: filename }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
expo-file-system 55 File write creates file if missing create() required
💡 Result:
In the modern expo-file-system API (part of the new file-system implementation), the behavior of file creation depends on which methods you use [1][2]. If you are using the new File object API (e.g., const file = new File(path);), you must explicitly call file.create before performing write operations [1][2]. By default, create will throw an error if the file already exists, unless you pass the { idempotent: true } option [2][3]. If you are using the legacy FileSystem.writeAsStringAsync method, it does not strictly require you to manually "create" the file first in the same way, but it also does not inherently perform safe "if missing" creation logic in a single atomic step without potentially overwriting existing content [4][5]. Key details: - File object API: Requires manual creation. You can use file.create({ idempotent: true }) to ensure the file exists without throwing an error if it is already there [2][3]. - Legacy API: FileSystem.writeAsStringAsync typically writes/overwrites the contents of the file at the specified URI [4][5]. If your goal is to ensure a file exists before writing, the recommended approach in the new API is: const file = new File(directory, 'filename.txt'); file.create({ idempotent: true }); // Ensures file exists; succeeds silently if already there file.write('Your content here'); [1][2]
Citations:
- 1: https://docs.expo.dev/versions/latest/sdk/filesystem/
- 2: https://docs.expo.dev/versions/v54.0.0/sdk/filesystem
- 3: https://github.com/expo/expo/blob/5cbac55c/packages/expo-file-system/build/ExpoFileSystem.types.d.ts
- 4: https://docs.expo.dev/versions/latest/sdk/filesystem-legacy/
- 5: https://cdn.jsdelivr.net/npm/expo-file-system@19.0.21/build/legacy/FileSystem.d.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate statement.ts =="
fd -a 'statement\.ts$' . | sed 's#^\./##'
echo "== relevant file outline =="
file="$(fd 'statement\.ts$' . | head -n 1)"
if [ -n "${file:-}" ]; then
ast-grep outline "$file" --view compact || true
echo "== relevant file excerpt =="
cat -n "$file" | sed -n '1,130p'
fi
echo "== dependency pins =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock bun.lockb; do
[ -e "$f" ] && { echo "--- $f ---" ; sed -n '1,180p' "$f" | rg -n "expo-file-system|@" -C 2 | head -n 80 || true; }
done
echo "== usages of share/isAvailableAsync/File =="
rg -n "function share|share\(bytes|isAvailableAsync|new File|let File|const File|export .*File|expo-file-system" .Repository: exactly/exa
Length of output: 12988
🌐 Web query:
expo-file-system 55 File write writeAsync missing file create ensureFileAsync idempotent
💡 Result:
In the current version of expo-file-system, there is no method named ensureFileAsync [1][2]. If you are using the modern File API introduced in recent SDK versions, you should explicitly manage file creation using the file.create method before attempting to write to the file [1][3][4]. Here is how the modern File API handles file operations: 1. File Creation: You can instantiate a File object for any path; the file does not need to exist upon instantiation [3][5][6]. To ensure the file exists on the filesystem, you must call the file.create method [1][7]. Note that file.create will throw an error if the file already exists or if permissions are insufficient [1][3]. 2. Writing Content: The file.write method is used to write content to the file [1][2]. Unlike some other APIs, it does not implicitly create the file if it is missing; you must have created it via file.create first [1][4]. 3. Idempotency: Because file.create throws an error if the file already exists, it is not inherently idempotent [1][3]. To achieve idempotent behavior (i.e., ensuring the file exists without throwing an error if it is already there), you should manually check for its existence first: // Example of idempotent-like creation const file = new File(Paths.document, 'example.txt'); const info = await FileSystem.getInfoAsync(file.uri); if (!info.exists) { await file.create; } await file.write('Your content here'); For legacy support or specific use cases, FileSystem.writeAsStringAsync can also write to files [8][2], but it generally requires the file (or the parent directory) to be properly prepared, depending on the platform and URI type [8]. For most new development, the File and Directory classes are the recommended approach [7][4].
Citations:
- 1: https://docs.expo.dev/versions/latest/sdk/filesystem/
- 2: https://docs.expo.dev/versions/v54.0.0/sdk/filesystem
- 3: https://github.com/expo/expo/blob/main/docs/pages/versions/unversioned/sdk/filesystem.mdx
- 4: https://expo.dev/blog/expo-file-system
- 5: https://github.com/expo/expo/blob/68661df5/packages/expo-file-system/src/FileSystem.ts
- 6: https://github.com/expo/expo/blob/5cbac55c/packages/expo-file-system/src/FileSystem.ts
- 7: https://www.mintlify.com/expo/expo/sdk/file-system
- 8: https://docs.expo.dev/versions/latest/sdk/filesystem-legacy/
Ensure the cache file exists before writing.
share writes before isAvailableAsync() can reject or shareAsync can fail, so the cached PDF can remain on the device. Also use if (file.exists) file.delete(); await file.create(); file.write(bytes); because expo-file-system File.write does not create a missing file.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/statement.ts (1)
70-76: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDelete the cached statement after sharing.
share()writes sensitive statement data before it checks availability, then never removes the cache file. Failed and completed share flows leave the PDF inPaths.cache.Check
isAvailableAsync()before writing. Deletefilein afinallyblock aftershareAsync()resolves or rejects.This repeats the prior cache-retention finding.
src/i18n/es.json (1)
20-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate “late fee” as a late-payment charge.
"{{percent}} de interés"means interest. It changes the financial meaning of"{{percent}} late fee".Use a late-payment fee term, such as
"{{percent}} de recargo por mora".This repeats the prior translation finding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96143a4e-1466-4038-bd37-419c2788b7fa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.changeset/curvy-jobs-sort.mdapp.config.tspackage.jsonsrc/app/(main)/payment-history/_layout.tsxsrc/app/(main)/payment-history/index.tsxsrc/app/(main)/statement/_layout.tsxsrc/app/(main)/statement/index.tsxsrc/components/pay/Breakdown.tsxsrc/components/pay/History.tsxsrc/components/pay/HistorySheet.tsxsrc/components/pay/Pay.tsxsrc/components/pay/PaymentHistory.tsxsrc/components/pay/PaymentRow.tsxsrc/components/pay/PaymentSheet.tsxsrc/components/pay/Repay.tsxsrc/components/pay/StatementActions.tsxsrc/components/shared/ModalSheet.tsxsrc/components/statement/Statement.tsxsrc/i18n/es.jsonsrc/i18n/pt.jsonsrc/utils/server.tssrc/utils/statement.tssrc/utils/useStatement.tssrc/utils/useStatements.ts
| {!empty && ( | ||
| <XStack gap="$s2" alignItems="center" cursor="pointer" aria-disabled={downloading} onPress={download}> | ||
| <Text emphasized subHeadline color="$interactiveBaseBrandDefault"> | ||
| {t("Download")} | ||
| </Text> | ||
| {downloading ? ( | ||
| <Spinner color="$interactiveBaseBrandDefault" /> | ||
| ) : ( | ||
| <Download size={20} color="$interactiveBaseBrandDefault" /> | ||
| )} | ||
| </XStack> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose each statement action as an accessible button.
These controls attach onPress to presentation components without a button role or accessible name. Keyboard and screen-reader users cannot reliably operate them on web.
src/components/statement/Statement.tsx#L108-L118: use an accessible button pattern for the download action. Set its accessible name tot("Download").src/components/pay/Breakdown.tsx#L43-L48: use an accessible button pattern for the view-statement action. Set its accessible name tot("View statement").src/components/pay/Breakdown.tsx#L119-L128: use an accessible button pattern for the back action. Set its accessible name tot("Back").
The Action component in src/components/pay/StatementActions.tsx Lines 73-81 provides the local pattern. This repeats the prior statement-control finding.
#!/bin/bash
set -euo pipefail
rg -n -C 3 'role=.*button|aria-label=.*onPress|onPress=' \
src/components/statement/Statement.tsx \
src/components/pay/Breakdown.tsx \
src/components/pay/StatementActions.tsx📍 Affects 2 files
src/components/statement/Statement.tsx#L108-L118(this comment)src/components/pay/Breakdown.tsx#L43-L48src/components/pay/Breakdown.tsx#L119-L128
2 participants
Summary by CodeRabbit