Skip to content

Feat/init app DRAFT#1

Open
bohdanvenhrenovych wants to merge 21 commits intometatech-university:mainfrom
bohdanvenhrenovych:feat/init-app
Open

Feat/init app DRAFT#1
bohdanvenhrenovych wants to merge 21 commits intometatech-university:mainfrom
bohdanvenhrenovych:feat/init-app

Conversation

@bohdanvenhrenovych
Copy link
Copy Markdown

No description provided.

@bohdanvenhrenovych
Copy link
Copy Markdown
Author

Issue link #9

@bohdanvenhrenovych bohdanvenhrenovych changed the title Feat/init app Feat/init app DRAFT Jan 18, 2026
Comment on lines +54 to +65
const params = useParams()
const setPageTitle = useSetPageTitle()

useEffect(() => {
setPageTitle(getPageTitleFromParams(params))
}, [
params.feedId,
params.chatId,
params.contactId,
params.folderId,
params.itemId,
])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to extract this into a separate hook, something like useSetPageTitleFromParams

FormProps<FormValues> &
Omit<
DetailedHTMLProps<FormHTMLAttributes<HTMLFormElement>, HTMLFormElement>,
'onSubmit'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should be consistent and either omit all props we override 'onSubmit' | 'onReset' | 'noValidate' or let to override onSubmit as well

Comment on lines +35 to +39
const [isMobile, setIsMobile] = useState(false)

useEffect(() => {
setIsMobile(isMobilePlatform())
}, [])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like using isMobile state here could be redundant.
useState vs simple regExp.test() could be an overkill.


export function getFeedIconSrc(id: FeedIconId): string {
const src = srcById.get(id)
if (!src) throw new Error(`Unknown feed icon: ${id}`)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be an overreaction to crash the whole page because of a missing icon.

Comment on lines +10 to +15
const [messageList, setMessageList] = useState<Message[]>(messages)
const scrollContainerRef = useRef<HTMLDivElement>(null)

useEffect(() => {
setMessageList(messages)
}, [messages])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if possible we should only use messages directly.
ATM we have two sources for messageList, it can change because parent component updated the messages or we called handleReactionClick.

is can lead to unpredictable behaviour.

most likely handling reactions logic should be moved to parent component or state management lib

Comment on lines +22 to +25
function getPageTitleFromParams(
params: Record<string, string | undefined>
): PageTitleState {
const { feedId, chatId, contactId, folderId, itemId } = params as RouteParams
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function getPageTitleFromParams(
params: Record<string, string | undefined>
): PageTitleState {
const { feedId, chatId, contactId, folderId, itemId } = params as RouteParams
function getPageTitleFromParams(params: RouteParams): PageTitleState {
const { feedId, chatId, contactId, folderId, itemId } = params

Copy link
Copy Markdown

@agil agil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- ChatTimeline: use messages prop directly, optional onReactionClick
- Chat route: own messages state; reaction click merges by emoji (increment count)
- useSetPageTitleFromParams: type params as RouteParams, remove redundant assertion

Made-with: Cursor
@bohdanvenhrenovych bohdanvenhrenovych requested a review from agil March 1, 2026 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants