Skip to content

Revert "Restore: Show up to three report fields per row and edit them inline" - #104513

Closed
AndrewGable wants to merge 1 commit into
mainfrom
revert-104124-claude-reportFieldsGridInlineEditing-restoreAgain
Closed

AndrewGable wants to merge 1 commit into
mainfrom
revert-104124-claude-reportFieldsGridInlineEditing-restoreAgain

Conversation

@AndrewGable

@AndrewGable AndrewGable commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Straight revert #104124

Fixes $ #104507

@AndrewGable
AndrewGable requested review from a team as code owners October 20, 2026 21:36
@melvin-bot

melvin-bot Bot commented Oct 20, 2026

Copy link
Copy Markdown

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@melvin-bot
melvin-bot Bot requested review from JmillsExpensify and removed request for a team October 20, 2026 21:37
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-20T21:47:59.734362Z ce431ea PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@melvin-bot
melvin-bot Bot requested review from inimaga and removed request for a team October 20, 2026 21:37
@melvin-bot

melvin-bot Bot commented Oct 20, 2026

Copy link
Copy Markdown

@inimaga Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines Coverage Δ
src/CONST/index.ts 95.90% <ø> (ø)
src/components/DatePicker/index.tsx 63.88% <100.00%> (-0.34%) ⬇️
...equestReportView/MoneyRequestReportActionsList.tsx 96.84% <100.00%> (ø)
...estReportView/MoneyRequestReportEmptyStateView.tsx 100.00% <100.00%> (ø)
...nents/Search/FilterDropdowns/FilterPopupButton.tsx 100.00% <100.00%> (ø)
src/libs/ReportActionsUtils.ts 84.14% <ø> (-0.21%) ⬇️
src/libs/ReportFieldOptionsListUtils.ts 96.15% <100.00%> (-3.85%) ⬇️
src/libs/actions/Report/index.ts 74.39% <ø> (ø)
src/styles/index.ts 66.22% <ø> (ø)
...rc/components/ReportActionItem/MoneyReportView.tsx 96.05% <87.50%> (-3.95%) ⬇️
... and 3 more
... and 10 files with indirect coverage changes

@JmillsExpensify JmillsExpensify left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No product review required

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce431eaf75

ℹ️ 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".

Comment on lines +147 to +148
if (value !== '') {
updateReportField({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Persist cleared optional text fields

When a deletable text report field is cleared, EditReportFieldText permits the empty value because the field is not required, but this guard skips updateReportField() and immediately navigates away. The previous value therefore remains stored, so users can replace the value but cannot clear it; allow an empty value for optional text fields while continuing to reject empty report titles.

Useful? React with 👍 / 👎.

Comment on lines +59 to +64
<OfflineWithFeedback
// Need to return undefined when we have pendingAction to avoid the duplicate pending action
pendingAction={pendingAction ? undefined : report?.pendingFields?.[reportField.fieldKey as keyof typeof report.pendingFields]}
errorRowStyles={styles.ph5}
key={`menuItem-${reportField.fieldKey}`}
onClose={() => clearReportFieldKeyErrors(report?.reportID, reportField.fieldKey)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Pass field update failures to OfflineWithFeedback

When a report-field update fails on a multi-expense report, updateReportField() rolls back the value and stores the failure in report.errorFields[fieldKey], but this wrapper no longer passes that error to OfflineWithFeedback. The field consequently reverts without displaying the stored failure or a dismiss control; restore the errors prop alongside pendingAction as is still done in MoneyReportView.

Useful? React with 👍 / 👎.

Comment on lines 2435 to 2437
if (!oldValue) {
return translate('report.actions.type.changeFieldEmpty', newValue, fieldName);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle cleared field actions as removals

For an OldDot CHANGE_FIELD action with a populated oldValue and an empty newValue, this now falls through to changeField and renders text such as changed Project to "" (previously "Apollo"). These actions can still arrive from other clients even while this commit prevents the local clear flow, so retain the removed-field branch and translation for accurate report history.

Useful? React with 👍 / 👎.

@daledah

daledah commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Let's give QA some more time to run additional tests. If there are only 1–2 blockers, I'll go ahead and create a follow-up PR soon

@daledah

daledah commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Currently there's only one blocker. Should I create a follow-up PR to fix it instead of reverting our PR?

Copy link
Copy Markdown
Contributor

I would prefer to try if there is just one.

@inimaga

inimaga commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

@daledah Could you raise the follow-up PR?

@daledah

daledah commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Melvin created a follow up PR here #104551
I'm reviewing it now

@daledah

daledah commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

@inimaga @JmillsExpensify @AndrewGable Here's the PR: #104551

It currently shows removed Text with no (previously "init text") for a text report field that has an initial value. That's because the backend's copy of the CHANGE_FIELD action comes back without an oldValue

Is that acceptable? If we want (previously "init text") here, the backend has to record the old value too. It can't be fixed on the client alone.

Screen.Recording.2026-10-21.at.18.36.28.mov

Copy link
Copy Markdown
Contributor

That looks fine to me.

@grgia

grgia commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Closing, CPing the fix #104551

@grgia grgia closed this Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants