emoji suggestions - #40
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds an end-to-end emoji suggestion workflow. Users can submit, view, and cancel suggestions. Moderators can list, accept, and reject suggestions. The change includes persistence, API contracts, frontend pages, localization, and service tests. ChangesEmoji suggestions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EmojiSuggestionsPage
participant EmojiSuggestionAPI
participant EmojiSuggestionService
participant StorageAndEmojiService
User->>EmojiSuggestionsPage: Submit or act on suggestion
EmojiSuggestionsPage->>EmojiSuggestionAPI: Call suggestion endpoint
EmojiSuggestionAPI->>EmojiSuggestionService: Validate and process request
EmojiSuggestionService->>StorageAndEmojiService: Copy file and create emoji on acceptance
EmojiSuggestionAPI-->>EmojiSuggestionsPage: Return packed suggestion or emoji
EmojiSuggestionsPage-->>User: Update suggestion list
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/frontend/src/pages/about.emojis.vueESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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 |
![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: 7631d717fc
ℹ️ 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".
|
|
||
| // Consume the suggestion before doing any work. This makes acceptance, | ||
| // cancellation, rejection, and another acceptance mutually exclusive. | ||
| const claimed = await this.emojiSuggestionsRepository.delete({ |
There was a problem hiding this comment.
Deleting the row before the potentially slow uploadFromUrl operation releases both unique indexes. If another create request for the same file or name arrives before the emoji is inserted, it passes both duplicate checks and leaves a pending suggestion for an emoji that was just accepted; if acceptance instead fails, restoreSuggestion() can collide with that new row and mask the original error. Claim the row without releasing its uniqueness reservations until acceptance has completed.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (2)
packages/backend/src/core/EmojiSuggestionService.ts (1)
134-153: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffConsider a transaction for the claim-and-restore sequence.
The claim deletes the row, and
restoreSuggestionre-inserts it through a separate statement. A process crash or a connection loss between the two leaves the suggestion permanently deleted while no emoji exists. ASELECT ... FOR UPDATEinside a transaction, or a status column instead of a delete, would make the claim recoverable.The current design is defensible for a moderation queue. Treat this as a durability trade-off to record, not a blocker.
🤖 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 `@packages/backend/src/core/EmojiSuggestionService.ts` around lines 134 - 153, Document the durability trade-off around the claim-and-restore sequence in the relevant method of EmojiSuggestionService, noting that the separate delete and restoreSuggestion insert can permanently lose the suggestion after a crash or connection failure. Record that a transaction with row locking or a status-column claim would make it recoverable, without changing the current moderation-queue behavior.packages/backend/test/unit/EmojiSuggestionService.ts (1)
89-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining
createguard branches.The tests cover
noSuchFileandduplicateSuggestion. Three guards stay untested:unsupportedFileTypeon Line 78,duplicateNameon Line 79, andtooManyPendingSuggestionson Line 90 ofpackages/backend/src/core/EmojiSuggestionService.ts. ThetooManyPendingSuggestionsboundary matters because the check uses>=againstMAX_PENDING_EMOJI_SUGGESTIONS.rejectalso has no test.I can generate these cases if you want them.
🤖 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 `@packages/backend/test/unit/EmojiSuggestionService.ts` around lines 89 - 124, Add tests for the remaining EmojiSuggestionService.create guards: unsupportedFileType, duplicateName, and tooManyPendingSuggestions, including the exact MAX_PENDING_EMOJI_SUGGESTIONS boundary, plus a reject test covering its expected behavior. Reuse the existing createService fixtures and assertions to verify each domain result and that persistence is not performed when appropriate.
🤖 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 `@packages/backend/migration/1786507200000-EmojiSuggestion.js`:
- Line 16: Add the missing semicolon to the name assignment in the
EmojiSuggestion1786507200000 migration, matching the semicolon style used by the
other migrations.
In `@packages/backend/src/core/EmojiSuggestionService.ts`:
- Around line 192-214: Update the catch block in EmojiSuggestionService to
preserve the original createEmoji error while isolating failures from deleteFile
and restoreSuggestion cleanup. Ensure both cleanup operations are attempted
without replacing the caught error, then allow isDuplicateKeyValueError(error)
to map duplicate failures to duplicateName and rethrow the original error
otherwise; use the service’s injected logger, adding LoggerService if needed, to
record cleanup failures.
In `@packages/frontend/src/pages/about.emojis.vue`:
- Around line 8-11: Update the custom emoji manager button condition in the
about page to include $i.isAdmin, matching the access condition used in
common.ts while preserving the existing moderator and canManageCustomEmojis
checks.
---
Nitpick comments:
In `@packages/backend/src/core/EmojiSuggestionService.ts`:
- Around line 134-153: Document the durability trade-off around the
claim-and-restore sequence in the relevant method of EmojiSuggestionService,
noting that the separate delete and restoreSuggestion insert can permanently
lose the suggestion after a crash or connection failure. Record that a
transaction with row locking or a status-column claim would make it recoverable,
without changing the current moderation-queue behavior.
In `@packages/backend/test/unit/EmojiSuggestionService.ts`:
- Around line 89-124: Add tests for the remaining EmojiSuggestionService.create
guards: unsupportedFileType, duplicateName, and tooManyPendingSuggestions,
including the exact MAX_PENDING_EMOJI_SUGGESTIONS boundary, plus a reject test
covering its expected behavior. Reuse the existing createService fixtures and
assertions to verify each domain result and that persistence is not performed
when appropriate.
🪄 Autofix
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: 83bf692c-0e74-4a66-8e80-b50b3b4a2c65
📒 Files selected for processing (29)
locales/index.d.tspackages/backend/migration/1786507200000-EmojiSuggestion.jspackages/backend/src/core/CoreModule.tspackages/backend/src/core/EmojiSuggestionService.tspackages/backend/src/core/entities/EmojiSuggestionEntityService.tspackages/backend/src/di-symbols.tspackages/backend/src/misc/json-schema.tspackages/backend/src/models/EmojiSuggestion.tspackages/backend/src/models/RepositoryModule.tspackages/backend/src/models/_.tspackages/backend/src/models/json-schema/emoji-suggestion.tspackages/backend/src/postgres.tspackages/backend/src/server/api/emoji-suggestion.tspackages/backend/src/server/api/endpoint-list.tspackages/backend/src/server/api/endpoints/admin/emoji-suggestions/accept.tspackages/backend/src/server/api/endpoints/admin/emoji-suggestions/list.tspackages/backend/src/server/api/endpoints/admin/emoji-suggestions/reject.tspackages/backend/src/server/api/endpoints/emoji-suggestions/cancel.tspackages/backend/src/server/api/endpoints/emoji-suggestions/create.tspackages/backend/src/server/api/endpoints/emoji-suggestions/list.tspackages/backend/test/unit/EmojiSuggestionService.tspackages/frontend/src/pages/about.emojis.vuepackages/frontend/src/pages/emoji-edit-dialog.vuepackages/frontend/src/pages/emoji-suggestions.vuepackages/frontend/src/router.definition.tspackages/frontend/src/ui/_common_/common.tspackages/misskey-js/src/api.types.tspackages/misskey-js/src/entities.tssharkey-locales/en-US.yml
1 participant
What
Why
Additional info (optional)
Checklist
Summary by CodeRabbit