Skip to content

feat: Temporary remove percentage until backend reports type in conversation list#85

Open
spoonman01 wants to merge 1 commit into
mainfrom
feat/temporary-remove-percentage
Open

feat: Temporary remove percentage until backend reports type in conversation list#85
spoonman01 wants to merge 1 commit into
mainfrom
feat/temporary-remove-percentage

Conversation

@spoonman01

Copy link
Copy Markdown

@spoonman01
spoonman01 requested a review from a team as a code owner July 18, 2026 15:55
@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

PR Review: Temporarily remove percentage until backend reports type (WPB-27239)

Nice, focused change. The intent is clear and I like that every workaround is tagged with the WPB-27239 ticket so the eventual revert is easy to find. A few things worth addressing before merge.

🐞 Potential issues

1. The entire StatsFormattingServiceTest is @Ignored, not just the percentage parts

@Ignore("Percentage voting blocked until WPB-27239 is done")
class StatsFormattingServiceTest {

This disables all tests in the class — including ones unrelated to percentages (title formatting, mention-offset math, empty-stats handling). StatsFormattingService is still very much live code, so this leaves it with zero regression coverage while it keeps shipping. Consider instead:

  • Updating the affected assertions to the new expected output (the format change is deterministic and easy to assert), or
  • @Ignore-ing only the specific percentage/placeholder test cases and keeping the rest green.

2. Unbounded emoji string in PollVoteCountProgress.display()

val progressString = (0 until totalVoteCount).joinToString("") { "🟢" }

The previous implementation was capped at BLOCKS = 10. This now emits one 🟢 per vote with no upper bound — in a large conversation a poll could render dozens/hundreds of emoji into the conversation-list preview. The same applies to formatVotes now that maximumSize = mostPopularOptionVoteCount (previously bounded by min(conversationMembers, …)). Worth confirming this is acceptable for large polls, or keeping a sane cap.

3. Leading space when count is 0

initial() is (0, 0), so display() returns " (0)" (empty progress string + leading space), and PollVoteCountProgress.new() feeds this into the conversation list. Minor, but a trim() or conditional join would avoid the stray leading space.

🧹 Dead code / stale docs

  • BLOCKS and PERCENTAGE_FACTOR in PollVoteCountProgress are now unused, as is MAX_VOTE_PLACEHOLDER_COUNT in StatsFormattingService. If they are kept intentionally to ease the revert, a one-line note would help; otherwise detekt/lint will likely flag them.
  • The formatVotes KDoc still documents the MAX_VOTE_PLACEHOLDER_COUNT / conversationMembers trailing-placeholder behavior that no longer runs, and the inline comment // we can use assert as the result size is checked no longer matches the code. These are now misleading — a quick update keeps the file self-consistent.
  • @Suppress("UnusedParameter") on formatVotes is a reasonable temporary measure since conversationMembers is now unused but call sites still pass it.

✅ Nits / positives

  • Consistent ticket references throughout — makes the eventual revert trivial. 👍
  • Removing the now-unused imports (kotlin.math.min, VoteDisplay, roundToInt) was handled cleanly.

Overall this is a small, well-scoped temporary change. The main thing I would push back on is @Ignore-ing the whole test class rather than the percentage-specific cases — that is a meaningful, if temporary, loss of coverage on code that still runs in production.

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.

1 participant