feat: bulk update of movies, music and games (#89) - #161
Conversation
) - JsonSerializer.Deserialize<TEnum> with no options only accepts numeric JSON tokens; the default converter has no string-name support, unlike the app-wide JsonOptions used for normal DTO binding. - Every enum-typed field elsewhere in the app accepts a JSON string like "Sold"; the bulk endpoint's enum parser was the only place that would 400 on that same input. - Runbook defect found by probe tests before Increment 3 tests were authored (see session report).
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b72af1a5b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const updates: BulkUpdates = {}; | ||
| if (bulkStatus) updates.status = bulkStatus; | ||
| if (bulkRating != null) updates.personalRating = bulkRating; | ||
| if (bulkTags.length > 0) updates.tags = bulkTags; |
There was a problem hiding this comment.
When a user enters any tag, the client includes tags in updates, but none of the Movies, Music, or Games BulkFields dictionaries registers that key. The generic endpoint therefore rejects every tag-only or mixed-field edit as an unknown field before reaching its special tag-resolution branch, so the entire bulk operation returns 400 and persists nothing. Register tags as supported or exempt it from the whitelist check.
Useful? React with 👍 / 👎.
| BulkFields = new Dictionary<string, BulkField<Movie>> | ||
| { | ||
| ["status"] = BulkFieldBuilder.Enum<Movie, CollectionStatus>("status", (e, v) => e.Status = v), | ||
| ["condition"] = BulkFieldBuilder.Enum<Movie, Condition>("condition", (e, v) => e.Condition = v), |
There was a problem hiding this comment.
When an API client sends the declared condition: null update to clear this nullable field, every media configuration routes it through the non-nullable enum builder, which cannot pass null to the setter and rejects the request instead. This differs from POST/PUT behavior for the nullable Condition? property and from the new BulkUpdates type; use a nullable-enum parser for condition.
Useful? React with 👍 / 👎.
| value = el.ValueKind == JsonValueKind.String | ||
| ? System.Enum.Parse<TEnum>(el.GetString()!) | ||
| : JsonSerializer.Deserialize<TEnum>(el.GetRawText()); |
There was a problem hiding this comment.
When a client sends a valid enum name with different casing, such as "sold", POST/PUT accepts it through DefinedEnumConverter, which parses names case-insensitively, but this one-argument Enum.Parse is case-sensitive and makes the bulk endpoint return 400. Parse with ignoreCase: true so bulk updates preserve the existing write-boundary contract.
Useful? React with 👍 / 👎.
|
|
||
| <div className="flex justify-end gap-2"> | ||
| <Button type="button" variant="secondary" onClick={closeBulkEdit}>Cancel</Button> | ||
| <Button type="button" onClick={confirmBulkEdit} disabled={bulkUpdate.isPending}>Confirm</Button> |
There was a problem hiding this comment.
If a user opens the modal and immediately presses Confirm, all controls still represent “leave unchanged,” so confirmBulkEdit sends an empty updates object. The server explicitly rejects that payload, causing a guaranteed request error from an enabled normal UI path; disable Confirm until at least one field is set or handle the no-op locally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b72af1a5b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const confirmBulkEdit = () => { | ||
| const updates: BulkUpdates = {}; | ||
| if (bulkStatus) updates.status = bulkStatus; | ||
| if (bulkRating != null) updates.personalRating = bulkRating; |
There was a problem hiding this comment.
When a user needs to clear the existing ratings on the selected items, the modal cannot represent that operation: null is both the initial “leave unchanged” state and the value produced by RatingInput's Clear action, and this guard then omits personalRating from the request. Although the bulk API and BulkUpdates explicitly support personalRating: null, the UI can only assign ratings, so users cannot remove them in bulk. Verdict: defer merge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — agreed: the modal cannot clear an existing rating (null is both 'unchanged' and 'cleared'), and hasBulkEdit omits personalRating at that state. Real gap affecting tags/acquiredOn clears too. Tracked as issue #167. Server and BulkUpdates already support personalRating: null; client tri-state follow-up, not a merge-blocker for the endpoint.
…ity, UI guards (#89)
|
Round 1 findings addressed in commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2815719b12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasBulkEdit = Boolean( | ||
| bulkStatus || bulkRating != null || bulkTags.length > 0 || bulkAcquiredOn | ||
| || (category === 'movies' && bulkWatchStatus), |
There was a problem hiding this comment.
When selected records already have ratings, tags, or acquisition dates, the modal cannot clear them: null, [], and '' are simultaneously the initial “leave unchanged” values, and this guard disables Confirm for those values while confirmBulkEdit omits the corresponding keys. The new hasBulkEdit guard added in 2815719 is fresh evidence for the previously reported rating issue because clearing a chosen rating now explicitly returns the modal to a disabled state; use touched flags or an explicit tri-state so these API-supported clear operations can be submitted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — real gap. The modal overloads null/[]/'' as both 'unchanged' and 'clear', so API-supported clears (personalRating: null, clearing tags, clearing acquiredOn) aren't reachable from the UI. Tracked as issue #167. Not merge-blocking for the bulk-update feature itself (server handles the clears correctly when sent); filed for a client follow-up.
| var names = tagsEl.ValueKind == JsonValueKind.Null | ||
| ? null | ||
| : JsonSerializer.Deserialize<string[]?>(tagsEl.GetRawText()); |
There was a problem hiding this comment.
When an authenticated client sends a non-array tag value such as {"updates":{"tags":42}}, JsonSerializer.Deserialize<string[]?> throws JsonException; unlike every BulkFieldBuilder parser, this path does not catch it or return a validation result, so malformed input reaches the unhandled-exception response rather than the endpoint's stable 400 error shape. Validate tagsEl as an array and catch deserialization failures before invoking TagResolver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 53c2d08: the tags deserialize is wrapped in try/catch returning 400 { error='tags: invalid value for tags.' }. Driver-verified — reverted the try/catch → BulkUpdate_Tags_MalformedValue_Returns400 goes RED 3/3, restored green. Matches every sibling BulkField.Apply. No global exception middleware needed.
| if (s.Length != 3) return $"{name} must be 3 characters."; | ||
| set(e, s.Trim().ToUpperInvariant()); |
There was a problem hiding this comment.
For a three-character value containing surrounding whitespace, such as " U ", the raw-length check succeeds and the subsequent trim persists the one-character value "U", violating this builder's three-letter currency contract. Check s.Trim().Length before storing the normalized value, or stop trimming if raw three-character parity is intended.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid parity gap, but intentionally NOT applied in this PR: it lives in BulkFieldBuilder.cs (outside the round's allow-list). Filed as follow-up issue #162 (P3, cosmetic, ' U ' edge case). Agreed the cleanest fix is s.Trim().Length parity with the single-write boundary.
…ullable-enum case tests (#89)
|
@codex please re-review against the current head 53c2d08. Status of your 8 inline findings: 5 fixed/verified (tags whitelist, nullable condition, case-insensitive enum, confirm-disable, malformed-tags 400); 3 tracked as follow-up issues — client clear-state gap #167 (CollectionList:271/272), currency parity #162 (BulkFieldBuilder:215). Appreciate verdicts on whether anything is still merge-blocking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53c2d08459
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const btnClass = category ? BTN_CLASS[category] : ''; | ||
|
|
||
| // ─── Bulk selection + edit ───────────────────────────────────── | ||
| const [selected, setSelected] = useState<Set<number>>(new Set()); |
There was a problem hiding this comment.
When a user selects rows and then changes the search or filters, items is replaced but this state is never reconciled; confirmBulkEdit still submits every ID remaining in selected. The modal can therefore overwrite status, tags, or other fields on records that are no longer displayed, despite the selection control being scoped to the current page. Clear or intersect the selection with the current result IDs when the query, filters, or result set changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real and confirmed independently (selected state line 233 is never reconciled when items/query/filters change; confirmBulkEdit submits [...selected] line 283). This is a genuine P1 data-integrity gap. Fix dispatched (reconcile selected against current result set + RED test).
| value = el.ValueKind == JsonValueKind.String | ||
| ? System.Enum.Parse<TEnum>(el.GetString()!, ignoreCase: true) | ||
| : JsonSerializer.Deserialize<TEnum>(el.GetRawText()); |
There was a problem hiding this comment.
When an authenticated client sends an enum string outside the underlying integer range, such as "999999999999999999999999", Enum.Parse throws OverflowException, which is not caught by this parser, so the bulk endpoint returns 500 instead of its documented validation response. The same pattern exists in NullableEnum; use TryParse or catch OverflowException in both helpers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real: Enum.Parse throws OverflowException on huge numeric enum strings, and both Enum and NullableEnum catch only JsonException + ArgumentException (OverflowException derives from ArithmeticException, so catch(ArgumentException) does not cover it). Fix dispatched (add OverflowException catch → 400 in both builders + RED test).
|
@codex please re-review against the current head d6eecc8. Your two findings (stale-selection P1, enum-overflow P2) are fixed in d6eecc8 with RED-capable tests (BulkUpdate_EnumValue_Overflows_Returns400 ×3, client stale-selection test) + both mutations reddened by the driver and restored green. Server 667/667, client 184/184, build clean. Appreciate a fresh verdict on this head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6eecc8c84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ["description"] = BulkFieldBuilder.Text<Game>("description", (e, v) => e.Description = v), | ||
| ["notes"] = BulkFieldBuilder.Text<Game>("notes", (e, v) => e.Notes = v), | ||
| ["completionStatus"] = BulkFieldBuilder.Enum<Game, CompletionStatus>("completionStatus", (e, v) => e.CompletionStatus = v), | ||
| ["hoursPlayed"] = BulkFieldBuilder.Scalar<Game, int>("hoursPlayed", (e, v) => e.HoursPlayed = v), |
There was a problem hiding this comment.
When an authenticated API client bulk-updates games with {"updates":{"hoursPlayed":-1}}, the generic scalar parser accepts and persists the negative value. Playtime is treated as non-negative elsewhere—the game form enforces min="0", and the adjacent watch/listen count bulk fields use the validating Count builder—so this route can introduce invalid collection data across many games at once. Use a nullable non-negative integer parser for hoursPlayed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real: hoursPlayed uses Scalar<Game,int> (accepts negatives) while watchCount/listenCount use the Count builder that rejects negatives (BulkFieldBuilder.cs:202) and the game form enforces min=0. Bulk hoursPlayed:-1 persists invalid data. Fix dispatched (non-negative nullable parser for hoursPlayed + RED test).
There was a problem hiding this comment.
hoursPlayed now non-negative in bulk (NonNegativeInt, RED/GREEN verified). Independent round-5 reviewer flagged the companion gap: the single-write PUT path (GamesEndpoints Validate/ApplyDto) still has no HoursPlayed<0 guard — a negative value is rejectable via bulk-PATCH but still persistable via create/update. Filing as a follow-up ticket for parity.
|
@codex please re-review against current head d6f404d. Your negative-hoursPlayed finding is fixed via NonNegativeInt (null clears, negative 400, non-negative accepted); independent follow-up NullableEnum overflow path now has its own test. Driver-verified both mutations RED then restored GREEN; server 674/674. Please return a fresh verdict on this head. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Bulk update movies/music/games from the collection lists: select rows, set shared/per-type
fields in one PATCH /api/:type/bulk. One generic endpoint reused across all three media types (#97
generic module). Partial-update semantics; atomic ownership isolation; write-boundary enum validation.
Closes #89