feat(page-controller): reject misused actions and verify input results - #718
gaomeng1900 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new “discarded input” detection can incorrectly throw when the page reformats input back to a value the element already contained, which conflicts with the intended “reformatted is OK” behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves @page-agent/page-controller action reliability by rejecting unsupported interactions (e.g., clicking native <select>, typing into non-text inputs) and by verifying that text input and option selection actually “stick” after dispatching events.
Changes:
- Add actionable rejections for misused actions (native
<select>click;input_texton non-text input types /<select>). - Make
input_textreturn and surface the post-input value (including masked/reformatted values), and treat discarded input as an error. - Make
select_dropdown_optionselect viaselectedIndex, dispatchinput+change, and verify the final selection; add unit tests for these behaviors.
File summaries
| File | Description |
|---|---|
| packages/page-controller/src/PageController.ts | Uses the returned post-input value to add a warning when the page reformats user input. |
| packages/page-controller/src/actions.ts | Enforces invalid-action rejection and adds post-action verification for input and select operations. |
| packages/page-controller/src/actions.test.ts | Adds Vitest coverage for the new rejection and verification behaviors. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const textAfter = readText(element) | ||
| const matches = textAfter.trim() === text.trim() | ||
| const discarded = textAfter === textBefore || textAfter.trim() === '' | ||
| if (!matches && discarded) { | ||
| throw new Error( | ||
| `The page discarded the input. The element now contains ${JSON.stringify(textAfter)}. Inspect the current page state before retrying.` | ||
| ) | ||
| } | ||
| return textAfter |
| const note = | ||
| actualText.trim() === text.trim() | ||
| ? '' | ||
| : ` ⚠️ The page changed it; the element now contains (${actualText}).` |
![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: d3dd98f40e
ℹ️ 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 waitFor(0.1) | ||
|
|
||
| const textAfter = readText(element) | ||
| const matches = textAfter.trim() === text.trim() |
There was a problem hiding this comment.
When a caller intentionally enters leading/trailing whitespace—or only whitespace—and the page normalizes it away, this trimmed comparison treats the requested and actual values as equal. In the whitespace-only case, an input listener that resets " " to "" bypasses the discard check, and PageController.inputText repeats the trimmed comparison, returning an unqualified success even though the field is empty. Compare exact values for verification and only normalize where the input contract explicitly permits it.
AGENTS.md reference: AGENTS.md:L151-L152
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
d3dd98f to
bb5238e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5238e572
ℹ️ 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".
| // Pages may legitimately reformat the value (masks, normalizers), so a different value | ||
| // is not a failure. An unchanged or emptied field means the page discarded the input. | ||
| const value = input.value | ||
| if (value !== text && (value === valueBefore || value === '')) { |
There was a problem hiding this comment.
When an input mask or listener converts the requested text into a different non-empty value, this branch intentionally treats the operation as successful, but inputTextElement returns no value and PageController.inputText still reports the original text at lines 281-284. The agent therefore receives a misleading success message and cannot inspect the actual page state; return the observed value so the wrapper can report the transformation.
AGENTS.md reference: AGENTS.md:L151-L152
Useful? React with 👍 / 👎.
bb5238e to
c0f0b4e
Compare
|
Reproduced on IMask’s phone demo: inputting Please keep the action-type guards. For text input, briefly report the requested and actual values when they differ, without failing the action: an unchanged formatted value does not prove rejection. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f4f0fb010
ℹ️ 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 waitFor(0.1) // Wait to ensure change event processing completes | ||
|
|
||
| if (selectElement.selectedIndex !== option.index) { |
There was a problem hiding this comment.
When an input or change handler rebuilds the options while preserving the requested choice, option refers to the removed node and its live index no longer represents the replacement option's position. This check can therefore throw “discarded” even though the current select still contains and selects the requested option; verify the current selectedOptions using stable expected text/value instead of the original node's index.
AGENTS.md reference: AGENTS.md:L151-L152
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7ba6eb6f1
ℹ️ 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 waitFor(0.1) | ||
|
|
||
| return input.value |
There was a problem hiding this comment.
When a synchronous input handler replaces this element, dispatchEvent completes after the replacement, but this line still reads the detached input. PageController.inputText can therefore report the requested value as a successful current value while the live replacement is empty or normalized differently; detect a disconnected element and fail or resolve the live indexed element before reporting its value.
AGENTS.md reference: AGENTS.md:L151-L152
Useful? React with 👍 / 👎.
What
click/input_texton a native<select>, andinput_texton non-text inputs (checkbox, radio, file, buttons), now fail instead of silently doing nothing.input_texton<input>/<textarea>andselect_dropdown_optionfail when the page discards the value; a reformatted value (input masks, normalizers) is still a success. Contenteditable behavior is unchanged.Type
Testing
npm run cipassesRequirements / 要求