Conversation
WalkthroughAdds a persisted survey-popup flag and show/hide APIs to banners state, introduces a new Vue survey-popup component, integrates a delayed/transitioned survey flow into the app menu that follows or replaces the Solana staking banner, and renames one analytics event. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AppMenu as AppMenu (index.vue)
participant BState as BannersState
participant Storage as Storage
participant Solana as SolanaStakingBanner
participant Survey as SurveyPopup
Note over AppMenu: onMounted
AppMenu->>BState: showSolanaStakingBanner()
BState-->>AppMenu: boolean
alt Solana banner shown
AppMenu->>Solana: render
User-->>Solana: close
Solana-->>AppMenu: onClose
AppMenu->>AppMenu: openSurveyPopup()
else Solana not shown
AppMenu->>AppMenu: openSurveyPopup()
end
AppMenu->>BState: showSurveyPopup()
BState->>Storage: read isHideSurveyPopup
Storage-->>BState: flag
BState-->>AppMenu: boolean (show?)
alt show == true
AppMenu->>AppMenu: wait 4s
AppMenu->>Survey: render (Transition)
User-->>Survey: CTA click or close
Survey-->>AppMenu: emit close
AppMenu->>BState: hideSurveyPopup()
BState->>Storage: set isHideSurveyPopup = true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/extension/src/ui/action/components/app-menu/components/solana-staking-banner.vue (1)
34-38: Confirm analytics event rename and pipeline alignment.Renaming to
SolanaStakingBannerEvents.NetworkListClickedis fine, but please verify this enum value exists and downstream dashboards/ETL expect the new name. Also consider awaiting the tracking call (if it returns a Promise) rather than relying on a fixed 1s timeout before navigation.packages/extension/src/ui/action/components/app-menu/components/survey-popup.vue (4)
6-9: Use a button for the CTA instead of an anchor with javascript:void(0).Improves accessibility and avoids anti-pattern anchors.
- <a href="javascript:void(0);" @click="openSurveyLink" class="button" - ><span>Click here</span></a - > + <button type="button" @click="openSurveyLink" class="button"> + <span>Click here</span> + </button>
11-14: Add an accessible label to the close control.Screen readers need a label; keep the visual as-is.
- <a class="survey-popup__close" @click="close"> + <a class="survey-popup__close" @click="close" aria-label="Close survey popup"> <close-icon /> </a>
28-31: Consider adding click/close telemetry for the survey.Track “SurveyOpened/Clicked/Closed” to measure engagement, similar to the staking banner.
If you want, I can draft a tiny metrics helper (types + track calls) aligned with your existing metrics library.
75-83: Redundant CSS declaration.
.buttonsetswidth: auto;then immediately overrides towidth: 163px;. Remove the first one.- width: auto; width: 163px;packages/extension/src/libs/banners-state/index.ts (1)
58-67: New show/hide API for survey popup is straightforward.Optional: add explicit return types to keep the class API self-documenting.
- async hideSurveyPopup() { + async hideSurveyPopup(): Promise<void> { const state = await this.getOrInitializeState(); state.isHideSurveyPopup = true; await this.storage.set(StorageKeys.bannersInfo, state); }packages/extension/src/ui/action/components/app-menu/index.vue (3)
161-173: Prevent overlap during switch with transition mode.Add
mode="out-in"so the leaving banner finishes before the survey enters.- <Transition name="slide-fade"> + <Transition name="slide-fade" mode="out-in">
273-285: Gate survey trigger by expansion state (optional).You call
openSurveyPopup()even when collapsed; it then appears instantly when the menu is expanded. If you prefer the 4s delay only when visible, guard byisExpanded.value.- if (!isSolanaStakingBanner.value) { - openSurveyPopup(); - } + if (!isSolanaStakingBanner.value && isExpanded.value) { + openSurveyPopup(); + }
556-577: Manage survey timer lifecycle; avoid stray timeouts and show-only-when-expanded.Store/clear the timeout, check
isExpandedbefore showing, and clear on unmount.-const isSurveyPopup = ref(false); +const isSurveyPopup = ref(false); +const surveyTimer = ref<ReturnType<typeof setTimeout> | null>(null); @@ -const closeSolanaStakingBanner = () => { +const closeSolanaStakingBanner = () => { isSolanaStakingBanner.value = false; bannersState.hideSolanaStakingBanner(); openSurveyPopup(); }; const openSurveyPopup = async () => { if (await bannersState.showSurveyPopup()) { - setTimeout(() => { - isSurveyPopup.value = true; - }, 4000); + if (surveyTimer.value) { + clearTimeout(surveyTimer.value); + surveyTimer.value = null; + } + surveyTimer.value = setTimeout(() => { + if (isExpanded.value) { + isSurveyPopup.value = true; + } + surveyTimer.value = null; + }, 4000); } }; -const closeSurveyPopup = () => { +const closeSurveyPopup = () => { isSurveyPopup.value = false; bannersState.hideSurveyPopup(); + if (surveyTimer.value) { + clearTimeout(surveyTimer.value); + surveyTimer.value = null; + } };Add unmount cleanup and import:
-import { PropType, ref, computed, onMounted } from 'vue'; +import { PropType, ref, computed, onMounted, onUnmounted } from 'vue'; @@ onMounted(async () => { … }); + +onUnmounted(() => { + if (surveyTimer.value) { + clearTimeout(surveyTimer.value); + surveyTimer.value = null; + } +});
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/extension/src/libs/banners-state/index.ts(3 hunks)packages/extension/src/libs/banners-state/types.ts(1 hunks)packages/extension/src/ui/action/components/app-menu/components/solana-staking-banner.vue(1 hunks)packages/extension/src/ui/action/components/app-menu/components/survey-popup.vue(1 hunks)packages/extension/src/ui/action/components/app-menu/index.vue(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (3)
packages/extension/src/libs/banners-state/types.ts (1)
8-8: State shape extension LGTM.Adding
isHideSurveyPopup: booleancleanly extends the banner state.Please confirm no migration is required for existing stored states; with missing fields defaulting to
undefined,showSurveyPopup()will returntrueas desired.packages/extension/src/libs/banners-state/index.ts (1)
20-25: Initialization and reset paths look correct.
isHideSurveyPopupis properly initialized and reset.Also applies to: 28-34
packages/extension/src/ui/action/components/app-menu/index.vue (1)
583-595: Transition styles LGTM.The slide-fade timings read well for unobtrusive UI.
3 participants
Summary by CodeRabbit
New Features
Style
Chores