Skip to content

Archiving feedback#874

Merged
tudddorrr merged 1 commit intodevelopfrom
archived-feedback
Mar 1, 2026
Merged

Archiving feedback#874
tudddorrr merged 1 commit intodevelopfrom
archived-feedback

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Mar 1, 2026
@tudddorrr tudddorrr force-pushed the archived-feedback branch from 00c9459 to 4d21988 Compare March 1, 2026 21:05
@claude
Copy link

claude bot commented Mar 1, 2026

PR Review: Archiving Feedback

Overall the implementation is clean and well-tested. One bug worth fixing before merge.


🟡 Potential Bugs

Duplicate activity log entries on repeated calls

toggle-archived.ts unconditionally creates an activity log entry without checking whether the state is actually changing. If a client sends { archived: true } on feedback that is already archived, deletedAt gets updated to a fresh timestamp and a new GAME_FEEDBACK_ARCHIVED activity is appended to the audit log. Same applies in reverse for restore.

// toggle-archived.ts handler - no guard before logging
feedback.deletedAt = archived ? new Date() : null

createGameActivity(em, {
  type: archived
    ? GameActivityType.GAME_FEEDBACK_ARCHIVED
    : GameActivityType.GAME_FEEDBACK_RESTORED,
  ...
})

Fix: bail out early (or skip the activity) when state is unchanged:

const alreadyArchived = feedback.deletedAt !== null

if (archived === alreadyArchived) {
  return { status: 200, body: { feedback } }
}

🔵 Minor

andWhere({}) as a no-op is non-obvious

.andWhere(withDeleted ? {} : { deletedAt: null })

Passing an empty object to andWhere relies on MikroORM silently ignoring it. A conditional call is clearer and removes the implicit assumption:

if (!withDeleted) {
  query.andWhere({ deletedAt: null })
}

Test Coverage

No issues — the new test file covers permissions, not-found, archive, restore, anonymised players, and activity log content. The list tests for the withDeleted flag are thorough.

@codecov
Copy link

codecov bot commented Mar 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.03%. Comparing base (21d756a) to head (a4238d6).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #874      +/-   ##
===========================================
+ Coverage    97.00%   97.03%   +0.02%     
===========================================
  Files          388      389       +1     
  Lines         6139     6163      +24     
  Branches       785      790       +5     
===========================================
+ Hits          5955     5980      +25     
+ Misses          95       94       -1     
  Partials        89       89              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tudddorrr tudddorrr force-pushed the archived-feedback branch from 4d21988 to f15e83a Compare March 1, 2026 21:29
@tudddorrr tudddorrr force-pushed the archived-feedback branch from f15e83a to a4238d6 Compare March 1, 2026 21:56
@tudddorrr tudddorrr merged commit e484a89 into develop Mar 1, 2026
10 checks passed
@tudddorrr tudddorrr deleted the archived-feedback branch March 1, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant