Conversation
✅ Deploy Preview for transformerlab canceled.
|
|
Paragon Review Skipped Hi @deep1401! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
aliasaria
left a comment
aliasaria
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a per-user "recently opened experiments" feature, an experiments manager modal, rename/share modals, and a new user_experiment_access table. The shape is solid — service-layer separation is clean, route ordering for /recent vs /{id} is correct, and require_permission is wired in properly. The most important issues are a race condition in the touch_experiment upsert, missing cleanup of access records on experiment delete, an unhandled ValueError path on rename, and a few client-side robustness/typing issues.
Must Fix (blocks merge)
-
[Correctness] Race condition in
touch_experimentSELECT-then-INSERT. Two concurrent calls (e.g. fast double-click, navigation + manual touch) both observerecord is None, bothsession.add(...), the second commit raisesIntegrityErroragainstuq_user_experiment_access. The route returns 500 and the touch silently fails. This is a frontload-on-startup endpoint, so concurrency isn't theoretical.Use a portable upsert pattern that works on both SQLite and Postgres. Either retry-on-IntegrityError, or update-then-insert-if-zero-rows:
async def touch_experiment(session, user_id, team_id, experiment_id): now = datetime.now(timezone.utc) result = await session.execute( update(UserExperimentAccess) .where( UserExperimentAccess.user_id == user_id, UserExperimentAccess.team_id == team_id, UserExperimentAccess.experiment_id == experiment_id, ) .values(last_opened_at=now) ) if result.rowcount == 0: try: session.add(UserExperimentAccess( user_id=user_id, team_id=team_id, experiment_id=experiment_id, last_opened_at=now, )) await session.commit() except IntegrityError: await session.rollback() # lost the race — the other writer won, that's fine else: await session.commit()
-
[Correctness] Stale access records leak when experiments are deleted.
experiment_delete(inexperiment_service.py) does not delete the matchinguser_experiment_accessrows. Per CLAUDE.md the project intentionally avoids FK constraints, so cleanup must be explicit. The/recentendpoint then returns ids that no longer exist;experiment_service.experiment_get(exp_id)returns falsy and they are filtered out — so the user-visible result is a "recent" list of fewer than 3 items (or empty, falling through to fallback). It also bloats the table over time.Delete the rows in
experiment_delete:async def experiment_delete(id): try: exp = await Experiment.get(id) await exp.delete() # Clean up per-user access records (no FK in this DB by convention). async with get_async_session_ctx() as session: await session.execute( delete(UserExperimentAccess).where( UserExperimentAccess.experiment_id == str(id) ) ) await session.commit() await cache.invalidate("experiments") ...
-
[Correctness]
experiment_renameraisesValueError→ 500 on sanitized-empty input. Router validatesnew_nameis non-empty, butsecure_filename("..."),secure_filename("\$\$\$"),secure_filename("../"), etc. return""and the service raisesValueError("Invalid experiment name")— which propagates as a 500. Should be a 422.Catch in the router (or raise
HTTPExceptionfrom the service):try: result = await experiment_service.experiment_rename(id, new_name) except ValueError as e: raise HTTPException(status_code=422, detail=str(e)) if result is None: raise HTTPException(status_code=404, detail=f"Experiment {id} not found")
Should Fix (important but not blocking)
-
[Performance] N+1 permission checks + N+1 experiment reads in
/recentfallback. When a non-owner with no recent records hits/recent, you iterate every experiment, callingcheck_permissionfor each, until you collect 3. For workspaces with hundreds of experiments and a non-owner user, that's hundreds of DB roundtrips on every dropdown open. A simpler, faster pattern: reuseexperiments_get_all's already-filtered list and slice the first 3. The same applies to the recent path — fetch all permitted experiments once, intersect withrecent_ids, preserve order. -
[Maintainability] Owner column shows raw UUID prefix instead of email.
ExperimentsManagerModal.tsx:664-668falls back tocreated_by.slice(0, 8) + '…'when the creator isn't the current user. You already havemembersloaded — look up the email there:```ts
const ownerLabel =
exp.config?.created_by === currentUserId
? 'you'
: members.find((m) => m.user_id === exp.config?.created_by)?.email
?? 'unknown';
``` -
[Correctness]
ShareExperimentModalsilently swallows delete errors.onRemove'scatch {}leaves the row in the UI even when the request fails. At minimum, surface viasetErrorlikeonAdddoes, and don't optimistically remove fromrulesuntil the response is OK. -
[Maintainability] Stale "recent" mutate after rename only — not after share or permission changes.
mutateRecentis plumbed through rename and delete but not after sharing. If a teammate shares an experiment with the user, the recent list won't reflect access changes until next refresh. Either passmutateRecenttoShareExperimentModal(onClosewould call it) or use SWR key-based revalidation. -
[Style]
anytypes violate the CLAUDE.md "Avoidany" rule. Several spots:(exp: any)atSelectExperimentMenu.tsx,err: any/e: anyinRenameExperimentModal.tsxandShareExperimentModal.tsx. Catch handlers should useunknownand narrow. -
[Correctness] Frontend missing
last_opened_atupdate on the open path ofExperimentsManagerModal.handleOpencallsonExperimentSelect(exp.id)→createHandleClose→/touch+mutate()of the Recent SWR. But the manager's ownuseSWR(GetAll())cache isn't invalidated, so reopening the manager shows stalelast_opened_at. Callmutate()from the manager too, or share the cache key.
Consider Improving
- The migration adds
idx_user_experiment_access_user_teambutget_recent_experiment_idsorders bylast_opened_at DESC. With limit=3 and per-user data this is fine today; a composite(user_id, team_id, last_opened_at DESC)index would be tidier if the table grows. experiment_access_service.touch_experimentcallsawait session.commit()from inside a service that receives the session as a dependency. The session lifecycle is owned by the FastAPI dependency; committing inside the service couples it to that lifecycle. Consider letting the caller commit (consistent with how some other services in this repo are structured).formatRelativeTimeusesMath.floor((Date.now() - date) / 86400000)— wall-clock-day rough math, not calendar days. "Yesterday" is wrong across DST boundaries. Minor, butdate-fns/formatDistanceToNowwould be sturdier.- The new
LEVEL_ACTIONS.admin = ['read', 'write', 'execute', 'delete', 'admin']is defined inline; verify it matches the canonicalVALID_ACTIONSset inpermission_service.pyso the share UI stays in sync with the backend.
What's Working Well
- Route ordering in
experiment.pyis correct:/recentis registered before/{id}, so it won't be shadowed by the dynamic-id route. Easy mistake to make; it was avoided here. - Service-layer split (
experiment_access_service.py) follows the project's "business logic in services, not routers" convention from CLAUDE.md. - The migration uses
table_exists+if_exists=Trueon drops — idempotent and consistent with existing migration patterns. - The unit tests in
test_experiment_access_service.pycorrectly test the upsert branches at the service layer (rather than going through the full router stack), which matches the testing guidance in CLAUDE.md.
|
I was trying this out -- I can validate the renaming issue. nothing happens on renaming the experiment from the list. |
|
Fixed everything now! Renaming was working but the experiments were cached. (@greninja could you check again once?) |
|
Fixed one more issue to mutate current experimentinfo upon rename as well |
|
rename is still not working, had a chat with claude about this and I manually traced the path and verified, it seems: the experiment_rename writes the new name to index.json["config"]["name"], but the readers in experiment_get_all and experiment_get both read the top-level name field, so maybe rename never reachs the UI. |
|
I also saw another potential issue but it seems a bit non-deterministic so not able to reproduce -- when i open the drop down list to see all the experiments on the top left, I sometimes see 3, sometimes 2, sometimes just 1 even though all these times I had atleast 6 in my workspace |
This is not an issue. We see the 3 most recently used ones. If there's no recent then we show them alphabetically but if there's atleast one then we show them as-is |
|
and also when creating a new experiment with an id that already exists it throws this error (for "test" as the id): in the logs + UI error but still allowed me to create create the experiment, was that intentional? or should we not allow duplicate names/ids |
oh I thought we show 3 no matter what as long as there are atleast 3 |
|
@greninja Okay sorry about this, I disabled the experiment rename functionality because it sort of required a migration of the folder if things were on aws and it required moving most files. So thats why I just removed it instead of making it more confusing while waiting for the migration |
Thanks fixed this, it was silently swallowing the error. Fixed it |
yeah this fails loudly now |
3 participants
No description provided.