Add option to send replies as original author replica#105
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
db978e1 to
d9b26b1
Compare
|
I have read the CLA Document and I hereby sign the CLA |
Kyrela
left a comment
There was a problem hiding this comment.
Thanks for this pull request. I haven't tested it yet, but I have a couple issues with the code.
Don't hesitate to debate/argue with me if you don't agree with some of my comments, I'm open to discussion.
On another note, did you fully vibe-coded it, and/or are you a developper? More details about the implication of AI in this PR would be appreciated.
There was a problem hiding this comment.
The PR is missing the migration file related to this change
| name: "Reply method" | ||
| description: "Change the behavior on the reply" | ||
| content: "**Change what to do on the reply**\n- %{state}\n- %{silent}%{perms}" | ||
| content: "**Change what to do on the reply**\n- %{state}\n- %{silent}\n- %{avatar}%{perms}" |
There was a problem hiding this comment.
Why "avatar"? That isn't related to the avatar by any mean
| "false": "🔔 Send with a notification" | ||
| original_sender_profile: | ||
| button: | ||
| "true": "Send as Original Sender Profile" |
There was a problem hiding this comment.
"original sender profile" doesn't really makes sense imo. Specifially the "profile" part.
"Sending as fake user/author" would be better imo, but, to be closer with what you wrote, "send as original sender/author replica" might also be a solution.
In any case, any change do the naming here have to be mirrored across every symbols of the PR.
| original_sender_profile: | ||
| button: | ||
| "true": "Send as Original Sender Profile" | ||
| "false": "Send as Bot Profile" |
There was a problem hiding this comment.
Same here : "Send as FixTweet" (with, of course, "FixTweet" as determined by the bot's name) seems more adapted.
| if not guild.reply_as_original_sender_profile: | ||
| async with Typing(channel): | ||
| rendered_links = [link for link in links if await link.render()] | ||
| if not rendered_links: | ||
| return | ||
| not_sent, messages = await send_fixed_links(rendered_links, guild, original_message) | ||
| else: |
There was a problem hiding this comment.
Why disabling typing when faking user? It's still a working indicator, so it's kind of important.
Also, If typing had to be disabled, duplicating the code isn't the proper solution.
There was a problem hiding this comment.
It seems to be completely missing the troubleshoot permission update
| self.ctx.guild.update({'reply_silently': self.reply_silently}) | ||
| await view.refresh(interaction) | ||
|
|
||
| async def toggle_reply_as_original_sender_profile(self, view: SettingsView, interaction: discore.Interaction, _) -> None: |
There was a problem hiding this comment.
As webhooks can't reply, enabling webhook send should disable the reply method option, both visually and by setting it to "send" in the DB.
| return links_failed, messages_sent | ||
|
|
||
|
|
||
| async def get_or_create_webhook(channel: discore.TextChannel | discore.Thread) -> discore.Webhook | None: |
There was a problem hiding this comment.
the typehint for channel isn't good ; this can be a lot more that a TextChannel or a Thread
| try: | ||
| webhooks = await webhook_channel.webhooks() | ||
| except discore.HTTPException: | ||
| _logger.exception("Failed to fetch webhooks") | ||
| return None |
There was a problem hiding this comment.
HTTPException is too broad and catching an error like this hides it. There should be a narrower error handling or no error handling at all, allowing the global error handler to catch it and report it properly.
| if w.name == WEBHOOK_NAME and getattr(w.user, 'id', None) == channel.guild.me.id | ||
| ), None) | ||
| if webhook is not None: | ||
| return webhook | ||
| sent, webhook = await safe_send_coro(webhook_channel.create_webhook(name=WEBHOOK_NAME), forbidden=True) |
There was a problem hiding this comment.
WEBHOOK_NAME shouldn't be a variable. Instead, the bot's display name should be used, to avoid confusion and incoherent branding across unofficial instances.
|
Thanks for the review! I’m a UI/UX designer, not a developer. I can read code to some extent, but most of this was vibe-coded with Codex and then adjusted while testing locally. I agree with most of your points, and I’ll update the PR. For the typing indicator: I disabled it in webhook mode because the message is sent by a webhook identity instead of the bot user. In my local test, Discord kept showing the bot as typing for a while even after the webhook message had already been sent. |
d9b26b1 to
0f1537d
Compare
0f1537d to
451ca15
Compare
Summary
Adds a new Reply Method option to send fixed-link replies as a replica of the original message author instead of the bot.
This was written with Codex, then tested locally.
Context
Reference: Discord report
https://discord.com/channels/1160873160665731134/1215363563791585350
Changes
Send as Original Author Replica/Send as %{bot}toggleManage Webhookspermission text, including troubleshooting permissionsTested
Tested locally after adding the setting column to the local test database.