Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces a test-view feature for visitor tracking. It updates the LinkedIn icon SVG, implements test avatar and visitor profile overrides, adds test-view creation and deletion functionality to the visitors table UI, creates a test-view generation module for synthetic page-view events, and updates the document statistics API to separately handle test and real visitors. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const response = await fetch( | ||
| `/api/teams/${teamId}/documents/${documentId}/test-view`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }, | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 30 days ago
In general, to address SSRF findings you must ensure that user-controlled values cannot arbitrarily influence the destination of an outgoing request, especially the host, protocol, and sensitive path components. Common strategies are: (1) use allow‑lists to map user inputs to known safe identifiers, (2) strictly validate and constrain IDs used in paths, and (3) avoid passing raw query/route parameters into URLs without checks.
For this specific case, the risk is that documentId comes straight from router.query.id. The simplest fix that preserves existing behavior is to validate documentId as soon as it is derived from the router: ensure it is a simple, expected identifier (for example, a UUID‑like or slug string with limited characters) and treat anything else as invalid. Once we constrain documentId to a safe format, interpolating it into the relative URL path for fetch is no longer a problem from an SSRF perspective.
Concretely, in components/visitors/visitors-table.tsx:
- Replace the destructuring assignment
const { id: documentId } = router.query as { id: string };with logic that:- Reads
router.query.id, - Ensures it’s a string,
- Validates it against a conservative regular expression (e.g., allow alphanumerics, dashes, and underscores),
- Stores
nullif it’s invalid.
- Reads
- Update the type annotation for
documentIdaccordingly (e.g.,string | null). - Keep the existing guards (
if (!teamId || !documentId) return;) so that if the ID is missing or invalid, we simply do not issue thefetchrequest.
No other behavior needs to change: successful, valid IDs will produce exactly the same URL and behavior as before, while invalid/malicious IDs will be rejected early by not triggering the API call.
| @@ -107,7 +107,12 @@ | ||
| isVideo?: boolean; | ||
| }) { | ||
| const router = useRouter(); | ||
| const { id: documentId } = router.query as { id: string }; | ||
| const rawDocumentId = (router.query as { id?: string }).id; | ||
| const documentId = | ||
| typeof rawDocumentId === "string" && | ||
| /^[a-zA-Z0-9_-]+$/.test(rawDocumentId) | ||
| ? rawDocumentId | ||
| : null; | ||
| const teamInfo = useTeam(); | ||
| const teamId = teamInfo?.currentTeam?.id; | ||
| const [currentPage, setCurrentPage] = useState<number>(1); |
| const response = await fetch( | ||
| `/api/teams/${teamId}/documents/${documentId}/test-view`, | ||
| { | ||
| method: "DELETE", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }, | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 30 days ago
In general: avoid interpolating raw, user-controlled values into outbound request URLs. Instead, validate or normalize those values first, and only proceed if they match an expected pattern or allow-list. For identifiers, enforce a strict format (e.g., UUID, database ID regex). If validation fails, avoid making the request and surface an error to the user.
For this file, the minimal, non-breaking fix is to introduce a small validation step for documentId before constructing the fetch URL in handleDeleteTestView. Next.js DocumentVersion IDs are often UUIDs, so we can enforce that documentId matches a UUID v4-like pattern. Concretely:
- Define a helper function
isValidDocumentIdin this component file that checksdocumentIdagainst a regular expression for UUIDs. - Update
handleDeleteTestViewso that it:- Returns early (with a toast error) if
documentIdis missing or invalid, instead of using it in the URL. - Only constructs the
fetchURL ifisValidDocumentId(documentId)is true.
- Returns early (with a toast error) if
This keeps existing behavior for valid IDs while preventing unvalidated input from being used in the URL, and it directly addresses the CodeQL finding.
| @@ -99,6 +99,16 @@ | ||
| }, | ||
| }; | ||
|
|
||
| function isValidDocumentId(id: string | undefined | null): boolean { | ||
| if (!id || typeof id !== "string") { | ||
| return false; | ||
| } | ||
| // Accept UUID-like document identifiers (common for Prisma/Next.js IDs) | ||
| const uuidRegex = | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; | ||
| return uuidRegex.test(id); | ||
| } | ||
|
|
||
| export default function VisitorsTable({ | ||
| primaryVersion, | ||
| isVideo = false, | ||
| @@ -200,7 +210,10 @@ | ||
| }; | ||
|
|
||
| const handleDeleteTestView = async () => { | ||
| if (!teamId || !documentId) return; | ||
| if (!teamId || !documentId || !isValidDocumentId(documentId)) { | ||
| toast.error("Invalid document identifier. Unable to delete test views."); | ||
| return; | ||
| } | ||
|
|
||
| setIsDeletingTestView(true); | ||
| try { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@components/visitors/visitor-avatar.tsx`:
- Around line 10-16: TEST_AVATARS duplicates test viewer emails already defined
in generate-test-view.ts; replace this local map with an import from the
centralized test viewer config (extend that module to export avatar URLs if
needed) and remove the hardcoded TEST_AVATARS constant. Update
components/visitors/visitor-avatar.tsx to import the avatars mapping (named
export) from generate-test-view.ts (or its new exported config) and use that
mapping where TEST_AVATARS was referenced (preserve keys like
"marc@papermark.com" and "iuliia@papermark.com"). Ensure any consumers expect
the same Record<string,string> shape and add/adjust exports in
generate-test-view.ts to include avatar URLs.
In `@components/visitors/visitors-table.tsx`:
- Around line 73-100: The TEST_VIEWER_EMAILS and TEST_VIEWER_PROFILES constants
are duplicated here; remove these local definitions and instead import and use
the centralized test viewer data (the exports used in
lib/test-views/generate-test-view.ts or stats.ts) so there's a single source of
truth. Locate uses of TEST_VIEWER_EMAILS and TEST_VIEWER_PROFILES in this file
(visitors-table.tsx) and replace them with the imported symbols, delete the
local declarations, and ensure any type shapes align with the imported types
(adjust local typings or add a re-export/type import from the central module if
needed).
- Around line 141-153: The current logic hides the "Delete test views" button
when any real view exists because showTestViewButton is computed from
hasRealViews; change this so test-view removal remains possible: update the
showTestViewButton calculation (which currently uses hasTestView, hasRealViews,
and testViewsDeleted) to show the delete button whenever hasTestView is true and
testViewsDeleted is false (i.e., drop the !hasRealViews requirement), or
alternatively implement server-side auto-deletion of test views when the first
real view is created by adding that behavior to the views handling endpoint;
adjust the UI to use the new showTestViewButton and ensure
views?.viewsWithDuration and TEST_VIEWER_EMAILS logic remains unchanged.
In `@lib/test-views/generate-test-view.ts`:
- Around line 4-14: TEST_VIEWERS is duplicated as raw email lists elsewhere; add
and export a derived constant like TEST_VIEWER_EMAILS = TEST_VIEWERS.map(v =>
v.email) from this module and update the other modules to import it instead of
hardcoding emails. Specifically, export TEST_VIEWER_EMAILS from this file
alongside TEST_VIEWERS, then replace the hardcoded lists/keys referenced as
TEST_VIEWER_EMAILS (in the stats module), TEST_VIEWER_EMAILS (in
visitors-table), and the email keys used to build TEST_AVATARS (in
visitor-avatar) by importing TEST_VIEWER_EMAILS and using it for lookups/keys so
all consumers share the single source of truth.
In `@pages/api/teams/`[teamId]/documents/[id]/stats.ts:
- Around line 19-27: Replace the duplicated test constants by removing
TEST_VIEWER_STATS and TEST_VIEWER_EMAILS from
pages/api/teams/[teamId]/documents/[id]/stats.ts and import the canonical
definitions from the central module (lib/test-views/generate-test-view.ts);
update any usage in this file to reference the imported TEST_VIEWER_STATS and
TEST_VIEWER_EMAILS identifiers so you no longer maintain two copies of the same
test data.
- Around line 247-284: The mixed-views weighted-average is wrong because
avgCompletionRate and totalDocumentDuration include test-view random Tinybird
data; fix by ensuring Tinybird queries exclude test view IDs (use
excludedViewIds) so totalDocumentDuration and avgCompletionRate are computed
only from realViews, then compute finalAvgCompletionRate and finalTotalDuration
by combining the real-view aggregates with the hardcoded test values from
TEST_VIEWER_STATS (compute testViewsCompletionSum and testViewsDurationSum as
already done and then weight by realViews.length + testViews.length), i.e.,
remove the subtraction of test durations from totalDocumentDuration and avoid
multiplying a mixed avg by realViews.length—use real-only aggregates plus test
sums to compute the final averages.
In `@pages/api/teams/`[teamId]/documents/[id]/test-view.ts:
- Around line 136-156: The DELETE path currently calls prisma.view.deleteMany
with only documentId and viewerEmail; first verify the document belongs to the
provided teamId by fetching it (e.g., use prisma.document.findUnique or
findFirst with { where: { id: documentId, teamId } }) and return 404/403 if not
found, then run prisma.view.deleteMany (still filtering by documentId and
TEST_VIEWERS emails) to perform the deletion; alternatively, include the teamId
in the deleteMany via the relation filter (e.g., where: { viewerEmail: { in:
testEmails }, document: { id: documentId, teamId } }) so that
prisma.view.deleteMany enforces the team ownership.
🧹 Nitpick comments (4)
pages/api/teams/[teamId]/documents/[id]/test-view.ts (2)
90-97:completionRatesarray is positionally coupled toTEST_VIEWERS.If
TEST_VIEWERSchanges order or gains a new entry, this mapping silently produces incorrect assignments. Consider embeddingcompletionPercentagedirectly in eachTEST_VIEWERSentry ingenerate-test-view.ts.
95-126: Partial state on failure: views are created sequentially without a transaction.If
generateTestPageViewsor the secondprisma.view.createfails, the first view record remains in the DB with no cleanup. Since the Tinybird events can't be rolled back anyway, this may be acceptable for test data, but worth noting. At minimum, wrapping the DB creates in aprisma.$transactionwould keep the database consistent.components/visitors/visitors-table.tsx (2)
190-193: Fragile: arbitrary 500ms delay beforerouter.reload().The 500ms timeout is presumably waiting for Tinybird event ingestion, but this is not guaranteed. On slower networks or higher Tinybird load, the data may not be available yet. Also,
router.reload()causes a full page refresh —handleDeleteTestViewusesmutateViews()instead, so the two handlers are inconsistent.Consider using
mutateViews()(plus mutating the stats SWR key) instead ofrouter.reload(), or at least add a comment explaining why a full reload is necessary.
130-138:localStoragepersistence is per-browser, not per-user or per-team.If another team member opens the same document, they won't see the
testViewsDeletedflag and may re-trigger test view creation. This may be the intended behavior for an onboarding-style feature, but worth confirming. Also, these keys are never cleaned up — they'll accumulate indefinitely in localStorage.
| // Custom avatars for test views | ||
| const TEST_AVATARS: Record<string, string> = { | ||
| "marc@papermark.com": | ||
| "https://img.papermarkassets.com/upload/file_5wiyjQw7immnVwTRS4kwkw-1579073467174.jpeg", | ||
| "iuliia@papermark.com": | ||
| "https://img.papermarkassets.com/upload/file_XwVCDHcBYjtAF5AVWsghBW-1761133587990.jpeg", | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Another duplication of test viewer emails — see centralization comment on generate-test-view.ts.
TEST_AVATARS repeats the same test viewer emails. Consider extending the centralized test viewer config with avatar URLs and importing here.
🤖 Prompt for AI Agents
In `@components/visitors/visitor-avatar.tsx` around lines 10 - 16, TEST_AVATARS
duplicates test viewer emails already defined in generate-test-view.ts; replace
this local map with an import from the centralized test viewer config (extend
that module to export avatar URLs if needed) and remove the hardcoded
TEST_AVATARS constant. Update components/visitors/visitor-avatar.tsx to import
the avatars mapping (named export) from generate-test-view.ts (or its new
exported config) and use that mapping where TEST_AVATARS was referenced
(preserve keys like "marc@papermark.com" and "iuliia@papermark.com"). Ensure any
consumers expect the same Record<string,string> shape and add/adjust exports in
generate-test-view.ts to include avatar URLs.
| const TEST_VIEWER_EMAILS = ["marc@papermark.com", "iuliia@papermark.com"]; | ||
|
|
||
| // Test viewer profiles with hardcoded display values | ||
| const TEST_VIEWER_PROFILES: Record< | ||
| string, | ||
| { | ||
| title: string; | ||
| linkedin: string; | ||
| twitter: string; | ||
| duration: number; // in milliseconds | ||
| completionRate: number; // percentage | ||
| } | ||
| > = { | ||
| "marc@papermark.com": { | ||
| title: "Co-Founder of Papermark", | ||
| linkedin: "https://www.linkedin.com/in/marcseitz/", | ||
| twitter: "https://x.com/mfts0", | ||
| duration: 342000, // 5:42 mins | ||
| completionRate: 100, | ||
| }, | ||
| "iuliia@papermark.com": { | ||
| title: "Co-Founder of Papermark", | ||
| linkedin: "https://www.linkedin.com/in/iuliia-shnai/", | ||
| twitter: "https://x.com/shnai0", | ||
| duration: 185000, // 3:05 mins | ||
| completionRate: 60, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same DRY concern: test viewer data duplicated here as well.
TEST_VIEWER_EMAILS and TEST_VIEWER_PROFILES duplicate data from lib/test-views/generate-test-view.ts and stats.ts. See the centralization comment on generate-test-view.ts.
🤖 Prompt for AI Agents
In `@components/visitors/visitors-table.tsx` around lines 73 - 100, The
TEST_VIEWER_EMAILS and TEST_VIEWER_PROFILES constants are duplicated here;
remove these local definitions and instead import and use the centralized test
viewer data (the exports used in lib/test-views/generate-test-view.ts or
stats.ts) so there's a single source of truth. Locate uses of TEST_VIEWER_EMAILS
and TEST_VIEWER_PROFILES in this file (visitors-table.tsx) and replace them with
the imported symbols, delete the local declarations, and ensure any type shapes
align with the imported types (adjust local typings or add a re-export/type
import from the central module if needed).
| const hasTestView = views?.viewsWithDuration?.some( | ||
| (view) => | ||
| view.viewerEmail && TEST_VIEWER_EMAILS.includes(view.viewerEmail), | ||
| ); | ||
|
|
||
| // Check if there are any real (non-test) views | ||
| const hasRealViews = views?.viewsWithDuration?.some( | ||
| (view) => | ||
| !view.viewerEmail || !TEST_VIEWER_EMAILS.includes(view.viewerEmail), | ||
| ); | ||
|
|
||
| // Only show test view buttons if there are no real views and test views haven't been deleted | ||
| const showTestViewButton = !hasRealViews && !testViewsDeleted; |
There was a problem hiding this comment.
UI gap: test views become unmanageable once a real view arrives.
showTestViewButton is false whenever hasRealViews is true. If a real view arrives while test views still exist in the database, the "Delete test views" button disappears and there's no UI path to remove the stale test data. The hardcoded overrides in stats.ts will continue to skew the aggregate metrics.
Consider either:
- Always showing the "Delete test views" button when
hasTestViewis true (regardless of real views), or - Auto-deleting test views server-side when the first real view is created.
🤖 Prompt for AI Agents
In `@components/visitors/visitors-table.tsx` around lines 141 - 153, The current
logic hides the "Delete test views" button when any real view exists because
showTestViewButton is computed from hasRealViews; change this so test-view
removal remains possible: update the showTestViewButton calculation (which
currently uses hasTestView, hasRealViews, and testViewsDeleted) to show the
delete button whenever hasTestView is true and testViewsDeleted is false (i.e.,
drop the !hasRealViews requirement), or alternatively implement server-side
auto-deletion of test views when the first real view is created by adding that
behavior to the views handling endpoint; adjust the UI to use the new
showTestViewButton and ensure views?.viewsWithDuration and TEST_VIEWER_EMAILS
logic remains unchanged.
| // Test viewers details (Iuliia first, then Marc) | ||
| const TEST_VIEWERS = [ | ||
| { | ||
| email: "iuliia@papermark.com", | ||
| name: "Iuliia Shnai", | ||
| }, | ||
| { | ||
| email: "marc@papermark.com", | ||
| name: "Marc Seitz", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how many files duplicate test viewer emails
echo "=== Files referencing test viewer emails ==="
rg -n "marc@papermark.com|iuliia@papermark.com" --type=ts --type=tsx -g '!node_modules' -g '!*.test.*'
echo ""
echo "=== Files with TEST_VIEWER definitions ==="
rg -n "TEST_VIEWER" --type=ts --type=tsx -g '!node_modules'Repository: mfts/papermark
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Search for test viewer email references across ts/tsx files
echo "=== Files referencing marc@papermark.com or iuliia@papermark.com ==="
rg "marc@papermark\.com|iuliia@papermark\.com" -g "*.ts" -g "*.tsx"
echo ""
echo "=== Files with TEST_VIEWER definitions ==="
rg "TEST_VIEWER" -g "*.ts" -g "*.tsx"Repository: mfts/papermark
Length of output: 5041
🏁 Script executed:
#!/bin/bash
echo "=== Contents of stats.ts TEST_VIEWER definitions ==="
rg -A 8 "const TEST_VIEWER_STATS" pages/api/teams/[teamId]/documents/[id]/stats.ts
echo ""
echo "=== Contents of visitors-table.tsx TEST_VIEWER definitions ==="
rg -A 6 "const TEST_VIEWER_EMAILS" components/visitors/visitors-table.tsx | head -15
echo ""
echo "=== visitor-avatar.tsx test viewer references ==="
rg -B 2 -A 2 "marc@papermark.com|iuliia@papermark.com" components/visitors/visitor-avatar.tsx | head -20Repository: mfts/papermark
Length of output: 1063
🏁 Script executed:
#!/bin/bash
echo "=== Check if stats.ts imports from generate-test-view.ts ==="
head -30 pages/api/teams/[teamId]/documents/[id]/stats.ts | grep -E "^import|TEST_VIEWER"
echo ""
echo "=== Check if visitors-table.tsx imports from generate-test-view.ts ==="
head -30 components/visitors/visitors-table.tsx | grep -E "^import|TEST_VIEWER"
echo ""
echo "=== Check if visitor-avatar.tsx imports from generate-test-view.ts ==="
head -30 components/visitors/visitor-avatar.tsx | grep -E "^import|TEST_"Repository: mfts/papermark
Length of output: 1455
Centralize test viewer email list to reduce duplication across four files.
TEST_VIEWERS is defined here, but the email list is separately hardcoded in:
pages/api/teams/[teamId]/documents/[id]/stats.ts→TEST_VIEWER_EMAILSderived fromTEST_VIEWER_STATScomponents/visitors/visitors-table.tsx→TEST_VIEWER_EMAILScomponents/visitors/visitor-avatar.tsx→TEST_AVATARSkeys
This violates DRY. Since test-view.ts already imports TEST_VIEWERS from this module, other files should do the same. Extract TEST_VIEWER_EMAILS as TEST_VIEWERS.map(v => v.email) or export it from here, then import it in stats.ts, visitors-table.tsx, and visitor-avatar.tsx instead of hardcoding the email list.
🤖 Prompt for AI Agents
In `@lib/test-views/generate-test-view.ts` around lines 4 - 14, TEST_VIEWERS is
duplicated as raw email lists elsewhere; add and export a derived constant like
TEST_VIEWER_EMAILS = TEST_VIEWERS.map(v => v.email) from this module and update
the other modules to import it instead of hardcoding emails. Specifically,
export TEST_VIEWER_EMAILS from this file alongside TEST_VIEWERS, then replace
the hardcoded lists/keys referenced as TEST_VIEWER_EMAILS (in the stats module),
TEST_VIEWER_EMAILS (in visitors-table), and the email keys used to build
TEST_AVATARS (in visitor-avatar) by importing TEST_VIEWER_EMAILS and using it
for lookups/keys so all consumers share the single source of truth.
| // Hardcoded test viewer stats | ||
| const TEST_VIEWER_STATS: Record< | ||
| string, | ||
| { duration: number; completionRate: number } | ||
| > = { | ||
| "marc@papermark.com": { duration: 342000, completionRate: 100 }, // 5:42 mins | ||
| "iuliia@papermark.com": { duration: 185000, completionRate: 60 }, // 3:05 mins | ||
| }; | ||
| const TEST_VIEWER_EMAILS = Object.keys(TEST_VIEWER_STATS); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate of test viewer constants — see comment on lib/test-views/generate-test-view.ts.
TEST_VIEWER_STATS and TEST_VIEWER_EMAILS duplicate data already defined in lib/test-views/generate-test-view.ts. Import from the central module instead.
🤖 Prompt for AI Agents
In `@pages/api/teams/`[teamId]/documents/[id]/stats.ts around lines 19 - 27,
Replace the duplicated test constants by removing TEST_VIEWER_STATS and
TEST_VIEWER_EMAILS from pages/api/teams/[teamId]/documents/[id]/stats.ts and
import the canonical definitions from the central module
(lib/test-views/generate-test-view.ts); update any usage in this file to
reference the imported TEST_VIEWER_STATS and TEST_VIEWER_EMAILS identifiers so
you no longer maintain two copies of the same test data.
| // Calculate final stats using hardcoded values for test views | ||
| let finalAvgCompletionRate = avgCompletionRate; | ||
| let finalTotalDuration = | ||
| filteredViews.length > 0 | ||
| ? (totalDocumentDuration.data[0]?.sum_duration ?? 0) / | ||
| filteredViews.length | ||
| : 0; | ||
|
|
||
| if (testViews.length > 0) { | ||
| // Calculate hardcoded totals for test views | ||
| const testViewsCompletionSum = testViews.reduce((sum, view) => { | ||
| const stats = TEST_VIEWER_STATS[view.viewerEmail || ""]; | ||
| return sum + (stats?.completionRate || 0); | ||
| }, 0); | ||
| const testViewsDurationSum = testViews.reduce((sum, view) => { | ||
| const stats = TEST_VIEWER_STATS[view.viewerEmail || ""]; | ||
| return sum + (stats?.duration || 0); | ||
| }, 0); | ||
|
|
||
| if (realViews.length === 0) { | ||
| // Only test views - use hardcoded averages | ||
| finalAvgCompletionRate = testViewsCompletionSum / testViews.length; | ||
| finalTotalDuration = testViewsDurationSum / testViews.length; | ||
| } else { | ||
| // Mix of test and real views - calculate weighted average | ||
| const realViewsCompletionSum = avgCompletionRate * realViews.length; | ||
| const realViewsDurationSum = | ||
| (totalDocumentDuration.data[0]?.sum_duration ?? 0) - | ||
| testViewsDurationSum; | ||
|
|
||
| finalAvgCompletionRate = | ||
| (testViewsCompletionSum + realViewsCompletionSum) / | ||
| filteredViews.length; | ||
| finalTotalDuration = | ||
| (testViewsDurationSum + realViewsDurationSum) / | ||
| filteredViews.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect weighted-average computation when mixing real and test views.
Two issues in the "mixed" branch (Lines 270-283):
-
Line 272:
avgCompletionRatewas computed from allfilteredViews(including test views) at Lines 163-234. Multiplying it byrealViews.lengthto recover the "real views completion sum" is mathematically wrong — it double-counts test-view influence. -
Line 274:
totalDocumentDuration.data[0]?.sum_durationcomes from Tinybird, which contains the random durations emitted bygenerateTestPageViews. Subtracting the hardcodedtestViewsDurationSum(342000 + 185000 = 527000) from a random Tinybird sum will produce an incorrect (possibly negative) remainder.
The root cause is that the Tinybird analytics data for test views uses random values while the display layer uses hardcoded overrides — you can't reconcile them by subtraction.
Possible fix: Exclude test-view IDs from the Tinybird queries (excludedViewIds) and compute the final average using only real-view Tinybird data plus the hardcoded test-view values.
Sketch of a corrected approach
- // Separate test views from real views
- const testViews = filteredViews.filter(
- (view) =>
- view.viewerEmail && TEST_VIEWER_EMAILS.includes(view.viewerEmail),
- );
- const realViews = filteredViews.filter(
- (view) =>
- !view.viewerEmail || !TEST_VIEWER_EMAILS.includes(view.viewerEmail),
- );
-
- // Calculate final stats using hardcoded values for test views
- let finalAvgCompletionRate = avgCompletionRate;
- let finalTotalDuration =
- filteredViews.length > 0
- ? (totalDocumentDuration.data[0]?.sum_duration ?? 0) /
- filteredViews.length
- : 0;
-
- if (testViews.length > 0) {
- // Calculate hardcoded totals for test views
- const testViewsCompletionSum = testViews.reduce((sum, view) => {
- const stats = TEST_VIEWER_STATS[view.viewerEmail || ""];
- return sum + (stats?.completionRate || 0);
- }, 0);
- const testViewsDurationSum = testViews.reduce((sum, view) => {
- const stats = TEST_VIEWER_STATS[view.viewerEmail || ""];
- return sum + (stats?.duration || 0);
- }, 0);
-
- if (realViews.length === 0) {
- // Only test views - use hardcoded averages
- finalAvgCompletionRate = testViewsCompletionSum / testViews.length;
- finalTotalDuration = testViewsDurationSum / testViews.length;
- } else {
- // Mix of test and real views - calculate weighted average
- const realViewsCompletionSum = avgCompletionRate * realViews.length;
- const realViewsDurationSum =
- (totalDocumentDuration.data[0]?.sum_duration ?? 0) -
- testViewsDurationSum;
-
- finalAvgCompletionRate =
- (testViewsCompletionSum + realViewsCompletionSum) /
- filteredViews.length;
- finalTotalDuration =
- (testViewsDurationSum + realViewsDurationSum) /
- filteredViews.length;
- }
- }
+ // Separate test views from real views
+ const testViews = filteredViews.filter(
+ (view) =>
+ view.viewerEmail && TEST_VIEWER_EMAILS.includes(view.viewerEmail),
+ );
+ const realViews = filteredViews.filter(
+ (view) =>
+ !view.viewerEmail || !TEST_VIEWER_EMAILS.includes(view.viewerEmail),
+ );
+
+ // Exclude test-view IDs from Tinybird queries to get clean real-view data
+ const allExcludedForReal = [...allExcludedViews, ...testViews];
+ const [realDuration, realTotalDocDuration] = testViews.length > 0
+ ? await Promise.all([
+ getTotalAvgPageDuration({
+ documentId: docId,
+ excludedLinkIds: "",
+ excludedViewIds: allExcludedForReal.map((v) => v.id).join(","),
+ since: 0,
+ }),
+ getTotalDocumentDuration({
+ documentId: docId,
+ excludedLinkIds: "",
+ excludedViewIds: allExcludedForReal.map((v) => v.id).join(","),
+ since: 0,
+ }),
+ ])
+ : [duration, totalDocumentDuration];
+
+ // Compute hardcoded sums for test views
+ const testViewsCompletionSum = testViews.reduce((sum, view) => {
+ const stats = TEST_VIEWER_STATS[view.viewerEmail || ""];
+ return sum + (stats?.completionRate || 0);
+ }, 0);
+ const testViewsDurationSum = testViews.reduce((sum, view) => {
+ const stats = TEST_VIEWER_STATS[view.viewerEmail || ""];
+ return sum + (stats?.duration || 0);
+ }, 0);
+
+ // Compute real-view completion using only real views (re-query if needed)
+ // ... (recompute avgCompletionRate excluding test views) ...
+
+ // Weighted average
+ let finalAvgCompletionRate = avgCompletionRate; // placeholder
+ let finalTotalDuration = 0;
+ if (filteredViews.length > 0) {
+ const realDurSum = realTotalDocDuration.data[0]?.sum_duration ?? 0;
+ finalTotalDuration =
+ (testViewsDurationSum + realDurSum) / filteredViews.length;
+ // Similar for completion ...
+ }🤖 Prompt for AI Agents
In `@pages/api/teams/`[teamId]/documents/[id]/stats.ts around lines 247 - 284, The
mixed-views weighted-average is wrong because avgCompletionRate and
totalDocumentDuration include test-view random Tinybird data; fix by ensuring
Tinybird queries exclude test view IDs (use excludedViewIds) so
totalDocumentDuration and avgCompletionRate are computed only from realViews,
then compute finalAvgCompletionRate and finalTotalDuration by combining the
real-view aggregates with the hardcoded test values from TEST_VIEWER_STATS
(compute testViewsCompletionSum and testViewsDurationSum as already done and
then weight by realViews.length + testViews.length), i.e., remove the
subtraction of test durations from totalDocumentDuration and avoid multiplying a
mixed avg by realViews.length—use real-only aggregates plus test sums to compute
the final averages.
| } else if (req.method === "DELETE") { | ||
| // Delete test views for this document (all test viewers) | ||
| try { | ||
| const testEmails = TEST_VIEWERS.map((v) => v.email); | ||
| const result = await prisma.view.deleteMany({ | ||
| where: { | ||
| documentId, | ||
| viewerEmail: { | ||
| in: testEmails, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| return res.status(200).json({ | ||
| message: "Test views deleted successfully", | ||
| deletedCount: result.count, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Error deleting test views:", error); | ||
| errorhandler(error, res); | ||
| } |
There was a problem hiding this comment.
Authorization gap: DELETE doesn't verify the document belongs to the team.
The POST path correctly filters by teamId when fetching the document (Line 54), but the DELETE path directly calls deleteMany using only documentId and viewerEmail without confirming the document belongs to teamId. A user who belongs to team A could craft a request with team A's teamId but another team's documentId to delete test views from documents they don't own.
🔒 Proposed fix: add teamId check to DELETE
} else if (req.method === "DELETE") {
// Delete test views for this document (all test viewers)
try {
+ // Verify document belongs to team
+ const document = await prisma.document.findUnique({
+ where: { id: documentId, teamId },
+ select: { id: true },
+ });
+
+ if (!document) {
+ return res.status(404).json({ error: "Document not found" });
+ }
+
const testEmails = TEST_VIEWERS.map((v) => v.email);
const result = await prisma.view.deleteMany({
where: {
documentId,
viewerEmail: {
in: testEmails,
},
},
});🤖 Prompt for AI Agents
In `@pages/api/teams/`[teamId]/documents/[id]/test-view.ts around lines 136 - 156,
The DELETE path currently calls prisma.view.deleteMany with only documentId and
viewerEmail; first verify the document belongs to the provided teamId by
fetching it (e.g., use prisma.document.findUnique or findFirst with { where: {
id: documentId, teamId } }) and return 404/403 if not found, then run
prisma.view.deleteMany (still filtering by documentId and TEST_VIEWERS emails)
to perform the deletion; alternatively, include the teamId in the deleteMany via
the relation filter (e.g., where: { viewerEmail: { in: testEmails }, document: {
id: documentId, teamId } }) so that prisma.view.deleteMany enforces the team
ownership.
1 participant
Summary by CodeRabbit
New Features
UI Updates