feat/thread-deep-link#664
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements deep-linking and hash-based navigation to specific messages and events within threads. It adds backend support for retrieving spam/trashed threads, frontend hooks for copying deep-link URLs to clipboard, mailbox-context-aware data loading for missing threads, hash parsing with deferred smooth-scroll and highlight mechanics, UI copy-link integrations, tests, locales, and highlight animations. ChangesDeep-linking and hash-based navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
jbpenrath
force-pushed
the
feat/thread-deep-link
branch
2 times, most recently
from
72c4152 to
a16bc49
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
210-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnable long-press actions for non-authors on touch devices.
Line 210 to Line 213 gate long-press handlers behind
canModify, but the copy-link action is available to everyone (Line 226 onward). On touch-only devices, non-authors can’t open the actions UI, so they can’t copy event links.Suggested fix
- onTouchStart={canModify ? handleTouchStart : undefined} - onTouchEnd={canModify ? cancelLongPress : undefined} - onTouchMove={canModify ? cancelLongPress : undefined} - onTouchCancel={canModify ? cancelLongPress : undefined} + onTouchStart={handleTouchStart} + onTouchEnd={cancelLongPress} + onTouchMove={cancelLongPress} + onTouchCancel={cancelLongPress}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx` around lines 210 - 213, The touch handlers are gated by canModify so non-authors on touch devices cannot trigger the long-press actions; remove the canModify condition from the JSX props so onTouchStart uses handleTouchStart and onTouchEnd/onTouchMove/onTouchCancel use cancelLongPress for all users (leave internal logic in the action UI or handlers to hide actions that require modification); specifically update the elements that currently conditionally set onTouchStart={canModify ? handleTouchStart : undefined} and onTouchEnd/onTouchMove/onTouchCancel={canModify ? cancelLongPress : undefined} to always attach handleTouchStart and cancelLongPress so the copy-link and other public actions are reachable via long-press.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/layouts/components/thread-panel/index.tsx`:
- Around line 65-77: The effect that updates selectedThreadWasInListRef can
carry over truthy state from a previous thread; to fix, reset the per-thread
flag whenever the selected thread changes: add a prevSelectedIdRef (or reuse
selectedThreadWasInListRef for storing the last selected id) and at the start of
the useEffect (which currently references selectedThread, threads and calls
unselectThread) check if selectedThread?.id !== prevSelectedIdRef.current — if
so set selectedThreadWasInListRef.current = false and update
prevSelectedIdRef.current = selectedThread?.id — then continue to compute
isInList and handle unselectThread as before so the “was in list” tracking is
fresh for each new selectedThread.
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 366-370: The fallback deep-link fetch via useThreadsRetrieve (the
fallbackThreadQuery) is missing the mailbox context, so update the call to pass
the current mailbox id (e.g., mailboxId, mailboxIdFromRoute, or
selectedMailbox?.id) into the retrieve options so mailbox-scoped
serialization/annotations are computed correctly; specifically, add mailbox_id
into the params/options object you pass to useThreadsRetrieve (while keeping the
existing enabled condition that uses threadIdFromRoute, flattenThreads and
threadInList).
- Around line 388-397: The effect handling fallbackThreadQuery.isError is
currently redirecting on any error; change it to only redirect when the query
error indicates authorization or not-found (e.g., HTTP status 401, 403, or 404
or a domain-specific error.code like 'NOT_FOUND'/'UNAUTHORIZED') by replacing
the simple truthy check with a guard that inspects fallbackThreadQuery.error
(e.g., fallbackThreadQuery.error?.status or error.code) before setting
redirectedOnErrorRef.current and calling
router.replace(`/mailbox/${selectedMailbox.id}?${defaultFilter}`) so transient
network/server errors (5xx, network) do not trigger the deep-link fallback; keep
existing usage of redirectedOnErrorRef, selectedMailbox, threadIdFromRoute and
MAILBOX_FOLDERS() in the same effect.
---
Outside diff comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 210-213: The touch handlers are gated by canModify so non-authors
on touch devices cannot trigger the long-press actions; remove the canModify
condition from the JSX props so onTouchStart uses handleTouchStart and
onTouchEnd/onTouchMove/onTouchCancel use cancelLongPress for all users (leave
internal logic in the action UI or handlers to hide actions that require
modification); specifically update the elements that currently conditionally set
onTouchStart={canModify ? handleTouchStart : undefined} and
onTouchEnd/onTouchMove/onTouchCancel={canModify ? cancelLongPress : undefined}
to always attach handleTouchStart and cancelLongPress so the copy-link and other
public actions are reachable via long-press.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 089579f2-7af0-4c57-bddb-cb35dad050e9
📒 Files selected for processing (14)
src/backend/core/api/viewsets/thread.pysrc/backend/core/tests/api/test_threads_list.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/index.tsxsrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-copy-deep-link.tsxsrc/frontend/src/features/providers/mailbox.tsx
jbpenrath
force-pushed
the
feat/thread-deep-link
branch
3 times, most recently
from
de85d02 to
e00d926
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/frontend/src/features/providers/mailbox.tsx (1)
395-405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict the fallback redirect to auth/not-found failures.
This still redirects on any retrieve error, so a transient network or 5xx failure drops the user out of the deep-link flow instead of letting the query recover. Only 401/403/404-style failures should trigger the mailbox fallback.
💡 Suggested fix
useEffect(() => { if (!fallbackThreadQuery.isError) { redirectedOnErrorRef.current = false; return; } + const error = fallbackThreadQuery.error; + if (!(error instanceof APIError) || ![401, 403, 404].includes(error.code)) { + return; + } if (redirectedOnErrorRef.current) return; if (!selectedMailbox || !threadIdFromRoute) return; redirectedOnErrorRef.current = true; const defaultFilter = new URLSearchParams(MAILBOX_FOLDERS()[0].filter).toString(); router.replace(`/mailbox/${selectedMailbox.id}?${defaultFilter}`); - }, [fallbackThreadQuery.isError, selectedMailbox, threadIdFromRoute]); + }, [fallbackThreadQuery.isError, fallbackThreadQuery.error, selectedMailbox, threadIdFromRoute, router]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/providers/mailbox.tsx` around lines 395 - 405, The effect currently redirects whenever fallbackThreadQuery.isError is true; change it to only redirect for authentication/not-found failures (HTTP 401, 403, 404). In the useEffect that references fallbackThreadQuery.isError, inspect fallbackThreadQuery.error (or its status/code) to determine the HTTP status and only proceed with setting redirectedOnErrorRef.current = true and calling router.replace(`/mailbox/${selectedMailbox.id}?${defaultFilter}`) when the status is 401, 403, or 404; for other errors (network/5xx) return early and let the query recover. Keep the existing guards for redirectedOnErrorRef, selectedMailbox, and threadIdFromRoute and ensure redirectedOnErrorRef is only set when performing the redirect; use MAILBOX_FOLDERS() to build defaultFilter as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 194-208: When a deep-link hash (hashMatch) targets a trashed
message that isn't visible, enable showTrashedMessages before the filtered
timeline is built so the DOM element exists for selector/#thread-message-…;
specifically, in the initialization where you compute hash, hashMatch, selector,
scrollBehavior and highlightTargetId, detect the hash target id and, if that
message is trashed and showTrashedMessages is false (and we’re not already in
the trash view), set showTrashedMessages = true (or call the setter) before the
code that builds/filters the timeline so the subsequent scroll/highlight logic
(selector, scrollBehavior, highlightTargetId) can find the element.
---
Duplicate comments:
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 395-405: The effect currently redirects whenever
fallbackThreadQuery.isError is true; change it to only redirect for
authentication/not-found failures (HTTP 401, 403, 404). In the useEffect that
references fallbackThreadQuery.isError, inspect fallbackThreadQuery.error (or
its status/code) to determine the HTTP status and only proceed with setting
redirectedOnErrorRef.current = true and calling
router.replace(`/mailbox/${selectedMailbox.id}?${defaultFilter}`) when the
status is 401, 403, or 404; for other errors (network/5xx) return early and let
the query recover. Keep the existing guards for redirectedOnErrorRef,
selectedMailbox, and threadIdFromRoute and ensure redirectedOnErrorRef is only
set when performing the redirect; use MAILBOX_FOLDERS() to build defaultFilter
as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2750da11-f7a0-4a41-87c0-8cb65c9ffa61
📒 Files selected for processing (14)
src/backend/core/api/viewsets/thread.pysrc/backend/core/tests/api/test_threads_list.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/index.tsxsrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-copy-deep-link.tsxsrc/frontend/src/features/providers/mailbox.tsx
jbpenrath
force-pushed
the
feat/thread-deep-link
branch
from
e00d926 to
b747c4e
Compare
Allow user to copy/paste a thread link with other mailbox users. Currently, if the user copy the current url, the link is broken once the thread has been moved from the origin folder. It is also possible to target a message or internal message.
jbpenrath
force-pushed
the
feat/thread-deep-link
branch
from
b747c4e to
8c3d576
Compare
jbpenrath
added a commit
that referenced
this pull request
May 20, 2026
Allow user to copy/paste a thread link with other mailbox users. Currently, if the user copy the current url, the link is broken once the thread has been moved from the origin folder. It is also possible to target a message or internal message.
jbpenrath
added a commit
that referenced
this pull request
May 20, 2026
Allow user to copy/paste a thread link with other mailbox users. Currently, if the user copy the current url, the link is broken once the thread has been moved from the origin folder. It is also possible to target a message or internal message.
jbpenrath
added a commit
that referenced
this pull request
May 20, 2026
Allow user to copy/paste a thread link with other mailbox users. Currently, if the user copy the current url, the link is broken once the thread has been moved from the origin folder. It is also possible to target a message or internal message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Add thread/message deep-linking feature to allow user to share thread with other mailbox users with ease.
Also, it allows to reference a thread-event or a message to automatically scroll to this resource.
CleanShot.2026-05-19.at.18.12.19.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests