Feature/sort facts by value#462
Merged
gabber235 merged 4 commits intoMay 24, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCommands now pair readable facts with per-target FactData, sort inspect and query outputs by descending FactData.value, and render facts using a new FactData-aware formatting overload. ChangesFact Value Sorting and Formatting
sequenceDiagram
participant TypewriterCommand
participant ReadableFactEntry
participant FactData
TypewriterCommand->>ReadableFactEntry: call readForPlayersGroup(target)
ReadableFactEntry->>FactData: return FactData (value, lastUpdate)
TypewriterCommand->>TypewriterCommand: build (entry, FactData) list and sort by value desc
TypewriterCommand->>ReadableFactEntry: call format(player, FactData) for rendering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt (1)
332-333: ⚖️ Poor tradeoffPotential performance hit from repeated
readForPlayersGroupcalls during sorting (TypewriterCommand.kt ~332-333 and ~349-350)
ReadableFactEntry.readForPlayersGroupdelegates toreadForGroup; the defaultreadForGroupcan iterate the wholegroup.playersand callreadSinglePlayerper member, then sum values—so repeated selector calls duringsortedByDescendingcan multiply that cost by the sort’s comparison behavior.- If the facts involved implement
CachableFactEntry,readForGroupis backed byFactDatabase’s in-memoryConcurrentHashMap, making repeated reads much cheaper.fact.format(target)callsreadForPlayersGroupagain per displayed fact, so some duplication remains.⚡ Optional optimization if facts aren’t reliably `CachableFactEntry` (sort-key precompute)
For inspect command (~332-333):
- val facts = page().entries.filterIsInstance<ReadableFactEntry>() - .sortedByDescending { it.readForPlayersGroup(target).value } + val facts = page().entries.filterIsInstance<ReadableFactEntry>() + .map { it to it.readForPlayersGroup(target).value } + .sortedByDescending { it.second } + .map { it.first }For query/list command (~349-350):
- val factEntries = Query.find<ReadableFactEntry>().toList() - .sortedByDescending { it.readForPlayersGroup(target).value } + val factEntries = Query.find<ReadableFactEntry>().toList() + .map { it to it.readForPlayersGroup(target).value } + .sortedByDescending { it.second } + .map { it.first }🤖 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 `@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt` around lines 332 - 333, The sort is repeatedly calling ReadableFactEntry.readForPlayersGroup (which delegates to readForGroup and can be expensive), causing performance issues; before calling page().entries.sortedByDescending { ... } compute and cache each entry's score by mapping entries to their readForPlayersGroup(target).value once (e.g., build a List<Pair<ReadableFactEntry, Int>> or a Map) and sort by that cached score, then use the cached score again when rendering (avoid calling fact.format(target) if it re-invokes readForPlayersGroup, or pass the cached score into formatting); this applies to the inspect and query/list usages around page().entries.sortedByDescending and any subsequent fact.format(target) calls, and leaves CachableFactEntry-backed paths unchanged while protecting non-cached facts.
🤖 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.
Nitpick comments:
In
`@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt`:
- Around line 332-333: The sort is repeatedly calling
ReadableFactEntry.readForPlayersGroup (which delegates to readForGroup and can
be expensive), causing performance issues; before calling
page().entries.sortedByDescending { ... } compute and cache each entry's score
by mapping entries to their readForPlayersGroup(target).value once (e.g., build
a List<Pair<ReadableFactEntry, Int>> or a Map) and sort by that cached score,
then use the cached score again when rendering (avoid calling
fact.format(target) if it re-invokes readForPlayersGroup, or pass the cached
score into formatting); this applies to the inspect and query/list usages around
page().entries.sortedByDescending and any subsequent fact.format(target) calls,
and leaves CachableFactEntry-backed paths unchanged while protecting non-cached
facts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf9cb82c-8b3c-4b7f-babb-d3eb9ff35bad
📒 Files selected for processing (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt
gabber235
requested changes
May 23, 2026
Owner
gabber235
left a comment
gabber235
left a comment
There was a problem hiding this comment.
Requested changes in CodeRabbit Change Stack
RenaudRl
force-pushed
the
feature/sort-facts-by-value
branch
from
9240119 to
fe88c66
Compare
gabber235
requested changes
May 23, 2026
RenaudRl
force-pushed
the
feature/sort-facts-by-value
branch
from
fe88c66 to
65a0a1e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt`:
- Around line 343-344: The calls to ReadableFactEntry.format use the arguments
in the wrong order; change the call sites (the loop using "for ((fact, data) in
facts)" where sender.sendMini(fact.format(data, target)) is invoked and the
other invocation near the same area) to pass the Player/target first and
FactData second by replacing fact.format(data, target) with fact.format(target,
data) to match the signature private fun ReadableFactEntry.format(player:
Player, data: FactData? = null).
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c55cced-98a6-48ae-9729-521e5455fa81
📒 Files selected for processing (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt
RenaudRl
force-pushed
the
feature/sort-facts-by-value
branch
from
65a0a1e to
4756fb1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt (1)
343-344:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
format(...)argument order to match the Kotlin signature.Line 344 and Line 363 currently call
format(data, target), but the declared signature isformat(player: Player, data: FactData? = null). This won’t compile due to type mismatch.Suggested fix
- sender.sendMini(fact.format(data, target)) + sender.sendMini(fact.format(target, data)) ... - sender.sendMini(entry.format(data, target)) + sender.sendMini(entry.format(target, data))#!/bin/bash set -euo pipefail FILE="engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt" echo "Formatter declaration:" rg -nP 'private\s+fun\s+ReadableFactEntry\.format\s*\(\s*player:\s*Player,\s*data:\s*FactData\?\s*=\s*null\s*\)' "$FILE" echo echo "Current call sites:" rg -nP '\.format\s*\(\s*data\s*,\s*target\s*\)' "$FILE" echo echo "Expected call shape:" rg -nP '\.format\s*\(\s*target\s*,\s*data\s*\)' "$FILE" || trueAlso applies to: 362-363
🤖 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 `@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt` around lines 343 - 344, The calls to ReadableFactEntry.format currently pass arguments in the wrong order (format(data, target)); change them to match the declared signature format(player: Player, data: FactData? = null) by swapping the arguments to format(target, data) at each call site (e.g., the loop using for ((fact, data) in facts) and the other call around lines 362-363) so the first argument is the Player (target) and the second is the FactData (data).
🤖 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.
Duplicate comments:
In
`@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt`:
- Around line 343-344: The calls to ReadableFactEntry.format currently pass
arguments in the wrong order (format(data, target)); change them to match the
declared signature format(player: Player, data: FactData? = null) by swapping
the arguments to format(target, data) at each call site (e.g., the loop using
for ((fact, data) in facts) and the other call around lines 362-363) so the
first argument is the Player (target) and the second is the FactData (data).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65e7c032-36f2-4795-8eae-bd68ace18723
📒 Files selected for processing (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt (1)
363-363:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix reversed argument order—compilation error.
Line 363 passes
entry.format(data, target)but the function signature at line 381 expectsformat(player: Player, data: FactData? = null). This tries to passFactDataas thePlayerparameter and will not compile.🔧 Proposed fix
- sender.sendMini(entry.format(data, target)) + sender.sendMini(entry.format(target, data))🤖 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 `@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt` at line 363, The call to entry.format is using reversed arguments causing a type mismatch: change the call at sender.sendMini(entry.format(data, target)) to pass the Player first and FactData second so it matches the function signature format(player: Player, data: FactData? = null), i.e. call entry.format(target, data) (keep the surrounding sender.sendMini(...) unchanged).
🤖 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.
Duplicate comments:
In
`@engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt`:
- Line 363: The call to entry.format is using reversed arguments causing a type
mismatch: change the call at sender.sendMini(entry.format(data, target)) to pass
the Player first and FactData second so it matches the function signature
format(player: Player, data: FactData? = null), i.e. call entry.format(target,
data) (keep the surrounding sender.sendMini(...) unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75bb62ea-adfd-49dc-bed5-135ebc3799dd
📒 Files selected for processing (1)
engine/engine-paper/src/main/kotlin/com/typewritermc/engine/paper/command/TypewriterCommand.kt
RenaudRl
force-pushed
the
feature/sort-facts-by-value
branch
from
7535429 to
cc1677d
Compare
Replace nullable parameter with default value in parameter declaration, removing the null-check indirection in the function body.
gabber235
approved these changes
May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://typewriter.fibery.io/@public/Development/Features-91#Development/Features/Sort-tw-facts-by-Value-103
Summary by CodeRabbit
Bug Fixes
Improvements