Fix Details links opened in a new tab showing the candidates page - #559
Conversation
The Details button and the View Spectrogram Details menu item pointed at a modal hash fragment, so opening them in a new tab loaded the candidates page instead of the detection. Point their href at the existing detection details route, keep the in-page modal on normal click via an explicit data-target and preventDefault, and let middle-click, ctrl+click, and open-in-new-tab follow the real URL. Fixes orcasound#555
There was a problem hiding this comment.
Pull request overview
This PR fixes the “Details” links in the AIForOrcas moderator UI so that modifier-based navigation (open in new tab/window) goes to the proper detection details route, while a plain click continues to open the in-page spectrogram modal/player.
Changes:
- Updates “Details” / “View Spectrogram Details” anchors to use the existing detection details route (
LinkUrl) instead of only a modal hash fragment. - Adds a modifier-aware JavaScript
OpenSpectrogramModalhandler to preserve modal behavior on plain click while allowing ctrl/cmd/shift navigation. - Renders annotation regions JSON into a
data-regionsattribute (via a newRegionsJsonproperty) and enables cache-busting forai-for-orcas.jsviaasp-append-version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/wwwroot/js/ai-for-orcas.js | Adds OpenSpectrogramModal to decide between modal-open vs. normal navigation based on modifier keys. |
| ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/Pages/_Host.cshtml | Enables versioned script URL for ai-for-orcas.js to reduce stale caching. |
| ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/Components/DetectionComponent.razor.cs | Extracts annotation-region JSON generation into RegionsJson for client-side initialization. |
| ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/Components/DetectionComponent.razor | Switches anchors to LinkUrl and wires up the new JS handler + data-regions payload. |
Comments suppressed due to low confidence (1)
ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/Components/DetectionComponent.razor:184
- Same concern here: embedding
@Detection.AudioUridirectly into an inline JS string inonclickis fragile and can become an injection vector. Pass it through adata-*attribute and read it fromthis.datasetinstead.
Submit
</button>
</Authorized>
</AuthorizeView>
I just tested this locally using Chrome on Windows. Plain click works normally. Ctrl+click opens in new tab, but details page does NOT render. |
jumperck
added
the
moderator portal
|
I posted a summary of the underlying behavior and the naming history on #555. I'm confirming with Isabelle what she expects to see in the new tab and holding further changes here until then. This PR stays scoped to fixing the broken anchor. If the annotated spectrogram view turns out to be what's wanted, I'll handle that as a follow-up. |
Serialize spectrogram regions with System.Text.Json so the output stays valid JSON regardless of server culture, pass detection values to the modal handler via data attributes instead of interpolated inline script, drop the misleading detectionId parameter name, and make the ai-for-orcas.js path app-rooted so asp-append-version resolves reliably.
… player The in-page modal already draws the shaded regions where the detector found a call, but the card player used on the candidates list and the standalone details page did not. Moderators rely on those regions to judge a detection, so pass the same regions data to the card player and enable the regions plugin there.
The 2021-03-15 release notes state regions on the spectrogram modal should not be resizable, but only drag was ever disabled; the wavesurfer regions plugin defaults to resizable, so every region rendered with active resize handles. Set resize to false alongside drag so the shaded detector regions are strictly read-only in both the modal and card players.
Detector regions only appeared after pressing play, because wavesurfer needs the audio loaded to know the clip duration. Moderators judging from the static spectrogram had no cue where the detector heard a call. Draw lightweight overlay strips on the card image as soon as it renders, using an audio element with preload=metadata to fetch just the clip duration. The strips reuse the wavesurfer-region styling for a consistent look and are removed when the player starts, which then draws its own regions.
The preview strips hardcoded a grey tint while the wavesurfer regions plugin defaults to a white tint, so the shading changed once playback started. Declare the color on each region in RegionsJson; the plugin already honors the per-region color option and the preview now reads the same value, keeping both renderings identical.
|
Following up on the scope here. After confirming with Isabelle what moderators actually need from the Details view, I extended the branch beyond the original broken-anchor fix:
All four Copilot review comments have been addressed as well; I replied inline to each with the specifics. Isabelle also raised two further points. She confirmed the underlying pain behind opening Details in a new tab is losing her place in the candidate queue when navigating back, which is already tracked in #554; I plan to pick that up next. She also suggested a zoom improvement for the spectrogram. That one needs some design work, so I've opened #563 to collect the requirements and track it; I'm leaving both out of this PR. Happy to split any of the region commits out if you'd prefer this PR stay strictly on the anchor fix. |
PreviewCardRegions is invoked from OnAfterRenderAsync, which can fire before ai-for-orcas.js finishes loading when it sits at the bottom of the page (cold cache makes the window wider). The resulting JSException kills the circuit with Could not find 'PreviewCardRegions'. Loading the script ahead of blazor.server.js guarantees the functions exist before the circuit renders; the file only declares functions, so early loading has no side effects.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/wwwroot/js/ai-for-orcas.js:395
OpenSpectrogramModalonly treats Ctrl/Cmd/Shift as “modified” clicks. In browsers where a middle-click triggers theclickhandler, this will incorrectly open the modal and returnfalse, preventing the expected “open in new tab” behavior.
function OpenSpectrogramModal(event, anchor) {
// Ctrl/Cmd/Shift clicks open the details page in a new tab or window instead.
if (event.ctrlKey || event.metaKey || event.shiftKey) {
return true;
}
ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/Pages/_Host.cshtml:48
ai-for-orcas.jsnow loads beforeblazor.server.js, but its interop functions depend on jQuery/Bootstrap/WaveSurfer which are currently loaded after Blazor. That can cause early JS interop (e.g., first render) or fast user clicks to throwReferenceError/TypeErrorbecause$/WaveSurfer/.modal()are not defined yet, potentially breaking the Blazor Server circuit.
<!-- Loaded before blazor.server.js so interop functions exist before the circuit's first render -->
<script src="~/js/ai-for-orcas.js" asp-append-version="true"></script>
<script src="_framework/blazor.server.js"></script>
ModeratorFrontEnd/AIForOrcas/AIForOrcas.Client.Web/wwwroot/js/ai-for-orcas.js:268
PreviewCardRegionsis invoked on every Blazor render. If multiple renders happen before the firstloadedmetadatafires, this function can start multiple parallel audio metadata requests for the same card because the "preview exists" guard isn’t set until after metadata is loaded and the preview is drawn.
var image = document.getElementById('spectrogram-card-' + cardId);
var waveform = document.getElementById('waveform-card-' + cardId);
if (image === null || waveform === null || document.getElementById('regions-preview-card-' + cardId) !== null) {
return;
dthaler
left a comment
dthaler
left a comment
There was a problem hiding this comment.
Still not sure whether the open-in-new-tab behavior is what moderators want or not, but I am approving and merging since the behavior is better than before now that the lines appear on the candidates page.
Problem
Fixes #555
The Details button (and the View Spectrogram Details menu item) is an anchor whose href is only the modal hash fragment (
#spectrogram-panel-modal-<id>). A normal click opens the modal, but "open link in new tab", middle-click, and ctrl+click follow the href and just reload the candidates page.Solution
/detections/detection/{id}, via the existingLinkUrlproperty)OpenSpectrogramModalJavaScript handler keeps the in-page behavior: plain click opens the modal with the player; ctrl/cmd/shift+click, middle-click, and "open in new tab" go to the details pagedata-regionsattribute (logic extracted into aRegionsJsonproperty) so the handler can initialize the player without a server round tripai-for-orcas.jsis now referenced withasp-append-version="true", so browsers pick up script changes instead of serving a stale cached copyAdditions since the PR was opened
After talking to Isabelle (moderator) about how she reviews detections, the scope grew to cover the visible gap between the modal and the other spectrogram views:
resize: falserestores the behavior described in the 2021 release notes (the plugin default had been re-exposing resize handles)data-*attributes, parameter rename, app-rooted script path)ai-for-orcas.jsnow loads beforeblazor.server.js: the region preview is invoked fromOnAfterRenderAsync, which could fire before the script finished loading and killed the circuit with "Could not find 'PreviewCardRegions'"The remaining moderator asks are tracked separately: #554 (queue position lost on back navigation) and #563 (spectrogram zoom).
Verification
Manually verified on a local run of AIForOrcas.Client.Web:
No test project exists for AIForOrcas.Client.Web, so no automated tests are included.