diff --git a/alembic/versions/a7c3e9d1f5b8_feedback_library_and_language.py b/alembic/versions/a7c3e9d1f5b8_feedback_library_and_language.py new file mode 100644 index 00000000000..25c115d2c31 --- /dev/null +++ b/alembic/versions/a7c3e9d1f5b8_feedback_library_and_language.py @@ -0,0 +1,42 @@ +"""feedback_library_and_language + +The 👍/👎 buttons moved from the floating feedback widget onto the plot +itself, so a reaction is now about one implementation rather than a page. +Store that implementation explicitly — `library_id` + `language` next to the +existing `spec_id` — instead of parsing it back out of `path`, and index the +(spec_id, library_id) pair so votes can be counted per image later. + +Both columns are nullable: page-level feedback from the floating widget +(messages, bug/idea reactions) still leaves them empty. + +Revision ID: a7c3e9d1f5b8 +Revises: f4b8d2c6a9e1 +Create Date: 2026-09-10 + +""" + +from typing import Sequence + +import sqlalchemy as sa + +from alembic import op + + +revision: str = "a7c3e9d1f5b8" +down_revision: str | None = "f4b8d2c6a9e1" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + with op.batch_alter_table("feedback") as batch_op: + batch_op.add_column(sa.Column("library_id", sa.String(50), nullable=True)) + batch_op.add_column(sa.Column("language", sa.String(50), nullable=True)) + batch_op.create_index("ix_feedback_spec_library", ["spec_id", "library_id"]) + + +def downgrade() -> None: + with op.batch_alter_table("feedback") as batch_op: + batch_op.drop_index("ix_feedback_spec_library") + batch_op.drop_column("language") + batch_op.drop_column("library_id") diff --git a/api/routers/feedback.py b/api/routers/feedback.py index 60133fd6298..f76d78e2798 100644 --- a/api/routers/feedback.py +++ b/api/routers/feedback.py @@ -28,7 +28,14 @@ MAX_MESSAGE_LENGTH = 500 MAX_CONTACT_LENGTH = 255 RATE_LIMIT_WINDOW = timedelta(minutes=1) +# Free-text entries: 5 per minute per IP, counted over message-bearing rows only. RATE_LIMIT_MAX = 5 +# Reaction-only entries (a 👍/👎 tap on a plot): someone flipping through the +# library carousel rates several images in quick succession, so the cap is +# looser and counted over every row from that IP. +REACTION_RATE_LIMIT_MAX = 30 +# The two reactions the plot buttons send — the ones subject to one-vote-per-image. +PLOT_VOTE_REACTIONS = ("thumbs_up", "thumbs_down") # Anti-spam heuristics — see _is_link_stuffed and the duplicate check below. # Both branches return 200 silently (mirroring the honeypot) so bots can't @@ -89,18 +96,36 @@ async def submit_feedback( if ip_hash: since = now_utc_naive - RATE_LIMIT_WINDOW - recent = await repo.count_recent_by_ip(ip_hash, since) - if recent >= RATE_LIMIT_MAX: + if message is not None: + recent = await repo.count_recent_by_ip(ip_hash, since, messages_only=True) + limit = RATE_LIMIT_MAX + else: + recent = await repo.count_recent_by_ip(ip_hash, since) + limit = REACTION_RATE_LIMIT_MAX + if recent >= limit: raise HTTPException(status_code=429, detail="Too many feedback submissions, please slow down") # Silent duplicate suppression: same message text from the same IP or # session id within DUPLICATE_LOOKBACK is dropped without an error — keeps # repeated bot copy-paste out of the DB without revealing the filter. + session_id = (payload.session_id or None) and payload.session_id[:64] if message and await repo.has_recent_duplicate( - message, - ip_hash or None, - (payload.session_id or None) and payload.session_id[:64], - now_utc_naive - DUPLICATE_LOOKBACK, + message, ip_hash or None, session_id, now_utc_naive - DUPLICATE_LOOKBACK + ): + return FeedbackResponse(status="ok") + + # One 👍/👎 per image per session: the plot buttons lock after the first + # tap, so a second vote from the same session for the same implementation + # only ever comes from someone working around the UI — drop it silently. + spec_id = (payload.spec_id or None) and payload.spec_id[:100] + library_id = (payload.library_id or None) and payload.library_id[:50] + language = (payload.language or None) and payload.language[:50] + if ( + reaction in PLOT_VOTE_REACTIONS + and session_id + and spec_id + and library_id + and await repo.has_plot_vote(session_id, spec_id, library_id, language) ): return FeedbackResponse(status="ok") @@ -112,9 +137,11 @@ async def submit_feedback( "reaction": reaction, "contact": contact, "path": (payload.path or None) and payload.path[:500], - "spec_id": (payload.spec_id or None) and payload.spec_id[:100], + "spec_id": spec_id, + "library_id": library_id, + "language": language, "viewport": (payload.viewport or None) and payload.viewport[:20], - "session_id": (payload.session_id or None) and payload.session_id[:64], + "session_id": session_id, "user_agent": user_agent, "ip_hash": ip_hash or None, } diff --git a/api/schemas.py b/api/schemas.py index e09a19ddc68..b9524192934 100644 --- a/api/schemas.py +++ b/api/schemas.py @@ -162,6 +162,10 @@ class FeedbackRequest(BaseModel): contact: str | None = None path: str | None = None spec_id: str | None = None + # The implementation a 👍/👎 on a plot refers to; page-level feedback from + # the floating widget sends neither. + library_id: str | None = None + language: str | None = None viewport: str | None = None session_id: str | None = None # Honeypot field — real users never fill this in. Bots auto-fill all diff --git a/app/src/components/FeedbackWidget.tsx b/app/src/components/FeedbackWidget.tsx index e21ec8b2493..4e190f14e0a 100644 --- a/app/src/components/FeedbackWidget.tsx +++ b/app/src/components/FeedbackWidget.tsx @@ -19,10 +19,10 @@ import Tooltip from '@mui/material/Tooltip'; import { useAnalytics } from 'src/hooks'; import { useLocalStorage } from 'src/hooks/useLocalStorage'; import { apiPost, endpoints } from 'src/lib/api'; -import { RESERVED_TOP_LEVEL } from 'src/routes/paths'; +import { specIdFromPath } from 'src/routes/paths'; +import { FEEDBACK_SESSION_KEY, newFeedbackSessionId } from 'src/utils/feedback'; const MAX_MESSAGE_LENGTH = 500; -const SESSION_KEY = 'anyplot_feedback_session'; const THANKS_TIMEOUT_MS = 1200; // Floating quick-action buttons sit on the page background so they read as @@ -46,28 +46,6 @@ const REACTIONS = [ type Reaction = (typeof REACTIONS)[number]['value']; type Mode = 'closed' | 'quick' | 'full'; -function specIdFromPath(pathname: string): string | undefined { - const segments = pathname.split('/').filter(Boolean); - if (segments.length === 0) return undefined; - if (RESERVED_TOP_LEVEL.has(segments[0])) return undefined; - return segments[0]; -} - -function newSessionId(): string { - if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { - return crypto.randomUUID(); - } - if (typeof crypto !== 'undefined' && typeof crypto.getRandomValues === 'function') { - const bytes = new Uint8Array(16); - crypto.getRandomValues(bytes); - return `s-${Array.from(bytes, b => b.toString(16).padStart(2, '0')).join('')}`; - } - // Browser without Web Crypto support (e.g. very old, or insecure context). The - // session id is an opaque correlation handle, not a credential — a coarse - // timestamp-derived id is acceptable here, but we never use Math.random(). - return `s-${Date.now().toString(36)}`; -} - /** * Floating quick-feedback widget (issue #5662). The FAB opens a small * vertical stack of 👍 / 👎 / 💬: the thumbs submit a reaction-only entry @@ -141,11 +119,11 @@ export function FeedbackWidget() { ? `translateY(-${lift - FAB_CENTER_FROM_BOTTOM_XS}px)` : 'none'; - const [sessionId, setSessionId] = useLocalStorage(SESSION_KEY, ''); + const [sessionId, setSessionId] = useLocalStorage(FEEDBACK_SESSION_KEY, ''); const ensureSessionId = (): string => { if (sessionId) return sessionId; - const fresh = newSessionId(); + const fresh = newFeedbackSessionId(); setSessionId(fresh); return fresh; }; diff --git a/app/src/hooks/useQuickReaction.ts b/app/src/hooks/useQuickReaction.ts new file mode 100644 index 00000000000..a637323af95 --- /dev/null +++ b/app/src/hooks/useQuickReaction.ts @@ -0,0 +1,66 @@ +import { useCallback } from 'react'; + +import { useAnalytics } from 'src/hooks/useAnalytics'; +import { useLocalStorage } from 'src/hooks/useLocalStorage'; +import { apiPost, endpoints } from 'src/lib/api'; +import { + FEEDBACK_SESSION_KEY, + newFeedbackSessionId, + type QuickReaction, + type VoteTarget, +} from 'src/utils/feedback'; + +/** + * Submit a reaction-only feedback entry (👍 / 👎) for one implementation. + * + * Headless counterpart to the FeedbackWidget's quick-stack: same endpoint, + * same session correlation and the same `feedback_submitted` analytics event + * (with `mode: "plot_overlay"`), but it leaves all UI to the caller so the + * buttons can sit directly on the plot. Unlike the widget it names the + * implementation (`library_id`, `language`) so votes can be counted per image. + * + * @returns An async `submit(reaction, target)` resolving to `true` when the + * server accepted the entry, `false` on any non-OK response or network error. + */ +export function useQuickReaction() { + const { trackEvent } = useAnalytics(); + const [sessionId, setSessionId] = useLocalStorage(FEEDBACK_SESSION_KEY, ''); + + return useCallback( + async (reaction: QuickReaction, target: VoteTarget): Promise => { + const session = sessionId || newFeedbackSessionId(); + if (!sessionId) setSessionId(session); + + const path = window.location.pathname + window.location.search; + + try { + await apiPost(endpoints.feedback, { + message: null, + reaction, + contact: null, + path, + spec_id: target.specId, + library_id: target.libraryId, + language: target.language, + viewport: `${window.innerWidth}x${window.innerHeight}`, + session_id: session, + website: '', + }); + } catch { + // Non-2xx or network failure — report as unsuccessful so the caller + // can roll back its optimistic UI. No retry: a quick vote is low-stakes. + return false; + } + trackEvent('feedback_submitted', { + path: path || undefined, + reaction, + has_contact: 'false', + spec_id: target.specId, + library: target.libraryId, + mode: 'plot_overlay', + }); + return true; + }, + [sessionId, setSessionId, trackEvent] + ); +} diff --git a/app/src/pages/SpecPage.tsx b/app/src/pages/SpecPage.tsx index 3ff41107770..c9ae8805c13 100644 --- a/app/src/pages/SpecPage.tsx +++ b/app/src/pages/SpecPage.tsx @@ -595,6 +595,7 @@ export function SpecPage() { /> setImageLoaded(true)} onCopyCode={handleCopyCode} onDownload={handleDownload} - onReport={() => - trackEvent('report_issue', { - spec: specId, - library: selectedLibrary || undefined, - }) - } onTrackEvent={trackEvent} /> diff --git a/app/src/routes/paths.test.ts b/app/src/routes/paths.test.ts index cea56928f55..bccc96fd208 100644 --- a/app/src/routes/paths.test.ts +++ b/app/src/routes/paths.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from 'vitest'; -import { langFromPath, paths, RESERVED_TOP_LEVEL, specPath } from 'src/routes/paths'; +import { + langFromPath, + paths, + RESERVED_TOP_LEVEL, + specIdFromPath, + specPath, +} from 'src/routes/paths'; describe('specPath', () => { it('builds the cross-language hub path', () => { @@ -36,6 +42,23 @@ describe('langFromPath', () => { }); }); +describe('specIdFromPath', () => { + it('returns the first segment of a spec route', () => { + expect(specIdFromPath('/scatter-basic')).toBe('scatter-basic'); + expect(specIdFromPath('/scatter-basic/python/altair')).toBe('scatter-basic'); + }); + + it('returns undefined for the root path', () => { + expect(specIdFromPath('/')).toBeUndefined(); + expect(specIdFromPath('')).toBeUndefined(); + }); + + it('returns undefined when the first segment is a reserved route', () => { + expect(specIdFromPath('/stats')).toBeUndefined(); + expect(specIdFromPath('/about/team')).toBeUndefined(); + }); +}); + describe('paths registry', () => { it('exposes every static route', () => { expect(paths.home).toBe('/'); diff --git a/app/src/routes/paths.ts b/app/src/routes/paths.ts index 0af6b5fcee6..d2e23694bea 100644 --- a/app/src/routes/paths.ts +++ b/app/src/routes/paths.ts @@ -44,6 +44,17 @@ export function langFromPath(pathname: string): string | undefined { return segments[1]; } +/** + * Parse the spec id from a pathname, returns undefined for the root path or + * when the first segment is a reserved top-level route. + */ +export function specIdFromPath(pathname: string): string | undefined { + const segments = pathname.split('/').filter(Boolean); + if (segments.length === 0) return undefined; + if (RESERVED_TOP_LEVEL.has(segments[0])) return undefined; + return segments[0]; +} + /** * Central route registry — the single source of truth for app URLs. * Components navigate via `paths.*` instead of hardcoded strings; spec-detail diff --git a/app/src/sections/spec-detail/SpecDetailView.test.tsx b/app/src/sections/spec-detail/SpecDetailView.test.tsx index 569534c4a80..b90ecbed15b 100644 --- a/app/src/sections/spec-detail/SpecDetailView.test.tsx +++ b/app/src/sections/spec-detail/SpecDetailView.test.tsx @@ -1,8 +1,8 @@ -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ThemeContext, type ThemeContextValue } from 'src/hooks/useLayoutContext'; import { SpecDetailView } from 'src/sections/spec-detail/SpecDetailView'; -import { render, screen, userEvent } from 'src/test-utils'; +import { fireEvent, render, screen, userEvent, waitFor } from 'src/test-utils'; import type { Implementation } from 'src/types'; const darkThemeValue: ThemeContextValue = { @@ -38,6 +38,7 @@ const implC = makeImpl({ }); const defaultProps = { + specId: 'scatter-basic', specTitle: 'Basic Scatter Plot', selectedLibrary: 'matplotlib', currentImpl: implB, @@ -46,16 +47,28 @@ const defaultProps = { codeCopied: null, downloadDone: null, viewMode: 'preview' as const, - reportUrl: 'https://github.com/example/anyplot/issues/new?template=report-plot-issue.yml', onImageLoad: vi.fn(), onCopyCode: vi.fn(), onDownload: vi.fn(), onViewModeChange: vi.fn(), - onReport: vi.fn(), onTrackEvent: vi.fn(), }; +const okResponse = () => + new Response(JSON.stringify({ status: 'ok' }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + describe('SpecDetailView', () => { + beforeEach(() => { + localStorage.clear(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + it('renders image with correct alt text', () => { render(); const img = screen.getByRole('img'); @@ -171,5 +184,123 @@ describe('SpecDetailView', () => { render(); expect(screen.queryByRole('img')).not.toBeInTheDocument(); expect(screen.queryByRole('button', { name: /copy code/i })).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /thumbs up/i })).not.toBeInTheDocument(); + }); + + describe('plot vote', () => { + it('renders 👍 and 👎 over the plot and no report action', () => { + render(); + expect(screen.getByRole('button', { name: /thumbs up/i })).toHaveAttribute( + 'aria-pressed', + 'false' + ); + expect(screen.getByRole('button', { name: /thumbs down/i })).toHaveAttribute( + 'aria-pressed', + 'false' + ); + expect(screen.queryByRole('link', { name: /report issue/i })).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /report issue/i })).not.toBeInTheDocument(); + }); + + it('renders both thumbs on the interactive surface too', () => { + render( + + ); + expect(screen.getByRole('button', { name: /thumbs up/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /thumbs down/i })).toBeInTheDocument(); + }); + + it('submits a thumbs_up for the current implementation and inks the button', async () => { + const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(okResponse()); + const user = userEvent.setup(); + render(); + + const up = screen.getByRole('button', { name: /thumbs up/i }); + await user.click(up); + + // Optimistic highlight + toast are applied immediately. + expect(up).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByText('>>> .liked')).toBeInTheDocument(); + + await waitFor(() => expect(fetchSpy).toHaveBeenCalledTimes(1)); + expect(String(fetchSpy.mock.calls[0][0])).toMatch(/\/feedback$/); + const body = JSON.parse((fetchSpy.mock.calls[0][1] as RequestInit).body as string); + expect(body).toMatchObject({ + message: null, + reaction: 'thumbs_up', + spec_id: 'scatter-basic', + library_id: 'matplotlib', + language: 'python', + }); + expect(typeof body.session_id).toBe('string'); + }); + + it('locks the vote: neither thumb posts again once one is chosen', async () => { + const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(okResponse()); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole('button', { name: /thumbs up/i })); + await waitFor(() => expect(fetchSpy).toHaveBeenCalledTimes(1)); + + const down = screen.getByRole('button', { name: /thumbs down/i }); + expect(down).toBeDisabled(); + await user.click(screen.getByRole('button', { name: /thumbs up/i })); + // The disabled thumb rejects pointer events; a synthetic click stands in + // for a user who works around the UI (e.g. via devtools). + fireEvent.click(down); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(screen.getByRole('button', { name: /thumbs up/i })).toHaveAttribute( + 'aria-pressed', + 'true' + ); + expect(down).toHaveAttribute('aria-pressed', 'false'); + }); + + it('rolls the highlight back when the submit fails', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(null, { status: 500 })); + const user = userEvent.setup(); + render(); + + const down = screen.getByRole('button', { name: /thumbs down/i }); + await user.click(down); + await waitFor(() => expect(down).toHaveAttribute('aria-pressed', 'false')); + expect(screen.queryByText('>>> .disliked')).not.toBeInTheDocument(); + }); + + it('remembers the vote per implementation across remounts and library switches', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(okResponse()); + const user = userEvent.setup(); + const { rerender, unmount } = render(); + + await user.click(screen.getByRole('button', { name: /thumbs up/i })); + await waitFor(() => + expect(screen.getByRole('button', { name: /thumbs up/i })).toHaveAttribute( + 'aria-pressed', + 'true' + ) + ); + + // Another library of the same spec starts unrated. + rerender(); + expect(screen.getByRole('button', { name: /thumbs up/i })).toHaveAttribute( + 'aria-pressed', + 'false' + ); + + // Coming back — even after a full remount — shows the earlier vote. + unmount(); + render(); + expect(screen.getByRole('button', { name: /thumbs up/i })).toHaveAttribute( + 'aria-pressed', + 'true' + ); + }); }); }); diff --git a/app/src/sections/spec-detail/SpecDetailView.tsx b/app/src/sections/spec-detail/SpecDetailView.tsx index 4a7fbbd490b..bf1761ea3f9 100644 --- a/app/src/sections/spec-detail/SpecDetailView.tsx +++ b/app/src/sections/spec-detail/SpecDetailView.tsx @@ -5,14 +5,17 @@ * Toggles between static preview (PNG) and interactive HTML iframe. */ -import { useCallback, useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import ContentCopyIcon from '@mui/icons-material/ContentCopy'; import DownloadIcon from '@mui/icons-material/Download'; -import FlagOutlinedIcon from '@mui/icons-material/FlagOutlined'; import ImageOutlinedIcon from '@mui/icons-material/ImageOutlined'; import OpenInNewIcon from '@mui/icons-material/OpenInNew'; import PlayArrowIcon from '@mui/icons-material/PlayArrow'; +import ThumbDownIcon from '@mui/icons-material/ThumbDown'; +import ThumbDownOutlinedIcon from '@mui/icons-material/ThumbDownOutlined'; +import ThumbUpIcon from '@mui/icons-material/ThumbUp'; +import ThumbUpOutlinedIcon from '@mui/icons-material/ThumbUpOutlined'; import Box from '@mui/material/Box'; import IconButton from '@mui/material/IconButton'; import Skeleton from '@mui/material/Skeleton'; @@ -20,15 +23,20 @@ import Tooltip from '@mui/material/Tooltip'; import { API_URL } from 'src/constants'; import { useTheme } from 'src/hooks/useLayoutContext'; +import { useLocalStorage } from 'src/hooks/useLocalStorage'; +import { useQuickReaction } from 'src/hooks/useQuickReaction'; import { colors, fontSize, overlayButtonSx, typography } from 'src/theme'; import type { Implementation } from 'src/types'; +import { PLOT_VOTES_KEY, type QuickReaction, voteKey, type VoteTarget } from 'src/utils/feedback'; import { buildDetailSrcSet, DETAIL_SIZES } from 'src/utils/responsiveImage'; import { selectPreviewHtml, selectPreviewUrl } from 'src/utils/themedPreview'; const INITIAL_WIDTH = 1600; const INITIAL_HEIGHT = 900; +const VOTE_TOAST_MS = 1200; interface SpecDetailViewProps { + specId: string; specTitle: string; selectedLibrary: string; currentImpl: Implementation | null; @@ -37,16 +45,15 @@ interface SpecDetailViewProps { codeCopied: string | null; downloadDone: string | null; viewMode: 'preview' | 'interactive'; - reportUrl: string; onViewModeChange: (mode: 'preview' | 'interactive') => void; onImageLoad: () => void; onCopyCode: (impl: Implementation) => void; onDownload: (impl: Implementation) => void; - onReport: () => void; onTrackEvent: (event: string, props?: Record) => void; } export function SpecDetailView({ + specId, specTitle, selectedLibrary, currentImpl, @@ -55,12 +62,10 @@ export function SpecDetailView({ codeCopied, downloadDone, viewMode, - reportUrl, onViewModeChange, onImageLoad, onCopyCode, onDownload, - onReport, onTrackEvent, }: SpecDetailViewProps) { const sortedImpls = [...implementations].sort((a, b) => a.library_id.localeCompare(b.library_id)); @@ -83,6 +88,51 @@ export function SpecDetailView({ } }, [selectedLibrary]); + // 👍 / 👎 on the plot — one tap rates THIS implementation, once: the vote is + // final for the session, so nobody can flip up/down at will (the server + // drops repeat votes from the same session for the same image as well). + // The visitor's own vote is kept per spec/language/library in localStorage + // so the thumb stays inked when they come back or flip through the carousel; + // the server row is what gets counted (FeedbackRepository.reaction_counts). + const submitReaction = useQuickReaction(); + const [votes, setVotes] = useLocalStorage>(PLOT_VOTES_KEY, {}); + const voteTarget = useMemo( + () => + currentImpl + ? { specId, language: currentImpl.language, libraryId: currentImpl.library_id } + : null, + [specId, currentImpl] + ); + const vote = voteTarget ? (votes[voteKey(voteTarget)] ?? null) : null; + const [voteToast, setVoteToast] = useState(null); + const voteToastTimerRef = useRef>(null); + const handleVote = useCallback( + (reaction: QuickReaction) => { + if (!voteTarget || vote) return; + const key = voteKey(voteTarget); + // Optimistic: ink the thumb now, roll back only if the server refused. + setVotes(prev => ({ ...prev, [key]: reaction })); + setVoteToast(reaction); + if (voteToastTimerRef.current) clearTimeout(voteToastTimerRef.current); + voteToastTimerRef.current = setTimeout(() => setVoteToast(null), VOTE_TOAST_MS); + void submitReaction(reaction, voteTarget).then(ok => { + if (ok) return; + setVotes(prev => { + const next = { ...prev }; + delete next[key]; + return next; + }); + setVoteToast(null); + }); + }, + [voteTarget, vote, setVotes, submitReaction] + ); + useEffect(() => { + return () => { + if (voteToastTimerRef.current) clearTimeout(voteToastTimerRef.current); + }; + }, []); + // Interactive iframe state — scaled to fit container const interactiveContainerRef = useRef(null); const [scale, setScale] = useState(1); @@ -196,12 +246,116 @@ export function SpecDetailView({ const previewHtml = selectPreviewHtml(currentImpl, isDark); const interactiveAvailable = !!previewHtml; - // Overlay action buttons (report/copy/download/open/preview/raw) sit on top + // Overlay action buttons (vote/copy/download/open/preview/raw) sit on top // of the preview image — shared theme-aware style, see src/theme/tokens.ts. const overlayBtnSx = overlayButtonSx(isDark); const proxyUrl = (url: string) => `${API_URL}/proxy/html?url=${encodeURIComponent(url)}&origin=${encodeURIComponent(window.location.origin)}`; + // 👍 sits top-left, 👎 bottom-left — the two corners the plot content + // rarely uses — so rating an image is one obvious tap without covering the + // title or the legend. The chosen thumb stays inked (green / matte red) so + // the choice reads as committed, not merely hovered, and the other thumb + // fades out once a vote is in. The same elements are rendered on both the + // static and the interactive surface. + const thumbSx = (active: boolean, locked: boolean, activeColor: string) => { + if (active) { + return { + ...overlayBtnSx, + color: activeColor, + '&:hover': { ...overlayBtnSx['&:hover'], color: activeColor }, + }; + } + if (locked) { + // Keep the overlay surface under the disabled thumb; MUI would drop it. + return { + ...overlayBtnSx, + '&.Mui-disabled': { bgcolor: overlayBtnSx.bgcolor, opacity: 0.45 }, + }; + } + return overlayBtnSx; + }; + const thumbButton = ( + reaction: QuickReaction, + label: string, + verb: string, + activeColor: string, + Outlined: typeof ThumbUpOutlinedIcon, + Filled: typeof ThumbUpIcon + ) => { + if (!currentImpl) return null; + const active = vote === reaction; + const locked = !!vote && !active; + return ( + // A natively disabled button emits no pointer events, so the tooltip + // listens on a wrapping span (MUI's documented pattern for this case). + + + { + (e.currentTarget as HTMLElement).blur(); + handleVote(reaction); + }} + aria-label={label} + aria-pressed={active} + disabled={locked} + sx={thumbSx(active, locked, activeColor)} + size="medium" + > + {active ? : } + + + + ); + }; + const thumbUpButton = thumbButton( + 'thumbs_up', + 'Thumbs up', + '.like()', + colors.primary, + ThumbUpOutlinedIcon, + ThumbUpIcon + ); + const thumbDownButton = thumbButton( + 'thumbs_down', + 'Thumbs down', + '.dislike()', + colors.error, + ThumbDownOutlinedIcon, + ThumbDownIcon + ); + // Centre toast, same shape as the `.copied` / `.downloaded` confirmations. + const toastText = voteToast + ? voteToast === 'thumbs_up' + ? '>>> .liked' + : '>>> .disliked' + : currentImpl && codeCopied === currentImpl.library_id + ? '>>> .copied' + : currentImpl && downloadDone === currentImpl.library_id + ? '>>> .downloaded' + : null; + const toast = toastText && ( + + {toastText} + + ); + return ( {viewMode === 'interactive' && interactiveAvailable && previewHtml ? ( @@ -245,21 +399,13 @@ export function SpecDetailView({ /> + {toast} + - - - - - + {thumbUpButton} + + + {thumbDownButton} @@ -371,28 +517,7 @@ export function SpecDetailView({ )} - {currentImpl && - (codeCopied === currentImpl.library_id || downloadDone === currentImpl.library_id) && ( - - {codeCopied === currentImpl.library_id ? '>>> .copied' : '>>> .downloaded'} - - )} + {toast} e.stopPropagation()} @@ -404,23 +529,19 @@ export function SpecDetailView({ gap: 0.5, }} > - - { - (e.currentTarget as HTMLElement).blur(); - onReport(); - }} - aria-label="Report issue" - sx={overlayBtnSx} - size="medium" - > - - - + {thumbUpButton} + + e.stopPropagation()} + sx={{ + position: 'absolute', + bottom: 8, + left: 8, + display: zoomed ? 'none' : 'flex', + gap: 0.5, + }} + > + {thumbDownButton} { + it('exposes stable storage keys and the two quick reactions', () => { + expect(FEEDBACK_SESSION_KEY).toBe('anyplot_feedback_session'); + expect(PLOT_VOTES_KEY).toBe('anyplot_plot_votes'); + expect(QUICK_REACTIONS).toEqual(['thumbs_up', 'thumbs_down']); + }); +}); + +describe('voteKey', () => { + it('keys a vote by spec, language and library', () => { + expect(voteKey({ specId: 'scatter-basic', language: 'python', libraryId: 'matplotlib' })).toBe( + 'scatter-basic/python/matplotlib' + ); + }); +}); + +describe('newFeedbackSessionId', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('prefers crypto.randomUUID when available', () => { + vi.spyOn(crypto, 'randomUUID').mockReturnValue('11111111-1111-4111-8111-111111111111'); + expect(newFeedbackSessionId()).toBe('11111111-1111-4111-8111-111111111111'); + }); + + it('falls back to getRandomValues when randomUUID is unavailable', () => { + const original = crypto.randomUUID; + Object.defineProperty(crypto, 'randomUUID', { value: undefined, configurable: true }); + try { + expect(newFeedbackSessionId()).toMatch(/^s-[0-9a-f]{32}$/); + } finally { + Object.defineProperty(crypto, 'randomUUID', { value: original, configurable: true }); + } + }); + + it('degrades to a timestamp id when Web Crypto is entirely absent', () => { + const original = globalThis.crypto; + Object.defineProperty(globalThis, 'crypto', { value: undefined, configurable: true }); + try { + expect(newFeedbackSessionId()).toMatch(/^s-[0-9a-z]+$/); + } finally { + Object.defineProperty(globalThis, 'crypto', { value: original, configurable: true }); + } + }); +}); diff --git a/app/src/utils/feedback.ts b/app/src/utils/feedback.ts new file mode 100644 index 00000000000..e49fd7cf883 --- /dev/null +++ b/app/src/utils/feedback.ts @@ -0,0 +1,48 @@ +/** + * Shared helpers for the lightweight feedback channel (issue #5662). + * + * Both the floating FeedbackWidget and the 👍/👎 buttons on a plot submit to + * the same `/feedback` endpoint and correlate a visitor across submissions + * with an opaque, locally stored session id. Keeping the key and the id + * generator here keeps the two entry points in lock-step. + */ + +export const FEEDBACK_SESSION_KEY = 'anyplot_feedback_session'; + +/** localStorage key for the visitor's own 👍/👎 per implementation. */ +export const PLOT_VOTES_KEY = 'anyplot_plot_votes'; + +export const QUICK_REACTIONS = ['thumbs_up', 'thumbs_down'] as const; +export type QuickReaction = (typeof QUICK_REACTIONS)[number]; + +/** The implementation a plot vote is about. */ +export interface VoteTarget { + specId: string; + language: string; + libraryId: string; +} + +/** Stable key for one implementation in the persisted votes map. */ +export function voteKey({ specId, language, libraryId }: VoteTarget): string { + return `${specId}/${language}/${libraryId}`; +} + +/** + * Generate an opaque session id used purely as a correlation handle (never a + * credential), preferring Web Crypto and degrading gracefully on browsers + * without it. Never falls back to `Math.random()`. + */ +export function newFeedbackSessionId(): string { + if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { + return crypto.randomUUID(); + } + if (typeof crypto !== 'undefined' && typeof crypto.getRandomValues === 'function') { + const bytes = new Uint8Array(16); + crypto.getRandomValues(bytes); + return `s-${Array.from(bytes, b => b.toString(16).padStart(2, '0')).join('')}`; + } + // Browser without Web Crypto support (e.g. very old, or insecure context). The + // session id is an opaque correlation handle, not a credential — a coarse + // timestamp-derived id is acceptable here. + return `s-${Date.now().toString(36)}`; +} diff --git a/changelog.d/thumbs-on-plot.md b/changelog.d/thumbs-on-plot.md new file mode 100644 index 00000000000..b6a787f67c5 --- /dev/null +++ b/changelog.d/thumbs-on-plot.md @@ -0,0 +1,29 @@ +### Added + +- **👍 / 👎 directly on the plot.** A spec detail view now carries a thumbs-up + button top-left and a thumbs-down button bottom-left over the image — both + on the static preview and the interactive surface — so rating an + implementation is one tap instead of a trip through the floating feedback + menu. The tap inks the thumb, flashes `>>> .liked` / `>>> .disliked` in the + centre like `.copied` does, and posts a reaction-only entry to `/feedback` + with `library_id` and `language` (a new Alembic revision adds both columns + and a `(spec_id, library_id)` index), so votes can be counted per image later + via `FeedbackRepository.reaction_counts` — nothing is displayed yet. One vote + per image per session: the choice is remembered in localStorage and the other + thumb fades out, and the server drops a repeat vote from the same session for + the same implementation silently, so nobody can flip up and down at will. + Supersedes the stale PR #8126, which placed both thumbs top-left. + +### Changed + +- **Reaction-only feedback has its own rate limit.** Free-text entries stay at + 5 per minute per IP, now counted over message-bearing rows only; a 👍/👎 tap + is capped at 30 per minute, so flipping through the library carousel and + rating several plots no longer trips the free-text limit or blocks a message + written right after. + +### Removed + +- **The `.report()` flag over the plot.** Reporting stays on the spec hub page + (`report issue ↗`) and via the GitHub issue template; the in-plot flag was + the least-used overlay action and its corner now belongs to the vote. diff --git a/core/database/models.py b/core/database/models.py index 467b964ed74..2fd8f714478 100644 --- a/core/database/models.py +++ b/core/database/models.py @@ -252,6 +252,11 @@ class Feedback(Base): # Context captured automatically with the message path: Mapped[str | None] = mapped_column(String(500), nullable=True) spec_id: Mapped[str | None] = mapped_column(String(MAX_SPEC_ID_LENGTH), nullable=True) + # Set only by the 👍/👎 buttons on a plot: the implementation the reaction + # is about, so votes can be counted per image (spec × language × library). + # Page-level feedback from the floating widget leaves both NULL. + library_id: Mapped[str | None] = mapped_column(String(MAX_LIBRARY_ID_LENGTH), nullable=True) + language: Mapped[str | None] = mapped_column(String(MAX_LANGUAGE_ID_LENGTH), nullable=True) user_agent: Mapped[str | None] = mapped_column(String(500), nullable=True) viewport: Mapped[str | None] = mapped_column(String(20), nullable=True) session_id: Mapped[str | None] = mapped_column(String(64), nullable=True) @@ -273,6 +278,9 @@ class Feedback(Base): # does not propose dropping them. Index("ix_feedback_created_at", text("created_at DESC")), Index("ix_feedback_ip_hash_created_at", "ip_hash", text("created_at DESC")), + # Per-image vote counts (migration a7c3e9d1f5b8): reactions are looked up + # by the implementation they belong to. + Index("ix_feedback_spec_library", "spec_id", "library_id"), ) diff --git a/core/database/repositories.py b/core/database/repositories.py index 8ba91adb52d..1804ae86d56 100644 --- a/core/database/repositories.py +++ b/core/database/repositories.py @@ -420,13 +420,69 @@ class FeedbackRepository(BaseRepository[Feedback]): model = Feedback updatable_fields = frozenset({"status"}) - async def count_recent_by_ip(self, ip_hash: str, since: datetime) -> int: + async def count_recent_by_ip(self, ip_hash: str, since: datetime, messages_only: bool = False) -> int: """Count entries from this IP hash since the given UTC datetime — used for rate limiting. - `since` must be tz-naive UTC because feedback.created_at is TIMESTAMP WITHOUT TIME ZONE.""" + `since` must be tz-naive UTC because feedback.created_at is TIMESTAMP WITHOUT TIME ZONE. + With `messages_only`, reaction-only rows are left out so a handful of 👍/👎 taps + never blocks the free-text form.""" + stmt = select(func.count(Feedback.id)).where(Feedback.ip_hash == ip_hash, Feedback.created_at >= since) + if messages_only: + stmt = stmt.where(Feedback.message.is_not(None)) + result = await self.session.execute(stmt) + return result.scalar_one() or 0 + + async def has_plot_vote(self, session_id: str, spec_id: str, library_id: str, language: str | None) -> bool: + """True iff this session already left a 👍/👎 on this implementation. + + One vote per image per session: the router drops later votes from the + same session silently, so a visitor cannot flip up/down at will. + """ + stmt = select(func.count(Feedback.id)).where( + Feedback.session_id == session_id, + Feedback.spec_id == spec_id, + Feedback.library_id == library_id, + Feedback.reaction.in_(("thumbs_up", "thumbs_down")), + ) + if language is not None: + stmt = stmt.where(Feedback.language == language) + result = await self.session.execute(stmt) + return (result.scalar_one() or 0) > 0 + + async def reaction_counts(self, spec_id: str, library_id: str, language: str | None = None) -> dict[str, int]: + """Count 👍/👎 for one implementation — `{"thumbs_up": n, "thumbs_down": n}`. + + The router already refuses a second vote per session, but the table is + append-only and rows from before that guard (or without it) may repeat, + so only the newest row per session counts. Rows without a session id + cannot be de-duplicated and count once each. `language` narrows the + lookup when the same library id exists in more than one language. + """ + thumbs = ("thumbs_up", "thumbs_down") + filters = [Feedback.spec_id == spec_id, Feedback.library_id == library_id, Feedback.reaction.in_(thumbs)] + if language is not None: + filters.append(Feedback.language == language) + # One partition per session; a session-less row is its own partition + # (the primary key stands in for the missing session id). The id is + # also the tiebreaker, so two rows sharing a created_at rank the same + # way on every run. + partition = func.coalesce(Feedback.session_id, cast(Feedback.id, String)) + ranked = ( + select( + Feedback.reaction.label("reaction"), + func.row_number() + .over(partition_by=partition, order_by=(Feedback.created_at.desc(), Feedback.id.desc())) + .label("rank"), + ) + .where(*filters) + .subquery() + ) result = await self.session.execute( - select(func.count(Feedback.id)).where(Feedback.ip_hash == ip_hash, Feedback.created_at >= since) + select(ranked.c.reaction, func.count().label("count")).where(ranked.c.rank == 1).group_by(ranked.c.reaction) ) - return result.scalar_one() or 0 + counts = dict.fromkeys(thumbs, 0) + for row in result.all(): + counts[row.reaction] = row.count + return counts async def list_recent(self, limit: int = 100) -> list[Feedback]: """List most recent entries first — for admin/triage tooling.""" diff --git a/docs/contributing.md b/docs/contributing.md index 5d1e81405a1..89441f2f792 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -38,7 +38,7 @@ By participating, you agree to our [Code of Conduct](../CODE_OF_CONDUCT.md). Found a problem with a plot (specification or implementation)? -1. **From anyplot.ai**: Click "report issue" on any spec or implementation page +1. **From anyplot.ai**: Click "report issue ↗" on a spec page, or "report" in the footer 2. **From GitHub**: Use the [Report Plot Issue](https://github.com/MarkusNeusinger/anyplot/issues/new?template=report-plot-issue.yml) template 3. **Select affected libraries** (or leave empty if all/unsure) 4. **Choose a category** (optional): Visual, Data, Functional, or Other diff --git a/docs/reference/plausible.md b/docs/reference/plausible.md index 0a597a5bb7b..20d1c1f311e 100644 --- a/docs/reference/plausible.md +++ b/docs/reference/plausible.md @@ -130,7 +130,7 @@ https://anyplot.ai/{spec_id}/{language}/{library}/{category}/{value}/... | `plot_rotate` | `spec` | SpecsListPage.tsx | User clicks image on specs page to rotate library | | `open_interactive` | `spec`, `library` | SpecOverview.tsx, SpecDetailView.tsx | User opens interactive HTML view | | `suggest_spec` | - | SpecsListPage.tsx | User clicks the `spec.suggest()` link on the specs list page. The mirror link on the landing page emits `nav_click` with `source: suggest_spec_link` instead. | -| `report_issue` | `spec`, `library`? | SpecPage.tsx | User clicks "report issue" link | +| `report_issue` | `spec`, `library`? | SpecPage.tsx | User clicks the "report issue ↗" link on a spec hub page. The in-plot `.report()` flag was replaced by the 👍/👎 vote buttons, so `library` is now only set when the hub page has a library selected. | | `tag_click` | `param`, `value`, `source` | SpecTabs.tsx, StatsPage.tsx | User clicks a tag chip to filter (`source` ∈ `spec_detail`, `stats`) | | `theme_toggle` | `to` | MastheadRule.tsx | User cycles tri-state theme mode (`to` ∈ `system`, `light`, `dark`). The cycle order is `system → light → dark → system`. | | `view_mode_change` | `mode`, `library` | SpecDetailView.tsx | User toggles preview ↔ interactive view inside a spec detail. `mode` ∈ `preview`, `interactive`. Fires on every toggle in either direction (cf. `open_interactive`, which only fires when the interactive HTML is opened in a new tab). | @@ -141,7 +141,7 @@ https://anyplot.ai/{spec_id}/{language}/{library}/{category}/{value}/... | `map_node_pin` | `spec` | MapPage.tsx | Touch device only: first tap on a node opens the preview panel + pin marker without navigating. A second tap on the same node fires `map_node_click` and navigates. | | `map_search_select` | `spec` | MapPage.tsx | User picks a result from the `/map` search dropdown (`⌘K` / `Ctrl+K` opens it). The camera flies to the node and the preview panel opens. | | `feedback_opened` | `path` | FeedbackWidget.tsx | User clicks the floating feedback FAB and the quick mini-stack of 👍 / 👎 / 💬 appears (issue #5662). `path` is `window.location.pathname + search` at open time. | -| `feedback_submitted` | `path`, `reaction`?, `has_contact`, `spec_id`?, `mode` | FeedbackWidget.tsx | User submits a feedback entry. `reaction` ∈ `thumbs_up`, `thumbs_down`, `bug`, `idea` (omitted if none selected). `mode` is `"quick"` for a one-tap 👍/👎 from the mini-stack, `"full"` for a submit from the detailed dialog. `has_contact` is `"true"`/`"false"` — the contact field is now a free-form name/email/handle, not strictly an email. `spec_id` is set when the current route resolves to a spec page. | +| `feedback_submitted` | `path`, `reaction`?, `has_contact`, `spec_id`?, `library`?, `mode` | FeedbackWidget.tsx, SpecDetailView.tsx (via useQuickReaction.ts) | User submits a feedback entry. `reaction` ∈ `thumbs_up`, `thumbs_down`, `bug`, `idea` (omitted if none selected). `mode` is `"quick"` for a one-tap 👍/👎 from the mini-stack, `"full"` for a submit from the detailed dialog, and `"plot_overlay"` for a 👍 (top-left) / 👎 (bottom-left) tap directly on the plot in a spec detail view — that one also carries `library`, because the vote is about one implementation and the server stores `library_id` + `language` with it. `has_contact` is `"true"`/`"false"` — the contact field is now a free-form name/email/handle, not strictly an email. `spec_id` is set when the current route resolves to a spec page. | ### Diagnostics @@ -456,7 +456,7 @@ To see event properties in Plausible dashboard, you **MUST** register them as cu |----------|-------------|----------------| | `spec` | Plot specification ID | `copy_code`, `download_image`, `plot_rotate`, `external_link`, `internal_link`, `open_interactive`, `report_issue`, `tag_click`, `og_image_view` | | `language` | Language slug (`python`, `r`, `julia`, `javascript`) | `og_image_view` | -| `library` | Library name (matplotlib, seaborn, etc.) | `copy_code`, `download_image`, `external_link`, `internal_link`, `open_interactive`, `tab_toggle`, `og_image_view` | +| `library` | Library name (matplotlib, seaborn, etc.) | `copy_code`, `download_image`, `external_link`, `internal_link`, `open_interactive`, `tab_toggle`, `og_image_view`, `feedback_submitted` (plot-overlay votes only) | | `method` | Action method (card, image, tab, click, space, doubletap) | `copy_code`, `random_filter` | | `page` | Page context (home, plots, spec_overview, spec_detail) | `copy_code`, `download_image`, `og_image_view` | | `platform` | Bot/platform name (twitter, whatsapp, teams, etc.) | `og_image_view` | diff --git a/tests/integration/api/test_api_endpoints.py b/tests/integration/api/test_api_endpoints.py index 94153ba820f..b4fa0f224c3 100644 --- a/tests/integration/api/test_api_endpoints.py +++ b/tests/integration/api/test_api_endpoints.py @@ -268,8 +268,10 @@ async def test_submit_with_context(self, client, test_db_with_data): "message": "Bug on mobile", "reaction": "bug", "contact": "user@example.com", - "path": "/scatter-basic", + "path": "/scatter-basic/python/matplotlib", "spec_id": "scatter-basic", + "library_id": "matplotlib", + "language": "python", "viewport": "375x812", "session_id": "abc-123", } @@ -287,6 +289,8 @@ async def test_submit_with_context(self, client, test_db_with_data): assert row.reaction == "bug" assert row.contact == "user@example.com" assert row.spec_id == "scatter-basic" + assert row.library_id == "matplotlib" + assert row.language == "python" assert row.viewport == "375x812" assert row.session_id == "abc-123" @@ -337,3 +341,27 @@ async def test_rate_limit_triggers_after_threshold(self, client): blocked = await client.post("/feedback", json={"message": "spam-blocked"}, headers=headers) assert blocked.status_code == 429 + + # A 👍 on a plot is not blocked by the free-text limit — its own cap is looser. + vote = await client.post( + "/feedback", + json={"reaction": "thumbs_up", "spec_id": "scatter-basic", "library_id": "matplotlib"}, + headers=headers, + ) + assert vote.status_code == 200 + + async def test_one_plot_vote_per_session_and_image(self, client, test_db_with_data): + """A second 👍/👎 from the same session on the same image is dropped silently.""" + vote = {"spec_id": "scatter-basic", "library_id": "matplotlib", "language": "python", "session_id": "s1"} + first = await client.post("/feedback", json={**vote, "reaction": "thumbs_up"}) + flipped = await client.post("/feedback", json={**vote, "reaction": "thumbs_down"}) + other_image = await client.post("/feedback", json={**vote, "library_id": "plotly", "reaction": "thumbs_down"}) + assert (first.status_code, flipped.status_code, other_image.status_code) == (200, 200, 200) + + from sqlalchemy import select + + from core.database.models import Feedback + + result = await test_db_with_data.execute(select(Feedback).order_by(Feedback.library_id)) + rows = list(result.scalars().all()) + assert [(r.library_id, r.reaction) for r in rows] == [("matplotlib", "thumbs_up"), ("plotly", "thumbs_down")] diff --git a/tests/integration/test_repositories.py b/tests/integration/test_repositories.py index d79b602ea3d..9f4276aace9 100644 --- a/tests/integration/test_repositories.py +++ b/tests/integration/test_repositories.py @@ -427,3 +427,63 @@ async def test_count_recent_by_ip(self, test_session): assert await repo.count_recent_by_ip("aaa", since) == 2 assert await repo.count_recent_by_ip("bbb", since) == 1 assert await repo.count_recent_by_ip("ccc", since) == 0 + + async def test_count_recent_by_ip_messages_only(self, test_session): + """Reaction-only rows are skipped when counting message-bearing entries.""" + from datetime import datetime, timedelta, timezone + + repo = FeedbackRepository(test_session) + + await repo.create({"message": "one", "ip_hash": "aaa"}) + await repo.create({"reaction": "thumbs_up", "ip_hash": "aaa"}) + await repo.create({"reaction": "thumbs_down", "ip_hash": "aaa"}) + + # tz-naive UTC, as the repository contract asks (created_at is TIMESTAMP WITHOUT TIME ZONE). + since = datetime.now(timezone.utc).replace(tzinfo=None) - timedelta(minutes=1) + assert await repo.count_recent_by_ip("aaa", since) == 3 + assert await repo.count_recent_by_ip("aaa", since, messages_only=True) == 1 + + async def test_has_plot_vote(self, test_session): + """A session's earlier 👍/👎 on the same implementation is found; others are not.""" + repo = FeedbackRepository(test_session) + vote = {"spec_id": "scatter-basic", "library_id": "matplotlib", "language": "python", "session_id": "s1"} + await repo.create({**vote, "reaction": "thumbs_up"}) + await repo.create({**vote, "reaction": "bug", "library_id": "plotly"}) + + assert await repo.has_plot_vote("s1", "scatter-basic", "matplotlib", "python") is True + assert await repo.has_plot_vote("s1", "scatter-basic", "matplotlib", None) is True + assert await repo.has_plot_vote("s1", "scatter-basic", "matplotlib", "r") is False + assert await repo.has_plot_vote("s1", "scatter-basic", "plotly", "python") is False + assert await repo.has_plot_vote("s2", "scatter-basic", "matplotlib", "python") is False + + async def test_reaction_counts_per_implementation(self, test_session): + """Counts 👍/👎 per (spec, library), newest row per session wins.""" + from datetime import datetime, timedelta + + repo = FeedbackRepository(test_session) + base = datetime(2026, 9, 10, 12, 0, 0) + vote = {"spec_id": "scatter-basic", "library_id": "matplotlib", "language": "python"} + + # Session s1 changes its mind: 👍 then 👎 — only the 👎 counts. + await repo.create({**vote, "reaction": "thumbs_up", "session_id": "s1", "created_at": base}) + await repo.create( + {**vote, "reaction": "thumbs_down", "session_id": "s1", "created_at": base + timedelta(seconds=5)} + ) + # Session s2 and a session-less row both like it. + await repo.create({**vote, "reaction": "thumbs_up", "session_id": "s2", "created_at": base}) + await repo.create({**vote, "reaction": "thumbs_up", "created_at": base}) + # Other implementation, other spec, page-level widget row: all ignored. + await repo.create({**vote, "library_id": "plotly", "reaction": "thumbs_up", "session_id": "s3"}) + await repo.create({**vote, "spec_id": "bar-basic", "reaction": "thumbs_up", "session_id": "s3"}) + await repo.create({"spec_id": "scatter-basic", "reaction": "thumbs_up", "session_id": "s3"}) + await repo.create({**vote, "reaction": "bug", "session_id": "s4"}) + + counts = await repo.reaction_counts("scatter-basic", "matplotlib") + assert counts == {"thumbs_up": 2, "thumbs_down": 1} + + assert await repo.reaction_counts("scatter-basic", "matplotlib", language="python") == counts + assert await repo.reaction_counts("scatter-basic", "matplotlib", language="r") == { + "thumbs_up": 0, + "thumbs_down": 0, + } + assert await repo.reaction_counts("scatter-basic", "plotly") == {"thumbs_up": 1, "thumbs_down": 0} diff --git a/tests/unit/api/test_feedback_router.py b/tests/unit/api/test_feedback_router.py index d24221925a5..b5da7c3a52e 100644 --- a/tests/unit/api/test_feedback_router.py +++ b/tests/unit/api/test_feedback_router.py @@ -140,6 +140,70 @@ def test_happy_path_calls_repo_create(self, client): assert kwargs["ip_hash"] # sha256 hex, not raw IP assert "198.51.100.7" not in kwargs["ip_hash"] + def test_plot_vote_persists_library_and_language(self, client): + """A 👍 from the plot overlay carries the implementation it is about.""" + instance = AsyncMock() + instance.count_recent_by_ip = AsyncMock(return_value=0) + instance.has_plot_vote = AsyncMock(return_value=False) + instance.create = AsyncMock(return_value=None) + + with patch("api.routers.feedback.FeedbackRepository", return_value=instance): + response = client.post( + "/feedback", + json={ + "reaction": "thumbs_down", + "spec_id": "scatter-basic", + "library_id": "matplotlib", + "language": "python", + "session_id": "s1", + }, + ) + + assert response.status_code == 200 + instance.has_plot_vote.assert_awaited_once_with("s1", "scatter-basic", "matplotlib", "python") + kwargs = instance.create.await_args.args[0] + assert kwargs["library_id"] == "matplotlib" + assert kwargs["language"] == "python" + + def test_second_plot_vote_from_same_session_is_silently_dropped(self, client): + """One 👍/👎 per image per session — a repeat returns 200 but writes nothing.""" + instance = AsyncMock() + instance.count_recent_by_ip = AsyncMock(return_value=0) + instance.has_plot_vote = AsyncMock(return_value=True) + instance.create = AsyncMock(return_value=None) + + with patch("api.routers.feedback.FeedbackRepository", return_value=instance): + response = client.post( + "/feedback", + json={ + "reaction": "thumbs_up", + "spec_id": "scatter-basic", + "library_id": "matplotlib", + "language": "python", + "session_id": "s1", + }, + ) + + assert response.status_code == 200 + assert response.json() == {"status": "ok"} + instance.create.assert_not_awaited() + + def test_page_level_reaction_skips_the_vote_guard(self, client): + """The floating widget's 👍 names no library, so it is never treated as a plot vote.""" + instance = AsyncMock() + instance.count_recent_by_ip = AsyncMock(return_value=0) + instance.has_plot_vote = AsyncMock(return_value=True) + instance.create = AsyncMock(return_value=None) + + with patch("api.routers.feedback.FeedbackRepository", return_value=instance): + response = client.post( + "/feedback", json={"reaction": "thumbs_up", "spec_id": "scatter-basic", "session_id": "s1"} + ) + + assert response.status_code == 200 + instance.has_plot_vote.assert_not_awaited() + instance.create.assert_awaited_once() + def test_rate_limit_returns_429(self, client): """Should return 429 when the rate-limit query reports too many recent entries.""" instance = AsyncMock() @@ -151,6 +215,37 @@ def test_rate_limit_returns_429(self, client): assert response.status_code == 429 instance.create.assert_not_awaited() + # Free-text entries are throttled on message-bearing rows only. + assert instance.count_recent_by_ip.await_args.kwargs == {"messages_only": True} + + def test_reaction_only_uses_the_looser_limit(self, client): + """Five recent rows block a message, but a plot vote still goes through.""" + instance = AsyncMock() + instance.count_recent_by_ip = AsyncMock(return_value=5) + instance.create = AsyncMock(return_value=None) + + with patch("api.routers.feedback.FeedbackRepository", return_value=instance): + response = client.post( + "/feedback", json={"reaction": "thumbs_up"}, headers={"x-forwarded-for": "198.51.100.8"} + ) + + assert response.status_code == 200 + instance.create.assert_awaited_once() + assert instance.count_recent_by_ip.await_args.kwargs == {} + + def test_reaction_only_rate_limit_returns_429(self, client): + """Reaction-only entries are still capped, just higher than free text.""" + instance = AsyncMock() + instance.count_recent_by_ip = AsyncMock(return_value=30) + instance.create = AsyncMock(return_value=None) + + with patch("api.routers.feedback.FeedbackRepository", return_value=instance): + response = client.post( + "/feedback", json={"reaction": "thumbs_up"}, headers={"x-forwarded-for": "198.51.100.8"} + ) + + assert response.status_code == 429 + instance.create.assert_not_awaited() class TestClientIpResolution: