quests: refresh after the backend credits an action, and warn on short replies - #1394
Conversation
…t replies Two client-side halves of the same complaint: people do something, the quest counter does not move, and nothing explains why. scheduleQuestsRefresh invalidated the quests query 4 seconds after a points-earning action. A chain action is not credited that fast: it has to be verified against the chain and processed first, which lands a little over a minute later. So the refetch read the pre-action numbers and then marked them fresh for the query's staleTime, which actively prevented the real update from being picked up. The delay now matches the backend. The other half is silent rejection. The points backend drops a comment whose body is at or under a minimum length once URLs are stripped, so 'Thank you', an emoji, or an image-only reply earns nothing and never counts toward the daily comment quest. That is a deliberate anti-spam rule that was invisible in the product. The minimum and the URL-stripping measurement now live in the shared quest catalog, mirroring the backend constant the same way STREAK_FREEZE_PRICE does, and the reply composer says so before the user submits. It does not block submitting, and it stays quiet for logged-out users and in edit mode. The leaderboard badge tooltip now says the day is UTC. The badge is wiped at 00:00 UTC while the board itself ranks a rolling 24 hours, so people lose it mid-afternoon in Asia with no explanation. Keeping the boundary and naming it is the honest fix: the user's own quest card resets on the same boundary, so making the badge roll instead would have desynced the two. Refs #1391, #1392, ecency/esync-py#26
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds shared quest-content eligibility helpers, short-reply feedback in the comment composer, tests for both behaviors, and a 70-second quest refresh delay. It also clarifies link exclusion and UTC quest completion messaging. ChangesQuest behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/web/src/specs/utils/refresh-quests.spec.ts (1)
42-49: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete quest cache key.
The current assertions only check key membership. An incorrect key such as
["quests", "other", "alice"]could still pass.Proposed fix
+import { QueryKeys } from "`@ecency/sdk`"; + - const [{ queryKey }] = invalidateQueries.mock.calls[0]; - expect(queryKey).toContain("alice"); - expect(queryKey).not.toContain("`@alice`"); + expect(invalidateQueries).toHaveBeenCalledWith({ + queryKey: QueryKeys.quests.status("alice") + });As per coding guidelines, use
QueryKeysfrom@ecency/sdkas the single source of truth for cache keys.🤖 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/web/src/specs/utils/refresh-quests.spec.ts` around lines 42 - 49, Update the test for scheduleQuestsRefresh to assert the complete queryKey against the expected quest cache key generated via QueryKeys from `@ecency/sdk`, rather than checking only containment. Preserve verification that the leading @ is removed from the account value and ensure no unrelated key segments are accepted.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.
Inline comments:
In `@apps/web/src/features/shared/comment/index.tsx`:
- Around line 336-340: Add role="status" to the comment-short-reply-hint element
in apps/web/src/features/shared/comment/index.tsx (lines 336-340), then update
the related tests in apps/web/src/specs/features/shared/comment.spec.tsx (lines
139-168) to use renderWithQueryClient, screen.getByRole("status"),
screen.queryByRole("status"), and screen.getByRole("textbox") instead of CSS
selectors and getByTestId.
In `@apps/web/src/specs/utils/refresh-quests.spec.ts`:
- Around line 18-29: Strengthen the test around scheduleQuestsRefresh to assert
the refresh does not occur throughout the required 60–75 second window,
including at 60 seconds, and only occurs once the intended delay has elapsed.
Replace the current 10-second-only assertion with boundary checks that reject
early timers while preserving the single invalidateQueries call.
- Around line 4-5: Replace the untyped queryClient test double in refresh-quests
tests with a typed QueryClient approach: instantiate a real QueryClient and spy
on invalidateQueries, or type the dependency as Pick<QueryClient,
"invalidateQueries">. Preserve the existing invalidateQueries mock behavior
while removing the as any cast.
In `@packages/sdk/src/modules/quests/catalog.ts`:
- Around line 60-61: Update measureQuestContentLength in
packages/sdk/src/modules/quests/catalog.ts:60-61 to apply Array.from(...).length
after removing URLs, so length counts Unicode code points. Add a boundary test
with 13 astral characters in
packages/sdk/src/modules/quests/catalog.spec.ts:39-45. Update the SDK mocks in
apps/web/src/specs/features/shared/comment.spec.tsx:67-71 and
apps/web/src/specs/setup-any-spec.ts:104-109 to match the revised behavior.
---
Nitpick comments:
In `@apps/web/src/specs/utils/refresh-quests.spec.ts`:
- Around line 42-49: Update the test for scheduleQuestsRefresh to assert the
complete queryKey against the expected quest cache key generated via QueryKeys
from `@ecency/sdk`, rather than checking only containment. Preserve verification
that the leading @ is removed from the account value and ensure no unrelated key
segments are accepted.
🪄 Autofix
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 Plus
Run ID: 5593e624-eff3-4fc5-80a2-74fcadf69c07
📒 Files selected for processing (8)
apps/web/src/features/i18n/locales/en-US.jsonapps/web/src/features/shared/comment/index.tsxapps/web/src/specs/features/shared/comment.spec.tsxapps/web/src/specs/setup-any-spec.tsapps/web/src/specs/utils/refresh-quests.spec.tsapps/web/src/utils/refresh-quests.tspackages/sdk/src/modules/quests/catalog.spec.tspackages/sdk/src/modules/quests/catalog.ts
A single shared timer let a second account cancel the first one's only scheduled invalidation. It was survivable at the old 4 second debounce; at the delay this PR moves it to, the window is wide enough for an account switch to land inside it, so pending refreshes are now keyed by username. Same finding as ecency/vision-mobile#3478, raised there by review.
The content length check was measuring UTF-16 code units while the backend measures with Python len() on a str. A reply of 13 astral emoji scored 13 there and 26 here, so the composer would have stayed silent and promised points that were then refused: the exact confusion the hint exists to end. Measuring code points makes the two agree. The hint is a live region now (role=status), so it is announced rather than only seen, and its tests query by role instead of a class name. Two spec fixes: the refresh timing test allowed anything under 10 seconds to pass, which a regression to a 15 second delay would have survived, so the lower bound is now just under a minute. And the QueryClient double was cast through any, which would outlive a signature change, so it is a real client with a spy.
Picks up the shared quest content rule (QUEST_MIN_CONTENT_LENGTH plus the URL-stripping, code-point-counting measurement) that the website released in ecency/vision-web#1394, which the composer hint in #3473 needs. Nothing in this PR uses those exports yet; the bump lands here so the dependency is in place ahead of it. The two versions in between are patches: 2.3.77 moved useAccountUpdateRecovery to the AuthContextV2 adapter, which this app does not call, and 2.3.78 was a documentation guard. Verified against the published package rather than the older copy in the shared install: all 143 named imports this app takes from @ecency/sdk still resolve, and typecheck, lint and the unit suite pass against 2.3.79.
Resolves the version collision introduced by the label-triggered changeset run: both this branch and #1394 on develop bumped @ecency/sdk to 2.3.79, and 2.3.79 is already published to npm, so merging as-is would have failed `publish:sdk`. - packages/sdk/CHANGELOG.md: keep develop's 2.3.79 entry, move this PR's entry to a new 2.3.80 section. - @ecency/sdk 2.3.79 -> 2.3.80, @ecency/wallets 5.0.79 -> 5.0.80 (dependent bump), with the matching wallets CHANGELOG entry. - packages/sdk/dist: regenerated from the merged source rather than hand-merged. Verified the bundle carries both sides — this branch's account-history changes and develop's quests catalog. @ecency/sdk test 706 passed, @ecency/web test 2590 passed, apps/web tsc --noEmit 0 errors.
Closes #1391. Closes #1392.
Both are the client half of the same complaint: people do something, the quest counter does not move, and nothing explains why.
Refresh timing (#1391)
scheduleQuestsRefreshinvalidated the quests query 4 seconds after a points-earning action. A chain action is not credited that fast: it has to be verified against the chain and processed first, which lands a little over a minute later. So the refetch read the pre-action numbers and then marked them fresh for the query'sstaleTime, which actively prevented the real update from being picked up. The navbar streak pill and the /perks tiles then sat on stale values until something remounted them much later.The delay now matches the backend, extracted as a named constant with the reasoning next to it. The coalescing behaviour and the deliberate exclusion of the vote path are unchanged.
Silent rejection (#1392)
The points backend drops a comment whose body is at or under a minimum length once URLs are stripped out, so "Thank you", an emoji, or an image-only reply earns nothing and never counts toward the daily comment quest. Deliberate anti-spam rule, completely invisible in the product, and the single biggest source of these reports.
QUEST_MIN_CONTENT_LENGTH,measureQuestContentLengthandearnsQuestContentCreditgo in the shared quest catalog, mirroring the backend constant the same waySTREAK_FREEZE_PRICEalready does. Both clients need the rule and it should not be typed twice.Leaderboard badge label
The
quests-donetooltip now says the day is UTC. The badge is wiped at 00:00 UTC while the board itself ranks a rolling 24 hours, so people lose it mid-afternoon in Asia with nothing explaining it. See ecency/esync-py#26: keeping the boundary and naming it is the honest fix, because the user's own quest card resets on the same boundary and making the badge roll instead would have desynced the two.Needs the
patch:sdklabelapps/webresolves@ecency/sdkthrough the committeddist/*.d.ts, so typecheck will be red until the label is applied and the auto-changeset bot pushes the version bump plus the rebuilt dist to this branch. Not adding the label here, that is a manual call.Verified locally with a throwaway
pnpm --filter @ecency/sdk build(discarded before committing, no dist in this diff):Verification
New tests:
scheduleQuestsRefreshdoes not fire at 10s, does fire after the real delay, coalesces a burst into one request, strips a leading@, and no-ops without a usernameTwo mocks needed the new exports. The global
@ecency/sdkmock insetup-any-spec.tsgained the real pure implementations plus a minimalQueryKeys(it had none, so anything importing it already failed), andcomment.spec.tsxhas its own factory which vitest 4 does not merge, so it needed them too.Follow-up
The app side is tracked separately in ecency/vision-mobile#3471 and #3473, and #3473 waits on this SDK release.
Summary by CodeRabbit
New Features
Bug Fixes