Save PDF annotations by naming EmbedPDF's numeric subtype - #856
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (6)Use shadcn/ui for UI components (Radix-based, in `@/components/ui/`)📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Path aliases: `@/` maps to `packages/web/src/`📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use lucide-react for the icon library Use TanStack Query for server state management (`useQuery`, `useMutation`) Import Zustand stores directly from `@/stores/` instead of prop-drilling shared state Avoid `useMemo` or `useCallback` - let th...📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use import aliases from tsconfig.json Code comments should explain why something is being done or provide context, not repeat what the code is saying Use TODO(agent) pattern for incomplete work or flagging items for future attention, with b...📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
For UI icons, use `lucide-react` library or SVGs only (never emojis)📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
NEVER use emojis anywhere - not in code, comments, documentation, plan files, commit messages, or examples.📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughAnnotation types are normalized before sync mutations. Outcome chips now wrap long names and use content-sized layouts. ChangesAnnotation Type Normalization
Outcome Chip Layout
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to Annotation subtype normalization satisfies the sync mutation contract while preserving raw restoration data. No current merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation For Resolution In ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
EmbedPDF reports an annotation's subtype as a numeric enum (HIGHLIGHT = 9), and the viewer's `...annotation` spread put that number on the payload's `type`. The synced `annotations.type` column is a string, so every `annotation.add` and `annotation.update` failed the client's args check with InvalidArgs and never reached the Durable Object: no highlight has ever been saved in production, and reviewers just saw a "Change rejected" toast. Name the subtype before the mutation. `embedPdfData` keeps the numeric type, so importing a stored annotation back into the viewer is unchanged. Fixes #855 Claude-Session: https://claude.ai/code/session_01H77BMkZ9GhK8D98TG4cpTg
336762b to
ec2c90d
Compare
1 participant
Fixes #855.
The bug
EmbedPDF reports an annotation's subtype as a numeric enum (
PdfAnnotationSubtype.HIGHLIGHT = 9), and the viewer's...annotationspread lands that number on the payload'stype. The syncedannotations.typecolumn is a string, so the sync client's args check rejected everyannotation.addandannotation.updatewithInvalidArgsbefore they left the browser. Reviewers got a "Change rejected" toast and no highlight was ever stored - all four active production projects export zeroannotationsrows. Broken since the sync-engine migration inba6f6c8c.The fix
annotationTypeName()maps the subtype to its EmbedPDF name (highlight,ink, ...) at both call sites.embedPdfDatastill carries the raw numeric type, so importing a stored annotation back into the viewer is untouched - which matters, because the restore path rebuilds annotations fromembedPdfDataalone.Verified in the running app
Seeded a local project, uploaded a PDF, opened the appraisal, and drove the viewer through agent-browser:
type: "highlight"withembedPdfDataholding"type":9.annotation.addandannotation.updatelanded,updatedAtadvanced, stored astype: "ink".client.sync.mutation_rejectedlines across the whole run.Unit tests cover the mapping and assert the pre-fix numeric payload is exactly what the mutator's schema rejects.
pnpm typecheck,pnpm lintand the web suite (325 tests) pass.Noticed, not fixed
Dragging the ink annotation logs a React "two children with the same key" warning from the viewer's annotation layer. It predates this change and is unrelated to persistence.
https://claude.ai/code/session_01H77BMkZ9GhK8D98TG4cpTg
Summary by CodeRabbit