feat: recurring asset reminders (auto-repeat on a cadence) - #2685
Open
carlosvirreira wants to merge 6 commits into
Open
feat: recurring asset reminders (auto-repeat on a cadence)#2685carlosvirreira wants to merge 6 commits into
carlosvirreira wants to merge 6 commits into
Conversation
Reminders can now repeat on a cadence (daily, weekly, every 2 weeks, monthly, every 3/6 months, yearly) with an optional end date. Closes #2659; also addresses the recurring asks in #483, #1445 and #2520. Design: one AssetReminder row per series. The worker advances alertDateTime to the next occurrence in place on each fire, so every upcoming surface (home widget, overview card, advanced-index column, CSV export) and the existing status/edit/cancel guards stay correct without forked logic. Scheduling stays on pg-boss sendAfter one-shots (noScheduling untouched) as a self-rescheduling chain, the same pattern the audit reminder chain already uses, hardened with: - singletonKey dedupe + retryLimit 3 on all recurring jobs - compare-and-swap row advance (safe under bluegreen double-workers) - stale-job guard in the worker (also closes the pre-existing double-fire window after a failed cancel on edit) - orphan-recovery on retry when the advance committed but scheduling failed - boot-time reconciliation (1h grace, per-row fault isolation) as the no-cron safety net for dead chains Recurrence math is wall-clock stable in the timezone captured from client hints at create/edit (luxon; DST-correct, month-end clamps Jan 31 -> Feb 28). Catch-up policy: occurrences missed during downtime are skipped, never bursted. Gating: one-shot reminders stay free. Recurrence requires the new canUseRecurringReminders tier flag (TierLimit false by default, enabled for tier_1/tier_2, CustomTierLimit true), asserted server-side at both mutation entry points. Edits only assert when recurrence is added or changed, so downgraded workspaces can still edit other fields, turn recurrence off or delete. If a workspace loses the capability, the series pauses at its next fire (notifies, does not reschedule) and can be resumed after upgrading. Migrations follow the canImportNRM/canHideShelfBranding two-file precedent and were verified with prisma migrate deploy + migrate status (zero drift) against a clean Postgres 16. Drive-by fixes on touched lines: org-scope validation (assertAssetsBelongToOrg) in createAssetReminder per the org-scope-user-supplied-ids rule; reminders pagination totalPages divided by the raw per_page param (Infinity without it); asset-overview reminder card ordered desc (showed furthest-future instead of next); reminder schema future-check evaluated new Date() at module load. Deliberate non-goals for v1: duplicating an asset does not copy its reminders, no per-occurrence editing, no calendar surface, no mobile surface (reminders do not exist on mobile today), no maintenance module (own scope). Tests: recurrence math (DST both directions, month-end, catch-up, end date), chain advance/reconcile, worker handler (order, guards, retry paths), form schema parsing (empty endsAt, missing repeat, past dates), and the endsAt timezone round-trip. First tests for the asset-reminder module.
🩺 React Doctor — webappFindings on the files changed by this PR:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4744100e01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/modules/asset-reminder/service.server.ts (1)
309-335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet explicit 4xx status for intentional edit blocks.
These
ShelfErrors usecause: nullwithoutstatus, so they default to 500 even though they are expected user/business-rule denials.Proposed fix
throw new ShelfError({ cause: null, message: "Edit is not allowed for this reminder.", label: "Asset Reminder", additionalData: { id }, shouldBeCaptured: false, + status: 403, });throw new ShelfError({ cause: null, title: "Not allowed", message: "Recurring reminders are not available on your workspace's current plan. Please upgrade your subscription to unlock this feature, or set a one-time reminder instead.", label: "Tier", additionalData: { id, organizationId }, shouldBeCaptured: false, + status: 403, });🤖 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 `@apps/webapp/app/modules/asset-reminder/service.server.ts` around lines 309 - 335, The intentional edit blocks in service.server.ts are throwing ShelfError from the reminder edit flow without an explicit client-error status, so they default to a 500. Update the ShelfError instances in the reminder validation logic around the edit restrictions to set an explicit 4xx status (for example, 400/403 as appropriate) while keeping the existing message, label, and additionalData fields. Use the surrounding reminder edit checks and the recurring-reminder branch to locate the two throws.
🧹 Nitpick comments (12)
apps/webapp/app/modules/asset-reminder/service.server.ts (1)
36-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the changed exported service APIs.
createAssetReminderandeditAssetRemindernow expose recurrence/tier behavior; document parameters, return value, andShelfErrorcases. As per coding guidelines, “every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors.”Also applies to: 271-293
🤖 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 `@apps/webapp/app/modules/asset-reminder/service.server.ts` around lines 36 - 56, The exported service APIs in createAssetReminder and editAssetReminder need JSDoc comments. Add docs above each function describing the parameters (including recurrence/tier-related inputs), the return value, and the ShelfError cases they can throw; make sure the comments cover the updated behavior in the asset-reminder service module and match the project’s exported-function documentation rule.Source: Coding guidelines
apps/webapp/app/routes/_layout+/assets.$assetId.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required file/action JSDoc.
The file starts with an import, and the exported
actionhandler is undocumented. As per coding guidelines, every TypeScript file must start with a JSDoc purpose block and every exported function must have JSDoc.Also applies to: 272-272
🤖 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 `@apps/webapp/app/routes/_layout`+/assets.$assetId.tsx at line 1, The route module currently starts with an import and the exported action handler lacks documentation; add a JSDoc purpose block at the top of this TypeScript file and a JSDoc comment for the exported action function in assets.$assetId.tsx. Use the existing action symbol and module entrypoint as anchors, and ensure the docs describe the file’s purpose and the handler’s behavior without changing logic.Source: Coding guidelines
apps/webapp/app/routes/_layout+/assets.$assetId.reminders.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required route JSDoc.
This file starts with an import and the exported
loader/actionhandlers are undocumented. As per coding guidelines, every TypeScript file must start with a JSDoc purpose block and every exported function must have JSDoc.Also applies to: 23-23, 74-74
🤖 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 `@apps/webapp/app/routes/_layout`+/assets.$assetId.reminders.tsx at line 1, The route module is missing the required JSDoc documentation, so add a file-level purpose block at the top of the asset reminders route and JSDoc comments for each exported handler in this module, especially the loader and action functions. Update the documentation near the loader/action symbols so they describe their purpose and align with the project’s TypeScript JSDoc convention.Source: Coding guidelines
apps/webapp/app/routes/_layout+/reminders._index.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required route JSDoc.
This file starts with an import and the exported
loader/actionhandlers are undocumented. As per coding guidelines, every TypeScript file must start with a JSDoc purpose block and every exported function must have JSDoc.Also applies to: 23-23, 74-74
🤖 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 `@apps/webapp/app/routes/_layout`+/reminders._index.tsx at line 1, The route module is missing the required JSDoc purpose block at the top of the file, and the exported `loader` and `action` handlers are undocumented. Add a brief file-level JSDoc describing the route purpose before the imports, then add JSDoc comments for each exported function (`loader` and `action`) in this route module, following the existing coding guidelines.Source: Coding guidelines
apps/webapp/app/modules/asset-reminder/recurrence.ts (2)
82-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd full
@param/@returnsJSDoc to these exported helpers.
isRecurringReminder,repeatValueFromRecurrence,describeRecurrence, andresolveRecurrenceZoneeach have only a one-line or partial doc comment, missing@param/@returnstags (unlikegetNextOccurrencebelow, which is fully documented).As per coding guidelines, "every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
Also applies to: 89-93, 120-143, 145-153
🤖 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 `@apps/webapp/app/modules/asset-reminder/recurrence.ts` around lines 82 - 87, Add full JSDoc to the exported helpers in recurrence.ts: isRecurringReminder, repeatValueFromRecurrence, describeRecurrence, and resolveRecurrenceZone should each document their parameters and return values with `@param` and `@returns` tags, matching the style already used by getNextOccurrence. Keep the existing symbols and behavior unchanged; only expand the comments so the exported function docs are complete and consistent.Source: Coding guidelines
94-106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFallback to
"monthly"risks silently rewriting stored cadence for non-preset data.The comment on this function explicitly anticipates "future custom cadences" as unit+interval data that won't match any preset. When that happens, this fallback returns
"monthly"regardless of the actual stored unit/interval, so the edit dialog would show "Monthly" and a no-op save would silently change the reminder's real cadence.♻️ Proposed fix: fall back to same-unit preset instead of a fixed one
const match = Object.entries(REMINDER_REPEAT_PRESETS).find( ([, preset]) => preset.unit === reminder.recurrenceUnit && preset.interval === reminder.recurrenceInterval ); - - return (match?.[0] as ReminderRepeatValue) ?? "monthly"; + if (match) return match[0] as ReminderRepeatValue; + + // No exact preset match (custom cadence). Falling back to a fixed preset + // like "monthly" would silently rewrite the stored cadence on a no-op + // save — pick the closest same-unit preset so at least the unit survives. + const sameUnitFallback = Object.entries(REMINDER_REPEAT_PRESETS).find( + ([, preset]) => preset.unit === reminder.recurrenceUnit + ); + return (sameUnitFallback?.[0] as ReminderRepeatValue) ?? "monthly";🤖 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 `@apps/webapp/app/modules/asset-reminder/recurrence.ts` around lines 94 - 106, The fallback in repeatValueFromRecurrence currently returns a fixed "monthly" value for any non-preset recurrence, which can misrepresent future custom cadences and silently rewrite stored data. Update repeatValueFromRecurrence to preserve the same recurrenceUnit/recurrenceInterval semantics by deriving a fallback from the reminder’s actual unit rather than hardcoding "monthly", and keep the preset lookup via REMINDER_REPEAT_PRESETS for known values. Ensure the edit dialog’s value mapping remains stable for custom cadence data in AssetReminder recurrence handling.apps/webapp/app/utils/subscription.server.ts (1)
432-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
@paramdocs fororganizationIdandorganizations.The JSDoc documents
@throwsbut omits parameter descriptions.As per coding guidelines, "every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
🤖 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 `@apps/webapp/app/utils/subscription.server.ts` around lines 432 - 468, The exported assertUserCanUseRecurringReminders function already documents the throw behavior, but it is missing JSDoc `@param` entries for organizationId and organizations. Update the existing JSDoc above assertUserCanUseRecurringReminders to describe both parameters clearly, keeping the current `@throws` note intact and matching the project rule that exported functions document parameters, return values, and errors.Source: Coding guidelines
apps/webapp/app/components/assets/asset-reminder-cards.tsx (1)
58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCache
describeRecurrence(reminder)instead of calling it twice.Sibling components (
reminders-table.tsx,advanced-asset-columns.tsx) compute the recurrence label once and reuse it; here it's invoked twice per reminder row.♻️ Proposed fix
{reminders.map((reminder) => { const slicedTeamMembers = reminder.teamMembers.slice(0, 10); const remainingTeamMembers = reminder.teamMembers.length - slicedTeamMembers.length; const isAlreadySent = new Date() > new Date(reminder.alertDateTime); + const recurrenceLabel = describeRecurrence(reminder); return ( <div key={reminder.id} className="border-b px-4 py-3"> @@ <p className="mb-2"> <DateS date={reminder.alertDateTime} includeTime /> </p> - {describeRecurrence(reminder) ? ( + {recurrenceLabel ? ( <p className="mb-2 flex items-center gap-1 text-xs text-gray-500"> <RepeatIcon className="size-3" aria-hidden="true" /> - {describeRecurrence(reminder)} + {recurrenceLabel} </p> ) : null}🤖 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 `@apps/webapp/app/components/assets/asset-reminder-cards.tsx` around lines 58 - 63, The recurrence label in asset-reminder-cards.tsx is computed twice by calling describeRecurrence(reminder) in the conditional and again when rendering. Update the AssetReminderCards rendering logic to store the result in a local variable and reuse it for both the truthy check and the displayed text, following the same pattern used in reminders-table.tsx and advanced-asset-columns.tsx.apps/webapp/app/entry.server.tsx (1)
85-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: mixed logging conventions for reconciliation results.
The success path uses
console.logwhile the failure path right below usesLogger.error(new ShelfError(...)). For consistency and structured observability (searchable/filterable boot logs), consider routing the success message throughLogger.infoas well.🔧 Suggested tweak
.then(({ scanned, rearmed }) => { if (scanned > 0) { - console.log( + Logger.info( `Recurring reminders reconciled: ${rearmed}/${scanned} chains re-armed` ); } })🤖 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 `@apps/webapp/app/entry.server.tsx` around lines 85 - 109, The reconciliation success path in entry.server.tsx is using console.log while the failure path uses Logger.error with ShelfError, so make the logging consistent. Update the success branch in reconcileRecurringReminders to use Logger.info with the same “Recurring reminders reconciled” message instead of console.log, keeping the existing structured logging pattern alongside Logger.error and ShelfError.apps/webapp/app/modules/asset-reminder/chain.server.ts (1)
190-241: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftReconciliation candidate set has no upper bound and can grow unboundedly for permanently tier-paused chains.
Two related scalability gaps in
reconcileRecurringReminders:
findManyhas notake/pagination limit — as the number of recurring reminders grows, boot-time reconciliation performs an ever-larger unbounded query and then processes rows sequentially in aforloop (one DB round-trip per row viaadvanceRecurringReminder), adding to boot latency on every deploy.- When
advanceRecurringReminderreturnspaused: true(org lost tier access),alertDateTimeis never advanced andrecurrenceUnit/recurrenceEndsAtare left untouched. That row will keep matching this query'swhereclause (recurrenceUnit: { not: null },alertDateTime: { lt: deadBefore }, endsAt null/future) on every subsequent boot indefinitely, re-running theorganization.findUnique+getUserTierLimitlookup each time with no way to short-circuit until the org re-upgrades or the row is edited.Given "Shelf deploys frequently" (per the file's own doc comment), paused chains accumulating over time will make this scan progressively more expensive at every deploy.
Consider adding a
takelimit with cursor/pagination for the candidate scan, and consider persisting an explicit "paused" signal (or bumpingalertDateTimeforward on pause) so downgraded chains stop being rescanned on every boot until something changes.🤖 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 `@apps/webapp/app/modules/asset-reminder/chain.server.ts` around lines 190 - 241, `reconcileRecurringReminders` currently scans an unbounded candidate set and will keep rescaning permanently paused recurring reminders on every boot. Add pagination/`take` (with cursor or batching) to the `db.assetReminder.findMany` query and process batches incrementally so boot-time reconciliation stays bounded. Also update the `advanceRecurringReminder` paused path so paused chains are marked in a way that excludes them from the existing `where` clause in future runs, preventing repeated `organization.findUnique`/tier checks until the reminder is changed or reactivated.apps/webapp/app/modules/asset-reminder/emails.tsx (1)
53-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on exported, signature-changed function.
assetAlertEmailTextgained a newnextOccurrenceparameter but has no JSDoc block describing parameters/returns. Per repo guidelines, every exported function must document its parameters and return value.As per coding guidelines: "every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
🤖 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 `@apps/webapp/app/modules/asset-reminder/emails.tsx` around lines 53 - 61, The exported assetAlertEmailText function is missing the required JSDoc block after its signature changed to include nextOccurrence. Add a JSDoc comment above assetAlertEmailText documenting all parameters in AssetAlertEmailProps, including nextOccurrence, and describing the returned email text; keep the documentation aligned with the function’s behavior and update any return/throws notes if applicable.Source: Coding guidelines
apps/webapp/app/modules/asset-reminder/worker.server.test.ts (1)
1-286: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid coverage of ordering/staleness/re-arm branches — nicely done.
Mocks are all annotated with
// why:comments and only mock external boundaries (db, scheduler, email, note) plus the already-separately-testedadvanceRecurringReminder, consistent with repo test conventions.One gap worth adding: a test for the "series naturally ended" outcome (
advanceRecurringReminderresolving{ next: null, advanced: true, paused: false }) — see the related comment onworker.server.ts(lines 121-170) about that branch currently being indistinguishable from a one-shot fire.As per path instructions: "Every mock in tests must be accompanied by a
// why:comment explaining the reason for mocking."🤖 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 `@apps/webapp/app/modules/asset-reminder/worker.server.test.ts` around lines 1 - 286, Add a test in the REMINDER worker handler suite for the “series naturally ended” path where advanceRecurringReminder resolves to { next: null, advanced: true, paused: false }. This branch is currently not covered and should verify the handler treats it distinctly from a one-shot reminder by asserting the expected note/log behavior and that no re-scheduling occurs; use the existing handler/JOB setup plus the mocked advanceRecurringReminder, sendEmail, and createNote symbols to locate the flow.Source: Path instructions
🤖 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 `@apps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsx`:
- Around line 142-143: The repeat selection in SetOrEditReminderDialog is only
initialized once via useState(initialRepeat), so reopening the dialog or
switching to a different reminder can leave stale cadence state. Update the
state in the dialog component when the dialog opens or when reminder changes,
using the existing SetOrEditReminderDialog and repeat/setRepeat state so the
selected value always reflects the current reminder instead of the previous
session.
- Around line 58-72: The future-date validation in setOrEditReminderSchema is
using z.coerce.date() input time, which can disagree with the client-hint
timezone used later for scheduling. Move the “Please select a date in the
future” check into the timezone-aware parse/validation path for alertDateTime,
and compare against the resolved instant rather than Date.now() on the raw
coerced date. Use the existing setOrEditReminderSchema and alertDateTime
validation block as the fix point.
In `@apps/webapp/app/modules/asset-reminder/service.server.ts`:
- Around line 319-349: The recurring-reminder downgrade gate in
service.server.ts only compares recurrence unit, interval, and endsAt, but it
ignores recurrenceTimezone even though assetReminder.update writes it. Update
the recurrenceChanged check in the recurrence validation block to include
recurrence.timezone versus reminder.recurrenceTimezone so timezone-only changes
are also blocked for plans without recurring reminders.
In `@apps/webapp/app/modules/asset-reminder/utils.server.ts`:
- Around line 48-77: The recurrence builder in the asset reminder helper is
overwriting the stored series timezone with the current request’s client hint,
which can shift existing reminders and trigger unintended recurrence changes
during edits. Update the logic in the reminder parsing/helper path to prefer the
persisted recurrence timezone passed through the form/action (for example, the
existing recurrenceTimezone) when editing an existing series, and only fall back
to the client hint-derived zone for new reminders or when no stored timezone
exists. Make sure the recurrence object assembled in this helper preserves the
original timezone instead of always using getHints(request) and
IANAZone.isValidZone(...).
In `@apps/webapp/app/modules/asset-reminder/worker.server.ts`:
- Around line 217-231: The next-occurrence date formatting in the asset reminder
worker duplicates the same Luxon formatting logic used in the email recurrence
helper, so extract it into the shared recurrence module. Move the
DateTime.fromJSDate(...).setZone(...).toFormat("d LLL yyyy, HH:mm") logic into a
reusable formatRecurrenceDateTime(date, timezone) helper in ./recurrence, then
update worker.server.ts and emails.tsx to call that helper instead of formatting
inline. Keep the existing zone resolution behavior aligned with the
recurrence-related symbols already in use, such as resolveRecurrenceZone and
recurrenceLine.
- Around line 121-170: The recurring reminder flow in worker.server.ts only
distinguishes nextOccurrence and seriesPaused, so a naturally ending series is
treated like a generic one-shot reminder. Update advanceRecurringReminder to
return an explicit “series ended” signal, then consume that result in the
recurring branch here (alongside nextOccurrence and seriesPaused) so the final
fire is handled with dedicated ended messaging instead of falling through to the
default text.
In `@apps/webapp/app/routes/_layout`+/assets.$assetId.tsx:
- Around line 379-383: The recurring-reminders tier check is surfacing as a 500
because `assertUserCanUseRecurringReminders` throws a `ShelfError` without an
explicit status. Update that utility so the denial path includes `status: 403`
on the `ShelfError`, and keep the route in `assets.$assetId.tsx` unchanged
except to rely on the corrected helper behavior.
---
Outside diff comments:
In `@apps/webapp/app/modules/asset-reminder/service.server.ts`:
- Around line 309-335: The intentional edit blocks in service.server.ts are
throwing ShelfError from the reminder edit flow without an explicit client-error
status, so they default to a 500. Update the ShelfError instances in the
reminder validation logic around the edit restrictions to set an explicit 4xx
status (for example, 400/403 as appropriate) while keeping the existing message,
label, and additionalData fields. Use the surrounding reminder edit checks and
the recurring-reminder branch to locate the two throws.
---
Nitpick comments:
In `@apps/webapp/app/components/assets/asset-reminder-cards.tsx`:
- Around line 58-63: The recurrence label in asset-reminder-cards.tsx is
computed twice by calling describeRecurrence(reminder) in the conditional and
again when rendering. Update the AssetReminderCards rendering logic to store the
result in a local variable and reuse it for both the truthy check and the
displayed text, following the same pattern used in reminders-table.tsx and
advanced-asset-columns.tsx.
In `@apps/webapp/app/entry.server.tsx`:
- Around line 85-109: The reconciliation success path in entry.server.tsx is
using console.log while the failure path uses Logger.error with ShelfError, so
make the logging consistent. Update the success branch in
reconcileRecurringReminders to use Logger.info with the same “Recurring
reminders reconciled” message instead of console.log, keeping the existing
structured logging pattern alongside Logger.error and ShelfError.
In `@apps/webapp/app/modules/asset-reminder/chain.server.ts`:
- Around line 190-241: `reconcileRecurringReminders` currently scans an
unbounded candidate set and will keep rescaning permanently paused recurring
reminders on every boot. Add pagination/`take` (with cursor or batching) to the
`db.assetReminder.findMany` query and process batches incrementally so boot-time
reconciliation stays bounded. Also update the `advanceRecurringReminder` paused
path so paused chains are marked in a way that excludes them from the existing
`where` clause in future runs, preventing repeated
`organization.findUnique`/tier checks until the reminder is changed or
reactivated.
In `@apps/webapp/app/modules/asset-reminder/emails.tsx`:
- Around line 53-61: The exported assetAlertEmailText function is missing the
required JSDoc block after its signature changed to include nextOccurrence. Add
a JSDoc comment above assetAlertEmailText documenting all parameters in
AssetAlertEmailProps, including nextOccurrence, and describing the returned
email text; keep the documentation aligned with the function’s behavior and
update any return/throws notes if applicable.
In `@apps/webapp/app/modules/asset-reminder/recurrence.ts`:
- Around line 82-87: Add full JSDoc to the exported helpers in recurrence.ts:
isRecurringReminder, repeatValueFromRecurrence, describeRecurrence, and
resolveRecurrenceZone should each document their parameters and return values
with `@param` and `@returns` tags, matching the style already used by
getNextOccurrence. Keep the existing symbols and behavior unchanged; only expand
the comments so the exported function docs are complete and consistent.
- Around line 94-106: The fallback in repeatValueFromRecurrence currently
returns a fixed "monthly" value for any non-preset recurrence, which can
misrepresent future custom cadences and silently rewrite stored data. Update
repeatValueFromRecurrence to preserve the same recurrenceUnit/recurrenceInterval
semantics by deriving a fallback from the reminder’s actual unit rather than
hardcoding "monthly", and keep the preset lookup via REMINDER_REPEAT_PRESETS for
known values. Ensure the edit dialog’s value mapping remains stable for custom
cadence data in AssetReminder recurrence handling.
In `@apps/webapp/app/modules/asset-reminder/service.server.ts`:
- Around line 36-56: The exported service APIs in createAssetReminder and
editAssetReminder need JSDoc comments. Add docs above each function describing
the parameters (including recurrence/tier-related inputs), the return value, and
the ShelfError cases they can throw; make sure the comments cover the updated
behavior in the asset-reminder service module and match the project’s
exported-function documentation rule.
In `@apps/webapp/app/modules/asset-reminder/worker.server.test.ts`:
- Around line 1-286: Add a test in the REMINDER worker handler suite for the
“series naturally ended” path where advanceRecurringReminder resolves to { next:
null, advanced: true, paused: false }. This branch is currently not covered and
should verify the handler treats it distinctly from a one-shot reminder by
asserting the expected note/log behavior and that no re-scheduling occurs; use
the existing handler/JOB setup plus the mocked advanceRecurringReminder,
sendEmail, and createNote symbols to locate the flow.
In `@apps/webapp/app/routes/_layout`+/assets.$assetId.reminders.tsx:
- Line 1: The route module is missing the required JSDoc documentation, so add a
file-level purpose block at the top of the asset reminders route and JSDoc
comments for each exported handler in this module, especially the loader and
action functions. Update the documentation near the loader/action symbols so
they describe their purpose and align with the project’s TypeScript JSDoc
convention.
In `@apps/webapp/app/routes/_layout`+/assets.$assetId.tsx:
- Line 1: The route module currently starts with an import and the exported
action handler lacks documentation; add a JSDoc purpose block at the top of this
TypeScript file and a JSDoc comment for the exported action function in
assets.$assetId.tsx. Use the existing action symbol and module entrypoint as
anchors, and ensure the docs describe the file’s purpose and the handler’s
behavior without changing logic.
In `@apps/webapp/app/routes/_layout`+/reminders._index.tsx:
- Line 1: The route module is missing the required JSDoc purpose block at the
top of the file, and the exported `loader` and `action` handlers are
undocumented. Add a brief file-level JSDoc describing the route purpose before
the imports, then add JSDoc comments for each exported function (`loader` and
`action`) in this route module, following the existing coding guidelines.
In `@apps/webapp/app/utils/subscription.server.ts`:
- Around line 432-468: The exported assertUserCanUseRecurringReminders function
already documents the throw behavior, but it is missing JSDoc `@param` entries for
organizationId and organizations. Update the existing JSDoc above
assertUserCanUseRecurringReminders to describe both parameters clearly, keeping
the current `@throws` note intact and matching the project rule that exported
functions document parameters, return values, and errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 802fe815-60cf-4989-8c5a-d373c11a2e41
📒 Files selected for processing (29)
apps/webapp/app/components/asset-reminder/actions-dropdown.tsxapps/webapp/app/components/asset-reminder/reminders-table.tsxapps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.schema.test.tsapps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsxapps/webapp/app/components/assets/asset-reminder-cards.tsxapps/webapp/app/components/assets/assets-index/advanced-asset-columns.tsxapps/webapp/app/components/home/upcoming-reminders.tsxapps/webapp/app/entry.server.tsxapps/webapp/app/modules/asset-reminder/chain.server.test.tsapps/webapp/app/modules/asset-reminder/chain.server.tsapps/webapp/app/modules/asset-reminder/emails.tsxapps/webapp/app/modules/asset-reminder/recurrence.test.tsapps/webapp/app/modules/asset-reminder/recurrence.tsapps/webapp/app/modules/asset-reminder/scheduler.server.tsapps/webapp/app/modules/asset-reminder/service.server.tsapps/webapp/app/modules/asset-reminder/utils.server.test.tsapps/webapp/app/modules/asset-reminder/utils.server.tsapps/webapp/app/modules/asset-reminder/worker.server.test.tsapps/webapp/app/modules/asset-reminder/worker.server.tsapps/webapp/app/modules/asset/query.server.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/asset/types.tsapps/webapp/app/routes/_layout+/assets.$assetId.reminders.tsxapps/webapp/app/routes/_layout+/assets.$assetId.tsxapps/webapp/app/routes/_layout+/reminders._index.tsxapps/webapp/app/utils/subscription.server.tspackages/database/prisma/migrations/20260702120000_add_recurring_reminders/migration.sqlpackages/database/prisma/migrations/20260702120100_enable_recurring_reminders_for_paid_tiers/migration.sqlpackages/database/prisma/schema.prisma
Codex + CodeRabbit review of #2685: - Preserve the series' stored recurrence timezone on edits: a plain edit (message/recipients) from another timezone no longer re-anchors the schedule or shifts occurrences across mismatched DST boundaries. The submitted end date's calendar day is re-anchored (end-of-day) into the stored zone via a new rebaseEndOfDayToZone helper. Because the zone is never taken from the request for an existing series, it also cannot be mutated by downgraded workspaces (closes the tier-gate omission on the timezone field). - Reconciliation orphan recovery: when the CAS advance commits but scheduling the next occurrence throws, the per-row catch now re-arms the row's current occurrence best-effort (singleton-deduped) instead of leaving the series unmatched by the dead-chain query until it goes stale. - Move the future-date check off the shared zod schema for SERVER parses: z.coerce.date() reads the raw datetime-local string in the process zone and wrongly rejected valid future times for users west of UTC. The client schema keeps the check (browser zone is the user's zone); the server now enforces it in resolveReminderPayloadDates against the client-hint-resolved instant, surfacing a field-level 400. - Return 403 (not 500) when the tier gate denies recurrence (create + edit). - Explicit 'last occurrence' note when a series ends at its end date, so the final fire is distinguishable from a one-shot in the asset activity. - Reset the Repeat selection when the dialog reopens (stale cadence could be resubmitted after cancel). - Extract shared occurrence formatting into recurrence.ts (formatOccurrenceInZone), deduplicating worker note + email copy. - JSDoc for the changed service/subscription APIs; cache a duplicated describeRecurrence call. Tests: 10 new/updated (timezone preservation round-trip, server-vs-client schema split, resolved-instant validation, reconcile orphan re-arm, ended note).
Contributor
Author
|
AI-review processing log (all rounds):
16/16 review threads resolved, no open findings. Ready for human review. (This comment was originally the round-1 summary; consolidated into the full log after a tooling slip edited it in place.) |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Nice work getting ✏️ Learnings added
|
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds recurring reminder support across schema, scheduling, worker processing, edit/create flows, email content, tier gating, and reminder list/UI surfaces. ChangesRecurring Asset Reminders
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
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)
apps/webapp/app/modules/asset-reminder/scheduler.server.ts (1)
46-87: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard the dedupe path before updating
activeSchedulerReference.sendAfter()can returnnullfor an already-queued singleton job, and writing that straight to the row can clear a live reference and make later edit/delete cancellation skip the still-queued job. Resolve the existing job by singletonKey (pg-bossfindJobs) or avoid overwriting the row withnull.🤖 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 `@apps/webapp/app/modules/asset-reminder/scheduler.server.ts` around lines 46 - 87, The dedupe path in scheduleAssetReminder is overwriting activeSchedulerReference with null when scheduler.sendAfter returns null, which can clear a live job reference. Update scheduleAssetReminder to detect the singletonKey dedupe case before db.assetReminder.update, and either look up the existing pg-boss job (for example via findJobs) to persist its real reference or skip updating activeSchedulerReference when no new job reference is returned. Keep the fix focused in scheduleAssetReminder and the sendAfter result handling.
🧹 Nitpick comments (7)
apps/webapp/app/modules/asset-reminder/recurrence.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the exported type.
ReminderRepeatValueis exported but has no direct JSDoc block.As per coding guidelines,
**/*.{ts,tsx}exported types must have a JSDoc comment.Proposed documentation addition
+/** Repeat selector values supported by the reminder dialog. */ export type ReminderRepeatValue = (typeof REMINDER_REPEAT_VALUES)[number];🤖 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 `@apps/webapp/app/modules/asset-reminder/recurrence.ts` at line 33, Add a JSDoc block for the exported type ReminderRepeatValue in recurrence.ts. Document what the type represents by placing the comment directly above the export type declaration so it satisfies the exported-types documentation guideline.Source: Coding guidelines
apps/webapp/app/entry.server.tsx (1)
92-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Logger.infoinstead ofconsole.logfor the reconciliation summary.The rest of the boot sequence (and this same catch block) uses
Logger. A rawconsole.loghere bypasses whatever structured-logging/observability pipelineLoggerfeeds into.🧹 Suggested tweak
- if (scanned > 0) { - console.log( - `Recurring reminders reconciled: ${rearmed}/${scanned} chains re-armed` - ); - } + if (scanned > 0) { + Logger.info( + `Recurring reminders reconciled: ${rearmed}/${scanned} chains re-armed` + ); + }🤖 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 `@apps/webapp/app/entry.server.tsx` around lines 92 - 98, The reconciliation summary in entry.server.tsx is using a raw console.log inside the then handler for the recurring reminders reconciliation. Replace that call with Logger.info so the summary follows the same structured logging path as the rest of the boot sequence; use the existing Logger symbol in this flow and keep the same message content/context when logging scanned and rearmed counts.apps/webapp/app/modules/asset-reminder/chain.server.ts (1)
182-286: 🧹 Nitpick | 🔵 TrivialBoot-only reconciliation leaves long recovery windows between deploys.
reconcileRecurringRemindersonly runs once at boot, per the module docstring's "no cron by design" rationale. If a chain dies and the next deploy is far off, the series stays dead until then. Given this is explicitly documented as an accepted tradeoff, consider whether a lightweight periodic trigger (e.g. every N hours in addition to boot) would meaningfully shrink the exposure window for infrequently-deployed environments, without reintroducing the cron infrastructure being avoided.🤖 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 `@apps/webapp/app/modules/asset-reminder/chain.server.ts` around lines 182 - 286, `reconcileRecurringReminders` is boot-only, so dead recurring reminder chains can stay unrecovered until the next deploy; consider adding a lightweight periodic trigger in addition to the boot sweep. Update the scheduling/entry path around `reconcileRecurringReminders` so it still runs at boot but also executes every N hours in environments with infrequent deploys, while preserving the existing no-cron, fault-isolated behavior and the current `advanceRecurringReminder`/`scheduleAssetReminder` flow.apps/webapp/app/modules/asset-reminder/worker.server.ts (1)
244-244: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFinal/paused occurrence emails don't tell recipients the series stopped.
nextOccurrenceis the only recurrence signal passed toassetAlertEmailHtmlString/assetAlertEmailText. When the series ends (seriesEnded) or is paused by a downgrade (seriesPaused),nextOccurrenceisnull, sorecurrenceLine()inemails.tsxreturnsnulland the outgoing email is silent about it — only the internal asset-activity note (Line 229/231) mentions it. Recipients of what they know is a recurring reminder get no signal that no further reminders are coming, which is important for the downgrade-pause case especially (they may need to re-enable the plan or recreate the reminder manually).Consider threading
seriesEnded/seriesPausedthrough to the email props alongsidenextOccurrencesorecurrenceLinecan render an appropriate closing line.♻️ Sketch of the change
- nextOccurrence, + nextOccurrence, + seriesEnded, + seriesPaused, });(mirrored for the
assetAlertEmailTextcall at Line 257, plus extendingAssetAlertEmailProps/recurrenceLineinemails.tsxto consume the new flags.)Also applies to: 257-257
🤖 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 `@apps/webapp/app/modules/asset-reminder/worker.server.ts` at line 244, The recurring reminder emails are missing the “series ended/paused” status because only nextOccurrence is passed into assetAlertEmailHtmlString and assetAlertEmailText, so recurrenceLine in emails.tsx has no way to render a closing message. Thread seriesEnded and seriesPaused through the worker.server.ts email payloads alongside nextOccurrence, then update AssetAlertEmailProps and recurrenceLine in emails.tsx to use those flags and emit an appropriate final/paused recurrence line.apps/webapp/app/modules/asset-reminder/emails.tsx (1)
18-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSee linked comment on
worker.server.ts(Lines 244, 257).
recurrenceLinecorrectly falls back tonullwhennextOccurrenceis absent, but that also means it can't render an "ended"/"paused" message even though the reminder itself is flagged as recurring — the caller never passes that context through. This is the natural place to accept it onceworker.server.tsthreads the extra state.Also applies to: 27-48
🤖 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 `@apps/webapp/app/modules/asset-reminder/emails.tsx` at line 18, The recurrence email rendering path is missing the reminder state needed to show an “ended” or “paused” message when there is no next occurrence. Update the recurrence-related helper/rendering in emails.tsx (including the recurrenceLine logic that uses describeRecurrence and formatOccurrenceInZone) to accept the extra state passed from worker.server.ts, and use it to render the appropriate fallback message instead of returning null whenever nextOccurrence is absent.apps/webapp/app/modules/asset-reminder/utils.server.test.ts (1)
40-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRound-trip test doesn't exercise editing from a different timezone than the original series.
Both calls in this test use
requestWithZone(zone)with the identical zone, so it validates self-consistency within one timezone but doesn't cover the scenario raised previously (editing an existing recurring series while the current request's client-hint zone differs from the series' stored zone). GivenresolveReminderPayloadDateshas no parameter for a stored zone (see companion comment inutils.server.ts), a test asserting the cross-zone-edit behavior would help confirm whether that concern is actually resolved.🤖 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 `@apps/webapp/app/modules/asset-reminder/utils.server.test.ts` around lines 40 - 68, The round-trip test in dialogEndsAtDefault and resolveReminderPayloadDates only checks the same timezone twice, so it misses the cross-zone edit case. Update the test to simulate editing an existing recurring series whose stored end date was created in a different zone than the current requestWithZone zone, then verify the displayed date and re-submitted getTime() remain stable across that mismatch. Use the existing helpers dialogEndsAtDefault, resolveReminderPayloadDates, requestWithZone, and formData to cover the original stored zone versus the current client-hint zone.apps/webapp/app/modules/asset-reminder/utils.server.ts (1)
89-93: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueInvalid
endsAtinput silently becomes "no end date" instead of a validation error.If
rawEndsAtfails to parse (malformed string), the code falls back tonullsilently rather than surfacing a field-level validation error like thealertDateTimecheck does. A user who mistypes the end date could unknowingly create a never-ending recurrence.Also applies to: 101-101
🤖 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 `@apps/webapp/app/modules/asset-reminder/utils.server.ts` around lines 89 - 93, The `endsAt` parsing in `utils.server.ts` currently turns any malformed `rawEndsAt` into `null`, which hides invalid user input. Update the validation flow in the same place as the `alertDateTime` check so `DateTime.fromFormat(..., "yyyy-MM-dd", { zone })` is verified before constructing the date, and surface a field-level validation error when parsing fails instead of treating it as “no end date.” Ensure the logic around `endsAt` and the existing recurrence validation path uses a clear parse-failure branch rather than falling back silently.
🤖 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 `@apps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsx`:
- Around line 312-320: The disabled “Repeat” label in
set-or-edit-reminder-dialog.tsx uses text-gray-400 via the
canUseRecurringReminders ternary, which does not meet contrast requirements.
Update the label styling in the “Repeat” label block to use a darker gray such
as text-gray-500 or another color that achieves at least 4.5:1 contrast while
preserving the enabled/disabled state distinction.
- Around line 344-407: The edit flow in SetOrEditReminderDialog is validating
hidden recurrence data even when canUseRecurringReminders is false, which can
block submission without any visible error. Update the validation/rendering
around endsAtOrderingRefinement and the endsAt input so downgraded workspaces
either skip that refinement for hidden recurring fields or surface the error on
a visible control in set-or-edit-reminder-dialog.tsx, using the existing
zo.errors.endsAt(), validationErrors?.endsAt, and canUseRecurringReminders /
initialRepeat / endsAtDefault logic to keep the form submittable.
- Around line 48-70: The endsAt validation in endsAtOrderingRefinement is
comparing UTC-parsed dates against a local alertDateTime, which can reject valid
same-day recurring reminders for some time zones. Move this ordering check out
of the shared base reminder schema path and into the zone-resolved reminder
payload path used after localization, or make the comparison zone-aware using
the resolved reminder date/time values in setReminderSchema and
setReminderServerSchema.
In `@apps/webapp/app/modules/asset-reminder/service.server.ts`:
- Around line 261-263: The `totalPages` calculation in `getAssetReminders` is
still using `perPage`, but the query actually paginates with the clamped `take`
value, so the page count can be wrong when the cookie-resolved page size is
invalid. Update the `Math.ceil(totalReminders / ...)` logic to use `take` (the
effective page size used by the query) and keep the surrounding pagination flow
in sync with `perPage`, `perPageParam`, and `take`.
- Around line 368-375: The recurrence-change guard in
asset-reminder/service.server.ts only compares recurrence unit/interval/endsAt,
so downgraded recurring reminders can still change schedule by moving the next
fire time. Update the recurrenceChanged logic in the create/update flow around
effectiveRecurrence and isRecurringReminder to also compare the current
next-occurrence value derived from alertDateTime against the effective
recurrence’s next fire time, and treat that as a recurrence change when
canUseRecurringReminders is false.
In
`@packages/database/prisma/migrations/20260702120000_add_recurring_reminders/migration.sql`:
- Around line 8-11: The migration for AssetReminder currently allows invalid
recurrence cadence states because recurrenceUnit and recurrenceInterval can be
saved independently or with a non-positive interval. Add a database check
constraint in this migration that enforces the invariant: both cadence fields
are null, or both are non-null with recurrenceInterval > 0; use the
AssetReminder table and the recurrenceUnit/recurrenceInterval columns as the key
symbols to update.
---
Outside diff comments:
In `@apps/webapp/app/modules/asset-reminder/scheduler.server.ts`:
- Around line 46-87: The dedupe path in scheduleAssetReminder is overwriting
activeSchedulerReference with null when scheduler.sendAfter returns null, which
can clear a live job reference. Update scheduleAssetReminder to detect the
singletonKey dedupe case before db.assetReminder.update, and either look up the
existing pg-boss job (for example via findJobs) to persist its real reference or
skip updating activeSchedulerReference when no new job reference is returned.
Keep the fix focused in scheduleAssetReminder and the sendAfter result handling.
---
Nitpick comments:
In `@apps/webapp/app/entry.server.tsx`:
- Around line 92-98: The reconciliation summary in entry.server.tsx is using a
raw console.log inside the then handler for the recurring reminders
reconciliation. Replace that call with Logger.info so the summary follows the
same structured logging path as the rest of the boot sequence; use the existing
Logger symbol in this flow and keep the same message content/context when
logging scanned and rearmed counts.
In `@apps/webapp/app/modules/asset-reminder/chain.server.ts`:
- Around line 182-286: `reconcileRecurringReminders` is boot-only, so dead
recurring reminder chains can stay unrecovered until the next deploy; consider
adding a lightweight periodic trigger in addition to the boot sweep. Update the
scheduling/entry path around `reconcileRecurringReminders` so it still runs at
boot but also executes every N hours in environments with infrequent deploys,
while preserving the existing no-cron, fault-isolated behavior and the current
`advanceRecurringReminder`/`scheduleAssetReminder` flow.
In `@apps/webapp/app/modules/asset-reminder/emails.tsx`:
- Line 18: The recurrence email rendering path is missing the reminder state
needed to show an “ended” or “paused” message when there is no next occurrence.
Update the recurrence-related helper/rendering in emails.tsx (including the
recurrenceLine logic that uses describeRecurrence and formatOccurrenceInZone) to
accept the extra state passed from worker.server.ts, and use it to render the
appropriate fallback message instead of returning null whenever nextOccurrence
is absent.
In `@apps/webapp/app/modules/asset-reminder/recurrence.ts`:
- Line 33: Add a JSDoc block for the exported type ReminderRepeatValue in
recurrence.ts. Document what the type represents by placing the comment directly
above the export type declaration so it satisfies the exported-types
documentation guideline.
In `@apps/webapp/app/modules/asset-reminder/utils.server.test.ts`:
- Around line 40-68: The round-trip test in dialogEndsAtDefault and
resolveReminderPayloadDates only checks the same timezone twice, so it misses
the cross-zone edit case. Update the test to simulate editing an existing
recurring series whose stored end date was created in a different zone than the
current requestWithZone zone, then verify the displayed date and re-submitted
getTime() remain stable across that mismatch. Use the existing helpers
dialogEndsAtDefault, resolveReminderPayloadDates, requestWithZone, and formData
to cover the original stored zone versus the current client-hint zone.
In `@apps/webapp/app/modules/asset-reminder/utils.server.ts`:
- Around line 89-93: The `endsAt` parsing in `utils.server.ts` currently turns
any malformed `rawEndsAt` into `null`, which hides invalid user input. Update
the validation flow in the same place as the `alertDateTime` check so
`DateTime.fromFormat(..., "yyyy-MM-dd", { zone })` is verified before
constructing the date, and surface a field-level validation error when parsing
fails instead of treating it as “no end date.” Ensure the logic around `endsAt`
and the existing recurrence validation path uses a clear parse-failure branch
rather than falling back silently.
In `@apps/webapp/app/modules/asset-reminder/worker.server.ts`:
- Line 244: The recurring reminder emails are missing the “series ended/paused”
status because only nextOccurrence is passed into assetAlertEmailHtmlString and
assetAlertEmailText, so recurrenceLine in emails.tsx has no way to render a
closing message. Thread seriesEnded and seriesPaused through the
worker.server.ts email payloads alongside nextOccurrence, then update
AssetAlertEmailProps and recurrenceLine in emails.tsx to use those flags and
emit an appropriate final/paused recurrence line.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a5230a2-7709-4b73-9e03-d5eb66aad3aa
📒 Files selected for processing (29)
apps/webapp/app/components/asset-reminder/actions-dropdown.tsxapps/webapp/app/components/asset-reminder/reminders-table.tsxapps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.schema.test.tsapps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsxapps/webapp/app/components/assets/asset-reminder-cards.tsxapps/webapp/app/components/assets/assets-index/advanced-asset-columns.tsxapps/webapp/app/components/home/upcoming-reminders.tsxapps/webapp/app/entry.server.tsxapps/webapp/app/modules/asset-reminder/chain.server.test.tsapps/webapp/app/modules/asset-reminder/chain.server.tsapps/webapp/app/modules/asset-reminder/emails.tsxapps/webapp/app/modules/asset-reminder/recurrence.test.tsapps/webapp/app/modules/asset-reminder/recurrence.tsapps/webapp/app/modules/asset-reminder/scheduler.server.tsapps/webapp/app/modules/asset-reminder/service.server.tsapps/webapp/app/modules/asset-reminder/utils.server.test.tsapps/webapp/app/modules/asset-reminder/utils.server.tsapps/webapp/app/modules/asset-reminder/worker.server.test.tsapps/webapp/app/modules/asset-reminder/worker.server.tsapps/webapp/app/modules/asset/query.server.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/asset/types.tsapps/webapp/app/routes/_layout+/assets.$assetId.reminders.tsxapps/webapp/app/routes/_layout+/assets.$assetId.tsxapps/webapp/app/routes/_layout+/reminders._index.tsxapps/webapp/app/utils/subscription.server.tspackages/database/prisma/migrations/20260702120000_add_recurring_reminders/migration.sqlpackages/database/prisma/migrations/20260702120100_enable_recurring_reminders_for_paid_tiers/migration.sqlpackages/database/prisma/schema.prisma
- Zone-safe endsAt ordering: the shared refinement compared a UTC-midnight type=date coercion against a local datetime-local coercion and wrongly rejected valid same-day evening reminders for users west of UTC. The client schema now compares CALENDAR DAYS; the authoritative check runs in resolveReminderPayloadDates on the zone-resolved instants (field-level 400). Server schemas carry no date refinements at all now. - Surface endsAt validation errors in the locked (downgraded) dialog state, where the Ends-on input does not render but its hidden value still validates — previously the submit could block with no visible cause. - Gate next-fire-date moves for downgraded recurring reminders: moving alertDateTime re-anchors the series, so it now counts as a recurrence change (message/recipient edits round-trip the prefilled date unchanged and still pass). - DB CHECK constraint for the cadence invariant (both cadence fields null, or both present with a positive interval), amended into the unreleased migration; re-verified on a clean Postgres 16 (deploy + drift + violation probes both rejected). - totalPages divides by the clamped page size the query actually used. - Disabled Repeat label: text-gray-500 for WCAG AA contrast. Tests: 6 new/updated (calendar-day ordering, same-day west-of-UTC pass, resolved ordering 400, server-schema no-refinement contract).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsx`:
- Around line 398-405: The locked-state endsAt validation message in
SetOrEditReminderDialog is not announced to assistive technologies because it is
rendered outside any input association. Update the error paragraph in the locked
branch to use an alert/live region, and ensure the endsAt message remains
reachable for screen readers even when the “Ends on” field is not rendered. Keep
the change localized to SetOrEditReminderDialog and the endsAt error rendering
logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc15e371-3d67-460c-8bd3-db6a81262e44
📒 Files selected for processing (6)
apps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.schema.test.tsapps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsxapps/webapp/app/modules/asset-reminder/service.server.tsapps/webapp/app/modules/asset-reminder/utils.server.test.tsapps/webapp/app/modules/asset-reminder/utils.server.tspackages/database/prisma/migrations/20260702120000_add_recurring_reminders/migration.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.schema.test.ts
- apps/webapp/app/modules/asset-reminder/utils.server.test.ts
- apps/webapp/app/modules/asset-reminder/utils.server.ts
- apps/webapp/app/modules/asset-reminder/service.server.ts
added 2 commits
July 2, 2026 19:20
role="alert" on the downgraded-dialog error paragraph: it isn't tied to a rendered input (the Ends-on field is hidden in that state), so screen readers otherwise get no notification when a submit is blocked.
- Extract ReminderRecurrenceFields (Repeat select, locked/upgrade state,
Ends-on input) into its own component (no-giant-component).
- Replace the reset-on-open effect with the adjust-state-during-render
pattern (no-effect-event-handler); the reset now applies in the same
render pass. The pattern's prop-seeded useState trips no-derived-useState,
documented as an accepted residual per CLAUDE.md ('repeat' is user-editable
state that must reset on open, not derivable state).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/components/asset-reminder/reminder-recurrence-fields.tsx (1)
25-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on exported type/component.
ReminderRecurrenceFieldsPropsand the default-exportedReminderRecurrenceFieldscomponent lack a JSDoc block describing their purpose/parameters/return value (only per-field inline comments exist on the type). The file-level doc at the top explains the file's purpose but doesn't substitute for per-entity docs.As per coding guidelines, "every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
📝 Proposed doc addition
+/** + * Props for {`@link` ReminderRecurrenceFields}. + */ type ReminderRecurrenceFieldsProps = { /** Whether the workspace tier includes recurring reminders. */ canUseRecurringReminders: boolean; ... }; ... +/** + * Renders the Repeat select and optional "Ends on" date input for the + * set/edit reminder dialog, locking the controls with an upgrade nudge + * when the workspace tier lacks recurring reminders. + * + * `@param` props - See {`@link` ReminderRecurrenceFieldsProps}. + * `@returns` The recurrence form fields fragment. + */ export default function ReminderRecurrenceFields({Also applies to: 57-67
🤖 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 `@apps/webapp/app/components/asset-reminder/reminder-recurrence-fields.tsx` around lines 25 - 42, Add JSDoc blocks for both exported symbols: the ReminderRecurrenceFieldsProps type and the default-exported ReminderRecurrenceFields component. Describe the props/purpose for ReminderRecurrenceFieldsProps and document the component’s role plus its parameters/returned JSX in ReminderRecurrenceFields, keeping the existing inline field comments intact.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/webapp/app/components/asset-reminder/reminder-recurrence-fields.tsx`:
- Around line 25-42: Add JSDoc blocks for both exported symbols: the
ReminderRecurrenceFieldsProps type and the default-exported
ReminderRecurrenceFields component. Describe the props/purpose for
ReminderRecurrenceFieldsProps and document the component’s role plus its
parameters/returned JSX in ReminderRecurrenceFields, keeping the existing inline
field comments intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 306a99ab-e82b-460f-b23c-1fb7428b8676
📒 Files selected for processing (2)
apps/webapp/app/components/asset-reminder/reminder-recurrence-fields.tsxapps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/components/asset-reminder/set-or-edit-reminder-dialog.tsx
Addresses the CodeRabbit nitpick on the extracted component: exported symbols require JSDoc per the repo documentation guidelines.
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
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
1 participant
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.
Closes #2659. Also addresses the long-standing recurrence asks in #483, #1445 and #2520 (calibration / service-interval workflows that today require manually re-creating a reminder after each fire).
What
Reminders can now repeat on a cadence: daily, weekly, every 2 weeks, monthly, every 3 months, every 6 months, or yearly, with an optional end date. One-time reminders are unchanged and stay available to everyone; recurrence is a paid-tier capability (see Gating).
Design: one row per series,
alertDateTime= next occurrenceA recurring reminder is the existing
AssetReminderrow. When it fires, the worker advancesalertDateTimein place to the next occurrence and schedules the next job. This keeps every existing surface correct with no forked logic:gte: nowfilters)Fire history remains visible as asset notes (one per fire, now including the next scheduled date).
Scheduling: self-rescheduling chain, no cron
noScheduling: trueis untouched. Recurrence is a self-reschedulingsendAfterchain, the same in-house pattern the audit reminder chain (24h → 4h → 1h → overdue) and the booking checkin → overdue hop already use, hardened with:updateManyguarded on the oldalertDateTime) so concurrent workers/reconciliation/edits can never double-advance (relevant under bluegreen deploys where two machines briefly poll)pg-boss 9 retention is safe for far-future occurrences:
keepUntilanchors tostartAfter, so a job scheduled months out is not reaped (verified in the vendored source).Timezone: the cadence is wall-clock stable in the IANA zone captured from client hints at create/edit (luxon; DST-correct). Month-end anniversaries clamp (Jan 31 → Feb 28 and the advanced date becomes the new anchor) — intended v1 behavior, covered by tests. Catch-up policy: occurrences missed during downtime are skipped, never bursted.
Gating
canUseRecurringReminderstier flag (TierLimitdefault false, enabled fortier_1/tier_2;CustomTierLimitdefault true), following thecanHideShelfBrandingprecedent, asserted server-side at both mutation entry points.ENABLE_PREMIUM_FEATURES=false): everything enabled, per the existing convention.Migrations
Two migrations mirroring the
canImportNRM/canHideShelfBrandingpair: enum + 4 nullableAssetRemindercolumns + both tier-table booleans, then theUPDATEenabling paid tiers. Verified on a clean Postgres 16:prisma migrate deployapplies cleanly andmigrate statusreports zero drift; a live-Prisma script additionally exercised the reconcile query and the CAS advance against the real database.Drive-by fixes (each on a line this change already touches)
createAssetRemindernow runsassertAssetsBelongToOrgbefore creating (org-scope-user-supplied-ids rule: create paths too)totalPagesdivided by the rawper_pageparam →Infinitywhen absent; now uses the cookie-resolvedperPagedesc(showed the two furthest-future reminders instead of the next two)new Date()once at module load; now validated at parse timeDeliberate non-goals (v1)
Tests
First tests for the asset-reminder module (44 new): recurrence math (DST both directions, month-end clamp, catch-up skip, end-date termination, invalid-zone fallback, interval clamp), chain advance/reconcile (CAS, grace window, per-row isolation, tier pause, fail-open), worker handler (advance-before-notify order, stale-job guard, orphan re-arm, rethrow path, one-shot unchanged), form schema parsing (empty
endsAt, missingrepeat, past dates), and theendsAttimezone round-trip.pnpm webapp:validategreen (3073 tests).Verification honesty
Statically verified + live-DB-verified as above. Not click-through-verified in a browser: the shared dev/QA database cannot take this migration (it is on a diverged schema), so reviewers should expect to exercise the dialog in their own environment. The UI changes follow existing primitives (Radix Select, locked-control precedent,
DateS).🤖 Generated with Claude Code
Summary by CodeRabbit