mengw15 commented on code in PR #5271: URL: https://github.com/apache/texera/pull/5271#discussion_r3684460006
########## frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html: ########## @@ -0,0 +1,63 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +<div + class="draggable-panel" + *ngIf="isVisible" + cdkDrag + cdkDragBoundary="texera-workspace"> + <div + class="panel-header" + cdkDragHandle> + <span>Jupyter Notebook</span> + <div class="panel-buttons"> + <!-- Minimize button --> + <button + class="minimize-button" + (click)="minimizePanel()"> + <i + nz-icon + nzType="minus" + nzTheme="outline"></i> + </button> + <!-- Delete button --> + <button + class="delete-button" + nz-popconfirm + nzPopconfirmTitle="Confirm to delete Jupyter notebook" Review Comment: This button says "Confirm to delete Jupyter notebook", but `closePanel()` → `closeJupyterNotebookPanel()` only hides the panel and drops the in-memory mapping — the DB rows stay. That leaves a state the user can't get out of from the UI: 1. Click delete → mapping cleared, panel hidden. `jupyterNotebookExists$` is *not* reset (it's only set in `init()`), so #5273's menu button stays visible. 2. Click that button → `openJupyterNotebookPanel()` checks `hasMapping()`, finds nothing, and warns "No Jupyter notebook associated with this workflow" instead of opening. 3. The notebook is still in the DB; the only way back is to switch workflows and return, which re-fetches and restores the mapping. So today "delete" doesn't delete, but it does break reopening for the rest of the session. Deferring the real backend delete is fine — #4301 lists it as a requirement ("can also delete the notebook associated with the workflow"). Could you link the issue tracking it from the description and here? A few things that might be worth capturing in that issue, since they aren't obvious from the current code: - `closeJupyterNotebookPanel()` is also called by `init()` on every workflow change. Dropping the in-memory mapping there is right (it's just cache eviction — `fetchNotebookAndMapping` restores it on the way back), but a backend delete can't live in the same method, or switching workflows would destroy the stored notebook. Splitting "hide the panel" from "delete the notebook" keeps the two apart. - `notebook.wid` is UNIQUE and `workflow_notebook_mapping` has `FOREIGN KEY (wid, nid) ... ON DELETE CASCADE`, so a single `DELETE FROM notebook WHERE wid = ?` covers both tables. - Beyond the DB rows: the in-memory mapping, `jupyterNotebookExists$`, `cellToHighlightMapping`, and the `notebook.ipynb` still sitting in the Jupyter container all need clearing — and ideally only after the backend confirms, so a failed request doesn't leave the same dead end. Not blocking. -- 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]
