fix : replace manual index.html patching with config-based plugin (#114)#135
fix : replace manual index.html patching with config-based plugin (#114)#135Dbansal06 wants to merge 6 commits intosoft-eng-practicum:developfrom
Conversation
…ugin (soft-eng-practicum#114) The postMessage signal AnalySim relies on to detect when JupyterLite is ready was injected by patching a manually copied index.html file. Every jupyter lite build regenerates that file, silently wiping the patch out. This is why upgrading JupyterLite has historically required manual work. Changes in this commit: - analysim_notifier plugin: the postMessage logic now lives in a proper JupyterLite plugin using app.started, which is the stable API for this. The plugin survives version upgrades without any manual steps. - jupyter_lite_config.json: explicit build config so output directory is always dist/ and no manual steps are needed after building. - jupyter-lite.json: runtime config for app name and extension settings, previously absent entirely. - overrides.json: UI defaults (theme) in one declarative place. - Consolidated JupyterLiteStorageService: the service existed in two near-identical copies in admin and project folders. Moved to app/shared/services/ with old files re-exporting for compatibility. - Fixed event listener leak in ProjectNotebookItemDisplayComponent: the window message listener added in ngOnInit was never removed in ngOnDestroy, causing listeners to accumulate on repeated opens. Fixes soft-eng-practicum#114
79bc27a to
ace5af6
Compare
cengique
left a comment
There was a problem hiding this comment.
@Dbansal06 I actually like this solution compared to #120 as it's simpler. However, I am not able to run this version because it gives the "it took to long" error and not staying in the JupyterLite interface. I did follow the instructions in the PR to build JupyterLite and I don't see any illuminating errors in the console. Except these:
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 2ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 7ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 7ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 7ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 6ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 11ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 11ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 11ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 8ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 7ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 7ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 3ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
[HTTP/2 404 11ms]
XHR
GET
https://localhost:5001/assets/jupyter/dist/api/contents/all.json
|
@cengique |
cengique
left a comment
There was a problem hiding this comment.
still doesn't work for me. Please add debug statements to troubleshoot.
| .DS_Store | ||
|
|
||
|
|
||
| .jupyterlite.doit.db |
|
|
||
|
|
||
| .jupyterlite.doit.db | ||
| package-lock.json |
| .jupyterlite.doit.db | ||
| package-lock.json | ||
|
|
||
| src/Analysim.Web/ClientApp/src/assets/jupyter/dist/ |
There was a problem hiding this comment.
already in separate .gitignore
Note: PR #120 by @mohab-elshamy addresses the same core issue using a PyPI-published extension. This PR takes a complementary approach — shipping the plugin locally in the repo — and additionally fixes three other problems not covered by #120: consolidating the duplicate JupyterLiteStorageService, fixing a window event listener memory leak in ProjectNotebookItemDisplayComponent, and adding the jupyter_lite_config.json build config so the output path is explicit.
Closes #114
What was wrong
The
postMessage('jupyterlite-load')signal that tells AnalySim whenJupyterLite is ready was injected by patching a manually copied
index.htmlfile. Everyjupyter lite buildregenerates that file,silently wiping out the patch. This is why upgrading JupyterLite has
historically required manual steps after every version bump.
What I changed
analysim_notifier/pluginThe postMessage logic now lives in a proper JupyterLite plugin that uses
app.started— the stable API for knowing when the environment is ready.It gets picked up automatically at build time and survives version upgrades
without any manual intervention.
Config files
jupyter_lite_config.json— explicit build config, output always goesto
dist/, no guessing requiredjupyter-lite.json— runtime config for app name and disabled extensions,previously absent
overrides.json— UI theme defaults in one declarative placeConsolidated
JupyterLiteStorageServiceThe IndexedDB service existed in two near-identical copies — one in the
admin folder (
forageIndexDb.ts) and one in the project folder(
localforageIndexdb.ts). They used slightly different storage namesmeaning bug fixes had to be applied twice. Moved to
app/shared/services/with the old files re-exporting from the new location for backwards
compatibility.
Fixed event listener memory leak
ProjectNotebookItemDisplayComponentadded awindowmessage listenerin
ngOnInitbut never removed it inngOnDestroy. Opening and closinga notebook repeatedly would accumulate stale listeners. Fixed by storing
a reference to the bound handler and removing it properly on destroy.
How to test
cd src/Analysim.Web/ClientApp/src/assets/jupyter pip install -r requirements.txt jupyter lite buildRun the app and open any notebook — it should load exactly as before,
but no manual file copying is needed after building.
Notes
The
dist/output path is unchanged so nothing else in the codebaseneeds updating. The
index.htmlthat was manually maintained is deleted— its only custom logic (the postMessage) now lives in the plugin.