Skip to content

[CP Staging] fix: keep a capped column from claiming width it does not need - #102107

Merged
lakchote merged 6 commits into
Expensify:mainfrom
Krishna2323:krishna/102102-capped-column-overflow
Sep 24, 2026
Merged

lakchote merged 6 commits into
Expensify:mainfrom
Krishna2323:krishna/102102-capped-column-overflow

Conversation

@Krishna2323

@Krishna2323 Krishna2323 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

Fixed Issues

$ #102102
PROPOSAL:

Tests

  • Precondition:
    • Set first name as: AAAA BBBB CCCCCC, last name as applause.expensifail.com
    • Workspace has direct feed with two cards.
  1. Assign a card to yourself.
  2. Leave another card unassigned.
  3. Connect workspace to Sage Intacct.
  4. Go to staging.new.expensify.com
  5. Go to workspace settings > Company cards.
  6. Verify assign and arrow button on the right are within the table.
  • Verify that no errors appear in the JS console

Offline tests

  • Same as tests

QA Steps

  • Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Monosnap.screencast.2026-09-24.02-30-57.mp4

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323
Krishna2323 marked this pull request as ready for review September 23, 2026 21:02
@Krishna2323
Krishna2323 requested review from a team as code owners September 23, 2026 21:02
@melvin-bot
melvin-bot Bot requested review from a team, JmillsExpensify and situchan and removed request for a team September 23, 2026 21:02
@melvin-bot

melvin-bot Bot commented Sep 23, 2026

Copy link
Copy Markdown

@situchan 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]

@situchan

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-09-23.at.10.08.10.PM.mov

@melvin-bot
melvin-bot Bot requested a review from JS00001 September 23, 2026 21:13
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323

Copy link
Copy Markdown
Contributor Author

@situchan sorry, had to change the approach as the first one broke a few solver tests and would have narrowed capped columns on every dynamic table. Could you please re-test?

@codecov

codecov Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
...c/components/Table/calculateDynamicColumnWidths.ts 100.00% <100.00%> (ø)
src/components/Table/useDynamicColumnWidths.ts 9.52% <0.00%> (ø)
... and 34 files with indirect coverage changes

@MelvinBot

Copy link
Copy Markdown
Contributor

Reviewed at fa75c69 (the second commit). The fix works — 19/19 in calculateDynamicColumnWidthsTest.ts pass, and the Company cards case now resolves to [362, 180, 355, 154, 112] = 1163px. Two regressions and a few gaps below.

🔴 1. squeezeRatio can divide by zero, producing NaN grid tracks

The fall-through makes the squeeze branch reachable with totalDesiredWidth === totalMinWidth, which was impossible before (getting there used to require totalDesired > availableWidth >= totalMin). Then calculateDynamicColumnWidths.ts:191 is x / 0 = Infinity, and 0 * Infinity = NaN at line 199.

Ran it against this branch's source — constraints [{content: 70, min: 70, max: ∞}, {content: 30, min: 30, max: 40}] at availableWidth: 105:

widths                  : [NaN, NaN]
shouldScrollHorizontally: false

The ?? guards in the hook only catch null/undefined, so NaN flows through useDynamicColumnWidths.ts:189 into line 192 as "NaNpx" — an invalid track list, with shouldScrollHorizontally: false so there's no fallback either.

Reachability: I could not construct it from today's column configs. It needs every column non-squeezable (minWidth >= contentWidth) plus a capped column over-claiming, and on Company cards / Expensify cards the wide columns that make the row tight are exactly the squeezable ones. So it's latent, not live — but it's one line to close:

// A table that fell through from the fit branch has nothing to squeeze, so there is no ratio to work out.
if (totalSqueezableWidth <= 0) {
    return {widths: minWidths.map((minWidth) => Math.ceil(minWidth)), shouldScrollHorizontally: false};
}

🟡 2. Behavior 2's fairness rule is dropped for the whole table on fall-through

Line 75 states the design: split leftover equally, so an already-wide column doesn't also take the largest share of the slack. The squeeze path splits it in proportion to squeezable room, so one over-claiming column flips the entire table to the opposite rule.

It shows in your own new test: the actions column lands at exactly its content (112) while the 359px and 353px columns absorb the 5px of slack (→ 362, 355). Small here, larger when slack is larger.

The over-claim at line 100 is the actual defect — distributeAvailableWidth hands a capped column maxWidth when it only asked for desiredWidth. Your first commit fixed that directly and broke exactly one test (sizes the columns explicitly when a maximum is narrower than an equal share), whose 200 expectation just encodes the over-claim rather than a real requirement. Worth reconsidering — it also makes finding #1 impossible instead of latent.

🟡 3. Comments now describe the opposite of what happens

On the fall-through path the content does fit and squeezeRatio > 1, so columns gain room. But line 188 says "the content doesn't fit, so every column gives up room", line 168 says "even squeezed to their minimums the columns don't fit", and the behavior 3/4 list at the top is now wrong for this path. This file's comments are its documentation — they shouldn't contradict the code.

🟡 4. Test gaps

  • The new clamp is untested. In the new test it never binds (154 < 180, and the 180 column is min + 0 * ratio). A 40k-case sweep confirmed the clamp fires only on the fall-through path — so nothing in the suite exercises it. A case that does: [{300,300,∞}, {30,30,60}, {200,10,50}] at 400 → [320, 30, 50] with the clamp, [300, 30, 70] without (70px against a 50px max).
  • Math.floor(availableWidth) is untested — there is no test file for useDynamicColumnWidths at all.
  • No case covers the zero-squeezable fall-through from Some initial fixes and code style updates #1.

🔵 5. Minor: flooring can flip shouldScrollHorizontally

Math.floor at useDynamicColumnWidths.ts:143 is a good change, but it also shaves up to 1px off the budget that feeds totalMinWidth > availableWidth. At a sub-pixel boundary a table that previously just fit can now turn on horizontal scroll. Low risk, just flagging.

What I verified and what I didn't

Verified by running: the repo suite at fa75c69 (19/19 pass); the NaN result above, reproduced against a byte-identical copy of fa75c69's source (blob c71624b6); the clamp-binding case with and without the clamp; a 40,000-case randomized sweep over 2–4 column sets (128 divergences between clamped and unclamped, all on the fall-through path, all genuine maxWidth violations; 66 cases producing NaN).

Not verified: the UI itself. Reproducing it needs a workspace with a direct card feed plus a Sage Intacct connection, which a throwaway test account can't have, so I reviewed the layout math instead. Your recording covers the reported case.

Also confirmed: distributeAvailableWidth over-allocating is the real root cause. At the old code the Company cards constraints resolve to [359, 180, 353, 180, 112] = 1184px against a 1163px row — the 21px that pushed the Assign button out.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

@situchan

Copy link
Copy Markdown
Contributor

Tests good. But please check above ^

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-reviewed at 9af52d2. All three of my previous findings are resolved and CI is green. One new regression, plus two open gaps.

🔴 New: the squeeze can now return a negative width

The fall-through makes squeezeRatio > 1 reachable for the first time (calculateDynamicColumnWidths.ts:202). For a column where minWidth > contentWidth, desiredWidth - minWidth is negative, so line 210 drives the width below zero. The Math.min(…, maxWidth) clamp you added only bounds the top.

calculateDynamicColumnWidths([{contentWidth: 0, minWidth: 1, maxWidth: 2},
                              {contentWidth: 5, minWidth: 0, maxWidth: 8}], 6)

  9af52d2 -> widths: [-1, 7]
  main    -> widths: [2, 5]

All-integer input, and worst case in the sweep was -216px. main scores zero negatives across every sweep because the squeeze was only reachable with totalDesiredWidth > availableWidth, forcing squeezeRatio < 1 — which makes the expression a convex combination of two non-negative numbers.

Reachability: latent, same shape as the NaN one. No shipped column sets it — useDynamicColumnWidths always derives minWidth at or below contentWidth, and Search clamps the same way. But types.ts:71 exposes dynamicSizing.minWidth publicly, and the first column to set it above a narrow or empty column's content gets a negative CSS grid track — useDynamicColumnWidths renders ${width}px unguarded.

Cheapest fix is to close the bottom of the clamp the same way you closed the top:

