Skip to content

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented May 2, 2025

  • Encapsulate the fact that we prefix files in s3 with package name into s3paths.canonicalKey function
  • Resolve some tech dept and move routes functions used for FileEditor into FileEditor/routes.ts
  • Unit tests
  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

Copy link

codecov bot commented May 2, 2025

Codecov Report

Attention: Patch coverage is 70.73171% with 12 lines in your changes missing coverage. Please review.

Project coverage is 38.72%. Comparing base (21fbe9a) to head (1b708a1).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
catalog/app/components/FileEditor/CreateFile.tsx 30.00% 7 Missing ⚠️
.../app/containers/Bucket/PackageTree/PackageTree.tsx 0.00% 3 Missing ⚠️
...iners/Bucket/PackageDialog/PackageCreationForm.tsx 0.00% 1 Missing ⚠️
...og/app/containers/Bucket/PackageDialog/Uploads.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4397      +/-   ##
==========================================
+ Coverage   38.69%   38.72%   +0.02%     
==========================================
  Files         795      795              
  Lines       35208    35211       +3     
  Branches     5384     5384              
==========================================
+ Hits        13625    13636      +11     
+ Misses      21037    21029       -8     
  Partials      546      546              
Flag Coverage Δ
api-python 91.41% <ø> (ø)
catalog 17.78% <70.73%> (+0.04%) ⬆️
lambda 91.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR refactors file path handling in the UI to properly support package root prefixing when creating and editing files in packages, introducing a new s3paths.canonicalKey function to centralize path generation logic.

  • Hardcoded packageRoot value 'r/o/o/t' in Config.ts overrides configuration, potentially breaking custom package root setups
  • New canonicalKey function in s3paths.ts needs more comprehensive test coverage for edge cases
  • Refactored FileEditor routes to use hooks pattern, improving code organization but increasing complexity
  • Simplified file path handling in PackageCreationForm and Uploads components by using canonicalKey instead of manual path manipulation
  • Changed useAddFileInPackage and useEditFileInPackage hooks to return functions that accept paths, supporting better package root handling

10 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

@fiskus fiskus requested a review from nl0 May 5, 2025 10:01
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

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

mostly lgtm, some nitpicks inline

@fiskus fiskus changed the title Fix creating text files in UI with cfg.packageRoot defined Respect cfg.packageRoot config property when creating text files using UI May 6, 2025
@fiskus fiskus changed the title Respect cfg.packageRoot config property when creating text files using UI Respect cfg.packageRoot when creating text files using UI May 6, 2025
@fiskus fiskus requested a review from nl0 May 7, 2025 12:49
@fiskus fiskus added this pull request to the merge queue May 7, 2025
Merged via the queue into master with commit 7c670de May 7, 2025
37 checks passed
@fiskus fiskus deleted the package-root-create-file-from-catalog-ui branch May 7, 2025 14:08
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.

2 participants