fix: improve runtime placeholder handling#4447
fix: improve runtime placeholder handling#4447GordonSmith merged 1 commit intohpcc-systems:candidate-3.x.xfrom
Conversation
Fixes previous issues with update and remove Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
e3a1ae8 to
237c452
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves runtime placeholder handling by refactoring the notebook compilation and runtime management system. The changes rename the main compilation function and enhance the NotebookRuntime class with better cell lifecycle management.
- Renamed
compileKittocompileNotebookand addedcompileCellfor individual cell compilation - Enhanced NotebookRuntime with separate
add,update, andclearmethods for better cell management - Added proper error handling for non-existent cells and duplicate cell IDs
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/observablehq-compiler/tests/index-notebookkit.ts | Updated imports and function calls to use renamed compilation functions |
| packages/observablehq-compiler/tests/index-notebookkit.js | Updated imports and function calls to use renamed compilation functions |
| packages/observablehq-compiler/src/kit/runtime.ts | Enhanced runtime with improved cell lifecycle management and error handling |
| packages/observablehq-compiler/src/kit/index.ts | Reordered exports for better organization |
| packages/observablehq-compiler/src/kit/compiler.ts | Refactored compilation to support individual cells and renamed main function |
| packages/observablehq-compiler/src/index.ts | Updated exports to include all kit functionality |
| packages/observablehq-compiler/src/compiler.ts | Updated function call to use renamed compilation function |
| packages/observablehq-compiler/index-kit.html | Uncommented CSS styles for better presentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!state) { | ||
| throw new Error(`Cell with id ${cellId} does not exist`); | ||
| } | ||
| void this.clear(cellId); |
There was a problem hiding this comment.
The void operator is being used to explicitly ignore the Promise returned by this.clear(cellId). However, this could lead to issues if the clear operation fails. Consider awaiting the operation or handling potential errors.
| void this.clear(cellId); | |
| await this.clear(cellId); |
There was a problem hiding this comment.
The void is documenting that await is not needed here.
| ...cell, | ||
| mode: "md", | ||
| value: `\ | ||
| const sourceIDOffset = 1000000; |
There was a problem hiding this comment.
The magic number 1000000 should be defined as a named constant to improve code readability and maintainability. Consider defining it as const SOURCE_ID_OFFSET = 1000000; at the top of the file.
jeclrsg
left a comment
There was a problem hiding this comment.
@GordonSmith Would approve. Copilot had that code-style nitpick about the source id offset being a CONST up at the top of the file. I only mention it because you'd 👎 the other one about the void
Fixes previous issues with update and remove
Checklist:
Testing: