Repository navigation
Conversation
d2e4b1e to
66dde06
Compare
remko48
left a comment
remko48
left a comment
There was a problem hiding this comment.
Review (Thorough, round 2)
Each finding is an inline comment on the line it is about, marked 🔴 Blocker / 🟡 Concern / 🟢 Minor. This round takes the updated base stack into account (#13, #14 and #16 fixes, including 2ebf8c7, 2da6fa5 and 777c1b0); the diff of this PR itself is unchanged since it was rebased.
✅ Resolved by the base — saving no longer re-checks every target. 2da6fa5 keeps a target that matches the stored grid by widget id without checking the saver's access, and marks it broken if it is gone. GridView keeps widget ids, and SchemaForm writes targets as { type, id } in the stored key order, so the exemption applies. The only exception is the #18 blocker, which resets the target.
What was verified
- Read the full diff against
stack/g5-widget-forms, and confirmed it is byte-identical to round 1. - Traced the legacy
ViewController::update→ViewUpdateInput→ViewService::updatepath fortechnicalName, plusbuildDefaultTechnicalNameon create (2ebf8c7). - Checked the new
ApiViewsController::createsignature (777c1b0). - Checked
canManageElement/canManageTablefor views without a table. - Grepped every emitter and listener of the new events, in g6 and in
stack/m1–m4. - Checked
updateView's error handling, theloadContextViewpath thatView.vueuses, and the commit trailers.
Not run
eslint, stylelint, build, any tests, the app. Nothing was reproduced; every finding is from reading the code.
| - SPDX-License-Identifier: AGPL-3.0-or-later | ||
| --> | ||
| <template> | ||
| <div class="grid-view" data-cy="grid-view"> |
There was a problem hiding this comment.
🟡 Concern — No end-to-end coverage for the page flow
data-cy hooks were added throughout, but no Playwright spec uses them. The suite exists and already uses data-cy 514 times.
Impact: edit → add → configure → save → reload has no regression guard. That is exactly the path the #18 blocker and the technical-name blocker break.
Suggested fix: one Playwright spec covering that flow, including editing the grid view's title.
A grid view opens as a page of widgets. In edit mode widgets are added, configured, moved and removed, and the whole grid is saved at once. A dialog creates a grid view or edits its title, description and technical name. - grid views are kept out of the table view lists in the navigation - views without a table survive a reload of the tables in the store Co-authored-by: Ruben van der Linde <ruben@conduction.nl> Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
66dde06 to
5d8e00b
Compare
…om the navigation - the grid view dialog no longer sends a technical name, which the server refuses for a view without a table, and only sends the fields the create endpoint takes; its fields are labelled and Submit is disabled while saving - grid views get their own list in the navigation, with an action to create one, so the create flow has an entry point - leaving a grid page with unsaved changes asks first, and the browser warns before closing the tab - loading the tables keeps shared views and grid views but drops views of tables that are gone again - the grid page imports the layout helpers from gridLayout.js, disables move steps that cannot apply and hides them on a narrow canvas - a Playwright spec covers creating, editing, configuring and saving a page Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
|
🟡 Concern — The PR description no longer matches the code After 213235a the description says the opposite of what the PR does in two places:
Impact: reviewers and the changelog get the wrong picture, and showing grid views in the navigation is a visible product change that the description should state. Suggested fix: update both bullets. Also mention the guard for unsaved changes and the new Playwright spec. |
…instance The draft, the stored grid, the unsaved-changes check and the discard confirmation live in setup(), so the route guards use them directly and the options API reads the same state, without getCurrentInstance(). Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
Signed-off-by: Remko <remko@conduction.nl>
…type select in the e2e test - the grid views list now follows the applications, which the Cypress helper for creating an application addresses as the second list - the Playwright spec opens the widget type select from its indicator, like the other specs do, instead of clicking the component root Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
…test Escape only reaches the type select, which keeps the focus after a pick, so the dialog stayed open. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko <remko@conduction.nl>
1 participant
A grid view now opens as a page: its widgets on the grid, with an edit mode to add, configure, move, resize and remove widgets.
What changes
GridViewpage: "Edit page" works on a copy of the grid; "Save" sends the whole grid in one update and "Cancel" throws the copy away. The server validates the grid on save (see the earlier backend PR).GridViewSettingsdialog: create a grid view (title, emoji, description, optional technical name) or edit those fields on an existing one. Editing a grid view from its menu opens this dialog instead of the table view settings./view/{id}shows the grid page for a grid view; only users who can manage the view see the edit button.insertStandaloneViewcreates a view throughPOST /api/2/views. Reloading the tables no longer drops shared views and views without a table from the store.🖼️ Screenshots
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)