"Add Resources from GP Cloud" improvements

[dbarashev]

• Properly handle exceptions in HTTP layer
• Enable/disable "Add" button depending on the selected resources

[claude]

Claude finished @dbarashev's task in 3m 57s —— View job

---

Code review — PR #2814

• Read PR diff and changed files
• Analyze correctness / error handling
• Post review

The refactoring is a clear improvement: check state moved onto the DTO instead of a side map, exceptions no longer swallowed inside loadTeams, and the coroutine scope is now per-dialog and cancellable. A few things look broken though — most importantly the checkbox itself no longer drives the model.

---

Blocking

1. Clicking the checkbox does not update the model — Apply stays disabled (GPCloudResources.kt:220-236)

item.isChecked and onCheckedToggle() are only touched by the double-click handler on the labels VBox. The CheckBox is bidirectionally bound to the cell-local isChecked property, and nothing propagates that to the item or notifies the page.

Repro: open the dialog, click the checkbox next to a resource → checkbox visibly toggles, but canAddResourcesProperty stays false, so Apply remains disabled; even if it were enabled, addResourcesToProject() filters on it.isChecked and would add nothing. The only way to select a resource is the (undiscoverable) double-click on the name/email labels.

Suggested fix — drive everything from one listener instead of the click handler:

private val isChecked = SimpleBooleanProperty().also { prop ->
  checkBox.selectedProperty().bindBidirectional(prop)   // bind once, see #2
  prop.addListener { _, _, newValue ->
    item?.let { it.isChecked = newValue }
    onCheckedToggle()
  }
}

Fix this →

2. Cell recycling leaks check state into other rows (GPCloudResources.kt:209, 223, 238-241)

isChecked is per-cell and is never re-initialized from item.isChecked in updateItem. Since in-project resources are now sorted to the top (:103-105), their cells set isChecked.value = true and are then recycled for non-project rows as the user scrolls:

• a row whose item.isChecked == false renders with a checked box → misleading, and Apply won't add it;
• worse, the first double-click on such a row flips the stale true to false, so the user has to double-click twice to select it;
• conversely a genuinely checked item scrolled out and back can render unchecked.

updateItem should reset the cell from the item, e.g. isChecked.value = item.isChecked || isInProject(item) before building the graphic.

Related: checkBox.selectedProperty().bindBidirectional(isChecked) at :223 runs on every updateItem call. JavaFX does not de-duplicate bidirectional bindings, so each scroll/refresh adds another listener pair to the same cell. Bind once in the field initializer / init block.

3. reduce throws when the user has no teams (GPCloudResources.kt:101)

loadTeams() legitimately returns emptyList() (no teams owned/participated, or a non-array response). The chain then hits reduce { acc, list -> acc + list } on an empty list → UnsupportedOperationException("Empty collection can't be reduced."). That is not a JsonHttpException, so the catch at :107 doesn't handle it; it escapes to the new CoroutineExceptionHandler and the user gets an alert whose body is that Kotlin internal message, with no list ever filled.

.flatten() handles the empty case naturally and is cheaper than repeated acc + list concatenation:

loadTeams().map { async { loadTeamResources(it) } }.awaitAll().flatten().distinctBy { it.email }

Fix this →

---

Worth fixing

4. JsonHttpException from a team request produces two alerts (GPCloudResources.kt:94-113)

The try/catch sits inside withContext(Dispatchers.IO) { ... }, and the failing request is an async child of that scope. When a child fails, the scope's job completes exceptionally regardless of the catch in the body, so withContext re-throws the JsonHttpException after your handler already showed the alert → the outer launch fails → handleAsyncException shows a second alert. Moving the try/catch outside withContext, or wrapping the per-team call (async { runCatching { loadTeamResources(it) } } / supervisorScope), avoids the double report. This only affects failures from loadTeamResources; a loadTeams() failure is caught correctly since it isn't a child coroutine.

5. isReadOnly is dead code (TeamResources.kt:42, used at GPCloudResources.kt:230)

Nothing ever assigns isReadOnly = true, so !item.isReadOnly is always true. The in-project case is currently handled by isDisable = true on the HBox, which happens to block the mouse events. Either set the flag when the DTO is known to be in the project, or drop it and use isInProject(item) in the guard.

6. fillListView appends instead of replacing (GPCloudResources.kt:118-122)

listView.items.addAll(resources) — active can be set to true more than once for the same page (GPCloudUiFlow.kt:136-146, e.g. after a sign-in round trip), which would duplicate every row. And fillListView(emptyList()) on the error path is a no-op, so stale rows survive an error. listView.items.setAll(resources) fixes both; consider also resetting canAddResourcesProperty there.

7. The scope is only cancelled by the Cancel button (GPCloudResources.kt:181-183)

ESC and the window close button go through hide() (Dialog.kt:520-527) and never invoke the CancelAction; Apply doesn't cancel either. In-flight requests keep running and can pop an alert on a dialog that is already gone. dlg.onClosed = { coroutineScope.cancel() } covers every exit path (and makes the Cancel handler redundant).

8. HTTP layer only wraps SocketTimeoutException (Task.kt:61-65)

Given the PR's goal, this leaves the common failures uncovered: UnknownHostException (message is just the hostname — the alert body would read like cloud.ganttproject.biz), ConnectException, SSLHandshakeException. Catching IOException and wrapping with a meaningful phrase would be more complete. Two smaller points on the same lines: -1 deserves a named constant (STATUS_CODE_UNKNOWN), and "Connection timed out" is a hard-coded English string that ends up in a user-visible alert while the rest of this dialog goes through RootLocalizer — worth an i18n key.

---

Nits

• GPCloudResources.kt:246 — private val i18n is now unused (its only consumer was the removed http.error alert); it will produce an "unused" warning.
• GPCloudResources.kt:69 / :139 — listView.items.filter { … }.any() allocates an intermediate list; listView.items.any { … } is equivalent. (:139 needs the list, so only :69 applies.)
• TeamResources.kt:40-43 — isChecked/isReadOnly on a data class DTO are excluded from equals/hashCode/copy/toString, which is surprising, and it couples the HTTP layer to view state. A thin view-model wrapper in biz.ganttproject.resource would keep the DTO pure. Not a bug here since only distinctBy { it.email } is used.
• GPCloudResources.kt:38 — the wildcard import kotlinx.coroutines.* replaces the explicit imports; most files in this package list them explicitly.
• GPCloudResources.kt:66, :206-207 — minor spacing ({ ResourceListCell, (ResourceDto)-> Boolean, ()->Unit).

I did not build or run the module, so the above is from reading the code only.
· branch dbarashev/resource-cloud-dialog-error-handling

[github-actions]

Test Results

351 tests  ±0   351 ✅ ±0   17s ⏱️ -8s
 96 suites ±0     0 💤 ±0 
 96 files   ±0     0 ❌ ±0 

Results for commit 523b8b9. ± Comparison against base commit b8abea8.