mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(collab): notes show and keep their website on the phone
The note website round-trips correctly through the API and the desktop grid renders it, but the mobile notes tab never carried the field: the form had no input, the payload had no key, and neither the card nor the read-only sheet showed a stored link. Since an editor only ever opens the form and a viewer only ever opens the sheet, no role on a phone could see or reach a website saved from a desktop. Editing a note on mobile did not wipe an existing link. The key was omitted rather than nulled, and the server's CASE WHEN guard kept the stored value, so this was a visibility gap rather than a destructive one. The form now always sends the field, null included, because the server only clears on an explicit non-undefined value. Every render path goes through safeExternalHref: the REST contract accepts any string, so an unguarded href would execute a stored javascript: value in this origin. The desktop expanded view had the same omission and is fixed alongside it, so a member without collab_edit no longer loses the link there either. Closes #2222
This commit is contained in:
@@ -1275,7 +1275,7 @@ describe('CollabNotes', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// FE-W5CNT-001 to FE-W5CNT-029
|
||||
// FE-W5CNT-001 to FE-W5CNT-036
|
||||
// Fills in the load/error/attachment/category branches of useCollabNotes and the
|
||||
// view modal that the smoke tests above do not reach.
|
||||
|
||||
@@ -1947,4 +1947,32 @@ describe('CollabNotes details', () => {
|
||||
expect(full.querySelector(`[id="${href.slice(1)}"]`)).not.toBeNull();
|
||||
expect(ref).not.toHaveAttribute('target');
|
||||
});
|
||||
|
||||
it('FE-W5CNT-035: the expanded note links its website out to a new tab', async () => {
|
||||
const user = userEvent.setup();
|
||||
serveNotes({ notes: [buildNote({ id: 9, title: 'Hotel', website: 'www.hotel.test' })] });
|
||||
server.use(http.get('/api/trips/1/collab/link-preview', () => HttpResponse.json({ title: 'Hotel', image: null })));
|
||||
render(<CollabNotes {...defaultProps} />);
|
||||
await screen.findByText('Hotel');
|
||||
await user.click(screen.getByTitle('collab.notes.expand'));
|
||||
const full = await openedNoteBody();
|
||||
|
||||
// A member without collab_edit never opens the edit modal, so this is the
|
||||
// only place the link is readable for them (#2222).
|
||||
const link = within(full).getByRole('link');
|
||||
expect(link).toHaveAttribute('href', 'https://www.hotel.test');
|
||||
expect(link).toHaveAttribute('rel', 'noopener noreferrer');
|
||||
expect(link).toHaveTextContent('hotel.test');
|
||||
});
|
||||
|
||||
it('FE-W5CNT-036: a javascript: website stays unlinked in the expanded note', async () => {
|
||||
const user = userEvent.setup();
|
||||
serveNotes({ notes: [buildNote({ id: 10, title: 'Hotel', website: 'javascript:alert(1)' })] });
|
||||
render(<CollabNotes {...defaultProps} />);
|
||||
await screen.findByText('Hotel');
|
||||
await user.click(screen.getByTitle('collab.notes.expand'));
|
||||
const full = await openedNoteBody();
|
||||
|
||||
expect(within(full).queryAllByRole('link')).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,7 +4,7 @@ import remarkGfm from 'remark-gfm'
|
||||
import remarkBreaks from 'remark-breaks'
|
||||
import { sanitizedMarkdownComponents, sanitizedMarkdownPlugins } from '../shared/markdownSanitize'
|
||||
import { createPortal } from 'react-dom'
|
||||
import { Plus, Pencil, X, StickyNote, Settings } from 'lucide-react'
|
||||
import { Plus, Pencil, X, StickyNote, Settings, ExternalLink } from 'lucide-react'
|
||||
import { collabApi } from '../../api/client'
|
||||
import { useCanDo } from '../../store/permissionsStore'
|
||||
import { useTripStore } from '../../store/tripStore'
|
||||
@@ -21,6 +21,7 @@ import { CategorySettingsModal } from './CollabNotesCategorySettingsModal'
|
||||
import { NoteCard } from './CollabNotesCard'
|
||||
import { FilePreviewPortal } from './CollabNotesFilePreviewPortal'
|
||||
import { AuthedImg } from './CollabNotesAuthedImg'
|
||||
import { safeExternalHref } from '../../utils/safeUrl'
|
||||
|
||||
// ── Main Component ──────────────────────────────────────────────────────────
|
||||
interface CollabNotesProps {
|
||||
@@ -393,9 +394,18 @@ function CollabNotesGrid(S: NotesState) {
|
||||
)
|
||||
}
|
||||
|
||||
/** Label for the link chip: the host is what people recognise, "www." is noise. */
|
||||
function linkHost(url: string): string {
|
||||
try { return new URL(url).hostname.replace(/^www\./, '') } catch { return url }
|
||||
}
|
||||
|
||||
function ViewNoteModal(S: NotesState) {
|
||||
const { viewingNote, setViewingNote, canEdit, setEditingNote, getCategoryColor, t, setPreviewFile } = S
|
||||
if (!viewingNote) return null
|
||||
// A member without collab_edit only ever gets this modal, so the link has to
|
||||
// be here too (#2222). Allow-listed like the card tile: the field takes any
|
||||
// string, and a javascript: one would run in this origin.
|
||||
const websiteHref = safeExternalHref(viewingNote.website)
|
||||
return createPortal(
|
||||
<div
|
||||
style={{
|
||||
@@ -447,6 +457,17 @@ function ViewNoteModal(S: NotesState) {
|
||||
</div>
|
||||
<div className="collab-note-md-full" style={{ padding: '16px 20px', overflowY: 'auto', fontSize: 'calc(14px * var(--fs-scale-body, 1))', color: 'var(--text-primary)', lineHeight: 1.7 }}>
|
||||
<Markdown remarkPlugins={[remarkGfm, remarkBreaks]} rehypePlugins={sanitizedMarkdownPlugins} components={sanitizedMarkdownComponents}>{viewingNote.content || ''}</Markdown>
|
||||
{websiteHref && (
|
||||
<a href={websiteHref} target="_blank" rel="noopener noreferrer"
|
||||
style={{
|
||||
display: 'inline-flex', alignItems: 'center', gap: 6, marginTop: 12,
|
||||
padding: '6px 12px', borderRadius: 999, border: '1px solid var(--border-primary)',
|
||||
fontSize: 'calc(12px * var(--fs-scale-caption, 1))', fontWeight: 600,
|
||||
color: 'var(--text-primary)', textDecoration: 'none',
|
||||
}}>
|
||||
<ExternalLink size={13} color="var(--text-muted)" /> {linkHost(websiteHref)}
|
||||
</a>
|
||||
)}
|
||||
{(viewingNote.attachments || []).length > 0 && (
|
||||
<div style={{ marginTop: 16, paddingTop: 16, borderTop: '1px solid var(--border-primary)' }}>
|
||||
<div style={{ fontSize: 'calc(11px * var(--fs-scale-caption, 1))', fontWeight: 600, color: 'var(--text-faint)', textTransform: 'uppercase', letterSpacing: 0.5, marginBottom: 10 }}>{t('files.title')}</div>
|
||||
|
||||
@@ -3,11 +3,12 @@ import Markdown from 'react-markdown'
|
||||
import remarkGfm from 'remark-gfm'
|
||||
import remarkBreaks from 'remark-breaks'
|
||||
import { sanitizedMarkdownPlugins, sanitizedMarkdownComponents } from '../../../../components/shared/markdownSanitize'
|
||||
import { Check, FileText, Paperclip, Pin, PinOff, Plus, StickyNote, Trash2, X } from 'lucide-react'
|
||||
import { Check, ExternalLink, FileText, Paperclip, Pin, PinOff, Plus, StickyNote, Trash2, X } from 'lucide-react'
|
||||
import MDancingTrek from '../../../components/MDancingTrek'
|
||||
import { collabApi } from '../../../../api/client'
|
||||
import { addListener, removeListener } from '../../../../api/websocket'
|
||||
import { openFile } from '../../../../utils/fileDownload'
|
||||
import { safeExternalHref } from '../../../../utils/safeUrl'
|
||||
import MSheet from '../../../components/MSheet'
|
||||
import { Eyebrow, FIELD_AREA_CLS, FIELD_CLS, FormSheetFooter, FormSheetHeader } from '../sheets/PlSheetChrome'
|
||||
import MConfirmSheet from '../../settings/MConfirmSheet'
|
||||
@@ -36,9 +37,15 @@ interface NoteFormSubmitData {
|
||||
content?: string
|
||||
category?: string
|
||||
color: string
|
||||
website: string | null
|
||||
pendingFiles: File[]
|
||||
}
|
||||
|
||||
/** Label for a link chip: the host is what people recognise, "www." is noise. */
|
||||
function linkHost(url: string): string {
|
||||
try { return new URL(url).hostname.replace(/^www\./, '') } catch { return url }
|
||||
}
|
||||
|
||||
/**
|
||||
* Trip-tab Collab / Notes. Same architecture as MCollabChat: own state, own
|
||||
* `collabApi` calls, own WebSocket listener — no `tripStore`/`tripActions`
|
||||
@@ -98,6 +105,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) {
|
||||
try {
|
||||
const res = (await collabApi.createNote(tripId, {
|
||||
title: data.title, content: data.content, category: data.category, color: data.color,
|
||||
website: data.website,
|
||||
})) as NoteResponse
|
||||
created = res.note
|
||||
} catch {
|
||||
@@ -119,7 +127,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) {
|
||||
|
||||
const handleUpdate = useCallback(async (
|
||||
noteId: number,
|
||||
data: { title?: string; content?: string; category?: string; color?: string; pinned?: boolean },
|
||||
data: { title?: string; content?: string; category?: string; color?: string; pinned?: boolean; website?: string | null },
|
||||
pendingFiles: File[] = [],
|
||||
) => {
|
||||
let updated: CollabNoteData | undefined
|
||||
@@ -256,6 +264,7 @@ export default function MCollabNotes({ planner }: MCollabNotesProps) {
|
||||
if (formTarget && formTarget !== 'new') {
|
||||
await handleUpdate(formTarget.id, {
|
||||
title: data.title, content: data.content, category: data.category, color: data.color,
|
||||
website: data.website,
|
||||
}, data.pendingFiles)
|
||||
} else {
|
||||
await handleCreate(data)
|
||||
@@ -296,6 +305,10 @@ function NoteCardRow({ note, color, canEdit, onTap, onTogglePin, onDelete, t }:
|
||||
t: TripPlanner['t']
|
||||
}) {
|
||||
const initial = (note.username || '?')[0]?.toUpperCase() || '?'
|
||||
// An editor is routed to the form sheet, never to the viewer, so the card is
|
||||
// the only surface where both roles can reach the link (#2222). The field
|
||||
// takes any string, so the href is allow-listed the way the desktop tile is.
|
||||
const websiteHref = safeExternalHref(note.website)
|
||||
|
||||
return (
|
||||
<div className="mt-2 overflow-hidden rounded-2xl border border-[color:var(--m-rowbr)] bg-[color:var(--m-ic)]">
|
||||
@@ -312,6 +325,17 @@ function NoteCardRow({ note, color, canEdit, onTap, onTogglePin, onDelete, t }:
|
||||
</span>
|
||||
)}
|
||||
</button>
|
||||
{websiteHref && (
|
||||
<a
|
||||
href={websiteHref}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
aria-label={t('collab.notes.website')}
|
||||
className="flex h-[26px] w-[26px] flex-none items-center justify-center rounded-full bg-[color:var(--m-ic)] text-m-muted"
|
||||
>
|
||||
<ExternalLink size={12} strokeWidth={2.2} />
|
||||
</a>
|
||||
)}
|
||||
{canEdit && (
|
||||
<button
|
||||
type="button"
|
||||
@@ -376,6 +400,7 @@ function NoteFormSheet({ open, target, categories, colorMap, canUploadFiles, onC
|
||||
const [category, setCategory] = useState<string | null>(null)
|
||||
const [addingCategory, setAddingCategory] = useState(false)
|
||||
const [newCategoryDraft, setNewCategoryDraft] = useState('')
|
||||
const [website, setWebsite] = useState('')
|
||||
const [pendingFiles, setPendingFiles] = useState<File[]>([])
|
||||
const [existingFiles, setExistingFiles] = useState<CollabNoteFile[]>([])
|
||||
const [submitting, setSubmitting] = useState(false)
|
||||
@@ -388,6 +413,7 @@ function NoteFormSheet({ open, target, categories, colorMap, canUploadFiles, onC
|
||||
setCategory(note?.category || null)
|
||||
setAddingCategory(false)
|
||||
setNewCategoryDraft('')
|
||||
setWebsite(note?.website || '')
|
||||
setPendingFiles([])
|
||||
setExistingFiles(note?.attachments || [])
|
||||
}, [open, note])
|
||||
@@ -403,6 +429,9 @@ function NoteFormSheet({ open, target, categories, colorMap, canUploadFiles, onC
|
||||
content: content.trim() || undefined,
|
||||
category: category || undefined,
|
||||
color: getCategoryColor(category, colorMap),
|
||||
// Always sent, never undefined: the server only writes the column when
|
||||
// the key is present, so an emptied field has to arrive as null to clear.
|
||||
website: website.trim() || null,
|
||||
pendingFiles,
|
||||
})
|
||||
onClose()
|
||||
@@ -500,6 +529,16 @@ function NoteFormSheet({ open, target, categories, colorMap, canUploadFiles, onC
|
||||
)}
|
||||
</div>
|
||||
|
||||
<Eyebrow className="mb-[5px] mt-3 uppercase">{t('collab.notes.website')}</Eyebrow>
|
||||
<input
|
||||
type="url"
|
||||
inputMode="url"
|
||||
value={website}
|
||||
onChange={e => setWebsite(e.target.value)}
|
||||
placeholder={t('collab.notes.websitePlaceholder')}
|
||||
className={FIELD_CLS}
|
||||
/>
|
||||
|
||||
{canUploadFiles && (
|
||||
<>
|
||||
<Eyebrow className="mb-[6px] mt-3 uppercase">{t('collab.notes.attachFiles')}</Eyebrow>
|
||||
@@ -588,6 +627,7 @@ function NoteViewSheet({ open, note, onClose, t }: {
|
||||
useEffect(() => {
|
||||
if (open) setSnapshot(note)
|
||||
}, [open, note])
|
||||
const websiteHref = safeExternalHref(snapshot?.website)
|
||||
|
||||
return (
|
||||
<MSheet open={open} onClose={onClose} ariaLabel={snapshot?.title}>
|
||||
@@ -604,6 +644,17 @@ function NoteViewSheet({ open, note, onClose, t }: {
|
||||
<Markdown remarkPlugins={[remarkGfm, remarkBreaks]} rehypePlugins={sanitizedMarkdownPlugins} components={sanitizedMarkdownComponents}>{snapshot.content}</Markdown>
|
||||
</div>
|
||||
)}
|
||||
{websiteHref && (
|
||||
<a
|
||||
href={websiteHref}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="mt-3 inline-flex items-center gap-[5px] rounded-full border border-[color:var(--m-rowbr)] bg-m-card px-3 py-[7px] font-geist text-[0.71875rem] font-semibold text-m-ink"
|
||||
>
|
||||
<ExternalLink size={13} strokeWidth={2} className="text-m-muted" />
|
||||
{linkHost(websiteHref)}
|
||||
</a>
|
||||
)}
|
||||
{(snapshot?.attachments.length ?? 0) > 0 && (
|
||||
<div className="mt-4 flex flex-col gap-[6px]">
|
||||
{snapshot?.attachments.map(f => (
|
||||
|
||||
@@ -10,7 +10,7 @@ import { resetAllStores } from '../../../helpers/store'
|
||||
import { server } from '../../../helpers/msw/server'
|
||||
import { act, fireEvent, render, screen, waitFor, within } from '../../../helpers/render'
|
||||
|
||||
// FE-MOB-CNOTE-001 to FE-MOB-CNOTE-026
|
||||
// FE-MOB-CNOTE-001 to FE-MOB-CNOTE-033
|
||||
|
||||
// react-markdown ships as ESM-only chunks that jsdom chokes on; the note cards
|
||||
// only care that the raw text reaches the renderer.
|
||||
@@ -251,7 +251,7 @@ describe('MCollabNotes', () => {
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' }))
|
||||
|
||||
await waitFor(() => expect(createdBodies).toEqual([
|
||||
{ title: 'Insurance', content: 'call the hotline', category: 'Docs', color: '#ef4444' },
|
||||
{ title: 'Insurance', content: 'call the hotline', category: 'Docs', color: '#ef4444', website: null },
|
||||
]))
|
||||
expect(await screen.findByText('Insurance')).toBeInTheDocument()
|
||||
await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument())
|
||||
@@ -270,7 +270,7 @@ describe('MCollabNotes', () => {
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' }))
|
||||
|
||||
await waitFor(() => expect(createdBodies).toEqual([
|
||||
{ title: 'Ferry', category: 'Transport', color: '#6366f1' },
|
||||
{ title: 'Ferry', category: 'Transport', color: '#6366f1', website: null },
|
||||
]))
|
||||
})
|
||||
|
||||
@@ -292,7 +292,7 @@ describe('MCollabNotes', () => {
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' }))
|
||||
|
||||
await waitFor(() => expect(createdBodies).toEqual([
|
||||
{ title: 'Ferry', category: 'Food', color: '#6366f1' },
|
||||
{ title: 'Ferry', category: 'Food', color: '#6366f1', website: null },
|
||||
]))
|
||||
})
|
||||
|
||||
@@ -324,7 +324,7 @@ describe('MCollabNotes', () => {
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.save' }))
|
||||
|
||||
await waitFor(() => expect(updatedBodies).toEqual([
|
||||
{ id: '1', body: { title: 'Visa (done)', content: 'apply early', category: 'Docs', color: '#ef4444' } },
|
||||
{ id: '1', body: { title: 'Visa (done)', content: 'apply early', category: 'Docs', color: '#ef4444', website: null } },
|
||||
]))
|
||||
expect(await screen.findByText('Visa (done)')).toBeInTheDocument()
|
||||
})
|
||||
@@ -515,4 +515,96 @@ describe('MCollabNotes', () => {
|
||||
unmount()
|
||||
expect(removeListener).toHaveBeenCalledWith(handler)
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-027: creates a note with the trimmed website', async () => {
|
||||
serveNotes([])
|
||||
serveWrites()
|
||||
await renderNotes()
|
||||
|
||||
fireEvent.click(screen.getByRole('button', { name: /collab\.notes\.new/ }))
|
||||
fireEvent.change(screen.getByPlaceholderText('collab.notes.titlePlaceholder'), { target: { value: 'Ferry' } })
|
||||
fireEvent.change(screen.getByPlaceholderText('collab.notes.websitePlaceholder'), { target: { value: ' https://ferry.test/booking ' } })
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' }))
|
||||
|
||||
await waitFor(() => expect(createdBodies).toEqual([
|
||||
{ title: 'Ferry', color: '#6366f1', website: 'https://ferry.test/booking' },
|
||||
]))
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-028: a blank website is posted as null instead of an empty string', async () => {
|
||||
serveNotes([])
|
||||
serveWrites()
|
||||
await renderNotes()
|
||||
|
||||
fireEvent.click(screen.getByRole('button', { name: /collab\.notes\.new/ }))
|
||||
fireEvent.change(screen.getByPlaceholderText('collab.notes.titlePlaceholder'), { target: { value: 'Ferry' } })
|
||||
fireEvent.change(screen.getByPlaceholderText('collab.notes.websitePlaceholder'), { target: { value: ' ' } })
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.create' }))
|
||||
|
||||
await waitFor(() => expect(createdBodies).toEqual([
|
||||
{ title: 'Ferry', color: '#6366f1', website: null },
|
||||
]))
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-029: editing prefills the stored website and saves it back unchanged', async () => {
|
||||
serveNotes([note(1, { title: 'Visa', website: 'https://visa.test' })])
|
||||
serveWrites()
|
||||
await renderNotes()
|
||||
|
||||
fireEvent.click(screen.getByText('Visa'))
|
||||
expect(screen.getByPlaceholderText('collab.notes.websitePlaceholder')).toHaveValue('https://visa.test')
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.save' }))
|
||||
|
||||
await waitFor(() => expect(updatedBodies).toEqual([
|
||||
{ id: '1', body: { title: 'Visa', color: '#6366f1', website: 'https://visa.test' } },
|
||||
]))
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-030: clearing the website sends an explicit null so the server drops it', async () => {
|
||||
serveNotes([note(1, { title: 'Visa', website: 'https://visa.test' })])
|
||||
serveWrites()
|
||||
await renderNotes()
|
||||
|
||||
fireEvent.click(screen.getByText('Visa'))
|
||||
fireEvent.change(screen.getByPlaceholderText('collab.notes.websitePlaceholder'), { target: { value: '' } })
|
||||
fireEvent.click(screen.getByRole('button', { name: 'collab.notes.save' }))
|
||||
|
||||
await waitFor(() => expect(updatedBodies).toEqual([
|
||||
{ id: '1', body: { title: 'Visa', color: '#6366f1', website: null } },
|
||||
]))
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-031: the viewer sheet links a bare host out to a new tab', async () => {
|
||||
serveNotes([note(1, { title: 'Visa', website: 'www.visa.test' })])
|
||||
await renderNotes(buildPlanner({ can: (() => false) as TripPlanner['can'] }))
|
||||
|
||||
fireEvent.click(screen.getByText('Visa'))
|
||||
const link = within(screen.getByRole('dialog', { name: 'Visa' })).getByRole('link')
|
||||
expect(link).toHaveAttribute('href', 'https://www.visa.test')
|
||||
expect(link).toHaveAttribute('target', '_blank')
|
||||
expect(link).toHaveAttribute('rel', 'noopener noreferrer')
|
||||
expect(link).toHaveTextContent('visa.test')
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-032: a javascript: website is never turned into a link', async () => {
|
||||
serveNotes([note(1, { title: 'Visa', website: 'javascript:alert(1)' })])
|
||||
await renderNotes(buildPlanner({ can: (() => false) as TripPlanner['can'] }))
|
||||
|
||||
expect(screen.queryAllByRole('link')).toHaveLength(0)
|
||||
fireEvent.click(screen.getByText('Visa'))
|
||||
expect(screen.getByRole('dialog', { name: 'Visa' })).toBeInTheDocument()
|
||||
expect(screen.queryAllByRole('link')).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('FE-MOB-CNOTE-033: only a card with a website carries the link affordance', async () => {
|
||||
serveNotes([
|
||||
note(1, { title: 'Visa', website: 'https://visa.test' }),
|
||||
note(2, { title: 'Tent' }),
|
||||
])
|
||||
await renderNotes()
|
||||
|
||||
const links = screen.getAllByRole('link', { name: 'collab.notes.website' })
|
||||
expect(links).toHaveLength(1)
|
||||
expect(links[0]).toHaveAttribute('href', 'https://visa.test')
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user