Completed Monthly Dump Feature and Custom Grid#111
Merged
Conversation
|
|
Contributor
WalkthroughAdds backend monthly-dump completion filtering, a month-batched notification enqueue flow, and frontend monthly-dumps UI: viewer with timed slides, recap banner, and a full photo grid picker (2x2/2x3) with capture, gallery/entries sources, caching, and optimistic grid creation; plus AI skill docs. ChangesMonthly Dumps Feature Implementation
AI Skills Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 13
🧹 Nitpick comments (11)
frontend/maestro/monthly-dumps/create-grid-from-gallery-flow.yaml (1)
9-10: 💤 Low valueMisleading test ID naming.
The test ID
monthly-dump-grid-open-entries-buttonis used in the gallery flow, which is confusing since this flow uses Gallery as the source, not entries. The button appears to be a generic "open picker" action but the name suggests it's specific to entries. Consider renaming to something likemonthly-dump-grid-open-picker-buttonormonthly-dump-grid-expand-source-button.Also applies to: 17-17, 21-21, 25-25, 29-29, 33-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/maestro/monthly-dumps/create-grid-from-gallery-flow.yaml` around lines 9 - 10, Replace the misleading test ID monthly-dump-grid-open-entries-button (and its repeated occurrences at the other positions) with a name that reflects the Gallery source and generic picker action, e.g. monthly-dump-grid-open-picker-button or monthly-dump-grid-expand-source-button; update every instance of monthly-dump-grid-open-entries-button used in this YAML flow (the tapOn keys at lines where it appears) so the identifier consistently reflects "picker" or "source" rather than "entries" to avoid confusion.frontend/maestro/monthly-dumps/create-grid-from-entries-flow.yaml (2)
4-4: ⚡ Quick winHardcoded date will become stale.
The flow uses
2026-05which will eventually become invalid as time passes. Consider parameterizing the date or using a dynamic value that references "current month" or "previous month".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/maestro/monthly-dumps/create-grid-from-entries-flow.yaml` at line 4, The openLink value currently hardcodes "keepsafe://monthly-dumps/2026-05" which will become stale; change the openLink in create-grid-from-entries-flow.yaml to use a parameter or runtime expression (e.g., a flow variable like MONTH or an expression that computes current/previous month) instead of a literal date, update any callers or defaults to provide that variable, and ensure downstream steps that parse the path expect the same variable format.
37-37: ⚡ Quick winVerify grid creation success beyond UI disappearance.
The flow only asserts that "Create Your Dump" is no longer visible, but doesn't verify the grid was actually created successfully. Consider adding assertions for:
- Grid image visibility
- Success toast/message
- Navigation to viewing state
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/maestro/monthly-dumps/create-grid-from-entries-flow.yaml` at line 37, The current step only uses assertNotVisible "Create Your Dump" which only verifies the modal closed; update the flow to also assert the grid was created by adding checks after the creation step: verify the grid image is visible (e.g., an assertGridImageVisible check targeting the new grid thumbnail selector), assert a success toast/message is shown (e.g., assertToastVisible with the expected success text), and assert navigation to the grid viewing state (e.g., assertNavigationToViewingState or an assertion that the URL or page title corresponds to the newly created grid). Ensure these additional assertions run immediately after the existing assertNotVisible step so the flow confirms creation, UI feedback, and navigation..agents/skills/frontend-design/SKILL.md (2)
42-42: ⚡ Quick winReplace clichéd phrase with more distinctive language.
The phrase "thinking outside the box" is flagged as a cliché. This is particularly noteworthy given that this PR also introduces a writing quality skill (stop-slop) that prohibits predictable AI patterns. Consider more specific, evocative language that models the creativity you're asking the agent to demonstrate.
✍️ Suggested alternatives
-Remember: Claude is capable of extraordinary creative work. Don't hold back, show what can truly be created when thinking outside the box and committing fully to a distinctive vision. +Remember: Claude is capable of extraordinary creative work. Don't hold back, show what can truly be created when breaking conventions and committing fully to a distinctive vision.Or:
-Remember: Claude is capable of extraordinary creative work. Don't hold back, show what can truly be created when thinking outside the box and committing fully to a distinctive vision. +Remember: Claude is capable of extraordinary creative work. Don't hold back, show what can truly be created when challenging design norms and committing fully to a distinctive vision.🤖 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 @.agents/skills/frontend-design/SKILL.md at line 42, Replace the clichéd phrase "thinking outside the box" in the SKILL.md sentence that begins "Remember: Claude is capable..." with a more specific, evocative alternative; edit that line to use language such as "forge unexpected connections," "reimagine boundaries," or "make bold, original leaps" (or another distinctive phrase of similar tone) so the guidance models the creative, non-predictable style the stop-slop skill enforces.
32-32: ⚡ Quick winSpecify which Motion library to use.
The instruction "Use Motion library for React when available" is ambiguous. Multiple popular React animation libraries exist (Framer Motion, React Spring, React Motion). Specify which library to use for consistency.
📝 Suggested clarification
-- **Motion**: Use animations for effects and micro-interactions. Prioritize CSS-only solutions for HTML. Use Motion library for React when available. Focus on high-impact moments: one well-orchestrated page load with staggered reveals (animation-delay) creates more delight than scattered micro-interactions. Use scroll-triggering and hover states that surprise. +- **Motion**: Use animations for effects and micro-interactions. Prioritize CSS-only solutions for HTML. Use Framer Motion for React when available. Focus on high-impact moments: one well-orchestrated page load with staggered reveals (animation-delay) creates more delight than scattered micro-interactions. Use scroll-triggering and hover states that surprise.🤖 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 @.agents/skills/frontend-design/SKILL.md at line 32, Update the ambiguous "Use Motion library for React when available" line to explicitly name the chosen library (e.g., "Use Framer Motion for React when available") so guidance is consistent; locate the phrase "Use Motion library for React when available" in SKILL.md and replace it with the chosen library name (reference symbol: the sentence starting with "Use Motion library for React when available"), and keep the rest of the guidance about CSS-first, staggered reveals, scroll-triggering, and hover states unchanged.frontend/components/monthly-dumps/monthly-dump-grid-icons.tsx (1)
26-41: ⚡ Quick winRemove or utilize the unused
mutedColorprop.The
mutedColorprop is destructured but never used in the rendering logic. Either remove it from the prop interface or incorporate it into the icon rendering.♻️ Remove unused prop
type GridIconProps = { size?: number; color?: string; - mutedColor?: string; }; export function MonthlyDumpGrid2x2Icon({ size = 22, color = '`#F8FAFC`', - mutedColor = 'rgba(248,250,252,0.28)', }: GridIconProps) {Apply the same change to
MonthlyDumpGrid2x3Icon(lines 43-46).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/monthly-dumps/monthly-dump-grid-icons.tsx` around lines 26 - 41, The mutedColor prop is destructured but unused; either remove it from the component signature and the GridIconProps type or apply it to some GridCell fills (e.g., pass mutedColor to one or more GridCell components to render muted cells). Update MonthlyDumpGrid2x2Icon and MonthlyDumpGrid2x3Icon accordingly, ensuring any change to the prop list is mirrored in GridIconProps and that GridCell uses the correct color prop when you choose to utilize mutedColor.frontend/components/monthly-dumps/monthly-dump-video-slide.tsx (1)
35-54: ⚡ Quick winRefactor conditional to check error condition first.
The component checks
!hasPlaybackErrorbefore rendering the video, but the coding guideline requires evaluating negative conditions first with early returns.As per coding guidelines: Always evaluate negative conditions first (e.g., use early returns with
if (!condition) return;instead ofif (condition) { doSomething(); }).♻️ Refactor to check error condition first
- if (!hasPlaybackError) { + if (hasPlaybackError) { + return ( + <View style={styles.fallbackContainer}> + <Text style={styles.fallbackTitle}>Video cannot be played</Text> + <Text style={styles.fallbackSubtitle}>This clip is unavailable on this device.</Text> + </View> + ); + } + + return ( <View style={styles.videoContainer}> <VideoView player={player} style={styles.media} contentFit="cover" /> {isLoading ? ( <BlurView intensity={48} tint="dark" style={styles.loadingOverlay}> <ActivityIndicator size="large" color="white" /> </BlurView> ) : null} </View> - ); - } - - return ( - <View style={styles.fallbackContainer}> - <Text style={styles.fallbackTitle}>Video cannot be played</Text> - <Text style={styles.fallbackSubtitle}>This clip is unavailable on this device.</Text> - </View> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/monthly-dumps/monthly-dump-video-slide.tsx` around lines 35 - 54, The render logic currently checks !hasPlaybackError and nests the main video UI, but per guidelines invert the condition to early-return the error/fallback UI first; update the component (monthly-dump-video-slide) to if (hasPlaybackError) return the <View> with styles.fallbackContainer and the two <Text> elements, then proceed to render the <View style={styles.videoContainer}> with VideoView (player), the isLoading conditional showing BlurView and ActivityIndicator, ensuring references to hasPlaybackError, VideoView, isLoading, BlurView, ActivityIndicator, and styles remain unchanged.frontend/components/monthly-dumps/grid-image-picker-bottom-tray.tsx (1)
18-34: ⚡ Quick winAdd accessibility labels to icon-only buttons.
These actions are icon-only with no
accessibilityLabel/accessibilityRole, so screen-reader users can't identify them.testIDis for tests, not assistive tech.♿ Proposed fix
<TouchableOpacity testID="monthly-dump-grid-open-entries-button" + accessibilityRole="button" + accessibilityLabel="Add photo from entries" activeOpacity={0.85} onPress={onOpenEntries} style={styles.trayActionButton} > <ImagePlus size={22} color="`#F8FAFC`" strokeWidth={2.2} /> </TouchableOpacity> <TouchableOpacity testID="monthly-dump-grid-open-camera-button" + accessibilityRole="button" + accessibilityLabel="Capture photo with camera" activeOpacity={0.85} onPress={onOpenCamera} style={styles.trayActionButton} > <Camera size={20} color="`#F8FAFC`" strokeWidth={2.2} /> </TouchableOpacity>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/monthly-dumps/grid-image-picker-bottom-tray.tsx` around lines 18 - 34, The two icon-only TouchableOpacity buttons (the ones rendering ImagePlus and Camera and using testIDs "monthly-dump-grid-open-entries-button" and "monthly-dump-grid-open-camera-button" with handlers onOpenEntries and onOpenCamera) need accessibility props so screen readers can identify them; add accessibilityLabel (e.g., "Open entries" and "Open camera"), set accessibilityRole="button", and optionally add accessibilityHint for extra context, while keeping existing styles.trayActionButton and handlers intact.frontend/app/monthly-dumps/[month].tsx (1)
102-103: ⚡ Quick winRemove debug log from the slide effect.
logger.info("current slide", { currentSlide })runs on every slide transition and emits the full slide payload (including URLs). This looks like a leftover debug artifact and adds noise on the hot navigation path.♻️ Proposed change
const currentSlide = allSlides[currentIndex]; - logger.info("current slide", { currentSlide }) if (!currentSlide || currentSlide.type === 'grid_prompt') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/monthly-dumps/`[month].tsx around lines 102 - 103, Remove the noisy debug log in the slide effect: delete or disable the logger.info("current slide", { currentSlide }) call inside the component effect that computes currentSlide (referenced as currentSlide and currentIndex). If you still need telemetry, replace it with a privacy-safe, low-frequency log (e.g., logger.debug with only index or a hash) or wrap the log behind a development-only guard so it does not run on every slide transition.frontend/app/capture/__tests__/banner-visibility.test.tsx (1)
152-178: 💤 Low valueTest asserts nothing after the press.
After pressing
banner-trigger-button, there is no assertion verifying the toggle effect — the test only confirms the screen renders and the press doesn't crash. Consider asserting an observable consequence (e.g., a toggledtestID/accessibilityState, orpointerEventschange) so the test name reflects what it verifies. As-is it's a smoke test, not a toggle test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/capture/__tests__/banner-visibility.test.tsx` around lines 152 - 178, The test currently presses the banner trigger but asserts nothing about visibility; update the test for CaptureScreen to verify an observable consequence after pressing 'banner-trigger-button' — e.g., after the act/fireEvent.press, assert that the banner's testID 'monthly-dump-banner' has changed (presence/absence via queryByTestId), or check a toggled prop such as accessibilityState.expanded or the banner element's pointerEvents/style to reflect the open state; use getByTestId/queryByTestId on 'monthly-dump-banner' (or add a distinct testID like 'monthly-dump-banner-open' if needed) and assert the expected value so the test actually verifies the toggle behavior.frontend/jest.setup.js (1)
45-54: ⚡ Quick winMake the
lucide-react-nativemock resilient to any icon.This mock only exports
Sparkles,Play,UserPlus, andX. Any component under test that imports a different lucide icon will receiveundefinedand crash with an "Element type is invalid" error. AProxyreturning a stubViewfor every key avoids this brittleness as icon usage grows.♻️ Proposed Proxy-based mock
-jest.mock('lucide-react-native', () => { - const React = require('react'); - const { View } = require('react-native'); - return { - Sparkles: () => <View />, - Play: () => <View />, - UserPlus: () => <View />, - X: () => <View />, - }; -}); +jest.mock('lucide-react-native', () => { + const React = require('react'); + const { View } = require('react-native'); + const Icon = () => <View />; + return new Proxy( + { __esModule: true }, + { get: (target, prop) => (prop in target ? target[prop] : Icon) } + ); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/jest.setup.js` around lines 45 - 54, The current jest.mock for 'lucide-react-native' only defines a few named icon components and breaks when tests import other icons; replace the static object with a Proxy that returns a stub React component (rendering react-native's View) for any accessed property so any named export works. In the mock implementation (inside jest.mock('lucide-react-native', ...)), require('react') and require('react-native') as before, create a generic component that returns <View /> and then return a Proxy that maps any property access to that component (also expose the Proxy as the default export / __esModule if needed) so all icon imports resolve to the stub without throwing.
🤖 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 @.agents/skills/frontend-design/SKILL.md:
- Around line 1-5: The skills-lock.json entry for
skills.frontend-design.skillPath is pointing to
plugins/frontend-design/skills/frontend-design/SKILL.md but the actual skill
file is .agents/skills/frontend-design/SKILL.md; update the mismatch by either
changing the skills-lock.json value for skills.frontend-design.skillPath to
".agents/skills/frontend-design/SKILL.md" or move the SKILL.md file into the
plugins/frontend-design/skills/frontend-design/ directory so the path in
skills-lock.json and the repository file location match; ensure you update any
other references to this skillPath to the chosen canonical path.
- Line 4: Frontmatter in .agents/skills/frontend-design/SKILL.md references a
non-existent LICENSE.txt and skills-lock.json contains wrong paths and some
ambiguous/awkward copy: update the frontmatter license field to point to an
existing license entry or add a LICENSE.txt to the repo; fix skills-lock.json
entries so "frontend-design" points to .agents/skills/frontend-design/SKILL.md
and "stop-slop" points to .agents/skills/stop-slop/SKILL.md; in the SKILL.md
content replace the ambiguous “Use Motion library…” with a specific library name
(e.g., Framer Motion or the intended lib) and reword the typography guideline to
remove the semicolon-separated awkward phrasing; also remove or rephrase the
concluding “thinking outside the box” line to avoid encouraging clichés.
In @.agents/skills/stop-slop/README.md:
- Around line 13-22: Update the fenced code block that shows the directory tree
(the block starting with ```) to include a language identifier by changing the
opening fence to ```text so markdownlint recognizes it as a plain-text code
block; specifically edit the README.md's directory-structure code fence that
contains "stop-slop/ ├── SKILL.md ..." to start with ```text instead of just
```.
In `@backend/services/notification_enqueue_service.py`:
- Around line 296-305: Replace the two early-return logger.info calls that embed
month_name in the message with structured logging using the extra kwarg: for the
check on filtered_recipients (referencing filtered_recipients and month_name)
and the check on push_tokens (referencing push_tokens and month_name) change the
calls to something like logger.info("No recipients with push_notifications
enabled for dump batch", extra={"month_name": month_name}) and logger.info("No
push tokens found for dump batch", extra={"month_name": month_name})
respectively, leaving the surrounding logic and return True intact so dynamic
context is passed via extra instead of interpolated into the message.
In `@backend/tests/test_monthly_dump_queue_notifications.py`:
- Around line 9-16: The test defines an unused mock_supabase fixture and
instantiates MonthlyDumpQueueService() which calls get_supabase_client() and
will fail if SUPABASE_* settings are empty; either remove the unused
mock_supabase fixture or, preferably, patch
services.queues.monthly_dump_queue_service.get_supabase_client to return a
MagicMock before creating MonthlyDumpQueueService so __init__ does not require
real env secrets (use the existing mock_supabase or a monkeypatch/patcher to
replace get_supabase_client during the test).
In `@frontend/components/monthly-dumps/monthly-dump-grid-prompt-slide.tsx`:
- Line 4: The file monthly-dump-grid-prompt-slide.tsx imports Sparkles from
'lucide-react-native' but never uses it; remove the unused Sparkles symbol from
the import statement (keep ArrowRight) in the import line so the module only
imports what is used and eliminates the unused-import lint warning.
In `@frontend/components/monthly-dumps/photo-grid-picker.tsx`:
- Around line 279-291: applyLayoutChange currently passes slots straight to
resizeSlots which truncates by index and can drop non-contiguous filled photos;
before calling resizeSlots (inside applyLayoutChange) compact the filled slots:
take the filtered prev array, extract only non-null slot entries in their
existing order, then pad with nulls up to filtered.length (or up to
nextConfig.requiredPhotos if needed) and pass that compacted array into
resizeSlots so filled photos are preserved when shrinking layouts; reference
functions/vars: applyLayoutChange, resizeSlots, GRID_LAYOUTS, setGridSlots,
nextConfig, removePhotoIds.
In `@frontend/hooks/phone-number/use-manage-phone-sheet.ts`:
- Around line 12-45: The effect currently queries TABLES.PHONE_NUMBER_UPDATES
inside useEffect via checkShouldShowPhonePrompt and uses manual cancelled/.catch
lifecycle handling; move that Supabase read into a useQuery (e.g., create a
query hook that selects id from TABLES.PHONE_NUMBER_UPDATES filtered by user.id)
and derive the showPhoneSheet state from the query result plus
getPhonePromptState, removing the internal async fetch and cancelled flag in
useEffect; also validate user.id with zod before invoking the query and handle
the pendingRecord?.id logic and setShowPhoneSheet updates from the
query/getPhonePromptState results instead of doing the supabase call inside
checkShouldShowPhonePrompt.
- Around line 1-10: Remove the unused import useCallback, add a short docstring
above the useManagePhoneSheet function describing its purpose/returns, and
consolidate the two calls to useAuthContext() into a single destructure (e.g.,
const { user, profile } = useAuthContext()) so both user and profile are
obtained from one call; update imports accordingly to drop useCallback and
ensure the docstring is present for useManagePhoneSheet.
In `@frontend/hooks/use-monthly-dump.ts`:
- Around line 36-41: The fallback bucket in buildSupabasePublicUrl currently
uses STORAGE_BUCKETS.MEDIA causing mismatch with
MonthlyDumpService.resolveStorageTarget; change the default bucket assignment so
that when hasBucketPrefix is false the bucket is STORAGE_BUCKETS.MONTHLY_DUMPS
(i.e., update the bucket variable assignment where possibleBucket/remaining are
computed) so prefix-less paths resolve to MONTHLY_DUMPS instead of MEDIA.
- Around line 57-58: Add a docstring to the custom hook function useMonthlyDump
describing what the hook does, its parameter and return shape; specifically
document the optional requestedMonth parameter (string | null) and the returned
UseMonthlyDumpResult, mirroring the style used in useMonthlyEntries and
useGalleryImages (purpose, args, returns). Place the docstring immediately above
the function declaration for useMonthlyDump so it satisfies the "ALL custom
hooks must have docstrings" guideline.
In `@frontend/services/monthly-dump-service.ts`:
- Around line 286-301: The strict monthlyDumpSlideSchema requiring
duration_seconds >= 1 causes setCachedMonthlyDump (which parses payload.slides)
to throw and make useMonthlyDump fall back when remote slides have missing or
zero duration_seconds; relax the schema to accept missing/zero values by
providing a default or coercion for duration_seconds in monthlyDumpSlideSchema
(or alternatively validate/ensure the backend sets duration_seconds for all
slide types) so that setCachedMonthlyDump no longer rejects freshly fetched
dumps due to missing duration_seconds.
- Around line 264-271: apiFetch is fine as-is (it doesn't double-prefix
BACKEND_URL); instead fix three issues: relax or handle strict slide validation
in setCachedMonthlyDump/monthlyDumpSlideSchema by allowing duration_seconds to
be 0 or optional (or validate per-slide and drop/repair invalid slides before
calling setCachedMonthlyDump) so a single bad slide won't make useMonthlyDump
fall back to cache; unify bucket defaults between buildSupabasePublicUrl and
resolveStorageTarget (make both default to STORAGE_BUCKETS.MONTHLY_DUMPS or
centralize the default constant) so URL resolution is consistent; and ensure
getMonthlyDump always hydrates slide.url via resolveStorageTarget (or populate
url before returning) so hooks that rely on buildSupabasePublicUrl never receive
slides lacking a url.
---
Nitpick comments:
In @.agents/skills/frontend-design/SKILL.md:
- Line 42: Replace the clichéd phrase "thinking outside the box" in the SKILL.md
sentence that begins "Remember: Claude is capable..." with a more specific,
evocative alternative; edit that line to use language such as "forge unexpected
connections," "reimagine boundaries," or "make bold, original leaps" (or another
distinctive phrase of similar tone) so the guidance models the creative,
non-predictable style the stop-slop skill enforces.
- Line 32: Update the ambiguous "Use Motion library for React when available"
line to explicitly name the chosen library (e.g., "Use Framer Motion for React
when available") so guidance is consistent; locate the phrase "Use Motion
library for React when available" in SKILL.md and replace it with the chosen
library name (reference symbol: the sentence starting with "Use Motion library
for React when available"), and keep the rest of the guidance about CSS-first,
staggered reveals, scroll-triggering, and hover states unchanged.
In `@frontend/app/capture/__tests__/banner-visibility.test.tsx`:
- Around line 152-178: The test currently presses the banner trigger but asserts
nothing about visibility; update the test for CaptureScreen to verify an
observable consequence after pressing 'banner-trigger-button' — e.g., after the
act/fireEvent.press, assert that the banner's testID 'monthly-dump-banner' has
changed (presence/absence via queryByTestId), or check a toggled prop such as
accessibilityState.expanded or the banner element's pointerEvents/style to
reflect the open state; use getByTestId/queryByTestId on 'monthly-dump-banner'
(or add a distinct testID like 'monthly-dump-banner-open' if needed) and assert
the expected value so the test actually verifies the toggle behavior.
In `@frontend/app/monthly-dumps/`[month].tsx:
- Around line 102-103: Remove the noisy debug log in the slide effect: delete or
disable the logger.info("current slide", { currentSlide }) call inside the
component effect that computes currentSlide (referenced as currentSlide and
currentIndex). If you still need telemetry, replace it with a privacy-safe,
low-frequency log (e.g., logger.debug with only index or a hash) or wrap the log
behind a development-only guard so it does not run on every slide transition.
In `@frontend/components/monthly-dumps/grid-image-picker-bottom-tray.tsx`:
- Around line 18-34: The two icon-only TouchableOpacity buttons (the ones
rendering ImagePlus and Camera and using testIDs
"monthly-dump-grid-open-entries-button" and
"monthly-dump-grid-open-camera-button" with handlers onOpenEntries and
onOpenCamera) need accessibility props so screen readers can identify them; add
accessibilityLabel (e.g., "Open entries" and "Open camera"), set
accessibilityRole="button", and optionally add accessibilityHint for extra
context, while keeping existing styles.trayActionButton and handlers intact.
In `@frontend/components/monthly-dumps/monthly-dump-grid-icons.tsx`:
- Around line 26-41: The mutedColor prop is destructured but unused; either
remove it from the component signature and the GridIconProps type or apply it to
some GridCell fills (e.g., pass mutedColor to one or more GridCell components to
render muted cells). Update MonthlyDumpGrid2x2Icon and MonthlyDumpGrid2x3Icon
accordingly, ensuring any change to the prop list is mirrored in GridIconProps
and that GridCell uses the correct color prop when you choose to utilize
mutedColor.
In `@frontend/components/monthly-dumps/monthly-dump-video-slide.tsx`:
- Around line 35-54: The render logic currently checks !hasPlaybackError and
nests the main video UI, but per guidelines invert the condition to early-return
the error/fallback UI first; update the component (monthly-dump-video-slide) to
if (hasPlaybackError) return the <View> with styles.fallbackContainer and the
two <Text> elements, then proceed to render the <View
style={styles.videoContainer}> with VideoView (player), the isLoading
conditional showing BlurView and ActivityIndicator, ensuring references to
hasPlaybackError, VideoView, isLoading, BlurView, ActivityIndicator, and styles
remain unchanged.
In `@frontend/jest.setup.js`:
- Around line 45-54: The current jest.mock for 'lucide-react-native' only
defines a few named icon components and breaks when tests import other icons;
replace the static object with a Proxy that returns a stub React component
(rendering react-native's View) for any accessed property so any named export
works. In the mock implementation (inside jest.mock('lucide-react-native',
...)), require('react') and require('react-native') as before, create a generic
component that returns <View /> and then return a Proxy that maps any property
access to that component (also expose the Proxy as the default export /
__esModule if needed) so all icon imports resolve to the stub without throwing.
In `@frontend/maestro/monthly-dumps/create-grid-from-entries-flow.yaml`:
- Line 4: The openLink value currently hardcodes
"keepsafe://monthly-dumps/2026-05" which will become stale; change the openLink
in create-grid-from-entries-flow.yaml to use a parameter or runtime expression
(e.g., a flow variable like MONTH or an expression that computes
current/previous month) instead of a literal date, update any callers or
defaults to provide that variable, and ensure downstream steps that parse the
path expect the same variable format.
- Line 37: The current step only uses assertNotVisible "Create Your Dump" which
only verifies the modal closed; update the flow to also assert the grid was
created by adding checks after the creation step: verify the grid image is
visible (e.g., an assertGridImageVisible check targeting the new grid thumbnail
selector), assert a success toast/message is shown (e.g., assertToastVisible
with the expected success text), and assert navigation to the grid viewing state
(e.g., assertNavigationToViewingState or an assertion that the URL or page title
corresponds to the newly created grid). Ensure these additional assertions run
immediately after the existing assertNotVisible step so the flow confirms
creation, UI feedback, and navigation.
In `@frontend/maestro/monthly-dumps/create-grid-from-gallery-flow.yaml`:
- Around line 9-10: Replace the misleading test ID
monthly-dump-grid-open-entries-button (and its repeated occurrences at the other
positions) with a name that reflects the Gallery source and generic picker
action, e.g. monthly-dump-grid-open-picker-button or
monthly-dump-grid-expand-source-button; update every instance of
monthly-dump-grid-open-entries-button used in this YAML flow (the tapOn keys at
lines where it appears) so the identifier consistently reflects "picker" or
"source" rather than "entries" to avoid confusion.
🪄 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: c5c7ac46-109f-414f-9c9d-09bb73767647
⛔ Files ignored due to path filters (8)
frontend/bun.lockis excluded by!**/*.locksupabase/.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 (54)
.agents/skills/frontend-design/SKILL.md.agents/skills/stop-slop/CHANGELOG.md.agents/skills/stop-slop/LICENSE.agents/skills/stop-slop/README.md.agents/skills/stop-slop/SKILL.md.agents/skills/stop-slop/references/examples.md.agents/skills/stop-slop/references/phrases.md.agents/skills/stop-slop/references/structures.mdbackend/.gitignorebackend/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/index.tsxfrontend/app/monthly-dumps/[month].tsxfrontend/components/capture/capture-header.tsxfrontend/components/date-container.tsxfrontend/components/monthly-dumps/__tests__/photo-grid-picker.test.tsxfrontend/components/monthly-dumps/grid-image-picker-bottom-tray.tsxfrontend/components/monthly-dumps/grid-image-picker-camera-modal.tsxfrontend/components/monthly-dumps/grid-image-picker-capture-canvas.tsxfrontend/components/monthly-dumps/grid-image-picker-cell.tsxfrontend/components/monthly-dumps/grid-image-picker-empty-state.tsxfrontend/components/monthly-dumps/grid-image-picker-layout-popover.tsxfrontend/components/monthly-dumps/grid-image-picker-right-actions.tsxfrontend/components/monthly-dumps/grid-image-picker-selection-pill.tsxfrontend/components/monthly-dumps/grid-image-picker.tsxfrontend/components/monthly-dumps/monthly-dump-audio-slide.tsxfrontend/components/monthly-dumps/monthly-dump-banner.tsxfrontend/components/monthly-dumps/monthly-dump-grid-icons.tsxfrontend/components/monthly-dumps/monthly-dump-grid-prompt-slide.tsxfrontend/components/monthly-dumps/monthly-dump-image-slide.tsxfrontend/components/monthly-dumps/monthly-dump-progress-bar-item.tsxfrontend/components/monthly-dumps/monthly-dump-status-screen.tsxfrontend/components/monthly-dumps/monthly-dump-video-slide.tsxfrontend/components/monthly-dumps/photo-grid-empty-state.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-gallery-images.tsfrontend/hooks/use-monthly-dump.tsfrontend/hooks/use-monthly-entries.tsfrontend/jest.setup.jsfrontend/maestro/monthly-dumps/create-grid-from-entries-flow.yamlfrontend/maestro/monthly-dumps/create-grid-from-gallery-flow.yamlfrontend/package.jsonfrontend/services/monthly-dump-service.tsfrontend/supabase/migrations/20260420043600_init_monthly_dump_next_run.sqlfrontend/types/database.tsskills-lock.json
Comment on lines
+12
to
+45
| useEffect(() => { | ||
| let cancelled = false; | ||
|
|
||
| const checkShouldShowPhonePrompt = async () => { | ||
| if (!user?.id) return; | ||
| if (profile?.phone_number) { | ||
| if (!cancelled) setShowPhoneSheet(false); | ||
| return; | ||
| } | ||
|
|
||
| // If the user already has a pending OTP record, always show the sheet. | ||
| const { data: pendingRecord } = await supabase | ||
| .from(TABLES.PHONE_NUMBER_UPDATES) | ||
| .select('id') | ||
| .eq('user_id', user.id) | ||
| .maybeSingle() as { data: { id: string } | null }; | ||
|
|
||
| if (pendingRecord?.id) { | ||
| if (!cancelled) setShowPhoneSheet(true); | ||
| return; | ||
| } | ||
|
|
||
| const state = await getPhonePromptState(user.id); | ||
| const now = Date.now(); | ||
| const shouldShow = !state.dontAskAgain && (!state.nextPromptAtMs || now >= state.nextPromptAtMs); | ||
|
|
||
| if (!cancelled) setShowPhoneSheet(shouldShow); | ||
| }; | ||
|
|
||
| checkShouldShowPhonePrompt().catch(() => { }); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [profile?.phone_number, user?.id]); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Fetch the pending OTP record with useQuery instead of inside useEffect.
This effect reads from TABLES.PHONE_NUMBER_UPDATES directly inside useEffect (no Realtime subscription involved), which the project guidelines prohibit. Move the Supabase read into a useQuery and derive showPhoneSheet from the query result; this also removes the manual cancelled/.catch(() => {}) lifecycle handling. Add zod validation for user.id before the call as well.
As per coding guidelines: "DO NOT call supabase tables inside of useEffect, ALWAYS use useQuery for fetching data from supabase, UNLESS you are using Supabase Realtime" and "Before making any calls to supabase, perform input validation with zod".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/hooks/phone-number/use-manage-phone-sheet.ts` around lines 12 - 45,
The effect currently queries TABLES.PHONE_NUMBER_UPDATES inside useEffect via
checkShouldShowPhonePrompt and uses manual cancelled/.catch lifecycle handling;
move that Supabase read into a useQuery (e.g., create a query hook that selects
id from TABLES.PHONE_NUMBER_UPDATES filtered by user.id) and derive the
showPhoneSheet state from the query result plus getPhonePromptState, removing
the internal async fetch and cancelled flag in useEffect; also validate user.id
with zod before invoking the query and handle the pendingRecord?.id logic and
setShowPhoneSheet updates from the query/getPhonePromptState results instead of
doing the supabase call inside checkShouldShowPhonePrompt.
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 @.github/workflows/frontend-tests.yml:
- Around line 18-21: Update the workflow to pin third-party actions to immutable
commit SHAs instead of tag refs and disable checkout credential persistence:
replace the tag-based refs for actions/checkout and actions/setup-node with
their corresponding commit SHAs, and add persist-credentials: false to the
actions/checkout step so credentials are not retained by subsequent steps;
ensure the commit SHAs are the official ones from the actions repositories and
preserve any existing step configuration while making these changes.
🪄 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: b7dd4218-2451-41bc-a2a3-69e55000cfda
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/frontend-tests.ymlbackend/services/notification_enqueue_service.pybackend/tests/test_monthly_dump_queue_notifications.pyfrontend/hooks/use-monthly-dump.tsfrontend/services/__tests__/monthly-dump-service.test.tsfrontend/services/monthly-dump-service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/hooks/use-monthly-dump.ts
- frontend/services/monthly-dump-service.ts
- backend/services/notification_enqueue_service.py
Comment on lines
+18
to
+21
| - uses: actions/checkout@v5 | ||
|
|
||
| - name: Set up Node | ||
| uses: actions/setup-node@v4 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/frontend-tests.yml"
echo "Checking unpinned GitHub Actions refs..."
rg -n 'uses:\s*actions/(checkout|setup-node)`@v`[0-9]+' "$file" || true
echo "Checking checkout credential persistence flag..."
rg -n 'uses:\s*actions/checkout@|persist-credentials:\s*false' "$file"Repository: fortune710/keepsafe
Length of output: 260
Pin GitHub Actions by commit SHA and disable checkout credential persistence.
.github/workflows/frontend-tests.ymluses tag-based action refs (actions/checkout@v5,actions/setup-node@v4) instead of immutable SHAs.actions/checkoutis not configured withpersist-credentials: false, so credentials may be persisted into later steps.
Suggested hardening patch
- - uses: actions/checkout@v5
+ - uses: actions/checkout@<full_commit_sha>
+ with:
+ persist-credentials: false
- - name: Set up Node
- uses: actions/setup-node@v4
+ - name: Set up Node
+ uses: actions/setup-node@<full_commit_sha>
with:
node-version: 20
cache: npm
cache-dependency-path: frontend/package-lock.json🧰 Tools
🪛 zizmor (1.25.2)
[warning] 18-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 21-21: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/frontend-tests.yml around lines 18 - 21, Update the
workflow to pin third-party actions to immutable commit SHAs instead of tag refs
and disable checkout credential persistence: replace the tag-based refs for
actions/checkout and actions/setup-node with their corresponding commit SHAs,
and add persist-credentials: false to the actions/checkout step so credentials
are not retained by subsequent steps; ensure the commit SHAs are the official
ones from the actions repositories and preserve any existing step configuration
while making these changes.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Bug Fixes
Documentation
Tests