fix(webui): Improve Presto search metadata handling: #1280
Conversation
WalkthroughConsolidates Presto search signalling to QUERYING/FAILED/DONE, updates server to set DONE on completion (targeting searchJobId), adjusts client to reset result counters on submit and to treat DONE (not FINISHED) as completion, and exposes/resetters plus SEARCH_STATE_DEFAULT for those resets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Client UI
participant Store as Search Store
participant API as Server API
participant DB as Metadata Store
User->>UI: Submit Presto query
UI->>Store: handlePrestoQuerySubmit()
Store->>Store: clear results
Store->>Store: reset numSearchResultsTable/numSearchResultsMetadata
Store->>Store: set UI = QUERY_ID_PENDING
UI->>API: POST /presto-search
API->>DB: insert metadata { lastSignal: QUERYING, queryEngine: Presto }
API-->>UI: { searchJobId }
UI->>Store: update jobId, set UI = QUERYING
note over API,DB: Presto execution proceeds
API->>API: On success, ensure searchJobId resolved
alt searchJobId unresolved
API->>API: log early finish and return
else resolved
API->>DB: update {_id: searchJobId} set lastSignal = DONE
API->>API: log success
end
UI->>Store: useUpdateStateWithMetadata(lastSignal)
Store->>Store: if DONE or RESP_DONE => set UI = DONE
sequenceDiagram
autonumber
participant Old as Old signalling
participant New as New signalling
Old->>Old: Many states (QUEUED..FINISHED)
Old->>Old: UI reacted to FINISHED
New->>New: States reduced to QUERYING/FAILED/DONE
New->>New: UI reacts to DONE (FINISHED removed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
davemarco
changed the title
davemarco
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/server/src/routes/api/presto-search/index.ts (1)
199-203: Possible race with createCollection; may throw if inserts auto-create the collection
insertPrestoRowsToMongocan auto-create the collection before this call, causingNamespaceExistserrors and failing the route.Apply one of:
- Guard-and-ignore “already exists”:
- await mongoDb.createCollection(searchJobId); + await mongoDb.createCollection(searchJobId).catch((err: any) => { + // Mongo error code 48 / codeName "NamespaceExists" + if (48 !== err?.code && "NamespaceExists" !== err?.codeName) { + throw err; + } + });
- Or remove
createCollectionentirely and rely on auto-creation (preferred unless special options are needed).components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
79-81: Submit failure leaves UI stuck at QUERY_ID_PENDING — set FAILEDOn error, the UI never leaves QUERY_ID_PENDING, confusing users and blocking resubmits conditioned on state. Set FAILED in the catch.
Apply:
.catch((err: unknown) => { - console.error("Failed to submit query:", err); + updateSearchUiState(SEARCH_UI_STATE.FAILED); + console.error("Failed to submit query:", err); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts(1 hunks)components/webui/common/index.ts(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/common/index.tscomponents/webui/client/src/pages/SearchPage/SearchControls/search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.tscomponents/webui/server/src/routes/api/presto-search/index.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)
components/webui/common/index.ts (1)
PRESTO_SEARCH_SIGNAL(152-152)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)
🔇 Additional comments (4)
components/webui/common/index.ts (1)
106-108: No remainingFINISHEDreferences found; all consumers now usePRESTO_SEARCH_SIGNAL.DONE.components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (1)
93-94: Approve changes: default metadata count reset verified
SEARCH_STATE_DEFAULT.numSearchResultsMetadata is set to 0 and the updateNumSearchResultsMetadata setter is defined (index.tsx:21,127).components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (2)
8-8: Good: centralised default state import for resetsUsing SEARCH_STATE_DEFAULT avoids magic numbers and keeps resets in sync with the store.
97-105: Provided script to verify enum contents.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/server/src/routes/api/presto-search/index.ts (1)
199-203: Race: createCollection can fail if implicit collection already existsInserts may auto-create the collection before this
createCollectionruns, causingNamespaceExistsand a 500. Either create the collection immediately after resolvingqueryId(before inserts) or ignore “already exists”.Apply:
- await mongoDb.createCollection(searchJobId); + try { + await mongoDb.createCollection(searchJobId); + } catch (err: any) { + // Ignore if collection already exists (code 48) + if (48 !== err?.code && "NamespaceExists" !== err?.codeName) { + throw err; + } + }
♻️ Duplicate comments (2)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)
3-6: Gate RESP_DONE by engine to avoid reintroducing premature DONE for PrestoKeep
PRESTO_SEARCH_SIGNAL.DONEas the only completion trigger for Presto. Still supportSEARCH_SIGNAL.RESP_DONEfor non-Presto engines. This prevents legacy RESP_DONE from flipping Presto UI to DONE prematurely.Apply:
import { - PRESTO_SEARCH_SIGNAL, - SEARCH_SIGNAL, + CLP_QUERY_ENGINES, + PRESTO_SEARCH_SIGNAL, + SEARCH_SIGNAL, } from "@webui/common"; @@ - switch (resultsMetadata.lastSignal) { - case SEARCH_SIGNAL.RESP_DONE: - case PRESTO_SEARCH_SIGNAL.DONE: - updateSearchUiState(SEARCH_UI_STATE.DONE); - break; + switch (resultsMetadata.lastSignal) { + case PRESTO_SEARCH_SIGNAL.DONE: + // Presto is DONE only when all results have been delivered. + updateSearchUiState(SEARCH_UI_STATE.DONE); + break; + case SEARCH_SIGNAL.RESP_DONE: + // Preserve RESP_DONE for non-Presto engines. + if (resultsMetadata.queryEngine !== CLP_QUERY_ENGINES.PRESTO) { + updateSearchUiState(SEARCH_UI_STATE.DONE); + } + break;#!/bin/bash # Verify no remaining Presto paths transition to DONE on FINISHED/RESP_DONE. rg -nP --type=ts --type=tsx 'PRESTO_SEARCH_SIGNAL\.FINISHED|RESP_DONE' components/webui/client | sed -n '1,200p'Also applies to: 39-44
components/webui/server/src/routes/api/presto-search/index.ts (1)
175-188: Defer DONE until Mongo inserts settle; persist final count (zero-row safe)As written, DONE may be set before all async inserts finish, and zero-row queries may never set
numTotalResults. Wait for pending insert promises, then atomically setlastSignal: DONEandnumTotalResults.Apply:
success: () => { - if (false === isResolved) { - request.log.error( - "Presto query finished before searchJobId was resolved; " - ); - return; - } - searchResultsMetadataCollection.updateOne( - {_id: searchJobId}, - {$set: {lastSignal: PRESTO_SEARCH_SIGNAL.DONE}} - ).catch((err: unknown) => { - request.log.error(err, "Failed to update Presto metadata"); - }); - - request.log.info("Presto search succeeded"); + if (false === isResolved) { + request.log.error( + "Presto query finished before searchJobId was resolved; " + ); + return; + } + // Finalize only after all inserts complete; also persist final total count. + void Promise.allSettled(pendingWrites) + .then(async () => { + await searchResultsMetadataCollection.updateOne( + {_id: searchJobId}, + { + $set: { + lastSignal: PRESTO_SEARCH_SIGNAL.DONE, + numTotalResults: totalResultsCount, + }, + } + ); + request.log.info( + {searchJobId, totalResultsCount}, + "Presto search succeeded" + ); + }) + .catch((err: unknown) => { + request.log.error(err, "Failed to finalize Presto metadata"); + }); },Additions outside this hunk:
// Near counters (after totalResultsCount/storedResultsCount) const pendingWrites: Promise<unknown>[] = [];And push insert promises in the data handler:
const p = insertPrestoRowsToMongo(dataToInsert, columns, searchJobId, mongoDb) .catch((err: unknown) => { request.log.error(err, "Failed to insert Presto results into MongoDB"); }); pendingWrites.push(p);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts(1 hunks)components/webui/common/src/index.ts(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.tscomponents/webui/common/src/index.tscomponents/webui/server/src/routes/api/presto-search/index.ts
⏰ 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)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
components/webui/common/src/index.ts
Outdated
| // Used internally by the UI to mark when all data has been received, since Presto may report | ||
| // `FINISHED` before the result stream is fully delivered. `DONE` state is never set by Presto. | ||
| DONE = "DONE" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DONE marker is fine; document cross-surface contract explicitly
Since DONE is an internal UI marker inside a Presto-states enum, add a short note near SearchResultsMetadataDocument.lastSignal JSDoc clarifying it may carry the internal PRESTO_SEARCH_SIGNAL.DONE. Prevents future confusion when reading metadata directly.
🤖 Prompt for AI Agents
In components/webui/common/src/index.ts around lines 110 to 113, add a brief
JSDoc note on the SearchResultsMetadataDocument.lastSignal property clarifying
that it may contain the internal UI-only marker PRESTO_SEARCH_SIGNAL.DONE (used
when Presto reports FINISHED before the full result stream is delivered); update
the doc to explicitly state this is an internal cross-surface contract and
should not be interpreted as a Presto-set state, so consumers reading metadata
directly understand the special-case value and handle or ignore it
appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/common/src/index.ts (1)
111-114: DONE enum looks good; document cross-surface contract and verify exhaustiveness.Add a short JSDoc on
SearchResultsMetadataDocument.lastSignalnoting it may carry the internalPRESTO_SEARCH_SIGNAL.DONE(UI-only), so external consumers don’t misinterpret it. Also verify all switches/conditions handleDONE.#!/bin/bash # Find places that key off FINISHED/DONE and review handling rg -nP --type=ts -C3 '\bPRESTO_SEARCH_SIGNAL\.(FINISHED|DONE)\b|lastSignal\s*===\s*["'\''](FINISHED|DONE)["'\'']' # Locate switches on lastSignal lacking a DONE case rg -nPU --type=ts -C2 '(?s)switch\s*\(\s*[^)]*lastSignal[^)]*\)\s*{.*?case\s+PRESTO_SEARCH_SIGNAL\.DONE' | sed -n '1,120p'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)components/webui/common/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/common/src/index.ts
⏰ 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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think the PR fixes the problem, but I have a few thoughts.
The core issue seems to be the order of callbacks in presto-client: the state callback fires before the data callback. This means our state-updating operation ( searchResultsMetadataCollection.updateOne) is queued before the data-updating one (insertPrestoRowsToMongo), which can cause issues.
My initial thought is a more direct fix. Could we wrap the state update in a Promise? This should ensure the state update is queued after the data update. This would look something like this:
state: (_, queryId, stats) => {
...
new Promise(() => {
if (false === isResolved) {
// Insert metadata and resolve queryId on first call
...
} else {
// Update metadata on subsequent calls
searchResultsMetadataCollection.updateOne(...).catch(...)
}
})
}
Alternatively, we can stick with the solution in the PR. If we do, many of the PRESTO_SEARCH_SIGNAL values will become redundant since the state update is removed. If we go this route, I suggest we simplify PRESTO_SEARCH_SIGNAL to just three values and we delete the state callback completely:
QUERYING: Set when we first resolve the search ID and insert it into the metadata DB.FAILED: Set in the error callback.DONE: Set in the success callback.
I prefer this working on it |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/server/src/routes/api/presto-search/index.ts (1)
198-203: Race: createCollection may conflict with auto-creation by insertscreateCollection after inserts can throw NamespaceExists and fail the request. Pre-create on first state callback and ignore NamespaceExists; remove the late await.
Move creation into the first-state block:
if (false === isResolved) { searchResultsMetadataCollection.insertOne({ _id: queryId, errorMsg: null, errorName: null, lastSignal: PRESTO_SEARCH_SIGNAL.QUERYING, queryEngine: CLP_QUERY_ENGINES.PRESTO, }).catch((err: unknown) => { request.log.error(err, "Failed to insert Presto metadata"); }); + void mongoDb.createCollection(queryId).catch((err: unknown) => { + // Ignore if collection already exists + if (!err || (err as any).codeName !== "NamespaceExists") { + request.log.error(err, "Failed to create results collection"); + } + }); isResolved = true; resolve(queryId); }Then remove the late creation:
- await mongoDb.createCollection(searchJobId); - reply.code(StatusCodes.CREATED);Also applies to: 158-171
♻️ Duplicate comments (3)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)
39-43: Remove legacy RESP_DONE → DONE fallbackKeep DONE driven solely by PRESTO_SEARCH_SIGNAL.DONE to avoid premature DONE from stale signals.
switch (resultsMetadata.lastSignal) { - case SEARCH_SIGNAL.RESP_DONE: case PRESTO_SEARCH_SIGNAL.DONE: updateSearchUiState(SEARCH_UI_STATE.DONE); break;Also drop the now-unused import:
-import { - PRESTO_SEARCH_SIGNAL, - SEARCH_SIGNAL, -} from "@webui/common/metadata"; +import {PRESTO_SEARCH_SIGNAL} from "@webui/common/metadata";#!/bin/bash # Ensure no remaining FINISHED/RESP_DONE fallback remains in client rg -nP --type=ts --type=tsx 'PRESTO_SEARCH_SIGNAL\.FINISHED|RESP_DONE' components/webui/clientcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (2)
46-52: Also reset timeline count for Presto pathKeep parity with non-Presto submissions to avoid stale timeline numbers.
const { + updateNumSearchResultsTimeline, updateNumSearchResultsTable, updateNumSearchResultsMetadata, updateSearchJobId, updateSearchUiState, searchUiState, } = useSearchStore.getState(); @@ - updateNumSearchResultsTable(SEARCH_STATE_DEFAULT.numSearchResultsTable); - updateNumSearchResultsMetadata(SEARCH_STATE_DEFAULT.numSearchResultsMetadata); + updateNumSearchResultsTable(SEARCH_STATE_DEFAULT.numSearchResultsTable); + updateNumSearchResultsTimeline(SEARCH_STATE_DEFAULT.numSearchResultsTimeline); + updateNumSearchResultsMetadata(SEARCH_STATE_DEFAULT.numSearchResultsMetadata);Also applies to: 67-69
67-70: Optional: single action to reset all countsSame consolidation as the non-Presto path.
- updateNumSearchResultsTable(SEARCH_STATE_DEFAULT.numSearchResultsTable); - updateNumSearchResultsTimeline(SEARCH_STATE_DEFAULT.numSearchResultsTimeline); - updateNumSearchResultsMetadata(SEARCH_STATE_DEFAULT.numSearchResultsMetadata); + resetSearchResultCounts();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.tscomponents/webui/server/src/routes/api/presto-search/index.tscomponents/webui/client/src/pages/SearchPage/SearchControls/search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (2)
components/webui/common/src/index.ts (1)
PRESTO_SEARCH_SIGNAL(154-154)components/webui/common/src/metadata.ts (1)
PRESTO_SEARCH_SIGNAL(54-54)
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (1)
components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(163-163)
🔇 Additional comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (1)
96-99: Good: reset metadata count on new submitThis prevents stale counts on fresh queries.
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/common/src/metadata.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/common/src/metadata.ts
⏰ 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)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
components/webui/common/src/metadata.ts (1)
36-43: Add tolerant coercion for legacy Presto states when reading persisted metadataRepo search found no literal legacy state tokens, but persisted Mongo documents may still contain older values — add a small coercion helper (e.g. coercePrestoSearchSignal(s: string): PRESTO_SEARCH_SIGNAL | undefined) near the PRESTO_SEARCH_SIGNAL enum and call it at DB→API and API→client boundaries to normalize legacy strings to the current enum. Apply at least at:
- components/webui/common/src/metadata.ts (types / enum area)
- components/webui/server/src/routes/api/presto-search/index.ts
- components/webui/server/src/routes/api/search/index.ts and utils.ts
- components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (before switching on lastSignal)
Treat undefined from the coercer with an explicit fallback (e.g. QUERYING) so the UI cannot misread legacy values.
|
@hoophalab i made changes discussed |
|
how about this title: |
davemarco
changed the title
davemarco
changed the title
davemarco
changed the title
kirkrodrigues
changed the title
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Deferring to @hoophalab's review.
3 participants
Description
I noticed that sometimes the state is updated to Presto
FINISHED, and the results are shown in green (UIDONEstate), but some more results can still stream in. Not sure if always happens or just due to slow/overloaded machine i'm using. Nonetheless, effectively it appears the issue is that Presto completes the query and updates the state, but our server has yet to dump all the results in Mongo.In addition, there was a minor issue where the number of results did not update to 0 when a new query was launched.
This PR fixes the issue by setting the done state only after all the data is recieved. PR also resets number of results to 0.
Checklist
breaking change.
Validation performed
Both issues appear fixed
Summary by CodeRabbit
Refactor
Bug Fixes