Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe pull request removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the project to a newer @billos/firefly-iii-sdk version and removes the axios dependency by migrating HTTP calls to the built-in fetch, while also tightening dependency installation by ignoring lifecycle scripts in CI and the runtime Docker image.
Changes:
- Bump
@billos/firefly-iii-sdkto6.5.5-sdk.2and update client configuration accordingly. - Replace
axiosusage withfetchin the auto-import job and notifier implementations (Discord/Gotify), and adjust unit tests. - Harden installs by adding
--ignore-scriptsin GitHub Actions and the runtime Docker image.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Updates lockfile to reflect SDK bump and removal of axios-related deps. |
package.json |
Bumps @billos/firefly-iii-sdk and removes axios dependency. |
src/client.ts |
Updates SDK client option naming (baseURL → baseUrl). |
src/queues/jobs/autoImport.ts |
Switches importer trigger from axios POST to fetch. |
src/__tests__/autoImport.test.ts |
Updates auto-import job tests to mock fetch instead of axios. |
src/modules/notifiers/discord.ts |
Replaces axios webhook calls with fetch. |
src/modules/notifiers/gotify.ts |
Replaces axios Gotify calls with fetch and adds basic error handling in one path. |
Dockerfile |
Adds apk update and installs prod deps with --ignore-scripts. |
.github/workflows/ci.yml |
Installs deps with npm i --ignore-scripts. |
.github/workflows/release.yml |
Installs deps with npm i --ignore-scripts. |
.github/workflows/coverage-pages.yml |
Installs deps with npm i --ignore-scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/autoImport.test.ts (1)
4-28:⚠️ Potential issue | 🔴 CriticalFix global
fetchmocking setup (currently breaks all tests).
vi.mock("fetch")is invalid—fetchis a global, not a module. This causesvi.mocked(fetch).mockResolvedValue(...)to fail with "mockResolvedValue is not a function" at line 24 becausevi.mocked()returns the real fetch, which has no mock methods.Use
vi.stubGlobal("fetch", vi.fn())instead, and addvi.unstubAllGlobals()to the afterEach cleanup.Suggested fix
-vi.mock("fetch") @@ beforeEach(() => { - vi.mocked(fetch).mockResolvedValue({ - ok: true, - status: 200, - json: async () => ({}), - } as Response) + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: async () => ({}), + } as Response), + ) }) @@ afterEach(() => { + vi.unstubAllGlobals() vi.resetModules() vi.unstubAllEnvs() vi.clearAllMocks() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/autoImport.test.ts` around lines 4 - 28, Replace the invalid module mock for the global fetch with a global stub: remove vi.mock("fetch") and instead call vi.stubGlobal("fetch", vi.fn()) before the tests (e.g., at top-level) so vi.mocked(fetch) will return a mocked fn; in the test lifecycle, keep the beforeEach that sets vi.mocked(fetch).mockResolvedValue(...) but add an afterEach that calls vi.unstubAllGlobals() to restore the real global fetch; update references around the beforeEach/afterEach and any uses of vi.mocked(fetch) accordingly (look for vi.mocked(fetch), beforeEach, afterEach, vi.stubGlobal, vi.unstubAllGlobals).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 13: The Dockerfile currently runs "RUN apk update && apk add tzdata"
which leaves package index caches and increases image size; replace that RUN
invocation to use the no-cache option (i.e., use apk add --no-cache tzdata) so
the package index isn't stored in the image and image layers stay small; update
the RUN line that currently contains "apk update && apk add tzdata" accordingly.
In `@src/modules/notifiers/discord.ts`:
- Around line 18-28: The override sendMessageImpl in the Discord notifier uses a
single parameter so it treats the passed-in title as the message body, breaking
the AbstractNotifier.sendMessageImpl(title, content) contract; update the
Discord implementation to match the two-argument signature (e.g., override async
sendMessageImpl(title: string, content: string): Promise<string>) and build the
posted payload using both values (for example combine title and content into the
JSON body so the webhook posts both the title and the message body), keeping the
method name sendMessageImpl and returning the created message id as before.
- Around line 11-15: The fetch calls in notifyImpl and deleteMessageImpl that
POST to `${env.discordWebhook}?wait=true` lack response.ok checks like
sendMessageImpl; update both functions (notifyImpl and deleteMessageImpl) to
capture the fetch response, check response.ok, and handle non-OK responses
(throw or log with response status/text) so HTTP 4xx/5xx don't silently succeed;
for deleteMessageImpl specifically, only unset or remove the stored message ID
after a successful (response.ok) delete to avoid losing the ID on failed
deletes.
In `@src/modules/notifiers/gotify.ts`:
- Around line 53-55: The fetch calls in deleteAllMessagesImpl and
hasMessageIdImpl use relative URLs which fail in Node.js; update both fetch
invocations to use absolute URLs by prepending env.gotifyUrl (same pattern as
notifyImpl/sendMessageImpl), and also ensure hasMessageIdImpl does not swallow
errors—propagate or return false only after logging the error so
deleteMessageImpl can correctly decide to attempt deletions; locate these
changes in the functions deleteAllMessagesImpl, hasMessageIdImpl, and where
deleteMessageImpl relies on hasMessageIdImpl.
- Around line 20-24: notifyImpl and deleteMessageImpl are calling fetch but not
checking HTTP status; mirror sendMessageImpl by capturing the fetch response,
verify response.ok, and throw or return an error when not ok (include
response.status/text in the error message) so 4xx/5xx are surfaced;
additionally, in deleteMessageImpl do not unset the stored message ID before
performing the fetch—only clear the stored ID after a successful response.ok to
allow retries if deletion fails; reference the functions notifyImpl,
deleteMessageImpl and the existing sendMessageImpl guard pattern to implement
these checks.
In `@src/queues/jobs/autoImport.ts`:
- Around line 53-54: The fetch call using url is not checking HTTP status so
logger.info and the subsequent notifications run even on 4xx/5xx; update the
code that calls await fetch(url, { method: "POST" }) to capture the response
(const res = await fetch(...)), check res.ok, and only run logger.info and the
success notification when res.ok is true; on non-ok responses log a descriptive
error via logger.error including res.status and await res.text() (or JSON) and
surface or throw an error so callers/queues know the job failed; reference the
fetch call, the url variable, logger (logger.info/logger.error), and the
notification code so they are gated behind the res.ok check.
---
Outside diff comments:
In `@src/__tests__/autoImport.test.ts`:
- Around line 4-28: Replace the invalid module mock for the global fetch with a
global stub: remove vi.mock("fetch") and instead call vi.stubGlobal("fetch",
vi.fn()) before the tests (e.g., at top-level) so vi.mocked(fetch) will return a
mocked fn; in the test lifecycle, keep the beforeEach that sets
vi.mocked(fetch).mockResolvedValue(...) but add an afterEach that calls
vi.unstubAllGlobals() to restore the real global fetch; update references around
the beforeEach/afterEach and any uses of vi.mocked(fetch) accordingly (look for
vi.mocked(fetch), beforeEach, afterEach, vi.stubGlobal, vi.unstubAllGlobals).
🪄 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: b2d638d1-0759-47a4-b64e-4b40707b0175
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/coverage-pages.yml.github/workflows/release.ymlDockerfilepackage.jsonsrc/__tests__/autoImport.test.tssrc/client.tssrc/modules/notifiers/discord.tssrc/modules/notifiers/gotify.tssrc/queues/jobs/autoImport.ts
Bumps `@billos/firefly-iii-sdk` to version `6.5.5-sdk.2`. This update includes a change in the client configuration, where the `baseURL` property has been renamed to `baseUrl`. Adjusts client initialization to align with the new property name.
Replaces the `axios` library with the built-in `fetch` API for all HTTP requests. This change reduces project dependencies and leverages native platform capabilities. It impacts the Discord and Gotify notifiers, the auto-import job, and their associated unit tests. Cleans up `axios` and its transitive dependencies from the project.
Ensures external API calls using `fetch` explicitly check for non-successful HTTP responses and throw detailed errors when requests fail. This improves the reliability and provides clearer diagnostics for integrations with Discord, Gotify, and the auto-import service.
Replaces previous `fetch` mocking approach with `vi.stubGlobal` in auto import tests. This simplifies and standardizes how the global `fetch` API is stubbed for test execution.
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit