Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds localization support, updates existing front-end blocks to use translated labels and shared button layout, and introduces notifier configuration state, endpoints, runtime resolution, and UI for selecting and editing notifier settings. ChangesLocalization, shared UI, and existing block updates
Notifier configuration plumbing, runtime, and UI
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/molecules/ButtonList.vue (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify static class binding.
The class list is static, so wrapping it in an array via
:class="[...]"is unnecessary; a plainclassattribute is simpler and avoids an unneeded reactive binding.♻️ Proposed simplification
- <div :class="['flex flex-1 flex-row flex-wrap gap-2 justify-center items-start']"> + <div class="flex flex-1 flex-row flex-wrap gap-2 justify-center items-start">🤖 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 `@frontend/molecules/ButtonList.vue` at line 4, The class binding in ButtonList.vue is static, so remove the unnecessary Vue binding from the root <div> and use a plain class attribute instead. Update the template where the flex layout classes are defined so the ButtonList component no longer creates an unneeded reactive :class array.frontend/organisms/AboutBlock.vue (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOverly generic i18n keys risk collisions.
name,version,author,license,repositoryare single common-word keys, unlike the descriptive/namespaced convention used elsewhere in this PR (title_about,desc_show_hide_categories,label_current_account, etc.). Generic keys like these are likely to be reused/overwritten by unrelated features needing a translation for "name" or "version" in a different context.♻️ Suggested namespacing
- <LabeledValue :label="$t('name')" :value="props.about.name" /> - <LabeledValue :label="$t('version')" :value="props.about.version" /> - <LabeledValue :label="$t('author')" :value="props.about.author" /> - <LabeledValue :label="$t('license')" :value="props.about.license" /> - <LabeledValue :label="$t('repository')" :value="props.about.repository" :link="true" /> + <LabeledValue :label="$t('about_name')" :value="props.about.name" /> + <LabeledValue :label="$t('about_version')" :value="props.about.version" /> + <LabeledValue :label="$t('about_author')" :value="props.about.author" /> + <LabeledValue :label="$t('about_license')" :value="props.about.license" /> + <LabeledValue :label="$t('about_repository')" :value="props.about.repository" :link="true" />🤖 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 `@frontend/organisms/AboutBlock.vue` around lines 15 - 19, The AboutBlock.vue labels are using overly generic translation keys that can collide with other features. Update the LabeledValue bindings in AboutBlock to use descriptive, namespaced i18n keys consistent with the rest of the PR, and then add or rename the corresponding entries in the translation files so the keys remain unique and context-specific.frontend/organisms/SchedulesBlock.vue (1)
19-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused helper and avoid shadowing
isHidden
label()is unused in this file; drop it if nothing else references it.- Rename the local
isHiddeninsidebackground()or call the helper directly to avoid the collision.🤖 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 `@frontend/organisms/SchedulesBlock.vue` around lines 19 - 39, The SchedulesBlock.vue helpers include dead code and a name collision: remove the unused label() helper if nothing references it, and in background() avoid shadowing the existing isHidden(name) helper by renaming the local variable or using the helper directly. Keep the hidden-budget check behavior the same while making the function names clear and non-conflicting.
🤖 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 `@frontend/locales/fr-FR.json`:
- Line 21: The desc_schedules entry in the French locale does not preserve the
English meaning about the cron schedule format. Update the translation in the
fr-FR JSON locale so it explicitly describes the cron pattern guidance shown to
users, matching the intent of the source string rather than only saying to
manage scheduled actions.
In `@frontend/main.js`:
- Around line 5-17: The i18n setup in createI18n currently only registers French
messages under fr-FR, so navigator.language values like fr or fr-CA fall back to
English. Update the locale mapping in main.js by either adding fr to the
messages object with the same fr_FR bundle or normalizing navigator.language to
fr-FR before passing it into createI18n, so French variants resolve correctly.
In `@frontend/organisms/CategoryBlock.vue`:
- Around line 39-55: The ActionButton label in CategoryBlock is now empty for
categories missing attributes.name, so restore a fallback label in the v-for
over props.config.categories. Update the :label binding to use a translated
placeholder when category.attributes?.name is absent, keeping it consistent with
the existing background(category) guard and the category.id-based action/key
usage.
In `@frontend/organisms/SchedulesBlock.vue`:
- Around line 64-79: The budget list in SchedulesBlock.vue is using a non-unique
Vue key based on value.attributes.name, which can collide when names repeat.
Update the v-for key on the ActionButton to use value.id instead, since it is
already available and unique, and keep the rest of the budget rendering logic
unchanged.
---
Nitpick comments:
In `@frontend/molecules/ButtonList.vue`:
- Line 4: The class binding in ButtonList.vue is static, so remove the
unnecessary Vue binding from the root <div> and use a plain class attribute
instead. Update the template where the flex layout classes are defined so the
ButtonList component no longer creates an unneeded reactive :class array.
In `@frontend/organisms/AboutBlock.vue`:
- Around line 15-19: The AboutBlock.vue labels are using overly generic
translation keys that can collide with other features. Update the LabeledValue
bindings in AboutBlock to use descriptive, namespaced i18n keys consistent with
the rest of the PR, and then add or rename the corresponding entries in the
translation files so the keys remain unique and context-specific.
In `@frontend/organisms/SchedulesBlock.vue`:
- Around line 19-39: The SchedulesBlock.vue helpers include dead code and a name
collision: remove the unused label() helper if nothing references it, and in
background() avoid shadowing the existing isHidden(name) helper by renaming the
local variable or using the helper directly. Keep the hidden-budget check
behavior the same while making the function names clear and non-conflicting.
🪄 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: f1280904-3f74-4ca4-8076-9810880cb5ad
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
frontend/locales/en.jsonfrontend/locales/fr-FR.jsonfrontend/main.jsfrontend/molecules/ButtonList.vuefrontend/molecules/CronInput.vuefrontend/organisms/AboutBlock.vuefrontend/organisms/CategoryBlock.vuefrontend/organisms/ControlBlock.vuefrontend/organisms/CurrentAccountsBlock.vuefrontend/organisms/RolesBlock.vuefrontend/organisms/SchedulesBlock.vuepackage.json
| import en from "./locales/en.json" | ||
| import fr_FR from "./locales/fr-FR.json" | ||
|
|
||
| createApp(App).mount("#app") | ||
| const locale = navigator.language | ||
| // const locale = "fr" | ||
| const i18n = createI18n({ | ||
| locale, | ||
| fallbackLocale: "en", | ||
| messages: { | ||
| en, | ||
| "fr-FR": fr_FR, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does vue-i18n implicit locale fallback work from base language to a territory-specific variant (e.g. "fr" resolving messages registered only under "fr-FR")?
💡 Result:
No, the implicit fallback in Vue I18n works in the opposite direction of what you described [1][2]. Vue I18n's implicit fallback mechanism automatically resolves from a territory-specific locale to its base language, not from a base language to a specific territory [1][2]. For example, if your locale is set to "fr-FR" and a translation key is missing, Vue I18n will automatically attempt to fall back to "fr" [2]. It does not automatically resolve from a base language (like "fr") to a territory-specific variant (like "fr-FR") because "fr" is considered more general than "fr-FR" [2]. If you require "fr" to access messages registered under "fr-FR", you must explicitly configure a fallback chain using the fallbackLocale option [2]. You can define a decision map to specify exactly how locales should chain together: const i18n = createI18n({ locale: 'fr', fallbackLocale: { 'fr': ['fr-FR'], 'default': ['en'] }, //... }) In this configuration, when a key is missing in "fr", Vue I18n will look for it in "fr-FR" before proceeding to the default fallback [2].
Citations:
- 1: https://vue-i18n.intlify.dev/guide/essentials/fallback
- 2: https://kazupon.github.io/vue-i18n/guide/fallback.html
🌐 Web query:
vue-i18n locale fallback chain base language territory-specific variant fr fr-FR documentation
💡 Result:
In vue-i18n, when a locale string includes a territory (e.g., fr-FR), the library automatically activates an implicit fallback mechanism [1][2]. The fallback behavior for a locale with a territory follows a specific chain: 1. Implicit Fallback: If you set the locale to fr-FR, vue-i18n will automatically attempt to resolve keys by first checking fr-FR, then falling back to the base language, fr [1][2]. 2. Explicit Fallback: If the key is not found in the base language (fr), the system will then proceed to the locale(s) defined in your fallbackLocale configuration [1]. For example, if locale is set to fr-FR and fallbackLocale is set to en, the full lookup chain is: fr-FR -> fr -> en [1]. Key technical details: - Automatic Implicit Fallback: This process occurs automatically for any locale string containing a hyphen-separated territory or dialect [1][2]. - Suppressing Implicit Fallback: You can disable this implicit fallback behavior for a specific locale by appending an exclamation mark (!) to the locale string (e.g., fr-FR!) [1]. - Fallback Configuration: The fallbackLocale option can accept a single string, an array of strings (for multiple fallback priorities), or a complex decision map for advanced routing [3][1]. The underlying implementation uses a fallback chain algorithm that parses the locale string by splitting it at hyphens and iteratively stripping the last segment to generate the fallback sequence [4].
Citations:
- 1: https://kazupon.github.io/vue-i18n/guide/fallback.html
- 2: https://vue-i18n.intlify.dev/api/composition
- 3: https://vue-i18n.intlify.dev/guide/essentials/fallback
- 4: https://github.com/intlify/vue-i18n/blob/2d4d2a342f8bae134665a0b7cd945fb8b638839a/packages/core-base/src/fallbacker.ts
🌐 Web query:
site:vue-i18n.intlify.dev locale fallback chain territory language base locale fr-FR fr documentation
💡 Result:
In the context of software localization (specifically frameworks like Vue I18n), the locale fallback chain is a mechanism used to resolve missing translations by moving from a specific locale to more general ones [1]. For a locale string like fr-FR (where fr is the language and FR is the territory), the automatic fallback chain typically proceeds as follows: 1. fr-FR (Specific locale) 2. fr (Base locale/Language) When a translation key is missing in the primary locale (e.g., fr-FR), the system automatically attempts to resolve it using the base language (fr) before eventually falling back to a globally configured fallback locale (such as en) if provided [1]. This process is often referred to as an implicit fallback [1][2][3]. If a locale string contains both a territory and a dialect (e.g., de-DE-bavarian), the chain extends further: de-DE-bavarian > de-DE > de [1]. To stop this automatic implicit fallback, developers can often append an exclamation mark (e.g., fr-FR!) to the locale string [1]. The fallback behavior can also be explicitly customized by providing an array of locales or a decision map in the configuration [1].
Citations:
- 1: https://vue-i18n.intlify.dev/guide/essentials/fallback
- 2: https://vue-i18n.intlify.dev/api/legacy
- 3: https://vue-i18n.intlify.dev/api/composition
🌐 Web query:
site:vue-i18n.intlify.dev fallbackLocale locale chain 'fr-FR' 'fr' vue-i18n
💡 Result:
In vue-i18n, the locale fallback mechanism is designed to handle hierarchical or chained fallbacks automatically [1][2]. For a locale like 'fr-FR', vue-i18n includes an implicit fallback mechanism [3][4]. If a translation key is missing for 'fr-FR', the system will automatically attempt to resolve it by stripping the region/dialect information, effectively falling back to 'fr' [3][4]. To explicitly define a fallback chain, you can configure the fallbackLocale option in your createI18n instance [5][6]. 1. Implicit Fallback: If you set your locale to 'fr-FR', vue-i18n natively attempts to resolve keys using the 'fr' locale if they are not found in 'fr-FR' [3][4]. 2. Explicit Fallback Chain: If you need to specify a more complex or custom chain (e.g., if 'fr' is also missing, you want to fall back to 'en'), you can provide an array to fallbackLocale [2]: const i18n = createI18n({ locale: 'fr-FR', fallbackLocale: ['fr', 'en'], // Try 'fr', then 'en' if 'fr' is missing messages: { 'fr-FR': {... }, 'fr': {... }, 'en': {... } } }) The default behavior, known as the fallbackWithLocaleChain algorithm, ensures that the system traverses from the specific locale to the base language and then to the specified fallback locales [1][2].
Citations:
- 1: https://vue-i18n.intlify.dev/api/general
- 2: https://vue-i18n.intlify.dev/guide/advanced/lite
- 3: https://vue-i18n.intlify.dev/api/composition
- 4: https://vue-i18n.intlify.dev/api/legacy
- 5: https://vue-i18n.intlify.dev/guide/essentials/fallback
- 6: https://vue-i18n.intlify.dev/guide/essentials/started
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'frontend/main.js\n'
if [ -f frontend/main.js ]; then
cat -n frontend/main.js | sed -n '1,40p'
fiRepository: Billos/Sparkleft
Length of output: 721
Map French messages to fr as well as fr-FR. navigator.language can be fr or fr-CA, and vue-i18n won’t resolve those to fr-FR; they’ll fall through to English here. Register fr: fr_FR or normalize the locale before creating i18n.
🤖 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 `@frontend/main.js` around lines 5 - 17, The i18n setup in createI18n currently
only registers French messages under fr-FR, so navigator.language values like fr
or fr-CA fall back to English. Update the locale mapping in main.js by either
adding fr to the messages object with the same fr_FR bundle or normalizing
navigator.language to fr-FR before passing it into createI18n, so French
variants resolve correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/organisms/GotifyBlock.vue (1)
17-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepetitive TextInput blocks could be data-driven.
The four TextInput usages differ only by label/placeholder/action/model path. Consider iterating over a small array of field descriptors to reduce duplication, since more Gotify fields (or other providers) will likely follow the same shape.
🤖 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 `@frontend/organisms/GotifyBlock.vue` around lines 17 - 49, The GotifyBlock component repeats four nearly identical TextInput blocks; make this data-driven by defining a small field-descriptor array and rendering it with a loop in GotifyBlock.vue. Keep the existing bindings to props.config.token, props.config.notifierConfig, and the update:config emit, but move the varying label, placeholder, action, and config key into the descriptor so the template is easier to extend for future Gotify fields.frontend/organisms/NotifiersBlock.vue (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded "Not implemented yet" string.
Inconsistent with the rest of this component, which uses
$t(...)for header/subtitle text.🤖 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 `@frontend/organisms/NotifiersBlock.vue` at line 42, The fallback text in NotifiersBlock.vue is hardcoded and should use localization like the rest of the component. Update the v-else branch in NotifiersBlock to replace the literal “Not implemented yet” with a $t(...) lookup, matching the existing translation pattern used for the header and subtitle text.frontend/molecules/TextInput.vue (2)
17-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLabel not programmatically associated with the input.
The label is a plain
<div>with nofor/idlinkage to the<input>, reducing screen-reader usability for this reusable field.🤖 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 `@frontend/molecules/TextInput.vue` around lines 17 - 30, The TextInput component’s label is only rendered as a plain div, so it is not programmatically tied to the input for assistive tech. Update the TextInput.vue template to use a real label element (or equivalent accessible association) connected to the input via matching for/id values, and ensure the component exposes/uses a stable unique id alongside the existing label and model bindings.
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded "Save" label breaks i18n consistency.
Every other new label in this cohort (GotifyBlock, NotifiersBlock) uses
$t(...). This shared, reused component hardcodes "Save" in English for all consumers.🤖 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 `@frontend/molecules/TextInput.vue` at line 35, The TextInput.vue component has a hardcoded English label "Save" on the label prop which breaks internationalization consistency. Replace the hardcoded string "Save" with the appropriate `$t(...)` translation function call (using a suitable translation key like "save" or similar) to match the i18n pattern used in other components like GotifyBlock and NotifiersBlock, ensuring the label respects the user's locale settings.
🤖 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/endpoints/config.ts`:
- Around line 129-136: The config read response in the config endpoint is
echoing notifier secrets in cleartext. Update the notifierConfig object returned
by the config read flow so that `discordWebhook`, `gotifyToken`, and
`gotifyUserToken` are masked or replaced with a configured boolean, and keep raw
values only in the write path handled by `setNotifierField`; use the existing
`notifierConfig`/`notifier` response assembly to locate and adjust this
behavior.
- Around line 50-51: The config response shape is too narrow because
`configEndpoint` can return a null notifier from
`DynamicConfig.get(VConfig.Notifier)` while `Config.notifier` is typed as
`Notifiers`. Update the `Config` contract and any related mapping in
`configEndpoint` to allow `notifier` to be `Notifiers | null`, or ensure it is
always initialized before the endpoint returns. Keep the change aligned with the
existing `configEndpoint` and `DynamicConfig.get(VConfig.Notifier)` flow so the
API type matches the runtime value.
In `@src/endpoints/setNotifierField.ts`:
- Around line 17-23: The route param in setNotifierField is being typed too
narrowly as Notifiers even though it arrives as an untrusted string and is later
narrowed by isNotifierField before calling DynamicConfig.set(). Update the
Request generic in setNotifierField to use string for field, then keep the
existing runtime validation/narrowing flow so only validated values reach
DynamicConfig.set().
---
Nitpick comments:
In `@frontend/molecules/TextInput.vue`:
- Around line 17-30: The TextInput component’s label is only rendered as a plain
div, so it is not programmatically tied to the input for assistive tech. Update
the TextInput.vue template to use a real label element (or equivalent accessible
association) connected to the input via matching for/id values, and ensure the
component exposes/uses a stable unique id alongside the existing label and model
bindings.
- Line 35: The TextInput.vue component has a hardcoded English label "Save" on
the label prop which breaks internationalization consistency. Replace the
hardcoded string "Save" with the appropriate `$t(...)` translation function call
(using a suitable translation key like "save" or similar) to match the i18n
pattern used in other components like GotifyBlock and NotifiersBlock, ensuring
the label respects the user's locale settings.
In `@frontend/organisms/GotifyBlock.vue`:
- Around line 17-49: The GotifyBlock component repeats four nearly identical
TextInput blocks; make this data-driven by defining a small field-descriptor
array and rendering it with a loop in GotifyBlock.vue. Keep the existing
bindings to props.config.token, props.config.notifierConfig, and the
update:config emit, but move the varying label, placeholder, action, and config
key into the descriptor so the template is easier to extend for future Gotify
fields.
In `@frontend/organisms/NotifiersBlock.vue`:
- Line 42: The fallback text in NotifiersBlock.vue is hardcoded and should use
localization like the rest of the component. Update the v-else branch in
NotifiersBlock to replace the literal “Not implemented yet” with a $t(...)
lookup, matching the existing translation pattern used for the header and
subtitle text.
🪄 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: cfde74d4-d121-4de0-8f4f-547d9150c500
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
frontend/App.vuefrontend/locales/en.jsonfrontend/locales/fr-FR.jsonfrontend/molecules/TextInput.vuefrontend/organisms/GotifyBlock.vuefrontend/organisms/NotifiersBlock.vuesrc/endpoints/config.tssrc/endpoints/setNotifier.tssrc/endpoints/setNotifierField.tssrc/modules/config/dynamic.tssrc/modules/notifiers/types.tssrc/server.ts
✅ Files skipped from review due to trivial changes (2)
- src/modules/notifiers/types.ts
- frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/locales/fr-FR.json
- frontend/App.vue
| notifier: Notifiers | ||
| notifierConfig: NotifierConfig |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify DynamicConfig.get() return type
fd -a 'dynamic.ts' src/modules/config --exec cat -n {}Repository: Billos/Sparkleft
Length of output: 2567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the config endpoint and notifier-related types/usages
ast-grep outline src/endpoints/config.ts --view expanded
printf '\n--- FILE: src/endpoints/config.ts (relevant range) ---\n'
cat -n src/endpoints/config.ts | sed -n '1,220p'
printf '\n--- SEARCH: Notifiers definition/usages ---\n'
rg -n "export (enum|type|interface) Notifiers|Notifiers" src -g '!**/*.map'Repository: Billos/Sparkleft
Length of output: 7247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- FILE: src/modules/notifiers/types.ts ---\n'
cat -n src/modules/notifiers/types.ts
printf '\n--- FILE: src/endpoints/setNotifier.ts ---\n'
cat -n src/endpoints/setNotifier.ts
printf '\n--- SEARCH: writes to VConfig.Notifier ---\n'
rg -n "VConfig\.Notifier|\"notifier\"" src -g '!**/*.map'Repository: Billos/Sparkleft
Length of output: 2407
Make notifier nullable in the config response
DynamicConfig.get(VConfig.Notifier) can return null, and configEndpoint currently casts that to Notifiers. Expose Config.notifier as Notifiers | null (or initialize it before serving this endpoint) so the API contract matches the runtime 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/endpoints/config.ts` around lines 50 - 51, The config response shape is
too narrow because `configEndpoint` can return a null notifier from
`DynamicConfig.get(VConfig.Notifier)` while `Config.notifier` is typed as
`Notifiers`. Update the `Config` contract and any related mapping in
`configEndpoint` to allow `notifier` to be `Notifiers | null`, or ensure it is
always initialized before the endpoint returns. Keep the change aligned with the
existing `configEndpoint` and `DynamicConfig.get(VConfig.Notifier)` flow so the
API type matches the runtime value.
| notifier: notifier as Notifiers, | ||
| notifierConfig: { | ||
| discordWebhook: notifierDiscordWebhook, | ||
| gotifyUrl: notifierGotifyUrl, | ||
| gotifyToken: notifierGotifyToken, | ||
| gotifyUserToken: notifierGotifyUserToken, | ||
| gotifyApplicationId: notifierGotifyApplicationId, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Secrets returned in cleartext via the config read endpoint.
discordWebhook, gotifyToken, and gotifyUserToken are sent back verbatim to any client calling this endpoint. Since this is a persistent config-read surface (not just a one-time write ack), consider returning only a "configured" boolean/masked value and letting the write endpoints (setNotifierField) remain the only place raw values are transmitted (write-only, never echoed back).
🤖 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/endpoints/config.ts` around lines 129 - 136, The config read response in
the config endpoint is echoing notifier secrets in cleartext. Update the
notifierConfig object returned by the config read flow so that `discordWebhook`,
`gotifyToken`, and `gotifyUserToken` are masked or replaced with a configured
boolean, and keep raw values only in the write path handled by
`setNotifierField`; use the existing `notifierConfig`/`notifier` response
assembly to locate and adjust this behavior.
| function isNotifierField(value: string): value is VConfig { | ||
| return valid.includes(value as VConfig) | ||
| } | ||
|
|
||
| export async function setNotifierField(req: Request<{ field: Notifiers }, unknown, { value: string }>, res: Response) { | ||
| const { field } = req.params | ||
| const { value } = req.body |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'types.ts' src/modules/notifiers --exec cat -n {}Repository: Billos/Sparkleft
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## setNotifierField.ts\n'
cat -n src/endpoints/setNotifierField.ts
printf '\n## VConfig references\n'
rg -n "enum VConfig|type VConfig|interface VConfig|valid.includes|set\\(" src -S
printf '\n## notifier types\n'
fd -a 'types.ts' src | xargs -r sed -n '1,120p'Repository: Billos/Sparkleft
Length of output: 2710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## src/endpoints/setNotifierField.ts\n'
cat -n src/endpoints/setNotifierField.ts
printf '\n## VConfig definition and uses\n'
rg -n "enum VConfig|type VConfig|interface VConfig|valid =|valid\\.includes\\(|DynamicConfig\\.set|setNotifierField" src -S
printf '\n## all config enum definitions\n'
rg -n "enum .*Config|type .*Config" src -SRepository: Billos/Sparkleft
Length of output: 3277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/modules/config/dynamic.tsRepository: Billos/Sparkleft
Length of output: 2567
field should be typed as string, not Notifiers.
field is a raw route param that gets narrowed to VConfig before DynamicConfig.set(). Declaring it as Notifiers is misleading and can hide real type mismatches.
🔧 Proposed fix
-export async function setNotifierField(req: Request<{ field: Notifiers }, unknown, { value: string }>, res: Response) {
+export async function setNotifierField(req: Request<{ field: string }, unknown, { value: string }>, res: Response) {📝 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.
| function isNotifierField(value: string): value is VConfig { | |
| return valid.includes(value as VConfig) | |
| } | |
| export async function setNotifierField(req: Request<{ field: Notifiers }, unknown, { value: string }>, res: Response) { | |
| const { field } = req.params | |
| const { value } = req.body | |
| function isNotifierField(value: string): value is VConfig { | |
| return valid.includes(value as VConfig) | |
| } | |
| export async function setNotifierField(req: Request<{ field: string }, unknown, { value: string }>, res: Response) { | |
| const { field } = req.params | |
| const { value } = req.body |
🤖 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/endpoints/setNotifierField.ts` around lines 17 - 23, The route param in
setNotifierField is being typed too narrowly as Notifiers even though it arrives
as an untrusted string and is later narrowed by isNotifierField before calling
DynamicConfig.set(). Update the Request generic in setNotifierField to use
string for field, then keep the existing runtime validation/narrowing flow so
only validated values reach DynamicConfig.set().
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/queues/jobs/uncategorizedTransactions.ts (1)
113-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInit log messages say "UnbudgetedTransactions" instead of "UncategorizedTransactions" and hardcode count as
0.Both log messages in
init()are copy-pasted fromUnbudgetedTransactionsJoband incorrectly say "UnbudgetedTransactions". The count is also hardcoded to0regardless of actual transactions processed.🔧 Proposed fix
override async init(): Promise<void> { - logger.info("Initializing UnbudgetedTransactions jobs for all unbudgeted transactions") + logger.info("Initializing UncategorizedTransactions jobs for all uncategorized transactions") const notifier = await getNotifier() + let count = 0 if (notifier) { const startDate = Temporal.Now.zonedDateTimeISO(env.timezone).subtract({ months: 3 }).startOfDay() const start = startDate.toPlainDate().toString() const endDate = Temporal.Now.zonedDateTimeISO(env.timezone) const end = endDate.toPlainDate().toString() if (!end) { logger.error("Failed to get current date in ISO format") return } const uncategorizedTransactionsList = await getUncategorizedTransactions(start, end) + count = uncategorizedTransactionsList.length for (const { id: transactionId } of uncategorizedTransactionsList) { await addTransactionJobToQueue(this, transactionId) } } - logger.info("Initialized UnbudgetedTransactions jobs for %d transactions", 0) + logger.info("Initialized UncategorizedTransactions jobs for %d transactions", count) }🤖 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/queues/jobs/uncategorizedTransactions.ts` around lines 113 - 131, The init logging in UncategorizedTransactionsJob is using copied UnbudgetedTransactions wording and a hardcoded transaction count. Update the logger.info messages in init() to reference “UncategorizedTransactions” consistently, and replace the final 0 with the actual number of transactions processed from getUncategorizedTransactions. Use the init() method, uncategorizedTransactionsList, and addTransactionJobToQueue as the main points to adjust.src/queues/jobs/unbudgetedTransactions.ts (1)
89-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHardcoded
0in init log always reports zero transactions.Line 98 logs
0regardless of how many transactions were actually queued. When a notifier is configured and transactions are processed, the log is misleading.🔧 Proposed fix
override async init(): Promise<void> { logger.info("Initializing UnbudgetedTransactions jobs for all unbudgeted transactions") const notifier = await getNotifier() + let count = 0 if (notifier) { const { data } = await BudgetsService.listTransactionWithoutBudget({ client, query: { page: 1, limit: 50 } }) + count = data.length for (const { id: transactionId } of data) { await addTransactionJobToQueue(this, transactionId) } } - logger.info("Initialized UnbudgetedTransactions jobs for %d transactions", 0) + logger.info("Initialized UnbudgetedTransactions jobs for %d transactions", count) }🤖 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/queues/jobs/unbudgetedTransactions.ts` around lines 89 - 99, The init log in UnbudgetedTransactions always reports 0 transactions, which makes the success message misleading. In init() of UnbudgetedTransactions, track how many transaction IDs are actually queued while iterating over BudgetsService.listTransactionWithoutBudget() and pass that count to the final logger.info call instead of a hardcoded value. Keep the existing notifier check and use the same addTransactionJobToQueue flow so the log reflects the real number processed.
🧹 Nitpick comments (1)
src/queues/index.ts (1)
176-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnawaited
sendMessageinside async handler.
notifier.sendMessage(...)is not awaited, so a rejection becomes an unhandled promise rejection (the surroundingtry/logging won't catch it). Await it, or attach a.catchto log failures.♻️ Proposed fix
if (notifier) { - notifier.sendMessage( - "Job Failed", - `Job **${job.data.job}** (${job.id}) failed with error ${err.message} and data ${JSON.stringify(job.data)}`, - ) + await notifier.sendMessage( + "Job Failed", + `Job **${job.data.job}** (${job.id}) failed with error ${err.message} and data ${JSON.stringify(job.data)}`, + ) } else {🤖 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/queues/index.ts` around lines 176 - 180, The notifier call in the async job-failure handler is fire-and-forget, so any rejection from notifier.sendMessage in the job failure path can escape as an unhandled promise rejection. Update the failure-notification logic in the queue handler that uses job.data, job.id, and notifier.sendMessage to either await the sendMessage promise or attach a .catch that logs the notification failure, keeping it inside the surrounding error handling flow.
🤖 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/queues/index.ts`:
- Around line 156-167: The `worker.on("completed")` handler in
`src/queues/index.ts` returns early when `getNotifier()` is null, which prevents
`logJobDuration(true, id ?? "unknown", name ?? "unknown")` from running for
delayed jobs. Update the `completed` callback so the notifier lookup and
`deleteMessage` path are only conditional, but `logJobDuration` always executes
after the delayed-message branch, using the existing `worker.on("completed")`
and `logJobDuration` symbols to keep timing/log cleanup consistent.
In `@src/queues/jobs/BaseJob.ts`:
- Around line 95-99: The auto-import test mock for the notifiers module is
missing the getNotifier export used by BaseJob, so importing the job module
fails. Update the ../modules/notifiers mock in src/__tests__/autoImport.test.ts
to include getNotifier alongside the existing notifier mock, keeping the mock
shape aligned with BaseJob’s getNotifier usage so
import("../queues/jobs/autoImport.js") succeeds.
In `@src/queues/jobs/setBudgetForTransaction.ts`:
- Line 22: The `setBudgetForTransaction` change now calls `getNotifier()`, but
the `src/__tests__/jobs/setBudgetForTransaction.test.ts` mock for
`../../modules/notifiers` still only exposes the old `notifier` value. Update
the test mock to include a `getNotifier` export that returns a mock resolved
notifier (or `null` when needed) so the `setBudgetForTransaction` job can be
exercised without missing-export failures.
In `@src/queues/jobs/setCategoryForTransaction.ts`:
- Around line 22-34: The notifier cleanup in setCategoryForTransaction is
swallowing the real failure and logging a misleading “No notifier message to
delete” message. Update the try/catch around getMessageId,
unbindTransactionToNotification, and notifier.deleteMessage to capture the
thrown error, and log that error with context in the catch block instead of
implying the message is simply missing. Keep the existing logger, notifier, and
unbindTransactionToNotification flow, but make the failure message reflect an
actual delete/unbind error for the transaction id.
---
Outside diff comments:
In `@src/queues/jobs/unbudgetedTransactions.ts`:
- Around line 89-99: The init log in UnbudgetedTransactions always reports 0
transactions, which makes the success message misleading. In init() of
UnbudgetedTransactions, track how many transaction IDs are actually queued while
iterating over BudgetsService.listTransactionWithoutBudget() and pass that count
to the final logger.info call instead of a hardcoded value. Keep the existing
notifier check and use the same addTransactionJobToQueue flow so the log
reflects the real number processed.
In `@src/queues/jobs/uncategorizedTransactions.ts`:
- Around line 113-131: The init logging in UncategorizedTransactionsJob is using
copied UnbudgetedTransactions wording and a hardcoded transaction count. Update
the logger.info messages in init() to reference “UncategorizedTransactions”
consistently, and replace the final 0 with the actual number of transactions
processed from getUncategorizedTransactions. Use the init() method,
uncategorizedTransactionsList, and addTransactionJobToQueue as the main points
to adjust.
---
Nitpick comments:
In `@src/queues/index.ts`:
- Around line 176-180: The notifier call in the async job-failure handler is
fire-and-forget, so any rejection from notifier.sendMessage in the job failure
path can escape as an unhandled promise rejection. Update the
failure-notification logic in the queue handler that uses job.data, job.id, and
notifier.sendMessage to either await the sendMessage promise or attach a .catch
that logs the notification failure, keeping it inside the surrounding error
handling flow.
🪄 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: 65b4769c-3712-49c7-bcaa-9539e7119954
📒 Files selected for processing (13)
src/config.tssrc/modules/notifiers/discord.tssrc/modules/notifiers/gotify.tssrc/modules/notifiers/index.tssrc/queues/index.tssrc/queues/jobs/BaseJob.tssrc/queues/jobs/checkBudgetLimit.tssrc/queues/jobs/removeTransactionMessages.tssrc/queues/jobs/setBudgetForTransaction.tssrc/queues/jobs/setCategoryForTransaction.tssrc/queues/jobs/unbudgetedTransactions.tssrc/queues/jobs/uncategorizedTransactions.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/config.ts
| worker.on("completed", async ({ id, name, data }) => { | ||
| if (data.delayedMessageId) { | ||
| logger.info("Deleting delayed message %s for job %s (%s)", data.delayedMessageId, id, name) | ||
| const notifier = await getNotifier() | ||
| if (!notifier) { | ||
| logger.warn("No notifier configured, skipping message deletion for job %s (%s)", id, name) | ||
| return | ||
| } | ||
| await notifier.deleteMessage(data.delayedMessageId) | ||
| } | ||
| logJobDuration(true, id ?? "unknown", name ?? "unknown") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
completed handler skips logJobDuration when no notifier is configured.
When data.delayedMessageId is set but getNotifier() returns null, the early return at Line 162 bypasses logJobDuration(true, ...) at Line 166, so completed delayed jobs won't be timed/logged (and startedAt won't be cleaned up). Fold the notifier guard into the branch so the duration log always runs.
🐛 Proposed fix
worker.on("completed", async ({ id, name, data }) => {
if (data.delayedMessageId) {
logger.info("Deleting delayed message %s for job %s (%s)", data.delayedMessageId, id, name)
const notifier = await getNotifier()
- if (!notifier) {
- logger.warn("No notifier configured, skipping message deletion for job %s (%s)", id, name)
- return
- }
- await notifier.deleteMessage(data.delayedMessageId)
+ if (notifier) {
+ await notifier.deleteMessage(data.delayedMessageId)
+ } else {
+ logger.warn("No notifier configured, skipping message deletion for job %s (%s)", id, name)
+ }
}
logJobDuration(true, id ?? "unknown", name ?? "unknown")
})📝 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.
| worker.on("completed", async ({ id, name, data }) => { | |
| if (data.delayedMessageId) { | |
| logger.info("Deleting delayed message %s for job %s (%s)", data.delayedMessageId, id, name) | |
| const notifier = await getNotifier() | |
| if (!notifier) { | |
| logger.warn("No notifier configured, skipping message deletion for job %s (%s)", id, name) | |
| return | |
| } | |
| await notifier.deleteMessage(data.delayedMessageId) | |
| } | |
| logJobDuration(true, id ?? "unknown", name ?? "unknown") | |
| }) | |
| worker.on("completed", async ({ id, name, data }) => { | |
| if (data.delayedMessageId) { | |
| logger.info("Deleting delayed message %s for job %s (%s)", data.delayedMessageId, id, name) | |
| const notifier = await getNotifier() | |
| if (notifier) { | |
| await notifier.deleteMessage(data.delayedMessageId) | |
| } else { | |
| logger.warn("No notifier configured, skipping message deletion for job %s (%s)", id, name) | |
| } | |
| } | |
| logJobDuration(true, id ?? "unknown", name ?? "unknown") | |
| }) |
🤖 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/queues/index.ts` around lines 156 - 167, The `worker.on("completed")`
handler in `src/queues/index.ts` returns early when `getNotifier()` is null,
which prevents `logJobDuration(true, id ?? "unknown", name ?? "unknown")` from
running for delayed jobs. Update the `completed` callback so the notifier lookup
and `deleteMessage` path are only conditional, but `logJobDuration` always
executes after the delayed-message branch, using the existing
`worker.on("completed")` and `logJobDuration` symbols to keep timing/log cleanup
consistent.
| const notifier = await getNotifier() | ||
| if (notifier) { | ||
| try { | ||
| logger.info("Deleting notifier message") | ||
| const messageId = await notifier.getMessageId("CategoryMessageId", id) | ||
| await unbindTransactionToNotification(id, "CategoryMessageId", messageId) | ||
| await notifier.deleteMessage(messageId) | ||
| } catch { | ||
| logger.error("No notifier message to delete for transaction %s", id) | ||
| } | ||
| } else { | ||
| logger.warn("No notifier configured, skipping message removal for transaction %s", id) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Catch block discards error details and uses misleading log message.
The catch block doesn't capture the error, so the actual failure reason (network error, API failure, etc.) is lost. The log message "No notifier message to delete" is misleading — the failure may not be a missing message.
🔧 Proposed fix
} catch {
- logger.error("No notifier message to delete for transaction %s", id)
+ } catch (err) {
+ logger.error({ err }, "Failed to delete notifier message for transaction %s", id)
}📝 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 notifier = await getNotifier() | |
| if (notifier) { | |
| try { | |
| logger.info("Deleting notifier message") | |
| const messageId = await notifier.getMessageId("CategoryMessageId", id) | |
| await unbindTransactionToNotification(id, "CategoryMessageId", messageId) | |
| await notifier.deleteMessage(messageId) | |
| } catch { | |
| logger.error("No notifier message to delete for transaction %s", id) | |
| } | |
| } else { | |
| logger.warn("No notifier configured, skipping message removal for transaction %s", id) | |
| } | |
| const notifier = await getNotifier() | |
| if (notifier) { | |
| try { | |
| logger.info("Deleting notifier message") | |
| const messageId = await notifier.getMessageId("CategoryMessageId", id) | |
| await unbindTransactionToNotification(id, "CategoryMessageId", messageId) | |
| await notifier.deleteMessage(messageId) | |
| } catch (err) { | |
| logger.error({ err }, "Failed to delete notifier message for transaction %s", id) | |
| } | |
| } else { | |
| logger.warn("No notifier configured, skipping message removal for transaction %s", id) | |
| } |
🤖 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/queues/jobs/setCategoryForTransaction.ts` around lines 22 - 34, The
notifier cleanup in setCategoryForTransaction is swallowing the real failure and
logging a misleading “No notifier message to delete” message. Update the
try/catch around getMessageId, unbindTransactionToNotification, and
notifier.deleteMessage to capture the thrown error, and log that error with
context in the catch block instead of implying the message is simply missing.
Keep the existing logger, notifier, and unbindTransactionToNotification flow,
but make the failure message reflect an actual delete/unbind error for the
transaction id.
Summary by CodeRabbit