feat: integrate nightly survey system into app#8480
Open
christian-byrne wants to merge 1 commit intomainfrom
Open
feat: integrate nightly survey system into app#8480christian-byrne wants to merge 1 commit intomainfrom
christian-byrne wants to merge 1 commit intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a nightly survey feature consisting of a lazy-loaded controller component that renders survey popover components for enabled surveys, alongside a feature tracking composable that monitors survey feature usage with full test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
🎭 Playwright: ✅ 533 passed, 0 failed · 1 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
dosubot
bot
added
the
size:L
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
christian-byrne
force-pushed
the
feat/survey-popover
branch
from
19a90e5 to
1b8b0fa
Compare
christian-byrne
force-pushed
the
feat/survey-popover
branch
from
1b8b0fa to
e25b7b8
Compare
christian-byrne
added a commit
that referenced
this pull request
Feb 23, 2026
## Summary Adds NightlySurveyPopover component that displays a Typeform survey to eligible nightly users after a configurable delay. ## Changes - **What**: Vue component that uses `useSurveyEligibility` to show/hide a survey popover with accept, dismiss, and opt-out actions. Loads Typeform embed script dynamically with HTTPS and deduplication. ## Review Focus - Typeform script injection security (HTTPS-only, load-once guard, typeformId alphanumeric validation) - Timeout lifecycle (clears pending timeout when eligibility changes) ## Part of Nightly Survey System This is part 4 of a stacked PR chain: 1. ✅ feat/feature-usage-tracker - useFeatureUsageTracker (merged in #8189) 2. ✅ feat/survey-eligibility - useSurveyEligibility (#8189, merged) 3. ✅ feat/survey-config - surveyRegistry.ts (#8355, merged) 4. **feat/survey-popover** - NightlySurveyPopover.vue (this PR) 5. feat/survey-integration - NightlySurveyController.vue (#8480) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9083-feat-add-NightlySurveyPopover-component-for-feature-surveys-30f6d73d365081d1beb2f92555a4b2f4) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
size:XL
christian-byrne
force-pushed
the
feat/survey-integration
branch
from
19e4751 to
7cfa2e3
Compare
dosubot
bot
added
size:L
📦 Bundle: 4.37 MB gzip 🔴 +3.02 kBDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 916 kB (baseline 916 kB) • 🔴 +271 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 10 added / 10 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 738 B (baseline 738 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47 kB (baseline 47 kB) • ⚪ 0 BReusable component library chunks
Status: 7 added / 7 removed Data & Services — 2.51 MB (baseline 2.51 MB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 58.3 kB (baseline 58.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 8.83 MB (baseline 8.83 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.63 MB (baseline 7.62 MB) • 🔴 +8.92 kBBundles that do not match a named category
Status: 59 added / 58 removed |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/sidebar/SideToolbar.vue`:
- Line 54: The async component NightlySurveyController is being rendered
directly; wrap its usage in a Vue <Suspense> so the defineAsyncComponent
resolves correctly — replace the direct <component :is="NightlySurveyController"
v-if="NightlySurveyController" /> with a <Suspense> that conditionally renders
the async component in the default slot (and provide a simple fallback in the
fallback slot or via template `#fallback`) to satisfy async-component guidelines;
ensure you still check NightlySurveyController (or guard in setup) so the
Suspense only mounts when the async is available.
In `@src/platform/surveys/useSurveyFeatureTracking.test.ts`:
- Around line 3-15: The tests use a module-level mutable object
mockSurveyConfigs and a vi.mock that reads it; replace this pattern by hoisting
a mock function with vi.hoisted and using that in the mock implementation for
getSurveyConfig, then reset/configure that hoisted mock per test instead of
mutating mockSurveyConfigs. Concretely, create a hoisted const (e.g., const
hoistedGetSurveyConfig = vi.hoisted(() => vi.fn())) and have
vi.mock('./surveyRegistry', () => ({ getSurveyConfig: hoistedGetSurveyConfig }))
so the mock is created before module init; in beforeEach call
hoistedGetSurveyConfig.mockReset() and set
hoistedGetSurveyConfig.mockImplementationOnce(...) or mockReturnValue(...) for
each test to control returned { enabled } values rather than clearing a shared
object.
| </div> | ||
| </div> | ||
| <HelpCenterPopups :is-small="isSmall" /> | ||
| <component :is="NightlySurveyController" v-if="NightlySurveyController" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/sidebar/SideToolbar.vue | head -110Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4978
Wrap the async controller in <Suspense>.
The component is lazy-loaded via defineAsyncComponent, so it must be rendered within <Suspense> to comply with the repository guideline for async components.
Suggested update
- <component :is="NightlySurveyController" v-if="NightlySurveyController" />
+ <Suspense v-if="NightlySurveyController">
+ <component :is="NightlySurveyController" />
+ </Suspense>📝 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
| <component :is="NightlySurveyController" v-if="NightlySurveyController" /> | |
| <Suspense v-if="NightlySurveyController"> | |
| <component :is="NightlySurveyController" /> | |
| </Suspense> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/sidebar/SideToolbar.vue` at line 54, The async component
NightlySurveyController is being rendered directly; wrap its usage in a Vue
<Suspense> so the defineAsyncComponent resolves correctly — replace the direct
<component :is="NightlySurveyController" v-if="NightlySurveyController" /> with
a <Suspense> that conditionally renders the async component in the default slot
(and provide a simple fallback in the fallback slot or via template `#fallback`)
to satisfy async-component guidelines; ensure you still check
NightlySurveyController (or guard in setup) so the Suspense only mounts when the
async is available.
Comment on lines
+3
to
+15
| const mockSurveyConfigs: Record<string, { enabled: boolean }> = {} | ||
|
|
||
| vi.mock('./surveyRegistry', () => ({ | ||
| getSurveyConfig: (featureId: string) => mockSurveyConfigs[featureId] | ||
| })) | ||
|
|
||
| describe('useSurveyFeatureTracking', () => { | ||
| beforeEach(() => { | ||
| localStorage.clear() | ||
| vi.resetModules() | ||
| Object.keys(mockSurveyConfigs).forEach( | ||
| (key) => delete mockSurveyConfigs[key] | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/platform/surveys/useSurveyFeatureTracking.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2115
Avoid module-level mutable mock state; use a hoisted mock function instead.
The current pattern uses a module-level mockSurveyConfigs object that is mutated across tests and manually cleaned up. This violates the principle of keeping mocks contained within their test scope. Use vi.hoisted() to create the mock function before module initialization, then reset and configure it per test with Vitest's mock utilities.
♻️ Suggested refactor
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
-
-const mockSurveyConfigs: Record<string, { enabled: boolean }> = {}
-
-vi.mock('./surveyRegistry', () => ({
- getSurveyConfig: (featureId: string) => mockSurveyConfigs[featureId]
-}))
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const getSurveyConfig = vi.hoisted(() =>
+ vi.fn<(featureId: string) => { enabled: boolean } | undefined>()
+)
+
+vi.mock('./surveyRegistry', () => ({
+ getSurveyConfig
+}))- Object.keys(mockSurveyConfigs).forEach(
- (key) => delete mockSurveyConfigs[key]
- )
+ getSurveyConfig.mockReset()- mockSurveyConfigs['test-feature'] = { enabled: true }
+ getSurveyConfig.mockReturnValue({ enabled: true })- mockSurveyConfigs['disabled-feature'] = { enabled: false }
+ getSurveyConfig.mockReturnValue({ enabled: false })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/surveys/useSurveyFeatureTracking.test.ts` around lines 3 - 15,
The tests use a module-level mutable object mockSurveyConfigs and a vi.mock that
reads it; replace this pattern by hoisting a mock function with vi.hoisted and
using that in the mock implementation for getSurveyConfig, then reset/configure
that hoisted mock per test instead of mutating mockSurveyConfigs. Concretely,
create a hoisted const (e.g., const hoistedGetSurveyConfig = vi.hoisted(() =>
vi.fn())) and have vi.mock('./surveyRegistry', () => ({ getSurveyConfig:
hoistedGetSurveyConfig })) so the mock is created before module init; in
beforeEach call hoistedGetSurveyConfig.mockReset() and set
hoistedGetSurveyConfig.mockImplementationOnce(...) or mockReturnValue(...) for
each test to control returned { enabled } values rather than clearing a shared
object.
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
Wires the nightly survey system into the app by adding a controller component and a convenience composable for feature-site usage tracking.
Changes
Review Focus
Part of Nightly Survey System
This is part 5 of a stacked PR chain: