feat(viewboxes): open the new table's configuration when it is added #535

Merged

allan merged 3 commits from fix/viewbox-focus-on-add into release/7.23 2026-11-08 19:09:02 +00:00

Conversation 5 Commits 3 Files Changed 2

+42 -2

hermes commented 2026-11-08 17:25:52 +00:00

Collaborator

Copy link

Copy source

#533: choosing a table in the Viewboxes panel now moves to the right hand side, with that table's configuration open.

What changed

tableOnClick (viewboxes.component.ts:259) already added the viewbox and loaded its data. After that it now:

• selects the new viewbox with resetSelectedViewbox - the same call the row's own click makes - and forces change detection, because the addition is async and the row is new;
• scrolls its row into view, so a second or third viewbox does not appear off-screen.

Each open viewbox row carries id="open-viewbox_<id>" for that lookup. The tree is untouched: it stays usable for adding another table, and the selection lives on the right.

Verified

Build clean, karma 896 SUCCESS.

Not verified, and the reason matters

The visual check did not happen, and not for want of trying: the local estate is on the FREE tier, so the first added viewbox trips "Viewbox open limit reached" and tableOnClick returns at that guard before reaching this code. The panel therefore never shows an added row to observe - every screenshot I took shows the limit notice instead. I also found that opening a table from the navigation tree already creates viewbox #1, which is what put the local estate at the limit.

So this needs either a licence key with a viewbox limit above one, or CI, where the Cypress runs carry a licence. If you tell me which, I will do the four-state check from #533 properly - empty, one added, two added, and a third while the list is scrolled.

#533: choosing a table in the Viewboxes panel now moves to the right hand side, with that table's configuration open. **What changed** `tableOnClick` (`viewboxes.component.ts:259`) already added the viewbox and loaded its data. After that it now: - selects the new viewbox with `resetSelectedViewbox` - the same call the row's own click makes - and forces change detection, because the addition is async and the row is new; - scrolls its row into view, so a second or third viewbox does not appear off-screen. Each open viewbox row carries `id="open-viewbox_<id>"` for that lookup. The tree is untouched: it stays usable for adding another table, and the selection lives on the right. **Verified** Build clean, `karma` 896 SUCCESS. **Not verified, and the reason matters** The visual check did not happen, and not for want of trying: the local estate is on the FREE tier, so the first added viewbox trips **"Viewbox open limit reached"** and `tableOnClick` returns at that guard before reaching this code. The panel therefore never shows an added row to observe - every screenshot I took shows the limit notice instead. I also found that opening a table from the *navigation* tree already creates viewbox #1, which is what put the local estate at the limit. So this needs either a licence key with a viewbox limit above one, or CI, where the Cypress runs carry a licence. If you tell me which, I will do the four-state check from #533 properly - empty, one added, two added, and a third while the list is scrolled.

hermes added 1 commit 2026-11-08 17:25:53 +00:00

feat(viewboxes): open the new table's configuration when it is added ... 87b6b84b5f

Choosing a table in the panel's tree adds the viewbox on the right hand side,
where the filters and column selection live - but nothing moved the user there,
so the tree row they had just used stayed under the pointer and the
configuration stayed on "select viewbox from above".

The new viewbox is now selected once its table is registered - the same
resetSelectedViewbox the row's own click runs, forced through change detection
because the addition is async - and its row is scrolled into view, so a second or
third viewbox does not appear off-screen. Each open viewbox row carries an id
(open-viewbox_<id>) to make that lookup possible.

Deliberately not done here: collapsing the tree, or changing the tree's own
selection. The tree stays usable for adding another viewbox; the selection is on
the right hand side.

Karma: 896 SUCCESS. The visual check did not run: the local estate is on the FREE
tier, where a second viewbox trips "Viewbox open limit reached" and tableOnClick
returns before this code, so the panel never shows an added row to observe. CI
runs with a licence key and is where this gets looked at.

hermes reviewed 2026-11-08 17:55:34 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

Reviewed the full diff of #535 at head 87b6b84b5f: the row id in the template and the deferred select-and-scroll in tableOnClick. CI at this head is not green yet - Build-and-ng-test is success, Build-and-test-development and lighthouse are pending - so this is code review only. Two findings.

• client/src/app/shared/viewboxes/viewboxes.component.ts:298 const row = document.getElementById('open-viewbox_' + viewbox.id) - the lookup is only correct while viewbox.id is unique, and it is not. The id is minted at :269 as id: this.viewboxes.length + 1, and removal at :1258 is this.viewboxes.splice(index, 1) with no renumbering: two viewboxes (ids 1, 2), remove the first, add another and the new one is id 2 again. Two rows then carry open-viewbox_2, getElementById answers the first in document order, so the older row is scrolled into view - and resetSelectedViewbox (:1392, viewboxTables.find((vbt) => vbt.viewboxId === viewbox.id)) selects the older entry's table as well. Mint the id from a monotonic counter that never reuses a value, or scope the lookup to this component's own rows instead of a document-wide id.

• same file :295-300 }, 1200) - the delay the comment above it justifies does not exist: it says "the same one the table build uses before the viewbox table is registered", and the waits in that path are 100 and 500 ms (:492, :497); 1200 appears nowhere else in the file. A fixed wall-clock guess also misses at both ends - the row exists once the view has rendered (detectChanges() is what makes it so), so the highlight and scroll land 1.2 s after the click, and if the load takes longer than 1.2 s the pane still has no table. Select and scroll right after snapToGrid(), and let the re-select already inside loadData's 500 ms callback (:496, if (this.selectedViewbox) this.resetSelectedViewbox(viewbox)) populate the configuration pane once the table is registered. If a timer stays, clear it in ngOnDestroy - the component is destroyed by ordinary navigation, and this.cdf.detectChanges() on a destroyed view throws.

2 findings above for review.

Reviewed the full diff of #535 at head 87b6b84b5f: the row id in the template and the deferred select-and-scroll in `tableOnClick`. CI at this head is not green yet - Build-and-ng-test is success, Build-and-test-development and lighthouse are pending - so this is code review only. Two findings. - client/src/app/shared/viewboxes/viewboxes.component.ts:298 `const row = document.getElementById('open-viewbox_' + viewbox.id)` - the lookup is only correct while `viewbox.id` is unique, and it is not. The id is minted at :269 as `id: this.viewboxes.length + 1`, and removal at :1258 is `this.viewboxes.splice(index, 1)` with no renumbering: two viewboxes (ids 1, 2), remove the first, add another and the new one is id 2 again. Two rows then carry `open-viewbox_2`, `getElementById` answers the first in document order, so the older row is scrolled into view - and `resetSelectedViewbox` (:1392, `viewboxTables.find((vbt) => vbt.viewboxId === viewbox.id)`) selects the older entry's table as well. Mint the id from a monotonic counter that never reuses a value, or scope the lookup to this component's own rows instead of a document-wide id. - same file :295-300 `}, 1200)` - the delay the comment above it justifies does not exist: it says "the same one the table build uses before the viewbox table is registered", and the waits in that path are 100 and 500 ms (:492, :497); 1200 appears nowhere else in the file. A fixed wall-clock guess also misses at both ends - the row exists once the view has rendered (`detectChanges()` is what makes it so), so the highlight and scroll land 1.2 s after the click, and if the load takes longer than 1.2 s the pane still has no table. Select and scroll right after `snapToGrid()`, and let the re-select already inside `loadData`'s 500 ms callback (:496, `if (this.selectedViewbox) this.resetSelectedViewbox(viewbox)`) populate the configuration pane once the table is registered. If a timer stays, clear it in `ngOnDestroy` - the component is destroyed by ordinary navigation, and `this.cdf.detectChanges()` on a destroyed view throws. 2 findings above for review.

gitea-actions Bot commented 2026-11-08 17:58:17 +00:00

Copy link

Copy source

Lighthouse

Route	Performance	Accessibility	Best practices	SEO
#/home/tables	0.84	0.93	1.00	1.00
#/editor/DC996664.MPE_X_TEST	0.31	0.96	1.00	1.00
#/view/data	0.74	0.92	1.00	1.00
#/view/data/DC996664	0.73	0.92	1.00	1.00
#/view/data/DC996664.MPE_X_TEST	0.54	0.92	1.00	1.00
#/view/usernav/groups	0.73	0.92	1.00	1.00
#/view/usernav/groups/1	0.72	0.92	1.00	1.00
#/view/usernav/users/1	0.70	0.92	1.00	1.00
#/home/excel-maps	0.79	0.92	1.00	1.00
#/home/excel-maps/BASEL-CR2	0.73	0.87	1.00	1.00
#/home/multi-load	0.81	0.90	1.00	1.00
#/review/submitted	0.88	0.91	1.00	1.00
#/review/approve	0.86	0.91	1.00	1.00
#/review/history	0.87	0.91	1.00	1.00
#/stage/DC20221006T142649516_059582_7169	0.87	0.96	1.00	0.92
#/review/submitted/DC20221006T142649516_059582_7169	0.85	0.91	1.00	1.00
#/system	0.90	0.93	1.00	1.00

Lowest accessibility score: 0.87 (the gate is a median of 0.85 while issue 497 is open).

Run: https://git.datacontroller.io/dc/dc/actions/runs/3560

<!-- lighthouse-report --> ### Lighthouse | Route | Performance | Accessibility | Best practices | SEO | |---|---|---|---|---| | `#/home/tables` | 0.84 | 0.93 | 1.00 | 1.00 | | `#/editor/DC996664.MPE_X_TEST` | 0.31 | 0.96 | 1.00 | 1.00 | | `#/view/data` | 0.74 | 0.92 | 1.00 | 1.00 | | `#/view/data/DC996664` | 0.73 | 0.92 | 1.00 | 1.00 | | `#/view/data/DC996664.MPE_X_TEST` | 0.54 | 0.92 | 1.00 | 1.00 | | `#/view/usernav/groups` | 0.73 | 0.92 | 1.00 | 1.00 | | `#/view/usernav/groups/1` | 0.72 | 0.92 | 1.00 | 1.00 | | `#/view/usernav/users/1` | 0.70 | 0.92 | 1.00 | 1.00 | | `#/home/excel-maps` | 0.79 | 0.92 | 1.00 | 1.00 | | `#/home/excel-maps/BASEL-CR2` | 0.73 | 0.87 | 1.00 | 1.00 | | `#/home/multi-load` | 0.81 | 0.90 | 1.00 | 1.00 | | `#/review/submitted` | 0.88 | 0.91 | 1.00 | 1.00 | | `#/review/approve` | 0.86 | 0.91 | 1.00 | 1.00 | | `#/review/history` | 0.87 | 0.91 | 1.00 | 1.00 | | `#/stage/DC20221006T142649516_059582_7169` | 0.87 | 0.96 | 1.00 | 0.92 | | `#/review/submitted/DC20221006T142649516_059582_7169` | 0.85 | 0.91 | 1.00 | 1.00 | | `#/system` | 0.90 | 0.93 | 1.00 | 1.00 | Lowest accessibility score: **0.87** (the gate is a median of 0.85 while issue 497 is open). Run: https://git.datacontroller.io/dc/dc/actions/runs/3560

hermes added 1 commit 2026-11-08 18:11:20 +00:00

fix(viewboxes): mint viewbox ids that cannot repeat, and select without a timer ... 0bd553fb00

Two review findings, both right:

- `id: this.viewboxes.length + 1` was not unique. Removal does not renumber, so
  with two viewboxes, remove the first and add another and the new one is 2
  again: two rows carried `open-viewbox_2`, getElementById answered the first in
  document order, and `resetSelectedViewbox` then selected the older entry's
  table as well. Ids come from a counter that takes the highest existing id into
  account every time, so a restored viewbox cannot be collided with either.
- The 1200ms delay was a guess, and the comment justified it with waits that do
  not exist - the table build waits 100 and 500 ms, and 1200 appears nowhere.
  `detectChanges` is what puts the row in the view, so selecting and scrolling
  straight after it is the earliest point that works and does not guess how long
  the table takes; loadData's existing callback re-selects the viewbox once its
  table is registered, which is what fills the pane.

The row lookup is scoped to the component's own Currently open list through a
ViewChild rather than a document-wide id, which is the other half of the
duplicate-id problem.

Build clean, karma 896 SUCCESS.

hermes commented 2026-11-08 18:11:33 +00:00

Author

Collaborator

Copy link

Copy source

Both, at 0bd553fb.

The id. You are right that it was not unique, and the consequence was worse than a stray scroll: resetSelectedViewbox looks the table up by viewboxId, so the older entry's table would have been selected too - the wrong configuration pane, not just the wrong row. Ids now come from a counter that takes the highest existing id into account each time, which also covers a viewbox restored from the URL carrying a higher number than the counter has reached.

The timer. Also right: the comment cited waits that do not exist (100 and 500 ms are the ones in that path; 1200 is nowhere), so it justified a number by inventing a precedent. There is no timer now - resetSelectedViewbox, detectChanges(), then scroll straight after - and loadData's existing callback re-selects the viewbox once its table is registered, which is what actually fills the pane. No ngOnDestroy cleanup needed as a result.

And the lookup is scoped, through a @ViewChild('currentlyOpen') on the Currently open list rather than document.getElementById, which is the other half of the duplicate-id problem you pointed at.

Build clean, karma 896 SUCCESS. The visual check is still outstanding and now has a concrete unblock: the local estate is on the FREE tier (viewbox_limit: 1 in free-tier.config.ts), and tableOnClick returns at that guard before any of this runs. CI's specs install a licence with cy.loginAndUpdateValidKey(true), which is why CI never sees it. I am installing the same kind of key locally so the four states can be photographed rather than argued.

Both, at 0bd553fb. **The id.** You are right that it was not unique, and the consequence was worse than a stray scroll: `resetSelectedViewbox` looks the table up by `viewboxId`, so the older entry's table would have been selected too - the wrong configuration pane, not just the wrong row. Ids now come from a counter that takes the highest existing id into account each time, which also covers a viewbox restored from the URL carrying a higher number than the counter has reached. **The timer.** Also right: the comment cited waits that do not exist (100 and 500 ms are the ones in that path; 1200 is nowhere), so it justified a number by inventing a precedent. There is no timer now - `resetSelectedViewbox`, `detectChanges()`, then scroll straight after - and `loadData`'s existing callback re-selects the viewbox once its table is registered, which is what actually fills the pane. No `ngOnDestroy` cleanup needed as a result. **And the lookup is scoped**, through a `@ViewChild('currentlyOpen')` on the Currently open list rather than `document.getElementById`, which is the other half of the duplicate-id problem you pointed at. Build clean, karma 896 SUCCESS. The visual check is still outstanding and now has a concrete unblock: the local estate is on the FREE tier (`viewbox_limit: 1` in `free-tier.config.ts`), and `tableOnClick` returns at that guard before any of this runs. CI's specs install a licence with `cy.loginAndUpdateValidKey(true)`, which is why CI never sees it. I am installing the same kind of key locally so the four states can be photographed rather than argued.

hermes reviewed 2026-11-08 18:20:26 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

Re-review of #535 at 0bd553fb. Delta from 87b6b84b is one commit, "fix(viewboxes): mint viewbox ids that cannot repeat, and select without a timer". Both findings from that review are closed: ids come from mintViewboxId() (:1407) instead of viewboxes.length + 1, the row is found through @ViewChild('currentlyOpen') (:61, :304) rather than a document-wide id, and the 1200 ms timer is gone. CI at this head: Build-and-ng-test success, Build-and-test-development and lighthouse in progress - code review only. One finding, in the new block itself.

• client/src/app/shared/viewboxes/viewboxes.component.ts:1393 /** - the JSDoc that documents resetSelectedViewbox ("Re setting the viewbox from parameter to selected viewbox ... @param viewbox") now has a second doc block appended directly under it at :1398 for the new nextViewboxId field, so the first block documents a number and resetSelectedViewbox at :1416 is left undocumented. Move the id comment above private nextViewboxId = 1 (:1405) and leave the reset JSDoc immediately above the method.

1 finding above for review.

Re-review of #535 at 0bd553fb. Delta from 87b6b84b is one commit, "fix(viewboxes): mint viewbox ids that cannot repeat, and select without a timer". Both findings from that review are closed: ids come from `mintViewboxId()` (:1407) instead of `viewboxes.length + 1`, the row is found through `@ViewChild('currentlyOpen')` (:61, :304) rather than a document-wide id, and the 1200 ms timer is gone. CI at this head: Build-and-ng-test success, Build-and-test-development and lighthouse in progress - code review only. One finding, in the new block itself. - client/src/app/shared/viewboxes/viewboxes.component.ts:1393 `/**` - the JSDoc that documents `resetSelectedViewbox` ("Re setting the viewbox from parameter to selected viewbox ... @param viewbox") now has a second doc block appended directly under it at :1398 for the new `nextViewboxId` field, so the first block documents a number and `resetSelectedViewbox` at :1416 is left undocumented. Move the id comment above `private nextViewboxId = 1` (:1405) and leave the reset JSDoc immediately above the method. 1 finding above for review.

hermes referenced this pull request 2026-11-08 18:39:51 +00:00

perf(client): register only the Clarity icon shapes this app uses #529

hermes added 1 commit 2026-11-08 18:41:46 +00:00

docs(viewboxes): keep the resetSelectedViewbox doc with its method ... 6404d6bd73

The new field's doc block was inserted between resetSelectedViewbox's own JSDoc
and the method, so the reset doc described an id counter and the method lost its
doc entirely. The counter's block now sits with the counter.

hermes reviewed 2026-11-08 19:04:12 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

Re-review of #535 at 6404d6bd. Delta since 0bd553fb (review 940) is one commit, 6404d6bd "docs(viewboxes): keep the resetSelectedViewbox doc with its method": the JSDoc for resetSelectedViewbox now sits immediately above the method at :1411-1415 and mintViewboxId keeps the id comment, so the finding at 940 is closed. CI at this head: Build-and-ng-test success, lighthouse success, Build-and-test-development still running - code review only. No new findings.

No blocking issues.

Re-review of #535 at 6404d6bd. Delta since 0bd553fb (review 940) is one commit, 6404d6bd "docs(viewboxes): keep the resetSelectedViewbox doc with its method": the JSDoc for resetSelectedViewbox now sits immediately above the method at :1411-1415 and mintViewboxId keeps the id comment, so the finding at 940 is closed. CI at this head: Build-and-ng-test success, lighthouse success, Build-and-test-development still running - code review only. No new findings. No blocking issues.

allan merged commit a1fc80cdd1 into release/7.23 2026-11-08 19:09:02 +00:00

allan deleted branch fix/viewbox-focus-on-add 2026-11-08 19:09:02 +00:00

hermes referenced this pull request 2026-11-08 23:16:36 +00:00

feat(release): 7.23 - licence state as a banner, an 849 kB initial load, numbered logs #481

Sign in to join this conversation.

Reviewers

No Reviewers

hermes

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

allan (Allan) dcbot DylanLeong henrik hermes (hermes) mihajlo sabir sead trevor Yury

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dc/dc#535