fix(post): deleting a comment from the options sheet no longer pops the screen - #3408
Conversation
…he screen postComments mounted the sheet for comments without an onDelete, so the sheet's own delete path ran: it called navigation.goBack() and left the post the user was reading, and skipped _handleDeleteComment, which the same screen already passes to the inline delete button and which owns the error extraction and deleted-key cache work. Passes onDelete forwarding to that handler, with the same arguments the inline button uses. Also makes the fallback safe rather than relying on every consumer remembering. Going back only makes sense when the deleted content is the screen, which is true for a root post and false for a comment in a list, so the fallback now pops only for content with no parent. The existing comment on _deletePost already warned about this and it was still missed twice while adding consumers. Audited every PostOptionsModal consumer. postScreen and editorScreen delete the content that is the screen, so the pop is correct there. postsListContainer has the same gap for posts in a feed and no list removal to delegate to; filed as #3407 rather than half-fixed here, since suppressing the pop without removal would leave a stale feed with no feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 364ad95d3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!content?.parent_author) { | ||
| navigation.goBack(); | ||
| } |
There was a problem hiding this comment.
When the options modal is opened from PostScreen for a comment or wave, that content is the screen itself, but PostScreen mounts the modal without an onDelete callback. This condition therefore deletes the item and leaves the user on its now-deleted detail screen. The new PostComments callback already prevents list deletions from reaching this fallback, so the fallback should continue navigating back after successful deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both of you caught this independently and you are right - the heuristic was a regression. Reverted in f12935f.
postScreen renders comments and waves as primary content (postScreen.tsx:51 checks parent_author || parent_permlink, :69 gates on depth > 0 && parent_author), and it mounts the sheet with no onDelete. So gating the pop on parent_author left a comment's own detail screen sitting on content that had just been deleted.
The mistake was assuming content shape could stand in for "is this the screen". It cannot: a comment in a list and a comment as the screen are identical objects. Only the caller knows.
So the fallback pops unconditionally again, and consumers that own a surrounding list must pass onDelete. That is now true of all of them except postsListContainer, tracked in #3407.
Worth recording that the safe-by-default goal still stands, it just needs the other shape: an explicit opt-in like popScreenOnDelete rather than inference. Forgetting an opt-in leaves you on the screen, which is mildly wrong; forgetting onDelete navigates you away, which is badly wrong. I have added this attempt and why it failed to #3407 so the next person does not retry the heuristic.
| (comment) => | ||
| _handleDeleteComment( | ||
| comment.permlink, |
There was a problem hiding this comment.
When deleting a comment from this options menu, PostOptionsModal._deletePost has already shown and received confirmation from its action modal before invoking onDelete. Delegating here to _handleDeleteComment opens the same confirmation flow again, so users must confirm deletion twice (and can cancel the second prompt after confirming the first); delegate to a mutation-only handler or otherwise let only one layer own confirmation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in f12935f. PostOptionsModal._deletePost confirms via ACTION_MODAL before invoking onDelete, and _handleDeleteComment then confirmed again, so one delete needed two confirmations and could be cancelled at the second after being confirmed at the first.
Split the handler in two rather than suppressing either prompt:
_deleteCommentConfirmedmutates with no prompt, and is what the sheet delegates to_handleDeleteCommentprompts and then calls it, for the inline delete button which has no confirmation of its own
That keeps exactly one confirmation on each path and leaves the inline button's behaviour unchanged.
Checked the equivalent path added in #3405: commentsContainer has no confirmation step at all, so commentsView delegating to it was already single-prompt and is unaffected.
Greptile SummaryThe PR now routes comment-list deletions through the owning list’s mutation and cache-handling callback while retaining back navigation when the deleted content is the primary screen.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported stale primary-content screen is resolved by navigating back after fallback deletion, while list-owned comment deletion now uses its consumer callback.
|
| Filename | Overview |
|---|---|
| src/components/postComments/container/postComments.tsx | Separates confirmation from the confirmed mutation and supplies the modal with a list-owned deletion callback that removes the comment without navigating away. |
| src/components/postOptionsModal/container/postOptionsModal.tsx | Restores unconditional back navigation in the fallback deletion path, resolving the previously reported stale comment or wave detail screen. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Confirm deletion in PostOptionsModal] --> B{Consumer provides onDelete?}
B -->|Yes: surrounding list| C[Invoke consumer deletion handler]
C --> D[Update list/cache and remain on screen]
B -->|No: primary content| E[Run fallback deletion mutation]
E --> F[Navigate back from deleted content]
Reviews (2): Last reviewed commit: "fix(post): drop the parent_author pop he..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe post comments screen now centralizes confirmed comment deletion and routes options-modal deletions through the shared flow. The options modal always navigates back for local deletion, while delegated comment deletion remains owned by the caller. ChangesComment deletion behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PostOptionsModal
participant PostComments
participant CommentCache
User->>PostOptionsModal: Select delete
PostOptionsModal->>PostComments: Pass comment metadata
PostComments->>CommentCache: Optimistically hide comment
PostComments->>CommentCache: Delete or restore cached comment
PostComments-->>User: Show deletion error if mutation fails
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/postComments/container/postComments.tsx (1)
273-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the obsolete navigation claim from this comment.
PostOptionsModalnow skipsnavigation.goBack()whencontent.parent_authoridentifies a comment. Keep the comment focused on routing through this screen’s error handling and deleted-key cache updates.Proposed comment update
- // The sheet is opened for comments here, so its own delete path would call - // navigation.goBack() and leave the post the user is reading, and would skip - // the error handling and deleted-key cache work this screen already owns. + // Route comment deletion through this screen's handler so it preserves the + // comment-specific error handling and deleted-key cache updates.🤖 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/components/postComments/container/postComments.tsx` around lines 273 - 276, Update the comment adjacent to the PostOptionsModal delete flow in the post comments container: remove the obsolete claim that the sheet’s delete path calls navigation.goBack() or leaves the post. Keep only the rationale that this screen owns the error handling and deleted-key cache updates, while preserving the note about matching the inline delete button’s arguments.
🤖 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/components/postComments/container/postComments.tsx`:
- Around line 473-477: Update the comment-deletion flow connecting
PostOptionsModal’s onDelete callback and _handleDeleteComment so only one
confirmation sheet is shown. Make the delegated callback non-confirming, or
disable PostOptionsModal’s confirmation for this path, while preserving the
existing confirmation behavior for other deletion flows.
---
Nitpick comments:
In `@src/components/postComments/container/postComments.tsx`:
- Around line 273-276: Update the comment adjacent to the PostOptionsModal
delete flow in the post comments container: remove the obsolete claim that the
sheet’s delete path calls navigation.goBack() or leaves the post. Keep only the
rationale that this screen owns the error handling and deleted-key cache
updates, while preserving the note about matching the inline delete button’s
arguments.
🪄 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: CHILL
Plan: Pro Plus
Run ID: e229603c-2fca-48b8-a308-390cab85f02b
📒 Files selected for processing (2)
src/components/postComments/container/postComments.tsxsrc/components/postOptionsModal/container/postOptionsModal.tsx
…mation Two review findings, both regressions in the previous commit. postScreen renders comments and waves as primary content, so gating navigation.goBack() on parent_author left a comment's own detail screen showing content that had just been deleted. Content shape cannot distinguish 'comment in a list' from 'comment as the screen' - only the caller knows - so the fallback pops unconditionally again and consumers that own a list must pass onDelete. #3407 tracks inverting this into an explicit opt-in, which is the version that fails safe. The delete delegation also asked twice: PostOptionsModal confirms before invoking onDelete, and _handleDeleteComment then confirmed again, so one action needed two confirmations and could be cancelled at the second after being confirmed at the first. Splits that handler in two. _deleteCommentConfirmed mutates without prompting and is what the sheet delegates to; _handleDeleteComment prompts and then calls it, for the inline delete button which has no confirmation of its own. commentsContainer has no confirmation step, so the equivalent path added in #3405 was never affected.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/postComments/container/postComments.tsx (1)
216-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocalize the delete-failure toast message.
The error toast in
_deleteCommentConfirmeduses a raw hardcoded string, notintl.formatMessage. Every other toast in this file and inpostOptionsModal.tsx's delegated-error path usesintl.formatMessage. Localize this message for consistency.🌐 Proposed fix
- dispatch(toastNotification(`Failed to delete comment: ${errorDetail}`)); + dispatch( + toastNotification( + `${intl.formatMessage({ id: 'alert.fail' })}: ${errorDetail}`, + ), + );🤖 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/components/postComments/container/postComments.tsx` around lines 216 - 221, Update the delete-failure toast in _deleteCommentConfirmed to use intl.formatMessage with the existing localized message conventions, preserving the extracted errorDetail in the rendered message and the current stillExists branching behavior.
🤖 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.
Nitpick comments:
In `@src/components/postComments/container/postComments.tsx`:
- Around line 216-221: Update the delete-failure toast in
_deleteCommentConfirmed to use intl.formatMessage with the existing localized
message conventions, preserving the extracted errorDetail in the rendered
message and the current stillExists branching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d93a09f-3b3c-4305-84a0-255162536992
📒 Files selected for processing (2)
src/components/postComments/container/postComments.tsxsrc/components/postOptionsModal/container/postOptionsModal.tsx
1 participant
Closes #3406
postComments.tsxmountedPostOptionsModalfor comments with noonDelete, so deleting a comment from the sheet ran the sheet's own path:navigation.goBack()atpostOptionsModal.tsx:478, which navigates away from the post the user is reading. It also skipped_handleDeleteComment(postComments.tsx:174), which the same screen already passes to the inline delete button and which owns the error extraction and deleted-key cache handling. The sheet and the button did different things for the same action.This predates #3405; that PR fixed the same gap for five list surfaces, and this was the remaining comment consumer.
Two parts
The consumer.
postCommentsnow passesonDeleteforwarding to its existing_handleDeleteComment, with the same five arguments the inline button uses.The fallback. Relying on every consumer remembering
onDeletehas not worked - the existing comment on_deletePostalready warned about this for waves, and it was still missed twice while adding consumers. Going back only makes sense when the deleted content is the screen, which is true for a root post and false for a comment in a list, so the fallback now pops only for content with noparent_author.Consumer audit
onDeletewavesScreenwavesQuery.deleteWavecommentsViewpostCommentspostScreeneditorScreenpostsListContainerpostsListContainerhas the same gap for posts in a feed, and no list removal to delegate to. Filed as #3407 rather than half-fixed: a post in a list is indistinguishable from a post on its own screen by content shape, and suppressing the pop without adding removal would leave a stale feed with no feedback that anything happened. That issue also raises whether the prop should be inverted to an opt-inpopScreenOnDelete, since forgetting an opt-in leaves you on the screen while forgettingonDeletenavigates you away.Testing
yarn test:ci: 730 passed, 1 skipped, 49 suites.yarn lint: 0 errors.Device checks:
Summary by CodeRabbit