chore: updated strucutre of main page #396

ChoudharyHarish · 2024-10-28T06:53:22Z

Updated the file structure of main page

sayan-truefoundry · 2024-10-28T07:12:09Z

frontend/src/screens/dashboard/docsqa/main/partials/Form.tsx

+import Input from '@/components/base/atoms/Input'
+import Button from '@/components/base/atoms/Button'
+
+const Form = (props: any) => {


this component name is confusing ... Form represents an atomic component ... this could be PromptForm or something like that

I agree with this, additionally, instead of doing an onclick handler, use the form onSubmit handler instead.

sayan-truefoundry · 2024-10-28T07:20:51Z

frontend/src/screens/dashboard/docsqa/main/partials/option.tsx

+  handleOnChange: (e: any) => void
+}
+
+const Option = (props: OptionProps) => {


let's call it ConfigSelector if we want to create this component dedicatedly for configs select dropdown in main page ... but if we want to make it more generic then we have to refactor this component

I hade made an atomic picker component, its actually there in my webcrawler branch https://github.com/truefoundry/cognita/blob/feat/web-crawler/frontend/src/components/base/atoms/Picker.tsx

Blakeinstein · 2024-10-28T16:55:57Z

frontend/src/screens/dashboard/docsqa/main/partials/NoAnswer.tsx

+    <div className="h-[calc(100%-3.125rem)] flex justify-center items-center overflow-y-auto">
+      <div className="min-h-[23rem]">
+        <DocsQaInformation
+          header={'Welcome to DocsQA'}


Why is this in curly braces?

It was like this only i will remove it

Blakeinstein · 2024-10-28T16:56:59Z

frontend/src/screens/dashboard/docsqa/main/partials/index.tsx

+import ErrorAnswer from './ErrorAnswer'
+import Answer from './Answer'
+
+export { Option, ApplicationModal, Form, NoAnswer, ErrorAnswer, Answer }


Lets avoid maintaining an index.tsx, I'd rather suggest using index.tsx in main screen pages where all the components are imported instead.

I was thinking like in /screens we will have one main index.tsx which will contain path to pages only and inside page folder and inside that partials we can have one index.tsx more for the component path we can remove it as well it was optional.

Blakeinstein · 2024-10-28T16:59:15Z

frontend/src/screens/dashboard/docsqa/main/partials/option.tsx

+  handleOnChange: (e: any) => void
+}
+
+const Option = (props: OptionProps) => {


I hade made an atomic picker component, its actually there in my webcrawler branch https://github.com/truefoundry/cognita/blob/feat/web-crawler/frontend/src/components/base/atoms/Picker.tsx

Blakeinstein

So i submitted a review as soon as you updated... oh well. I think it would be better to break the page into components for sections and use React Context to avoid prop drilling.

Blakeinstein · 2024-10-28T17:00:51Z

frontend/src/screens/dashboard/docsqa/main/index.tsx

+        ) : selectedCollection ? (
+          <>
+            <div className="h-full border rounded-lg border-[#CEE0F8] w-[23.75rem] bg-white p-4 overflow-auto">
+              <div className="flex flex-col gap-3">


I suppose this can also be broken down into <Sidebar /> or <QueryConfig />

Yes i was also thinking same but in that case will have to do more prop drilling like sidebar have all config data and right section will have that form so we need to supply it to both so either we can pass from parent so that will be a lot of props for both file if it is fine i can do like that

Blakeinstein · 2024-10-28T17:01:29Z

frontend/src/screens/dashboard/docsqa/main/index.tsx

+                onClick={() => setIsCreateApplicationModalOpen(true)}
+              />
+            </div>
+            <div className="h-full border rounded-lg border-[#CEE0F8] w-[calc(100%-25rem)] bg-white p-4">


And this into <QA /> or something?

ChoudharyHarish · 2024-11-04T21:52:40Z

@Blakeinstein Review it now and check if everything is as we discussed in meet or not

Blakeinstein

LGTM, minor changes needed.

Blakeinstein · 2024-11-04T21:59:36Z

frontend/src/screens/dashboard/docsqa/main/components/ApplicationModal.tsx

+    isCreateApplicationModalOpen,
+    setIsCreateApplicationModalOpen,


Why is the modal state part of the context? Shouldn't the pattern be passing the modal state as prop to this component, and conditionally rendering it as a component in the parent (and not in this component?), effectively when isCreateApplicationModalOpen = false this is a dead component with an empty react fragment, instead of not being rendered at all.

Blakeinstein · 2024-11-04T22:01:17Z

frontend/src/screens/dashboard/docsqa/main/components/Left.tsx

+        className="w-full btn-sm mt-4"
+        onClick={() => setIsCreateApplicationModalOpen(true)}
+      />
+    </div>


I would add the conditional rendering of the modal here, since this page controls it, maintain the modal state here and pass it as props instead of having it in the context.

Blakeinstein · 2024-11-04T22:02:06Z

frontend/src/screens/dashboard/docsqa/main/components/NoAnswer.tsx

@@ -0,0 +1,25 @@
+import React from 'react'
+
+import DocsQaInformation from '../../DocsQaInformation'


Why is this a parent import? I am assuming the component is also used elsewhere?

yes yes it is used in 2,3 files.

Blakeinstein · 2024-11-04T22:03:05Z

frontend/src/screens/dashboard/docsqa/main/components/Right.tsx

@@ -0,0 +1,31 @@
+import React from 'react'


Can we call this something more meaningful than Right?

Blakeinstein · 2024-11-04T22:03:20Z

frontend/src/screens/dashboard/docsqa/main/components/Left.tsx

@@ -0,0 +1,141 @@
+import { MenuItem, Switch, TextareaAutosize } from '@mui/material'


Can we call this something more meaningful than Left?

Blakeinstein · 2024-11-04T22:05:32Z

frontend/src/screens/dashboard/docsqa/main/context.tsx

+    }))
+  }, [selectedQueryController, openapiSpecs])
+
+  const handlePromptSubmit = async () => {


Define submit handler where used, Are multiple components submitting the form?

no no, i have changes this one and also that createApplication function

Blakeinstein · 2024-11-04T22:06:56Z

frontend/src/screens/dashboard/docsqa/main/context.tsx

+    }
+  }
+
+  const createChatApplication = async (


Same for this.

Blakeinstein · 2024-11-04T22:07:30Z

frontend/src/screens/dashboard/docsqa/main/context.tsx

+    if (collections && collections.length) setSelectedCollection(collections[0])
+
+    if (allQueryControllers && allQueryControllers.length)
+      setSelectedQueryController(allQueryControllers[0])
+
+    if (allEnabledModels && allEnabledModels.length)
+      setSelectedQueryModel(allEnabledModels[0].name)
+
+    if (allRetrieverOptions && allRetrieverOptions.length) {
+      setSelectedRetriever(allRetrieverOptions[0])
+      setPromptTemplate(allRetrieverOptions[0].promptTemplate)
+    }
+    if (selectedRetriever)
+      setRetrieverConfig(JSON.stringify(selectedRetriever.config, null, 2))
+  }, [
+    collections,
+    allQueryControllers,
+    allEnabledModels,
+    allRetrieverOptions,
+    selectedRetriever,


Are these interdependent, can we break this use-effect if not?

we can use multiple use effect as well they are not connected

updated strucutre of main page

2a222eb

ChoudharyHarish requested review from Blakeinstein and sayan-truefoundry October 28, 2024 06:53

sayan-truefoundry changed the title ~~updated strucutre of main page~~ chore: updated strucutre of main page Oct 28, 2024

sayan-truefoundry reviewed Oct 28, 2024

View reviewed changes

updated the names

a0c406a

Blakeinstein reviewed Oct 28, 2024

View reviewed changes

modifed main folder

87ca6a7

Blakeinstein reviewed Nov 4, 2024

View reviewed changes

removed some variables from context and refined some names

1f6542e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: updated strucutre of main page #396

chore: updated strucutre of main page #396

ChoudharyHarish commented Oct 28, 2024

sayan-truefoundry Oct 28, 2024

Blakeinstein Oct 28, 2024

sayan-truefoundry Oct 28, 2024

Blakeinstein Oct 28, 2024

Blakeinstein Oct 28, 2024

ChoudharyHarish Oct 28, 2024

Blakeinstein Oct 28, 2024

ChoudharyHarish Oct 28, 2024

Blakeinstein Oct 28, 2024

Blakeinstein left a comment

Blakeinstein Oct 28, 2024

ChoudharyHarish Oct 28, 2024

Blakeinstein Oct 28, 2024

ChoudharyHarish commented Nov 4, 2024

Blakeinstein left a comment

Blakeinstein Nov 4, 2024

Blakeinstein Nov 4, 2024

Blakeinstein Nov 4, 2024

ChoudharyHarish Nov 5, 2024

Blakeinstein Nov 4, 2024

Blakeinstein Nov 4, 2024

Blakeinstein Nov 4, 2024

ChoudharyHarish Nov 5, 2024

Blakeinstein Nov 4, 2024

Blakeinstein Nov 4, 2024

ChoudharyHarish Nov 5, 2024

		isCreateApplicationModalOpen,
		setIsCreateApplicationModalOpen,

		@@ -0,0 +1,25 @@
		import React from 'react'

		import DocsQaInformation from '../../DocsQaInformation'

		@@ -0,0 +1,141 @@
		import { MenuItem, Switch, TextareaAutosize } from '@mui/material'

chore: updated strucutre of main page #396

Are you sure you want to change the base?

chore: updated strucutre of main page #396

Conversation

ChoudharyHarish commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blakeinstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChoudharyHarish commented Nov 4, 2024

Blakeinstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment