UI for pasting auth token from GP Cloud

[dbarashev]

No description provided.

[claude]

PR Review: UI for pasting auth token from GP Cloud

• Read repo guidelines & context
• Review the diff
• Post review feedback

Overall this is a nice simplification — dropping GlobalScope/Dispatchers in favor of FXThread.runLater and always showing the URL/token pane (instead of only on browser-open failure) is cleaner and removes the createUrlPane duplication. openInBrowser internally hops to the Swing EDT via SwingUtilities.invokeLater, so moving its call onto the FX thread does not block the UI — good. A few things to consider below.

---

🐞 Correctness

1. submitToken does not URL-decode the pasted values (SigninPane.kt:161)
The real /auth HTTP path reads params via NanoHTTPD's session.parameters (GPCloudHttpImpl.kt:452), which URL-decodes them. The manual paste path splits the raw string on &/= with no decoding, so a value containing %XX or + would reach onTokenReceived differently than the browser path — potentially a corrupted token/websocketToken. Recommend decoding each value with URLDecoder.decode(value, "UTF-8") to keep the two paths consistent.

val params = rawInput.split("&").associate {
  val parts = it.split("=", limit = 2)
  val key = URLDecoder.decode(parts[0], Charsets.UTF_8.name())
  key to (parts.getOrNull(1)?.let { v -> URLDecoder.decode(v, Charsets.UTF_8.name()) } ?: "")
}

Fix this →

2. Manual paste path skips onAuthReceived (SigninPane.kt:167)
The browser flow calls onTokenReceived and onAuthReceived (GPCloudHttpImpl.kt:468-469); onAuthReceived is what disables the token field. When the user pastes and clicks Submit, only onTokenReceived fires, so the field is never disabled and the user gets no visual confirmation the token was accepted. Consider invoking onAuthReceived (or otherwise updating status/spinner) after a successful manual submit.

3. Pasting a bare token value silently produces token=null
If the user pastes only the token string (not the full token=...&validity=... query string), params["token"] is null and auth will just fail with no feedback. Worth deciding whether the UX expects the full query string, and validating/feeding back accordingly.

---

🎨 UX

4. text.browser_failed is now a permanent label (SigninPane.kt:118)
The URL field's label uses the text.browser_failed message, but the token/URL pane is now shown unconditionally — so the "browser failed / copy this link" wording appears even when the browser opened successfully. That reads as an error on the happy path. Consider a neutral label for the URL row and keeping browser_failed only in statusText on the timeout branch (:155).

---

🧹 Cleanups

• Placeholder option ids ObservableString("foo1", "") / "foo2" (SigninPane.kt:71-72) — give these meaningful ids (e.g. "signin.url", "signin.token"); ids surface elsewhere in the property framework.
• Leftover debug SCSS (GPCloudSignupPane.scss): -fx-border-color: red; -fx-border-width: 0px; looks like debugging residue and should be removed.
• Duplicated timeout constant: startBrowserTimeout hardcodes 60000 (:152) while the new SIGNIN_TIMEOUT_SECONDS = 60 exists — reuse SIGNIN_TIMEOUT_SECONDS * 1000L.
• Mixed helpers: the file uses both FXThread.runLater and FXUtil.runLater (:183) — they're equivalent (FXUtil delegates to FXThread); pick one for consistency.
• Visibility: urlOption / tokenOption are public vals; they look like internal state and could be private.
• Minor: stray double blank line at SigninPane.kt:131-132.

---

✅ Looks good

• PropertySheet.kt change from hardcoded colspan 2 to grid.columnCount correctly adapts single-column ABOVE layouts (the grid here has one ColumnConstraints, so colspan resolves to 1). This is a shared method, but the behavior is equal-or-better for existing 2-column panes.
• The .css → .scss move plus the normalized (leading-slash-free) stylesheet path are consistent with the other entries.

View job run · branch dbarashev/tkt-2808-auth-token-paste

[github-actions]

Test Results

351 tests  ±0   351 ✅ ±0   20s ⏱️ +4s
 96 suites ±0     0 💤 ±0 
 96 files   ±0     0 ❌ ±0 

Results for commit 83c83a4. ± Comparison against base commit adafeef.

♻️ This comment has been updated with latest results.

[claude]

Claude finished @dbarashev's task in 2m 4s —— View job

---

PR Review: UI for pasting auth token from GP Cloud (re-review)

• Read repo guidelines & context
• Review the current diff
• Post review feedback

Thanks for the updates — the previous round of feedback was largely addressed. This is now a clean refactor.

---

✅ Resolved since last review

• URL-decoding — parseTokenString now decodes each key/value with URLDecoder, matching the NanoHTTPD /auth path (SigninPane.kt:173-179).
• onAuthReceived on the manual path — submitToken now invokes both onTokenReceived and onAuthReceived (:164-170), and HttpServerImpl fires it on the browser path too (GPCloudHttpImpl.kt:468). The setController handler disables the token field on either path.
• Bare/partial token feedback — the new validateTokenString validator requires all four params and drives the Submit button's disabled state via the new isValid observable, so garbage input can't be submitted silently (:181-187, :126-129).
• Meaningful option ids (cloud.signin.url / cloud.signin.token), SIGNIN_TIMEOUT_SECONDS reuse in startBrowserTimeout, private visibility, and the red debug border in SCSS — all done.

The isValid wiring is correct: the same ObservableBoolean instance flows from the DSL config into PropertyPaneBuilderImpl, which updates it on validation change, and the watcher added in SigninPane reacts to it. Nice.

---

🎨 UX — one carried-over item

text.browser_failed is still the permanent label for the URL row (SigninPane.kt:116)
The token/URL pane is shown unconditionally now, so the "browser failed" wording appears on the happy path (browser opened fine) as the label above the copyable link. It reads as an error where there isn't one. Consider a neutral label for the URL row and keeping browser_failed only in statusText on the timeout branch (:156).
Fix this →

---

🧹 Minor cleanups

• Unused import: import biz.ganttproject.FXUtil (SigninPane.kt:21) is no longer referenced now that everything uses FXThread.runLater.
• Dead SCSS: the commented-out .btn-secondary { ... } block at the top of GPCloudSignupPane.scss (lines 1-25) is leftover residue and can be dropped.
• ObservableBoolean("",true) (PropertyPaneBuilder.kt:104): empty id and missing space after the comma — give it an id for consistency with the other options.
• Stray double blank line at SigninPane.kt:188-189.

---

💡 Nit (optional)

submitToken relies entirely on the Submit button being disabled for invalid input. That's sound today since the button is the only trigger, but a one-line early return on empty/invalid params would make the function safe if it's ever called from elsewhere.

Nothing blocking here — the remaining items are cosmetic apart from the browser_failed label wording. 351 tests still green.
· branch dbarashev/tkt-2808-auth-token-paste