Feature-7111-Unify column width definitions across spreadsheet components#8730
Open
Seth-Wadsworth wants to merge 2 commits intomakeplane:previewfrom
Open
Feature-7111-Unify column width definitions across spreadsheet components#8730Seth-Wadsworth wants to merge 2 commits intomakeplane:previewfrom
Seth-Wadsworth wants to merge 2 commits intomakeplane:previewfrom
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughThis PR extracts column width definitions and issue update logic into dedicated TypeScript modules for the spreadsheet layout. It centralizes width mappings for issue columns and header columns, along with a utility function for handling issue updates safely. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx`:
- Around line 54-56: The onChange callback is currently fire-and-forgeting the
promise from handleUpdateIssueLogic(updateIssue, issue, data) which can drop
rejections; change this to either make the callback async and await the call
inside a try-catch or explicitly attach .catch() to the returned promise, and in
the catch log the error (using the same logging mechanism as the component) and
surface a user-friendly failure path; update references are
handleUpdateIssueLogic and updateIssue so wrap that invocation in try { await
handleUpdateIssueLogic(updateIssue, issue, data); } catch (err) { /* log/handle
err */ } or at minimum handleUpdateIssueLogic(...).catch(err => { /* log/handle
err */ }).
In
`@apps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.logic.ts`:
- Around line 3-22: HEADER_COLUMN_WIDTHS duplicates COLUMN_WIDTHS causing drift;
remove the duplicate map and have getHeaderColumnWidth read from the single
source COLUMN_WIDTHS (e.g., replace HEADER_COLUMN_WIDTHS[property] with
COLUMN_WIDTHS[property] ?? "auto"), ensure COLUMN_WIDTHS is imported/available
in this module and typed compatibly with Partial<Record<keyof
IIssueDisplayProperties, string>>, and if HEADER_COLUMN_WIDTHS is exported
elsewhere update those consumers to use COLUMN_WIDTHS so there's only one
canonical widths map.
In
`@apps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsx`:
- Around line 39-42: The th element's className contains the Tailwind utility
"min-w-36" which forces a 9rem minimum and overrides the fixed widths from
getHeaderColumnWidth(property); remove "min-w-36" from the className and instead
drive minWidth from the same helper by adding a minWidth style (e.g., set style
to include width: getHeaderColumnWidth(property) and minWidth:
getHeaderColumnWidth(property)) so columns like state/priority/estimate can
actually render at the computed 80px width; update the th that uses
tableHeaderCellRef and getHeaderColumnWidth accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f478554-a1c1-4fe8-a32d-c257b8e8d008
📒 Files selected for processing (6)
apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.logic.tsapps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsxapps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.logic.tsapps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsxpackages/codemods/tests/issue-column.logic.test.tspackages/codemods/tests/spreadsheet-header-column.logic.test.ts
Comment on lines
+54
to
+56
| onChange={(issue, data) => { | ||
| void handleUpdateIssueLogic(updateIssue, issue, data); | ||
| }} |
Contributor
There was a problem hiding this comment.
Don’t fire-and-forget the issue update promise.
Line 55 drops rejections from the async update path, so a failed write can turn into an unhandled promise rejection with no user-visible recovery. Please catch/log the error here, or make the callback async and handle failures explicitly. As per coding guidelines, "Use try-catch with proper error types and log errors appropriately for error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx`
around lines 54 - 56, The onChange callback is currently fire-and-forgeting the
promise from handleUpdateIssueLogic(updateIssue, issue, data) which can drop
rejections; change this to either make the callback async and await the call
inside a try-catch or explicitly attach .catch() to the returned promise, and in
the catch log the error (using the same logging mechanism as the component) and
surface a user-friendly failure path; update references are
handleUpdateIssueLogic and updateIssue so wrap that invocation in try { await
handleUpdateIssueLogic(updateIssue, issue, data); } catch (err) { /* log/handle
err */ } or at minimum handleUpdateIssueLogic(...).catch(err => { /* log/handle
err */ }).
Comment on lines
+3
to
+22
| export const HEADER_COLUMN_WIDTHS: Partial<Record<keyof IIssueDisplayProperties, string>> = { | ||
| state: "80px", | ||
| priority: "80px", | ||
| assignee: "120px", | ||
| estimate: "80px", | ||
| labels: "80px", | ||
| start_date: "120px", | ||
| due_date: "120px", | ||
| created_on: "120px", | ||
| updated_on: "120px", | ||
| link: "80px", | ||
| attachment_count: "80px", | ||
| sub_issue_count: "80px", | ||
| cycle: "120px", | ||
| modules: "120px", | ||
| }; | ||
|
|
||
| export function getHeaderColumnWidth(property: keyof IIssueDisplayProperties): string { | ||
| return HEADER_COLUMN_WIDTHS[property] ?? "auto"; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep a single source of truth for column widths.
HEADER_COLUMN_WIDTHS duplicates COLUMN_WIDTHS entry-for-entry, so the next width change can desync header and body cells again. This refactor is supposed to centralize sizing; please reuse one shared map/helper here instead of maintaining a second copy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.logic.ts`
around lines 3 - 22, HEADER_COLUMN_WIDTHS duplicates COLUMN_WIDTHS causing
drift; remove the duplicate map and have getHeaderColumnWidth read from the
single source COLUMN_WIDTHS (e.g., replace HEADER_COLUMN_WIDTHS[property] with
COLUMN_WIDTHS[property] ?? "auto"), ensure COLUMN_WIDTHS is imported/available
in this module and typed compatibly with Partial<Record<keyof
IIssueDisplayProperties, string>>, and if HEADER_COLUMN_WIDTHS is exported
elsewhere update those consumers to use COLUMN_WIDTHS so there's only one
canonical widths map.
Comment on lines
39
to
42
| <th | ||
| className="h-11 min-w-36 items-center bg-layer-1 text-13 font-medium py-1 border border-b-0 border-t-0 border-subtle" | ||
| style={{ width: getHeaderColumnWidth(property) }} | ||
| ref={tableHeaderCellRef} |
Contributor
There was a problem hiding this comment.
min-w-36 is overriding the new fixed widths.
Line 40 still forces every header cell to at least 9rem, so columns like state, priority, and estimate will never render at the 80px width returned on Line 41. In a table layout that widens the whole column, not just the header. Remove that utility or drive minWidth from the same helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsx`
around lines 39 - 42, The th element's className contains the Tailwind utility
"min-w-36" which forces a 9rem minimum and overrides the fixed widths from
getHeaderColumnWidth(property); remove "min-w-36" from the className and instead
drive minWidth from the same helper by adding a minWidth style (e.g., set style
to include width: getHeaderColumnWidth(property) and minWidth:
getHeaderColumnWidth(property)) so columns like state/priority/estimate can
actually render at the computed 80px width; update the th that uses
tableHeaderCellRef and getHeaderColumnWidth accordingly.
Seth-Wadsworth
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors the spreadsheet layout to make column sizing more consistent and predictable across the issue list. The previous implementation contained duplicated width logic across header and row components, which led to inconsistent behavior and made future changes difficult to maintain. This update introduces a centralized getColumnWidth utility that determines column widths based on content characteristics, allowing flexible columns such as Title and Description to expand while keeping short, fixed‑width fields like ID, State, and Estimate static. The refactor removes inline width definitions, unifies sizing logic across components, and improves layout stability without introducing resizable columns, which were evaluated but determined to be incompatible with Plane’s dynamic rendering and MobX‑driven architecture.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
Testing included both unit tests and manual regression testing:
Unit tests were added for getColumnWidth to verify that static and flexible columns return the correct width values under different conditions.
Manual testing confirmed that header and row cells remain aligned across all column configurations.
Verified that flexible columns expand appropriately when additional content or new columns are introduced.
Ensured that no regressions occurred in virtualization behavior, horizontal scrolling, or responsive layout handling.
Confirmed that both header and row components now consume width values from the same centralized source.
References
#7111
Summary by CodeRabbit
Refactor
Tests