Conversation
|
A preview of 7afbae9 is uploaded and can be seen here: ✨ https://revisit.dev/study/PR1454 ✨ Changes may take a few minutes to propagate. |
![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: 1123ef3d18
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {(isDelayedDisabled: boolean) => ( | ||
| <div | ||
| style={{ | ||
| pointerEvents: isDelayedDisabled ? 'none' : 'auto', |
There was a problem hiding this comment.
When delay is set, this CSS suppresses only pointer targeting. A textOnly prompt can contain Markdown links and TextOnlyInput never consumes the disabled state, so Tab+Enter activates those links immediately; likewise, ResponseSwitcher returns false before consulting its disabled prop when the next component enables previousButton or the current component precedes a dynamic block, leaving normal inputs keyboard-editable. These paths let keyboard participants interact before the experimental delay expires and can invalidate timing results; make the wrapper inert or otherwise suppress focus while also preserving native disabled state.
Useful? React with 👍 / 👎.
| if ('restartEnumeration' in response) { | ||
| index = 0; |
There was a problem hiding this comment.
When a textOnly response explicitly sets restartEnumeration: false, the in check still succeeds, resetting the index so the next numbered response incorrectly starts over at 1. This is a valid schema-backed configuration and differs from the previous truthiness check; test the property's boolean value rather than merely its presence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4d9d663c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Slider | ||
| await page.locator('.mantine-Slider-track').click(); | ||
| page.locator('[data-question-id="q-slider"] .mantine-Slider-track').click(); |
There was a problem hiding this comment.
In the form-elements Playwright workflow, this click is started without awaiting its promise, so it can race the subsequent short- and long-text interactions; if the locator is still auto-waiting or the click fails, the rejection is detached from the intended test step and can make the suite flaky. Restore the await before continuing.
Useful? React with 👍 / 👎.
JackWilb
left a comment
JackWilb
left a comment
There was a problem hiding this comment.
Thanks for the focused delay and enumeration work. I found two existing behavior regressions that need to be addressed before merge: the added wrapper changes the response DOM structure used by shipped and external styles, and passing disabledAttempts into the inert wrapper overrides existing editability behavior in training and navigation flows. I also left smaller comments on the schema reference, delay validation, test coverage, and an unnecessary compatibility branch.
| /> | ||
| > | ||
| {(isDelayedDisabled: boolean) => ( | ||
| <div |
There was a problem hiding this comment.
This extra div is now inserted around every response, including responses without a delay. The demo-style layout and existing study styles rely on .responseBlock > div > .response and on the response being the direct child of the outer wrapper, so these selectors stop matching. Please keep the existing DOM structure and apply the delay state to the existing .response element, or otherwise avoid adding a wrapper node.
| config={config} | ||
| <DelayedResponseWrapper | ||
| delay={response.delay} | ||
| disabled={disabledAttempts} |
There was a problem hiding this comment.
Passing disabledAttempts into this wrapper makes the whole response inert, even in the existing flows where ResponseSwitcher intentionally keeps inputs editable before a dynamic block or when the next page has a Previous button. Please keep the timer-disabled state separate from the existing training-state behavior and only make the response inert while its delay timer is active.
| "max": 100, | ||
| "min": 0, | ||
| "withDontKnow": true, | ||
| "delay": 5000 |
There was a problem hiding this comment.
This demo still points at the v2.4.3 schema, but that schema does not define delay and rejects this new property as an additional property. Please point the example at the current/dev schema so the example validates and editor tooling recognizes the field.
| /** Exclude response from randomization. If present, will override the `responseOrder` randomization setting in the components. Defaults to false. */ | ||
| excludeFromRandomization?: boolean; | ||
| /** Delay in milliseconds before an element activates. Defaults to 0 meaning the element is always visible. */ | ||
| delay?: number; |
There was a problem hiding this comment.
The schema currently accepts negative delays, but the implementation silently treats them as zero. A negative duration has no meaningful interpretation here. Please constrain delay to non-negative values in the type metadata and regenerate both schemas.
| expect(wrapperWithStyles?.style.opacity).toBe('0.4'); | ||
| }); | ||
|
|
||
| test('does not apply delay styles to non-delayed responses and maintains sequential index', async () => { |
There was a problem hiding this comment.
The test name says it checks sequential enumeration, but it never asserts either response index; it only checks styles on q1. The divider regression would still pass if numbering were wrong. Please capture the index or render the real labels and cover divider plus restartEnumeration false and true.
| if (isInCurrentLocation) { | ||
| // Increment index for each response, unless it is a textOnly response | ||
| if (response.type !== 'textOnly') { | ||
| const isNonQuestion = response.type === 'textOnly' || response.type === 'divider' || ('divider' in response && Boolean(response.divider)); |
There was a problem hiding this comment.
No response type or generated schema defines a divider property; the supported response-level field is withDivider, while an actual divider is identified by type: divider. This compatibility branch cannot be reached by a valid parsed config and adds unnecessary complexity. Please remove it.
|
delay until stimulus is satisfied |
2 participants
Does this PR close any open issues?
Closes #613
Give a longer description of what this PR addresses and why it's needed
Arbitrary Form elements can now be disabled for a specified amount of time in ms. This PR also fixes the enumeration issue with dividers.