Repository navigation
Conversation
e383537 to
fef7ef5
Compare
tbain
left a comment
tbain
left a comment
There was a problem hiding this comment.
No particular questions or bugs from my human review, however Codex had a few bugs and questions:
- (Bug) [CriteriaGroupBox.tsx:74-100]() and [RuleBox.tsx:85-100]() place interactive dropdown/input controls inside elements with
role="button". This creates invalid nested interaction semantics and can make the actual edit controls inaccessible or incorrectly announced to assistive-technology users.
This seems like something we need to address, main issue is just the interactive sub-elements within the main interactive element that is listed as a button.
- (Bug) [ScoreThresholdField.tsx:73-103]( ) submits on Enter and again on blur while the persisted prop remains unchanged. Rapid edits can issue concurrent mutations whose responses complete out of order, leaving an earlier score persisted after a later edit.
This seems like something to address as well, I know this kind of behavior can be tricky, though. In the Codex output I did ask for a suggested remediation, but basically it boils down to locking the property with an isSaving sort of property until the mutation is complete. However you want to approach it is fine with me.
- [ScoreThresholdField.tsx:80-87]() compares rounded display values and skips the request. An override such as
70.4%and a default of70%both display as70; entering70therefore cannot clear the override, violating the ticket’s reset-to-default scenario.
Do we need some sort of logic to handle this rounding error? I asked Codex for more clarification on this issue (see raw output below) and it seemed to be more a question - do we need to support decimal percents, like 70.4%, and if so, do we need additional support to handle it rounding back to the default value? I think this is more a question than the bug it proposed mainly because I think the use case for this feature is just whole percent values so it's not really an issue if we round it back to 70%, or whatever.
Additionally, please remember to update the PR with manual testing steps (if applicable), and an AI usage attribution. Thanks!
Raw Codex Output:
Details
Verdict: Request changes.-
Bug. [ScoreThresholdField.tsx:80-87](
) compares rounded display values and skips the request. An override such as70.4%and a default of70%both display as70; entering70therefore cannot clear the override, violating the ticket’s reset-to-default scenario. -
Bug. [CriteriaGroupBox.tsx:74-100](
) and [RuleBox.tsx:85-100]() place interactive dropdown/input controls inside elements withrole="button". This creates invalid nested interaction semantics and can make the actual edit controls inaccessible or incorrectly announced to assistive-technology users. -
Bug. [ScoreThresholdField.tsx:73-103](
) submits on Enter and again on blur while the persisted prop remains unchanged. Rapid edits can issue concurrent mutations whose responses complete out of order, leaving an earlier score persisted after a later edit. -
Bug. The live PR description is still the untouched template: it lacks manual testing steps and attribution to an AI agent or an explicit statement that no AI was used. These are required Open edX PR requirements.
Checks: 22 competency-management suites passed (166 tests), TypeScript passed, lint passed with Node 24, and git diff --check passed.
For the first bug, I agree wit would work that way, and it's not necessarily my preference either. However, does this seem like a valid bug? Does the ticket ask for us to accept decimal places for the percentages, or is the rounding behavior actually correct?
You’re right to challenge the original wording. The ticket does not require decimal entry; the sibling UI ticket explicitly expects decimal, non-numeric, and out-of-range input to be rejected.
So the finding is not “the field must accept decimals.” The narrower concern is:
- The backend supports fractional values (
0.704). - The UI displays whole percentages by rounding.
- If an existing override is
70.4%and the default is70%, both display as70%. - Re-entering
70is treated as unchanged, so the request that clears the override is never sent.
I’d retain this as a valid edge-case concern only if fractional persisted values are supported API data. Otherwise, my Bug severity was too strong; it should be downgraded to a Question or omitted pending confirmation that stored thresholds are always whole percentages.
For the third bug;
"""
Bug. ScoreThresholdField.tsx:73-103 submits on Enter and again on blur while the persisted prop remains unchanged. Rapid edits can issue concurrent mutations whose responses complete out of order, leaving an earlier score persisted after a later edit.
"""
What's the remediation for this?
The simplest remediation is to serialize saves at the field:
const saveInFlight = useRef(false);
const [isSaving, setIsSaving] = useState(false);
const commit = async () => {
if (saveInFlight.current) {
return;
}
// Validate and build newRulePayload...
saveInFlight.current = true;
setIsSaving(true);
try {
await onChange(newRulePayload);
} catch {
revertToLastPersisted();
} finally {
saveInFlight.current = false;
setIsSaving(false);
}
};Then disable the input while saving:
<Form.Control
disabled={isSaving}
onBlur={commit}
onKeyDown={(event) => {
if (event.key === 'Enter') {
event.preventDefault();
void commit();
}
}}
/>The ref is important because it blocks the second synchronous onBlur call even before React re-renders. The await ensures the lock remains until the mutation completes.
It would also be preferable for the mutation hook to return/await invalidateQueries, so onChange does not resolve until the refreshed server value is available. This is UI serialization rather than backend atomicity; the backend batch update can remain unchanged.
tbain
left a comment
tbain
left a comment
There was a problem hiding this comment.
I think the aria attributes still need attention, unless you think otherwise. Codex is still reporting the following concern:
- (Bug) The nested interactive-role issue was removed, but both wrappers remain focusable and keyboard-activatable bare elements without an interaction role. aria-current does not provide button/control semantics, so screen readers may not announce these as actionable controls.
- CriteriaGroupBox.tsx:88
- RuleBox.tsx:91
The fix should use a dedicated semantic control separate from the nested input/dropdown, or remove the wrapper’s keyboard interaction entirely.
However, I'm not super familiar with how this UI renders. Does this make sense to you as a concern, or because of the actual use case is this implemented properly? See the Raw Codex output for how it suggested implementing the changes.
- There was also a suggestion that, while the new logic properly locks the mutation, it releases after the POST rather than waiting for the entire "change -> save via POST -> invalidate queries -> refresh values for render" cycle happens. So it's suggesting that we need to put an
awaiton theinvalidateQueries()call associated with that logic so this issue can fully protect those values from timing issues;
(Suggestion) There is still a narrow duplicate-save window because the mutation lock is released when the PATCH resolves, while invalidateQueries() is not awaited. Returning/awaiting that invalidation would keep the field locked until the persisted props refresh:
It seems worth implementing this, but I defer to you on the use case if this is appropriate or not.
Raw Codex Output:
Details
Follow-up review completed on PR #2 at `3367d3f8`, against issue openedx#794.Findings
-
Bug [P2]: The nested interactive-role issue was removed, but both wrappers remain focusable and keyboard-activatable bare
<div>elements without an interaction role.aria-currentdoes not provide button/control semantics, so screen readers may not announce these as actionable controls.- [CriteriaGroupBox.tsx:88](codex://review?pr=https%3A%2F%2Fgithub.com%2FAShatsila%2Ffrontend-app-authoring%2Fpull%2F2&path=src%2Ftaxonomy%2Fcompetency-management%2Fcriteria-groups%2FCriteriaGroupBox.tsx&line=88&side=right)
- [RuleBox.tsx:91](codex://review?pr=https%3A%2F%2Fgithub.com%2FAShatsila%2Ffrontend-app-authoring%2Fpull%2F2&path=src%2Ftaxonomy%2Fcompetency-management%2Fcriteria-groups%2FRuleBox.tsx&line=91&side=right)
The fix should use a dedicated semantic control separate from the nested input/dropdown, or remove the wrapper’s keyboard interaction entirely.
-
Suggestion: The main concurrent-mutation bug is fixed:
saveInFlightblocks duplicate commits and the input is disabled while saving. Anawaitalone would not have been sufficient.There is still a narrow duplicate-save window because the mutation lock is released when the PATCH resolves, while
invalidateQueries()is not awaited. Returning/awaiting that invalidation would keep the field locked until the persisted props refresh:- [ScoreThresholdField.tsx:113](codex://review?pr=https%3A%2F%2Fgithub.com%2FAShatsila%2Ffrontend-app-authoring%2Fpull%2F2&path=src%2Ftaxonomy%2Fcompetency-management%2Fcriteria-groups%2FScoreThresholdField.tsx&line=113&side=right)
- [apiHooks.ts:133](codex://review?pr=https%3A%2F%2Fgithub.com%2FAShatsila%2Ffrontend-app-authoring%2Fpull%2F2&path=src%2Ftaxonomy%2Fcompetency-management%2Fdata%2FapiHooks.ts&line=133&side=right)
The rounding behavior remains, but I am not counting it as a bug because the ticket does not require decimal-place input.
Verification: 14 suites / 83 tests passed, TypeScript passed, oxlint passed, and stylelint passed. Full dprint lint could not run because the environment could not download its remote plugin.
In brief, can you give an example of how to implement a fix for the continued bug for the aria roles?
Use a real button for the selectable area, while keeping the dropdown/input outside it:
<div className="criteria-group-box__header">
<button
type="button"
aria-pressed={isFocused}
onClick={() => focusGroup(group.id)}
>
By completing
</button>
<LogicOperatorSelect
value={group.logicOperator}
onChange={...}
/>
<span>of the following</span>
</div>For RuleBox, use a separate “select rule” button alongside the editable content:
<div className="rule-box">
<button
type="button"
className="rule-box__select"
aria-pressed={isFocused}
aria-label="Select this rule"
onClick={() => focusRuleBox(groupId, ruleKey)}
/>
<div className="rule-box__content">
<ScoreThresholdField ... />
<CriterionChipList ... />
</div>
</div>The important part is that the semantic button does not contain the input or dropdown. Remove tabIndex and keyboard handlers from the surrounding div; the native button supplies those behaviors.
Is it the pattern in similar logic to put an 'await' on the invalidateQueries call, as per the suggestion?
Yes—this is a common pattern when the mutation caller must wait for the refreshed query data before continuing.
onSuccess: (_data, { tagId }) => (
queryClient.invalidateQueries({
queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId),
})
),Or equivalently:
onSuccess: async (_data, { tagId }) => {
await queryClient.invalidateQueries({
queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId),
});
},Because ScoreThresholdField awaits onChange, returning the invalidateQueries() promise keeps the save lock active until the active query has refetched. It is not required for every mutation, but it is appropriate here because the UI relies on the refreshed persisted prop.
Okay, thanks
You’re welcome!
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude and I reviewed together. Claude drafted this comment:
The behavior looks right and the tests pass locally (22 suites, 167 tests), but there are a few functional items to address, plus a large number of comments and tests that can be trimmed. Line numbers below are against 3367d3f.
Needs changes
1. PR description. The description is still the unfilled template. Please add a summary of the change, manual testing steps, and an AI usage attribution (or a statement that no AI was used). The last two are required by the Open edX community.
2. Score input validation. ScoreThresholdField.tsx:89-112 only rejects empty or non-numeric input, so out-of-range and fractional values go straight to the backend: typing 150 sends 1.5, -5 sends -0.05, and 85.5 sends 0.855. Please validate inline that the value is a whole number from 0 to 100, show an inline message when it isn't, and block the commit, the same way the duplicate-score message is handled. The new message needs a messages.ts entry with a description. A test for each rejected case (out of range, negative, decimal) would be good.
3. Lint. oxlint --type-aware reports no-floating-promises on apiHooks.ts:106 and apiHooks.ts:134, where queryClient.invalidateQueries(...) is neither awaited nor returned. The same error exists at apiHooks.ts:81 on the base branch. Changing all three to return queryClient.invalidateQueries(...) fixes the lint error, and it also makes the mutation wait for the refetch before resolving, which keeps the score input locked until the refreshed value has arrived.
4. The real duplicate-score check is untested. RuleBoxList.tsx:45-54 builds the actual duplicate check, but every test that covers duplicates mocks getInlineValidationMessage out, so this logic never runs in a test. Please add tests in RuleBoxList.test.tsx with two rule boxes in one group covering: changing one box's score to match the other's is refused, and re-entering a box's own current score is not treated as a duplicate of itself.
5. updateRuleScore success test doesn't prove what its name claims. CompetencyAssociationsContext.test.tsx:565 says focus is repaired from the response rather than the request, but the request and the mocked response both carry 0.8, so an implementation using the request would pass too. Please make the mocked response differ from the request (for example, a "back to default" response with rule_profile_id set and both overrides null), and assert the focus key that the response implies.
6. Silent no-op in updateRuleScore. In CompetencyAssociationsContext.tsx:383-394, both early returns are Promise.resolve(), which tells ScoreThresholdField the save succeeded. If either branch is ever hit, the save is silently skipped with no toast and the input keeps the typed value. These branches shouldn't be reachable, but returning Promise.reject() (or throwing) would at least make the input revert. A one-line comment saying why the guards exist (TypeScript narrowing of index and systemDefaultProfile) would also help.
Duplicate or unclear tests
7. CriteriaGroupBox.edit.test.tsx:107-130 and LogicOperatorSelect.test.tsx:39-60 test the same thing: re-rendering with an unchanged value keeps the old label. Please keep the LogicOperatorSelect one, since that component owns the behavior, and remove the other.
8. Several RuleBox.edit.test.tsx tests duplicate ScoreThresholdField.test.tsx: line 83 duplicates 131 (an inline message blocks the commit), line 100 duplicates 170 (an unchanged value isn't sent), and line 113 duplicates 87 (revert on rejection). Please keep the ScoreThresholdField versions. Separately, the two tests at lines 83 and 100 read as if 85 and 70 were duplicates of something, when 85 is "duplicate" only because the validator is mocked to say so, and 70 is just the box's own unchanged value.
9. CriteriaGroupBox.test.tsx:129-140: it isn't obvious why the first render gives ['false', 'false'] and the second ['true', 'false']. The second render focuses the group with ruleKey: null, meaning the group is selected but no rule box is. Splitting this into two tests named for those two states would make it self-explanatory.
10. apiHooks.test.tsx:192 mostly tests React Query's own query-key matching rather than our code. Consider dropping it.
11. api.test.ts:136 and :165: the tests are fine, but the names could say what they pin (the URL, the snake_case request body, and the camelCased response). "(already uppercase on the wire)" can go.
12. Naming. The ScoreThresholdField.test.tsx:40 constant rulePayload and the RuleBox.edit.test.tsx:25 helper renderRuleBox would be easier to follow in the tests below them with names that carry the value, e.g. rulePayload70Percent and render70PercentRuleBox.
13. Nit. RuleBox.edit.test.tsx:53 and :60 are nearly identical (canEdit false vs. omitted). One is enough.
Comments
The PR adds a lot of long comments, many of which narrate the code, restate a docstring elsewhere, or record history ("callers that predate this prop", "deliberately not SelectMenu"). History belongs in the commit message or PR description. Specific requests:
14. Delete: CriteriaGroupBox.tsx:14-18 and :79-87; RuleBox.tsx:23-27, :29-32, and :85-90; RuleBoxList.tsx:13-17 and :26-30; LogicOperatorSelect.tsx:30-34; ScoreThresholdField.tsx:106-107 and :118-120; CompetencyAssociationsContext.tsx:61-63 (it's on a pre-existing field and repeats the CriteriaFocus docstring); RuleBox.test.tsx:59-62; RuleBoxList.test.tsx:85-86; CriteriaGroupBox.edit.test.tsx:89-91 and :119-121; CriteriaGroupBox.test.tsx:72-73; LogicOperatorSelect.test.tsx:49-55; CompetencyAssociationsContext.test.tsx:588-591, :596-598, and :611-614.
15. Fix, it's inaccurate: ScoreThresholdField.tsx:18-21 says the validator is "per-keystroke" and has the "same reservation as onChange", but it's only called at commit time (line 104) and nothing is reserved anymore.
16. Shorten to one line: CriteriaGroupBox.tsx:57-59 and RuleBox.tsx:65-67 (the guard is needed, e.g. "Ignore keys bubbled up from the nested control"); RuleBoxList.tsx:42-44 (worth keeping, since it explains why the check compares rounded percents rather than ruleKeyOf keys); ScoreThresholdField.tsx:11-15, :51-55, :58-67, and :96-100; CriteriaGroupBox.test.tsx:143-148; CompetencyAssociationsContext.test.tsx:197-200; types.ts:169-175; the three new comments in criteria-groups.scss.
17. Drop everything after the dash: ScoreThresholdField.tsx:34-36 and :42-43; api.ts:51-53 and :133-135.
18. CompetencyAssociationsContext.tsx:82-100: the two new docstrings are 4 and 9 lines, while their neighbors in the same interface are 1-4. Two lines each is enough.
19. apiHooks.ts:112-115: please match the docstring above it: "Invalidates that competency's criteria-groups query on success so the new score shows up without a manual refetch." I'd keep a short note that tagId is only used for that invalidation, since the URL doesn't need it.
20. LogicOperatorSelect.tsx:27-28: can shrink to "Read-only unless onChange is given."
Possible cleanups
21. CriteriaGroupBox.test.tsx:26-40: the new courseGroup fixture may not be needed anymore. canEdit is now a plain prop and nothing in CriteriaGroupBox derives the course from the tree. Could you check whether removing the fixture leaves the tests green?
22. ScoreThresholdField.tsx:136 and RuleBox.tsx:68-70: both stop the input's key events from reaching the rule box's handler. One of the two is enough.
23. ScoreThresholdField.tsx:85: commit could have a more descriptive name, e.g. saveScoreIfChanged.
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thanks for the updates. Nearly everything from the last round is addressed, and the competency-management tests (22 suites, 166 tests), tsc, oxlint --type-aware on the changed files, and stylelint all pass locally. A few items are still open. Line numbers below are against 3d33375.
Still open from the last review
1. PR description. The description is still the unfilled template. Please add a summary of the change, manual testing steps, and an AI usage attribution (or a statement that no AI was used). The last two are required by the Open edX community, and Tyler asked for them as well.
2. Dangling comment line. LogicOperatorSelect.test.tsx:49 still has the last line of the comment block that was deleted ("// keeps its closed menu's items in the DOM rather than removing them."). On its own it no longer makes sense, so please delete it too.
3. Two comments that were meant to be deleted are still there: the "Deliberately not Paragon's SelectMenu" paragraph in LogicOperatorSelect.tsx:26-31, and the "Defaults to true, matching testHelpers.tsx..." comment in CriteriaGroupBox.test.tsx:55-56. Both record reasoning that belongs in the commit message or PR description rather than in the code.
4. LogicOperatorSelect.tsx:23-25: the sentence starting "Read-only (plain text) unless onChange is given, in which case..." can shrink to "Read-only unless onChange is given." The first sentence of the docstring, describing what the control is, can stay.
5. criteria-groups.scss: the three comments this PR added at :64-67 (the Dropdown display note), :70-72 (the baseline nudge), and :115-118 (the Form.Control width note) can each be one line.
New
6. The "retyping a box's own current score" test can't fail. RuleBoxList.test.tsx:205 is meant to prove the duplicate check excludes the box being edited, but saveScoreIfChanged returns early at ScoreThresholdField.tsx:78 when the typed value equals the box's current percent, before getInlineValidationMessage is ever called. I confirmed this by removing the other.key !== candidateBox.key check from RuleBoxList.tsx locally: the test still passes. Since that path isn't reachable through the UI, I'd simply delete this test. My earlier request for it assumed the self-exclusion could be reached, and it can't. The other new test (a score that matches another box is refused) is good as it is.
7. Question: is the keyboard focus ring visible on the new select buttons? I haven't checked this in a browser, so it may be fine. .criteria-group-box__select-button and .rule-box__select-button both use all: unset, which makes them inline, and their only content is an sr-only span, so the button box itself is close to zero size. The :focus-visible outline at criteria-groups.scss:53-56 and :108-111 is drawn around that box, not around the stretched ::after overlay that covers the header or rule box. If tabbing to a rule box shows no outline, moving the outline to &:focus-visible::after should fix it. A visible focus indicator is a WCAG AA requirement (2.4.7).
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thanks for the updates. Everything from the last round is addressed in the code, including the focus ring: moving the outline to &:focus-visible::after is the fix I had in mind. stylelint passes on the updated criteria-groups.scss. A few small items remain. Line numbers below are against 013995e.
PR description
1. The links in "Supporting information" point to the wrong places. Because this PR lives in a fork, GitHub resolves a bare #123 against the upstream openedx/frontend-app-authoring repo rather than openedx-core, and a repo#123 reference without the owner doesn't link at all. Here is where each one currently goes:
| Written | Currently links to | Should be |
|---|---|---|
openedx-core#794 |
Nothing. It renders as plain text. | openedx/openedx-core#794 |
#759 |
frontend-app-authoring PR 759, "chore: update flcc to version 1.177.8" | openedx/openedx-core#759 |
#760 |
frontend-app-authoring PR 760, "feat: assign taxonomy to organizations" | openedx/openedx-core#760 |
#672 |
frontend-app-authoring PR 672, "refactor: files-and-videos folder" | openedx/openedx-core#672. That ticket is titled "Create & View Competency Criteria", so the name in parentheses can change to match. |
#672-01, #672-02 |
PR 672 again, followed by the plain text "-01" or "-02" | openedx#3275 and #1. The second one needs the owner and repo spelled out, because a bare #1 here would also resolve against the upstream repo. |
criteria-groups.scss
2. criteria-groups.scss:106: please delete this comment rather than pointing to the one above it. The rule under it is identical to the one at :56-59, so a reader already sees the pattern.
3. criteria-groups.scss:53-56: the new focus-ring comment can be one line, for example "// Outline the stretched ::after overlay; the button's own box is near zero size."
4. Nit, optional. criteria-groups.scss:114: after shortening, "this inline sentence" no longer says which sentence it means, and at 129 characters the line is over the 120-character width that dprint.json sets for the rest of the repo. Something like "// Form.Control is block-level and full-width by default; keep it inline in "With a score of X% or higher"." would fix both.
a24b04a to
683ef43
Compare
AShatsila
changed the title
e4cdc28 to
3a3f92c
Compare
683ef43 to
3d6e1c8
Compare
3a3f92c to
4a17042
Compare
Adds the course group, rule box and criterion chip components and wires gradeable-subsection association selection into the competency management page's course search panel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The associations panel shows its loading state until course outlines resolve, and subsection rows offer the add control only once the criteria groups and default rule profile have loaded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nest course groups, criteria groups and rule boxes with a left accent bar on the focused item at each level. Render the subsection association as a single pill with applied and available states. Out-of-scope actions (removing a criterion or an association, editing the score threshold) are shown as inert icons. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3d6e1c8 to
d0b4439
Compare
4a17042 to
fdcf0fb
Compare
2088580 to
6e48b2c
Compare
Description
Makes a persisted bottom-tier criteria group's any/all combining operator, and a persisted rule box's score threshold, editable in place (openedx/openedx-core#794). Previously both
LogicOperatorSelectandScoreThresholdField(built in openedx/openedx-core#672) rendered as read-only text. Editing is gated behind the existingcanEditCoursepermission check: a course author without edit rights still sees plain text, never the control.Impacts the "Course Author" role (anyone viewing or managing a course's competency criteria in the Manage & Apply Competencies workspace).
Key behavior:
<button>(using Bootstrap's.stretched-linkutility to cover the whole row), kept as a DOM sibling of the dropdown/input inside it rather than a wrapping ancestor, so nobutton-role element contains another interactive element.Supporting information
Testing instructions
Other information
<button>rather than a non-semantic wrapper element made keyboard-interactive, so assistive tech announces it as an actionable control.Best Practices Checklist
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks).messages.tsfiles have adescriptionfor translators to use.../import paths added.🤖 Generated with Claude Code