charm-cleanup-again #608
charm-cleanup-again #608
Conversation
| await charmManager.add([cell]); | ||
| log(charmManager, "charmmanager after adding cell"); | ||
| // await charmManager.add([cell]); | ||
| // log(charmManager, "charmmanager after adding cell"); |
There was a problem hiding this comment.
this is half-way written code by @ellyxir - I asked her if I can just comment out - she said yep
| }, | ||
| }); | ||
|
|
||
| failed = !await t.step({ |
There was a problem hiding this comment.
Is this the idiomatic way to use Deno.test? I've never seen !await in my life!
Why did we have to move away from BDD?
There was a problem hiding this comment.
might not need to now - I was getting BDD hard killing after a minute .. I think
I'll try to switch back
There was a problem hiding this comment.
While making deno-web-test, discovered that e.g. @std/testing/bdd is implemented via Deno.test((t) => {}), where the nested BDD scopes are "steps" inside Deno.test
There was a problem hiding this comment.
That being said, I don't see the benefit of handrolling that
jumble/integration/utils.ts
Outdated
| export const login = async (page: Page) => { | ||
| export const login = async (page: Page, additionalWaitTime: number) => { | ||
| // Wait a second :( | ||
| // See if #user-avatar is rendered | ||
| // Check if we're already logged in | ||
| await sleep(1000); | ||
| await sleep(1000 + additionalWaitTime); |
| const ir = getIframeRecipe(charm); | ||
| iframeRecipe = ir?.iframe ?? null; | ||
| } catch (e) { | ||
| console.info(e); |
There was a problem hiding this comment.
I think console.warn('Failed to find iframe recipe' e); would be more appropriate
There was a problem hiding this comment.
I was debating this - as this most likely means it isn't an iframe charm. I think I should have a way to check "is iframe" first - and then if getting the recipe fails then explode
There was a problem hiding this comment.
leaving this for now - happy to change in next round
jumble/src/utils/charm-operations.ts
Outdated
| if (!fixedCode) { | ||
| throw new Error("Could not extract fixed code from LLM response"); | ||
| } | ||
| ))!; |
There was a problem hiding this comment.
Seems like a trap to ! this, what if it fails?
| console.group("Extending Charm"); | ||
| console.log("Performing extension"); | ||
| console.log("Focused Charm ID", focusedCharmId); | ||
| console.log("Focused Replica ID", focusedReplicaId); | ||
| console.log("Input", input); | ||
| console.log("Variants", variants); | ||
| console.log("Preferred Model", preferredModel); | ||
| const charm = await charmManager.get(focusedCharmId); | ||
| console.log("CHARM", charm); |
There was a problem hiding this comment.
Why did this have to go? It's very useful and it's in a console group already
There was a problem hiding this comment.
I'll re-add - I thought it was just claude
| generateNewRecipeVersion( | ||
| charmManager, | ||
| charm, | ||
| workingSrc, | ||
| iframeRecipe.spec, | ||
| ).then((newCharm) => { | ||
| console.log("Fixme, navigate to new charm", newCharm); | ||
| // navigate(createPath("charmShow", { | ||
| // charmId: charmId(newCharm)!, | ||
| // replicaName: replicaName, | ||
| // })); | ||
| }); |
There was a problem hiding this comment.
Seems like we should finish this?
There was a problem hiding this comment.
yes, this is the ticket around finishing code editting which is what I want to get to next
| // NOTE(ja): this doesn't sync recipe to storage | ||
| if (recipeId) await storage.syncCellById(this.space, { "/": recipeId }); |
There was a problem hiding this comment.
it just creates a cell ... with nothing in it
| // FIXME(JA): this really really really needs to be revisited | ||
| async syncRecipe(charm: Cell<Charm>): Promise<string | undefined> { | ||
| async syncRecipe(charm: Cell<Charm>): Promise<string> { |
There was a problem hiding this comment.
Can you say more about the future improvements we need here?
There was a problem hiding this comment.
We should clarify what recipes go where - since right now we are throwing everything at blobby. should we rely on blobby for anything more than "spellbook/sharing"
how does that impact spellcasting? maybe everything should go?
would love to whiteboard about this
There was a problem hiding this comment.
Nice work on this file, thanks for the sanity pass.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
No description provided.