Repository navigation
Allow editing Kanban card properties inline - #9102
Open
elias-marrero-gm wants to merge 2 commits into
Open
elias-marrero-gm wants to merge 2 commits into
elias-marrero-gm wants to merge 2 commits into
Conversation
Contributor
Reviewer's GuideIntroduces desktop Kanban card inline property editing by adding an explicit editing session, reusing existing cell popovers, managing save/focus/outside-click behavior, and covering the feature with extensive Windows integration tests. Sequence diagram for inline Kanban card property editingsequenceDiagram
actor User
participant Card as RowCard
participant Bloc as CardBloc
participant Cell as BoardCardCell
participant Editor as EditableCellWidget
participant Popover as PropertyPopover
User->>Card: onStartEditing()
Card->>Bloc: setIsEditing(true)
Bloc-->>Card: showAllFieldsWhenEditing
User->>Cell: Tap property
Cell->>Editor: buildStyled()
Editor->>Popover: Open property editor
User->>Editor: Change value
User->>Card: Tap outside or DoneCardButton
Card->>Editor: unfocus()
Card->>Bloc: setIsEditing(false)
Bloc-->>Card: Restore visible fields
Flow diagram for Kanban card editing and focus navigationflowchart TD
A[Reading mode: visible properties] -->|Edit card| B[Editing mode: all properties]
B --> C[Click property control]
C --> D[Open existing cell editor or popover]
B --> E[Tab or Shift+Tab]
E --> F[Commit current field and move focus]
D --> G{Outside click}
G -->|Property popover open| H[Close popover and keep card editing]
G -->|No popover| I[Unfocus and finish editing]
B --> J[Click title]
J --> K[Edit title without opening card]
B --> L[OpenCardAccessory]
L --> I
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="frontend/appflowy_flutter/lib/plugins/database/board/presentation/board_page.dart" line_range="624-628" />
<code_context>
+ if (_isEditing) {
+ final String groupId = widget.afGroupData.id;
+ final String rowId = widget.groupItem.id;
+ WidgetsBinding.instance.addPostFrameCallback((_) {
+ if (!_boardActions.isClosed) {
+ _boardActions
+ .endEditing(GroupedRowId(groupId: groupId, rowId: rowId));
+ }
+ });
+ }
</code_context>
<issue_to_address>
**Disposed card ends newer edit**
When an edited card is disposed and another card starts editing before the deferred post-frame callback runs, the deferred callback calls `endEditing` without checking which row is currently being edited; `BoardActionsCubit.endEditing` emits an unscoped end-editing event, so every card listener ends its session, including a different card’s newly started session.
Before ending the session, verify that the disposed card is still the active edit target, or make the end-editing event target-aware and ignore it in other cards.
</issue_to_address>
### Comment 2
<location path="frontend/appflowy_flutter/lib/plugins/database/widgets/card/card.dart" line_range="198" />
<code_context>
+ // particular, closing the date picker keeps its saved selection
+ // and leaves the card open for further editing.
+ if (PopoverState.rootEntry.isEmpty) {
+ _finishEditing();
+ }
+ },
</code_context>
<issue_to_address>
**Unrelated popovers keep cards editing**
When an unrelated popover is open as the user clicks outside an editing card, `PopoverState.rootEntry` tracks popovers globally, so the condition skips `_finishEditing()` and the card remains in edit mode after the user clicks away.
Check whether a popover belonging to this card is open instead of checking the global popover stack.
</issue_to_address>
### Comment 3
<location path="frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart" line_range="964" />
<code_context>
+
+ final DateCellDataPB data =
+ DateCellDataPB.fromBuffer((await _getCell(card, field)).data);
+ expect(data.hasTimestamp(), isTrue);
+ expect(find.byType(RowDetailPage), findsNothing);
+
</code_context>
<issue_to_address>
**Wrong dates still pass**
When the date editor persists a timestamp for a different day than the one selected, when the date editor persists a timestamp for a different day than the one selected, the `set and clear a date from the board` test at line 964 and the corresponding checks at lines 576 and 965 only require `hasTimestamp()` to be true, so the tests pass while the saved date is wrong.
Assert that the persisted date matches the day selected in each test.
Also at `frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart:575`, `frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart:770`.
</issue_to_address>
### Comment 4
<location path="frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart" line_range="968-970" />
<code_context>
+ expect(find.byType(RowDetailPage), findsNothing);
+
+ await tester.tapButton(property);
+ await tester.clearDate();
+ await tester.dismissCellEditor();
+ await tester.pumpUntilFound(
+ find.descendant(of: property, matching: find.text(field.name)),
+ );
</code_context>
<issue_to_address>
**Clearing date is not verified**
When the clear action updates the editor's local display but leaves the saved cell unchanged, when the clear action updates only the editor's local display but leaves the saved cell unchanged, the `set and clear a date from the board` test at lines 967-972 only waits for the field-name placeholder, so it passes with the old date still persisted.
Read the cell after clearing and assert that its timestamp is absent.
</issue_to_address>
Comment on lines
+624
to
+628
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| if (!_boardActions.isClosed) { | ||
| _boardActions | ||
| .endEditing(GroupedRowId(groupId: groupId, rowId: rowId)); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Medium · Disposed card ends newer edit
When an edited card is disposed and another card starts editing before the deferred post-frame callback runs, the deferred callback calls endEditing without checking which row is currently being edited; BoardActionsCubit.endEditing emits an unscoped end-editing event, so every card listener ends its session, including a different card’s newly started session.
Before ending the session, verify that the disposed card is still the active edit target, or make the end-editing event target-aware and ignore it in other cards.
Prompt for AI agents
In `frontend/appflowy_flutter/lib/plugins/database/board/presentation/board_page.dart` at lines 624-628:
**Disposed card ends newer edit**
When an edited card is disposed and another card starts editing before the deferred post-frame callback runs, the deferred callback calls `endEditing` without checking which row is currently being edited; `BoardActionsCubit.endEditing` emits an unscoped end-editing event, so every card listener ends its session, including a different card’s newly started session.
Before ending the session, verify that the disposed card is still the active edit target, or make the end-editing event target-aware and ignore it in other cards.| // particular, closing the date picker keeps its saved selection | ||
| // and leaves the card open for further editing. | ||
| if (PopoverState.rootEntry.isEmpty) { | ||
| _finishEditing(); |
Contributor
There was a problem hiding this comment.
🟡 Medium · Unrelated popovers keep cards editing
When an unrelated popover is open as the user clicks outside an editing card, PopoverState.rootEntry tracks popovers globally, so the condition skips _finishEditing() and the card remains in edit mode after the user clicks away.
Check whether a popover belonging to this card is open instead of checking the global popover stack.
Prompt for AI agents
In `frontend/appflowy_flutter/lib/plugins/database/widgets/card/card.dart` at line 198:
**Unrelated popovers keep cards editing**
When an unrelated popover is open as the user clicks outside an editing card, `PopoverState.rootEntry` tracks popovers globally, so the condition skips `_finishEditing()` and the card remains in edit mode after the user clicks away.
Check whether a popover belonging to this card is open instead of checking the global popover stack.|
|
||
| final DateCellDataPB data = | ||
| DateCellDataPB.fromBuffer((await _getCell(card, field)).data); | ||
| expect(data.hasTimestamp(), isTrue); |
Contributor
There was a problem hiding this comment.
🟡 Medium · Wrong dates still pass
When the date editor persists a timestamp for a different day than the one selected, when the date editor persists a timestamp for a different day than the one selected, the set and clear a date from the board test at line 964 and the corresponding checks at lines 576 and 965 only require hasTimestamp() to be true, so the tests pass while the saved date is wrong.
Assert that the persisted date matches the day selected in each test.
Also at frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart:575, frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart:770.
Prompt for AI agents
In `frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart` at line 964:
**Wrong dates still pass**
When the date editor persists a timestamp for a different day than the one selected, when the date editor persists a timestamp for a different day than the one selected, the `set and clear a date from the board` test at line 964 and the corresponding checks at lines 576 and 965 only require `hasTimestamp()` to be true, so the tests pass while the saved date is wrong.
Assert that the persisted date matches the day selected in each test.
Also at `frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart:575`, `frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart:770`.
Comment on lines
+968
to
+970
| await tester.clearDate(); | ||
| await tester.dismissCellEditor(); | ||
| await tester.pumpUntilFound( |
Contributor
There was a problem hiding this comment.
🟡 Medium · Clearing date is not verified
When the clear action updates the editor's local display but leaves the saved cell unchanged, when the clear action updates only the editor's local display but leaves the saved cell unchanged, the set and clear a date from the board test at lines 967-972 only waits for the field-name placeholder, so it passes with the old date still persisted.
Read the cell after clearing and assert that its timestamp is absent.
Prompt for AI agents
In `frontend/appflowy_flutter/integration_test/desktop/board/board_property_edit_test.dart` at lines 968-970:
**Clearing date is not verified**
When the clear action updates the editor's local display but leaves the saved cell unchanged, when the clear action updates only the editor's local display but leaves the saved cell unchanged, the `set and clear a date from the board` test at lines 967-972 only waits for the field-name placeholder, so it passes with the old date still persisted.
Read the cell after clearing and assert that its timestamp is absent.This branch has not been deployed
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
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.
Feature Preview
Adds inline property editing to Kanban cards, allowing users to update properties without opening the card.
AppFlowy.1.mp4
Validation
The full suite is not passing locally:
PR Checklist
(I didn't fix an issue, I added some functionalities)
(Referenced in Validation)
Summary by Sourcery
Add inline Kanban card property editing with reliable saving, keyboard navigation, and card-opening behavior.
New Features:
Bug Fixes:
Enhancements:
Tests: