fix(viewboxes): mint viewbox ids that cannot repeat, and select without a timer
Build / Build-and-ng-test (pull_request) Successful in 3m11s
Build / lighthouse (pull_request) Successful in 6m43s
Build / Build-and-test-development (pull_request) Successful in 21m52s

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.
This commit is contained in:
dc committed 2026-11-08 18:11:17 +00:00
1 parent 87b6b84b5f
commit 0bd553fb00
2 files changed
+34 -10

No files matched your search

@@ -42,7 +42,7 @@
}
</div>
<div class="currently-open">
<div class="currently-open" #currentlyOpen>
<div class="d-flex clr-justify-content-between">
<p class="mt-0">Currently open</p>
@@ -16,6 +16,7 @@ import {
OnInit,
Output,
QueryList,
ViewChild,
ViewChildren,
ViewEncapsulation
} from '@angular/core'
@@ -56,6 +57,9 @@ import { Viewbox } from './models/viewbox.model'
standalone: false
})
export class ViewboxesComponent implements OnInit, AfterViewInit, OnDestroy {
/** The Currently open list, so the new row is found among this component's own rows. */
@ViewChild('currentlyOpen') currentlyOpen?: ElementRef<HTMLElement>
@ViewChildren('resizeBox') resizeBoxQuery!: QueryList<ElementRef> //make query list, handle multiple
@ViewChildren('dragHandleCorner')
dragHandleCornerQuery!: QueryList<ElementRef>
@@ -266,7 +270,7 @@ export class ViewboxesComponent implements OnInit, AfterViewInit, OnDestroy {
library: data.library.LIBRARYNAME,
loadingData: true,
filter_pk: '0',
id: this.viewboxes.length + 1,
id: this.mintViewboxId(),
x: window.innerWidth - this.defaultConfig.width,
y: 150
}
@@ -289,15 +293,17 @@ export class ViewboxesComponent implements OnInit, AfterViewInit, OnDestroy {
on the right hand side - so select it there and bring it into view. Without
this the user is left looking at the tree they just used, and the tree row
stays under the pointer even though it is no longer the thing to act on.
The delay is the same one the table build uses before the viewbox table is
registered, so the configuration pane is populated when it opens.
No timer: detectChanges is what puts the row in the view, so selecting and
scrolling straight after it is the earliest point that works, and it does
not guess how long the table takes. loadData's own callback re-selects the
viewbox once its table is registered, which is what fills the pane.
*/
setTimeout(() => {
this.resetSelectedViewbox(viewbox)
this.cdf.detectChanges()
const row = document.getElementById('open-viewbox_' + viewbox.id)
if (row) row.scrollIntoView({ block: 'nearest' })
}, 1200)
this.resetSelectedViewbox(viewbox)
this.cdf.detectChanges()
this.currentlyOpen?.nativeElement
.querySelector('#open-viewbox_' + viewbox.id)
?.scrollIntoView({ block: 'nearest' })
}
/**
@@ -1389,6 +1395,24 @@ export class ViewboxesComponent implements OnInit, AfterViewInit, OnDestroy {
* And sets the correct data for that viewbox
* @param viewbox
*/
/**
* Ids are used as DOM anchors and as the key that ties a viewbox to its table
* entry, so they must never repeat. `viewboxes.length + 1` did: remove the
* first of two and the next addition was 2 again, which left two rows with
* the same id - getElementById answers the first in document order, and
* resetSelectedViewbox found the older entry's table.
*/
private nextViewboxId = 1
private mintViewboxId(): number {
const highest = this.viewboxes.reduce(
(max, viewbox) => Math.max(max, viewbox.id || 0),
0
)
this.nextViewboxId = Math.max(this.nextViewboxId, highest + 1)
return this.nextViewboxId++
}
resetSelectedViewbox(viewbox: Viewbox) {
this.selectedViewbox = viewbox
this.selectedViewboxTable = this.viewboxTables.find(