issue-274 #278
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "issue-274"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #274
Tasks
appData
get from all Blossom serverszipUrls
to the meta.json - location of the encrypted zip files, it can be on multiple file serverszipUrls
and use the first which has the correct hashIssues introduced by refactor
Deployed version
https://test.sigit.io/
Diagrams
@ -0,0 +80,4 @@
// Check if new server is a valid URL
if (
!/^(https?:\/\/)(?!-)([A-Za-z0-9-]{1,63}\.)+[A-Za-z]{2,6}$/gim.test(
should be a utility
WIP: issue-274to issue-274It needs to be thoroughly tested: https://test.sigit.io
@ -45,0 +43,4 @@
console.info(
`${file.name} uploaded to following file storages: ${urls.join(', ')}`
)
// This bit was returning an url, and return of this function is being set to mark.value, so it kind of
Can you please re-phrease this comment to make it clearer? Or better yet, add a comment to the overall function.
@ -156,21 +172,32 @@ const PdfMarking = (props: PdfMarkingProps) => {
<PdfView
currentFile={currentFile}
files={files}
Is it the case that if there are no files found in the zip,
files
will be an empty array? Can we rely on the length of the files array instead of introducing thenoFiles
flag?Can you also add some comments, here and in the code, to clarify what the
noFiles
flag is meant to do?@ -162,3 +179,4 @@
currentUserMarks={currentUserMarks}
otherUserMarks={otherUserMarks}
/>
{noFiles ? (
If you keep the
noFiles
flag, the following two evaluations should be refactored using the ternary operator.@ -45,0 +44,4 @@
`${file.name} uploaded to following file storages: ${urls.join(', ')}`
)
// This bit was returning an url, and return of this function is being set to mark.value, so it kind of
// does not make sense to return an url to the file storage
Original value holds coordinates that replicate the signature strokes and depending on the person's signature can be quite large.
Online flow - It's uploaded as a separate blossom file and returned as URL to be fetched when needed.
Offline - the actual value as is kept as is and not replaced by a URL since we don't care about size in this case.
Hm, I did not see how it is fetched when mark.value is a URL, can you point to a file where that happens?
the
MarkStrategy
interface andfetchAndDecrypt
andencryptAndUpload
functions handle that@ -168,6 +169,7 @@ export const SignPage = () => {
createSignatureContent.markConfig,
usersPubkey!
)
// TODO figure out why markConfig does not contain the usersPubkey when multiple signer
Is this possibly a new issue/bug?
sorry, leftover todo, I introduced the issue while refactoring, and also fixed it
@ -490,6 +520,189 @@ export const getUserAppDataFromBlossom = async (
return parsedContent
}
// /**
Is this left from the NDK merge (the whole commented block)?
@ -94,2 +97,3 @@
const ndkRelayList = await getNDKRelayList(pubkey)
const ndkRelayListPromise = getNDKRelayList(pubkey)
const serverMapPromise = getFileServerMap(pubkey, fetchEvent)
What is the advantage of using a map over list/array?
In my view, a map is useful when it is possible to query items by key, which is not something we will do with blossom drives.
An array seems a more straightforward choice. You won't need to change an object into an array further down the line when you intract with it.
We were discussing that in future, we will have read and write servers, so I prepared for it:
let's put this explanation in a code comment
You could have an entity FileServer:
{ url: string, read: boolean, write: boolean }
servers
would hold an array of FileServer objects.Again, in my view, that is a preferred approach, because we have no ability (or reason, really) to query file servers by key. And in the majority of cases that I've seen, the fileServerMap needs to be transformed into an array of keys, values, or entries, in order to interact with it.
@ -96,0 +99,4 @@
const serverMapPromise = getFileServerMap(pubkey, fetchEvent)
const [ndkRelayList, serverMap] = await Promise.all([
ndkRelayListPromise,
Instead of storing promises in a separate variable, you can pass in the functions that return promises directly in
Promise.all
.@ -100,3 +110,4 @@
return appPrivateRoutes.relays
}
if (Object.keys(serverMap).length < 1) {
This transformation can be avoided if you change
serverMap
to be an array.#278 (comment)
@ -79,2 +79,3 @@
* We keep the last 10 versions
*/
blossomUrls: string[]
blossomVersions: BlossomVersion[]
Why not keep the existing property,blossomUrls
? It looks like the contect is exactly the same, an array of strings.Ah, I get it - we now have an archive of blossom urls.
@ -244,3 +255,2 @@
// Insert new blossom URL at the start of the array
blossomUrls.unshift(newBlossomUrl)
// insert new server (blossom) urls at the start of the array
Should it be added at the end of the array instead? It would make sense, going from the first version to the last version, i.e. 1, 2 .... n
This is how it was done so far, the latest version was on top I don't know the reasoning
@ -0,0 +50,4 @@
const fetchFileServers = async () => {
if (usersPubkey) {
await getFileServerMap(usersPubkey, fetchEvent).then((res) => {
You should use either the async await syntax or promise chaining, not both of them at the same time.
It's probably best to refactor to:
const servers = await getFileServerMap(usersPubkey, fetch event)
ah very strange, it was on accident
@ -0,0 +167,4 @@
if (res && res.data?.toLowerCase().includes('blossom server')) {
resolve(true)
} else {
reject(false)
I don't think you need to reject the promise in this scenario. You can return a resolved promise with either
true
(if you received the expected response) orfalse
, and you won't need to do rely on catching the error in the client code to make a business logic decision.It makes very much sense, another "will fix it later" which never happened...
@ -0,0 +109,4 @@
setBlossomServersMap(tempBlossomServersMap)
setNewServerURL('')
publishFileServersList(tempBlossomServersMap)
Similar to my point elsewhere in the PR, I don't think we should be storing blossoms in a map. It should be an array instead.
#278 (comment)
@ -168,6 +169,7 @@ export const SignPage = () => {
createSignatureContent.markConfig,
usersPubkey!
)
Sounds like a pretty critical issue. Can you please add more information and create an issue?
I am not sure what you mean, can you check if you maybe missed the place in the code?
@ -298,0 +298,4 @@
return
}
for (let i = 0; i < zipUrls.length; i++) {
Does it mean that if a user has 3 blossom servrs, there will be 2 requests?
Of these two, the second request will over-write the result of the first one?
If you do need to iterate, you can probably use
.map
to handle promises and then set the value once they have been awaited, to make the code more declarative.It will take the first one which is valid, and it will only send more requests if the hash of the first one fails or so, I did iteration to be sequential on purpose, for that reason
rather than comment here, can we provide a comment in the code?
Ok, thanks.
What is the reason to add
break
at the end of the last loop? Isn't the loop going to finish naturally because you incrementi
?A bit of a nitpick, but I would also suggest using the
for ... of
loop over the 'vanilla' JS loop, because it is a little bit more declarative and still gives you the same sequential processing option and breaking out of the loop once you get the result.@ -63,6 +63,7 @@ import { MarkRender } from '../../components/MarkTypeStrategy/MarkRender.tsx'
interface PdfViewProps {
files: CurrentUserFile[]
noFiles?: boolean
The same question as before - do we need
noFiles
, and if yes, can you some comment as to why.@ -432,2 +460,3 @@
if (!zip) return
if (!zip) {
if (!isLastZipUrl) continue // Skip to next zipUrl
Can you specify why it needs to break out of the loop if it's the last zip url?
I added a comment
@ -455,3 +484,2 @@
const hash = await getHash(arrayBuffer)
const hash = await getHash(entryArrayBuffer)
if (hash) {
You don't need this if statement.
getHash
returns a string or null, so you can directly assignhash
instead of re-assigningnull
.@ -0,0 +2,4 @@
import { RestoreState } from '../actions'
import { ServerMap } from '../../types'
export type ServersState = {
I think
Servers
is too ambiguous to use as a type. It should either beFileServer
orBlossomServer
.FileServer
is a better option if we want to allow for a possibility that in the future there will be other compatible standards other than Blossom.@ -22,6 +27,8 @@ export const DEFAULT_LOOK_UP_RELAY_LIST = [
'wss://purplepag.es'
]
export const MAXIMUM_BLOSSOMS_LENGTH = 3
Should it be configurable as well?
For now I don't think so, what do @b thinks?
it can be a seperate issue / PR if we decide to do that (probably we'd combine it with read-only servers)
@ -0,0 +27,4 @@
authors: [npub]
}
const event = await fetchEvent(eventFilter).catch((err) => {
It would be better to wrap the whole body of the function in the
try catch
block, rather than just catch an error in this one instance.@ -411,2 +416,2 @@
// Return the URL of the uploaded file
return response.data.url as string
// Upload the file to the file storage services using Axios
for (const preferredServer of preferredServers) {
You could use
.map
to collect promises inPromise.all
to make the code more declarative.Checkout
From your project repository, check out a new branch and test the changes.