feat: enable max brightness for QR-Code read #653
Conversation
WalkthroughAjout d’un contrôle de luminosité autour de la vue QR : capture de la luminosité au montage, réglage à 1 pendant l’affichage, écoute d’AppState pour restaurer/réappliquer la luminosité lors des transitions, et restauration finale au démontage. Changes
Diagrammes de séquencesequenceDiagram
participant Component as Composant QR
participant AppState as AppState
participant Brightness as API Brightness
participant Device as Appareil
Component->>Brightness: getBrightnessAsync()
Brightness-->>Component: currentBrightness
Component->>Brightness: setBrightnessAsync(1)
Brightness->>Device: appliquer luminosité maximale
AppState->>Component: changement → background/inactive
Component->>Brightness: setBrightnessAsync(currentBrightness)
Brightness->>Device: appliquer luminosité précédente
AppState->>Component: changement → active
Component->>Brightness: setBrightnessAsync(1)
Brightness->>Device: appliquer luminosité maximale
Component->>Brightness: Unmount → setBrightnessAsync(currentBrightness)
Brightness->>Device: appliquer luminosité précédente
Estimated code review effort🎯 3 (Modéré) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/(features)/(cards)/qrcode.tsx (1)
41-79: Appels de permissions superflus et gestion d'erreurs manquanteDeux observations sur cette implémentation :
Permissions :
requestPermissionsAsync()n'est pas requis poursetBrightnessAsync()(luminosité au niveau de l'app). Cette permission est uniquement nécessaire poursetSystemBrightnessAsync()(luminosité système). Les trois appels aux lignes 44-46, 56-57 et 70-71 sont superflus.Gestion d'erreurs : Les opérations async sur la luminosité ne sont pas encapsulées dans des blocs try-catch. Il est recommandé d'ajouter une gestion d'erreurs pour éviter les rejets non gérés.
♻️ Proposition de refactorisation
useEffect(() => { const enableBrightness = async () => { - const { status } = await Brightness.requestPermissionsAsync(); - if (status !== "granted") return; - - if (previousBrightness.current === null) { - previousBrightness.current = await Brightness.getBrightnessAsync(); + try { + if (previousBrightness.current === null) { + previousBrightness.current = await Brightness.getBrightnessAsync(); + } + await Brightness.setBrightnessAsync(1); + } catch (error) { + console.warn("Failed to set brightness:", error); } - - await Brightness.setBrightnessAsync(1); }; const restoreBrightness = async () => { - if (previousBrightness.current !== null) { - const { status } = await Brightness.requestPermissionsAsync(); - if (status === "granted") { + try { + if (previousBrightness.current !== null) { await Brightness.setBrightnessAsync(previousBrightness.current); } + } catch (error) { + console.warn("Failed to restore brightness:", error); } }; enableBrightness(); const subscription = AppState.addEventListener("change", async (nextAppState) => { if (nextAppState.match(/inactive|background/)) { await restoreBrightness(); } else if (nextAppState === "active") { - const { status } = await Brightness.requestPermissionsAsync(); - if (status === "granted") { + try { await Brightness.setBrightnessAsync(1); + } catch (error) { + console.warn("Failed to re-enable brightness:", error); } } });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/(features)/(cards)/qrcode.tsxpackage.json
🔇 Additional comments (2)
app/(features)/(cards)/qrcode.tsx (1)
6-7: LGTM - Imports ajoutés pour la gestion de luminositéLes imports nécessaires pour
AppState,useEffect,useRefetexpo-brightnesssont correctement ajoutés pour implémenter la fonctionnalité.Also applies to: 22-22
package.json (1)
53-53: L'ajout est conforme —expo-brightness ~14.0.8est la version groupée avec Expo SDK 54 et garantit une compatibilité optimale. Aucun problème de version détecté.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/(features)/(cards)/qrcode.tsx (1)
53-59: Bloc catch vide et message d'erreur incomplet.Le bloc
catchvide à la ligne 58 masque silencieusement les erreurs. De plus, le message de warning à la ligne 49 ne contient pas l'erreur capturée, ce qui complique le débogage.♻️ Correction proposée
const enableBrightness = async () => { try { if (previousBrightness.current === null) { previousBrightness.current = await Brightness.getBrightnessAsync(); } await Brightness.setBrightnessAsync(1); } catch (error) { - console.warn("Failed to set brightness on value 1"); + console.warn("Failed to set brightness:", error); } }; const restoreBrightness = async () => { try { if (previousBrightness.current !== null) { await Brightness.setBrightnessAsync(previousBrightness.current); } - } catch (error) {} + } catch (error) { + console.warn("Failed to restore brightness:", error); + } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(features)/(cards)/qrcode.tsx
🔇 Additional comments (2)
app/(features)/(cards)/qrcode.tsx (2)
6-7: LGTM !Les imports sont appropriés pour la fonctionnalité de contrôle de luminosité ajoutée.
Also applies to: 22-22
71-75: LGTM !Le nettoyage est correctement implémenté : suppression de l'écouteur AppState et restauration de la luminosité au démontage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/(features)/(cards)/qrcode.tsx (1)
73-76: Le nettoyage est correct mais pourrait être renforcé de manière défensive.La suppression de la souscription et l'appel à
restoreBrightness()sont appropriés. Notez querestoreBrightness()n'est pas awaité (les fonctions de nettoyage React ne peuvent pas être async), donc le composant pourrait se démonter avant que la luminosité ne soit complètement restaurée, mais c'est acceptable en pratique.🛡️ Suggestion optionnelle : ajout d'une vérification de montage
Pour une approche plus défensive, vous pourriez ajouter un ref pour suivre si le composant est monté et vérifier avant d'appeler les fonctions async :
const previousBrightness = useRef<number | null>(null); +const isMounted = useRef(true); useEffect(() => { const enableBrightness = async () => { try { + if (!isMounted.current) return; if (previousBrightness.current === null) { previousBrightness.current = await Brightness.getBrightnessAsync(); } + if (!isMounted.current) return; await Brightness.setBrightnessAsync(1); } catch (error) { console.warn("Failed to set brightness:", error); } }; const restoreBrightness = async () => { try { + if (!isMounted.current) return; if (previousBrightness.current !== null) { await Brightness.setBrightnessAsync(previousBrightness.current); } } catch (error) { console.warn("Failed to restore brightness:", error); } }; // ... rest of the code return () => { + isMounted.current = false; subscription.remove(); restoreBrightness(); }; }, []);Cependant, cela pourrait être excessif pour ce cas d'usage.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(features)/(cards)/qrcode.tsx
🔇 Additional comments (6)
app/(features)/(cards)/qrcode.tsx (6)
6-7: Les imports sont corrects et bien structurés.Les nouveaux imports nécessaires pour la gestion de la luminosité sont appropriés : AppState pour surveiller le cycle de vie de l'application, les hooks React pour la logique de composant, et expo-brightness pour l'API de luminosité.
Also applies to: 22-22
39-39: Utilisation appropriée de useRef pour stocker la luminosité précédente.Le choix de useRef est correct pour maintenir la valeur de luminosité sans déclencher de re-rendus, et l'initialisation à null permet de vérifier si la luminosité a déjà été capturée.
53-61: La fonction de restauration de la luminosité est correctement implémentée.La vérification que la luminosité a été sauvegardée avant de la restaurer est appropriée, et la gestion des erreurs est en place.
65-71: Vérifiez le comportement avec l'état "inactive" sur iOS.L'écouteur AppState utilise
nextAppState.match(/inactive|background/)qui correspond à la fois aux états "inactive" et "background". Sur iOS, l'état "inactive" est un état de transition bref lors du passage entre premier plan et arrière-plan. Cela pourrait causer une brève restauration de la luminosité pendant la transition, puis une réactivation au retour à "active", créant un potentiel scintillement.Considérez de ne correspondre que l'état "background" si ce comportement pose problème :
if (nextAppState === "background") { restoreBrightness(); } else if (nextAppState === "active") { enableBrightness(); }Cela dit, l'implémentation actuelle peut être intentionnelle pour garantir une restauration rapide de la luminosité lors de la sortie de l'écran.
41-77: Le tableau de dépendances vide est approprié.Le tableau de dépendances vide
[]garantit que l'effet s'exécute uniquement au montage et au démontage, ce qui est le comportement souhaité. L'écouteur AppState gère les changements d'état dynamiques pendant la durée de vie du composant.
42-51: L'APIexpo-brightnesssupporte correctement les méthodes utilisées.La fonction
enableBrightnessimplémente correctement l'APIexpo-brightnessversion 14.0.8 :
getBrightnessAsync()retourne bien unPromise<number>entre 0 et 1setBrightnessAsync(1)définit correctement la luminosité maximaleLa logique de capture de la luminosité initiale au premier appel (via la vérification
previousBrightness.current === null) est appropriée et garantit la restauration ultérieure vers la luminosité d'origine. La gestion des erreurs avecconsole.warnest adéquate.
raphckrman
left a comment
raphckrman
left a comment
There was a problem hiding this comment.
Hello, utilise de préférence le Logger au lieu d'afficher les logs dans la console, où est-ce que tu demandes la permission à l'utilisateur de modifier sa luminosité ? Sur certains téléphones il faut demander la permission
Hello, pour les permissions, selon la doc de expo si on utilise "Brightness.setSystemBrightnessAsync(brightnessValue)", on a besoin des perms "SYSTEM_BRIGHTNESS" car on modifie tt la luminiosité du système, mais j'ai utilisé "Brightness.setBrightnessAsync(brightnessValue)" qui modifie seulement la luminosité de l'application utilisé et sur les docs de expo y'a aucune mention à une quelconque permission requise. J'ai testé sur Android : effectivement, "setSystemBrightnessAsync" bloquait sans permissions et il faillait utiliser "Brightness.requestPermissionsAsync()", mais setBrightnessAsync fonctionne nativement sans friction. |
2 participants
Règles de contribution
Caution
Afin de garantir une application stable et pérenne dans le temps, nous t'invitons à vérifier que tu as bien respecté les règles de contribution. Sans cela, ta Pull Request ne pourra pas être examinée.
Résumé des changements
Ajout d'un système de gestion de la luminosité lors de l'affichage d'un QR-Code. L'objectif est de garantir une lisibilité maximale pour les scanners de cantine en forçant l'écran à 100% de luminosité.
Utilisation de "expo-brightness" :
La luminosité revient à sa valeur initiale quand on clique sur la croix, quand on swipe le QR-Code vers le bas et quand on quitte l'application. Réactivation automatique du mode 100% si l'utilisateur revient sur l'application.
Summary by CodeRabbit
Nouvelles Fonctionnalités
Chores
✏️ Tip: You can customize this high-level summary in your review settings.