Improve job list view, unify ID rendering #9068
Conversation
📝 WalkthroughWalkthroughRefactors job data flow: introduces a compact job shape (JobCompactInfo), extracts result-link logic into JobResultLinks, renames job commandArgs→args, changes timestamps to Instant, exposes findAllCompact for list retrieval, updates controller/service serialization, and migrates frontend types and usages from APIJobType to APIJobCommand. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fm3
changed the title
|
|
||
| useEffect(() => { | ||
| fetchData(); | ||
| initialize(); |
There was a problem hiding this comment.
Extracted this so I could make it async (and actually await the fetchData). React wouldn’t let me do that inline in useEffect.
fm3
changed the title
|
|
||
| def exportFileName: Option[String] = argAsStringOpt("export_file_name") | ||
|
|
||
| def datasetName: Option[String] = argAsStringOpt("dataset_name") |
There was a problem hiding this comment.
these lines were moved to a trait, see below
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
frontend/javascripts/admin/voxelytics/workflow_list_view.tsx (1)
191-197: Table empty state now reflects loading vs. “no workflows yet”; consider optional error handlingUsing
locale.emptyTextwithisLoading ? <Spin size="large" /> : "There are no voxelytics workflow reports yet."cleanly solves the “loading shows as no data” problem whenrenderRunsis empty and the initial fetch is in progress, aligning with the issue’s objective.One optional improvement: on fetch errors (where
Toast.erroris shown andworkflowsremains empty), this will still render the “no workflow reports yet” message. If distinguishing “error” from “truly empty” is important, you could add anerrorstate flag and branchemptyTextaccordingly.frontend/javascripts/admin/job/job_hooks.ts (1)
34-52: Confirmcreatedis numeric and consider using state updater for the latest-job logicThe switch from
createdAttocreatedin both the sort and the “most recent successful job” check looks correct, assumingAPIJob["created"]is still a numeric timestamp.To avoid any future drift in the type (e.g., moving to ISO strings or
Dateobjects) and to make the async updater more robust, you could:
- Ensure
createdis typed asnumberinAPIJob.- Update the state write to rely on the previous value instead of the captured
mostRecentSuccessfulJob, e.g.:setMostRecentSuccessfulJob((prev) => prev == null || job.created > prev.created ? job : prev, );This keeps the “latest job” invariant correct even if multiple updates race.
frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (1)
196-198: Make access totrainingAnnotationsresilient to missingargsThe move to
model.trainingJob.args.trainingAnnotationsmatches the new command/args model, but it will throw iftrainingJob.argsis everundefined(e.g., for older or partially‑migrated jobs).You can keep behavior identical while hardening against that case:
const { voxelyticsWorkflowHash, state: trainingJobState, args } = model.trainingJob; const trainingAnnotations = args?.trainingAnnotations;The existing
trainingAnnotations != nullcheck then safely handles “no annotations” without risking a runtime error.frontend/javascripts/components/formatted_id.tsx (1)
1-27: Harden clipboard handling and improve click semanticsThe component does what we want functionally, but two small tweaks would make it more robust:
- Guard clipboard access / handle errors
On some environments
navigator.clipboardmay be unavailable orwriteTextmay reject. Wrapping the call avoids hard crashes and lets you surface a better message if copy fails:onClick={async () => { try { if (!navigator.clipboard?.writeText) { throw new Error("Clipboard API not available"); } await navigator.clipboard.writeText(id); Toast.success("Copied ID to clipboard."); } catch (e) { Toast.error("Could not copy ID to clipboard."); // optionally: console.error(e); } }}
- Use button semantics for accessibility
A clickable
<div>is not focusable or keyboard‑accessible. Consider rendering a<button type="button">(or addingrole="button"andtabIndex={0}plus anonKeyDownhandler for Enter/Space) so keyboard users can trigger the copy interaction as well.These keep the UX but make the component safer and more accessible across the app.
frontend/javascripts/types/api_types.ts (1)
808-820: Consider makingtrainingAnnotationsnullable for consistency.All other fields in
ApiJobArgsare marked as| null | undefined, buttrainingAnnotationsis typed as a non-nullable array. If jobs without training annotations don't include this field in the response, accessing it could cause runtime issues.export type ApiJobArgs = { readonly datasetId: string | null | undefined; readonly datasetName: string | null | undefined; readonly mergeSegments: boolean | null | undefined; - readonly trainingAnnotations: Array<{ annotationId: string }>; + readonly trainingAnnotations: Array<{ annotationId: string }> | null | undefined; readonly datasetDirectoryName: string | null | undefined; readonly annotationId: string | null | undefined; readonly annotationLayerName: string | null | undefined; readonly layerName: string | null | undefined; readonly modelId: string | null | undefined; readonly boundingBox: string | null | undefined; readonly ndBoundingBox: WkLibsNdBoundingBox | null | undefined; };frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (1)
681-684: Use the enum constant instead of a string literal for consistency.Line 683 uses the string literal
"compute_mesh_file"while lines 437 and 762 useAPIJobCommand.COMPUTE_MESH_FILE. Use the enum constant for consistency and type safety.const meshJobsForDataset = jobs.filter( - (job) => - job.command === "compute_mesh_file" && job.args.datasetName === this.props.datasetName, + (job) => + job.command === APIJobCommand.COMPUTE_MESH_FILE && job.args.datasetName === this.props.datasetName, );frontend/javascripts/admin/job/job_list_view.tsx (1)
415-416: Use the enum constant instead of a string literal for consistency.This line uses the string literal
"find_largest_segment_id"while other comparisons in this file useAPIJobCommand.*constants (e.g., lines 378-381, 393, 404, 418-424, 437-438).- } else if (job.command === "find_largest_segment_id") { + } else if (job.command === APIJobCommand.FIND_LARGEST_SEGMENT_ID) { return <span>{job.returnValue}</span>;frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (1)
276-279: Use the enum constant instead of a string literal for consistency.Line 277 uses the string literal
"find_largest_segment_id"while line 518 usesAPIJobCommand.FIND_LARGEST_SEGMENT_ID. Use the enum constant for consistency.initialJobKeyExtractor: (job) => - job.command === "find_largest_segment_id" && job.args.datasetName === dataset?.name + job.command === APIJobCommand.FIND_LARGEST_SEGMENT_ID && job.args.datasetName === dataset?.name ? (job.args.datasetName ?? "largest_segment_id") : null,frontend/javascripts/dashboard/dataset_view.tsx (1)
428-472: NewJobsAlert: command/created migration looks correct; consider avoiding string literalThe migration to
job.command,job.created, andjob.args.datasetNameis consistent with the new APIJob shape, andFormattedDatewill handlecreatedfine as long as it’s a timestamp or ISO string. To keep things type‑safe and aligned with the rest of the PR, consider replacing the hard‑coded"convert_to_wkw"with the correspondingAPIJobCommandconstant so refactors of command names stay centralized.-import type { APIDatasetCompact, APIJob, APIUser, FolderItem } from "types/api_types"; +import type { APIDatasetCompact, APIJob, APIUser, FolderItem } from "types/api_types"; +import { APIJobCommand } from "types/api_types"; @@ - .filter( - (job) => - job.command === "convert_to_wkw" && + .filter( + (job) => + job.command === APIJobCommand.CONVERT_TO_WKW && dayjs.duration(now.diff(job.created)).asDays() <= RECENT_DATASET_DAY_THRESHOLD, )app/models/job/JobResultLinks.scala (1)
35-38: Simplify redundant map operation.The
.map { resultAnnotationLink => resultAnnotationLink }is a no-op identity transformation.case JobCommand.infer_neurons if this.argAsBooleanOpt("do_evaluation").getOrElse(false) => - returnValue.map { resultAnnotationLink => - resultAnnotationLink - } + returnValue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
app/controllers/JobController.scala(1 hunks)app/models/job/Job.scala(8 hunks)app/models/job/JobResultLinks.scala(1 hunks)app/models/job/JobService.scala(5 hunks)frontend/javascripts/admin/api/jobs.ts(2 hunks)frontend/javascripts/admin/dataset/dataset_upload_view.tsx(2 hunks)frontend/javascripts/admin/job/job_hooks.ts(2 hunks)frontend/javascripts/admin/job/job_list_view.tsx(12 hunks)frontend/javascripts/admin/scripts/script_list_view.tsx(2 hunks)frontend/javascripts/admin/task/task_list_view.tsx(2 hunks)frontend/javascripts/admin/tasktype/task_type_list_view.tsx(2 hunks)frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(1 hunks)frontend/javascripts/admin/voxelytics/workflow_list_view.tsx(2 hunks)frontend/javascripts/components/formatted_id.tsx(1 hunks)frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx(5 hunks)frontend/javascripts/dashboard/dataset_view.tsx(2 hunks)frontend/javascripts/dashboard/explorative_annotations_view.tsx(2 hunks)frontend/javascripts/dashboard/folders/details_sidebar.tsx(2 hunks)frontend/javascripts/libs/format_utils.ts(0 hunks)frontend/javascripts/types/api_types.ts(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/constants.ts(2 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/align_sections_form.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/custom_ai_model_inference_form.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/mitochondria_segmentation_form.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/start_job_form.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/materialize_volume_annotation_modal.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/tabs/run_ai_model_tab.tsx(5 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/utils.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/create_animation_modal.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/download_modal_view.tsx(2 hunks)frontend/javascripts/viewer/view/action-bar/tools/skeleton_specific_ui.tsx(2 hunks)frontend/javascripts/viewer/view/action_bar_view.tsx(3 hunks)frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/bounding_box_tab.tsx(2 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx(4 hunks)frontend/stylesheets/main.less(1 hunks)unreleased_changes/9068.md(1 hunks)util/src/main/scala/com/scalableminds/util/requestlogging/RequestLogging.scala(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/libs/format_utils.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-27T11:32:54.033Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:32:54.033Z
Learning: In the webknossos frontend AI job forms, `datasetConfiguration.layers[colorLayer.name]` is assumed to always be non-null when accessing layer properties like `isInverted`. The selected color layer is guaranteed to exist in the dataset configuration.
Applied to files:
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsxfrontend/javascripts/viewer/view/action_bar_view.tsxfrontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsxfrontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/dashboard/explorative_annotations_view.tsxfrontend/javascripts/admin/voxelytics/ai_model_list_view.tsxfrontend/javascripts/viewer/view/action-bar/create_animation_modal.tsxfrontend/javascripts/viewer/view/action-bar/ai_job_modals/materialize_volume_annotation_modal.tsx
📚 Learning: 2025-01-13T09:06:15.202Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Applied to files:
app/models/job/JobService.scalaapp/models/job/JobResultLinks.scalaapp/controllers/JobController.scalaapp/models/job/Job.scala
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/admin/job/job_list_view.tsx
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/view/action-bar/ai_job_modals/materialize_volume_annotation_modal.tsx
📚 Learning: 2025-08-27T11:34:16.411Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8796
File: frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx:68-77
Timestamp: 2025-08-27T11:34:16.411Z
Learning: The webknossos codebase uses `getDefaultLayerViewConfiguration` to ensure that layer configurations always exist in `datasetConfiguration.layers`, making direct access like `datasetConfiguration.layers[colorLayer.name]` safe across multiple files including api_latest.ts and load_histogram_data_saga.ts.
Applied to files:
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx
📚 Learning: 2025-08-04T11:49:30.012Z
Learnt from: frcroth
Repo: scalableminds/webknossos PR: 8821
File: app/models/dataset/Dataset.scala:864-866
Timestamp: 2025-08-04T11:49:30.012Z
Learning: In WebKnossos Scala codebase, when querying database tables with Slick, explicit column listing in SELECT statements is preferred over SELECT * to ensure columns are returned in the exact order expected by case class mappings. This prevents parsing failures when the physical column order in the production database doesn't match the schema definition order.
Applied to files:
app/models/job/Job.scala
🧬 Code graph analysis (16)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/constants.ts (1)
frontend/javascripts/viewer/constants.ts (1)
Vector3(14-14)
frontend/javascripts/admin/scripts/script_list_view.tsx (1)
frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/mitochondria_segmentation_form.tsx (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/utils.tsx (1)
isDatasetOrBoundingBoxTooSmall(105-124)
frontend/javascripts/admin/api/jobs.ts (2)
frontend/javascripts/types/api_types.ts (1)
APIJob(822-836)frontend/javascripts/admin/api/api_utils.ts (1)
assertResponseLimit(6-12)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/utils.tsx (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/constants.ts (1)
MIN_BBOX_EXTENT(25-29)
frontend/javascripts/dashboard/explorative_annotations_view.tsx (2)
frontend/javascripts/types/api_types.ts (1)
APIAnnotationInfo(510-531)frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
frontend/javascripts/admin/task/task_list_view.tsx (1)
frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
frontend/javascripts/viewer/view/action_bar_view.tsx (1)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
setAIJobModalStateAction(144-148)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/tabs/run_ai_model_tab.tsx (5)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
setAIJobModalStateAction(144-148)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (1)
NeuronSegmentationForm(25-155)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx (1)
NucleiDetectionForm(14-79)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/mitochondria_segmentation_form.tsx (1)
MitochondriaSegmentationForm(14-73)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/align_sections_form.tsx (1)
AlignSectionsForm(11-54)
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
frontend/javascripts/admin/job/job_list_view.tsx (5)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getReadableURLPart(747-751)frontend/javascripts/types/api_types.ts (1)
APIJob(822-836)frontend/javascripts/libs/format_utils.ts (2)
formatWkLibsNdBBox(540-553)formatCreditsString(555-559)frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)frontend/javascripts/components/formatted_date.tsx (1)
FormattedDate(19-42)
frontend/javascripts/components/formatted_id.tsx (1)
frontend/javascripts/components/fast_tooltip.tsx (1)
FastTooltip(54-123)
frontend/javascripts/dashboard/dataset_view.tsx (3)
util/src/main/scala/com/scalableminds/util/time/Instant.scala (1)
now(48-48)frontend/javascripts/viewer/constants.ts (1)
Unicode(279-283)frontend/javascripts/components/formatted_date.tsx (1)
FormattedDate(19-42)
app/models/job/JobResultLinks.scala (3)
app/models/job/JobCommand.scala (1)
JobCommand(5-22)app/models/job/Job.scala (3)
id(38-38)effectiveState(51-51)effectiveState(85-85)util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala (1)
fromStringSync(22-26)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/utils.tsx (1)
isDatasetOrBoundingBoxTooSmall(105-124)
app/controllers/JobController.scala (1)
app/models/job/Job.scala (2)
findAllCompact(164-217)enrich(87-90)
🔇 Additional comments (52)
util/src/main/scala/com/scalableminds/util/requestlogging/RequestLogging.scala (1)
18-23: Log formatting change looks good; just confirm no rigid log parsing downstreamWrapping the result body in backticks improves readability and makes it clearer where the payload starts/ends. Since this slightly changes the log string format, double‑check that no log parsers/alerts rely on the previous
: <body>shape without backticks.frontend/javascripts/admin/voxelytics/workflow_list_view.tsx (1)
4-4: Spin import is correctly wired to the new loading state UIImporting
Spinalongside the existing antd components is consistent with its usage in the table’semptyTextbelow; no issues here.unreleased_changes/9068.md (1)
1-7: Changelog accurately reflects the functional changesThe entries clearly summarize the unified ID display, workflow report link, performance work, and loading-indicator fixes; nothing to adjust from a technical perspective.
frontend/javascripts/admin/tasktype/task_type_list_view.tsx (1)
17-18: Nice reuse ofFormattedIdfor task type IDsSwitching the ID column to
<FormattedId id={id} />cleanly aligns this view with the unified ID UX while preserving sorting/filtering behavior on the full ID value. No further changes needed here.Also applies to: 151-158
frontend/javascripts/dashboard/folders/details_sidebar.tsx (1)
11-11: Consistent ID UX in dataset detailsUsing
<FormattedId id={fullDataset.id} />inside the Tag brings the dataset details sidebar in line with the new unified ID behavior, including copy‑to‑clipboard, without affecting surrounding layout or logic.Also applies to: 174-179
frontend/javascripts/dashboard/explorative_annotations_view.tsx (1)
24-25: ID column refactor toFormattedIdlooks goodThe ID column now:
- Uses
<FormattedId id={annotation.id} />for a consistent short-ID + copy UX.- Keeps sorting based on the full ID (
sorter: Utils.localeCompareBy((annotation) => annotation.id)).- Still renders the “read-only” and “locked” status lines under the ID.
Imports (
FormattedId, updated AntD components,stringToColor) line up with the usage. No issues spotted.Also applies to: 29-34, 625-639
frontend/stylesheets/main.less (1)
712-716: Cleanup verified as safe—no remaining.monospace-idusages foundThe verification confirms that
.monospace-idhas been completely removed with no lingering references in the frontend code. The style cleanup is safe and complete.frontend/javascripts/admin/api/jobs.ts (2)
15-20: LGTM on the simplified transformation.The spread pattern with
_.mapKeysfor camelCasing args is clean and idiomatic. The approach correctly delegates structure to the backend while normalizing argument keys for frontend consumption.
22-26: Verify: Sorting removal is intentional.The previous implementation sorted jobs by
createdAtbefore returning. This change removes that sort, so the order now depends on the backend. Confirm this is intentional and that the backend provides jobs in a consistent/expected order (e.g., newest first).frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
67-67: LGTM on APIJobType → APIJobCommand migration.The import and usage changes are consistent with the project-wide migration to command-based job modeling. The enum value name remains unchanged, so behavior is preserved.
Also applies to: 688-688
frontend/javascripts/types/api_types.ts (3)
780-798: LGTM onAPIJobCommandenum.The enum provides clear identifiers for all job commands with appropriate deprecation markers for backwards compatibility. The snake_case values align with backend conventions.
822-836: LGTM on flattenedAPIJobtype.The new structure with explicit owner fields (
ownerFirstName,ownerLastName,ownerEmail) and centralizedargspayload improves clarity. Thecreatedtimestamp andreturnValuenaming align with backend conventions.
778-778: No breaking changes detected. TheAPIJobStatetype is already non-nullable, and frontend code consistently treatsjob.stateas a required field.All usages throughout the codebase—direct string comparisons in segments_view.tsx, array lookups in dataset_view.tsx and job_list_view.tsx, and state checks in job_hooks.ts—assume
stateis always defined. No defensive null checks or type assertions exist, indicating the code was never designed to handle nullable state.frontend/javascripts/viewer/view/right-border-tabs/bounding_box_tab.tsx (1)
10-10: LGTM on APIJobType → APIJobCommand migration.The import and usage changes correctly update to the new command-based enum while preserving the export TIFF capability check logic.
Also applies to: 121-123
frontend/javascripts/viewer/view/left-border-tabs/layer_settings_tab.tsx (1)
51-51: LGTM on APIJobType → APIJobCommand migration.The import and usage changes correctly update to the new command-based enum for the segment index file computation capability check.
Also applies to: 710-713
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx (3)
52-52: LGTM!Import correctly updated to use the new
APIJobCommandenum.
436-441: LGTM!Correctly uses the
APIJobCommand.COMPUTE_MESH_FILEenum constant for the supported jobs check.
760-763: LGTM!Correctly uses the
APIJobCommand.COMPUTE_MESH_FILEenum constant for the tooltip condition.frontend/javascripts/admin/job/job_list_view.tsx (5)
142-151: LGTM!Good refactoring to extract the async initialization logic. This correctly ensures
fetchDatacompletes before settingisLoadingto false, which addresses the loading indicator issue mentioned in the PR objectives.
169-179: LGTM!The fallback logic for generating dataset links correctly handles both the new
datasetId-based approach and the legacyorganizationId/datasetNameapproach, ensuring backward compatibility.
330-334: LGTM!The workflow link rendering is clean and correctly returns
nullwhen no workflow hash is available.
499-548: LGTM!The table changes correctly implement:
- Loading spinner wrapper
- Search filtering using
job.args.datasetName- FormattedId for copyable IDs
- Flattened owner fields
- New Cost in Credits and Date columns
- Conditional Voxelytics column for superusers
181-327: LGTM!The
renderDescriptionfunction correctly migrates to the command-based approach usingjob.commandandjob.args.*fields. The conditional rendering logic properly handles all job types.frontend/javascripts/dashboard/dataset/dataset_settings_data_tab.tsx (3)
283-291: LGTM!The job start callback correctly uses
job.args.datasetNameto extract the dataset name from the new API structure.
517-518: LGTM!Correctly uses
APIJobCommand.FIND_LARGEST_SEGMENT_IDenum constant.
551-554: LGTM!Correctly updated to use
mostRecentSuccessfulJob.returnValuematching the new API field name.frontend/javascripts/admin/scripts/script_list_view.tsx (2)
5-5: LGTM!Import added for the new
FormattedIdcomponent.
140-147: LGTM!The ID column now uses
FormattedIdfor consistent ID rendering with single-click copy functionality, aligning with the PR objective of unified ID display across views.frontend/javascripts/admin/task/task_list_view.tsx (2)
30-30: LGTM!Import added for the new
FormattedIdcomponent.
251-259: LGTM!The ID column now uses
FormattedIdfor consistent ID rendering with single-click copy functionality. The width increase to 120 appropriately accommodates the new format (copy icon + shortened ID).frontend/javascripts/viewer/view/action-bar/create_animation_modal.tsx (1)
23-23: LGTM! Enum migration is consistent.The migration from
APIJobTypetoAPIJobCommandis correctly applied in both the import and feature flag check.Also applies to: 333-333
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/mitochondria_segmentation_form.tsx (1)
8-8: LGTM! Consistent enum usage throughout.All references to the mitochondria inference job type have been correctly updated to use
APIJobCommand.INFER_MITOCHONDRIA.Also applies to: 36-49
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/custom_ai_model_inference_form.tsx (1)
14-14: LGTM! Enum migration applied correctly.The import and
StartJobFormprop have been consistently updated to useAPIJobCommand.INFER_NEURONS.Also applies to: 100-100
frontend/javascripts/viewer/view/action-bar/ai_job_modals/constants.ts (1)
1-7: LGTM! Type definitions correctly updated.Both the
ModalJobTypesunion andMEAN_VX_SIZErecord type have been properly migrated to useAPIJobCommandinstead ofAPIJobType.Also applies to: 31-34
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/start_job_form.tsx (1)
11-11: LGTM! Public API signature updated correctly.The
StartJobFormProps.jobNametype has been properly updated to useAPIJobCommand, maintaining consistency with the broader migration.Also applies to: 42-42
frontend/javascripts/viewer/view/action-bar/download_modal_view.tsx (1)
41-41: LGTM! Export feature check updated correctly.The TIFF export availability check now uses
APIJobCommand.EXPORT_TIFFconsistently with the migration pattern.Also applies to: 594-594
frontend/javascripts/viewer/view/action-bar/ai_job_modals/materialize_volume_annotation_modal.tsx (1)
7-7: LGTM! Materialization job correctly updated.The import and
StartJobFormusage have been consistently updated to useAPIJobCommand.MATERIALIZE_VOLUME_ANNOTATION.Also applies to: 131-131
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (1)
10-10: LGTM! All neuron inference references updated.The migration is complete across all three usage sites:
getBestFittingMagComparedToTrainingDS,isDatasetOrBoundingBoxTooSmall, and theStartJobFormprop.Also applies to: 63-65, 123-123
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/align_sections_form.tsx (1)
7-30: AlignSectionsForm: command enum migration looks consistentUsing
APIJobCommand.ALIGN_SECTIONSforjobNameand importingAPIJobCommandkeeps this form aligned with the new command-based job model; no functional regressions visible here.frontend/javascripts/viewer/view/action-bar/tools/skeleton_specific_ui.tsx (1)
19-64: Materialize-volume capability check correctly switched to commandsSwitching the capability check to
APIJobCommand.MATERIALIZE_VOLUME_ANNOTATIONmatches the new command-based backend while preserving the previous boolean logic; no issues spotted.app/controllers/JobController.scala (1)
87-92: Job list now uses compact projection; verify ordering and field paritySwitching
listtojobDAO.findAllCompactand returningjobsCompact.map(_.enrich)is a good fit for the new compact job representation and should help with performance. One thing to double‑check: thatfindAllCompact(plusenrich) preserves the expected default ordering and field set compared to the previouslistimplementation, especially if any UI relied on “newest first” or additional fields frompublicWrites.frontend/javascripts/viewer/view/action-bar/ai_job_modals/utils.tsx (1)
5-21: AI job utils: command-based typing and branching remain consistentUpdating
getMinimumDSSizeandgetBestFittingMagComparedToTrainingDSto useAPIJobCommand(including the INFER_MITOCHONDRIA special case) keeps the behavior identical while aligning with the new enum. The interactions withMIN_BBOX_EXTENTandModalJobTypesstill look coherent; no adjustments needed here.Also applies to: 42-47
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx (1)
6-39: NucleiDetectionForm: migration to APIJobCommand is straightforwardPassing
APIJobCommand.INFER_NUCLEIthrough to mag selection, bounding‑box validation, andjobNamekeeps this form consistent with the other AI job forms and the new command‑centric model; no issues found.Also applies to: 52-57
frontend/javascripts/viewer/view/action-bar/ai_job_modals/tabs/run_ai_model_tab.tsx (1)
4-5: RunAiModelTab: command-based selection wiring looks solidThe radio button checks, dispatch payloads, and conditional rendering have all been updated to
APIJobCommand.*in a consistent way, so the tab continues to map each selection to the correct form (neurons, mitochondria, nuclei, align sections). AssumingStartAIJobModalStatehas been updated to accept these command values, this wiring looks good.Also applies to: 66-133
frontend/javascripts/viewer/view/action_bar_view.tsx (1)
13-14: Action bar AI entrypoint: capability check and default command updated correctlyUsing
APIJobCommand.INFER_NEURONSfor the AI Analysis button and checkingdataset.dataStore.jobsSupportedByAvailableWorkersagainst the four relevantAPIJobCommandvalues preserves the previous gating behavior while aligning with the new command enum. The conditions still ensure the button only appears and is enabled when at least one supported AI job is available.Also applies to: 301-335
app/models/job/JobResultLinks.scala (1)
56-61: LGTM!The
resultLinkPublicmethod correctly handles prefixing relative URLs with the public URL while preserving absolute URLs.app/models/job/JobService.scala (2)
171-196: LGTM! Clean refactor to use JobCompactInfo.The
publicWritesmethod now properly constructs aJobCompactInfowith appropriate field mappings. Token sanitization is correctly applied to remove sensitive credentials from the args.
186-186: Token sanitization is properly consistent across both code paths.Verification confirms that both sanitization approaches are correctly implemented:
- JobService.publicWrites (line 186): Constructs
JobCompactInfofrom a fullJobobject and removes tokens- JobCompactInfo.enrich (Job.scala lines 87-90): Enriches an existing
JobCompactInfoand removes the same tokensThese are not redundant paths—they serve different purposes:
publicWritestransforms a fullJobinto a serializableJobCompactInfoenrichis called onJobCompactInfoobjects fromfindAllCompactto add calculated fieldsBoth remove identical tokens (
"webknossos_token"and"user_auth_token"), ensuring consistent sanitization across all code paths. No issues found.app/models/job/Job.scala (4)
67-92: JobCompactInfo extends JobResultLinks but lacks required abstract members.
JobResultLinksdeclaresdef args: JsObject,def command: JobCommand,def returnValue: Option[String], andprotected def id: ObjectId. WhileJobCompactInfohas fields forargs,command,returnValue, andid, it's missing theprotected def id: ObjectIdoverride sinceidis a publicval. This works in Scala because avalsatisfies adef, but the visibility differs (public vs protected).Additionally,
datasetNameanddatasetIdmethods fromJobResultLinksare public, exposing internal parsing logic. This is acceptable if intentional.
83-90: Potential issue:enrichcallsconstructResultLinkwhich accessesargsbefore sanitization.In the
enrichmethod,constructResultLink(organizationId)is called onthis, which still contains the unsanitizedargs(with tokens). TheconstructResultLinklogic reads fromargsto extractdataset_name,dataset_id, anddo_evaluation. While these specific keys are not sensitive, be aware thatargsisn't sanitized until afterconstructResultLinkruns. This appears correct since the sensitive keys aren't used in link construction.
164-215: Well-designed compact query for improved performance.The
findAllCompactmethod joins all required tables in a single query, avoiding N+1 queries. This aligns with the PR objective of improving job list loading performance. The use ofCOALESCE(j.manualState, j.state)correctly handles the effective state logic at the database level.Minor observation: The tuple type annotation is verbose but necessary for Slick's type inference. Consider adding a brief comment explaining the column mapping for maintainability.
149-155: No SQL injection risk identified—prefix parameter is properly controlled across all callers.The review concern has been verified. In Job.scala,
listAccessQWithPrefixaccepts aprefixparameter that's interpolated via theq""SQL interpolator. All callers of this method pass safe, hardcoded prefixes:
- Line 150: empty prefix (
q"")- Line 166: table alias (
q"j.")The same safe pattern is used in User.scala where
buildSelectionPredicatesis called only with hardcoded prefixSqlToken.raw("u."). Theprefixparameter is never exposed to user-controlled input.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/models/job/JobResultLinks.scala (1)
35-38: Simplify redundant identity map.The map operation on
returnValueis redundant since it simply returns the value unchanged.case JobCommand.infer_neurons if this.argAsBooleanOpt("do_evaluation").getOrElse(false) => - returnValue.map { resultAnnotationLink => - resultAnnotationLink - } + returnValueapp/models/job/Job.scala (1)
164-215: Consider named tuple extraction for maintainability.The positional tuple access (
row._1,row._2, etc.) across 14 columns is fragile and harder to maintain if column order changes. This aligns with the learning that explicit column listing is preferred, but the parsing side could also benefit from named extraction or a helper structure.The current approach works but consider defining an intermediate case class or using Slick's built-in mapping to improve readability and reduce risk of column misalignment during future modifications.
Additionally,
Fox.serialCombinedprocesses rows sequentially. If parsing performance becomes a concern,Fox.combinedcould parallelize parsing:// Alternative if parallelism is acceptable parsed <- Fox.combined(rows.map { row => for { command <- JobCommand.fromString(row._2).toFox effectiveState <- JobState.fromString(row._8).toFox commandArgs <- JsonHelper.parseAs[JsObject](row._7).toFox } yield JobCompactInfo(...) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/job/Job.scala(8 hunks)app/models/job/JobResultLinks.scala(1 hunks)app/models/job/JobService.scala(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-13T09:06:15.202Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Applied to files:
app/models/job/JobResultLinks.scalaapp/models/job/JobService.scalaapp/models/job/Job.scala
📚 Learning: 2025-08-04T11:49:30.012Z
Learnt from: frcroth
Repo: scalableminds/webknossos PR: 8821
File: app/models/dataset/Dataset.scala:864-866
Timestamp: 2025-08-04T11:49:30.012Z
Learning: In WebKnossos Scala codebase, when querying database tables with Slick, explicit column listing in SELECT statements is preferred over SELECT * to ensure columns are returned in the exact order expected by case class mappings. This prevents parsing failures when the physical column order in the production database doesn't match the schema definition order.
Applied to files:
app/models/job/Job.scala
🧬 Code graph analysis (2)
app/models/job/JobResultLinks.scala (3)
app/models/job/JobCommand.scala (1)
JobCommand(5-22)app/models/job/Job.scala (3)
id(38-38)effectiveState(51-51)effectiveState(85-85)util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala (1)
fromStringSync(22-26)
app/models/job/Job.scala (6)
app/models/job/JobState.scala (1)
JobState(5-8)util/src/main/scala/com/scalableminds/util/time/Instant.scala (5)
Instant(14-45)Instant(47-103)now(48-48)fromSql(58-58)fromString(75-78)app/models/job/JobResultLinks.scala (1)
constructResultLink(24-56)app/utils/sql/SimpleSQLDAO.scala (1)
run(28-48)app/models/job/JobCommand.scala (1)
JobCommand(5-22)util/src/main/scala/com/scalableminds/util/tools/JsonHelper.scala (1)
JsonHelper(16-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (14)
app/models/job/JobResultLinks.scala (3)
1-7: LGTM on imports and package declaration.Clean imports with appropriate dependencies for the trait's functionality.
8-23: LGTM on trait definition and helpers.Well-structured trait with clear abstraction boundaries. The protected accessors and JSON extraction helpers provide clean encapsulation.
56-61: LGTM on resultLinkPublic.Defensive handling of both relative and absolute paths is appropriate given the varying return formats from
constructResultLink.app/models/job/JobService.scala (5)
17-17: LGTM on import addition.Adding
JsValueto imports is necessary for the updatedpublicWritesreturn type.
161-161: LGTM on field rename alignment.Correctly updated to use the renamed
argsfield.
171-196: LGTM on publicWrites refactoring.The method now correctly constructs
JobCompactInfowith sanitized args and proper token removal. TheresultLinkis computed via the trait method, maintaining consistency with the compact path.
208-208: LGTM on parameterWrites update.Correctly uses the renamed
argsfield while appending the authentication token for worker processing.
226-226: LGTM on return type alignment.Return type correctly updated to match the new
publicWritessignature.app/models/job/Job.scala (6)
9-9: LGTM on OFormat import.Required for the implicit JSON format definition in
JobCompactInfocompanion object.
67-96: LGTM on JobCompactInfo structure.Well-designed compact representation with the
enrichmethod properly computing the result link and sanitizing sensitive tokens from args. The effective state is correctly pre-resolved viaCOALESCEin the query.
125-126: LGTM on Instant mapping.Correctly uses
Instant.fromSqlto convert SQL timestamps to the newInstanttype.
149-155: LGTM on listAccessQ refactoring.The prefix parameterization enables query reuse across both
findAllandfindAllCompactwhile maintaining consistent access control logic.
296-296: LGTM on insertOne field alignment.Correctly uses the renamed
argsfield for database insertion.
20-65: LGTM on Job case class refactoring.The field rename (
commandArgs→args) and type changes (Option[Long]→Option[Instant]) improve consistency. TheJobResultLinkstrait integration properly provides the protected accessors and enables link construction. Verified that the customInstantclass correctly definesdef -(other: Instant): FiniteDuration, confirming thedurationmethod returns the expected type.
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Thanks for the improvements. I only checked out the backend code. If I should look at the frontend as well, please let me know:
Didn't really find anything, only two minor things and a mini question :)
util/src/main/scala/com/scalableminds/util/requestlogging/RequestLogging.scala
Show resolved
Hide resolved
|
@philippotto when reviewing the frontend, could you also have a quick look at #9095? I’m not super familiar with react or antd, so that one might have better solutions for some of the problems. If you think so, I’m happy to incorporate those here. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/models/job/Job.scala(7 hunks)app/models/job/JobService.scala(5 hunks)frontend/javascripts/admin/job/job_list_view.tsx(12 hunks)frontend/javascripts/admin/task/task_list_view.tsx(2 hunks)frontend/javascripts/admin/tasktype/task_type_list_view.tsx(2 hunks)frontend/javascripts/dashboard/explorative_annotations_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/task/task_list_view.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-21T10:54:16.468Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8961
File: frontend/javascripts/viewer/model/actions/annotation_actions.ts:80-82
Timestamp: 2025-10-21T10:54:16.468Z
Learning: In frontend/javascripts/viewer/model/actions/annotation_actions.ts, the ShowManyBucketUpdatesWarningAction is intentionally not included in the AnnotationActionTypes union because it's a UI-only action that doesn't modify the annotation state through reducers.
Applied to files:
frontend/javascripts/dashboard/explorative_annotations_view.tsx
📚 Learning: 2025-01-13T09:06:15.202Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Applied to files:
app/models/job/Job.scalaapp/models/job/JobService.scala
📚 Learning: 2025-08-04T11:49:30.012Z
Learnt from: frcroth
Repo: scalableminds/webknossos PR: 8821
File: app/models/dataset/Dataset.scala:864-866
Timestamp: 2025-08-04T11:49:30.012Z
Learning: In WebKnossos Scala codebase, when querying database tables with Slick, explicit column listing in SELECT statements is preferred over SELECT * to ensure columns are returned in the exact order expected by case class mappings. This prevents parsing failures when the physical column order in the production database doesn't match the schema definition order.
Applied to files:
app/models/job/Job.scala
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/admin/job/job_list_view.tsx
🧬 Code graph analysis (4)
frontend/javascripts/admin/tasktype/task_type_list_view.tsx (1)
frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
app/models/job/Job.scala (1)
app/models/job/JobCommand.scala (1)
JobCommand(5-22)
frontend/javascripts/admin/job/job_list_view.tsx (4)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getReadableURLPart(747-751)frontend/javascripts/types/api_types.ts (1)
APIJob(822-836)frontend/javascripts/libs/format_utils.ts (2)
formatWkLibsNdBBox(540-553)formatCreditsString(555-559)frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
app/models/job/JobService.scala (2)
app/models/job/Job.scala (5)
JobCompactInfo(67-92)JobCompactInfo(94-96)id(38-38)effectiveState(51-51)effectiveState(85-85)app/models/job/JobResultLinks.scala (1)
constructResultLink(24-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (12)
frontend/javascripts/admin/tasktype/task_type_list_view.tsx (2)
17-17: LGTM! Clean import of the new FormattedId component.The import statement is correct and aligns with the project-wide refactoring to unify ID rendering across admin list views.
157-157: FormattedId component lacks keyboard accessibility.The FormattedId component at
frontend/javascripts/components/formatted_id.tsxuses a<div onClick={...}>without keyboard accessibility support. It's missing:
role="button"to identify the interactive elementtabIndex={0}to make it keyboard-focusableonKeyDownhandler to trigger copy on Enter/Space keysThis prevents keyboard users from copying IDs and violates WCAG Level A accessibility requirements. The fix should be applied to the FormattedId component to affect all usages across admin list views.
⛔ Skipped due to learnings
Learnt from: MichaelBuessemeyer Repo: scalableminds/webknossos PR: 8824 File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039 Timestamp: 2025-09-08T13:33:47.097Z Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.frontend/javascripts/dashboard/explorative_annotations_view.tsx (1)
621-635: Clean refactor to use the unified FormattedId component.The ID column now uses the shared
FormattedIdcomponent, which provides consistent copy-to-clipboard behavior and tooltip across the application. The width increase to 120 appropriately accommodates the component's layout.frontend/javascripts/admin/job/job_list_view.tsx (4)
141-150: LGTM - clean async initialization pattern.Extracting the async
initializefunction allows proper awaiting offetchDatabefore loading persisted state. Error handling is already present infetchDatavia thegetJobscall.
168-178: LGTM - dataset link construction handles both new and legacy formats.The function correctly prefers the new
datasetId-based path while falling back to legacy organization-based paths for older jobs.
329-333: LGTM - Voxelytics workflow link for superusers implemented correctly.The new
renderWorkflowLinkhelper and conditional column visibility align with the PR objective to show workflow links to superusers.Also applies to: 543-545
498-542: LGTM - new columns and filter properly use the updated job data structure.The filter, "Cost in Credits" column, and "Date" column correctly reference the new
job.argsandjob.createdfields. TheformatCreditsStringutility handles credit display formatting appropriately.app/models/job/JobService.scala (2)
171-196: LGTM - publicWrites correctly constructs JobCompactInfo for single-job responses.The method properly sanitizes sensitive tokens from args, constructs the result link, and handles credit cost negation (as noted in past reviews). The inline construction (rather than calling
enrich) is appropriate here sincepublicWritesoperates on a fullJobobject and directly computes all fields.
161-161: Field access updated to use renamedargsfield.The changes from
commandArgstoargsare consistent with the field rename in theJobcase class.Also applies to: 208-208
app/models/job/Job.scala (3)
67-96: LGTM - JobCompactInfo provides a clean compact representation for list queries.The case class properly extends
JobResultLinksto access link construction logic, and theenrichmethod defers expensive computations and sanitizes sensitive tokens. TheeffectiveStateaccessor correctly returnsstatesince the query already appliesCOALESCE(manualState, state).
154-205: LGTM - findAllCompact provides efficient bulk job retrieval.The query joins necessary tables to avoid N+1 queries, uses
LEFT JOINfor optional credit transactions, and the parsing correctly handles command/state validation. The credit cost sign convention is properly documented.
33-38: LGTM - consistent type updates for Instant and field rename to args.The migration from
LongtoInstantfor timestamps is properly applied throughout parsing and computation. Thedurationcalculation correctly usesInstantsubtraction which returnsFiniteDuration.Also applies to: 49-49, 125-127, 286-286
philippotto
left a comment
philippotto
left a comment
There was a problem hiding this comment.
cool stuff! the type clean up is especially welcome. however, I suggest to merge this only after #8873 is merged, because there will be quite a few conflicts and your PR is smaller in comparison (and therefore easier to doublecheck again).
| jobs | ||
| .map(transformBackendJobToAPIJob) | ||
| // Newest jobs should be first | ||
| .sort((a: APIJob, b: APIJob) => a.createdAt > b.createdAt) |
There was a problem hiding this comment.
this doesn't need to happen anymore? I see that there is one other place where the jobs are sorted, but i didn't check whether that covers all usages.
There was a problem hiding this comment.
The server should sort them by creation already. I’ll double-check that it works as expected.
| creditCost: job.creditCost, | ||
| modelId: job.commandArgs.model_id, | ||
| ...job, | ||
| args: _.mapKeys(job.args, (_value, key) => _.camelCase(key)), |
There was a problem hiding this comment.
for some backend types, we have CI checks that they match the TS types. I think it would be cool to have that for the job stuff, too. this would require some dummy jobs in the test db. do you think this is easy to do @fm3 ? see writeTypeCheckingFile in the e2e tests.
There was a problem hiding this comment.
I’m afraid, since the backend does not actually validate the schema in the args field in the database (it’s just a json object there), that this test would not help us much. It would validate that the frontend types match what’s in the test database, but that doesn’t mean that it would match what the production code actually reads or writes. So I don’t think it’s worth adding the test at this point. What do you think?
| // To simplify the frontend code, this is just the union, | ||
| // listing all possible frontend-relevant arguments. |
There was a problem hiding this comment.
TS supports discriminated unions, so it would be possible to separate the properties depending on job command. that would be cleaner and more type safe, but if we are unlucky, it could make some usages more complicated.
but maybe you could add some comments that clarify which props belong to which jobs? then, refactoring it at a later point would be easier for the next reader.
There was a problem hiding this comment.
Since they overlap heavily, mayte this flat union isn’t such a bad option. Everything has to be considered optional in this way, which may make for resilient rendering code. This also allows some older jobs to be rendered, even when their exact parameter set changes for new job instances.
yes, #9095 makes use of react-query which is an elegant solution in my opinion. I'd use that 🙈 we could merge the other pr first, you undo your changes here and then pull (or pull directly and resolve potentially more conflicts). |
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Backend looks good. Thanks for implementing the feedback so fast 🎉
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/admin/job/job_list_view.tsx (2)
135-158: Async initialization + loading state fix the missing spinner but lack error handling.Extracting
initializeand awaitingfetchDatais the right way to make the initial<Spin>work reliably. To avoid a stuck spinner or silent failures on network errors, consider guarding withtry/finallyand surfacing an error toast:- async function initialize() { - await fetchData(); - const { searchQuery } = persistence.load(); - setSearchQuery(searchQuery || ""); - setIsLoading(false); - } + async function initialize() { + try { + await fetchData(); + } catch (e) { + console.error("Failed to load jobs", e); + Toast.error("Failed to load jobs. Please try again."); + } finally { + const { searchQuery } = persistence.load(); + setSearchQuery(searchQuery || ""); + setIsLoading(false); + } + }
180-326: Description rendering matches the newcommand/argsschema; a couple of small robustness nits.The switch from
type+ top-level fields tocommand+job.args.*looks thorough and consistent across all branches. Two minor edge cases worth tightening:
- EXPORT_TIFF with missing
layerName:
IfannotationLayerNameandlayerNameare both absent, the description renderslayer undefined. A small fallback improves UX:- } else if (job.command === APIJobCommand.EXPORT_TIFF && linkToDataset != null) { + } else if (job.command === APIJobCommand.EXPORT_TIFF && linkToDataset != null) { const labelToAnnotationOrDataset = job.args.annotationId != null ? ( <Link to={`/annotations/${job.args.annotationId}`}> annotation of dataset {job.args.datasetName} </Link> ) : ( <Link to={linkToDataset}>dataset {job.args.datasetName}</Link> ); + const effectiveLayerName = layerName ?? "unknown layer"; return ( <span> - Tiff export of layer {layerName} from {labelToAnnotationOrDataset} (Bounding Box{" "} + Tiff export of layer {effectiveLayerName} from {labelToAnnotationOrDataset} (Bounding Box{" "} {job.args.ndBoundingBox ? formatWkLibsNdBBox(job.args.ndBoundingBox) : job.args.boundingBox} ) </span> );
- Training jobs with an empty
trainingAnnotationsarray:
numberOfTrainingAnnotationsis computed defensively, butgetShowTrainingDataLink(job.args.trainingAnnotations)will still try to accesstrainingAnnotations[0]when the array is empty, producing/annotations/undefined. You can harden the helper to treat[]like “no training data”:export const getShowTrainingDataLink = ( trainingAnnotations: { annotationId: string; }[], ) => { - return trainingAnnotations == null ? null : trainingAnnotations.length > 1 ? ( + if (trainingAnnotations == null || trainingAnnotations.length === 0) { + return null; + } + return trainingAnnotations.length > 1 ? (Overall, the mapping from commands to human-readable descriptions looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/job/Job.scala(7 hunks)frontend/javascripts/admin/job/job_list_view.tsx(12 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
📚 Learning: 2025-01-13T09:06:15.202Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Applied to files:
frontend/javascripts/admin/job/job_list_view.tsxapp/models/job/Job.scala
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/admin/job/job_list_view.tsx
📚 Learning: 2025-08-04T11:49:30.012Z
Learnt from: frcroth
Repo: scalableminds/webknossos PR: 8821
File: app/models/dataset/Dataset.scala:864-866
Timestamp: 2025-08-04T11:49:30.012Z
Learning: In WebKnossos Scala codebase, when querying database tables with Slick, explicit column listing in SELECT statements is preferred over SELECT * to ensure columns are returned in the exact order expected by case class mappings. This prevents parsing failures when the physical column order in the production database doesn't match the schema definition order.
Applied to files:
app/models/job/Job.scala
🧬 Code graph analysis (2)
frontend/javascripts/admin/job/job_list_view.tsx (4)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (1)
getReadableURLPart(747-751)frontend/javascripts/types/api_types.ts (1)
APIJob(822-836)frontend/javascripts/libs/format_utils.ts (2)
formatWkLibsNdBBox(540-553)formatCreditsString(555-559)frontend/javascripts/components/formatted_id.tsx (1)
FormattedId(7-27)
app/models/job/Job.scala (5)
app/models/job/JobState.scala (1)
JobState(5-8)app/models/job/JobResultLinks.scala (1)
constructResultLink(24-56)app/utils/sql/SimpleSQLDAO.scala (1)
run(28-48)app/models/job/JobCommand.scala (1)
JobCommand(5-22)util/src/main/scala/com/scalableminds/util/tools/JsonHelper.scala (1)
JsonHelper(16-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (11)
app/models/job/Job.scala (6)
33-34: LGTM: Type safety improvement.The change from
Option[Long]toOption[Instant]for timestamp fields improves type safety and makes the intent clearer. This aligns well with the Instant utility class used throughout the codebase.
67-96: LGTM: Well-designed compact data structure with security consideration.JobCompactInfo is well-structured for efficient job listing. The
enrichmethod correctly removes sensitive tokens (webknossos_tokenanduser_auth_token) from args before exposure, which is an important security measure.
125-126: LGTM: Consistent timestamp parsing.The use of
Instant.fromSqlaligns correctly with the type change fromOption[Long]toOption[Instant]for the started and ended fields.
154-205: LGTM: Efficient compact job listing with proper access control.The
findAllCompactmethod uses a single JOIN query to fetch enriched job data, which is more efficient than N+1 queries. The inline comment on line 202 clearly documents the credit cost sign convention. The 14-tuple result type is complex but necessary given the comprehensive data requirements.The access control using
listAccessQwith theq"j."prefix is correctly implemented, and the parsing logic properly handles type conversions and validation.
277-290: LGTM: Field rename consistency.The change from
j.commandArgstoj.argson line 286 correctly aligns with the field rename in the Job case class.
45-49: The Instant subtraction operator is correctly implemented and the code is valid.The Instant class explicitly defines a
-operator at line 32 ofutil/src/main/scala/com/scalableminds/util/time/Instant.scala:def -(other: Instant): FiniteDuration = (epochMillis - other.epochMillis) millisecondsThis operator correctly calculates the duration between two Instant values. The
sincemethod (line 59) is a separate utility that calculates duration from a past instant to the present (now), not a replacement for the operator. The Job code correctly applies the-operator to subtract two Instant values.frontend/javascripts/admin/job/job_list_view.tsx (5)
18-31: New imports forFormattedIdandAPIJobCommandare consistent with the refactor.Using
FormattedIdfor IDs andAPIJobCommandfor command checks aligns this view with the new API surface and shared ID-rendering behavior.
168-177: Dataset link construction withjob.argslooks correct and backward compatible.Preferring
datasetId+getReadableURLPartand falling back toorganizationId+datasetDirectoryName/datasetNamekeeps old jobs working while generating nice URLs for new ones. No issues spotted here.
329-333: Voxelytics workflow link and column gating look correct.
renderWorkflowLinkonly renders a link whenvoxelyticsWorkflowHashis present, and the column itself is shown only to superusers. That matches the stated requirement and avoids leaking workflow links to regular users.Also applies to: 543-545
375-455: Action rendering now consistently usesAPIJobCommand; behavior aligns with descriptions.The conditional branches for actions (view/download/result text) mirror the command-based descriptions and use
job.resultLink/job.returnValueappropriately. The previous issue with a raw"find_largest_segment_id"string is gone; everything is onAPIJobCommandnow. No functional problems spotted.
498-542: Search, ID rendering, owner, cost, and date columns are aligned with the new API surface.
- Filtering via
Utils.filterWithSearchQueryANDonjob.args.datasetNamematches the args-based shape.Job Idcolumn now usesFormattedId, which satisfies the “single-click copy” requirement.- Owner column uses
ownerLastNamefor sorting and renders last name, first name, and email clearly.Cost in Creditscorrectly formats non-nullcreditCostviaformatCreditsStringand falls back to "-".- Date column uses
FormattedDateonjob.createdwith descending default sort; assumingcreatedis epoch-millis, this should behave as expected.This whole table configuration looks consistent and well-wired.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/javascripts/admin/job/job_list_view.tsx (3)
135-141: React-query integration and loading spinner behavior look correctUsing
useQuerywithdata: jobs, isLoadingand wrapping theTablein<Spin spinning={isLoading}>should reliably show a loading indicator during the initial fetch, addressing the previous “No Data” vs. loading issue. Optional: if you ever want to show a spinner during background refetches as well, you could considerisFetchinginstead of (or in addition to)isLoading.Also applies to: 486-493
30-30: Command-based description/action rendering is consistent with the new API shapeThe switch to
APIJobCommandplusjob.args.*throughoutgetLinkToDataset,renderDescription, andrenderActionsis cohesive and matches the backend’s command/args model. Link generation viajob.args.datasetId(with a legacy fallback usingorganizationId+ dataset name/directory) and the use ofndBoundingBoxvs.boundingBoxlook correct.One small UX nit: in the actions block, the
"View Segmentation"tooltip is reused for several commands (includingCOMPUTE_MESH_FILEandMATERIALIZE_VOLUME_ANNOTATION) where a more generic label like “View Result” or per-command wording might be clearer.Also applies to: 158-167, 170-317, 366-368, 380-391, 403-411, 424-425, 443-443
19-19: Unified Job Id formatting and owner display look goodUsing
<FormattedId id={id} />for the Job Id column is a nice way to centralize ID rendering (copyability, truncation, etc.). The Owner column’s sort byownerLastNameand rendering ofLastName, FirstNamewith the email beneath is clear and should be easy to scan, assuming these fields are always present from the API.Also applies to: 506-506, 513-519
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/admin/job/job_list_view.tsx(11 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
📚 Learning: 2025-01-13T09:06:15.202Z
Learnt from: MichaelBuessemeyer
Repo: scalableminds/webknossos PR: 8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Applied to files:
frontend/javascripts/admin/job/job_list_view.tsx
📚 Learning: 2025-11-03T17:50:57.587Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9025
File: frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx:10-10
Timestamp: 2025-11-03T17:50:57.587Z
Learning: In the webknossos codebase, both `import { Component } from "antd";` and `import { Component } from "antd/lib";` are valid import paths for antd components. Do not flag imports from "antd/lib" as errors.
Applied to files:
frontend/javascripts/admin/job/job_list_view.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (2)
frontend/javascripts/admin/job/job_list_view.tsx (2)
319-323: Workflow link for superusers is straightforward and scoped correctly
renderWorkflowLinkand the conditional “Voxelytics” column for superusers cleanly surface the/workflows/${job.voxelyticsWorkflowHash}link without affecting regular users. This matches the PR objective of exposing workflow reports only to superusers.Also applies to: 533-535
488-492: Dataset-name search now aligns with args-based jobsFeeding
dataSourcethroughUtils.filterWithSearchQueryANDusingjob.args.datasetNamematches the new argument-centric job shape and keeps the search focused on dataset names. As long as non-dataset jobs are not expected to be searchable here, this looks good.
Description
Fixed loading indicators for job and workflow lists.already done in Add loading state spinner for job and workflow list views #9095Steps to test:
yarn enable jobsTODOs:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)