return Math.min(Math.max(minWidth, minWidth + (desiredWidth - minWidth) * squeezeRatio), maxWidths.at(index) ?? 0);

That's a no-op on the real squeeze path (ratio < 1 already keeps the result at or above minWidth). Alternatively enforce minWidth <= desiredWidth once where minWidths is derived, which kills this and makes totalSqueezableWidth non-negative by construction.

🟡 Math.floor went to one of the two callers

useSearchColumnWidths.ts:154 builds its budget the same way and doesn't floor it, so roundWidths still hands it a fractional remainder — 38.5% of Search-shaped cases produce a fractional track, e.g. [246.39999999999998, 176, 90]. That hook's own comment says its widths are "applied as a flex basis that still grows and shrinks", so this may be deliberate rather than an oversight — but if a sub-pixel track is worth flooring for in one caller, it's worth a sentence explaining why the other doesn't need it.

Good news: the new fall-through path is unreachable from Search. Its hugged columns set contentWidth === minWidth === maxWidth and its free-text columns are uncapped, so no column can over-claim.

🟡 Both hook-level changes are still untested

There is no test file for useDynamicColumnWidths at all, so Math.floor(availableWidth) has no coverage. Everything added this round tests the pure resolver, which was already the well-covered half.

🔵 Nit: behavior 2's doc still promises what the fall-through breaks

You updated behavior 3 to mention the fall-through, but line 127 still says a long column "grows only as far as its content, rather than also claiming the largest share of the slack", and line 75 still gives the equal-split as the reason why. On the fall-through path the slack goes out in proportion to squeezable room instead — in your own new test the 359px and 353px columns take it (→ 362, 355) while the actions column stays at 112. Worth a qualifier on those two.

What I verified

Previous findings, all resolved: the totalSqueezableWidth <= 0 guard at line 195 closes the divide-by-zero, and the three new tests cover it, the clamp actually binding, and the Company cards case.

CI at 9af52d2: ESLint, typecheck, all 8 jest shards, Oxfmt, React Compiler — green. Only Check independent approval fails, which is expected.

Sweep: ~5.2M deterministic cases (seeded PRNG, 1–6 columns, fractional and integer, Infinity and finite caps, availableWidth from negative to far above total content), A/B'd against main:

Invariant Result
No NaN/Infinity ✅ 0 violations
No overflow when not scrolling ✅ 6,593 → 0 on integer input vs main; worst case 435px → 0
Scrolling covers the row ✅ 0 violations
Integer widths ✅ 0 violations for integer availableWidth (hence the Search note above)
No negative width ❌ 0 on main → 89/250k here
width <= maxWidth ⚠️ pre-existing, fractional maxWidth only, from the Math.ceil(minWidth) branches; 0 on integer input

So the fix does what it set out to do — the overflow that pushed the Assign button out is gone, and it's gone for the whole input space, not just the reported table.

Not verified: the UI. Reproducing it needs a workspace with a direct card feed plus Sage Intacct, which a throwaway test account can't have.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@JS00001

JS00001 commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

lmk when this one is ready please

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-reviewed at e12beb1. The negative-width regression is fixed, and I can't find a new one. CI is green (only Check independent approval fails, as expected) and 22/22 unit tests pass. Nothing blocking below — one doc nit and two gaps carried over.

✅ Negative width is gone

The reported input now resolves cleanly:

calculateDynamicColumnWidths([{contentWidth: 0, minWidth: 1, maxWidth: 2},
                              {contentWidth: 5, minWidth: 0, maxWidth: 8}], 6)

  e12beb1 -> [0, 6]   (sum 6)
  9af52d2 -> [-1, 7]  (negative)
  main    -> [2, 5]   (sum 7, overflows)

Zero negative widths across 2.4M randomized cases plus an exhaustive 171,875-case integer grid. Clamping minWidths to desiredWidth at calculateDynamicColumnWidths.ts:184 makes desiredWidth - minWidth non-negative by construction, which is the right place to fix it — it also makes totalSqueezableWidth non-negative for free.

Worth knowing: it's provably a no-op on well-formed input. When minWidth <= contentWidth and minWidth <= maxWidth, min(minWidth, desiredWidth) === min(minWidth, maxWidth). I A/B'd e12beb1 against 9af52d2 over 1.2M well-formed cases — zero output differences. So this commit can't have disturbed anything a real caller does.

