LOG-9015: fix: add fallback to numeric comparator to respect natural sorting#357
Conversation
|
@jgbernalp: This pull request references LOG-9015 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
WalkthroughThe PR adds shared numeric sorting utilities ( ChangesLog Sorting Implementation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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 |
openshift-ci
Bot
added
the
approved
|
/cherry-pick release-6.1 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/sort-utils.ts`:
- Around line 11-12: The stability fallback currently multiplies
fallbackComparison by directionMultiplier which reverses natural order for tied
timestamps in descending mode; change the return so that when result === 0 and
fallbackComparison !== undefined you return fallbackComparison (without applying
directionMultiplier) to preserve input/backend order, updating any tests (e.g.,
in web/src/__tests__/numeric-comparator.spec.ts) that expected reversed order to
now expect the original input order; refer to the variables result,
fallbackComparison, and directionMultiplier in sort-utils.ts when making this
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ab7dc00-997d-4385-8ded-f27bbe265b5d
📒 Files selected for processing (3)
web/src/__tests__/numeric-comparator.spec.tsweb/src/components/logs-table.tsxweb/src/sort-utils.ts
|
/test test-unit |
|
@jgbernalp There are still some records out of order. template used in this test https://redhat.atlassian.net/browse/OU-1280?focusedCommentId=16554580 
The correct order: Message --- openshift_sequence |
jgbernalp
force-pushed
the
fix-numeric-comparator-toaccount-for-natural-sorting
branch
from
1ca8bc8 to
ed4ea5e
Compare
|
@jgbernalp: This pull request references LOG-9015 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/src/sort-utils.ts (1)
11-12:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
directionMultipliermust not be applied to the stability fallback.This is the same defect flagged previously and matches anpingli's review report. With the current code, a descending primary sort (
directionMultiplier = -1) inverts the tie-break:observedTimestampDifference(a, b) = a - bbecomesb - a, so within a group of equaltimestamprows the table renders by descendingobservedTimestampinstead of ascending. anpingli's expected ordering for the tied OU‑1280 stack‑trace rows is ascending byopenshift_sequence/observedTimestamp(...560125first →...906546last), which only works if the fallback is returned as‑is.🐛 Proposed fix
const result = a < b ? -1 : a > b ? 1 : 0; if (result === 0 && fallbackComparison !== undefined) { - return fallbackComparison * directionMultiplier; + return fallbackComparison; } return result * directionMultiplier;Also update the descending tie‑break expectations in
web/src/__tests__/numeric-comparator.spec.ts(theshould apply direction multiplier to fallback comparisonandshould use logIndex as tiebreaker ... in descending sortcases) to reflect preserved tie‑break direction.🤖 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 `@web/src/sort-utils.ts` around lines 11 - 12, The stability fallback comparison is being multiplied by directionMultiplier, which incorrectly inverts tie-break ordering for descending sorts; in the comparator function in web/src/sort-utils.ts (the block computing result, fallbackComparison and using directionMultiplier) return the fallbackComparison without applying directionMultiplier when result === 0 and fallbackComparison !== undefined so tied items preserve their natural ordering, and update the unit tests named "should apply direction multiplier to fallback comparison" and "should use logIndex as tiebreaker ... in descending sort" in web/src/__tests__/numeric-comparator.spec.ts to expect the preserved (non-inverted) tie-break order.
🤖 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 `@web/src/__tests__/numeric-comparator.spec.ts`:
- Around line 26-29: The tests currently assert that the fallback comparison is
multiplied by directionMultiplier (buggy behavior); update the expectations so
the fallback preserves natural ordering even when directionMultiplier is
negative: in the spec cases referencing numericComparator (the "should apply
direction multiplier to fallback comparison" test) and the descending logIndex
tie-break tests, reverse the expected signs for the assertions that compare
equal primary keys (i.e., where numericComparator(5,5,-1,3) and
numericComparator(5,5,-1,-2) are asserted) so they expect the fallback result
without an extra direction flip; locate these assertions by the test titles and
any references to numericComparator or the logIndex tie-break and change the
expected values accordingly.
In `@web/src/components/logs-table.tsx`:
- Around line 66-67: The observedTimestamp fallback currently uses
BigInt(timestamp) where timestamp was computed with parseFloat(String(value[0]))
causing precision loss for nanosecond epochs; instead feed the raw string into
BigInt so both paths use the exact integer string: construct observedTimestamp
from BigInt(stream.stream.observedTimestamp ?? String(value[0])) (keep the
existing parseFloat(String(value[0])) only if the float `timestamp` is still
needed elsewhere, but do not use that parsed `timestamp` when building
`observedTimestamp`).
---
Duplicate comments:
In `@web/src/sort-utils.ts`:
- Around line 11-12: The stability fallback comparison is being multiplied by
directionMultiplier, which incorrectly inverts tie-break ordering for descending
sorts; in the comparator function in web/src/sort-utils.ts (the block computing
result, fallbackComparison and using directionMultiplier) return the
fallbackComparison without applying directionMultiplier when result === 0 and
fallbackComparison !== undefined so tied items preserve their natural ordering,
and update the unit tests named "should apply direction multiplier to fallback
comparison" and "should use logIndex as tiebreaker ... in descending sort" in
web/src/__tests__/numeric-comparator.spec.ts to expect the preserved
(non-inverted) tie-break order.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c932bf0a-c965-4ca9-9510-df2f1fb554e9
📒 Files selected for processing (6)
web/cypress/e2e/integration/logs-sorting.cy.tsweb/src/__tests__/numeric-comparator.spec.tsweb/src/__tests__/observed-timestamp-difference.spec.tsweb/src/components/logs-table.tsxweb/src/logs.types.tsweb/src/sort-utils.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/e2e/integration/logs-sorting.cy.ts
jgbernalp
force-pushed
the
fix-numeric-comparator-toaccount-for-natural-sorting
branch
2 times, most recently
from
13706d2 to
f31f52c
Compare
|
/cherry-pick release-coo-ocp-4.22 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-coo-ocp-4.15 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-coo-ocp-4.12 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test test-unit |
|
/test test-e2e |
|
/retest |
1 similar comment
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, PeterYurkovich The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@jgbernalp I made some comments in jira ticket. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
holding in case qe label is added by mistake. |
jgbernalp
force-pushed
the
fix-numeric-comparator-toaccount-for-natural-sorting
branch
from
f31f52c to
eb281c1
Compare
|
New changes are detected. LGTM label has been removed. |
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 `@web/src/components/logs-table.tsx`:
- Around line 139-141: The comparator uses a truthy check for openshiftSequence
which treats 0n as falsy; update the conditional in the sort comparator (where
bigIntDifference is called) to explicitly check for undefined (e.g.,
a.openshiftSequence !== undefined && b.openshiftSequence !== undefined) so that
0n is honored as a valid tie-break value and only falls back to
observedTimestamp when openshiftSequence is actually undefined.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 25450afb-b4bb-44ab-b326-b39b23a0917c
📒 Files selected for processing (6)
web/cypress/e2e/integration/logs-sorting.cy.tsweb/package.jsonweb/src/__tests__/numeric-comparator.spec.tsweb/src/components/logs-table.tsxweb/src/logs.types.tsweb/src/sort-utils.ts
jgbernalp
force-pushed
the
fix-numeric-comparator-toaccount-for-natural-sorting
branch
from
eb281c1 to
06f6c4d
Compare
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
fix-numeric-comparator-toaccount-for-natural-sorting
branch
from
06f6c4d to
7d9d7fb
Compare
|
/test test-unit |
1 similar comment
|
/test test-unit |
|
@jgbernalp: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Needs openshift/release#79643 |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
6 participants
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests