[codex] Use score as secondary dashboard sort#381
Open
BobbyWang0120 wants to merge 1 commit intosantifer:mainfrom
Open
[codex] Use score as secondary dashboard sort#381BobbyWang0120 wants to merge 1 commit intosantifer:mainfrom
BobbyWang0120 wants to merge 1 commit intosantifer:mainfrom
Conversation
📝 WalkthroughWalkthroughRefactored sorting logic in the pipeline view by extracting comparator functions for primary sorting ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dashboard/internal/ui/screens/pipeline_test.go`:
- Around line 116-193: Add a grouped-mode test that verifies sortCompany uses
Score as the tie-breaker within a status group: create a small apps slice with
at least two entries sharing Company and Status but different Score (and one in
a different status), instantiate NewPipelineModel, set pm.sortMode = sortCompany
(and ensure grouped view if needed), call pm.applyFilterAndSort(), then use
assertFilteredOrder to assert the higher-Score record appears before the
lower-Score record within the same status; this will exercise
lessWithinStatusGroup and lock in the grouped-company tie-break behavior.
- Around line 174-193: The test relies on NewPipelineModel defaulting viewMode
to "grouped", so make the test explicit by setting pm.viewMode = viewModeGrouped
(or the string "grouped" if the constant isn't available) immediately after
creating pm in TestGroupedDateSortUsesScoreTieBreakerWithinStatusGroup, then
call pm.applyFilterAndSort() as before; this ensures the test exercises grouped
behavior even if NewPipelineModel's default changes.
In `@dashboard/internal/ui/screens/pipeline.go`:
- Around line 486-492: Remove the redundant "case sortScore: fallthrough" and
the duplicate trailing "return left.Score > right.Score" in the switch that
decides ordering (the block referencing sortScore and comparing
left.Score/right.Score); instead keep a single path for score comparison—either
the default branch in the switch that returns left.Score > right.Score or a
single post-switch return—but not both. Apply the same cleanup pattern in
lessWithinStatusGroup where the same redundant case and trailing return are
present.
- Around line 466-519: lessWithinStatusGroup duplicates the comparator logic in
lessBySortMode; change lessWithinStatusGroup to simply call lessBySortMode
unless m.sortMode == sortStatus, in which case return the score tie-break
(left.Score > right.Score). Concretely, inside lessWithinStatusGroup check if
m.sortMode == sortStatus then return left.Score > right.Score, otherwise return
m.lessBySortMode(left, right); remove the duplicated switch/cases from
lessWithinStatusGroup so future comparator changes live only in lessBySortMode.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53ef458c-7c84-474b-a38c-149947c95dc0
📒 Files selected for processing (2)
dashboard/internal/ui/screens/pipeline.godashboard/internal/ui/screens/pipeline_test.go
Comment on lines
+116
to
+193
| func TestFlatNonScoreSortsUseScoreTieBreaker(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| sortMode string | ||
| apps []model.CareerApplication | ||
| want []int | ||
| }{ | ||
| { | ||
| name: "date", | ||
| sortMode: sortDate, | ||
| apps: []model.CareerApplication{ | ||
| {Number: 101, Company: "Acme", Role: "Backend Engineer", Status: "Applied", Date: "2026-04-18", Score: 3.2}, | ||
| {Number: 102, Company: "Zulu", Role: "AI Engineer", Status: "Applied", Date: "2026-04-18", Score: 4.8}, | ||
| {Number: 103, Company: "Beta", Role: "Platform Engineer", Status: "Applied", Date: "2026-04-17", Score: 4.1}, | ||
| }, | ||
| want: []int{102, 101, 103}, | ||
| }, | ||
| { | ||
| name: "company", | ||
| sortMode: sortCompany, | ||
| apps: []model.CareerApplication{ | ||
| {Number: 201, Company: "Acme", Role: "Backend Engineer", Status: "Applied", Score: 3.1}, | ||
| {Number: 202, Company: "Acme", Role: "AI Engineer", Status: "Applied", Score: 4.7}, | ||
| {Number: 203, Company: "Beta", Role: "Platform Engineer", Status: "Applied", Score: 4.9}, | ||
| }, | ||
| want: []int{202, 201, 203}, | ||
| }, | ||
| { | ||
| name: "status", | ||
| sortMode: sortStatus, | ||
| apps: []model.CareerApplication{ | ||
| {Number: 301, Company: "Acme", Role: "Backend Engineer", Status: "Applied", Score: 3.1}, | ||
| {Number: 302, Company: "Zulu", Role: "AI Engineer", Status: "Applied", Score: 4.7}, | ||
| {Number: 303, Company: "Beta", Role: "Platform Engineer", Status: "Interview", Score: 2.0}, | ||
| }, | ||
| want: []int{303, 302, 301}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| pm := NewPipelineModel( | ||
| theme.NewTheme("catppuccin-mocha"), | ||
| tc.apps, | ||
| model.PipelineMetrics{Total: len(tc.apps)}, | ||
| "..", | ||
| 120, | ||
| 40, | ||
| ) | ||
| pm.viewMode = "flat" | ||
| pm.sortMode = tc.sortMode | ||
| pm.applyFilterAndSort() | ||
|
|
||
| assertFilteredOrder(t, pm.filtered, tc.want) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGroupedDateSortUsesScoreTieBreakerWithinStatusGroup(t *testing.T) { | ||
| apps := []model.CareerApplication{ | ||
| {Number: 401, Company: "Acme", Role: "Backend Engineer", Status: "Applied", Date: "2026-04-18", Score: 3.2}, | ||
| {Number: 402, Company: "Zulu", Role: "AI Engineer", Status: "Applied", Date: "2026-04-18", Score: 4.8}, | ||
| {Number: 403, Company: "Beta", Role: "Platform Engineer", Status: "Interview", Date: "2026-04-18", Score: 2.1}, | ||
| } | ||
|
|
||
| pm := NewPipelineModel( | ||
| theme.NewTheme("catppuccin-mocha"), | ||
| apps, | ||
| model.PipelineMetrics{Total: len(apps)}, | ||
| "..", | ||
| 120, | ||
| 40, | ||
| ) | ||
| pm.sortMode = sortDate | ||
| pm.applyFilterAndSort() | ||
|
|
||
| assertFilteredOrder(t, pm.filtered, []int{403, 402, 401}) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test coverage looks solid.
The tie-breaker assertions cover the three non-score flat sort modes plus the grouped date case, and the expected orderings correctly reflect the comparator contract (primary field first, Score descending on ties). The table-driven structure and assertFilteredOrder helper keep the intent clear.
One small gap worth considering: there's no test asserting that sortCompany ties fall back to score within a status group (grouped view). Given the refactor explicitly added lessWithinStatusGroup, a second grouped-mode case (e.g. sortCompany with two same-company apps inside the same status bucket) would lock in that behavior and guard against future regressions in the grouped branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/internal/ui/screens/pipeline_test.go` around lines 116 - 193, Add a
grouped-mode test that verifies sortCompany uses Score as the tie-breaker within
a status group: create a small apps slice with at least two entries sharing
Company and Status but different Score (and one in a different status),
instantiate NewPipelineModel, set pm.sortMode = sortCompany (and ensure grouped
view if needed), call pm.applyFilterAndSort(), then use assertFilteredOrder to
assert the higher-Score record appears before the lower-Score record within the
same status; this will exercise lessWithinStatusGroup and lock in the
grouped-company tie-break behavior.
Comment on lines
+174
to
+193
| func TestGroupedDateSortUsesScoreTieBreakerWithinStatusGroup(t *testing.T) { | ||
| apps := []model.CareerApplication{ | ||
| {Number: 401, Company: "Acme", Role: "Backend Engineer", Status: "Applied", Date: "2026-04-18", Score: 3.2}, | ||
| {Number: 402, Company: "Zulu", Role: "AI Engineer", Status: "Applied", Date: "2026-04-18", Score: 4.8}, | ||
| {Number: 403, Company: "Beta", Role: "Platform Engineer", Status: "Interview", Date: "2026-04-18", Score: 2.1}, | ||
| } | ||
|
|
||
| pm := NewPipelineModel( | ||
| theme.NewTheme("catppuccin-mocha"), | ||
| apps, | ||
| model.PipelineMetrics{Total: len(apps)}, | ||
| "..", | ||
| 120, | ||
| 40, | ||
| ) | ||
| pm.sortMode = sortDate | ||
| pm.applyFilterAndSort() | ||
|
|
||
| assertFilteredOrder(t, pm.filtered, []int{403, 402, 401}) | ||
| } |
There was a problem hiding this comment.
Test implicitly depends on viewMode default being "grouped".
The grouped-view test relies on NewPipelineModel defaulting viewMode to "grouped" (set at line 123 of pipeline.go) rather than setting it explicitly. If that default ever changes, this test silently becomes a flat-mode test and still passes — for the wrong reason.
✅ Proposed hardening
pm.sortMode = sortDate
+ pm.viewMode = "grouped"
pm.applyFilterAndSort()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/internal/ui/screens/pipeline_test.go` around lines 174 - 193, The
test relies on NewPipelineModel defaulting viewMode to "grouped", so make the
test explicit by setting pm.viewMode = viewModeGrouped (or the string "grouped"
if the constant isn't available) immediately after creating pm in
TestGroupedDateSortUsesScoreTieBreakerWithinStatusGroup, then call
pm.applyFilterAndSort() as before; this ensures the test exercises grouped
behavior even if NewPipelineModel's default changes.
Comment on lines
+466
to
+519
| // lessBySortMode keeps score as the secondary key for every non-score sort so | ||
| // the highest-value jobs stay at the top when the primary field ties. | ||
| func (m PipelineModel) lessBySortMode(left, right model.CareerApplication) bool { | ||
| switch m.sortMode { | ||
| case sortDate: | ||
| if left.Date != right.Date { | ||
| return left.Date > right.Date | ||
| } | ||
| case sortCompany: | ||
| leftCompany := strings.ToLower(left.Company) | ||
| rightCompany := strings.ToLower(right.Company) | ||
| if leftCompany != rightCompany { | ||
| return leftCompany < rightCompany | ||
| } | ||
| case sortStatus: | ||
| leftPriority := data.StatusPriority(left.Status) | ||
| rightPriority := data.StatusPriority(right.Status) | ||
| if leftPriority != rightPriority { | ||
| return leftPriority < rightPriority | ||
| } | ||
| case sortScore: | ||
| fallthrough | ||
| default: | ||
| return left.Score > right.Score | ||
| } | ||
|
|
||
| return left.Score > right.Score | ||
| } | ||
|
|
||
| // lessWithinStatusGroup applies the selected secondary sort inside one grouped | ||
| // status bucket while still falling back to score when the chosen field ties. | ||
| func (m PipelineModel) lessWithinStatusGroup(left, right model.CareerApplication) bool { | ||
| switch m.sortMode { | ||
| case sortDate: | ||
| if left.Date != right.Date { | ||
| return left.Date > right.Date | ||
| } | ||
| case sortCompany: | ||
| leftCompany := strings.ToLower(left.Company) | ||
| rightCompany := strings.ToLower(right.Company) | ||
| if leftCompany != rightCompany { | ||
| return leftCompany < rightCompany | ||
| } | ||
| case sortStatus: | ||
| // Status is already fixed by the group header, so score becomes the | ||
| // next meaningful ordering signal within that bucket. | ||
| case sortScore: | ||
| fallthrough | ||
| default: | ||
| return left.Score > right.Score | ||
| } | ||
|
|
||
| return left.Score > right.Score | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating the two comparators to reduce duplication.
lessBySortMode and lessWithinStatusGroup are nearly identical — the only meaningful difference is that the grouped variant skips the sortStatus priority comparison (since status is fixed within a bucket). The duplicated sortDate / sortCompany / score-fallback branches make future changes error‑prone (any tweak to the date or company comparator has to be mirrored in both places).
One low‑risk consolidation is to have lessWithinStatusGroup delegate to lessBySortMode except when the sort mode is sortStatus:
♻️ Proposed refactor
func (m PipelineModel) lessWithinStatusGroup(left, right model.CareerApplication) bool {
- switch m.sortMode {
- case sortDate:
- if left.Date != right.Date {
- return left.Date > right.Date
- }
- case sortCompany:
- leftCompany := strings.ToLower(left.Company)
- rightCompany := strings.ToLower(right.Company)
- if leftCompany != rightCompany {
- return leftCompany < rightCompany
- }
- case sortStatus:
- // Status is already fixed by the group header, so score becomes the
- // next meaningful ordering signal within that bucket.
- case sortScore:
- fallthrough
- default:
- return left.Score > right.Score
- }
-
- return left.Score > right.Score
+ // Status is already fixed by the group header, so score becomes the
+ // next meaningful ordering signal within that bucket.
+ if m.sortMode == sortStatus {
+ return left.Score > right.Score
+ }
+ return m.lessBySortMode(left, right)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/internal/ui/screens/pipeline.go` around lines 466 - 519,
lessWithinStatusGroup duplicates the comparator logic in lessBySortMode; change
lessWithinStatusGroup to simply call lessBySortMode unless m.sortMode ==
sortStatus, in which case return the score tie-break (left.Score > right.Score).
Concretely, inside lessWithinStatusGroup check if m.sortMode == sortStatus then
return left.Score > right.Score, otherwise return m.lessBySortMode(left, right);
remove the duplicated switch/cases from lessWithinStatusGroup so future
comparator changes live only in lessBySortMode.
Comment on lines
+486
to
+492
| case sortScore: | ||
| fallthrough | ||
| default: | ||
| return left.Score > right.Score | ||
| } | ||
|
|
||
| return left.Score > right.Score |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: redundant case sortScore: fallthrough plus trailing return.
The case sortScore: fallthrough / default: return left.Score > right.Score combo plus the final return left.Score > right.Score after the switch is functionally correct but slightly harder to read than necessary. You can drop the explicit sortScore case (it's already covered by default) and rely on the post-switch return for the tie-break paths:
♻️ Optional simplification
case sortStatus:
leftPriority := data.StatusPriority(left.Status)
rightPriority := data.StatusPriority(right.Status)
if leftPriority != rightPriority {
return leftPriority < rightPriority
}
- case sortScore:
- fallthrough
- default:
- return left.Score > right.Score
}
return left.Score > right.ScoreSame applies to lessWithinStatusGroup. Purely a readability nit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case sortScore: | |
| fallthrough | |
| default: | |
| return left.Score > right.Score | |
| } | |
| return left.Score > right.Score | |
| case sortStatus: | |
| leftPriority := data.StatusPriority(left.Status) | |
| rightPriority := data.StatusPriority(right.Status) | |
| if leftPriority != rightPriority { | |
| return leftPriority < rightPriority | |
| } | |
| } | |
| return left.Score > right.Score |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/internal/ui/screens/pipeline.go` around lines 486 - 492, Remove the
redundant "case sortScore: fallthrough" and the duplicate trailing "return
left.Score > right.Score" in the switch that decides ordering (the block
referencing sortScore and comparing left.Score/right.Score); instead keep a
single path for score comparison—either the default branch in the switch that
returns left.Score > right.Score or a single post-switch return—but not both.
Apply the same cleanup pattern in lessWithinStatusGroup where the same redundant
case and trailing return are present.
clmoon2
pushed a commit
to clmoon2/career-ops
that referenced
this pull request
Apr 20, 2026
- Accept remote (v132) scan-history, pipeline, scan-summary - Append v108 unique contribution: C3 AI Ascend intern evaluated at 4.0/5 (upgraded from 3.5/5 pipeline estimate in v98; full report santifer#387 written locally) - Merge remote evaluations santifer#380-386 from parallel scans: Subimage (santifer#380), Robinhood Agentic ML (santifer#381), Replit new grad (santifer#382), Harvey new grad (santifer#383), Figma SWE intern (santifer#384), Marshall Wace tech intern (santifer#385), Illumio SRE intern (santifer#386) https://claude.ai/code/session_01T5e8o3uios3ZnZHySM7CXU
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
1 participant
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.
Summary
Testing
cd dashboard && go test ./...cd dashboard && go build ./...Bug-fix note: this is a focused dashboard sorting fix, so I did not open a separate issue.
Summary by CodeRabbit
Refactor
Tests