-
Notifications
You must be signed in to change notification settings - Fork 197
fixes blur issue on char type in Headers table #1111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes blur issue on char type in Headers table #1111
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughModified row key generation in the headers table to use the row index as the React key for each TableRow; prior conditional logic that derived a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
e02b598 to
e4552d3
Compare
d2cd8a1 to
c3bbb0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/components/ui/headersTable.tsx (1)
101-101: The fix resolves the blur issue but introduces the index-as-key anti-pattern.Switching to
key={index}successfully fixes the blur issue that occurred when typing in the key field (React was unmounting/remounting rows as the key value changed). However, using array indices as React keys is generally problematic for dynamic lists:
- Row deletion: When a row is deleted, React reuses DOM nodes from subsequent indices, which can cause confusion if a user has focus on a specific row.
- Reconciliation issues: React may incorrectly match component instances during updates, though controlled inputs mitigate data corruption.
Recommended approach: Use stable IDs for each row instead of indices. For example:
-const rows = [...headerEntries, ["", ""]]; +const rows = [...headerEntries.map(([k, v]) => ({ id: k, key: k, value: v })), { id: '__empty__', key: '', value: '' }];Then use
key={row.id}for stable identity. For the empty row, use a constant sentinel ID like'__empty__'that never conflicts with user-entered keys.Since the current fix is functional and the blur issue is critical UX, this change is acceptable as-is, but the stable-ID approach would be more robust for future maintenance.
Note: Per coding guidelines, this review should consider the entire Graphite stack (#1111, #1104, main), but the related PRs were not provided. If there are related changes to this component in the stack, please ensure they account for the key generation logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/components/ui/headersTable.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/components/ui/headersTable.tsx
🧬 Code graph analysis (1)
ui/components/ui/headersTable.tsx (1)
ui/components/ui/table.tsx (1)
TableRow(71-71)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
c3bbb0a to
6e483b1
Compare
e4552d3 to
93c6ab1
Compare
Merge activity
|

Summary
Fixed a key generation issue in the HeadersTable component by using array index as the React key instead of attempting to use header key values.
Changes
Type of change
Affected areas
How to test
Verify that the headers table renders correctly and maintains proper state when adding/removing rows:
Navigate to any page with the HeadersTable component and verify that rows can be added, edited, and removed without issues.
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No security implications.
Checklist
docs/contributing/README.mdand followed the guidelines