Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the ability to nuke all comments of a post in comment-nuke app #157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ni5arga
Copy link

@ni5arga ni5arga commented Jan 16, 2025

💸 TL;DR

Added the ability to remove & lock all comments of a post by introducing the handleNukePost function.

handleNukePost allows a moderator to lock or remove all comments in a post. It fetches a post using the provided post ID, checks if the current user has the necessary mod permissions, and then proceeds to lock or remove all comments in the post based on the provided properties. If the skipDistinguished property is true, distinguished comments are skipped. The function logs any failed attempts to lock or remove comments and returns a success message along with the number of comments handled.

Also added a separate form - nukePostForm in main.ts for this purpose.

Created this PR after discussing with @pl00h on Discord, the old PR #5 was closed due to changes in the repository structure.

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee

Signed-off-by: Nisarga <[email protected]>
Signed-off-by: Nisarga <[email protected]>
subredditId: string;
};

async function getAllCommentsInPost(
Copy link
Member

Choose a reason for hiding this comment

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

consider using a generator here. the current approach will attempt to get all comments. if there are too many comments, the platform will time out before any comments can be deleted and retries will likely have the same issue.

something like this untested code:

/** yield all replies under a comment depth-first excluding the root. */
async function* getAllCommentsInThread(comment: Comment): AsyncGenerator<Comment> {
  const replies = await comment.replies.all();
  for (const reply of replies) {
    yield* getAllCommentsInThread(reply);
    yield reply;
  }
}

// fetch comments one level at a time and print depth first.
for await (const reply of getAllCommentsInThread(someComment)) {
  console.log(reply.approve())
}
console.log(someComment)

Comment on lines +37 to +43
const shouldLock = props.lock;
const shouldRemove = props.remove;
const skipDistinguished = props.skipDistinguished;

if (!shouldLock && !shouldRemove) {
return { message: 'You must select either lock or remove.', success: false };
}
Copy link
Member

Choose a reason for hiding this comment

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

consider moving the input validation and error handling to the call site. here are the benefits I see:

  1. breaks up the responsibilities of this function which is a little long.
  2. moves user input handling closer to where input is collected; this reduces the chance of user input getting further into the program.
  3. makes the rest of the code reusable by the program without mimicking user input.
  4. allows the caller to decide how to handle errors.
  5. keeps the try block small clarifying failure handling intent (everything, in this case) without costing an indent.
  6. allows more flexible typing. in this case, we might ask for a parameter like op: 'Lock' | 'Remove' and then it makes lock AND remove impossible for the rest of the function if that's wanted.

Comment on lines +46 to +47
const post = await context.reddit.getPostById(props.postId);
const user = await context.reddit.getCurrentUser();
Copy link
Member

Choose a reason for hiding this comment

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

🔕 I think these can run in parallel if you like:

const [user, post] = await Promise.all([
  context.reddit.getCurrentUser(),
  context.reddit.getPostById(props.postId)
])


const allComments = await getAllCommentsInPost(post);
const comments = allComments.filter(
(eachComment: any) => !(skipDistinguished && eachComment.isDistinguished())
Copy link
Member

Choose a reason for hiding this comment

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

we try hard to avoid any because it disables typechecking. I think the type here should infer as Comment if we don't force any.

}

const allComments = await getAllCommentsInPost(post);
const comments = allComments.filter(
Copy link
Member

Choose a reason for hiding this comment

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

I think we can move the conditional outside of the loop like const actionableComments = skipDistinguished ? allComments.filter(comment => !comment.isDistinguished()) : comments. are we supposed to preserve the subtrees of distinguished comments? if so, consider moving the check into getAllCommentsInPost(post, skipDistinguished).

const verbage =
shouldLock && shouldRemove ? 'removed and locked' : shouldLock ? 'locked' : 'removed';

if (shouldRemove) {
Copy link
Member

Choose a reason for hiding this comment

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

this block here looks beefy enough to be its own function. maybe call it removeComment() or something like that?

Comment on lines +12 to +13
remove: boolean;
lock: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

can a comment be both removed AND locked? I saw the conditional requiring at least one or the other but what if both are active?

} ${verbage}! Refresh the page to see the cleanup.`;

const finishTime = new Date();
const timeElapsed = (finishTime as any) - (startTime as any);
Copy link
Member

Choose a reason for hiding this comment

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

I think we can hold use Date.now() instead to get the toal number of milliseconds to avoid a cast down here.

const finishTime = new Date();
const timeElapsed = (finishTime as any) - (startTime as any);
console.info(`${comments.length} comment(s) handled in ${timeElapsed / 1000} seconds.`);
} catch (err: any) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need any typing for the caught error? if so, I think we can use unknown.

} catch (err: any) {
success = false;
message = 'Mop failed! Please try again later.';
console.error(`${err.toString()}`);
Copy link
Member

Choose a reason for hiding this comment

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

suggest just console.error(err) to include the stacktrace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants