Conversation
- [x] allow chat messages to be stored in markdown - [x] func to convert tiptap JSON -> markdown - [x] convert at mentions to @ + username - [ ] script to migrate private messages - [ ] script to migrate comments - [x] component to display markdown as rich text - [ ] allow editor to ingest markdown
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMigrates private chat message content from Tiptap JSON to Markdown strings across frontend, backend, schema, types, and DB; adds conversion utilities and rendering components; updates validations, removes Tiptap runtime dependencies, and provides a migration script and SQL migration to change the DB column type. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Editor as "Editor (Tiptap)"
participant Frontend as Frontend
participant API as "Backend API"
participant DB as Database
User->>Editor: Compose message
Editor->>Frontend: editor.getJSON()
Frontend->>Frontend: tiptapToMarkdown(editor.getJSON())
Frontend->>API: POST /create-private-user-message (content: Markdown)
API->>DB: Insert message.content as text
DB-->>API: OK
API-->>Frontend: Response
Frontend-->>User: Display message (rendered via Richify)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
web/components/widgets/editor.tsx (1)
82-89: 💡 Verification agent🧩 Analysis chain
Public API no longer accepts
'lg', but callers can still send it
useTextEditornow restrictssizeto'sm' | 'md', however other helpers (proseClass,Richify) still accept / generate'lg'. Down‑stream components that previously passed'lg'will now fail type‑check. If'lg'is truly deprecated, remove it fromproseClassand theRichifyfallback; otherwise keep it in the public type:-export function useTextEditor(props: { … size?: 'sm' | 'md' … }) +export function useTextEditor(props: { … size?: 'sm' | 'md' | 'lg' … })Please audit all call‑sites or complete the deprecation by deleting the
'lg'branch inproseClassandRichify.
🏁 Script executed:
#!/bin/bash set -e echo "1. useTextEditor signature (with surrounding context):" rg -n "export function useTextEditor" -C3 web/components/widgets/editor.tsx || echo "No match found" echo echo "2. proseClass definition (with fallback branches):" rg -n "proseClass" -C3 echo echo "3. Richify definition (with fallback branches):" rg -n "Richify" -C3 echo echo "4. Call‑sites passing size 'lg':" rg -n "size *= *['\"]lg['\"]" echo echo "5. All literal occurrences of 'lg':" rg -n "'lg'"Length of output: 7028
Align
sizeunion in useTextEditor, Richify, and proseClassThe hook and component APIs restrict
sizeto'sm' | 'md', but
proseClassstill accepts'lg'(editor.tsx:65)Richifyfalls back to'lg'internally (richify.tsx:42)- Several consumers pass
size="lg"(e.g. user-hovercard.tsx:63, loading-indicator.tsx:21)This mismatch will break callers. You can either:
• Re‑expose
'lg'in the public APIs:// web/components/widgets/editor.tsx -export function useTextEditor(props: { … size?: 'sm' | 'md'; … }) +export function useTextEditor(props: { … size?: 'sm' | 'md' | 'lg'; … }) // web/components/widgets/richify.tsx -export function Richify(props: { … size?: 'sm' | 'md'; … }) +export function Richify(props: { … size?: 'sm' | 'md' | 'lg'; … })• Or remove
'lg'support entirely by:
– Deleting the'lg'branch inproseClassand the fallback inRichify
– Updating all<… size="lg" />call‑sites accordinglyPlease pick one approach and update the affected declarations and call‑sites.
backend/api/src/junk-drawer/private-messages.ts (1)
25-36: 🛠️ Refactor suggestion
insertPrivateMessagestill expectsJson– mismatch with new format
contentis now a Markdown string, but the parameter type is stillJson. This compiles (becausestring⊂Json), yet the name is misleading and invites future misuse.-export const insertPrivateMessage = async ( - content: Json, +export const insertPrivateMessage = async ( + content: string,Also update the SQL parameter type comment so future migrations don’t revert it to
jsonb.
🧹 Nitpick comments (7)
todos.md (1)
29-32: Typo in checklist item: "mesages" should be "messages"Line 32 contains a typo in the word "mesages" (missing an 's'). Consider correcting it to "messages" for consistency and accuracy.
- - [x] migrate mesages + - [x] migrate messagesbackend/scripts/messages-to-markdown.ts (1)
46-59: Consider using the existing bulkUpdate helper function.You're implementing a custom bulkUpdate function when there's an existing utility in the codebase. According to the relevant code snippets, there's a
bulkUpdatehelper function inbackend/shared/src/supabase/utils.tsthat could potentially be used instead.However, I note your implementation may be more efficient for this specific use case as it constructs a single SQL statement with VALUES. If performance is critical, your implementation is appropriate, but consider adding a comment explaining why you're not using the existing helper.
const bulkUpdate = ( tx: SupabaseTransaction, updates: { id: number; content: string }[] ) => { + // Custom implementation for better performance than the standard helper + // Uses a single SQL statement with VALUES instead of multiple updates const values = updates .map((u) => pgp.as.format('($(id), $(content))', u)) .join(',') return tx.none( `update private_user_messages p set content = v.content from (values ${values}) as v(id, content) where p.id = v.id` ) }common/src/util/tiptap-to-markdown.ts (2)
67-75: Ordered‑list formatting breaks for 100+ items & loses “start” attr
padEnd(4, ' ')assumes at most two‑digit indices. For 100+ items the spacing is off and the list is no longer aligned. Also, ifstart≠ 1 the rendered Markdown still starts at1.because we don’t emit thestart="n"attribute.- content.map( - (child, i) => `${start + i}.`.padEnd(4, ' ') + recurse(child) - ) + content.map((child, i) => { + const num = start + i + return `${num}. `.padEnd(String(num).length + 2, ' ') + recurse(child) + })Optionally prepend
<!--start:${start}-->or similar comment so the original start index survives round‑trip conversions.
124-135:joinBlockChildrenis O(n²) in the worst caseBoth
while (md.endsWith('\n'))and string concatenation inside the loop scan the accumulated string repeatedly, which becomes quadratic for long docs. Consider tracking trailing / leading newline counts numerically and append in one go, or useArray.pushandjoin.This will noticeably speed up conversion when pasting large documents or code blocks.
web/components/widgets/editor.tsx (1)
250-259:Richifyalready appliesproseClass; double‑styling is possible
ContentforwardsclassNamedown toRichify, which then merges it with its own computed prose classes. WhenContentitself is also rendered inside aprosecontainer the styles will nest, inflating margins and font sizes.Consider letting
Contentbe a thin wrapper:- return typeof content === 'string' ? ( - <Richify text={content} size={size} className={className} /> - ) : ( - <RichContent {...(props as any)} /> - ) + if (typeof content === 'string') + return <Richify text={content} size={size} className={className} /> + return <RichContent {...props} className={className} />…and document expected usage so the caller provides the outer container.
🧰 Tools
🪛 Biome (1.9.4)
[error] 250-250: Shouldn't redeclare 'Content'. Consider to delete it or rename it.
'Content' is defined here:
(lint/suspicious/noRedeclare)
backend/api/src/junk-drawer/private-messages.ts (1)
66-90: Missing length / size validation for user‑supplied contentThe old JSON pathway rejected oversized payloads; the Markdown path does not. Extremely long strings can bloat the
textcolumn and degrade client performance when rendered.Add a simple guard, e.g.:
const MAX_LEN = 10_000 // tweak if (content.length > MAX_LEN) throw new APIError(400, 'Message too long')web/components/widgets/richify.tsx (1)
25-36: Shadowing proptextobscures intent
const text = props.text …masks the outertextidentifier and complicates debugging. Just rename the derived variable (e.g.processedText) to keep the original value available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
backend/api/src/create-private-user-message.ts(1 hunks)backend/api/src/junk-drawer/private-messages.ts(3 hunks)backend/scripts/messages-to-markdown.ts(1 hunks)backend/supabase/migrations/20250415011659_dm_to_markdown.sql(1 hunks)common/src/api/schema.ts(2 hunks)common/src/chat-message.ts(1 hunks)common/src/util/tiptap-to-markdown.ts(1 hunks)todos.md(1 hunks)web/components/editor/image.tsx(1 hunks)web/components/messaging/send-message-button.tsx(2 hunks)web/components/widgets/editor.tsx(5 hunks)web/components/widgets/linkify.tsx(1 hunks)web/components/widgets/richify.tsx(1 hunks)web/lib/supabase/chat-messages.ts(2 hunks)web/package.json(1 hunks)web/pages/messages/[channelId].tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
web/pages/messages/[channelId].tsx (1)
common/src/util/tiptap-to-markdown.ts (1)
tiptapToMarkdown(3-6)
web/lib/supabase/chat-messages.ts (1)
common/src/util/parse.ts (1)
parseJsonContentToText(77-79)
web/components/messaging/send-message-button.tsx (1)
common/src/util/tiptap-to-markdown.ts (1)
tiptapToMarkdown(3-6)
backend/scripts/messages-to-markdown.ts (5)
backend/scripts/run-script.ts (1)
runScript(10-22)common/src/supabase/utils.ts (1)
Row(18-22)common/src/util/tiptap-to-markdown.ts (1)
tiptapToMarkdown(3-6)backend/shared/src/supabase/utils.ts (1)
bulkUpdate(56-74)backend/shared/src/supabase/init.ts (2)
SupabaseTransaction(37-37)pgp(13-31)
common/src/api/schema.ts (1)
common/src/comment.ts (1)
MAX_COMMENT_LENGTH(3-3)
web/components/widgets/editor.tsx (1)
web/components/widgets/richify.tsx (1)
Richify(13-73)
🔇 Additional comments (16)
web/lib/supabase/chat-messages.ts (2)
6-6: LGTM! Import replaced to support both JSON and string content formatsThe replacement of
richTextToStringwithparseJsonContentToTextappropriately adapts the code to handle both JSONContent and string formats during the migration from Tiptap JSON to Markdown.
131-131: LGTM! Updated function call matches import changeThe function call has been correctly updated to use
parseJsonContentToText, which will handle both legacy JSONContent and new Markdown string formats appropriately.web/pages/messages/[channelId].tsx (2)
47-47: LGTM! Import added for Markdown conversionProperly added the import for the
tiptapToMarkdownutility function that will be used to convert editor content to Markdown format.
169-169: Conversion of editor content to Markdown looks goodThe message submission logic now correctly converts the Tiptap editor JSON content to Markdown using the
tiptapToMarkdownutility before sending it to the backend API, aligning with the migration to store message content as Markdown strings.backend/api/src/create-private-user-message.ts (2)
10-10: LGTM! Removed JSON length validationThe validation for maximum JSON string length has been correctly removed since the content is now a Markdown string rather than a JSON object.
15-15:Details
✅ Verification successful
Consider adding string length validation
While JSON validation has been removed, there should be a validation for the maximum length of the Markdown string to prevent excessively large messages.
+ if (content.length > MAX_COMMENT_LENGTH) { + throw new APIError(400, 'Message too long') + }
🌐 Web query:
What are best practices for limiting message length in chat applications?💡 Result:
Best Practices for Limiting Message Length in Chat Applications
Keep Messages Short and Concise
- Short messages are core to chat as a medium, enhancing readability, engagement, and user response rates. Studies show messages under 100 characters can achieve 2-5x higher response rates compared to longer messages[1][2][6].
- Aim for tweet-sized messages (typically around 140 characters or less), which fit well within common chat UIs and avoid overwhelming users with large text blocks[2][6].
Set Character Limits
- Most chat and SMS platforms set a hard cap: for SMS, this is traditionally 160 characters[4][7][9]. Exceeding this can result in messages being split (and potentially additional costs).
- For in-app or online chat, it’s common to apply a 500–1,000 character limit for general user messages, though the ideal length is often much shorter to avoid excessive scrolling or clutter.
UI and UX Considerations
- Design chat interfaces to visually discourage lengthy posts by restricting message box size or providing real-time character counters[2][6][7].
- Break longer content into multiple short messages or conversational turns, making the dialogue feel natural and easy to follow[2][6].
Technical Tips
- Use templates and predefined phrases to keep messages standard and within length limits[7].
- Employ URL shorteners to avoid long links consuming too many characters[4][7][9].
- Avoid unnecessary emojis or special characters, as these can drastically reduce available character space due to encoding constraints (Unicode messages may drop the cap to 70 characters)[4][7][9].
Content Quality
- Get to the point quickly—remove filler words and extraneous context[4][9].
- Use clear language; abbreviations are acceptable but only when the audience will easily understand them[9].
- For bots or automated systems, keep initial and response messages especially brief to maintain engagement and prevent user drop-off[2][6].
Monitor and Enforce Limits
- Integrate real-time counters or warnings in the chat input field to help users stay within the allowed message length[4][7][9].
- For advanced chat applications, consider summarizing or truncating longer messages, particularly when managing persistent chat history[8].
Summary Table: Message Length Guidelines
Platform/Context Hard Limit Recommended Length Notes SMS 160 characters (GSM) ≤100 characters Unicode: 70 char max[4][7][9] Chatbots/Apps Varies (often <500–1000) ~140 characters Twitter rule; split long messages[2][6] Social Media (e.g. Twitter, Facebook) 280 (Twitter), 80–100 (FB optimal) 71–100 characters (higher engagement) [3] Key Takeaways
- Limit messages to around 100–140 characters for optimal engagement and readability[1][2][6].
- Implement technical constraints and UI nudges to enforce and encourage brevity[4][7][9].
- Prioritize clear, actionable, and concise communication to enhance the user experience and reduce chat fatigue[2][5][6].
Adopting these best practices will help keep conversations manageable, engaging, and user-friendly in any chat application.
Citations:
- 1: https://textus.com/ideal-length-of-text-messages/
- 2: https://uxdesign.cc/chatbot-building-best-practices-why-message-length-matters-e951bed1b550
- 3: https://kudosity.com/resources/articles/ideal-character-lengths-for-sms-email-and-social-media
- 4: https://www.infobip.com/blog/sms-character-limit
- 5: https://support.anthropic.com/en/articles/9797557-usage-limit-best-practices
- 6: https://hellotars.com/blog/chatbot-message-length
- 7: https://textdrip.com/blog/sms-character-limit
- 8: https://community.openai.com/t/how-long-is-it-optimal-to-keep-the-message-for-the-chat-history/173682
- 9: https://emotive.io/blog/sms-character-limit
- 10: https://support.google.com/chat/thread/184647551/how-we-can-increase-message-length-in-google-chat-application
🏁 Script executed:
#!/bin/bash set -e echo "Displaying first 200 lines of backend/api/src/create-private-user-message.ts for context:" sed -n '1,200p' backend/api/src/create-private-user-message.tsLength of output: 1002
🏁 Script executed:
#!/bin/bash set -e echo "Searching for constants related to message length..." rg -i "MAX_.+LENGTH" -n . echo "Locating 'private-messages' implementation..." fd "private-messages*" -t fLength of output: 2868
🏁 Script executed:
#!/bin/bash set -e echo "Locating createPrivateUserMessageMain definition and usage:" grep -R "createPrivateUserMessageMain" -n backend/api/src/junk-drawer/private-messages.ts echo echo "Displaying its implementation:" sed -n '1,200p' backend/api/src/junk-drawer/private-messages.tsLength of output: 5183
🏁 Script executed:
#!/bin/bash set -e echo "Searching for 'create-private-user-message' in common API schema:" rg -n "create-private-user-message" -n common/src/api/schema.ts || true echo echo "Searching nearby lines for content field:" rg -n "content:" -C3 -n common/src/api/schema.tsLength of output: 846
Input length is already enforced by the shared schema
Thecreate-private-user-messageendpoint uses a Zod schema that applies
z.string().max(MAX_COMMENT_LENGTH)(see common/src/api/schema.ts:408),
so manual length checks increate-private-user-message.tsaren’t needed.web/components/widgets/linkify.tsx (1)
6-9: Appropriate deprecation notice added.The JSDoc comment clearly marks this component as deprecated and directs developers to use the new
Richifycomponent instead, which aligns with the migration to Markdown-based content rendering.common/src/chat-message.ts (1)
8-8: Expanded type definition accommodates migration strategy.The change to allow both
JSONContentandstringfor thecontentproperty enables a smooth transition from Tiptap JSON to Markdown strings. This union type supports backward compatibility with existing data while allowing new Markdown format.common/src/api/schema.ts (2)
19-19: Import added for message length validation.The MAX_COMMENT_LENGTH import brings in the constant needed for validation in the updated schema.
408-408: API schema updated to validate string content.The validation for message content has been correctly updated to expect a string with maximum length constraint, replacing the previous JSONContent validation. This change is consistent with the migration from Tiptap JSON to Markdown format.
web/components/messaging/send-message-button.tsx (2)
19-19: Added import for Markdown conversion utility.The import brings in the utility needed to convert Tiptap JSON to Markdown strings.
73-73: Content converted to Markdown before API submission.The implementation now correctly converts the editor's JSON content to Markdown format before sending it to the API. This change maintains compatibility with the updated backend expectations.
web/components/editor/image.tsx (1)
19-24: Looks good: Image component export and size enhancement.The component changes make
ExpandingImageexportable and enhance its size prop to support both 'sm' and 'md' values, which aligns with the existing implementation logic at lines 37-40. This enables reuse in the new Markdown rendering components that are part of the migration from Tiptap JSON to Markdown.web/package.json (1)
59-65: Dependencies added to support Markdown rendering.The added Markdown-related packages are appropriate for the migration from Tiptap JSON to Markdown strings:
react-markdownfor rendering Markdown in React componentsrehype-minify-whitespacefor HTML whitespace handlingremark-breaksfor preserving line breaks (aligns with your concern about newline handling in the PR description)remark-gfmfor GitHub Flavored Markdown featuresremark-mentionsfor handling @mentions syntaxbackend/supabase/migrations/20250415011659_dm_to_markdown.sql (2)
1-13: Verify this assertion logic for production deployment.The assertion will fail if no Tiptap messages are found, which might be appropriate for development but could cause issues in production environments where the data has already been migrated or if a fresh deployment occurs.
Consider whether this assertion should be conditionally applied or if there should be a more graceful fallback for production environments.
14-23: Data migration strategy is appropriate.The schema migration drops the JSONB column and adds a text column with a temporary default value, then removes the default constraint. This approach works well with the separate data migration script mentioned in the comment.
| runScript(async ({ pg }) => { | ||
| const messages = await pg.manyOrNone<Row<'private_user_messages'>>( | ||
| `select * from private_user_messages` | ||
| ) | ||
|
|
||
| const updates = messages.map((m) => ({ | ||
| id: m.id, | ||
| content: tiptapToMarkdown(m.content as any), | ||
| })) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for content conversion.
The script uses as any to bypass type checking when converting content from JSON to Markdown. This could lead to runtime errors if the content structure doesn't match expectations.
- const updates = messages.map((m) => ({
- id: m.id,
- content: tiptapToMarkdown(m.content as any),
- }))
+ const updates = messages.map((m) => ({
+ id: m.id,
+ content: typeof m.content === 'object' && m.content !== null
+ ? tiptapToMarkdown(m.content)
+ : String(m.content),
+ }))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runScript(async ({ pg }) => { | |
| const messages = await pg.manyOrNone<Row<'private_user_messages'>>( | |
| `select * from private_user_messages` | |
| ) | |
| const updates = messages.map((m) => ({ | |
| id: m.id, | |
| content: tiptapToMarkdown(m.content as any), | |
| })) | |
| runScript(async ({ pg }) => { | |
| const messages = await pg.manyOrNone<Row<'private_user_messages'>>( | |
| `select * from private_user_messages` | |
| ) | |
| const updates = messages.map((m) => ({ | |
| id: m.id, | |
| content: typeof m.content === 'object' && m.content !== null | |
| ? tiptapToMarkdown(m.content) | |
| : String(m.content), | |
| })) | |
| // … | |
| }) |
There was a problem hiding this comment.
@sipec You're right, I apologize for my incorrect suggestion. I misunderstood the context of your migration script. The as any type assertion is appropriate here since this is a one-time migration script where all content is expected to be in Tiptap JSON format. My suggested type checking would have added unnecessary complexity.
| await pg.tx(async (tx) => { | ||
| await tx.none( | ||
| `alter table private_user_messages | ||
| drop column content, | ||
| add column content text not null default ''` | ||
| ) | ||
|
|
||
| for (const c of chunk(updates, 1000)) { | ||
| console.log('updating chunk') | ||
| await bulkUpdate(tx, c) | ||
| } | ||
|
|
||
| await tx.none( | ||
| `alter table private_user_messages | ||
| alter column content drop default` | ||
| ) | ||
|
|
||
| const values = await tx.many( | ||
| `select * from private_user_messages | ||
| limit 10` | ||
| ) | ||
|
|
||
| console.log(values) | ||
|
|
||
| throw new Error(`don't commit`) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider making the transaction commit optional.
The script:
- Deliberately throws an error to prevent committing the transaction
- Lacks progress reporting for large message counts
- Has no error handling for the conversion process
Consider:
- Making the transaction commit configurable via a parameter
- Adding error handling and better progress reporting
- Providing a more descriptive error message
await pg.tx(async (tx) => {
await tx.none(
`alter table private_user_messages
drop column content,
add column content text not null default ''`
)
+ let processedCount = 0;
for (const c of chunk(updates, 1000)) {
- console.log('updating chunk')
+ console.log(`Updating chunk ${processedCount + 1} to ${processedCount + c.length} of ${updates.length}`)
await bulkUpdate(tx, c)
+ processedCount += c.length;
}
await tx.none(
`alter table private_user_messages
alter column content drop default`
)
const values = await tx.many(
`select * from private_user_messages
limit 10`
)
console.log(values)
- throw new Error(`don't commit`)
+ // Default to not committing for safety
+ if (process.env.COMMIT_CHANGES !== 'true') {
+ console.log('Dry run completed. Set COMMIT_CHANGES=true to commit changes.')
+ throw new Error('Migration completed but not committed (dry run)')
+ }
+ console.log('Migration completed and committed successfully.')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await pg.tx(async (tx) => { | |
| await tx.none( | |
| `alter table private_user_messages | |
| drop column content, | |
| add column content text not null default ''` | |
| ) | |
| for (const c of chunk(updates, 1000)) { | |
| console.log('updating chunk') | |
| await bulkUpdate(tx, c) | |
| } | |
| await tx.none( | |
| `alter table private_user_messages | |
| alter column content drop default` | |
| ) | |
| const values = await tx.many( | |
| `select * from private_user_messages | |
| limit 10` | |
| ) | |
| console.log(values) | |
| throw new Error(`don't commit`) | |
| }) | |
| await pg.tx(async (tx) => { | |
| await tx.none( | |
| `alter table private_user_messages | |
| drop column content, | |
| add column content text not null default ''` | |
| ) | |
| let processedCount = 0; | |
| for (const c of chunk(updates, 1000)) { | |
| console.log(`Updating chunk ${processedCount + 1} to ${processedCount + c.length} of ${updates.length}`) | |
| await bulkUpdate(tx, c) | |
| processedCount += c.length; | |
| } | |
| await tx.none( | |
| `alter table private_user_messages | |
| alter column content drop default` | |
| ) | |
| const values = await tx.many( | |
| `select * from private_user_messages | |
| limit 10` | |
| ) | |
| console.log(values) | |
| // Default to not committing for safety | |
| if (process.env.COMMIT_CHANGES !== 'true') { | |
| console.log('Dry run completed. Set COMMIT_CHANGES=true to commit changes.') | |
| throw new Error('Migration completed but not committed (dry run)') | |
| } | |
| console.log('Migration completed and committed successfully.') | |
| }) |
| // escape characters | ||
| if (!node.marks?.some((mark) => mark.type === 'code')) { | ||
| text = text | ||
| .replace(/\\/g, '\\\\') // \ -> \\ | ||
| .replace(/\*/g, '\\*') | ||
| .replace(/_/g, '\\_') | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Escape table is incomplete – some characters can still break Markdown
Only \, *, and _ are currently escaped. Un‑escaped instances of `, ~, [, ], <, and > may be interpreted as back‑ticks, strike‑through, link delimiters or raw HTML, respectively, altering the intended output or even creating XSS vectors once the Markdown is rendered as HTML.
- .replace(/\\/g, '\\\\') // \ -> \\
- .replace(/\*/g, '\\*')
- .replace(/_/g, '\\_')
+ .replace(/\\/g, '\\\\') // \
+ .replace(/`/g, '\\`') // code
+ .replace(/\*/g, '\\*') // bold / italic
+ .replace(/_/g, '\\_') // italic
+ .replace(/~/g, '\\~') // strike
+ .replace(/\[/g, '\\[') // link
+ .replace(/]/g, '\\]') // link
+ .replace(/</g, '<') // raw html
+ .replace(/>/g, '>') // raw html📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // escape characters | |
| if (!node.marks?.some((mark) => mark.type === 'code')) { | |
| text = text | |
| .replace(/\\/g, '\\\\') // \ -> \\ | |
| .replace(/\*/g, '\\*') | |
| .replace(/_/g, '\\_') | |
| } | |
| // escape characters | |
| if (!node.marks?.some((mark) => mark.type === 'code')) { | |
| text = text | |
| .replace(/\\/g, '\\\\') // \ | |
| .replace(/`/g, '\\`') // code | |
| .replace(/\*/g, '\\*') // bold / italic | |
| .replace(/_/g, '\\_') // italic | |
| .replace(/~/g, '\\~') // strike | |
| .replace(/\[/g, '\\[') // link | |
| .replace(/]/g, '\\]') // link | |
| .replace(/</g, '<') // raw html | |
| .replace(/>/g, '>') // raw html | |
| } |
| proseClass(size === 'sm' ? 'sm' : 'lg'), | ||
| 'empty:prose-p:-mt-px empty:prose-p:pt-px' // make empty paragraphs have height | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent size mapping – 'md' becomes 'lg'
proseClass(size === 'sm' ? 'sm' : 'lg') silently upgrades the default 'md' size to 'lg', changing font metrics compared with JSON rendering. Either:
- Pass
sizethrough unchanged, or - Rename prop to
large/compactto avoid semantic confusion.
- proseClass(size === 'sm' ? 'sm' : 'lg'),
+ proseClass(size),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| proseClass(size === 'sm' ? 'sm' : 'lg'), | |
| 'empty:prose-p:-mt-px empty:prose-p:pt-px' // make empty paragraphs have height | |
| )} | |
| proseClass(size), | |
| 'empty:prose-p:-mt-px empty:prose-p:pt-px' // make empty paragraphs have height | |
| )} |
| del: 's', | ||
| img: ({ src, alt, title }) => ( | ||
| <ExpandingImage size={size} src={src!} alt={alt} title={title} /> | ||
| ), | ||
| a: ({ href, children }) => ( | ||
| <Link href={href!} className={clsx(linkClass, 'text-primary-700')}> | ||
| {children} | ||
| </Link> | ||
| ), | ||
| }} |
There was a problem hiding this comment.
Potential XSS via href="javascript:…"
Links are rendered without validation and handed straight to <a>. Browsers will execute javascript: URIs when users click them.
Add a simple allow‑list (http/https/mailto) or use rehype-sanitize with a custom schema:
- a: ({ href, children }) => (
- <Link href={href!} …>
+ a: ({ href = '', children }) => {
+ const safe = /^(https?:|mailto:|\/)/i.test(href) ? href : '#'
+ return (
+ <Link href={safe} …>
{children}
- </Link>
- ),
+ </Link>
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| del: 's', | |
| img: ({ src, alt, title }) => ( | |
| <ExpandingImage size={size} src={src!} alt={alt} title={title} /> | |
| ), | |
| a: ({ href, children }) => ( | |
| <Link href={href!} className={clsx(linkClass, 'text-primary-700')}> | |
| {children} | |
| </Link> | |
| ), | |
| }} | |
| del: 's', | |
| img: ({ src, alt, title }) => ( | |
| <ExpandingImage size={size} src={src!} alt={alt} title={title} /> | |
| ), | |
| a: ({ href = '', children }) => { | |
| const safe = /^(https?:|mailto:|\/)/i.test(href) ? href : '#' | |
| return ( | |
| <Link href={safe} className={clsx(linkClass, 'text-primary-700')}> | |
| {children} | |
| </Link> | |
| ) | |
| }, | |
| }} |
|
@coderabbitai do you think this migration is better than doing nothing? |
|
Tip For best results, initiate chat on the files or code changes. @sipec Based on the PR context, I think this migration is better than doing nothing, despite the challenges you noted. Moving to Markdown provides clear benefits for interoperability (making it easier to read/write profiles via API and separate sites as you mentioned), human readability, and simpler validation. It also likely reduces storage requirements compared to the Tiptap JSON format. While you ran into implementation challenges with preserving newlines and extending the Markdown ecosystem (requiring what you called "hacky and ad-hoc patches"), these issues would likely need to be solved eventually as your federated ecosystem grows. The fact that you completed the migration suggests you saw enough value to justify the effort. The tradeoffs seem reasonable - some upfront implementation complexity for longer-term benefits of working with a more standard format. This should make your system more accessible to external consumers and simplify future development, even if the implementation wasn't as clean as you initially hoped. Did you encounter any unexpected issues after deploying this change that make you question the migration? |
sloppy and irrational |
|
@sipec quick two cents: I would oppose deviating from the Markdown standard. I use LaTeX and wikitext heavily and both of those also have a long-established pattern where one newline does nothing (except make the source a little easier to read) and two newlines adds an actual paragraph break. So I think manifold love should do the same. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/api/src/create-comment.ts`:
- Around line 49-53: The notification is being built with Markdown via
tiptapToMarkdown(content) in the call to createNewCommentOnLoverNotification
which ends up in Notification.sourceText and gets sent to emails; replace
tiptapToMarkdown(content) with a plain-text serialization of the tiptap content
(e.g., a tiptapToPlainText or plainText serializer / markdown-stripper) so
Notification.sourceText contains only plain text (no **bold**, # headings, or
other markup); update the call site in createNewCommentOnLoverNotification to
pass the plain-text string instead of tiptapToMarkdown(content).
In `@common/package.json`:
- Line 21: Restore "@tiptap/core" from devDependencies back into dependencies in
package.json because common/src/util/parse.ts performs a non-type import (import
{ JSONContent } from '@tiptap/core') which emits a runtime require; ensure
package.json lists "@tiptap/core" under "dependencies" so consumers can resolve
it at runtime and downstream packages do not get MODULE_NOT_FOUND.
In `@common/src/util/parse.ts`:
- Around line 28-29: parseJsonContentToText currently returns Markdown because
it forwards non-string input to tiptapToMarkdown, violating its "text
serializer" contract; change parseJsonContentToText to return plain text by
either calling a plain-text serializer (e.g., tiptapToPlainText) if available or
by post-processing tiptapToMarkdown output to strip Markdown formatting (remove
headings, emphasis, code fences, links, etc.) before returning, keeping the
function name and signature so callers get plain strings; if you prefer
returning Markdown instead, rename parseJsonContentToText to something like
parseJsonContentToMarkdown and update all callers to use the new name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad8ef6b9-e2f7-42d8-b697-3e5b74182554
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
backend/api/package.jsonbackend/api/src/create-comment.tsbackend/shared/package.jsoncommon/package.jsoncommon/src/util/parse.tsweb/components/editor/iframe-extension.tsweb/components/widgets/editor.tsx
💤 Files with no reviewable changes (1)
- backend/shared/package.json
| await createNewCommentOnLoverNotification( | ||
| onUser, | ||
| creator, | ||
| richTextToString(content), | ||
| tiptapToMarkdown(content), | ||
| comment.id, |
There was a problem hiding this comment.
Don’t send Markdown to notification copy.
This value becomes Notification.sourceText, and backend/email/emails/functions/helpers.tsx:1-30 forwards the same string unchanged into the email flow. Using tiptapToMarkdown(content) here will surface raw Markdown like **bold** and # heading in notifications/emails whenever a comment uses formatting. Use a plain-text serializer for notification text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/api/src/create-comment.ts` around lines 49 - 53, The notification is
being built with Markdown via tiptapToMarkdown(content) in the call to
createNewCommentOnLoverNotification which ends up in Notification.sourceText and
gets sent to emails; replace tiptapToMarkdown(content) with a plain-text
serialization of the tiptap content (e.g., a tiptapToPlainText or plainText
serializer / markdown-stripper) so Notification.sourceText contains only plain
text (no **bold**, # headings, or other markup); update the call site in
createNewCommentOnLoverNotification to pass the plain-text string instead of
tiptapToMarkdown(content).
| "zod": "3.21.4" | ||
| }, | ||
| "devDependencies": { | ||
| "@tiptap/core": "2.3.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `@tiptap/core` is imported as a value (non-type) in common.
# Expected: no matches if safe to keep `@tiptap/core` only in devDependencies.
rg -nP --type=ts "import\s+\{(?!\s*type\b)[^}]+\}\s+from\s+['\"]@tiptap/core['\"]" common/src
rg -nP --type=ts "import\s+[^;]*from\s+['\"]@tiptap/core['\"]" common/src -C2Repository: sipec/polylove
Length of output: 1478
🏁 Script executed:
cat -n common/src/util/parse.ts | head -30Repository: sipec/polylove
Length of output: 1394
@tiptap/core must remain in dependencies, not devDependencies.
common/src/util/parse.ts imports JSONContent as a non-type import (import { JSONContent } from '@tiptap/core' on line 1), which causes TypeScript to emit a runtime require() statement in the compiled JavaScript even though JSONContent is used only in type annotations (line 28). Moving this to devDependencies will cause downstream packages to fail with MODULE_NOT_FOUND at runtime.
Restore @tiptap/core to dependencies:
Fix
"dependencies": {
+ "@tiptap/core": "2.3.2",
"@supabase/supabase-js": "2.38.5",
"dayjs": "1.11.4",
"lodash": "4.17.21",
"string-similarity": "4.0.4",
"zod": "3.21.4"
},
"devDependencies": {
- "@tiptap/core": "2.3.2",
"@types/jest": "29.2.4",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@tiptap/core": "2.3.2", | |
| "dependencies": { | |
| "@tiptap/core": "2.3.2", | |
| "@supabase/supabase-js": "2.38.5", | |
| "dayjs": "1.11.4", | |
| "lodash": "4.17.21", | |
| "string-similarity": "4.0.4", | |
| "zod": "3.21.4" | |
| }, | |
| "devDependencies": { | |
| "@types/jest": "29.2.4", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/package.json` at line 21, Restore "@tiptap/core" from devDependencies
back into dependencies in package.json because common/src/util/parse.ts performs
a non-type import (import { JSONContent } from '@tiptap/core') which emits a
runtime require; ensure package.json lists "@tiptap/core" under "dependencies"
so consumers can resolve it at runtime and downstream packages do not get
MODULE_NOT_FOUND.
| export function parseJsonContentToText(content: JSONContent | string) { | ||
| return typeof content === 'string' ? content : richTextToString(content) | ||
| return typeof content === 'string' ? content : tiptapToMarkdown(content) |
There was a problem hiding this comment.
Keep parseJsonContentToText a text serializer.
common/src/util/tiptap-to-markdown.ts:3-6 returns Markdown, not plain text. That silently changes this helper’s contract, so any existing caller that still uses it for previews, search, or other plain-string output will now receive formatting tokens like ** and #. Either restore plain-text serialization here or rename the API and migrate callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/src/util/parse.ts` around lines 28 - 29, parseJsonContentToText
currently returns Markdown because it forwards non-string input to
tiptapToMarkdown, violating its "text serializer" contract; change
parseJsonContentToText to return plain text by either calling a plain-text
serializer (e.g., tiptapToPlainText) if available or by post-processing
tiptapToMarkdown output to strip Markdown formatting (remove headings, emphasis,
code fences, links, etc.) before returning, keeping the function name and
signature so callers get plain strings; if you prefer returning Markdown
instead, rename parseJsonContentToText to something like
parseJsonContentToMarkdown and update all callers to use the new name.
Linus-style screed
@duck-master the reason i want to have this specific change is that frankly, the wikitext and markdown standards are WRONG WHAT YOU SEE IS WHAT YOU GET well that's really not quite it either. people want an input box that lets them type everything they need obsidian.md is BARELY USABLE if you install ENOUGH PLUGINS and CSS to make it as nice as NOTION and then it still sucks but at least it is SOVREIGN you are clearly not a designer how are we supposed to import 50,000 art hoes if the website sucks??? yeah so pressing ENTER should make line go NEXT LINE like EVERY DOCUMENT EDITOR EVER EVERYONE IS WRONG ABOUT THIS Actual content
|
2 participants
I want you honest opinion on a high level if this is even a good idea. The intent is to make it easier down the line for humans and AIs to read and write profiles via api and separate sites, and move towards an interoperable federated ecosystem. My takes:
I've deviated from the spec in preserving single line breaks as
<br />.Additionally, if there's "extra" new lines, we preserve it and don't collapsed.
This is all needed for wysiwg. this respects user intent.
\chars into the text and then deleting them later just to preserve the newlines.All in all, the complexity is higher and benefit lower than what i thought. Hacky, ugly ad-hoc patches on top of a complex external library - that's what I was trying to avoid by moving to markdown. And yet.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores