pedroreyesalvarado commented on PR #8637: URL: https://github.com/apache/texera/pull/8637#issuecomment-6354144238
@kunwp1 — all ten, plus the five from @Copilot. Three were in `7ee6339`; the rest are in `e8798c9` and `e4e474f2`. **Read-only workflows.** You were right that this was worse than a missed check. `enableWorkflowModification()` is a no-op while the workflow is read-only, so the unlock the panel waits for never arrives — and running only needs write access to the computing unit, so a read-only user can reach a paused run. The old path killed their run on their behalf and then made them wait fifteen seconds to be told it would not stop. The lock is now refused up front unless a live execution is what holds it, since that is the only lock killing a run can clear. **Indentation was applied twice.** A match that spans lines already carries the absolute indentation of the region it came from, and the model's reply mirrors it, so prefixing the context indentation on top doubled it. Only a single-line match needs the adjustment, which is the case the function was written for. **Zero occurrences.** The panel only surfaces that count when it is above one, so a snippet the model invented was indistinguishable from any other suggestion — and Apply then turned it away as a *stale* snippet, blaming an edit that never happened. The analysis reports it now instead of offering an Apply that cannot work. That one made me look at the rest of the apply path for the same shape, and there was a second instance. `original` for a property change is the field's **current** value, read so the diff has a "before" — so naming a field the operator does not have rendered an empty one, and naming `code` rendered the entire UDF source as one unwrapped line in a `white-space: pre` box. Apply refused both, but only after the panel had shown them. Both are rejected at the analysis now. That also turned up a tautological test of mine. The apply-side field check had a test that passed with the check deleted, because the staleness guard covered the case on its own: an absent field reads as `""` and no longer matches the captured original. With the field *empty* at the analysis both read `""`, and only the presence check can refuse — so it fails without it now. That is the sixth test of mine in this PR that passed either way; red-checking each one is the only reason I keep finding them. **`code` went to the model twice** — fenced under "Operator code" and JSON-escaped inside the configuration, because `code` is a property like any other. The escaped copy is the one no snippet can be quoted from. **The state model.** `unsupported` is a status now rather than a status plus an `errorType`, so the template reads one field to decide one thing and `IDLE_STATE` carries no placeholder classification. `errorType` stays, but only the apply path reads it, to refuse a configuration change for anything but `model_not_found`. **`trackBy`** — you were right, and it undercut the change it was meant to support: `keyvalue` hands `*ngFor` a fresh pair object per entry, so every tab rebuilt whenever one was added, which is exactly the rerender the narrowed console subscription exists to avoid. The tabs are keyed by name now. I should be straight about one thing: **this one has no test.** I tried to assert in the DOM that the tabs survive, but the loop only renders with the panel open, and that mounts the real frames — jsdom dies in `HTMLCanvasElement.getContext`. It is verified by reading, not by a test, and I would rather say so than let it look covered. **Comments that narrated history** now state what the code does and which hazard the shape avoids, and the `buildFixPrompt` doc block is back on `buildFixPrompt`. **From @Copilot:** `onApply` drops its promise explicitly; `executeWorkflow`'s contract now says the request was *sent* rather than that the run started, which is the honest claim — a deployment can still reject it, and that arrives on the execution state stream; the diff falls back to `monospace` rather than `serif`, since the `+`/`-` columns stop lining up the moment monaco is missing; and the duplicated comment block is gone. On the font: `monaco, serif` is the established idiom in this folder — console-frame and error-frame use it in seven other places. I only changed it in the diff, where the alignment actually matters, rather than touching files this PR has no business in. Happy to open a separate one for the rest if you want it. Also worth flagging: Copilot's review did not finish — "couldn't run its full agentic review because it didn't start before the timeout", then "the job was not acquired by a runner even after multiple attempts". So those five are a partial pass, not a clean one. Re-request it if you want the full sweep; I would rather it find something now than after a merge. Frontend suite: **6745 passing**, 290 of them across the result-panel specs (57 service, 24 frame, 8 prompt builder, 67 result panel). `eslint ./src`, `prettier --check` and `tsc --noEmit` clean. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
