-
Notifications
You must be signed in to change notification settings - Fork 148
Allow starting up web when workflow data directory is not present #680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Allow starting up web when workflow data directory is not present #680
Conversation
… auto-detect workflow data directory in web UI after a workflow is run Signed-off-by: Karthik Kalyanaraman <karthik@scale3labs.com>
🦋 Changeset detectedLatest commit: 7287486 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
|
@karthikscale3 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
| if (!envVars.WORKFLOW_LOCAL_DATA_DIR) { | ||
| const cwd = getWorkflowConfig().workingDir; | ||
| const repoRoot = await findRepoRoot(cwd, cwd); | ||
| const searchDir = repoRoot && repoRoot !== cwd ? `${cwd}:${repoRoot}` : cwd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed so the server action can constrain the search for data dir within the project root
| * @returns The detected data directory path, or null if not found | ||
| */ | ||
| export async function detectWorkflowDataDir( | ||
| searchDir?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is passed down from the client
|
|
||
| // On initial load, try to detect the data directory if not explicitly set | ||
| useEffect(() => { | ||
| if (!isLocalBackend || dataDirFromUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't run if dataDir is already set or if its not a local backend
|
Will review this soon! Note that I also have a thicker PR for this here: #626 but haven't gotten to finish it. I think yours is more concise. Once I'm back in ~half a day to review this, I'll probably take a look to ensure the CLI still errors if the local directory isn't found (since the CLI doesn't have the same refreshing behavior), but otherwise the PR seems good at first glance |
VaguelySerious
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming back to review - this is going in the right direction, but I think still needs some work so that:
- The CLI should still tell you if it can't find the dataDir
- The web UI should not rely on pressing "refresh" to be accurate. I think it should just tell you e.g. "hey, couldn't find the data dir in , totally fine, once you run some workflows, they should show up here"
- Check once a second or do a file system subscription for the folder so it automatically resolves
Otherwise, there might be cases where users get very confused as to why they aren't seeing any runs, despite them existing in a different folder, and there's little debugging indication
Also, separately, it'd be neat if the dataDir was simply initialized as part of the initial bundling step, so that "an app running workflows" guarantees "workflow-data can be found", and the CLI/UI warning/error messages make more sense. That's what we should have done from the beginning, but I think both that and the UI/CLI improvements are useful in separation too
|
I'm adding these smaller fixes here:
After which this PR could use the dataDir helper introduced in those PRs, show a warning if the dir can't be found, and if not found (or the list is empty), auto-refreshing every second until the first time something is found. |
Thanks for the feedback @VaguelySerious ! And yes, I initially thought about setting the correct data directory so we do not need to do any search. But, I was under the impression that the data directory could change based on the framework or if the user set a different path using the env var. But, I guess we could still deterministically infer it. Let me iterate on this further and update soon. |
This PR makes the changes to allow starting up the o11y dashboard(
web) even when the workflow data directory is not already present in the project.Current behaviour:
Currently, when we run
npx workflow webfrom a project that has no workflow data directory(no workflow runs), the o11y dashboard fails to startup with an error stating no workflow data directory is present.New behaviour:
With the changes from this PR, when the o11y dashboard startups,
searchDirurl param pointing to the project root and will allow starting up the o11y dashboard.webwill search the workflow data directory inside thesearchDirand will set thedataDircorrectly after it finds it. This will happen ONLY whendataDiris not set and ONLY for local backend.