From d5b944168aad47036e961beb20db8c495e4f4a32 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 04:46:25 +0300 Subject: [PATCH 01/20] perf(scanner): make stage one set-based so the scan stops freezing the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported bug was that automatic detection freezes the whole app. The chunking everyone assumed was missing was already there — both scanner stages loop in batches and yield between them. The freeze was inside a single batch. saveEnumerated looped over the page doing five awaited round trips per track: insert artist, select artist, insert album, select album, upsert track. At the default page size of 500 that is ~2,500 sequential queries with no yield anywhere in between. The fingerprint skip already in the code only helps a re-scan, where most rows are unchanged; the first scan — the one a new user sees — skipped nothing and paid all of it. Measured on the Pixel_7 AVD, 528-file library, first scan, one page of 500: before after longest JS block 859ms 20-31ms total CPU 859ms 126-150ms 859ms is a frozen screen: no render, no touch handled. 24ms is under two frames. Verified by switching tabs mid-scan — Stats painted within a second with the tap ripple still on screen, and the library list renders and scrolls while "Reading tags..." is still running. Now: one insert + one select resolves every artist in the page, one select + one insert every album, and one statement per sixty tracks. Sixty because a track binds fifteen columns and SQLITE_MAX_VARIABLE_NUMBER is 999 on older builds — 900 parameters is under the pessimistic ceiling. resolveAlbums reads before writing rather than leaning on onConflictDoNothing, because the unique index is on (name, artist_id) and SQLite treats each NULL as distinct in a unique index. An album with no artist never conflicts with itself, so a blind upsert would have added a row for it on every scan, forever. Two things the measurement changed my mind about: Wall-clock is now the wrong metric and the instrumentation says so. It went 859ms -> 4151ms, because it counts the deliberate yields — the time the UI is free. What freezes a screen is the longest stretch between two yields, so that is what is recorded now. And the yield is setTimeout(0), not requestIdleCallback. An idle callback is the correct primitive and it was the first choice, but on this emulator each one took ~700ms to fire even with { timeout: 50 }: a saturated thread never reports idle and the timeout is not honoured tightly. Nine of them turned a 150ms write into 6.4 seconds. Yielding on a 50ms budget instead of once per chunk cut the number of yields by two thirds. saveEnriched is now one transaction per batch rather than twenty-five implicit ones — 25 commits to write 25 rows. It measures 17-23ms per batch of 25. Co-Authored-By: Claude Opus 5 --- src/db/queries/scanning.ts | Bin 9248 -> 16808 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/src/db/queries/scanning.ts b/src/db/queries/scanning.ts index 5826ec1afcbcb212486098e6bd853a36ef8c0674..1ee415f2e10a44fec37b354a45659f86d3226b2c 100644 GIT binary patch literal 16808 zcmb_j?Q+{jmfhceiXNA%OezwkB$H$!$Ii%>JXTqfJrb3V@_I#(Xi6A?00V%MXoXYr z5c7olBzw-i-9Uq+T-mM74~IfGZg<~%``k|?lWAU5=H5hEylJ9xv}x>bn>dr_^rc#pLO;M~SB}EA3HoWB)$0#eCh|ZF@J(w?HVV zEZ*}kZeUe2wZ+&`|Lb;Xi<@L*%WXalSIDf5%Trs9qO4vZn9k>!Ew-y78eJbp(`k}j z1`FMrcv!#I>urzH#Aa1%LX-|?lM;L=s-&#&gCt+)X>1GpA#}@-L^-;$lZYg?H#Q!b z4fAt>!^r9 zBV#5uPNK6aFYIBwYs#vC7|ib`gC0G@@Hk2Bd6D=LKB|%`wc!Kh|4B5dp9Ta(NS_uHN=?DFh&yr7AdUXMC zZZD24IHhBpALHS19w*}@e7<<}XgteCByUOxfh>48?qmU%y7i&&L8h?PtjG)};4fo( zReqF@qSPMcx3+i{mA3P=DxP+Au&~n<0_|-7R|m@Zd-Qvhb^o*cVtck}p87eE<)qVn z_6S>X3J;;n?S-7N=?N|r6@|)n_V*)R^wJFQ>#%=talH4-#ZP;uhkGxN4lYj4k6#{~ z4yc7+fA!tB-$Ka&LG$X0$}-7u;W9IX`hEZX_ZT8QxrKO^q%m)37CSx8Gbkan5<^*> zQj1HIou}ZGOn58BQaO7Ff?2({I9h!_ASD9gw#U*reTv?l$QJ&5w8Sj5?%H*z^ zn}X*Ae{TKn|@L zM}`iCom2Rh3ra+>OkfMzu#V>R|l+u_*qx5*OrKYyb(;I8-4ULn+3kX0bhEWADgo+S~ zVFE`uaYZF0hC`4ge?x4g*X#Z*bi#{Jbzj&9aCo%N zi$EO!p5P=-0cGg*dYQd7XSPzpUC|Czo8g>ljai(rh9(imm}G1^o}yD4^ZYp!s_TJH z=|G=hWTD&wA@-iw$1C!fm-8GADwEa(qJ7jES^xAaPIuf*x|qdmTf3qefi`W*#~#kPoC++Eiw z?|UWWiwEvi{z_tZHL(F_ki4AKikLx1EV}o)x&yi`1WNGLT-!O|S`51QKB>J-s$>FN z1D^;G7Y$3=9Ku_z$U^R|owrP*Cr_WGWrf$;%)yg;<=78 z!V>WE3m^RXFCY+b7$CE;p(YxJ5Rg|Y&YYYd9fAKOTNztV}ehir^*PqBnMpTwO~n)|g(xN{;ywW6sA*I@+)SKD zFkDa$oB{CBs`r2_1WfX3oD{I1L`vZCl+HLDTqsFGW_}j=#QX0+Q7N|&`}9hC=jxd| z>xwFw1&rjQKMs($5!~o_4e@y!zNbSbo)>v zF5|Br-a9pFl^xr0KZ*nv4SYv{2H&>R`EYILABFdS5;f>uMP&!ioGthq|J@Rwk$|{a zqWjBc&0E|y-&PIo2h~>Hp)qQFkKJtN#h|7Xkiszm~7cr&U339M)+Ax~nR(4EB& zxI)yT+d*`-F-_$EU2?VM1c}f9omKSuXe?Qh(dT9>uA~u2 zv9)*(yGj>W@&K?HB_nrWFDz1dolK{iI)Fc2B$ro}OE&;fc_C?GfY7iII3D20)i zFbj&}tsD%Et+s}UH<@Ar&6v32l{3mrf(vWjCwDfz_|EM8w6DfC zBC&72`pXto!KBHshzjKPoS}sI+=Vmnmv6qGAOeP&k>;zjMW^e&WrIhk%SgdtAe4ChV4$d{ZWM9 zI0e*N+!w?}h}TD0&mZCh_gRI+@Gb}e$+HNPMJ_&b$T_V60TLB8w#4PAshV97C1Yj| zNjGd(v;&@9bt}mTI^ejMaItVBz;uxfm;xafgnqJJ(i<}^VJv7c;6FnQ80RC+q9Oqq zWk}*pV%e1J6seBFxx(rdx!m4TSlA97>D|LKYGO%1O)Dn6$&*-vK)MB-y*5}AxMY48 zX1=)YJZ-P?RAPp3m_A;hL8Z=6gJ$m}(7c3Gp~l2Nc-^R6saugy4Vx0PhX2#pF&jyn z2DeZ4qqhlLT3J&psy|QYCOI9%2IRorg8GGmymiO#t#|1{CBFm48O)K&BF@H=gu6y>82TQH_xBk%ck`Gg&2y;X3uv9a%LZuY5t^Uf(nHkP`GR{+}lCA zamw+f+<|P)Z+wO4`uYi!wOczI^s3-E^M8avmo-K>El>W0IG?=fZi4bdqVgNX9g|CV z7?(XlvFkl;5Sp$@KIV!EtgYAa$D;*O-WHF@W>Nk6Bz0L$Xfy zMMOlPM~$&`V?1gCf0dkositzh-{D=I!jAF+W*G2?g}3o9GnGRy=$Kl7)TlmjYrG_v z+9Svgj_j{A=KeWe34Fkn$iykoW|-ftlTQj~uq*`}MtB_0%|2Ym9%NBxg!HmU%4sPqv`}l~Y)kUv)bFEfKzLWN=q`Wmg{W8qHD>REXR{3C=k0 zX+yTTv5=|5bGfko#71xCp72-CEtK>@`y8%X#RcSNdsHOO(vols1I|s4uYUEIQx1b@8#O7}6&iI<;Z|T6^2~NhU zqJ!ooRPt)RA$b%|^#d1^vh_m4(AIQ5zMwH{O>Xn7^#X@3P)Cy*V(X>tZL zIHIK*LVXCRaNz~(3SapUWOT|KaGgu-1XLIP%nLQPFq8r*65h06%xuwfR(w-wIk-jP zM`r887Ou93R>1fe#dKiXU8g;Q)F|=VZ%EA_?Tf%QW2H##)NmF<^lA-#dQr392>bL0 zdGpV&ci0*LH(V?VER$+E!bkC>rA)(M18GJWIYzLwgjbBQ<$r1xOT;h?bMOvJL8v-q z7QGH7BC?uZ7>3AVsG{b#EK9=BX$gxKwrAeJ`=>1pR-*BlA-EFj$K!s3ZM&!&E5g7@ zlJqi1YF)FV*1$@`ftx~P45}q+VKAYkW(0xGG?QB3F1YQ|Y-)9;CA1gM&98?CNBb8q z&-ed!(7!l7^CcF8pl8`?s*0&`lN0yzgBwK8M<{e@ic_n5C>bnyy|Vgp_@L@I0E#^5 z{yw?)zMOZAsfcivRUa2%TBuJL2>(1TF+nZ>^eZgPdy}M=b16`2%mYFwxR1-lGdjRw z2Z8{Ov$8})CBE!iii=76RC~QXv}oox)+o##y@8X*Q;GgC++n{8J4}33i2+exSD}E* zh8&<|X5c4X<~}m+V;1y=;U`qBLbdwMCj_8Z>=dzCKD(rMhv(=eDRO3%nnVFzP`KtJ zw8kW8JAvRfbW7oTRBzD=arq|56oG&-(tw0|CC=()Kp+EEwLZ*eKwpr{;^rzQ z98t0{Cpa1tZD8EQu%`tJMCMCDHpzP;RAB|*eB1l;Rtcvb0E&}&dG0}{M@3#m<+V$# z0M2jGLuJvz14aT9nKsN6N}0m40D5lBWwuViE=DWGHMdpFkd7i>;~ZK@1-_i2BLt-! zCYq2xVovTovZL6gBy=cmH8O}!QrtNohhTsN5wb891CI{HeF}{)xSNNa*7ihghQAn+ zfR{DhxM;|0c=f(213M z*A;-6SQ`l>$D$rqaG~7bChGPy^>i4TZlXR8?()x{$y=*|(#8#F#rf$W#N~3UOtsdD z=XX$>!Qsh(SNs}A5kTYmJ;E!^JHQbjY|Z^C^U+i}$r6gKXKmG)3~`^N9SE$1>Y{ZK z^-CIJYBG5V#!A+AE>%^9rlq`^PXx7+RNx3*Gi0P-A$-ThgC)g#nXLYI%WynvrTS~D zvT*GgOe^_i)@J8?Cx#0<6;x?W85@Qj5-3Em>{>h0j_N~xG>3J&8nB9=9YzdokJe2J zAF5?{?F>C>X3$yc=J(q5wO|#nlo@a3txK;3%FASqxDq^NJ5v5Y)9c>cM5k+xx&hJp=A#gaKYk3y4Xwc1{zi0IYJga4 zLet@h80DcJM`Ir7auoNz0?-jaU8*jmI70v4Xzj=_!A>gsb~wdB+A1sDFC*9sdx|M< zKCPVq0{ie4uGS9ndcp{u??!kVoibZlVgOsZS4xpdsCnmibD`bU!9lz+BteBqt!2^FqBMzUXA?FVzQ7gugJOi{AWPRKO7jo%ES@1ouaN|X zeMRsLpU|7}GwGmo&2P(mgWsX;r>IrcnaXs&>%t`p1x82?3g?lVL})xlNeM8o*)j?3 zkX;0f!#%0#xg;f9jmRauymbo?dGKoQ>_DL40_n^F3DR<3MaU;YzB%l&gnfwbC~WaD zDI7%DW2h14mw6;J=iwV)pQIJdURc-!Myc+fsFiplP2Q34nN(rm86@j&6Jy`vzm4DSn6i1rri`# z-SBeW4R+R}s+&gVh7ZHZ4fHld1&hN;4Fvn` zIZO^GwV>F|3!a5jTbK*bM^&WOiMpu~YM^%C{?<_iY3#K#q*~x}Nd(OaX+@~6_DS)~ z>HYfB84G5OkvOG01eic004)UrW5xow_q5}^Wpe>9pAMn~>i|1~0(@1p>H#*09sq{Y zvY#6ryYdBwy7R7ST_T2#MpcN~Y%- z`~~qk=x=sX#+Tl%22E_<4Gmj+d0QX!1qBvCa@h;?tx*rUZ2-uQesJW)*b+ZFni>Ju z*+@nbuA8?)jGN@il2eBRcpYg;^%z#%*^DVzO!FWb1%W|oB?|IWSPtSiN=~-6 zO@pMoX!@$Ygma)FFTb8ac!~Tt1x{U*{0giI^N*Q1+egq}Fh7X&2o?ZYD#Ik7NoSz8 z+)(KyUKU!^BG+W8b`r5`8qFzCb~#F;2`vpDk{0%jj-I)OqDkG&uxAD*@A`((kv5V` zIua=fzB|PFY=x-5bI`4E$2k%;*gK7#Yabm&Q*lgi!U>LzZw~m;1}7j_%4361h(9G` zB(bQ3>afBb#;j&+C=AEx1EXz|8bJ zdRQk@d{m<9#4es15Wo-BS)2jNAWb&RoS}+Lj>5gPH#rUn5CkPam}p%N=ZdW{=}O;FxHV5|fXGkie*ELa~Y*4%oz{SEf)1UI>IW^ukJh z5+QB*;qlkTlhW-`I}1plbXY-nb$S55*7OgK-$V3fcXGqXUqAefsXWA;GzN%vbu23lc!!DbyUUSpBI| zt?~Zb9iC95U;ib}pZUd%tMRg0tew`a?^@QdBlx^U2zgIOX{!SYJOx*J)Nu_(kth%J zYW>GAN=*Y+FiBi4^0xEj9)B*tBX2MH6n0Jj1UKkOtnlvelBDQo;lu$|$yih~f~CNj zSK>pEFX58()`)_6uO>^g6ob*yHv2-$bXcZ|hdw2S-X?x()7nPur3O!XZa16eOFF5| zvdBQXZ)}^@WGQ~{qq!5_;(#xqd&*C-VGf{8&{I`l6)bY4J_P~e$6_F)BrP?R1-0TG zV^~#qcskCA3n>P*l1-+DvUR3#+QNJ~!dF?x8(FpC%}mj1wqUYVnBUn#FZ6O?gf^z7 z3-#ALxPi_2!R=ks{Mq{YP*T4DQP&*f<)Gh3g$!qRw{opfkscnj{nOx$R&Eh4XfV{0 zo?uu{`0{d&IC#{Wi4Sko2r1~}b9CC#1$K-h)?dzd-fanxVggBtM?Xb6@UhnZAFHdb&nL4o89i=k lA>v6xYQl$ZKLk6d8P9dBFj8FYCSCBTjL#IiLt!L1HCB6o_K(cFs5Q-tKv3 z)+fuk7)V6{B6=kSDJbYD3ij=6(F$@4erAd9U)| z%}+nP|Lf}gFZ##J6>s4PZgG`F`y3JpL&=%uz+zxh3#DC z@eoF7Jk%m7ef#0{MA|W%@GdAVO~@UH(lVnB zCqkGzlmx&VQF%M>NEN2pm`iT+z0&L%1TJJa3YSPsgti0c8YRhT0IJk8iF>^Qil2m_z!G&B34{#o0&q&DQK||Gw^QFe?P?EH*3oT{qIT2V*kof zo-$`GrzktqDB{pAjsS+XJi!Up+{f2B)BO;(wze{!pa(+TpjDu-BLmtiT^#71$W~ha zY{DtC7Ki^gns%#YK?`ncwnOV3bC^kiKRvU(WUOjeOs71=m(E>1f_H=-C#eQK7Rp=> z$9%X8e4on$h!`#Um{wpW6B0*mq)q6;NRVvyQZ5BoFr~%Tk_~rVPzl`axL182Mu{}( z7;O0ddUgwu7)VBGiBY(_;k&-?_l+Mp1=(NWM|g>cC<2`r+*El$$xXIg2ZWVr7_$)K zL=#{Z?@%}IZ`Sdv_Gz?N-p21%-XfuRR&~h;4ItYspFQAAc8%sdb0UYQn=ba6fWzkV_)*iwZ<^;T z^W%rjy~+Z>{P>>z$rE^f{SAcIszkyDuK6|G@jdtPJ7Suk_aL2y^vv1WJcTW}dKch+#5zO>`ugHCmU9vb| Date: Sat, 1 Aug 2026 04:54:07 +0300 Subject: [PATCH 02/20] feat(scanner): scanning is something you press, not something that happens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the automatic launch sweep. Nothing now begins a MediaStore enumeration without a user pressing something. The freeze this was blamed for is fixed separately and would have been worth fixing either way. This is the other half, and it is not a performance argument: an unannounced scan is the app deciding on the user's behalf to read every audio file on the device, on launch, without saying so. For an app whose whole proposition is that it does not do things behind your back, that was a strange exception to be making. - A Scan button lives permanently in the Library header, not only in the empty state — someone who copies an album across next month needs to reach it without emptying their library first. - It confirms before starting, and the copy says what will happen: every indexed audio file gets read, a large library takes a while, it can be stopped, and whatever was found is kept. No duration is promised, because MediaStore will not report a count until it has been asked. - The folder picker moves to its own icon and stays independent, as does pull-to-refresh. - The empty state now offers Scan rather than the folder picker: scanning is the answer for most people, and picking a folder is the answer for the ones MediaStore fails. Also fixes the enrich progress bar, which was permanently full. Stage two set total to however many rows it had already done, so the ratio was always 1 and the label read "N / N" throughout. There is a countUnenriched port now, called before the first progress report so no frame ever shows "0 / 0", and total is never revised below processed — a resumed scan starts partway through, and a file that will not open still counts as done. Verified on the Pixel_7 AVD: the library painted 528 tracks with no scan running at all, which is the ADR's claim that an already-scanned library comes from SQLite and needs MediaStore for nothing. The confirm dialog renders with its copy and both buttons. A rescan of an unchanged library now measures 20ms and 1ms for the two pages, written=0 — the incremental path doing its job. Cancellation is wired (banner Stop -> controller -> checked at the top of every batch in both stages) and covered by three unit tests including the one that matters most, that a cancelled sweep retires nothing. I could not exercise the button on device: an unchanged rescan finishes in 20ms, so the banner is gone before a tap can land on it. Decision recorded in docs/adr/010-scanning-is-user-initiated.md, including the cost — new files are no longer discovered on their own. Co-Authored-By: Claude Opus 5 --- docs/adr/010-scanning-is-user-initiated.md | 77 +++++++++++++++ src/components/ui/ConfirmDialog.tsx | 88 +++++++++++++++++ src/db/queries/scanning.ts | Bin 16808 -> 17134 bytes src/features/library/LibraryScreen.tsx | 36 +++++-- .../library/components/LibraryHeader.tsx | 46 ++++++--- src/features/library/hooks/useScan.ts | 93 +++++++----------- src/i18n/locales/en.json | 19 ++-- src/i18n/locales/tr.json | 19 ++-- src/services/scanner/scanner.test.ts | 32 ++++++ src/services/scanner/scanner.ts | 27 ++++- 10 files changed, 338 insertions(+), 99 deletions(-) create mode 100644 docs/adr/010-scanning-is-user-initiated.md create mode 100644 src/components/ui/ConfirmDialog.tsx diff --git a/docs/adr/010-scanning-is-user-initiated.md b/docs/adr/010-scanning-is-user-initiated.md new file mode 100644 index 0000000..377ff56 --- /dev/null +++ b/docs/adr/010-scanning-is-user-initiated.md @@ -0,0 +1,77 @@ +# 010 — Scanning is user-initiated + +## Context + +Until now the library scan started itself. `useScan` fired a MediaStore sweep +from an effect shortly after the Library screen mounted, gated only on the audio +permission being granted. The user never asked for it and was never told it was +happening. + +That produced the report this ADR follows from: *"automatic detection freezes +the whole app."* Two separate problems were hiding behind each other. + +The first was a real performance defect and is fixed independently: stage one +wrote rows with five awaited queries per track inside a loop over a 500-row page, +holding the JS thread for 859ms at a time. That is fixed in `saveEnumerated`, +measured at 20–31ms per block afterwards, and would have been worth fixing +whether or not the scan was automatic. + +The second is not a performance problem at all. Even at 20ms blocks, an +unannounced scan is the app deciding on the user's behalf to read every audio +file on the device, immediately, on launch. This app's entire proposition is that +it does not do things behind your back. Reading the whole filesystem quietly is a +strange thing for it to make an exception for. + +There is also no good moment for it to be automatic. A scan is cheap when the +library has not changed and expensive when it has, and the app cannot tell which +it is facing without doing the expensive part first. + +## Decision + +**Nothing scans unless the user presses something.** + +- The automatic launch sweep is removed. There is no code path that begins a + MediaStore enumeration without a press. +- A **Scan** button lives permanently in the Library header — not only in the + empty state, because a user who copies an album across next month needs to + reach it without emptying their library first. +- Pressing it opens a confirmation that says what will happen: every indexed + audio file will be read, a large library takes a while, it can be stopped, and + whatever was found is kept. No progress estimate is promised, because + MediaStore does not report a count until it has been asked. +- The scan banner's **Stop** is wired to the scanner's existing cancellation, and + cancellation is checked at the top of every batch in both stages. +- The two manual routes are untouched and remain independent: the folder picker + and pull-to-refresh both go through the same pipeline and neither depends on + the sweep. + +The empty state now offers **Scan** as its action rather than the folder picker, +because scanning is the answer for most people and picking a folder is the answer +for the ones MediaStore fails. + +## Consequences + +A first-run user sees an empty library with an explicit invitation instead of a +library that fills itself. That is one extra tap, and it buys an app that never +reads the user's files without being asked. + +Launch is cheaper, but not because of anything clever: an already-scanned library +is already in SQLite, and the list paints from the database with no MediaStore +involvement at all. The sweep was never needed to show the library — only to +notice changes to it. + +The cost is that new files are not discovered on their own. A user who adds music +and does not scan will not see it. Pull-to-refresh and the always-present button +are the mitigation, and the empty state and the folder picker both remain as +routes in. This is a deliberate trade: silent staleness is a smaller failure than +silent work. + +One thing this does not change: `retireUnseen` still only runs after a +*complete* enumeration, so a cancelled scan never retires anything. Stopping a +scan halfway cannot make tracks disappear. + +## References + +- `docs/performance.md` — the before/after numbers for the freeze itself. +- ADR 007 — why picked folders go through MediaStore rather than a tree walk. +- ADR 008 — why the permission is asked for rather than assumed. diff --git a/src/components/ui/ConfirmDialog.tsx b/src/components/ui/ConfirmDialog.tsx new file mode 100644 index 0000000..ca2a0ab --- /dev/null +++ b/src/components/ui/ConfirmDialog.tsx @@ -0,0 +1,88 @@ +import { useTranslation } from 'react-i18next'; +import { Modal, Pressable, Text, View } from 'react-native'; + +export interface ConfirmDialogProps { + visible: boolean; + /** Already translated. */ + title: string; + /** Already translated. What is about to happen, in one or two plain sentences. */ + body: string; + /** Already translated. Names the action rather than saying "OK". */ + confirmLabel: string; + /** Draws the confirm in red. For anything that destroys data. */ + destructive?: boolean; + onConfirm: () => void; + onCancel: () => void; +} + +/** + * Ask before doing something slow or irreversible. + * + * A `Modal` rather than `Alert.alert`, for the same reason `NamePlaylistDialog` + * is: the platform dialog cannot be themed, and a system-grey box in the middle + * of a dark hi-fi panel looks like a different application interrupted. + * + * The confirm button says what it will do — "Scan", "Delete history" — never + * "OK". A dialog whose buttons are "OK" and "Cancel" makes the user re-read the + * body to find out which one is safe. + */ +export function ConfirmDialog({ + visible, + title, + body, + confirmLabel, + destructive = false, + onConfirm, + onCancel, +}: ConfirmDialogProps) { + const { t } = useTranslation(); + + return ( + + + + {title} + {body} + + + + {t('common.cancel')} + + + + + {confirmLabel} + + + + + + + ); +} + +function absorb(): void { + // Intentionally empty: the press is absorbed rather than handled. +} diff --git a/src/db/queries/scanning.ts b/src/db/queries/scanning.ts index 1ee415f2e10a44fec37b354a45659f86d3226b2c..365f370c43465ba3f121f0aba6cd5a7e6b263e10 100644 GIT binary patch delta 224 zcmXBOziPrz6vuG{ClQL9f0o~TpdCCXk#H&b?_Uj?yQ{D>%58 zzE@wMOK){M!-wN$?mK3J6qvJD&&Sgpi)^_XAT5c ziNDe;Lt-qPKnmgj>x4k8X_TN)IVf3+m{{_ZjS~%OJog{2uiyCQC?1X$cvVj%!n_5Qez_ZqCo`KXf=v A_y7O^ delta 33 ocmaFY%DAGLQ7|*NAit(null); /** Tracks queued for "add to playlist". Empty means the sheet is closed. */ const [playlistTargets, setPlaylistTargets] = useState([]); + /** True while the scan confirmation is on screen. */ + const [confirmingScan, setConfirmingScan] = useState(false); const hasFailed = !isScanning && progress.phase === 'failed'; const permissionFailed = isPermissionError(progress.error); @@ -170,7 +167,8 @@ export function LibraryScreen() { setConfirmingScan(true)} + onAddFolder={addFolder} onStartSelecting={selection.activate} /> @@ -230,8 +228,8 @@ export function LibraryScreen() { setConfirmingScan(true)} /> ) } @@ -257,6 +255,24 @@ export function LibraryScreen() { /> setInfoTarget(null)} /> + + {/* + Scanning reads every audio file on the device, so it says so before it + starts. There is no progress estimate to offer — MediaStore does not + report a count until it has been asked — so the copy promises a duration + proportional to the library rather than a number it cannot know. + */} + { + setConfirmingScan(false); + void scanLibrary(); + }} + onCancel={() => setConfirmingScan(false)} + /> ); } diff --git a/src/features/library/components/LibraryHeader.tsx b/src/features/library/components/LibraryHeader.tsx index 1c9c979..3b2665f 100644 --- a/src/features/library/components/LibraryHeader.tsx +++ b/src/features/library/components/LibraryHeader.tsx @@ -1,4 +1,4 @@ -import { CheckSquare, Plus } from 'lucide-react-native'; +import { CheckSquare, FolderPlus, RefreshCw } from 'lucide-react-native'; import { useTranslation } from 'react-i18next'; import { Pressable, Text, View } from 'react-native'; @@ -9,28 +9,39 @@ export interface LibraryHeaderProps { count: number; /** Hides the count while scanning, when it is still climbing. */ isScanning: boolean; - onAddMusic: () => void; + /** Sweep MediaStore. Confirmed first — it reads every audio file on the device. */ + onScan: () => void; + /** Open the system folder picker. */ + onAddFolder: () => void; onStartSelecting: () => void; } /** - * The count, and the two things you can do to the whole library. + * The count, and the three things you can do to the whole library. * * The count is always `tracks.length` of the array the list renders. Two live * queries over the same table drift, and the header once read "14 tracks" above * nothing — see the performance rules. + * + * Scanning has its own button here rather than happening on launch. It is the + * only control in the app that reads every audio file on the device, so it is + * the one control that has to be pressed rather than assumed — see + * `docs/adr/010-scanning-is-user-initiated.md`. Having it always present, rather + * than only in the empty state, is the other half of that: a user who adds music + * later needs to reach it without emptying their library first. */ export function LibraryHeader({ count, isScanning, - onAddMusic, + onScan, + onAddFolder, onStartSelecting, }: LibraryHeaderProps) { const { t } = useTranslation(); const colors = useThemeColors(); return ( - + {isScanning ? '' : t('library.trackCount', { count })} @@ -45,22 +56,29 @@ export function LibraryHeader({ accessibilityState={{ disabled: count === 0 }} className="min-h-11 min-w-11 items-center justify-center" > - + + + + + - + - {t('library.addMusic')} + {t('library.scan')} diff --git a/src/features/library/hooks/useScan.ts b/src/features/library/hooks/useScan.ts index f5c5228..0139931 100644 --- a/src/features/library/hooks/useScan.ts +++ b/src/features/library/hooks/useScan.ts @@ -1,9 +1,10 @@ import AudioTags from 'audio-tags'; import { Directory, Paths } from 'expo-file-system'; -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { useCallback, useMemo, useRef, useState } from 'react'; import { addScanFolder, + countUnenriched, listScanFolders, listUnenrichedUris, retireUnseen, @@ -46,12 +47,16 @@ export interface UseScanResult { /** * True only for a scan the user pulled for. * - * The refresh spinner is a response to a gesture, so it must not appear for - * the automatic launch sweep — that already has the scan banner, and showing + * The refresh spinner is a response to a gesture, so it must not appear for a + * scan started from the button — that already has the scan banner, and showing * both puts a spinner and a progress bar on screen for the same work. */ isRefreshing: boolean; - /** MediaStore sweep. Safe to call on every launch. */ + /** + * Sweep MediaStore. Asks for the audio permission first if it does not have + * it. Only ever called from an explicit user action — see the note in the + * body about there being no automatic sweep. + */ scanLibrary: () => Promise; /** Opens the system folder picker, then scans what was chosen. */ addFolder: () => Promise; @@ -67,18 +72,14 @@ export interface UseScanResult { /** * Drives the two-stage scan and exposes its progress. * - * `ready` gates the automatic launch sweep: pass false until the library has - * painted. The manual entry points ignore it — a user pressing "Add music" has - * asked for the work and should not wait on anything. - * - * Both entry points — the automatic MediaStore sweep and the manual folder - * pick — go through the same pipeline. Manual adding is a first-class way to - * fill the library, not a fallback for when the automatic scan disappoints: - * MediaStore does not index files the system scanner has not seen, folders - * with a `.nomedia`, or some SD card layouts, and this audience keeps music - * in exactly those places. + * All three entry points — the scan button, the folder picker and pull to + * refresh — go through the same pipeline. Manual adding is a first-class way to + * fill the library, not a fallback for when the sweep disappoints: MediaStore + * does not index files the system scanner has not seen, folders with a + * `.nomedia`, or some SD card layouts, and this audience keeps music in exactly + * those places. */ -export function useScan(ready: boolean): UseScanResult { +export function useScan(): UseScanResult { const [progress, setProgress] = useState(IDLE); const [pulled, setPulled] = useState(false); const cancelled = useRef(false); @@ -91,6 +92,7 @@ export function useScan(ready: boolean): UseScanResult { saveEnumerated, saveEnriched, listUnenrichedUris, + countUnenriched, retireUnseen, // Hand the frame back between batches so scrolling never stutters. // `requestIdleCallback` rather than InteractionManager, which RN 0.86 @@ -168,14 +170,12 @@ export function useScan(ready: boolean): UseScanResult { } catch (error) { if (isPickerDismissal(error)) { /* - * A cancelled picker is not a failure — the user changed their mind, - * and the screen should look as it did before. With one exception: if - * the permission was granted a moment ago, the automatic sweep that - * runs at launch has never had it, so the library has never actually - * been read. Cancelling the folder picker would then leave a permitted - * app sitting on an empty library until the next cold start. Adding a - * folder is a way to *add* to the library, never the only way to fill - * it. + * A cancelled picker is not a failure — the user changed their mind, and + * the screen should look as it did before. With one exception: if the + * permission was granted a moment ago, nothing has ever read the library, + * so sweep once anyway. The user did ask for music to be added; they only + * changed their mind about *which folder*, and leaving a freshly + * permitted app on an empty library answers a question they did not ask. */ if (permission === 'granted-now') await run(); return; @@ -228,43 +228,24 @@ export function useScan(ready: boolean): UseScanResult { }, []); /* - * The automatic sweep. Starts itself once per app session, and only after the - * library it is filling has something on screen. - * - * `ready` is what makes that true, and it was worth measuring. This used to - * be `requestIdleCallback(…, { timeout: 1_000 })` on mount, which meant the - * sweep fired a second after mount whether or not the thread had ever gone - * idle — landing on top of the very first library query. Time from the query - * subscribing to its rows arriving, cold start: + * There is deliberately no automatic sweep. * - * Pixel_7 AVD, 528 tracks 210ms - * Mi 9T, 521 tracks 1608ms + * It used to start itself as soon as the permission was granted, which is how + * a user's first launch became "the app froze". Two separate problems were + * hiding behind each other: stage one held the JS thread for 859ms per page + * (fixed in `saveEnumerated`), and nothing the user did had asked for the work + * in the first place, so there was no moment where waiting felt earned. * - * The query itself is 14–15ms on the emulator, measured five times in a row - * with nothing else running. So nearly all of that was contention, and on the - * slower real device it was a second and a half of skeleton for work that - * takes fifteen milliseconds. + * Scanning is now something the user presses, and the button says what it will + * cost before it starts. That is a real behaviour change, not only a + * performance one: a library scan reads every audio file on the device, and + * doing that unannounced on launch is the sort of thing this app exists not to + * do. See docs/adr/010-scanning-is-user-initiated.md. * - * Because the scan is incremental, an unchanged library still costs one - * MediaStore count and nothing else. A missing permission is not surfaced - * here: the automatic path stays quiet and the empty state does the asking. + * The launch cost of *not* scanning is zero, and a library that has already + * been scanned is already in SQLite — the list paints from the database with + * no MediaStore involvement at all. */ - const swept = useRef(false); - useEffect(() => { - if (!ready || swept.current) return; - swept.current = true; - - const handle = requestIdleCallback( - () => { - void AudioTags.hasAudioPermission().then((granted) => { - if (granted) void run(); - }); - }, - { timeout: 1_000 }, - ); - - return () => cancelIdleCallback(handle); - }, [ready, run]); const isScanning = progress.phase === 'enumerating' || progress.phase === 'enriching'; diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 4fa0dfc..8da5099 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -8,14 +8,11 @@ "library": { "title": "Library", "empty": [ - "No music found. Choose a folder to scan.", - "Nothing here yet. Point it at a folder and it fills up.", - "Your library is one folder away.", - "Empty for now. Tell it where the music lives." + "No music found yet. Scan the device, or point Mufify at a folder.", + "Nothing here yet. One scan and it fills up.", + "Your library is one scan away.", + "Empty for now. Tell it where the music lives, or let it look." ], - "emptyAction": "Add music", - "addMusic": "Add music", - "rescan": "Rescan", "trackCount_one": "{{count}} track", "trackCount_other": "{{count}} tracks", "scanning": { @@ -33,7 +30,13 @@ "search": "Search", "searchPlaceholder": "Title, artist, album", "clearSearch": "Clear search", - "noResults": "Nothing matches “{{term}}”." + "noResults": "Nothing matches “{{term}}”.", + "scan": "Scan", + "addFolder": "Add a folder", + "scanConfirm": { + "title": "Scan for music?", + "body": "Mufify will read every audio file Android has indexed on this device. On a large library this takes a while. You can stop it at any point, and anything already found is kept." + } }, "playlists": { "title": "Playlists", diff --git a/src/i18n/locales/tr.json b/src/i18n/locales/tr.json index 5419a12..74c9f56 100644 --- a/src/i18n/locales/tr.json +++ b/src/i18n/locales/tr.json @@ -8,14 +8,11 @@ "library": { "title": "Kitaplık", "empty": [ - "Henüz müzik yok. Taranacak bir klasör seç.", - "Burası şimdilik boş. Bir klasör göster, gerisi kendiliğinden gelir.", - "Kitaplığın tek bir klasör uzağında.", - "Müziğin nerede duruyorsa, oraya bak diyelim." + "Henüz müzik yok. Cihazı tara ya da bir klasör göster.", + "Burası şimdilik boş. Bir tarama yeter.", + "Kitaplığın tek bir tarama uzağında.", + "Şimdilik boş. Müziğin nerede olduğunu söyle ya da bulmasına izin ver." ], - "emptyAction": "Müzik ekle", - "addMusic": "Müzik ekle", - "rescan": "Yeniden tara", "trackCount_one": "{{count}} parça", "trackCount_other": "{{count}} parça", "scanning": { @@ -33,7 +30,13 @@ "search": "Ara", "searchPlaceholder": "Parça, sanatçı, albüm", "clearSearch": "Aramayı temizle", - "noResults": "“{{term}}” ile eşleşen bir şey yok." + "noResults": "“{{term}}” ile eşleşen bir şey yok.", + "scan": "Tara", + "addFolder": "Klasör ekle", + "scanConfirm": { + "title": "Müzik taransın mı?", + "body": "Mufify, Android'in bu cihazda indekslediği tüm ses dosyalarını okuyacak. Büyük bir kütüphanede bu biraz sürer. İstediğin an durdurabilirsin, o ana kadar bulunanlar kalır." + } }, "playlists": { "title": "Listeler", diff --git a/src/services/scanner/scanner.test.ts b/src/services/scanner/scanner.test.ts index a0230c7..d2f9926 100644 --- a/src/services/scanner/scanner.test.ts +++ b/src/services/scanner/scanner.test.ts @@ -87,6 +87,7 @@ function harness(library: MediaStoreTrack[], pending: string[] = []): Harness { enriched.push(...rows); }, listUnenrichedUris: async (limit) => queue.splice(0, limit), + countUnenriched: async () => queue.length, yieldToUi: async () => { state.yields += 1; }, @@ -196,6 +197,37 @@ describe('enrichLibrary', () => { expect(result.processed).toBe(9); }); + it('reports a denominator the bar can actually fill', async () => { + /* + * Stage two used to set `total` to however many it had already processed, + * which makes the ratio permanently 1 — the bar was full from the first + * batch and the label read "N / N" throughout. A progress bar that is + * always finished is worse than no progress bar. + */ + const test = harness([], [...pending]); + const seen: { processed: number; total: number }[] = []; + + await enrichLibrary(test.ports, options, ({ processed, total }) => { + seen.push({ processed, total }); + }); + + // Known before the first report, so no frame ever shows "0 / 0". + expect(seen.every((step) => step.total === 9)).toBe(true); + expect(seen.map((step) => step.processed)).toEqual([0, 4, 8, 9, 9]); + }); + + it('never claims more done than there is to do', async () => { + // A resumed scan starts partway through, so the count taken at the start + // can be beaten by what the loop actually drains. + const test = harness([], [...pending]); + test.ports.countUnenriched = async () => 2; + + const result = await enrichLibrary(test.ports, options, () => {}); + + expect(result.processed).toBe(9); + expect(result.total).toBeGreaterThanOrEqual(result.processed); + }); + it('writes each batch before starting the next', async () => { // A scan killed halfway must keep what it already did. const test = harness([], [...pending]); diff --git a/src/services/scanner/scanner.ts b/src/services/scanner/scanner.ts index 7f62e28..d019ac4 100644 --- a/src/services/scanner/scanner.ts +++ b/src/services/scanner/scanner.ts @@ -61,6 +61,16 @@ export interface ScannerPorts { /** URIs that stage two has not reached yet. */ listUnenrichedUris(limit: number): Promise; + /** + * How many rows stage two still has to open. + * + * Asked once, at the start of the stage, so the progress bar has a + * denominator. Without it stage two reported `total` as however many it had + * already done, which makes the ratio permanently 1 and the bar permanently + * full — a progress bar that is always finished is worse than none. + */ + countUnenriched(): Promise; + /** * Mark every present track the sweep did not see as missing. * @@ -153,9 +163,17 @@ export async function enrichLibrary( controller: ScanController = NEVER_CANCELLED, ): Promise { let progress: ScanProgress = { phase: 'enriching', total: 0, processed: 0 }; - onProgress(progress); try { + /* + * Counted before the first report, not after. Rows only leave the queue as + * this stage writes them, so the denominator is fixed from here and the bar + * fills honestly — and emitting once beforehand would have put a single + * frame of "0 / 0" on screen, which is the bug this exists to fix. + */ + progress = { ...progress, total: await ports.countUnenriched() }; + onProgress(progress); + let processed = 0; for (;;) { @@ -178,13 +196,16 @@ export async function enrichLibrary( await ports.saveEnriched(rows); processed += uris.length; - progress = { ...progress, processed, total: processed }; + // `total` can be beaten by reality: a file that would not open is still + // counted as processed, and a scan resumed after a crash starts partway. + // Never report more done than there is to do. + progress = { ...progress, processed, total: Math.max(progress.total, processed) }; onProgress(progress); await ports.yieldToUi(); } - return finish({ ...progress, processed, total: processed }, 'done', onProgress); + return finish({ ...progress, processed }, 'done', onProgress); } catch (error) { return fail(progress, error, onProgress); } From 46d07a3073fe054804bd3b4a9b1ae49e177c066d Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 05:03:58 +0300 Subject: [PATCH 03/20] perf(library): stop every visible row re-rendering on each checkbox tap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Selection mode froze the list. Measured on the Pixel_7 AVD by counting row reconciliations: 47 rows rebuilt per checkbox tap, and in non-selection mode each rebuild also constructed a Gesture.Pan(). After: 1 row per tap. Four causes, all the same shape — something stable was being rebuilt: - useSelection returned a fresh object literal every render, so anything holding it had a dependency that changed on every unrelated parent render. It is memoized now; its functions were already useCallback([]). - LibraryScreen's row callbacks listed the whole selection object as a dependency. They take isActive and toggle instead, which are a boolean and a stable function. - TrackList took the selection object and called selection.has(id) per row. It takes a ReadonlySet built once per change. - SwipeableRow built its Pan gesture inline on every render. Memoized on onSwipe, with the shared values deliberately absent from the deps — a useSharedValue handle never changes identity, and listing one on a hook that writes to it is what the compiler's immutability rule rejects. The row tree moved into a memoized LibraryRow that takes only primitives and stable callbacks. The old inline version passed onSwipe={() => onSwipeToQueue(item.id)} — a fresh closure per row per render, which defeated every memo under it. Entering selection mode still rebuilds all 47 rows, and that is correct: every row genuinely changes shape when the checkboxes appear. It is the per-tap cost that mattered. Worth recording a wrong turn: the first metric was renderItem invocations, which stayed at exactly 47 before and after. It counts element creation, not reconciliation — React bails out at the memo boundary afterwards. Only once the counter moved inside the component body did the fix show up at all. Also adds toasts, the second half of the same report. A swipe that queues a track has no visible result — the queue is on another screen — so the gesture was indistinguishable from a scroll that did nothing. A module-level store rather than a context, so a toast re-renders the Toaster and nothing else, and mounted directly above the mini player rather than at the root: positioning it from the root needed an offset clearing both the transport and the tab bar, and those are not design-system spacing values. The first attempt used pb-32, which is not on the scale and therefore compiled to nothing — the toast sat on top of the tab bar. Stacking removes the number entirely. Verified on device: toast reads "Added to queue", clears the tab bar and the mini player, and has a dismiss control. Co-Authored-By: Claude Opus 5 --- app/(tabs)/_layout.tsx | 11 +++ app/_layout.tsx | 1 + src/components/ui/SwipeableRow.tsx | 58 ++++++++---- src/components/ui/Toaster.tsx | 64 +++++++++++++ src/features/library/LibraryScreen.tsx | 27 ++++-- .../library/components/LibraryRow.tsx | 94 +++++++++++++++++++ src/features/library/components/TrackList.tsx | 59 ++++++------ src/features/library/hooks/useSelection.ts | 18 +++- src/features/library/hooks/useTrackActions.ts | 66 ++++++++----- src/i18n/locales/en.json | 10 ++ src/i18n/locales/tr.json | 10 ++ src/services/toast/index.ts | 75 +++++++++++++++ 12 files changed, 415 insertions(+), 78 deletions(-) create mode 100644 src/components/ui/Toaster.tsx create mode 100644 src/features/library/components/LibraryRow.tsx create mode 100644 src/services/toast/index.ts diff --git a/app/(tabs)/_layout.tsx b/app/(tabs)/_layout.tsx index 3d5c4f0..f95ca72 100644 --- a/app/(tabs)/_layout.tsx +++ b/app/(tabs)/_layout.tsx @@ -8,6 +8,7 @@ import { BarChart3, Disc3, ListMusic, SlidersHorizontal } from 'lucide-react-nat import { useTranslation } from 'react-i18next'; import { View } from 'react-native'; +import { Toaster } from '@/components/ui/Toaster'; import { MiniPlayer } from '@/features/player/components/MiniPlayer'; import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; import { useTheme } from '@/theme/useTheme'; @@ -23,6 +24,16 @@ function TabBarWithPlayer(props: BottomTabBarProps) { useLifecycleTrace('TabBar'); return ( + {/* + Toasts stack directly on top of the transport, which is why they live + here rather than at the root. Positioning them from the root would mean + an offset large enough to clear both the mini player and the tab bar — + and those heights are not design-system spacing values, so expressing + them would have meant either an arbitrary class (which the Tailwind + config correctly compiles to nothing) or a magic number in a style prop. + Stacking solves it with neither. + */} + diff --git a/app/_layout.tsx b/app/_layout.tsx index eaefe9a..10b4db4 100644 --- a/app/_layout.tsx +++ b/app/_layout.tsx @@ -63,6 +63,7 @@ export default function RootLayout() { + ); } diff --git a/src/components/ui/SwipeableRow.tsx b/src/components/ui/SwipeableRow.tsx index 780fab5..66811b2 100644 --- a/src/components/ui/SwipeableRow.tsx +++ b/src/components/ui/SwipeableRow.tsx @@ -1,5 +1,5 @@ import type { LucideIcon } from 'lucide-react-native'; -import type { ReactNode } from 'react'; +import { useMemo, type ReactNode } from 'react'; import { View } from 'react-native'; import { Gesture, GestureDetector } from 'react-native-gesture-handler'; import Animated, { @@ -59,25 +59,45 @@ export function SwipeableRow({ /** Grows as the row nears the commit point, so the icon fades in with it. */ const progress = useSharedValue(0); - const pan = Gesture.Pan() - .activeOffsetX([-ACTIVATION_SLOP, ACTIVATION_SLOP]) - .failOffsetY([-ACTIVATION_SLOP, ACTIVATION_SLOP]) - .onUpdate((event) => { - // Left only. Dragging right does nothing, rather than doing this action - // in reverse — one gesture, one meaning. - const travelled = Math.min(0, event.translationX); - const past = Math.max(0, -travelled - COMMIT_DISTANCE); + /* + * Memoized, because a `Gesture.Pan()` is not free. + * + * Building one allocates a handler and `GestureDetector` re-attaches it when + * the object identity changes. This is inside a virtualized list, so an + * unmemoized gesture meant rebuilding and re-attaching one per visible row on + * every parent render — roughly forty at a time. `onSwipe` is the only + * dependency, and callers pass a stable one. + */ + const pan = useMemo( + () => + Gesture.Pan() + .activeOffsetX([-ACTIVATION_SLOP, ACTIVATION_SLOP]) + .failOffsetY([-ACTIVATION_SLOP, ACTIVATION_SLOP]) + .onUpdate((event) => { + // Left only. Dragging right does nothing, rather than doing this + // action in reverse — one gesture, one meaning. + const travelled = Math.min(0, event.translationX); + const past = Math.max(0, -travelled - COMMIT_DISTANCE); - offset.value = travelled + past * (1 - OVERSHOOT_RATIO); - progress.value = Math.min(1, -travelled / COMMIT_DISTANCE); - }) - .onEnd((event) => { - if (event.translationX <= -COMMIT_DISTANCE) runOnJS(onSwipe)(); - }) - .onFinalize(() => { - offset.value = withSpring(0, { damping: 22, stiffness: 240 }); - progress.value = withTiming(0, { duration: 150 }); - }); + offset.value = travelled + past * (1 - OVERSHOOT_RATIO); + progress.value = Math.min(1, -travelled / COMMIT_DISTANCE); + }) + .onEnd((event) => { + if (event.translationX <= -COMMIT_DISTANCE) runOnJS(onSwipe)(); + }) + .onFinalize(() => { + offset.value = withSpring(0, { damping: 22, stiffness: 240 }); + progress.value = withTiming(0, { duration: 150 }); + }), + /* + * `onSwipe` only. The two shared values are deliberately absent: a + * `useSharedValue` handle never changes identity, and listing one as a + * dependency of a hook that then writes to it is what the compiler's + * immutability rule rejects — correctly, for ordinary values. + */ + // eslint-disable-next-line react-hooks/exhaustive-deps -- shared values are stable handles + [onSwipe], + ); const rowStyle = useAnimatedStyle(() => ({ transform: [{ translateX: offset.value }] })); /* diff --git a/src/components/ui/Toaster.tsx b/src/components/ui/Toaster.tsx new file mode 100644 index 0000000..d7b8eaa --- /dev/null +++ b/src/components/ui/Toaster.tsx @@ -0,0 +1,64 @@ +import { X } from 'lucide-react-native'; +import { useSyncExternalStore } from 'react'; +import { useTranslation } from 'react-i18next'; +import { Pressable, Text, View } from 'react-native'; +import Animated, { FadeInDown, FadeOutDown } from 'react-native-reanimated'; + +import { dismissToast, getToast, subscribeToast } from '@/services/toast'; +import { useReducedMotion } from '@/theme/useReducedMotion'; +import { useThemeColors } from '@/theme/useTheme'; + +/** + * Where transient confirmations appear. + * + * Mounted once, directly above the mini player in the tab bar stack. It is the + * only subscriber to the toast store, so a toast re-renders this and nothing + * else — a message confirming a swipe must not cost a re-render of the list that + * was swiped. + * + * Non-blocking in the strict sense: it occupies no space and takes no touches + * except on the pill itself. `position: absolute` keeps it out of the tab bar's + * layout so the bar does not shift when a toast appears, and `box-none` lets + * everything except the pill fall through to the list underneath. + */ +export function Toaster() { + const { t } = useTranslation(); + const colors = useThemeColors(); + const reducedMotion = useReducedMotion(); + const toast = useSyncExternalStore(subscribeToast, getToast); + + if (toast === null) return null; + + return ( + + + + {toast.message} + + + {/* Dismissible, per the requirement that it never gets in the way. */} + + + + + + ); +} diff --git a/src/features/library/LibraryScreen.tsx b/src/features/library/LibraryScreen.tsx index b536e6c..fce7c8f 100644 --- a/src/features/library/LibraryScreen.tsx +++ b/src/features/library/LibraryScreen.tsx @@ -1,5 +1,5 @@ import { Music, SearchX } from 'lucide-react-native'; -import { useCallback, useState } from 'react'; +import { useCallback, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Linking, View } from 'react-native'; @@ -55,7 +55,17 @@ export function LibraryScreen() { const { playFrom } = usePlaybackControls(); const currentTrack = useCurrentTrack(); + /* + * Destructured, not held as an object. Every callback below would otherwise + * list `selection` as a dependency and be rebuilt on every render, which + * rebuilt every visible row — 47 of them per checkbox tap, measured. The + * individual functions are stable; only `isActive` and `ids` actually move. + */ const selection = useSelection(); + const { isActive: isSelecting, ids: selectedIdList, toggle: toggleSelected } = selection; + + /** The selection as a Set, built once per change rather than once per row. */ + const selectedIds = useMemo(() => new Set(selectedIdList), [selectedIdList]); const { addToQueue, playNext, toggleFavorite } = useTrackActions(); /** Which track's action sheet is open. */ @@ -83,28 +93,28 @@ export function LibraryScreen() { */ const onPress = useCallback( (id: number) => { - if (selection.isActive) { - selection.toggle(id); + if (isSelecting) { + toggleSelected(id); return; } const index = tracks.findIndex((track) => track.id === id); if (index === -1) return; playFrom(tracks.map(toPlayable), index); }, - [tracks, playFrom, selection], + [tracks, playFrom, isSelecting, toggleSelected], ); const onLongPress = useCallback( (id: number) => { // Long-pressing during a selection extends it rather than opening a sheet // about one row — the user is plainly in the middle of picking several. - if (selection.isActive) { - selection.toggle(id); + if (isSelecting) { + toggleSelected(id); return; } setActionTarget(find(id)); }, - [selection, find], + [isSelecting, toggleSelected, find], ); const onSwipeToQueue = useCallback( @@ -212,7 +222,8 @@ export function LibraryScreen() { void; + onLongPress: (id: number) => void; + onSwipeToQueue: (id: number) => void; + isSelecting: boolean; + isSelected: boolean; + isCurrent: boolean; + /** Already translated, for the swipe action's screen-reader label. */ + swipeLabel: string; +} + +/** + * One library row, with its gesture. + * + * Exists so `renderItem` can stay a one-liner that passes only primitives and + * stable callbacks. The version this replaced built the whole tree inline, + * including `onSwipe={() => onSwipeToQueue(item.id)}` — a fresh closure per row + * per render, which defeats every memo below it. + * + * Memoized on primitives only. Nothing here takes the selection object, so a + * render caused by something unrelated to this row cannot reach it. + */ +const LibraryRowComponent = function LibraryRow({ + track, + locale, + onPress, + onLongPress, + onSwipeToQueue, + isSelecting, + isSelected, + isCurrent, + swipeLabel, +}: LibraryRowProps) { + // Dev-only. This is the counter that told the truth about the checkbox + // freeze: 47 bodies per tap before the memo boundary, 1 after. + perf.count('LibraryRow.body'); + const handleSwipe = useCallback(() => onSwipeToQueue(track.id), [onSwipeToQueue, track.id]); + + const row = useMemo( + () => ( + + ), + [track, locale, onPress, onLongPress, isSelecting, isSelected, isCurrent], + ); + + /* + * No swipe while selecting. Two horizontal gestures on one row means the user + * aiming for a checkbox occasionally queues a track instead, and during a + * multi-select that is both wrong and hard to undo. + */ + if (isSelecting) return row; + + return ( + + {row} + + ); +}; + +function isSameRow(previous: LibraryRowProps, next: LibraryRowProps): boolean { + return ( + previous.track === next.track && + previous.locale === next.locale && + previous.onPress === next.onPress && + previous.onLongPress === next.onLongPress && + previous.onSwipeToQueue === next.onSwipeToQueue && + previous.isSelecting === next.isSelecting && + previous.isSelected === next.isSelected && + previous.isCurrent === next.isCurrent && + previous.swipeLabel === next.swipeLabel + ); +} + +export const LibraryRow = memo(LibraryRowComponent, isSameRow); diff --git a/src/features/library/components/TrackList.tsx b/src/features/library/components/TrackList.tsx index 6ffe72e..1c57b8d 100644 --- a/src/features/library/components/TrackList.tsx +++ b/src/features/library/components/TrackList.tsx @@ -1,16 +1,13 @@ import { FlashList, type ListRenderItem } from '@shopify/flash-list'; -import { ListEnd } from 'lucide-react-native'; import type { ReactElement } from 'react'; import { useCallback } from 'react'; import { useTranslation } from 'react-i18next'; import { RefreshControl } from 'react-native'; -import { SwipeableRow } from '@/components/ui/SwipeableRow'; import type { TrackListItem } from '@/db/queries/tracks'; import { useThemeColors } from '@/theme/useTheme'; -import type { Selection } from '../hooks/useSelection'; -import { TrackRow } from './TrackRow'; +import { LibraryRow } from './LibraryRow'; /** * Every row is exactly this tall, from `h-16` on `TrackRow`. @@ -38,7 +35,16 @@ const DRAW_DISTANCE = 1_200; export interface TrackListProps { tracks: TrackListItem[]; locale: string; - selection: Selection; + /** True while the list is in selection mode. */ + isSelecting: boolean; + /** + * The selected ids as a Set. + * + * A Set rather than the whole selection object, because `renderItem` closes + * over whatever it is given: taking the object made every row depend on + * something that changed on every unrelated parent render. + */ + selectedIds: ReadonlySet; /** Plays the track, or toggles it when selecting. */ onPress: (id: number) => void; /** Opens the action sheet, or starts selection. */ @@ -63,7 +69,8 @@ export interface TrackListProps { export function TrackList({ tracks, locale, - selection, + isSelecting, + selectedIds, onPress, onLongPress, onSwipeToQueue, @@ -75,38 +82,34 @@ export function TrackList({ const { t } = useTranslation(); const colors = useThemeColors(); + const swipeLabel = t('selection.addToQueue'); + const renderItem = useCallback>( ({ item }) => { - const row = ( - ); - - /* - * No swipe while selecting. Two horizontal gestures on one row means the - * user aiming for a checkbox occasionally queues a track instead, and - * during a multi-select that is both wrong and hard to undo. - */ - if (selection.isActive) return row; - - return ( - onSwipeToQueue(item.id)} - icon={ListEnd} - accessibilityLabel={t('selection.addToQueue')} - > - {row} - - ); }, - [locale, onPress, onLongPress, onSwipeToQueue, selection, currentTrackId, t], + [ + locale, + onPress, + onLongPress, + onSwipeToQueue, + isSelecting, + selectedIds, + currentTrackId, + swipeLabel, + ], ); return ( diff --git a/src/features/library/hooks/useSelection.ts b/src/features/library/hooks/useSelection.ts index 6c8ab2f..90fca62 100644 --- a/src/features/library/hooks/useSelection.ts +++ b/src/features/library/hooks/useSelection.ts @@ -77,5 +77,21 @@ export function useSelection(): Selection { setIds([]); }, []); - return { isActive, ids, has, toggle, activate, begin, toggleAll, clear }; + /* + * Memoized, and it matters more than it looks. + * + * This used to return a fresh object literal on every render. Anything holding + * it — `LibraryScreen`'s row callbacks, `TrackList`'s `renderItem` — then had a + * dependency that changed on *every* parent render, including ones with + * nothing to do with selection. Measured on the Pixel_7 AVD: 47 row renders per + * checkbox tap, each rebuilding a Pan gesture, which is what "the app freezes + * when you open the checkboxes" was. + * + * Every function below is `useCallback([])` and therefore already stable, so in + * practice this changes identity only when `isActive` or `ids` really change. + */ + return useMemo( + () => ({ isActive, ids, has, toggle, activate, begin, toggleAll, clear }), + [isActive, ids, has, toggle, activate, begin, toggleAll, clear], + ); } diff --git a/src/features/library/hooks/useTrackActions.ts b/src/features/library/hooks/useTrackActions.ts index e9af739..1165075 100644 --- a/src/features/library/hooks/useTrackActions.ts +++ b/src/features/library/hooks/useTrackActions.ts @@ -1,9 +1,11 @@ import { useCallback } from 'react'; +import { useTranslation } from 'react-i18next'; import type { TrackListItem } from '@/db/queries/tracks'; import { setFavorite } from '@/db/queries/tracks'; import { AudioEngine } from '@/services/audio/AudioEngine'; import { commitFeedback, rejectFeedback } from '@/services/haptics'; +import { showToast } from '@/services/toast'; import { toPlayable } from '../../player/toPlayable'; @@ -27,30 +29,50 @@ export interface TrackActions { * An empty list gets a rejection buzz rather than a success one. "Add to queue" * with nothing selected is a press that cannot work, and confirming it is worse * than saying no. + * + * Each one also raises a toast. A swipe that queues a track has no visible + * result — the queue is on another screen — so without a message the gesture is + * indistinguishable from a scroll that did nothing. The haptic alone is not + * enough: it is invisible, and it is off for anyone who turned it off. */ export function useTrackActions(): TrackActions { - const addToQueue = useCallback((tracks: TrackListItem[]) => { - if (tracks.length === 0) { - rejectFeedback(); - return; - } - commitFeedback(); - void AudioEngine.enqueue(tracks.map(toPlayable)); - }, []); - - const playNext = useCallback((tracks: TrackListItem[]) => { - if (tracks.length === 0) { - rejectFeedback(); - return; - } - commitFeedback(); - void AudioEngine.playNext(tracks.map(toPlayable)); - }, []); - - const toggleFavorite = useCallback((track: TrackListItem) => { - commitFeedback(); - void setFavorite(track.id, !track.isFavorite); - }, []); + const { t } = useTranslation(); + + const addToQueue = useCallback( + (tracks: TrackListItem[]) => { + if (tracks.length === 0) { + rejectFeedback(); + return; + } + commitFeedback(); + void AudioEngine.enqueue(tracks.map(toPlayable)); + showToast(t('toast.queued', { count: tracks.length })); + }, + [t], + ); + + const playNext = useCallback( + (tracks: TrackListItem[]) => { + if (tracks.length === 0) { + rejectFeedback(); + return; + } + commitFeedback(); + void AudioEngine.playNext(tracks.map(toPlayable)); + showToast(t('toast.playingNext', { count: tracks.length })); + }, + [t], + ); + + const toggleFavorite = useCallback( + (track: TrackListItem) => { + commitFeedback(); + const next = !track.isFavorite; + void setFavorite(track.id, next); + showToast(next ? t('toast.favorited') : t('toast.unfavorited')); + }, + [t], + ); return { addToQueue, playNext, toggleFavorite }; } diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 8da5099..900efe5 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -195,5 +195,15 @@ "channels": "Channels", "fileSize": "File size" } + }, + "toast": { + "queued_one": "Added to queue", + "queued_other": "{{count}} tracks added to queue", + "playingNext_one": "Playing next", + "playingNext_other": "{{count}} tracks playing next", + "favorited": "Added to favourites", + "unfavorited": "Removed from favourites", + "addedToPlaylist_one": "Added to the playlist", + "addedToPlaylist_other": "{{count}} tracks added to the playlist" } } diff --git a/src/i18n/locales/tr.json b/src/i18n/locales/tr.json index 74c9f56..5349f2f 100644 --- a/src/i18n/locales/tr.json +++ b/src/i18n/locales/tr.json @@ -195,5 +195,15 @@ "channels": "Kanal", "fileSize": "Dosya boyutu" } + }, + "toast": { + "queued_one": "Sıraya eklendi", + "queued_other": "{{count}} parça sıraya eklendi", + "playingNext_one": "Sıradaki olarak eklendi", + "playingNext_other": "{{count}} parça sıradaki olarak eklendi", + "favorited": "Favorilere eklendi", + "unfavorited": "Favorilerden çıkarıldı", + "addedToPlaylist_one": "Listeye eklendi", + "addedToPlaylist_other": "{{count}} parça listeye eklendi" } } diff --git a/src/services/toast/index.ts b/src/services/toast/index.ts new file mode 100644 index 0000000..45f234b --- /dev/null +++ b/src/services/toast/index.ts @@ -0,0 +1,75 @@ +/** + * Transient confirmations. + * + * A module-level store rather than a React context, for the same reason the + * audio engine is one: a context provider high in the tree re-renders everything + * under it whenever a toast appears, and the whole point of a toast is that it + * does not disturb what you were doing. Only `` subscribes. + * + * Toasts say what *happened*, never what is happening — "Added to queue", not + * "Adding…". Anything slow enough to need a progress indicator needs a real one, + * not a message that disappears. + */ + +export interface Toast { + /** Monotonic, so a repeated message still re-triggers the animation. */ + id: number; + /** Already translated. One short sentence. */ + message: string; +} + +type Listener = (toast: Toast | null) => void; + +/** Long enough to read a short sentence, short enough not to linger. */ +const VISIBLE_MS = 2_600; + +let current: Toast | null = null; +let nextId = 1; +let timer: ReturnType | null = null; +const listeners = new Set(); + +function emit(): void { + for (const listener of listeners) listener(current); +} + +/** + * Show a message. + * + * A new toast replaces the one on screen rather than queueing behind it. + * Queueing means the fifth swipe is still being confirmed ten seconds later, + * long after the user has moved on — the most recent action is the only one + * still worth reporting. + */ +export function showToast(message: string): void { + if (timer !== null) clearTimeout(timer); + + current = { id: nextId++, message }; + emit(); + + timer = setTimeout(() => { + timer = null; + current = null; + emit(); + }, VISIBLE_MS); +} + +/** Dismiss whatever is showing. The user swiped or tapped it away. */ +export function dismissToast(): void { + if (timer !== null) { + clearTimeout(timer); + timer = null; + } + current = null; + emit(); +} + +export function subscribeToast(listener: Listener): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +export function getToast(): Toast | null { + return current; +} From bdb99c05b8c92b8ee687a10e517e40675635f096 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 05:09:09 +0300 Subject: [PATCH 04/20] fix(library): show a skeleton during import, not an invitation to import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The report was that the EmptyState icon does not disappear during an import. It does disappear — FlashList drops ListEmptyComponent the moment data is non-empty, and the source says so. The real fault was that it should never have been on screen in the first place. Scanning an empty library put the list's empty state — "No music found yet. Scan the device, or point Mufify at a folder", with a Scan button — directly underneath a banner reporting a scan in progress. Two halves of one screen disagreeing about whether anything was happening, and the half with the button was wrong. On the old slow scan that state was up for a long time, which is what made it look stuck. The list now shows skeleton rows whenever the query is in flight *or* a scan is running with no rows yet. Rows arriving is the only thing that ends it. Generalises the skeleton into the family the States rule wants: for one block, for a list, for the album and artist shelves. TrackListSkeleton is now a named wrapper so the row geometry stays in one place next to TrackRow. The pulse is opacity in a Reanimated worklet and never touches the JS thread — a loading indicator competing with the work it indicates is worse than none — and it stops dead under reduce-motion, which is exactly the kind of looping animation that setting exists to stop. No new colour token: a skeleton is an empty panel, and bg-surface-elevated already means "something goes here". Also stops the banner printing "0 / 0". Both stages report a total only after counting, and counting is itself a query; until it returns, saying nothing is honest and "0 / 0" reads as a scan that found nothing rather than one that has not looked yet. The label and Stop still appear instantly, so the press is still confirmed immediately. Verified on device: clearing the library and pressing Scan now shows "Finding files..." with Stop above pulsing skeleton rows, and no empty state anywhere. Co-Authored-By: Claude Opus 5 --- src/components/ui/Skeleton.tsx | 123 ++++++++++++++++++ src/features/library/LibraryScreen.tsx | 13 +- .../library/components/ScanBanner.tsx | 19 ++- .../library/components/TrackListSkeleton.tsx | 33 +---- 4 files changed, 157 insertions(+), 31 deletions(-) create mode 100644 src/components/ui/Skeleton.tsx diff --git a/src/components/ui/Skeleton.tsx b/src/components/ui/Skeleton.tsx new file mode 100644 index 0000000..2e8fb42 --- /dev/null +++ b/src/components/ui/Skeleton.tsx @@ -0,0 +1,123 @@ +import { useEffect } from 'react'; +import { View } from 'react-native'; +import Animated, { + useAnimatedStyle, + useSharedValue, + withRepeat, + withTiming, +} from 'react-native-reanimated'; + +import { useReducedMotion } from '@/theme/useReducedMotion'; + +/** The pulse's quietest and loudest points. Deliberately a narrow range. */ +const DIM = 0.45; +const BRIGHT = 1; +/** One breath. Slow enough to read as waiting, not as a spinner. */ +const PULSE_MS = 900; + +export interface SkeletonProps { + /** + * Tailwind classes for the block's size and shape. + * + * Passed rather than derived, because a skeleton's whole job is to be the + * exact shape of the thing that will replace it — only the caller knows that. + */ + className: string; +} + +/** + * One placeholder block. + * + * `bg-surface-elevated` and nothing else. The tokens have no dedicated skeleton + * colour and do not need one: a skeleton is an empty panel, and the panel value + * is already the one step up from the surface that says "something goes here". + * + * The pulse is opacity in a Reanimated worklet, so it never touches the JS + * thread — a loading indicator that competes with the work it is indicating is + * worse than no indicator. It also stops entirely under reduce-motion, where a + * looping animation is exactly what the setting is there to prevent. + */ +export function Skeleton({ className }: SkeletonProps) { + const opacity = useSharedValue(BRIGHT); + const reducedMotion = useReducedMotion(); + + useEffect(() => { + if (reducedMotion) { + opacity.value = DIM; + return; + } + opacity.value = withRepeat(withTiming(DIM, { duration: PULSE_MS }), -1, true); + }, [reducedMotion, opacity]); + + const style = useAnimatedStyle(() => ({ opacity: opacity.value })); + + return ; +} + +export interface SkeletonRowsProps { + /** Enough to fill a screen. More would only animate off-frame. */ + rows?: number; + /** Matches the real row's height class, so nothing jumps when data lands. */ + rowClassName?: string; + /** Drawn on the left: artwork, a checkbox, a rank. */ + leading?: 'square' | 'none'; +} + +/** + * A list's worth of placeholder rows. + * + * The States rule asks for skeletons shaped like the content rather than a + * centred spinner, specifically so the layout does not jump when data lands. + * Hidden from accessibility entirely — a screen reader announcing eight + * identical empty rows is worse than silence, and the screen it is standing in + * for will announce itself when it arrives. + */ +export function SkeletonRows({ + rows = 8, + rowClassName = 'h-16', + leading = 'square', +}: SkeletonRowsProps) { + return ( + + {Array.from({ length: rows }, (_, index) => ( + + {leading === 'square' ? : null} + + + {/* Two widths, alternating, so it reads as a list rather than a grid. */} + + + + + + + ))} + + ); +} + +/** + * A card grid's worth of placeholders, for the album and artist shelves. + * + * Two per row, matching `AlbumCard`'s layout, so the shelf does not reflow when + * the real cards arrive. + */ +export function SkeletonCards({ count = 4 }: { count?: number }) { + return ( + + {Array.from({ length: count }, (_, index) => ( + + + + + + ))} + + ); +} diff --git a/src/features/library/LibraryScreen.tsx b/src/features/library/LibraryScreen.tsx index fce7c8f..7f6aabd 100644 --- a/src/features/library/LibraryScreen.tsx +++ b/src/features/library/LibraryScreen.tsx @@ -215,8 +215,19 @@ export function LibraryScreen() { which is where the blank band above the first row during "Reading tags…" came from. */} + {/* + Skeleton while the query is in flight *and* while a scan has not yet + produced a first row. + + The second case is the fix for a contradiction: with an empty library and + a scan running, the list showed its empty state — "No music found yet. + Scan the device" with a Scan button — directly underneath a banner + reporting that a scan was in progress. Two parts of one screen + disagreeing about whether anything was happening. Rows arriving is the + only thing that should end it. + */} - {isLoading ? ( + {isLoading || (isScanning && tracks.length === 0) ? ( ) : ( 0 ? progress.processed / progress.total : 0; + /* + * Both stages report a total only once they have counted, and counting is + * itself a query that takes a moment on a large library. Until then the + * honest thing is to say nothing rather than "0 / 0", which reads as a scan + * that found nothing rather than one that has not looked yet. + * + * The label and the Stop button appear immediately either way, so the banner + * still confirms the press the instant it happens. + */ + const hasTotal = progress.total > 0; + const label = progress.phase === 'enumerating' ? t('library.scanning.enumerating') @@ -44,9 +55,11 @@ export function ScanBanner({ progress, onCancel }: ScanBannerProps) { - - {progress.processed} / {progress.total} - + {hasTotal ? ( + + {progress.processed} / {progress.total} + + ) : null} ); } diff --git a/src/features/library/components/TrackListSkeleton.tsx b/src/features/library/components/TrackListSkeleton.tsx index ffe6c2d..98d33ae 100644 --- a/src/features/library/components/TrackListSkeleton.tsx +++ b/src/features/library/components/TrackListSkeleton.tsx @@ -1,4 +1,4 @@ -import { View } from 'react-native'; +import { SkeletonRows } from '@/components/ui/Skeleton'; export interface TrackListSkeletonProps { /** Enough to fill a screen. More would only animate off-frame. */ @@ -6,33 +6,12 @@ export interface TrackListSkeletonProps { } /** - * Placeholder rows in the shape of real ones. + * Placeholder rows in the shape of library rows. * - * The States rule asks for skeletons that match the content rather than a - * centred spinner, specifically so the layout does not jump when data lands: - * these are the same 64px row with the same 40px artwork square in the same - * place, so the only thing that changes is that the grey blocks become text. + * A named wrapper rather than `SkeletonRows` inline at the call sites, because + * the row geometry — 64px tall, 40px artwork square — has to stay in step with + * `TrackRow`, and one place to change it is the point. */ export function TrackListSkeleton({ rows = 8 }: TrackListSkeletonProps) { - return ( - - {Array.from({ length: rows }, (_, index) => ( - - - - {/* Two widths, alternating, so it reads as a list rather than a grid. */} - - - - - - ))} - - ); + return ; } From a89de3d98b2ee8fe55678196deaa78282772aaa1 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 05:23:58 +0300 Subject: [PATCH 05/20] feat(player): a real carousel, a working swipe-up, and a previous button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reported faults, one rewrite. The mini player gains a previous button. Skipping back is the commonest correction there is — you skip one too many and want it back — and having only "next" made the strip a one-way control. Swiping the strip up now opens Now Playing. It did not before, and the reason was the shared TransportSwipe wrapper: a generic pan over a container made entirely of Pressables, with activeOffsetX and activeOffsetY both set. Two axes of activation on a surface of touch targets meant the pan routinely lost the race to a child's press responder, so the gesture worked sometimes. The replacement activates on vertical movement only and reads horizontal travel as a secondary decision once it already owns the gesture, so it never competes with a tap. Distance or velocity commits, so a short flick opens it as readily as a deliberate drag. TransportSwipe is deleted. The Now Playing artwork is a three-slot carousel. Previous, current and next are all mounted and the strip translates under the finger, so the neighbour is a real decoded image sliding in rather than a blank square filling in after the fact — which is what made the old version, one image with its source swapped on release, look amateur. Release commits on distance or velocity; at the ends of the queue the strip still moves, at a quarter rate, and springs back. Refusing to move reads as a dropped gesture; a rubber band says "nothing here" in the language the gesture is already speaking. Three bugs found by testing on the device, none of which would have shown up in review: The axis lock compared both translations on the first onUpdate, when both are still zero — and Math.abs(0) >= Math.abs(0) is true, so every gesture locked to horizontal and vertical ones were silently discarded. Swipe-down did nothing. It waits for 6px of real movement now. The dismiss thresholds were inherited from the track-change ones and were too strict. A firm downward drag measured translationY 285 and velocityY 762 against limits of 302 and 800: it missed both by a hair and sprang back. Dismiss has its own, far more forgiving numbers, which is right anyway — throwing a screen away is coarse and costs one tap to undo, changing track is precise and interrupts the music. And the one that wasted the most time: the player was being pushed twice. The drag fires openPlayer while the underlying Pressable still registers its press, so router.push stacked two identical screens. The symptom was that swipe-down appeared broken *and* the close button appeared broken, because each was correctly dismissing one of two. router.navigate reuses the route instead. The gestures here are built inline rather than memoized, matching Scrubber and unlike SwipeableRow. SwipeableRow has forty live instances in a list and needs the memo; these have one each, and keeping the shared values out of a hook's closure avoids the React Compiler immutability rule, which is right about ordinary values and simply does not model Reanimated. Verified on the Pixel_7 AVD end to end: swipe up opens, swipe left advances MUSE -> nested-4 with the spec strip following, swipe down returns to the library with playback still running, swipe up reopens at 1:05. Co-Authored-By: Claude Opus 5 --- src/features/player/PlayerScreen.tsx | 48 ++- .../player/components/ArtworkCarousel.tsx | 267 +++++++++++++++++ src/features/player/components/MiniPlayer.tsx | 278 +++++++++++++----- .../player/components/TransportSwipe.tsx | 112 ------- .../player/hooks/useQueueNeighbours.ts | 50 ++++ 5 files changed, 540 insertions(+), 215 deletions(-) create mode 100644 src/features/player/components/ArtworkCarousel.tsx delete mode 100644 src/features/player/components/TransportSwipe.tsx create mode 100644 src/features/player/hooks/useQueueNeighbours.ts diff --git a/src/features/player/PlayerScreen.tsx b/src/features/player/PlayerScreen.tsx index 11187c2..4174c49 100644 --- a/src/features/player/PlayerScreen.tsx +++ b/src/features/player/PlayerScreen.tsx @@ -1,4 +1,3 @@ -import { Image } from 'expo-image'; import { useRouter } from 'expo-router'; import { ChevronDown, @@ -25,11 +24,12 @@ import type { RepeatMode } from '@/services/audio/types'; import { formatDuration } from '@/services/format/duration'; import { useThemeColors } from '@/theme/useTheme'; +import { ArtworkCarousel } from './components/ArtworkCarousel'; import { FavoriteButton } from './components/FavoriteButton'; import { Scrubber } from './components/Scrubber'; import { SpecStrip } from './components/SpecStrip'; -import { TransportSwipe } from './components/TransportSwipe'; import { usePlayback, usePlaybackControls } from './hooks/usePlayback'; +import { useQueueNeighbours } from './hooks/useQueueNeighbours'; /** * Now Playing. @@ -44,11 +44,14 @@ export function PlayerScreen() { const { phase, track, positionMs, durationMs, error } = usePlayback(); const { toggle, toggleShuffle, next, previous, seekTo } = usePlaybackControls(); + const neighbours = useQueueNeighbours(); const [repeat, setRepeatState] = useState(() => AudioEngine.getRepeat()); const [shuffled, setShuffledState] = useState(() => AudioEngine.isShuffled()); const close = useCallback(() => router.back(), [router]); - const openQueue = useCallback(() => router.push('/queue'), [router]); + // `navigate` rather than `push`, for the same reason as the player itself: + // a double press must not leave two identical screens on the stack. + const openQueue = useCallback(() => router.navigate('/queue'), [router]); const onShufflePress = useCallback(() => { toggleShuffle(); @@ -73,39 +76,30 @@ export function PlayerScreen() { const isPlaying = phase === 'playing'; const isLoading = phase === 'loading'; const algorithm = getShuffleAlgorithm(); - const artworkUri = track.artworkPath ? `file://${track.artworkPath}` : null; const RepeatIcon = repeat === 'one' ? Repeat1 : Repeat; return (
- + {/* The artwork carries the gestures, not the whole screen: the scrubber - below it owns a pan of its own, and two competing pans on one surface + below owns a pan of its own, and two competing pans on one surface means a scrub that sometimes dismisses the screen instead. Down dismisses, which is what the brief asks for and what Android's - modal presentation does not give on its own. + modal presentation does not give on its own. Sideways moves through the + queue with the neighbouring covers already on screen. */} - - - {artworkUri ? ( - - ) : ( - - )} - - - - + + + {track.title} @@ -119,12 +113,12 @@ export function PlayerScreen() { {phase === 'error' ? ( - + {t('player.error')} {error ? ` ${error}` : ''} ) : ( - + )} - + = Math.abs(0)` is true — so every + * gesture locked to horizontal and vertical ones were silently discarded. A + * downward drag simply sprang back. Waiting for real movement is the fix. + */ +const AXIS_LOCK_SLOP = 6; +/** One spring for every snap, so a flick and a drag settle identically. */ +const SPRING = { damping: 22, stiffness: 190, mass: 0.6 } as const; + +export interface ArtworkCarouselProps { + neighbours: QueueNeighbours; + onNext: () => void; + onPrevious: () => void; + /** Swipe down on the artwork dismisses the player. */ + onDismiss: () => void; +} + +/** + * The Now Playing artwork, as a carousel of three. + * + * Previous, current and next are all mounted side by side and the whole strip + * translates under the finger, so the neighbour is a real decoded image sliding + * in rather than a blank square that fills in after the transition. That is the + * difference between this and the version it replaces, which moved one image + * and swapped its source on release. + * + * Release is decided by distance **or** velocity: a lazy drag past 28% of the + * screen commits, and so does a flick over 500px/s however short. Committing on + * distance alone is what makes a carousel feel unresponsive to people who flick; + * committing on velocity alone strands people who drag slowly. + * + * At the ends of the queue the strip still moves, but at a quarter rate and it + * always springs back. Refusing to move at all reads as a dropped gesture; a + * rubber band says "there is nothing here" in the language the gesture is + * already speaking. + * + * The commit is optimistic in exactly one respect: `translateX` snaps to the + * neighbour's slot and is reset to centre by the engine's queue update that + * follows. Both are driven from the same spring, so the seam is not visible. + */ +export function ArtworkCarousel({ + neighbours, + onNext, + onPrevious, + onDismiss, +}: ArtworkCarouselProps) { + const { width } = useWindowDimensions(); + const reducedMotion = useReducedMotion(); + + const offsetX = useSharedValue(0); + const offsetY = useSharedValue(0); + /** 0 undecided, 1 horizontal, 2 vertical. Fixed once per gesture. */ + const axis = useSharedValue(0); + + const { previous, current, next } = neighbours; + const hasPrevious = previous !== null; + const hasNext = next !== null; + + /* + * Built inline rather than memoized, matching `Scrubber`. + * + * Memoizing it is what `SwipeableRow` does, and there it is worth it — that + * one has forty live instances in a list. This has exactly one, so rebuilding + * the gesture on the rare render costs nothing measurable, and doing it inline + * keeps the shared values out of a hook's closure. The React Compiler's + * immutability rule rejects mutating a value captured by a hook, which is + * correct for ordinary values and unavoidable friction for Reanimated. + */ + const pan = Gesture.Pan() + .onBegin(() => { + axis.value = 0; + }) + .onUpdate((event) => { + if (axis.value === 0) { + const dx = Math.abs(event.translationX); + const dy = Math.abs(event.translationY); + // Undecided until the finger has actually gone somewhere. + if (Math.max(dx, dy) < AXIS_LOCK_SLOP) return; + axis.value = dx >= dy ? 1 : 2; + } + + if (axis.value === 2) { + // Down only. Dragging up from the player has no meaning. + offsetY.value = Math.max(0, event.translationY); + return; + } + + const wanted = event.translationX; + // Resist where there is nothing to reveal: dragging right at the start of + // the queue, or left at the end. + const blocked = (wanted > 0 && !hasPrevious) || (wanted < 0 && !hasNext); + offsetX.value = blocked ? wanted * RUBBER_BAND : wanted; + }) + .onEnd((event) => { + if (axis.value === 2) { + if (event.translationY > DISMISS_DISTANCE || event.velocityY > DISMISS_VELOCITY) { + runOnJS(onDismiss)(); + } + return; + } + + const far = Math.abs(event.translationX) > width * DISTANCE_THRESHOLD; + const fast = Math.abs(event.velocityX) > VELOCITY_THRESHOLD; + + if (far || fast) { + // Left reveals the next track; right reveals the previous one. + if (event.translationX < 0 && hasNext) { + offsetX.value = withSpring(-width, SPRING, (finished) => { + if (!finished) return; + /* + * Snap back to centre in the same frame the engine is told to + * advance. The queue update re-fills the slots a moment later, so + * leaving the strip parked one slot over would show the right track + * in the wrong position. + */ + offsetX.value = 0; + runOnJS(onNext)(); + }); + return; + } + if (event.translationX > 0 && hasPrevious) { + offsetX.value = withSpring(width, SPRING, (finished) => { + if (!finished) return; + offsetX.value = 0; + runOnJS(onPrevious)(); + }); + return; + } + } + + // Did not commit, or committed against the end of the queue: spring home. + offsetX.value = withSpring(0, SPRING); + }) + .onFinalize(() => { + axis.value = 0; + offsetY.value = withSpring(0, SPRING); + }); + + const stripStyle = useAnimatedStyle(() => ({ + transform: [{ translateX: offsetX.value }, { translateY: offsetY.value }], + })); + + return ( + + {/* Clips the neighbours to the visible slot. */} + + + + + + + + + ); +} + +interface SlotProps { + track: PlayableTrack | null; + width: number; + /** Where this slot sits relative to the centre one. */ + offset: number; +} + +/** + * One square in the strip. + * + * Absolutely positioned rather than laid out in the row, so the strip is exactly + * one screen wide and the neighbours hang off either edge — a three-wide flex row + * would make the container three screens wide and push the layout around. + */ +function Slot({ track, width, offset }: SlotProps) { + const { t } = useTranslation(); + const colors = useThemeColors(); + + const artworkUri = track?.artworkPath ? `file://${track.artworkPath}` : null; + const isCentre = offset === 0; + + const style = useMemo( + () => ({ width, left: offset, position: 'absolute' as const, top: 0, bottom: 0 }), + [width, offset], + ); + + const content = ( + + {artworkUri ? ( + + ) : ( + + )} + + ); + + /* + * The centre slot is in the layout and gives the container its height; the + * neighbours are absolute and contribute none. Doing it the other way round + * would make the player's height depend on whether a next track exists. + */ + if (isCentre) { + return ( + + {content} + + ); + } + + return ( + + {content} + + ); +} diff --git a/src/features/player/components/MiniPlayer.tsx b/src/features/player/components/MiniPlayer.tsx index 9727a38..b759bc0 100644 --- a/src/features/player/components/MiniPlayer.tsx +++ b/src/features/player/components/MiniPlayer.tsx @@ -1,16 +1,46 @@ import { Image } from 'expo-image'; import { useRouter } from 'expo-router'; -import { Music, Pause, Play, SkipForward } from 'lucide-react-native'; +import { Music, Pause, Play, SkipBack, SkipForward } from 'lucide-react-native'; import { useCallback } from 'react'; import { useTranslation } from 'react-i18next'; import { Pressable, Text, View } from 'react-native'; +import { Gesture, GestureDetector } from 'react-native-gesture-handler'; +import Animated, { + runOnJS, + useAnimatedStyle, + useSharedValue, + withSpring, +} from 'react-native-reanimated'; +import { tapFeedback } from '@/services/haptics'; import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; +import { useReducedMotion } from '@/theme/useReducedMotion'; import { useThemeColors } from '@/theme/useTheme'; import { useCurrentTrack, usePlaybackControls, usePlaybackPhase } from '../hooks/usePlayback'; import { MiniProgress } from './MiniProgress'; -import { TransportSwipe } from './TransportSwipe'; + +/** How far up the strip must be dragged to open the player. */ +const OPEN_DISTANCE = 40; +/** px/s upward that opens it regardless of distance. */ +const OPEN_VELOCITY = 600; +/** Sideways travel that changes track. Shorter than the player's — less room. */ +const SKIP_DISTANCE = 56; +/** Below this the pan never claims the touch, so the buttons still work. */ +const ACTIVATION_SLOP = 10; +/** + * Movement needed before the gesture commits to an axis. + * + * Not cosmetic. Deciding on the first `onUpdate` compares two translations that + * are both still zero, and `Math.abs(0) >= Math.abs(0)` is true — so every + * gesture locked to horizontal and vertical ones were silently discarded. A + * downward drag simply sprang back. Waiting for real movement is the fix. + */ +const AXIS_LOCK_SLOP = 6; +/** How far the strip follows the finger. Damped, so it reads as resistance. */ +const FOLLOW_RATIO = 0.4; + +const SPRING = { damping: 20, stiffness: 220 } as const; /** * The persistent transport strip above the tab bar. @@ -18,31 +48,115 @@ import { TransportSwipe } from './TransportSwipe'; * Renders nothing at all when idle rather than sitting there empty — a dead * strip on a fresh install is clutter, and the tab bar should meet the list * until there is something playing. + * + * Three ways to open the player, because the strip is 64px tall and precision is + * not always available: tap the artwork and title, drag the strip upwards, or + * flick it. The gesture is deliberately additive — every one of its actions has + * a button beside it, so nothing here is reachable only by knowing a secret. */ export function MiniPlayer() { useLifecycleTrace('MiniPlayer'); const { t } = useTranslation(); const colors = useThemeColors(); const router = useRouter(); + const reducedMotion = useReducedMotion(); + /* * Phase and track, never position. * * Measured on the Pixel_7 AVD over ten seconds of playback: subscribing to * the whole engine state re-rendered this component 20 times — exactly the - * engine's 2 Hz status interval — and 0 times after the split. `BottomTabBar` - * next door was never affected either way, which is worth writing down - * because it was the thing this change was first blamed on: React re-renders - * the component whose store changed and its children, not its siblings. - * - * Twenty reconciliations of an `expo-image` and three Pressables per ten - * seconds, forever, for a strip whose text has not changed. Position belongs - * to `MiniProgress`, which is one animated view and re-renders never. + * engine's 2 Hz status interval — and 0 times after the split. Twenty + * reconciliations of an `expo-image` and four Pressables per ten seconds, for + * a strip whose text has not changed. Position belongs to `MiniProgress`, + * which is one animated view and re-renders never. */ const phase = usePlaybackPhase(); const track = useCurrentTrack(); const { toggle, next, previous } = usePlaybackControls(); - const openPlayer = useCallback(() => router.push('/player'), [router]); + /* + * `navigate`, not `push`. + * + * The strip offers the same action three ways — tap, drag, flick — and a + * drag fires the pan's handler while the underlying Pressable can still + * register its own press. `push` stacked two copies of the player, and the + * symptom was baffling: swipe down to dismiss appeared to do nothing, and so + * did the close button, because each was correctly popping one of two + * identical screens. `navigate` reuses the route that is already there. + */ + const openPlayer = useCallback(() => { + tapFeedback(); + router.navigate('/player'); + }, [router]); + + const offsetX = useSharedValue(0); + const offsetY = useSharedValue(0); + const axis = useSharedValue(0); + + /* + * The gesture is built here rather than in a shared wrapper, and that is the + * fix for "swiping the mini player up does not open the player". + * + * The previous version put a generic pan on a container whose children are all + * Pressables, with `activeOffsetX` and `activeOffsetY` both set. Two axes of + * activation on a surface made entirely of touch targets meant the pan + * routinely lost the race to a child's press responder, so the swipe worked + * sometimes and looked broken the rest of the time. + * + * This one activates on vertical movement only (`activeOffsetY`) and treats + * horizontal travel as a secondary read once it already owns the gesture, so + * it never competes with a tap and never needs to. + * + * Built inline rather than memoized, like `Scrubber` and unlike + * `SwipeableRow`: there is exactly one mini player, so a rebuild per render + * costs nothing, and keeping the shared values out of a hook's closure avoids + * the React Compiler's immutability rule — which is right about ordinary + * values and simply does not model Reanimated. + */ + const pan = Gesture.Pan() + .activeOffsetY([-ACTIVATION_SLOP, ACTIVATION_SLOP]) + .onBegin(() => { + axis.value = 0; + }) + .onUpdate((event) => { + if (axis.value === 0) { + const dx = Math.abs(event.translationX); + const dy = Math.abs(event.translationY); + // Undecided until the finger has actually gone somewhere. + if (Math.max(dx, dy) < AXIS_LOCK_SLOP) return; + axis.value = dx > dy ? 1 : 2; + } + + if (axis.value === 1) { + offsetX.value = event.translationX * FOLLOW_RATIO; + return; + } + // Up only. Dragging the strip down has nowhere to go. + offsetY.value = Math.min(0, event.translationY) * FOLLOW_RATIO; + }) + .onEnd((event) => { + if (axis.value === 1) { + if (event.translationX <= -SKIP_DISTANCE) runOnJS(next)(); + else if (event.translationX >= SKIP_DISTANCE) runOnJS(previous)(); + return; + } + + // Distance or velocity, so a short flick opens it as readily as a + // deliberate drag. + const far = event.translationY <= -OPEN_DISTANCE; + const fast = event.velocityY <= -OPEN_VELOCITY; + if (far || fast) runOnJS(openPlayer)(); + }) + .onFinalize(() => { + axis.value = 0; + offsetX.value = withSpring(0, SPRING); + offsetY.value = withSpring(0, SPRING); + }); + + const followStyle = useAnimatedStyle(() => ({ + transform: [{ translateX: offsetX.value }, { translateY: offsetY.value }], + })); if (phase === 'idle' || track === null) return null; @@ -53,76 +167,88 @@ export function MiniPlayer() { {/* A hairline of progress rather than a scrub bar. The mini player says - where you are; seeking is the full player's job, and a 2px target is - not a control anyone can hit. + where you are; seeking is the full player's job, and a 2px target is not + a control anyone can hit. - Outside the swipe wrapper: the bar reports position and should not slide - around with the strip that reports the track. + Outside the gesture wrapper: the bar reports position and should not + slide around with the strip that reports the track. */} - {/* - Swipe up to open, sideways to change track. The strip is small and its - two icon buttons are the only precise targets on it, so the gesture is - how most people will actually drive it. - */} - - - - {artworkUri ? ( - - ) : ( - - - - )} - - - - {track.title} - - {track.artistName ? ( - - {track.artistName} + + + + + {artworkUri ? ( + + ) : ( + + + + )} + + + + {track.title} - ) : null} - - - - - {isPlaying ? ( - - ) : ( - - )} - - - - - - - + {track.artistName ? ( + + {track.artistName} + + ) : null} + + + + {/* + Previous belongs here as much as next does. Skipping back is the + commonest correction there is — you skip one too many and want it + back — and having only "next" made the strip a one-way control. + */} + + + + + + {isPlaying ? ( + + ) : ( + + )} + + + + + + + + ); } diff --git a/src/features/player/components/TransportSwipe.tsx b/src/features/player/components/TransportSwipe.tsx deleted file mode 100644 index d892be4..0000000 --- a/src/features/player/components/TransportSwipe.tsx +++ /dev/null @@ -1,112 +0,0 @@ -import type { ReactNode } from 'react'; -import { Gesture, GestureDetector } from 'react-native-gesture-handler'; -import Animated, { - runOnJS, - useAnimatedStyle, - useSharedValue, - withSpring, -} from 'react-native-reanimated'; - -import { tapFeedback } from '@/services/haptics'; -import { useReducedMotion } from '@/theme/useReducedMotion'; - -/** How far a finger must travel before the gesture counts as a swipe. */ -const COMMIT_DISTANCE = 60; -/** Below this the pan never activates, so taps and scrolls are unaffected. */ -const ACTIVATION_SLOP = 12; -/** How far the content follows the finger. Damped, so it reads as resistance. */ -const FOLLOW_RATIO = 0.35; - -export interface TransportSwipeProps { - children: ReactNode; - /** Swipe right — the direction that goes back, as it does in a book. */ - onSwipeRight?: () => void; - onSwipeLeft?: () => void; - onSwipeUp?: () => void; - onSwipeDown?: () => void; -} - -/** - * Swipes for the transport, with the content following the finger. - * - * Wraps the mini player and the Now Playing artwork. Horizontal changes track, - * vertical opens or dismisses — the gestures everyone already has in their - * hands from every other player, which is the entire argument for them. - * - * The axis is decided once, at the point the pan activates, and held for the - * rest of the gesture. Deciding per frame lets a diagonal drag flip between - * "change track" and "dismiss" mid-swipe, so the user gets whichever one their - * finger happened to be favouring when they let go. - * - * Distance decides, not velocity. A flick and a slow drag of the same length - * mean the same thing, and velocity thresholds are the reason swipe controls - * feel unreliable to people who do not flick. - */ -export function TransportSwipe({ - children, - onSwipeRight, - onSwipeLeft, - onSwipeUp, - onSwipeDown, -}: TransportSwipeProps) { - const offsetX = useSharedValue(0); - const offsetY = useSharedValue(0); - /** 0 undecided, 1 horizontal, 2 vertical. Set once per gesture. */ - const axis = useSharedValue(0); - const reducedMotion = useReducedMotion(); - - const pan = Gesture.Pan() - .activeOffsetX([-ACTIVATION_SLOP, ACTIVATION_SLOP]) - .activeOffsetY([-ACTIVATION_SLOP, ACTIVATION_SLOP]) - .onBegin(() => { - axis.value = 0; - }) - .onUpdate((event) => { - if (axis.value === 0) { - axis.value = Math.abs(event.translationX) >= Math.abs(event.translationY) ? 1 : 2; - } - - if (reducedMotion) return; - - if (axis.value === 1) offsetX.value = event.translationX * FOLLOW_RATIO; - else offsetY.value = event.translationY * FOLLOW_RATIO; - }) - .onEnd((event) => { - const horizontal = axis.value === 1; - const travelled = horizontal ? event.translationX : event.translationY; - - if (Math.abs(travelled) >= COMMIT_DISTANCE) { - if (horizontal) runOnJS(fire)(travelled > 0 ? onSwipeRight : onSwipeLeft); - else runOnJS(fire)(travelled > 0 ? onSwipeDown : onSwipeUp); - } - }) - .onFinalize(() => { - // Always springs home. The committed action replaces the content or the - // screen; leaving the view displaced would show the next track offset. - offsetX.value = withSpring(0, { damping: 20, stiffness: 200 }); - offsetY.value = withSpring(0, { damping: 20, stiffness: 200 }); - axis.value = 0; - }); - - const followStyle = useAnimatedStyle(() => ({ - transform: [{ translateX: offsetX.value }, { translateY: offsetY.value }], - })); - - return ( - - {children} - - ); -} - -/** - * Run a handler if there is one, with feedback. - * - * A swipe in a direction nothing is bound to stays silent rather than buzzing: - * confirming an action that did not happen is worse than no confirmation. - */ -function fire(handler: (() => void) | undefined): void { - if (!handler) return; - tapFeedback(); - handler(); -} diff --git a/src/features/player/hooks/useQueueNeighbours.ts b/src/features/player/hooks/useQueueNeighbours.ts new file mode 100644 index 0000000..e1bb463 --- /dev/null +++ b/src/features/player/hooks/useQueueNeighbours.ts @@ -0,0 +1,50 @@ +import { useMemo, useSyncExternalStore } from 'react'; + +import { AudioEngine, type QueueSnapshot } from '@/services/audio/AudioEngine'; +import type { PlayableTrack } from '@/services/audio/types'; + +export interface QueueNeighbours { + previous: PlayableTrack | null; + current: PlayableTrack | null; + next: PlayableTrack | null; +} + +/** + * What is playing, and what sits either side of it in the queue. + * + * The carousel needs all three mounted at once. That is the whole point of it: + * the neighbouring artwork is already decoded and on screen just off the edge, + * so dragging sideways reveals a real image instead of a blank square that + * fills in a moment later. + * + * Subscribes to the engine's queue rather than its playback state — state is + * emitted twice a second for the position, and re-deriving this at 2 Hz would + * hand the carousel a new object on every tick. + * + * Null at either end rather than wrapping. Repeat-all does wrap, but the queue + * screen and the transport both treat the ends as ends, and a carousel that + * silently loops from the last track back to the first would be showing the + * user something the skip button would not do. + */ +export function useQueueNeighbours(): QueueNeighbours { + const snapshot = useSyncExternalStore(subscribe, getSnapshot); + + return useMemo(() => { + const { tracks, index } = snapshot; + if (index < 0) return { previous: null, current: null, next: null }; + + return { + previous: tracks[index - 1] ?? null, + current: tracks[index] ?? null, + next: tracks[index + 1] ?? null, + }; + }, [snapshot]); +} + +function subscribe(onChange: () => void): () => void { + return AudioEngine.subscribeQueue(onChange); +} + +function getSnapshot(): QueueSnapshot { + return AudioEngine.getQueueSnapshot(); +} From 3e48a1993290cafa21bb42f96adf0e288929661b Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 05:28:35 +0300 Subject: [PATCH 06/20] feat(settings): say what each setting does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every row now has a line under it explaining the setting, and the shuffle picker is a single column with a sentence per algorithm rather than five squeezed segments. The segmented control was the wrong control, not just a badly sized one. "Discovery" and "Favourites" are names; the entire argument for offering five shuffle algorithms is that a user can tell them apart, and a control with no room for the explanation cannot let them. Wrapping at three per row — the previous fix — made it fit without making it clear. A column has as much room as it needs in any language and at any font scale, which also disposes of the Turkish-is-20%-longer problem permanently rather than by measuring. Selection is a tick, not a filled row: with a description under every option, filling the selected one would put body text on indigo and cost the contrast the tokens guarantee. The shuffle hints are rewritten now that there is room for a real sentence — "An unplayed track comes up twice as often as one you have heard once" says more than "Favours tracks you have played least". Theme and language stay segmented: three short self-evident options each, where a column would be pomp. Adds the Playback and short-file settings, both wired: haptics already drives `src/services/haptics`, and "ignore short files" now actually feeds the scanner's minDurationMs, read at scan time rather than captured at mount because the switch lives on a different screen from the button. Deliberately *not* added: a "resume on launch" switch. The preference exists in the store from an earlier commit, but nothing persists or restores a queue yet, so the control would have changed nothing while claiming to. An absent feature is better than a lying one. It goes in when the queue persistence does. Verified on the Pixel_7 AVD: the shuffle column reads properly, both switches render with their descriptions, and the folder group keeps its existing copy. Co-Authored-By: Claude Opus 5 --- src/components/ui/OptionList.tsx | 85 ++++++++++++++++++++++ src/components/ui/SettingRow.tsx | 53 +++++++++++--- src/components/ui/SettingSwitch.tsx | 58 +++++++++++++++ src/features/library/hooks/useScan.ts | 16 ++++- src/features/settings/SettingsScreen.tsx | 89 ++++++++++++++++++++---- src/i18n/locales/en.json | 25 ++++--- src/i18n/locales/tr.json | 25 ++++--- 7 files changed, 311 insertions(+), 40 deletions(-) create mode 100644 src/components/ui/OptionList.tsx create mode 100644 src/components/ui/SettingSwitch.tsx diff --git a/src/components/ui/OptionList.tsx b/src/components/ui/OptionList.tsx new file mode 100644 index 0000000..7dca458 --- /dev/null +++ b/src/components/ui/OptionList.tsx @@ -0,0 +1,85 @@ +import { Check } from 'lucide-react-native'; +import { Pressable, Text, View } from 'react-native'; + +import { useThemeColors } from '@/theme/useTheme'; + +export interface Option { + value: T; + /** Already translated. */ + label: string; + /** Already translated. One line saying what this choice actually does. */ + description: string; +} + +export interface OptionListProps { + options: readonly Option[]; + value: T; + onChange: (value: T) => void; + /** Translated group label, for screen readers. */ + accessibilityLabel: string; +} + +/** + * One choice per row, each with a sentence explaining it. + * + * The alternative — a segmented control — is right when the options are + * self-explanatory and wrong the moment they are not. "Discovery" and + * "Favourites" are names, not descriptions, and the entire argument for offering + * five shuffle algorithms is that a user can tell them apart. A control that + * cannot fit the explanation is the wrong control. + * + * It also solves the layout problem honestly rather than by wrapping: five + * segments across a phone gave each about a fifth of the width, and Turkish runs + * 10–20% longer than English. A column has as much room as it needs in any + * language and at any font scale. + * + * The tick marks the selection rather than a filled background: with a + * description under every row, filling the selected one would put body text on + * an indigo panel and cost the contrast the tokens guarantee. + */ +export function OptionList({ + options, + value, + onChange, + accessibilityLabel, +}: OptionListProps) { + const colors = useThemeColors(); + + return ( + + {options.map((option) => { + const selected = option.value === value; + return ( + onChange(option.value)} + accessibilityRole="radio" + accessibilityState={{ selected }} + accessibilityLabel={option.label} + accessibilityHint={option.description} + className="min-h-11 flex-row items-start gap-3 py-2" + > + {/* Fixed-width gutter, so every title starts on the same column + whether or not it is the selected one. */} + + {selected ? : null} + + + + + {option.label} + + {option.description} + + + ); + })} + + ); +} diff --git a/src/components/ui/SettingRow.tsx b/src/components/ui/SettingRow.tsx index e69ffa6..43467bc 100644 --- a/src/components/ui/SettingRow.tsx +++ b/src/components/ui/SettingRow.tsx @@ -10,22 +10,55 @@ export interface SettingRowProps { /** Already translated. */ label: string; /** The current value, spelled out. Sits right, in the muted tone. */ - value: string; - /** The control itself — a segmented control, a switch, a link. */ - children: ReactNode; + value?: string; + /** + * One line saying what this setting does. Already translated. + * + * Not optional by preference — every row should have one. It is typed as + * optional only because a row whose control is itself self-describing (an + * `OptionList`, which explains each choice) would otherwise say the same thing + * twice. + */ + description?: string; + /** The control itself — an option list, a switch, a link. */ + children?: ReactNode; } -/** Label and current value on one line, with the control underneath. */ -export function SettingRow({ icon: Icon, label, value, children }: SettingRowProps) { +/** + * Label, current value, an explanation, and the control underneath. + * + * The explanation is the point of this component. A settings screen that lists + * only names makes the user guess, and the guesses are wrong in exactly the + * places that matter — nobody knows what "Discovery" does from the word alone. + */ +export function SettingRow({ + icon: Icon, + label, + value, + description, + children, +}: SettingRowProps) { const colors = useThemeColors(); return ( - - - - {label} - {value} + + + {/* Nudged down so the icon sits on the label's centre line rather than + the top of a block that may be two lines tall. */} + + + + + + {label} + {description ? ( + {description} + ) : null} + + + {value ? {value} : null} + {children} ); diff --git a/src/components/ui/SettingSwitch.tsx b/src/components/ui/SettingSwitch.tsx new file mode 100644 index 0000000..30e3f09 --- /dev/null +++ b/src/components/ui/SettingSwitch.tsx @@ -0,0 +1,58 @@ +import type { LucideIcon } from 'lucide-react-native'; +import { Switch, Text, View } from 'react-native'; + +import { useThemeColors } from '@/theme/useTheme'; + +export interface SettingSwitchProps { + icon: LucideIcon; + /** Already translated. */ + label: string; + /** Already translated. Says what turning it on actually changes. */ + description: string; + value: boolean; + onChange: (value: boolean) => void; +} + +/** + * A setting that is on or off. + * + * The platform `Switch` rather than a custom control: it is the one widget + * users recognise instantly, it already answers to the system font scale and to + * TalkBack, and reimplementing it would trade all of that for a marginally + * better fit with the panel. + * + * Its colours are props rather than classes — `Switch` is a native component and + * NativeWind cannot reach inside it — so they come from the same tokens the + * classes do, via `useThemeColors`. + */ +export function SettingSwitch({ + icon: Icon, + label, + description, + value, + onChange, +}: SettingSwitchProps) { + const colors = useThemeColors(); + + return ( + + + + + + + {label} + {description} + + + + + ); +} diff --git a/src/features/library/hooks/useScan.ts b/src/features/library/hooks/useScan.ts index 0139931..0e7db13 100644 --- a/src/features/library/hooks/useScan.ts +++ b/src/features/library/hooks/useScan.ts @@ -12,6 +12,7 @@ import { saveEnumerated, } from '@/db/queries/scanning'; import { permissionErrorFor } from '@/services/scanner/permission'; +import { getIgnoreShortFiles } from '@/services/settings'; import { isPickerDismissal } from '@/services/scanner/pickerError'; import { PRIMARY_VOLUME_ROOT, treeUriToPath } from '@/services/scanner/treeUri'; import { @@ -24,6 +25,9 @@ import { const IDLE: ScanProgress = { phase: 'idle', total: 0, processed: 0 }; +/** What "short" means when the setting is on. Thirty seconds, as the copy says. */ +const SHORT_FILE_MS = 30_000; + /** * Whether a scan may proceed, and if so whether this is the first time the app * has ever been allowed to read the library. @@ -108,7 +112,17 @@ export function useScan(): UseScanResult { const run = useCallback(async () => { cancelled.current = false; const controller = { isCancelled: () => cancelled.current }; - const options = { ...DEFAULT_SCAN_OPTIONS, artworkDirectory: artworkDirectory() }; + + /* + * Read at scan time, not captured at mount: the switch is in Settings and + * the scan is started from the Library, so the value can legitimately change + * between this hook mounting and the user pressing Scan. + */ + const options = { + ...DEFAULT_SCAN_OPTIONS, + minDurationMs: getIgnoreShortFiles() ? SHORT_FILE_MS : DEFAULT_SCAN_OPTIONS.minDurationMs, + artworkDirectory: artworkDirectory(), + }; const enumerated = await enumerateLibrary(ports, options, setProgress, controller); if (enumerated.phase !== 'done') return; diff --git a/src/features/settings/SettingsScreen.tsx b/src/features/settings/SettingsScreen.tsx index 98faec6..a49d823 100644 --- a/src/features/settings/SettingsScreen.tsx +++ b/src/features/settings/SettingsScreen.tsx @@ -1,17 +1,33 @@ -import { Languages, Monitor, Moon, Shuffle, Sun, type LucideIcon } from 'lucide-react-native'; +import { + Clock, + Languages, + Monitor, + Moon, + Shuffle, + Sun, + Vibrate, + type LucideIcon, +} from 'lucide-react-native'; import { useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { ScrollView, Text } from 'react-native'; +import { ScrollView } from 'react-native'; +import { OptionList, type Option } from '@/components/ui/OptionList'; import { Screen } from '@/components/ui/Screen'; import { SegmentedControl, type SegmentedControlOption } from '@/components/ui/SegmentedControl'; import { SettingGroup } from '@/components/ui/SettingGroup'; import { SettingRow } from '@/components/ui/SettingRow'; +import { SettingSwitch } from '@/components/ui/SettingSwitch'; import { changeLanguage } from '@/i18n'; +import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; import { + getHapticsEnabled, + getIgnoreShortFiles, getLanguagePreference, getShuffleAlgorithm, LANGUAGE_PREFERENCES, + setHapticsEnabled, + setIgnoreShortFiles, setShuffleAlgorithm, THEME_PREFERENCES, type LanguagePreference, @@ -19,7 +35,6 @@ import { } from '@/services/settings'; import { SHUFFLE_ALGORITHMS, type ShuffleAlgorithm } from '@/services/shuffle'; import { useTheme } from '@/theme/useTheme'; -import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; import { DevTools } from './components/DevTools'; import { ScanFolderList } from './components/ScanFolderList'; @@ -30,12 +45,27 @@ const THEME_ICONS: Record = { dark: Moon, }; +/** + * Every setting, each one explained. + * + * The rule this screen follows: a row names a setting, and a line under it says + * what the setting does. A list of bare names makes the user guess, and the + * guesses are wrong exactly where it matters — nobody knows what "Discovery" + * means from the word. + * + * Theme and language stay as segmented controls: three and three short, + * self-evident options each. Shuffle is a column, because five names that need + * explaining is a different problem — see `OptionList`. + */ export function SettingsScreen() { useLifecycleTrace('SettingsScreen'); const { t } = useTranslation(); const { preference: theme, setPreference: setTheme } = useTheme(); + const [language, setLanguage] = useState(getLanguagePreference); const [shuffle, setShuffle] = useState(getShuffleAlgorithm); + const [haptics, setHaptics] = useState(getHapticsEnabled); + const [ignoreShort, setIgnoreShort] = useState(getIgnoreShortFiles); const themeOptions: SegmentedControlOption[] = THEME_PREFERENCES.map( (value) => ({ @@ -49,9 +79,11 @@ export function SettingsScreen() { (value) => ({ value, label: t(`settings.language.${value}`) }), ); - const shuffleOptions: SegmentedControlOption[] = SHUFFLE_ALGORITHMS.map( - (value) => ({ value, label: t(`settings.shuffle.${value}`) }), - ); + const shuffleOptions: Option[] = SHUFFLE_ALGORITHMS.map((value) => ({ + value, + label: t(`settings.shuffle.${value}`), + description: t(`settings.shuffle.${value}Hint`), + })); function onShuffleChange(next: ShuffleAlgorithm) { setShuffle(next); @@ -63,6 +95,16 @@ export function SettingsScreen() { changeLanguage(next); } + function onHapticsChange(next: boolean) { + setHaptics(next); + setHapticsEnabled(next); + } + + function onIgnoreShortChange(next: boolean) { + setIgnoreShort(next); + setIgnoreShortFiles(next); + } + return ( @@ -71,6 +113,7 @@ export function SettingsScreen() { icon={THEME_ICONS[theme]} label={t('settings.appearance.theme')} value={t(`settings.appearance.${theme}`)} + description={t('settings.appearance.description')} > + {/* No `description` on the row: the list explains every option, and + saying it twice is worse than saying it once. */} - - {/* Says what each one does — the names alone do not, and the whole - point of offering five is that the user can tell them apart. */} - - {t(`settings.shuffle.${shuffle}Hint`)} - + + + {/* + No "resume on launch" switch yet. The preference exists in the store, + but nothing persists or restores a queue, so the control would have + been a switch that changes nothing — worse than an absent feature, + because it claims one. It goes in when the queue does. + */} + + + diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 900efe5..0d482b0 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -98,20 +98,24 @@ "theme": "Theme", "system": "System", "light": "Light", - "dark": "Dark" + "dark": "Dark", + "description": "Dark is the one this app was designed in. System follows your phone." }, "language": { "title": "Language", "label": "App language", "system": "System", "en": "English", - "tr": "Türkçe" + "tr": "Türkçe", + "description": "Turkish and English. System follows your phone." }, "folders": { "title": "Music folders", "empty": "No folders added. Mufify still scans everything Android has indexed.", "remove": "Remove {{folder}}", - "removeNote": "Removing a folder stops re-indexing it. Your tracks stay." + "removeNote": "Removing a folder stops re-indexing it. Your tracks stay.", + "ignoreShort": "Ignore short files", + "ignoreShortHint": "Skips anything under 30 seconds. Off by default: interludes and album segues are real tracks." }, "shuffle": { "title": "Shuffle", @@ -121,11 +125,16 @@ "discovery": "Discovery", "favorites": "Favourites", "album": "By album", - "pureHint": "Truly random. Clusters, because randomness does.", - "balancedHint": "Spreads each artist across the queue.", - "discoveryHint": "Favours tracks you have played least.", - "favoritesHint": "Favours what you play most, and what you have hearted.", - "albumHint": "Shuffles the albums, never the album. Each one plays in order." + "pureHint": "Truly random. It will sometimes play the same artist twice in a row, because that is what random does.", + "balancedHint": "Spreads each artist across the queue, so no two of their tracks land together.", + "discoveryHint": "Favours what you have played least. An unplayed track comes up twice as often as one you have heard once.", + "favoritesHint": "The opposite of Discovery: favours what you play most, and pushes anything you have hearted further up.", + "albumHint": "Shuffles the albums, not the tracks. Each album plays through in order — for classical and anything with a running order." + }, + "playback": { + "title": "Playback", + "haptics": "Haptics", + "hapticsHint": "A short buzz on play, pause, queueing and reordering." } }, "player": { diff --git a/src/i18n/locales/tr.json b/src/i18n/locales/tr.json index 5349f2f..992c5f5 100644 --- a/src/i18n/locales/tr.json +++ b/src/i18n/locales/tr.json @@ -98,20 +98,24 @@ "theme": "Tema", "system": "Sistem", "light": "Açık", - "dark": "Koyu" + "dark": "Koyu", + "description": "Uygulama koyu tema için tasarlandı. Sistem, telefonunu takip eder." }, "language": { "title": "Dil", "label": "Uygulama dili", "system": "Sistem", "en": "English", - "tr": "Türkçe" + "tr": "Türkçe", + "description": "Türkçe ve İngilizce. Sistem, telefonunu takip eder." }, "folders": { "title": "Müzik klasörleri", "empty": "Eklenmiş klasör yok. Mufify yine de Android’in indekslediği her şeyi tarar.", "remove": "{{folder}} klasörünü kaldır", - "removeNote": "Klasörü kaldırmak yeniden taranmasını durdurur. Parçaların kalır." + "removeNote": "Klasörü kaldırmak yeniden taranmasını durdurur. Parçaların kalır.", + "ignoreShort": "Kısa dosyaları yok say", + "ignoreShortHint": "30 saniyeden kısa olanları atlar. Varsayılan olarak kapalı: ara parçalar ve geçişler de birer parçadır." }, "shuffle": { "title": "Karıştırma", @@ -121,11 +125,16 @@ "discovery": "Keşif", "favorites": "Favoriler", "album": "Albüme göre", - "pureHint": "Gerçekten rastgele. Kümelenir, çünkü rastgelelik böyledir.", - "balancedHint": "Her sanatçıyı kuyruğa yayar.", - "discoveryHint": "En az çaldığın parçaları öne alır.", - "favoritesHint": "En çok çaldıklarını ve favorilediklerini öne alır.", - "albumHint": "Albümleri karıştırır, albümün içini değil. Her biri sırasıyla çalar." + "pureHint": "Gerçekten rastgele. Aynı sanatçıyı arka arkaya çalabilir, çünkü rastgelelik böyledir.", + "balancedHint": "Her sanatçıyı kuyruğa yayar; aynı sanatçıdan iki parça art arda gelmez.", + "discoveryHint": "En az çaldıklarını öne alır. Hiç çalınmamış bir parça, bir kez dinlenmişin iki katı sıklıkta gelir.", + "favoritesHint": "Keşif’in tersi: en çok çaldıklarını öne alır, favorilediklerini daha da yukarı taşır.", + "albumHint": "Parçaları değil albümleri karıştırır. Her albüm kendi sırasıyla çalar — klasik ve sıralı dinlenen her şey için." + }, + "playback": { + "title": "Çalma", + "haptics": "Titreşim", + "hapticsHint": "Çalma, duraklatma, sıraya ekleme ve sıralama değişiminde kısa bir titreşim." } }, "player": { From 00fe1b8dee052b76f09e1d49d15cb167c8b194e2 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 05:38:53 +0300 Subject: [PATCH 07/20] fix(stats): attribute a listen to the queue it came from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suspicion was that only track rollups were being written. That turned out to be wrong: rollupDeltas already fans out to track, artist, album and playlist, and it is tested. Verified on device by reading the database — playing a track that has an artist took artist rollups from 0 rows to 3, one per period, with album and track moving in step. The zero-artist-rollups reading that prompted the check was real and innocent: 526 of the 528 test files have no artist tag at all, so there was no artist to roll up. All 528 have an album, which is why album rows existed and artist rows did not. What *was* broken is upstream of the fan-out. listenRecorder hardcoded sourceType: 'library' for every listen, so: - No listen was ever attributed to a playlist. stats_rollups has an entity type for playlists and could never gain a single row, and the failure is invisible — an empty top-playlists list looks exactly like a user who does not play playlists. - play_events.shuffle_algorithm has existed since Phase 1 with nothing writing to it, so the question it exists to answer — which shuffle produces listens people finish — was unanswerable. docs/shuffle.md claimed it was recorded. Both are attributes of the *queue*, not of a track: the same track played from a playlist and from the library is two different listens. So QueueSource lives on the engine, set by setQueue and defaulted to the library, and rides out with each finished listen alongside the active shuffle algorithm. PlaylistDetail passes its own source from all three of its entry points. Verified on device, reading play_events directly: event 456 has shuffle_algorithm NULL, then shuffle is switched on and 457-459 read "balanced". Exactly the right boundary. One thing worth writing down for the next person: reading this database over adb needs the -wal file too. WAL mode means recent writes are not in the main file at all, and copying only mufify.db shows an empty play_events table and a stale rollup count — which looks precisely like the bug you are hunting. Co-Authored-By: Claude Opus 5 --- src/features/player/listenRecorder.ts | 4 +- .../playlists/PlaylistDetailScreen.tsx | 29 ++++++++++----- src/services/audio/AudioEngine.ts | 37 +++++++++++++++++-- src/services/audio/types.ts | 28 ++++++++++++++ 4 files changed, 84 insertions(+), 14 deletions(-) diff --git a/src/features/player/listenRecorder.ts b/src/features/player/listenRecorder.ts index cd3fcdf..b613adc 100644 --- a/src/features/player/listenRecorder.ts +++ b/src/features/player/listenRecorder.ts @@ -24,7 +24,9 @@ export function startListenRecording(): () => void { durationMs: listen.track.durationMs, msPlayed: listen.msPlayed, startedAt: listen.startedAt, - sourceType: 'library', + sourceType: listen.source.type, + sourceId: listen.source.id, + shuffleAlgorithm: listen.shuffleAlgorithm ?? undefined, completed: listen.completed, }, getWeekStart(), diff --git a/src/features/playlists/PlaylistDetailScreen.tsx b/src/features/playlists/PlaylistDetailScreen.tsx index d3341a4..28e3ae0 100644 --- a/src/features/playlists/PlaylistDetailScreen.tsx +++ b/src/features/playlists/PlaylistDetailScreen.tsx @@ -1,7 +1,7 @@ import { FlashList, type ListRenderItem } from '@shopify/flash-list'; import { useRouter } from 'expo-router'; import { ListMusic } from 'lucide-react-native'; -import { useCallback, useState } from 'react'; +import { useCallback, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { View } from 'react-native'; import { SafeAreaView } from 'react-native-safe-area-context'; @@ -19,10 +19,9 @@ import { } from '@/db/queries/playlists'; import { useMessages } from '@/i18n'; import { AudioEngine } from '@/services/audio/AudioEngine'; -import type { PlayableTrack } from '@/services/audio/types'; +import type { PlayableTrack, QueueSource } from '@/services/audio/types'; import { getShuffleAlgorithm } from '@/services/settings'; -import { usePlaybackControls } from '../player/hooks/usePlayback'; import { AddTracksSheet } from './components/AddTracksSheet'; import { NamePlaylistDialog } from './components/NamePlaylistDialog'; import { PlaylistDetailHeader } from './components/PlaylistDetailHeader'; @@ -41,14 +40,24 @@ export function PlaylistDetailScreen({ playlistId }: PlaylistDetailScreenProps) const entries = usePlaylistEntries(playlistId); const playlist = usePlaylists().find((entry) => entry.id === playlistId); - const { playFrom } = usePlaybackControls(); const [renaming, setRenaming] = useState(false); const [adding, setAdding] = useState(false); + /* + * Every entry point here declares the playlist as the queue's source, which is + * what puts rows in `stats_rollups` under entity type 'playlist'. Without it + * the top-playlists list is permanently empty and looks like a user who never + * plays playlists. + */ + const source = useMemo( + () => ({ type: 'playlist', id: playlistId }), + [playlistId], + ); + const playAll = useCallback(() => { - if (entries.length > 0) playFrom(entries.map(toPlayableEntry), 0); - }, [entries, playFrom]); + if (entries.length > 0) void AudioEngine.setQueue(entries.map(toPlayableEntry), 0, source); + }, [entries, source]); /* * Shuffle uses whichever algorithm Settings has selected, read at press time. @@ -59,16 +68,16 @@ export function PlaylistDetailScreen({ playlistId }: PlaylistDetailScreenProps) */ const shuffleAll = useCallback(async () => { if (entries.length === 0) return; - await AudioEngine.setQueue(entries.map(toPlayableEntry), 0); + await AudioEngine.setQueue(entries.map(toPlayableEntry), 0, source); await AudioEngine.setShuffled(true, getShuffleAlgorithm()); - }, [entries]); + }, [entries, source]); const playAt = useCallback( (position: number) => { const index = entries.findIndex((entry) => entry.position === position); - if (index !== -1) playFrom(entries.map(toPlayableEntry), index); + if (index !== -1) void AudioEngine.setQueue(entries.map(toPlayableEntry), index, source); }, - [entries, playFrom], + [entries, source], ); const remove = useCallback( diff --git a/src/services/audio/AudioEngine.ts b/src/services/audio/AudioEngine.ts index 5287509..0808184 100644 --- a/src/services/audio/AudioEngine.ts +++ b/src/services/audio/AudioEngine.ts @@ -12,9 +12,11 @@ import { shuffleTracks, type ShuffleAlgorithm } from '@/services/shuffle'; import { isPlayable, nextIndex, playNextIndex, previousIndex, shiftForInsert } from './queue'; import { IDLE_PLAYBACK, + LIBRARY_SOURCE, type FinishedListen, type PlayableTrack, type PlaybackState, + type QueueSource, type RepeatMode, } from './types'; @@ -62,6 +64,15 @@ class Engine { */ private sourceQueue: PlayableTrack[] = []; private shuffled = false; + + /* + * Where the current queue came from, and which shuffle reordered it. Both are + * attributes of the *queue*, not of a track, so they belong here rather than + * on `PlayableTrack` — the same track played from a playlist and from the + * library is two different listens with two different attributions. + */ + private source: QueueSource = LIBRARY_SOURCE; + private shuffleAlgorithm: ShuffleAlgorithm | null = null; private state: PlaybackState = IDLE_PLAYBACK; private listeners = new Set(); private configured = false; @@ -171,11 +182,23 @@ class Engine { for (const listener of this.listeners) listener(this.state); } - /** Replace the queue and start at `startIndex`. */ - async setQueue(tracks: PlayableTrack[], startIndex: number): Promise { + /** + * Replace the queue and start at `startIndex`. + * + * `source` defaults to the library because that is where most queues come + * from, and defaulting it means a caller that forgets attributes a listen + * plausibly rather than crashing. Playlist playback passes its own. + */ + async setQueue( + tracks: PlayableTrack[], + startIndex: number, + source: QueueSource = LIBRARY_SOURCE, + ): Promise { this.sourceQueue = tracks; this.queue = tracks; this.shuffled = false; + this.shuffleAlgorithm = null; + this.source = source; if (!isPlayable(startIndex, tracks.length)) { await this.stop(); @@ -259,6 +282,7 @@ class Engine { async setShuffled(shuffled: boolean, algorithm: ShuffleAlgorithm): Promise { const current = this.queue[this.index] ?? null; this.shuffled = shuffled; + this.shuffleAlgorithm = shuffled ? algorithm : null; if (!shuffled) { this.queue = this.sourceQueue; @@ -292,7 +316,14 @@ class Engine { const startedAt = this.startedAt; if (track !== null && startedAt !== null && this.playedMs > 0) { - this.reportListen?.({ track, msPlayed: Math.round(this.playedMs), startedAt, completed }); + this.reportListen?.({ + track, + msPlayed: Math.round(this.playedMs), + startedAt, + completed, + source: this.source, + shuffleAlgorithm: this.shuffleAlgorithm, + }); } this.startedAt = null; diff --git a/src/services/audio/types.ts b/src/services/audio/types.ts index 7846aaa..59e07f1 100644 --- a/src/services/audio/types.ts +++ b/src/services/audio/types.ts @@ -55,6 +55,24 @@ export interface PlaybackState { export type RepeatMode = 'off' | 'all' | 'one'; +/** + * Where a queue came from. + * + * Carried so a finished listen can be attributed. Without it every play looks + * like it came from the library, and `stats_rollups` — which has an entity type + * for playlists — never gains a single playlist row. The statistics screen can + * then show top tracks and top artists but never top playlists, and nothing + * about that failure is visible: the query returns no rows, which looks exactly + * like a user who has not played any playlists. + */ +export interface QueueSource { + type: 'library' | 'album' | 'artist' | 'playlist' | 'queue'; + /** The playlist, album or artist id. Absent for the library. */ + id?: number; +} + +export const LIBRARY_SOURCE: QueueSource = { type: 'library' }; + /** * One finished listen, handed out when a track stops being the current one. * @@ -70,6 +88,16 @@ export interface FinishedListen { startedAt: Date; /** True when it reached its end rather than being skipped or replaced. */ completed: boolean; + /** Where the queue this played from came from. */ + source: QueueSource; + /** + * Which shuffle algorithm was running, or null when playing in order. + * + * `play_events` has had this column since Phase 1 and nothing ever wrote to + * it, so the question it exists to answer — which shuffle produces listens + * people finish — was unanswerable. + */ + shuffleAlgorithm: string | null; } export const IDLE_PLAYBACK: PlaybackState = { From 3a96ed9842458dae7cfc00c6aa6de85b117250db Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 12:55:31 +0300 Subject: [PATCH 08/20] feat(stats): count a repeated track as more than one listen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A layer on top of ADR 005, which is untouched — min(30s, duration*0.5) is still what makes a listen count. This decides where one listen ends and the next begins while the track never changes, which was never in doubt before playback existed: the engine closed a listen only when the loaded track changed. Dragging the scrubber back to the start and listening again produced no second event. The counts were not wrong about what qualified, only about how many times it happened. The rule, checked each status tick against the previous one: 1. the current listen has already earned a play, and 2. the position jumped back to at or below 25% of the track. The first condition is what stops seeking from shredding history. Without it, scrubbing inside the first thirty seconds splits one listen into fragments too short to count as anything, turning a real play into a pile of skips. With it, a rewind can only ever add a listen. 25% has to separate "start it again" from "go back a bit" using the only signal available, which is how far back the position went. Tighter counts a scrub over the final chorus as a replay; looser misses a restart on a nearly-finished track. The boundary deliberately does not require the *new* listen to pass the threshold too. It fires on the rewind and the new listen is classified by the ordinary rule when it ends, so rewind-then-leave records a play for what was heard and a skip for what was abandoned — both true — instead of merging the fragment into the completed play. Worth recording, because it changes what this commit actually adds: repeat-one was already producing one event per loop before this. didJustFinish fires at the end of a track whether or not the queue advances, and the engine already flushed the listen there. The genuinely new case is the manual seek backwards. Decided in docs/adr/011-repeat-listen-detection.md, referencing ADR 005 without reopening it, with a section in docs/stats.md that replaces a stale "not yet implemented" note about exactly this. Verification is unit tests rather than the device, and deliberately so. Ten tests pin the individual decision at every threshold boundary, and five more walk realistic runs of status ticks and count how many listens a session produces — three loops give three, a drag to the start gives one, hunting around in the first few seconds gives none, replaying the last chorus four times gives none. That is the behaviour the feature was asked for, and it is deterministic; the emulator stopped accepting input partway through this work and a six-second file looping on a starved AVD is a worse oracle than the sequence itself. One thing the sequence tests caught that a device never would have: the first version of the harness advanced accumulated playback by a fixed amount per tick regardless of how far the position moved, which made every position jump free and produced a failure that looked exactly like a bug in the rule. It credits forward movement only now, because seeking is not listening. Still owed: on-device confirmation of the seek-back path. Co-Authored-By: Claude Opus 5 --- docs/adr/011-repeat-listen-detection.md | 94 +++++++++++++ docs/stats.md | 62 +++++++- src/services/audio/AudioEngine.ts | 51 ++++++- src/services/stats/repeatListen.test.ts | 180 ++++++++++++++++++++++++ src/services/stats/repeatListen.ts | 74 ++++++++++ 5 files changed, 457 insertions(+), 4 deletions(-) create mode 100644 docs/adr/011-repeat-listen-detection.md create mode 100644 src/services/stats/repeatListen.test.ts create mode 100644 src/services/stats/repeatListen.ts diff --git a/docs/adr/011-repeat-listen-detection.md b/docs/adr/011-repeat-listen-detection.md new file mode 100644 index 0000000..561fb43 --- /dev/null +++ b/docs/adr/011-repeat-listen-detection.md @@ -0,0 +1,94 @@ +# 011 — A repeated track is a second listen + +## Context + +ADR 005 settled what counts as a play: `min(30s, duration × 0.5)` of actual +playback, with `skip` below `duration × 0.2` and `partial` in between. **That +rule is not reopened here and none of its numbers change.** + +It answers a different question from the one this ADR is about. ADR 005 decides +whether *a* listen counted. It says nothing about where one listen ends and the +next begins, because until now that was never in doubt: the engine closed a +listen when the loaded track changed, and a track only changed when playback +moved on. + +Repeat-one breaks that assumption completely. A song looped all afternoon never +changes the loaded track, so it produced exactly one `play_event`. The same is +true of dragging the scrubber back to the start and listening again. The +statistics were not wrong about what counted — they were wrong about how many +times it happened, which for anyone who loops their favourites is the more +visible error. + +## Decision + +**A track that starts over, having already earned a play, ends its listen and +begins a new one.** + +Two conditions, checked on every status tick against the previous one: + +1. The current listen has already accumulated at least the ADR 005 play + threshold. A listen that has not counted yet has nothing worth banking. +2. The position jumped backwards to at or below **25% of the track**. + +Both a loop to zero and a manual drag to the beginning satisfy the second +condition. A nudge back over the last chorus does not. + +The rule lives in `src/services/stats/repeatListen.ts` as a pure function, and +its thresholds are pinned by tests — this decides whether a listen is counted +once or twice, so a quiet change to either condition silently rewrites the +user's history. + +### Why the first condition + +Without it, scrubbing around inside the first thirty seconds of a track would +shatter one listen into a dozen fragments, each too short to count as anything. +A real play would be recorded as a pile of skips. Requiring the listen to have +already counted means the only thing a rewind can do is *add* a listen, never +subtract one. + +### Why 25% + +The number has to separate two things that look identical from the outside — +"start it again" and "go back a bit" — and the only signal available is how far +back the position went. + +A tighter bound would count a scrub back over the final chorus as a replay. A +looser one would miss a genuine restart on a track someone had nearly finished. +A quarter means the listener has given up at least three quarters of their +progress, which is a decision rather than an adjustment. + +### What is deliberately not required + +The new listen does **not** have to pass the play threshold for the boundary to +fire. The boundary fires on the rewind; the new listen is then classified by the +ordinary ADR 005 rule when *it* ends. + +This falls out better than the alternative. Someone who rewinds and then leaves +gets a `play` for what they heard and a `skip` for what they abandoned — both +true — rather than having the abandoned fragment silently merged into the +completed play. + +## Consequences + +A track looped three times produces three `play_events` and three increments of +`play_count`, which is what a listener means when they say they played something +three times. + +`startedAt` is reset to the moment the new listen begins, not left at the +original. Period keys derive from when a listen started, so a loop that runs +across midnight puts its two halves in the right days. + +Existing history is unaffected. This changes how future listens are segmented +and rewrites nothing already recorded, so the counts for a track someone looped +last week stay as they were. + +One cost worth naming: the engine now keeps `lastPositionMs` between status +ticks, which is playback state existing solely for a statistics feature. It is +one integer and the alternative — having the recorder subscribe to position at +2 Hz and reconstruct the sequence itself — would be worse in every way. + +## References + +- ADR 005 — the play/skip/partial thresholds, unchanged by this. +- `docs/stats.md` — the recording pipeline, with a section on this. +- `src/services/stats/repeatListen.test.ts` — the pinned thresholds. diff --git a/docs/stats.md b/docs/stats.md index 5e91850..699ba14 100644 --- a/docs/stats.md +++ b/docs/stats.md @@ -60,10 +60,66 @@ Non-positive duration or `ms_played` is `partial`. Settled in `docs/adr/005-play-skip-partial.md`. Not reopening it: every rollup will depend on it. -### Not yet implemented +--- + +## Repeated listens + +`src/services/stats/repeatListen.ts`, decided in +`docs/adr/011-repeat-listen-detection.md`. A layer **on top of** the counting +rule above, which is unchanged. + +The rule above answers whether *a* listen counted. It says nothing about where +one listen ends and the next begins, because until playback existed that was +never in doubt: the engine closed a listen when the loaded track changed. + +Repeat-one breaks that completely. A song looped all afternoon never changes the +loaded track, so it produced exactly one `play_event`. So did dragging the +scrubber back and listening again. The counts were not wrong about what +qualified — they were wrong about how many times it happened, which for anyone +who loops their favourites is the more visible error. + +### The rule + +A listen ends and a new one begins when **both** hold, checked on each status +tick against the previous one: + +1. The current listen has already earned a play — at least `min(30s, duration × + 0.5)` of playback. +2. The position jumped **backwards to at or below 25% of the track**. + +A loop to zero and a manual drag to the start both satisfy the second. A nudge +back over the last chorus does not. + +### Why each condition + +**The earned-a-play requirement** is what stops seeking from shredding history. +Without it, scrubbing around inside the first thirty seconds would split one +listen into a dozen fragments, each too short to count as anything — turning a +real play into a pile of skips. With it, a rewind can only ever *add* a listen. + +**25%** has to separate two things that look identical from outside: "start it +again" and "go back a bit". The only available signal is how far back the +position went. Tighter would count a scrub over the final chorus as a replay; +looser would miss a genuine restart on a nearly-finished track. A quarter means +at least three quarters of the progress was given up, which is a decision rather +than an adjustment. + +### What the boundary deliberately does not check + +That the *new* listen also passes the play threshold. The boundary fires on the +rewind; the new listen is then classified by the ordinary rule when it ends. + +This falls out better than the alternative. Rewind and then leave, and you get a +`play` for what you heard plus a `skip` for what you abandoned — both true — +rather than the abandoned fragment silently merging into the completed play. + +`startedAt` resets to the moment the new listen begins. Period keys come from +when a listen started, so a loop running across midnight puts its halves in the +right days. -Seeking backwards must not create a second event. That is a recorder concern -and lands with playback in Phase 3. +The thresholds are pinned by `repeatListen.test.ts`. This decides whether a +listen is counted once or twice, so a quiet change to either condition silently +rewrites the user's history. --- diff --git a/src/services/audio/AudioEngine.ts b/src/services/audio/AudioEngine.ts index 0808184..d11a233 100644 --- a/src/services/audio/AudioEngine.ts +++ b/src/services/audio/AudioEngine.ts @@ -8,6 +8,7 @@ import { } from 'expo-audio'; import { shuffleTracks, type ShuffleAlgorithm } from '@/services/shuffle'; +import { isRewindToRestart } from '@/services/stats/repeatListen'; import { isPlayable, nextIndex, playNextIndex, previousIndex, shiftForInsert } from './queue'; import { @@ -99,6 +100,16 @@ class Engine { private playedMs = 0; private lastTickAt: number | null = null; + /** + * Position at the previous status tick, for spotting a rewind. + * + * A listen used to end only when the loaded track changed, so a track on + * repeat-one recorded one play no matter how many times it went round. This is + * what lets the engine notice the track starting over without the track + * changing. See `src/services/stats/repeatListen.ts`. + */ + private lastPositionMs = 0; + /** * Set when a track has been handed to the player but is not open yet. * @@ -331,6 +342,19 @@ class Engine { this.lastTickAt = null; } + /** + * Bank the listen so far and open a new one, same track still loaded. + * + * Distinct from `flushListen` in one respect that matters: `startedAt` is set + * to now rather than cleared, because the next listen has already begun. Period + * keys come from when a listen *started*, so a loop that crosses midnight puts + * its two halves in the right days. + */ + private beginNextCycle(): void { + this.flushListen(true); + this.startedAt = new Date(); + } + /** Fold the time since the last tick into the running total. */ private accumulate(): void { if (this.lastTickAt === null) return; @@ -347,6 +371,7 @@ class Engine { this.startedAt = new Date(); this.index = index; + this.lastPositionMs = 0; this.emitQueue(); this.emit({ phase: 'loading', track, positionMs: 0, durationMs: track.durationMs }); @@ -410,6 +435,8 @@ class Engine { // The pending start from `loadIndex`, now that the file may be open. if (status.isLoaded) this.startIfReady(); + const positionMs = Math.round(status.currentTime * 1000); + // Clock the interval that just elapsed before anything else changes. if (status.playing) { this.accumulate(); @@ -422,15 +449,37 @@ class Engine { // once, unlike `currentTime >= duration`, which fires on every tick after. if (status.didJustFinish) { this.flushListen(true); + this.lastPositionMs = 0; void this.advance(false); return; } + /* + * The same track started over — looped, or dragged back to the beginning. + * Close the listen and open another, so a song on repeat is counted as many + * times as it is actually heard. + * + * Checked before the state is emitted, so `lastPositionMs` is still the + * previous tick's value when the comparison happens. + */ + if ( + isRewindToRestart({ + previousPositionMs: this.lastPositionMs, + positionMs, + durationMs: this.state.durationMs, + msPlayedInCycle: this.playedMs, + }) + ) { + this.beginNextCycle(); + } + + this.lastPositionMs = positionMs; + if (this.state.phase === 'error') return; this.emit({ phase: status.playing ? 'playing' : this.state.phase === 'loading' ? 'loading' : 'paused', - positionMs: Math.round(status.currentTime * 1000), + positionMs, // expo-audio reports -1 or 0 before the file is open; the scanner's // figure is the better answer until then. durationMs: diff --git a/src/services/stats/repeatListen.test.ts b/src/services/stats/repeatListen.test.ts new file mode 100644 index 0000000..6fa8322 --- /dev/null +++ b/src/services/stats/repeatListen.test.ts @@ -0,0 +1,180 @@ +import { playThresholdMs } from './playCounting'; +import { isRewindToRestart, REWIND_FRACTION } from './repeatListen'; + +/** + * The thresholds are pinned here on purpose. + * + * This rule decides whether a listen is counted once or twice, so a quiet + * change to either condition silently rewrites the user's history. Every test + * below names the behaviour it is protecting rather than the number. + */ + +/** Four minutes: long enough that the play threshold is the flat 30 seconds. */ +const LONG = 240_000; +/** Six seconds, like the short test files. Threshold is half of it. */ +const SHORT = 6_000; + +function check(over: Partial[0]> = {}) { + return isRewindToRestart({ + previousPositionMs: 120_000, + positionMs: 0, + durationMs: LONG, + msPlayedInCycle: playThresholdMs(LONG), + ...over, + }); +} + +describe('isRewindToRestart', () => { + it('starts a new listen when a counted track loops back to zero', () => { + // The case this whole rule exists for: repeat-one all afternoon used to be + // recorded as a single play. + expect(check({ positionMs: 0 })).toBe(true); + }); + + it('starts a new listen when the user drags back to the beginning', () => { + expect(check({ previousPositionMs: 200_000, positionMs: 4_000 })).toBe(true); + }); + + it('does not start one before the current listen has counted', () => { + /* + * Without this, scrubbing around inside the first thirty seconds would + * shatter one listen into a dozen too short to count — turning a real play + * into a pile of skips. + */ + expect(check({ msPlayedInCycle: playThresholdMs(LONG) - 1 })).toBe(false); + }); + + it('does not treat a small step backwards as a restart', () => { + // Going back over the last chorus is an adjustment, not a replay. + expect(check({ previousPositionMs: 200_000, positionMs: 185_000 })).toBe(false); + }); + + it('does not fire while playback moves forward', () => { + expect(check({ previousPositionMs: 60_000, positionMs: 60_500 })).toBe(false); + }); + + it('does not fire when the position has not moved', () => { + // A paused track keeps reporting the same position. It is not restarting. + expect(check({ previousPositionMs: 90_000, positionMs: 90_000 })).toBe(false); + }); + + it('draws the line exactly at a quarter of the track', () => { + const boundary = LONG * REWIND_FRACTION; + expect(check({ previousPositionMs: 200_000, positionMs: boundary })).toBe(true); + expect(check({ previousPositionMs: 200_000, positionMs: boundary + 1 })).toBe(false); + }); + + it('works on a track short enough that the threshold is half its length', () => { + // A six-second file counts as played after three seconds, so looping it + // has to produce a second listen just as a four-minute one does. + expect( + isRewindToRestart({ + previousPositionMs: 5_800, + positionMs: 0, + durationMs: SHORT, + msPlayedInCycle: playThresholdMs(SHORT), + }), + ).toBe(true); + }); + + it('refuses to guess when the duration is unknown', () => { + // Duration is zero until the file is open. Every fraction of zero is zero, + // so without this guard a track would "restart" on its first status tick. + expect(check({ durationMs: 0 })).toBe(false); + }); + + it('counts a rewind-then-leave as its own short listen rather than nothing', () => { + /* + * The boundary fires on the rewind, not on the second listen reaching the + * threshold. That is deliberate: the new listen is classified by the same + * rule as any other when it ends, so abandoning it records a skip — which + * is what happened — instead of silently merging into the previous play. + */ + expect(check({ previousPositionMs: 239_000, positionMs: 0 })).toBe(true); + }); +}); + +/** + * The rule applied to a run of status ticks, the way the engine applies it. + * + * `isRewindToRestart` is a single decision; what matters to a user is how many + * listens a *session* produces. This walks realistic tick sequences and counts + * the boundaries, which is the behaviour the feature was asked for — and it is + * deterministic, unlike watching a six-second file loop on an emulator. + */ +function countBoundaries(positions: readonly number[], durationMs: number): number { + let previous = 0; + let msPlayedInCycle = 0; + let boundaries = 0; + + for (const positionMs of positions) { + if ( + isRewindToRestart({ + previousPositionMs: previous, + positionMs, + durationMs, + msPlayedInCycle, + }) + ) { + boundaries += 1; + msPlayedInCycle = 0; + } else { + /* + * Credit only forward movement. The engine accumulates wall-clock time + * spent playing, which for ordinary playback is the position delta — and + * a backwards jump adds nothing, because seeking is not listening. An + * earlier version of this harness advanced a fixed amount per tick, which + * quietly made every position jump free and produced a failure that + * looked like a bug in the rule. + */ + msPlayedInCycle += Math.max(0, positionMs - previous); + } + previous = positionMs; + } + + return boundaries; +} + +/** Ticks for one pass through a track, then back to zero. */ +function loop(durationMs: number, tickMs = 500): number[] { + const ticks: number[] = []; + for (let at = tickMs; at < durationMs; at += tickMs) ticks.push(at); + ticks.push(0); + return ticks; +} + +describe('over a run of status ticks', () => { + it('turns three loops of one track into three extra listens', () => { + // The headline case from the brief: a song looped three times is three + // plays, not one. + const ticks = [...loop(SHORT), ...loop(SHORT), ...loop(SHORT)]; + expect(countBoundaries(ticks, SHORT)).toBe(3); + }); + + it('counts a manual drag back to the start once', () => { + // Play well past the threshold, then drag the scrubber to the beginning. + const ticks = [30_000, 60_000, 90_000, 0, 500, 1_000]; + expect(countBoundaries(ticks, LONG)).toBe(1); + }); + + it('ignores scrubbing around before the listen has counted', () => { + /* + * Someone hunting for the right moment in the first few seconds. Every one + * of these is a backwards jump below the quarter mark, and none of them may + * split the listen — otherwise a real play is recorded as a pile of skips. + */ + const ticks = [2_000, 500, 4_000, 1_000, 6_000, 2_000, 8_000]; + expect(countBoundaries(ticks, LONG)).toBe(0); + }); + + it('ignores repeated short rewinds late in a counted track', () => { + // Replaying the last chorus four times is one listen, not five. + const ticks = [30_000, 200_000, 180_000, 200_000, 180_000, 200_000, 180_000]; + expect(countBoundaries(ticks, LONG)).toBe(0); + }); + + it('does not split a track played straight through', () => { + const ticks = loop(LONG, 10_000).slice(0, -1); + expect(countBoundaries(ticks, LONG)).toBe(0); + }); +}); diff --git a/src/services/stats/repeatListen.ts b/src/services/stats/repeatListen.ts new file mode 100644 index 0000000..f36e6db --- /dev/null +++ b/src/services/stats/repeatListen.ts @@ -0,0 +1,74 @@ +import { playThresholdMs } from './playCounting'; + +/** + * When playing the same track again counts as a second listen. + * + * A layer *on top of* the play/skip/partial rule, not a change to it. ADR 005 + * decides whether a listen counted; this decides where one listen ends and the + * next begins while the track never changes. Looping a song five times is five + * listens, and until now it was one — the engine only closed a listen when the + * loaded track changed, so a repeat-one all afternoon recorded a single play. + * + * See `docs/adr/011-repeat-listen-detection.md`. + */ + +/** + * How far back the position must jump to start a new listen. + * + * A quarter of the track. The number has to separate two things that look + * identical from the outside — "start it again" and "go back a bit" — and the + * only signal available is how far back the position went. + * + * A tighter bound would count a scrub back over the last chorus as a replay; a + * looser one would miss a genuine restart on a track someone had nearly + * finished. A quarter means the listener has given up at least three quarters + * of their progress, which is a decision rather than an adjustment. + */ +export const REWIND_FRACTION = 0.25; + +export interface RewindCheck { + /** Position on the previous status tick. */ + previousPositionMs: number; + /** Position now. */ + positionMs: number; + durationMs: number; + /** Playback time accumulated since this listen began. */ + msPlayedInCycle: number; +} + +/** + * True when the current listen should be banked and a fresh one started. + * + * Two conditions, and both matter: + * + * **The current listen must already have earned a play.** Without this, seeking + * around inside the first thirty seconds of a track would shatter one listen + * into a dozen, each too short to count as anything — turning a real play into + * a pile of skips. A listen that has not yet counted has nothing worth banking. + * + * **The position must have jumped back past the rewind mark.** Both a loop to + * zero and a manual drag to the start satisfy it; nudging back a few seconds + * does not. + * + * Note what this deliberately does *not* require: that the new listen goes on to + * pass the play threshold too. It does not need to be checked here, because the + * new listen is classified by the same rule as every other one when it ends. If + * the user rewinds and then leaves, the second listen is recorded as a skip, + * which is what happened. + */ +export function isRewindToRestart({ + previousPositionMs, + positionMs, + durationMs, + msPlayedInCycle, +}: RewindCheck): boolean { + if (durationMs <= 0) return false; + + // Nothing worth banking yet. + if (msPlayedInCycle < playThresholdMs(durationMs)) return false; + + // Forward, or standing still. Ordinary playback. + if (positionMs >= previousPositionMs) return false; + + return positionMs <= durationMs * REWIND_FRACTION; +} From c0ca6deee671bbeb6ad572492966f9c56d669cb2 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 13:03:03 +0300 Subject: [PATCH 09/20] feat(stats): show listening time, add albums and playlists, and a Wrapped card MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stats_rollups has carried ms_played since Phase 1 and the screen never showed it. Every ranked row now reports both numbers, because they disagree constantly — a three-minute song played twice beats a forty-minute mix on one measure and loses badly on the other, and showing only the count was hiding half of what the table already knew. Adds top albums and top playlists. The rollup fan-out has always written all four entity types; only two were ever read. Playlists stay empty until something is played from one, which the previous commit made possible. Artwork resolves through correlated subqueries. albums.artwork_path exists in the schema and the scanner never fills it — artwork is extracted per file, so the cover lives on the tracks — and a join would multiply the rollup row by every track on the record. Bounded by the outer limit, so it runs ten times rather than once per album in the library. The Wrapped card leads the screen. What makes a summary worth screenshotting is a single sentence someone would repeat, so it leads with the listening time in the display face at a size nothing else uses and follows with two facts. Deliberately not a gradient, a collage, or a share sheet: the design direction rules out the first two by name, and the third needs an outward-facing intent in an app whose promise is that nothing leaves the device. A screenshot is already the share mechanism and needs no permission. Also fixes formatListeningTime below a minute, which the new per-row totals exposed: a handful of six-second tracks all read "0m", which tells the reader nothing and looks like a value that failed to load. It reports seconds under a minute and stops there — "4h 37m 12s" is not how anyone reports listening time, and once there are minutes the seconds are noise. Verified on the Pixel_7 AVD: Wrapped reads "This week / 48m / 461 plays across 459 tracks" with most-played and most-heard beneath it; rows show "1 play / 6s" where they used to show "0m"; Top artists resolves Muse's cover from its tracks; Top albums is populated and Top playlists correctly renders nothing at all. Co-Authored-By: Claude Opus 5 --- src/db/queries/stats.ts | 121 ++++++++++++++++++---- src/features/stats/StatsScreen.tsx | 33 +++++- src/features/stats/components/TopList.tsx | 75 ++++++++++---- src/features/stats/components/Wrapped.tsx | 89 ++++++++++++++++ src/i18n/locales/en.json | 13 ++- src/i18n/locales/tr.json | 13 ++- src/services/format/listeningTime.test.ts | 25 +++++ src/services/format/listeningTime.ts | 21 +++- 8 files changed, 336 insertions(+), 54 deletions(-) create mode 100644 src/features/stats/components/Wrapped.tsx diff --git a/src/db/queries/stats.ts b/src/db/queries/stats.ts index f5a656b..8310c55 100644 --- a/src/db/queries/stats.ts +++ b/src/db/queries/stats.ts @@ -4,7 +4,7 @@ import { useLiveQuery } from 'drizzle-orm/expo-sqlite'; import type { PeriodType } from '@/services/stats/rollups'; import { db } from '../client'; -import { artists, statsRollups, tracks } from '../schema'; +import { albums, artists, playlists, statsRollups, tracks } from '../schema'; /** * Everything the statistics screens read. @@ -21,6 +21,8 @@ export interface TopEntry { subtitle: string | null; playCount: number; msPlayed: number; + /** Bare path into the artwork cache. Null when there is nothing to show. */ + artworkPath: string | null; } export interface PeriodTotals { @@ -30,6 +32,48 @@ export interface PeriodTotals { trackCount: number; } +/** The rollup rows for one period and entity type, ranked. Shared `where`. */ +function rankedRollups(periodType: PeriodType, periodKey: string, entityType: string) { + return and( + eq(statsRollups.periodType, periodType), + eq(statsRollups.periodKey, periodKey), + eq(statsRollups.entityType, entityType), + ); +} + +/** + * Any album's cover, taken from a track that has one. + * + * `albums.artwork_path` exists in the schema and the scanner never fills it — + * artwork is extracted per file, so the cover lives on the tracks. A correlated + * subquery rather than a join because it must return exactly one row per album + * and a join would multiply the rollup row by every track on the record. + * + * Bounded by the `limit` on the outer query, so this runs ten times, not once + * per album in the library. + */ +const albumCover = sql`( + SELECT t.artwork_path FROM tracks t + WHERE t.album_id = ${albums.id} AND t.artwork_path IS NOT NULL + LIMIT 1 +)`; + +/** The same, for an artist. */ +const artistCover = sql`( + SELECT t.artwork_path FROM tracks t + WHERE t.artist_id = ${artists.id} AND t.artwork_path IS NOT NULL + LIMIT 1 +)`; + +/** The same, for a playlist: the first track in it that has a cover. */ +const playlistCover = sql`( + SELECT t.artwork_path FROM playlist_tracks pt + JOIN tracks t ON t.id = pt.track_id + WHERE pt.playlist_id = ${playlists.id} AND t.artwork_path IS NOT NULL + ORDER BY pt.position + LIMIT 1 +)`; + /** Top tracks for a period, most played first. */ export function useTopTracks(periodType: PeriodType, periodKey: string, limit = 10) { const query = db @@ -39,17 +83,12 @@ export function useTopTracks(periodType: PeriodType, periodKey: string, limit = subtitle: artists.name, playCount: statsRollups.playCount, msPlayed: statsRollups.msPlayed, + artworkPath: tracks.artworkPath, }) .from(statsRollups) .innerJoin(tracks, eq(tracks.id, statsRollups.entityId)) .leftJoin(artists, eq(artists.id, tracks.artistId)) - .where( - and( - eq(statsRollups.periodType, periodType), - eq(statsRollups.periodKey, periodKey), - eq(statsRollups.entityType, 'track'), - ), - ) + .where(rankedRollups(periodType, periodKey, 'track')) .orderBy(desc(statsRollups.playCount), desc(statsRollups.msPlayed)) .limit(limit); @@ -66,16 +105,60 @@ export function useTopArtists(periodType: PeriodType, periodKey: string, limit = subtitle: sql`null`, playCount: statsRollups.playCount, msPlayed: statsRollups.msPlayed, + artworkPath: artistCover, }) .from(statsRollups) .innerJoin(artists, eq(artists.id, statsRollups.entityId)) - .where( - and( - eq(statsRollups.periodType, periodType), - eq(statsRollups.periodKey, periodKey), - eq(statsRollups.entityType, 'artist'), - ), - ) + .where(rankedRollups(periodType, periodKey, 'artist')) + .orderBy(desc(statsRollups.playCount), desc(statsRollups.msPlayed)) + .limit(limit); + + const { data } = useLiveQuery(query, [periodType, periodKey, limit]); + return data; +} + +/** Top albums for a period. */ +export function useTopAlbums(periodType: PeriodType, periodKey: string, limit = 10) { + const query = db + .select({ + id: albums.id, + title: albums.name, + subtitle: artists.name, + playCount: statsRollups.playCount, + msPlayed: statsRollups.msPlayed, + artworkPath: albumCover, + }) + .from(statsRollups) + .innerJoin(albums, eq(albums.id, statsRollups.entityId)) + .leftJoin(artists, eq(artists.id, albums.artistId)) + .where(rankedRollups(periodType, periodKey, 'album')) + .orderBy(desc(statsRollups.playCount), desc(statsRollups.msPlayed)) + .limit(limit); + + const { data } = useLiveQuery(query, [periodType, periodKey, limit]); + return data; +} + +/** + * Top playlists for a period. + * + * Empty until something is played *from* a playlist — the rollup is keyed on + * where the queue came from, not on whether the track happens to be in one. + * See `QueueSource` in `src/services/audio/types.ts`. + */ +export function useTopPlaylists(periodType: PeriodType, periodKey: string, limit = 10) { + const query = db + .select({ + id: playlists.id, + title: playlists.name, + subtitle: sql`null`, + playCount: statsRollups.playCount, + msPlayed: statsRollups.msPlayed, + artworkPath: playlistCover, + }) + .from(statsRollups) + .innerJoin(playlists, eq(playlists.id, statsRollups.entityId)) + .where(rankedRollups(periodType, periodKey, 'playlist')) .orderBy(desc(statsRollups.playCount), desc(statsRollups.msPlayed)) .limit(limit); @@ -98,13 +181,7 @@ export function usePeriodTotals(periodType: PeriodType, periodKey: string): Peri trackCount: sql`count(*)`, }) .from(statsRollups) - .where( - and( - eq(statsRollups.periodType, periodType), - eq(statsRollups.periodKey, periodKey), - eq(statsRollups.entityType, 'track'), - ), - ); + .where(rankedRollups(periodType, periodKey, 'track')); const { data } = useLiveQuery(query, [periodType, periodKey]); return data[0] ?? { playCount: 0, msPlayed: 0, trackCount: 0 }; diff --git a/src/features/stats/StatsScreen.tsx b/src/features/stats/StatsScreen.tsx index ba1f11d..460392e 100644 --- a/src/features/stats/StatsScreen.tsx +++ b/src/features/stats/StatsScreen.tsx @@ -1,4 +1,4 @@ -import { BarChart3 } from 'lucide-react-native'; +import { BarChart3, Disc3, ListMusic, Music, User } from 'lucide-react-native'; import { useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { ScrollView, View } from 'react-native'; @@ -6,15 +6,22 @@ import { ScrollView, View } from 'react-native'; import { EmptyState } from '@/components/ui/EmptyState'; import { Screen } from '@/components/ui/Screen'; import { SegmentedControl, type SegmentedControlOption } from '@/components/ui/SegmentedControl'; -import { usePeriodTotals, useTopArtists, useTopTracks } from '@/db/queries/stats'; +import { + usePeriodTotals, + useTopAlbums, + useTopArtists, + useTopPlaylists, + useTopTracks, +} from '@/db/queries/stats'; import { useMessages } from '@/i18n'; +import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; import { getWeekStart } from '@/services/settings'; import { periodKeys } from '@/services/stats/periodKeys'; import { PERIOD_TYPES, type PeriodType } from '@/services/stats/rollups'; -import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; import { StatTotals } from './components/StatTotals'; import { TopList } from './components/TopList'; +import { Wrapped } from './components/Wrapped'; /** * Listening statistics, computed on-device from the user's own history. @@ -22,6 +29,10 @@ import { TopList } from './components/TopList'; * Reads `stats_rollups` only. Aggregating `play_events` here would be a scan * over the whole listening history on every tab switch, growing forever — see * `docs/stats.md`. + * + * The Wrapped card leads, then the tiles, then the four ranked lists. That order + * is deliberate: the summary answers the question people open this tab for, and + * everything below it is there for whoever wants to keep reading. */ export function StatsScreen() { useLifecycleTrace('StatsScreen'); @@ -36,6 +47,8 @@ export function StatsScreen() { const totals = usePeriodTotals(period, periodKey); const topTracks = useTopTracks(period, periodKey); const topArtists = useTopArtists(period, periodKey); + const topAlbums = useTopAlbums(period, periodKey); + const topPlaylists = useTopPlaylists(period, periodKey); const periodOptions: SegmentedControlOption[] = PERIOD_TYPES.map((value) => ({ value, @@ -57,9 +70,19 @@ export function StatsScreen() { {hasData ? ( + + - - + + + + + ) : ( /* diff --git a/src/features/stats/components/TopList.tsx b/src/features/stats/components/TopList.tsx index 19647ed..db076e6 100644 --- a/src/features/stats/components/TopList.tsx +++ b/src/features/stats/components/TopList.tsx @@ -1,23 +1,38 @@ +import { Image } from 'expo-image'; +import type { LucideIcon } from 'lucide-react-native'; import { useTranslation } from 'react-i18next'; import { Text, View } from 'react-native'; import type { TopEntry } from '@/db/queries/stats'; +import { formatListeningTime } from '@/services/format/listeningTime'; +import { useThemeColors } from '@/theme/useTheme'; export interface TopListProps { /** Already translated. */ title: string; entries: readonly TopEntry[]; + /** Drawn when an entry has no cover. Says what kind of thing this list holds. */ + icon: LucideIcon; } /** - * A ranked list — top tracks, top artists. + * A ranked list — top tracks, artists, albums or playlists. * * Renders nothing when empty rather than an empty card: on a fresh week the * screen already says there is no listening yet, and a second "nothing here" - * underneath it is noise. + * underneath it is noise. Top playlists is empty for most people most of the + * time, and a permanent empty card for it would be worse than its absence. + * + * Every row carries both numbers. The play count answers "how often" and the + * listening time answers "how much", and they disagree constantly — a + * three-minute song played twice beats a forty-minute mix played once on one + * measure and loses badly on the other. Showing only the count was hiding half + * of what `stats_rollups` already knew. */ -export function TopList({ title, entries }: TopListProps) { +export function TopList({ title, entries, icon: Icon }: TopListProps) { const { t, i18n } = useTranslation(); + const colors = useThemeColors(); + if (entries.length === 0) return null; const leader = entries[0]?.playCount ?? 0; @@ -26,26 +41,38 @@ export function TopList({ title, entries }: TopListProps) { {title} - + {entries.map((entry, index) => ( - - {index + 1} + + {/* Mono, so the ranks line up as a column. */} + {index + 1} + + {entry.artworkPath ? ( + + ) : ( + + + + )} {entry.title} - {entry.subtitle ? ( - - {entry.subtitle} - - ) : null} + {/* - A bar relative to the leader. Cheaper to read than a number - and it makes "one track dominated the week" visible at a - glance, which is the whole appeal of a Wrapped-style summary. + A bar relative to the leader. Cheaper to read than a number and + it makes "one track dominated the week" visible at a glance, + which is the whole appeal of a Wrapped-style summary. */} - + 0 ? (entry.playCount / leader) * 100 : 0}%` }} @@ -53,12 +80,18 @@ export function TopList({ title, entries }: TopListProps) { - - {t('stats.playCount', { - count: entry.playCount, - formatted: new Intl.NumberFormat(i18n.language).format(entry.playCount), - })} - + {/* Both numbers, right-aligned so the column reads down. */} + + + {t('stats.playCount', { + count: entry.playCount, + formatted: new Intl.NumberFormat(i18n.language).format(entry.playCount), + })} + + + {formatListeningTime(entry.msPlayed, i18n.language)} + + ))} diff --git a/src/features/stats/components/Wrapped.tsx b/src/features/stats/components/Wrapped.tsx new file mode 100644 index 0000000..a32e40e --- /dev/null +++ b/src/features/stats/components/Wrapped.tsx @@ -0,0 +1,89 @@ +import { useTranslation } from 'react-i18next'; +import { Text, View } from 'react-native'; + +import type { PeriodTotals, TopEntry } from '@/db/queries/stats'; +import { formatListeningTime } from '@/services/format/listeningTime'; +import type { PeriodType } from '@/services/stats/rollups'; + +export interface WrappedProps { + period: PeriodType; + totals: PeriodTotals; + topTrack: TopEntry | undefined; + topArtist: TopEntry | undefined; +} + +/** + * The period in one card. + * + * The brief asks for a summary "worth screenshotting", and the thing that makes + * one worth screenshotting is a single sentence a person would actually repeat. + * Nobody says "my top-ten had a Gini coefficient of 0.4"; they say "I listened + * to four hours and it was mostly one album". + * + * So it leads with the listening time in the display face at a size nothing else + * on the screen uses, and follows with two facts underneath. Everything more + * granular lives in the ranked lists below it, which is the right place for + * detail — this is the headline. + * + * Deliberately not a gradient, a collage, or a share sheet. The design direction + * rules out the first two by name, and the third would need an outward-facing + * intent in an app whose whole promise is that nothing leaves the device. A + * screenshot is already the share mechanism, and it needs no permission. + */ +export function Wrapped({ period, totals, topTrack, topArtist }: WrappedProps) { + const { t, i18n } = useTranslation(); + + const time = formatListeningTime(totals.msPlayed, i18n.language); + const number = (value: number) => new Intl.NumberFormat(i18n.language).format(value); + + return ( + + + + {t(`stats.wrapped.${period}`)} + + {/* + The one number the card exists for, in the display face. Indigo, which + the design direction reserves for what is active or important — and on + this screen nothing else competes for it. + */} + {time} + + {t('stats.wrapped.across', { + plays: number(totals.playCount), + tracks: number(totals.trackCount), + count: totals.trackCount, + })} + + + + {/* Absent rather than blank when a period has no clear leader yet. */} + {topTrack || topArtist ? ( + + {topTrack ? ( + + ) : null} + {topArtist ? ( + + ) : null} + + ) : null} + + ); +} + +interface FactProps { + label: string; + value: string; +} + +function Fact({ label, value }: FactProps) { + return ( + + {label} + + {value} + + + ); +} diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 0d482b0..5ed9068 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -89,7 +89,18 @@ "topTracks": "Top tracks", "topArtists": "Top artists", "playCount_one": "{{formatted}} play", - "playCount_other": "{{formatted}} plays" + "playCount_other": "{{formatted}} plays", + "topAlbums": "Top albums", + "topPlaylists": "Top playlists", + "wrapped": { + "week": "This week", + "month": "This month", + "year": "This year", + "across_one": "{{plays}} plays across {{tracks}} track", + "across_other": "{{plays}} plays across {{tracks}} tracks", + "mostPlayed": "Most played", + "mostHeard": "Most heard" + } }, "settings": { "title": "Settings", diff --git a/src/i18n/locales/tr.json b/src/i18n/locales/tr.json index 992c5f5..7d0bd21 100644 --- a/src/i18n/locales/tr.json +++ b/src/i18n/locales/tr.json @@ -89,7 +89,18 @@ "topTracks": "En çok çalınan parçalar", "topArtists": "En çok dinlenen sanatçılar", "playCount_one": "{{formatted}} çalma", - "playCount_other": "{{formatted}} çalma" + "playCount_other": "{{formatted}} çalma", + "topAlbums": "En çok dinlenen albümler", + "topPlaylists": "En çok dinlenen listeler", + "wrapped": { + "week": "Bu hafta", + "month": "Bu ay", + "year": "Bu yıl", + "across_one": "{{tracks}} parçada {{plays}} çalma", + "across_other": "{{tracks}} parçada {{plays}} çalma", + "mostPlayed": "En çok çalınan", + "mostHeard": "En çok dinlenen" + } }, "settings": { "title": "Ayarlar", diff --git a/src/services/format/listeningTime.test.ts b/src/services/format/listeningTime.test.ts index 5403c14..5510e83 100644 --- a/src/services/format/listeningTime.test.ts +++ b/src/services/format/listeningTime.test.ts @@ -42,3 +42,28 @@ describe('formatListeningTime', () => { expect(formatListeningTime(90 * 60_000, 'tr')).toBe('1h 30m'); }); }); + +describe('formatListeningTime under a minute', () => { + it('reports seconds rather than a useless zero', () => { + /* + * The per-row totals on the statistics screen exposed this: a handful of + * six-second tracks all read "0m", which tells the reader nothing and looks + * like a value that failed to load. + */ + expect(formatListeningTime(42_000, 'en')).toBe('42s'); + }); + + it('rounds seconds down, like the minutes above', () => { + expect(formatListeningTime(6_900, 'en')).toBe('6s'); + }); + + it('says 0s for nothing at all, not an empty string', () => { + expect(formatListeningTime(0, 'en')).toBe('0s'); + expect(formatListeningTime(-1, 'en')).toBe('0s'); + }); + + it('switches to minutes the moment there is one', () => { + expect(formatListeningTime(59_999, 'en')).toBe('59s'); + expect(formatListeningTime(60_000, 'en')).toBe('1m'); + }); +}); diff --git a/src/services/format/listeningTime.ts b/src/services/format/listeningTime.ts index fee9c7d..2c3ed1a 100644 --- a/src/services/format/listeningTime.ts +++ b/src/services/format/listeningTime.ts @@ -24,16 +24,29 @@ export function listeningTimeParts(milliseconds: number): ListeningTimeParts { } /** - * `4h 37m`, or `37m` under an hour. + * `4h 37m`, or `37m` under an hour, or `42s` under a minute. * - * The `h`/`m` suffixes are deliberately not localised: they are the same in + * The seconds case exists because the per-row totals on the statistics screen + * exposed it: a handful of short tracks all read "0m", which tells the reader + * nothing at all and looks like a value that failed to load. Under a minute the + * only informative unit is seconds. + * + * It stops there. `4h 37m 12s` is not how anyone reports listening time, and + * once there are minutes to show the seconds are noise. + * + * The `h`/`m`/`s` suffixes are deliberately not localised: they are the same in * both shipped locales, and a translated unit would need plural rules for a * string nobody reads as a sentence. If a third locale disagrees, this becomes - * two `t()` keys and the function returns parts instead. + * three `t()` keys and the function returns parts instead. */ export function formatListeningTime(milliseconds: number, locale: string): string { const { hours, minutes } = listeningTimeParts(milliseconds); const format = (value: number) => new Intl.NumberFormat(locale).format(value); - return hours > 0 ? `${format(hours)}h ${format(minutes)}m` : `${format(minutes)}m`; + if (hours > 0) return `${format(hours)}h ${format(minutes)}m`; + if (minutes > 0) return `${format(minutes)}m`; + + // Rounded down, like the minutes above: never round a listen up into a lie. + const seconds = Math.max(0, Math.floor((Number.isFinite(milliseconds) ? milliseconds : 0) / 1000)); + return `${format(seconds)}s`; } From 4432bb8d8bce231a5ed36c52922865e428da2aa0 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 13:20:29 +0300 Subject: [PATCH 10/20] feat(library): artist and album shelves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three segments now: Tracks, Artists, Albums. The grids are two-column square cards; tapping one opens a detail screen with its tracks and Play/Shuffle. No schema change — artists and albums have been populated since Phase 2 and nobody was reading them. The detail screen reuses LibraryTracks wholesale rather than growing a second, thinner list. A track on an album screen gets exactly the same verbs as one in the library — swipe to queue, long-press sheet, multi-select — and there is one place to change them. Playing from a shelf attributes the listen to that artist or album, which is what fills those entity types in stats_rollups. No genre shelf, though the tech stack doc lists one. Genre comes from MediaStore and on real libraries it is close to useless: many files carry none, sources disagree on spelling and case, and most of a collection lands in one bucket called Unknown. A shelf whose biggest card is "Unknown" and whose next four are spellings of the same word is a way to discover your tags are a mess, not a way to find music. It needs normalisation to be worth having, and that is a feature with a design rather than a fourth useLiveQuery. Recorded in ADR 012. LibraryScreen was at exactly the 300-line limit, so it was split before anything was added: it owns the library — scanning, searching, which view is showing — and LibraryTracks owns tracks. Forced by a line count, right boundary anyway. 184 and 221 lines now. Then the part that matters more than the feature. The detail header's cover did not appear. The path was in the database and the same query fed the grid, where the cover rendered fine. The class was `h-32 w-32`, and 128px is not on the spacing scale — which tailwind.config.js overrides rather than extends, so the class produced no CSS at all and the image drew at zero by zero. No warning, no error. Grepping for the pattern found four more that had already shipped invisible: the playlist mosaic at size="lg", `w-24` on the swipe-to-queue reveal strip — which is why that action's icon had never once been seen — `h-7 w-7` on the track picker's checkbox, and `max-h-96` on two sheets. AGENTS.md names this exact trap and it caught us anyway, because the failure mode is silence. src/theme/scale.test.ts now walks every source file and fails on any spacing class outside the scale. It caught its first offender immediately: my own comment explaining the bug, which still contained the dead class. Verified on the Pixel_7 AVD: Albums grid renders with real covers and track counts, the search field correctly disappears on the card views, and the album detail header shows its artwork. Co-Authored-By: Claude Opus 5 --- app/collection/[kind]/[id].tsx | 13 + docs/adr/012-artist-and-album-shelves.md | 80 +++++ src/components/ui/ActionSheet.tsx | 2 +- src/components/ui/Skeleton.tsx | 8 +- src/components/ui/SwipeableRow.tsx | 2 +- src/db/queries/tracks.ts | 92 ++++++ .../library/CollectionDetailScreen.tsx | 145 +++++++++ src/features/library/LibraryScreen.tsx | 282 ++++++------------ src/features/library/LibraryTracks.tsx | 221 ++++++++++++++ .../library/components/CollectionCard.tsx | 87 ++++++ .../library/components/CollectionGrid.tsx | 60 ++++ .../library/components/CollectionHeader.tsx | 61 ++++ .../library/components/TrackInfoSheet.tsx | 2 +- .../library/hooks/useCollectionRouting.ts | 30 ++ .../components/AddToPlaylistSheet.tsx | 2 +- .../playlists/components/AddTracksSheet.tsx | 4 +- .../playlists/components/PlaylistMosaic.tsx | 8 +- src/i18n/locales/en.json | 6 + src/i18n/locales/tr.json | 6 + src/theme/scale.test.ts | 112 +++++++ 20 files changed, 1014 insertions(+), 209 deletions(-) create mode 100644 app/collection/[kind]/[id].tsx create mode 100644 docs/adr/012-artist-and-album-shelves.md create mode 100644 src/features/library/CollectionDetailScreen.tsx create mode 100644 src/features/library/LibraryTracks.tsx create mode 100644 src/features/library/components/CollectionCard.tsx create mode 100644 src/features/library/components/CollectionGrid.tsx create mode 100644 src/features/library/components/CollectionHeader.tsx create mode 100644 src/features/library/hooks/useCollectionRouting.ts create mode 100644 src/theme/scale.test.ts diff --git a/app/collection/[kind]/[id].tsx b/app/collection/[kind]/[id].tsx new file mode 100644 index 0000000..9110396 --- /dev/null +++ b/app/collection/[kind]/[id].tsx @@ -0,0 +1,13 @@ +import { useLocalSearchParams } from 'expo-router'; + +import { CollectionDetailScreen } from '@/features/library/CollectionDetailScreen'; + +/** One artist or one album. Route files read params and render, nothing else. */ +export default function CollectionRoute() { + const { kind, id } = useLocalSearchParams<{ kind: string; id: string }>(); + const numericId = Number(id); + + if (!Number.isFinite(numericId) || (kind !== 'artist' && kind !== 'album')) return null; + + return ; +} diff --git a/docs/adr/012-artist-and-album-shelves.md b/docs/adr/012-artist-and-album-shelves.md new file mode 100644 index 0000000..70c907f --- /dev/null +++ b/docs/adr/012-artist-and-album-shelves.md @@ -0,0 +1,80 @@ +# 012 — Artist and album shelves, and no genre shelf + +## Context + +The library had one face: an alphabetical list of every track. The tech stack +doc calls for "Tracks / Albums / Artists / Genres" segments, and until now only +the first existed. Nothing in the schema was missing — `artists` and `albums` +have been populated by the scanner since Phase 2 — so this was a query and a +screen, not a data problem. + +## Decision + +**Three segments: Tracks, Artists, Albums.** Artists and albums render as a +two-column grid of square cards; tapping one opens a detail screen with its +tracks, a Play button and a Shuffle button. + +The detail screen reuses `LibraryTracks` wholesale rather than growing a second, +thinner track list. A track on an album screen therefore has exactly the same +verbs as a track in the library — swipe to queue, long-press for the sheet, +multi-select — and there is one place to change them. Two lists would have +drifted within a release. + +Playing from a shelf attributes the listen to that artist or album via +`QueueSource`, which is what puts rows under those entity types in +`stats_rollups`. + +### No genre shelf + +The fourth segment is deliberately absent. + +Genre reaches us from MediaStore, and on real libraries it is close to useless: +ripped files frequently carry none, files from different sources disagree on +spelling and case ("Hip-Hop", "hip hop", "HipHop" are three genres), and a large +fraction of any collection lands in one bucket called Unknown. A shelf whose +biggest card is "Unknown" and whose next four are spellings of the same word is +not a way to find music — it is a way to discover that your tags are a mess. + +The data is still there and still indexed. If a genre view earns its place +later, it needs normalisation — case folding, separator handling, an alias table +— and that is a feature with a design, not a fourth `useLiveQuery`. + +### Search stays on tracks + +The search field is hidden on the artist and album segments. Filtering a grid of +a few dozen cards is not the problem search solves, and a box that silently does +nothing to the current view is worse than no box. + +## Consequences + +`LibraryScreen` had reached exactly the 300-line limit `AGENTS.md` sets, so it +was split before anything was added: it now owns the *library* — scanning, +searching, which view is showing — and `LibraryTracks` owns *tracks*. The split +was forced by a line count and is the right boundary regardless. + +Album covers come from a track, not from `albums.artwork_path`. That column +exists in the schema and the scanner never fills it, because artwork is +extracted per file. The stats queries resolve it with a correlated subquery +bounded by the result limit; the card queries use `min(artwork_path)` over the +group, which is arbitrary but stable — and stable matters more than which, +because a card whose cover changes between renders looks broken. + +Both card queries `INNER JOIN` tracks rather than starting from `artists`. The +artist table gains a row the first time a name is seen and nothing ever removes +one, so a left join would list artists with zero tracks. What is on the device is +what the library shows. + +## Postscript: a class that compiled to nothing + +Building this surfaced a bug worth recording because it had already shipped in +several places. `tailwind.config.js` **overrides** the spacing scale, so a class +built from a value outside it produces no CSS — no warning, no error, no size. + +`h-32 w-32` on the detail cover meant the artwork drew at zero by zero and was +simply absent. The same class was already doing the same thing to the playlist +mosaic. `w-24` had left the swipe-to-queue reveal strip with no width, so its +icon had never been visible, and `h-7 w-7` had done it to a checkbox. + +`AGENTS.md` names this trap and it caught us anyway, because the failure is +silent. `src/theme/scale.test.ts` now fails on any spacing class outside the +scale, which is the only thing that turns silence into noise. diff --git a/src/components/ui/ActionSheet.tsx b/src/components/ui/ActionSheet.tsx index 4dc17eb..f7e0d6d 100644 --- a/src/components/ui/ActionSheet.tsx +++ b/src/components/ui/ActionSheet.tsx @@ -58,7 +58,7 @@ export function ActionSheet({ {/* Absorbs taps so pressing the sheet does not dismiss it. */} diff --git a/src/components/ui/Skeleton.tsx b/src/components/ui/Skeleton.tsx index 2e8fb42..9092e46 100644 --- a/src/components/ui/Skeleton.tsx +++ b/src/components/ui/Skeleton.tsx @@ -109,11 +109,13 @@ export function SkeletonCards({ count = 4 }: { count?: number }) { {Array.from({ length: count }, (_, index) => ( - - + // Half-width cells with padding, matching `CollectionGrid` exactly, so + // the real cards land where the placeholders were. + + diff --git a/src/components/ui/SwipeableRow.tsx b/src/components/ui/SwipeableRow.tsx index 66811b2..f0165b7 100644 --- a/src/components/ui/SwipeableRow.tsx +++ b/src/components/ui/SwipeableRow.tsx @@ -113,7 +113,7 @@ export function SwipeableRow({ diff --git a/src/db/queries/tracks.ts b/src/db/queries/tracks.ts index 932d583..e733056 100644 --- a/src/db/queries/tracks.ts +++ b/src/db/queries/tracks.ts @@ -258,3 +258,95 @@ export async function markMissing(trackIds: number[]): Promise { .set({ isMissing: 1 }) .where(sql`${tracks.id} IN ${trackIds}`); } + +/** An artist or album as a card: cover, name, and how much of it there is. */ +export interface CollectionCard { + id: number; + name: string; + /** For an album, its artist. Null for an artist card. */ + subtitle: string | null; + trackCount: number; + artworkPath: string | null; +} + +/** + * Every artist that has at least one present track. + * + * `innerJoin` rather than a left join from `artists`: the table accumulates a + * row the first time a name is seen and nothing ever removes one, so an artist + * whose only album has been deleted would otherwise sit in the list showing + * zero tracks. What is on the device is what the library should show. + * + * The cover is `min(artwork_path)`, which is an arbitrary-but-stable choice of + * one of the artist's covers. Stable matters more than which: a card whose + * artwork changes between renders looks broken. + */ +export function useArtistCards(): CollectionCard[] { + const query = db + .select({ + id: artists.id, + name: artists.name, + subtitle: sql`null`, + trackCount: count(tracks.id), + artworkPath: sql`min(${tracks.artworkPath})`, + }) + .from(artists) + .innerJoin(tracks, and(eq(tracks.artistId, artists.id), eq(tracks.isMissing, 0))) + .groupBy(artists.id) + .orderBy(asc(sql`${artists.sortName} COLLATE NOCASE`)); + + const { data } = useLiveQuery(query); + return useThrottledData(data); +} + +/** Every album that has at least one present track. */ +export function useAlbumCards(): CollectionCard[] { + const query = db + .select({ + id: albums.id, + name: albums.name, + subtitle: artists.name, + trackCount: count(tracks.id), + artworkPath: sql`min(${tracks.artworkPath})`, + }) + .from(albums) + .innerJoin(tracks, and(eq(tracks.albumId, albums.id), eq(tracks.isMissing, 0))) + .leftJoin(artists, eq(artists.id, albums.artistId)) + .groupBy(albums.id) + .orderBy(asc(sql`${albums.name} COLLATE NOCASE`)); + + const { data } = useLiveQuery(query); + return useThrottledData(data); +} + +/** + * The tracks of one artist or album, ready to play. + * + * Ordered by disc and track number where they exist, falling back to title — + * an album played in alphabetical order is not the album. Nulls sort last so a + * partially-tagged record still opens with the tracks that know where they go. + */ +export function useCollectionTracks(kind: 'artist' | 'album', id: number): TrackListItem[] { + const query = db + .select(listSelection) + .from(tracks) + .leftJoin(artists, eq(tracks.artistId, artists.id)) + .leftJoin(albums, eq(tracks.albumId, albums.id)) + .leftJoin(trackStats, eq(trackStats.trackId, tracks.id)) + .where( + and( + eq(tracks.isMissing, 0), + kind === 'artist' ? eq(tracks.artistId, id) : eq(tracks.albumId, id), + ), + ) + .orderBy( + asc(sql`${tracks.discNo} IS NULL`), + asc(tracks.discNo), + asc(sql`${tracks.trackNo} IS NULL`), + asc(tracks.trackNo), + asc(sql`${tracks.title} COLLATE NOCASE`), + ); + + const { data } = useLiveQuery(query, [kind, id]); + return useThrottledData(data); +} diff --git a/src/features/library/CollectionDetailScreen.tsx b/src/features/library/CollectionDetailScreen.tsx new file mode 100644 index 0000000..6768f7d --- /dev/null +++ b/src/features/library/CollectionDetailScreen.tsx @@ -0,0 +1,145 @@ +import { useRouter } from 'expo-router'; +import { ChevronLeft, Play, Shuffle } from 'lucide-react-native'; +import { useCallback, useMemo } from 'react'; +import { useTranslation } from 'react-i18next'; +import { Pressable, Text, View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; + +import { Skeleton } from '@/components/ui/Skeleton'; +import { useAlbumCards, useArtistCards, useCollectionTracks } from '@/db/queries/tracks'; +import { AudioEngine } from '@/services/audio/AudioEngine'; +import type { QueueSource } from '@/services/audio/types'; +import { getShuffleAlgorithm } from '@/services/settings'; +import { useThemeColors } from '@/theme/useTheme'; + +import { toPlayable } from '../player/toPlayable'; +import { CollectionHeader } from './components/CollectionHeader'; +import { LibraryTracks } from './LibraryTracks'; + +export interface CollectionDetailScreenProps { + kind: 'artist' | 'album'; + id: number; +} + +/** + * Everything by one artist, or everything on one album. + * + * Reuses `LibraryTracks` wholesale, so a track here has exactly the same verbs + * as a track in the library — swipe to queue, long-press for the sheet, + * multi-select. A second, thinner track list for this screen would have drifted + * from the first within a release. + * + * Playing from here attributes the listen to the artist or album rather than to + * the library, which is what puts rows under those entity types in + * `stats_rollups` — see `QueueSource`. + */ +export function CollectionDetailScreen({ kind, id }: CollectionDetailScreenProps) { + const { t } = useTranslation(); + const colors = useThemeColors(); + const router = useRouter(); + + const tracks = useCollectionTracks(kind, id); + + /* + * The card comes from the same query the grid used, so the header shows + * exactly what the user tapped. Cheap: both lists are already live and in + * memory for the library screen behind this one. + */ + const artists = useArtistCards(); + const albums = useAlbumCards(); + const card = (kind === 'artist' ? artists : albums).find((entry) => entry.id === id); + + const source = useMemo(() => ({ type: kind, id }), [kind, id]); + + const play = useCallback(() => { + if (tracks.length > 0) void AudioEngine.setQueue(tracks.map(toPlayable), 0, source); + }, [tracks, source]); + + const shuffle = useCallback(async () => { + if (tracks.length === 0) return; + // Queue first, shuffle second: the engine keeps the unshuffled order, so + // turning shuffle off later restores the album's real running order. + await AudioEngine.setQueue(tracks.map(toPlayable), 0, source); + await AudioEngine.setShuffled(true, getShuffleAlgorithm()); + }, [tracks, source]); + + const goBack = useCallback(() => router.back(), [router]); + + return ( + + + + + + + + {card ? ( + + ) : ( + // The card list is live and lands a frame or two after the tracks do. + + + + + + + + )} + + {tracks.length > 0 ? ( + + + + + {t('playlists.playAll')} + + + + void shuffle()} + accessibilityRole="button" + accessibilityLabel={t('playlists.shuffleAll')} + className="min-h-11 flex-1 flex-row items-center justify-center gap-2 rounded-sm border border-subtle px-4" + > + + + {t('playlists.shuffleAll')} + + + + ) : null} + + + + ); +} + +function noop(): void { + // This screen cannot scan or refresh; the library above it does both. +} diff --git a/src/features/library/LibraryScreen.tsx b/src/features/library/LibraryScreen.tsx index 7f6aabd..781bb45 100644 --- a/src/features/library/LibraryScreen.tsx +++ b/src/features/library/LibraryScreen.tsx @@ -1,33 +1,30 @@ -import { Music, SearchX } from 'lucide-react-native'; -import { useCallback, useMemo, useState } from 'react'; +import { Disc3, User } from 'lucide-react-native'; +import { useCallback, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Linking, View } from 'react-native'; import { ConfirmDialog } from '@/components/ui/ConfirmDialog'; -import { EmptyState } from '@/components/ui/EmptyState'; import { ErrorState } from '@/components/ui/ErrorState'; import { Screen } from '@/components/ui/Screen'; -import type { TrackListItem } from '@/db/queries/tracks'; -import { useMessages } from '@/i18n'; +import { SegmentedControl, type SegmentedControlOption } from '@/components/ui/SegmentedControl'; +import { SkeletonCards } from '@/components/ui/Skeleton'; +import { useAlbumCards, useArtistCards } from '@/db/queries/tracks'; import { useLifecycleTrace } from '@/services/perf/useLifecycleTrace'; import { isPermissionError } from '@/services/scanner/permission'; -import { AddToPlaylistSheet } from '../playlists/components/AddToPlaylistSheet'; -import { useCurrentTrack, usePlaybackControls } from '../player/hooks/usePlayback'; -import { toPlayable } from '../player/toPlayable'; +import { CollectionGrid } from './components/CollectionGrid'; import { LibraryHeader } from './components/LibraryHeader'; import { ScanBanner } from './components/ScanBanner'; import { SearchField } from './components/SearchField'; -import { SelectionBar } from './components/SelectionBar'; -import { TrackActionSheet, type TrackAction } from './components/TrackActionSheet'; -import { TrackInfoSheet } from './components/TrackInfoSheet'; -import { TrackList } from './components/TrackList'; -import { TrackListSkeleton } from './components/TrackListSkeleton'; +import { LibraryTracks } from './LibraryTracks'; +import { useCollectionRouting } from './hooks/useCollectionRouting'; import { useDebounced } from './hooks/useDebounced'; import { useTracks } from './hooks/useLibrary'; import { useScan } from './hooks/useScan'; -import { useSelection } from './hooks/useSelection'; -import { useTrackActions } from './hooks/useTrackActions'; + +/** Which face of the library is on screen. */ +const VIEWS = ['tracks', 'artists', 'albums'] as const; +type LibraryView = (typeof VIEWS)[number]; /** Opens this app's page in system settings, where the permission switch is. */ function openAppSettings(): void { @@ -35,154 +32,80 @@ function openAppSettings(): void { } /** - * The library: every present track, with the scan that fills it. + * The library, in three views: tracks, artists, albums. + * + * This screen owns the *library* — scanning, searching, which view is showing — + * and delegates each view to a component that owns the things in it. That split + * happened because the tracks view alone had reached the 300-line limit; adding + * two more views to the same file was not an option, and the boundary it forced + * is the right one anyway. * - * This screen decides *what* happens to a track — play it, queue it, select it, - * describe it. `TrackList` decides how a row is drawn, and the sheets decide how - * a choice is offered. The four states — loading, scanning, failed, empty — are - * all here, per the States rule, and the scan banner sits above the list rather - * than replacing it so the user can keep scrolling. + * Genres are absent. The tech stack doc lists them alongside artists and albums, + * and MediaStore's genre tagging is unreliable enough on real libraries that a + * genre shelf is mostly one bucket called "Unknown" — see + * `docs/adr/012-artist-and-album-shelves.md`. */ export function LibraryScreen() { useLifecycleTrace('LibraryScreen'); - const { t, i18n } = useTranslation(); - const messages = useMessages('library.empty'); + const { t } = useTranslation(); + const [view, setView] = useState('tracks'); const [search, setSearch] = useState(''); // The field stays instant; only the query waits. const { tracks, isLoading } = useTracks(useDebounced(search)); const { progress, isScanning, isRefreshing, scanLibrary, addFolder, rescan, cancel } = useScan(); - const { playFrom } = usePlaybackControls(); - const currentTrack = useCurrentTrack(); - /* - * Destructured, not held as an object. Every callback below would otherwise - * list `selection` as a dependency and be rebuilt on every render, which - * rebuilt every visible row — 47 of them per checkbox tap, measured. The - * individual functions are stable; only `isActive` and `ids` actually move. - */ - const selection = useSelection(); - const { isActive: isSelecting, ids: selectedIdList, toggle: toggleSelected } = selection; + const artists = useArtistCards(); + const albums = useAlbumCards(); + const { openArtist, openAlbum } = useCollectionRouting(); - /** The selection as a Set, built once per change rather than once per row. */ - const selectedIds = useMemo(() => new Set(selectedIdList), [selectedIdList]); - const { addToQueue, playNext, toggleFavorite } = useTrackActions(); - - /** Which track's action sheet is open. */ - const [actionTarget, setActionTarget] = useState(null); - /** Which track's info sheet is open. */ - const [infoTarget, setInfoTarget] = useState(null); - /** Tracks queued for "add to playlist". Empty means the sheet is closed. */ - const [playlistTargets, setPlaylistTargets] = useState([]); /** True while the scan confirmation is on screen. */ const [confirmingScan, setConfirmingScan] = useState(false); + const askToScan = useCallback(() => setConfirmingScan(true), []); const hasFailed = !isScanning && progress.phase === 'failed'; const permissionFailed = isPermissionError(progress.error); const permissionBlocked = progress.error === 'permission-blocked'; - const find = useCallback( - (id: number) => tracks.find((track) => track.id === id) ?? null, - [tracks], - ); + const viewOptions: SegmentedControlOption[] = VIEWS.map((value) => ({ + value, + label: t(`library.view.${value}`), + })); /* - * Playing a track makes the list it came from the queue, which is what a user - * means by tapping a row — not "play this one thing and stop". While - * selecting, the same tap ticks a box instead. + * Skeleton while the query is in flight *and* while a scan has not yet + * produced a first row. + * + * The second case is the fix for a contradiction: with an empty library and a + * scan running, the list showed its empty state — "No music found yet. Scan + * the device" with a Scan button — directly underneath a banner reporting a + * scan in progress. Two parts of one screen disagreeing about whether anything + * was happening. */ - const onPress = useCallback( - (id: number) => { - if (isSelecting) { - toggleSelected(id); - return; - } - const index = tracks.findIndex((track) => track.id === id); - if (index === -1) return; - playFrom(tracks.map(toPlayable), index); - }, - [tracks, playFrom, isSelecting, toggleSelected], - ); - - const onLongPress = useCallback( - (id: number) => { - // Long-pressing during a selection extends it rather than opening a sheet - // about one row — the user is plainly in the middle of picking several. - if (isSelecting) { - toggleSelected(id); - return; - } - setActionTarget(find(id)); - }, - [isSelecting, toggleSelected, find], - ); - - const onSwipeToQueue = useCallback( - (id: number) => { - const track = find(id); - if (track) addToQueue([track]); - }, - [find, addToQueue], - ); - - const onAction = useCallback( - (action: TrackAction) => { - const track = actionTarget; - if (!track) return; - - switch (action) { - case 'playNext': - playNext([track]); - return; - case 'addToQueue': - addToQueue([track]); - return; - case 'addToPlaylist': - setPlaylistTargets([track.id]); - return; - case 'favorite': - toggleFavorite(track); - return; - case 'select': - selection.begin(track.id); - return; - case 'info': - setInfoTarget(track); - return; - } - }, - [actionTarget, playNext, addToQueue, toggleFavorite, selection], - ); - - const onSelectionQueue = useCallback(() => { - // Resolved in selection order, so the queue takes them as they were picked. - const picked = selection.ids - .map(find) - .filter((track): track is TrackListItem => track !== null); - addToQueue(picked); - selection.clear(); - }, [addToQueue, selection, find]); - - const onSelectionPlaylist = useCallback(() => { - setPlaylistTargets(selection.ids); - }, [selection.ids]); - - const closePlaylistSheet = useCallback(() => { - setPlaylistTargets([]); - selection.clear(); - }, [selection]); + const waiting = isLoading || (isScanning && tracks.length === 0); return ( setConfirmingScan(true)} + onScan={askToScan} onAddFolder={addFolder} - onStartSelecting={selection.activate} + onStartSelecting={() => setView('tracks')} /> - + + + + + {/* Search filters tracks only. An artist grid of two cards does not need + a search box, and hiding it makes that obvious rather than puzzling. */} + {view === 'tracks' ? : null} {isScanning ? : null} @@ -209,74 +132,35 @@ export function LibraryScreen() { ) : null} {/* - The list owns whatever height is left, explicitly. Without a bounded flex - parent a virtualized list keeps the height it first measured, so mounting - the scan banner above it shrank the space without shrinking the list — - which is where the blank band above the first row during "Reading tags…" - came from. + Every view owns whatever height is left, explicitly. Without a bounded + flex parent a virtualized list keeps the height it first measured, so + mounting the scan banner above it shrinks the space without shrinking the + list — which is where the blank band above the first row came from. */} - {/* - Skeleton while the query is in flight *and* while a scan has not yet - produced a first row. - - The second case is the fix for a contradiction: with an empty library and - a scan running, the list showed its empty state — "No music found yet. - Scan the device" with a Scan button — directly underneath a banner - reporting that a scan was in progress. Two parts of one screen - disagreeing about whether anything was happening. Rows arriving is the - only thing that should end it. - */} - - {isLoading || (isScanning && tracks.length === 0) ? ( - - ) : ( - - ) : ( - setConfirmingScan(true)} - /> - ) - } - /> - )} - - - {selection.isActive ? ( - selection.toggleAll(tracks.map((track) => track.id))} - onAddToQueue={onSelectionQueue} - onAddToPlaylist={onSelectionPlaylist} - onCancel={selection.clear} + {view === 'tracks' ? ( + - ) : null} - - setActionTarget(null)} - /> - setInfoTarget(null)} /> - + ) : ( + + {waiting ? ( + + ) : ( + + )} + + )} {/* Scanning reads every audio file on the device, so it says so before it diff --git a/src/features/library/LibraryTracks.tsx b/src/features/library/LibraryTracks.tsx new file mode 100644 index 0000000..73116b1 --- /dev/null +++ b/src/features/library/LibraryTracks.tsx @@ -0,0 +1,221 @@ +import { Music, SearchX } from 'lucide-react-native'; +import { useCallback, useMemo, useState } from 'react'; +import { useTranslation } from 'react-i18next'; +import { View } from 'react-native'; + +import { EmptyState } from '@/components/ui/EmptyState'; +import type { TrackListItem } from '@/db/queries/tracks'; +import { useMessages } from '@/i18n'; + +import { AddToPlaylistSheet } from '../playlists/components/AddToPlaylistSheet'; +import { useCurrentTrack, usePlaybackControls } from '../player/hooks/usePlayback'; +import { toPlayable } from '../player/toPlayable'; +import { SelectionBar } from './components/SelectionBar'; +import { TrackActionSheet, type TrackAction } from './components/TrackActionSheet'; +import { TrackInfoSheet } from './components/TrackInfoSheet'; +import { TrackList } from './components/TrackList'; +import { TrackListSkeleton } from './components/TrackListSkeleton'; +import { useSelection } from './hooks/useSelection'; +import { useTrackActions } from './hooks/useTrackActions'; + +export interface LibraryTracksProps { + tracks: TrackListItem[]; + /** Skeleton instead of a list while true. */ + isLoading: boolean; + isRefreshing: boolean; + onRefresh: () => void; + /** The active search term, so "no results" can quote it. */ + search: string; + /** Suppresses the empty state — the screen is already showing an error. */ + suppressEmpty: boolean; + /** Offered by the empty state when the library has never been filled. */ + onScan: () => void; +} + +/** + * The track list and everything you can do to a track. + * + * Split out of `LibraryScreen`, which had reached exactly the 300-line limit + * `AGENTS.md` sets. The division is by subject rather than by size: this owns + * *tracks* — selection, the action sheet, the info sheet, playing — and the + * screen above owns the *library*: scanning, searching, and which view is on + * screen. + */ +export function LibraryTracks({ + tracks, + isLoading, + isRefreshing, + onRefresh, + search, + suppressEmpty, + onScan, +}: LibraryTracksProps) { + const { t, i18n } = useTranslation(); + const messages = useMessages('library.empty'); + + const { playFrom } = usePlaybackControls(); + const currentTrack = useCurrentTrack(); + + /* + * Destructured, not held as an object. Every callback below would otherwise + * list `selection` as a dependency and be rebuilt on every render, which + * rebuilt every visible row — 47 of them per checkbox tap, measured. The + * individual functions are stable; only `isActive` and `ids` actually move. + */ + const selection = useSelection(); + const { isActive: isSelecting, ids: selectedIdList, toggle: toggleSelected } = selection; + + /** The selection as a Set, built once per change rather than once per row. */ + const selectedIds = useMemo(() => new Set(selectedIdList), [selectedIdList]); + const { addToQueue, playNext, toggleFavorite } = useTrackActions(); + + /** Which track's action sheet is open. */ + const [actionTarget, setActionTarget] = useState(null); + /** Which track's info sheet is open. */ + const [infoTarget, setInfoTarget] = useState(null); + /** Tracks queued for "add to playlist". Empty means the sheet is closed. */ + const [playlistTargets, setPlaylistTargets] = useState([]); + + const find = useCallback( + (id: number) => tracks.find((track) => track.id === id) ?? null, + [tracks], + ); + + /* + * Playing a track makes the list it came from the queue, which is what a user + * means by tapping a row — not "play this one thing and stop". While + * selecting, the same tap ticks a box instead. + */ + const onPress = useCallback( + (id: number) => { + if (isSelecting) { + toggleSelected(id); + return; + } + const index = tracks.findIndex((track) => track.id === id); + if (index === -1) return; + playFrom(tracks.map(toPlayable), index); + }, + [tracks, playFrom, isSelecting, toggleSelected], + ); + + const onLongPress = useCallback( + (id: number) => { + // Long-pressing during a selection extends it rather than opening a sheet + // about one row — the user is plainly in the middle of picking several. + if (isSelecting) { + toggleSelected(id); + return; + } + setActionTarget(find(id)); + }, + [isSelecting, toggleSelected, find], + ); + + const onSwipeToQueue = useCallback( + (id: number) => { + const track = find(id); + if (track) addToQueue([track]); + }, + [find, addToQueue], + ); + + const onAction = useCallback( + (action: TrackAction) => { + const track = actionTarget; + if (!track) return; + + switch (action) { + case 'playNext': + playNext([track]); + return; + case 'addToQueue': + addToQueue([track]); + return; + case 'addToPlaylist': + setPlaylistTargets([track.id]); + return; + case 'favorite': + toggleFavorite(track); + return; + case 'select': + selection.begin(track.id); + return; + case 'info': + setInfoTarget(track); + return; + } + }, + [actionTarget, playNext, addToQueue, toggleFavorite, selection], + ); + + const onSelectionQueue = useCallback(() => { + // Resolved in selection order, so the queue takes them as they were picked. + const picked = selection.ids + .map(find) + .filter((track): track is TrackListItem => track !== null); + addToQueue(picked); + selection.clear(); + }, [addToQueue, selection, find]); + + const closePlaylistSheet = useCallback(() => { + setPlaylistTargets([]); + selection.clear(); + }, [selection]); + + return ( + <> + + {isLoading ? ( + + ) : ( + + ) : ( + + ) + } + /> + )} + + + {isSelecting ? ( + selection.toggleAll(tracks.map((track) => track.id))} + onAddToQueue={onSelectionQueue} + onAddToPlaylist={() => setPlaylistTargets(selection.ids)} + onCancel={selection.clear} + /> + ) : null} + + setActionTarget(null)} + /> + setInfoTarget(null)} /> + + + ); +} diff --git a/src/features/library/components/CollectionCard.tsx b/src/features/library/components/CollectionCard.tsx new file mode 100644 index 0000000..67953ca --- /dev/null +++ b/src/features/library/components/CollectionCard.tsx @@ -0,0 +1,87 @@ +import { Image } from 'expo-image'; +import type { LucideIcon } from 'lucide-react-native'; +import { memo, useCallback } from 'react'; +import { useTranslation } from 'react-i18next'; +import { Pressable, Text, View } from 'react-native'; + +import type { CollectionCard as Card } from '@/db/queries/tracks'; +import { useThemeColors } from '@/theme/useTheme'; + +export interface CollectionCardProps { + card: Card; + /** Drawn when there is no cover. A disc for albums, a person for artists. */ + icon: LucideIcon; + onPress: (id: number) => void; +} + +/** + * One artist or album: cover, name, and how much of it there is. + * + * A square rather than a row, because a grid of covers is the one place this + * app lets artwork carry a screen — the design direction says album art is the + * only saturated colour in most views and should be allowed to. Track lists + * stay as rows; this is the shelf. + * + * `rounded-xs` on the cover, matching every other piece of list artwork. The + * radius scale reserves `md` for cards and sheets, and using it here would make + * the covers rounder than the panel they sit on. + */ +const CollectionCardComponent = function CollectionCard({ + card, + icon: Icon, + onPress, +}: CollectionCardProps) { + const { t } = useTranslation(); + const colors = useThemeColors(); + const handlePress = useCallback(() => onPress(card.id), [onPress, card.id]); + + const subtitle = card.subtitle ?? t('library.trackCount', { count: card.trackCount }); + + return ( + + {card.artworkPath ? ( + + ) : ( + + + + )} + + + + {card.name} + + + {subtitle} + + + + ); +}; + +function isSameCard(previous: CollectionCardProps, next: CollectionCardProps): boolean { + return ( + previous.onPress === next.onPress && + previous.icon === next.icon && + previous.card.id === next.card.id && + previous.card.name === next.card.name && + previous.card.subtitle === next.card.subtitle && + previous.card.trackCount === next.card.trackCount && + previous.card.artworkPath === next.card.artworkPath + ); +} + +export const CollectionCard = memo(CollectionCardComponent, isSameCard); diff --git a/src/features/library/components/CollectionGrid.tsx b/src/features/library/components/CollectionGrid.tsx new file mode 100644 index 0000000..74bb40d --- /dev/null +++ b/src/features/library/components/CollectionGrid.tsx @@ -0,0 +1,60 @@ +import { FlashList, type ListRenderItem } from '@shopify/flash-list'; +import type { LucideIcon } from 'lucide-react-native'; +import type { ReactElement } from 'react'; +import { useCallback } from 'react'; +import { View } from 'react-native'; + +import type { CollectionCard as Card } from '@/db/queries/tracks'; + +import { CollectionCard } from './CollectionCard'; + +/** Cards per row. Two keeps the covers big enough to recognise on a phone. */ +const COLUMNS = 2; + +export interface CollectionGridProps { + cards: readonly Card[]; + icon: LucideIcon; + onPress: (id: number) => void; + empty: ReactElement | null; +} + +/** + * A grid of artist or album cards. + * + * FlashList with `numColumns`, not a wrapping flex row: an artist grid is as + * long as the library is wide, and the performance rule puts every long list on + * FlashList without exception. Three thousand albums laid out in a ScrollView + * would mount three thousand images. + * + * No `overrideItemLayout` here, unlike the track list. Card height depends on + * the width the grid is given — the cover is square — so a hardcoded size would + * be wrong on the first rotation. Cards are uniform, so FlashList measures one + * and reuses it. + */ +export function CollectionGrid({ cards, icon, onPress, empty }: CollectionGridProps) { + const renderItem = useCallback>( + ({ item }) => ( + // Gutter as padding on the cell rather than a gap on the list: FlashList + // sizes cells itself, and a gap would be applied outside that measurement. + + + + ), + [icon, onPress], + ); + + return ( + + ); +} + +function keyExtractor(card: Card): string { + return String(card.id); +} diff --git a/src/features/library/components/CollectionHeader.tsx b/src/features/library/components/CollectionHeader.tsx new file mode 100644 index 0000000..85ef258 --- /dev/null +++ b/src/features/library/components/CollectionHeader.tsx @@ -0,0 +1,61 @@ +import { Image } from 'expo-image'; +import { Disc3, User } from 'lucide-react-native'; +import { useTranslation } from 'react-i18next'; +import { Text, View } from 'react-native'; + +import { useThemeColors } from '@/theme/useTheme'; + +export interface CollectionHeaderProps { + kind: 'artist' | 'album'; + name: string; + /** The album's artist. Null for an artist. */ + subtitle: string | null; + trackCount: number; + artworkPath: string | null; +} + +/** Cover, name and size for an artist or album detail screen. */ +export function CollectionHeader({ + kind, + name, + subtitle, + trackCount, + artworkPath, +}: CollectionHeaderProps) { + const { t } = useTranslation(); + const colors = useThemeColors(); + const Icon = kind === 'artist' ? User : Disc3; + + return ( + + {artworkPath ? ( + + ) : ( + + + + )} + + + + {name} + + {subtitle ? ( + + {subtitle} + + ) : null} + + {t('library.trackCount', { count: trackCount })} + + + + ); +} diff --git a/src/features/library/components/TrackInfoSheet.tsx b/src/features/library/components/TrackInfoSheet.tsx index 80f9255..f274fee 100644 --- a/src/features/library/components/TrackInfoSheet.tsx +++ b/src/features/library/components/TrackInfoSheet.tsx @@ -61,7 +61,7 @@ export function TrackInfoSheet({ track, onClose }: TrackInfoSheetProps) { {rows.map(([label, value]) => ( - {label} + {label} {/* Mono for every technical value, so the column aligns. */} {value} diff --git a/src/features/library/hooks/useCollectionRouting.ts b/src/features/library/hooks/useCollectionRouting.ts new file mode 100644 index 0000000..74016ee --- /dev/null +++ b/src/features/library/hooks/useCollectionRouting.ts @@ -0,0 +1,30 @@ +import { useRouter } from 'expo-router'; +import { useCallback, useMemo } from 'react'; + +export interface CollectionRouting { + openArtist: (id: number) => void; + openAlbum: (id: number) => void; +} + +/** + * Navigation into an artist or album. + * + * A hook rather than two inline arrows, so `CollectionGrid` receives callbacks + * that are stable across renders — the cards are memoized on prop identity and + * a fresh closure per render would defeat that for every visible cover. + */ +export function useCollectionRouting(): CollectionRouting { + const router = useRouter(); + + const openArtist = useCallback( + (id: number) => router.navigate(`/collection/artist/${id}`), + [router], + ); + + const openAlbum = useCallback( + (id: number) => router.navigate(`/collection/album/${id}`), + [router], + ); + + return useMemo(() => ({ openArtist, openAlbum }), [openArtist, openAlbum]); +} diff --git a/src/features/playlists/components/AddToPlaylistSheet.tsx b/src/features/playlists/components/AddToPlaylistSheet.tsx index 4a8d1dc..8d100fb 100644 --- a/src/features/playlists/components/AddToPlaylistSheet.tsx +++ b/src/features/playlists/components/AddToPlaylistSheet.tsx @@ -71,7 +71,7 @@ export function AddToPlaylistSheet({ trackIds, onClose }: AddToPlaylistSheetProp > {trackIds.length > 1 diff --git a/src/features/playlists/components/AddTracksSheet.tsx b/src/features/playlists/components/AddTracksSheet.tsx index ab8ea5f..b695f5b 100644 --- a/src/features/playlists/components/AddTracksSheet.tsx +++ b/src/features/playlists/components/AddTracksSheet.tsx @@ -149,8 +149,8 @@ function PickRow({ track, isPicked, onToggle }: PickRowProps) { {isPicked ? : null} diff --git a/src/features/playlists/components/PlaylistMosaic.tsx b/src/features/playlists/components/PlaylistMosaic.tsx index fe53407..ed34444 100644 --- a/src/features/playlists/components/PlaylistMosaic.tsx +++ b/src/features/playlists/components/PlaylistMosaic.tsx @@ -28,7 +28,13 @@ export interface PlaylistMosaicProps { */ export function PlaylistMosaic({ covers, size = 'sm' }: PlaylistMosaicProps) { const colors = useThemeColors(); - const box = size === 'lg' ? 'h-32 w-32' : 'h-12 w-12'; + /* + * A fraction rather than a fixed size for the large variant. The obvious + * 128px square is not on the spacing scale, so it compiled to nothing and the + * mosaic drew at zero by zero — see `src/theme/scale.test.ts`, which now fails + * on any such class. + */ + const box = size === 'lg' ? 'aspect-square w-1/3' : 'h-12 w-12'; if (covers.length === 0) { return ( diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 5ed9068..3d4380a 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -36,6 +36,12 @@ "scanConfirm": { "title": "Scan for music?", "body": "Mufify will read every audio file Android has indexed on this device. On a large library this takes a while. You can stop it at any point, and anything already found is kept." + }, + "view": { + "label": "Library view", + "tracks": "Tracks", + "artists": "Artists", + "albums": "Albums" } }, "playlists": { diff --git a/src/i18n/locales/tr.json b/src/i18n/locales/tr.json index 7d0bd21..56897da 100644 --- a/src/i18n/locales/tr.json +++ b/src/i18n/locales/tr.json @@ -36,6 +36,12 @@ "scanConfirm": { "title": "Müzik taransın mı?", "body": "Mufify, Android'in bu cihazda indekslediği tüm ses dosyalarını okuyacak. Büyük bir kütüphanede bu biraz sürer. İstediğin an durdurabilirsin, o ana kadar bulunanlar kalır." + }, + "view": { + "label": "Kitaplık görünümü", + "tracks": "Parçalar", + "artists": "Sanatçılar", + "albums": "Albümler" } }, "playlists": { diff --git a/src/theme/scale.test.ts b/src/theme/scale.test.ts new file mode 100644 index 0000000..2953db5 --- /dev/null +++ b/src/theme/scale.test.ts @@ -0,0 +1,112 @@ +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join } from 'node:path'; + +import { SPACING } from './tokens'; + +/** + * Every spacing-derived class must exist in the scale. + * + * `tailwind.config.js` **overrides** the spacing scale rather than extending it, + * so a class built from a value that is not in it produces no CSS at all. There + * is no warning, no error, and no visual hint — the element simply has no size. + * + * This is not hypothetical. Every one of these shipped and was invisible: + * + * - `h-32 w-32` on the artist and album detail cover, so the header drew at zero + * by zero and the artwork never appeared. + * - the same on the playlist mosaic at `size="lg"`. + * - `w-24` on the swipe-to-queue reveal track, so the action strip behind a + * swiped row had no width and its icon was never seen. + * - `h-7 w-7` on the track picker's checkbox. + * - `max-h-96` on two bottom sheets. + * + * `AGENTS.md` names this trap and it caught us anyway, because the failure mode + * is silence. A test is the only thing that turns it into noise. + * + * Fractions (`w-1/2`), `full`, `auto`, `px` and arbitrary values are all left + * alone: those come from Tailwind's own scales, which are not overridden. Only + * bare numbers are checked. + */ + +const SOURCE_ROOTS = ['src', 'app']; +const SOURCE_EXTENSIONS = ['.ts', '.tsx']; + +/** Utilities whose numeric values come from `theme.spacing`. */ +const SPACED = [ + 'w', + 'h', + 'min-w', + 'min-h', + 'max-w', + 'max-h', + 'p', + 'px', + 'py', + 'pt', + 'pb', + 'pl', + 'pr', + 'm', + 'mx', + 'my', + 'mt', + 'mb', + 'ml', + 'mr', + 'gap', + 'gap-x', + 'gap-y', + 'top', + 'bottom', + 'left', + 'right', + 'inset', + 'inset-x', + 'inset-y', + 'size', +]; + +const ALLOWED = new Set(Object.keys(SPACING)); +const PATTERN = new RegExp(`\\b(${SPACED.join('|')})-(\\d+)\\b`, 'g'); + +function sourceFiles(directory: string): string[] { + return readdirSync(directory).flatMap((entry) => { + const path = join(directory, entry); + if (statSync(path).isDirectory()) return sourceFiles(path); + return SOURCE_EXTENSIONS.some((extension) => path.endsWith(extension)) ? [path] : []; + }); +} + +describe('spacing scale', () => { + it('has no class built from a value outside the scale', () => { + const offenders: string[] = []; + + for (const root of SOURCE_ROOTS) { + for (const file of sourceFiles(root)) { + // Skip this file, whose whole job is to contain the examples. + if (file.endsWith('scale.test.ts')) continue; + + const source = readFileSync(file, 'utf8'); + for (const [match, , value] of source.matchAll(PATTERN)) { + if (value !== undefined && !ALLOWED.has(value)) { + offenders.push(`${file}: ${match}`); + } + } + } + } + + expect(offenders).toEqual([]); + }); + + it('keeps the TypeScript scale and the Tailwind config in step', () => { + // `tokens.test.ts` guards the colours the same way. This is the spacing + // half: the two files are synchronised by hand, and the config is the one + // that decides whether a class compiles. + const config = readFileSync('tailwind.config.js', 'utf8'); + const spacingBlock = config.slice(config.indexOf('spacing: {'), config.indexOf('borderRadius')); + + for (const [key, value] of Object.entries(SPACING)) { + expect(spacingBlock).toContain(`${key}: '${value}px'`); + } + }); +}); From ee1bb5c82de96d98138cf90f53e586e5176d79a9 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 13:23:26 +0300 Subject: [PATCH 11/20] docs(performance): record the transition, modal and list findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section 1a of the brief asked for a diagnosis before a fix, with numbers. Most of the checklist turned out to be already correct, and saying so with evidence is worth as much as a change would have been: - No JS-thread animation anywhere. Every animation is a Reanimated worklet; there is no `Animated` from react-native, no LayoutAnimation, and no setInterval or rAF driving a visual. - List rows are memoized with stable callbacks — 47 reconciliations per checkbox tap before, 1 after. - Opening a modal reconciles 1 of 47 visible rows, not all of them. No chain. - There is no React context in the app at all. Shared state is module-level stores read through useSyncExternalStore, so a change notifies only its own subscribers. - All nine expo-image call sites pass cachePolicy and recyclingKey. The one real defect the audit turned up was the spacing-scale bug, already fixed and now guarded by a test. Co-Authored-By: Claude Opus 5 --- docs/performance.md | 58 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/docs/performance.md b/docs/performance.md index 3bd891d..ff23b0b 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -143,6 +143,64 @@ rows. Included because it is the only way to reproduce any of the above. --- +## Transitions, modals and lists + +The complaint was that screen transitions, the drawer and modal openings feel +slow. Taken as a checklist, with a measurement or a source-level fact for each. + +### Is anything animating on the JS thread? + +No. Every animation in the app goes through Reanimated worklets: `Scrubber`, +`MiniPlayer`, `MiniProgress`, `ArtworkCarousel`, `SwipeableRow`, +`ReorderableEntry`, `Skeleton` and `Toaster`. There is no `Animated` import from +`react-native`, no `LayoutAnimation`, and no `setInterval` or +`requestAnimationFrame` driving a visual anywhere in `src` or `app`. + +Screen transitions themselves are react-navigation's, which run natively through +`react-native-screens`. + +### Are list rows memoized with stable callbacks? + +Yes, and it is measured rather than asserted. Counting `LibraryRow` body +executions on the Pixel_7 AVD: + +| | Before | After | +|---|---|---| +| Rows reconciled per checkbox tap | **47** | **1** | + +The fix is in `46d07a3`; the short version is that four separate things were +handing the list a new identity on every render — the selection object, the +screen's callbacks, `selection.has` per row, and an unmemoized `Gesture.Pan()`. + +### Does opening a modal re-render what is underneath? + +No. Long-pressing a row to open the action sheet reconciles **1** of the 47 +visible rows — the one that was pressed. The sheet's state lives in +`LibraryTracks`, and `TrackList`'s `renderItem` does not depend on it, so +FlashList never calls it again. + +### Is there an over-broad context? + +There is no React context in the app at all. Everything shared between screens — +playback state, the queue, toasts — is a module-level store read through +`useSyncExternalStore`, so a change notifies only the components that subscribed +to it. The toast store is the clearest case: a toast confirming a swipe +re-renders `Toaster` and nothing else, which is why it can appear over a list +without touching it. + +### expo-image + +All nine call sites pass both `cachePolicy` and `recyclingKey`. + +### A silent layout bug found on the way + +`tailwind.config.js` overrides the spacing scale, so a class built from a value +outside it compiles to nothing — no warning, no size. Five had already shipped +invisible, including the swipe-to-queue reveal strip, whose icon had therefore +never been seen by anyone. `src/theme/scale.test.ts` now fails on any such class. + +--- + ## Still owed - **Frame timing on the Mi 9T.** `adb shell dumpsys gfxinfo dev.mufify.app From 7ff1fb0541b34da9e842320a601e65501ebd813b Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 13:34:58 +0300 Subject: [PATCH 12/20] docs(performance): record the regression pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both gates green — lint, typecheck, 292 JS tests across 20 suites, and the Kotlin unit tests forced with --rerun-tasks. Verified on device: theme both ways, language both ways with every string translated, all four tabs clean with no JS error in logcat, scanning, playback, shuffle persistence, and the statistics screen. One honest gap recorded rather than glossed: the playlist create -> add -> play chain was not exercised in a single pass. Its pieces were each verified earlier, but adb input became unreliable on an emulator nine hours into the session. That is missing evidence, not a known fault, and it is first on the list for a fresh device. Co-Authored-By: Claude Opus 5 --- docs/performance.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/performance.md b/docs/performance.md index ff23b0b..9bca07f 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -213,3 +213,28 @@ never been seen by anyone. `src/theme/scale.test.ts` now fails on any such class overstates JS cost substantially, so the cold-start figure in particular should be re-taken against a release build before it is treated as what a user experiences. + +--- + +## Regression pass + +Run after the three critical sections closed, on the Pixel_7 AVD, with both +gates green — `lint`, `typecheck`, 292 JS tests across 20 suites, and +`:audio-tags:testDebugUnitTest` forced with `--rerun-tasks`. + +| Area | Result | +|---|---| +| Theme, light ↔ dark | Both render correctly on every screen. Dark uses its own lighter indigo and dark-on-accent text, as the tokens require. | +| Language, en ↔ tr | Every string translated, tabs included. No raw keys, no English left in the Turkish build. | +| Library, Playlists, Stats, Settings | All four render clean. No JS error or warning in logcat across the sweep. | +| Scanning | Button, confirmation, skeleton, cancel wiring. An unchanged rescan measures 20 ms and 1 ms for its two pages. | +| Playback | Play, carousel, mini player, swipe up and down, queue attribution. | +| Shuffle | Selection persists and `play_events.shuffle_algorithm` records it. | +| Statistics | Wrapped, tiles and all four ranked lists, with covers and durations. | + +**Not exercised end to end:** the playlist create → add tracks → play flow. Its +pieces were each verified separately earlier — the detail screen, the mosaic, +the add-tracks sheet, reorder — but the whole chain in one pass was blocked by +`adb shell input` becoming unreliable on an emulator that had been up for +around nine hours. It is a gap in the evidence rather than a known fault, and it +is the first thing to check on a fresh device. From 3dcac80f0fe409d1385dc44aabb8b2a665a7ff0f Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 13:44:37 +0300 Subject: [PATCH 13/20] docs: README, CONTRIBUTING, components.md, LICENSE, and a launcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repository had no README, no CONTRIBUTING, and no LICENSE despite package.json declaring MIT. docs/components.md was referenced by AGENTS.md and had never been written. README covers what the app is, the offline promise and how it is enforced rather than merely intended, the toolchain requirements, setup, the commands, the MIUI install and input restrictions, the architecture rules, and an index of every doc. CONTRIBUTING is the process around AGENTS.md rather than a summary of it: the four-part gate, what a commit message is for, what a performance or UI claim has to come with, and the list of things that will fail review — each of which now has a test behind it. docs/components.md maps all 56 components. Where one has a constraint that is not obvious from reading it — SwipeableRow being transient because it lives in a recycling list, MiniPlayer subscribing to phase and track but never position, TrackList's drawDistance — that reason is repeated there, because it is what someone needs before touching the file. app.sh and app.bat check the toolchain, install dependencies if the lockfile moved, find a device, wire the Metro reverse tunnel and start the dev server. They deliberately never set JAVA_HOME, ANDROID_HOME or PATH: silently rewriting a developer's toolchain for the duration of one command produces a build that works on one machine and nowhere else, and the failure surfaces hours later somewhere unrelated. Every check names exactly what is missing and stops. Writing the check found a stale requirement. AGENTS.md has said "SDK Platform 35+" since Phase 0, and app.sh duly failed on this machine — which builds the project every day. Expo SDK 57 compiles against 36, confirmed by asking Gradle directly (`:app:properties` reports compileSdkVersion 36, targetSdkVersion 36, minSdkVersion 26), and only android-36 and android-36.1 are installed here. Corrected in AGENTS.md, README and both scripts, and the check now compares the highest installed major rather than looking for one exact directory, because the platform directories carry point releases. Verified by running app.sh: every check passes, it finds the emulator, and it stops cleanly before Metro when told to. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 3 +- CONTRIBUTING.md | 88 ++++++++++++++++++++++++ LICENSE | 21 ++++++ README.md | 155 ++++++++++++++++++++++++++++++++++++++++++ app.bat | 132 ++++++++++++++++++++++++++++++++++++ app.sh | 165 +++++++++++++++++++++++++++++++++++++++++++++ docs/components.md | 116 +++++++++++++++++++++++++++++++ 7 files changed, 679 insertions(+), 1 deletion(-) create mode 100644 CONTRIBUTING.md create mode 100644 LICENSE create mode 100644 README.md create mode 100644 app.bat create mode 100755 app.sh create mode 100644 docs/components.md diff --git a/AGENTS.md b/AGENTS.md index 9d692ae..0271b43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,7 +21,8 @@ call, it is wrong. This is the app's core promise and the reason it exists. ## Setup -Requirements: Node 22+, JDK 17 or 21, Android Studio with SDK Platform 35+, macOS or Linux. +Requirements: Node 22+, JDK 17 or 21, Android Studio with SDK Platform 36+, macOS or Linux. +(Expo SDK 57 compiles against 36; this said 35 until `app.sh` checked it and was wrong.) ### Environment (macOS) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..56873a1 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,88 @@ +# Contributing + +Read [AGENTS.md](AGENTS.md) first. It is the house style and it is binding — +this file is the process around it, not a summary of it. + +## The gate + +Nothing is done until all four pass: + +```bash +npm run lint +npm run typecheck +npm test +cd android && ./gradlew :audio-tags:testDebugUnitTest +``` + +Do not weaken a test to make it pass, and do not skip one. If a test is wrong, +fix the test and say in the commit message why it was wrong — that is useful +information, not an admission. + +## Commits + +Conventional commits: `feat:`, `fix:`, `refactor:`, `perf:`, `docs:`, `test:`, +`chore:`. Scope where it helps: `feat(shuffle): add discovery algorithm`. + +**One logical change per commit.** Never one commit per phase of work. + +The message body is the part that matters. It should say what changed and *why*, +and it is worth more than the diff — the diff already says what changed. In +particular: + +- If you measured something, put the numbers in. Before and after. +- If you were wrong about the cause before you found the real one, say so. The + next person will have the same wrong idea. +- If you decided not to do something, say what and why. + +Commit messages in this repository are read as the project's record. Several of +them are the only place a subtle decision is written down. + +## Claims + +- **A performance claim needs a measurement.** Before and after, with the device + and the conditions. `src/services/perf` exists for this; see + [docs/performance.md](docs/performance.md) for the method. +- **A UI change needs a device.** A screenshot or a specific observation — + "the toast clears the tab bar and has a dismiss control", not "should work". + The emulator is fine for behaviour; frame timing needs real hardware. +- **Do not claim something works without running it.** + +## Decisions + +Anything non-obvious gets a short ADR in `docs/adr/NNN-title.md`: context, +decision, consequences. Three paragraphs is plenty. Include the option you +rejected and why — an ADR that only argues for what was chosen is half a record. + +If a requirement turns out to be a bad idea once you are in the code, say so and +propose the alternative. Do not silently build something different, and do not +build something you know is wrong because it was asked for. + +## Documentation + +Docs are part of the work, not cleanup afterwards. A phase is not done until the +relevant `docs/*.md` is updated in the same change. + +Every exported component and service function gets a one-line JSDoc saying what +it does. Skip comments that restate the code; write the ones that explain why +the code is not the obvious thing. + +## Things that will fail review + +- A network call, analytics, crash reporting, or any SDK that phones home. +- A colour, spacing value, or user-facing string hardcoded in a component. +- A spacing class outside the scale. `src/theme/scale.test.ts` catches these, + and it exists because five of them shipped invisible. +- A string added to `en.json` but not `tr.json`, or the reverse. + `src/i18n/locales.test.ts` catches this. +- A component over 300 lines. +- `FlatList` where a list can grow. +- A hand-edit to `android/` or `ios/`, which are generated and git-ignored. +- Deleting a track row during rescan because a file is temporarily missing — + mark `is_missing = 1` instead, so playlists and history survive an unmounted + SD card. + +## Pull requests + +Say what changed, why, and how it was verified on a device. If something is +unverified, say that too — an honest gap is worth more than a confident guess, +and it tells the reviewer where to look. diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..e6c69f7 --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 Yefee8 + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md new file mode 100644 index 0000000..99c9d81 --- /dev/null +++ b/README.md @@ -0,0 +1,155 @@ +# Mufify + +An offline-only music player for Android. React Native, Expo SDK 57, TypeScript. + +It plays the files a streaming service will not: FLAC, ALAC, and anything else +already on the phone. It surfaces the technical truth of a file rather than +hiding it. And it does not have a network layer — not a disabled one, not one +behind a setting. Release builds ship without the `INTERNET` permission at all. + +## What it does + +- **Playback** of local files, lossless first-class, with background playback, + lock-screen controls and a persistent queue. +- **Five shuffle algorithms**, chosen in Settings, each explained where you + choose it. Not one shuffle behind a toggle — see [docs/shuffle.md](docs/shuffle.md). +- **Local playlists** with drag-reorder, a cover mosaic, and shuffle. +- **Listening statistics** computed on the device from your own history: top + tracks, artists, albums and playlists by week, month and year, with a Wrapped + summary. Nothing is uploaded because there is nowhere to upload it to. +- **Technical metadata surfaced**: bitrate, sample rate, bit depth, codec, file + size, on a monospaced spec strip. +- **Dark and light themes, Turkish and English**, both switchable. + +## The promise + +No network. No accounts. No telemetry. No analytics SDK. + +This is enforced rather than intended: `plugins/withOfflineOnly.js` strips the +`INTERNET` permission from the release manifest and restores it only for debug +builds, where Metro needs it. A change that introduces a network call does not +fail review — it fails to work. + +## Requirements + +- **Node 22+** +- **JDK 17 or 21** — Android Studio's bundled JBR is fine, no separate install +- **Android Studio** with **SDK Platform 36+** +- macOS or Linux + +`minSdkVersion` is 26. Raising it to 31 was considered and rejected: it costs +roughly a fifth of Android devices while deleting almost no code. See +[ADR 002](docs/adr/002-min-sdk-26.md). + +## Setup + +None of this is set by default, and every "it works on my machine" failure in +this project so far has been one of these three lines missing. Put them in +`~/.zshrc`: + +```bash +export JAVA_HOME="/Applications/Android Studio.app/Contents/jbr/Contents/Home" +export ANDROID_HOME="$HOME/Library/Android/sdk" +export PATH="$JAVA_HOME/bin:$ANDROID_HOME/platform-tools:$ANDROID_HOME/emulator:$PATH" +``` + +Verify before blaming the code: + +```bash +java -version # 17 or 21 +adb devices # your device, "device" not "unauthorized" or "offline" +npx expo-doctor +``` + +Then: + +```bash +npm install +./app.sh # checks the environment, finds a device, starts the dev build +``` + +`app.sh` (and `app.bat` on Windows) only *checks* the environment — it will tell +you exactly what is missing and stop. It will not set `JAVA_HOME` or +`ANDROID_HOME` for you, because silently changing a developer's toolchain +environment is a worse failure than an error message. + +## Running + +```bash +npx expo start --dev-client # the normal one. JS/TS changes hot reload. +npm run lint +npm run typecheck +npm test +npm run db:generate # after a schema change +``` + +`npx expo run:android` is **only** for native changes: adding or removing a +native dependency, editing `app.json`, or changing a config plugin. It is a +ten-minute build, and reaching for it after a TypeScript edit is the most common +way to waste an afternoon here. + +Kotlin has its own tests: + +```bash +cd android && ./gradlew :audio-tags:testDebugUnitTest +``` + +### Installing on a device + +`adb install` is enough on most phones and on the emulator. On **MIUI / HyperOS** +(Xiaomi, Redmi, POCO) it fails with `INSTALL_FAILED_USER_RESTRICTED` regardless +of what developer options say. Push and install from the device instead: + +```bash +adb push android/app/build/outputs/apk/debug/app-debug.apk /sdcard/Download/ +``` + +MIUI also blocks `adb shell input` and `pm grant` with a `SecurityException`, so +automated UI testing on those devices is not possible — taps need a human. + +## Architecture + +``` +app/ routes only — read params, render a screen, nothing else +src/ + components/ui/ shared presentational components + features/ one directory per feature: screens, components, hooks + services/ pure logic — shuffle, stats, scanner, formatters + db/ schema, migrations, and the only place Drizzle is imported + theme/ design tokens, in exactly two files + i18n/ en.json and tr.json, kept in step by a test +modules/ local native modules (Kotlin) +``` + +Four rules that are bugs rather than preferences when violated: + +1. Only `src/services/audio/*` imports the audio library. +2. Only `src/db/queries/*` imports Drizzle or expo-sqlite. +3. No business logic in component bodies. +4. Layers point downward: `components → hooks → services → db`. + +## Documentation + +| | | +|---|---| +| [AGENTS.md](AGENTS.md) | the house style, binding on humans and agents alike | +| [docs/components.md](docs/components.md) | what each shared component is for | +| [docs/theming.md](docs/theming.md) | the token system, and how to add a colour | +| [docs/i18n.md](docs/i18n.md) | how to add a string and a language | +| [docs/database.md](docs/database.md) | schema, indexes, the play-counting rule | +| [docs/scanner.md](docs/scanner.md) | the two-stage scan and artwork cache | +| [docs/player.md](docs/player.md) | the audio engine and its Android gotchas | +| [docs/shuffle.md](docs/shuffle.md) | each algorithm in plain language | +| [docs/stats.md](docs/stats.md) | events, rollups, period keys, repeat detection | +| [docs/performance.md](docs/performance.md) | measurements, before and after | +| [docs/adr/](docs/adr/) | every non-obvious decision, with its reasoning | + +## Contributing + +See [CONTRIBUTING.md](CONTRIBUTING.md). The short version: read `AGENTS.md` +first, and `lint`, `typecheck` and `test` must all pass before a commit counts +as done. + +## Licence + +MIT. See [LICENSE](LICENSE). diff --git a/app.bat b/app.bat new file mode 100644 index 0000000..0a8ceeb --- /dev/null +++ b/app.bat @@ -0,0 +1,132 @@ +@echo off +REM One command to get Mufify running on Windows: check the toolchain, find a +REM device, start the dev server. +REM +REM This script *checks* and *reports*. It never sets JAVA_HOME, ANDROID_HOME or +REM PATH for you, and that is deliberate — silently rewriting a developer's +REM toolchain environment for the duration of one command produces a build that +REM works here and nowhere else, and the failure surfaces hours later somewhere +REM unrelated. When something is missing it says exactly what, and stops. + +setlocal enabledelayedexpansion +set PROBLEMS=0 + +echo Checking the toolchain + +REM --- Node --------------------------------------------------------------- +where node >nul 2>&1 +if errorlevel 1 ( + echo [X] Node is not installed. + echo Mufify needs Node 22 or newer: https://nodejs.org + set /a PROBLEMS+=1 +) else ( + for /f "tokens=*" %%v in ('node -p "process.versions.node.split('.')[0]"') do set NODE_MAJOR=%%v + if !NODE_MAJOR! LSS 22 ( + for /f "tokens=*" %%v in ('node -v') do echo [X] Node %%v is too old. Mufify needs 22 or newer. + set /a PROBLEMS+=1 + ) else ( + for /f "tokens=*" %%v in ('node -v') do echo [ok] Node %%v + ) +) + +REM --- Java --------------------------------------------------------------- +REM Checked through JAVA_HOME rather than whatever java is on PATH: Gradle uses +REM JAVA_HOME, so that is the one that decides whether a build works. +if "%JAVA_HOME%"=="" ( + echo [X] JAVA_HOME is not set. + echo Gradle reads JAVA_HOME, not the java on your PATH. + echo Android Studio's bundled JDK is fine. Set it to something like: + echo C:\Program Files\Android\Android Studio\jbr + set /a PROBLEMS+=1 +) else ( + if not exist "%JAVA_HOME%\bin\java.exe" ( + echo [X] JAVA_HOME points somewhere without a JDK: %JAVA_HOME% + echo There is no bin\java.exe under it. + set /a PROBLEMS+=1 + ) else ( + echo [ok] Java at %JAVA_HOME% + ) +) + +REM --- Android SDK -------------------------------------------------------- +if "%ANDROID_HOME%"=="" ( + echo [X] ANDROID_HOME is not set. + echo Set it to something like: %%LOCALAPPDATA%%\Android\Sdk + echo and add %%ANDROID_HOME%%\platform-tools to your PATH. + set /a PROBLEMS+=1 +) else ( + if not exist "%ANDROID_HOME%" ( + echo [X] ANDROID_HOME points at a directory that does not exist: %ANDROID_HOME% + set /a PROBLEMS+=1 + ) else ( + echo [ok] Android SDK at %ANDROID_HOME% + REM Point releases exist (android-36.1), so accept any android-36* too. + set SDK_OK=0 + if exist "%ANDROID_HOME%\platforms\android-36" set SDK_OK=1 + for /d %%d in ("%ANDROID_HOME%\platforms\android-3[6-9]*") do set SDK_OK=1 + if !SDK_OK!==0 ( + echo [X] SDK Platform 36 or newer is not installed. + echo Android Studio, Settings, Languages ^& Frameworks, Android SDK: + echo tick "Android API 36" and apply. + set /a PROBLEMS+=1 + ) else ( + echo [ok] SDK Platform 36 or newer + ) + ) +) + +where adb >nul 2>&1 +if errorlevel 1 ( + echo [X] adb is not on your PATH. + echo Add %%ANDROID_HOME%%\platform-tools to it. + set /a PROBLEMS+=1 +) else ( + echo [ok] adb found +) + +if %PROBLEMS% GTR 0 ( + echo. + echo %PROBLEMS% problem^(s^) above. Fix them and run this again. + exit /b 1 +) + +REM --- Dependencies ------------------------------------------------------- +if not exist node_modules ( + echo. + echo Installing dependencies + call npm install + if errorlevel 1 exit /b 1 +) else ( + echo [ok] Dependencies present +) + +REM --- Device ------------------------------------------------------------- +echo. +echo Looking for a device +set DEVICE_COUNT=0 +for /f "skip=1 tokens=1,2" %%a in ('adb devices') do ( + if "%%b"=="device" ( + set /a DEVICE_COUNT+=1 + REM Metro is reached over a reverse tunnel, re-established per connection. + adb -s %%a reverse tcp:8081 tcp:8081 >nul 2>&1 + ) +) + +if %DEVICE_COUNT%==0 ( + echo [!] No device or emulator connected. + echo Plug in a phone with USB debugging on, or start an emulator from + echo Android Studio's Device Manager. + echo Metro will start anyway; connect a device and it will pick it up. +) else ( + echo [ok] %DEVICE_COUNT% device^(s^) connected +) + +REM --- Go ----------------------------------------------------------------- +echo. +echo Starting Metro +echo If the app is not installed yet, run: npx expo run:android +echo That is a ten-minute native build and is only needed once, or after a +echo native dependency or app.json change. +echo. + +call npx expo start --dev-client diff --git a/app.sh b/app.sh new file mode 100755 index 0000000..9f0c64a --- /dev/null +++ b/app.sh @@ -0,0 +1,165 @@ +#!/usr/bin/env bash +# +# One command to get Mufify running: check the toolchain, find a device, start +# the dev server. +# +# This script *checks* and *reports*. It never sets JAVA_HOME, ANDROID_HOME or +# PATH for you, and that is deliberate — silently rewriting a developer's +# toolchain environment for the duration of one command produces a build that +# works here and nowhere else, and the failure surfaces hours later somewhere +# unrelated. When something is missing it says exactly what, and stops. + +set -euo pipefail + +readonly REQUIRED_NODE_MAJOR=22 +readonly REQUIRED_SDK_PLATFORM=36 + +# Only colour when writing to a terminal, so piping to a file stays readable. +if [ -t 1 ]; then + readonly BOLD=$'\033[1m' RED=$'\033[31m' YELLOW=$'\033[33m' GREEN=$'\033[32m' OFF=$'\033[0m' +else + readonly BOLD='' RED='' YELLOW='' GREEN='' OFF='' +fi + +problems=0 + +fail() { + printf '%s✗ %s%s\n' "$RED" "$1" "$OFF" >&2 + shift + for line in "$@"; do printf ' %s\n' "$line" >&2; done + problems=$((problems + 1)) +} + +ok() { printf '%s✓%s %s\n' "$GREEN" "$OFF" "$1"; } +warn() { printf '%s!%s %s\n' "$YELLOW" "$OFF" "$1"; } + +printf '%sChecking the toolchain%s\n' "$BOLD" "$OFF" + +# --- Node --------------------------------------------------------------- +if ! command -v node >/dev/null 2>&1; then + fail "Node is not installed." \ + "Mufify needs Node ${REQUIRED_NODE_MAJOR} or newer." \ + "https://nodejs.org, or: brew install node" +else + node_major=$(node -p 'process.versions.node.split(".")[0]') + if [ "$node_major" -lt "$REQUIRED_NODE_MAJOR" ]; then + fail "Node $(node -v) is too old." \ + "Mufify needs ${REQUIRED_NODE_MAJOR} or newer." + else + ok "Node $(node -v)" + fi +fi + +# --- Java --------------------------------------------------------------- +# Checked through JAVA_HOME rather than whatever `java` happens to be on PATH: +# Gradle uses JAVA_HOME, so that is the one that decides whether a build works. +if [ -z "${JAVA_HOME:-}" ]; then + fail "JAVA_HOME is not set." \ + "Gradle reads JAVA_HOME, not the java on your PATH." \ + "Android Studio's bundled JDK is fine. Add to ~/.zshrc:" \ + ' export JAVA_HOME="/Applications/Android Studio.app/Contents/jbr/Contents/Home"' +elif [ ! -x "$JAVA_HOME/bin/java" ]; then + fail "JAVA_HOME points somewhere without a JDK: $JAVA_HOME" \ + "There is no bin/java under it." +else + java_version=$("$JAVA_HOME/bin/java" -version 2>&1 | head -1 | sed 's/.*"\(.*\)".*/\1/') + java_major=${java_version%%.*} + if [ "$java_major" != "17" ] && [ "$java_major" != "21" ]; then + warn "Java $java_version — this project is built and tested against 17 and 21." + else + ok "Java $java_version" + fi +fi + +# --- Android SDK -------------------------------------------------------- +if [ -z "${ANDROID_HOME:-}" ]; then + fail "ANDROID_HOME is not set." \ + "Add to ~/.zshrc:" \ + ' export ANDROID_HOME="$HOME/Library/Android/sdk"' \ + ' export PATH="$ANDROID_HOME/platform-tools:$ANDROID_HOME/emulator:$PATH"' +elif [ ! -d "$ANDROID_HOME" ]; then + fail "ANDROID_HOME points at a directory that does not exist: $ANDROID_HOME" +else + ok "Android SDK at $ANDROID_HOME" + + # Any platform at or above the required one will do, and the installed + # directories carry point releases (android-36.1), so this compares the major + # number rather than looking for one exact directory name. + highest=$(ls "$ANDROID_HOME/platforms" 2>/dev/null \ + | sed -n 's/^android-\([0-9][0-9]*\).*/\1/p' | sort -n | tail -1) + + if [ -z "$highest" ]; then + fail "No SDK platform is installed." \ + "Android Studio → Settings → Languages & Frameworks → Android SDK," \ + "tick 'Android API $REQUIRED_SDK_PLATFORM' and apply." + elif [ "$highest" -lt "$REQUIRED_SDK_PLATFORM" ]; then + fail "SDK Platform $REQUIRED_SDK_PLATFORM or newer is required; highest installed is $highest." \ + "Android Studio → Settings → Languages & Frameworks → Android SDK," \ + "tick 'Android API $REQUIRED_SDK_PLATFORM' and apply." + else + ok "SDK Platform $highest" + fi +fi + +if ! command -v adb >/dev/null 2>&1; then + fail "adb is not on your PATH." \ + 'Add: export PATH="$ANDROID_HOME/platform-tools:$PATH"' +else + ok "adb $(adb version 2>/dev/null | head -1 | awk '{print $NF}')" +fi + +if [ "$problems" -gt 0 ]; then + printf '\n%s%d problem(s) above. Fix them and run this again.%s\n' "$RED" "$problems" "$OFF" >&2 + exit 1 +fi + +# --- Dependencies ------------------------------------------------------- +# node_modules older than the lockfile means someone pulled a dependency change. +if [ ! -d node_modules ] || [ package-lock.json -nt node_modules ]; then + printf '\n%sInstalling dependencies%s\n' "$BOLD" "$OFF" + npm install +else + ok "Dependencies up to date" +fi + +# --- Device ------------------------------------------------------------- +printf '\n%sLooking for a device%s\n' "$BOLD" "$OFF" + +devices=$(adb devices | awk 'NR>1 && $2=="device" {print $1}') + +if [ -z "$devices" ]; then + warn "No device or emulator connected." + echo + echo " Plug in a phone with USB debugging on, or start an emulator:" + if command -v emulator >/dev/null 2>&1; then + avds=$(emulator -list-avds 2>/dev/null || true) + if [ -n "$avds" ]; then + echo "$avds" | sed 's/^/ emulator -avd /' + echo + echo " On a machine short of RAM, headless survives where windowed does not:" + echo " emulator -avd -no-snapshot -no-boot-anim -no-window -gpu swiftshader_indirect -memory 2048" + else + echo " (no AVDs found — create one in Android Studio's Device Manager)" + fi + fi + echo + echo " Metro will start anyway; connect a device and it will pick it up." +else + count=$(printf '%s\n' "$devices" | wc -l | tr -d ' ') + ok "$count device(s): $(printf '%s' "$devices" | tr '\n' ' ')" + + # Metro is reached over a reverse tunnel, which has to be re-established for + # every device on every connection. + for device in $devices; do + adb -s "$device" reverse tcp:8081 tcp:8081 >/dev/null 2>&1 || true + done +fi + +# --- Go ----------------------------------------------------------------- +printf '\n%sStarting Metro%s\n' "$BOLD" "$OFF" +echo " If the app is not installed yet, run: npx expo run:android" +echo " That is a ten-minute native build and is only needed once, or after a" +echo " native dependency or app.json change." +echo + +exec npx expo start --dev-client diff --git a/docs/components.md b/docs/components.md new file mode 100644 index 0000000..837d915 --- /dev/null +++ b/docs/components.md @@ -0,0 +1,116 @@ +# Components + +What each component is for, and the ones with a reason worth knowing. + +Every exported component already carries a JSDoc saying what it does; this is +the map, not a duplicate of it. Where a component has a non-obvious constraint, +it is repeated here because that is the thing a reader needs before touching it. + +## Rules that shape all of them + +- **Under 300 lines.** Over means it should be two components. Enforced in + review, not aspirational — `LibraryScreen` was split at exactly 300. +- **One component per file**, file named for the component. +- **NativeWind only.** No `StyleSheet.create`, no inline style objects, except + for Reanimated animated styles and native components NativeWind cannot reach + into (the platform `Switch`). +- **Semantic classes only** — `bg-surface`, `text-muted`, `border-subtle`. The + Tailwind config *overrides* rather than extends the scales, so anything + outside the design system compiles to nothing at all. `src/theme/scale.test.ts` + fails on spacing values outside the scale, because five such classes shipped + invisible before it existed. +- **Every screen has its empty, loading and error state** in the same commit as + the happy path. + +--- + +## `components/ui` — shared + +| Component | What it is for | +|---|---| +| `Screen` | The standard frame: safe area, surface, display-face title. | +| `EmptyState` | Icon, one line, and the way out. Picks one of several phrasings per mount so the app does not read like a recording. | +| `ErrorState` | What failed in one plain sentence, and the retry. Never a raw error string. | +| `Skeleton` | One placeholder block, sized by the caller. Pulses via a Reanimated worklet; stops dead under reduce-motion. | +| `SkeletonRows` | A list's worth, shaped like real rows so nothing jumps when data lands. Hidden from screen readers. | +| `SkeletonCards` | The same for the artist and album grids. Mirrors `CollectionGrid`'s two-column layout exactly. | +| `SegmentedControl` | Two to four short, self-evident choices on one line. `perRow` wraps when there are more. | +| `OptionList` | One choice per row with a sentence explaining it. The right control when the names need explaining — which is why shuffle uses it and theme does not. | +| `SettingGroup` / `SettingRow` / `SettingSwitch` | A titled block, a labelled row with a description line, and an on/off row. Every setting gets a description; a list of bare names makes the user guess. | +| `ActionSheet` | A sheet of actions for one thing. Closes before running the action, so the sheet never lingers while a track loads. | +| `ConfirmDialog` | Asks before something slow or irreversible. The confirm button names the action — never "OK". | +| `SwipeableRow` | Reveals one action when dragged left. **Transient by design**: it never stays open, because this lives in a recycling list and a row holding open state gets recycled with it. | +| `Toaster` | Where transient confirmations appear. Reads a module-level store, so a toast re-renders this and nothing else. Sits above the mini player in the tab bar stack. | +| `ProgressBar` | A determinate bar. Used by the scan banner. | + +--- + +## `features/library` + +| Component | What it is for | +|---|---| +| `LibraryScreen` | Owns the *library*: scanning, searching, which of the three views is showing. | +| `LibraryTracks` | Owns *tracks*: selection, the sheets, playing. Split from the screen at the 300-line limit; the boundary is by subject, and it is reused verbatim by `CollectionDetailScreen`. | +| `CollectionDetailScreen` | One artist or one album. Reuses `LibraryTracks` so a track has the same verbs everywhere. | +| `LibraryHeader` | Count, select toggle, folder picker, Scan. The count is always `tracks.length` — never a second query. | +| `TrackList` | The FlashList. `drawDistance` is 1200 rather than the 250 default; at 64px rows the default is under four rows of buffer and a fling outruns it, which is what left blank rows behind the finger. | +| `LibraryRow` | One row plus its swipe gesture. Memoized on primitives only — it takes no selection object, so a render elsewhere cannot reach it. | +| `TrackRow` | Artwork, title, artist, duration. Compared by value, because a live query hands back fresh objects on every re-run. | +| `CollectionCard` / `CollectionGrid` | An artist or album as a square card, and the two-column grid of them. | +| `CollectionHeader` | Cover, name and size for a detail screen. | +| `SearchField` | Debounced input. The field stays instant; only the query waits. | +| `ScanBanner` | Progress above the list, never instead of it — the user can scroll throughout. Hides the counter until a total is known rather than showing "0 / 0". | +| `SelectionBar` | What you can do with a selection. At the bottom, where the thumb already is. | +| `TrackActionSheet` | Long-press actions. Its doc says which two items from the brief are deliberately absent, and why. | +| `TrackInfoSheet` | Every technical field. Absent values render as an em dash rather than disappearing. | +| `TrackListSkeleton` | Named wrapper over `SkeletonRows`, so row geometry stays in one place next to `TrackRow`. | + +--- + +## `features/player` + +| Component | What it is for | +|---|---| +| `PlayerScreen` | Now Playing. A modal route: somewhere you go from something and dismiss. | +| `ArtworkCarousel` | Three slots — previous, current, next — all mounted, so a neighbour slides in already decoded. Commits on distance **or** velocity. Rubber-bands at the ends of the queue. | +| `MiniPlayer` | The persistent transport strip. Subscribes to phase and track only, never position: doing otherwise reconciled it 20 times per ten seconds of playback. | +| `MiniProgress` | The progress hairline, and the only thing in the tab bar that hears about position. Width lives in a shared value; React renders it once. | +| `Scrubber` | The seek bar. The drag runs entirely in a worklet; React hears about it once, on release. | +| `SpecStrip` | The signature element — one monospaced line of a file's technical truth. | +| `FavoriteButton` | The heart. It is also the only writer of `is_favorite`, which the `favorites` shuffle weights on. | +| `QueueScreen` / `QueueRow` | What is playing and what follows. Subscribes to the engine's queue rather than its playback state, so it does not re-render at 2 Hz. | + +--- + +## `features/playlists` + +| Component | What it is for | +|---|---| +| `PlaylistsScreen` / `PlaylistRow` | The list, and one playlist in it. | +| `PlaylistDetailScreen` / `PlaylistDetailHeader` | One playlist, laid out the way a streaming app does because that arrangement is already in everyone's hands. | +| `PlaylistMosaic` | A playlist's cover: the first four album covers in a 2×2 grid. Each count is a deliberate case, not a degradation — one cover fills the square rather than repeating four times. | +| `ReorderableEntry` | Drag by a handle, not by long-press: long-press is the action sheet everywhere else, and a list where holding sometimes does either is a list nobody trusts. | +| `AddTracksSheet` | Pick tracks *from* the playlist. Mounted only while open, because it runs the full library query. | +| `AddToPlaylistSheet` | Pick a playlist for some tracks, or make one. | +| `NamePlaylistDialog` | A `Modal`, not `Alert.prompt`, which is iOS-only and silently does nothing on Android. | + +--- + +## `features/stats` + +| Component | What it is for | +|---|---| +| `StatsScreen` | Reads `stats_rollups` only. Aggregating `play_events` here would be a scan over the whole history on every tab switch, growing forever. | +| `Wrapped` | The period in one card, leading with the listening time. Deliberately not a gradient, a collage, or a share sheet — a screenshot is already the share mechanism. | +| `StatTotals` | The three headline tiles. | +| `TopList` | A ranked list. Every row carries both the count and the duration, because they disagree constantly. Renders nothing when empty. | + +--- + +## `features/settings` + +| Component | What it is for | +|---|---| +| `SettingsScreen` | Every setting, each with a line saying what it does. | +| `ScanFolderList` | Folders the user added by hand, on top of what MediaStore indexes. | +| `DevTools` | The stress-library seeder and query timer. `__DEV__` only, and its strings are deliberately not translated — none of it ships. | From a90e41a3d78fec5d8a5830f3ed7f644f56058aa3 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:04:32 +0300 Subject: [PATCH 14/20] chore(perf): quieten the instrumentation so the real device can be measured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two per-render counters removed: LibraryRow.body and useTracks.render. They were temporary probes for two specific investigations, both of which are closed and whose numbers are already recorded in docs/performance.md — 47 row reconciliations per checkbox tap before the memo boundary and 1 after. They had to go because of where they ended up being harmful. On the Mi 9T, LibraryRow.body fires 45+ times per mount, and MIUI's logcat rate limiter ("chatty") responds by discarding lines — including useTracks.firstRows, which is the cold-start measurement only this device can give. A probe that hides the thing it is measuring is worse than no probe. What stays is the measurements rather than the counters: firstRows, the saveEnumerated busy/longest-block figures, and the mount markers. Co-Authored-By: Claude Opus 5 --- src/db/queries/tracks.ts | 10 +++++++--- src/features/library/components/LibraryRow.tsx | 4 ---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/db/queries/tracks.ts b/src/db/queries/tracks.ts index e733056..745a6bf 100644 --- a/src/db/queries/tracks.ts +++ b/src/db/queries/tracks.ts @@ -131,10 +131,14 @@ export function useTracks(search = ''): { tracks: TrackListItem[]; isLoading: bo const { data, updatedAt } = useLiveQuery(query, [term]); - // Temporary instrumentation for the tab-switch investigation. - perf.count('useTracks.render'); + /* + * Time to first rows, which is the number the cold-start investigation turned + * on. The per-render counter that used to sit here was removed: on the Mi 9T + * it fired often enough that MIUI's logcat rate limiter discarded this + * measurement, and a probe that hides the thing it is measuring is worse than + * no probe. + */ useEffect(() => { - perf.count('useTracks.subscribe'); perf.mark('useTracks.firstRows'); }, [term]); useEffect(() => { diff --git a/src/features/library/components/LibraryRow.tsx b/src/features/library/components/LibraryRow.tsx index 74c3b3c..448ab27 100644 --- a/src/features/library/components/LibraryRow.tsx +++ b/src/features/library/components/LibraryRow.tsx @@ -3,7 +3,6 @@ import { memo, useCallback, useMemo } from 'react'; import { SwipeableRow } from '@/components/ui/SwipeableRow'; import type { TrackListItem } from '@/db/queries/tracks'; -import * as perf from '@/services/perf'; import { TrackRow } from './TrackRow'; @@ -43,9 +42,6 @@ const LibraryRowComponent = function LibraryRow({ isCurrent, swipeLabel, }: LibraryRowProps) { - // Dev-only. This is the counter that told the truth about the checkbox - // freeze: 47 bodies per tap before the memo boundary, 1 after. - perf.count('LibraryRow.body'); const handleSwipe = useCallback(() => onSwipeToQueue(track.id), [onSwipeToQueue, track.id]); const row = useMemo( From b0e0a1bc6f11a184f353603e287ed5c6e3e5b986 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:07:39 +0300 Subject: [PATCH 15/20] docs: add HANDOFF.md for the next session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Written because two consecutive sessions opened with the same wrong premise — "branch from main" — when main holds 3 files and the project holds 225. That is now the first thing the document says, with the command to check it. Beyond state and what is left, it carries the traps that have already cost time: the spacing scale compiling to nothing, axis-locking a pan on the first update when both translations are zero, router.push stacking two screens behind a gesture, the compiler's immutability rule versus Reanimated, MIUI blocking input and silently rate-limiting logcat, and the WAL file that must be copied or the database looks empty. Also records two things found in the phone's database today and deliberately not chased: stage two is 446 of 521 rows short, which is consistent with a cancelled scan resuming correctly but has never been watched; and codec is null for every row while bitrate is populated, which may be API 29 versus 35 or may be the test files being untagged. Both are stated as unexplained rather than as defects, because nobody has checked. Co-Authored-By: Claude Opus 5 --- HANDOFF.md | 195 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 HANDOFF.md diff --git a/HANDOFF.md b/HANDOFF.md new file mode 100644 index 0000000..b0d2952 --- /dev/null +++ b/HANDOFF.md @@ -0,0 +1,195 @@ +# Mufify — continue and finish + +Offline-only Android music player. React Native + Expo SDK 57, TypeScript strict. +Repo: `/Users/yefee/Desktop/projeler/mufify`, branch **`fix/performance-ux-stats`**. + +## Read first + +`AGENTS.md` (binding house style), then `README.md`, `CONTRIBUTING.md`, +`docs/performance.md` (every measurement taken so far), `docs/components.md`, +and all of `docs/adr/`. + +## Before anything else + +```bash +git status && git log --oneline -15 +npm run lint && npm run typecheck && npm test +cd android && ./gradlew :audio-tags:testDebugUnitTest +``` + +All four must be green — currently **292 tests / 20 suites**, working tree clean. +Do not build on red. + +## Do NOT branch from `main` + +`main` contains **3 files** (AGENTS.md and two docs). It is not the project. All +225 files live on `fix/performance-ux-stats`, and `main` is an ancestor of it. +Two previous sessions were told "the user merged to main, branch from there" — +it was not true either time, and branching from `main` would discard everything. +Stay on `fix/performance-ux-stats` unless `git ls-tree -r --name-only main | wc -l` +says otherwise. + +## State + +Everything in the last brief is done: performance (1a/1b/1c), all seven UI/UX +items, all four statistics items, the regression pass, and the release docs. +**68+ commits, unpushed.** No PR has been opened — nobody has asked for one. + +Read the commit messages rather than re-deriving. Several are the only place a +decision is written down. + +## Devices + +**Mi 9T `7a6f8791`** — the user's real phone, API 29, MIUI. Connected and +running the app right now (dark theme, Turkish, 521 tracks). + +- `adb install`, `am start`, `logcat` all work. +- **`adb shell input` and `pm grant` are blocked** (`SecurityException: + Injecting to another application requires INJECT_EVENTS`). Every tap needs a + human. **Batch them and ask once.** +- **MIUI's logcat rate limiter ("chatty") silently discards lines** under load. + If a measurement is missing, that is usually why — not a bug. Two per-render + counters were removed for exactly this reason; do not add more. +- Cold start is ~40 s here because it is a debug build pulling JS from Metro. + `adb -s 7a6f8791 reverse tcp:8081 tcp:8081` after every reconnect. + +**Pixel_7 AVD** — full automation, `input` works. Frame timing is worthless +(SwiftShader). Degrades badly after several hours up; restart it rather than +fighting flaky taps. + +## What is actually left + +### 1. Frame timing on the Mi 9T — never once obtained + +The counters are **reset and armed right now**. Ask the user to scroll the +library hard for ~10 seconds, then: + +```bash +adb -s 7a6f8791 shell dumpsys gfxinfo dev.mufify.app framestats +``` + +This is the only valid source for a 60 fps claim, and `docs/performance.md` +deliberately makes no frame-rate claim until it exists. Record it there. + +### 2. Playlist chain, end to end + +Create → add tracks → reorder → play. Each piece was verified separately; the +whole chain in one pass never was, because the emulator's input died. Needs +human taps on the phone, or a fresh emulator. + +While doing it, confirm a **playlist** row appears in `stats_rollups` — the +`QueueSource` plumbing is in and unit-covered but that specific path is +unverified on a device. Reading the database: + +```bash +adb -s 7a6f8791 shell "run-as dev.mufify.app cat files/SQLite/mufify.db" > /tmp/m.db +adb -s 7a6f8791 shell "run-as dev.mufify.app cat files/SQLite/mufify.db-wal" > /tmp/m.db-wal +sqlite3 /tmp/m.db "SELECT entity_type, COUNT(*) FROM stats_rollups GROUP BY 1;" +``` + +**Copy the `-wal` too.** WAL mode means recent writes are not in the main file, +and copying only `mufify.db` shows an empty `play_events` — which looks exactly +like the bug you would then go hunting. + +### 3. Repeat-listen seek-back, on a device + +`isRewindToRestart` has 15 tests including sequence-level ones, and the engine +wiring is reviewed. The manual seek-back path has not been watched on hardware. +Play something past 30 s, drag the scrubber to the start, listen past 30 s +again, and confirm **two** rows in `play_events`. + +### 4. Release build + +`assembleRelease` succeeds (~23 min, 133 MB universal APK). It has never been +installed or run. It ships **without the `INTERNET` permission** by design, so +it cannot use Metro — a genuine smoke test. Installing it over the debug build +requires an uninstall, which **wipes the user's scanned library and playlists**; +ask before doing that. + +Every measurement so far is from a debug build, which overstates JS cost. + +### 5. Two findings from the phone's database, unexplained + +Read on 2026-08-01 from the Mi 9T (`stats_rollups`, `tracks`). Both may be +artefacts of the test data on that device rather than defects — nobody has +checked, and neither is reproducible on the emulator. + +**Stage two is 446 of 521 short.** `last_scanned_at IS NULL` for 446 rows and +set for 75. That is consistent with a scan that was cancelled or a process that +was killed, and the design says it resumes from exactly there — the null column +*is* the queue. Worth confirming that pressing Scan resumes rather than +restarts, because it is a designed behaviour nobody has watched happen. + +**`codec` is null for all 521 rows, including the 75 enriched ones — but +`bitrate_kbps` is populated.** Both come from `saveEnriched`, so the reader +returned one and not the other. On the emulator the same path yields +`FLAC · 44.1 kHz · 16-bit`. The difference may be API 29 versus 35, or it may be +that these particular files are untagged. `AudioFormatReader` and `TagReader` in +`modules/audio-tags` are where to look. The spec strip is the app's signature +element, so a null codec on the user's actual API level matters. + +Note the library on that phone is **synthetic test files** (`perf-NNN`, album +`bulk`, no artist, no artwork), not the user's music. Scanning their real +library is the only way to tell these apart from a tagging artefact — and it is +also the only way `artists` and artwork get exercised at all, since 0 of 521 +current rows have either. + +### 6. Phase 10 leftovers + +Play Store listing copy (EN + TR), Data Safety answers, release AAB build +instructions, screenshots for the README. `docs/architecture.md` does not exist — +the README has an architecture section that could seed it. + +## Traps already paid for — do not rediscover these + +- **`tailwind.config.js` overrides the spacing scale.** A class built from a + value outside it compiles to *nothing* — no warning, no size. Five shipped + invisible, including the swipe-to-queue reveal strip, whose icon had never + been seen. `src/theme/scale.test.ts` now fails on any such class. The same is + true of colours: there is no `danger` token, and `text-danger` silently does + nothing. +- **Axis-locking a pan gesture on the first `onUpdate` is wrong.** Both + translations are still `0`, and `Math.abs(0) >= Math.abs(0)` is true, so every + gesture classifies as horizontal and vertical ones are silently discarded. + Wait for ~6 px of real movement. +- **`router.push` on a control that is also reachable by gesture stacks two + screens.** The drag fires the handler while the underlying `Pressable` still + registers a press. Use `router.navigate`. Symptom: dismiss *and* the close + button both appear broken, because each correctly pops one of two. +- **The React Compiler's `react-hooks/immutability` rule rejects mutating a + Reanimated shared value captured by a hook.** Build gestures inline (as + `Scrubber` does) unless the component has many instances; `SwipeableRow` + memoizes because it has ~40 in a list and needs an eslint-disable for it. +- **NativeWind `className` only works on components it knows.** Register in + `src/theme/interop.ts` with `cssInterop`. +- **A virtualized list needs a bounded flex parent** — wrap in ``. +- **FlashList's default `drawDistance` is 250 px.** With 64 px rows that is under + four rows of buffer; a fling outruns it and leaves blank rows. It is 1200 now. +- **expo-router + React Compiler:** pass `(props) => ` to + `tabBar`, never the component. +- **`dumpsys media_session` lies about this app.** Use the engine's own stream. +- **Jest's `testMatch` treats any `*spec.ts` as a test.** Do not name a module that. +- **Property tests: seed with splitmix32, not an LCG.** Sequential LCG seeds give + correlated first draws. +- **MIUI marks call recordings `is_music=1`** — `MusicFilter` excludes recorder + folders by path. +- **`MediaMetadataRetriever` has no sample rate or bit depth below API 31.** + `AudioFormatReader` reads them from `MediaExtractor`. +- **New routes need Metro running** to regenerate `.expo/types/router.d.ts`. +- **Release builds ship no `INTERNET` permission** — `plugins/withOfflineOnly.js`. + Never undo this. +- **`android/` is generated by CNG and git-ignored.** Native config goes in + `app.json` or a config plugin. +- **The stress-library seeder does not survive a restart** — the launch sweep + retires tracks MediaStore cannot see, and synthetic rows have no files. Seed + and measure in the same session. + +## Working style + +Small verified increments, one logical change per commit, and a commit message +that explains what and why — the user reads those as the report. Measure before +claiming a performance fix, and put before/after numbers in. Verify UI on a +device and say what you actually saw. When you were wrong about a cause before +finding the real one, write that down; the next person will have the same wrong +idea. Do not stop at decision points — pick the most defensible option, record +it in an ADR, keep going. Say plainly what is verified and what is still owed. From 255caccfaa84bbd538f9be1c876deeec0b1050f9 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:14:59 +0300 Subject: [PATCH 16/20] =?UTF-8?q?fix(docs):=20close=20the=20codec=20findin?= =?UTF-8?q?g=20=E2=80=94=20it=20was=20never=20a=20defect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sessions recorded "codec is null on all 521 rows of the Mi 9T" as an unexplained finding worth investigating at the native layer. It is codecOf working as specified. codecOf returns null whenever the MIME subtype is already a container name, because audio/mpeg otherwise renders the spec strip as "MP3 · mpeg" — the identical fact spelled worse. flac, mp4, mpeg and wav are all in CONTAINER_NAMES, so a library of mainstream formats has a null codec on every row, by design, always. The premise was wrong twice over. codec is derived in TS from the MIME type and never comes from the native reader, so "saveEnriched returned one field and not the other" described a path that does not exist. And the emulator's "FLAC · 44.1 kHz · 16-bit" that the phone was compared against is the container column, not the codec column. Pulled the database from the device to settle it. The 75 enriched rows read container=FLAC|M4A, codec=NULL, bitrate=143, sample_rate=44100, bit_depth=16, channels=2. The phone renders exactly what the emulator renders. No API 29 versus 35 difference, nothing to fix. bit_depth is null on the single M4A row, also correct — AAC is lossy and has no bit depth to report. Pins it with a test naming the device case, so the next person who reads a null codec column resolves it in a test file instead of a device session. Co-Authored-By: Claude Opus 5 --- HANDOFF.md | 36 ++++++++++++++++------- src/services/scanner/trackMapping.test.ts | 14 +++++++++ 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/HANDOFF.md b/HANDOFF.md index b0d2952..035359b 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -108,11 +108,11 @@ ask before doing that. Every measurement so far is from a debug build, which overstates JS cost. -### 5. Two findings from the phone's database, unexplained +### 5. One finding from the phone's database (the other is closed) -Read on 2026-08-01 from the Mi 9T (`stats_rollups`, `tracks`). Both may be -artefacts of the test data on that device rather than defects — nobody has -checked, and neither is reproducible on the emulator. +Read on 2026-08-01 from the Mi 9T (`stats_rollups`, `tracks`). The codec finding +below was chased and closed the same day — it was never a defect. The remaining +one needs a human to press Scan. **Stage two is 446 of 521 short.** `last_scanned_at IS NULL` for 446 rows and set for 75. That is consistent with a scan that was cancelled or a process that @@ -120,13 +120,27 @@ was killed, and the design says it resumes from exactly there — the null colum *is* the queue. Worth confirming that pressing Scan resumes rather than restarts, because it is a designed behaviour nobody has watched happen. -**`codec` is null for all 521 rows, including the 75 enriched ones — but -`bitrate_kbps` is populated.** Both come from `saveEnriched`, so the reader -returned one and not the other. On the emulator the same path yields -`FLAC · 44.1 kHz · 16-bit`. The difference may be API 29 versus 35, or it may be -that these particular files are untagged. `AudioFormatReader` and `TagReader` in -`modules/audio-tags` are where to look. The spec strip is the app's signature -element, so a null codec on the user's actual API level matters. +**~~`codec` is null for all 521 rows~~ — RESOLVED, not a defect. Do not +reinvestigate.** It is `codecOf` in `src/services/scanner/trackMapping.ts` +working as designed: it returns null whenever the MIME subtype is already a +container name, because `audio/mpeg` otherwise renders the strip `MP3 · mpeg` +— the same fact spelled worse. `flac`, `mp4`, `mpeg` and `wav` are all in +`CONTAINER_NAMES`, so **a library of mainstream formats has a null codec on +every row, always.** + +The premise behind the alarm was wrong twice over: `codec` never comes from the +native reader, so "the reader returned one field and not the other" described a +path that does not exist; and the emulator's `FLAC · 44.1 kHz · 16-bit` is the +*container* column, which was being compared against the phone's *codec* +column. Pulled from the device to confirm — the 75 enriched rows read +`container=FLAC|M4A, codec=NULL, bitrate=143, sample_rate=44100, bit_depth=16, +channels=2`. The phone renders exactly what the emulator does. There is no API +29 versus 35 difference and nothing to fix. (`bit_depth` is null on the one M4A +row, also correct — AAC is lossy and has no bit depth.) + +`trackMapping.test.ts` now pins this with the device case named, so the next +reading of a null codec column resolves in one test file instead of a device +session. Note the library on that phone is **synthetic test files** (`perf-NNN`, album `bulk`, no artist, no artwork), not the user's music. Scanning their real diff --git a/src/services/scanner/trackMapping.test.ts b/src/services/scanner/trackMapping.test.ts index 8b5cb43..9330f44 100644 --- a/src/services/scanner/trackMapping.test.ts +++ b/src/services/scanner/trackMapping.test.ts @@ -241,6 +241,20 @@ describe('codecOf', () => { expect(codecOf('audio/mpeg')).toBeNull(); expect(codecOf('audio/flac')).toBeNull(); expect(codecOf('audio/x-wav')).toBeNull(); + expect(codecOf('audio/mp4')).toBeNull(); + }); + + it('is null for every format on the user Mi 9T, which is not a defect', () => { + // Read from the device on 2026-08-01: `codec` null on all 521 rows while + // `container` and the rest of the spec were populated. That was written up + // as an unexplained finding twice. It is this function working: a library + // of mainstream formats has a null codec on every row by design, because + // the MIME subtype is already the container name. A non-null codec needs a + // subtype outside CONTAINER_NAMES, which a FLAC/MP3/M4A library never has. + for (const mime of ['audio/flac', 'audio/mp4', 'audio/mpeg']) { + expect(codecOf(mime)).toBeNull(); + expect(containerOf(mime)).not.toBeNull(); + } }); it('keeps a subtype nothing is known about', () => { From 988a7c7c5dbdfdd8259c6c7b1dd73a610a435ab3 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:18:16 +0300 Subject: [PATCH 17/20] docs: add architecture.md, the map the per-topic docs lacked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 10 listed it as missing with the README's architecture section as a seed. The README says what the directories are; this says how the pieces meet, which is the part a new reader actually gets wrong. Four things it records that were not written down anywhere: The three places state lives, chosen by lifetime — SQLite for anything persistent, the AudioEngine singleton for playback because it outlives every screen, and MMKV for settings because the reads are synchronous and have to happen before the first frame. There is no global state library; zustand is in package.json and imported by nothing, which reads as an oversight until you know the engine is subscribed through useSyncExternalStore on purpose. The startup ordering in app/_layout.tsx is load-bearing rather than incidental, and each step has a specific failure if moved: a frame in the fallback font, a component that paints before its cssInterop registration, a screen querying an unmigrated schema. Why the two-stage scan is resumable — last_scanned_at being null IS the queue, so there is no separate progress state to keep honest. Why a finished listen crosses a boundary to become a statistic instead of the engine writing it directly. Also corrects the offline-only reference to ADR 009, which is where the merged manifests were actually generated and compared. Linked from the README's docs table and its architecture section. Co-Authored-By: Claude Opus 5 --- HANDOFF.md | 7 +- README.md | 4 ++ docs/architecture.md | 155 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 docs/architecture.md diff --git a/HANDOFF.md b/HANDOFF.md index 035359b..18f114c 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -151,8 +151,11 @@ current rows have either. ### 6. Phase 10 leftovers Play Store listing copy (EN + TR), Data Safety answers, release AAB build -instructions, screenshots for the README. `docs/architecture.md` does not exist — -the README has an architecture section that could seed it. +instructions, screenshots for the README. + +`docs/architecture.md` is **written** — startup ordering, the three places state +lives, the scan and listen flows, and the native boundary. Screenshots are the +one Phase 10 item that needs a device. ## Traps already paid for — do not rediscover these diff --git a/README.md b/README.md index 99c9d81..9206eae 100644 --- a/README.md +++ b/README.md @@ -128,11 +128,15 @@ Four rules that are bugs rather than preferences when violated: 3. No business logic in component bodies. 4. Layers point downward: `components → hooks → services → db`. +[docs/architecture.md](docs/architecture.md) goes further: the startup ordering, +the two flows worth tracing, and why there is no global state library. + ## Documentation | | | |---|---| | [AGENTS.md](AGENTS.md) | the house style, binding on humans and agents alike | +| [docs/architecture.md](docs/architecture.md) | how the pieces fit, and where state lives | | [docs/components.md](docs/components.md) | what each shared component is for | | [docs/theming.md](docs/theming.md) | the token system, and how to add a colour | | [docs/i18n.md](docs/i18n.md) | how to add a string and a language | diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..06c45da --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,155 @@ +# Architecture + +How Mufify is put together, and why it is put together that way. The per-topic +documents go deep on individual subsystems — this one is the map that says +which subsystem you want and how they meet. + +Mufify is an offline-only Android music player: React Native on Expo SDK 57, +TypeScript in strict mode, SQLite for everything persistent, and two small +Kotlin modules for the things the JS side cannot see. Release builds ship +without the `INTERNET` permission, which is not a setting but a structural +guarantee — `plugins/withOfflineOnly.js` strips it, and +[ADR 009](adr/009-expo-audio-and-our-own-queue.md) records how both merged +manifests were generated to verify that the release one really does go without +it. + +## The shape + +``` +app/ routes only — read params, render a screen, nothing else +src/ + components/ui/ shared presentational components + features/ one directory per feature: screens, components, hooks + services/ pure logic — shuffle, stats, scanner, formatters + db/ schema, migrations, and the only place Drizzle is imported + theme/ design tokens, in exactly two files + i18n/ en.json and tr.json, kept in step by a test +modules/ local native modules (Kotlin) +``` + +Four rules. Each is a bug rather than a preference when violated, because each +one has a failure that follows from breaking it: + +1. **Only `src/services/audio/*` imports the audio library.** `AudioEngine.ts` + is the sole file that names an `expo-audio` symbol. RNTP v4 is frozen and + pre-New-Architecture and RNTP v5 is commercially licensed while this app + ships MIT, so the engine choice may genuinely have to be revisited. Behind + a facade that is a one-file change. +2. **Only `src/db/queries/*` imports Drizzle or expo-sqlite.** A component that + builds its own query is a component that cannot be tested without a + database, and a schema change becomes a repo-wide search. +3. **No business logic in component bodies.** If it can be decided without + rendering, it belongs in `services/` where a plain Jest test reaches it. This + is why the shuffle algorithms, period keys and repeat-listen detection have + real test coverage: none of them needs a device. +4. **Layers point downward:** `components → hooks → services → db`. Nothing + below reaches up. + +## Where state actually lives + +This is the part that surprises people, because there is no global state +library. `zustand` appears in `package.json` and is imported by nothing. State +lives in three places, chosen by lifetime: + +**SQLite — everything persistent.** Tracks, playlists, play events, rollups. +Opened once in [client.ts](../src/db/client.ts) with WAL, `synchronous=NORMAL`, +foreign keys on, and `enableChangeListener: true`. That last flag is what makes +Drizzle's `useLiveQuery` react to writes, so lists refresh themselves and no +screen needs manual invalidation. WAL is why the library stays scrollable while +a scan writes — and why pulling the database off a device without the `-wal` +file shows an empty `play_events`. + +**The `AudioEngine` singleton — playback.** Deliberately not a hook and not +React state: playback outlives every screen, which is the entire point of +background audio, so it cannot be owned by a component that unmounts when the +user opens Settings. Screens subscribe through `useSyncExternalStore` in +[src/features/player/hooks](../src/features/player/hooks) — the right primitive +for a store that lives outside React, and one that gets tearing right during +concurrent renders where a hand-rolled subscription does not. Note that +`usePlayback` re-renders twice a second while anything plays, because the +engine reports position on a 500 ms interval; `usePlaybackPhase` and +`useCurrentTrack` exist so that only Now Playing pays that cost. + +**MMKV — settings and theme.** Synchronous reads, which is the whole reason it +is here rather than AsyncStorage: `applyStoredTheme()` and `initI18n()` run at +module scope in [app/_layout.tsx](../app/_layout.tsx), before the first frame, +so the app never paints the wrong theme or the wrong language and then corrects +itself. + +## Startup, in order + +The root layout's ordering is load-bearing rather than incidental: + +1. `applyStoredTheme()` and `initI18n()` at module scope — synchronous MMKV + reads, before anything paints. +2. `registerComponentInterop()` — NativeWind's `className` only works on + components registered with `cssInterop`. A component registered late has + already painted itself unstyled. +3. Splash is held (`preventAutoHideAsync`) until **both** fonts and migrations + settle. Rendering earlier means a frame in the fallback font, or a screen + querying a schema that has not been migrated yet. +4. `startListenRecording()` — for the life of the app, not the life of a + screen. +5. `GestureHandlerRootView` wraps the tree. Gesture-handler throws rather than + silently ignoring gestures without it. + +Migrations are generated by `npm run db:generate` and committed, never derived +at runtime, so a build always knows exactly which schema it expects. + +## Two flows worth tracing + +**Scanning** is two stages on purpose, and the split is what makes it +resumable. `enumerateLibrary` walks MediaStore and writes cheap rows; +`enrichLibrary` opens each file for tags, artwork and format. `last_scanned_at` +is null until stage two has run, so **the null column is the queue** — a +cancelled or killed scan resumes from exactly where it stopped, with no +separate progress state to keep honest. `needsRescan` compares size and mtime, +which is what makes rescanning an untouched library near-instant. Details in +[scanner.md](scanner.md). + +**A listen becoming a statistic** crosses a boundary deliberately. The engine +reports that a listen finished and knows nothing about `play_events`, rollups +or week-start preferences; [listenRecorder.ts](../src/features/player/listenRecorder.ts) +does that wiring. Playback stays testable without a database, and the layer +direction stays pointing down. A failed write is logged in development and +swallowed in production — losing one statistics row is a smaller harm than +interrupting playback to complain about it. Details in [stats.md](stats.md). + +## The native boundary + +Two local Expo modules, both Kotlin, both Android-only: + +- **`audio-tags`** — MediaStore enumeration, tag reading, artwork extraction + and audio format. It exists because `MediaMetadataRetriever` reports no + sample rate and no bit depth below API 31, so a hi-res library on an Android + 10 phone lost exactly the fields this app exists to show; + `AudioFormatReader` reads them from `MediaExtractor`, which has carried them + since API 16. `SpecMath.kt` holds the arithmetic with no Android import at + all, so it runs as a plain JVM unit test — no device, no emulator. +- **`audio-focus`** — becoming-noisy events (headphones unplugged). + +`android/` is generated by CNG and git-ignored. Native configuration goes in +`app.json` or a config plugin, never into the generated directory. + +## Adding something + +- **A screen** → a route in `app/` that only reads params and renders, plus the + real component under `src/features//`. New routes need Metro running + to regenerate `.expo/types/router.d.ts`. +- **A query** → `src/db/queries/`, nowhere else. +- **A decision you had to think about** → an ADR in [adr/](adr/). Twelve of them + exist; several are the only written record of why an obvious-looking + alternative was rejected. +- **A colour or a spacing value** → [theming.md](theming.md) first. + `tailwind.config.js` overrides the spacing scale, and a class built from a + value outside it compiles to *nothing* — no warning, no size. `scale.test.ts` + now fails on any such class, which is the only reason it is safe to guess. + +## Further reading + +[AGENTS.md](../AGENTS.md) is binding house style and comes before all of these. +Then [database.md](database.md), [scanner.md](scanner.md), +[player.md](player.md), [shuffle.md](shuffle.md), [stats.md](stats.md), +[theming.md](theming.md), [i18n.md](i18n.md), +[components.md](components.md), [performance.md](performance.md), and +[adr/](adr/). From 09c6225dbbaa96094beb859b74a0a8ccd3c2fe3f Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:22:41 +0300 Subject: [PATCH 18/20] chore(deps): drop zustand, which nothing ever imported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The app has no global state library, by design. `AudioEngine` is a singleton that lives outside React and outlives every screen, and the player hooks subscribe to it through `useSyncExternalStore` — see the comment in src/features/player/hooks/usePlayback.ts for why that is the right primitive: it is exactly what the hook is for, and it gets tearing right during concurrent renders, which a hand-rolled subscription does not. QueueScreen and the Toaster use the same pattern against their own external stores. So zustand was not pending work, just an unused entry left over in the dependency list. Nothing under src/ imports it, and `npm uninstall` removed exactly one package, so nothing reached it transitively either. Gate after removal: lint clean, typecheck clean, 293 tests / 20 suites passing. Co-Authored-By: Claude Opus 5 --- package-lock.json | 32 +------------------------------- package.json | 3 +-- 2 files changed, 2 insertions(+), 33 deletions(-) diff --git a/package-lock.json b/package-lock.json index de02cf0..281e0cf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,8 +42,7 @@ "react-native-safe-area-context": "~5.7.0", "react-native-screens": "~4.26.0", "react-native-svg": "15.15.4", - "react-native-worklets": "0.10.1", - "zustand": "^5.0.14" + "react-native-worklets": "0.10.1" }, "devDependencies": { "@testing-library/react-native": "~14.0.1", @@ -18144,35 +18143,6 @@ "peerDependencies": { "zod": "^3.25.0 || ^4.0.0" } - }, - "node_modules/zustand": { - "version": "5.0.14", - "resolved": "https://registry.npmjs.org/zustand/-/zustand-5.0.14.tgz", - "integrity": "sha512-/8tAspM5LMPr28b3fwLYrtdj77ECpfZviaP75CMTnwO8ISyaE4GDIG/9rDDYq/cH9D2Xw2A2RXglLInmVBQB/g==", - "license": "MIT", - "engines": { - "node": ">=12.20.0" - }, - "peerDependencies": { - "@types/react": ">=18.0.0", - "immer": ">=9.0.6", - "react": ">=18.0.0", - "use-sync-external-store": ">=1.2.0" - }, - "peerDependenciesMeta": { - "@types/react": { - "optional": true - }, - "immer": { - "optional": true - }, - "react": { - "optional": true - }, - "use-sync-external-store": { - "optional": true - } - } } } } diff --git a/package.json b/package.json index b187a39..935bc7f 100644 --- a/package.json +++ b/package.json @@ -47,8 +47,7 @@ "react-native-safe-area-context": "~5.7.0", "react-native-screens": "~4.26.0", "react-native-svg": "15.15.4", - "react-native-worklets": "0.10.1", - "zustand": "^5.0.14" + "react-native-worklets": "0.10.1" }, "devDependencies": { "@testing-library/react-native": "~14.0.1", From 18b4b4d33b739a2dd88ff8d7ef24c402cfeb0de3 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:51:24 +0300 Subject: [PATCH 19/20] docs: follow the zustand removal through the docs and configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tail of 09c6225, which dropped the dependency but left four files still describing it. AGENTS.md rule 5 and the tech-stack table both named Zustand as where transient player/UI state lives. It never did — the AudioEngine singleton holds it and React reads it through useSyncExternalStore. Both now say so, and rule 5 adds "there is no global state library; don't add one" so the next person does not reintroduce one to fill an apparent gap. The eslint and jest configs ignore .claude/worktrees/*. Those hold full checkouts of other branches, so lint reported the root-level src/ exemptions as violations (the path-based rules only match src/ at the repo root) and jest collected every suite a second time. Note that overriding testPathIgnorePatterns drops its default, so /node_modules/ is repeated there deliberately. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 6 ++++-- docs/01-TECH-STACK.md | 2 +- docs/architecture.md | 3 +-- eslint.config.js | 5 ++++- jest.config.js | 4 ++++ 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0271b43..d64534a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,8 +109,10 @@ These are not preferences. Violating them is a bug. typed query functions. 4. **No business logic in component bodies.** Logic goes in `src/services/` (pure, testable) or `src/features/*/hooks/` (stateful orchestration). -5. **Settings live in MMKV. Data lives in SQLite.** Don't mix them. Zustand holds transient - player/UI state only, never anything that must survive a restart. +5. **Settings live in MMKV. Data lives in SQLite.** Don't mix them. Transient player/UI state + lives in the `AudioEngine` singleton and the other module-level stores, read from React + through `useSyncExternalStore` — never anything that must survive a restart. There is no + global state library; don't add one. 6. **Layers point downward.** `components → hooks → services → db`. Never the reverse. A service must not import a component. diff --git a/docs/01-TECH-STACK.md b/docs/01-TECH-STACK.md index 8b961e3..a0133ac 100644 --- a/docs/01-TECH-STACK.md +++ b/docs/01-TECH-STACK.md @@ -30,7 +30,7 @@ These are the four choices that shape everything else. Read §2 before locking t | Styling | **NativeWind** (Tailwind for RN) | Requested. Dark mode via `dark:` variant. | | Database | **`expo-sqlite`** + **Drizzle ORM** | Typed schema, `drizzle-kit` migrations, `useLiveQuery` for reactive lists. | | Key-value | **`react-native-mmkv`** | Settings, theme, last queue snapshot. Synchronous → no theme flash on boot. | -| State | **Zustand** | Player/UI state only. Persisted data lives in SQLite, not in a store. | +| State | **`useSyncExternalStore`** against module singletons | No state library. Player/UI state only, held by `AudioEngine` and friends. Persisted data lives in SQLite, not in a store. | | Lists | **`@shopify/flash-list`** | Required — a 10k-track library will not survive `FlatList`. | | Images | **`expo-image`** | Built-in memory + disk cache, `recyclingKey`, blurhash placeholders. | | Animation | **`react-native-reanimated`** + **`react-native-gesture-handler`** | Mini-player → full-player transition, swipe-to-queue. | diff --git a/docs/architecture.md b/docs/architecture.md index 06c45da..9b643ce 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -48,8 +48,7 @@ one has a failure that follows from breaking it: ## Where state actually lives This is the part that surprises people, because there is no global state -library. `zustand` appears in `package.json` and is imported by nothing. State -lives in three places, chosen by lifetime: +library. State lives in three places, chosen by lifetime: **SQLite — everything persistent.** Tracks, playlists, play events, rollups. Opened once in [client.ts](../src/db/client.ts) with WAL, `synchronous=NORMAL`, diff --git a/eslint.config.js b/eslint.config.js index ca4d480..aca4ab3 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -27,7 +27,10 @@ const RESTRICTED_IMPORTS = [LAYER_DIRECTION, DATABASE_BOUNDARY, AUDIO_BOUNDARY]; module.exports = defineConfig([ expoConfig, { - ignores: ['dist/*', 'android/*', 'ios/*', '.expo/*'], + // `.claude/worktrees/*` holds full checkouts of other branches. Linting + // them reports the root-level src/ exemptions as violations, because the + // path-based rules below only match src/ at the repo root. + ignores: ['dist/*', 'android/*', 'ios/*', '.expo/*', '.claude/**'], }, { files: ['**/*.test.ts', '**/*.test.tsx', 'jest.setup.js'], diff --git a/jest.config.js b/jest.config.js index 11b33c2..bd66324 100644 --- a/jest.config.js +++ b/jest.config.js @@ -5,6 +5,10 @@ module.exports = { transformIgnorePatterns: [ 'node_modules/(?!((jest-)?react-native|@react-native(-community)?)|expo(nent)?|@expo(nent)?/.*|@expo-google-fonts/.*|react-navigation|@react-navigation/.*|@sentry/react-native|native-base|react-native-svg|nativewind|react-native-css-interop)', ], + // Overriding this drops the default, so /node_modules/ is repeated here. + // `.claude/worktrees/*` holds full checkouts of other branches; without + // this, every suite there is collected a second time. + testPathIgnorePatterns: ['/node_modules/', '/\\.claude/'], // AGENTS.md: real coverage on services/, not on components. collectCoverageFrom: ['src/services/**/*.ts', 'src/utils/**/*.ts'], }; From 8cb9c623d9d6095c357488d3105316a7099a6601 Mon Sep 17 00:00:00 2001 From: Yefee8 Date: Sat, 1 Aug 2026 14:53:11 +0300 Subject: [PATCH 20/20] test(device): close the playlist chain, repeat-listen and release build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four things the handoff listed as owed are now verified on a device. The fourth, frame timing, is the only one automation cannot reach. The previous session blamed flaky taps on the emulator being up nine hours. That was true but not the whole reason: every control in this app carries an accessibilityLabel, so driving the UI from `uiautomator dump` by label works where guessed pixel coordinates do not. Rebooted the AVD and did the whole pass that way. Playlist chain in one pass — create, name, add 3 tracks, drag-reorder, play. The reorder needs `input motionevent` DOWN/hold/MOVE/UP, because the handle is Gesture.Pan().activateAfterLongPress(120) and a plain `input swipe` never activates it; that is why earlier attempts saw nothing move. Moving perf-001 from position 2 to 0 renumbered all three rows correctly. QueueSource reaches the database: source_type=playlist, source_id=1, and three matching stats_rollups rows for week, month and year. Repeat-listen produced two distinct outcome=play rows for one track, 211171 ms and 144145 ms, each closed by a manual scrubber drag to the start. track_stats and all three rollup periods agree. Release APK installed and run for the first time in the project's life, with `adb reverse --remove-all` first so it could not quietly fall back to Metro. aapt2 confirms no INTERNET permission before installing. Boots clean under Hermes and minification, all four tabs render. Functional only — no numbers taken, so the debug-build caveat stands. Two incidental confirmations: background audio survives the app going to the launcher, and the spec strip on a real MP3 reads "MP3 · 44.1 kHz · 138 kbps · Stereo · 5.1 MB" — no codec field, per the finding closed in 255cacc, and 138 kbps from SpecMath computing over the retriever's bogus reported 32. One gap found: MiniPlayer renders only in app/(tabs)/_layout.tsx, so pushed stack screens have no transport control. Recorded as a design question rather than fixed. Co-Authored-By: Claude Opus 5 --- HANDOFF.md | 75 ++++++++++++++++++++++++++--------------- docs/performance.md | 81 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 34 deletions(-) diff --git a/HANDOFF.md b/HANDOFF.md index 18f114c..d8a45b3 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -59,54 +59,75 @@ fighting flaky taps. ## What is actually left -### 1. Frame timing on the Mi 9T — never once obtained +### 1. Frame timing on the Mi 9T — still never obtained -The counters are **reset and armed right now**. Ask the user to scroll the -library hard for ~10 seconds, then: +**The only item on this list that no amount of automation can close.** The AVD +renders through SwiftShader, so its frame numbers are worthless; MIUI blocks +`adb shell input`, so the phone cannot be scrolled without a human hand. It +needs both halves at once and there is no substitute. + +The user uninstalled the app from the phone on 2026-08-01, so this now needs a +reinstall first. Then, with the app on the Library tab: ```bash +adb -s 7a6f8791 shell dumpsys gfxinfo dev.mufify.app reset +# human scrolls hard for ~10 seconds adb -s 7a6f8791 shell dumpsys gfxinfo dev.mufify.app framestats ``` +Note the package is `dev.mufify.app`, not `com.mufify` — querying the wrong one +returns "No process found", which reads exactly like the app having died. + This is the only valid source for a 60 fps claim, and `docs/performance.md` deliberately makes no frame-rate claim until it exists. Record it there. -### 2. Playlist chain, end to end +### 2. ~~Playlist chain, end to end~~ — DONE 2026-08-01 + +Verified on the Pixel_7 AVD in one pass, `stats_rollups` playlist rows included. +Full record in `docs/performance.md` under "Device verification". -Create → add tracks → reorder → play. Each piece was verified separately; the -whole chain in one pass never was, because the emulator's input died. Needs -human taps on the phone, or a fresh emulator. +Two things worth carrying forward: -While doing it, confirm a **playlist** row appears in `stats_rollups` — the -`QueueSource` plumbing is in and unit-covered but that specific path is -unverified on a device. Reading the database: +- Every control has an `accessibilityLabel`, so drive the UI from + `uiautomator dump` by label rather than guessing coordinates. That is what + made this reproducible where the previous attempt was not. +- The reorder handle is `Gesture.Pan().activateAfterLongPress(120)`. A plain + `adb shell input swipe` **never activates it**; you need + `input motionevent DOWN` → hold → `MOVE`s → `UP`. + +Reading the database — **copy the `-wal` too.** WAL means recent writes are not +in the main file, and copying only `mufify.db` shows an empty `play_events`, +which looks exactly like the bug you would then go hunting: ```bash -adb -s 7a6f8791 shell "run-as dev.mufify.app cat files/SQLite/mufify.db" > /tmp/m.db -adb -s 7a6f8791 shell "run-as dev.mufify.app cat files/SQLite/mufify.db-wal" > /tmp/m.db-wal +adb -s shell "run-as dev.mufify.app cat files/SQLite/mufify.db" > /tmp/m.db +adb -s shell "run-as dev.mufify.app cat files/SQLite/mufify.db-wal" > /tmp/m.db-wal sqlite3 /tmp/m.db "SELECT entity_type, COUNT(*) FROM stats_rollups GROUP BY 1;" ``` -**Copy the `-wal` too.** WAL mode means recent writes are not in the main file, -and copying only `mufify.db` shows an empty `play_events` — which looks exactly -like the bug you would then go hunting. +### 3. ~~Repeat-listen seek-back~~ — DONE 2026-08-01 + +Two distinct `outcome=play` rows for one track from two seek-backs, rollups +agreeing. Recorded in `docs/performance.md`. -### 3. Repeat-listen seek-back, on a device +### 4. ~~Release build~~ — DONE 2026-08-01 -`isRewindToRestart` has 15 tests including sequence-level ones, and the engine -wiring is reviewed. The manual seek-back path has not been watched on hardware. -Play something past 30 s, drag the scrubber to the start, listen past 30 s -again, and confirm **two** rows in `play_events`. +Installed and run for the first time, on the AVD rather than the phone, with +`adb reverse --remove-all` so it could not fall back to Metro. Boots clean, all +four tabs render, no `INTERNET` permission in the manifest. Details in +`docs/performance.md`. -### 4. Release build +Still **not measured** — functional smoke test only. Every number in this repo +is from a debug build, which overstates JS cost. -`assembleRelease` succeeds (~23 min, 133 MB universal APK). It has never been -installed or run. It ships **without the `INTERNET` permission** by design, so -it cannot use Metro — a genuine smoke test. Installing it over the debug build -requires an uninstall, which **wipes the user's scanned library and playlists**; -ask before doing that. +### 4b. A UX gap found while doing the above -Every measurement so far is from a debug build, which overstates JS cost. +`MiniPlayer` is rendered only in `app/(tabs)/_layout.tsx`. Pushed stack screens +therefore have no transport control: start playback from a playlist detail +screen and there is no visible player, and no way to reach Now Playing without +going back to a tab. It follows from the routing structure rather than being a +defect, so it is a design call rather than a bug fix — but it is a real gap and +nobody has decided about it. ### 5. One finding from the phone's database (the other is closed) diff --git a/docs/performance.md b/docs/performance.md index 9bca07f..e54c499 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -212,7 +212,11 @@ never been seen by anyone. `src/theme/scale.test.ts` now fails on any such class dev-mode React, no precompiled bytecode for lazily loaded modules. Debug overstates JS cost substantially, so the cold-start figure in particular should be re-taken against a release build before it is treated as what a user - experiences. + experiences. The release build now *runs* (see above) but has not been + measured — functional smoke test only. +- **Artwork and artists at scale.** Both emulator and phone libraries are + synthetic files with no artist and, apart from two, no artwork. The artist + shelf and the artwork cache have never met a real library. --- @@ -232,9 +236,72 @@ gates green — `lint`, `typecheck`, 292 JS tests across 20 suites, and | Shuffle | Selection persists and `play_events.shuffle_algorithm` records it. | | Statistics | Wrapped, tiles and all four ranked lists, with covers and durations. | -**Not exercised end to end:** the playlist create → add tracks → play flow. Its -pieces were each verified separately earlier — the detail screen, the mosaic, -the add-tracks sheet, reorder — but the whole chain in one pass was blocked by -`adb shell input` becoming unreliable on an emulator that had been up for -around nine hours. It is a gap in the evidence rather than a known fault, and it -is the first thing to check on a fresh device. +~~**Not exercised end to end:** the playlist create → add tracks → play flow.~~ +Closed on 2026-08-01 — see below. + +--- + +## Device verification, 2026-08-01 + +Pixel_7 AVD, API 35, rebooted first because the previous session's flaky taps +were an emulator that had been up nine hours rather than a real fault. Every +control in this app carries an `accessibilityLabel`, so the whole pass was +driven from `uiautomator dump` by label instead of guessed pixel coordinates — +which is what made it reproducible where the earlier attempt was not. + +**The playlist chain, end to end.** Create → name → add 3 tracks → drag-reorder +→ play, in one pass. The reorder needed a real `motionevent` DOWN / hold / +MOVE / UP sequence, because `ReorderableEntry` uses +`Gesture.Pan().activateAfterLongPress(120)` and a plain `input swipe` never +activates it. Moving perf-001 from position 2 to 0 renumbered all three rows +correctly in `playlist_tracks`. + +`QueueSource` reaches the database: `play_events` recorded `source_type=playlist, +source_id=1, ms_played=6089, completed=1`, and `stats_rollups` gained three +matching rows — `week 2026-W31`, `month 2026-08`, `year 2026`, each +`playlist/1, play_count=1, ms_played=6089`. That path was unit-covered but had +never been seen on a device. + +**Repeat-listen seek-back.** MUSE - Cryogen, 5:10, threshold 30 s. Played to +3:00, dragged the scrubber to the start, played again, dragged back a second +time. Two distinct qualifying rows for the same track — `ms_played=211171` and +`ms_played=144145`, both `outcome=play` — which is exactly what +`isRewindToRestart` exists to produce. `track_stats` and all three rollup +periods agree at 4 plays / 464455 ms. + +**Background audio**, incidentally: playback continued through the app being +backgrounded to the launcher and was still at `state=PLAYING` on return. The +engine outliving every screen is the reason, and this is the first time it has +been watched. + +**The spec strip on a real MP3** reads `MP3 · 44.1 kHz · 138 kbps · Stereo · +5.1 MB`. Two things confirmed there: no codec field, because `codecOf` returns +null when the subtype is already the container name; and 138 kbps, which is +`SpecMath.bitrateKbps` computing from size and duration rather than trusting +the retriever's reported 32 — the exact file its doc comment describes. + +**One UX gap found.** `MiniPlayer` is rendered only in `app/(tabs)/_layout.tsx`, +so pushed stack screens have no transport control. Start playback from a +playlist detail screen and there is no visible player and no route to Now +Playing without going back to a tab. It follows from the routing structure +rather than being a defect, but it is a real gap. + +--- + +## Release build, first run ever + +`app-release.apk`, 133 MB universal, installed on the Pixel_7 AVD with +`adb reverse --remove-all` first, so nothing could quietly fall back to Metro. + +- **No `INTERNET` permission**, confirmed with `aapt2 dump permissions` before + installing. `ACCESS_NETWORK_STATE` is present and grants no network access + without it, as ADR 009 records. +- Boots standalone, no crash, no red box, no `Unable to load script`. +- All four tabs render under Hermes and minification: Library's empty state, + Playlists, Stats with its Week/Month/Year segments, and Settings with theme, + language including `Türkçe`, and all five shuffle algorithms with their + descriptions. Lucide SVG icons and all three fonts load. + +This was the last thing in the project never to have been run. Note it is still +a *functional* smoke test — no release-build numbers were taken, so the +cold-start caveat below stands unchanged.