Stamps Backend#1493
Conversation
|
Current issues yet to fix:
Also:
|
|
Stamps is ready for a PR review. Little video of how it works Stamps.webmThis PR still has a few issues:
I'll wait for the comments with the required changes from the review. |
|
Something I forgot to put in my previous comment Disclaimer: Many of the UI designs(Like CSS and JS DOM interactions) were made using AI, because I'm not proficient with the UI side. Also in this new commit, the hscan mock was made also by with AI to save me some time. I feel like is necessary to mention these kind of things. |
Difegue
left a comment
There was a problem hiding this comment.
Thanks for splitting the frontend, that makes review easier on my end.
The API looks sound to me, but I have questions on the DB structure itself as I think we could do it in a potentially simpler way.
Also misc points:
- This needs documentation of the DB structure in Architecture
- Stamps should probably be added to backup.json exports like categories.
Difegue
left a comment
There was a problem hiding this comment.
Sorry for the delay 😶🌫️
The changes to the previous comments look OK to me, I guess we just need the DB structure revamp and the architecture doc updates?
dda997c to
1f78a7f
Compare
|
A couple questions on the feature spec, I know the code probably answers this but it's better/easier to just have these as comments for future reference in case the code changes (Also, bc a few integration tests are failing, but they could just be AI doing AI things)
|
|
Most of the design decisions I've made on Stamps are following how we do things on other parts of the platform, so I'm not gonna say that this is the right way to make this API, if you think that something can be done differently for the purpose of the tests, I'm open to do the changes. Now answering your questions:
These 3 actions are served through the render_api_response method, so in case of an error, returns a 400. In the code, I check that the Stamp id exist in the DB, and returns an error if doesn't.
Currently they are kept, I haven't touched the cleaning part, because I wanted to be sure of the structure of the DB first. Originally, with the
We check that the archive exists beforehand, so the first case is not possible, it throws an error, the second case is possible, since the page number is just a value, but the stamp will not be accessible later without hardcoding the number in the request. Although, I guess that I need to check on the Frontend what happens when stampedpages EP returns an out of index page at filtering the pages in the Overlay.
Client-side concern, position is just a normalized 0-100 number(Both Width and height), so at backend-side there is nothing to transform.
It's just a string, so the content is theoretically only limited by Redis natural limits(And I think URL size, since we send the value as a query param), we can set a limit, but that would depend on what people are using it for. I mean, I don't see why someone will leave a marker larger than a tweet, unless you're using it for a professional environment.
No, they are their own set of tables, and its own attribute, they do not share anything else than the archive_id. |
…archive is deleted
|
Added a little cleanup code to remove the stamps when the archive gets deleted. I guess we are only missing to add the stamps to the backup file. |
|
No request for change. Just nailing things down so I'm not testing or expecting the wrong things.
An integer or a float? I'm considering pages that exceed 1080p, 100 integer is a bit coarse.
Is the archive check/stamp write exclusive? If not, it is still possible (toctou), though infrequent and carries risk. (Though tbh if it is confirmed, I wouldn't bother fixing it...)
If I were ambitious: I might use stamps for an OCR/translation data storage layer which can be rendered frontend to override original JP display on the fly. This may be stored as a JSON stamp carrying raw/translated data, which is interpreted by client-side customizations. So idk, it might not always be a tweet's length Also agree, stamps are dependent on the archive, if an archive goes then there's no point in keeping the stamp around. |
|
Ok, then I should be a little more specific with my answers:
It is a float, calculated with the formula (x/image_width)*100, which is the same for the height but with y coordinate, and I think I'm not fixing the number of decimals. The idea here is that we calculate the proportional distance to top left of the image(Plus a minor fix due to HTML things), this way the stamp will be placed at the right place whether you size up or down the image.
No, we do not set a lock for the archive itself, so it's theoretically possible to do the check, just after that the archive gets deleted, and then cause an error because after creating the stamp, we have to add it the stamps attribute of the first. I'm not sure how possible is this scenario is without a machine doing it.
Yeah, that's why I didn't set a limit, my comment was more about that, for a casual reader, the bigger use-case would be to save an authors tweet that references a page, but otherwise, only people doing professional things with the platform will make use of the unlimited space. |
|
I guess with this commit all topics related to stamps for the backend should be completed. |
Difegue
left a comment
There was a problem hiding this comment.
Mostly wording/doc nits left, I'm happy with this overall.
Please update architecture.md with the finalized DB structure for stamps as well pls 🙏

Add support for Stamps in the backend.
This is just an initial PR to share the progress with the backend changes, just so I can start the get comments about the implementation. Will be working on some UI aspects on my next commits.
Stamps(AKA stickies/favorites/bookmark) are integrated using Hash tables in Redis. The logic is that the field is composed of a string like "page:creation_timestamp", so that way is easy to identify which page the stamp belongs to, also we have the creation date in case is useful. The value inside the stamp is conformed by a string in the shape of "position|content", where content is the text we want the stamp to have, and position for the moment is a combo field just to show that is possible to add more fields inside as long as we follow this structure of using | as a separator. I'm calling it position in case some client developer wants to use them as positional annotations, then it is possible to save the coordinates in this space, otherwise it does nothing.
There might be errors related to OpenAPI in this version, since I don't really have a lot of time to test the specifications, but I hope this can be solved with the time.