Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr force-pushed the remove-socket-events branch from 993392f to 6a50fe0 Compare December 4, 2025 08:31
@claude
Copy link

claude bot commented Dec 4, 2025

PR Review: Remove socket events table, delete unfinished sessions

Overall, this is a clean refactor that simplifies the codebase by removing socket event tracking. The changes are well-structured with proper test updates.


🔴 Critical

Missing migration to drop socket_events table

The PR removes the CreateSocketEventsTable migration from the list but does not add a new migration to drop the table for existing installations. This will leave orphaned tables in ClickHouse for users upgrading from previous versions.

Fix: Add a new migration (e.g., 007DropSocketEventsTable.ts):

export const DropSocketEventsTable = `DROP TABLE IF EXISTS ${process.env.CLICKHOUSE_DB}.socket_events;`

And register it in src/migrations/clickhouse/index.ts after the existing migrations.

Location: src/migrations/clickhouse/index.ts


🟡 Major

No issues found.


🔵 Minor

Incomplete comment in cleanupOnlinePlayers.ts

Line 81 has a TODO comment that could be more descriptive about what needs investigation.

// todo, find out how this is happening

Consider updating to:

// TODO: Investigate why PlayerPresence records exist with null player references

Location: src/tasks/cleanupOnlinePlayers.ts:81


Code Quality and Best Practices

No issues found.


Performance Considerations

No issues found. The simplified logic in cleanupOnlinePlayers that directly deletes old sessions is actually more performant than the previous complex session reconstruction.


Security Concerns

No issues found.


Test Coverage

The test changes appropriately reflect the simplified functionality. The removal of socketEvents.test.ts is correct since the feature is being removed entirely.

@tudddorrr tudddorrr force-pushed the remove-socket-events branch from 6a50fe0 to 20a9898 Compare December 4, 2025 08:34
@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.20%. Comparing base (8fb5c4e) to head (1797b2e).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #722      +/-   ##
===========================================
- Coverage    99.21%   99.20%   -0.01%     
===========================================
  Files          225      223       -2     
  Lines        16621    16473     -148     
  Branches      1533     1507      -26     
===========================================
- Hits         16490    16342     -148     
- Misses         128      129       +1     
+ Partials         3        2       -1     

☔ 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 remove-socket-events branch from 20a9898 to d2ea985 Compare December 4, 2025 08:47
@tudddorrr tudddorrr force-pushed the remove-socket-events branch from d2ea985 to 1797b2e Compare December 4, 2025 09:02
@tudddorrr tudddorrr merged commit 360c830 into develop Dec 4, 2025
5 of 6 checks passed
@tudddorrr tudddorrr deleted the remove-socket-events branch December 4, 2025 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants