refactor(webui): Move shared types to common workspace. #1289
Conversation
WalkthroughRefactors and centralizes TypeScript types into @webui/common subpaths, replaces local enums/types with shared modules, updates imports across client and server, adds domain-specific common modules (socket, metadata, presto, query, config), removes the monolithic common index, and adjusts ESLint export patterns. Control flow unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client UI
participant ClientAPI as Client API (search/presto)
participant Server as Server Routes
note right of ClientAPI: payload types now from @webui/common/schemas/*
UI->>ClientAPI: handleQuerySubmit(payload: QueryJobCreation)
ClientAPI->>Server: POST /search (body typed as QueryJobCreation)
Server-->>ClientAPI: 200 OK (body typed as QueryJob with numeric IDs)
ClientAPI-->>UI: response (IDs numeric)
UI->>UI: store IDs as strings (toString)
UI->>ClientAPI: handleQueryCancel({ searchJobId: Number(id), aggregationJobId: Number(id) })
ClientAPI->>Server: POST /search/cancel (typed as QueryJob)
Server-->>ClientAPI: 200 OK
ClientAPI-->>UI: success ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit configuration file
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
✨ Finishing touches
🧪 Generate unit tests
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 Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts
Show resolved
Hide resolved
| capIsNewExceptions: [ | ||
| "Type.Any", | ||
| "Type.Enum", | ||
| "Type.Integer", | ||
| "Type.Literal", | ||
| "Type.Null", | ||
| "Type.Required", | ||
| "Type.Union", | ||
| "Value.Errors", | ||
| "Value.Parse", | ||
| ], |
There was a problem hiding this comment.
| capIsNewExceptions: [ | |
| "Type.Any", | |
| "Type.Enum", | |
| "Type.Integer", | |
| "Type.Literal", | |
| "Type.Null", | |
| "Type.Required", | |
| "Type.Union", | |
| "Value.Errors", | |
| "Value.Parse", | |
| ], | |
| capIsNewExceptionPattern: "(^Type\\..)|(^Value\\..)", |
How about using this flag? If yes, we should also update eslint.config.mjs in client and server. Let me know if you feel this is a bad idea.
There was a problem hiding this comment.
I think good idea I will test
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/webui/common/eslint.config.mjs (2)
16-26: Pattern-based TypeBox exemption looks good and addresses prior feedbackSwitching to capIsNewExceptionPattern for Type/Value resolves the earlier reviewer concern about incomplete exceptions. Thanks for aligning with client/server.
22-22: Tighten regex to only match PascalCase members under Type/ValueSmall hardening to avoid exempting unrelated members: anchor the property to uppercase.
Apply:
- capIsNewExceptionPattern: "^(Type|Value)\\.", + capIsNewExceptionPattern: "^(Type|Value)\\.[A-Z]",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/webui/client/eslint.config.mjs(1 hunks)components/webui/common/eslint.config.mjs(1 hunks)components/webui/server/eslint.config.mjs(1 hunks)
🔇 Additional comments (2)
components/webui/client/eslint.config.mjs (1)
34-40: Remove explicit "Decode"/"Encode" exceptions — no bare usages foundSearch of components/webui/client returned no bare Decode/Encode call sites; the regex capIsNewExceptionPattern (^(Type|Value).) covers Value.Decode/Encode, so remove "Decode" and "Encode" from capIsNewExceptions in components/webui/client/eslint.config.mjs.
components/webui/server/eslint.config.mjs (1)
22-22: Regex approach matches client/common; nice consistencyThis removes the need to enumerate TypeBox factories.
| "Type.Union", | ||
| "Value.Parse", | ||
| ], | ||
| capIsNewExceptionPattern: "^(Type|Value)\\.", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: restrict the exemption to PascalCase members
Prevents accidentally exempting non-factory members.
- capIsNewExceptionPattern: "^(Type|Value)\\.",
+ capIsNewExceptionPattern: "^(Type|Value)\\.[A-Z]",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| capIsNewExceptionPattern: "^(Type|Value)\\.", | |
| capIsNewExceptionPattern: "^(Type|Value)\\.[A-Z]", |
🤖 Prompt for AI Agents
In components/webui/client/eslint.config.mjs around line 39, the current
capIsNewExceptionPattern ("^(Type|Value)\\.") is too broad and may exempt
non-factory members; change the pattern to require PascalCase names before the
dot (e.g., require the identifier to start with an uppercase letter followed by
letters/digits) so only PascalCase members are exempted — update the pattern
accordingly to something that matches an initial uppercase letter then
alphanumerics before the dot (for example: start with ^, then [A-Z] followed by
[A-Za-z0-9]*, then \\.), and replace the existing value with that stricter
regex.
| "Value.Errors", | ||
| "Value.Parse", | ||
| ], | ||
| capIsNewExceptionPattern: "^(Type|Value)\\.", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: narrow the regex to uppercase members
Same tightening as elsewhere.
- capIsNewExceptionPattern: "^(Type|Value)\\.",
+ capIsNewExceptionPattern: "^(Type|Value)\\.[A-Z]",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| capIsNewExceptionPattern: "^(Type|Value)\\.", | |
| capIsNewExceptionPattern: "^(Type|Value)\\.[A-Z]", |
🤖 Prompt for AI Agents
In components/webui/server/eslint.config.mjs around line 22, the regex
capIsNewExceptionPattern is too broad (matches any member after "Type." or
"Value."); tighten it to only match when the member name starts with an
uppercase letter by requiring an uppercase char after the dot (update the
pattern string accordingly, keeping proper escaping).
davemarco
changed the title
3 participants
Description
Moves common types into common workspace. Includes route types, and some other shared types like nullable.
Imports keep common folder structure, instead of using barrel file which is discouraged.
I didnt move some stream metadata types since the types are not exactly the same on server and client. This is due to the types existing prior to new webui work. These types/code need to be refactored first, then can be moved.
Checklist
breaking change.
Validation performed
UI loads. Query still sent
Summary by CodeRabbit
Bug Fixes
Refactor
Style
Chores