Expensify - export report #18320
Expensify - export report #18320
Conversation
WalkthroughAdds a new Expensify "Export Report" action (expensify-export-report) with templating, filters, onFinish behaviors, and file download; introduces policyExportIds and listPolicies() to the Expensify app; and increments versions for several Expensify actions and the component package. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant A as Export Report Action
participant E as Expensify API
participant FS as Filesystem (/tmp)
U->>A: Provide props (reportIds/startDate/approvedAfter, format, templatePath?, emailRecipients?, markAsExported?, test?)
A->>A: Validate inputs (require reportIds OR startDate OR approvedAfter)
A->>E: POST export job (inputSettings, outputSettings, onFinish, test)
E-->>A: immediate fileName
A->>E: Download file by fileName
E-->>A: File content (binary)
A->>FS: Write /tmp/<fileName>
alt onFinish configured and not test
A->>E: Execute onFinish actions (email / markAsExported)
E-->>A: Acknowledgements
end
A-->>U: Return local path to exported file
sequenceDiagram
autonumber
actor U as User (UI)
participant APP as Expensify App
participant E as Expensify API
U->>APP: Open Policy IDs dropdown
APP->>APP: listPolicies()
APP->>E: POST (inputSettings.type = "policyList")
E-->>APP: Policy list [{id,name},...]
APP-->>U: Render options (label=name, value=id)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks (2 passed, 3 warnings)❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/expensify/app/expensify.app.ts (1)
65-77: Bug: Invalid HTTP method in updateCustomer().
method: "update"is not a valid HTTP verb and will 4xx. Use POST like other calls.Apply:
- async updateCustomer({ - $, data, - }) { - return this._makeRequest({ - method: "update", - data: { - type: "create", - inputSettings: { - type: "expenses", - ...data, - }, - }, - }, $); - }, + async updateCustomer({ $, data }) { + return this._makeRequest({ + method: "post", + data: { + type: "create", + inputSettings: { + type: "expenses", + ...data, + }, + }, + }, $); + },
🧹 Nitpick comments (8)
components/expensify/app/expensify.app.ts (1)
106-116: listPolicies shape — LGTM.
Straightforward wrapper around _makeRequest. Consider surfacing API errors with a user-friendly message in the future.components/expensify/actions/export-report/export-report.ts (7)
12-12: Description mismatches capability (not PDF-only).
Align with supported formats to avoid user confusion.- description: "Export a report to a PDF file. [See the documentation](https://integrations.expensify.com/Integration-Server/doc/#report-exporter)", + description: "Export reports in multiple formats (csv, xls, xlsx, txt, pdf, json, xml). [See the documentation](https://integrations.expensify.com/Integration-Server/doc/#report-exporter)",
18-40: Prop copy/validation inconsistencies and typos.
- Fix typos/backticks and reflect true requirement: at least one of reportIds, startDate, approvedAfter.
- description: "The IDs of the reports to be exported. Required if `startDate` or a`pprovedAfter` are not specified.", + description: "IDs of reports to export. Provide this OR `startDate` OR `approvedAfter`.", ... - description: "The start date of the report. Format: YYYY-MM-DD. Required if `reportIds` or `approvedAfter ` are not specified.", + description: "Start date (YYYY-MM-DD). Optional. Provide this OR `reportIds` OR `approvedAfter`.", ... - description: "The end date of the report. Format: YYYY-MM-DD. Conditionally required, if either `startDate` or `approvedAfter` is more than one year ago.", + description: "End date (YYYY-MM-DD). Optional. Used with `startDate` to bound the range.", ... - description: "Filters out all reports approved before the given date, whichever occurred last (inclusive). Required if `reportIds` or `startDate` are not specified", + description: "Include reports approved on/after this date (inclusive). Optional. Provide this OR `reportIds` OR `startDate`.",Also fix grammar:
- description: "The reports will be exported from that the specified employee email", + description: "Only export reports for this employee email",
41-54: Set a safe default for fileExtension.
PR objectives call out defaults (XML for review, PDF for saving). At minimum, default to "pdf".fileExtension: { type: "string", label: "File Extension", description: "Specifies the format of the generated report", options: [ "csv", "xls", "xlsx", "txt", "pdf", "json", "xml", ], + default: "pdf", },
98-109: Email onFinish lacks subject/message support (per objectives).
Expose optional subject/message and plumb them through.emailRecipients: { type: "string[]", label: "Email Recipients", description: "People to email at the end of the export", optional: true, }, + emailSubject: { + type: "string", + label: "Email Subject", + optional: true, + }, + emailMessage: { + type: "string", + label: "Email Message", + optional: true, + }, ... if (this.emailRecipients) { onFinish.push({ actionName: "email", recipients: this.emailRecipients.join(","), + subject: this.emailSubject, + message: this.emailMessage, }); }Also applies to: 139-152
200-221: Consider reusing app._makeRequest for consistency and auth handling.
Reduces duplication and centralizes request shaping.
213-216: Template read path handling: wrap readFileSync in try/catch.
Avoids action failure with opaque error if path is wrong.- template: fs.readFileSync(this.templatePath.includes("tmp/") - ? this.templatePath - : `/tmp/${this.templatePath}`, "utf8"), + template: (() => { + try { + return fs.readFileSync( + this.templatePath.includes("tmp/") ? this.templatePath : `/tmp/${this.templatePath}`, + "utf8", + ); + } catch (err) { + throw new ConfigurationError(`Failed to read template at "${this.templatePath}": ${err?.message || err}`); + } + })(),
154-192: Rate limiting not enforced.
Objectives specify 5 req/10s, 20 req/60s. Add a simple limiter to guard export + download bursts.I can propose a lightweight token-bucket or Bottleneck-based limiter integrated in the app wrapper so all actions inherit it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/expensify/actions/create-expense/create-expense.ts(1 hunks)components/expensify/actions/export-report-to-pdf/export-report-to-pdf.ts(1 hunks)components/expensify/actions/export-report/export-report.ts(1 hunks)components/expensify/app/expensify.app.ts(2 hunks)
🔇 Additional comments (5)
components/expensify/actions/create-expense/create-expense.ts (1)
6-6: Version bump only — LGTM.
No behavioral change. Thanks for keeping versions in sync.components/expensify/actions/export-report-to-pdf/export-report-to-pdf.ts (1)
7-7: Version bump only — LGTM.
No functional diffs observed.components/expensify/app/expensify.app.ts (1)
8-24: Dynamic policy options — LGTM.
Options mapping and fallback to [] look good.components/expensify/actions/export-report/export-report.ts (2)
128-133: syncDir is never used; write to it when provided. Also remove unnecessary await on writeFileSync.
Ensures files persist via Pipedream’s synced directory and avoids blocking issues being mislabeled async.- syncDir: { + syncDir: { type: "dir", accessMode: "write", sync: true, }, ... - const path = `/tmp/${fileName}`; - - await fs.writeFileSync(path, fileBuffer); + const outDir = this.syncDir || "/tmp"; + const path = `${outDir}/${fileName}`; + fs.writeFileSync(path, fileBuffer);Also applies to: 228-234
⛔ Skipped due to learnings
Learnt from: js07 PR: PipedreamHQ/pipedream#17375 File: components/tinypng/actions/convert-image/convert-image.mjs:33-37 Timestamp: 2025-07-01T16:56:20.177Z Learning: 'dir' props with sync functionality do not expose a path to the sync directory directly. For files to be synced, they must be either: (1) written to `process.env.STASH_DIR` (`/tmp/__stash`), or (2) written to `/tmp` and have their file path returned from the `run` method as a string, `[filepath]`, `[_, filepath]`, `{ filePath }`, `{ filepath }`, or `{ path }`.Learnt from: js07 PR: PipedreamHQ/pipedream#17375 File: components/tinypng/actions/compress-image/compress-image.mjs:18-23 Timestamp: 2025-07-01T17:01:46.327Z Learning: In TinyPNG compress-image action (components/tinypng/actions/compress-image/compress-image.mjs), the syncDir property uses accessMode: "read" because this action only reads input files and returns API responses without writing files to /tmp, unlike other TinyPNG actions that save processed files to disk.
165-185: Ignore the suggested filter relocation—employeeEmail,reportStates, andlimitare correctly defined at the root ofinputSettingsper Expensify’s documentation.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
components/expensify/actions/export-report/export-report.ts (2)
55-60: Use filters.markedAsExported (string label), not a boolean.
Replace markAsExportedFilter with markedAsExported:string per Expensify exporter.- markAsExportedFilter: { - type: "boolean", - label: "Mark as Exported (Filter)", - description: "Filters out reports that have already been exported with that label out", - optional: true, - }, + markedAsExported: { + type: "string", + label: "Marked as Exported (Filter)", + description: "Exclude reports already marked exported with this label (e.g., “Expensify Export”).", + optional: true, + },
116-121: limit should be integer, not string.
Matches API expectation; avoid coercion.- limit: { - type: "string", + limit: { + type: "integer", label: "Limit", description: "Maximum number of reports to export", optional: true, },
🧹 Nitpick comments (13)
components/expensify/app/expensify.app.ts (2)
27-27: Tiny copy tweak (optional).
“The expenses will be created in this account” → “Create expenses in this account” reads cleaner.- description: "The expenses will be created in this account", + description: "Create expenses in this account",
170-180: Deduplicate listPolicies by reusing getPolicyList (optional).
Avoids a second “policyList” call path.- async listPolicies({ $ = this } = {}) { - return this._makeRequest({ - method: "post", - data: { - type: "get", - inputSettings: { - type: "policyList", - }, - }, - }, $); - }, + async listPolicies({ $ = this, userEmail, adminOnly = true } = {}) { + return this.getPolicyList({ $, userEmail, adminOnly }); + },components/expensify/actions/export-report/export-report.ts (11)
12-12: Action description mentions only PDF but supports many formats.
Generalize to reduce confusion.- description: "Export a report to a PDF file. [See the documentation](https://integrations.expensify.com/Integration-Server/doc/#report-exporter)", + description: "Export Expensify report(s) in CSV, XLS, XLSX, TXT, PDF, JSON, or XML. [Docs](https://integrations.expensify.com/Integration-Server/doc/#report-exporter)",
17-22: Fix typos in validation text.
Backtick in “a`pprovedAfter” and wording.- description: "The IDs of the reports to be exported. Required if `startDate` or a`pprovedAfter` are not specified.", + description: "The IDs of the reports to export. Required if `startDate` or `approvedAfter` are not specified.",
24-34: Minor copy edits (optional).
Tighten language; remove stray backtick.- description: "The start date of the report. Format: YYYY-MM-DD. Required if `reportIds` or `approvedAfter ` are not specified.", + description: "Start date (YYYY-MM-DD). Required if neither `reportIds` nor `approvedAfter` are specified.",- description: "The end date of the report. Format: YYYY-MM-DD. Conditionally required, if either `startDate` or `approvedAfter` is more than one year ago.", + description: "End date (YYYY-MM-DD). Required if either `startDate` or `approvedAfter` is more than one year ago.",
41-54: Set a default file format (optional).
Defaults help; PDF is a sensible default per PR objectives.fileExtension: { type: "string", label: "File Extension", description: "Specifies the format of the generated report", + default: "pdf", options: [
110-115: Constrain templatePath to /tmp (optional hardening).
Normalize/verify the path resolves inside /tmp to avoid unintended reads.Add:
import path from "path";Then resolve:
const resolvedTemplatePath = this.templatePath.includes("tmp/") ? this.templatePath : path.join("/tmp", this.templatePath); const template = fs.readFileSync(resolvedTemplatePath, "utf8");
128-133: syncDir prop is unused directly; clarify or write to it (optional).
Either write the file into syncDir or document it’s only enabling /tmp sync.Option A — write to syncDir:
syncDir: { type: "dir", accessMode: "write", sync: true, },And below:
- const path = `/tmp/${fileName}`; + const path = `${this.syncDir}/${fileName}`;Option B — rename label/description to clarify its “enable /tmp sync” purpose.
200-207: Consistent headers (optional).
Set Content-Type for both branches for parity (platform usually infers it).fileName = await axios($, { ...args, data: qs.stringify({ requestJobDescription: JSON.stringify(data), template: "default", }), + headers: { "Content-Type": "application/x-www-form-urlencoded" }, });
228-231: Don’t await a sync write; or use async write.
Remove await or switch to fs.promises.writeFile.- await fs.writeFileSync(path, fileBuffer); + fs.writeFileSync(path, fileBuffer);
232-236: Summary wording (optional).
“In” → “to”.- $.export("$summary", `Successfully exported report in ${path}`); + $.export("$summary", `Successfully exported report to ${path}`);
135-137: Validation is good; consider enforcing 1-year window (optional).
Optionally guard date ranges > 1 year to preempt API errors.I can add a small validator that checks startDate/approvedAfter against endDate/now and throws a ConfigurationError if > 365 days.
154-164: Credentials placement is OK but consider using app _makeRequest for consistency (optional).
Would centralize timeouts/retries and error handling.I can refactor this action to call a new app method (e.g., exportReport) that wraps the exporter call and reuses _makeRequest.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/expensify/actions/create-expense/create-expense.ts(1 hunks)components/expensify/actions/create-report/create-report.ts(1 hunks)components/expensify/actions/export-report-to-pdf/export-report-to-pdf.ts(1 hunks)components/expensify/actions/export-report/export-report.ts(1 hunks)components/expensify/app/expensify.app.ts(2 hunks)components/expensify/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- components/expensify/package.json
- components/expensify/actions/create-report/create-report.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/expensify/actions/export-report-to-pdf/export-report-to-pdf.ts
⏰ 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). (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
components/expensify/actions/create-expense/create-expense.ts (1)
6-6: Version bump only — LGTM.
No behavioral changes; safe to ship.components/expensify/app/expensify.app.ts (2)
9-23: New policyExportIds prop + dynamic options — looks good.
Options map id/name correctly via listPolicies(); sensible default to [].
33-33: Policy ID description — LGTM.
Clearer wording.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/expensify/actions/export-report/export-report.ts (2)
222-225: Improve template file path handling.The current logic for detecting
/tmp/in the path can be error-prone. Consider using a more explicit approach.- template: fs.readFileSync(this.templatePath.includes("tmp/") - ? this.templatePath - : `/tmp/${this.templatePath}`, "utf8"), + template: fs.readFileSync(this.templatePath.startsWith("/tmp/") + ? this.templatePath + : `/tmp/${this.templatePath}`, "utf8"),
239-239: Use async/await instead of sync file operations.Using
fs.writeFileSyncblocks the event loop. Consider using the async version for better performance.- await fs.writeFileSync(path, fileBuffer); + await fs.promises.writeFile(path, fileBuffer);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/expensify/actions/export-report/export-report.ts(1 hunks)
⏰ 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). (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/expensify/actions/export-report/export-report.ts (1)
116-121: Use integer type for limit.
The API expects a number; using string risks coercion issues.- limit: { - type: "string", + limit: { + type: "integer", label: "Limit", description: "Maximum number of reports to export", optional: true, },
🧹 Nitpick comments (2)
components/expensify/actions/export-report/export-report.ts (2)
148-154: Support optional email message in onFinish email.
Objectives mention an email message; add a prop and pass it.if (this.emailRecipients) { - onFinish.push({ - actionName: "email", - recipients: this.emailRecipients.join(","), - }); + onFinish.push({ + actionName: "email", + recipients: this.emailRecipients.join(","), + message: this.emailMessage, + }); }Additional prop (near Lines 98–109):
emailRecipients: { type: "string[]", label: "Email Recipients", description: "People to email at the end of the export", optional: true, }, + emailMessage: { + type: "string", + label: "Email Message", + description: "Optional message to include in the on-finish email", + optional: true, + },
204-207: Set form-encoding header for both code paths.
Be consistent by moving the header to args and removing the duplicate.const args = { method: "post", url: `${this.expensify._apiUrl()}`, + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, };- headers: { - "Content-Type": "application/x-www-form-urlencoded", - },Also applies to: 226-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/expensify/actions/export-report/export-report.ts(1 hunks)
⏰ 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). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (6)
components/expensify/actions/export-report/export-report.ts (6)
26-26: Clarify conditional requirement; remove stray space.
[ suggest_nitpick ]- description: "The start date of the report. Format: YYYY-MM-DD. Required if `reportIds` or `approvedAfter ` are not specified.", + description: "The start date of the report. Format: YYYY-MM-DD. Required if both `reportIds` and `approvedAfter` are not specified.",
38-38: Clarify conditional requirement punctuation and conjunction.
[ suggest_nitpick ]- description: "Filters out all reports approved before the given date, whichever occurred last (inclusive). Required if `reportIds` or `startDate` are not specified", + description: "Filters out all reports approved before the given date, whichever occurred last (inclusive). Required if both `reportIds` and `startDate` are not specified.",
61-73: Report state input + mapping LGTM.
Array input with join to inputSettings.reportState is UX-friendly and correct.
74-79: Employee email field LGTM.
Grammar looks good and placement under inputSettings is correct.
195-198: Correct casing for fileBasename LGTM.
Expensify expects outputSettings.fileBasename; mapping from fileBaseName prop is correct.
239-239: Avoid awaiting a synchronous write; use async fs instead.
[ suggest_optional_refactor ]- await fs.writeFileSync(path, fileBuffer); + await fs.promises.writeFile(path, fileBuffer);
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/expensify/actions/export-report/export-report.ts (1)
218-230: Critical security vulnerability: prevent path traversal attacks.The current template path handling using
includes("tmp/")is vulnerable to directory traversal attacks and should be hardened with proper path validation.} else { + const safePath = path.resolve("/tmp", this.templatePath.replace(/^\/+/, "")); + if (!safePath.startsWith(path.resolve("/tmp") + path.sep)) { + throw new ConfigurationError("Template path must be within /tmp directory"); + } fileName = await axios($, { ...args, data: qs.stringify({ requestJobDescription: JSON.stringify(data), - template: fs.readFileSync(this.templatePath.includes("tmp/") - ? this.templatePath - : `/tmp/${this.templatePath}`, "utf8"), + template: fs.readFileSync(safePath, "utf8"), }), headers: { "Content-Type": "application/x-www-form-urlencoded", }, });
🧹 Nitpick comments (1)
components/expensify/actions/export-report/export-report.ts (1)
242-242: Improve summary message formatting.Based on retrieved learnings about proper summary message formatting, ensure consistent and clear success messages.
- $.export("$summary", `Successfully exported report in ${path}`); + $.export("$summary", `Successfully exported report to ${path}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/expensify/actions/export-report/export-report.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-04T18:11:59.822Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#12731
File: components/hackerone/actions/get-members/get-members.mjs:3-28
Timestamp: 2024-07-04T18:11:59.822Z
Learning: When exporting a summary message in the `run` method of an action, ensure the message is correctly formatted. For example, in the `hackerone-get-members` action, the correct format is `Successfully retrieved ${response.data.length} members`.
Applied to files:
components/expensify/actions/export-report/export-report.ts
⏰ 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). (4)
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
Resolves #18191
Summary by CodeRabbit
New Features
Chores