Skip to content

Conversation

@khahani
Copy link
Contributor

@khahani khahani commented Dec 21, 2025

@khahani khahani self-assigned this Dec 21, 2025
@aburchell
Copy link
Contributor

aburchell commented Dec 22, 2025

What is the point of this PR?

  1. You're moving a ton of exports to the "default" style -- which we don't use pretty much anywhere else in the codebase, and doesn't appear to actually change the consumed API. We're adding lines of code that add no functionality, and moving the exports further from where they're defined. What's the point of this? So that I have to do two go-to definition's to get to the function's actually definition? I could see changes like this if you were simplifying the API, actually restructuring so that we export fewer functions by grouping them into more idiomatic Class's, but this is not that. Is there something I'm missing that makes this export method superior? It seems to me like the blind acceptance of an LLM suggestion...

  2. It appears that you're adding a new import retry, but not using this. Am I missing something, or are we just adding an unused external dependency? Again, this feels like an LLM addition that was not reviewed (or I'm missing somewhere it's referenced! Please always let me know if I'm mistaken)

Let's try to focus on decreasing complexity and code length wherever possible, not adding to it.

@khahani
Copy link
Contributor Author

khahani commented Dec 22, 2025

Thanks for the detailed feedback—appreciate you taking the time to review this. I'll address your points below to clarify my thinking, and I'm open to discussing further if anything doesn't land.

On the export style changes: You're right that we're shifting to a more "default"-style grouping, which isn't our usual pattern. The goal here isn't to change the consumed API but to start tidying up the module's interface as a first step in refactoring. This module has grown to ~2800 lines, making it harder to maintain, so I grouped the ~40 scattered exports into 27 logical categories (e.g., by functionality) with readable comments at the end. This reduces the cognitive load when scanning the module's contract—about a 30% reduction in exported items—without breaking anything. It sets us up for deeper restructuring, like moving to classes as you suggested.
That said, I agree we should align with the team's coding guidelines. If grouping exports at the end feels off or adds unnecessary indirection (e.g., extra go-to-definitions), I'm happy to revert and keep them inline. Is there a preferred way you'd suggest for organizing large modules like this?

On the new retry import: Good catch—it's currently unused in this PR, but it's planned for the next iteration to handle some flaky API calls we're seeing in related flows. If testing shows we don't need it (or if there's a lighter alternative in our codebase), I'll remove it to avoid adding dependencies. Let me know if you spot any risks I'm missing here.

Overall, I'm with you on keeping things simple and reducing code length— that's exactly why I'm approaching this iteratively. I do use Claude to boost productivity (e.g., for initial refactoring ideas), but I always review and adapt suggestions manually. No blind acceptance on my end!

Let me know your thoughts or if you'd like me to make adjustments before merging.

@aburchell
Copy link
Contributor

Ahh makes sense! Thanks for reviewing. My biggest concern was the unused dependency, so glad to know there's more in store for that incoming haha.

I think for the export issue -- it's probably a problem that can't be solved by structuring exports. You make a very good point about it getting long, and I see the logic for grouping the exports now. This is probably a sign we should break up this file/functionality though, and/or structure the code more tightly together through unified Class-es to codify the structured export logic. But that's probably beyond the scope of this bug fix PR.

It's not clear to me which (between the old, and from this PR) export scheme is better, so I'll leave it to you if you want to include the change in this PR or not -- either is fine with me, now that I understand the intention of the change. I personally prefer exporting functions directly, so that go-to's are more likely to take me directly to the code of the function (ie less redirection); it also reduces LOC, but that feels like a flimsy metric even in this case.

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