No reviewers
Labels
No labels
source:analyste
source:defenseur
source:human
source:medic
status:approved
status:blocked
status:in-progress
status:needs-fix
status:ready
status:review
status:triage
type:bug
type:feature
type:infra
type:refactor
type:schema
type:security
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: maximus/simpl-liste#64
Loading…
Reference in a new issue
No description provided.
Delete branch "issue-60-fix-duplicate-inbox"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #60
Fixes #61
Fixes #62
Fixes #63
Changements
#60 — Duplicate inbox
sync/route.ts: merge inbox dans une transaction atomique lors du sync mobile#61 — Refresh
RefreshCwdansTaskList.tsxRefreshControl(swipe-to-refresh) sur inbox et liste#62 — Profondeur sous-tâches
depth >= 1parentId#63 — Chevron vs détail
>/v: toggle la visibilité des sous-tâches (affiché seulement si sous-tâches existent)Test plan
Review — PR #64
Verdict: ✅ APPROVE
Le fix est correct et bien ciblé. La logique de déduplication d'inbox lors du sync est solide : vérifier l'existence, réassigner les tâches, soft-delete l'ancien. Le fix du seed avec un ID fixe est une bonne mesure préventive.
Suggestions (non-bloquantes)
route.ts:200-218— Pas de transaction : Les trois opérations (select inbox, reassign tasks, soft-delete old inbox) ne sont pas dans une transaction. Si le serveur crash entre le reassign et le soft-delete, on se retrouve avec deux inboxes actives. Unedb.transaction()serait plus robuste.seed.ts:22— ID fixe partagé entre users : L'ID00000000-0000-0000-0000-000000000001est identique pour tous les users. Si deux users différents exécutent le seed, il y aura un conflit de PK. Pas un problème en pratique actuellement, mais une fragilité à garder en tête.route.ts:214— Sous-tâches orphelines : Le reassign ne met à jour que les tâches directement dans l'ancien inbox (listId = existingInbox.id). Les sous-tâches avec unlistIddifférent ne seraient pas migrées (peu probable avec le schéma actuel).Review automatisée par Claude Code
PR Review: fix: resolve duplicate inbox on web after mobile sync (#60)
Verdict: APPROVE
Summary
The sync endpoint now correctly handles the case where a mobile client pushes an inbox list that conflicts with an existing web inbox. Tasks are reassigned within a transaction and the old inbox is soft-deleted. The approach is sound and well-scoped.
Suggestions (non-blocking)
PR body inaccuracy — The PR description says seed.ts now uses the fixed ID
00000000-0000-0000-0000-000000000001, but the actual diff only updates a comment. The seed still lets the DB generate a random UUID. The code is correct (the sync endpoint handles dedup regardless), but the PR description is misleading.Minor race condition (
route.ts:205-231) — Two concurrent sync requests for the same user could both find the same existing inbox before either soft-deletes it, leading to both trying to reassign tasks and soft-delete. Under READ COMMITTED isolation this is unlikely to cause data loss (the second update would be a no-op since the tasks were already moved), but aSELECT ... FOR UPDATEon the existing inbox row inside the transaction would make it bulletproof.Code duplication (
route.ts:222-240vsroute.ts:233-241) — Thedb.insert(slLists).values(...)block is duplicated between the inbox and non-inbox branches. A small refactor could extract the values object to reduce duplication. Not blocking since the logic is clear as-is.Review by Claude Code
fix: resolve duplicate inbox on web after mobile sync (#60)to fix: resolve duplicate inbox on web after mobile sync (#60) + refresh (#61)fix: resolve duplicate inbox on web after mobile sync (#60) + refresh (#61)to fix: issues #60 #61 #62 — inbox dupe, refresh, subtask depthfix: issues #60 #61 #62 — inbox dupe, refresh, subtask depthto fix: issues #60 #61 #62 #63 — inbox, refresh, subtask depth, chevron/detailPR Review —
fix: issues #60 #61 #62 #63Verdict: APPROVE ✅
Les quatre corrections sont cohérentes, bien séparées, et couvrent les trois couches (mobile, web, API) quand nécessaire.
Analyse par issue
#60 — Duplicate inbox (sync/route.ts)
La transaction atomique qui vérifie l'existence d'une inbox, réassigne les tâches et soft-delete l'ancienne est correcte. Le guard
existingInbox.id !== entityIdévite de supprimer la même inbox.onConflictDoNothingà l'intérieur de la transaction est le bon choix — si l'ID existe déjà, pas de doublon.#61 — Refresh
RefreshControlavecawait loadTasks()— propre et attendu.router.refresh()avec unsetTimeout(500)pour le feedback visuel.#62 — Profondeur sous-tâches
Protection sur 3 niveaux :
tasks/route.ts) : rejette siparent.parentIdest non-null — ✅TaskItem.tsx) : masque "Ajouter sous-tâche" sidepth < 1— ✅task/[id].tsx) : masque la section sous-tâches sitask?.parentId— ✅#63 — Chevron vs détail
Séparation correcte : chevron toggle
expanded(sous-tâches), loupe/titre toggledetailOpen(notes, édition, suppression). Le chevron est masqué quand il n'y a pas de sous-tâches (spacerw-[18px]pour l'alignement).Suggestions (non-bloquantes)
TaskList.tsx:30—setTimeoutarbitraire : LesetTimeout(() => setRefreshing(false), 500)ne reflète pas la fin réelle du refresh.router.refresh()retournevoiddonc c'est difficile à attendre, mais un commentaire expliquant ce choix serait utile.sync/route.ts— Race condition théorique : Si deux appareils sync leur inbox simultanément, les deux transactions pourraient trouver la mêmeexistingInboxet tenter la même migration. LeonConflictDoNothingsur l'insert protège, mais le soft-delete de l'ancien inbox pourrait s'exécuter deux fois (idempotent, donc pas de bug réel). À surveiller en cas de sync multi-device intensif.TaskItem.tsx— Icône Search : La loupe (Search) pour ouvrir le détail est fonctionnelle, mais une icôneEyeouInfoserait peut-être plus intuitive sémantiquement. Suggestion mineure.Review par Claude Code — PR #64