Feat/apd 22 header selection updates.#534
Conversation
…both entitlment on and off
carlbrugger
force-pushed
the
feat/apd-22-header-selection
branch
from
2e4458c to
16943cb
Compare
WalkthroughThe recent updates introduce significant changes across various modules to enhance functionality and ensure immutability. The primary focus is on replacing the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Listener
participant ExcelExtractor
participant DelimiterExtractor
User->>Listener: Initiates extraction
Listener->>ExcelExtractor: Uses ExcelExtractor
ExcelExtractor-->>Listener: Data extracted
Listener->>DelimiterExtractor: Uses DelimiterExtractor with .txt config
DelimiterExtractor-->>Listener: Data extracted
Listener-->>User: Extraction complete
sequenceDiagram
participant System
participant Extractor
participant EntitlementsService
participant Workbook
System->>Extractor: Call Extractor with spaceId
Extractor->>EntitlementsService: Fetch entitlements for spaceId
EntitlementsService-->>Extractor: Return entitlements
Extractor->>Workbook: Update sheet metadata if headerSelectionEnabled
Workbook-->>Extractor: Metadata updated
Extractor-->>System: Extraction complete
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review due to trivial changes (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
damonbanks
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (10)
plugins/xml-extractor/src/parser.spec.ts (1)
Line range hint
68-156: Convert function expressions to arrow functions for simplicity.- describe('parser', function () { + describe('parser', () => { - describe('xmlToJson', function () { + describe('xmlToJson', () => { - test('with multiple items', function () { + test('with multiple items', () => { - test('with one item', function () { + test('with one item', () => { - test('headersFromObjectList', function () { + test('headersFromObjectList', () => { - describe('findRoot', function () { + describe('findRoot', () => { - test('skips _declaration', function () { + test('skips _declaration', () => { - test('works with no declaration', function () { + test('works with no declaration', () => { - describe('parser', function () { + describe('parser', () => { - test('test duplicate header', function () { + test('test duplicate header', () => {plugins/xlsx-extractor/src/parser.spec.ts (1)
38-38: Themetadataproperty is set toundefined. If this is intentional to represent no metadata, consider adding a comment to clarify this, as it might be mistaken for an oversight.plugins/json-extractor/src/parser.spec.ts (2)
Line range hint
5-147: Convert the function expression to an arrow function for consistency and to align with modern JavaScript practices.- describe('parser', function () { + describe('parser', () => {
118-118: Themetadataproperty is set toundefined. If this is intentional, consider adding a comment to clarify this choice.plugins/xlsx-extractor/src/header.detection.ts (3)
41-42: Add documentation for the newlettersproperty inGetHeadersResult.It would be beneficial to include comments explaining the purpose and usage of the
lettersproperty to aid future developers.
Line range hint
111-134: Refactor duplicated logic inOriginalDetector.getHeaders.The logic to resolve the promise with
{ header, skip, letters }is duplicated in the 'finish' and 'close' event handlers. Consider extracting this into a separate method to reduce duplication and improve maintainability.+ private resolveHeaders(resolve, header, skip, letters) { + resolve({ header, skip, letters }); + } ... detector.on('finish', () => { - resolve({ header, skip, letters }); + this.resolveHeaders(resolve, header, skip, letters); }); dataStream.on('close', () => { - resolve({ header, skip, letters }); + this.resolveHeaders(resolve, header, skip, letters); });Also applies to: 137-137
Line range hint
62-62: Remove unnecessary constructor inHeaderizer.The constructor in
Headerizeris flagged as unnecessary by static analysis. Consider removing it to simplify the class definition.- constructor() {}plugins/delimiter-extractor/src/header.detection.ts (3)
41-42: Add documentation for the newlettersproperty inGetHeadersResult.It would be beneficial to include comments explaining the purpose and usage of the
lettersproperty to aid future developers.
Line range hint
111-134: Refactor duplicated logic inOriginalDetector.getHeaders.The logic to resolve the promise with
{ header, skip, letters }is duplicated in the 'finish' and 'close' event handlers. Consider extracting this into a separate method to reduce duplication and improve maintainability.+ private resolveHeaders(resolve, header, skip, letters) { + resolve({ header, skip, letters }); + } ... detector.on('finish', () => { - resolve({ header, skip, letters }); + this.resolveHeaders(resolve, header, skip, letters); }); dataStream.on('close', () => { - resolve({ header, skip, letters }); + this.resolveHeaders(resolve, header, skip, letters); });Also applies to: 137-137
Line range hint
62-62: Remove unnecessary constructor inHeaderizer.The constructor in
Headerizeris flagged as unnecessary by static analysis. Consider removing it to simplify the class definition.- constructor() {}
2 participants
Please explain how to summarize this PR for the Changelog:
This formats excel and delimiter plugins to not truncate the incoming data to the detected header row but instead preserves the original format of the file, opting to update the sheet.metadata column with the header information.
Adds sheets.metadata support for future releases of Flatfile Platform. This formats the metadata.headers to support Flatfile platform expectations.
Tell code reviewer how and what to test:
The PR Essentially updates how the file extraction operates:
What this will enable us to do is to allow the user or AI (eventually) to update the header row.
Testing
On the platform project:
Setup:
pnpm run migrate:recordsfrom the apps/platform folderpnpm run cli entitlement add --key headerSelection --resource dev_acc_yourLocalAccountIdpnpm run cli entitlement rm --key headerSelection --resource dev_acc_yourLocalAccountIdTesting Areas:
File upload
- Different Files. See not above on CSVs only getting the new functionality atm
- You should be able to upload a csv with the header row on line > 1 and still see the previous rows
- With the new entitlement turned off you should see things work as they do today
File Mapping