🔵 The minWidth contract changed, and two docstrings still promise the old one

The flip side of the above: when a caller passes minWidth > contentWidth, that minimum is now silently dropped and the column can be squeezed below it. That's a defensible reading — the alternative in that regime was a negative track — but both docstrings still state the old promise without the caveat:

No shipped column sets dynamicSizing.minWidth, and both hooks derive it at or below content width, so this is inert today. But minWidth is public API and the next caller to set it above a narrow column's content gets the opposite of what the type says. One clause on each.

🟡 Still open: Math.floor went to one of the two callers

useSearchColumnWidths.ts:154 builds its budget the same way and still doesn't floor it. Possibly deliberate given that hook applies widths as a flex basis — but a sentence saying so would settle it.

🟡 Still open: the hook-level change has no test

There's no test file for useDynamicColumnWidths at all, so Math.floor(availableWidth) remains uncovered. This round again added only resolver tests, which was already the well-covered half.

What I verified, and one pre-existing thing I'd leave alone

Sweeps: 2.4M randomized cases (two seeds, 1–6 columns, integer and fractional, Infinity and finite caps, availableWidth from negative to 2.5× total content) plus an exhaustive 2-column integer grid, each A/B'd against main and 9af52d2. Versions were read straight from the git object store and blob-hash-verified before running.

Invariant e12beb1 vs 9af52d2
No NaN/non-finite equal (0)
No negative width 0 vs 664
No overflow when not scrolling 4,050 vs 9,138 (main: 15,648)
Integer widths for integer availableWidth equal (0)
width <= maxWidth 153,486 vs 197,720
Scrolling covers the row equal (0)
width >= caller minWidth worse in raw count, but identical on every well-formed input — all differences are minWidth > contentWidth

Pre-existing, not worth churn: Math.ceil on fractional minimums in the two early-return branches (line 188 and the zero-squeeze guard at line 201) can sum a hair above the row while reporting shouldScrollHorizontally: false. The guard adds one new instance of that pattern, but the total is still well below main (23,799 vs 31,315 per 1.2M well-formed cases), and it needs fractional minimums, which neither hook produces — everything is Math.ceil'd or an integer constant.

Not verified: the UI. Reproducing it needs a workspace with a direct card feed plus Sage Intacct, which a throwaway test account can't have.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323

Copy link
Copy Markdown
Contributor Author

@JS00001 ready for your review.

* 3. The columns need more room than the row has, so they are squeezed toward their minimum widths, in proportion to
* how much room each has to give up. Free-text columns truncate as they shrink. A column holding a known, short set
* of values has its content width as its minimum, so it keeps every value in full. A table whose content fits but
* whose capped columns claim more than the row holds is resolved here too.

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.

The inline comments are too long. Minify as much as possible

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-reviewed at a14d32c. No regressions — the only change since e12beb1 is the two docstrings, which close the nit I raised last round. No executable line moved, so the behavior I swept last time still stands, and CI is green (Jest Unit Tests at a14d32c, 22/22 in the resolver suite; only Check independent approval fails, as expected).

Two small accuracy points on the new wording, then the same two gaps.

🔵 "ignored" isn't what happens — it's capped

types.ts:72 says a minWidth above the content width "is ignored". The clamp at calculateDynamicColumnWidths.ts:187 caps it, which is a third outcome:

A free-text column with 200px of content defaults to a 120px minimum. A caller setting minWidth: 300 gets 200 — not 300 (honored), and not 120 (ignored).

So the value still raises the minimum, just no further than the content. "Capped at the column's content width" would say it.

🔵 The minimum is also capped by maxWidth

calculateDynamicColumnWidths.ts:10 names contentWidth as the ceiling, but the clamp is against desiredWidth, which is min(contentWidth, maxWidth). The maxWidth half predates this PR, so nothing is newly wrong — it's just the one bound the sentence leaves out. Optional.

🟡 Still open (unchanged, both pre-existing)

  • useSearchColumnWidths.ts:154 builds its budget the same way and still isn't floored. Fine if deliberate — that hook applies widths as a flex basis — but a sentence would settle it.
  • useDynamicColumnWidths still has no test file, so Math.floor(availableWidth) stays uncovered. All 22 tests are resolver tests, which was already the well-covered half.
What I checked this round

Diffed e12beb1...a14d32c — two files, comment hunks only, no executable change. That makes the resolver byte-identical in behavior to the version I swept last round (2.4M randomized cases plus an exhaustive 2-column integer grid, A/B'd against main and 9af52d2: zero NaN, zero negative widths, overflow-when-not-scrolling down from main's 15,648 to 4,050).

I re-read the other new comments against the code and they hold: behavior 2's "unless the columns resolve wider than the row, which hands the table to behavior 3", the fall-through note on distributeAvailableWidth, and behavior 3 absorbing the capped-overflow case all describe what the code does.

Not verified: the UI. Reproducing it needs a workspace with a direct card feed plus Sage Intacct, which a throwaway test account can't have.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@MelvinBot

Copy link
Copy Markdown
Contributor

Final review at 48825f5: no regressions found. For every input the app can actually produce, this is a strict improvement over main. Nothing blocking.

Since a14d32c only comments and docstrings changed — no executable line moved — so the behavior I swept last round still stands, and I re-swept anyway.

Evidence

  • 22/22 in calculateDynamicColumnWidthsTest.ts at head; CI green (only Check independent approval fails, as expected).
  • ~2.08M-case differential sweep, head vs main, 1–8 columns, seeded, covering negative/zero/fractional availableWidth, Infinity caps, minWidth > contentWidth, contentWidth > maxWidth, all-zero content.
  • Cases where head violates an invariant and main is valid: 0 (within the documented contract minWidth <= contentWidth).
  • With all-integer inputs — which is what the app passes — head has zero violations across 440k cases. main overflowed the row 14,935 times on the same inputs.
Worst case head main
Row overflow 6.5px (needs fractional input) 2541px
Width over max(content, available) 0.9px 5000px

Production inputs are all integers — measureColumnContentWidth and measureHeaderLabelWidth both Math.ceil, and every style constant feeding extraWidth is a whole px — so with Math.floor now applied at useDynamicColumnWidths.ts:143 the sub-pixel artifacts can't be reached at all. That change also closes a fractional-track artifact main already had.

The totalSqueezableWidth <= 0 guard at line 191 is load-bearing, not defensive — it changed the result in 17,904 of 400,000 cases, and 100% of those would otherwise have been Infinity/NaN. Good that it's there and tested.

Non-blocking follow-ups

  1. The root cause is still at line 100 — distributeAvailableWidth hands a capped column maxWidth even when that exceeds its desiredWidth. The PR detects the bad total at line 164 and discards the whole split instead. That's a defensible call given clamping at the source broke a solver test, and the guard + the new tests make it safe — but the overflow is still constructed and then thrown away. Worth a follow-up, not a change here.
  2. useDynamicColumnWidths still has no test file, so the Math.floor change is only covered indirectly.
  3. minWidth > contentWidth is now silently capped rather than rejected. Nothing sets dynamicSizing.minWidth today so it's unreachable, and the new docstrings say so — just flagging that a future caller passing one won't get feedback.
What I verified and what I didn't

Verified by running: the repo suite at 48825f5 (22/22); the differential sweep above, driving byte-verified copies of both the head and main versions (base blob b9ac9ebb69, matching the diff header); a guard-ablation build confirming the NaN path, e.g. availableWidth: 101, [{content: 0, min: 0, max: 3}, {content: 100, min: 100, max: ∞}] → head [0, 100], head-without-guard [Infinity, Infinity], main [3, 100] (sum 103, overflows).

Also confirmed: no column in the codebase sets dynamicSizing.minWidth, and both default paths (shouldFitContent ? contentWidth : Math.min(contentWidth, readableWidth)) are <= contentWidth. So the minWidths change at line 177 is a no-op for every table that exists today — it only guards the negative-width case.

Not verified: the UI. Reproducing it needs a workspace with a direct card feed plus a Sage Intacct connection, which a throwaway test account can't have, so I reviewed the layout math instead. Your recording and situchan's re-test cover the reported case.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

@lakchote lakchote changed the title fix: keep a capped column from claiming width it does not need [CP Staging] fix: keep a capped column from claiming width it does not need Sep 24, 2026

@lakchote lakchote 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.

LGTM

@lakchote
lakchote merged commit d23683b into Expensify:main Sep 24, 2026
35 of 36 checks passed
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions

Copy link
Copy Markdown
Contributor

🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here.

OSBotify pushed a commit that referenced this pull request Sep 24, 2026
…umn-overflow

[CP Staging] fix: keep a capped column from claiming width it does not need

(cherry picked from commit d23683b)

(cherry-picked to staging by lakchote)
@OSBotify OSBotify added the CP Staging marks PRs that have been CP'd to staging label Sep 24, 2026
@os-botify

os-botify Bot commented Sep 24, 2026

Copy link
Copy Markdown

🔁 Filed a Staging retest request for deploy blockers #102102 after this PR was cherry-picked to staging.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by https://github.com/lakchote in version: 9.4.92-7 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site changes are required, so I did not create a docs PR.

This PR only changes how the table grid resolves its column widths — calculateDynamicColumnWidths.ts, useDynamicColumnWidths.ts, a doc-comment touch-up in types.ts, and unit tests. The Company cards table keeps the same columns, labels, and buttons; the Assign button and the arrow just stay inside the table when a cardholder name is long.

Nothing a help article documents changed: no new or renamed feature, tab, setting, field, or button, and no change to any workflow a user follows.

What I checked
  • docs/HELP_AUTHORING_GUIDELINES.md requires every article to solve one primary user workflow, with task-based headings describing an action the user takes. A rendering fix has no such workflow to document.
  • Searched docs/articles/ for anything describing table column layout, resizing, truncation, or the arrow button. The only hit is a keywords line in Using-Reports-in-New-Expensify.md about the Search table's Edit columns feature, which this PR does not touch.
  • Reviewed the 14 Company cards articles (Assign company cards, Configure company card settings, Set up a direct connection, and the rest). They describe what admins do, not how the table lays out.

Separately, thanks for picking up the earlier review — the zero-denominator guard, capping minWidth at contentWidth, and the four new tests all look right to me.

Krishna2323 — no docs PR to review here. If you think a help article should call out something about the Company cards table that I missed, reply with the article and I'll draft the update.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/lakchote in version: 9.4.92-8 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by https://github.com/lakchote in version: 9.4.93-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site changes are required, so I did not create a docs PR.

This PR only changes internal layout math in the shared Table component — how the dynamic column widths are resolved in calculateDynamicColumnWidths.ts and useDynamicColumnWidths.ts, plus JSDoc on types.ts and unit tests.

Nothing in the diff adds, renames, moves, or removes anything a help article describes:

Docs trigger In this PR?
New or renamed feature, tab, setting, or button label No
Changed navigation path or workflow steps No
Changed behavior a member or admin has to act on No
New or changed error message No
Copy in src/languages/* No files touched

The user-visible effect is that the Assign button and caret now render inside the table instead of spilling past its right edge. Assign-Company-Cards.md and Configure-Company-Card-Settings.md describe the steps and settings, not the column layout, so both stay accurate before and after this fix.

What I checked
  • The full merged diff across all 6 commits (48825f5): only src/components/Table/calculateDynamicColumnWidths.ts, src/components/Table/types.ts, src/components/Table/useDynamicColumnWidths.ts, and tests/unit/calculateDynamicColumnWidthsTest.ts.
  • docs/articles/ for Company cards coverage — the relevant articles are Assign Company Cards and Configure Company Card Settings. Neither documents table columns, column order, or column widths.
  • No src/languages/* strings were added or changed, which is the usual signal that a UI label in the docs has gone stale.

I left out the "please review the linked help site PR" line from your template on purpose — there is no linked help site PR to review.

Separately, and not caused by this PR: Assign-Company-Cards.md step 4 says to select Assign card on the card row, but the row button string is Assign (en.ts:6436). I read that from the source strings, not the live UI, and it is pre-existing drift outside the scope of this request — worth a separate docs pass if you want it corrected.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/lakchote in version: 9.4.93-3 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CP Staging marks PRs that have been CP'd to staging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants