Skip to content

[feat] Sort speed-review session-complete list by outcome#2635

Open
dewierwan wants to merge 1 commit into
masterfrom
dewi/speed-review-settings
Open

[feat] Sort speed-review session-complete list by outcome#2635
dewierwan wants to merge 1 commit into
masterfrom
dewi/speed-review-settings

Conversation

@dewierwan

Copy link
Copy Markdown
Contributor

What

On the speed-review session complete screen, the Accept and Reject columns now order applications by review outcome strength instead of the order they happened to be rated in:

  • Accept column: strong-yes → weak-yes → neutral
  • Reject column: neutral → weak-no → most-negative (moved-to-AGISC last)

So the strongest yeses sit at the top and the most-negative at the bottom, making it easy to scan the best/worst at a glance.

How

Added a RATING_RANK map in SessionComplete.tsx and sorted the accepted / rejected lists by it. The sort keys off effectiveRating (not the raw rating), so if you re-rate a row in the summary, it re-sorts immediately.

Scope note

Earlier iterations of this branch also explored a pre-round settings panel (configurable timer + expand-responses toggle); those were dropped at the author's request. This PR is only the sort change — a single-file, ~13-line diff with no layout/CSS changes.

Testing

  • npm run typecheck, npm run lint, npm test all pass.
  • Verified locally against the real round/application data: rated across the full scale, concluded, and confirmed both columns order strongest-first and that re-rating a row re-sorts it.

Screenshots

Not included. The session-complete screen sits behind Keycloak auth and is only reachable after submitting real review decisions to Airtable, so it can't be captured in a headless/throwaway run without writing to production data. The change is pure list-ordering with no visual restyling, so there's nothing new to show beyond the existing column layout.

🤖 Generated with Claude Code

Accept and Reject columns now order strongest-first (strong-yes → neutral
on Accept, neutral → most-negative on Reject) via a RATING_RANK map, instead
of appearing in the order they were rated. Sort keys off effectiveRating so
re-rating a row in the summary re-orders it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dewierwan

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown

Claude finished @dewierwan's task —— View job


Code Review Complete ✅

Todo List:

  • Read and analyze the modified SessionComplete.tsx file
  • Review the RATING_RANK mapping and sorting logic
  • Check for potential bugs, performance issues, or code quality concerns
  • Verify TypeScript/React best practices
  • Provide comprehensive review feedback

Review Summary

This is a clean, well-implemented change that adds logical sorting to the session-complete results. The implementation is solid with good attention to detail.

✅ What's Done Well

  1. Clear Intent & Implementation: The RATING_RANK mapping clearly expresses the intended sort order with helpful comments.

  2. Correct Use of effectiveRating(): The sorting function correctly uses effectiveRating(a) instead of a.rating, ensuring re-rated items sort immediately after updates.

  3. Comprehensive Rating Coverage: All possible RatingValue types are included in the ranking, including the special moved-to-agisc case.

  4. Type Safety: The Record<RatingValue, number> type ensures all rating values are handled and provides compile-time safety.

  5. Performance: The sorting is efficient and only applied to the filtered results, not the entire dataset.

🔍 Code Analysis

Key Changes (apps/speed-review/src/components/SessionComplete.tsx):

  • Lines 25-33: Added RATING_RANK mapping with logical progression from strongest yes (0) to most negative (5)
  • Line 97: Added byStrength comparator function using the rank mapping
  • Lines 98-99: Applied sorting to both accepted and rejected arrays

Logic Verification:

  • Accept column: strong-yes (0) → yes (1) → neutral-accept (2) ✅
  • Reject column: neutral-reject (3) → no (4) → moved-to-agisc (5) ✅
  • Sort is stable and deterministic ✅

Minor Observations

  1. Performance: The byStrength function is recreated on every render. While the performance impact is minimal for typical result sets, it could be memoized if needed:

    const byStrength = useCallback(
      (a: RatedApplication, b: RatedApplication) => 
        RATING_RANK[effectiveRating(a)] - RATING_RANK[effectiveRating(b)],
      [overrides]
    );

    However, this is likely premature optimization for the expected data size.

  2. Consistency: The comment mentions "most-negative (moved-to-AGISC last)" but moved-to-agisc appears in both accept and reject scenarios. The current implementation correctly handles this by sorting within each decision category.

🎯 Verdict

LGTM - This is a focused, well-implemented improvement that enhances the UX without introducing complexity or risk. The code is clean, type-safe, and correctly handles all edge cases including re-rating scenarios.

The implementation aligns well with the existing codebase patterns and provides immediate value to users by surfacing the most important results first.


@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds outcome-strength sorting to the session-complete screen's Accept and Reject columns in the speed-review app. The implementation is a single-file, ~13-line change that introduces a RATING_RANK lookup map and a byStrength comparator applied after the existing accepted/rejected filter step.

  • Accept column now orders: Strong yes → Weak yes → Neutral (ranks 0–2); Reject column orders: Neutral → Weak no → Moved-to-AGISC (ranks 3–5), matching the intended UX.
  • Record<RatingValue, number> is used for RATING_RANK, so TypeScript enforces exhaustive coverage of all six RatingValue variants at compile time; the comparator reads effectiveRating so any inline re-rate on the summary screen triggers an immediate re-sort.

Confidence Score: 5/5

Safe to merge — the change is isolated to list ordering with no effect on data submission, API calls, or application state.

The diff is minimal and self-contained: a compile-time-exhaustive rank map and a comparator wired into two existing .filter().sort() chains. The sort key uses effectiveRating (already in scope) so live re-rating continues to work correctly. No logic outside the display order is touched.

No files require special attention.

Important Files Changed

Filename Overview
apps/speed-review/src/components/SessionComplete.tsx Adds RATING_RANK map and sorts accepted/rejected lists by outcome strength; logic is correct, TypeScript enforces exhaustive coverage of all RatingValue variants, and effectiveRating is used so overrides trigger live re-sorting.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[rated array] --> B[filter out resetIds to get active]
    B --> C{toDecision of effectiveRating}
    C -->|Accept| D[accepted list]
    C -->|Reject| E[rejected list]
    D --> F[sort by RATING_RANK\nstrong-yes=0, yes=1, neutral-accept=2]
    E --> G[sort by RATING_RANK\nneutral-reject=3, no=4, moved-to-agisc=5]
    F --> H[Render Accept column strongest first]
    G --> I[Render Reject column mildest first]
Loading

Reviews (1): Last reviewed commit: "[feat] sort speed-review session-complet..." | Re-trigger Greptile

Comment on lines +97 to +99
const byStrength = (a: RatedApplication, b: RatedApplication) => RATING_RANK[effectiveRating(a)] - RATING_RANK[effectiveRating(b)];
const accepted = active.filter((r) => toDecision(effectiveRating(r)) === 'Accept').sort(byStrength);
const rejected = active.filter((r) => toDecision(effectiveRating(r)) === 'Reject').sort(byStrength);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No unit tests for sorting logic

The sorting comparator and RATING_RANK map introduce new behaviour that is straightforward to unit-test (pass a mixed RatedApplication[], assert the resulting order), yet there are no tests for SessionComplete.tsx and none were added here. If RATING_RANK is ever extended or a rank value is accidentally changed, a regression would be invisible until manual testing.

Rule Used: Consider adding tests for any new functionality in... (source)

Learned From
bluedotimpact/bluedot#956
bluedotimpact/bluedot#969
bluedotimpact/bluedot#958

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@dewierwan

Copy link
Copy Markdown
Contributor Author

Thanks both — both passes are clean (Greptile 5/5, Claude bot LGTM), no P1/P2 findings.

The one suggestion (memoizing byStrength with useCallback) I'm intentionally not applying: .sort() is called inline on each render regardless, so memoizing the comparator doesn't avoid any work, and the sorted lists are a single review session's worth of rows. It'd add a dependency-array footgun for no measurable gain. Leaving as-is.

No code changes, so no re-trigger needed.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9cbdfffe-4cea-4285-9912-da3dc1d5e69c

📥 Commits

Reviewing files that changed from the base of the PR and between ef5dc36 and 3accd84.

📒 Files selected for processing (1)
  • apps/speed-review/src/components/SessionComplete.tsx

📝 Walkthrough

Walkthrough

This PR adds deterministic ordering to session result rendering. A new RATING_RANK map assigns numeric strength values to each RatingValue option. The Accept and Reject result arrays, derived from effectiveRating assessments, are now sorted using a comparator based on this strength ranking, replacing the prior unsorted behaviour.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description provides comprehensive context including What, How, Scope, Testing, and Screenshots sections. However, the required template sections (Issue, Developer checklist, and Screenshots table) are largely absent or incomplete. Complete the Description template by adding an Issue link (or noting if none exists), filling the Developer checklist items, and properly completing the Screenshots section following the template format.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarises the main change: sorting the session-complete list by rating outcome strength. It is specific, relevant, and matches the primary objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dewi/speed-review-settings

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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