fix: Text input fields on ModalBlockView fail to accept user input.#6980
fix: Text input fields on ModalBlockView fail to accept user input.#6980OtavioStasiak wants to merge 10 commits intodevelopfrom
Conversation
WalkthroughRefactors the modal wrapper from a curried factory to a single-component export, updates the modal view to use the refactor with a computed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/views/ModalBlockView.tsx`:
- Around line 259-261: The modalKey computation in ModalBlockView uses
`${data.viewId}-${blocks.length}-${blocks.map((b: any) => b.blockId ||
b.type).join('-')}` which fails to detect reordering or same-type replacements
because it falls back to b.type for blocks without blockId; update the key
generation (modalKey) to include more specific identifiers such as
b.element?.actionId (and/or other unique element IDs) as an additional fallback
alongside b.blockId and b.type so that input blocks and reordered/replaced
blocks produce distinct key segments and force the tree to remount when
structure changes.
🧹 Nitpick comments (3)
app/containers/UIKit/UiKitModal.stories.tsx (1)
787-798: Stale closure: useprevinstead ofblocksinside the updater callback.Lines 788 and 794 reference the outer
blocksstate variable insidesetBlocks(prev => ...). IfaddFieldwere called in quick succession,blocks.lengthwould be stale. Useprev.lengthinstead for correctness.Proposed fix
const addField = () => { - const nextId = `input-${blocks.length + 1}`; setBlocks(prev => [ ...prev, { type: 'input', - element: { type: 'plain_text_input', actionId: nextId }, - label: { type: 'plain_text', text: `Field ${blocks.length + 1}`, emoji: true }, + element: { type: 'plain_text_input', actionId: `input-${prev.length + 1}` }, + label: { type: 'plain_text', text: `Field ${prev.length + 1}`, emoji: true }, placeholder: { type: 'plain_text', text: 'Type here…', emoji: true } } ]); };app/containers/UIKit/MessageBlock.tsx (1)
15-19:value={props}leaks all caller props intoKitContext.Every prop passed to
ModalBlockWithContext(e.g.blocks,language,viewId, etc.) becomes part of the context value consumed by allKitContextconsumers. This works today because ofanytyping, but it's fragile — any new prop added by the caller silently enters the context. Consider constructing an explicit context object from the relevant props instead.That said, this is consistent with the existing codebase patterns, so this is a low-priority nit.
app/views/ModalBlockView.tsx (1)
245-251:this.setState({})is needed but deserves a comment.Since
this.valuesis a mutable instance field (not React state), callingthis.setState({})is the only way to force a re-render so that the updatedvaluesref is passed down toModalBlockWithContext. This is correct but non-obvious — a brief inline comment would help future readers understand the intent.Suggested comment
changeState = ({ actionId, value, blockId = 'default' }: IActions) => { this.values[actionId] = { blockId, value }; + // Force re-render so updated this.values is passed as a prop to ModalBlockWithContext this.setState({}); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/containers/UIKit/__snapshots__/UiKitModal.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
app/containers/UIKit/MessageBlock.tsxapp/containers/UIKit/UiKitModal.stories.tsxapp/views/ModalBlockView.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/containers/UIKit/MessageBlock.tsx (1)
app/containers/UIKit/utils.ts (1)
KitContext(17-17)
app/views/ModalBlockView.tsx (1)
app/containers/UIKit/MessageBlock.tsx (1)
ModalBlockWithContext(15-19)
app/containers/UIKit/UiKitModal.stories.tsx (2)
app/containers/UIKit/utils.ts (2)
KitContext(17-17)defaultContext(10-15)app/containers/UIKit/index.tsx (2)
UiKitComponent(214-214)UiKitModal(212-212)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: Build Android Experimental / Build
🔇 Additional comments (1)
app/views/ModalBlockView.tsx (1)
265-275: Looks good — theReact.Fragmentkey-based remount is the right approach for the hooks-count mismatch fix.When blocks are added/removed, the key changes and forces a full remount of
ModalBlockWithContext, ensuring the internal hook count of the UIKit parser stays consistent. Typed values won't change the key, so input focus is preserved during typing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Proposed changes
Text input fields on ModalBlockView fail to accept user input.
Issue(s)
How to test or reproduce
/polland click on send;Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Refactor