Fix/requirements upload timing#1769
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: f600ddd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConverted multiple sharing-editor callbacks to async and awaited iframe postMessage; added a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Package Stats on
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the timing of requirements file uploads to ensure Python package dependencies are installed before other files are written to the file system.
- Made file operation callbacks async (
onFileWrite,onFileRename,onFileDelete,onRequirementsChange,onEntrypointChange) - Implemented file sorting logic to prioritize requirements files during batch uploads
- Added a
basenameutility function to extract filenames from paths
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/sharing-editor/src/Editor/index.tsx | Refactored handleFileUpload to sort files (requirements first) and process them sequentially with async/await |
| packages/sharing-editor/src/Editor/file.ts | Added basename utility function to extract the last component of a file path |
| packages/sharing-editor/src/App.tsx | Updated all file operation handlers to be async functions |
| .changeset/ninety-areas-film.md | Added changelog entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/sharing-editor/src/Editor/file.ts (1)
17-23: Tightenbasenameimplementation and edge‑case handlingCurrent logic works for normal paths, but you can simplify and make trailing‑slash behavior more robust:
path.split("/")on a string never yieldsparts.length === 0, so that guard is effectively dead code.- For paths ending with
/(e.g.,"foo/bar/"), this returns""rather than"bar".Consider:
-export function basename(path: string): string | undefined { - const parts = path.split("/"); - if (parts.length === 0) { - return undefined; - } - return parts[parts.length - 1]; -} +export function basename(path: string): string | undefined { + const parts = path.split("/").filter(Boolean); + return parts.pop(); +}This keeps semantics for normal paths while handling empty strings and trailing slashes more predictably.
packages/sharing-editor/src/Editor/index.tsx (1)
40-47: Consider allowingvoid | Promise<void>inEditorPropsfor smoother adoptionThe callbacks in
EditorPropsnow requirePromise<void>returns:onFileWrite: (path: string, value: string | Uint8Array) => Promise<void>; onFileRename: (oldPath: string, newPath: string) => Promise<void>; onFileDelete: (path: string) => Promise<void>; onRequirementsChange: (requirements: string[]) => Promise<void>; onEntrypointChange: (entrypoint: string) => Promise<void>;At runtime,
awaitworks fine with both sync and async callbacks, but this stricter type can break existing TypeScript consumers that previously passed plainvoidcallbacks.To keep the new async behavior while remaining backwards‑compatible for callers, consider widening these to
void | Promise<void>:- onFileWrite: (path: string, value: string | Uint8Array) => Promise<void>; - onFileRename: (oldPath: string, newPath: string) => Promise<void>; - onFileDelete: (path: string) => Promise<void>; - onRequirementsChange: (requirements: string[]) => Promise<void>; - onEntrypointChange: (entrypoint: string) => Promise<void>; + onFileWrite: ( + path: string, + value: string | Uint8Array, + ) => void | Promise<void>; + onFileRename: (oldPath: string, newPath: string) => void | Promise<void>; + onFileDelete: (path: string) => void | Promise<void>; + onRequirementsChange: (requirements: string[]) => void | Promise<void>; + onEntrypointChange: (entrypoint: string) => void | Promise<void>;That preserves existing usages while still supporting fully async handlers inside
App.tsx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/ninety-areas-film.md(1 hunks)packages/sharing-editor/src/App.tsx(6 hunks)packages/sharing-editor/src/Editor/file.ts(1 hunks)packages/sharing-editor/src/Editor/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Follow Prettier formatting (2 spaces, trailing commas, semicolons by default) as enforced by the project's configuration
Files:
packages/sharing-editor/src/Editor/file.tspackages/sharing-editor/src/Editor/index.tsxpackages/sharing-editor/src/App.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace ESLint configs for linting
Files:
packages/sharing-editor/src/Editor/file.tspackages/sharing-editor/src/Editor/index.tsxpackages/sharing-editor/src/App.tsx
🧬 Code graph analysis (1)
packages/sharing-editor/src/Editor/index.tsx (2)
packages/sharing-editor/src/Editor/file.ts (1)
basename(17-23)packages/common/src/requirements-txt.ts (1)
parseRequirementsTxt(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: test-sharing-editor
- GitHub Check: test-stlite-lib
🔇 Additional comments (4)
.changeset/ninety-areas-film.md (1)
1-5: Changeset description matches behavioral changeThe summary accurately reflects the new upload behavior (install requirements before writing files) and is clear enough for consumers.
packages/sharing-editor/src/App.tsx (1)
147-181: Async handlers now correctly await iframe messaging before state updatesConverting the file/requirements/entrypoint handlers to
asyncand awaitingiframeRef.current?.postMessage(...)before callingupdateAppDatagives a clear ordering: iframe side sees the operation first, then local app state is updated. The optional‑chaining pattern gracefully no‑ops when the iframe ref is not yet set.This looks consistent across write/rename/delete/install/reboot handlers and aligns well with the Editor’s new Promise‑based callbacks.
Also applies to: 183-223, 225-248, 250-269, 271-288
packages/sharing-editor/src/Editor/index.tsx (2)
167-188: Requirements‑first upload flow looks goodDefining
isRequirementsFilewithbasenameand sorting so that requirements files come first, thenawait‑ingonRequirementsChangeinside the loop, does enforce the intended ordering: requirements are installed before any non‑requirements files are written.The overall structure of
handleFileUpload(decode text vs. binary, update tab order, and focus the new tab after each write) is coherent and matches the changes described in the changeset.
165-209: This concern is not applicable—onFileWriteis typed to always returnPromise<void>The
EditorPropsinterface definesonFileWrite: (path: string, value: string | Uint8Array) => Promise<void>, which means the type system guarantees this callback returns a Promise. TypeScript will reject any consumer that provides a non-async implementation, so calling.then()directly on line 201 is type-safe and correct. The proposedPromise.resolve()wrapper is unnecessary and would not add value here.Likely an incorrect or invalid review comment.
Contributor
Bundle visualizer reports for
|
Contributor
|
Deployment completed successfully (log). Importable URLs:
import { StliteApp, createKernel } from "https://c9acd33f.stlite-react-preview.pages.dev/stlite.js";
import "https://c9acd33f.stlite-react-preview.pages.dev/stlite.css"; |
Contributor
|
Deployment completed successfully (log).
|
Contributor
|
Deployment completed successfully (log).
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>Stlite Browser preview</title>
<link rel="stylesheet" href="https://e1952cd3.stlite-browser-preview.pages.dev/stlite.css" />
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<script type="module">
import { mount } from "https://e1952cd3.stlite-browser-preview.pages.dev/stlite.js"
mount(
{
entrypoint: "streamlit_app.py",
files: {
"streamlit_app.py": `
import streamlit as st
st.write("Hello world")
`,
},
requirements: [],
},
document.getElementById("root"),
);
</script>
</body>
</html> |
Contributor
Package Stats on
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (path, value) => { | ||
| iframeRef.current?.postMessage({ | ||
| async (path, value) => { | ||
| await iframeRef.current?.postMessage({ |
There was a problem hiding this comment.
Using await with optional chaining (?.) is problematic. If iframeRef.current is null or undefined, the expression evaluates to undefined (not a Promise), and await undefined immediately resolves to undefined. This defeats the purpose of making the function async. Consider either checking for null explicitly before the postMessage call, or removing the optional chaining operator.
Suggested change
| await iframeRef.current?.postMessage({ | |
| if (!iframeRef.current) { | |
| return; | |
| } | |
| await iframeRef.current.postMessage({ |
| } | ||
|
|
||
| iframeRef.current?.postMessage({ | ||
| await iframeRef.current?.postMessage({ |
There was a problem hiding this comment.
Using await with optional chaining (?.) is problematic. If iframeRef.current is null or undefined, the expression evaluates to undefined (not a Promise), and await undefined immediately resolves to undefined. This defeats the purpose of making the function async. Consider either checking for null explicitly before the postMessage call, or removing the optional chaining operator.
Suggested change
| await iframeRef.current?.postMessage({ | |
| if (!iframeRef.current) { | |
| // Optionally, you could throw an error or handle this case differently | |
| return; | |
| } | |
| await iframeRef.current.postMessage({ |
| (path) => { | ||
| iframeRef.current?.postMessage({ | ||
| async (path) => { | ||
| await iframeRef.current?.postMessage({ |
There was a problem hiding this comment.
Using await with optional chaining (?.) is problematic. If iframeRef.current is null or undefined, the expression evaluates to undefined (not a Promise), and await undefined immediately resolves to undefined. This defeats the purpose of making the function async. Consider either checking for null explicitly before the postMessage call, or removing the optional chaining operator.
Suggested change
| await iframeRef.current?.postMessage({ | |
| if (!iframeRef.current) return; | |
| await iframeRef.current.postMessage({ |
| (requirements) => { | ||
| iframeRef.current?.postMessage({ | ||
| async (requirements) => { | ||
| await iframeRef.current?.postMessage({ |
There was a problem hiding this comment.
Using await with optional chaining (?.) is problematic. If iframeRef.current is null or undefined, the expression evaluates to undefined (not a Promise), and await undefined immediately resolves to undefined. This defeats the purpose of making the function async. Consider either checking for null explicitly before the postMessage call, or removing the optional chaining operator.
| (entrypoint) => { | ||
| iframeRef.current?.postMessage({ | ||
| async (entrypoint) => { | ||
| await iframeRef.current?.postMessage({ |
There was a problem hiding this comment.
Using await with optional chaining (?.) is problematic. If iframeRef.current is null or undefined, the expression evaluates to undefined (not a Promise), and await undefined immediately resolves to undefined. This defeats the purpose of making the function async. Consider either checking for null explicitly before the postMessage call, or removing the optional chaining operator.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/sharing-editor/src/Editor/file.spec.ts (1)
1-28: Good coverage of basename behaviour, including edge cases.The tests exercise typical paths, no‑slash names, trailing slashes, and empty strings, which covers the important behaviours of
basename. As a small optional enhancement, you could add a case like"/dir/file.txt"to document behaviour for absolute paths, but the current suite is already solid.Based on learnings, colocating
file.spec.tsbesidefile.tsmatches the repo’s testing conventions.packages/sharing-editor/src/Editor/index.tsx (1)
165-209: Requirements‑first upload flow looks correct; consider avoiding duplicate tabs on re‑upload.The new
isRequirementsFilehelper plusbasenameand the sortedfilesarray correctly ensure that requirements files (including nestedrequirements.txt) are processed first, andawait onRequirementsChange(...)gives you the intended timing guarantees before other uploads run. DeferringfocusTabNext/setTabFileNamesuntilonFileWriteresolves is also a nice improvement.One small UX issue:
setTabFileNames((cur) => [...cur, file.name]);will add duplicate entries if a user uploads a file whose name already exists. You can cheaply guard against that:- onFileWrite(file.name, data).then(() => { - focusTabNext(file.name); - setTabFileNames((cur) => [...cur, file.name]); - }); + onFileWrite(file.name, data).then(() => { + focusTabNext(file.name); + setTabFileNames((cur) => + cur.includes(file.name) ? cur : [...cur, file.name], + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sharing-editor/src/Editor/file.spec.ts(1 hunks)packages/sharing-editor/src/Editor/file.ts(1 hunks)packages/sharing-editor/src/Editor/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Follow Prettier formatting (2 spaces, trailing commas, semicolons by default) as enforced by the project's configuration
Files:
packages/sharing-editor/src/Editor/file.spec.tspackages/sharing-editor/src/Editor/file.tspackages/sharing-editor/src/Editor/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace ESLint configs for linting
Files:
packages/sharing-editor/src/Editor/file.spec.tspackages/sharing-editor/src/Editor/file.tspackages/sharing-editor/src/Editor/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-03T02:33:22.625Z
Learnt from: CR
Repo: whitphx/stlite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T02:33:22.625Z
Learning: Applies to **/*.test.{ts,tsx} : Keep test specs beside components with naming convention `Component.test.tsx`
Applied to files:
packages/sharing-editor/src/Editor/file.spec.ts
📚 Learning: 2025-12-03T02:33:22.625Z
Learnt from: CR
Repo: whitphx/stlite PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T02:33:22.625Z
Learning: Applies to packages/browser/e2e-tests/**/*.{ts,tsx,js,jsx} : Browser E2E scenarios live in `packages/browser/e2e-tests`; extend them when touching mounting, requirements parsing, or iframe messaging
Applied to files:
packages/sharing-editor/src/Editor/file.spec.ts
🧬 Code graph analysis (1)
packages/sharing-editor/src/Editor/file.spec.ts (1)
packages/sharing-editor/src/Editor/file.ts (1)
basename(17-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: test-sharing-editor
- GitHub Check: test-stlite-lib
🔇 Additional comments (1)
packages/sharing-editor/src/Editor/file.ts (1)
17-20: basename implementation is correct and matches expected semantics.Splitting on
/and returning the last segment behaves correctly for empty strings, trailing slashes, and nested paths, and aligns with the new tests. No changes needed.
Comment on lines
39
to
47
| export interface EditorProps { | ||
| appData: AppData; | ||
| pythonCodeCompletionCallback: CodeCompletionFn; | ||
| onFileWrite: (path: string, value: string | Uint8Array) => void; | ||
| onFileRename: (oldPath: string, newPath: string) => void; | ||
| onFileDelete: (path: string) => void; | ||
| onRequirementsChange: (requirements: string[]) => void; | ||
| onEntrypointChange: (entrypoint: string) => void; | ||
| onFileWrite: (path: string, value: string | Uint8Array) => Promise<void>; | ||
| onFileRename: (oldPath: string, newPath: string) => Promise<void>; | ||
| onFileDelete: (path: string) => Promise<void>; | ||
| onRequirementsChange: (requirements: string[]) => Promise<void>; | ||
| onEntrypointChange: (entrypoint: string) => Promise<void>; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see the structure of the Editor component
cd packages/sharing-editor/src/Editor
ls -laRepository: whitphx/stlite
Length of output: 767
🏁 Script executed:
# Get the full Editor component file to check implementations
wc -l index.tsxRepository: whitphx/stlite
Length of output: 98
🏁 Script executed:
# Read the Editor component to understand the structure and callback usage
cat -n index.tsx | head -100Repository: whitphx/stlite
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Read the full Editor index.tsx file with correct path
cat -n packages/sharing-editor/src/Editor/index.tsx | head -350Repository: whitphx/stlite
Length of output: 13985
🏁 Script executed:
#!/shell
# Check if there's an ESLint or Prettier config to verify formatting compliance
find packages/sharing-editor -name ".eslintrc*" -o -name "prettier.config*" -o -name "tsconfig.json" | head -20Repository: whitphx/stlite
Length of output: 137
🏁 Script executed:
# Check the root ESLint/Prettier configs
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "prettier.config*" -o -name "package.json" \) | grep -E "^\./\." | head -10Repository: whitphx/stlite
Length of output: 40
🏁 Script executed:
# Let's check the specific callback invocations more carefully
cat -n packages/sharing-editor/src/Editor/index.tsx | sed -n '127p;139p;143p;146p;154p;165p;195p;201p;211p;218p;224p;228p;321p;322p;330p;334p;336p'Repository: whitphx/stlite
Length of output: 1018
Async EditorProps callbacks are treated as sync fire‑and‑forget in several places, risking unhandled promise rejections and state divergence.
onFileWrite, onFileRename, onFileDelete, onRequirementsChange, and onEntrypointChange are typed to return Promise<void>, but most call sites invoke them without await or .catch():
handleSave(line 139, 143): callsonRequirementsChangeandonFileWritewithout awaitinghandleBinaryFileChange(line 154): callsonFileWritewithout awaitinghandleFileDelete(line 218): callsonFileDeletewithout awaitinghandleCreateFile(line 228): callsonFileWritewithout awaitingonFileNameChangehandler (line 322, 330): callsonFileRenameandonEntrypointChangewithout awaitingonEntrypointSethandler (line 336): callsonEntrypointChangewithout awaiting
In each case, UI state updates occur immediately after the call, meaning the UI can reflect success even if the underlying operation fails. This creates unhandled promise rejections and state divergence between the Editor and its parent.
handleFileUpload partially handles this correctly (line 195 awaits onRequirementsChange), but line 201 uses .then() instead of await.
Consider either keeping these callbacks void if Editor should not be aware of async operations, or make the relevant handlers async and await the calls with try/catch (or at minimum .catch() for logging), deferring state updates until after the promise resolves.
Contributor
Bundle visualizer reports for
|
Contributor
|
Deployment completed successfully (log). Importable URLs:
import { StliteApp, createKernel } from "https://7ccb7d3c.stlite-react-preview.pages.dev/stlite.js";
import "https://7ccb7d3c.stlite-react-preview.pages.dev/stlite.css"; |
Contributor
|
Deployment completed successfully (log).
|
Contributor
|
Deployment completed successfully (log).
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>Stlite Browser preview</title>
<link rel="stylesheet" href="https://718344bf.stlite-browser-preview.pages.dev/stlite.css" />
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<script type="module">
import { mount } from "https://718344bf.stlite-browser-preview.pages.dev/stlite.js"
mount(
{
entrypoint: "streamlit_app.py",
files: {
"streamlit_app.py": `
import streamlit as st
st.write("Hello world")
`,
},
requirements: [],
},
document.getElementById("root"),
);
</script>
</body>
</html> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Successor to #1766
For #1100
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.