emoji suggestions - #39
Conversation
i/update wrote to the user_profile table but only emitted 'userUpdated', which purges userByIdCache alone. The userProfileCache entry (populated at the start of the same request) was never invalidated - not even in the local process - so the endpoint returned stale profile data and all workers kept serving old settings until the 30-minute cache lifetime expired. Every other profile-writing call site already emits 'updateUserProfile'; this was the one exception. This also un-breaks federation of profile-only changes: the stale re-fetch made profileNeedsPublishing() compare the cached object against itself, so bio/location/birthday/fields edits were never delivered to followers unless a user-table field changed too. Also: - compare 'fields' structurally in profileNeedsPublishing; Array.join rendered every element as "[object Object]" so changes were invisible - emit 'updateUserProfile' before packing the response in i/update-email (and include the email keys), so the response reflects the new data - correct a backwards comment in CacheService.onProfileEvent https://claude.ai/code/session_01AB5H3dSqWnemYD2EjYRZkp
The randomized refresh delay was written as 10_1000 (101,000 ms), an apparent typo for 10_000. Stale cached instance meta could persist for over a minute and a half after page load before the async refresh ran. https://claude.ai/code/session_01AB5H3dSqWnemYD2EjYRZkp
The "active user" refactor (126d7e2) made isActiveLocalUser() require user.approved unconditionally, while its sibling assertActiveLocalUser() and the signin check only enforce approval when meta.approvalRequiredForSignup is enabled. Local accounts created before the approval-signup migration (1697580470000) default to approved=false and were never backfilled, so on instances that don't require approval, every such account started returning 404 from /@user.atom, /@user.rss, and /@user.json (and other isActiveUser-gated routes like WebFinger and user SSR). Align isActiveLocalUser() with assertActiveLocalUser(): only treat an unapproved user as inactive when the instance actually requires approval for signup. https://claude.ai/code/session_01ALjqseHZ7wvaepdXwBko4d
Both endpoints wrote to the users table but neither injected InternalEventService nor emitted an invalidation event, leaving userByIdCache stale for up to 5 minutes after a moderator action. Also fix a pre-existing missing await on moderationLogService.log() in unset-user-banner, which could silently swallow audit log write errors.
… activities attachLdSignature() called this.jsonLdService.signRsaSignature2017(...) directly, but JsonLdService only exposes use(), which returns the JsonLd instance that actually implements signRsaSignature2017. This caused "this.jsonLdService.signRsaSignature2017 is not a function" when signing activities delivered to relays. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JzqoS7cjvodFiWW6qZxyXL
Fix jsonLdService method call in ApRendererService
Fixes all findings from the Lua MRF review: P0 wasm stack corruption, decision loss on snapshot failure, unsanitized rewrites, missing filter validation, DI singleton, structured source errors, moderation logs, policy caching, unlist/host-prefix bugs, typing and response schemas.
WalkthroughAdds emoji suggestion persistence, moderation services, six API endpoints, Misskey client types, frontend submission and review pages, navigation, routing, and English localization. ChangesEmoji suggestions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor Proposer
participant EmojiSuggestionsPage
participant EmojiSuggestionAPI
participant EmojiSuggestionService
actor Moderator
participant EmojiEntityService
Proposer->>EmojiSuggestionsPage: Submit emoji suggestion
EmojiSuggestionsPage->>EmojiSuggestionAPI: Call emoji-suggestions/create
EmojiSuggestionAPI->>EmojiSuggestionService: Validate and store suggestion
EmojiSuggestionService-->>EmojiSuggestionsPage: Return packed suggestion
Moderator->>EmojiSuggestionsPage: Accept suggestion
EmojiSuggestionsPage->>EmojiSuggestionAPI: Call admin/emoji-suggestions/accept
EmojiSuggestionAPI->>EmojiSuggestionService: Claim and create emoji
EmojiSuggestionService->>EmojiEntityService: Create accepted emoji
EmojiEntityService-->>Moderator: Return packed emoji
🚥 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. packages/frontend/src/pages/emoji-edit-dialog.vueESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/frontend/src/pages/emoji-suggestions.vueESLint skipped: the ESLint configuration for this file 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: d514a6d52b
ℹ️ 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".
| const claimed = await this.emojiSuggestionsRepository.delete({ | ||
| id: suggestion.id, | ||
| userId: suggestion.userId, | ||
| fileId: suggestion.fileId, | ||
| }); |
There was a problem hiding this comment.
Deleting the row here releases the unique name constraint before the file copy and emoji creation finish. If another user submits the same name during that interval, the new suggestion succeeds; a successful acceptance then leaves that duplicate pending even though the emoji now exists, while a failed acceptance cannot restore the original because restoreSuggestion() hits the new row's unique constraint. Use a durable claimed/status state that continues to reserve the name until acceptance commits or rolls back.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/backend/test/unit/EmojiSuggestionService.ts (1)
89-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining decision branches.
The suite covers the concurrency paths well. These branches remain untested:
unsupportedFileTypeandduplicateNameincreate.tooManyPendingSuggestionsat theMAX_PENDING_EMOJI_SUGGESTIONSboundary.- NFC normalization of
name,category, andaliases.cancelfor the owner and for a non-owner, which asserts theuserIdscoping in the delete criteria.reject.Do you want me to generate these tests?
🤖 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 - 239, Add unit tests in the EmojiSuggestionService suite for the remaining decision branches: verify create returns unsupportedFileType and duplicateName, enforces tooManyPendingSuggestions at MAX_PENDING_EMOJI_SUGGESTIONS, and NFC-normalizes name, category, and aliases; also cover cancel success for the owner and failure for a non-owner with userId-scoped delete criteria, plus the reject flow.
🤖 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`:
- Around line 15-17: Add the required semicolon to the name property declaration
in the EmojiSuggestion1786507200000 class, satisfying the `@stylistic/semi` lint
rule without changing its value or surrounding migration logic.
In `@packages/backend/src/core/EmojiSuggestionService.ts`:
- Around line 132-153: Replace the delete-then-restore flow in the suggestion
acceptance path with durable status-based claiming: add and use a
pending/accepted/rejected status, conditionally update the row from pending when
claiming, and retain it until emoji creation succeeds. Update the success and
rejection paths to persist the corresponding status instead of deleting the
suggestion, preserving the audit record and avoiding reliance on
restoreSuggestion after crashes or failed compensation.
- Around line 203-207: Update the cleanup flow around driveService.deleteFile
and restoreSuggestion so cleanup failures are caught, logged through an injected
logger, and not rethrown, preserving the original error for propagation at Line
213. Add the logger dependency using the service’s existing dependency-injection
conventions and log failures from both cleanup operations with relevant context.
- Around line 81-91: Update the duplicate check in EmojiSuggestionService using
emojiSuggestionsRepository.exists so the name condition is evaluated globally
rather than combined with userId. Preserve the fileId condition for
file-specific duplicates and continue returning duplicateSuggestion when either
check matches.
In `@packages/backend/src/server/api/endpoints/emoji-suggestions/create.ts`:
- Around line 36-45: Update EmojiSuggestionService.create to enforce the
per-user pending-suggestion limit atomically: perform the count check and
suggestion insert within the same transaction while locking the relevant
user/count state, or use an equivalent database-enforced reservation. Preserve
the existing limit behavior while preventing concurrent requests from exceeding
it; the endpoint handler should continue delegating through create.
---
Nitpick comments:
In `@packages/backend/test/unit/EmojiSuggestionService.ts`:
- Around line 89-239: Add unit tests in the EmojiSuggestionService suite for the
remaining decision branches: verify create returns unsupportedFileType and
duplicateName, enforces tooManyPendingSuggestions at
MAX_PENDING_EMOJI_SUGGESTIONS, and NFC-normalizes name, category, and aliases;
also cover cancel success for the owner and failure for a non-owner with
userId-scoped delete criteria, plus the reject flow.
🪄 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: db7d6e1d-a216-4645-9522-67778ed7a4a5
📒 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
| export class EmojiSuggestion1786507200000 { | ||
| name = 'EmojiSuggestion1786507200000' | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing semicolon.
ESLint reports @stylistic/semi at Line 16. Lint will fail in CI.
🔧 Proposed fix
- name = 'EmojiSuggestion1786507200000'
+ name = 'EmojiSuggestion1786507200000';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class EmojiSuggestion1786507200000 { | |
| name = 'EmojiSuggestion1786507200000' | |
| export class EmojiSuggestion1786507200000 { | |
| name = 'EmojiSuggestion1786507200000'; |
🧰 Tools
🪛 ESLint
[error] 16-17: Missing semicolon.
(@stylistic/semi)
🤖 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/migration/1786507200000-EmojiSuggestion.js` around lines 15
- 17, Add the required semicolon to the name property declaration in the
EmojiSuggestion1786507200000 class, satisfying the `@stylistic/semi` lint rule
without changing its value or surrounding migration logic.
Source: Linters/SAST tools
| const [pendingCount, duplicateSuggestion] = await Promise.all([ | ||
| this.emojiSuggestionsRepository.countBy({ userId: user.id }), | ||
| this.emojiSuggestionsRepository.exists({ | ||
| where: [ | ||
| { userId: user.id, name }, | ||
| { fileId: file.id }, | ||
| ], | ||
| }), | ||
| ]); | ||
| if (pendingCount >= MAX_PENDING_EMOJI_SUGGESTIONS) return { ok: false, reason: 'tooManyPendingSuggestions' }; | ||
| if (duplicateSuggestion) return { ok: false, reason: 'duplicateSuggestion' }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the pending name globally to return the correct reason.
IDX_emoji_suggestion_name is globally unique, but the preflight query scopes the name to userId. If another user already has a pending suggestion for the same name, the insert violates the constraint and the caller receives duplicateSuggestion, which describes their own duplicate submission. Scope the name check to the name only.
🔧 Proposed fix
const [pendingCount, duplicateSuggestion] = await Promise.all([
this.emojiSuggestionsRepository.countBy({ userId: user.id }),
this.emojiSuggestionsRepository.exists({
where: [
- { userId: user.id, name },
+ { name },
{ fileId: file.id },
],
}),
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [pendingCount, duplicateSuggestion] = await Promise.all([ | |
| this.emojiSuggestionsRepository.countBy({ userId: user.id }), | |
| this.emojiSuggestionsRepository.exists({ | |
| where: [ | |
| { userId: user.id, name }, | |
| { fileId: file.id }, | |
| ], | |
| }), | |
| ]); | |
| if (pendingCount >= MAX_PENDING_EMOJI_SUGGESTIONS) return { ok: false, reason: 'tooManyPendingSuggestions' }; | |
| if (duplicateSuggestion) return { ok: false, reason: 'duplicateSuggestion' }; | |
| const [pendingCount, duplicateSuggestion] = await Promise.all([ | |
| this.emojiSuggestionsRepository.countBy({ userId: user.id }), | |
| this.emojiSuggestionsRepository.exists({ | |
| where: [ | |
| { name }, | |
| { fileId: file.id }, | |
| ], | |
| }), | |
| ]); | |
| if (pendingCount >= MAX_PENDING_EMOJI_SUGGESTIONS) return { ok: false, reason: 'tooManyPendingSuggestions' }; | |
| if (duplicateSuggestion) return { ok: false, reason: 'duplicateSuggestion' }; |
🤖 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 81 - 91,
Update the duplicate check in EmojiSuggestionService using
emojiSuggestionsRepository.exists so the name condition is evaluated globally
rather than combined with userId. Preserve the fileId condition for
file-specific duplicates and continue returning duplicateSuggestion when either
check matches.
| // Consume the suggestion before doing any work. This makes acceptance, | ||
| // cancellation, rejection, and another acceptance mutually exclusive. | ||
| const claimed = await this.emojiSuggestionsRepository.delete({ | ||
| id: suggestion.id, | ||
| userId: suggestion.userId, | ||
| fileId: suggestion.fileId, | ||
| }); | ||
| if (claimed.affected !== 1) return { ok: false, reason: 'noSuchSuggestion' }; | ||
|
|
||
| const restoreSuggestion = async () => { | ||
| await this.emojiSuggestionsRepository.insert({ | ||
| id: suggestion.id, | ||
| userId: suggestion.userId, | ||
| fileId: suggestion.fileId, | ||
| name: suggestion.name, | ||
| category: suggestion.category, | ||
| aliases: suggestion.aliases, | ||
| license: suggestion.license, | ||
| localOnly: suggestion.localOnly, | ||
| isSensitive: suggestion.isSensitive, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The delete-then-restore compensation can lose suggestions permanently.
The claim deletes the row before any external work starts. Recovery depends on restoreSuggestion running in the same process. If the process crashes, the database connection drops, or the compensating insert fails, the suggestion is gone and no emoji exists. The proposer loses the submission with no record.
Two durable alternatives:
- Add a status column (
pending/accepted/rejected) and claim by a conditionalUPDATE ... WHERE status = 'pending'. Delete or keep the row only after the emoji creation succeeds. - Wrap the claim and the emoji insert in one transaction with
SELECT ... FOR UPDATEon the suggestion row, and move the Drive copy before the transaction.
The first option also preserves an audit trail of moderation decisions.
🤖 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 132 - 153,
Replace the delete-then-restore flow in the suggestion acceptance path with
durable status-based claiming: add and use a pending/accepted/rejected status,
conditionally update the row from pending when claiming, and retain it until
emoji creation succeeds. Update the success and rejection paths to persist the
corresponding status instead of deleting the suggestion, preserving the audit
record and avoiding reliance on restoreSuggestion after crashes or failed
compensation.
| try { | ||
| await this.driveService.deleteFile(emojiFile, false, moderator); | ||
| } finally { | ||
| await restoreSuggestion(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve the original failure when cleanup fails.
If driveService.deleteFile throws, its error propagates out of the catch block and replaces error. The same happens if restoreSuggestion throws inside finally. The root cause of the acceptance failure is then lost, and no logger records it.
Swallow and log the cleanup errors so the original error reaches Line 213.
🔧 Proposed fix
- try {
- await this.driveService.deleteFile(emojiFile, false, moderator);
- } finally {
- await restoreSuggestion();
- }
+ try {
+ await this.driveService.deleteFile(emojiFile, false, moderator);
+ } catch (cleanupError) {
+ this.logger.error('Failed to delete the emoji file copy after a failed acceptance', { cleanupError });
+ }
+ try {
+ await restoreSuggestion();
+ } catch (restoreError) {
+ this.logger.error('Failed to restore the emoji suggestion after a failed acceptance', { restoreError });
+ }This requires an injected logger. Do you want me to add one?
🤖 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 203 - 207,
Update the cleanup flow around driveService.deleteFile and restoreSuggestion so
cleanup failures are caught, logged through an injected logger, and not
rethrown, preserving the original error for propagation at Line 213. Add the
logger dependency using the service’s existing dependency-injection conventions
and log failures from both cleanup operations with relevant context.
| super(meta, paramDef, async (ps, me) => { | ||
| const result = await this.emojiSuggestionService.create({ | ||
| name: ps.name, | ||
| fileId: ps.fileId, | ||
| category: ps.category ?? null, | ||
| aliases: ps.aliases ?? [], | ||
| license: ps.license ?? null, | ||
| isSensitive: ps.isSensitive ?? false, | ||
| localOnly: ps.localOnly ?? false, | ||
| }, me); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Enforce the pending-suggestion limit atomically.
EmojiSuggestionService.create counts existing rows before it inserts the suggestion. Concurrent requests can all pass the limit check before any insert completes. A user with nine pending suggestions can then create more than ten distinct suggestions.
Lock the per-user count and insert in one transaction, or use a database-enforced reservation mechanism. The endpoint rate limit does not prevent this race.
🤖 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/server/api/endpoints/emoji-suggestions/create.ts` around
lines 36 - 45, Update EmojiSuggestionService.create to enforce the per-user
pending-suggestion limit atomically: perform the count check and suggestion
insert within the same transaction while locking the relevant user/count state,
or use an equivalent database-enforced reservation. Preserve the existing limit
behavior while preventing concurrent requests from exceeding it; the endpoint
handler should continue delegating through create.
2 participants
What
Why
Additional info (optional)
Checklist
Summary by CodeRabbit