feat(015): 回访花园闭环 — 留存触达隐喻与 Maestro UAT - #10
Merged
Conversation
…aestro UAT Phase 5 links revisit/weekly banners to garden metaphor copy, syncs warm notification and Profile subtext in zh/en, and adds Maestro 015 with dev seed for full RET-01–RET-05 acceptance. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds a "retention garden loop" feature: a dev-mockable retention clock, subtitle-stage resolution for the revisit banner, updated retention copy in both locales, banner UI/testID wiring, a dev-only Maestro seed service and deep-link route, new Maestro E2E flows/subflows, and openspec documentation with tests. ChangesRetention Garden Loop feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
shared/retention/resolveRevisitSubtitleKey.ts (2)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary array copy before filtering.
excludeSoftDeletedEntriesalready acceptsreadonly MoodEntry[], so spreadingentriesinto a new array here is redundant.🧹 Optional cleanup
- const visible = excludeSoftDeletedEntries([...entries]); + const visible = excludeSoftDeletedEntries(entries);🤖 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 `@shared/retention/resolveRevisitSubtitleKey.ts` at line 17, The call in resolveRevisitSubtitleKey unnecessarily copies entries before filtering even though excludeSoftDeletedEntries accepts readonly MoodEntry[]; remove the spread and pass entries directly to the filtering step so the existing array is reused.
15-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated business logic risks silent drift across three files.
computeResolveRateandgrowthStageFromRateexplicitly duplicate logic fromservices/gardenMilestone.tsandcomponents/Insights/utils.tsx(per the comments themselves). If either source changes its rate formula or thresholds, this file will silently diverge and the revisit-banner subtitle stage will no longer match the garden growth stage shown elsewhere in the app.Consider extracting a single shared
computeResolveRate/growthStageFromRate(or threshold table) into a shared module that all three call sites import, rather than maintaining three independent copies.🤖 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 `@shared/retention/resolveRevisitSubtitleKey.ts` around lines 15 - 32, The resolve-rate and stage-threshold logic is duplicated in computeResolveRate and growthStageFromRate, which can drift from the implementations in services/gardenMilestone and components/Insights/utils. Extract the shared resolve-rate calculation and growth-stage mapping into a single reusable module or shared helper, then update resolveRevisitSubtitleKey and the other call sites to import and use those symbols instead of keeping separate copies.services/maestroRetentionSeed.ts (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize these retention/storage keys.
GUEST_STORAGE_KEY,DISMISS_REVISIT_KEY, andDISMISS_WEEKLY_KEYare duplicated here and instore/modules/storage.ts,components/retention/RevisitBanner.tsx, andcomponents/retention/WeeklyReviewBanner.tsx. Import the canonical constants instead of repeating raw strings to avoid drift.🤖 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 `@services/maestroRetentionSeed.ts` around lines 23 - 26, The retention/storage key strings are duplicated across the seed module and the storage/banner components, so update maestroRetentionSeed to use the canonical constants instead of local raw string copies. Import and reuse the existing symbols from store/modules/storage.ts in maestroRetentionSeed, and ensure RevisitBanner and WeeklyReviewBanner continue referencing the same shared constants so all retention keys stay aligned.
🤖 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 @.maestro/flows/015-retention-garden-loop.yaml:
- Around line 265-272: The reseed flow in the core-loop branches can start
before back navigation finishes, so add the same extendedWaitUntil used
elsewhere after review-export-back-button and before the
seed-retention-touchpoints runFlow in 015-retention-garden-loop. Update both
branches around the tapOn/review-export-back-button step to wait for
insights-screen before starting the reseed, matching the existing pattern used
for UAT 7/8.
In `@app/dev-seed-retention.tsx`:
- Around line 21-47: The useEffect in dev-seed-retention.tsx is intentionally
using granular params fields instead of the whole params object, but the
exhaustive-deps warning is still left unresolved. Keep the existing dependency
list on router, params.scenario, params.locale, and params.reminders, and add an
explicit lint suppression for the exhaustive-deps rule on this effect so the
pr-gate lint check passes cleanly without introducing a loop risk.
In `@shared/retention/getRetentionNow.ts`:
- Around line 10-29: Wire hydrateDevRetentionMockNow() into the app startup path
so the persisted mock time is restored before retention logic runs. The current
runMaestroRetentionSeed() only saves MOCK_NOW_KEY to AsyncStorage, so add a
bootstrap call to hydrateDevRetentionMockNow() in the main app initialization
flow (where retention/mock time is first configured) and make sure any consumers
of getRetentionNow() can see the rehydrated value on a fresh JS load.
---
Nitpick comments:
In `@services/maestroRetentionSeed.ts`:
- Around line 23-26: The retention/storage key strings are duplicated across the
seed module and the storage/banner components, so update maestroRetentionSeed to
use the canonical constants instead of local raw string copies. Import and reuse
the existing symbols from store/modules/storage.ts in maestroRetentionSeed, and
ensure RevisitBanner and WeeklyReviewBanner continue referencing the same shared
constants so all retention keys stay aligned.
In `@shared/retention/resolveRevisitSubtitleKey.ts`:
- Line 17: The call in resolveRevisitSubtitleKey unnecessarily copies entries
before filtering even though excludeSoftDeletedEntries accepts readonly
MoodEntry[]; remove the spread and pass entries directly to the filtering step
so the existing array is reused.
- Around line 15-32: The resolve-rate and stage-threshold logic is duplicated in
computeResolveRate and growthStageFromRate, which can drift from the
implementations in services/gardenMilestone and components/Insights/utils.
Extract the shared resolve-rate calculation and growth-stage mapping into a
single reusable module or shared helper, then update resolveRevisitSubtitleKey
and the other call sites to import and use those symbols instead of keeping
separate copies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4a915bd-c699-491d-a385-360b614fd8eb
📒 Files selected for processing (22)
.maestro/config.yaml.maestro/flows/015-retention-garden-loop.yaml.maestro/subflows/apply-retention-seed.yaml.maestro/subflows/seed-retention-touchpoints.yaml__tests__/unit/i18n/bilingualSmokeCopy.test.ts__tests__/unit/i18n/retentionCopy.test.ts__tests__/unit/shared/retention/resolveRevisitSubtitleKey.test.tsapp/dev-seed-retention.tsxcomponents/retention/RevisitBanner.tsxcomponents/retention/WeeklyReviewBanner.tsxcomponents/retention/retention.styles.tscomponents/settings/GroupedSettingsCard.tsxfeatures/profile/ProfileScreen.tsxfeatures/profile/components/ProfileSettingsSection.tsxlocales/en-US/retention.jsonlocales/zh-Hans/retention.jsonopenspec/changes/015-retention-garden-loop/SPEC.mdopenspec/changes/015-retention-garden-loop/VERIFICATION.mdpackage.jsonservices/maestroRetentionSeed.tsshared/retention/getRetentionNow.tsshared/retention/resolveRevisitSubtitleKey.ts
Comment on lines
+265
to
+272
| - tapOn: | ||
| id: review-export-back-button | ||
| - runFlow: | ||
| file: ../subflows/seed-retention-touchpoints.yaml | ||
| env: | ||
| SCENARIO: revisit | ||
| LOCALE: zh-Hans | ||
| REMINDERS: default |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".maestro/flows/015-retention-garden-loop.yaml"
echo "== File size =="
wc -l "$FILE"
echo
echo "== UAT 3/4 surrounding lines =="
sed -n '70,125p' "$FILE" | cat -n
echo
echo "== UAT 7 surrounding lines =="
sed -n '250,285p' "$FILE" | cat -n
echo
echo "== UAT 8 surrounding lines =="
sed -n '328,365p' "$FILE" | cat -nRepository: moyunzero/Emotion-Diary
Length of output: 4518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== seed-retention-touchpoints subflow =="
FILE=".maestro/subflows/seed-retention-touchpoints.yaml"
wc -l "$FILE"
sed -n '1,260p' "$FILE" | cat -n
echo
echo "== Search for similar back-button -> runFlow patterns =="
rg -n -A3 -B3 "review-export-back-button|seed-retention-touchpoints|extendedWaitUntil" .maestro/flows .maestro/subflowsRepository: moyunzero/Emotion-Diary
Length of output: 44331
Wait for insights-screen before reseeding in UAT 7/8. Add the same extendedWaitUntil after review-export-back-button and before the runFlow reseed in both core-loop branches; otherwise the back navigation can still be in flight when the new flow starts.
🤖 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 @.maestro/flows/015-retention-garden-loop.yaml around lines 265 - 272, The
reseed flow in the core-loop branches can start before back navigation finishes,
so add the same extendedWaitUntil used elsewhere after review-export-back-button
and before the seed-retention-touchpoints runFlow in 015-retention-garden-loop.
Update both branches around the tapOn/review-export-back-button step to wait for
insights-screen before starting the reseed, matching the existing pattern used
for UAT 7/8.
Comment on lines
+21
to
+47
| useEffect(() => { | ||
| if (!__DEV__) { | ||
| router.replace("/"); | ||
| return; | ||
| } | ||
|
|
||
| void (async () => { | ||
| const seedOptions = parseMaestroRetentionSeedParams(params); | ||
| if (!seedOptions) { | ||
| router.replace("/"); | ||
| return; | ||
| } | ||
|
|
||
| await runMaestroRetentionSeed(seedOptions); | ||
| await useAppStore.getState()._loadEntries(); | ||
|
|
||
| if (seedOptions.locale) { | ||
| await useAppStore.getState().setLocale(seedOptions.locale); | ||
| } | ||
|
|
||
| if (seedOptions.scenario === "weekly") { | ||
| router.replace("/insights"); | ||
| } else { | ||
| router.replace("/"); | ||
| } | ||
| })(); | ||
| }, [router, params.scenario, params.locale, params.reminders]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Silence the exhaustive-deps warning explicitly rather than leaving it unresolved.
The pr-gate check flags params as a missing dependency at Line 47. Adding the whole params object would likely reintroduce an effect-loop risk if useLocalSearchParams returns a new object reference each render; the current granular deps (params.scenario, params.locale, params.reminders) are intentionally more correct. Suppress the rule explicitly so CI lint passes cleanly instead of leaving an open warning.
🔇 Proposed fix
- }, [router, params.scenario, params.locale, params.reminders]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- params object identity is unstable; track primitive fields instead
+ }, [router, params.scenario, params.locale, params.reminders]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!__DEV__) { | |
| router.replace("/"); | |
| return; | |
| } | |
| void (async () => { | |
| const seedOptions = parseMaestroRetentionSeedParams(params); | |
| if (!seedOptions) { | |
| router.replace("/"); | |
| return; | |
| } | |
| await runMaestroRetentionSeed(seedOptions); | |
| await useAppStore.getState()._loadEntries(); | |
| if (seedOptions.locale) { | |
| await useAppStore.getState().setLocale(seedOptions.locale); | |
| } | |
| if (seedOptions.scenario === "weekly") { | |
| router.replace("/insights"); | |
| } else { | |
| router.replace("/"); | |
| } | |
| })(); | |
| }, [router, params.scenario, params.locale, params.reminders]); | |
| useEffect(() => { | |
| if (!__DEV__) { | |
| router.replace("/"); | |
| return; | |
| } | |
| void (async () => { | |
| const seedOptions = parseMaestroRetentionSeedParams(params); | |
| if (!seedOptions) { | |
| router.replace("/"); | |
| return; | |
| } | |
| await runMaestroRetentionSeed(seedOptions); | |
| await useAppStore.getState()._loadEntries(); | |
| if (seedOptions.locale) { | |
| await useAppStore.getState().setLocale(seedOptions.locale); | |
| } | |
| if (seedOptions.scenario === "weekly") { | |
| router.replace("/insights"); | |
| } else { | |
| router.replace("/"); | |
| } | |
| })(); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- params object identity is unstable; track primitive fields instead | |
| }, [router, params.scenario, params.locale, params.reminders]); |
🧰 Tools
🪛 GitHub Check: pr-gate
[warning] 47-47:
React Hook useEffect has a missing dependency: 'params'. Either include it or remove the dependency array
🤖 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 `@app/dev-seed-retention.tsx` around lines 21 - 47, The useEffect in
dev-seed-retention.tsx is intentionally using granular params fields instead of
the whole params object, but the exhaustive-deps warning is still left
unresolved. Keep the existing dependency list on router, params.scenario,
params.locale, and params.reminders, and add an explicit lint suppression for
the exhaustive-deps rule on this effect so the pr-gate lint check passes cleanly
without introducing a loop risk.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+10
to
+29
| export async function hydrateDevRetentionMockNow(): Promise<void> { | ||
| if (!__DEV__) { | ||
| devMockNowMs = null; | ||
| return; | ||
| } | ||
| try { | ||
| const { default: AsyncStorage } = await import( | ||
| "@react-native-async-storage/async-storage" | ||
| ); | ||
| const raw = await AsyncStorage.getItem(MOCK_NOW_KEY); | ||
| if (!raw) { | ||
| devMockNowMs = null; | ||
| return; | ||
| } | ||
| const parsed = Date.parse(raw); | ||
| devMockNowMs = Number.isNaN(parsed) ? null : parsed; | ||
| } catch { | ||
| devMockNowMs = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "hydrateDevRetentionMockNow" --type=ts --type=tsx -C3 2>/dev/null || rg -n "hydrateDevRetentionMockNow" -C3Repository: moyunzero/Emotion-Diary
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files mentioning retention mock helpers ==\n'
rg -n "setDevRetentionMockNow|MOCK_NOW_KEY|runMaestroRetentionSeed|hydrateDevRetentionMockNow|devMockNowMs" . --type=ts --type=tsx -C 2 || true
printf '\n== Likely bootstrap entry points ==\n'
rg -n "App\.|registerRootComponent|bootstrap|persist|rehydrate|AsyncStorage" . --type=ts --type=tsx -C 2 || trueRepository: moyunzero/Emotion-Diary
Length of output: 301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Retention helper references ==\n'
rg -n "hydrateDevRetentionMockNow|setDevRetentionMockNow|MOCK_NOW_KEY|runMaestroRetentionSeed|devMockNowMs" . -t ts -t tsx -C 2 || true
printf '\n== Startup/bootstrap references ==\n'
rg -n "registerRootComponent|AppRegistry|bootstrap|root component|AsyncStorage|rehydrate" . -t ts -t tsx -C 2 || trueRepository: moyunzero/Emotion-Diary
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Tracked TS/TSX files containing retention helper symbols ==\n'
git ls-files | rg '\.(ts|tsx)$' | xargs rg -n "hydrateDevRetentionMockNow|setDevRetentionMockNow|MOCK_NOW_KEY|runMaestroRetentionSeed|devMockNowMs" -C 2 || true
printf '\n== Tracked TS/TSX files containing startup/bootstrap wiring ==\n'
git ls-files | rg '\.(ts|tsx)$' | xargs rg -n "registerRootComponent|AppRegistry|bootstrap|rehydrate|AsyncStorage" -C 2 || trueRepository: moyunzero/Emotion-Diary
Length of output: 48076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Imports/uses of shared/retention/getRetentionNow ==\n'
git ls-files | rg '\.(ts|tsx)$' | xargs rg -n 'from "`@/shared/retention/getRetentionNow`"|from "\./shared/retention/getRetentionNow"|from "\.\./shared/retention/getRetentionNow"|from "../shared/retention/getRetentionNow"' -C 2 || true
printf '\n== App entry/bootstrap files ==\n'
git ls-files | rg '(^app/.*\.(ts|tsx)$|(^|/)index\.(ts|tsx)$|(^|/)main\.(ts|tsx)$|(^|/)App\.(ts|tsx)$|(^|/)entry\.(ts|tsx)$)' | sed -n '1,120p'Repository: moyunzero/Emotion-Diary
Length of output: 1980
Wire hydrateDevRetentionMockNow() into app bootstrap. runMaestroRetentionSeed persists the mock ISO to AsyncStorage, but nothing rehydrates it on a fresh JS load, so any reload/cold start falls back to real device time and breaks Maestro retention scenarios.
🤖 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 `@shared/retention/getRetentionNow.ts` around lines 10 - 29, Wire
hydrateDevRetentionMockNow() into the app startup path so the persisted mock
time is restored before retention logic runs. The current
runMaestroRetentionSeed() only saves MOCK_NOW_KEY to AsyncStorage, so add a
bootstrap call to hydrateDevRetentionMockNow() in the main app initialization
flow (where retention/mock time is first configured) and make sure any consumers
of getRetentionNow() can see the rehydrated value on a fresh JS load.
7 tasks
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
daysSince主标题，新增花园成长阶段副句（resolveRevisitSubtitleKey）及 Maestro testIDretention.jsonzh/en 同步温暖隐喻通知与 Profile 副文案retentionCopy.test.ts，扩展bilingualSmokeCopy（含 onboarding 回归键）015-retention-garden-loop（8 个 UAT 场景 + 18 张本地验收截图）、__DEV__dev-seed 深链、getRetentionNow周末 mockTest plan
yarn typecheckyarn test（360 passed）yarn test:maestro:preflight && yarn test:maestro:015（18/18 截图 + flow 全绿）Maestro 本地复跑：
yarn start yarn test:maestro:preflight && yarn test:maestro:015验收截图在
.maestro/acceptance/015-retention-garden-loop/（gitignore，本地留存）。Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes