pedroreyesalavrado commented on PR #8637: URL: https://github.com/apache/texera/pull/8637#issuecomment-6084826127
Thanks @kunwp1 — this was the review that found the most, and all seventeen are addressed. Grouping them rather than walking one by one. **The credentials one was a real security bug and the most important thing in your review.** The whole operator configuration was serialized into the prompt, so anyone using the panel on a HuggingFace operator or a SQL source sent that token or password to the model provider. Fixed in 81e7d26: credential fields are dropped before the prompt is built, behind two barriers — the operator's schema marks them with the password widget, and a name pattern covers a schema that cannot be read or an operator missing the annotation. They are dropped rather than masked, since none of the four supported fixes touches a credential. **The panel rerender was a regression I introduced** in the previous round while making the tab appear at all. 383d3a06 gives the console stream its own subscription that toggles only the AI Fix entry, and registers the display-name handler once from ngOnInit rather than on every rerender. **Apply was writing through the modification lock in the primary case, not an edge one** — the traceback arrives while the execution is still `Running`, which is exactly when Texera locks modification. Disabling the button would have made the feature useless, so 587b7b42 kills the paused execution first, which also answers your question about the computing unit it was still holding. The same commit validates field names against the operator, refuses `code` outright, preserves the field's type, and classifies on the final exception line. **The remaining ones** are in 45f3f6e1 (analysis numbering across the unsupported early return, non-string snippets, a way out of the unsupported state, resetting when a run ends), a251a878 (every input port's schema) and b784df20 (indentation of continuation lines, the occurrence count shown beside the confidence badge, and a distinct state for a refused write that keeps the suggestion on screen). **Two places I did something different from what you suggested, both deliberate:** *Resetting the panel.* You suggested resetting when a new execution starts. Our own re-run goes through `Initializing`, so that would wipe the "Fix applied — re-running..." message at the moment it is actually true. It resets when the run **ends** instead, which keeps the message honest while it runs and clears it once it is stale. The kill that Apply itself performs is ignored, or the panel would drop to idle and offer Analyze mid-apply. *`canRun()`.* I took your other option — `executeWorkflow` now returns whether it started (f48df00f). Both entry points already had the shape for it, so it was three `return;` into `return false;`, and the three existing callers ignore the result. The panel now reports what it is told and drops its own copy of the condition along with the `WarehouseService` and `GuiConfigService` it only injected to compute it. Net -16 lines, and the two specs that covered the warehouse case now cover any refusal, which was your actual point: the panel should not know the list of reasons. **Not done:** the shared helper with `notebook-migration/migration-llm.ts`. You flagged it as worth factoring out and I agree, but it is a refactor of code this PR does not otherwise touch, so I would rather do it as a follow-up than widen this further. Say the word if you want it here. 267 specs across the result panel, 6650 across the frontend, lint clean, production build clean. Every fix above was confirmed failing against the code before it — which caught five tests of mine that passed either way and had to be rewritten. -- 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]
