Waves editing fixes - #3196
Conversation
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
feruzm has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds deterministic permlink generation for wave replies, delegates post deletion via optional callback with eligibility update, routes deletes through per-feed registered deleters, hardens editor submission guards, and normalizes post URLs with tests. ChangesWave submission idempotency and delegated deletion
Sequence DiagramsequenceDiagram
participant User as User (Compose/Delete)
participant Submitter as usePostSubmitter
participant EditorUtil as generateContentBasedPermlink
participant PostModal as PostOptionsModal
participant WavesScreen as WavesScreen
participant FeedDeleter as Feed.deleteWave
User->>Submitter: _submitReply (WAVE)
Submitter->>EditorUtil: generateContentBasedPermlink(contentKey)
EditorUtil->>EditorUtil: SHA-256 -> hex -> truncate
EditorUtil-->>Submitter: deterministic permlink
Submitter-->>User: submit with deterministic permlink
User->>PostModal: open -> confirm delete
PostModal->>WavesScreen: onDelete(content)
WavesScreen->>FeedDeleter: deleteWave(permlink, parent_permlink)
FeedDeleter-->>WavesScreen: Promise<void> resolution
WavesScreen-->>PostModal: completion / Alert on missing handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/editor/container/editorContainer.tsx (1)
283-291:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical bug: Reply submissions are completely broken.
Setting
_isSubmitting = trueat line 1291 before calling_submitReplyat line 1294 causes_submitReplyto exit early on its first check at line 958. The first submission attempt itself would fail for replies because_submitReplychecksthis._isSubmittingat the very beginning and returns if it's true.For edits and new posts, this works correctly because an Alert is shown first and the actual submission happens asynchronously in the alert callback. But for replies,
_submitReplyis called directly without an alert, so it immediately hits the guard check and exits.Move
this._isSubmitting = trueto only the edit/post code paths, not before theif (isReply && !isEdit)branch. The reply path has its own guard in_submitReplythat will prevent duplicate submissions.🤖 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/screens/editor/container/editorContainer.tsx` around lines 283 - 291, The bug is that _isSubmitting is set to true before calling _submitReply for replies, causing the early-return guard in _submitReply to block the first reply submission; change the logic in the component method that currently sets this._isSubmitting = true (before the isReply check) so that _isSubmitting is only set in the edit/new-post code paths (the branches that show the Alert and then call _submitReply asynchronously) and not in the if (isReply && !isEdit) branch; leave _submitReply and its internal guard unchanged so replies rely on their existing duplication protection.
🤖 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/postOptionsModal/container/postOptionsModal.tsx`:
- Around line 352-363: The onDelete branch currently only logs errors so users
get no feedback; update the catch block for the onDelete(content) call to also
display a user-visible failure toast (use the same toast/helper used elsewhere
in this component, e.g., showToast/toast.error) with a friendly message like
"Failed to delete post" and optionally include err.message for debugging, keep
the console.warn and then return as before; modify the catch in the onDelete
handler around onDelete and content to call the toast helper.
In `@src/screens/waves/screen/wavesScreen.tsx`:
- Around line 387-397: The onDelete currently uses optional chaining on
activeDeleteWaveRef.current which can silently drop confirmed deletes; change it
to explicitly check activeDeleteWaveRef.current and, if present, invoke it with
the permlink/parent_permlink, otherwise perform a clear fallback (e.g., call the
same deletion logic used by the feed such as wavesQuery.deleteWave(permlink,
parent_permlink) and/or navigate back with navigation.goBack(), and log or
surface an error) so deletes are never no-ops; locate the onDelete handler and
update the logic around activeDeleteWaveRef.current to implement this explicit
branch and fallback.
---
Outside diff comments:
In `@src/screens/editor/container/editorContainer.tsx`:
- Around line 283-291: The bug is that _isSubmitting is set to true before
calling _submitReply for replies, causing the early-return guard in _submitReply
to block the first reply submission; change the logic in the component method
that currently sets this._isSubmitting = true (before the isReply check) so that
_isSubmitting is only set in the edit/new-post code paths (the branches that
show the Alert and then call _submitReply asynchronously) and not in the if
(isReply && !isEdit) branch; leave _submitReply and its internal guard unchanged
so replies rely on their existing duplication protection.
🪄 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
Run ID: 021d221f-da8f-420a-86f8-7e6b0b9e9885
📒 Files selected for processing (5)
src/components/postOptionsModal/container/postOptionsModal.tsxsrc/components/quickPostModal/usePostSubmitter.tssrc/screens/editor/container/editorContainer.tsxsrc/screens/waves/screen/wavesScreen.tsxsrc/utils/editor.ts
Greptile SummaryThis PR fixes several waves editing bugs: duplicate submission guards now release
Confidence Score: 5/5Safe to merge — all previously flagged submit-lock wedging, feed-deleter clobbering, and duplicate-wave issues are addressed with clean, well-commented fixes. All six bugs raised in earlier review rounds are resolved: the HiveAuth dismissal path now releases the submit lock before awaiting; the recursive re-arm is correctly gated on manageSubmittingState; _setScheduledPost is awaited and its failure routes through _handleSubmitFailure; wave permlinks are now content-derived making retries idempotent; and the per-feed delete Map prevents the two simultaneously-mounted feeds from overwriting each other's deleter. No new defects identified. No files require special attention. Important Files Changed
Reviews (11): Last reviewed commit: "fix recur" | Re-trigger Greptile |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/screens/waves/screen/wavesScreen.tsx (1)
395-401:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the handler async to properly propagate errors from the mutation.
PostOptionsModalawaits theonDeletecallback (line 359) and wraps it in a try/catch to show a toast on failure. SincedeleteWaveis an async function (wavesQueries.ts:93) that returns a Promise, calling it withoutawaitmeans any rejection will not be caught by the modal's error handler, leaving the delete silently failed after the modal closes.Proposed fix
- onDelete={(content) => { + onDelete={async (content) => { // Route the options-menu delete through the active feed's // `wavesQuery.deleteWave`, which both broadcasts the delete and // removes the wave from the waves infinite-query cache. The // modal's default path uses `navigation.goBack()` and never // updates the cache, so the wave would stay visible on the feed // even after a successful delete. const deleteWave = activeDeleteWaveRef.current; if (deleteWave) { - deleteWave({ + await deleteWave({ _permlink: content.permlink, _parent_permlink: content.parent_permlink, }); return; }Also update the
onVisibilityChangetype (lines 64-71) to reflect thatdeleteWavereturnsPromise<void>.🤖 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/screens/waves/screen/wavesScreen.tsx` around lines 395 - 401, The delete handler calls the ref'd function without awaiting it so rejections escape the caller; update the handler that invokes activeDeleteWaveRef.current (the local deleteWave invocation) to be async and await deleteWave({...}) so the Promise rejection is propagated to PostOptionsModal's onDelete try/catch, and adjust the onVisibilityChange callback type to return Promise<void> (and any related types) to reflect that deleteWave (from wavesQueries.ts) is async.
🤖 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.
Duplicate comments:
In `@src/screens/waves/screen/wavesScreen.tsx`:
- Around line 395-401: The delete handler calls the ref'd function without
awaiting it so rejections escape the caller; update the handler that invokes
activeDeleteWaveRef.current (the local deleteWave invocation) to be async and
await deleteWave({...}) so the Promise rejection is propagated to
PostOptionsModal's onDelete try/catch, and adjust the onVisibilityChange
callback type to return Promise<void> (and any related types) to reflect that
deleteWave (from wavesQueries.ts) is async.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2342c2b9-cdd4-43ae-946d-78ade8da32eb
📒 Files selected for processing (4)
src/components/postOptionsModal/container/postOptionsModal.tsxsrc/screens/editor/container/editorContainer.tsxsrc/screens/waves/screen/wavesScreen.tsxsrc/utils/editor.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/postOptionsModal/container/postOptionsModal.tsx (1)
670-675: ⚡ Quick winWrap the
case 'copy'declaration in a block.Line 673 declares
const _urldirectly in a switch clause without block scope. While the project's ESLint configuration hasno-case-declarationsdisabled, wrapping the case with braces prevents potential variable hoisting issues and improves code clarity.Proposed fix
case 'copy': + { // Mirror the canonical-form normalization used in `_share` so copied // links go to `/@author/permlink` instead of the legacy // `/<category>/@author/permlink` (which the web 302s anyway). const _url = isWave ? `/@${content.author}/${content.permlink}` : stripCategoryFromPostPath(content.url); await writeToClipboard(getPostUrl(_url)); alertTimer.current = setTimeout(() => { dispatch( toastNotification( intl.formatMessage({ id: 'alert.copied', }), ), ); alertTimer.current = null; }, 300); break; + }🤖 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/postOptionsModal/container/postOptionsModal.tsx` around lines 670 - 675, Wrap the case 'copy' branch in postOptionsModal.tsx in a block to avoid declaring const _url directly inside the switch clause; specifically, surround the case 'copy' logic with { } and move the declaration of _url (which uses isWave, content.author, content.permlink, and stripCategoryFromPostPath(content.url)) inside that block so the constant is scoped locally to the case and prevents hoisting/clarity issues.
🤖 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/postOptionsModal/container/postOptionsModal.tsx`:
- Around line 670-675: Wrap the case 'copy' branch in postOptionsModal.tsx in a
block to avoid declaring const _url directly inside the switch clause;
specifically, surround the case 'copy' logic with { } and move the declaration
of _url (which uses isWave, content.author, content.permlink, and
stripCategoryFromPostPath(content.url)) inside that block so the constant is
scoped locally to the case and prevents hoisting/clarity issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 139a4a84-f2d9-45c8-9843-3dfef1b9a8a4
📒 Files selected for processing (7)
src/components/comments/container/commentsContainer.tsxsrc/components/postOptionsModal/container/postOptionsModal.tsxsrc/components/quickPostModal/usePostSubmitter.tssrc/providers/queries/postQueries/repostQueries.tssrc/screens/editor/container/editorContainer.tsxsrc/utils/post.test.tssrc/utils/post.ts
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements
New Features
Tests