[Demo PR] Optimize the function feedback interface - #3493
wenjiangping wants to merge 43 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review: Optimize the function feedback interface
Solid, well-tested UI work. Strong accessibility (focus trap + restoration in UIModal, aria-invalid focus-on-error, real <button>s with aria-expanded/aria-haspopup, prefers-reduced-motion), a clean DRY refactor extracting Copilot context into diagnostic-context.ts with focused tests, and a safe Copilot prepare_feedback tool that only opens an editable draft and never auto-submits. All dynamic feedback/notification content is rendered via {{ }} interpolation (not v-html), so there is no XSS in the code as written.
The feature is scoped as a feedback-demo prototype (mock data + localStorage), so several backend behaviors described in the docs are intentionally out of scope. Findings focus on real runtime behavior and on latent issues that activate once this UI renders real server data. A few cross-cutting notes beyond the inline comments:
Runtime behavior (worth addressing):
blob:screenshot URLs are persisted (won't survive a reload) and never revoked on the failure path (FeedbackDemoUI.vuecaptureFeedbackScreenshot~L185-191 /handleSubmit). Convert to a durable form before persisting, or don't persist screenshots.
Latent / demo-scope notes (not blocking):
- The
/admin/feedbacksroute intentionally bypasses all auth (isFeedbackDemoRouteshort-circuits inadmin/index.vue), whiledocs/product/feedback.mdstates acanManageFeedback/feedbackAdmingate. Fine for a demo, but the real admin page must restore the auth gate and the docs should be scoped as a forward-looking spec vs. the shipped mock. data:image/*sources are accepted as renderable images without a scheme allowlist — harmless with mock data, but restrict tohttps:/blob:before wiring to real, cross-user attachments.- EN/ZH doc nits:
## User Stories(EN) vs## User Story(ZH); ZH “工程” vs the shipped UI's “项目”. - New mock IDs use
feedback-${feedbacks.length + 1001}, which can collide with fixture IDs after add/remove; prefer a monotonic counter/nanoid.
No blocking issues.
| <div | ||
| v-if="visible" | ||
| class="fixed inset-0 z-1100" | ||
| :class="mask ? 'bg-overlay-modal' : 'bg-transparent'" |
There was a problem hiding this comment.
When mask: false (used by the notification center), this full-screen fixed inset-0 overlay renders bg-transparent but keeps capturing pointer events across the entire viewport — so the page behind becomes unclickable and any outside click hits handleMaskClick. The inner surface gets pointer-events-auto, but the overlay itself needs pointer-events-none when !mask for a dropdown-like surface to behave as intended. Relatedly, aria-modal="true"/role="dialog" (L22) are hardcoded; for the non-modal popover usage this misleads assistive tech — consider making aria-modal conditional on props.mask.
| watch( | ||
| model.data, | ||
| (data) => { | ||
| localStorage.setItem(feedbackDemoStorageKey, JSON.stringify({ version: feedbackDemoMockVersion, data })) |
There was a problem hiding this comment.
This deep watcher is installed app-wide (via provideFeedbackDemoModel() at the root for all non-admin pages). Every nested mutation — submitting feedback, marking one notification read, markAllNotificationsRead looping over N notifications — triggers a full JSON.stringify of the entire dataset (feedbacks with embedded diagnostic contexts, code samples, runtime outputs) plus a synchronous localStorage.setItem. This grows increasingly expensive as mock data accumulates and blocks the main thread. Consider debouncing the persist or using { flush: 'post' } with throttling.
| desc: notification.readAt == null ? 'Unread reply from XBuilder Support' : 'Reply from XBuilder Support' | ||
| }" | ||
| class="group relative block w-full cursor-pointer rounded-lg border-0 bg-white p-3 text-left transition-colors hover:bg-grey-300 focus-visible:relative focus-visible:z-1 focus-visible:outline-2 focus-visible:outline-primary-main" | ||
| :class="notification.readAt == null ? 'bg-white' : 'bg-white'" |
There was a problem hiding this comment.
Both branches of this conditional are identical (bg-white), so the binding is a no-op. Either the unread state (readAt == null) was meant to have a distinct background, or this :class should be removed.
| <a | ||
| v-if="attachment.url != null" | ||
| class="inline-flex items-center gap-2 rounded-md bg-grey-300 px-3 py-2 text-xs text-grey-900 no-underline transition-colors hover:bg-grey-400 hover:text-primary-main focus-visible:outline-2 focus-visible:outline-primary-main" | ||
| :href="attachment.url" |
There was a problem hiding this comment.
:href="attachment.url" is rendered with no scheme allowlist in an administrator's session. Per the design doc, attachment URLs originate from submitting users; if a record ever carries an attacker-controlled url (e.g. javascript:/data:), this becomes a script-execution/phishing vector in the admin origin. Validate the scheme (allow only https:/server-issued blob:) before rendering the anchor. Also add noopener explicitly (rel="noopener noreferrer") alongside target="_blank" rather than relying on noreferrer implying it. Latent for the current mock, but worth fixing before this renders real data.
| v-radar="notificationRadar" | ||
| :aria-label="notificationLabel" | ||
| type="button" | ||
| class="h-full cursor-pointer border-0 bg-transparent px-3 text-grey-900 hover:bg-grey-400 focus-visible:outline-primary-main" |
There was a problem hiding this comment.
This button uses focus-visible:outline-primary-main but omits the focus-visible:outline-2 width applied consistently elsewhere in this PR (e.g. NavbarProfile.vue), so the focus ring will be inconsistent / near-invisible here. Add focus-visible:outline-2.
Related
Related to #3484
Based on #3390
Background
The feedback notification experience needs closer alignment with the latest design. The notification entry, list, detail view, and attachment preview currently have inconsistent visual hierarchy and interaction behavior.
Experience changes
Impact
Verification