Implement push notifications and frontend for monthly dumps#110
Merged
Conversation
Implemented Push Notifications on Successful Dump Creation
Implemented Frontend Interface for Monthly Dumps
|
|
Contributor
|
Warning Review limit reached
More reviews will be available in 55 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (48)
WalkthroughThis PR adds a comprehensive monthly recap feature ("monthly dumps") with backend notification enqueueing and frontend viewing/curation. It introduces a monthly dump queue processor that notifies users of completed dumps, backend services for fetching and caching dumps, frontend components for viewing slides (video/audio/photo/grid), and integration with the capture screen via an animated banner. ChangesMonthly Dumps Feature
Sequence Diagram(s)sequenceDiagram
participant Backend as Backend Queue
participant NotifSvc as Notification Service
participant FrontendHook as Frontend useMonthlyDump
participant Supabase as Supabase
participant User as User
Backend->>Backend: process_queue() reads messages
Backend->>Backend: _process_message(): fetch entries,<br/>build dump, return user_id
Backend->>NotifSvc: enqueue_monthly_dump_notifications(user_ids, month)
NotifSvc->>Supabase: filter users by push_notifications<br/>fetch push tokens
NotifSvc->>NotifSvc: construct month-specific payload<br/>with page_url = /monthly-dumps/{month}
NotifSvc->>Supabase: enqueue push notification
Supabase-->>User: push notification received
User->>FrontendHook: tap banner / navigate to dump
FrontendHook->>FrontendHook: isEnabled = true?<br/>(first 4 or last 3 days)
FrontendHook->>Supabase: fetch monthly_dumps (completed)
Supabase-->>FrontendHook: return dump + slides
FrontendHook->>FrontendHook: transform URLs<br/>merge pending local slides
FrontendHook-->>User: render slides<br/>video/audio/photo/grid-prompt
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 8
🧹 Nitpick comments (5)
frontend/hooks/phone-number/use-manage-phone-sheet.ts (2)
7-7: ⚡ Quick winAdd a docstring to this custom hook.
New hooks in
frontend/**/hooks/**should include a docstring explaining purpose and behavior.As per coding guidelines
frontend/**/hooks/**/*.{ts,tsx}: “ALL custom hooks created must have docstrings”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/phone-number/use-manage-phone-sheet.ts` at line 7, Add a JSDoc-style docstring above the custom hook function useManagePhoneSheet explaining its purpose and behavior: what the hook manages (e.g., opening/closing the phone management sheet, form state, side effects), the core behaviors (inputs, outputs, returned values or handlers), and any important invariants or lifecycle notes; use a /** ... */ block placed immediately above the export function useManagePhoneSheet() and include short `@returns` and `@example` or `@remarks` lines if applicable to clarify the hook's API.
41-41: ⚡ Quick winAvoid swallowing async errors silently.
The empty catch hides failures and makes prompt-state bugs hard to diagnose. At minimum, set a deterministic fallback and log/report once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/phone-number/use-manage-phone-sheet.ts` at line 41, The call to checkShouldShowPhonePrompt() currently swallows any error; change its catch to log or report the error and apply a deterministic fallback so failures don't leave the UI in an indeterminate state — e.g., replace the empty catch with one that calls your logger/reporting utility (or console.error) with the caught error and then sets the prompt visibility fallback (for example via whatever state setter controls the sheet) so the app has a known behavior when checkShouldShowPhonePrompt() rejects.frontend/app/capture/__tests__/banner-visibility.test.tsx (1)
106-113: ⚡ Quick winAssert the toggle outcome, not just that pressing doesn't crash.
Because the banner mock always renders
monthly-dump-banner, the third test still passes if the press handler stops toggling anything. Make the mock reflect the banner's visible/open prop (or render conditionally) and assert the before/after state so this catches regressions in the actual interaction.Also applies to: 152-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/capture/__tests__/banner-visibility.test.tsx` around lines 106 - 113, The mock for the MonthlyDumpBanner currently always renders the testID "monthly-dump-banner", so presses can’t fail the visibility assertion; update the jest.mock block to accept props (e.g., ({ visible, open })) and render conditionally (return <View testID="monthly-dump-banner" /> only when visible/open is true), then update the tests that simulate the press to assert the banner's presence before the press and its absence (or vice versa) after the press using queryByTestId/getByTestId; locate the jest.mock block and the tests that press the toggle and replace their current existence-only assertions with before/after assertions against "monthly-dump-banner".backend/tests/test_monthly_dump_queue_notifications.py (1)
15-16: ⚡ Quick winPatch constructor dependencies before creating the service.
MonthlyDumpQueueService()still callsget_supabase_client(),QueueService(),MonthlyDumpService(), andMonthlyDumpController()before the test swaps in mocks, so these cases can fail on env/config setup instead of the queue logic under test.🧪 Suggested setup
- service = MonthlyDumpQueueService() + with patch("services.queues.monthly_dump_queue_service.get_supabase_client", return_value=MagicMock()), \ + patch("services.queues.monthly_dump_queue_service.QueueService"), \ + patch("services.queues.monthly_dump_queue_service.MonthlyDumpService"), \ + patch("services.queues.monthly_dump_queue_service.MonthlyDumpController"): + service = MonthlyDumpQueueService()Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_monthly_dump_queue_notifications.py` around lines 15 - 16, The test creates a MonthlyDumpQueueService instance before its external dependencies are patched, causing real calls to get_supabase_client(), QueueService(), MonthlyDumpService(), and MonthlyDumpController; change the test (and the other occurrence at lines ~56-57) to patch/monkeypatch/mock these constructors/functions first (patch get_supabase_client, QueueService, MonthlyDumpService, MonthlyDumpController) and only then instantiate MonthlyDumpQueueService in test_monthly_dump_queue_processes_missing_entries_as_failed so the service uses the mocked dependencies during the test.frontend/hooks/use-monthly-dump.ts (1)
58-58: ⚡ Quick winAdd a docstring for this exported hook.
useMonthlyDumphas a non-obvious contract: it computes a month window, conditionally enables fetching, and merges cached local slides. A short JSDoc here will save callers from re-deriving those rules.Suggested docstring
+/** + * Returns the current user's monthly dump data. + * If `requestedMonth` is omitted, fetching is only enabled during the + * first 4 days of a month or the last 3 days of the current month. + */ export function useMonthlyDump(requestedMonth?: string): UseMonthlyDumpResult {As per coding guidelines "ALL custom hooks created must have docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/hooks/use-monthly-dump.ts` at line 58, Add a JSDoc block above the exported hook function useMonthlyDump that documents its public contract: explain the requestedMonth? parameter (format/expected values and that it will be used to compute the month window), describe that the hook conditionally enables data fetching (when/why fetch is skipped), state that it merges cached local slides with fetched data, and list the shape/meaning of the returned UseMonthlyDumpResult (key fields consumers will use). Keep it short, focused, and placed directly above the useMonthlyDump function declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/services/notification_enqueue_service.py`:
- Around line 298-299: Replace interpolated log messages that embed dynamic
values with structured logging using the extra dict: for the logger.info call
that currently uses f"No recipients with push_notifications enabled for
{month_name} dump batch" (and the similar logger calls around the same block at
lines 304-305), pass a static message string and supply the dynamic value(s) via
extra (e.g. logger.info("No recipients with push_notifications enabled for dump
batch", extra={"month_name": month_name})). Locate the calls by searching for
the logger.info statements in the notification enqueue routine (the function
containing the shown lines) and update all such interpolated logs to use extra
with descriptive keys for each dynamic variable.
In `@backend/services/queues/monthly_dump_queue_service.py`:
- Around line 116-119: The current logic calls
NotificationEnqueueService.enqueue_monthly_dump_notifications after the
monthly-dump job is marked completed and the source queue message deleted (in
process_queue()), which can lose notifications if enqueue fails; fix by making
the enqueue durable: either move the enqueue call into the per-message retry
path inside process_queue() so the source message is not acknowledged/deleted
until enqueue_monthly_dump_notifications(user_ids, msg_month) succeeds, or
persist a notification-pending record (e.g., set a "notification_pending"
flag/state tied to the monthly dump/job or each msg_month in the DB) before
deleting/marking the source message and have a separate retry worker that reads
those pending records and calls
NotificationEnqueueService.enqueue_monthly_dump_notifications until success;
update the code paths that currently call enqueue_monthly_dump_notifications
(including where month_to_user_ids is iterated) to implement one of these
durable approaches and ensure failures are retried or retrievable.
In `@frontend/app/capture/index.tsx`:
- Around line 131-132: canShowRecap currently returns true when month is present
and isEnabled is true even if no dump exists; update the boolean expression so
the recap CTA only appears when a dump actually exists by requiring hasDump to
be truthy as well (i.e., change the condition involving canShowRecap to include
hasDump alongside month and isEnabled — reference symbols: canShowRecap, month,
hasDump, isEnabled).
In `@frontend/app/monthly-dumps/`[month].tsx:
- Around line 33-36: The allSlides useMemo always appends a { type:
'grid_prompt' } slide even when there is no monthly dump, causing
MonthlyDumpService.processGridQueueItem() to fail; update the useMemo
(allSlides) to only append the grid_prompt when the dump exists (use the hasDump
flag returned from useMonthlyDump(month) or equivalent) so that when hasDump is
false you return just baseSlides (or an empty state UI) and do not enqueue the
grid prompt.
In `@frontend/hooks/phone-number/use-manage-phone-sheet.ts`:
- Line 1: The import list in use-manage-phone-sheet.ts includes an unused symbol
useCallback; edit the top-level import statement that currently reads "import {
useState, useEffect, useCallback } from 'react';" and remove useCallback so only
used hooks (useState, useEffect) are imported to satisfy lint rules and
eliminate the unused-import error.
- Line 16: The early return in the use-manage-phone-sheet hook (the clause
checking if (!user?.id)) leaves the previous showPhoneSheet state unchanged and
can leave the sheet visible after logout; update that branch to explicitly reset
the visibility (call the state updater that controls showPhoneSheet, e.g.,
setShowPhoneSheet(false) or equivalent) before returning so the sheet is closed
whenever there is no user id.
- Around line 12-36: The Supabase table read inside useEffect (the
supabase.from(TABLES.PHONE_NUMBER_UPDATES).select... call in
checkShouldShowPhonePrompt) must be refactored to use a useQuery hook instead:
create a query (e.g., useQuery(['phone_updates', user?.id], fetcher)) that calls
supabase.from(TABLES.PHONE_NUMBER_UPDATES).select('id').eq('user_id',
user.id).maybeSingle(), import and use that hook outside of useEffect, and read
pendingRecord from the query result inside useEffect; keep getPhonePromptState
and the cancelled/setShowPhoneSheet logic in checkShouldShowPhonePrompt but
replace the inline Supabase call with the query result (handle loading and
undefined user.id by early returning). Ensure the query key includes user.id and
avoid calling setShowPhoneSheet when the effect is cancelled or when the query
is still loading.
In `@frontend/hooks/use-monthly-dump.ts`:
- Around line 61-85: The hook currently trusts requestedMonth and may forward a
malformed value into downstream calls; validate requestedMonth with your
existing zod schema (the same one used by getCachedMonthlyDump) before using it
in the useMemo that computes isEnabled and dumpMonth — if validation fails,
return { isEnabled: false, dumpMonth: '' } (or undefined) so no backend call is
made; update the useMemo block that references requestedMonth and the returned
values isEnabled/dumpMonth accordingly and ensure getCachedMonthlyDump is only
invoked when the validated dumpMonth passes zod.
---
Nitpick comments:
In `@backend/tests/test_monthly_dump_queue_notifications.py`:
- Around line 15-16: The test creates a MonthlyDumpQueueService instance before
its external dependencies are patched, causing real calls to
get_supabase_client(), QueueService(), MonthlyDumpService(), and
MonthlyDumpController; change the test (and the other occurrence at lines
~56-57) to patch/monkeypatch/mock these constructors/functions first (patch
get_supabase_client, QueueService, MonthlyDumpService, MonthlyDumpController)
and only then instantiate MonthlyDumpQueueService in
test_monthly_dump_queue_processes_missing_entries_as_failed so the service uses
the mocked dependencies during the test.
In `@frontend/app/capture/__tests__/banner-visibility.test.tsx`:
- Around line 106-113: The mock for the MonthlyDumpBanner currently always
renders the testID "monthly-dump-banner", so presses can’t fail the visibility
assertion; update the jest.mock block to accept props (e.g., ({ visible, open
})) and render conditionally (return <View testID="monthly-dump-banner" /> only
when visible/open is true), then update the tests that simulate the press to
assert the banner's presence before the press and its absence (or vice versa)
after the press using queryByTestId/getByTestId; locate the jest.mock block and
the tests that press the toggle and replace their current existence-only
assertions with before/after assertions against "monthly-dump-banner".
In `@frontend/hooks/phone-number/use-manage-phone-sheet.ts`:
- Line 7: Add a JSDoc-style docstring above the custom hook function
useManagePhoneSheet explaining its purpose and behavior: what the hook manages
(e.g., opening/closing the phone management sheet, form state, side effects),
the core behaviors (inputs, outputs, returned values or handlers), and any
important invariants or lifecycle notes; use a /** ... */ block placed
immediately above the export function useManagePhoneSheet() and include short
`@returns` and `@example` or `@remarks` lines if applicable to clarify the hook's API.
- Line 41: The call to checkShouldShowPhonePrompt() currently swallows any
error; change its catch to log or report the error and apply a deterministic
fallback so failures don't leave the UI in an indeterminate state — e.g.,
replace the empty catch with one that calls your logger/reporting utility (or
console.error) with the caught error and then sets the prompt visibility
fallback (for example via whatever state setter controls the sheet) so the app
has a known behavior when checkShouldShowPhonePrompt() rejects.
In `@frontend/hooks/use-monthly-dump.ts`:
- Line 58: Add a JSDoc block above the exported hook function useMonthlyDump
that documents its public contract: explain the requestedMonth? parameter
(format/expected values and that it will be used to compute the month window),
describe that the hook conditionally enables data fetching (when/why fetch is
skipped), state that it merges cached local slides with fetched data, and list
the shape/meaning of the returned UseMonthlyDumpResult (key fields consumers
will use). Keep it short, focused, and placed directly above the useMonthlyDump
function declaration.
🪄 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: 2e549207-f4a9-4faa-bafd-c8ba6a1a7dfb
⛔ Files ignored due to path filters (7)
supabase/.temp/cli-latestis excluded by!**/.temp/**supabase/.temp/gotrue-versionis excluded by!**/.temp/**supabase/.temp/pooler-urlis excluded by!**/.temp/**supabase/.temp/postgres-versionis excluded by!**/.temp/**supabase/.temp/project-refis excluded by!**/.temp/**supabase/.temp/rest-versionis excluded by!**/.temp/**supabase/.temp/storage-versionis excluded by!**/.temp/**
📒 Files selected for processing (24)
backend/controllers/monthly_dump_controller.pybackend/services/notification_enqueue_service.pybackend/services/queues/monthly_dump_queue_service.pybackend/tests/test_monthly_dump_queue_notifications.pyfrontend/AGENTS.mdfrontend/app/capture/__tests__/banner-visibility.test.tsxfrontend/app/capture/index.tsxfrontend/app/monthly-dumps/[month].tsxfrontend/components/capture/capture-header.tsxfrontend/components/date-container.tsxfrontend/components/monthly-dumps/monthly-dump-audio-slide.tsxfrontend/components/monthly-dumps/monthly-dump-banner.tsxfrontend/components/monthly-dumps/monthly-dump-grid-prompt-slide.tsxfrontend/components/monthly-dumps/monthly-dump-progress-bar-item.tsxfrontend/components/monthly-dumps/monthly-dump-video-slide.tsxfrontend/components/monthly-dumps/photo-grid-picker.tsxfrontend/constants/supabase.tsfrontend/hooks/__tests__/use-monthly-dump.test.tsxfrontend/hooks/phone-number/use-manage-phone-sheet.tsfrontend/hooks/use-monthly-dump.tsfrontend/jest.setup.jsfrontend/services/monthly-dump-service.tsfrontend/supabase/migrations/20260420043600_init_monthly_dump_next_run.sqlfrontend/types/database.ts
Codex/monthly dump
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.
Summary by CodeRabbit
New Features