Skip to content

Conversation

@jvsena42
Copy link
Member

Description

This PR fixes the unused code warnings from the lint

Preview

N/A

QA Notes

N/A

@jvsena42 jvsena42 requested a review from ovitrif November 28, 2025 16:09
@jvsena42 jvsena42 self-assigned this Nov 28, 2025
@jvsena42 jvsena42 enabled auto-merge November 28, 2025 16:09
@claude
Copy link

claude bot commented Nov 28, 2025

Code Review

Thank you for this cleanup PR! This is a good refactoring effort to remove unused code. Here's my feedback:

Positive Aspects

  1. Excellent Code Cleanup: Removing 110 lines of unused onCloseClick parameters is a solid improvement that reduces code complexity and maintenance burden.

  2. Consistent Approach: The changes are applied consistently across all affected files - removing both parameter declarations and their usage at call sites.

  3. Follows Repository Guidelines: This aligns well with the copilot instructions at line 37: "ensure unused code is removed after refactoring."

  4. Low Risk: Since these are unused parameters being removed, the risk of introducing bugs is minimal.

🔍 Code Quality Observations

ActivityIcon.kt Modifier Position ✅

The change on line 36-39 correctly places the modifier parameter in the last position, which follows the Compose best practice mentioned in the repository's guidelines (copilot-instructions.md:31).

Before:

fun ActivityIcon(
    activity: Activity,
    size: Dp = 32.dp,
    modifier: Modifier = Modifier,
)

After:

fun ActivityIcon(
    activity: Activity,
    modifier: Modifier = Modifier,
    size: Dp = 32.dp,
)

This is the only substantive change in the PR - everything else is purely removing unused parameters. ✅

🧪 Testing Considerations

Recommendation: While this is a low-risk change, consider:

  1. Manual Testing: Verify that navigation flows work correctly, especially:

    • Back button behavior on all modified screens
    • Transfer flows (Savings/Spending/Funding/External)
    • Settings screens
    • Activity detail screens
  2. Regression Testing: Since navigation parameters were removed, ensure that closing/exiting flows still work as expected through the remaining navigation callbacks.

  3. Preview Functions: The preview functions in files like SpendingConfirmScreen.kt were also updated to remove the unused parameter - these should still render correctly in Android Studio's preview pane.

🔒 Security & Performance

  • ✅ No security concerns
  • ✅ No performance implications
  • ✅ No memory leak risks introduced

📝 Suggestions

  1. Verification: Double-check that no onCloseClick callbacks were actually being used somewhere that might have been missed. A quick grep to confirm:

    grep -r "onCloseClick" app/src/main/java/

    This should return no results (or only definitions that will be unused).

  2. Consider Follow-up: If there are other lint warnings, they could be addressed in subsequent PRs to keep changes focused.

📊 Summary

Category Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Clean, consistent removal of unused code
Best Practices ⭐⭐⭐⭐⭐ Follows Compose conventions (modifier positioning)
Risk Level 🟢 Low Only removes unused parameters
Test Coverage ⚠️ N/A Manual testing recommended
Security 🟢 No concerns No security implications

Recommendation: Approve

This is a clean refactoring that improves code maintainability. The changes are straightforward and follow best practices. Once manual testing confirms navigation flows work correctly, this is ready to merge.

Great work on keeping the codebase clean! 👍

Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

LGTM 🚀

Thanks for this cleanup, merging it

@ovitrif ovitrif disabled auto-merge November 28, 2025 16:21
@ovitrif ovitrif enabled auto-merge November 28, 2025 16:21
@ovitrif
Copy link
Collaborator

ovitrif commented Nov 28, 2025

Thanks for this cleanup, merging it

Re-enabled auto-merge;

Needs retrigger of settings e2e tests after the whole suite passes, there was a docker error of some sort.

@ovitrif ovitrif merged commit 1996e17 into master Nov 28, 2025
30 of 41 checks passed
@ovitrif ovitrif deleted the chore/lint branch November 28, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants