Implement operator-aware stat conditions support#182
Merged
Conversation
Fix build-breaking type change from Task 1. The spec did not require refactoring PublicLabel.conditions to use LineConditions. This change caused compilation errors in the label properties UI which expects numeric stats. - Revert PublicLabel.conditions to inline shape with numeric stats - Keep LineConditions, ComparisonOperator, StatCondition unchanged
- Update rpy-parser to capture comparison operators from stat conditions - Update TechnicalConstructs interface to use StatCondition - Add normalization functions to convert numbers to StatCondition with default operator - Update label_lines schema to support both number and StatCondition - Update tests to match new shape Passes all 656 backend unit tests.
…tTechnicalConstructs
4 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements end-to-end support for operator-aware stat conditions. The parser now extracts comparison operators ( ChangesOperator-aware stat conditions (parsing, persistence, and UI display)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
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 docstrings
🧪 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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/frontend/tsconfig.json (1)
18-18: 💤 Low valueConsider removing
vitest/globalsfrom types array.The
@testing-library/jest-domentry is correct and necessary for matchers liketoBeInTheDocument()used throughout the tests.However, all test files explicitly import test functions from vitest:
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";Adding
vitest/globalsto the types array is intended for projects that use globaldescribe/it/expectwithout importing them. Since this codebase uses explicit imports, includingvitest/globalsmay be unnecessary and could cause confusion about whether globals or imports should be used.Consider this alternative if globals aren't needed
- "types": ["vitest/globals", "`@testing-library/jest-dom`"], + "types": ["`@testing-library/jest-dom`"],If you prefer to switch to globals instead, you could remove the explicit imports from test files and keep
vitest/globalsin the types array.🤖 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 `@apps/frontend/tsconfig.json` at line 18, The tsconfig "types" array currently includes "vitest/globals" even though test files explicitly import test helpers from "vitest"; remove "vitest/globals" from the types array (leaving "`@testing-library/jest-dom`") so type resolution matches the code style, or alternatively remove explicit imports in test files and keep "vitest/globals" if you intentionally want globals—update the entry in the tsconfig types list accordingly.apps/frontend/src/components/__tests__/TechnicalPopover.test.tsx (1)
1-3: ⚡ Quick winUse the frontend
@/alias for the component import.Please switch the
TechnicalPopoverimport to the repo-standard src alias instead of a relative path.As per coding guidelines, Frontend: Use
@/alias for src directory imports:import { thing } from "@/lib/thing".🤖 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 `@apps/frontend/src/components/__tests__/TechnicalPopover.test.tsx` around lines 1 - 3, The test imports TechnicalPopover via a relative path; update the import in TechnicalPopover.test.tsx to use the frontend src alias (use "`@/`..." style) so the TechnicalPopover component is imported via the repo-standard alias (referencing the TechnicalPopover symbol in the test) instead of "../write-mode/TechnicalPopover".apps/backend/src/services/__tests__/label-line-mapper.service.unit.test.ts (1)
69-119: ⚡ Quick winAdd one case that keeps
statDeltasintact.These new assertions cover operator normalization, but the helper now rebuilds the
conditionsobject. A single test with bothstatsandstatDeltaswould catch accidental field loss on the parser-to-mapper path.🤖 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 `@apps/backend/src/services/__tests__/label-line-mapper.service.unit.test.ts` around lines 69 - 119, Add a new unit test that passes an entry to mapEntriesToLabelLineValues containing both conditions.stats (as a plain number or with non-default operator) and conditions.statDeltas, then assert that result[0].conditions preserves statDeltas exactly and also normalizes stats (e.g., stats.affection_luna becomes { value: 50, operator: ">=" } or preserves a provided operator like ">"); this will exercise the parser-to-mapper path and prevent accidental loss of the statDeltas field when mapEntriesToLabelLineValues rebuilds conditions.apps/backend/src/services/__tests__/technical-parser.service.unit.test.ts (1)
98-140: ⚡ Quick winAdd one explicit
elifoperator case.This suite covers the new operator shapes well, but every added example still goes through
if. Since the feature is scoped toif/elifparsing, oneelif ...:assertion here would guard the separate branch from drifting.🤖 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 `@apps/backend/src/services/__tests__/technical-parser.service.unit.test.ts` around lines 98 - 140, Add a dedicated test that uses an "elif" line (e.g., "elif affection_luna >= 50:") to assert extractTechnicalConstructs handles elif the same as if; update the tests in technical-parser.service.unit.test.ts by adding a new it block that calls extractTechnicalConstructs with an elif condition and expects result.conditions.stats to contain the same operator/value mapping (reference extractTechnicalConstructs and the existing "captures >= operator" test as the template).apps/backend/scripts/seed-technical-badges.ts (1)
335-355: ⚡ Quick winKeep the seed fixtures ordered by
sequence.The new
sequence: 10record is inserted before thesequence: 9block, so the array no longer matches the numbered comments or the manual test steps. Moving this block below Line 9 will keep the fixture easier to read and verify.🤖 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 `@apps/backend/scripts/seed-technical-badges.ts` around lines 335 - 355, The new fixture object with sequence: 10 (identifier: labelId, sequence, conditions) is placed before the existing sequence: 9 block, breaking the ordered sequence; move the entire object whose sequence is 10 so it appears after the block with sequence: 9 so the array remains sorted by sequence and matches the numbered comments and manual test steps, ensuring you keep the object intact (including its conditions.stats entries) and update surrounding comments if necessary.
🤖 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 `@apps/backend/src/services/label-line-mapper.ts`:
- Around line 33-36: The exported LineConditions type is missing statDeltas
which the parser function extractTechnicalConstructs still emits as
conditions.statDeltas; update LineConditions to include a statDeltas? field (the
same shape as stats, e.g. Record<string, number | StatCondition>) so parser
output can be passed through without casts and runtime normalization that
spreads extra keys remains type-safe.
In `@apps/frontend/src/lib/api/__tests__/gitlab.unit.test.ts`:
- Line 13: The test uses an unnecessary double-cast when assigning the mock
fetch; replace the line that assigns globalThis.fetch (currently using mockFetch
as unknown as typeof fetch) with a direct assignment to globalThis.fetch =
mockFetch; locate the assignment referencing globalThis.fetch and the mockFetch
variable in the gitlab unit test (function/test block where mockFetch is
defined) and remove the redundant casts so the code matches auth.unit.test.ts
and variables.unit.test.ts.
In `@packages/shared/src/index.ts`:
- Around line 331-340: PublicLabel's conditions.stats is still declared as
Record<string, number> while LineConditions.stats was changed to Record<string,
StatCondition>, causing an API mismatch; update the PublicLabel type
(specifically the PublicLabel.conditions.stats declaration) to use
Record<string, StatCondition> so it matches StatCondition and LineConditions,
and search for any usages of PublicLabel (e.g., serialization/deserialization,
tests, or consumers) to update or adapt them to the new StatCondition shape.
---
Nitpick comments:
In `@apps/backend/scripts/seed-technical-badges.ts`:
- Around line 335-355: The new fixture object with sequence: 10 (identifier:
labelId, sequence, conditions) is placed before the existing sequence: 9 block,
breaking the ordered sequence; move the entire object whose sequence is 10 so it
appears after the block with sequence: 9 so the array remains sorted by sequence
and matches the numbered comments and manual test steps, ensuring you keep the
object intact (including its conditions.stats entries) and update surrounding
comments if necessary.
In `@apps/backend/src/services/__tests__/label-line-mapper.service.unit.test.ts`:
- Around line 69-119: Add a new unit test that passes an entry to
mapEntriesToLabelLineValues containing both conditions.stats (as a plain number
or with non-default operator) and conditions.statDeltas, then assert that
result[0].conditions preserves statDeltas exactly and also normalizes stats
(e.g., stats.affection_luna becomes { value: 50, operator: ">=" } or preserves a
provided operator like ">"); this will exercise the parser-to-mapper path and
prevent accidental loss of the statDeltas field when mapEntriesToLabelLineValues
rebuilds conditions.
In `@apps/backend/src/services/__tests__/technical-parser.service.unit.test.ts`:
- Around line 98-140: Add a dedicated test that uses an "elif" line (e.g., "elif
affection_luna >= 50:") to assert extractTechnicalConstructs handles elif the
same as if; update the tests in technical-parser.service.unit.test.ts by adding
a new it block that calls extractTechnicalConstructs with an elif condition and
expects result.conditions.stats to contain the same operator/value mapping
(reference extractTechnicalConstructs and the existing "captures >= operator"
test as the template).
In `@apps/frontend/src/components/__tests__/TechnicalPopover.test.tsx`:
- Around line 1-3: The test imports TechnicalPopover via a relative path; update
the import in TechnicalPopover.test.tsx to use the frontend src alias (use
"`@/`..." style) so the TechnicalPopover component is imported via the
repo-standard alias (referencing the TechnicalPopover symbol in the test)
instead of "../write-mode/TechnicalPopover".
In `@apps/frontend/tsconfig.json`:
- Line 18: The tsconfig "types" array currently includes "vitest/globals" even
though test files explicitly import test helpers from "vitest"; remove
"vitest/globals" from the types array (leaving "`@testing-library/jest-dom`") so
type resolution matches the code style, or alternatively remove explicit imports
in test files and keep "vitest/globals" if you intentionally want globals—update
the entry in the tsconfig types list accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: 378a84a8-fc74-496e-82ab-6a48059ad478
📒 Files selected for processing (19)
apps/backend/scripts/seed-technical-badges.tsapps/backend/src/db/schema/tables/label-lines.tsapps/backend/src/services/__tests__/label-line-mapper.service.unit.test.tsapps/backend/src/services/__tests__/technical-parser.service.unit.test.tsapps/backend/src/services/label-line-mapper.tsapps/backend/src/services/rpy-parser.service.tsapps/frontend/src/components/__tests__/TechnicalPopover.test.tsxapps/frontend/src/components/write-mode/LabelPropertiesPanel.tsxapps/frontend/src/components/write-mode/TechnicalBadge.tsxapps/frontend/src/components/write-mode/TechnicalPopover.tsxapps/frontend/src/contexts/__tests__/ThemeContext.test.tsxapps/frontend/src/hooks/useTechnicalInfo.tsapps/frontend/src/lib/api/__tests__/auth.unit.test.tsapps/frontend/src/lib/api/__tests__/gitlab.unit.test.tsapps/frontend/src/lib/api/__tests__/variables.unit.test.tsapps/frontend/src/lib/prose-types.tsapps/frontend/tsconfig.jsondocs/superpowers/specs/2026-05-31-parser-comparison-operators-design.mdpackages/shared/src/index.ts
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 `@apps/backend/src/services/labels.service.ts`:
- Around line 1732-1746: The current transformation in transformedConditions
(mapping label.conditions.stats) always wraps each stat into { value, operator:
">=" }, which double-wraps already-normalized StatCondition objects produced by
normalizeLineConditions; update the mapping in the block producing
transformedConditions to detect if the original value is already a StatCondition
(e.g., an object with keys like "value" and "operator") and leave it as-is,
otherwise wrap legacy numeric values into a StatCondition with operator ">=";
keep the variables copy logic unchanged and ensure the resulting stats type
matches PublicLabel["conditions"].
🪄 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: CHILL
Plan: Pro
Run ID: cc38b435-3fb9-409b-97b7-ce20e24ec963
📒 Files selected for processing (9)
apps/backend/scripts/seed-technical-badges.tsapps/backend/src/services/__tests__/label-line-mapper.service.unit.test.tsapps/backend/src/services/__tests__/technical-parser.service.unit.test.tsapps/backend/src/services/label-line-mapper.tsapps/backend/src/services/labels.service.tsapps/frontend/src/components/__tests__/TechnicalPopover.test.tsxapps/frontend/src/lib/api/__tests__/gitlab.unit.test.tsapps/frontend/tsconfig.jsonpackages/shared/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- apps/frontend/src/lib/api/tests/gitlab.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/frontend/src/components/tests/TechnicalPopover.test.tsx
- packages/shared/src/index.ts
- apps/backend/scripts/seed-technical-badges.ts
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 by CodeRabbit
New Features
Documentation
Tests
Chores