feat(content-explorer): Disable selection while editing #4317
Conversation
WalkthroughI pity the fool who misses this: renamed side-panel toggle prop to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sub as SubHeaderRight
participant CE as ContentExplorer
participant MSP as MetadataSidePanel
participant MVC as MetadataViewContainer
participant MV as MetadataView
Note over Sub,CE: User toggles side panel
User->>Sub: click toggle
Sub-->>CE: onSidePanelToggle()
CE->>CE: sidePanelState = OPEN
CE->>MSP: render(isEditing=false, onEditingChange)
CE->>MVC: render(isEditing=false)
MVC->>MV: areSelectionCheckboxesDisabled = false
Note over User,MSP: Enter edit mode
User->>MSP: click "Edit"
MSP-->>CE: onEditingChange(true)
CE->>CE: sidePanelState = EDITING
CE->>MVC: render(isEditing=true)
MVC->>MV: areSelectionCheckboxesDisabled = true
alt Selection cleared while editing
CE->>CE: handleSelectedIdsChange -> closeSidePanel() -> CLOSED
end
MSP-->>CE: Save/Cancel -> onEditingChange(false)
CE->>CE: sidePanelState = OPEN
CE->>MVC: render(isEditing=false)
sequenceDiagram
autonumber
participant CE as ContentExplorer
participant Sel as Selection Logic
participant Ext as External Consumer
CE->>Sel: validateSelectedItemIds(items, selectedIds)
Sel-->>CE: validSelectedIds
alt validSelectedIds empty
CE->>CE: closeSidePanel() -> CLOSED
else
CE-->>Ext: onSelectionChange(validSelectedIds)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
jfox-box
force-pushed
the
disable-select-on-edit
branch
from
214b841 to
7f73016
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.json(2 hunks)src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(3 hunks)src/elements/content-explorer/Content.tsx(3 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(4 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/elements/content-explorer/MetadataViewContainer.tsx
- src/elements/content-explorer/MetadataSidePanel.tsx
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/tests/MetadataSidePanel.test.tsx
- src/elements/common/sub-header/SubHeaderRight.tsx
⏰ 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: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
jfox-box
force-pushed
the
disable-select-on-edit
branch
from
7f73016 to
9f6d311
Compare
jfox-box
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1033-1043: validateSelectedItemIds converts 'all' to concrete Set.I see you kept the design decision to materialize
'all'into a concrete Set of currently loaded item IDs rather than preserving the sentinel value, fool! As noted in the past review comment, this means the selection is scoped to loaded items. If that's your intended behavior and you're comfortable with it, then I won't argue further!Based on past review discussion with @jfox-box.
🧹 Nitpick comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
174-178: State machine pattern improves clarity, fool!The
SidePanelStateenum with CLOSED, OPEN, and EDITING states replaces the previous boolean flag, fool! This makes the state transitions more explicit and prevents invalid state combinations (e.g., being both closed and editing).You could consider adding JSDoc comments to document the state machine transitions if this enum is used elsewhere in the codebase, but that's just a nice-to-have, fool!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(2 hunks)src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(3 hunks)src/elements/content-explorer/Content.tsx(3 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(4 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-sidebar/SidebarPanels.js(1 hunks)src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/elements/common/sub-header/SubHeaderRight.tsx
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/content-explorer/MetadataViewContainer.tsx
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(25-41)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 177-177: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 178-178: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🪛 Gitleaks (8.28.0)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
[high] 262-262: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (18)
src/elements/content-sidebar/SidebarPanels.js (1)
176-179: Formatting is solid, sucker!Looks clean and keeps Flow types intact. I pity the fool who messes with consistency!
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
264-279: The test logic looks solid, and Mr. T approves!The play function properly verifies that checkboxes get disabled when edit mode is active. The assertions are clear and test the right behavior - selecting all items, opening the metadata panel, starting edit mode, then confirming both the "Select all" control and individual row checkboxes are disabled. That's what I'm talkin' about!
290-310: Clean test flow for clearing selection - Mr. T likes it!This story correctly tests that clearing selection while editing closes the sidebar and deselects all items. The assertions verify Cancel/Save buttons disappear, the Mock Template is hidden, and both the "Select all" and row checkboxes are unchecked. Good job testing this edge case, fool!
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (3)
66-86: LGTM! TestWrapper correctly simulates external state management.The TestWrapper component properly manages the external editing state and propagates changes through the
onStateChangecallback, fool! This mirrors how ContentExplorer will control the editing state in production. The Notification.Provider wrapping ensures the side panel's notifications work correctly in tests.
89-89: Type definition correctly reflects external state management.The
Omitutility type properly excludesisEditingandonEditingChangefrom defaultProps since TestWrapper now manages these props, fool! This prevents duplicate state management and ensures tests align with the new external state model.
98-104: renderComponent helper properly wires TestWrapper.The helper function correctly merges props and passes them to TestWrapper along with the optional
onStateChangecallback, fool! This maintains test flexibility while centralizing the external state management pattern.src/elements/content-explorer/Content.tsx (3)
35-35: Optional prop with sensible default, fool!The
isEditing?: booleanprop is correctly optional with a default value offalseon line 63. This maintains backward compatibility while enabling the new editing state flow.
63-63: Default value ensures safe fallback behavior.Setting
isEditing = falseas the default ensures that when the prop isn't provided, the component behaves as if editing is not active, fool! This is the correct fallback behavior.
99-99: Prop correctly threaded to MetadataViewContainer.The
isEditingprop is properly forwarded to MetadataViewContainer on line 99, fool! This completes the prop threading from ContentExplorer through Content to the container.src/elements/content-explorer/MetadataSidePanel.tsx (4)
28-29: External state contract properly defined, fool!The interface correctly adds
isEditing: booleanandonEditingChange: (isEditing: boolean) => voidprops to externalize editing state management. This enables the parent component (ContentExplorer) to coordinate editing behavior across the UI.
46-47: Props correctly consumed throughout component.The component properly destructures the external state props and uses
isEditingin theuseTemplateInstancehook on line 62, fool! This ensures the template instance reflects the current editing mode.Also applies to: 62-62
64-74: State transitions correctly delegate to parent.All editing state changes now call
onEditingChangeinstead of managing internal state, fool! Lines 65, 69, and 74 properly delegate state transitions to the parent component for edit start, cancel, and discard operations.
77-88: Success handler correctly resets editing state.The success handler calls
onEditingChange(false)on line 87 to exit editing mode after successful metadata update, fool! This ensures the UI returns to view mode after save.src/elements/content-explorer/ContentExplorer.tsx (5)
202-202: State field correctly initialized.The state field renamed from
isMetadataSidePanelOpentosidePanelStateand initialized toSidePanelState.CLOSED, fool! This aligns with the new state machine model.Also applies to: 319-319
1751-1772: Selection gating during editing prevents data loss.The
handleSelectedIdsChangefunction correctly blocks selection changes whensidePanelState === SidePanelState.EDITING(unlessallowDuringEditingis true), fool! This prevents users from accidentally losing unsaved edits by changing selection. The function also:
- Propagates selection changes to external handlers via
onSelectionChangeExternal- Updates internal state with new selection
- Closes the side panel when selection becomes empty
The
allowDuringEditingparameter allowsclearSelectedItemIdsto bypass the editing check, which is necessary for programmatic clearing operations.
1780-1797: Side panel control methods correctly manage state transitions.The refactored methods properly manage the side panel state machine, fool!
onSidePanelToggle(lines 1780-1785) toggles between CLOSED and OPEN statescloseSidePanel(lines 1787-1791) sets state to CLOSEDonMetadataEditingChange(lines 1793-1797) switches between OPEN and EDITING based on theisEditingparameterThese methods form a clean API for controlling the side panel state.
1894-1895: Computed values correctly derived from state machine.The
isSidePanelOpenandisEditingvalues are correctly computed fromsidePanelState, fool! This ensures consistent behavior throughout the render logic.
1930-1930: Props correctly wired to child components.All child components receive the correct props based on the new state model, fool!
- Line 1930: SubHeader receives
onSidePanelToggle(renamed fromonMetadataSidePanelToggle)- Line 1946: Content receives
isEditingprop- Line 1984: Conditional rendering uses
isSidePanelOpen- Lines 1987-1990: MetadataSidePanel receives
isEditing,onClose(closeSidePanel), andonEditingChangecallbacksThe prop wiring correctly implements the external state management pattern throughout the component tree.
Also applies to: 1946-1946, 1984-1984, 1987-1990
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
jfox-box
force-pushed
the
disable-select-on-edit
branch
from
9f6d311 to
a1ab4cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
98-104: Add assertions for editing state transitionsExercise onStateChange by clicking Edit, then Cancel/Save, and assert [true] then [false]. Don’t just render it, fool—prove it.
src/elements/common/sub-header/SubHeaderRight.tsx (1)
110-113: Disable the button when no handler is providedIf
onSidePanelToggleis undefined, clicking does nothing. Considerdisabled={!onSidePanelToggle}to avoid dead controls, fool.src/elements/content-explorer/MetadataSidePanel.tsx (1)
28-30: Consider unsaved-changes guard on closeClosing the panel via the “X” or external close while editing skips the discard modal. Consider gating
onClosewhenisEditingand showing the same unsaved-changes flow, so users don’t lose work, sucka.src/elements/content-explorer/ContentExplorer.tsx (2)
1033-1039: Allow auto‑clear of selection during editing when it becomes empty
validateSelectedItemIdscallshandleSelectedIdsChange(validSelectedIds)but this is blocked during EDITING. If validation drops to empty, the panel won’t close. Let the auto‑clear through, fool.Apply this diff:
- if (!isEqual(validSelectedIds, selectedItemIds)) { - this.handleSelectedIdsChange(validSelectedIds); - } + if (!isEqual(validSelectedIds, selectedItemIds)) { + const allowDuringEditing = validSelectedIds !== 'all' && validSelectedIds.size === 0; + this.handleSelectedIdsChange(validSelectedIds, allowDuringEditing); + }Also applies to: 1041-1043
1780-1785: Optional: confirm on toggle while editingToggling while EDITING closes immediately. Consider prompting for unsaved changes to match the form’s discard flow, sucka.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(2 hunks)src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(3 hunks)src/elements/content-explorer/Content.tsx(3 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(4 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-sidebar/SidebarPanels.js(1 hunks)src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/content-explorer/MetadataViewContainer.tsx
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(25-41)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 177-177: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 178-178: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (8)
src/elements/content-sidebar/SidebarPanels.js (1)
176-179: LGTM, fool! Clean formatting update.I pity the fool who don't appreciate clean multi-line parameter formatting! This change improves readability and follows best practices with that trailing comma for easier git diffs.
The static analysis hints flagging "TypeScript only syntax" are false positives—this file uses Flow type annotations (note the
@flowcomment at the top), and Biome tool don't recognize Flow syntax properly, sucka!src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
66-86: Solid externalized editing wrapperClean TestWrapper with Notification context and onEditingChange plumbed right. I pity the fool who keeps state buried inside the component!
src/elements/common/sub-header/SubHeaderRight.tsx (1)
35-36: Prop rename wiring looks tight
onSidePanelTogglereplaces the old prop cleanly through destructure and usage. That’s clean, sucka.Also applies to: 56-57
src/elements/common/sub-header/SubHeader.tsx (1)
33-34: API rename is consistent end-to-endSubHeader now threads
onSidePanelToggledown to Right. Smooth and consistent. I pity the fool who leaves stale prop names.Also applies to: 60-61, 118-119
src/elements/content-explorer/MetadataSidePanel.tsx (1)
28-30: Externalized editing state is legitSwapping internal state for
isEditing/onEditingChangeis clean. Handlers flip state correctly on edit/cancel/submit. That’s how you do it, fool.Also applies to: 46-48, 65-76, 87-88
src/elements/content-explorer/ContentExplorer.tsx (3)
174-179: SidePanelState refactor is strongClear CLOSED/OPEN/EDITING states and default wiring. I pity the fool who still juggles booleans.
Also applies to: 202-203, 319-320
1751-1768: Good: block selection changes while editingGating selection updates during EDITING matches the goal. Add tests to prove grid/select-all changes are ignored until edit exits, fool.
1894-1896: Render wiring looks consistent
- Derived
isSidePanelOpen/isEditingare used correctly.onSidePanelToggleflows from SubHeader to panel visibility.isEditingpassed to Content and MetadataSidePanel withonEditingChange.Tidy state flow, fool.
Also applies to: 1930-1935, 1946-1947, 1984-1991
jfox-box
force-pushed
the
disable-select-on-edit
branch
from
ec5cceb to
fd34f1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/elements/content-explorer/Content.tsx (1)
95-104: Fool, you only fixed HALF the problem!The previous review told you to move
{...metadataViewProps}BEFOREisEditing={isEditing}so the explicit prop wins. Right now line 103 spreads AFTER line 99, so a caller can still overrideisEditingat runtime even though you fixed the type on line 43. I pity the fool who ships this—selection-disabling while editing can still be clobbered!Apply this diff to enforce precedence:
<MetadataViewContainer currentCollection={currentCollection} isLoading={percentLoaded !== 100} hasError={view === VIEW_ERROR} + {...metadataViewProps} isEditing={isEditing} metadataTemplate={metadataTemplate} onMetadataFilter={onMetadataFilter} onSortChange={onSortChange} - {...metadataViewProps} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-explorer/Content.tsx(3 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-08T20:47:32.413Z
Learnt from: jfox-box
PR: box/box-ui-elements#4317
File: src/elements/content-explorer/ContentExplorer.tsx:1033-1043
Timestamp: 2025-10-08T20:47:32.413Z
Learning: In ContentExplorer.tsx, the `validateSelectedItemIds` method converts the 'all' selection sentinel to a concrete Set of item IDs. This is intentional to prevent selection from expanding when the collection grows. For example, if 3 items are all selected and the collection refreshes with 10 items, only the original 3 should remain selected, not all 10. This design keeps selection sticky to specific items rather than allowing it to automatically include newly appearing items.
Applied to files:
src/elements/content-explorer/ContentExplorer.tsx
🧬 Code graph analysis (1)
src/elements/content-explorer/Content.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
MetadataViewContainerProps(108-116)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (16)
src/elements/content-explorer/Content.tsx (3)
35-35: LGTM, fool!Adding
isEditingto the interface is the right move. I pity the fool who tries to disable selection without tracking edit state! This prop enables the parent component to control editing behavior.
43-43: Good type update, but y'all ain't done yet!I see you updated the type to omit
isEditingfrommetadataViewProps, which is exactly what the previous review asked for. That's progress, fool! But the spread order on lines 99-103 still lets callers clobber theisEditingprop at runtime. Check my comment on lines 95-104.
63-63: LGTM!Destructuring
isEditingwith afalsedefault is clean and correct. I pity the fool who forgets to pass this prop—at least it won't break!src/elements/content-explorer/ContentExplorer.tsx (13)
174-178: Clean enum design, fool!Using
SidePanelStatewith three explicit states (CLOSED, OPEN, EDITING) is way better than juggling booleans. I pity the fool who tries to track editing state with multiple boolean flags—that's a recipe for bugs! This makes the state machine clear and maintainable.
202-202: LGTM!Migrating from
isMetadataSidePanelOpen: booleantosidePanelState: SidePanelStateis the right move. The enum captures all three states cleanly.
319-319: Good initialization!Starting with
SidePanelState.CLOSEDis correct. I pity the fool who forgets to initialize state properly!
1033-1043: LGTM—this approach is intentional!I see you're converting the
'all'sentinel to a concreteSetof IDs. Based on the learning from your previous explanation, this prevents selection from expanding when the collection grows (e.g., 3 selected items shouldn't balloon to 10 when collection refreshes). The design keeps selection sticky to specific items, which is what you want.But check my comment on the
handleSelectedIdsChangecall at line 1042—there's an issue with validation during editing!Based on learnings.
1751-1768: Smart gating logic, fool!The
handleSelectedIdsChangemethod correctly blocks selection changes during editing (unlessallowDuringEditingis true). I pity the fool who tries to multi-select while editing metadata—that's confusing UX! The logic:
- Returns early if editing and not allowed (lines 1754-1756)
- Calls external handler (line 1758)
- Updates state (lines 1760-1762)
- Closes panel when selection is empty (lines 1764-1768)
This is solid! Just fix the validation issue I flagged at lines 1030-1044.
1770-1772: LGTM!
clearSelectedItemIdspassingallowDuringEditing: truemakes sense—users should be able to clear selection even while editing. I pity the fool who traps users in editing mode without an escape hatch!
1780-1785: Clean toggle logic!
onSidePanelToggle(renamed fromonMetadataSidePanelToggle) toggles between CLOSED and OPEN, never touching EDITING. That's correct—the toggle button shouldn't put you in edit mode, fool!
1787-1791: LGTM!
closeSidePanel(renamed fromcloseMetadataSidePanel) simply sets state to CLOSED. Clean and straightforward.
1793-1801: Good editing transition handler!
onEditingChangeswitches between OPEN and EDITING states when the panel is open. The guard on line 1796 ensures a closed panel doesn't reopen, which is correct—I pity the fool who lets a callback reopen a panel the user just closed!
1652-1672: Wiring looks solid!Line 1664 wires up
handleSelectedIdsChangeasonSelectionChange, so selection changes from the metadata view get intercepted and gated during editing. This is the key integration point for the "disable selection while editing" feature.
1515-1515: Good cleanup!
closeModalsnow resetssidePanelStateto CLOSED, ensuring the panel closes when any modal opens. I pity the fool who forgets to reset state in cleanup methods!
1868-1868: Clean derived state!Lines 1898-1899 compute
isSidePanelOpenandisEditingfromsidePanelState. Deriving booleans from the enum keeps the render logic readable while maintaining a single source of truth. Smart move, fool!Also applies to: 1898-1899
1934-1934: Render wiring looks good!The render method correctly:
- Passes
onSidePanelToggleto SubHeader (line 1934)- Passes
isEditingto Content (line 1950)- Conditionally renders MetadataSidePanel when
isSidePanelOpen(line 1988)- Passes
isEditing,onClose, andonEditingChangeto MetadataSidePanel (lines 1991, 1993-1994)I pity the fool who messes up prop threading through this many components—but you got it right!
Also applies to: 1950-1950, 1988-1999
jfox-box
force-pushed
the
disable-select-on-edit
branch
from
90b8836 to
2d92ca1
Compare
jfox-box
force-pushed
the
disable-select-on-edit
branch
from
2d92ca1 to
96e5d3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
253-278: Stabilize async checks when disabling selection in edit modeI pity the fool who trusts instant DOM updates! Wrap the disabled assertions in waitFor and await the first Select all query to avoid flake, sucka.
play: async ({ canvas }) => { - await waitFor(() => { - expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); - }); + await waitFor(() => { + expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); + }); // Start editing - await userEvent.click(canvas.getByLabelText('Select all')); + const selectAll = await canvas.findByLabelText('Select all'); + await userEvent.click(selectAll); await userEvent.click(canvas.getByRole('button', { name: 'Metadata' })); await userEvent.click(canvas.getByLabelText('Edit templateName')); expect(canvas.getByRole('button', { name: 'Cancel' })).toBeInTheDocument(); expect(canvas.getByRole('button', { name: 'Save' })).toBeInTheDocument(); // Verify checkboxes are disabled - expect(canvas.getByLabelText('Select all')).toBeDisabled(); - expect(within(canvas.getByRole('row', { name: /Child 2/i })).getByRole('checkbox')).toBeDisabled(); + await waitFor(() => { + expect(canvas.getByLabelText('Select all')).toBeDisabled(); + expect( + within(canvas.getByRole('row', { name: /Child 2/i })).getByRole('checkbox'), + ).toBeDisabled(); + }); },
288-309: Wait for UI to settle after “Clear selection” and assert re‑enableI pity the fool who makes brittle tests! After clearing, wait for the panel to close, then assert checkboxes are re‑enabled and unchecked.
- // Clear selection in subheader - await userEvent.click(canvas.getByLabelText('Clear selection')); - - // Verify sidebar is closed and no items are selected - expect(canvas.queryByRole('button', { name: 'Cancel' })).not.toBeInTheDocument(); - expect(canvas.queryByRole('button', { name: 'Save' })).not.toBeInTheDocument(); - expect(canvas.queryByText('Mock Template')).not.toBeInTheDocument(); - - expect(canvas.getByLabelText('Select all')).not.toBeChecked(); - expect(within(canvas.getByRole('row', { name: /Child 2/i })).getByRole('checkbox')).not.toBeChecked(); + // Clear selection in subheader + await userEvent.click(canvas.getByLabelText('Clear selection')); + + // Verify sidebar is closed and selection controls are reset and re-enabled + await waitFor(() => { + expect(canvas.queryByRole('button', { name: 'Cancel' })).not.toBeInTheDocument(); + expect(canvas.queryByRole('button', { name: 'Save' })).not.toBeInTheDocument(); + expect(canvas.queryByText('Mock Template')).not.toBeInTheDocument(); + + const selectAll = canvas.getByLabelText('Select all'); + expect(selectAll).toBeEnabled(); + expect(selectAll).not.toBeChecked(); + + const rowCheckbox = within(canvas.getByRole('row', { name: /Child 2/i })).getByRole('checkbox'); + expect(rowCheckbox).toBeEnabled(); + expect(rowCheckbox).not.toBeChecked(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(2 hunks)src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(3 hunks)src/elements/content-explorer/Content.tsx(3 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(4 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-sidebar/SidebarPanels.js(1 hunks)src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/elements/content-explorer/MetadataViewContainer.tsx
- package.json
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/ContentExplorer.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(25-41)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 177-177: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 178-178: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
35-35: I ain't got no time for inconsistent prop names, fool! This rename is tight.The prop rename from
onMetadataSidePanelToggletoonSidePanelToggleis consistent across the interface definition (line 35), destructuring (line 56), and button handler (line 110). The more generic name better reflects the broader side panel usage pattern, sucka. Nice work keeping it clean!Also applies to: 56-56, 110-110
src/elements/content-explorer/MetadataSidePanel.tsx (2)
28-29: I pity the fool who doesn't lift state when it needs lifting, and you ain't that fool!The addition of
isEditingandonEditingChangeprops is textbook state lifting, jack. The component now delegates editing state management to its parent (ContentExplorer), which gives better control over selection behavior during editing. The prop signatures are clean and follow React conventions, sucka!Also applies to: 46-47
65-65: Every state transition is accounted for, fool! That's how you do it!All internal
setIsEditingcalls have been properly replaced withonEditingChangecallbacks. The state transitions are complete and consistent:
- Line 65: Enters edit mode when user clicks edit
- Lines 69, 74: Exits edit mode on cancel/discard
- Line 87: Exits edit mode after successful update
I ain't seeing no orphaned state mutations, which means this refactor is clean, jack!
Also applies to: 69-69, 74-74, 87-87
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
66-86: I pity the fool who don't update their tests when they lift state, but you ain't that fool!The test infrastructure properly adapts to the external state management pattern:
- TestWrapper (lines 66-86) encapsulates the
isEditingstate that now lives in the parent, providing a clean test harness, sucka- defaultProps typing (line 89) correctly uses
Omitto exclude the externally-managed props- renderComponent (lines 98-104) properly wraps the component and allows optional state change observation
- The onStateChange callback is a nice touch for tests that need to verify state transitions, jack
All existing tests remain green with this setup. That's some solid test engineering right there!
Also applies to: 89-89, 98-104
Screen.Recording.2025-10-08.at.2.09.43.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores